[v3] hw: Strengthen SysBus & QBus API

[PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

Posted by Philippe Mathieu-Daudé 8 months, 1 week ago

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sparc/sun4m.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index e782c8ec7a..d52e6a7213 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
     dma = qdev_new(TYPE_SPARC32_DMA);
     espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
                                    OBJECT(dma), "espdma"));
-    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
 
     esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
 
     ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
                                  OBJECT(dma), "ledma"));
-    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
 
     lance = SYSBUS_PCNET(object_resolve_path_component(
                          OBJECT(ledma), "lance"));
@@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
     }
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
+
     sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
-- 
2.41.0

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

Posted by Mark Cave-Ayland 8 months, 1 week ago

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sparc/sun4m.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index e782c8ec7a..d52e6a7213 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>       dma = qdev_new(TYPE_SPARC32_DMA);
>       espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>                                      OBJECT(dma), "espdma"));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>   
>       esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
>   
>       ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
>                                    OBJECT(dma), "ledma"));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
>   
>       lance = SYSBUS_PCNET(object_resolve_path_component(
>                            OBJECT(ledma), "lance"));
> @@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>       }
>   
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
> +
>       sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
>   
>       sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

Posted by Peter Maydell 8 months, 1 week ago

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sparc/sun4m.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index e782c8ec7a..d52e6a7213 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>      dma = qdev_new(TYPE_SPARC32_DMA);
>      espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>                                     OBJECT(dma), "espdma"));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>
>      esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
>
>      ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
>                                   OBJECT(dma), "ledma"));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
>
>      lance = SYSBUS_PCNET(object_resolve_path_component(
>                           OBJECT(ledma), "lance"));
> @@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>      }
>
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
> +

(This confused me briefly until I realised that this function
is reaching into the dma device to find child objects to connect
the IRQs to. Perhaps it would be nicer if the wrapping 'dma' object
passed through those IRQs to avoid that.)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

Posted by Mark Cave-Ayland 8 months, 1 week ago

On 09/02/2024 11:37, Peter Maydell wrote:

> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> We should not wire IRQs on unrealized device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/sparc/sun4m.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index e782c8ec7a..d52e6a7213 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>>       dma = qdev_new(TYPE_SPARC32_DMA);
>>       espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>>                                      OBJECT(dma), "espdma"));
>> -    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>>
>>       esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
>>
>>       ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
>>                                    OBJECT(dma), "ledma"));
>> -    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
>>
>>       lance = SYSBUS_PCNET(object_resolve_path_component(
>>                            OBJECT(ledma), "lance"));
>> @@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>>       }
>>
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
>> +
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>> +
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
>> +
> 
> (This confused me briefly until I realised that this function
> is reaching into the dma device to find child objects to connect
> the IRQs to. Perhaps it would be nicer if the wrapping 'dma' object
> passed through those IRQs to avoid that.)

FWIW I can't say I'm a fan of passing through IRQs for container devices that contain 
one or more child devices, since you end up with an extra layer of wiring complexity 
that can change depending upon the child device IRQ wiring. I find it much easier to 
access the child devices directly, since then the wiring is always consistent across 
all users.


ATB,

Mark.