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

From: Hamish Martin <Hamish.Martin@alliedtelesis.co.nz>
To: "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH 1/2] usb: ohci: Add per-port overcurrent quirk
Date: Mon, 7 Sep 2020 01:50:10 +0000	[thread overview]
Message-ID: <9ba7b4dda9ef40e3c4c9b3f1c33075e04601ef61.camel@alliedtelesis.co.nz> (raw)
In-Reply-To: <20200904154517.GB694058@rowland.harvard.edu>

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