Re: [Qemu-devel] [PATCH v2 1/4] usb/ehci: Clean up SysBus and PCI EHCI split

[Andreas Färber]

Am 17.12.2012 14:29, schrieb Gerd Hoffmann:
> On 12/16/12 04:49, Andreas Färber wrote:
>> SysBus EHCI was introduced in a hurry before 1.3 Soft Freeze.
>> To use QOM casts in place of DO_UPCAST() / FROM_SYSBUS(), we need an
>> identifying type. Introduce generic abstract base types for PCI and
>> SysBus EHCI to allow multiple types to access the shared fields.
>>
>> While at it, move the state structs being amended with macros to the
>> header file so that they can be embedded.
> 
> --verbose please.
> 
> I fail to see the point.  EHCIPCIState should not be needed outside of
> hcd-ehci-pci.c and I'd prefer to leave it there.  Likewise for sysbus.

It is exactly what I commented on my v1 for needing a v2 and you seemed
to concur... In C, to embed a struct in another struct the compiler
needs the full struct definition (compare i440fx, prep_pci series) and
it thus needs to be in an #include'able header. Could be a dedicated
hcd-ehci-sysbus.h if you prefer to not have hcd-ehci.h pull in sysbus.h.

A patch says more than a thousand words (to be rebased on this series
once merged):

diff --git a/hw/arm/tegra2.h b/hw/arm/tegra2.h
index 8037362..c3906d7 100644
--- a/hw/arm/tegra2.h
+++ b/hw/arm/tegra2.h
@@ -115,6 +115,9 @@ typedef struct Tegra2State {
     TegraClocksState clocks;
     SDHCIState sdhci[4];
     TegraI2CState i2c[4];
+#if 0
+    EHCISysBusState usb[3];
+#endif
 } Tegra2State;

 #endif /* softmmu */
diff --git a/hw/arm/tegra2.c b/hw/arm/tegra2.c
index d414eec..6d343f2 100644
--- a/hw/arm/tegra2.c
+++ b/hw/arm/tegra2.c
@@ -272,6 +272,36 @@ static void tegra2_initfn(Object *obj)
     sysbusdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sysbusdev, 0, 0x7000d000);
     sysbus_connect_irq(sysbusdev, 0, s->irq[53]);
+
+    /* USB EHCI host controllers */
+#if 0
+    object_initialize(&s->usb[0], TYPE_TEGRA2_EHCI);
+    dev = DEVICE(&s->usb[0]);
+    qdev_set_parent_bus(dev, sysbus_get_default());
+    object_property_add_child(obj, "usb[0]", (Object *) dev, NULL);
+    qdev_init_nofail(dev);
+    sysbusdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(sysbusdev, 0, 0xc5000000);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[20]);
+
+    object_initialize(&s->usb[1], TYPE_TEGRA2_EHCI);
+    dev = DEVICE(&s->usb[1]);
+    qdev_set_parent_bus(dev, sysbus_get_default());
+    object_property_add_child(obj, "usb[1]", (Object *) dev, NULL);
+    qdev_init_nofail(dev);
+    sysbusdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(sysbusdev, 0, 0xc5004000);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[21]);
+
+    object_initialize(&s->usb[2], TYPE_TEGRA2_EHCI);
+    dev = DEVICE(&s->usb[2]);
+    qdev_set_parent_bus(dev, sysbus_get_default());
+    object_property_add_child(obj, "usb[2]", (Object *) dev, NULL);
+    qdev_init_nofail(dev);
+    sysbusdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(sysbusdev, 0, 0xc5005000);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[97]);
+#endif
 }

 static const TypeInfo tegra2_type_info = {

Cf. http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/tegra

The qdev_init_nofail() in the QOM initfn is still ugly of course but
currently the SoC is a simple Object container and is lacking realize
support; also I'm hoping for the IRQ part to get overhauled with the Pin
series, leaving the MMIO mapping to be redone in an
instance_init-friendly way. (CC'ing PMM for new-style Tegra preview)

When working with PCI-based chipsets rather than physical cards, it may
make sense to embed EHCIPCIState as well (thinking of q35), so I moved
both to a header rather than putting PCI macros in the source file first
and having a large movement later when someone needs it, clobbering
git-blame. (CC'ing Anthony and Jason)

Regards,
Andreas