usb: chipidea: imx: Allow OC polarity active low

* usb: chipidea: imx: Allow OC polarity active low
@ 2018-12-03  3:06 Peter Chen
  0 siblings, 0 replies; 5+ messages in thread
From: Peter Chen @ 2018-12-03  3:06 UTC (permalink / raw)
  To: Jun Li, mstarr, linux-usb; +Cc: u.kleine-koenig

> 
> > -----Original Message-----
> > From: Matthew Starr <mstarr@hedonline.com>
> > Sent: 2018年11月30日 23:09
> > To: PETER CHEN <peter.chen@nxp.com>; linux-usb@vger.kernel.org; Jun Li
> > <jun.li@nxp.com>
> > Subject: RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low
> >
> > > -----Original Message-----
> > > From: PETER CHEN [mailto:peter.chen@nxp.com]
> > > Sent: Thursday, November 29, 2018 9:30 PM
> > >
> > > >
> > > > The implementation for setting the over current polarity has
> > > > always been the over- current-active-high property.  The problem
> > > > with this is there is no way to enable over current active low
> > > > polarity since the default state of the register bit that controls
> > > > the over current polarity is a 0 which means active high.  This
> > > > value never actually did anything since active high is already the
> > > > default state. Also it is not used in any device tree source in
> > > > the kernel.  The default register bit
> > state was verified by checking the i.MX6Q and i.MX7Dual reference manuals.
> > > >
> > > > The change replaces the over-current-active-high property with the
> > > > over- current- active-low property.  Without this property the
> > > > over current polarity will be active high by default like it always has been.
> > > > With the property the over current polarity will be active low.
> > > >
> > >
> > > Would you please check it?  Why it is opposite for your patch and Matthew's?
> >
> > The i.MX6DQ Reference Manual, the i.MX6UL Reference Manual, and the
> > i.MX7D Reference Manual all show that the default reset value of the
> > OVER_CUR_POL bit in the USBNC_USB_OTG_CTRL register is 0.  Here is the
> > reference manual's description for the OVER_CUR_POL bit:
> >
> > OTG Polarity of Overcurrent
> > The polarity of OTG port overcurrent event
> > 1 Low active (low on this signal represents an overcurrent condition)
> > 0 High active (high on this signal represents an overcurrent
> > condition)
> >
> > I verified this on a custom made product using an i.MX6UL.  The
> > overcurrent detection on this product is active low triggered, and
> > only through the modification of the driver enabling active low
> > triggering of the overcurrent functionality was I able to see the USB detect the
> overcurrent and reset itself.
> >
> > I think this has been missed up till now because the bit next to the
> > OVER_CUR_POL bit is the PWR_POL which has the exact inverse logic with
> > a value of 1 meaning high enable and 0 meaning low enable.  Also I
> > rarely have seen the USB interface go into an overcurrent situation so
> > maybe no one ever caught this, but while forcing it during hardware testing the
> issue was discovered.
> 
> The intention of use active-high for OC was to make the most of users don't need
> to add the property, that is, by default, OC is active low, this is also matching the
> reality in most cases I think. Meanwhile to avoid break those existing HW before
> this property was introduced, the usbmisc driver wasn't updated to be OC active low,
> but hope the later SoCs correct this by a new init function.
> 
> Now we have the problem of the default value in opposite state(OC is enabled and
> polarity is active high), seems there is no easy way to resolve this, considering the
> existing property hasn't been used, change it to be "active low"
> like this may be the easiest way to resolve it.
> 
> For those platforms without any OC properties added, I don’t think all of them were
> using active high, may be the iomux wasn't configured to be OC input, so the
> default state(low) couldn’t generate false alarm.
> 

Hi Matt, Uwe, Jun,

Thanks for commenting. I suggest resetting all OC settings according to dts value and
keep current property "over-current-active-high" since most of users doesn't need to set
property. See my patch for detail.

Peter

> Li Jun
> 
> >
> > Matt Starr
> >
> > >
> > > > Signed-off-by: Matthew Starr <mstarr@hedonline.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt |  4 ++--
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c                     |  2 +-
> > > >  drivers/usb/chipidea/usbmisc_imx.c                     | 10 ++++++----
> > > >  3 files changed, 9 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > index 529e51879fb2..3dee58037839 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > @@ -87,8 +87,8 @@ i.mx specific properties
> > > >  - fsl,usbmisc: phandler of non-core register device, with one
> > > >    argument that indicate usb controller index
> > > >  - disable-over-current: disable over current detect
> > > > -- over-current-active-high: over current signal polarity is high
> > > > active,
> > > > -  typically over current signal polarity is low active.
> > > > +- over-current-active-low: over current signal polarity is low
> > > > +active,
> > > > +  the default signal polarity is high active.
> > > >  - external-vbus-divider: enables off-chip resistor divider for
> > > > Vbus
> > > >
> > > >  Example:
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 09b37c0d075d..f7069544fb42 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -135,7 +135,7 @@ static struct imx_usbmisc_data
> > > > *usbmisc_get_init_data(struct device *dev)
> > > >  	if (of_find_property(np, "disable-over-current", NULL))
> > > >  		data->disable_oc = 1;
> > > >
> > > > -	if (of_find_property(np, "over-current-active-high", NULL))
> > > > +	if (of_find_property(np, "over-current-active-low", NULL))
> > > >  		data->oc_polarity = 1;
> > > >
> > > >  	if (of_find_property(np, "external-vbus-divider", NULL)) diff
> > > > --git a/drivers/usb/chipidea/usbmisc_imx.c
> > > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > > index def80ff547e4..39223708e859 100644
> > > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > > @@ -341,8 +341,9 @@ static int usbmisc_imx6q_init(struct
> > imx_usbmisc_data *data)
> > > >  	if (data->disable_oc) {
> > > >  		reg |= MX6_BM_OVER_CUR_DIS;
> > > >  	} else if (data->oc_polarity == 1) {
> > > > -		/* High active */
> > > > -		reg &= ~(MX6_BM_OVER_CUR_DIS |
> > MX6_BM_OVER_CUR_POLARITY);
> > > > +		/* Low active */
> > > > +		reg &= ~(MX6_BM_OVER_CUR_DIS);
> > > > +		reg |= MX6_BM_OVER_CUR_POLARITY;
> > > >  	} else {
> > > >  		reg &= ~(MX6_BM_OVER_CUR_DIS);
> > > >  	}
> > > > @@ -445,8 +446,9 @@ static int usbmisc_imx7d_init(struct
> > imx_usbmisc_data *data)
> > > >  	if (data->disable_oc) {
> > > >  		reg |= MX6_BM_OVER_CUR_DIS;
> > > >  	} else if (data->oc_polarity == 1) {
> > > > -		/* High active */
> > > > -		reg &= ~(MX6_BM_OVER_CUR_DIS |
> > MX6_BM_OVER_CUR_POLARITY);
> > > > +		/* Low active */
> > > > +		reg &= ~(MX6_BM_OVER_CUR_DIS);
> > > > +		reg |= MX6_BM_OVER_CUR_POLARITY;
> > > >  	}
> > > >  	writel(reg, usbmisc->base);
> > > >
> > > > --
> > > > 2.17.1

^ permalink raw reply	[flat|nested] 5+ messages in thread