[Qemu-devel] RFC/PULL: usb: add support for companion controllers

[Qemu-devel] RFC/PULL: usb: add support for companion controllers

[Hans de Goede]

Hi Gerd et all,

These patches add support for adding an emulated uhci or ohci companion
controller to our emulated ehci controller. Usage examples:

-device usb-ehci,addr=0b.1,multifunction=on,id=ehci0
-device pci-ohci,addr=0b.0,multifunction=on,masterbus=ehci0.0,num-ports=4

-device usb-ehci,addr=0b.2,multifunction=on,id=ehci0
-device piix3-usb-uhci,addr=0b.0,multifunction=on,masterbus=ehci0.0,firstport=0
-device piix3-usb-uhci,addr=0b.1,multifunction=on,masterbus=ehci0.0,firstport=2

Please review and if there are no issues add these patches to your
usb tree.

These patches are also available in a git tree here:
http://cgit.freedesktop.org/~jwrdegoede/qemu/log/?h=usbredir
Note this tree also contains some other patches.

Thanks & Regards,

Hans

[Qemu-devel] [PATCH 01/11] usb: Add a register_companion USB bus op.

[Hans de Goede]

This is a preparation patch for adding support for USB companion controllers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-bus.c |   25 +++++++++++++++++++++++++
 hw/usb.h     |    4 ++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 2abce12..052fa66 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -63,6 +63,31 @@ USBBus *usb_bus_find(int busnr)
     return NULL;
 }
 
+int usb_bus_register_companion(const char *masterbus, USBPort *ports[],
+                               uint32_t portcount, uint32_t firstport)
+{
+    USBBus *bus;
+
+    QTAILQ_FOREACH(bus, &busses, next) {
+        if (strcmp(bus->qbus.name, masterbus) == 0) {
+            break;
+        }
+    }
+
+    if (!bus || !bus->ops->register_companion) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
+                      "an USB masterbus");
+        if (bus) {
+            error_printf_unless_qmp(
+                "USB bus '%s' does not allow companion controllers\n",
+                masterbus);
+        }
+        return -1;
+    }
+
+    return bus->ops->register_companion(bus, ports, portcount, firstport);
+}
+
 static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
diff --git a/hw/usb.h b/hw/usb.h
index b861b73..419ff0c 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -349,11 +349,15 @@ struct USBBus {
 };
 
 struct USBBusOps {
+    int (*register_companion)(USBBus *bus, USBPort *ports[],
+                              uint32_t portcount, uint32_t firstport);
     void (*device_destroy)(USBBus *bus, USBDevice *dev);
 };
 
 void usb_bus_new(USBBus *bus, USBBusOps *ops, DeviceState *host);
 USBBus *usb_bus_find(int busnr);
+int usb_bus_register_companion(const char *masterbus, USBPort *ports[],
+                               uint32_t portcount, uint32_t firstport);
 void usb_qdev_register(USBDeviceInfo *info);
 void usb_qdev_register_many(USBDeviceInfo *info);
 USBDevice *usb_create(USBBus *bus, const char *name);
-- 
1.7.5.1

[Qemu-devel] [PATCH 02/11] usb: Make port wakeup and complete ops take a USBPort instead of a Device

[Hans de Goede]

This makes them consistent with the attach and detach ops, and in general
it makes sense to make portops take a port as argument. This also makes
adding support for a companion controller easier / cleaner.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |    2 +-
 hw/usb-hub.c  |   10 +++++-----
 hw/usb-ohci.c |   12 +++++-------
 hw/usb-uhci.c |   11 +++++------
 hw/usb.c      |    4 ++--
 hw/usb.h      |    9 +++++++--
 6 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 91fb7de..051b040 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1111,7 +1111,7 @@ static int ehci_buffer_rw(EHCIQueue *q, int bytes, int rw)
     return 0;
 }
 
-static void ehci_async_complete_packet(USBDevice *dev, USBPacket *packet)
+static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
 {
     EHCIQueue *q = container_of(packet, EHCIQueue, packet);
 
diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 6e2a358..d324bba 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -246,10 +246,10 @@ static void usb_hub_detach(USBPort *port1)
     }
 }
 
-static void usb_hub_wakeup(USBDevice *dev)
+static void usb_hub_wakeup(USBPort *port1)
 {
-    USBHubState *s = dev->port->opaque;
-    USBHubPort *port = &s->ports[dev->port->index];
+    USBHubState *s = port1->opaque;
+    USBHubPort *port = &s->ports[port1->index];
 
     if (port->wPortStatus & PORT_STAT_SUSPEND) {
         port->wPortChange |= PORT_STAT_C_SUSPEND;
@@ -257,9 +257,9 @@ static void usb_hub_wakeup(USBDevice *dev)
     }
 }
 
-static void usb_hub_complete(USBDevice *dev, USBPacket *packet)
+static void usb_hub_complete(USBPort *port, USBPacket *packet)
 {
-    USBHubState *s = dev->port->opaque;
+    USBHubState *s = port->opaque;
 
     /*
      * Just pass it along upstream for now.
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 1c29b9f..c4703cf 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -367,15 +367,13 @@ static void ohci_detach(USBPort *port1)
         ohci_set_interrupt(s, OHCI_INTR_RHSC);
 }
 
-static void ohci_wakeup(USBDevice *dev)
+static void ohci_wakeup(USBPort *port1)
 {
-    USBBus *bus = usb_bus_from_device(dev);
-    OHCIState *s = container_of(bus, OHCIState, bus);
-    int portnum = dev->port->index;
-    OHCIPort *port = &s->rhport[portnum];
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
     uint32_t intr = 0;
     if (port->ctrl & OHCI_PORT_PSS) {
-        DPRINTF("usb-ohci: port %d: wakeup\n", portnum);
+        DPRINTF("usb-ohci: port %d: wakeup\n", port1->index);
         port->ctrl |= OHCI_PORT_PSSC;
         port->ctrl &= ~OHCI_PORT_PSS;
         intr = OHCI_INTR_RHSC;
@@ -602,7 +600,7 @@ static void ohci_copy_iso_td(OHCIState *ohci,
 
 static void ohci_process_lists(OHCIState *ohci, int completion);
 
-static void ohci_async_complete_packet(USBDevice *dev, USBPacket *packet)
+static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
 {
     OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
 #ifdef DEBUG_PACKET
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 405fa7b..7866726 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -620,11 +620,10 @@ static void uhci_detach(USBPort *port1)
     uhci_resume(s);
 }
 
-static void uhci_wakeup(USBDevice *dev)
+static void uhci_wakeup(USBPort *port1)
 {
-    USBBus *bus = usb_bus_from_device(dev);
-    UHCIState *s = container_of(bus, UHCIState, bus);
-    UHCIPort *port = s->ports + dev->port->index;
+    UHCIState *s = port1->opaque;
+    UHCIPort *port = &s->ports[port1->index];
 
     if (port->ctrl & UHCI_PORT_SUSPEND && !(port->ctrl & UHCI_PORT_RD)) {
         port->ctrl |= UHCI_PORT_RD;
@@ -657,7 +656,7 @@ static int uhci_broadcast_packet(UHCIState *s, USBPacket *p)
     return ret;
 }
 
-static void uhci_async_complete(USBDevice *dev, USBPacket *packet);
+static void uhci_async_complete(USBPort *port, USBPacket *packet);
 static void uhci_process_frame(UHCIState *s);
 
 /* return -1 if fatal error (frame must be stopped)
@@ -849,7 +848,7 @@ done:
     return len;
 }
 
-static void uhci_async_complete(USBDevice *dev, USBPacket *packet)
+static void uhci_async_complete(USBPort *port, USBPacket *packet)
 {
     UHCIAsync *async = container_of(packet, UHCIAsync, packet);
     UHCIState *s = async->uhci;
diff --git a/hw/usb.c b/hw/usb.c
index 4a39cbc..735ffd1 100644
--- a/hw/usb.c
+++ b/hw/usb.c
@@ -52,7 +52,7 @@ void usb_attach(USBPort *port, USBDevice *dev)
 void usb_wakeup(USBDevice *dev)
 {
     if (dev->remote_wakeup && dev->port && dev->port->ops->wakeup) {
-        dev->port->ops->wakeup(dev);
+        dev->port->ops->wakeup(dev->port);
     }
 }
 
@@ -335,7 +335,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     /* Note: p->owner != dev is possible in case dev is a hub */
     assert(p->owner != NULL);
-    dev->port->ops->complete(dev, p);
+    dev->port->ops->complete(dev->port, p);
     p->owner = NULL;
 }
 
diff --git a/hw/usb.h b/hw/usb.h
index 419ff0c..db53381 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -252,8 +252,13 @@ struct USBDeviceInfo {
 typedef struct USBPortOps {
     void (*attach)(USBPort *port);
     void (*detach)(USBPort *port);
-    void (*wakeup)(USBDevice *dev);
-    void (*complete)(USBDevice *dev, USBPacket *p);
+    void (*wakeup)(USBPort *port);
+    /*
+     * Note that port->dev will be different then the device from which
+     * the packet originated when a hub is involved, if you want the orginating
+     * device use p->owner
+     */
+    void (*complete)(USBPort *port, USBPacket *p);
 } USBPortOps;
 
 /* USB port on which a device can be connected */
-- 
1.7.5.1

[Qemu-devel] [PATCH 03/11] usb: Replace device_destroy bus op with a child_detach port op

[Hans de Goede]

Note this fixes 2 things in one go, first of all the device_destroy bus
op should be a device_detach bus op, as pending async packets from the
device should be cancelled on detach not on destroy.

Secondly having this as a bus op won't work with companion controllers, since
then there will be 1 bus driven by the ehci controller and thus 1 set of bus
ops, but the device being detached may be downstream of a handed over port.
Making the detach of a downstream device a port op allows the ehci controller
to forward this to the companion controller port for handed over ports.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/milkymist-softusb.c |    9 +++++++--
 hw/usb-bus.c           |    2 --
 hw/usb-ehci.c          |   18 ++++++++++--------
 hw/usb-hub.c           |   12 ++++++++++++
 hw/usb-musb.c          |   17 +++++++++++++----
 hw/usb-ohci.c          |   16 ++++++++++++----
 hw/usb-uhci.c          |   18 ++++++++++--------
 hw/usb.h               |    6 +++++-
 8 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
index 028f3b7..a59ef1b 100644
--- a/hw/milkymist-softusb.c
+++ b/hw/milkymist-softusb.c
@@ -247,16 +247,21 @@ static void softusb_attach(USBPort *port)
 {
 }
 
-static void softusb_device_destroy(USBBus *bus, USBDevice *dev)
+static void softusb_detach(USBPort *port)
+{
+}
+
+static void softusb_child_detach(USBPort *port, USBDevice *child)
 {
 }
 
 static USBPortOps softusb_ops = {
     .attach = softusb_attach,
+    .detach = softusb_detach,
+    .child_detach = softusb_child_detach,
 };
 
 static USBBusOps softusb_bus_ops = {
-    .device_destroy = softusb_device_destroy,
 };
 
 static void milkymist_softusb_reset(DeviceState *d)
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 052fa66..c55007c 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -107,12 +107,10 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
 static int usb_qdev_exit(DeviceState *qdev)
 {
     USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
-    USBBus *bus = usb_bus_from_device(dev);
 
     if (dev->attached) {
         usb_device_detach(dev);
     }
-    bus->ops->device_destroy(bus, dev);
     if (dev->info->handle_destroy) {
         dev->info->handle_destroy(dev);
     }
diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 051b040..f42648c 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -751,6 +751,8 @@ static void ehci_detach(USBPort *port)
 
     trace_usb_ehci_port_detach(port->index);
 
+    ehci_queues_rip_device(s, port->dev);
+
     *portsc &= ~PORTSC_CONNECT;
     *portsc |= PORTSC_CSC;
 
@@ -764,6 +766,13 @@ static void ehci_detach(USBPort *port)
     }
 }
 
+static void ehci_child_detach(USBPort *port, USBDevice *child)
+{
+    EHCIState *s = port->opaque;
+
+    ehci_queues_rip_device(s, child);
+}
+
 /* 4.1 host controller initialization */
 static void ehci_reset(void *opaque)
 {
@@ -2117,23 +2126,16 @@ static void ehci_map(PCIDevice *pci_dev, int region_num,
     cpu_register_physical_memory(addr, size, s->mem);
 }
 
-static void ehci_device_destroy(USBBus *bus, USBDevice *dev)
-{
-    EHCIState *s = container_of(bus, EHCIState, bus);
-
-    ehci_queues_rip_device(s, dev);
-}
-
 static int usb_ehci_initfn(PCIDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
     .detach = ehci_detach,
+    .child_detach = ehci_child_detach,
     .complete = ehci_async_complete_packet,
 };
 
 static USBBusOps ehci_bus_ops = {
-    .device_destroy = ehci_device_destroy,
 };
 
 static PCIDeviceInfo ehci_info = {
diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index d324bba..b7557ce 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -238,6 +238,9 @@ static void usb_hub_detach(USBPort *port1)
     USBHubState *s = port1->opaque;
     USBHubPort *port = &s->ports[port1->index];
 
+    /* Let upstream know the device on this port is gone */
+    s->dev.port->ops->child_detach(s->dev.port, port1->dev);
+
     port->wPortStatus &= ~PORT_STAT_CONNECTION;
     port->wPortChange |= PORT_STAT_C_CONNECTION;
     if (port->wPortStatus & PORT_STAT_ENABLE) {
@@ -246,6 +249,14 @@ static void usb_hub_detach(USBPort *port1)
     }
 }
 
+static void usb_hub_child_detach(USBPort *port1, USBDevice *child)
+{
+    USBHubState *s = port1->opaque;
+
+    /* Pass along upstream */
+    s->dev.port->ops->child_detach(s->dev.port, child);
+}
+
 static void usb_hub_wakeup(USBPort *port1)
 {
     USBHubState *s = port1->opaque;
@@ -537,6 +548,7 @@ static void usb_hub_handle_destroy(USBDevice *dev)
 static USBPortOps usb_hub_port_ops = {
     .attach = usb_hub_attach,
     .detach = usb_hub_detach,
+    .child_detach = usb_hub_child_detach,
     .wakeup = usb_hub_wakeup,
     .complete = usb_hub_complete,
 };
diff --git a/hw/usb-musb.c b/hw/usb-musb.c
index 21f35afa..21644c4 100644
--- a/hw/usb-musb.c
+++ b/hw/usb-musb.c
@@ -261,17 +261,18 @@
 
 static void musb_attach(USBPort *port);
 static void musb_detach(USBPort *port);
+static void musb_child_detach(USBPort *port, USBDevice *child);
 static void musb_schedule_cb(USBDevice *dev, USBPacket *p);
-static void musb_device_destroy(USBBus *bus, USBDevice *dev);
+static void musb_async_cancel_device(MUSBState *s, USBDevice *dev);
 
 static USBPortOps musb_port_ops = {
     .attach = musb_attach,
     .detach = musb_detach,
+    .child_detach = musb_child_detach,
     .complete = musb_schedule_cb,
 };
 
 static USBBusOps musb_bus_ops = {
-    .device_destroy = musb_device_destroy,
 };
 
 typedef struct MUSBPacket MUSBPacket;
@@ -498,10 +499,19 @@ static void musb_detach(USBPort *port)
 {
     MUSBState *s = (MUSBState *) port->opaque;
 
+    musb_async_cancel_device(s, port->dev);
+
     musb_intr_set(s, musb_irq_disconnect, 1);
     musb_session_update(s, 1, s->session);
 }
 
+static void musb_child_detach(USBPort *port, USBDevice *child)
+{
+    MUSBState *s = (MUSBState *) port->opaque;
+
+    musb_async_cancel_device(s, child);
+}
+
 static void musb_cb_tick0(void *opaque)
 {
     MUSBEndPoint *ep = (MUSBEndPoint *) opaque;
@@ -783,9 +793,8 @@ static void musb_rx_packet_complete(USBPacket *packey, void *opaque)
     musb_rx_intr_set(s, epnum, 1);
 }
 
-static void musb_device_destroy(USBBus *bus, USBDevice *dev)
+static void musb_async_cancel_device(MUSBState *s, USBDevice *dev)
 {
-    MUSBState *s = container_of(bus, MUSBState, bus);
     int ep, dir;
 
     for (ep = 0; ep < 16; ep++) {
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index c4703cf..a113c0f 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -124,6 +124,7 @@ struct ohci_hcca {
 };
 
 static void ohci_bus_stop(OHCIState *ohci);
+static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev);
 
 /* Bitfields for the first word of an Endpoint Desciptor.  */
 #define OHCI_ED_FA_SHIFT  0
@@ -351,6 +352,8 @@ static void ohci_detach(USBPort *port1)
     OHCIPort *port = &s->rhport[port1->index];
     uint32_t old_state = port->ctrl;
 
+    ohci_async_cancel_device(s, port1->dev);
+
     /* set connect status */
     if (port->ctrl & OHCI_PORT_CCS) {
         port->ctrl &= ~OHCI_PORT_CCS;
@@ -392,6 +395,13 @@ static void ohci_wakeup(USBPort *port1)
     ohci_set_interrupt(s, intr);
 }
 
+static void ohci_child_detach(USBPort *port1, USBDevice *child)
+{
+    OHCIState *s = port1->opaque;
+
+    ohci_async_cancel_device(s, child);
+}
+
 /* Reset the controller */
 static void ohci_reset(void *opaque)
 {
@@ -1673,10 +1683,8 @@ static void ohci_mem_write(void *ptr, target_phys_addr_t addr, uint32_t val)
     }
 }
 
-static void ohci_device_destroy(USBBus *bus, USBDevice *dev)
+static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
 {
-    OHCIState *ohci = container_of(bus, OHCIState, bus);
-
     if (ohci->async_td && ohci->usb_packet.owner == dev) {
         usb_cancel_packet(&ohci->usb_packet);
         ohci->async_td = 0;
@@ -1700,12 +1708,12 @@ static CPUWriteMemoryFunc * const ohci_writefn[3]={
 static USBPortOps ohci_port_ops = {
     .attach = ohci_attach,
     .detach = ohci_detach,
+    .child_detach = ohci_child_detach,
     .wakeup = ohci_wakeup,
     .complete = ohci_async_complete_packet,
 };
 
 static USBBusOps ohci_bus_ops = {
-    .device_destroy = ohci_device_destroy,
 };
 
 static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 7866726..b081b45 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -606,6 +606,8 @@ static void uhci_detach(USBPort *port1)
     UHCIState *s = port1->opaque;
     UHCIPort *port = &s->ports[port1->index];
 
+    uhci_async_cancel_device(s, port1->dev);
+
     /* set connect status */
     if (port->ctrl & UHCI_PORT_CCS) {
         port->ctrl &= ~UHCI_PORT_CCS;
@@ -620,6 +622,13 @@ static void uhci_detach(USBPort *port1)
     uhci_resume(s);
 }
 
+static void uhci_child_detach(USBPort *port1, USBDevice *child)
+{
+    UHCIState *s = port1->opaque;
+
+    uhci_async_cancel_device(s, child);
+}
+
 static void uhci_wakeup(USBPort *port1)
 {
     UHCIState *s = port1->opaque;
@@ -1095,22 +1104,15 @@ static void uhci_map(PCIDevice *pci_dev, int region_num,
     register_ioport_read(addr, 32, 1, uhci_ioport_readb, s);
 }
 
-static void uhci_device_destroy(USBBus *bus, USBDevice *dev)
-{
-    UHCIState *s = container_of(bus, UHCIState, bus);
-
-    uhci_async_cancel_device(s, dev);
-}
-
 static USBPortOps uhci_port_ops = {
     .attach = uhci_attach,
     .detach = uhci_detach,
+    .child_detach = uhci_child_detach,
     .wakeup = uhci_wakeup,
     .complete = uhci_async_complete,
 };
 
 static USBBusOps uhci_bus_ops = {
-    .device_destroy = uhci_device_destroy,
 };
 
 static int usb_uhci_common_initfn(PCIDevice *dev)
diff --git a/hw/usb.h b/hw/usb.h
index db53381..1b82d61 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -252,6 +252,11 @@ struct USBDeviceInfo {
 typedef struct USBPortOps {
     void (*attach)(USBPort *port);
     void (*detach)(USBPort *port);
+    /* 
+     * This gets called when a device downstream from the device attached to
+     * the port (iow attached through a hub) gets detached.
+     */
+    void (*child_detach)(USBPort *port, USBDevice *child);
     void (*wakeup)(USBPort *port);
     /*
      * Note that port->dev will be different then the device from which
@@ -356,7 +361,6 @@ struct USBBus {
 struct USBBusOps {
     int (*register_companion)(USBBus *bus, USBPort *ports[],
                               uint32_t portcount, uint32_t firstport);
-    void (*device_destroy)(USBBus *bus, USBDevice *dev);
 };
 
 void usb_bus_new(USBBus *bus, USBBusOps *ops, DeviceState *host);
-- 
1.7.5.1

[Qemu-devel] [PATCH 04/11] usb-ehci: drop unused num-ports state member

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index f42648c..158e147 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -373,7 +373,6 @@ struct EHCIState {
     qemu_irq irq;
     target_phys_addr_t mem_base;
     int mem;
-    int num_ports;
 
     /* properties */
     uint32_t freq;
-- 
1.7.5.1

[Qemu-devel] [PATCH 05/11] usb-ehci: Connect Status bit is read only, don't allow changing it by the guest

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 158e147..0b1cdaf 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -103,10 +103,10 @@
 #define PORTSC_BEGIN         PORTSC
 #define PORTSC_END           (PORTSC + 4 * NB_PORTS)
 /*
- * Bits that are reserverd or are read-only are masked out of values
+ * Bits that are reserved or are read-only are masked out of values
  * written to us by software
  */
-#define PORTSC_RO_MASK       0x007021c5
+#define PORTSC_RO_MASK       0x007021c4
 #define PORTSC_RWC_MASK      0x0000002a
 #define PORTSC_WKOC_E        (1 << 22)    // Wake on Over Current Enable
 #define PORTSC_WKDS_E        (1 << 21)    // Wake on Disconnect Enable
-- 
1.7.5.1

[Qemu-devel] [PATCH 06/11] usb-ehci: cleanup port reset handling

[Hans de Goede]

Doing a usb_attach when dev is NULL will just result in the
port detach op getting called even though nothing was connected in
the first place.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 0b1cdaf..cfda374 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -863,14 +863,9 @@ static void handle_port_status_write(EHCIState *s, int port, uint32_t val)
 
     if (!(val & PORTSC_PRESET) &&(*portsc & PORTSC_PRESET)) {
         trace_usb_ehci_port_reset(port, 0);
-        usb_attach(&s->ports[port], dev);
-
-        // TODO how to handle reset of ports with no device
         if (dev) {
+            usb_attach(&s->ports[port], dev);
             usb_send_msg(dev, USB_MSG_RESET);
-        }
-
-        if (s->ports[port].dev) {
             *portsc &= ~PORTSC_CSC;
         }
 
-- 
1.7.5.1

[Qemu-devel] [PATCH 07/11] usb: assert on calling usb_attach(port, NULL) on a port without a dev

[Hans de Goede]

with the "usb-ehci: cleanup port reset handling" patch in place no callers
are calling usb_attach(port, NULL) for a port where port->dev is NULL.

Doing that makes no sense as that causes the port detach op to get called
for a port with nothing attached. Add an assert that port->dev != NULL when
dev == NULL, and remove the check for not having a port->dev in the dev == NULL
case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/usb.c b/hw/usb.c
index 735ffd1..27a983c 100644
--- a/hw/usb.c
+++ b/hw/usb.c
@@ -40,12 +40,11 @@ void usb_attach(USBPort *port, USBDevice *dev)
     } else {
         /* detach */
         dev = port->dev;
+        assert(dev);
         port->ops->detach(port);
-        if (dev) {
-            usb_send_msg(dev, USB_MSG_DETACH);
-            dev->port = NULL;
-            port->dev = NULL;
-        }
+        usb_send_msg(dev, USB_MSG_DETACH);
+        dev->port = NULL;
+        port->dev = NULL;
     }
 }
 
-- 
1.7.5.1

[Qemu-devel] [PATCH 08/11] usb-ehci: Fix handling of PED and PEDC port status bits

[Hans de Goede]

The PED bit should only be set for highspeed devices and the PEDC bit
should not be set on "normal" PED bit changes, only on io errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index cfda374..0e8b3d1 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -106,7 +106,7 @@
  * Bits that are reserved or are read-only are masked out of values
  * written to us by software
  */
-#define PORTSC_RO_MASK       0x007021c4
+#define PORTSC_RO_MASK       0x007021c0
 #define PORTSC_RWC_MASK      0x0000002a
 #define PORTSC_WKOC_E        (1 << 22)    // Wake on Over Current Enable
 #define PORTSC_WKDS_E        (1 << 21)    // Wake on Disconnect Enable
@@ -752,7 +752,7 @@ static void ehci_detach(USBPort *port)
 
     ehci_queues_rip_device(s, port->dev);
 
-    *portsc &= ~PORTSC_CONNECT;
+    *portsc &= ~(PORTSC_CONNECT|PORTSC_PED);
     *portsc |= PORTSC_CSC;
 
     /*
@@ -847,16 +847,14 @@ static void ehci_mem_writew(void *ptr, target_phys_addr_t addr, uint32_t val)
 static void handle_port_status_write(EHCIState *s, int port, uint32_t val)
 {
     uint32_t *portsc = &s->portsc[port];
-    int rwc;
     USBDevice *dev = s->ports[port].dev;
 
-    rwc = val & PORTSC_RWC_MASK;
+    /* Clear rwc bits */
+    *portsc &= ~(val & PORTSC_RWC_MASK);
+    /* The guest may clear, but not set the PED bit */
+    *portsc &= val | ~PORTSC_PED;
     val &= PORTSC_RO_MASK;
 
-    // handle_read_write_clear(&val, portsc, PORTSC_PEDC | PORTSC_CSC);
-
-    *portsc &= ~rwc;
-
     if ((val & PORTSC_PRESET) && !(*portsc & PORTSC_PRESET)) {
         trace_usb_ehci_port_reset(port, 1);
     }
@@ -869,13 +867,13 @@ static void handle_port_status_write(EHCIState *s, int port, uint32_t val)
             *portsc &= ~PORTSC_CSC;
         }
 
-        /*  Table 2.16 Set the enable bit(and enable bit change) to indicate
+        /*
+         *  Table 2.16 Set the enable bit(and enable bit change) to indicate
          *  to SW that this port has a high speed device attached
-         *
-         *  TODO - when to disable?
          */
-        val |= PORTSC_PED;
-        val |= PORTSC_PEDC;
+        if (dev && (dev->speedmask & USB_SPEED_MASK_HIGH)) {
+            val |= PORTSC_PED;
+        }
     }
 
     *portsc &= ~PORTSC_RO_MASK;
-- 
1.7.5.1

[Qemu-devel] [PATCH 09/11] usb-ehci: Add support for registering companion controllers

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |  174 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 144 insertions(+), 30 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 0e8b3d1..ae39940 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -20,9 +20,6 @@
  *
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
- *
- * TODO:
- *  o Downstream port handoff
  */
 
 #include "hw.h"
@@ -106,7 +103,7 @@
  * Bits that are reserved or are read-only are masked out of values
  * written to us by software
  */
-#define PORTSC_RO_MASK       0x007021c0
+#define PORTSC_RO_MASK       0x007001c0
 #define PORTSC_RWC_MASK      0x0000002a
 #define PORTSC_WKOC_E        (1 << 22)    // Wake on Over Current Enable
 #define PORTSC_WKDS_E        (1 << 21)    // Wake on Disconnect Enable
@@ -373,6 +370,7 @@ struct EHCIState {
     qemu_irq irq;
     target_phys_addr_t mem_base;
     int mem;
+    int companion_count;
 
     /* properties */
     uint32_t freq;
@@ -408,6 +406,7 @@ struct EHCIState {
     int astate;                        // Current state in asynchronous schedule
     int pstate;                        // Current state in periodic schedule
     USBPort ports[NB_PORTS];
+    USBPort *companion_ports[NB_PORTS];
     uint32_t usbsts_pending;
     QTAILQ_HEAD(, EHCIQueue) queues;
 
@@ -730,17 +729,17 @@ static void ehci_attach(USBPort *port)
 
     trace_usb_ehci_port_attach(port->index, port->dev->product_desc);
 
+    if (*portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        companion->dev = port->dev;
+        companion->ops->attach(companion);
+        return;
+    }
+
     *portsc |= PORTSC_CONNECT;
     *portsc |= PORTSC_CSC;
 
-    /*
-     *  If a high speed device is attached then we own this port(indicated
-     *  by zero in the PORTSC_POWNER bit field) so set the status bit
-     *  and set an interrupt if enabled.
-     */
-    if ( !(*portsc & PORTSC_POWNER)) {
-        ehci_set_interrupt(s, USBSTS_PCD);
-    }
+    ehci_set_interrupt(s, USBSTS_PCD);
 }
 
 static void ehci_detach(USBPort *port)
@@ -750,36 +749,110 @@ static void ehci_detach(USBPort *port)
 
     trace_usb_ehci_port_detach(port->index);
 
+    if (*portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        companion->ops->detach(companion);
+        companion->dev = NULL;
+        return;
+    }
+
     ehci_queues_rip_device(s, port->dev);
 
     *portsc &= ~(PORTSC_CONNECT|PORTSC_PED);
     *portsc |= PORTSC_CSC;
 
-    /*
-     *  If a high speed device is attached then we own this port(indicated
-     *  by zero in the PORTSC_POWNER bit field) so set the status bit
-     *  and set an interrupt if enabled.
-     */
-    if ( !(*portsc & PORTSC_POWNER)) {
-        ehci_set_interrupt(s, USBSTS_PCD);
-    }
+    ehci_set_interrupt(s, USBSTS_PCD);
 }
 
 static void ehci_child_detach(USBPort *port, USBDevice *child)
 {
     EHCIState *s = port->opaque;
+    uint32_t portsc = s->portsc[port->index];
+
+    if (portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        companion->ops->child_detach(companion, child);
+        companion->dev = NULL;
+        return;
+    }
 
     ehci_queues_rip_device(s, child);
 }
 
+static void ehci_wakeup(USBPort *port)
+{
+    EHCIState *s = port->opaque;
+    uint32_t portsc = s->portsc[port->index];
+
+    if (portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        if (companion->ops->wakeup) {
+            companion->ops->wakeup(companion);
+        }
+    }
+}
+
+static int ehci_register_companion(USBBus *bus, USBPort *ports[],
+                                   uint32_t portcount, uint32_t firstport)
+{
+    EHCIState *s = container_of(bus, EHCIState, bus);
+    uint32_t i;
+
+    if (firstport + portcount > NB_PORTS) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
+                      "firstport on masterbus");
+        error_printf_unless_qmp(
+            "firstport value of %u makes companion take ports %u - %u, which "
+            "is outside of the valid range of 0 - %u\n", firstport, firstport,
+            firstport + portcount - 1, NB_PORTS - 1);
+        return -1;
+    }
+
+    for (i = 0; i < portcount; i++) {
+        if (s->companion_ports[firstport + i]) {
+            qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
+                          "an USB masterbus");
+            error_printf_unless_qmp(
+                "port %u on masterbus %s already has a companion assigned\n",
+                firstport + i, bus->qbus.name);
+            return -1;
+        }
+    }
+
+    for (i = 0; i < portcount; i++) {
+        s->companion_ports[firstport + i] = ports[i];
+        s->ports[firstport + i].speedmask |=
+            USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
+        /* Ensure devs attached before the initial reset go to the companion */
+        s->portsc[firstport + i] = PORTSC_POWNER;
+    }
+
+    s->companion_count++;
+    s->mmio[0x05] = (s->companion_count << 4) | portcount;
+
+    return 0;
+}
+
 /* 4.1 host controller initialization */
 static void ehci_reset(void *opaque)
 {
     EHCIState *s = opaque;
     int i;
+    USBDevice *devs[NB_PORTS];
 
     trace_usb_ehci_reset();
 
+    /*
+     * Do the detach before touching portsc, so that it correctly gets send to
+     * us or to our companion based on PORTSC_POWNER before the reset.
+     */
+    for(i = 0; i < NB_PORTS; i++) {
+        devs[i] = s->ports[i].dev;
+        if (devs[i]) {
+            usb_attach(&s->ports[i], NULL);
+        }
+    }
+
     memset(&s->mmio[OPREGBASE], 0x00, MMIO_SIZE - OPREGBASE);
 
     s->usbcmd = NB_MAXINTRATE << USBCMD_ITC_SH;
@@ -791,10 +864,13 @@ static void ehci_reset(void *opaque)
     s->attach_poll_counter = 0;
 
     for(i = 0; i < NB_PORTS; i++) {
-        s->portsc[i] = PORTSC_POWNER | PORTSC_PPOWER;
-
-        if (s->ports[i].dev) {
-            usb_attach(&s->ports[i], s->ports[i].dev);
+        if (s->companion_ports[i]) {
+            s->portsc[i] = PORTSC_POWNER | PORTSC_PPOWER;
+        } else {
+            s->portsc[i] = PORTSC_PPOWER;
+        }
+        if (devs[i]) {
+            usb_attach(&s->ports[i], devs[i]);
         }
     }
     ehci_queues_rip_all(s);
@@ -844,6 +920,34 @@ static void ehci_mem_writew(void *ptr, target_phys_addr_t addr, uint32_t val)
     exit(1);
 }
 
+static void handle_port_owner_write(EHCIState *s, int port, uint32_t owner)
+{
+    USBDevice *dev = s->ports[port].dev;
+    uint32_t *portsc = &s->portsc[port];
+    uint32_t orig;
+
+    if (s->companion_ports[port] == NULL)
+        return;
+
+    owner = owner & PORTSC_POWNER;
+    orig  = *portsc & PORTSC_POWNER;
+
+    if (!(owner ^ orig)) {
+        return;
+    }
+
+    if (dev) {
+        usb_attach(&s->ports[port], NULL);
+    }
+
+    *portsc &= ~PORTSC_POWNER;
+    *portsc |= owner;
+
+    if (dev) {
+        usb_attach(&s->ports[port], dev);
+    }
+}
+
 static void handle_port_status_write(EHCIState *s, int port, uint32_t val)
 {
     uint32_t *portsc = &s->portsc[port];
@@ -853,6 +957,9 @@ static void handle_port_status_write(EHCIState *s, int port, uint32_t val)
     *portsc &= ~(val & PORTSC_RWC_MASK);
     /* The guest may clear, but not set the PED bit */
     *portsc &= val | ~PORTSC_PED;
+    /* POWNER is masked out by RO_MASK as it is RO when we've no companion */
+    handle_port_owner_write(s, port, val);
+    /* And finally apply RO_MASK */
     val &= PORTSC_RO_MASK;
 
     if ((val & PORTSC_PRESET) && !(*portsc & PORTSC_PRESET)) {
@@ -956,7 +1063,7 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
         val &= 0x1;
         if (val) {
             for(i = 0; i < NB_PORTS; i++)
-                s->portsc[i] &= ~PORTSC_POWNER;
+                handle_port_owner_write(s, i, 0);
         }
         break;
 
@@ -1114,8 +1221,17 @@ static int ehci_buffer_rw(EHCIQueue *q, int bytes, int rw)
 
 static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
 {
-    EHCIQueue *q = container_of(packet, EHCIQueue, packet);
+    EHCIQueue *q;
+    EHCIState *s = port->opaque;
+    uint32_t portsc = s->portsc[port->index];
+
+    if (portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        companion->ops->complete(companion, packet);
+        return;
+    }
 
+    q = container_of(packet, EHCIQueue, packet);
     trace_usb_ehci_queue_action(q, "wakeup");
     assert(q->async == EHCI_ASYNC_INFLIGHT);
     q->async = EHCI_ASYNC_FINISHED;
@@ -1245,8 +1361,6 @@ static int ehci_execute(EHCIQueue *q)
         port = &q->ehci->ports[i];
         dev = port->dev;
 
-        // TODO sometime we will also need to check if we are the port owner
-
         if (!(q->ehci->portsc[i] &(PORTSC_CONNECT))) {
             DPRINTF("Port %d, no exec, not connected(%08X)\n",
                     i, q->ehci->portsc[i]);
@@ -1339,8 +1453,6 @@ static int ehci_process_itd(EHCIState *ehci,
                 port = &ehci->ports[j];
                 dev = port->dev;
 
-                // TODO sometime we will also need to check if we are the port owner
-
                 if (!(ehci->portsc[j] &(PORTSC_CONNECT))) {
                     continue;
                 }
@@ -2124,10 +2236,12 @@ static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
     .detach = ehci_detach,
     .child_detach = ehci_child_detach,
+    .wakeup = ehci_wakeup,
     .complete = ehci_async_complete_packet,
 };
 
 static USBBusOps ehci_bus_ops = {
+    .register_companion = ehci_register_companion,
 };
 
 static PCIDeviceInfo ehci_info = {
-- 
1.7.5.1

[Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Hans de Goede]

To use as a companion controller set the masterbus property.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-uhci.c |   48 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index b081b45..a7ab4a1 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -132,7 +132,7 @@ typedef struct UHCIPort {
 
 struct UHCIState {
     PCIDevice dev;
-    USBBus bus;
+    USBBus bus; /* Note unused when we're a companion controller */
     uint16_t cmd; /* cmd register */
     uint16_t status;
     uint16_t intr; /* interrupt enable register */
@@ -150,6 +150,10 @@ struct UHCIState {
     /* Active packets */
     QTAILQ_HEAD(,UHCIAsync) async_pending;
     uint8_t num_ports_vmstate;
+    
+    /* Properties */
+    char *masterbus;
+    uint32_t firstport;
 };
 
 typedef struct UHCI_TD {
@@ -1126,11 +1130,28 @@ static int usb_uhci_common_initfn(PCIDevice *dev)
     pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3
     pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
 
-    usb_bus_new(&s->bus, &uhci_bus_ops, &s->dev.qdev);
-    for(i = 0; i < NB_PORTS; i++) {
-        usb_register_port(&s->bus, &s->ports[i].port, s, i, &uhci_port_ops,
-                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
-        usb_port_location(&s->ports[i].port, NULL, i+1);
+    if (s->masterbus) {
+        USBPort *ports[NB_PORTS];
+        for(i = 0; i < NB_PORTS; i++) {
+            s->ports[i].port.ops = &uhci_port_ops;
+            s->ports[i].port.opaque = s;
+            s->ports[i].port.index = i;
+            s->ports[i].port.speedmask =
+                USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
+            usb_port_location(&s->ports[i].port, NULL, i+1);
+            ports[i] = &s->ports[i].port;
+        }
+        if (usb_bus_register_companion(s->masterbus, ports, NB_PORTS,
+                                       s->firstport) != 0) {
+            return -1;
+        }
+    } else {
+        usb_bus_new(&s->bus, &uhci_bus_ops, &s->dev.qdev);
+        for(i = 0; i < NB_PORTS; i++) {
+            usb_register_port(&s->bus, &s->ports[i].port, s, i, &uhci_port_ops,
+                              USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
+            usb_port_location(&s->ports[i].port, NULL, i+1);
+        }
     }
     s->frame_timer = qemu_new_timer_ns(vm_clock, uhci_frame_timer, s);
     s->num_ports_vmstate = NB_PORTS;
@@ -1171,6 +1192,11 @@ static PCIDeviceInfo uhci_info[] = {
         .device_id    = PCI_DEVICE_ID_INTEL_82371SB_2,
         .revision     = 0x01,
         .class_id     = PCI_CLASS_SERIAL_USB,
+        .qdev.props   = (Property[]) {
+            DEFINE_PROP_STRING("masterbus", UHCIState, masterbus),
+            DEFINE_PROP_UINT32("firstport", UHCIState, firstport, 0),
+            DEFINE_PROP_END_OF_LIST(),
+        },
     },{
         .qdev.name    = "piix4-usb-uhci",
         .qdev.size    = sizeof(UHCIState),
@@ -1180,6 +1206,11 @@ static PCIDeviceInfo uhci_info[] = {
         .device_id    = PCI_DEVICE_ID_INTEL_82371AB_2,
         .revision     = 0x01,
         .class_id     = PCI_CLASS_SERIAL_USB,
+        .qdev.props   = (Property[]) {
+            DEFINE_PROP_STRING("masterbus", UHCIState, masterbus),
+            DEFINE_PROP_UINT32("firstport", UHCIState, firstport, 0),
+            DEFINE_PROP_END_OF_LIST(),
+        },
     },{
         .qdev.name    = "vt82c686b-usb-uhci",
         .qdev.size    = sizeof(UHCIState),
@@ -1189,6 +1220,11 @@ static PCIDeviceInfo uhci_info[] = {
         .device_id    = PCI_DEVICE_ID_VIA_UHCI,
         .revision     = 0x01,
         .class_id     = PCI_CLASS_SERIAL_USB,
+        .qdev.props   = (Property[]) {
+            DEFINE_PROP_STRING("masterbus", UHCIState, masterbus),
+            DEFINE_PROP_UINT32("firstport", UHCIState, firstport, 0),
+            DEFINE_PROP_END_OF_LIST(),
+        },
     },{
         /* end of list */
     }
-- 
1.7.5.1

[Qemu-devel] [PATCH 11/11] usb-ohci: Add support for being a companion controller

[Hans de Goede]

To use as a companion controller, use pci-ohci as device and set the
masterbus and num-ports properties, ie:

-device usb-ehci,addr=0b.1,multifunction=on,id=ehci0
-device pci-ohci,addr=0b.0,multifunction=on,masterbus=ehci0.0,num-ports=4

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ohci.c |   59 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index a113c0f..4d6ef10 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1716,8 +1716,9 @@ static USBPortOps ohci_port_ops = {
 static USBBusOps ohci_bus_ops = {
 };
 
-static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
-                          int num_ports, uint32_t localmem_base)
+static int usb_ohci_init(OHCIState *ohci, DeviceState *dev,
+                         int num_ports, uint32_t localmem_base,
+                         char *masterbus, uint32_t firstport)
 {
     int i;
 
@@ -1737,39 +1738,64 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
                 usb_frame_time, usb_bit_time);
     }
 
+    ohci->num_ports = num_ports;
+    if (masterbus) {
+        USBPort *ports[OHCI_MAX_PORTS];
+        for(i = 0; i < num_ports; i++) {
+            ohci->rhport[i].port.ops = &ohci_port_ops;
+            ohci->rhport[i].port.opaque = ohci;
+            ohci->rhport[i].port.index = i;
+            ohci->rhport[i].port.speedmask =
+                USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
+            usb_port_location(&ohci->rhport[i].port, NULL, i+1);
+            ports[i] = &ohci->rhport[i].port;
+        }
+        if (usb_bus_register_companion(masterbus, ports, num_ports,
+                                       firstport) != 0) {
+            return -1;
+        }
+    } else {
+        usb_bus_new(&ohci->bus, &ohci_bus_ops, dev);
+        for (i = 0; i < num_ports; i++) {
+            usb_register_port(&ohci->bus, &ohci->rhport[i].port,
+                              ohci, i, &ohci_port_ops,
+                              USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
+            usb_port_location(&ohci->rhport[i].port, NULL, i+1);
+        }
+    }
+
     ohci->mem = cpu_register_io_memory(ohci_readfn, ohci_writefn, ohci,
                                        DEVICE_LITTLE_ENDIAN);
     ohci->localmem_base = localmem_base;
 
     ohci->name = dev->info->name;
 
-    usb_bus_new(&ohci->bus, &ohci_bus_ops, dev);
-    ohci->num_ports = num_ports;
-    for (i = 0; i < num_ports; i++) {
-        usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, &ohci_port_ops,
-                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
-        usb_port_location(&ohci->rhport[i].port, NULL, i+1);
-    }
-
     ohci->async_td = 0;
     qemu_register_reset(ohci_reset, ohci);
+
+    return 0;
 }
 
 typedef struct {
     PCIDevice pci_dev;
     OHCIState state;
+    char *masterbus;
+    uint32_t num_ports;
+    uint32_t firstport;
 } OHCIPCIState;
 
 static int usb_ohci_initfn_pci(struct PCIDevice *dev)
 {
     OHCIPCIState *ohci = DO_UPCAST(OHCIPCIState, pci_dev, dev);
-    int num_ports = 3;
 
     ohci->pci_dev.config[PCI_CLASS_PROG] = 0x10; /* OHCI */
     /* TODO: RST# value should be 0. */
     ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
 
-    usb_ohci_init(&ohci->state, &dev->qdev, num_ports, 0);
+    if (usb_ohci_init(&ohci->state, &dev->qdev, ohci->num_ports, 0,
+                      ohci->masterbus, ohci->firstport) != 0) {
+        return -1;
+    }
     ohci->state.irq = ohci->pci_dev.irq[0];
 
     /* TODO: avoid cast below by using dev */
@@ -1793,7 +1819,8 @@ static int ohci_init_pxa(SysBusDevice *dev)
 {
     OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
 
-    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset);
+    /* Cannot fail as we pass NULL for masterbus */
+    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0);
     sysbus_init_irq(dev, &s->ohci.irq);
     sysbus_init_mmio(dev, 0x1000, s->ohci.mem);
 
@@ -1808,6 +1835,12 @@ static PCIDeviceInfo ohci_pci_info = {
     .vendor_id    = PCI_VENDOR_ID_APPLE,
     .device_id    = PCI_DEVICE_ID_APPLE_IPID_USB,
     .class_id     = PCI_CLASS_SERIAL_USB,
+    .qdev.props   = (Property[]) {
+        DEFINE_PROP_STRING("masterbus", OHCIPCIState, masterbus),
+        DEFINE_PROP_UINT32("num-ports", OHCIPCIState, num_ports, 3),
+        DEFINE_PROP_UINT32("firstport", OHCIPCIState, firstport, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    },
 };
 
 static SysBusDeviceInfo ohci_sysbus_info = {
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 08/11] usb-ehci: Fix handling of PED and PEDC port status bits

[Brad Hards]

5/11 does this:
-#define PORTSC_RO_MASK       0x007021c5
+#define PORTSC_RO_MASK       0x007021c4

before 8/11 does this:
> -#define PORTSC_RO_MASK       0x007021c4
> +#define PORTSC_RO_MASK       0x007021c0

You could push those together if there was a v2. I don't think its important 
though.

Even nicer would be to build it out of symbolic constants / defines, so its 
obvious what is being masked.

Brad

Re: [Qemu-devel] [PATCH 08/11] usb-ehci: Fix handling of PED and PEDC port status bits

[Hans de Goede]

Hi,

On 06/25/2011 02:15 AM, Brad Hards wrote:
> 5/11 does this:
> -#define PORTSC_RO_MASK       0x007021c5
> +#define PORTSC_RO_MASK       0x007021c4
>
> before 8/11 does this:
>> -#define PORTSC_RO_MASK       0x007021c4
>> +#define PORTSC_RO_MASK       0x007021c0
>
> You could push those together if there was a v2. I don't think its important
> though.
>

I would rather keep them separate, they happen to
touch the same define, but for completely different
reasons.

Regards,

Hans

Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Gerd Hoffmann]

   Hi,

> +    if (s->masterbus) {
> +        USBPort *ports[NB_PORTS];
> +        for(i = 0; i<  NB_PORTS; i++) {
> +            s->ports[i].port.ops =&uhci_port_ops;
> +            s->ports[i].port.opaque = s;
> +            s->ports[i].port.index = i;
> +            s->ports[i].port.speedmask =
> +                USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
> +            usb_port_location(&s->ports[i].port, NULL, i+1);
> +            ports[i] =&s->ports[i].port;
> +        }
> +        if (usb_bus_register_companion(s->masterbus, ports, NB_PORTS,
> +                                       s->firstport) != 0) {
> +            return -1;
> +        }
> +    } else {
> +        usb_bus_new(&s->bus,&uhci_bus_ops,&s->dev.qdev);
> +        for(i = 0; i<  NB_PORTS; i++) {
> +            usb_register_port(&s->bus,&s->ports[i].port, s, i,&uhci_port_ops,
> +                              USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
> +            usb_port_location(&s->ports[i].port, NULL, i+1);
> +        }

This looks like we'll want a usb_register_companion_port() function 
which looks like usb_register_port() but accepts masterbus & portindex 
instead of a USBBus pointer.  Then register the companion ports one by 
one, so that the code path for the companion case looks almost identical 
to the non-companion case.

Otherwise the whole patchset looks very good.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Hans de Goede]

Hi,

On 06/29/2011 12:57 PM, Gerd Hoffmann wrote:
> Hi,
>
>> + if (s->masterbus) {
>> + USBPort *ports[NB_PORTS];
>> + for(i = 0; i< NB_PORTS; i++) {
>> + s->ports[i].port.ops =&uhci_port_ops;
>> + s->ports[i].port.opaque = s;
>> + s->ports[i].port.index = i;
>> + s->ports[i].port.speedmask =
>> + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
>> + usb_port_location(&s->ports[i].port, NULL, i+1);
>> + ports[i] =&s->ports[i].port;
>> + }
>> + if (usb_bus_register_companion(s->masterbus, ports, NB_PORTS,
>> + s->firstport) != 0) {
>> + return -1;
>> + }
>> + } else {
>> + usb_bus_new(&s->bus,&uhci_bus_ops,&s->dev.qdev);
>> + for(i = 0; i< NB_PORTS; i++) {
>> + usb_register_port(&s->bus,&s->ports[i].port, s, i,&uhci_port_ops,
>> + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
>> + usb_port_location(&s->ports[i].port, NULL, i+1);
>> + }
>
> This looks like we'll want a usb_register_companion_port() function which looks like usb_register_port() but accepts masterbus & portindex instead of a USBBus pointer.
 > Then register the companion ports one by one, so that the code path for the companion case looks almost identical to the non-companion case.

I agree, but there is a reason why I went with a usb_bus_register_companion
function instead of with a usb_bus_register_companion_port function, the
uhci controller needs to know how many companion controllers it has
(to report this in one of its registers). When we're registering
ports 1 by 1, it does not know.

We could also pass in a port owner pointer, and make the uhci code keep
a list of known companions and check how many companions there are that
way, but that is quite ugly.

Another problem with registering ports 1 by 1 is that registering a companion
port can fail, and if the 2nd or higher register fails we would need to
undo the previous registers. Granted this is only an issue on hotplug,
and to support hot-unplug we would also need an unregister ..

Thinking more about this I think that the best approach would be to move
the port setup code (setting index, ops, speedmask, etc.) to
usb_bus_register_companion, and keep doing the entire registration
of all the ports in one single call. Would that work for you?

This still leaves the building of the port pointers array, we could
pass in a stride parameter to usb_bus_register_companion and make it
build that list too, I'm not sure if that is a good idea though?

 > Otherwise the whole patchset looks very good.

I'm glad you like it :)

Regards,

Hans

Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Gerd Hoffmann]

   Hi,

> I agree, but there is a reason why I went with a usb_bus_register_companion
> function instead of with a usb_bus_register_companion_port function, the
> uhci controller needs to know how many companion controllers it has
> (to report this in one of its registers). When we're registering
> ports 1 by 1, it does not know.

Good point.

> Thinking more about this I think that the best approach would be to move
> the port setup code (setting index, ops, speedmask, etc.) to
> usb_bus_register_companion, and keep doing the entire registration
> of all the ports in one single call. Would that work for you?

Yes.  Or have some helper function to fill the USBPort struct (which 
could be called by usb_register_port too).  I just don't like the 
initialization being done by the host adapter in one case and by the usb 
core code in the other case as this is a bit confusing and makes the 
code harder to read.

> This still leaves the building of the port pointers array, we could
> pass in a stride parameter to usb_bus_register_companion and make it
> build that list too, I'm not sure if that is a good idea though?

Passing in the pointer array is fine with me.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Hans de Goede]

Hi,

On 06/29/2011 02:29 PM, Gerd Hoffmann wrote:
> Hi,
>
>> I agree, but there is a reason why I went with a usb_bus_register_companion
>> function instead of with a usb_bus_register_companion_port function, the
>> uhci controller needs to know how many companion controllers it has
>> (to report this in one of its registers). When we're registering
>> ports 1 by 1, it does not know.
>
> Good point.
>
>> Thinking more about this I think that the best approach would be to move
>> the port setup code (setting index, ops, speedmask, etc.) to
>> usb_bus_register_companion, and keep doing the entire registration
>> of all the ports in one single call. Would that work for you?
>
> Yes. Or have some helper function to fill the USBPort struct (which could be called by usb_register_port too). I just don't like the initialization being done by the host adapter in one case and by the usb core code in the other case as this is a bit confusing and makes the code harder to read.
>

Ok, I've created a helper function, there is a new version of this
patch-set (based on your usb.17) in my tree, which should be ready for
pulling, so please pull the usb-patches branch from:
git://anongit.freedesktop.org/~jwrdegoede/qemu

I'm operating under the assumption here that a branch to pull is easiest
for you, if you would like me to provide the patches in another
format let me know.

Regards,

Hans