Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device

[Philippe Mathieu-Daudé]

On 9/21/20 7:08 PM, Mark Cave-Ayland wrote:
> On 21/09/2020 10:57, Philippe Mathieu-Daudé wrote:
> 
>> Hi Mark,
>>
>> On 9/20/20 10:20 AM, Mark Cave-Ayland wrote:
>>> Instead use qdev_set_nic_properties() to configure the on-board NIC at the
>>> sun4m machine level.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/dma/sparc32_dma.c |  5 -----
>>>  hw/sparc/sun4m.c     | 21 +++++++++++++--------
>>>  2 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
>>> index 2cbe331959..b643b413c5 100644
>>> --- a/hw/dma/sparc32_dma.c
>>> +++ b/hw/dma/sparc32_dma.c
>>> @@ -342,12 +342,7 @@ static void sparc32_ledma_device_realize(DeviceState 
>>> *dev, Error **errp)
>>>  {
>>>      LEDMADeviceState *s = SPARC32_LEDMA_DEVICE(dev);
>>>      SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
>>> -    NICInfo *nd = &nd_table[0];
>>>  
>>> -    /* FIXME use qdev NIC properties instead of nd_table[] */
>>> -    qemu_check_nic_model(nd, TYPE_LANCE);
>>> -
>>> -    qdev_set_nic_properties(DEVICE(lance), nd);
>>>      object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), 
>>> &error_abort);
>>>      sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
>>>  }
>>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>>> index 947b69d159..ed5f3ccd9f 100644
>>> --- a/hw/sparc/sun4m.c
>>> +++ b/hw/sparc/sun4m.c
>>> @@ -319,7 +319,7 @@ static void *iommu_init(hwaddr addr, uint32_t version, 
>>> qemu_irq irq)
>>>  
>>>  static void *sparc32_dma_init(hwaddr dma_base,
>>>                                hwaddr esp_base, qemu_irq espdma_irq,
>>> -                              hwaddr le_base, qemu_irq ledma_irq)
>>> +                              hwaddr le_base, qemu_irq ledma_irq, NICInfo 
>>> *nd)
>>
>> Instead of passing NICInfo to sparc32_dma_init,
>> shouldn't you extract the lance code from it?
> 
> Hi Philippe,
> 
> I'm not sure I understand what you mean here? The sparc32-dma device is 
> realised
> within the sparc32_dma_init() function and qdev_set_nic_properties() must be 
> called
> before realise happens.
> 
> If you can explain a bit more about how you think it can be separated out 
> then I can
> take a look.

Sorry I guess I got confused by the 2 different sparc32_dma_init()
functions.

Since ledma always expose lance, maybe you can use:

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 2cbe331959a..9a907a30373 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -336,18 +336,14 @@ static void sparc32_ledma_device_init(Object *obj)
                           "ledma-mmio", DMA_SIZE);

     object_initialize_child(obj, "lance", &ls->lance, TYPE_LANCE);
+    qdev_alias_all_properties(DEVICE(&ls->lance), obj);
 }

This way you set the properties directly on the ledma and only
have to sysbus_map lance.

> 
> 
> ATB,
> 
> Mark.
>