> On 06/04/2012 04:46 PM, Stanislaw Ledwon wrote:
>> The host controller port status register supports CAS (Could Attach
>> Status) bit. This bit could be set when USB3.0 device is connectd
>> 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
>> 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.
> +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()?
> 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) {
>> + 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.
Thanks,
Staszek
>
> 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
