* [PATCH 0/2] usb: ohci: Per-port overcurrent protection
@ 2020-09-04 3:22 Hamish Martin
2020-09-04 3:22 ` [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk Hamish Martin
2020-09-04 3:22 ` [PATCH 2/2] dt-bindings: usb: generic-ohci: Document per-port-overcurrent property Hamish Martin
0 siblings, 2 replies; 8+ messages in thread
From: Hamish Martin @ 2020-09-04 3:22 UTC (permalink / raw)
To: gregkh, robh+dt, stern; +Cc: linux-usb, devicetree, linux-kernel, Hamish Martin
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-04 3:22 [PATCH 0/2] usb: ohci: Per-port overcurrent protection Hamish Martin
@ 2020-09-04 3:22 ` Hamish Martin
2020-09-04 15:45 ` Alan Stern
2020-09-04 3:22 ` [PATCH 2/2] dt-bindings: usb: generic-ohci: Document per-port-overcurrent property Hamish Martin
1 sibling, 1 reply; 8+ messages in thread
From: Hamish Martin @ 2020-09-04 3:22 UTC (permalink / raw)
To: gregkh, robh+dt, stern; +Cc: linux-usb, devicetree, 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 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 <hamish.martin@alliedtelesis.co.nz>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] dt-bindings: usb: generic-ohci: Document per-port-overcurrent property
2020-09-04 3:22 [PATCH 0/2] usb: ohci: Per-port overcurrent protection Hamish Martin
2020-09-04 3:22 ` [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk Hamish Martin
@ 2020-09-04 3:22 ` Hamish Martin
1 sibling, 0 replies; 8+ messages in thread
From: Hamish Martin @ 2020-09-04 3:22 UTC (permalink / raw)
To: gregkh, robh+dt, stern; +Cc: linux-usb, devicetree, linux-kernel, Hamish Martin
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 <hamish.martin@alliedtelesis.co.nz>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-04 3:22 ` [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk Hamish Martin
@ 2020-09-04 15:45 ` Alan Stern
2020-09-07 1:50 ` Hamish Martin
0 siblings, 1 reply; 8+ messages in thread
From: Alan Stern @ 2020-09-04 15:45 UTC (permalink / raw)
To: Hamish Martin; +Cc: gregkh, robh+dt, linux-usb, devicetree, linux-kernel
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 <hamish.martin@alliedtelesis.co.nz>
> ---
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-04 15:45 ` Alan Stern
@ 2020-09-07 1:50 ` Hamish Martin
2020-09-07 14:59 ` stern
0 siblings, 1 reply; 8+ messages in thread
From: Hamish Martin @ 2020-09-07 1:50 UTC (permalink / raw)
To: stern; +Cc: linux-kernel, gregkh, robh+dt, linux-usb, devicetree
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 <hamish.martin@alliedtelesis.co.nz>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-07 1:50 ` Hamish Martin
@ 2020-09-07 14:59 ` stern
2020-09-07 22:28 ` Hamish Martin
0 siblings, 1 reply; 8+ messages in thread
From: stern @ 2020-09-07 14:59 UTC (permalink / raw)
To: Hamish Martin; +Cc: linux-kernel, gregkh, robh+dt, linux-usb, devicetree
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 <hamish.martin@alliedtelesis.co.nz>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-07 14:59 ` stern
@ 2020-09-07 22:28 ` Hamish Martin
2020-09-08 14:32 ` stern
0 siblings, 1 reply; 8+ messages in thread
From: Hamish Martin @ 2020-09-07 22:28 UTC (permalink / raw)
To: stern; +Cc: linux-kernel, gregkh, robh+dt, linux-usb, devicetree
On Mon, 2020-09-07 at 10:59 -0400, stern@rowland.harvard.edu 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 <hamish.martin@alliedtelesis.co.nz
> > > > >
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
2020-09-07 22:28 ` Hamish Martin
@ 2020-09-08 14:32 ` stern
0 siblings, 0 replies; 8+ messages in thread
From: stern @ 2020-09-08 14:32 UTC (permalink / raw)
To: Hamish Martin; +Cc: linux-kernel, gregkh, robh+dt, linux-usb, devicetree
On Mon, Sep 07, 2020 at 10:28:26PM +0000, Hamish Martin wrote:
> On Mon, 2020-09-07 at 10:59 -0400, stern@rowland.harvard.edu 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 <hamish.martin@alliedtelesis.co.nz
> > > > > >
> > > > > ---
> > > > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-08 15:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 3:22 [PATCH 0/2] usb: ohci: Per-port overcurrent protection Hamish Martin
2020-09-04 3:22 ` [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk Hamish Martin
2020-09-04 15:45 ` Alan Stern
2020-09-07 1:50 ` Hamish Martin
2020-09-07 14:59 ` stern
2020-09-07 22:28 ` Hamish Martin
2020-09-08 14:32 ` stern
2020-09-04 3:22 ` [PATCH 2/2] dt-bindings: usb: generic-ohci: Document per-port-overcurrent property 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).