* [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
* 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
* [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
end of thread, other threads:[~2020-09-08 15:07 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).