diff mbox series

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

Message ID 20240717214708.78403-5-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series Reconstruct loongson ipi driver | expand

Commit Message

Philippe Mathieu-Daudé July 17, 2024, 9:46 p.m. UTC 
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(-)

Comments

Bibo Mao July 18, 2024, 2:11 a.m. UTC | #1 
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);
>       }
>   }
>   
>
Philippe Mathieu-Daudé July 18, 2024, 8:01 a.m. UTC | #2 
On 18/7/24 04:11, maobibo wrote:
> 
> 
> 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)?

No because g_new0() can not fail. Checking return value only
makes sense for g_try_new0() which returns.

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

Thanks!

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

Jiaxun, do you mind re-testing the series for your MIPS machine?

Regards,

Phil.
diff mbox series

Patch

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");
+    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);
     }
 }