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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 21.4.2023 13.56, WeitaoWang-oc@zhaoxin.com 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@zhaoxin.com>
>>> ---
>>>   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@linux.intel.com>

Thanks
Mathias