[PATCH 1/1] usb: chipidea: imx: improve the over current handling

[PATCH 1/1] usb: chipidea: imx: improve the over current handling

[PETER CHEN]

The current OC (Over Current) handling does not consider the default
and bootloader OC setting well, in this commit, we reset OC setting
according to dts value:
- If property "disable-over-current" is set, we will disable OC at
code; otherwise, we will enable OC.
- Since most of USB power control chips are low active for OC, we keep
"over-current-active-high" property unchanging to reduce users effort.
If this property is set, we set OC polarity as high explicitly;
otherwise, we set it as low explicitly.

Cc: stable <stable@vger.kernel.org>
Cc: Peter Chen <peter.chen@nxp.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Matthew Starr <mstarr@hedonline.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
 drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
 drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 56781c329db0..058468f2de8d 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -140,7 +140,7 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
 		data->disable_oc = 1;
 
 	if (of_find_property(np, "over-current-active-high", NULL))
-		data->oc_polarity = 1;
+		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,8 @@ 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 high if oc enabled */
+	unsigned int oc_polarity_high:1;
 	unsigned int evdo:1; /* set external vbus divider option */
 	unsigned int ulpi:1; /* connected to an ULPI phy */
 	unsigned int hsic:1; /* HSIC controlller */
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 43a15a6e86f5..30d1ada952e1 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
 		val = readl(usbmisc->base);
 		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);
+		val |= MX25_OTG_PM_BIT;
+		if (data->oc_polarity_high)
+			/* High active */
+			val |= MX25_OTG_OCPOL_BIT;
+		else
+			val &= ~MX25_OTG_OCPOL_BIT;
+
 		writel(val, usbmisc->base);
 		break;
 	case 1:
 		val = readl(usbmisc->base);
 		val &= ~(MX25_H1_SIC_MASK | MX25_H1_PP_BIT |  MX25_H1_IPPUE_UP_BIT);
 		val |= (MX25_EHCI_INTERFACE_SINGLE_UNI & MX25_EHCI_INTERFACE_MASK) << MX25_H1_SIC_SHIFT;
-		val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
+		val |= (MX25_H1_PM_BIT | MX25_H1_TLL_BIT |
 			MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
+		if (data->oc_polarity_high)
+			/* High active */
+			val |= MX25_H1_OCPOL_BIT;
+		else
+			val &= ~MX25_H1_OCPOL_BIT;
 
 		writel(val, usbmisc->base);
 
@@ -356,12 +367,14 @@ 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) {
+	} else if (data->oc_polarity_high == 1) {
 		/* High active */
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
 	} else {
-		reg &= ~(MX6_BM_OVER_CUR_DIS);
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+		reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
+
 	writel(reg, usbmisc->base + data->index * 4);
 
 	/* SoC non-burst setting */
@@ -552,10 +565,14 @@ 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) {
+	} else if (data->oc_polarity_high == 1) {
 		/* High active */
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+	} else {
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+		reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
+
 	writel(reg, usbmisc->base);
 
 	reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
-- 
2.14.1

[1/1] usb: chipidea: imx: improve the over current handling

[Peter Chen]

The current OC (Over Current) handling does not consider the default
and bootloader OC setting well, in this commit, we reset OC setting
according to dts value:
- If property "disable-over-current" is set, we will disable OC at
code; otherwise, we will enable OC.
- Since most of USB power control chips are low active for OC, we keep
"over-current-active-high" property unchanging to reduce users effort.
If this property is set, we set OC polarity as high explicitly;
otherwise, we set it as low explicitly.

Cc: stable <stable@vger.kernel.org>
Cc: Peter Chen <peter.chen@nxp.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Matthew Starr <mstarr@hedonline.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
 drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
 drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
 3 files changed, 25 insertions(+), 7 deletions(-)

-- 
2.14.1

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 56781c329db0..058468f2de8d 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -140,7 +140,7 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
 		data->disable_oc = 1;
 
 	if (of_find_property(np, "over-current-active-high", NULL))
-		data->oc_polarity = 1;
+		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,8 @@ 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 high if oc enabled */
+	unsigned int oc_polarity_high:1;
 	unsigned int evdo:1; /* set external vbus divider option */
 	unsigned int ulpi:1; /* connected to an ULPI phy */
 	unsigned int hsic:1; /* HSIC controlller */
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 43a15a6e86f5..30d1ada952e1 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
 		val = readl(usbmisc->base);
 		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);
+		val |= MX25_OTG_PM_BIT;
+		if (data->oc_polarity_high)
+			/* High active */
+			val |= MX25_OTG_OCPOL_BIT;
+		else
+			val &= ~MX25_OTG_OCPOL_BIT;
+
 		writel(val, usbmisc->base);
 		break;
 	case 1:
 		val = readl(usbmisc->base);
 		val &= ~(MX25_H1_SIC_MASK | MX25_H1_PP_BIT |  MX25_H1_IPPUE_UP_BIT);
 		val |= (MX25_EHCI_INTERFACE_SINGLE_UNI & MX25_EHCI_INTERFACE_MASK) << MX25_H1_SIC_SHIFT;
-		val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
+		val |= (MX25_H1_PM_BIT | MX25_H1_TLL_BIT |
 			MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
+		if (data->oc_polarity_high)
+			/* High active */
+			val |= MX25_H1_OCPOL_BIT;
+		else
+			val &= ~MX25_H1_OCPOL_BIT;
 
 		writel(val, usbmisc->base);
 
@@ -356,12 +367,14 @@ 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) {
+	} else if (data->oc_polarity_high == 1) {
 		/* High active */
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
 	} else {
-		reg &= ~(MX6_BM_OVER_CUR_DIS);
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+		reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
+
 	writel(reg, usbmisc->base + data->index * 4);
 
 	/* SoC non-burst setting */
@@ -552,10 +565,14 @@ 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) {
+	} else if (data->oc_polarity_high == 1) {
 		/* High active */
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+	} else {
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+		reg |= MX6_BM_OVER_CUR_POLARITY;
 	}
+
 	writel(reg, usbmisc->base);
 
 	reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);

Re: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

Hello,

On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> The current OC (Over Current) handling does not consider the default
> and bootloader OC setting well, in this commit, we reset OC setting
> according to dts value:
> - If property "disable-over-current" is set, we will disable OC at
> code; otherwise, we will enable OC.
> - Since most of USB power control chips are low active for OC, we keep
> "over-current-active-high" property unchanging to reduce users effort.
> If this property is set, we set OC polarity as high explicitly;
> otherwise, we set it as low explicitly.
> 
> Cc: stable <stable@vger.kernel.org>
> Cc: Peter Chen <peter.chen@nxp.com>
> Cc: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> Cc: Matthew Starr <mstarr@hedonline.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
>  drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
>  drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 56781c329db0..058468f2de8d 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -140,7 +140,7 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
>  		data->disable_oc = 1;
>  
>  	if (of_find_property(np, "over-current-active-high", NULL))
> -		data->oc_polarity = 1;
> +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -11,7 +11,8 @@ 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 high if oc enabled */
> +	unsigned int oc_polarity_high:1;
>  	unsigned int evdo:1; /* set external vbus divider option */
>  	unsigned int ulpi:1; /* connected to an ULPI phy */
>  	unsigned int hsic:1; /* HSIC controlller */
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index 43a15a6e86f5..30d1ada952e1 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
>  		val = readl(usbmisc->base);
>  		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);
> +		val |= MX25_OTG_PM_BIT;
> +		if (data->oc_polarity_high)
> +			/* High active */
> +			val |= MX25_OTG_OCPOL_BIT;
> +		else
> +			val &= ~MX25_OTG_OCPOL_BIT;
> +

I think we shouldn't change the i.MX25 behaviour in a patch that is
backported to stable. Even without this I'd put the change to i.MX25 in
a separate patch. (As I did in my series.)

Also the change is bad, because you're going from hard-coded active high
to active low for all users that don't specify "over-current-active-high".
This breaks (I think) most i.MX25 machines.

Affected in mainline are:

	imx25-pdk.dts (&usbotg + &usbhost1)
	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)

and out of tree probably more. (But maybe these use really active low
signalling? I don't know and didn't check.)

Also for i.MX6/7 the overhead isn't that bad to keep compatibility and
give the possibility to explicitly specify the polarity. So I still
prefer my patch set over your approach (which conceptually matches the
patch created by Matthew).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

Hello,

On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> The current OC (Over Current) handling does not consider the default
> and bootloader OC setting well, in this commit, we reset OC setting
> according to dts value:
> - If property "disable-over-current" is set, we will disable OC at
> code; otherwise, we will enable OC.
> - Since most of USB power control chips are low active for OC, we keep
> "over-current-active-high" property unchanging to reduce users effort.
> If this property is set, we set OC polarity as high explicitly;
> otherwise, we set it as low explicitly.
> 
> Cc: stable <stable@vger.kernel.org>
> Cc: Peter Chen <peter.chen@nxp.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Matthew Starr <mstarr@hedonline.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
>  drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
>  drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 56781c329db0..058468f2de8d 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -140,7 +140,7 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
>  		data->disable_oc = 1;
>  
>  	if (of_find_property(np, "over-current-active-high", NULL))
> -		data->oc_polarity = 1;
> +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -11,7 +11,8 @@ 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 high if oc enabled */
> +	unsigned int oc_polarity_high:1;
>  	unsigned int evdo:1; /* set external vbus divider option */
>  	unsigned int ulpi:1; /* connected to an ULPI phy */
>  	unsigned int hsic:1; /* HSIC controlller */
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index 43a15a6e86f5..30d1ada952e1 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data)
>  		val = readl(usbmisc->base);
>  		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);
> +		val |= MX25_OTG_PM_BIT;
> +		if (data->oc_polarity_high)
> +			/* High active */
> +			val |= MX25_OTG_OCPOL_BIT;
> +		else
> +			val &= ~MX25_OTG_OCPOL_BIT;
> +

I think we shouldn't change the i.MX25 behaviour in a patch that is
backported to stable. Even without this I'd put the change to i.MX25 in
a separate patch. (As I did in my series.)

Also the change is bad, because you're going from hard-coded active high
to active low for all users that don't specify "over-current-active-high".
This breaks (I think) most i.MX25 machines.

Affected in mainline are:

	imx25-pdk.dts (&usbotg + &usbhost1)
	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)

and out of tree probably more. (But maybe these use really active low
signalling? I don't know and didn't check.)

Also for i.MX6/7 the overhead isn't that bad to keep compatibility and
give the possibility to explicitly specify the polarity. So I still
prefer my patch set over your approach (which conceptually matches the
patch created by Matthew).

Best regards
Uwe

RE: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[PETER CHEN]

 
> 
> On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > The current OC (Over Current) handling does not consider the default
> > and bootloader OC setting well, in this commit, we reset OC setting
> > according to dts value:
> > - If property "disable-over-current" is set, we will disable OC at
> > code; otherwise, we will enable OC.
> > - Since most of USB power control chips are low active for OC, we keep
> > "over-current-active-high" property unchanging to reduce users effort.
> > If this property is set, we set OC polarity as high explicitly;
> > otherwise, we set it as low explicitly.
> >
> > Cc: stable <stable@vger.kernel.org>
> > Cc: Peter Chen <peter.chen@nxp.com>
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Matthew Starr <mstarr@hedonline.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> >  3 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 56781c329db0..058468f2de8d 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> >  		data->disable_oc = 1;
> >
> >  	if (of_find_property(np, "over-current-active-high", NULL))
> > -		data->oc_polarity = 1;
> > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > +	unsigned int oc_polarity_high:1;
> >  	unsigned int evdo:1; /* set external vbus divider option */
> >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > a/drivers/usb/chipidea/usbmisc_imx.c
> > b/drivers/usb/chipidea/usbmisc_imx.c
> > index 43a15a6e86f5..30d1ada952e1 100644
> > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> *data)
> >  		val = readl(usbmisc->base);
> >  		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);
> > +		val |= MX25_OTG_PM_BIT;
> > +		if (data->oc_polarity_high)
> > +			/* High active */
> > +			val |= MX25_OTG_OCPOL_BIT;
> > +		else
> > +			val &= ~MX25_OTG_OCPOL_BIT;
> > +
> 
> I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> in my series.)
> 
> Also the change is bad, because you're going from hard-coded active high to active
> low for all users that don't specify "over-current-active-high".
> This breaks (I think) most i.MX25 machines.
> 
> Affected in mainline are:
> 
> 	imx25-pdk.dts (&usbotg + &usbhost1)
> 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> 
> and out of tree probably more. (But maybe these use really active low signalling? I
> don't know and didn't check.)

Are you sure the boards you listed works well for OC now? Both you and Matthew posted
patch for OC issue, I think you both probably find the OC function works not well. I have
checked the boards (from imx35) at Freescale/NXP internal, all boards are active low for OC. 

My intention is to let most of boards work well without adding dts property since most of
USB power control chip is active low for OC, the OC function should not test well before.

Your patches do not break current setting, but need the users to add dts property
to let OC function work (Since most of chip are active low for OC), almost all i.mx
boards will see the warning message for without setting OC polarity, and must
add dts property to fix this warning message. 

I am OK to accept your patch if both Matthew and Jun are ok with it.

Peter

[1/1] usb: chipidea: imx: improve the over current handling

[Peter Chen]

> 
> On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > The current OC (Over Current) handling does not consider the default
> > and bootloader OC setting well, in this commit, we reset OC setting
> > according to dts value:
> > - If property "disable-over-current" is set, we will disable OC at
> > code; otherwise, we will enable OC.
> > - Since most of USB power control chips are low active for OC, we keep
> > "over-current-active-high" property unchanging to reduce users effort.
> > If this property is set, we set OC polarity as high explicitly;
> > otherwise, we set it as low explicitly.
> >
> > Cc: stable <stable@vger.kernel.org>
> > Cc: Peter Chen <peter.chen@nxp.com>
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Matthew Starr <mstarr@hedonline.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> >  3 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 56781c329db0..058468f2de8d 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> >  		data->disable_oc = 1;
> >
> >  	if (of_find_property(np, "over-current-active-high", NULL))
> > -		data->oc_polarity = 1;
> > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > +	unsigned int oc_polarity_high:1;
> >  	unsigned int evdo:1; /* set external vbus divider option */
> >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > a/drivers/usb/chipidea/usbmisc_imx.c
> > b/drivers/usb/chipidea/usbmisc_imx.c
> > index 43a15a6e86f5..30d1ada952e1 100644
> > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> *data)
> >  		val = readl(usbmisc->base);
> >  		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);
> > +		val |= MX25_OTG_PM_BIT;
> > +		if (data->oc_polarity_high)
> > +			/* High active */
> > +			val |= MX25_OTG_OCPOL_BIT;
> > +		else
> > +			val &= ~MX25_OTG_OCPOL_BIT;
> > +
> 
> I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> in my series.)
> 
> Also the change is bad, because you're going from hard-coded active high to active
> low for all users that don't specify "over-current-active-high".
> This breaks (I think) most i.MX25 machines.
> 
> Affected in mainline are:
> 
> 	imx25-pdk.dts (&usbotg + &usbhost1)
> 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> 
> and out of tree probably more. (But maybe these use really active low signalling? I
> don't know and didn't check.)

Are you sure the boards you listed works well for OC now? Both you and Matthew posted
patch for OC issue, I think you both probably find the OC function works not well. I have
checked the boards (from imx35) at Freescale/NXP internal, all boards are active low for OC. 

My intention is to let most of boards work well without adding dts property since most of
USB power control chip is active low for OC, the OC function should not test well before.

Your patches do not break current setting, but need the users to add dts property
to let OC function work (Since most of chip are active low for OC), almost all i.mx
boards will see the warning message for without setting OC polarity, and must
add dts property to fix this warning message. 

I am OK to accept your patch if both Matthew and Jun are ok with it.

Peter

Re: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 09:02:05AM +0000, PETER CHEN wrote:
>  
> > 
> > On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > > The current OC (Over Current) handling does not consider the default
> > > and bootloader OC setting well, in this commit, we reset OC setting
> > > according to dts value:
> > > - If property "disable-over-current" is set, we will disable OC at
> > > code; otherwise, we will enable OC.
> > > - Since most of USB power control chips are low active for OC, we keep
> > > "over-current-active-high" property unchanging to reduce users effort.
> > > If this property is set, we set OC polarity as high explicitly;
> > > otherwise, we set it as low explicitly.
> > >
> > > Cc: stable <stable@vger.kernel.org>
> > > Cc: Peter Chen <peter.chen@nxp.com>
> > > Cc: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> > > Cc: Matthew Starr <mstarr@hedonline.com>
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 56781c329db0..058468f2de8d 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > >  		data->disable_oc = 1;
> > >
> > >  	if (of_find_property(np, "over-current-active-high", NULL))
> > > -		data->oc_polarity = 1;
> > > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > > +	unsigned int oc_polarity_high:1;
> > >  	unsigned int evdo:1; /* set external vbus divider option */
> > >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> > >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 43a15a6e86f5..30d1ada952e1 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> > *data)
> > >  		val = readl(usbmisc->base);
> > >  		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);
> > > +		val |= MX25_OTG_PM_BIT;
> > > +		if (data->oc_polarity_high)
> > > +			/* High active */
> > > +			val |= MX25_OTG_OCPOL_BIT;
> > > +		else
> > > +			val &= ~MX25_OTG_OCPOL_BIT;
> > > +
> > 
> > I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> > stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> > in my series.)
> > 
> > Also the change is bad, because you're going from hard-coded active high to active
> > low for all users that don't specify "over-current-active-high".
> > This breaks (I think) most i.MX25 machines.
> > 
> > Affected in mainline are:
> > 
> > 	imx25-pdk.dts (&usbotg + &usbhost1)
> > 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> > 
> > and out of tree probably more. (But maybe these use really active low signalling? I
> > don't know and didn't check.)
> 
> Are you sure the boards you listed works well for OC now?

No I'm not. That's why I added the question in parentesis.

> Both you and Matthew posted patch for OC issue, I think you both
> probably find the OC function works not well. I have checked the
> boards (from imx35) at Freescale/NXP internal, all boards are active
> low for OC. 

I think it is unfortunate to have linux default to active low (in the
absense of an explicit configuration) while the reset default is active
high.

> My intention is to let most of boards work well without adding dts property since most of
> USB power control chip is active low for OC, the OC function should not test well before.
> 
> Your patches do not break current setting, but need the users to add dts property
> to let OC function work (Since most of chip are active low for OC), almost all i.mx
> boards will see the warning message for without setting OC polarity, and must
> add dts property to fix this warning message. 

You seem to consider having to explicitly define the polarity a
downside. I'd say it is an advantage have this information in the
machine description.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 09:02:05AM +0000, PETER CHEN wrote:
>  
> > 
> > On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > > The current OC (Over Current) handling does not consider the default
> > > and bootloader OC setting well, in this commit, we reset OC setting
> > > according to dts value:
> > > - If property "disable-over-current" is set, we will disable OC at
> > > code; otherwise, we will enable OC.
> > > - Since most of USB power control chips are low active for OC, we keep
> > > "over-current-active-high" property unchanging to reduce users effort.
> > > If this property is set, we set OC polarity as high explicitly;
> > > otherwise, we set it as low explicitly.
> > >
> > > Cc: stable <stable@vger.kernel.org>
> > > Cc: Peter Chen <peter.chen@nxp.com>
> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Cc: Matthew Starr <mstarr@hedonline.com>
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 56781c329db0..058468f2de8d 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > >  		data->disable_oc = 1;
> > >
> > >  	if (of_find_property(np, "over-current-active-high", NULL))
> > > -		data->oc_polarity = 1;
> > > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > > +	unsigned int oc_polarity_high:1;
> > >  	unsigned int evdo:1; /* set external vbus divider option */
> > >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> > >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 43a15a6e86f5..30d1ada952e1 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> > *data)
> > >  		val = readl(usbmisc->base);
> > >  		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);
> > > +		val |= MX25_OTG_PM_BIT;
> > > +		if (data->oc_polarity_high)
> > > +			/* High active */
> > > +			val |= MX25_OTG_OCPOL_BIT;
> > > +		else
> > > +			val &= ~MX25_OTG_OCPOL_BIT;
> > > +
> > 
> > I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> > stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> > in my series.)
> > 
> > Also the change is bad, because you're going from hard-coded active high to active
> > low for all users that don't specify "over-current-active-high".
> > This breaks (I think) most i.MX25 machines.
> > 
> > Affected in mainline are:
> > 
> > 	imx25-pdk.dts (&usbotg + &usbhost1)
> > 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> > 
> > and out of tree probably more. (But maybe these use really active low signalling? I
> > don't know and didn't check.)
> 
> Are you sure the boards you listed works well for OC now?

No I'm not. That's why I added the question in parentesis.

> Both you and Matthew posted patch for OC issue, I think you both
> probably find the OC function works not well. I have checked the
> boards (from imx35) at Freescale/NXP internal, all boards are active
> low for OC. 

I think it is unfortunate to have linux default to active low (in the
absense of an explicit configuration) while the reset default is active
high.

> My intention is to let most of boards work well without adding dts property since most of
> USB power control chip is active low for OC, the OC function should not test well before.
> 
> Your patches do not break current setting, but need the users to add dts property
> to let OC function work (Since most of chip are active low for OC), almost all i.mx
> boards will see the warning message for without setting OC polarity, and must
> add dts property to fix this warning message. 

You seem to consider having to explicitly define the polarity a
downside. I'd say it is an advantage have this information in the
machine description.

Best regards
Uwe

RE: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Matthew Starr]

> -----Original Message-----
> From: Uwe Kleine-K�nig [mailto:u.kleine-koenig@pengutronix.de]
> 
> On Mon, Dec 03, 2018 at 09:02:05AM +0000, PETER CHEN wrote:
> >
> > >
> > > On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > > > The current OC (Over Current) handling does not consider the default
> > > > and bootloader OC setting well, in this commit, we reset OC setting
> > > > according to dts value:
> > > > - If property "disable-over-current" is set, we will disable OC at
> > > > code; otherwise, we will enable OC.
> > > > - Since most of USB power control chips are low active for OC, we keep
> > > > "over-current-active-high" property unchanging to reduce users effort.
> > > > If this property is set, we set OC polarity as high explicitly;
> > > > otherwise, we set it as low explicitly.
> > > >
> > > > Cc: stable <stable@vger.kernel.org>
> > > > Cc: Peter Chen <peter.chen@nxp.com>
> > > > Cc: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> > > > Cc: Matthew Starr <mstarr@hedonline.com>
> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > ---
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> > > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 56781c329db0..058468f2de8d 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > > *usbmisc_get_init_data(struct device *dev)
> > > >  		data->disable_oc = 1;
> > > >
> > > >  	if (of_find_property(np, "over-current-active-high", NULL))
> > > > -		data->oc_polarity = 1;
> > > > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > > > +	unsigned int oc_polarity_high:1;
> > > >  	unsigned int evdo:1; /* set external vbus divider option */
> > > >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> > > >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > > index 43a15a6e86f5..30d1ada952e1 100644
> > > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> > > *data)
> > > >  		val = readl(usbmisc->base);
> > > >  		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);
> > > > +		val |= MX25_OTG_PM_BIT;
> > > > +		if (data->oc_polarity_high)
> > > > +			/* High active */
> > > > +			val |= MX25_OTG_OCPOL_BIT;
> > > > +		else
> > > > +			val &= ~MX25_OTG_OCPOL_BIT;
> > > > +
> > >
> > > I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> > > stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> > > in my series.)
> > >
> > > Also the change is bad, because you're going from hard-coded active high to active
> > > low for all users that don't specify "over-current-active-high".
> > > This breaks (I think) most i.MX25 machines.
> > >
> > > Affected in mainline are:
> > >
> > > 	imx25-pdk.dts (&usbotg + &usbhost1)
> > > 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> > >
> > > and out of tree probably more. (But maybe these use really active low signalling? I
> > > don't know and didn't check.)
> >
> > Are you sure the boards you listed works well for OC now?
> 
> No I'm not. That's why I added the question in parentesis.
> 
> > Both you and Matthew posted patch for OC issue, I think you both
> > probably find the OC function works not well. I have checked the
> > boards (from imx35) at Freescale/NXP internal, all boards are active
> > low for OC.
> 
> I think it is unfortunate to have linux default to active low (in the
> absense of an explicit configuration) while the reset default is active
> high.
> 
> > My intention is to let most of boards work well without adding dts property since most of
> > USB power control chip is active low for OC, the OC function should not test well before.
> >
> > Your patches do not break current setting, but need the users to add dts property
> > to let OC function work (Since most of chip are active low for OC), almost all i.mx
> > boards will see the warning message for without setting OC polarity, and must
> > add dts property to fix this warning message.
> 
> You seem to consider having to explicitly define the polarity a
> downside. I'd say it is an advantage have this information in the
> machine description.
> 

It seems based on many drivers I have seen that a boolean type of device tree property sets a condition and the absence of the property means the opposite condition.  This is the method I would normally prefer, but in this case since the original property prevented active low from being set in the kernel, people started to depend on the bootloader setting without the kernel touching it.  We now have three cases (active high, active low, and set by bootloader without kernel touching the OC polarity).  I agree with Uwe's implementation since it handles the three case situation.

One issue with Uwe's implementation is the lack of handling the error case when both properties (over-current-active-high and over-current-active-low) are both listed in a device tree.  In this case there should be an error or warning message and the kernel should not touch the OC polarity bit (leave the bootloader setting).

Matt Starr

[1/1] usb: chipidea: imx: improve the over current handling

[Matthew Starr]

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> 
> On Mon, Dec 03, 2018 at 09:02:05AM +0000, PETER CHEN wrote:
> >
> > >
> > > On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > > > The current OC (Over Current) handling does not consider the default
> > > > and bootloader OC setting well, in this commit, we reset OC setting
> > > > according to dts value:
> > > > - If property "disable-over-current" is set, we will disable OC at
> > > > code; otherwise, we will enable OC.
> > > > - Since most of USB power control chips are low active for OC, we keep
> > > > "over-current-active-high" property unchanging to reduce users effort.
> > > > If this property is set, we set OC polarity as high explicitly;
> > > > otherwise, we set it as low explicitly.
> > > >
> > > > Cc: stable <stable@vger.kernel.org>
> > > > Cc: Peter Chen <peter.chen@nxp.com>
> > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Cc: Matthew Starr <mstarr@hedonline.com>
> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > ---
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> > > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 56781c329db0..058468f2de8d 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > > *usbmisc_get_init_data(struct device *dev)
> > > >  		data->disable_oc = 1;
> > > >
> > > >  	if (of_find_property(np, "over-current-active-high", NULL))
> > > > -		data->oc_polarity = 1;
> > > > +		data->oc_polarity_high = 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 fcecab478934..5ea8bb239b38 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > @@ -11,7 +11,8 @@ 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 high if oc enabled */
> > > > +	unsigned int oc_polarity_high:1;
> > > >  	unsigned int evdo:1; /* set external vbus divider option */
> > > >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> > > >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > > index 43a15a6e86f5..30d1ada952e1 100644
> > > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> > > *data)
> > > >  		val = readl(usbmisc->base);
> > > >  		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);
> > > > +		val |= MX25_OTG_PM_BIT;
> > > > +		if (data->oc_polarity_high)
> > > > +			/* High active */
> > > > +			val |= MX25_OTG_OCPOL_BIT;
> > > > +		else
> > > > +			val &= ~MX25_OTG_OCPOL_BIT;
> > > > +
> > >
> > > I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> > > stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> > > in my series.)
> > >
> > > Also the change is bad, because you're going from hard-coded active high to active
> > > low for all users that don't specify "over-current-active-high".
> > > This breaks (I think) most i.MX25 machines.
> > >
> > > Affected in mainline are:
> > >
> > > 	imx25-pdk.dts (&usbotg + &usbhost1)
> > > 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> > >
> > > and out of tree probably more. (But maybe these use really active low signalling? I
> > > don't know and didn't check.)
> >
> > Are you sure the boards you listed works well for OC now?
> 
> No I'm not. That's why I added the question in parentesis.
> 
> > Both you and Matthew posted patch for OC issue, I think you both
> > probably find the OC function works not well. I have checked the
> > boards (from imx35) at Freescale/NXP internal, all boards are active
> > low for OC.
> 
> I think it is unfortunate to have linux default to active low (in the
> absense of an explicit configuration) while the reset default is active
> high.
> 
> > My intention is to let most of boards work well without adding dts property since most of
> > USB power control chip is active low for OC, the OC function should not test well before.
> >
> > Your patches do not break current setting, but need the users to add dts property
> > to let OC function work (Since most of chip are active low for OC), almost all i.mx
> > boards will see the warning message for without setting OC polarity, and must
> > add dts property to fix this warning message.
> 
> You seem to consider having to explicitly define the polarity a
> downside. I'd say it is an advantage have this information in the
> machine description.
> 

It seems based on many drivers I have seen that a boolean type of device tree property sets a condition and the absence of the property means the opposite condition.  This is the method I would normally prefer, but in this case since the original property prevented active low from being set in the kernel, people started to depend on the bootloader setting without the kernel touching it.  We now have three cases (active high, active low, and set by bootloader without kernel touching the OC polarity).  I agree with Uwe's implementation since it handles the three case situation.

One issue with Uwe's implementation is the lack of handling the error case when both properties (over-current-active-high and over-current-active-low) are both listed in a device tree.  In this case there should be an error or warning message and the kernel should not touch the OC polarity bit (leave the bootloader setting).

Matt Starr