Re: [Qemu-devel] [PATCHv4 11/13] sparc32_dma: introduce new SPARC32_DMA type container object

[Mark Cave-Ayland]

On 27/10/17 17:18, Philippe Mathieu-Daudé wrote:
> On 10/25/2017 12:59 PM, Mark Cave-Ayland wrote:
>> Create a new SPARC32_DMA container object (including an appropriate container
>> memory region) and add instances of the SPARC32_ESPDMA_DEVICE and
>> SPARC32_LEDMA_DEVICE as child objects. The benefit is that most of the gpio
>> wiring complexity between esp/espdma and lance/ledma is now hidden within the
>> SPARC32_DMA realize function.
>>
>> Since the sun4m IOMMU is already QOMified we can find a reference to
>> it using object_resolve_path_type() allowing us to completely remove all 
>> external
>> references to the iommu pointer.
>>
>> Finally we rework sun4m's sparc32_dma_init() to invoke the new SPARC32_DMA 
>> object
>> and wire up the remaining board memory regions/IRQs.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> Reviewed-by: Artyom Tarasenko <address@hidden>
>> ---
>>  hw/dma/sparc32_dma.c           |   70 
>> ++++++++++++++++++++++++++++++++++++++++
>>  hw/sparc/sun4m.c               |   66 ++++++++++++++++++-------------------
>>  include/hw/sparc/sparc32_dma.h |   12 +++++++
>>  3 files changed, 114 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
>> index d4cff74..582b7cc 100644
>> --- a/hw/dma/sparc32_dma.c
>> +++ b/hw/dma/sparc32_dma.c
>> @@ -30,6 +30,7 @@
>>  #include "hw/sparc/sparc32_dma.h"
>>  #include "hw/sparc/sun4m.h"
>>  #include "hw/sysbus.h"
>> +#include "qapi/error.h"
>>  #include "trace.h"
>>  
>>  /*
>> @@ -369,11 +370,80 @@ static const TypeInfo sparc32_ledma_device_info = {
>>      .class_init    = sparc32_ledma_device_class_init,
>>  };
>>  
>> +static void sparc32_dma_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SPARC32DMAState *s = SPARC32_DMA(dev);
>> +    DeviceState *espdma, *esp, *ledma, *lance;
>> +    SysBusDevice *sbd;
>> +    Object *iommu;
>> +
>> +    iommu = object_resolve_path_type("", TYPE_SUN4M_IOMMU, NULL);
>> +    if (!iommu) {
>> +        error_setg(errp, "unable to locate sun4m IOMMU device");
>> +        return;
>> +    }
>> +
>> +    espdma = qdev_create(NULL, TYPE_SPARC32_ESPDMA_DEVICE);
>> +    object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
>> +    object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma), errp);
>> +    qdev_init_nofail(espdma);
>> +
>> +    esp = DEVICE(object_resolve_path_component(OBJECT(espdma), "esp"));
> 
> TYPE_ESP?
> 
>> +    sbd = SYS_BUS_DEVICE(esp);
>> +    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(espdma, 0));
>> +    qdev_connect_gpio_out(espdma, 0, qdev_get_gpio_in(esp, 0));
>> +    qdev_connect_gpio_out(espdma, 1, qdev_get_gpio_in(esp, 1));
>> +
>> +    sbd = SYS_BUS_DEVICE(espdma);
>> +    memory_region_add_subregion(&s->dmamem, 0x0,
>> +                                sysbus_mmio_get_region(sbd, 0));
>> +
>> +    ledma = qdev_create(NULL, TYPE_SPARC32_LEDMA_DEVICE);
>> +    object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
>> +    object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma), errp);
>> +    qdev_init_nofail(ledma);
>> +
>> +    lance = DEVICE(object_resolve_path_component(OBJECT(ledma), "lance"));
> 
> TYPE_LANCE?
> 
>> +    sbd = SYS_BUS_DEVICE(lance);
>> +    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(ledma, 0));
>> +    qdev_connect_gpio_out(ledma, 0, qdev_get_gpio_in(lance, 0));
>> +
>> +    sbd = SYS_BUS_DEVICE(ledma);
>> +    memory_region_add_subregion(&s->dmamem, 0x10,
>> +                                sysbus_mmio_get_region(sbd, 0));
>> +}
>> +
>> +static void sparc32_dma_init(Object *obj)
>> +{
>> +    SPARC32DMAState *s = SPARC32_DMA(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +
>> +    memory_region_init(&s->dmamem, OBJECT(s), "dma", DMA_SIZE + 
>> DMA_ETH_SIZE);
>> +    sysbus_init_mmio(sbd, &s->dmamem);
>> +}
>> +
>> +static void sparc32_dma_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = sparc32_dma_realize;
>> +}
>> +
>> +static const TypeInfo sparc32_dma_info = {
>> +    .name          = TYPE_SPARC32_DMA,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(SPARC32DMAState),
>> +    .instance_init = sparc32_dma_init,
>> +    .class_init    = sparc32_dma_class_init,
>> +};
>> +
>> +
>>  static void sparc32_dma_register_types(void)
>>  {
>>      type_register_static(&sparc32_dma_device_info);
>>      type_register_static(&sparc32_espdma_device_info);
>>      type_register_static(&sparc32_ledma_device_info);
>> +    type_register_static(&sparc32_dma_info);
>>  }
>>  
>>  type_init(sparc32_dma_register_types)
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index ae486a4..5017ae5 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -307,18 +307,36 @@ static void *iommu_init(hwaddr addr, uint32_t version, 
>> qemu_irq irq)
>>      return s;
>>  }
>>  
>> -static void *sparc32_dma_init(hwaddr daddr, void *iommu, int is_ledma)
>> +static void *sparc32_dma_init(hwaddr dma_base,
>> +                              hwaddr esp_base, qemu_irq espdma_irq,
>> +                              hwaddr le_base, qemu_irq ledma_irq)
>>  {
>> -    DeviceState *dev;
>> -    SysBusDevice *s;
>> +    DeviceState *dma;
>> +    ESPDMADeviceState *espdma;
>> +    LEDMADeviceState *ledma;
>> +    SysBusESPState *esp;
>> +    SysBusPCNetState *lance;
>>  
>> -    dev = qdev_create(NULL, is_ledma ? "sparc32-ledma" : "sparc32-espdma");
> 
> TYPE_SPARC32_LEDMA_DEVICE, TYPE_SPARC32_ESPDMA_DEVICE
> 
>> -    object_property_set_link(OBJECT(dev), OBJECT(iommu), "iommu", 
>> &error_abort);
>> -    qdev_init_nofail(dev);
>> -    s = SYS_BUS_DEVICE(dev);
>> -    sysbus_mmio_map(s, 0, daddr);
>> +    dma = qdev_create(NULL, TYPE_SPARC32_DMA);
>> +    qdev_init_nofail(dma);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
>>  
>> -    return s;
>> +    espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>> +                                   OBJECT(dma), "espdma"));
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>> +
>> +    esp = ESP_STATE(object_resolve_path_component(OBJECT(espdma), "esp"));
> 
> TYPE_ESP?
> 
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
>> +
>> +    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"));
> 
> TYPE_LANCE?
> 
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
>> +
>> +    return dma;
>>  }
>>  
>>  static DeviceState *slavio_intctl_init(hwaddr addr,
>> @@ -798,9 +816,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
>> *hwdef,
>>      DeviceState *slavio_intctl;
>>      const char *cpu_model = machine->cpu_model;
>>      unsigned int i;
>> -    void *iommu, *nvram;
>> -    DeviceState *espdma, *esp, *ledma, *lance;
>> -    SysBusDevice *sbd;
>> +    void *nvram;
>>      qemu_irq *cpu_irqs[MAX_CPUS], slavio_irq[32], slavio_cpu_irq[MAX_CPUS];
>>      qemu_irq fdc_tc;
>>      unsigned long kernel_size;
>> @@ -848,8 +864,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
>> *hwdef,
>>          afx_init(hwdef->afx_base);
>>      }
>>  
>> -    iommu = iommu_init(hwdef->iommu_base, hwdef->iommu_version,
>> -                       slavio_irq[30]);
>> +    iommu_init(hwdef->iommu_base, hwdef->iommu_version, slavio_irq[30]);
>>  
>>      if (hwdef->iommu_pad_base) {
>>          /* On the real hardware (SS-5, LX) the MMU is not padded, but 
>> aliased.
>> @@ -859,26 +874,9 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
>> *hwdef,
>>          empty_slot_init(hwdef->iommu_pad_base,hwdef->iommu_pad_len);
>>      }
>>  
>> -    espdma = sparc32_dma_init(hwdef->dma_base, iommu, 0);
>> -    sbd = SYS_BUS_DEVICE(espdma);
>> -    sysbus_connect_irq(sbd, 0, slavio_irq[18]);
>> -
>> -    esp = DEVICE(object_resolve_path_component(OBJECT(espdma), "esp"));
>> -    sbd = SYS_BUS_DEVICE(esp);
>> -    sysbus_mmio_map(sbd, 0, hwdef->esp_base);
>> -    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(espdma, 0));
>> -    qdev_connect_gpio_out(espdma, 0, qdev_get_gpio_in(esp, 0));
>> -    qdev_connect_gpio_out(espdma, 1, qdev_get_gpio_in(esp, 1));
>> -
>> -    ledma = sparc32_dma_init(hwdef->dma_base + 16ULL, iommu, 1);
>> -    sbd = SYS_BUS_DEVICE(ledma);
>> -    sysbus_connect_irq(sbd, 0, slavio_irq[16]);
>> -
>> -    lance = DEVICE(object_resolve_path_component(OBJECT(ledma), "lance"));
>> -    sbd = SYS_BUS_DEVICE(lance);
>> -    sysbus_mmio_map(sbd, 0, hwdef->le_base);
>> -    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(ledma, 0));
>> -    qdev_connect_gpio_out(ledma, 0, qdev_get_gpio_in(lance, 0));
>> +    sparc32_dma_init(hwdef->dma_base,
>> +                     hwdef->esp_base, slavio_irq[18],
>> +                     hwdef->le_base, slavio_irq[16]);
>>  
>>      if (graphic_depth != 8 && graphic_depth != 24) {
>>          error_report("Unsupported depth: %d", graphic_depth);

I disagree with using the TYPE_* macros for the link property names
because semantically we aren't referencing the type, it just so happens
that I've used the same name as the type for the property name and I'd
like to keep this distinction.


ATB,

Mark.