Hi Peter, > -----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. 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
