> > > -----Original Message----- > > From: Matthew Starr <mstarr@xxxxxxxxxxxxx> > > Sent: 2018年11月30日 23:09 > > To: PETER CHEN <peter.chen@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jun Li > > <jun.li@xxxxxxx> > > Subject: RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low > > > > > -----Original Message----- > > > From: PETER CHEN [mailto:peter.chen@xxxxxxx] > > > 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@xxxxxxxxxxxxx> > > > > --- > > > > 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
