On 06/04/2012 09:34 PM, Stanislaw Ledwon wrote:
>> On 06/04/2012 04:46 PM, Stanislaw Ledwon wrote:
>>> The host controller port status register supports CAS (Could Attach
s/Could/Cold/
>>> Status) bit. This bit could be set when USB3.0 device is connectd
s/connectd/connected/
>>> when system is in Sx state. When the system wakes to S0 this port
>>> status with CAS bit is reported and this port can't be used by any
>>> device.
>>>
>>> When CAS bit is set the port should be reset by warm reset. This
>>> was not supported by xhci driver.
>>>
>>> The issue was found when pendrive was connected to suspended
>>> platfrom. The link state of "Compliance Mode" was reported togather
s/platfrom/platform/
s/togather/together/
>>> with CAS bit. This link state was also not supported by xhci and
>>> core/hub.c.
>>>
>>> The CAS bit is defined only for xhci root hub port and it is
>>> not supported on regular hubs. The link status is used to force
>>> warm reset on port. Make the USB core issue a warm reset when port
>>> is in ether the 'inactive' or 'compliance mode'. Change the xHCI driver
>>> to report 'compliance mode' when the CAS is set. This force warm reset
>>> on the root hub port.
>>>
>>
>> Why fake the port status to compliance mode? All you want to do is issue
>> a warm reset when finding a CAS bit set. Fake a bogus USB3 wPortStatus
>> bit is simpler and clearer.
>>
>> It's like this:
>>
>> 1. define a bogus bit in ch11.h. You can use bit13 since it's reserved.
>>
>> #define USB_SS_PORT_STAT_CAS_BOGUS 0x2000 /* bit 13 */
>>
>> 2. In GetPortStatus in xhci_hub_control, report the CAS.
>>
>> If (temp & PORT_CAS)
>> status |= USB_SS_PORT_STAT_CAS_BOGUS;
>>
>> 3. in hub.c, issue a warm reset if CAS is found.
>>
> The CAS bit is defined only on XHCI port status so I thought that this is
> better to not propagate this bit to core/hub. When the bogus bit is
> defined in the reserved area of the port status then some conflicts are
> possible on future USB products.
> I think that this bogus bit should not be associated only with CAS but
> should be used as general port reset request form controller specific
> layer.
> The reset on Compliance Modes should be supported as well - based on
> USB3.0 HUB state diagram this is only possible way to return from this
> state.
>
OK.
>> +static bool hub_port_warm_reset_required(struct usb_hub *hub, u16
>> portstatus)
>> {
>> return hub_is_superspeed(hub->hdev) &&
>> - (portstatus & USB_PORT_STAT_LINK_STATE) ==
>> - USB_SS_PORT_LS_SS_INACTIVE;
>> + (((portstatus & USB_PORT_STAT_LINK_STATE) ==
>> + USB_SS_PORT_LS_SS_INACTIVE) ||
>> + (portstatus & USB_PORT_STAT_CAS_BOGUS));
>> }
>>
>>> Signed-off-by: Stanislaw Ledwon <staszek.ledwon@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/usb/core/hub.c | 18 +++++++++-------
>>> drivers/usb/host/xhci-hub.c | 44
>>> +++++++++++++++++++++++++++++++++++++-----
>>> drivers/usb/host/xhci.h | 6 ++++-
>>> 3 files changed, 53 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>>> index ec6c97d..572769b 100644
>>> --- a/drivers/usb/core/hub.c
>>> +++ b/drivers/usb/core/hub.c
>>> @@ -2102,12 +2102,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>>> static int hub_port_reset(struct usb_hub *hub, int port1,
>>> struct usb_device *udev, unsigned int delay, bool warm);
>>>
>>> -/* Is a USB 3.0 port in the Inactive state? */
>>> -static bool hub_port_inactive(struct usb_hub *hub, u16 portstatus)
>>> +/* Is a USB 3.0 port in the Inactive or Complinance Mode state?
>>> + * Port worm reset is required to recover
>>> + */
>>> +static bool hub_port_warm_reset_required(struct usb_hub *hub, u16
>>> portstatus)
>>> {
>>> return hub_is_superspeed(hub->hdev) &&
>>> - (portstatus & USB_PORT_STAT_LINK_STATE) ==
>>> - USB_SS_PORT_LS_SS_INACTIVE;
>>> + (((portstatus & USB_PORT_STAT_LINK_STATE) ==
>>> + USB_SS_PORT_LS_SS_INACTIVE) ||
>>> + ((portstatus & USB_PORT_STAT_LINK_STATE) ==
>>> + USB_SS_PORT_LS_COMP_MOD)) ;
>>> }
>>>
>>> static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>>> @@ -2143,7 +2147,7 @@ static int hub_port_wait_reset(struct usb_hub
>>> *hub, int port1,
>>> *
>>> * See https://bugzilla.kernel.org/show_bug.cgi?id=41752
>>> */
>>> - if (hub_port_inactive(hub, portstatus)) {
>>> + if (hub_port_warm_reset_required(hub, portstatus)) {
>>> int ret;
>>>
>>> if ((portchange & USB_PORT_STAT_C_CONNECTION))
>>> @@ -3753,9 +3757,7 @@ static void hub_events(void)
>>> /* Warm reset a USB3 protocol port if it's in
>>> * SS.Inactive state.
>>> */
>>> - if (hub_is_superspeed(hub->hdev) &&
>>> - (portstatus & USB_PORT_STAT_LINK_STATE)
>>> - == USB_SS_PORT_LS_SS_INACTIVE) {
>>> + if (hub_port_warm_reset_required(hub, portstatus)) {
>>> dev_dbg(hub_dev, "warm reset port %d\n", i);
>>> hub_port_reset(hub, i, NULL,
>>> HUB_BH_RESET_TIME, true);
>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>> index 673ad12..58e1e73 100644
>>> --- a/drivers/usb/host/xhci-hub.c
>>> +++ b/drivers/usb/host/xhci-hub.c
>>> @@ -462,6 +462,42 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci,
>>> __le32 __iomem **port_array,
>>> }
>>> }
>>>
>>> +/* Updates Link Status for super Speed port */
>>> +void xhci_hub_set_link_status(u32 *status, u32 status_reg)
>>
>> The function name is ambiguous. Its purpose is to report a bogus port
>> status, not "set_link_status". There is another function named
>> xhci_set_link_state() in the same file, which does complete different
>> things.
>>
> What about xhci_report_link_state()?
>
That's better.
>> Also, this should be a static function.
> Yes.
>>
>>> +{
>>> + u32 pls = status_reg & PORT_PLS_MASK;
>>> +
>>> + /* resume state is a xHCI internal state.
>>> + * Do not report it to usb core.
>>> + */
>>> + if (pls == XDEV_RESUME)
>>> + return;
>>> +
>>> + /* When the CAS bit is set then warm reset
>>> + * should be performed on port
>>> + */
>>> + if (status_reg & PORT_CAS) {
>>> + /* The CAS bit can be set while the port is
>>> + * in any link state.
>>> + * Only roothubs have CAS bit, so we
>>> + * pretend to be in compliance mode
>>> + * unless we're already in compliance
>>> + * or the inactive state.
>>> + */
>>> + if (pls != USB_SS_PORT_LS_COMP_MOD &&
>>> + pls != USB_SS_PORT_LS_SS_INACTIVE) {
This if is not necessary if you want a warm reset anyway.
What is the real link state when CAS is set? Is it the same every time?
>>> + pls = USB_SS_PORT_LS_COMP_MOD;
>>> + }
>>> + /* Return also connection bit -
>>> + * hub state machine resets port
>>> + * when this bit is set.
>>> + */
>>> + pls |= USB_PORT_STAT_CONNECTION;
>>
>> Why do you need this setting? Does PORTSC return a PORT_CONNECT when CAS
>> is set?
> In case of exiting S3 when CAS bit is set, the PORT_CONNECT is not set and
> port status change is ignored by core/hub.
>
I see.
Thanks,
Andiry
>>
>>> + }
>>> + /* update status field */
>>> + *status |= pls;
>>> +}
>>> +
>>> int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>> u16 wIndex, char *buf, u16 wLength)
>>> {
>>> @@ -604,13 +640,9 @@ int xhci_hub_control(struct usb_hcd *hcd, u16
>>> typeReq, u16 wValue,
>>> else
>>> status |= USB_PORT_STAT_POWER;
>>> }
>>> - /* Port Link State */
>>> + /* Update Port Link State for super speed ports*/
>>> if (hcd->speed == HCD_USB3) {
>>> - /* resume state is a xHCI internal state.
>>> - * Do not report it to usb core.
>>> - */
>>> - if ((temp & PORT_PLS_MASK) != XDEV_RESUME)
>>> - status |= (temp & PORT_PLS_MASK);
>>> + xhci_hub_set_link_status(&status, temp);
>>> }
>>> if (bus_state->port_c_suspend & (1 << wIndex))
>>> status |= 1 << USB_PORT_FEAT_C_SUSPEND;
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 3d69c4b..7e8280a 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -341,7 +341,11 @@ struct xhci_op_regs {
>>> #define PORT_PLC (1 << 22)
>>> /* port configure error change - port failed to configure its link
>>> partner */
>>> #define PORT_CEC (1 << 23)
>>> -/* bit 24 reserved */
>>> +/* Cold Attach Status - xHC can set this bit to report device attached
>>> during
>>> + * Sx state. Warm port reset should be perfomed to clear this bit and
>>> move port
>>> + * to connected state.
>>> + */
>>> +#define PORT_CAS (1 << 24)
>>> /* wake on connect (enable) */
>>> #define PORT_WKCONN_E (1 << 25)
>>> /* wake on disconnect (enable) */
>>
>>
>>
>
>
--
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
