[PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[Weitao Wang]

Over Current condition is not standardized in the UHCI spec.
Zhaoxin UHCI controllers report OverCurrent bit active off.
Intel controllers report it active on, so we'll adjust the bit value.

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

diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
index 3592f757fe05..177e3c2aa287 100644
--- a/drivers/usb/host/uhci-pci.c
+++ b/drivers/usb/host/uhci-pci.c
@@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
 	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
 		uhci->oc_low = 1;
 
+	/* ZHAOXIN controllers report OverCurrent bit active off. */
+	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
+		uhci->oc_low = 1;
+
 	/* HP's server management chip requires a longer port reset delay. */
 	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_HP)
 		uhci->wait_for_hp = 1;
-- 
2.32.0

Re: [PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[Greg KH]

On Thu, Apr 20, 2023 at 07:14:45PM +0800, Weitao Wang wrote:
> Over Current condition is not standardized in the UHCI spec.
> Zhaoxin UHCI controllers report OverCurrent bit active off.
> Intel controllers report it active on, so we'll adjust the bit value.
> 
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>

Note, your Subject: line has odd characters in it, can you rewrite it to
just use ascii?  I think it's the '：' character which should be ':',
right?

> ---
>  drivers/usb/host/uhci-pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
> index 3592f757fe05..177e3c2aa287 100644
> --- a/drivers/usb/host/uhci-pci.c
> +++ b/drivers/usb/host/uhci-pci.c
> @@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
>  	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
>  		uhci->oc_low = 1;
>  
> +	/* ZHAOXIN controllers report OverCurrent bit active off. */
> +	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
> +		uhci->oc_low = 1;
> +

This should also go to the stable kernels, right?

But a new UHCI controller?  And this affects all of them?  And why is
Intel an issue here (as you mention in the changelog text)?

thanks,

greg k-h

Re: [PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[WeitaoWang-oc]

On 2023/4/20 16:05, Greg KH wrote:
> On Thu, Apr 20, 2023 at 07:14:45PM +0800, Weitao Wang wrote:
>> Over Current condition is not standardized in the UHCI spec.
>> Zhaoxin UHCI controllers report OverCurrent bit active off.
>> Intel controllers report it active on, so we'll adjust the bit value.
>>
>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> 
> Note, your Subject: line has odd characters in it, can you rewrite it to
> just use ascii?  I think it's the '：' character which should be ':',
> right?Yes,you are right. it's my typo mistake. I'll rewrite this patch. Thanks!

>> ---
>>   drivers/usb/host/uhci-pci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
>> index 3592f757fe05..177e3c2aa287 100644
>> --- a/drivers/usb/host/uhci-pci.c
>> +++ b/drivers/usb/host/uhci-pci.c
>> @@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
>>   	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
>>   		uhci->oc_low = 1;
>>   
>> +	/* ZHAOXIN controllers report OverCurrent bit active off. */
>> +	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
>> +		uhci->oc_low = 1;
>> +
> 
> This should also go to the stable kernels, right?
> 
> But a new UHCI controller?  And this affects all of them? 
All UHCI controller of zhaoxin have this issue, And Hope this
patch go to stable kernel.Could I resubmit this patch with
modify subject typo mistake?

> Intel an issue here (as you mention in the changelog text)?
> 
I got this info from uhci driver comments in function uhci_pci_init.

"Intel controllers report the OverCurrent bit active on.
VIA controllers report it active off, so we'll adjust the
bit value.  (It's not standardized in the UHCI spec.)"

Best Regards,
weitao

> thanks,
> 
> greg k-h
> .

Re: [PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[Greg KH]

On Fri, Apr 21, 2023 at 01:14:46AM +0800, WeitaoWang-oc@zhaoxin.com wrote:
> On 2023/4/20 16:05, Greg KH wrote:
> > On Thu, Apr 20, 2023 at 07:14:45PM +0800, Weitao Wang wrote:
> > > Over Current condition is not standardized in the UHCI spec.
> > > Zhaoxin UHCI controllers report OverCurrent bit active off.
> > > Intel controllers report it active on, so we'll adjust the bit value.
> > > 
> > > Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> > 
> > Note, your Subject: line has odd characters in it, can you rewrite it to
> > just use ascii?  I think it's the '：' character which should be ':',
> > right?Yes,you are right. it's my typo mistake. I'll rewrite this patch. Thanks!
> 
> > > ---
> > >   drivers/usb/host/uhci-pci.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
> > > index 3592f757fe05..177e3c2aa287 100644
> > > --- a/drivers/usb/host/uhci-pci.c
> > > +++ b/drivers/usb/host/uhci-pci.c
> > > @@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
> > >   	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
> > >   		uhci->oc_low = 1;
> > > +	/* ZHAOXIN controllers report OverCurrent bit active off. */
> > > +	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
> > > +		uhci->oc_low = 1;
> > > +
> > 
> > This should also go to the stable kernels, right?
> > 
> > But a new UHCI controller?  And this affects all of them?
> All UHCI controller of zhaoxin have this issue, And Hope this
> patch go to stable kernel.Could I resubmit this patch with
> modify subject typo mistake?

Yes, please do so and add the needed cc: stable as is documented
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

thanks,

greg k-h

Re: [PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[Alan Stern]

On Thu, Apr 20, 2023 at 07:14:45PM +0800, Weitao Wang wrote:
> Over Current condition is not standardized in the UHCI spec.
> Zhaoxin UHCI controllers report OverCurrent bit active off.
> Intel controllers report it active on, so we'll adjust the bit value.

The last sentence is irrelevant.  You should say instead that the 
uhci-hcd driver needs to be told to expect the active-off behavior.

> 
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> ---
>  drivers/usb/host/uhci-pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
> index 3592f757fe05..177e3c2aa287 100644
> --- a/drivers/usb/host/uhci-pci.c
> +++ b/drivers/usb/host/uhci-pci.c
> @@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
>  	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
>  		uhci->oc_low = 1;
>  
> +	/* ZHAOXIN controllers report OverCurrent bit active off. */
> +	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
> +		uhci->oc_low = 1;

This is really just a matter of taste, but IMO it would be better to 
combine this comment and test with the preceding one.  Something like:

	/*
	 * Intel controllers report the OverCurrent bit active on.  VIA
	 * and ZHAOXIN controllers report it active off, so we'll adjust
	 * the bit value.  (It's not standardized in the UHCI spec.)
	 */
	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA ||
	    to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
		uhci->oc_low = 1;

Alan Stern

Re: [PATCH] UHCI：adjust zhaoxin UHCI controllers OverCurrent bit value

[WeitaoWang-oc]

On 2023/4/20 22:49, Alan Stern wrote:
> On Thu, Apr 20, 2023 at 07:14:45PM +0800, Weitao Wang wrote:
>> Over Current condition is not standardized in the UHCI spec.
>> Zhaoxin UHCI controllers report OverCurrent bit active off.
>> Intel controllers report it active on, so we'll adjust the bit value.
> 
> The last sentence is irrelevant.  You should say instead that the
> uhci-hcd driver needs to be told to expect the active-off behavior.

Okay, I'll change this description.Thanks!

>>
>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
>> ---
>>   drivers/usb/host/uhci-pci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
>> index 3592f757fe05..177e3c2aa287 100644
>> --- a/drivers/usb/host/uhci-pci.c
>> +++ b/drivers/usb/host/uhci-pci.c
>> @@ -126,6 +126,10 @@ static int uhci_pci_init(struct usb_hcd *hcd)
>>   	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA)
>>   		uhci->oc_low = 1;
>>   
>> +	/* ZHAOXIN controllers report OverCurrent bit active off. */
>> +	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
>> +		uhci->oc_low = 1;
> 
> This is really just a matter of taste, but IMO it would be better to
> combine this comment and test with the preceding one.  Something like:

This advice is good and the code looks more concise.
I'll change these code and description in next patch version.

Thanks,
Weitao
> 	/*
> 	 * Intel controllers report the OverCurrent bit active on.  VIA
> 	 * and ZHAOXIN controllers report it active off, so we'll adjust
> 	 * the bit value.  (It's not standardized in the UHCI spec.)
> 	 */
> 	if (to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_VIA ||
> 	    to_pci_dev(uhci_dev(uhci))->vendor == PCI_VENDOR_ID_ZHAOXIN)
> 		uhci->oc_low = 1;
> 
> Alan Stern
> .