[Qemu-devel] [PATCH v2 00/11] Sysbus EHCI + Zynq USB.

[Qemu-devel] [PATCH v2 00/11] 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 wel
come.

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

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 (10):
  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 Infrastructure
  usb/ehci: Add Xilinx ps7 USB controller
  xilinx_zynq: add USB controllers
  usb/ehci: Guard definition of EHCI_DEBUG
  usb/ehci: Debug mode compile fixes
  usb/ehci: Put RAM in undefined MMIO regions

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

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

[Qemu-devel] [PATCH v2 01/11] 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 v2 02/11] 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>
---

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6c65a73..625ec2a 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2641,46 +2641,54 @@ 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;
-}
-
-static TypeInfo ehci_info = {
-    .name          = "usb-ehci",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ehci_class_init,
-};
+typedef struct EHCIClass {
+    union {
+        PCIDeviceClass pci;
+    };
+} EHCIClass;
 
-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;
+    /* FIXME: how do you do object check for a dynamic class? */
+    EHCIClass *k = (EHCIClass *)klass;
+    EHCIClass *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(EHCIClass),
+        .class_data    = (EHCIClass[]) {{{
+                .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(EHCIClass),
+        .class_data    = (EHCIClass[]) {{{
+                .pci.vendor_id = PCI_VENDOR_ID_INTEL,
+                .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
+                .pci.revision  = 0x03,
+            }
+        } }
+    }, { .name = NULL }
 };
 
 static int usb_ehci_initfn(PCIDevice *dev)
@@ -2769,8 +2777,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);
+    TypeInfo *ti;
+
+    for (ti = ehci_info; ti->name; ti++) {
+        type_register_static(ti);
+    }
 }
 
 type_init(ehci_register_types)
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 03/11] 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 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 |   80 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 625ec2a..1d99f5b 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);
 }
 
 
@@ -2645,6 +2644,9 @@ typedef struct EHCIClass {
     union {
         PCIDeviceClass pci;
     };
+
+    uint16_t capabase;
+    uint16_t opregbase;
 } EHCIClass;
 
 static void ehci_class_init(ObjectClass *klass, void *data)
@@ -2659,6 +2661,8 @@ 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->opregbase = template->opregbase;
+    k->capabase = template->capabase;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
@@ -2674,7 +2678,9 @@ static TypeInfo ehci_info[] = {
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
                 .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
                 .pci.revision  = 0x10,
-            }
+            },
+            0x00,
+            0x20,
         } }
     }, {
         .name          = "ich9-usb-ehci1",
@@ -2686,7 +2692,9 @@ static TypeInfo ehci_info[] = {
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
                 .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
                 .pci.revision  = 0x03,
-            }
+            },
+            0x00,
+            0x20,
         } }
     }, { .name = NULL }
 };
@@ -2694,6 +2702,7 @@ static TypeInfo ehci_info[] = {
 static int usb_ehci_initfn(PCIDevice *dev)
 {
     EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
+    EHCIClass *c = (EHCIClass *)object_get_class(OBJECT(dev));
     uint8_t *pci_conf = s->dev.config;
     int i;
 
@@ -2726,8 +2735,10 @@ static int usb_ehci_initfn(PCIDevice *dev)
     pci_conf[0x6e] = 0x00;
     pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
 
+    s->opregbase = c->opregbase;
+
     /* 2.2 host controller interface version */
-    s->caps[0x00] = (uint8_t) OPREGBASE;
+    s->caps[0x00] = (uint8_t)(s->opregbase - c->capabase);
     s->caps[0x01] = 0x00;
     s->caps[0x02] = 0x00;
     s->caps[0x03] = 0x01;        /* HC version */
@@ -2760,15 +2771,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->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 v2 04/11] 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 1d99f5b..28bd7e9 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",
@@ -2753,6 +2754,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 v2 05/11] 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 v1:
renamed VMSD defintions to preserve backwards compatibility (Gerd Review)

 hw/usb/hcd-ehci.c |  136 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 84 insertions(+), 52 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 28bd7e9..1c5e5ed 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,13 @@ struct EHCIState {
     uint32_t async_stepdown;
 };
 
+typedef struct EHCItfState {
+    union {
+        PCIDevice pcidev;
+    };
+    struct EHCIState ehci;
+} EHCIItfState;
+
 #define SET_LAST_RUN_CLOCK(s) \
     (s)->last_run_ns = qemu_get_clock_ns(vm_clock);
 
@@ -2539,7 +2545,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 +2606,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 +2641,19 @@ static const VMStateDescription vmstate_ehci = {
     }
 };
 
+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, EHCIItfState),
+        VMSTATE_STRUCT(ehci, EHCIItfState, 2, vmstate_ehci, EHCIState),
+    }
+};
+
 static Property ehci_properties[] = {
-    DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
+    DEFINE_PROP_UINT32("maxframes", EHCIItfState, ehci.maxframes, 128),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2657,23 +2673,33 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     EHCIClass *k = (EHCIClass *)klass;
     EHCIClass *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;
     k->opregbase = template->opregbase;
     k->capabase = template->capabase;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
 
+static void ehci_pci_class_init(ObjectClass *klass, void *data)
+{
+    /* FIXME: how do you do object check for a dynamic class? */
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    EHCIClass *template = data;
+
+    k->init = usb_ehci_pci_initfn;
+    k->vendor_id = template->pci.vendor_id;
+    k->device_id = template->pci.device_id; /* ich4 */
+    k->revision = template->pci.revision;
+    k->class_id = PCI_CLASS_SERIAL_USB;
+
+    ehci_class_init(klass, data);
+}
+
 static TypeInfo ehci_info[] = {
     {
         .name          = "usb-ehci",
         .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIState),
-        .class_init    = ehci_class_init,
+        .instance_size = sizeof(EHCIItfState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIClass),
         .class_data    = (EHCIClass[]) {{{
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2686,8 +2712,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(EHCIItfState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIClass),
         .class_data    = (EHCIClass[]) {{{
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2700,42 +2726,12 @@ static TypeInfo ehci_info[] = {
     }, { .name = NULL }
 };
 
-static int usb_ehci_initfn(PCIDevice *dev)
+
+static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
 {
-    EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
     EHCIClass *c = (EHCIClass *)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->opregbase;
 
     /* 2.2 host controller interface version */
@@ -2752,11 +2748,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);
@@ -2784,8 +2776,48 @@ static int usb_ehci_initfn(PCIDevice *dev)
     memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
     memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+}
+
+static int usb_ehci_pci_initfn(PCIDevice *dev)
+{
+    EHCIItfState *i = DO_UPCAST(EHCIItfState, pcidev, 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));
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
     return 0;
 }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 05/11] usb/ehci: seperate out PCIisms

[Gerd Hoffmann]

  Hi,

> +typedef struct EHCItfState {
> +    union {
> +        PCIDevice pcidev;
> +    };
> +    struct EHCIState ehci;
> +} EHCIItfState;

I still think we should have EHCIPCIState here, then add a
EHCISysbusState variant for sysbus.  Everybody else does it this way
(ohci, esp, serial, ...) and I'd like ehci follow.  Also makes it easier
to split the source code into core, pci and sysbus pieces, i.e. move all
the pci bits to hcd-ehci-pci.c and compile only for targets with pci
support.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 05/11] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

On Fri, Oct 26, 2012 at 10:24 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> +typedef struct EHCItfState {
>> +    union {
>> +        PCIDevice pcidev;
>> +    };
>> +    struct EHCIState ehci;
>> +} EHCIItfState;
>
> I still think we should have EHCIPCIState here, then add a
> EHCISysbusState variant for sysbus.  Everybody else does it this way
> (ohci, esp, serial, ...) and I'd like ehci follow.

There still has to be a way to share the Property[] array (currently
contains maxframes). Duplicating the properties array to all
definitions is verbose and fragile. If I want to add a new properties
to EHCI i need to put it in the props array of every subclass. serial
has this problem, with the "chardev" prop appearing in both isa and
pci variants (and the device variant out of tree that Anthony has). If
we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
times.

Whats the real answer here? Can we get the shared init function to add
to properties explicify? Blow away the dc->properties = foo and
replace with code that parses to prop array?

I just think its a false economy to have to replicate your code for
the sake of the anit-union movement,

Regards,
Peter

Also makes it easier
> to split the source code into core, pci and sysbus pieces, i.e. move all
> the pci bits to hcd-ehci-pci.c and compile only for targets with pci
> support.
>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v2 05/11] usb/ehci: seperate out PCIisms

[Peter Crosthwaite]

On Sat, Oct 27, 2012 at 10:32 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Fri, Oct 26, 2012 at 10:24 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   Hi,
>>
>>> +typedef struct EHCItfState {
>>> +    union {
>>> +        PCIDevice pcidev;
>>> +    };
>>> +    struct EHCIState ehci;
>>> +} EHCIItfState;
>>
>> I still think we should have EHCIPCIState here, then add a
>> EHCISysbusState variant for sysbus.  Everybody else does it this way
>> (ohci, esp, serial, ...) and I'd like ehci follow.
>

In the interest of hopefully getting something merged before freeze I
had remade it this way (With EHCIPCIState). Can revisit this code
replication issue another day. v3 will be up shortly.

Regards,
Peter

> There still has to be a way to share the Property[] array (currently
> contains maxframes). Duplicating the properties array to all
> definitions is verbose and fragile. If I want to add a new properties
> to EHCI i need to put it in the props array of every subclass. serial
> has this problem, with the "chardev" prop appearing in both isa and
> pci variants (and the device variant out of tree that Anthony has). If
> we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
> times.
>
> Whats the real answer here? Can we get the shared init function to add
> to properties explicify? Blow away the dc->properties = foo and
> replace with code that parses to prop array?
>
> I just think its a false economy to have to replicate your code for
> the sake of the anit-union movement,
>
> Regards,
> Peter
>
> Also makes it easier
>> to split the source code into core, pci and sysbus pieces, i.e. move all
>> the pci bits to hcd-ehci-pci.c and compile only for targets with pci
>> support.
>>
>> cheers,
>>   Gerd
>>
>>

Re: [Qemu-devel] [PATCH v2 05/11] usb/ehci: seperate out PCIisms

[Gerd Hoffmann]

  Hi,

> There still has to be a way to share the Property[] array (currently
> contains maxframes). Duplicating the properties array to all
> definitions is verbose and fragile. If I want to add a new properties
> to EHCI i need to put it in the props array of every subclass. serial
> has this problem, with the "chardev" prop appearing in both isa and
> pci variants (and the device variant out of tree that Anthony has). If
> we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
> times.

> Whats the real answer here? Can we get the shared init function to add
> to properties explicify? Blow away the dc->properties = foo and
> replace with code that parses to prop array?

Existing practice is to use a #define for that, see
DEFINE_NIC_PROPERTIES in net.h for example.

Maybe QOM allows us to do something more elegant here.

cheers,
  Gerd

[Qemu-devel] [PATCH v2 06/11] usb/ehci: Add Sysbus Infrastructure

[Peter Crosthwaite]

Add QOM class definition helpers for sysbus attached EHCI implementations.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
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 |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 1c5e5ed..50a85d5 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
 
@@ -448,6 +450,7 @@ struct EHCIState {
 typedef struct EHCItfState {
     union {
         PCIDevice pcidev;
+        SysBusDevice busdev;
     };
     struct EHCIState ehci;
 } EHCIItfState;
@@ -2546,6 +2549,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,
@@ -2652,6 +2656,16 @@ 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, EHCIItfState, 2, vmstate_ehci, EHCIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static Property ehci_properties[] = {
     DEFINE_PROP_UINT32("maxframes", EHCIItfState, ehci.maxframes, 128),
     DEFINE_PROP_END_OF_LIST(),
@@ -2660,6 +2674,7 @@ static Property ehci_properties[] = {
 typedef struct EHCIClass {
     union {
         PCIDeviceClass pci;
+        SysBusDeviceClass sysbus;
     };
 
     uint16_t capabase;
@@ -2694,6 +2709,14 @@ static void ehci_pci_class_init(ObjectClass *klass, void *data)
     ehci_class_init(klass, data);
 }
 
+static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = usb_ehci_sysbus_initfn;
+    ehci_class_init(klass, data);
+}
+
 static TypeInfo ehci_info[] = {
     {
         .name          = "usb-ehci",
@@ -2726,7 +2749,6 @@ static TypeInfo ehci_info[] = {
     }, { .name = NULL }
 };
 
-
 static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
 {
     EHCIClass *c = (EHCIClass *)object_get_class(OBJECT(dev));
@@ -2778,6 +2800,20 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
                                 &s->mem_ports);
 }
 
+static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
+{
+    EHCIItfState *i = FROM_SYSBUS(EHCIItfState, dev);
+    EHCIState *s = &i->ehci;
+
+    s->dma = &dma_context_memory;
+
+    usb_ehci_initfn(s, DEVICE(dev));
+    sysbus_init_irq(dev, &s->irq);
+    sysbus_init_mmio(dev, &s->mem);
+
+    return 0;
+}
+
 static int usb_ehci_pci_initfn(PCIDevice *dev)
 {
     EHCIItfState *i = DO_UPCAST(EHCIItfState, pcidev, dev);
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 07/11] usb/ehci: Add Xilinx ps7 USB controller

[Peter Crosthwaite]

Definition of the USB controller implemented in Zynq.

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

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 50a85d5..443038b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2746,6 +2746,16 @@ static TypeInfo ehci_info[] = {
             0x00,
             0x20,
         } }
+    }, {
+        .name          = "xlnx,ps7-usb",
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(EHCISysBusState),
+        .class_init    = ehci_sysbus_class_init,
+        .class_size    = sizeof(EHCIClass),
+        .class_data    = (EHCIClass[]) {{
+            .capabase = 0x100,
+            .opregbase = 0x140,
+        } }
     }, { .name = NULL }
 };
 
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 08/11] 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/usb/hcd-ehci.c |    2 +-
 hw/xilinx_zynq.c  |    3 +++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 443038b..4d5e476 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2749,7 +2749,7 @@ static TypeInfo ehci_info[] = {
     }, {
         .name          = "xlnx,ps7-usb",
         .parent        = TYPE_SYS_BUS_DEVICE,
-        .instance_size = sizeof(EHCISysBusState),
+        .instance_size = sizeof(EHCIItfState),
         .class_init    = ehci_sysbus_class_init,
         .class_size    = sizeof(EHCIClass),
         .class_data    = (EHCIClass[]) {{
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

Re: [Qemu-devel] [PATCH v2 08/11] xilinx_zynq: add USB controllers

[Gerd Hoffmann]

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

Nice.  Much better than the create-usb helper function setting properties.

cheers,
  Gerd

[Qemu-devel] [PATCH v2 09/11] 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 4d5e476..7de3e54 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

[Qemu-devel] [PATCH v2 10/11] usb/ehci: Debug mode compile fixes

[Peter Crosthwaite]

A few debug messages in EHCI must have missed out on updates during incremental
developments. Fixed.

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

 hw/usb/hcd-ehci.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 7de3e54..26086b2 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1486,8 +1486,8 @@ static void ehci_execute_complete(EHCIQueue *q)
     assert(p->async == EHCI_ASYNC_INITIALIZED ||
            p->async == EHCI_ASYNC_FINISHED);
 
-    DPRINTF("execute_complete: qhaddr 0x%x, next %x, qtdaddr 0x%x, status %d\n",
-            q->qhaddr, q->qh.next, q->qtdaddr, q->usb_status);
+    DPRINTF("execute_complete: qhaddr 0x%x, next %x, qtdaddr 0x%x\n",
+            q->qhaddr, q->qh.next, q->qtdaddr);
 
     if (p->usb_status < 0) {
         switch (p->usb_status) {
@@ -1546,6 +1546,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
     USBEndpoint *ep;
     int ret;
     int endp;
+    EHCIQueue *q = p->queue;
 
     assert(p->async == EHCI_ASYNC_NONE ||
            p->async == EHCI_ASYNC_INITIALIZED);
@@ -1557,7 +1558,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
 
     p->tbytes = (p->qtd.token & QTD_TOKEN_TBYTES_MASK) >> QTD_TOKEN_TBYTES_SH;
     if (p->tbytes > BUFF_SIZE) {
-        ehci_trace_guest_bug(p->queue->ehci,
+        ehci_trace_guest_bug(q->ehci,
                              "guest requested more bytes than allowed");
         return USB_RET_PROCERR;
     }
@@ -1578,8 +1579,8 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         break;
     }
 
-    endp = get_field(p->queue->qh.epchar, QH_EPCHAR_EP);
-    ep = usb_ep_get(p->queue->dev, p->pid, endp);
+    endp = get_field(q->qh.epchar, QH_EPCHAR_EP);
+    ep = usb_ep_get(q->dev, p->pid, endp);
 
     if (p->async == EHCI_ASYNC_NONE) {
         if (ehci_init_transfer(p) != 0) {
@@ -1591,12 +1592,10 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         p->async = EHCI_ASYNC_INITIALIZED;
     }
 
-    trace_usb_ehci_packet_action(p->queue, p, action);
-    ret = usb_handle_packet(p->queue->dev, &p->packet);
-    DPRINTF("submit: qh %x next %x qtd %x pid %x len %zd "
-            "(total %d) endp %x ret %d\n",
-            q->qhaddr, q->qh.next, q->qtdaddr, q->pid,
-            q->packet.iov.size, q->tbytes, endp, ret);
+    trace_usb_ehci_packet_action(q, p, action);
+    ret = usb_handle_packet(q->dev, &p->packet);
+    DPRINTF("submit: qh %x next %x qtd %x pid %x (total %d) endp %x ret %d\n",
+            q->qhaddr, q->qh.next, q->qtdaddr, p->pid, p->tbytes, endp, ret);
 
     if (ret > BUFF_SIZE) {
         fprintf(stderr, "ret from usb_handle_packet > BUFF_SIZE\n");
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

Just put RAM regions in the unimplemented spaces in the MMIO region. These
regions have undefined behaviour, but this at least stops QEMU from segfaulting
when the guest bangs on these registers (and sucessfully fakes reading and
writing the registers with no side effects).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v1:
greatly simplified implementation.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 26086b2..77874ae 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2797,7 +2797,7 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
     qemu_register_reset(ehci_reset, s);
     qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
 
-    memory_region_init(&s->mem, "ehci", MMIO_SIZE);
+    memory_region_init_ram(&s->mem, "ehci", MMIO_SIZE);
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
                           "capabilities", CAPA_SIZE);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 26 October 2012 06:47, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Just put RAM regions in the unimplemented spaces in the MMIO region. These
> regions have undefined behaviour, but this at least stops QEMU from segfaulting
> when the guest bangs on these registers (and sucessfully fakes reading and
> writing the registers with no side effects).

I definitely don't like this. RAZ/WI might be OK, but implementing
random areas of devices as RAM smells like a hack. Better would be
to identify what the guest is actually doing in these areas.

And we still need to actually identify what is segfaulting and
fix that as a separate bug -- can we have a backtrace please?

-- PMM

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Gerd Hoffmann]

On 10/26/12 07:47, Peter Crosthwaite wrote:
> Just put RAM regions in the unimplemented spaces in the MMIO region. These
> regions have undefined behaviour, but this at least stops QEMU from segfaulting
> when the guest bangs on these registers (and sucessfully fakes reading and
> writing the registers with no side effects).

Make that an io region, have the read() handler return 0xff, write
handler do nothing except maybe logging/tracing the access for debugging
purposes.  That is more correct for unassigned mmio space than backing
by memory.  Adding memory also breaks migration btw.

I somehow still think this should be handled one layer up (i.e. the
parent region) which could do the approximate arch-specific action.

Any chance the access you are seeing is at offset 0x68?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Peter Maydell]

On 26 October 2012 13:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/26/12 07:47, Peter Crosthwaite wrote:
>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>> when the guest bangs on these registers (and sucessfully fakes reading and
>> writing the registers with no side effects).
>
> Make that an io region, have the read() handler return 0xff, write
> handler do nothing except maybe logging/tracing the access for debugging
> purposes.  That is more correct for unassigned mmio space than backing
> by memory.  Adding memory also breaks migration btw.
>
> I somehow still think this should be handled one layer up (i.e. the
> parent region) which could do the approximate arch-specific action.

If it's really in the memory space of the device itself then our device
model should be handling it.

-- PMM

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

On Fri, Oct 26, 2012 at 10:39 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 26 October 2012 13:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/26/12 07:47, Peter Crosthwaite wrote:
>>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>>> when the guest bangs on these registers (and sucessfully fakes reading and
>>> writing the registers with no side effects).
>>
>> Make that an io region, have the read() handler return 0xff, write
>> handler do nothing except maybe logging/tracing the access for debugging
>> purposes.  That is more correct for unassigned mmio space than backing
>> by memory.  Adding memory also breaks migration btw.
>>
>> I somehow still think this should be handled one layer up (i.e. the
>> parent region) which could do the approximate arch-specific action.
>
> If it's really in the memory space of the device itself then our device
> model should be handling it.
>

Yeh I admit patch is a hack and ultimately out of scope of this
series. Im going to drop it and put it on my workarounds branch for
the moment (wont appear in v3). Ill get you guys that segfault info,
and see if Gerd has any insights on what the device should actually
do.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Peter Crosthwaite]

On Fri, Oct 26, 2012 at 10:36 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/26/12 07:47, Peter Crosthwaite wrote:
>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>> when the guest bangs on these registers (and sucessfully fakes reading and
>> writing the registers with no side effects).
>
> Make that an io region, have the read() handler return 0xff, write
> handler do nothing except maybe logging/tracing the access for debugging
> purposes.  That is more correct for unassigned mmio space than backing
> by memory.  Adding memory also breaks migration btw.
>
> I somehow still think this should be handled one layer up (i.e. the
> parent region) which could do the approximate arch-specific action.
>
> Any chance the access you are seeing is at offset 0x68?
>

0x1a8. which for the opregbase + 0x068 for zynq so probably what you
are thinking about. I think the linux kernel is trying to explicitly
put the device in root mode rather than device mode, but those regs
are unimplemented in EHCI.

Regards,
Peter

> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v2 11/11] usb/ehci: Put RAM in undefined MMIO regions

[Gerd Hoffmann]

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

On 10/27/12 02:42, Peter Crosthwaite wrote:

>> Any chance the access you are seeing is at offset 0x68?
> 
> 0x1a8. which for the opregbase + 0x068 for zynq so probably what you
> are thinking about.

Does the attached patch help?

cheers,
  Gerd


[-- Attachment #2: 0001-ehci-set-extended-capability-pointer-on-pci-only.patch --]
[-- Type: text/plain, Size: 1121 bytes --]

>From 6a131b1476640c07317a6f44b5bb54ec53974414 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Mon, 29 Oct 2012 08:32:47 +0100
Subject: [PATCH] ehci: set extended capability pointer on pci only

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index a07beff..a35cbf2 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2792,7 +2792,6 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev, EHCIInfo *ei)
     s->caps[0x06] = 0x00;
     s->caps[0x07] = 0x00;
     s->caps[0x08] = 0x80;        /* We can cache whole frame, no 64-bit */
-    s->caps[0x09] = 0x68;        /* EECP */
     s->caps[0x0a] = 0x00;
     s->caps[0x0b] = 0x00;
 
@@ -2880,6 +2879,8 @@ static int usb_ehci_pci_initfn(PCIDevice *dev)
     s->irq = dev->irq[3];
     s->dma = pci_dma_context(dev);
 
+    s->caps[0x09] = 0x68;        /* EECP */
+
     usb_ehci_initfn(s, DEVICE(dev), &c->ehci);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
-- 
1.7.1