linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: ohci: Default to per-port over-current protection
@ 2020-09-09  3:57 Hamish Martin
  2020-09-09 14:05 ` Alan Stern
  0 siblings, 1 reply; 3+ messages in thread
From: Hamish Martin @ 2020-09-09  3:57 UTC (permalink / raw)
  To: stern, gregkh; +Cc: linux-usb, linux-kernel, Hamish Martin

Some integrated OHCI controller hubs do not expose all ports of the hub
to pins on the SoC. In some cases the unconnected ports generate
spurious over-current events. For example the Broadcom 56060/Ranger 2 SoC
contains a nominally 3 port hub but only the first port is wired.

Default behaviour for ohci-platform driver is to use global over-current
protection mode (AKA "ganged"). This leads to the spurious over-current
events affecting all ports in the hub.

We now alter the default to use per-port over-current protection.

This patch results in the following configuration changes depending
on quirks:
- For quirk OHCI_QUIRK_SUPERIO no changes. These systems remain set up
  for ganged power switching and no over-current protection.
- For quirk OHCI_QUIRK_AMD756 or OHCI_QUIRK_HUB_POWER power switching
  remains at none, while over-current protection is now guaranteed to be
  set to per-port rather than the previous behaviour where it was either
  none or global over-current protection depending on the value at
  function entry.

Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
 drivers/usb/host/ohci-hcd.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index dd37e77dae00..8ab81f6ab150 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -673,20 +673,25 @@ static int ohci_run (struct ohci_hcd *ohci)
 
 	/* handle root hub init quirks ... */
 	val = roothub_a (ohci);
-	val &= ~(RH_A_PSM | RH_A_OCPM);
+	/* Configure for per-port over-current protection by default */
+	val &= ~RH_A_NOCP;
+	val |= RH_A_OCPM;
 	if (ohci->flags & OHCI_QUIRK_SUPERIO) {
-		/* NSC 87560 and maybe others */
+		/* NSC 87560 and maybe others.
+		 * Ganged power switching, no over-current protection.
+		 */
 		val |= RH_A_NOCP;
-		val &= ~(RH_A_POTPGT | RH_A_NPS);
-		ohci_writel (ohci, val, &ohci->regs->roothub.a);
+		val &= ~(RH_A_POTPGT | RH_A_NPS | RH_A_PSM | RH_A_OCPM);
 	} else if ((ohci->flags & OHCI_QUIRK_AMD756) ||
 			(ohci->flags & OHCI_QUIRK_HUB_POWER)) {
 		/* hub power always on; required for AMD-756 and some
-		 * Mac platforms.  ganged overcurrent reporting, if any.
+		 * Mac platforms.
 		 */
+		val &= ~RH_A_PSM;
 		val |= RH_A_NPS;
-		ohci_writel (ohci, val, &ohci->regs->roothub.a);
 	}
+	ohci_writel(ohci, val, &ohci->regs->roothub.a);
+
 	ohci_writel (ohci, RH_HS_LPSC, &ohci->regs->roothub.status);
 	ohci_writel (ohci, (val & RH_A_NPS) ? 0 : RH_B_PPCM,
 						&ohci->regs->roothub.b);
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] usb: ohci: Default to per-port over-current protection
  2020-09-09  3:57 [PATCH] usb: ohci: Default to per-port over-current protection Hamish Martin
@ 2020-09-09 14:05 ` Alan Stern
  2020-09-10 21:10   ` Hamish Martin
  0 siblings, 1 reply; 3+ messages in thread
From: Alan Stern @ 2020-09-09 14:05 UTC (permalink / raw)
  To: Hamish Martin; +Cc: gregkh, linux-usb, linux-kernel

On Wed, Sep 09, 2020 at 03:57:34PM +1200, Hamish Martin wrote:
> Some integrated OHCI controller hubs do not expose all ports of the hub
> to pins on the SoC. In some cases the unconnected ports generate
> spurious over-current events. For example the Broadcom 56060/Ranger 2 SoC
> contains a nominally 3 port hub but only the first port is wired.
> 
> Default behaviour for ohci-platform driver is to use global over-current
> protection mode (AKA "ganged"). This leads to the spurious over-current
> events affecting all ports in the hub.
> 
> We now alter the default to use per-port over-current protection.
> 
> This patch results in the following configuration changes depending
> on quirks:
> - For quirk OHCI_QUIRK_SUPERIO no changes. These systems remain set up
>   for ganged power switching and no over-current protection.

How about changing the quirk name to something more meaningful, such as 
OHCI_QUIRK_GANGED_POWER_NO_OVERCURRENT?

> - For quirk OHCI_QUIRK_AMD756 or OHCI_QUIRK_HUB_POWER power switching
>   remains at none, while over-current protection is now guaranteed to be
>   set to per-port rather than the previous behaviour where it was either
>   none or global over-current protection depending on the value at
>   function entry.

Also consider renaming OHCI_QUIRK_HUB_POWER to something like 
OHCI_QUIRK_PORT_POWER_ALWAYS_ON.

> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
> ---
>  drivers/usb/host/ohci-hcd.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index dd37e77dae00..8ab81f6ab150 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -673,20 +673,25 @@ static int ohci_run (struct ohci_hcd *ohci)
>  
>  	/* handle root hub init quirks ... */
>  	val = roothub_a (ohci);
> -	val &= ~(RH_A_PSM | RH_A_OCPM);
> +	/* Configure for per-port over-current protection by default */
> +	val &= ~RH_A_NOCP;
> +	val |= RH_A_OCPM;
>  	if (ohci->flags & OHCI_QUIRK_SUPERIO) {
> -		/* NSC 87560 and maybe others */
> +		/* NSC 87560 and maybe others.
> +		 * Ganged power switching, no over-current protection.
> +		 */
>  		val |= RH_A_NOCP;
> -		val &= ~(RH_A_POTPGT | RH_A_NPS);
> -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
> +		val &= ~(RH_A_POTPGT | RH_A_NPS | RH_A_PSM | RH_A_OCPM);
>  	} else if ((ohci->flags & OHCI_QUIRK_AMD756) ||
>  			(ohci->flags & OHCI_QUIRK_HUB_POWER)) {
>  		/* hub power always on; required for AMD-756 and some
> -		 * Mac platforms.  ganged overcurrent reporting, if any.
> +		 * Mac platforms.
>  		 */
> +		val &= ~RH_A_PSM;
>  		val |= RH_A_NPS;

PSM is ignored when NPS is on.  You needn't bother to set it.

> -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
>  	}
> +	ohci_writel(ohci, val, &ohci->regs->roothub.a);
> +
>  	ohci_writel (ohci, RH_HS_LPSC, &ohci->regs->roothub.status);
>  	ohci_writel (ohci, (val & RH_A_NPS) ? 0 : RH_B_PPCM,
>  						&ohci->regs->roothub.b);

You didn't actually change the default: distrust_firmware is still 
initialized to true.

Alan Stern

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] usb: ohci: Default to per-port over-current protection
  2020-09-09 14:05 ` Alan Stern
@ 2020-09-10 21:10   ` Hamish Martin
  0 siblings, 0 replies; 3+ messages in thread
From: Hamish Martin @ 2020-09-10 21:10 UTC (permalink / raw)
  To: stern; +Cc: linux-kernel, gregkh, linux-usb

On Wed, 2020-09-09 at 10:05 -0400, Alan Stern wrote:
> On Wed, Sep 09, 2020 at 03:57:34PM +1200, Hamish Martin wrote:
> > Some integrated OHCI controller hubs do not expose all ports of the
> > hub
> > to pins on the SoC. In some cases the unconnected ports generate
> > spurious over-current events. For example the Broadcom 56060/Ranger
> > 2 SoC
> > contains a nominally 3 port hub but only the first port is wired.
> > 
> > Default behaviour for ohci-platform driver is to use global over-
> > current
> > protection mode (AKA "ganged"). This leads to the spurious over-
> > current
> > events affecting all ports in the hub.
> > 
> > We now alter the default to use per-port over-current protection.
> > 
> > This patch results in the following configuration changes depending
> > on quirks:
> > - For quirk OHCI_QUIRK_SUPERIO no changes. These systems remain set
> > up
> >   for ganged power switching and no over-current protection.
> 
> How about changing the quirk name to something more meaningful, such
> as 
> OHCI_QUIRK_GANGED_POWER_NO_OVERCURRENT?
I'll look at that in a separate patch later. The code I'm modifying
seems to be cloned in various drivers so perhaps that needs to be
centralised as the next step? See refs to OHCI_QUIRK_SUPERIO in u132-
hcd.c and ftdi-elan.c.

> 
> > - For quirk OHCI_QUIRK_AMD756 or OHCI_QUIRK_HUB_POWER power
> > switching
> >   remains at none, while over-current protection is now guaranteed
> > to be
> >   set to per-port rather than the previous behaviour where it was
> > either
> >   none or global over-current protection depending on the value at
> >   function entry.
> 
> Also consider renaming OHCI_QUIRK_HUB_POWER to something like 
> OHCI_QUIRK_PORT_POWER_ALWAYS_ON.
Ditto the above comment.

> 
> > Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
> > ---
> >  drivers/usb/host/ohci-hcd.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-
> > hcd.c
> > index dd37e77dae00..8ab81f6ab150 100644
> > --- a/drivers/usb/host/ohci-hcd.c
> > +++ b/drivers/usb/host/ohci-hcd.c
> > @@ -673,20 +673,25 @@ static int ohci_run (struct ohci_hcd *ohci)
> >  
> >  	/* handle root hub init quirks ... */
> >  	val = roothub_a (ohci);
> > -	val &= ~(RH_A_PSM | RH_A_OCPM);
> > +	/* Configure for per-port over-current protection by default */
> > +	val &= ~RH_A_NOCP;
> > +	val |= RH_A_OCPM;
> >  	if (ohci->flags & OHCI_QUIRK_SUPERIO) {
> > -		/* NSC 87560 and maybe others */
> > +		/* NSC 87560 and maybe others.
> > +		 * Ganged power switching, no over-current protection.
> > +		 */
> >  		val |= RH_A_NOCP;
> > -		val &= ~(RH_A_POTPGT | RH_A_NPS);
> > -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
> > +		val &= ~(RH_A_POTPGT | RH_A_NPS | RH_A_PSM |
> > RH_A_OCPM);
> >  	} else if ((ohci->flags & OHCI_QUIRK_AMD756) ||
> >  			(ohci->flags & OHCI_QUIRK_HUB_POWER)) {
> >  		/* hub power always on; required for AMD-756 and some
> > -		 * Mac platforms.  ganged overcurrent reporting, if
> > any.
> > +		 * Mac platforms.
> >  		 */
> > +		val &= ~RH_A_PSM;
> >  		val |= RH_A_NPS;
> 
> PSM is ignored when NPS is on.  You needn't bother to set it.
OK, I'll remove that.

> 
> > -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
> >  	}
> > +	ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > +
> >  	ohci_writel (ohci, RH_HS_LPSC, &ohci->regs->roothub.status);
> >  	ohci_writel (ohci, (val & RH_A_NPS) ? 0 : RH_B_PPCM,
> >  						&ohci->regs-
> > >roothub.b);
> 
> You didn't actually change the default: distrust_firmware is still 
> initialized to true.
Yes. I didn't need to for my system but I understand you want to do the
change. I'll make that a second patch in this series to keep the
logical changes quite separate in case of regression.

v2 (3?) series coming soon.

Thanks,
Hamish M.

> 
> Alan Stern

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-10 21:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  3:57 [PATCH] usb: ohci: Default to per-port over-current protection Hamish Martin
2020-09-09 14:05 ` Alan Stern
2020-09-10 21:10   ` Hamish Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).