Re: [RFC] usb: Add support for root hub port status CAS

[Andiry Xu <andiry.xu@xxxxxxx>]

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