On Mon, Apr 14, 2014 at 11:24:18PM +0800, Mathias Nyman wrote:
> Hi
>
> On 04/11/2014 01:20 PM, Pratyush Anand wrote:
> > As best case, a host controller should support U0 to U1 switching for
> > the devices connected below any tier of hub level supported by usb
> > specification. Therefore xhci_check_default_tier_policy always returns
> > success.
> >
> > A host should be able to issue LGO_Ux after the timeout calculated as
> > per definition of system exit latency defined in C.1.5.2. Therefore
> > xhci_calculate_default_ux_timeout returns ux_params.sel as the default
> > implementation.
> >
> > Use default calculation in absence of any vendor specific limitations.
> >
> > Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
> > ---
> > drivers/usb/host/xhci.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 6cc58fe..930c01f 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -4140,6 +4140,53 @@ static u16 xhci_get_timeout_no_hub_lpm(struct usb_device *udev,
> > return USB3_LPM_DISABLED;
> > }
> >
> > +/* Returns the default hub-encoded U1 timeout value.
> > + */
> > +static u16 xhci_calculate_default_u1_timeout(struct usb_device *udev,
> > + struct usb_endpoint_descriptor *desc)
> > +{
> > + unsigned long long timeout_ns;
> > +
> > + timeout_ns = udev->u1_params.sel;
> > +
> > + /* The U1 timeout is encoded in 1us intervals. */
> > + timeout_ns = DIV_ROUND_UP_ULL(timeout_ns, 1000);
> > + /* Don't return a timeout of zero, because that's USB3_LPM_DISABLED. */
> > + if (timeout_ns == USB3_LPM_DISABLED)
> > + timeout_ns++;
> > +
> > + /* If the necessary timeout value is bigger than what we can set in the
> > + * USB 3.0 hub, we have to disable hub-initiated U1.
> > + */
> > + if (timeout_ns <= USB3_LPM_U1_MAX_TIMEOUT)
> > + return timeout_ns;
> > + dev_dbg(&udev->dev, "Hub-initiated U1 disabled "
> > + "due to long timeout %llu ms\n", timeout_ns);
> > + return xhci_get_timeout_no_hub_lpm(udev, USB3_LPM_U1);
> > +}
>
> xhci_calculate_default_u1_timeout looks like a copy of the bottom part
> of the same intel specific funtion.
>
> > +
> > +/* Returns the default hub-encoded U2 timeout value.
> > + */
> > +static u16 xhci_calculate_default_u2_timeout(struct usb_device *udev,
> > + struct usb_endpoint_descriptor *desc)
> > +{
> > + unsigned long long timeout_ns;
> > +
> > + timeout_ns = udev->u2_params.sel;
> > +
> > + /* The U2 timeout is encoded in 256us intervals */
> > + timeout_ns = DIV_ROUND_UP_ULL(timeout_ns, 256 * 1000);
> > + /* If the necessary timeout value is bigger than what we can set in the
> > + * USB 3.0 hub, we have to disable hub-initiated U2.
> > + */
> > + if (timeout_ns <= USB3_LPM_U2_MAX_TIMEOUT)
> > + return timeout_ns;
> > + dev_dbg(&udev->dev, "Hub-initiated U2 disabled "
> > + "due to long timeout %llu ms\n", timeout_ns);
> > + return xhci_get_timeout_no_hub_lpm(udev, USB3_LPM_U2);
> > +}
> > +
> > +
>
> Same with this one
>
>
> > /* Returns the hub-encoded U1 timeout value.
> > * The U1 timeout should be the maximum of the following values:
> > * - For control endpoints, U1 system exit latency (SEL) * 3
> > @@ -4241,9 +4288,13 @@ static u16 xhci_call_host_update_timeout_for_endpoint(struct xhci_hcd *xhci,
> > if (state == USB3_LPM_U1) {
> > if (xhci->quirks & XHCI_INTEL_HOST)
> > return xhci_calculate_intel_u1_timeout(udev, desc);
> > + else
> > + return xhci_calculate_default_u1_timeout(udev, desc);
> > } else {
> > if (xhci->quirks & XHCI_INTEL_HOST)
> > return xhci_calculate_intel_u2_timeout(udev, desc);
> > + else
> > + return xhci_calculate_default_u2_timeout(udev, desc);
> > }
>
> To avoid code duplications I think it would be best to just move the
> Intel quirk parts inside generic xhci_calculate_ux_timeout() functions.
>
> Something like:
>
> static u16 xhci_calculate_u1_timeout(struct usb_device *udev,
> struct usb_endpoint_descriptor *desc)
> {
> unsigned long long timeout_ns;
> timeout_ns = udev->u1_params.sel;
>
> if (xhci->quirks & XHCI_INTEL_HOST) {
> /* the Intel specific part */
> }
>
> /* The U1 timeout is encoded in 1us intervals. */
> timeout_ns = DIV_ROUND_UP_ULL(timeout_ns, 1000);
> ...
> }
>
> And the same for xhci_calculate_u2_timeout().
>
> The xhci_call_host_update_timeout_for_endpoint() is then simplified to:
>
> if (state == USB3_LPM_U1) {
> return xhci_calculate_u1_timeout;
> else
> return xhci_calculate_u2_timeout
>
>
> >
> > return USB3_LPM_DISABLED;
> > @@ -4291,6 +4342,14 @@ static int xhci_update_timeout_for_interface(struct xhci_hcd *xhci,
> > return 0;
> > }
> >
> > +static int xhci_check_default_tier_policy(struct usb_device *udev,
> > + enum usb3_link_state state)
> > +{
> > + /* Keeep Ux enabled for all devices */
> > +
> > + return 0;
> > +}
> > +
> > static int xhci_check_intel_tier_policy(struct usb_device *udev,
> > enum usb3_link_state state)
> > {
> > @@ -4321,6 +4380,8 @@ static int xhci_check_tier_policy(struct xhci_hcd *xhci,
> > {
> > if (xhci->quirks & XHCI_INTEL_HOST)
> > return xhci_check_intel_tier_policy(udev, state);
> > + else
> > + return xhci_check_default_tier_policy(udev, state);
> > return -EINVAL;
> > }
> >
>
> And the same thing for the tier policy. One xhci_check_tier_policy()
> function with the intel quirk check inside.
OK. In next version I will incorporate all the above.
Regards
Pratyush
>
> -Mathias
>
--
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
