Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a270bd46-fb39-50a7-c2a7-da98b06458ff@zhaoxin.com>
Date:   Fri, 21 Apr 2023 18:56:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 2/3] xhci: Add zhaoxin xHCI U1/U2 feature support
Content-Language: en-US
To:     Mathias Nyman <mathias.nyman@intel.com>,
        <gregkh@linuxfoundation.org>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     <tonywwang@zhaoxin.com>, <weitaowang@zhaoxin.com>
References: <20230420172130.375819-1-WeitaoWang-oc@zhaoxin.com>
 <20230420172130.375819-3-WeitaoWang-oc@zhaoxin.com>
 <92cf6ed0-be37-d8a6-14c7-c5043f076916@intel.com>
From:   "WeitaoWang-oc@zhaoxin.com" <WeitaoWang-oc@zhaoxin.com>
In-Reply-To: <92cf6ed0-be37-d8a6-14c7-c5043f076916@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

Thanks,
weitao
> Or possibly even add a xhci->max_tier_for_lpm that can be set during probe based on
> vendor or from device property.
> 
> Thanks
> -Mathias
> 
> .