2020-09-04 03:26:27

by Hamish Martin

[permalink] [raw]
Subject: [PATCH 0/2] usb: ohci: Per-port overcurrent protection

Add a dt-binding to select per-port overcurrent protection mode so handle
spurious overcurrent events from unconnected ports.

Hamish Martin (2):
usb: ohci: Add per-port overcurrent quirk
dt-bindings: usb: generic-ohci: Document per-port-overcurrent property

Documentation/devicetree/bindings/usb/generic-ohci.yaml | 5 +++++
drivers/usb/host/ohci-hcd.c | 4 ++++
drivers/usb/host/ohci-platform.c | 3 +++
drivers/usb/host/ohci.h | 1 +
4 files changed, 13 insertions(+)

--
2.28.0


2020-09-04 03:26:44

by Hamish Martin

[permalink] [raw]
Subject: [PATCH 2/2] dt-bindings: usb: generic-ohci: Document per-port-overcurrent property

OHCI overcurrent protection defaults to Global or "ganged" overcurrent
protection mode. This new property allows for the Individual Port
Over-current Protection to be selected when required.

Signed-off-by: Hamish Martin <[email protected]>
---
Documentation/devicetree/bindings/usb/generic-ohci.yaml | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/generic-ohci.yaml b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
index 2178bcc401bc..5a68a647d059 100644
--- a/Documentation/devicetree/bindings/usb/generic-ohci.yaml
+++ b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
@@ -70,6 +70,11 @@ properties:
description:
Overrides the detected port count

+ per-port-overcurrent:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Set this flag for per-port overcurrent protection mode
+
phys:
description: PHY specifier for the USB PHY

--
2.28.0

2020-09-04 03:26:52

by Hamish Martin

[permalink] [raw]
Subject: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

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 overcurrent 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 "ganged"
overcurrent protection mode. This leads to the spurious overcurrent
events affecting all ports in the hub.

Allow this to be rectified by specifying per-port overcurrent protection
mode via the device tree.

Signed-off-by: Hamish Martin <[email protected]>
---
drivers/usb/host/ohci-hcd.c | 4 ++++
drivers/usb/host/ohci-platform.c | 3 +++
drivers/usb/host/ohci.h | 1 +
3 files changed, 8 insertions(+)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index dd37e77dae00..01e3d75e29d9 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd *ohci)
val |= RH_A_NPS;
ohci_writel (ohci, val, &ohci->regs->roothub.a);
}
+ if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
+ val |= RH_A_OCPM;
+ 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);
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 4a8456f12a73..45e69ce4ef86 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -137,6 +137,9 @@ static int ohci_platform_probe(struct platform_device *dev)
if (of_property_read_bool(dev->dev.of_node, "no-big-frame-no"))
ohci->flags |= OHCI_QUIRK_FRAME_NO;

+ if (of_property_read_bool(dev->dev.of_node, "per-port-overcurrent"))
+ ohci->flags |= OHCI_QUIRK_PER_PORT_OC;
+
if (of_property_read_bool(dev->dev.of_node,
"remote-wakeup-connected"))
ohci->hc_control = OHCI_CTRL_RWC;
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index aac6285b37f8..9c2bc816246c 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -422,6 +422,7 @@ struct ohci_hcd {
#define OHCI_QUIRK_AMD_PREFETCH 0x400 /* pre-fetch for ISO transfer */
#define OHCI_QUIRK_GLOBAL_SUSPEND 0x800 /* must suspend ports */
#define OHCI_QUIRK_QEMU 0x1000 /* relax timing expectations */
+#define OHCI_QUIRK_PER_PORT_OC 0x2000 /* per-port overcurrent protection */

// there are also chip quirks/bugs in init logic

--
2.28.0

2020-09-04 15:46:39

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

On Fri, Sep 04, 2020 at 03:22:46PM +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 overcurrent 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 "ganged"
> overcurrent protection mode. This leads to the spurious overcurrent
> events affecting all ports in the hub.
>
> Allow this to be rectified by specifying per-port overcurrent protection
> mode via the device tree.
>
> Signed-off-by: Hamish Martin <[email protected]>
> ---
> drivers/usb/host/ohci-hcd.c | 4 ++++
> drivers/usb/host/ohci-platform.c | 3 +++
> drivers/usb/host/ohci.h | 1 +
> 3 files changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index dd37e77dae00..01e3d75e29d9 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd *ohci)
> val |= RH_A_NPS;
> ohci_writel (ohci, val, &ohci->regs->roothub.a);
> }
> + if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
> + val |= RH_A_OCPM;
> + ohci_writel(ohci, val, &ohci->regs->roothub.a);
> + }

I don't think this is right, for two reasons. First, isn't per-port
overcurrent protection the default?

Second, RH_A_OCPM doesn't do anything unless RH_A_NOCP is clear.

Alan Stern

> 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);
> diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
> index 4a8456f12a73..45e69ce4ef86 100644
> --- a/drivers/usb/host/ohci-platform.c
> +++ b/drivers/usb/host/ohci-platform.c
> @@ -137,6 +137,9 @@ static int ohci_platform_probe(struct platform_device *dev)
> if (of_property_read_bool(dev->dev.of_node, "no-big-frame-no"))
> ohci->flags |= OHCI_QUIRK_FRAME_NO;
>
> + if (of_property_read_bool(dev->dev.of_node, "per-port-overcurrent"))
> + ohci->flags |= OHCI_QUIRK_PER_PORT_OC;
> +
> if (of_property_read_bool(dev->dev.of_node,
> "remote-wakeup-connected"))
> ohci->hc_control = OHCI_CTRL_RWC;
> diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
> index aac6285b37f8..9c2bc816246c 100644
> --- a/drivers/usb/host/ohci.h
> +++ b/drivers/usb/host/ohci.h
> @@ -422,6 +422,7 @@ struct ohci_hcd {
> #define OHCI_QUIRK_AMD_PREFETCH 0x400 /* pre-fetch for ISO transfer */
> #define OHCI_QUIRK_GLOBAL_SUSPEND 0x800 /* must suspend ports */
> #define OHCI_QUIRK_QEMU 0x1000 /* relax timing expectations */
> +#define OHCI_QUIRK_PER_PORT_OC 0x2000 /* per-port overcurrent protection */
>
> // there are also chip quirks/bugs in init logic
>
> --
> 2.28.0
>

2020-09-07 01:51:51

by Hamish Martin

[permalink] [raw]
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

Hi Alan,

Thanks for your quick feedback. My replies are inline below.

On Fri, 2020-09-04 at 11:45 -0400, Alan Stern wrote:
> On Fri, Sep 04, 2020 at 03:22:46PM +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 overcurrent 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 "ganged"
> > overcurrent protection mode. This leads to the spurious overcurrent
> > events affecting all ports in the hub.
> >
> > Allow this to be rectified by specifying per-port overcurrent
> > protection
> > mode via the device tree.
> >
> > Signed-off-by: Hamish Martin <[email protected]>
> > ---
> > drivers/usb/host/ohci-hcd.c | 4 ++++
> > drivers/usb/host/ohci-platform.c | 3 +++
> > drivers/usb/host/ohci.h | 1 +
> > 3 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-
> > hcd.c
> > index dd37e77dae00..01e3d75e29d9 100644
> > --- a/drivers/usb/host/ohci-hcd.c
> > +++ b/drivers/usb/host/ohci-hcd.c
> > @@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd *ohci)
> > val |= RH_A_NPS;
> > ohci_writel (ohci, val, &ohci->regs->roothub.a);
> > }
> > + if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
> > + val |= RH_A_OCPM;
> > + ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > + }
>
> I don't think this is right, for two reasons. First, isn't per-port
> overcurrent protection the default?

Not as far as I understand the current code. Just above where my patch
applies, the RH_A_OCPM (and RH_A_PSM) bits are explicitly cleared in
'val' with:
val &= ~(RH_A_PSM | RH_A_OCPM);

This, coupled with the OHCI_QUIRK_HUB_POWER being set by virtue of the
'distrust_firmware' module param defaulting true, reads to me like the
default is for ganged over-current protection. And that is my
experience in this case.
If none of the quirks are selected then all of the fiddling with 'val'
never gets written to 'ohci->regs->roothub.a'

I'd appreciate your reading of that analysis because I'm by no means
sure of it.

>
> Second, RH_A_OCPM doesn't do anything unless RH_A_NOCP is clear.

Correct, and that is my mistake. If I progress to a v2 of this patch I
will update accordingly.

Thanks,
Hamish Martin

>
> Alan Stern
>
> > 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);
> > diff --git a/drivers/usb/host/ohci-platform.c
> > b/drivers/usb/host/ohci-platform.c
> > index 4a8456f12a73..45e69ce4ef86 100644
> > --- a/drivers/usb/host/ohci-platform.c
> > +++ b/drivers/usb/host/ohci-platform.c
> > @@ -137,6 +137,9 @@ static int ohci_platform_probe(struct
> > platform_device *dev)
> > if (of_property_read_bool(dev->dev.of_node, "no-big-
> > frame-no"))
> > ohci->flags |= OHCI_QUIRK_FRAME_NO;
> >
> > + if (of_property_read_bool(dev->dev.of_node, "per-port-
> > overcurrent"))
> > + ohci->flags |= OHCI_QUIRK_PER_PORT_OC;
> > +
> > if (of_property_read_bool(dev->dev.of_node,
> > "remote-wakeup-connected"))
> > ohci->hc_control = OHCI_CTRL_RWC;
> > diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
> > index aac6285b37f8..9c2bc816246c 100644
> > --- a/drivers/usb/host/ohci.h
> > +++ b/drivers/usb/host/ohci.h
> > @@ -422,6 +422,7 @@ struct ohci_hcd {
> > #define OHCI_QUIRK_AMD_PREFETCH 0x400 /*
> > pre-fetch for ISO transfer */
> > #define OHCI_QUIRK_GLOBAL_SUSPEND 0x800 /* must
> > suspend ports */
> > #define OHCI_QUIRK_QEMU 0x1000 /*
> > relax timing expectations */
> > +#define OHCI_QUIRK_PER_PORT_OC 0x2000 /*
> > per-port overcurrent protection */
> >
> > // there are also chip quirks/bugs in init logic
> >
> > --
> > 2.28.0
> >

2020-09-07 15:11:11

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

On Mon, Sep 07, 2020 at 01:50:10AM +0000, Hamish Martin wrote:
> Hi Alan,
>
> Thanks for your quick feedback. My replies are inline below.
>
> On Fri, 2020-09-04 at 11:45 -0400, Alan Stern wrote:
> > On Fri, Sep 04, 2020 at 03:22:46PM +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 overcurrent 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 "ganged"
> > > overcurrent protection mode. This leads to the spurious overcurrent
> > > events affecting all ports in the hub.
> > >
> > > Allow this to be rectified by specifying per-port overcurrent
> > > protection
> > > mode via the device tree.
> > >
> > > Signed-off-by: Hamish Martin <[email protected]>
> > > ---
> > > drivers/usb/host/ohci-hcd.c | 4 ++++
> > > drivers/usb/host/ohci-platform.c | 3 +++
> > > drivers/usb/host/ohci.h | 1 +
> > > 3 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-
> > > hcd.c
> > > index dd37e77dae00..01e3d75e29d9 100644
> > > --- a/drivers/usb/host/ohci-hcd.c
> > > +++ b/drivers/usb/host/ohci-hcd.c
> > > @@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd *ohci)
> > > val |= RH_A_NPS;
> > > ohci_writel (ohci, val, &ohci->regs->roothub.a);
> > > }
> > > + if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
> > > + val |= RH_A_OCPM;
> > > + ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > > + }
> >
> > I don't think this is right, for two reasons. First, isn't per-port
> > overcurrent protection the default?
>
> Not as far as I understand the current code. Just above where my patch
> applies, the RH_A_OCPM (and RH_A_PSM) bits are explicitly cleared in
> 'val' with:
> val &= ~(RH_A_PSM | RH_A_OCPM);
>
> This, coupled with the OHCI_QUIRK_HUB_POWER being set by virtue of the
> 'distrust_firmware' module param defaulting true, reads to me like the
> default is for ganged over-current protection. And that is my
> experience in this case.

You're right about that. I hadn't noticed before; it makes little sense
to have a quirk that defaults to true.

It's not easy to tell the full story from the kernel history; that
module parameter predates the Git era. I did learn that it was modified
in 2.6.3-rc3 and goes back even farther: see

https://marc.info/?l=linux-usb-devel&m=110628457424684&w=2

> If none of the quirks are selected then all of the fiddling with 'val'
> never gets written to 'ohci->regs->roothub.a'
>
> I'd appreciate your reading of that analysis because I'm by no means
> sure of it.
>
> >
> > Second, RH_A_OCPM doesn't do anything unless RH_A_NOCP is clear.
>
> Correct, and that is my mistake. If I progress to a v2 of this patch I
> will update accordingly.

Shall we try changing the parameter's default value? The USB subsystem
is a lot more mature and reliable now than it was back in 2004.

Alan Stern

2020-09-07 22:29:53

by Hamish Martin

[permalink] [raw]
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

On Mon, 2020-09-07 at 10:59 -0400, [email protected] wrote:
> On Mon, Sep 07, 2020 at 01:50:10AM +0000, Hamish Martin wrote:
> > Hi Alan,
> >
> > Thanks for your quick feedback. My replies are inline below.
> >
> > On Fri, 2020-09-04 at 11:45 -0400, Alan Stern wrote:
> > > On Fri, Sep 04, 2020 at 03:22:46PM +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 overcurrent 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 "ganged"
> > > > overcurrent protection mode. This leads to the spurious
> > > > overcurrent
> > > > events affecting all ports in the hub.
> > > >
> > > > Allow this to be rectified by specifying per-port overcurrent
> > > > protection
> > > > mode via the device tree.
> > > >
> > > > Signed-off-by: Hamish Martin <[email protected]
> > > > >
> > > > ---
> > > > drivers/usb/host/ohci-hcd.c | 4 ++++
> > > > drivers/usb/host/ohci-platform.c | 3 +++
> > > > drivers/usb/host/ohci.h | 1 +
> > > > 3 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/host/ohci-hcd.c
> > > > b/drivers/usb/host/ohci-
> > > > hcd.c
> > > > index dd37e77dae00..01e3d75e29d9 100644
> > > > --- a/drivers/usb/host/ohci-hcd.c
> > > > +++ b/drivers/usb/host/ohci-hcd.c
> > > > @@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd
> > > > *ohci)
> > > > val |= RH_A_NPS;
> > > > ohci_writel (ohci, val, &ohci->regs-
> > > > >roothub.a);
> > > > }
> > > > + if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
> > > > + val |= RH_A_OCPM;
> > > > + ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > > > + }
> > >
> > > I don't think this is right, for two reasons. First, isn't per-
> > > port
> > > overcurrent protection the default?
> >
> > Not as far as I understand the current code. Just above where my
> > patch
> > applies, the RH_A_OCPM (and RH_A_PSM) bits are explicitly cleared
> > in
> > 'val' with:
> > val &= ~(RH_A_PSM | RH_A_OCPM);
> >
> > This, coupled with the OHCI_QUIRK_HUB_POWER being set by virtue of
> > the
> > 'distrust_firmware' module param defaulting true, reads to me like
> > the
> > default is for ganged over-current protection. And that is my
> > experience in this case.
>
> You're right about that. I hadn't noticed before; it makes little
> sense
> to have a quirk that defaults to true.
>
> It's not easy to tell the full story from the kernel history; that
> module parameter predates the Git era. I did learn that it was
> modified
> in 2.6.3-rc3 and goes back even farther: see
>
> https://marc.info/?l=linux-usb-devel&m=110628457424684&w=2
>
> > If none of the quirks are selected then all of the fiddling with
> > 'val'
> > never gets written to 'ohci->regs->roothub.a'
> >
> > I'd appreciate your reading of that analysis because I'm by no
> > means
> > sure of it.
> >
> > >
> > > Second, RH_A_OCPM doesn't do anything unless RH_A_NOCP is clear.
> >
> > Correct, and that is my mistake. If I progress to a v2 of this
> > patch I
> > will update accordingly.
>
> Shall we try changing the parameter's default value? The USB
> subsystem
> is a lot more mature and reliable now than it was back in 2004.

That doesn't really help me in my particular case. I tried turning the
param off and that just leads to the roothub.a reg not being modified
at all (and ganged over-current protection being left in place).

So, I guess I'm still back to my original idea of adding a new quirk
(perhaps quirk is not the best name for it in this case) that allows
the per-port over-current to be selected.
If you would rather that this not be a quirk and I rework the code such
that if no other quirks are selected then we configure for per-port
over-current as the default then I can do that too. If you expect per-
port over-current to be the default then explicit code that enforces
that might be best.

What's the best approach?

Thanks,
Hamish M

>
> Alan Stern

2020-09-08 15:09:03

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk

On Mon, Sep 07, 2020 at 10:28:26PM +0000, Hamish Martin wrote:
> On Mon, 2020-09-07 at 10:59 -0400, [email protected] wrote:
> > On Mon, Sep 07, 2020 at 01:50:10AM +0000, Hamish Martin wrote:
> > > Hi Alan,
> > >
> > > Thanks for your quick feedback. My replies are inline below.
> > >
> > > On Fri, 2020-09-04 at 11:45 -0400, Alan Stern wrote:
> > > > On Fri, Sep 04, 2020 at 03:22:46PM +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 overcurrent 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 "ganged"
> > > > > overcurrent protection mode. This leads to the spurious
> > > > > overcurrent
> > > > > events affecting all ports in the hub.
> > > > >
> > > > > Allow this to be rectified by specifying per-port overcurrent
> > > > > protection
> > > > > mode via the device tree.
> > > > >
> > > > > Signed-off-by: Hamish Martin <[email protected]
> > > > > >
> > > > > ---
> > > > > drivers/usb/host/ohci-hcd.c | 4 ++++
> > > > > drivers/usb/host/ohci-platform.c | 3 +++
> > > > > drivers/usb/host/ohci.h | 1 +
> > > > > 3 files changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/host/ohci-hcd.c
> > > > > b/drivers/usb/host/ohci-
> > > > > hcd.c
> > > > > index dd37e77dae00..01e3d75e29d9 100644
> > > > > --- a/drivers/usb/host/ohci-hcd.c
> > > > > +++ b/drivers/usb/host/ohci-hcd.c
> > > > > @@ -687,6 +687,10 @@ static int ohci_run (struct ohci_hcd
> > > > > *ohci)
> > > > > val |= RH_A_NPS;
> > > > > ohci_writel (ohci, val, &ohci->regs-
> > > > > >roothub.a);
> > > > > }
> > > > > + if (ohci->flags & OHCI_QUIRK_PER_PORT_OC) {
> > > > > + val |= RH_A_OCPM;
> > > > > + ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > > > > + }
> > > >
> > > > I don't think this is right, for two reasons. First, isn't per-
> > > > port
> > > > overcurrent protection the default?
> > >
> > > Not as far as I understand the current code. Just above where my
> > > patch
> > > applies, the RH_A_OCPM (and RH_A_PSM) bits are explicitly cleared
> > > in
> > > 'val' with:
> > > val &= ~(RH_A_PSM | RH_A_OCPM);
> > >
> > > This, coupled with the OHCI_QUIRK_HUB_POWER being set by virtue of
> > > the
> > > 'distrust_firmware' module param defaulting true, reads to me like
> > > the
> > > default is for ganged over-current protection. And that is my
> > > experience in this case.
> >
> > You're right about that. I hadn't noticed before; it makes little
> > sense
> > to have a quirk that defaults to true.
> >
> > It's not easy to tell the full story from the kernel history; that
> > module parameter predates the Git era. I did learn that it was
> > modified
> > in 2.6.3-rc3 and goes back even farther: see
> >
> > https://marc.info/?l=linux-usb-devel&m=110628457424684&w=2
> >
> > > If none of the quirks are selected then all of the fiddling with
> > > 'val'
> > > never gets written to 'ohci->regs->roothub.a'
> > >
> > > I'd appreciate your reading of that analysis because I'm by no
> > > means
> > > sure of it.
> > >
> > > >
> > > > Second, RH_A_OCPM doesn't do anything unless RH_A_NOCP is clear.
> > >
> > > Correct, and that is my mistake. If I progress to a v2 of this
> > > patch I
> > > will update accordingly.
> >
> > Shall we try changing the parameter's default value? The USB
> > subsystem
> > is a lot more mature and reliable now than it was back in 2004.
>
> That doesn't really help me in my particular case. I tried turning the
> param off and that just leads to the roothub.a reg not being modified
> at all (and ganged over-current protection being left in place).
>
> So, I guess I'm still back to my original idea of adding a new quirk
> (perhaps quirk is not the best name for it in this case) that allows
> the per-port over-current to be selected.
> If you would rather that this not be a quirk and I rework the code such
> that if no other quirks are selected then we configure for per-port
> over-current as the default then I can do that too. If you expect per-
> port over-current to be the default then explicit code that enforces
> that might be best.
>
> What's the best approach?

In the absence of any evidence to the contrary, I think we should make
per-port overcurrent handling be the default. So yes, add code which
does that.

Alan Stern