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@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
> >
