diff mbox series

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

Message ID 20240208181245.96617-9-philmd@linaro.org
State Superseded
Headers show
Series hw: Strengthen SysBus & QBus API | expand

Commit Message

Philippe Mathieu-Daudé Feb. 8, 2024, 6:12 p.m. UTC 
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(-)

Comments

Peter Maydell Feb. 9, 2024, 11:37 a.m. UTC | #1 
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
Mark Cave-Ayland Feb. 9, 2024, 9:38 p.m. UTC | #2 
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.
Mark Cave-Ayland Feb. 9, 2024, 10:37 p.m. UTC | #3 
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.
diff mbox series

Patch

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);