Re: [PATCH 2/3] xhci: Add zhaoxin xHCI U1/U2 feature support

[Mathias Nyman]

On 21.4.2023 13.56, WeitaoWang-oc@xxxxxxxxxxx wrote:
> On 2023/4/20 22:07, Mathias Nyman wrote:
> > On 20.4.2023 20.21, Weitao Wang wrote:
> > > Add U1/U2 feature support of xHCI for zhaoxin.
> > >
> > > Signed-off-by: Weitao Wang <WeitaoWang-oc@xxxxxxxxxxx>
> > > ---
> > >   drivers/usb/host/xhci-pci.c |  5 +++++
> > >   drivers/usb/host/xhci.c     | 27 +++++++++++++++++++++++++--
> > >   2 files changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > > index 6db07ca419c3..a235effe8e5c 100644
> > > --- a/drivers/usb/host/xhci-pci.c
> > > +++ b/drivers/usb/host/xhci-pci.c
> > > @@ -334,6 +334,11 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> > >            pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4))
> > >           xhci->quirks |= XHCI_NO_SOFT_RETRY;
> > > +    if (pdev->vendor == PCI_VENDOR_ID_ZHAOXIN) {
> > > +        xhci->quirks |= XHCI_LPM_SUPPORT;
> > > +        xhci->quirks |= XHCI_ZHAOXIN_HOST;
> > > +    }
> > > +
> > >       /* xHC spec requires PCI devices to support D3hot and D3cold */
> > >       if (xhci->hci_version >= 0x120)
> > >           xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 6307bae9cddf..730c0f68518d 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -4802,7 +4802,7 @@ static u16 xhci_calculate_u1_timeout(struct xhci_hcd *xhci,
> > >           }
> > >       }
> > > -    if (xhci->quirks & XHCI_INTEL_HOST)
> > > +    if (xhci->quirks & (XHCI_INTEL_HOST | XHCI_ZHAOXIN_HOST))
> > >           timeout_ns = xhci_calculate_intel_u1_timeout(udev, desc);
> >
> > Looks odd to tie Zhaoxin vendor to Intel specific values but ok,
> > if they diverge in the future we anyway need to modify this.
>
> These Intel specific values look good for zhaoxin xHCI with test.
> Reused this piece of code for simplicity. If there are any difference
> for these value, update will be submitted in the future.
>
> > >       else
> > >           timeout_ns = udev->u1_params.sel;
> > > @@ -4866,7 +4866,7 @@ static u16 xhci_calculate_u2_timeout(struct xhci_hcd *xhci,
> > >           }
> > >       }
> > > -    if (xhci->quirks & XHCI_INTEL_HOST)
> > > +    if (xhci->quirks & (XHCI_INTEL_HOST | XHCI_ZHAOXIN_HOST))
> > >           timeout_ns = xhci_calculate_intel_u2_timeout(udev, desc);
> >
> > same.
> >
> > >       else
> > >           timeout_ns = udev->u2_params.sel;
> > > @@ -4938,6 +4938,27 @@ static int xhci_update_timeout_for_interface(struct xhci_hcd *xhci,
> > >       return 0;
> > >   }
> > > +static int xhci_check_zhaoxin_tier_policy(struct usb_device *udev,
> > > +        enum usb3_link_state state)
> > > +{
> > > +    struct usb_device *parent;
> > > +    unsigned int num_hubs;
> > > +
> > > +    /* Don't enable U1/U2 if the device is on an external hub. */
> > > +    for (parent = udev->parent, num_hubs = 0; parent->parent;
> > > +            parent = parent->parent)
> > > +        num_hubs++;
> > > +
> > > +    if (num_hubs < 1)
> > > +        return 0;
> > > +
> > > +    dev_dbg(&udev->dev, "Disabling U1/U2 link state for device"
> > > +            " below external hub.\n");
> > > +    dev_dbg(&udev->dev, "Plug device into root hub "
> > > +            "to decrease power consumption.\n");
> > > +    return -E2BIG;
> > > +}
> > > +
> >
> > I don't think we should add more vendor specific functions, this is almost
> > an exact copy of xhci_check_intel_tier_policy().
>
> Adding duplicate vendor related code is indeed a bit redundant.
>
> > How about getting rid of both of those and use something like this instead (untested):
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 2b280beb0011..e9a25e4d99cf 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -4926,10 +4926,24 @@ static int xhci_check_tier_policy(struct xhci_hcd *xhci,
> >                  struct usb_device *udev,
> >                  enum usb3_link_state state)
> >   {
> > -       if (xhci->quirks & XHCI_INTEL_HOST)
> > -               return xhci_check_intel_tier_policy(udev, state);
> > -       else
> > -               return 0;
> > +       struct usb_device *parent = udev->parent;
> > +       int tier = 1; /* roothub is tier1 */
> > +
> > +       while (parent) {
> > +               parent = parent->parent;
> > +               tier++;
> > +       }
> > +
> > +       if (xhci->quirks & XHCI_INTEL_HOST && tier > 3)
> > +               goto fail;
> > +       if (xhci->quirks & XHCI_ZHAOXIN_HOST && tier > 2)
> > +               goto fail;
> > +
> > +       return 0;
> > +fail:
> > +       dev_dbg(&udev->dev, "Tier policy prevents U1/U2 LPM states for devices at tier %d\n",
> > +               tier);
> > +       return -E2BIG;
> >   }
>
> These code looks very elegant. Could I resubmit patch using your code
> after testing pass on Intel and Zhaoxin platform.

Yes, you can add Suggested-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>

Thanks
Mathias