diff mbox series

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

Message ID 20200920082018.16135-5-mark.cave-ayland@ilande.co.uk
State Superseded
Headers show
Series QOM minor fixes | expand

Commit Message

Mark Cave-Ayland Sept. 20, 2020, 8:20 a.m. UTC 
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(-)

Comments

Markus Armbruster Sept. 21, 2020, 9:25 a.m. UTC | #1 
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

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

>  {

>      DeviceState *dma;

>      ESPDMADeviceState *espdma;

> @@ -328,16 +328,11 @@ static void *sparc32_dma_init(hwaddr dma_base,

>      SysBusPCNetState *lance;

>  

>      dma = qdev_new(TYPE_SPARC32_DMA);

> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);

> -    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);

> -

>      espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(

>                                     OBJECT(dma), "espdma"));

>      sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);

>  

>      esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));

> -    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);

> -    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);

>  

>      ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(

>                                   OBJECT(dma), "ledma"));

> @@ -345,6 +340,14 @@ static void *sparc32_dma_init(hwaddr dma_base,

>  

>      lance = SYSBUS_PCNET(object_resolve_path_component(

>                           OBJECT(ledma), "lance"));

> +    qdev_set_nic_properties(DEVICE(lance), nd);

> +

> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);

> +    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);

> +

> +    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);

> +    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);

> +

>      sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);

>  

>      return dma;


You delay a bit of work on devices @dma and @esp.  Can you explain why?

> @@ -854,6 +857,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>      unsigned int max_cpus = machine->smp.max_cpus;

>      Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,

>                                                    TYPE_MEMORY_BACKEND, NULL);

> +    NICInfo *nd = &nd_table[0];

>  

>      if (machine->ram_size > hwdef->max_mem) {

>          error_report("Too much memory for this machine: %" PRId64 ","

> @@ -914,9 +918,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>                          hwdef->iommu_pad_base, hwdef->iommu_pad_len);

>      }

>  

> +    qemu_check_nic_model(nd, TYPE_LANCE);

>      sparc32_dma_init(hwdef->dma_base,

>                       hwdef->esp_base, slavio_irq[18],

> -                     hwdef->le_base, slavio_irq[16]);

> +                     hwdef->le_base, slavio_irq[16], nd);

>  

>      if (graphic_depth != 8 && graphic_depth != 24) {

>          error_report("Unsupported depth: %d", graphic_depth);

> @@ -1047,7 +1052,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>                                      machine->initrd_filename,

>                                      machine->ram_size, &initrd_size);

>  

> -    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,

> +    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,

>                 machine->boot_order, machine->ram_size, kernel_size,

>                 graphic_width, graphic_height, graphic_depth,

>                 hwdef->nvram_machine_id, "Sun4m");
Philippe Mathieu-Daudé Sept. 21, 2020, 9:57 a.m. UTC | #2 
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?

>  {

>      DeviceState *dma;

>      ESPDMADeviceState *espdma;

> @@ -328,16 +328,11 @@ static void *sparc32_dma_init(hwaddr dma_base,

>      SysBusPCNetState *lance;

>  

>      dma = qdev_new(TYPE_SPARC32_DMA);

> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);

> -    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);

> -

>      espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(

>                                     OBJECT(dma), "espdma"));

>      sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);

>  

>      esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));

> -    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);

> -    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);

>  

>      ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(

>                                   OBJECT(dma), "ledma"));

> @@ -345,6 +340,14 @@ static void *sparc32_dma_init(hwaddr dma_base,

>  

>      lance = SYSBUS_PCNET(object_resolve_path_component(

>                           OBJECT(ledma), "lance"));

> +    qdev_set_nic_properties(DEVICE(lance), nd);

> +

> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);

> +    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);

> +

> +    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);

> +    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);

> +

>      sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);

>  

>      return dma;

> @@ -854,6 +857,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>      unsigned int max_cpus = machine->smp.max_cpus;

>      Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,

>                                                    TYPE_MEMORY_BACKEND, NULL);

> +    NICInfo *nd = &nd_table[0];

>  

>      if (machine->ram_size > hwdef->max_mem) {

>          error_report("Too much memory for this machine: %" PRId64 ","

> @@ -914,9 +918,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>                          hwdef->iommu_pad_base, hwdef->iommu_pad_len);

>      }

>  

> +    qemu_check_nic_model(nd, TYPE_LANCE);

>      sparc32_dma_init(hwdef->dma_base,

>                       hwdef->esp_base, slavio_irq[18],

> -                     hwdef->le_base, slavio_irq[16]);

> +                     hwdef->le_base, slavio_irq[16], nd);

>  

>      if (graphic_depth != 8 && graphic_depth != 24) {

>          error_report("Unsupported depth: %d", graphic_depth);

> @@ -1047,7 +1052,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,

>                                      machine->initrd_filename,

>                                      machine->ram_size, &initrd_size);

>  

> -    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,

> +    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,

>                 machine->boot_order, machine->ram_size, kernel_size,

>                 graphic_width, graphic_height, graphic_depth,

>                 hwdef->nvram_machine_id, "Sun4m");

>
Mark Cave-Ayland Sept. 21, 2020, 5:08 p.m. UTC | #3 
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.


ATB,

Mark.
Philippe Mathieu-Daudé Sept. 21, 2020, 5:58 p.m. UTC | #4 
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.

>
diff mbox series

Patch

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)
 {
     DeviceState *dma;
     ESPDMADeviceState *espdma;
@@ -328,16 +328,11 @@  static void *sparc32_dma_init(hwaddr dma_base,
     SysBusPCNetState *lance;
 
     dma = qdev_new(TYPE_SPARC32_DMA);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
-
     espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
                                    OBJECT(dma), "espdma"));
     sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
 
     esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
-    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
-    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
 
     ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
                                  OBJECT(dma), "ledma"));
@@ -345,6 +340,14 @@  static void *sparc32_dma_init(hwaddr dma_base,
 
     lance = SYSBUS_PCNET(object_resolve_path_component(
                          OBJECT(ledma), "lance"));
+    qdev_set_nic_properties(DEVICE(lance), nd);
+
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
+    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
+
     sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
 
     return dma;
@@ -854,6 +857,7 @@  static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     unsigned int max_cpus = machine->smp.max_cpus;
     Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
                                                   TYPE_MEMORY_BACKEND, NULL);
+    NICInfo *nd = &nd_table[0];
 
     if (machine->ram_size > hwdef->max_mem) {
         error_report("Too much memory for this machine: %" PRId64 ","
@@ -914,9 +918,10 @@  static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                         hwdef->iommu_pad_base, hwdef->iommu_pad_len);
     }
 
+    qemu_check_nic_model(nd, TYPE_LANCE);
     sparc32_dma_init(hwdef->dma_base,
                      hwdef->esp_base, slavio_irq[18],
-                     hwdef->le_base, slavio_irq[16]);
+                     hwdef->le_base, slavio_irq[16], nd);
 
     if (graphic_depth != 8 && graphic_depth != 24) {
         error_report("Unsupported depth: %d", graphic_depth);
@@ -1047,7 +1052,7 @@  static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                                     machine->initrd_filename,
                                     machine->ram_size, &initrd_size);
 
-    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
+    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
                machine->boot_order, machine->ram_size, kernel_size,
                graphic_width, graphic_height, graphic_depth,
                hwdef->nvram_machine_id, "Sun4m");