Re: [PATCH 0/3] Fix some issues of xHCI for zhaoxin

Re: [PATCH 0/3] Fix some issues of xHCI for zhaoxin

[Greg KH]

On Fri, Apr 21, 2023 at 01:21:27AM +0800, Weitao Wang wrote:
> Fix some issues of xHCI for zhaoxin.
> 
> Weitao Wang (3):
>   xhci: Add a quirk for zhaoxin xhci to fix issues.
>   xhci: Add zhaoxin xHCI U1/U2 feature support
>   xhci: Show zhaoxin xHCI root hub speed correctly
> 
>  drivers/usb/host/xhci-pci.c |  5 ++++
>  drivers/usb/host/xhci.c     | 49 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/host/xhci.h     |  1 +
>  3 files changed, 53 insertions(+), 2 deletions(-)
> 
> -- 
> 2.32.0
> 

Do these replace:
https://lore.kernel.org/r/20230420093603.3344-1-WeitaoWang-oc@zhaoxin.com
or are they on top of them?

thanks,

greg k-h

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

[Sergei Shtylyov]

On 4/20/23 8:21 PM, Weitao Wang wrote:

> Add U1/U2 feature support of xHCI for zhaoxin.
> 
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
[...]

> 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
[...]
> @@ -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");

   Please don't break up the message strings.

[...]
> @@ -4965,6 +4986,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 if (xhci->quirks & XHCI_ZHAOXIN_HOST)

   *else* not needed after *return*.

> +		return xhci_check_zhaoxin_tier_policy(udev, state);
>  	else
>  		return 0;
>  }

MBR, Sergey

Re: [PATCH 0/3] Fix some issues of xHCI for zhaoxin

[Greg KH]

On Fri, Apr 21, 2023 at 02:08:14AM +0800, WeitaoWang-oc@zhaoxin.com wrote:
> On 2023/4/20 17:29, Greg KH wrote:
> > On Fri, Apr 21, 2023 at 01:21:27AM +0800, Weitao Wang wrote:
> > > Fix some issues of xHCI for zhaoxin.
> > > 
> > > Weitao Wang (3):
> > >    xhci: Add a quirk for zhaoxin xhci to fix issues.
> > >    xhci: Add zhaoxin xHCI U1/U2 feature support
> > >    xhci: Show zhaoxin xHCI root hub speed correctly
> > > 
> > >   drivers/usb/host/xhci-pci.c |  5 ++++
> > >   drivers/usb/host/xhci.c     | 49 +++++++++++++++++++++++++++++++++++--
> > >   drivers/usb/host/xhci.h     |  1 +
> > >   3 files changed, 53 insertions(+), 2 deletions(-)
> > > 
> > > -- 
> > > 2.32.0
> > > 
> > 
> > Do these replace:
> > https://lore.kernel.org/r/20230420093603.3344-1-WeitaoWang-oc@zhaoxin.com
> > or are they on top of them?
> > 
> 
> This [patch 2/3] and [patch 3/3] share a xhci quirk flag XHCI_ZHAOXIN_HOST,
> So I put these independent functional patch in this set group.
> Above url and below url are independent xHCI patch for zhaoxin.
> Is it more suitable to put all the patch for zhaoxin xhci in one group?
> I Hope to receive your guidance. Thanks!
> 
> https://lore.kernel.org/all/20230420104826.4727-1-WeitaoWang-oc@zhaoxin.com/

Please resend them all as a patch series so we know what we are supposed
to be reviewing and accepting.  Otherwise it's quite confusing.

thanks,

greg k-h

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

[Mathias Nyman]

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.

>   	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().

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;
  }

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

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

[Sergei Shtylyov]

On 4/20/23 11:21 PM, WeitaoWang-oc@zhaoxin.com wrote:

>>> Add U1/U2 feature support of xHCI for zhaoxin.
>>>
>>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
>> [...]
>>
>>> 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
>> [...]
>>> @@ -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");
>>
>>     Please don't break up the message strings.
> 
> Thanks for your advice, and I will merge this message in next patch version.
>> [...]
>>> @@ -4965,6 +4986,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 if (xhci->quirks & XHCI_ZHAOXIN_HOST)
>>
>>     *else* not needed after *return*.
> This function need a "int" type return value. If remove "else" branch,
> vendor other than intel and zhaoxin will not get a return value.

   I didn't tell you to remove the whole branch, just the *else* keyword.

[...]
> Best Regards,
> Weitao

MBR, Sergey

[PATCH 0/3] Fix some issues of xHCI for zhaoxin

[Weitao Wang]

Fix some issues of xHCI for zhaoxin.

Weitao Wang (3):
  xhci: Add a quirk for zhaoxin xhci to fix issues.
  xhci: Add zhaoxin xHCI U1/U2 feature support
  xhci: Show zhaoxin xHCI root hub speed correctly

 drivers/usb/host/xhci-pci.c |  5 ++++
 drivers/usb/host/xhci.c     | 49 +++++++++++++++++++++++++++++++++++--
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 53 insertions(+), 2 deletions(-)

-- 
2.32.0

[PATCH 1/3] xhci: Add a quirk for zhaoxin xhci to fix issues.

[Weitao Wang]

Add a quirk for zhaoxin xhci to fix issues, there are
two cases will be used.
- add u1/u2 support.
- fix xHCI root hub speed show issue in zhaoxin platform.

Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
---
 drivers/usb/host/xhci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 786002bb35db..d95980fb5bd9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1905,6 +1905,7 @@ struct xhci_hcd {
 #define XHCI_EP_CTX_BROKEN_DCS	BIT_ULL(42)
 #define XHCI_SUSPEND_RESUME_CLKS	BIT_ULL(43)
 #define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
+#define XHCI_ZHAOXIN_HOST	BIT_ULL(46)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
2.32.0

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

[Weitao Wang]

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);
 	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);
 	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;
+}
+
 static int xhci_check_intel_tier_policy(struct usb_device *udev,
 		enum usb3_link_state state)
 {
@@ -4965,6 +4986,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 if (xhci->quirks & XHCI_ZHAOXIN_HOST)
+		return xhci_check_zhaoxin_tier_policy(udev, state);
 	else
 		return 0;
 }
-- 
2.32.0

[PATCH 3/3] xhci: Show zhaoxin xHCI root hub speed correctly

[Weitao Wang]

Some zhaoxin xHCI controllers follow usb3.1 spec,
but only support gen1 speed 5G. While in Linux kernel,
if xHCI suspport usb3.1,root hub speed will show on 10G.
To fix this issue of zhaoxin xHCI platforms, read usb speed ID
supported by xHCI to determine root hub speed.

Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
---
 drivers/usb/host/xhci.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 730c0f68518d..60b17799aa88 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5317,6 +5317,7 @@ static void xhci_hcd_init_usb2_data(struct xhci_hcd *xhci, struct usb_hcd *hcd)
 static void xhci_hcd_init_usb3_data(struct xhci_hcd *xhci, struct usb_hcd *hcd)
 {
 	unsigned int minor_rev;
+	unsigned int i, j;
 
 	/*
 	 * Early xHCI 1.1 spec did not mention USB 3.1 capable hosts
@@ -5346,6 +5347,27 @@ static void xhci_hcd_init_usb3_data(struct xhci_hcd *xhci, struct usb_hcd *hcd)
 		hcd->self.root_hub->ssp_rate = USB_SSP_GEN_2x1;
 		break;
 	}
+
+	/* Usb3.1 has gen1 and gen2, Some zhaoxin's xHCI controller
+	 * that follow usb3.1 spec but only support gen1.
+	 */
+	if (xhci->quirks & XHCI_ZHAOXIN_HOST) {
+		minor_rev = 0;
+		for (j = 0; j < xhci->num_port_caps; j++) {
+			for (i = 0; i < xhci->port_caps[j].psi_count; i++) {
+				if (XHCI_EXT_PORT_PSIV(xhci->port_caps[j].psi[i]) >= 5) {
+					minor_rev = 1;
+					break;
+				}
+			}
+			if (minor_rev)
+				break;
+		}
+		if (minor_rev != 1) {
+			hcd->speed = HCD_USB3;
+			hcd->self.root_hub->speed = USB_SPEED_SUPER;
+		}
+	}
 	xhci_info(xhci, "Host supports USB 3.%x %sSuperSpeed\n",
 		  minor_rev, minor_rev ? "Enhanced " : "");
 
-- 
2.32.0

Re: [PATCH 0/3] Fix some issues of xHCI for zhaoxin

[WeitaoWang-oc]

On 2023/4/20 17:29, Greg KH wrote:
> On Fri, Apr 21, 2023 at 01:21:27AM +0800, Weitao Wang wrote:
>> Fix some issues of xHCI for zhaoxin.
>>
>> Weitao Wang (3):
>>    xhci: Add a quirk for zhaoxin xhci to fix issues.
>>    xhci: Add zhaoxin xHCI U1/U2 feature support
>>    xhci: Show zhaoxin xHCI root hub speed correctly
>>
>>   drivers/usb/host/xhci-pci.c |  5 ++++
>>   drivers/usb/host/xhci.c     | 49 +++++++++++++++++++++++++++++++++++--
>>   drivers/usb/host/xhci.h     |  1 +
>>   3 files changed, 53 insertions(+), 2 deletions(-)
>>
>> -- 
>> 2.32.0
>>
> 
> Do these replace:
> https://lore.kernel.org/r/20230420093603.3344-1-WeitaoWang-oc@zhaoxin.com
> or are they on top of them?
> 

This [patch 2/3] and [patch 3/3] share a xhci quirk flag XHCI_ZHAOXIN_HOST,
So I put these independent functional patch in this set group.
Above url and below url are independent xHCI patch for zhaoxin.
Is it more suitable to put all the patch for zhaoxin xhci in one group?
I Hope to receive your guidance. Thanks!

https://lore.kernel.org/all/20230420104826.4727-1-WeitaoWang-oc@zhaoxin.com/

Best Regards,
Weitao

> thanks,
> 
> greg k-h
> .

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

[WeitaoWang-oc]

On 2023/4/20 17:39, Sergei Shtylyov wrote:
> On 4/20/23 8:21 PM, Weitao Wang wrote:
> 
>> Add U1/U2 feature support of xHCI for zhaoxin.
>>
>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> [...]
> 
>> 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
> [...]
>> @@ -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");
> 
>     Please don't break up the message strings.

Thanks for your advice, and I will merge this message in next patch version.
> [...]
>> @@ -4965,6 +4986,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 if (xhci->quirks & XHCI_ZHAOXIN_HOST)
> 
>     *else* not needed after *return*.
This function need a "int" type return value. If remove "else" branch,
vendor other than intel and zhaoxin will not get a return value.

Best Regards,
Weitao

>> +		return xhci_check_zhaoxin_tier_policy(udev, state);
>>   	else
>>   		return 0;
>>   }
> 
> MBR, Sergey
> .

Re: [PATCH 0/3] Fix some issues of xHCI for zhaoxin

[WeitaoWang-oc]

On 2023/4/20 18:14, Greg KH wrote:
> On Fri, Apr 21, 2023 at 02:08:14AM +0800, WeitaoWang-oc@zhaoxin.com wrote:
>> On 2023/4/20 17:29, Greg KH wrote:
>>> On Fri, Apr 21, 2023 at 01:21:27AM +0800, Weitao Wang wrote:
>>>> Fix some issues of xHCI for zhaoxin.
>>>>
>>>> Weitao Wang (3):
>>>>     xhci: Add a quirk for zhaoxin xhci to fix issues.
>>>>     xhci: Add zhaoxin xHCI U1/U2 feature support
>>>>     xhci: Show zhaoxin xHCI root hub speed correctly
>>>>
>>>>    drivers/usb/host/xhci-pci.c |  5 ++++
>>>>    drivers/usb/host/xhci.c     | 49 +++++++++++++++++++++++++++++++++++--
>>>>    drivers/usb/host/xhci.h     |  1 +
>>>>    3 files changed, 53 insertions(+), 2 deletions(-)
>>>>
>>>> -- 
>>>> 2.32.0
>>>>
>>>
>>> Do these replace:
>>> https://lore.kernel.org/r/20230420093603.3344-1-WeitaoWang-oc@zhaoxin.com
>>> or are they on top of them?
>>>
>>
>> This [patch 2/3] and [patch 3/3] share a xhci quirk flag XHCI_ZHAOXIN_HOST,
>> So I put these independent functional patch in this set group.
>> Above url and below url are independent xHCI patch for zhaoxin.
>> Is it more suitable to put all the patch for zhaoxin xhci in one group?
>> I Hope to receive your guidance. Thanks!
>>
>> https://lore.kernel.org/all/20230420104826.4727-1-WeitaoWang-oc@zhaoxin.com/
> 
> Please resend them all as a patch series so we know what we are supposed
> to be reviewing and accepting.  Otherwise it's quite confusing.

Okay, I'll resend all patch for zhaoxin xHCI in a patch series.

Best Regards,
weitao

> thanks,
> 
> greg k-h
> .

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

[Mathias Nyman]

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

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

[WeitaoWang-oc]

On 2023/4/20 22:22, Sergei Shtylyov wrote:
> On 4/20/23 11:21 PM, WeitaoWang-oc@zhaoxin.com wrote:
> 
>>>> Add U1/U2 feature support of xHCI for zhaoxin.
>>>>
>>>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
>>> [...]
>>>
>>>> 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
>>> [...]
>>>> @@ -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");
>>>
>>>      Please don't break up the message strings.
>>
>> Thanks for your advice, and I will merge this message in next patch version.
>>> [...]
>>>> @@ -4965,6 +4986,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 if (xhci->quirks & XHCI_ZHAOXIN_HOST)
>>>
>>>      *else* not needed after *return*.
>> This function need a "int" type return value. If remove "else" branch,
>> vendor other than intel and zhaoxin will not get a return value.
> 
>     I didn't tell you to remove the whole branch, just the *else* keyword.

I understand what you mean this time. Here's code needs more concise.
I'll try to do more think and test.Thanks again!

weitao
> [...]
>> Best Regards,
>> Weitao
> 
> MBR, Sergey
> .

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

[WeitaoWang-oc]

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
> 
> .