Re: [PATCH v3 04/17] hw/intc/loongson_ipi: Extract loongson_ipi_common_realize()

[maobibo]

On 2024/7/18 上午5:46, Philippe Mathieu-Daudé wrote:
> From: Bibo Mao <maobibo@loongson.cn>
>
> In preparation to extract common IPI code in few commits,
> extract loongson_ipi_common_realize().
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> [PMD: Extracted from bigger commit, added commit description]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/intc/loongson_ipi.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/loongson_ipi.c b/hw/intc/loongson_ipi.c
> index 3b3481c43e..40ac769aad 100644
> --- a/hw/intc/loongson_ipi.c
> +++ b/hw/intc/loongson_ipi.c
> @@ -275,7 +275,7 @@ static const MemoryRegionOps loongson_ipi64_ops = {
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
> -static void loongson_ipi_realize(DeviceState *dev, Error **errp)
> +static void loongson_ipi_common_realize(DeviceState *dev, Error **errp)
>   {
>       LoongsonIPIState *s = LOONGSON_IPI(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> @@ -301,20 +301,31 @@ static void loongson_ipi_realize(DeviceState *dev, Error 
> **errp)
>       sysbus_init_mmio(sbd, &s->ipi64_iocsr_mem);
>   
>       s->cpu = g_new0(IPICore, s->num_cpu);
> -    if (s->cpu == NULL) {
> -        error_setg(errp, "Memory allocation for IPICore faile");
Philippe,

Thanks for the whole series, it looks to me. It is split into small 
patches and adds new option CONFIG_LOONGSON_IPI_COMMON, it is easier to 
review and compile for multiple targets.

One small nit, do we need keep checking sentence for if (s->cpu == NULL)?

Overall, for the whole series it is ok for me and works well on 
LoongArch machine.

Reviewed-by: Bibo Mao <maobibo@loongson.cn>
Tested-by: Bibo Mao <maobibo@loongson.cn>

> +    for (i = 0; i < s->num_cpu; i++) {
> +        s->cpu[i].ipi = s;
> +
> +        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
> +    }
> +}
> +
> +static void loongson_ipi_realize(DeviceState *dev, Error **errp)
> +{
> +    LoongsonIPIState *s = LOONGSON_IPI(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    Error *local_err = NULL;
> +
> +    loongson_ipi_common_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>           return;
>       }
>   
> -    for (i = 0; i < s->num_cpu; i++) {
> -        s->cpu[i].ipi = s;
> +    for (unsigned i = 0; i < s->num_cpu; i++) {
>           s->cpu[i].ipi_mmio_mem = g_new0(MemoryRegion, 1);
>           g_autofree char *name = g_strdup_printf("loongson_ipi_cpu%d_mmio", i);
>           memory_region_init_io(s->cpu[i].ipi_mmio_mem, OBJECT(dev),
>                                 &loongson_ipi_core_ops, &s->cpu[i], name, 0x48);
>           sysbus_init_mmio(sbd, s->cpu[i].ipi_mmio_mem);
> -
> -        qdev_init_gpio_out(dev, &s->cpu[i].irq, 1);
>       }
>   }
>