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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add U1/U2 feature support of xHCI for zhaoxin.
Signed-off-by: Weitao Wang <[email protected]>
---
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
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/[email protected]
or are they on top of them?
thanks,
greg k-h
On 4/20/23 8:21 PM, Weitao Wang wrote:
> Add U1/U2 feature support of xHCI for zhaoxin.
>
> Signed-off-by: Weitao Wang <[email protected]>
[...]
> 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
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/[email protected]
> 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/[email protected]/
Best Regards,
Weitao
> thanks,
>
> greg k-h
> .
On Fri, Apr 21, 2023 at 02:08:14AM +0800, [email protected] 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/[email protected]
> > 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/[email protected]/
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
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 <[email protected]>
> [...]
>
>> 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
> .
On 2023/4/20 18:14, Greg KH wrote:
> On Fri, Apr 21, 2023 at 02:08:14AM +0800, [email protected] 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/[email protected]
>>> 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/[email protected]/
>
> 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
> .
On 20.4.2023 20.21, Weitao Wang wrote:
> Add U1/U2 feature support of xHCI for zhaoxin.
>
> Signed-off-by: Weitao Wang <[email protected]>
> ---
> 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
On 4/20/23 11:21 PM, [email protected] wrote:
>>> Add U1/U2 feature support of xHCI for zhaoxin.
>>>
>>> Signed-off-by: Weitao Wang <[email protected]>
>> [...]
>>
>>> 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
On 2023/4/20 22:22, Sergei Shtylyov wrote:
> On 4/20/23 11:21 PM, [email protected] wrote:
>
>>>> Add U1/U2 feature support of xHCI for zhaoxin.
>>>>
>>>> Signed-off-by: Weitao Wang <[email protected]>
>>> [...]
>>>
>>>> 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
> .
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 <[email protected]>
>> ---
>> 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
>
> .
On 21.4.2023 13.56, [email protected] 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
Thanks
Mathias