> 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?
The link state is not guarantied for CAS case. I observe 'Compliance Mode'
every time. This link state is set with PORT_CONNECTED bit clear and with
CSC (Connect Status Change) bit set. I'm not sure if this sate will always
be the same (on other xhci, other device) so I check the link state to be
sure that warm reset will be performed. If we change solution to bogus CAS
bit support in core/hub then this part is useless.
Thanks,
Staszek
>
>>>> + 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
