usb: chipidea: imx: unify over-current polarity handling

Message ID 20181130205309.17232-1-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • usb: chipidea: imx: unify over-current polarity handling
Related show

Commit Message

Uwe Kleine-König Nov. 30, 2018, 8:53 p.m.
The status quo is:
 - on i.MX25 the overcurrent polarity is always explicitly configured as
   active high which matches the reset default.
 - on i.MX6 and i.MX7 the overcurrent polarity is active high after
   reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current
   state (probably as setup by the bootloader or still the reset
   default) unless the polarity is explicitly configured as active high
   which then is configured. (So if the pin is active low and the
   bootloader didn't set this up the configuration is wrong.)

To improve the situation always configure the reset default value unless
the device tree configures the polarity (and then use this one).

Note that as the reset default is active high on all platforms there is
no need to check for the presence of "over-current-active-high". In the
absence of "over-current-active-low" it doesn't matter if active high is
configured because that's the default or because it is configured
explicitly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  1 +
 drivers/usb/chipidea/ci_hdrc_imx.c            |  9 ++++-
 drivers/usb/chipidea/ci_hdrc_imx.h            |  4 +-
 drivers/usb/chipidea/usbmisc_imx.c            | 39 +++++++++++++++++--
 4 files changed, 46 insertions(+), 7 deletions(-)

Comments

Fabio Estevam Nov. 30, 2018, 9:16 p.m. | #1
Hi Uwe,

[Adding Matt]

On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> The status quo is:
>  - on i.MX25 the overcurrent polarity is always explicitly configured as
>    active high which matches the reset default.
>  - on i.MX6 and i.MX7 the overcurrent polarity is active high after
>    reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current
>    state (probably as setup by the bootloader or still the reset
>    default) unless the polarity is explicitly configured as active high
>    which then is configured. (So if the pin is active low and the
>    bootloader didn't set this up the configuration is wrong.)
>
> To improve the situation always configure the reset default value unless
> the device tree configures the polarity (and then use this one).
>
> Note that as the reset default is active high on all platforms there is
> no need to check for the presence of "over-current-active-high". In the
> absence of "over-current-active-low" it doesn't matter if active high is
> configured because that's the default or because it is configured
> explicitly.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

It seems that Matt is also interested in adding the
'over-current-active-low' property:
https://patchwork.kernel.org/patch/10701311/
Uwe Kleine-König Nov. 30, 2018, 9:34 p.m. | #2
Hello Fabio,

On Fri, Nov 30, 2018 at 07:16:39PM -0200, Fabio Estevam wrote:
> [Adding Matt]
> 
> On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > The status quo is:
> >  - on i.MX25 the overcurrent polarity is always explicitly configured as
> >    active high which matches the reset default.
> >  - on i.MX6 and i.MX7 the overcurrent polarity is active high after
> >    reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current
> >    state (probably as setup by the bootloader or still the reset
> >    default) unless the polarity is explicitly configured as active high
> >    which then is configured. (So if the pin is active low and the
> >    bootloader didn't set this up the configuration is wrong.)
> >
> > To improve the situation always configure the reset default value unless
> > the device tree configures the polarity (and then use this one).
> >
> > Note that as the reset default is active high on all platforms there is
> > no need to check for the presence of "over-current-active-high". In the
> > absence of "over-current-active-low" it doesn't matter if active high is
> > configured because that's the default or because it is configured
> > explicitly.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> It seems that Matt is also interested in adding the
> 'over-current-active-low' property:
> https://patchwork.kernel.org/patch/10701311/

Looking at Matt's patch I noticed that I applied my patch on an old
kernel version (4.14) :-|

But I think Matt's patch is broken because in usbmisc_imx6q_init() in
the last else branch MX6_BM_OVER_CUR_POLARITY isn't cleared. Other than
that they don't look that different semantically.

I will respin my patch on Monday.

Best regards
Uwe
Matthew Starr Nov. 30, 2018, 11:39 p.m. | #3
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: Friday, November 30, 2018 3:35 PM
> 
> Hello Fabio,
> 
> On Fri, Nov 30, 2018 at 07:16:39PM -0200, Fabio Estevam wrote:
> > [Adding Matt]
> >
> > On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > The status quo is:
> > >  - on i.MX25 the overcurrent polarity is always explicitly configured as
> > >    active high which matches the reset default.
> > >  - on i.MX6 and i.MX7 the overcurrent polarity is active high after
> > >    reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current
> > >    state (probably as setup by the bootloader or still the reset
> > >    default) unless the polarity is explicitly configured as active high
> > >    which then is configured. (So if the pin is active low and the
> > >    bootloader didn't set this up the configuration is wrong.)
> > >
> > > To improve the situation always configure the reset default value unless
> > > the device tree configures the polarity (and then use this one).
> > >
> > > Note that as the reset default is active high on all platforms there is
> > > no need to check for the presence of "over-current-active-high". In the
> > > absence of "over-current-active-low" it doesn't matter if active high is
> > > configured because that's the default or because it is configured
> > > explicitly.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > It seems that Matt is also interested in adding the
> > 'over-current-active-low' property:
> > https://patchwork.kernel.org/patch/10701311/
> 
> Looking at Matt's patch I noticed that I applied my patch on an old
> kernel version (4.14) :-|
> 
> But I think Matt's patch is broken because in usbmisc_imx6q_init() in
> the last else branch MX6_BM_OVER_CUR_POLARITY isn't cleared. Other
> than
> that they don't look that different semantically.
> 
> I will respin my patch on Monday.
> 

Uwe,

You are correct that I missed the clearing of MX6_BM_OVER_CUR_POLARITY.

Also I don't see any reason why that last else that should clear MX6_BM_OVER_CUR_POLARITY and MX6_BM_OVER_CUR_DIS shouldn't also be in the usbmisc_imx7d_init function too.

I just resubmitted my patch based on 4.20-rc4 with the mentioned changes.

Matt Starr

Patch

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 0e03344e2e8b..b733b7c4bfdc 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -83,6 +83,7 @@  i.mx specific properties
 - 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.
 - 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 5f4a8157fad8..352bd0bfe161 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -141,8 +141,13 @@  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))
-		data->oc_polarity = 1;
+	/*
+	 * No need to check for "over-current-active-high" as this is the reset
+	 * default for all platforms and this is configured unless "active-low"
+	 * is specified.
+	 */
+	if (of_find_property(np, "over-current-active-low", NULL))
+		data->oc_polarity_low = 1;
 
 	if (of_find_property(np, "external-vbus-divider", NULL))
 		data->evdo = 1;
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h b/drivers/usb/chipidea/ci_hdrc_imx.h
index d666c9f036ba..dffa34f3777d 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -17,7 +17,9 @@  struct imx_usbmisc_data {
 	int index;
 
 	unsigned int disable_oc:1; /* over current detect disabled */
-	unsigned int oc_polarity:1; /* over current polarity if oc enabled */
+	/* over current polarity low if oc enabled */
+	unsigned int oc_polarity_low:1;
+
 	unsigned int evdo:1; /* set external vbus divider option */
 	unsigned int ulpi:1; /* connected to an ULPI phy */
 };
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 9f4a0185dd60..89c1a0c48f80 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -126,6 +126,15 @@  static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
 		val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
 		val |= (MX25_EHCI_INTERFACE_DIFF_UNI & MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
 		val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
+
+		/*
+		 * reset default for MX25_OTG_OCPOL_BIT is set (i.e. active
+		 * high). So only unset if explicitly configured and keep set
+		 * otherwise.
+		 */
+		if (data->oc_polarity_low)
+			val &= ~MX25_OTG_OCPOL_BIT;
+
 		writel(val, usbmisc->base);
 		break;
 	case 1:
@@ -135,6 +144,14 @@  static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
 		val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
 			MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
 
+		/*
+		 * reset default for MX25_H1_OCPOL_BIT is set (i.e. active
+		 * high). So only unset if explicitly configured and keep set
+		 * otherwise.
+		 */
+		if (data->oc_polarity_low)
+			val &= ~MX25_H1_OCPOL_BIT;
+
 		writel(val, usbmisc->base);
 
 		break;
@@ -340,9 +357,16 @@  static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
 	reg = readl(usbmisc->base + data->index * 4);
 	if (data->disable_oc) {
 		reg |= MX6_BM_OVER_CUR_DIS;
-	} else if (data->oc_polarity == 1) {
-		/* High active */
+	} else {
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+
+		/*
+		 * reset default for MX6_BM_OVER_CUR_POLARITY is unset (i.e.
+		 * active high). So only set if explicitly configured and keep
+		 * clear otherwise.
+		 */
+		if (data->oc_polarity_low)
+			reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
 	writel(reg, usbmisc->base + data->index * 4);
 
@@ -442,9 +466,16 @@  static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
 	reg = readl(usbmisc->base);
 	if (data->disable_oc) {
 		reg |= MX6_BM_OVER_CUR_DIS;
-	} else if (data->oc_polarity == 1) {
-		/* High active */
+	} else {
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+
+		/*
+		 * reset default for MX6_BM_OVER_CUR_POLARITY is unset (i.e.
+		 * active high). So only set if explicitly configured and keep
+		 * clear otherwise.
+		 */
+		if (data->oc_polarity_low)
+			reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
 	writel(reg, usbmisc->base);