[Qemu-devel] [PATCH 0/2] xhci: Better spec conforming behaviour

[Qemu-devel] [PATCH 0/2] xhci: Better spec conforming behaviour

[Sebastian Bauer]

This tiny patch series fixes some things in the xhci hcd to behave more 
spec compliant.

The first patch ensures the correctness of the csc port status bit as
soon as the controller is switched to running mode.

The second patch brings in the evaluation of the speed field in the
input slot context associated to a set address command. See 6.2.2.1 of
the xhci spec.

Sebastian Bauer (2):
  When the XHCI host controller is switched to the running mode, set
    the ccs bit for each port, to which a device is already attached.
  Respond a set address command with CC_PARAMETER_ERROR, if the speed
    field of the input context is not valid.

 hw/usb/hcd-xhci.c |   35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] When the XHCI host controller is switched to the running mode, set the ccs bit for each port, to which a device is already attached.

[Sebastian Bauer]

---
 hw/usb/hcd-xhci.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 37b3dbb..4c81dcc 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -990,10 +990,29 @@ static void xhci_er_reset(XHCIState *xhci, int v)
             v, intr->er_start, intr->er_size);
 }
 
+static void xhci_set_port_csc(XHCIState* xhci, XHCIPort* port, int set)
+{
+    port->portsc |= PORTSC_CSC;
+    XHCIEvent ev = { ER_PORT_STATUS_CHANGE, CC_SUCCESS, port->portnr << 24 };
+    xhci_event(xhci, &ev, 0);
+    DPRINTF("xhci: port change event for port %d\n", port->portnr);
+}
+
 static void xhci_run(XHCIState *xhci)
 {
+    int i;
+
     trace_usb_xhci_run();
     xhci->usbsts &= ~USBSTS_HCH;
+
+    for (i=0;i<MAXPORTS;i++) {
+        if (xhci->ports[i].uport) {
+            if (xhci->ports[i].portsc & PORTSC_CCS) {
+                xhci_set_port_csc(xhci,&xhci->ports[i],1);
+            }
+        }
+    }
+
     xhci->mfindex_start = qemu_get_clock_ns(vm_clock);
 }
 
@@ -2307,11 +2326,7 @@ static void xhci_update_port(XHCIState *xhci, XHCIPort *port, int is_detach)
     }
 
     if (xhci_running(xhci)) {
-        port->portsc |= PORTSC_CSC;
-        XHCIEvent ev = { ER_PORT_STATUS_CHANGE, CC_SUCCESS,
-                         port->portnr << 24};
-        xhci_event(xhci, &ev, 0);
-        DPRINTF("xhci: port change event for port %d\n", port->portnr);
+    	xhci_set_port_csc(xhci, port, 1);
     }
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] When the XHCI host controller is switched to the running mode, set the ccs bit for each port, to which a device is already attached.

[Gerd Hoffmann]

On 10/26/12 10:29, Sebastian Bauer wrote:

Wrong fix I think.  Just moving "port->portsc |= PORTSC_CSC" out of the
"if (running)" should do.  usb-next already has a patch which fixes this:

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

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] When the XHCI host controller is switched to the running mode, set the ccs bit for each port, to which a device is already attached.

[Sebastian Bauer]

Hi!

Am 26.10.2012 14:00, schrieb Gerd Hoffmann:
> Wrong fix I think.  Just moving "port->portsc |= PORTSC_CSC" out of 
> the
> "if (running)" should do.  usb-next already has a patch which fixes 
> this:

I'm not sure about that. According to the spec, when the hc is set to 
running mode (p.70)

"At this point, the host controller is up and running and the Root Hub 
ports (5.4.8) will begin reporting device
connects"

Therefore I made it slightly more complex. Granted, as far as I can 
see, the spec doesn't explicitly state that the devices are not reported 
when being not in the running mode.

It probably would be easier to mask out relevant bits when reading the 
port registers if the hcd would be strict.

> 
> http://www.kraxel.org/cgit/qemu/commit/?h=rebase/usb-next&id=84f84686c701731964a515e9bbcfb475cfc1de8c

Nice, this wasn't there when I looked into this today. However, does 
this commit also ensure that the status port change event is generated 
when the hc is switched to running mode and there were already some 
devices present?

Bye,
Sebastian

Re: [Qemu-devel] [PATCH 1/2] When the XHCI host controller is switched to the running mode, set the ccs bit for each port, to which a device is already attached.

[Gerd Hoffmann]

  Hi,

[ sorry for the delay, was busy with other stuff ]

> I'm not sure about that. According to the spec, when the hc is set to
> running mode (p.70)
> 
> "At this point, the host controller is up and running and the Root Hub
> ports (5.4.8) will begin reporting device
> connects"

I understand "report" as "stick events into the event ring".

So I think the portsc register should be updated no matter what, and
when xhci is running additionally queue an event.

> However, does
> this commit also ensure that the status port change event is generated
> when the hc is switched to running mode and there were already some
> devices present?

No.  I don't think the controller is supposed to do that.

cheers,
  Gerd

[Qemu-devel] [PATCH 2/2] Respond a set address command with CC_PARAMETER_ERROR, if the speed field of the input context is not valid.

[Sebastian Bauer]

---
 hw/usb/hcd-xhci.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4c81dcc..b556b6e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1841,6 +1841,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
     uint32_t ictl_ctx[2];
     uint32_t slot_ctx[4];
     uint32_t ep0_ctx[5];
+    uint32_t speed;
     int i;
     TRBCCode res;
 
@@ -1884,6 +1885,15 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
         return CC_USB_TRANSACTION_ERROR;
     }
 
+    /* Check for validness of the input contexts, see 6.2.2.1 */
+    speed = (slot_ctx[0] >> 20) & 0xf;
+    if (speed != uport->dev->speed + 1)
+    {
+        fprintf(stderr,"xhci: invalid device speed in slot context for slot %u (expected %d, got %d).\n",
+                slotid, uport->dev->speed+1, speed);
+        return CC_PARAMETER_ERROR;
+    }
+
     for (i = 0; i < MAXSLOTS; i++) {
         if (xhci->slots[i].uport == uport) {
             fprintf(stderr, "xhci: port %s already assigned to slot %d\n",
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] Respond a set address command with CC_PARAMETER_ERROR, if the speed field of the input context is not valid.

[Gerd Hoffmann]

Looks ok, but please add the description & spec reference from the cover
letter to the commit message.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 0/2] xhci: Better spec conforming behaviour

[Gerd Hoffmann]

On 10/26/12 10:29, Sebastian Bauer wrote:
> This tiny patch series fixes some things in the xhci hcd to behave more 
> spec compliant.
> 
> The first patch ensures the correctness of the csc port status bit as
> soon as the controller is switched to running mode.
> 
> The second patch brings in the evaluation of the speed field in the
> input slot context associated to a set address command. See 6.2.2.1 of
> the xhci spec.

Patch descriptions should go into the commit message so they show up in
the changelog.

cheers,
  Gerd