[Qemu-devel] [PATCH v2 0/4] usb: Clean up and extend SysBus EHCI

[Qemu-devel] [PATCH v2 0/4] usb: Clean up and extend SysBus EHCI

[Andreas Färber]

Hello Gerd,

Here's an updated patchset for SysBus EHCI models, fixing exynos4210 values.
As indicated, I've moved the state structs to the hcd-ehci.h header to allow my
experimental Tegra model to embed EHCISysBusState into the SoC object's struct.

When you and reviewers are happy, please apply through your USB queue.

Potential UHCI cleanups can be done in a separate series later on.
Also any multi-EHCI issues discussed are outside the scope of this series.

Regards,
Andreas

v1 -> v2:
* Moved structs to header and add TYPE_* and cast macros there instead
* Fixed exynos4210 opregbase as pointed out by Igor
* Renamed the new "exynos4210-ehci" type, suggested by Igor
* Dropped patch showcasing "tegra2-ehci[-usb]" model (deferred to Tegra series)
* Added gtk-doc annotations for the parent instance/class fields

Changes from Liming Wang's patchset:
* Introduce abstract sysbus-ehci-usb and pci-ehci-usb types
* Move class_init / instance_init to base type as far as possible
* Use TYPE_* constant for instantiating exynos4210 EHCI
* Added my tegra2 SysBus EHCI device to the mix

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Peter A. G. Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Igor Mitsyanko <i.mitsyanko@samsung.com>
Cc: Liming Wang <walimisdev@gmail.com>

Andreas Färber (3):
  usb/ehci: Clean up SysBus and PCI EHCI split
  usb/ehci: Move capsbase and opregbase into SysBus EHCI class
  usb/ehci: Add SysBus EHCI device for Exynos4210

Liming Wang (1):
  exynos4210: Add EHCI support

 hw/exynos4210.c          |    7 +++++++
 hw/exynos4210_gic.c      |    2 +-
 hw/usb/hcd-ehci-pci.c    |   39 ++++++++++++++++++++++--------------
 hw/usb/hcd-ehci-sysbus.c |   49 +++++++++++++++++++++++++++++++++++-----------
 hw/usb/hcd-ehci.h        |   46 +++++++++++++++++++++++++++++++++++++++++++
 5 Dateien geändert, 116 Zeilen hinzugefügt(+), 27 Zeilen entfernt(-)

-- 
1.7.10.4

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

[Andreas Färber]

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.

The VMSTATE_PCI_DEVICE() macro does not play nice with the QOM
parent_obj naming convention, so defer that cleanup.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/usb/hcd-ehci-pci.c    |   39 ++++++++++++++++++++++++---------------
 hw/usb/hcd-ehci-sysbus.c |   19 ++++++++++---------
 hw/usb/hcd-ehci.h        |   31 +++++++++++++++++++++++++++++++
 3 Dateien geändert, 65 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)

diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 41dbb53..c3843f6 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -16,14 +16,8 @@
  */
 
 #include "hw/usb/hcd-ehci.h"
-#include "hw/pci.h"
 #include "range.h"
 
-typedef struct EHCIPCIState {
-    PCIDevice pcidev;
-    EHCIState ehci;
-} EHCIPCIState;
-
 typedef struct EHCIPCIInfo {
     const char *name;
     uint16_t vendor_id;
@@ -33,7 +27,7 @@ typedef struct EHCIPCIInfo {
 
 static int usb_ehci_pci_initfn(PCIDevice *dev)
 {
-    EHCIPCIState *i = DO_UPCAST(EHCIPCIState, pcidev, dev);
+    EHCIPCIState *i = PCI_EHCI(dev);
     EHCIState *s = &i->ehci;
     uint8_t *pci_conf = dev->config;
 
@@ -83,7 +77,7 @@ static int usb_ehci_pci_initfn(PCIDevice *dev)
 static void usb_ehci_pci_write_config(PCIDevice *dev, uint32_t addr,
                                       uint32_t val, int l)
 {
-    EHCIPCIState *i = DO_UPCAST(EHCIPCIState, pcidev, dev);
+    EHCIPCIState *i = PCI_EHCI(dev);
     bool busmaster;
 
     pci_default_write_config(dev, addr, val, l);
@@ -115,12 +109,8 @@ static void ehci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-    EHCIPCIInfo *i = data;
 
     k->init = usb_ehci_pci_initfn;
-    k->vendor_id = i->vendor_id;
-    k->device_id = i->device_id;
-    k->revision = i->revision;
     k->class_id = PCI_CLASS_SERIAL_USB;
     k->config_write = usb_ehci_pci_write_config;
     k->no_hotplug = 1;
@@ -128,6 +118,24 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     dc->props = ehci_pci_properties;
 }
 
+static const TypeInfo ehci_pci_type_info = {
+    .name = TYPE_PCI_EHCI,
+    .parent = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(EHCIPCIState),
+    .abstract = true,
+    .class_init = ehci_class_init,
+};
+
+static void ehci_data_class_init(ObjectClass *klass, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    EHCIPCIInfo *i = data;
+
+    k->vendor_id = i->vendor_id;
+    k->device_id = i->device_id;
+    k->revision = i->revision;
+}
+
 static struct EHCIPCIInfo ehci_pci_info[] = {
     {
         .name      = "usb-ehci",
@@ -150,12 +158,13 @@ static struct EHCIPCIInfo ehci_pci_info[] = {
 static void ehci_pci_register_types(void)
 {
     TypeInfo ehci_type_info = {
-        .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIPCIState),
-        .class_init    = ehci_class_init,
+        .parent        = TYPE_PCI_EHCI,
+        .class_init    = ehci_data_class_init,
     };
     int i;
 
+    type_register_static(&ehci_pci_type_info);
+
     for (i = 0; i < ARRAY_SIZE(ehci_pci_info); i++) {
         ehci_type_info.name = ehci_pci_info[i].name;
         ehci_type_info.class_data = ehci_pci_info + i;
diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 803df92..d431193 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -16,12 +16,6 @@
  */
 
 #include "hw/usb/hcd-ehci.h"
-#include "hw/sysbus.h"
-
-typedef struct EHCISysBusState {
-    SysBusDevice busdev;
-    EHCIState ehci;
-} EHCISysBusState;
 
 static const VMStateDescription vmstate_ehci_sysbus = {
     .name        = "ehci-sysbus",
@@ -40,7 +34,7 @@ static Property ehci_sysbus_properties[] = {
 
 static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
 {
-    EHCISysBusState *i = FROM_SYSBUS(EHCISysBusState, dev);
+    EHCISysBusState *i = SYS_BUS_EHCI(dev);
     EHCIState *s = &i->ehci;
 
     s->capsbase = 0x100;
@@ -63,15 +57,22 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
     dc->props = ehci_sysbus_properties;
 }
 
-TypeInfo ehci_xlnx_type_info = {
-    .name          = "xlnx,ps7-usb",
+static const TypeInfo ehci_type_info = {
+    .name          = TYPE_SYS_BUS_EHCI,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(EHCISysBusState),
+    .abstract      = true,
     .class_init    = ehci_sysbus_class_init,
 };
 
+static const TypeInfo ehci_xlnx_type_info = {
+    .name          = "xlnx,ps7-usb",
+    .parent        = TYPE_SYS_BUS_EHCI,
+};
+
 static void ehci_sysbus_register_types(void)
 {
+    type_register_static(&ehci_type_info);
     type_register_static(&ehci_xlnx_type_info);
 }
 
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 772870b..9825b19 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -14,6 +14,8 @@
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
+#ifndef QEMU_USB_EHCI_H
+#define QEMU_USB_EHCI_H
 
 #include "hw/hw.h"
 #include "qemu-timer.h"
@@ -22,6 +24,8 @@
 #include "trace.h"
 #include "dma.h"
 #include "sysemu.h"
+#include "hw/pci.h"
+#include "hw/sysbus.h"
 
 #ifndef EHCI_DEBUG
 #define EHCI_DEBUG   0
@@ -318,3 +322,30 @@ struct EHCIState {
 extern const VMStateDescription vmstate_ehci;
 
 void usb_ehci_initfn(EHCIState *s, DeviceState *dev);
+
+
+#define TYPE_PCI_EHCI "pci-ehci-usb"
+#define PCI_EHCI(obj) OBJECT_CHECK(EHCIPCIState, (obj), TYPE_PCI_EHCI)
+
+typedef struct EHCIPCIState {
+    /*< private >*/
+    PCIDevice pcidev;
+    /*< public >*/
+
+    EHCIState ehci;
+} EHCIPCIState;
+
+
+#define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
+#define SYS_BUS_EHCI(obj) \
+    OBJECT_CHECK(EHCISysBusState, (obj), TYPE_SYS_BUS_EHCI)
+
+typedef struct EHCISysBusState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    EHCIState ehci;
+} EHCISysBusState;
+
+#endif
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 2/4] usb/ehci: Move capsbase and opregbase into SysBus EHCI class

[Andreas Färber]

This allows specific derived models to use different values.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/usb/hcd-ehci-sysbus.c |   15 +++++++++++++--
 hw/usb/hcd-ehci.h        |   13 +++++++++++++
 2 Dateien geändert, 26 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)

diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index d431193..5a94ce3 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -35,10 +35,11 @@ static Property ehci_sysbus_properties[] = {
 static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
 {
     EHCISysBusState *i = SYS_BUS_EHCI(dev);
+    SysBusEHCIClass *sec = SYS_BUS_EHCI_GET_CLASS(dev);
     EHCIState *s = &i->ehci;
 
-    s->capsbase = 0x100;
-    s->opregbase = 0x140;
+    s->capsbase = sec->capsbase;
+    s->opregbase = sec->opregbase;
     s->dma = &dma_context_memory;
 
     usb_ehci_initfn(s, DEVICE(dev));
@@ -63,11 +64,21 @@ static const TypeInfo ehci_type_info = {
     .instance_size = sizeof(EHCISysBusState),
     .abstract      = true,
     .class_init    = ehci_sysbus_class_init,
+    .class_size    = sizeof(SysBusEHCIClass),
 };
 
+static void ehci_xlnx_class_init(ObjectClass *oc, void *data)
+{
+    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
+
+    sec->capsbase = 0x100;
+    sec->opregbase = 0x140;
+}
+
 static const TypeInfo ehci_xlnx_type_info = {
     .name          = "xlnx,ps7-usb",
     .parent        = TYPE_SYS_BUS_EHCI,
+    .class_init    = ehci_xlnx_class_init,
 };
 
 static void ehci_sysbus_register_types(void)
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 9825b19..9a62cbe 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -339,6 +339,10 @@ typedef struct EHCIPCIState {
 #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
 #define SYS_BUS_EHCI(obj) \
     OBJECT_CHECK(EHCISysBusState, (obj), TYPE_SYS_BUS_EHCI)
+#define SYS_BUS_EHCI_CLASS(class) \
+    OBJECT_CLASS_CHECK(SysBusEHCIClass, (class), TYPE_SYS_BUS_EHCI)
+#define SYS_BUS_EHCI_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(SysBusEHCIClass, (obj), TYPE_SYS_BUS_EHCI)
 
 typedef struct EHCISysBusState {
     /*< private >*/
@@ -348,4 +352,13 @@ typedef struct EHCISysBusState {
     EHCIState ehci;
 } EHCISysBusState;
 
+typedef struct SysBusEHCIClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    uint16_t capsbase;
+    uint16_t opregbase;
+} SysBusEHCIClass;
+
 #endif
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 3/4] usb/ehci: Add SysBus EHCI device for Exynos4210

[Andreas Färber]

It uses a different capsbase and opregbase than the Xilinx device.

Signed-off-by: Liming Wang <walimisdev@gmail.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Cc: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/usb/hcd-ehci-sysbus.c |   15 +++++++++++++++
 hw/usb/hcd-ehci.h        |    2 ++
 2 Dateien geändert, 17 Zeilen hinzugefügt(+)

diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 5a94ce3..b68a66a 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -81,10 +81,25 @@ static const TypeInfo ehci_xlnx_type_info = {
     .class_init    = ehci_xlnx_class_init,
 };
 
+static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
+{
+    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
+
+    sec->capsbase = 0x0;
+    sec->opregbase = 0x10;
+}
+
+static const TypeInfo ehci_exynos4210_type_info = {
+    .name          = TYPE_EXYNOS4210_EHCI,
+    .parent        = TYPE_SYS_BUS_EHCI,
+    .class_init    = ehci_exynos4210_class_init,
+};
+
 static void ehci_sysbus_register_types(void)
 {
     type_register_static(&ehci_type_info);
     type_register_static(&ehci_xlnx_type_info);
+    type_register_static(&ehci_exynos4210_type_info);
 }
 
 type_init(ehci_sysbus_register_types)
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 9a62cbe..2620926 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -337,6 +337,8 @@ typedef struct EHCIPCIState {
 
 
 #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
+#define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
+
 #define SYS_BUS_EHCI(obj) \
     OBJECT_CHECK(EHCISysBusState, (obj), TYPE_SYS_BUS_EHCI)
 #define SYS_BUS_EHCI_CLASS(class) \
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 4/4] exynos4210: Add EHCI support

[Andreas Färber]

From: Liming Wang <walimisdev@gmail.com>

Add EHCI USB host controller to exynos4210.

Signed-off-by: Liming Wang <walimisdev@gmail.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Reviewed-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/exynos4210.c     |    7 +++++++
 hw/exynos4210_gic.c |    2 +-
 2 Dateien geändert, 8 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index 22148cd..29f55bb 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -27,6 +27,7 @@
 #include "arm-misc.h"
 #include "loader.h"
 #include "exynos4210.h"
+#include "usb/hcd-ehci.h"
 
 #define EXYNOS4210_CHIPID_ADDR         0x10000000
 
@@ -72,6 +73,9 @@
 /* Display controllers (FIMD) */
 #define EXYNOS4210_FIMD0_BASE_ADDR          0x11C00000
 
+/* EHCI */
+#define EXYNOS4210_EHCI_BASE_ADDR           0x12580000
+
 static uint8_t chipid_and_omr[] = { 0x11, 0x02, 0x21, 0x43,
                                     0x09, 0x00, 0x00, 0x00 };
 
@@ -338,5 +342,8 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
             s->irq_table[exynos4210_get_irq(11, 2)],
             NULL);
 
+    sysbus_create_simple(TYPE_EXYNOS4210_EHCI, EXYNOS4210_EHCI_BASE_ADDR,
+            s->irq_table[exynos4210_get_irq(28, 3)]);
+
     return s;
 }
diff --git a/hw/exynos4210_gic.c b/hw/exynos4210_gic.c
index 4fea098..959de56 100644
--- a/hw/exynos4210_gic.c
+++ b/hw/exynos4210_gic.c
@@ -140,7 +140,7 @@ combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
             EXT_GIC_ID_I2C4, EXT_GIC_ID_I2C5, EXT_GIC_ID_I2C6,
             EXT_GIC_ID_I2C7 },
     /* int combiner group 28 */
-    { EXT_GIC_ID_SPI0, EXT_GIC_ID_SPI1, EXT_GIC_ID_SPI2 },
+    { EXT_GIC_ID_SPI0, EXT_GIC_ID_SPI1, EXT_GIC_ID_SPI2 , EXT_GIC_ID_USB_HOST},
     /* int combiner group 29 */
     { EXT_GIC_ID_HSMMC0, EXT_GIC_ID_HSMMC1, EXT_GIC_ID_HSMMC2,
      EXT_GIC_ID_HSMMC3, EXT_GIC_ID_SDMMC },
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 3/4] usb/ehci: Add SysBus EHCI device for Exynos4210

[Igor Mitsyanko]

On 12/16/2012 07:49 AM, Andreas Färber wrote:
> It uses a different capsbase and opregbase than the Xilinx device.
>
> Signed-off-by: Liming Wang <walimisdev@gmail.com>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> Cc: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>   hw/usb/hcd-ehci-sysbus.c |   15 +++++++++++++++
>   hw/usb/hcd-ehci.h        |    2 ++
>   2 Dateien geändert, 17 Zeilen hinzugefügt(+)

Reviewed-by: Igor Mitsyanko<i.mitsyanko@samsung.com>



> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 5a94ce3..b68a66a 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -81,10 +81,25 @@ static const TypeInfo ehci_xlnx_type_info = {
>       .class_init    = ehci_xlnx_class_init,
>   };
>   
> +static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
> +{
> +    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> +
> +    sec->capsbase = 0x0;
> +    sec->opregbase = 0x10;
> +}
> +
> +static const TypeInfo ehci_exynos4210_type_info = {
> +    .name          = TYPE_EXYNOS4210_EHCI,
> +    .parent        = TYPE_SYS_BUS_EHCI,
> +    .class_init    = ehci_exynos4210_class_init,
> +};
> +
>   static void ehci_sysbus_register_types(void)
>   {
>       type_register_static(&ehci_type_info);
>       type_register_static(&ehci_xlnx_type_info);
> +    type_register_static(&ehci_exynos4210_type_info);
>   }
>   
>   type_init(ehci_sysbus_register_types)
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index 9a62cbe..2620926 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -337,6 +337,8 @@ typedef struct EHCIPCIState {
>   
>   
>   #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
> +#define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
> +
>   #define SYS_BUS_EHCI(obj) \
>       OBJECT_CHECK(EHCISysBusState, (obj), TYPE_SYS_BUS_EHCI)
>   #define SYS_BUS_EHCI_CLASS(class) \

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com

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

[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.

cheers,
  Gerd

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

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

[Gerd Hoffmann]

  Hi,

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

Sure.

> @@ -115,6 +115,9 @@ typedef struct Tegra2State {
>      TegraClocksState clocks;
>      SDHCIState sdhci[4];
>      TegraI2CState i2c[4];
> +#if 0
> +    EHCISysBusState usb[3];
> +#endif
>  } Tegra2State;

Ah, *that* is the place where you need it (outside hcd-ehci-sysbus.c).
Makes sense indeed.

I'll go put the bits as-is into the usb queue.

cheers,
  Gerd