[Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB.

[Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB.

[Peter Crosthwaite]

Added Sysbus variant of EHCI and attached it to Xilinx Zynq. The EHCI stuff is going to useful for Tegra too.

See http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04100.html thread for the inital RFC discussion on this development.

Regession tested using i386 Debian (dd of=/dev/sdx ...)  to get some test coverage outside of Zynq. More testing welcome.

Few other bits and pieces of cleanup in there too, around the EHCI_DEBUG printfery.

The series will rebase without conflict on the current pending usb patch queue (usb.68).

Changed since V2:
No more usages of unions, seperated out PCI and Sysbus structs and codepaths completely.
Removed init_ram hack for unimplemented EHCI regions.
Removed debug message cleanup patch (conflicts with pending USB [PULL] (usb.68))
Changed since V1:
Overhauled properties - moved them to class properties rather than object properties.
Various Review based fixes (Please see individual patch change logs)
Peter Crosthwaite (7):
  usb/ehci: Use class_data to init PCI variations
  usb/ehci: parameterise the register region offsets
  usb/ehci: Abstract away PCI DMA API
  usb/ehci: seperate out PCIisms
  usb/ehci: Add Sysbus variant and Xilinx Zynq USB
  xilinx_zynq: add USB controllers
  usb/ehci: Guard definition of EHCI_DEBUG

Peter Maydell (1):
  dma: Define dma_context_memory and use in sysbus-ohci

 dma.h             |    5 +
 exec.c            |    5 +
 hw/usb/hcd-ehci.c |  339 +++++++++++++++++++++++++++++++++++------------------
 hw/usb/hcd-ohci.c |    2 +-
 hw/xilinx_zynq.c  |    3 +
 5 files changed, 238 insertions(+), 116 deletions(-)

[Qemu-devel] [PATCH v3 1/8] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Crosthwaite]

From: Peter Maydell <peter.maydell@linaro.org>

Define a new global dma_context_memory which is a DMAContext corresponding
to the global address_space_memory AddressSpace. This can be used by
sysbus peripherals like sysbus-ohci which need to do DMA.

In particular, use it in the sysbus-ohci device, which fixes a
segfault when attempting to use that device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 dma.h             |    5 +++++
 exec.c            |    5 +++++
 hw/usb/hcd-ohci.c |    2 +-
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/dma.h b/dma.h
index 91ccdb5..eedf878 100644
--- a/dma.h
+++ b/dma.h
@@ -68,6 +68,11 @@ struct DMAContext {
     DMAUnmapFunc *unmap;
 };
 
+/* A global DMA context corresponding to the address_space_memory
+ * AddressSpace, for sysbus devices which do DMA.
+ */
+extern DMAContext dma_context_memory;
+
 static inline void dma_barrier(DMAContext *dma, DMADirection dir)
 {
     /*
diff --git a/exec.c b/exec.c
index b0ed593..d2f6e79 100644
--- a/exec.c
+++ b/exec.c
@@ -34,6 +34,7 @@
 #include "hw/xen.h"
 #include "qemu-timer.h"
 #include "memory.h"
+#include "dma.h"
 #include "exec-memory.h"
 #if defined(CONFIG_USER_ONLY)
 #include <qemu.h>
@@ -103,6 +104,7 @@ static MemoryRegion *system_io;
 
 AddressSpace address_space_io;
 AddressSpace address_space_memory;
+DMAContext dma_context_memory;
 
 MemoryRegion io_mem_ram, io_mem_rom, io_mem_unassigned, io_mem_notdirty;
 static MemoryRegion io_mem_subpage_ram;
@@ -3276,6 +3278,9 @@ static void memory_map_init(void)
     memory_listener_register(&core_memory_listener, &address_space_memory);
     memory_listener_register(&io_memory_listener, &address_space_io);
     memory_listener_register(&tcg_memory_listener, &address_space_memory);
+
+    dma_context_init(&dma_context_memory, &address_space_memory,
+                     NULL, NULL, NULL);
 }
 
 MemoryRegion *get_system_memory(void)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 0cc1e5d..a58dfa0 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1846,7 +1846,7 @@ static int ohci_init_pxa(SysBusDevice *dev)
 
     /* Cannot fail as we pass NULL for masterbus */
     usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
-                  NULL);
+                  &dma_context_memory);
     sysbus_init_irq(dev, &s->ohci.irq);
     sysbus_init_mmio(dev, &s->ohci.mem);
 
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations

[Peter Crosthwaite]

Got rid of the duplication of the class init functions for the two PCI EHCI
variants. The PCI specifics are passed in as as class_data and set by a common
class_init function.

Premeptively defined a new Class "EHCICLass" for the upcomming addition of new
fields. The class_data is an instance of EHCICLass that forms a template for the
class to generate.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Got rid of union for sharing EHCIClassDefinition - made PCI specific
Simplified literal class_data arrays in ehci_info accordingly
removed null sentinel from ehci_info and used ARRAY_SIZE for type_regsiter loop
	bound instead

 hw/usb/hcd-ehci.c |   76 ++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6c65a73..274225b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ehci_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_ehci_initfn;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
-    k->revision = 0x10;
-    k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_ehci;
-    dc->props = ehci_properties;
-}
+typedef struct EHCIPCIClass {
+    PCIDeviceClass pci;
+} EHCIPCIClass;
 
-static TypeInfo ehci_info = {
-    .name          = "usb-ehci",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ehci_class_init,
-};
-
-static void ich9_ehci_class_init(ObjectClass *klass, void *data)
+static void ehci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_ehci_initfn;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
-    k->revision = 0x03;
-    k->class_id = PCI_CLASS_SERIAL_USB;
+    EHCIPCIClass *k = (EHCIPCIClass *)klass;
+    EHCIPCIClass *template = data;
+
+    k->pci.init = usb_ehci_initfn;
+    k->pci.vendor_id = template->pci.vendor_id;
+    k->pci.device_id = template->pci.device_id; /* ich4 */
+    k->pci.revision = template->pci.revision;
+    k->pci.class_id = PCI_CLASS_SERIAL_USB;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
 
-static TypeInfo ich9_ehci_info = {
-    .name          = "ich9-usb-ehci1",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ich9_ehci_class_init,
+static TypeInfo ehci_info[] = {
+    {
+        .name          = "usb-ehci",
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(EHCIState),
+        .class_init    = ehci_class_init,
+        .class_size    = sizeof(EHCIPCIClass),
+        .class_data    = (EHCIPCIClass[]) {{
+            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
+            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
+            .pci.revision  = 0x10,
+        } }
+    }, {
+        .name          = "ich9-usb-ehci1",
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(EHCIState),
+        .class_init    = ehci_class_init,
+        .class_size    = sizeof(EHCIPCIClass),
+        .class_data    = (EHCIPCIClass[]) {{
+            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
+            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
+            .pci.revision  = 0x03,
+        } }
+    },
 };
 
 static int usb_ehci_initfn(PCIDevice *dev)
@@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
 
 static void ehci_register_types(void)
 {
-    type_register_static(&ehci_info);
-    type_register_static(&ich9_ehci_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
+        type_register_static(&ehci_info[i]);
+    }
 }
 
 type_init(ehci_register_types)
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 3/8] usb/ehci: parameterise the register region offsets

[Peter Crosthwaite]

The capabilities register and operational register offsets can vary from one
EHCI implementation to the next. Parameterise accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v2:
Added strcut EHCIInfo to hold these two properties and added struct to
class definition. This struct can be passed around instead of the class to
share init functionailty (rather than the union apporach in prev revision)
changed from v1:
Moved opregbase and capregbase to class_data (Gerd Review)
Fixed capa regs to 16 bytes in length (Gerd Review)
Removed C++ comments touched by this patch (Checkpatch)

 hw/usb/hcd-ehci.c |   78 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 274225b..2519484 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -48,20 +48,18 @@
 #define USB_RET_PROCERR   (-99)
 
 #define MMIO_SIZE        0x1000
+#define CAPA_SIZE        0x10
 
 /* Capability Registers Base Address - section 2.2 */
-#define CAPREGBASE       0x0000
-#define CAPLENGTH        CAPREGBASE + 0x0000  // 1-byte, 0x0001 reserved
-#define HCIVERSION       CAPREGBASE + 0x0002  // 2-bytes, i/f version #
-#define HCSPARAMS        CAPREGBASE + 0x0004  // 4-bytes, structural params
-#define HCCPARAMS        CAPREGBASE + 0x0008  // 4-bytes, capability params
+#define CAPLENGTH        0x0000  /* 1-byte, 0x0001 reserved */
+#define HCIVERSION       0x0002  /* 2-bytes, i/f version # */
+#define HCSPARAMS        0x0004  /* 4-bytes, structural params */
+#define HCCPARAMS        0x0008  /* 4-bytes, capability params */
 #define EECP             HCCPARAMS + 1
-#define HCSPPORTROUTE1   CAPREGBASE + 0x000c
-#define HCSPPORTROUTE2   CAPREGBASE + 0x0010
+#define HCSPPORTROUTE1   0x000c
+#define HCSPPORTROUTE2   0x0010
 
-#define OPREGBASE        0x0020        // Operational Registers Base Address
-
-#define USBCMD           OPREGBASE + 0x0000
+#define USBCMD           0x0000
 #define USBCMD_RUNSTOP   (1 << 0)      // run / Stop
 #define USBCMD_HCRESET   (1 << 1)      // HC Reset
 #define USBCMD_FLS       (3 << 2)      // Frame List Size
@@ -75,7 +73,7 @@
 #define USBCMD_ITC       (0x7f << 16)  // Int Threshold Control
 #define USBCMD_ITC_SH    16            // Int Threshold Control Shift
 
-#define USBSTS           OPREGBASE + 0x0004
+#define USBSTS           0x0004
 #define USBSTS_RO_MASK   0x0000003f
 #define USBSTS_INT       (1 << 0)      // USB Interrupt
 #define USBSTS_ERRINT    (1 << 1)      // Error Interrupt
@@ -92,18 +90,18 @@
  *  Interrupt enable bits correspond to the interrupt active bits in USBSTS
  *  so no need to redefine here.
  */
-#define USBINTR              OPREGBASE + 0x0008
+#define USBINTR              0x0008
 #define USBINTR_MASK         0x0000003f
 
-#define FRINDEX              OPREGBASE + 0x000c
-#define CTRLDSSEGMENT        OPREGBASE + 0x0010
-#define PERIODICLISTBASE     OPREGBASE + 0x0014
-#define ASYNCLISTADDR        OPREGBASE + 0x0018
+#define FRINDEX              0x000c
+#define CTRLDSSEGMENT        0x0010
+#define PERIODICLISTBASE     0x0014
+#define ASYNCLISTADDR        0x0018
 #define ASYNCLISTADDR_MASK   0xffffffe0
 
-#define CONFIGFLAG           OPREGBASE + 0x0040
+#define CONFIGFLAG           0x0040
 
-#define PORTSC               (OPREGBASE + 0x0044)
+#define PORTSC               0x0044
 #define PORTSC_BEGIN         PORTSC
 #define PORTSC_END           (PORTSC + 4 * NB_PORTS)
 /*
@@ -399,14 +397,15 @@ struct EHCIState {
 
     /* properties */
     uint32_t maxframes;
+    uint16_t opregbase;
 
     /*
      *  EHCI spec version 1.0 Section 2.3
      *  Host Controller Operational Registers
      */
-    uint8_t caps[OPREGBASE];
+    uint8_t caps[CAPA_SIZE];
     union {
-        uint32_t opreg[(PORTSC_BEGIN-OPREGBASE)/sizeof(uint32_t)];
+        uint32_t opreg[PORTSC_BEGIN/sizeof(uint32_t)];
         struct {
             uint32_t usbcmd;
             uint32_t usbsts;
@@ -505,8 +504,7 @@ static const char *state2str(uint32_t state)
 
 static const char *addr2str(hwaddr addr)
 {
-    return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names),
-                  addr + OPREGBASE);
+    return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names), addr);
 }
 
 static void ehci_trace_usbsts(uint32_t mask, int state)
@@ -1114,7 +1112,7 @@ static uint64_t ehci_opreg_read(void *ptr, hwaddr addr,
     uint32_t val;
 
     val = s->opreg[addr >> 2];
-    trace_usb_ehci_opreg_read(addr + OPREGBASE, addr2str(addr), val);
+    trace_usb_ehci_opreg_read(addr + s->opregbase, addr2str(addr), val);
     return val;
 }
 
@@ -1210,9 +1208,9 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
     uint32_t old = *mmio;
     int i;
 
-    trace_usb_ehci_opreg_write(addr + OPREGBASE, addr2str(addr), val);
+    trace_usb_ehci_opreg_write(addr + s->opregbase, addr2str(addr), val);
 
-    switch (addr + OPREGBASE) {
+    switch (addr) {
     case USBCMD:
         if (val & USBCMD_HCRESET) {
             ehci_reset(s);
@@ -1290,7 +1288,8 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
     }
 
     *mmio = val;
-    trace_usb_ehci_opreg_change(addr + OPREGBASE, addr2str(addr), *mmio, old);
+    trace_usb_ehci_opreg_change(addr + s->opregbase, addr2str(addr),
+                                *mmio, old);
 }
 
 
@@ -2641,8 +2640,14 @@ static Property ehci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+typedef struct EHCIInfo {
+    uint16_t capabase;
+    uint16_t opregbase;
+} EHCIInfo;
+
 typedef struct EHCIPCIClass {
     PCIDeviceClass pci;
+    EHCIInfo ehci;
 } EHCIPCIClass;
 
 static void ehci_class_init(ObjectClass *klass, void *data)
@@ -2656,6 +2661,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     k->pci.device_id = template->pci.device_id; /* ich4 */
     k->pci.revision = template->pci.revision;
     k->pci.class_id = PCI_CLASS_SERIAL_USB;
+    k->ehci = template->ehci;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
@@ -2671,6 +2677,8 @@ static TypeInfo ehci_info[] = {
             .pci.vendor_id = PCI_VENDOR_ID_INTEL,
             .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
             .pci.revision  = 0x10,
+            .ehci.capabase  = 0x0,
+            .ehci.opregbase = 0x20,
         } }
     }, {
         .name          = "ich9-usb-ehci1",
@@ -2682,6 +2690,8 @@ static TypeInfo ehci_info[] = {
             .pci.vendor_id = PCI_VENDOR_ID_INTEL,
             .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
             .pci.revision  = 0x03,
+            .ehci.capabase  = 0x0,
+            .ehci.opregbase = 0x20,
         } }
     },
 };
@@ -2689,6 +2699,7 @@ static TypeInfo ehci_info[] = {
 static int usb_ehci_initfn(PCIDevice *dev)
 {
     EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
+    EHCIPCIClass *c = (EHCIPCIClass *)object_get_class(OBJECT(dev));
     uint8_t *pci_conf = s->dev.config;
     int i;
 
@@ -2721,8 +2732,10 @@ static int usb_ehci_initfn(PCIDevice *dev)
     pci_conf[0x6e] = 0x00;
     pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
 
+    s->opregbase = c->ehci.opregbase;
+
     /* 2.2 host controller interface version */
-    s->caps[0x00] = (uint8_t) OPREGBASE;
+    s->caps[0x00] = (uint8_t)(s->opregbase - c->ehci.capabase);
     s->caps[0x01] = 0x00;
     s->caps[0x02] = 0x00;
     s->caps[0x03] = 0x01;        /* HC version */
@@ -2755,15 +2768,16 @@ static int usb_ehci_initfn(PCIDevice *dev)
 
     memory_region_init(&s->mem, "ehci", MMIO_SIZE);
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
-                          "capabilities", OPREGBASE);
+                          "capabilities", CAPA_SIZE);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,
-                          "operational", PORTSC_BEGIN - OPREGBASE);
+                          "operational", PORTSC_BEGIN);
     memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s,
                           "ports", PORTSC_END - PORTSC_BEGIN);
 
-    memory_region_add_subregion(&s->mem, 0,            &s->mem_caps);
-    memory_region_add_subregion(&s->mem, OPREGBASE,    &s->mem_opreg);
-    memory_region_add_subregion(&s->mem, PORTSC_BEGIN, &s->mem_ports);
+    memory_region_add_subregion(&s->mem, c->ehci.capabase, &s->mem_caps);
+    memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
+    memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
+                                &s->mem_ports);
 
     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 4/8] usb/ehci: Abstract away PCI DMA API

[Peter Crosthwaite]

Pull the DMAContext for the PCI DMA out at device init time and put it into
the device state. Use dma_memory_read/write() instead of pci specific versions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 2519484..f4c2884 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -390,6 +390,7 @@ struct EHCIState {
     USBBus bus;
     qemu_irq irq;
     MemoryRegion mem;
+    DMAContext *dma;
     MemoryRegion mem_caps;
     MemoryRegion mem_opreg;
     MemoryRegion mem_ports;
@@ -1302,7 +1303,7 @@ static inline int get_dwords(EHCIState *ehci, uint32_t addr,
     int i;
 
     for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        pci_dma_read(&ehci->dev, addr, buf, sizeof(*buf));
+        dma_memory_read(ehci->dma, addr, buf, sizeof(*buf));
         *buf = le32_to_cpu(*buf);
     }
 
@@ -1317,7 +1318,7 @@ static inline int put_dwords(EHCIState *ehci, uint32_t addr,
 
     for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint32_t tmp = cpu_to_le32(*buf);
-        pci_dma_write(&ehci->dev, addr, &tmp, sizeof(tmp));
+        dma_memory_write(ehci->dma, addr, &tmp, sizeof(tmp));
     }
 
     return 1;
@@ -1400,7 +1401,7 @@ static int ehci_init_transfer(EHCIPacket *p)
     cpage  = get_field(p->qtd.token, QTD_TOKEN_CPAGE);
     bytes  = get_field(p->qtd.token, QTD_TOKEN_TBYTES);
     offset = p->qtd.bufptr[0] & ~QTD_BUFPTR_MASK;
-    pci_dma_sglist_init(&p->sgl, &p->queue->ehci->dev, 5);
+    qemu_sglist_init(&p->sgl, 5, p->queue->ehci->dma);
 
     while (bytes > 0) {
         if (cpage > 4) {
@@ -1629,7 +1630,7 @@ static int ehci_process_itd(EHCIState *ehci,
                 return USB_RET_PROCERR;
             }
 
-            pci_dma_sglist_init(&ehci->isgl, &ehci->dev, 2);
+            qemu_sglist_init(&ehci->isgl, 2, ehci->dma);
             if (off + len > 4096) {
                 /* transfer crosses page border */
                 uint32_t len2 = off + len - 4096;
@@ -2389,7 +2390,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         }
         list |= ((ehci->frindex & 0x1ff8) >> 1);
 
-        pci_dma_read(&ehci->dev, list, &entry, sizeof entry);
+        dma_memory_read(ehci->dma, list, &entry, sizeof entry);
         entry = le32_to_cpu(entry);
 
         DPRINTF("PERIODIC state adv fr=%d.  [%08X] -> %08X\n",
@@ -2750,6 +2751,8 @@ static int usb_ehci_initfn(PCIDevice *dev)
 
     s->irq = s->dev.irq[3];
 
+    s->dma = pci_dma_context(dev);
+
     usb_bus_new(&s->bus, &ehci_bus_ops, &s->dev.qdev);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 5/8] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

Seperate the PCI stuff from the EHCI components. Extracted the PCIDevice
out into a new wrapper struct to make EHCIState non-PCI-specific. Seperated
tho non PCI init component out into a seperate "common" init function.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed from v2:
s/ehci_properties/ehci_pci_properties
Got rid of the union and made EHCIItfState PCI specific
s/EHCIItfState/EHCIPCIState
usb_ehci_initfn: Pass in EHCI info. This means this function doesnt need to do
any Class casting mkaing it generic and not needing the old union.
Changed from v1:
renamed VMSD defintions to preserve backwards compatibility (Gerd Review)

 hw/usb/hcd-ehci.c |  135 ++++++++++++++++++++++++++++++-----------------------
 1 files changed, 77 insertions(+), 58 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index f4c2884..df224b2 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -386,7 +386,6 @@ struct EHCIQueue {
 typedef QTAILQ_HEAD(EHCIQueueHead, EHCIQueue) EHCIQueueHead;
 
 struct EHCIState {
-    PCIDevice dev;
     USBBus bus;
     qemu_irq irq;
     MemoryRegion mem;
@@ -446,6 +445,11 @@ struct EHCIState {
     uint32_t async_stepdown;
 };
 
+typedef struct EHCIPCIState {
+    PCIDevice pcidev;
+    EHCIState ehci;
+} EHCIPCIState;
+
 #define SET_LAST_RUN_CLOCK(s) \
     (s)->last_run_ns = qemu_get_clock_ns(vm_clock);
 
@@ -2539,7 +2543,7 @@ static const MemoryRegionOps ehci_mmio_port_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int usb_ehci_initfn(PCIDevice *dev);
+static int usb_ehci_pci_initfn(PCIDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
@@ -2600,12 +2604,11 @@ static void usb_ehci_vm_state_change(void *opaque, int running, RunState state)
 }
 
 static const VMStateDescription vmstate_ehci = {
-    .name        = "ehci",
+    .name        = "ehci-core",
     .version_id  = 2,
     .minimum_version_id  = 1,
     .post_load   = usb_ehci_post_load,
     .fields      = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, EHCIState),
         /* mmio registers */
         VMSTATE_UINT32(usbcmd, EHCIState),
         VMSTATE_UINT32(usbsts, EHCIState),
@@ -2636,8 +2639,19 @@ static const VMStateDescription vmstate_ehci = {
     }
 };
 
-static Property ehci_properties[] = {
-    DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
+static const VMStateDescription vmstate_ehci_pci = {
+    .name        = "ehci",
+    .version_id  = 2,
+    .minimum_version_id  = 1,
+    .post_load   = usb_ehci_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(pcidev, EHCIPCIState),
+        VMSTATE_STRUCT(ehci, EHCIPCIState, 2, vmstate_ehci, EHCIState),
+    }
+};
+
+static Property ehci_pci_properties[] = {
+    DEFINE_PROP_UINT32("maxframes", EHCIPCIState, ehci.maxframes, 128),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2651,28 +2665,28 @@ typedef struct EHCIPCIClass {
     EHCIInfo ehci;
 } EHCIPCIClass;
 
-static void ehci_class_init(ObjectClass *klass, void *data)
+static void ehci_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     EHCIPCIClass *k = (EHCIPCIClass *)klass;
     EHCIPCIClass *template = data;
 
-    k->pci.init = usb_ehci_initfn;
+    k->pci.init = usb_ehci_pci_initfn;
     k->pci.vendor_id = template->pci.vendor_id;
     k->pci.device_id = template->pci.device_id; /* ich4 */
     k->pci.revision = template->pci.revision;
     k->pci.class_id = PCI_CLASS_SERIAL_USB;
     k->ehci = template->ehci;
-    dc->vmsd = &vmstate_ehci;
-    dc->props = ehci_properties;
+    dc->vmsd = &vmstate_ehci_pci;
+    dc->props = ehci_pci_properties;
 }
 
 static TypeInfo ehci_info[] = {
     {
         .name          = "usb-ehci",
         .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIState),
-        .class_init    = ehci_class_init,
+        .instance_size = sizeof(EHCIPCIState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIPCIClass),
         .class_data    = (EHCIPCIClass[]) {{
             .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2684,8 +2698,8 @@ static TypeInfo ehci_info[] = {
     }, {
         .name          = "ich9-usb-ehci1",
         .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIState),
-        .class_init    = ehci_class_init,
+        .instance_size = sizeof(EHCIPCIState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIPCIClass),
         .class_data    = (EHCIPCIClass[]) {{
             .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2697,46 +2711,14 @@ static TypeInfo ehci_info[] = {
     },
 };
 
-static int usb_ehci_initfn(PCIDevice *dev)
+static void usb_ehci_initfn(EHCIState *s, DeviceState *dev, EHCIInfo *ei)
 {
-    EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
-    EHCIPCIClass *c = (EHCIPCIClass *)object_get_class(OBJECT(dev));
-    uint8_t *pci_conf = s->dev.config;
     int i;
 
-    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
-
-    /* capabilities pointer */
-    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
-    //pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50);
-
-    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
-    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
-    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
-
-    // pci_conf[0x50] = 0x01; // power management caps
-
-    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); // release number (2.1.4)
-    pci_set_byte(&pci_conf[0x61], 0x20);  // frame length adjustment (2.1.5)
-    pci_set_word(&pci_conf[0x62], 0x00);  // port wake up capability (2.1.6)
-
-    pci_conf[0x64] = 0x00;
-    pci_conf[0x65] = 0x00;
-    pci_conf[0x66] = 0x00;
-    pci_conf[0x67] = 0x00;
-    pci_conf[0x68] = 0x01;
-    pci_conf[0x69] = 0x00;
-    pci_conf[0x6a] = 0x00;
-    pci_conf[0x6b] = 0x00;  // USBLEGSUP
-    pci_conf[0x6c] = 0x00;
-    pci_conf[0x6d] = 0x00;
-    pci_conf[0x6e] = 0x00;
-    pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
-
-    s->opregbase = c->ehci.opregbase;
+    s->opregbase = ei->opregbase;
 
     /* 2.2 host controller interface version */
-    s->caps[0x00] = (uint8_t)(s->opregbase - c->ehci.capabase);
+    s->caps[0x00] = (uint8_t)(ei->opregbase - ei->capabase);
     s->caps[0x01] = 0x00;
     s->caps[0x02] = 0x00;
     s->caps[0x03] = 0x01;        /* HC version */
@@ -2749,11 +2731,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
     s->caps[0x0a] = 0x00;
     s->caps[0x0b] = 0x00;
 
-    s->irq = s->dev.irq[3];
-
-    s->dma = pci_dma_context(dev);
-
-    usb_bus_new(&s->bus, &ehci_bus_ops, &s->dev.qdev);
+    usb_bus_new(&s->bus, &ehci_bus_ops, dev);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
                           USB_SPEED_MASK_HIGH);
@@ -2777,12 +2755,53 @@ static int usb_ehci_initfn(PCIDevice *dev)
     memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s,
                           "ports", PORTSC_END - PORTSC_BEGIN);
 
-    memory_region_add_subregion(&s->mem, c->ehci.capabase, &s->mem_caps);
-    memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
-    memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
+    memory_region_add_subregion(&s->mem, ei->capabase, &s->mem_caps);
+    memory_region_add_subregion(&s->mem, ei->opregbase, &s->mem_opreg);
+    memory_region_add_subregion(&s->mem, ei->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+}
+
+static int usb_ehci_pci_initfn(PCIDevice *dev)
+{
+    EHCIPCIState *i = DO_UPCAST(EHCIPCIState, pcidev, dev);
+    EHCIPCIClass *c = (EHCIPCIClass *)object_get_class(OBJECT(dev));
+    EHCIState *s = &i->ehci;
+    uint8_t *pci_conf = dev->config;
+
+    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
+
+    /* capabilities pointer */
+    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
+    /* pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50); */
+
+    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
+    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
+    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
+
+    /* pci_conf[0x50] = 0x01; *//* power management caps */
+
+    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); /* release # (2.1.4) */
+    pci_set_byte(&pci_conf[0x61], 0x20);  /* frame length adjustment (2.1.5) */
+    pci_set_word(&pci_conf[0x62], 0x00);  /* port wake up capability (2.1.6) */
+
+    pci_conf[0x64] = 0x00;
+    pci_conf[0x65] = 0x00;
+    pci_conf[0x66] = 0x00;
+    pci_conf[0x67] = 0x00;
+    pci_conf[0x68] = 0x01;
+    pci_conf[0x69] = 0x00;
+    pci_conf[0x6a] = 0x00;
+    pci_conf[0x6b] = 0x00;  /* USBLEGSUP */
+    pci_conf[0x6c] = 0x00;
+    pci_conf[0x6d] = 0x00;
+    pci_conf[0x6e] = 0x00;
+    pci_conf[0x6f] = 0xc0;  /* USBLEFCTLSTS */
+
+    s->irq = dev->irq[3];
+    s->dma = pci_dma_context(dev);
 
-    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
+    usb_ehci_initfn(s, DEVICE(dev), &c->ehci);
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
     return 0;
 }
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 6/8] usb/ehci: Add Sysbus variant and Xilinx Zynq USB

[Peter Crosthwaite]

Add QOM class definition helpers for sysbus attached EHCI implementations and
added Xilinx Zynq USB implementation.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v2:
Squashed Xilinx zynq Defintion into this patch. Otherwise complie fails due
to werror and unsued ehci_sysbus_class_init fn.
Duplicated state struct (as EHCISysBusState) - no more union.
changed from v1:
Dont create a QOM definition for Sysbus EHCI, rather just add all the bits
and pieces. (Multiple) sysbus EHCI defs can be created by adding to the
type_info[] table.
Use dma_context_memory for sysbus DMA (PMM review)

 hw/usb/hcd-ehci.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index df224b2..7948146 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -35,6 +35,8 @@
 #include "trace.h"
 #include "dma.h"
 #include "sysemu.h"
+#include "hw/sysbus.h"
+#include "exec-memory.h"
 
 #define EHCI_DEBUG   0
 
@@ -450,6 +452,11 @@ typedef struct EHCIPCIState {
     EHCIState ehci;
 } EHCIPCIState;
 
+typedef struct EHCISysBusState {
+    SysBusDevice busdev;
+    EHCIState ehci;
+} EHCISysBusState;
+
 #define SET_LAST_RUN_CLOCK(s) \
     (s)->last_run_ns = qemu_get_clock_ns(vm_clock);
 
@@ -2544,6 +2551,7 @@ static const MemoryRegionOps ehci_mmio_port_ops = {
 };
 
 static int usb_ehci_pci_initfn(PCIDevice *dev);
+static int usb_ehci_sysbus_initfn(SysBusDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
@@ -2650,11 +2658,26 @@ static const VMStateDescription vmstate_ehci_pci = {
     }
 };
 
+static const VMStateDescription vmstate_ehci_sysbus = {
+    .name        = "ehci-sysbus",
+    .version_id  = 2,
+    .minimum_version_id  = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(ehci, EHCISysBusState, 2, vmstate_ehci, EHCIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static Property ehci_pci_properties[] = {
     DEFINE_PROP_UINT32("maxframes", EHCIPCIState, ehci.maxframes, 128),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static Property ehci_sysbus_properties[] = {
+    DEFINE_PROP_UINT32("maxframes", EHCISysBusState, ehci.maxframes, 128),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 typedef struct EHCIInfo {
     uint16_t capabase;
     uint16_t opregbase;
@@ -2665,6 +2688,11 @@ typedef struct EHCIPCIClass {
     EHCIInfo ehci;
 } EHCIPCIClass;
 
+typedef struct EHCISysBusClass {
+    SysBusDeviceClass sdc;
+    EHCIInfo ehci;
+} EHCISysBusClass;
+
 static void ehci_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2681,6 +2709,18 @@ static void ehci_pci_class_init(ObjectClass *klass, void *data)
     dc->props = ehci_pci_properties;
 }
 
+static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    EHCISysBusClass *k = (EHCISysBusClass *)klass;
+    EHCISysBusClass *template = data;
+
+    k->sdc.init = usb_ehci_sysbus_initfn;
+    k->ehci = template->ehci;
+    dc->vmsd = &vmstate_ehci_sysbus;
+    dc->props = ehci_sysbus_properties;
+}
+
 static TypeInfo ehci_info[] = {
     {
         .name          = "usb-ehci",
@@ -2708,6 +2748,16 @@ static TypeInfo ehci_info[] = {
             .ehci.capabase  = 0x0,
             .ehci.opregbase = 0x20,
         } }
+    }, {
+        .name          = "xlnx,ps7-usb",
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(EHCISysBusState),
+        .class_init    = ehci_sysbus_class_init,
+        .class_size    = sizeof(EHCISysBusClass),
+        .class_data    = (EHCISysBusClass[]) {{
+            .ehci.capabase = 0x100,
+            .ehci.opregbase = 0x140,
+        } }
     },
 };
 
@@ -2761,6 +2811,21 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev, EHCIInfo *ei)
                                 &s->mem_ports);
 }
 
+static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
+{
+    EHCISysBusState *i = FROM_SYSBUS(EHCISysBusState, dev);
+    EHCISysBusClass *c = (EHCISysBusClass *)object_get_class(OBJECT(dev));
+    EHCIState *s = &i->ehci;
+
+    s->dma = &dma_context_memory;
+
+    usb_ehci_initfn(s, DEVICE(dev), &c->ehci);
+    sysbus_init_irq(dev, &s->irq);
+    sysbus_init_mmio(dev, &s->mem);
+
+    return 0;
+}
+
 static int usb_ehci_pci_initfn(PCIDevice *dev)
 {
     EHCIPCIState *i = DO_UPCAST(EHCIPCIState, pcidev, dev);
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 7/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

Add the two usb controllers in Zynq.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v1:
Simplified to use sysbus_create_simple - dont need prop anymore

 hw/xilinx_zynq.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index c55dafb..154e397 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -150,6 +150,9 @@ static void zynq_init(QEMUMachineInitArgs *args)
     zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET]);
     zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET]);
 
+    sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
+    sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[75-IRQ_OFFSET]);
+
     sysbus_create_simple("cadence_uart", 0xE0000000, pic[59-IRQ_OFFSET]);
     sysbus_create_simple("cadence_uart", 0xE0001000, pic[82-IRQ_OFFSET]);
 
-- 
1.7.0.4

[Qemu-devel] [PATCH v3 8/8] usb/ehci: Guard definition of EHCI_DEBUG

[Peter Crosthwaite]

Guard against re-definition of EHCI_DEBUG. Allows for turning on of debug info
from configure (using --qemu-extra-cflags="-DEHCI_DEBUG=1") rather than source
code hacking.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 7948146..8bd3a9f 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -38,7 +38,9 @@
 #include "hw/sysbus.h"
 #include "exec-memory.h"
 
+#ifndef EHCI_DEBUG
 #define EHCI_DEBUG   0
+#endif
 
 #if EHCI_DEBUG
 #define DPRINTF printf
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB.

[Gerd Hoffmann]

On 10/29/12 02:34, Peter Crosthwaite wrote:
> Added Sysbus variant of EHCI and attached it to Xilinx Zynq. The EHCI stuff is going to useful for Tegra too.

Patch series added to usb patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB.

[Andreas Färber]

Am 29.10.2012 08:48, schrieb Gerd Hoffmann:
> On 10/29/12 02:34, Peter Crosthwaite wrote:
>> Added Sysbus variant of EHCI and attached it to Xilinx Zynq. The EHCI stuff is going to useful for Tegra too.
> 
> Patch series added to usb patch queue.

Wasn't there resistance against dma_context_memory in the other thread,
which this series is based on?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations

[Andreas Färber]

Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
> Got rid of the duplication of the class init functions for the two PCI EHCI
> variants. The PCI specifics are passed in as as class_data and set by a common
> class_init function.
> 
> Premeptively defined a new Class "EHCICLass" for the upcomming addition of new

"Preemptively", "upcoming"

> fields. The class_data is an instance of EHCICLass that forms a template for the
> class to generate.

Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
do you have any thoughts on this? The usual way would be to have a
dedicated EHCIInfo struct or so.

> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Got rid of union for sharing EHCIClassDefinition - made PCI specific
> Simplified literal class_data arrays in ehci_info accordingly
> removed null sentinel from ehci_info and used ARRAY_SIZE for type_regsiter loop
> 	bound instead
> 
>  hw/usb/hcd-ehci.c |   76 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 6c65a73..274225b 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void ehci_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_ehci_initfn;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
> -    k->revision = 0x10;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> -    dc->vmsd = &vmstate_ehci;
> -    dc->props = ehci_properties;
> -}
> +typedef struct EHCIPCIClass {
> +    PCIDeviceClass pci;
> +} EHCIPCIClass;
>  
> -static TypeInfo ehci_info = {
> -    .name          = "usb-ehci",
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(EHCIState),
> -    .class_init    = ehci_class_init,
> -};
> -
> -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
> +static void ehci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_ehci_initfn;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
> -    k->revision = 0x03;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> +    EHCIPCIClass *k = (EHCIPCIClass *)klass;

Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)

In this case however, please keep using PCIDeviceClass rather than
trying to access it through a named member. If we need to access any
dedicated EHCIPCIClass fields later in the series we can add additional
variables the QOM way.

> +    EHCIPCIClass *template = data;
> +
> +    k->pci.init = usb_ehci_initfn;
> +    k->pci.vendor_id = template->pci.vendor_id;
> +    k->pci.device_id = template->pci.device_id; /* ich4 */
> +    k->pci.revision = template->pci.revision;
> +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
>      dc->vmsd = &vmstate_ehci;
>      dc->props = ehci_properties;
>  }
>  
> -static TypeInfo ich9_ehci_info = {
> -    .name          = "ich9-usb-ehci1",
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(EHCIState),
> -    .class_init    = ich9_ehci_class_init,
> +static TypeInfo ehci_info[] = {

Can this still be made const despite the embedded class_data?

> +    {
> +        .name          = "usb-ehci",
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(EHCIState),
> +        .class_init    = ehci_class_init,
> +        .class_size    = sizeof(EHCIPCIClass),
> +        .class_data    = (EHCIPCIClass[]) {{
> +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
> +            .pci.revision  = 0x10,
> +        } }
> +    }, {
> +        .name          = "ich9-usb-ehci1",

Do we have a suitable header to introduce TYPE_* constants for these
while at it? That would benefit q35.

Andreas

> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(EHCIState),
> +        .class_init    = ehci_class_init,
> +        .class_size    = sizeof(EHCIPCIClass),
> +        .class_data    = (EHCIPCIClass[]) {{
> +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
> +            .pci.revision  = 0x03,
> +        } }
> +    },
>  };
>  
>  static int usb_ehci_initfn(PCIDevice *dev)
> @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
>  
>  static void ehci_register_types(void)
>  {
> -    type_register_static(&ehci_info);
> -    type_register_static(&ich9_ehci_info);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
> +        type_register_static(&ehci_info[i]);
> +    }
>  }
>  
>  type_init(ehci_register_types)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB.

[Gerd Hoffmann]

On 10/29/12 09:53, Andreas Färber wrote:
> Am 29.10.2012 08:48, schrieb Gerd Hoffmann:
>> On 10/29/12 02:34, Peter Crosthwaite wrote:
>>> Added Sysbus variant of EHCI and attached it to Xilinx Zynq. The EHCI stuff is going to useful for Tegra too.
>>
>> Patch series added to usb patch queue.
> 
> Wasn't there resistance against dma_context_memory in the other thread,
> which this series is based on?

Avi acked it, resistance was more on the matter-of-taste level, and it
is a bug which needs fixing.  So I think it is ok to include.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 6181 bytes --]

On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaerber@suse.de> wrote:
>
> Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
> > Got rid of the duplication of the class init functions for the two PCI
EHCI
> > variants. The PCI specifics are passed in as as class_data and set by a
common
> > class_init function.
> >
> > Premeptively defined a new Class "EHCICLass" for the upcomming addition
of new
>
> "Preemptively", "upcoming"
>
> > fields. The class_data is an instance of EHCICLass that forms a
template for the
> > class to generate.
>
> Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
> do you have any thoughts on this? The usual way would be to have a
> dedicated EHCIInfo struct or so.

Why? The class struct defines the exactly all information needed. Seems
redundant and error prone to have to maintain two structs with the same
fields?

> >
> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> > Got rid of union for sharing EHCIClassDefinition - made PCI specific
> > Simplified literal class_data arrays in ehci_info accordingly
> > removed null sentinel from ehci_info and used ARRAY_SIZE for
type_regsiter loop
> >       bound instead
> >
> >  hw/usb/hcd-ehci.c |   76
++++++++++++++++++++++++++++------------------------
> >  1 files changed, 41 insertions(+), 35 deletions(-)
> >
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 6c65a73..274225b 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > -static void ehci_class_init(ObjectClass *klass, void *data)
> > -{
> > -    DeviceClass *dc = DEVICE_CLASS(klass);
> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > -
> > -    k->init = usb_ehci_initfn;
> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
> > -    k->revision = 0x10;
> > -    k->class_id = PCI_CLASS_SERIAL_USB;
> > -    dc->vmsd = &vmstate_ehci;
> > -    dc->props = ehci_properties;
> > -}
> > +typedef struct EHCIPCIClass {
> > +    PCIDeviceClass pci;
> > +} EHCIPCIClass;
> >
> > -static TypeInfo ehci_info = {
> > -    .name          = "usb-ehci",
> > -    .parent        = TYPE_PCI_DEVICE,
> > -    .instance_size = sizeof(EHCIState),
> > -    .class_init    = ehci_class_init,
> > -};
> > -
> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
> > +static void ehci_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > -
> > -    k->init = usb_ehci_initfn;
> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
> > -    k->revision = 0x03;
> > -    k->class_id = PCI_CLASS_SERIAL_USB;
> > +    EHCIPCIClass *k = (EHCIPCIClass *)klass;
>
> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)
>

How is this possible whe TYPE_EHCI_PCI doesn't exist ? The FOO_CLASS macros
require the name of the class but that does not exist as its a dynamic
class.

> In this case however, please keep using PCIDeviceClass rather than
> trying to access it through a named member. If we need to access any
> dedicated EHCIPCIClass fields later in the series we can add additional
> variables the QOM way.

What do you mean the Qom way? Don't we just add fields to the class and
class_info and class_init copies them across ?

> > +    EHCIPCIClass *template = data;
> > +
> > +    k->pci.init = usb_ehci_initfn;
> > +    k->pci.vendor_id = template->pci.vendor_id;
> > +    k->pci.device_id = template->pci.device_id; /* ich4 */
> > +    k->pci.revision = template->pci.revision;
> > +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
> >      dc->vmsd = &vmstate_ehci;
> >      dc->props = ehci_properties;
> >  }
> >
> > -static TypeInfo ich9_ehci_info = {
> > -    .name          = "ich9-usb-ehci1",
> > -    .parent        = TYPE_PCI_DEVICE,
> > -    .instance_size = sizeof(EHCIState),
> > -    .class_init    = ich9_ehci_class_init,
> > +static TypeInfo ehci_info[] = {
>
> Can this still be made const despite the embedded class_data?
>
> > +    {
> > +        .name          = "usb-ehci",
> > +        .parent        = TYPE_PCI_DEVICE,
> > +        .instance_size = sizeof(EHCIState),
> > +        .class_init    = ehci_class_init,
> > +        .class_size    = sizeof(EHCIPCIClass),
> > +        .class_data    = (EHCIPCIClass[]) {{
> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
> > +            .pci.revision  = 0x10,
> > +        } }
> > +    }, {
> > +        .name          = "ich9-usb-ehci1",
>
> Do we have a suitable header to introduce TYPE_* constants for these
> while at it? That would benefit q35.
>

No because there are no TYPE_ constants. The classes are private to
hcd-ehci.c.

Regards
Peter

> Andreas
>
> > +        .parent        = TYPE_PCI_DEVICE,
> > +        .instance_size = sizeof(EHCIState),
> > +        .class_init    = ehci_class_init,
> > +        .class_size    = sizeof(EHCIPCIClass),
> > +        .class_data    = (EHCIPCIClass[]) {{
> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
> > +            .pci.revision  = 0x03,
> > +        } }
> > +    },
> >  };
> >
> >  static int usb_ehci_initfn(PCIDevice *dev)
> > @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
> >
> >  static void ehci_register_types(void)
> >  {
> > -    type_register_static(&ehci_info);
> > -    type_register_static(&ich9_ehci_info);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
> > +        type_register_static(&ehci_info[i]);
> > +    }
> >  }
> >
> >  type_init(ehci_register_types)
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

[-- Attachment #2: Type: text/html, Size: 7927 bytes --]

Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations

[Andreas Färber]

Am 29.10.2012 12:43, schrieb Peter Crosthwaite:
> 
> On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaerber@suse.de
> <mailto:afaerber@suse.de>> wrote:
>>
>> Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
>> > Got rid of the duplication of the class init functions for the two
> PCI EHCI
>> > variants. The PCI specifics are passed in as as class_data and set
> by a common
>> > class_init function.
>> >
>> > Premeptively defined a new Class "EHCICLass" for the upcomming
> addition of new
>>
>> "Preemptively", "upcoming"
>>
>> > fields. The class_data is an instance of EHCICLass that forms a
> template for the
>> > class to generate.
>>
>> Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
>> do you have any thoughts on this? The usual way would be to have a
>> dedicated EHCIInfo struct or so.
> 
> Why? The class struct defines the exactly all information needed. Seems
> redundant and error prone to have to maintain two structs with the same
> fields?

This was a general discussion, involving Anthony and others. One reason
I can think of is that PCIDeviceClass is much larger than the actual
values you are interested in. I once did such a hack involving
XtensaCPUClass and worked around it for applying I believe.

>> >
>> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com
> <mailto:peter.crosthwaite@xilinx.com>>
>> > ---
>> > Got rid of union for sharing EHCIClassDefinition - made PCI specific
>> > Simplified literal class_data arrays in ehci_info accordingly
>> > removed null sentinel from ehci_info and used ARRAY_SIZE for
> type_regsiter loop
>> >       bound instead
>> >
>> >  hw/usb/hcd-ehci.c |   76
> ++++++++++++++++++++++++++++------------------------
>> >  1 files changed, 41 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
>> > index 6c65a73..274225b 100644
>> > --- a/hw/usb/hcd-ehci.c
>> > +++ b/hw/usb/hcd-ehci.c
>> > @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
>> >      DEFINE_PROP_END_OF_LIST(),
>> >  };
>> >
>> > -static void ehci_class_init(ObjectClass *klass, void *data)
>> > -{
>> > -    DeviceClass *dc = DEVICE_CLASS(klass);
>> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > -    k->init = usb_ehci_initfn;
>> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
>> > -    k->revision = 0x10;
>> > -    k->class_id = PCI_CLASS_SERIAL_USB;
>> > -    dc->vmsd = &vmstate_ehci;
>> > -    dc->props = ehci_properties;
>> > -}
>> > +typedef struct EHCIPCIClass {
>> > +    PCIDeviceClass pci;
>> > +} EHCIPCIClass;
>> >
>> > -static TypeInfo ehci_info = {
>> > -    .name          = "usb-ehci",
>> > -    .parent        = TYPE_PCI_DEVICE,
>> > -    .instance_size = sizeof(EHCIState),
>> > -    .class_init    = ehci_class_init,
>> > -};
>> > -
>> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
>> > +static void ehci_class_init(ObjectClass *klass, void *data)
>> >  {
>> >      DeviceClass *dc = DEVICE_CLASS(klass);
>> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > -    k->init = usb_ehci_initfn;
>> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
>> > -    k->revision = 0x03;
>> > -    k->class_id = PCI_CLASS_SERIAL_USB;
>> > +    EHCIPCIClass *k = (EHCIPCIClass *)klass;
>>
>> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)
>>
> 
> How is this possible whe TYPE_EHCI_PCI doesn't exist ? The FOO_CLASS
> macros require the name of the class but that does not exist as its a
> dynamic class.

Well, why doesn't it exist? Surely a type exists that uses your
EHCIPCIClass struct and if we don't have an abstract base type for
individual models then adding a TypeInfo alongside the struct surely is
clean and trivial.

>> In this case however, please keep using PCIDeviceClass rather than
>> trying to access it through a named member. If we need to access any
>> dedicated EHCIPCIClass fields later in the series we can add additional
>> variables the QOM way.
> 
> What do you mean the Qom way? Don't we just add fields to the class and
> class_info and class_init copies them across ?

The QOM way refers to accessing fields directly: k->foo, not x->bar.foo.
Your patch adds unnecessary indirection here when all you're actually
changing is the assignment of three values. This was discussed between
Anthony, mst and others in lengths.

>> > +    EHCIPCIClass *template = data;
>> > +
>> > +    k->pci.init = usb_ehci_initfn;
>> > +    k->pci.vendor_id = template->pci.vendor_id;
>> > +    k->pci.device_id = template->pci.device_id; /* ich4 */
>> > +    k->pci.revision = template->pci.revision;
>> > +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
>> >      dc->vmsd = &vmstate_ehci;
>> >      dc->props = ehci_properties;
>> >  }
>> >
>> > -static TypeInfo ich9_ehci_info = {
>> > -    .name          = "ich9-usb-ehci1",
>> > -    .parent        = TYPE_PCI_DEVICE,
>> > -    .instance_size = sizeof(EHCIState),
>> > -    .class_init    = ich9_ehci_class_init,
>> > +static TypeInfo ehci_info[] = {
>>
>> Can this still be made const despite the embedded class_data?
>>
>> > +    {
>> > +        .name          = "usb-ehci",
>> > +        .parent        = TYPE_PCI_DEVICE,
>> > +        .instance_size = sizeof(EHCIState),
>> > +        .class_init    = ehci_class_init,
>> > +        .class_size    = sizeof(EHCIPCIClass),
>> > +        .class_data    = (EHCIPCIClass[]) {{
>> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
>> > +            .pci.revision  = 0x10,
>> > +        } }
>> > +    }, {
>> > +        .name          = "ich9-usb-ehci1",
>>
>> Do we have a suitable header to introduce TYPE_* constants for these
>> while at it? That would benefit q35.
>>
> 
> No because there are no TYPE_ constants. The classes are private to
> hcd-ehci.c.

Note that I wrote "introduce TYPE_* constants". What about hw/usb.h at
least for instantiatable types? Question was equally addressed to Gerd.

No doubt this is v1.3 material but we shouldn't rush pulling something
in just because of the Soft Freeze. ;) Adding constants can be done as
follow-up, but the QOM issues are easier fixed from the start.

Regards,
Andreas

>> > +        .parent        = TYPE_PCI_DEVICE,
>> > +        .instance_size = sizeof(EHCIState),
>> > +        .class_init    = ehci_class_init,
>> > +        .class_size    = sizeof(EHCIPCIClass),
>> > +        .class_data    = (EHCIPCIClass[]) {{
>> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
>> > +            .pci.revision  = 0x03,
>> > +        } }
>> > +    },
>> >  };
>> >
>> >  static int usb_ehci_initfn(PCIDevice *dev)
>> > @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
>> >
>> >  static void ehci_register_types(void)
>> >  {
>> > -    type_register_static(&ehci_info);
>> > -    type_register_static(&ich9_ehci_info);
>> > +    int i;
>> > +
>> > +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
>> > +        type_register_static(&ehci_info[i]);
>> > +    }
>> >  }
>> >
>> >  type_init(ehci_register_types)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg