On Mon, Apr 25, 2011 at 04:23:52PM +0800, Xu, Andiry wrote: > > -----Original Message----- > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > Sent: Saturday, April 23, 2011 4:22 AM > > To: Xu, Andiry > > Cc: gregkh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > > stern@xxxxxxxxxxxxxxxxxxx > > Subject: Re: [PATCH 4/5 v2] xHCI: report USB3.0 portstatus comply with > > USB3.0 specification > > > > On Tue, Mar 29, 2011 at 10:59:13AM +0800, Andiry Xu wrote: > > > USB3.0 specification has different wPortStatus and wPortChange > > > definitions from USB2.0 specification. Since USB3 root hub and USB2 > > > root hub are split now and USB3 hub only has USB3 protocol ports, we > > > should modify the portstatus and portchange report of USB3 ports to > > > comply with USB3.0 specification. > > > > Andiry, > > > > It looks like you report the port link state and BH reset status for > > the USB 2.0 roothub, here: > > > > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci- > > hub.c > > > index a0aa431..7c9e171 100644 > > > --- a/drivers/usb/host/xhci-hub.c > > > +++ b/drivers/usb/host/xhci-hub.c > > > @@ -441,13 +441,19 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 > > typeReq, u16 wValue, > > > status |= USB_PORT_STAT_C_ENABLE << 16; > > > if ((temp & PORT_OCC)) > > > status |= USB_PORT_STAT_C_OVERCURRENT << 16; > > > + if ((temp & PORT_PLC)) > > > + status |= USB_PORT_STAT_C_LINK_STATE << 16; > > > + if ((temp & PORT_WRC)) > > > + status |= USB_PORT_STAT_C_BH_RESET << 16; > > > > You should only or in the link state and BH reset if this is a USB 3.0 > > roothub, correct? External USB 2.0 hubs won't report link state > > changes or warm reset changes. I suppose the xHCI host controller > > isn't supposed to set those bits for USB 2.0 ports, but I'd rather > > protect against buggy hosts. > > > > Yes, you're right. The warm reset change report should be applied to > USB3.0 hub only. But USB2 protocol ports will report port link state > change > too. Shall we just ignore it for USB2.0 root hub? Yes, I think the xHCI roothub should just act like an external USB 2.0 roothub, so we should just ignore the port link state change. I asked a co-worker about what the EHCI driver does with the link state change when L1 is enabled, and he said the EHCI driver doesn't pass it up to the USB core, it just handles the change internally. We'll probably have to do something similar when the xHCI driver supports L1. > > Also, I think the port link state reporting for the USB 3.0 roothub > has > > a bug in it: > > > > > + /* Port Link State */ > > > + if (hcd->speed == HCD_USB3) > > > + status |= (temp & PORT_PLS_MASK); > > > > The link state is almost identical between the external USB 3.0 hub > > fields and the xHCI fields, except that xHCI reports a "resume" state > > of 0xF, and the USB 3.0 bus spec lists the 0xF value as reserved. So > > the USB core wouldn't know what to make of the link state being set to > > resume. I'm not sure what state we should report to the USB core when > > the xHC sets the state to resume. Perhaps U0? The state machine in > > figure 34 shows that's the next state after the Resume state. > > > > Thanks for point out this, I didn't notice that Resume state is not in > USB3.0 specification. > What state does external USB3.0 hub report in this case? I don't think the USB 3.0 hub reports this state at all. I think this is only used as a temporary state because software needs to time the resume signaling when a device sends a remote wakeup, and turn it off by setting the port link state to U0. According to figure 10-10 in the USB 3.0 bus spec, once an external hub receives a link state request to put a downstream port into U0, or the downstream device sends a remote wakeup, the hub immediately transitions the internal port state into U0. I'm not sure why the xHCI roothub needs that extra resume state. I'll ask the xHCI spec architect. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
