| Message ID | 1308943978-6152-11-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 */ }
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(-)