diff mbox

[v1,12/13] q35: fill in usb pci slots with -usb

Message ID 66b81e2e5ffe78ac446fe4ba0a28e748595b7bf6.1351561225.git.jbaron@redhat.com
State New
Headers show

Commit Message

Jason Baron Oct. 30, 2012, 2:11 a.m. UTC 
From: Jason Baron <jbaron@redhat.com>

This fills out the usb slots on q35, when -usb is passed.
We now have (lspci output):

00:1d.0 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #1 (rev 03)
00:1d.1 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #2 (rev 03)
00:1d.2 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #3 (rev 03)
00:1d.7 USB Controller: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #1 (rev 03)

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 hw/ich9.h   |    5 ++++-
 hw/pc_q35.c |   26 ++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

Comments

Gerd Hoffmann Oct. 30, 2012, 6:34 a.m. UTC | #1 
Hi,

> +            uhci_devname[sizeof(uhci_devname) - 2] = ((char)'1') + i;

snprintf(devname, sizeof(devname), "...%d", i) is more readable.

> +            qdev_prop_set_string(usb_qdev, "masterbus", "ich9-usb-bus.0");

Any reason why you rename the usb bus?

cheers,
  Gerd
Jason Baron Oct. 30, 2012, 3:19 p.m. UTC | #2 
On Tue, Oct 30, 2012 at 07:34:26AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > +            uhci_devname[sizeof(uhci_devname) - 2] = ((char)'1') + i;
> 
> snprintf(devname, sizeof(devname), "...%d", i) is more readable.

ok.

> 
> > +            qdev_prop_set_string(usb_qdev, "masterbus", "ich9-usb-bus.0");
> 
> Any reason why you rename the usb bus?
> 

I wasn't sure if the user created usb devices on the command-line via
-device if that would break naming here. Thus, I added a 'private' name.
If the naming is stable, that works. It would be 'usb-bus.0', in that
case?

Thanks,

-Jason
Gerd Hoffmann Oct. 30, 2012, 4:19 p.m. UTC | #3 
On 10/30/12 16:19, Jason Baron wrote:
> On Tue, Oct 30, 2012 at 07:34:26AM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> +            uhci_devname[sizeof(uhci_devname) - 2] = ((char)'1') + i;
>>
>> snprintf(devname, sizeof(devname), "...%d", i) is more readable.
> 
> ok.
> 
>>
>>> +            qdev_prop_set_string(usb_qdev, "masterbus", "ich9-usb-bus.0");
>>
>> Any reason why you rename the usb bus?
>>
> 
> I wasn't sure if the user created usb devices on the command-line via
> -device if that would break naming here. Thus, I added a 'private' name.
> If the naming is stable, that works. It would be 'usb-bus.0', in that
> case?

"usb.0" would be the default name, but you don't need to know it, you
can just look up what qdev created.  See here:

http://www.kraxel.org/cgit/qemu/commit/?h=rebase/usb-next&id=70b9867011c4793787c5acee3d2005a6bc951f59

[ This is part of the "usb patch queue" patch series posted today,
  depending on how the qom discussions go and how fast it goes in
  you might just call the function the patch provides.  Or do
  something simliar in pc_q35.c and I'll drop the patch. ]

-usb for -M pc creates a "usb.0" bus too, so I don't expect trouble.

cheers,
  Gerd
Jason Baron Oct. 30, 2012, 6 p.m. UTC | #4 
On Tue, Oct 30, 2012 at 05:19:01PM +0100, Gerd Hoffmann wrote:
> On 10/30/12 16:19, Jason Baron wrote:
> > On Tue, Oct 30, 2012 at 07:34:26AM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> +            uhci_devname[sizeof(uhci_devname) - 2] = ((char)'1') + i;
> >>
> >> snprintf(devname, sizeof(devname), "...%d", i) is more readable.
> > 
> > ok.
> > 
> >>
> >>> +            qdev_prop_set_string(usb_qdev, "masterbus", "ich9-usb-bus.0");
> >>
> >> Any reason why you rename the usb bus?
> >>
> > 
> > I wasn't sure if the user created usb devices on the command-line via
> > -device if that would break naming here. Thus, I added a 'private' name.
> > If the naming is stable, that works. It would be 'usb-bus.0', in that
> > case?
> 
> "usb.0" would be the default name, but you don't need to know it, you
> can just look up what qdev created.  See here:
> 
> http://www.kraxel.org/cgit/qemu/commit/?h=rebase/usb-next&id=70b9867011c4793787c5acee3d2005a6bc951f59

yes, much better :)

> 
> [ This is part of the "usb patch queue" patch series posted today,
>   depending on how the qom discussions go and how fast it goes in
>   you might just call the function the patch provides.  Or do
>   something simliar in pc_q35.c and I'll drop the patch. ]
> 
> -usb for -M pc creates a "usb.0" bus too, so I don't expect trouble.
> 

I think your patch, is a generally useful helper function. Thus, I plan to
incorporate something similar to your patch, but less general in pc_q35.c. So
usb can get testing, and when your patch lands I will drop the extra usb
bits in pc_q35.c.

Thanks,

-Jason
diff mbox

Patch

diff --git a/hw/ich9.h b/hw/ich9.h
index 10c6d47..c4d04a7 100644
--- a/hw/ich9.h
+++ b/hw/ich9.h
@@ -86,8 +86,11 @@  typedef struct ICH9LPCState {
 
 
 /* D29:F0 USB UHCI Controller #1 */
-#define ICH9_USB_UHCI1_DEV                      29
+#define ICH9_USB_DEV                            29
 #define ICH9_USB_UHCI1_FUNC                     0
+#define ICH9_USB_UHCI2_FUNC                     1
+#define ICH9_USB_UHCI3_FUNC                     2
+#define ICH9_USB_EHCI1_FUNC                     7
 
 /* D30:F0 DMI-to-PCI brdige */
 #define ICH9_D2P_BRIDGE                         "ICH9 D2P BRIDGE"
diff --git a/hw/pc_q35.c b/hw/pc_q35.c
index a9a7f6c..714aeaf 100644
--- a/hw/pc_q35.c
+++ b/hw/pc_q35.c
@@ -191,11 +191,29 @@  static void pc_q35_init_late(BusState **idebus, ISADevice *rtc_state,
     idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
 
     if (usb_enabled(false)) {
+        int i;
+        PCIDevice *usb;
+        DeviceState *usb_qdev;
+        char uhci_devname[] = "ich9-usb-uhciX";
+
         /* Should we create 6 UHCI according to ich9 spec? */
-        pci_create_simple_multifunction(
-            host_bus, PCI_DEVFN(ICH9_USB_UHCI1_DEV, ICH9_USB_UHCI1_FUNC),
-            true, "ich9-usb-uhci1");
-        /* XXX: EHCI */
+        usb = pci_create_multifunction(
+            host_bus, PCI_DEVFN(ICH9_USB_DEV, ICH9_USB_EHCI1_FUNC),
+            true, "ich9-usb-ehci1");
+        usb_qdev = &usb->qdev;
+        usb_qdev->id = g_strdup("ich9-usb-bus");
+        qdev_init_nofail(usb_qdev);
+
+        for (i = 0; i < 3; i++) {
+            uhci_devname[sizeof(uhci_devname) - 2] = ((char)'1') + i;
+            usb = pci_create_multifunction(
+                host_bus, PCI_DEVFN(ICH9_USB_DEV, ICH9_USB_UHCI1_FUNC + i),
+                true, uhci_devname);
+            usb_qdev = &usb->qdev;
+            qdev_prop_set_string(usb_qdev, "masterbus", "ich9-usb-bus.0");
+            qdev_prop_set_uint32(usb_qdev, "firstport", i * 2);
+            qdev_init_nofail(usb_qdev);
+        }
     }
 
     /* TODO: Populate SPD eeprom data.  */