[v3,1/3] usb: chipidea: imx: support configuring for active low oc signal

Message ID 20181204083131.12524-2-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • usb: chipidea: imx: improve oc handling
Related show

Commit Message

Uwe Kleine-König Dec. 4, 2018, 8:31 a.m.
The status quo on i.MX6 is that if "over-current-active-high" is
specified in the device tree this is configured as expected. If
the property is missing polarity isn't changed and so the
polarity is kept as setup by the bootloader. Reset default is
active high, so active low can only be used with help by the
bootloader. On i.MX7 it is similar, but there disabling of
over current detection has a similar inconsistency.

This patch introduces a new property that allows to explicitly
configure for active low over current detection and consistently
sets this up. In the absence of an explicit configuration the
bit is kept as is. On i.MX7 over current detection is used unless
disabled in the device tree.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
 drivers/usb/chipidea/ci_hdrc_imx.c            | 16 ++++++++---
 drivers/usb/chipidea/ci_hdrc_imx.h            |  8 +++++-
 drivers/usb/chipidea/usbmisc_imx.c            | 28 ++++++++++++++-----
 4 files changed, 43 insertions(+), 14 deletions(-)

Comments

Stefan Wahren Dec. 4, 2018, 8:40 a.m. | #1
Hi Uwe,

Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
> The status quo on i.MX6 is that if "over-current-active-high" is
> specified in the device tree this is configured as expected. If
> the property is missing polarity isn't changed and so the
> polarity is kept as setup by the bootloader. Reset default is
> active high, so active low can only be used with help by the
> bootloader. On i.MX7 it is similar, but there disabling of
> over current detection has a similar inconsistency.
>
> This patch introduces a new property that allows to explicitly
> configure for active low over current detection and consistently
> sets this up. In the absence of an explicit configuration the
> bit is kept as is. On i.MX7 over current detection is used unless
> disabled in the device tree.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
>  drivers/usb/chipidea/ci_hdrc_imx.c            | 16 ++++++++---
>  drivers/usb/chipidea/ci_hdrc_imx.h            |  8 +++++-
>  drivers/usb/chipidea/usbmisc_imx.c            | 28 ++++++++++++++-----
>  4 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..c32f6e983cf6 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -87,8 +87,9 @@ 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 active low.
> +- over-current-active-high: over current signal polarity is active high.
> +  It's recommended to specify the over current polarity.
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
>  
>  Example:

thanks for making this configurable. But shouldn't this be a separate
patch and reviewed by the devicetree guys?
Uwe Kleine-König Dec. 4, 2018, 8:52 a.m. | #2
Hello Stefan,

On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote:
> Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
> > The status quo on i.MX6 is that if "over-current-active-high" is
> > specified in the device tree this is configured as expected. If
> > the property is missing polarity isn't changed and so the
> > polarity is kept as setup by the bootloader. Reset default is
> > active high, so active low can only be used with help by the
> > bootloader. On i.MX7 it is similar, but there disabling of
> > over current detection has a similar inconsistency.
> >
> > This patch introduces a new property that allows to explicitly
> > configure for active low over current detection and consistently
> > sets this up. In the absence of an explicit configuration the
> > bit is kept as is. On i.MX7 over current detection is used unless
> > disabled in the device tree.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
> >  drivers/usb/chipidea/ci_hdrc_imx.c            | 16 ++++++++---
> >  drivers/usb/chipidea/ci_hdrc_imx.h            |  8 +++++-
> >  drivers/usb/chipidea/usbmisc_imx.c            | 28 ++++++++++++++-----
> >  4 files changed, 43 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..c32f6e983cf6 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -87,8 +87,9 @@ 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 active low.
> > +- over-current-active-high: over current signal polarity is active high.
> > +  It's recommended to specify the over current polarity.
> >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> >  
> >  Example:
> 
> thanks for making this configurable. But shouldn't this be a separate
> patch and reviewed by the devicetree guys?

I'm not sure about the separate patch part. My impression is it depends
on the maintainer if a change to the binding (opposed to the
introduction of a binding) should be separate or not.

But for the review you are right, I added the dt people to Cc for them
to comment. In v2 Matthew also noted that he would prefer to handle the
situation when both over-current-active-low and over-current-active-high
were given differently. I think we don't need that as this is a case of
"broken dt" and it doesn't matter much what is done then. (With my patch
we're configuring for active high in that case.) A feedback here would
be great, too.

Best regards
Uwe
Stefan Wahren Dec. 4, 2018, 11:43 a.m. | #3
Am 04.12.18 um 09:52 schrieb Uwe Kleine-König:
> Hello Stefan,
>
> On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote:
>> Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
>>> The status quo on i.MX6 is that if "over-current-active-high" is
>>> specified in the device tree this is configured as expected. If
>>> the property is missing polarity isn't changed and so the
>>> polarity is kept as setup by the bootloader. Reset default is
>>> active high, so active low can only be used with help by the
>>> bootloader. On i.MX7 it is similar, but there disabling of
>>> over current detection has a similar inconsistency.
>>>
>>> This patch introduces a new property that allows to explicitly
>>> configure for active low over current detection and consistently
>>> sets this up. In the absence of an explicit configuration the
>>> bit is kept as is. On i.MX7 over current detection is used unless
>>> disabled in the device tree.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
>>>  drivers/usb/chipidea/ci_hdrc_imx.c            | 16 ++++++++---
>>>  drivers/usb/chipidea/ci_hdrc_imx.h            |  8 +++++-
>>>  drivers/usb/chipidea/usbmisc_imx.c            | 28 ++++++++++++++-----
>>>  4 files changed, 43 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> index 529e51879fb2..c32f6e983cf6 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> @@ -87,8 +87,9 @@ 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 active low.
>>> +- over-current-active-high: over current signal polarity is active high.
>>> +  It's recommended to specify the over current polarity.
>>>  - external-vbus-divider: enables off-chip resistor divider for Vbus
>>>  
>>>  Example:
>> thanks for making this configurable. But shouldn't this be a separate
>> patch and reviewed by the devicetree guys?
> I'm not sure about the separate patch part. My impression is it depends
> on the maintainer if a change to the binding (opposed to the
> introduction of a binding) should be separate or not.
>
> But for the review you are right, I added the dt people to Cc for them
> to comment. In v2 Matthew also noted that he would prefer to handle the
> situation when both over-current-active-low and over-current-active-high
> were given differently. I think we don't need that as this is a case of
> "broken dt" and it doesn't matter much what is done then. (With my patch
> we're configuring for active high in that case.) A feedback here would
> be great, too.

AFAIR such invalid settings should be catched with a proper error
message and abort of the probing.

Regards
Stefan

>
> Best regards
> Uwe
>
Uwe Kleine-König Dec. 4, 2018, 1:39 p.m. | #4
Hello,

On Tue, Dec 04, 2018 at 12:43:21PM +0100, Stefan Wahren wrote:
> Am 04.12.18 um 09:52 schrieb Uwe Kleine-König:
> > But for the review you are right, I added the dt people to Cc for them
> > to comment. In v2 Matthew also noted that he would prefer to handle the
> > situation when both over-current-active-low and over-current-active-high
> > were given differently. I think we don't need that as this is a case of
> > "broken dt" and it doesn't matter much what is done then. (With my patch
> > we're configuring for active high in that case.) A feedback here would
> > be great, too.
> 
> AFAIR such invalid settings should be catched with a proper error
> message and abort of the probing.

I remember a review commend from Rob[1]: "It is not really the kernel's
job to validate crap in bindings. If you put [strange things] in your
DT, then the kernel may or may not handle that."

If this applies here, too, the current patches are fine.

Best regards
Uwe

[1] https://patchwork.kernel.org/patch/8776441/

Patch

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..c32f6e983cf6 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -87,8 +87,9 @@  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 active low.
+- over-current-active-high: over current signal polarity is active high.
+  It's recommended to specify the over current polarity.
 - 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..18b0ca87799c 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -132,11 +132,19 @@  static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
 
 	data->dev = &misc_pdev->dev;
 
-	if (of_find_property(np, "disable-over-current", NULL))
+	/*
+	 * Check the various over current related properties. If over current
+	 * detection is disabled we're not interested in the polarity.
+	 */
+	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;
+	} else if (of_find_property(np, "over-current-active-high", NULL)) {
+		data->oc_pol_active_low = 0;
+		data->oc_pol_configured = 1;
+	} else if (of_find_property(np, "over-current-active-low", NULL)) {
+		data->oc_pol_active_low = 1;
+		data->oc_pol_configured = 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 204275f47573..640721fef0d7 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,13 @@  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 */
+
+	/* true if over-current polarity is active low */
+	unsigned int oc_pol_active_low:1;
+
+	/* true if dt specifies polarity */
+	unsigned int oc_pol_configured: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 def80ff547e4..4a8de1b37f67 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -340,11 +340,17 @@  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 */
-		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
 	} else {
-		reg &= ~(MX6_BM_OVER_CUR_DIS);
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+
+		/*
+		 * If the polarity is not configured keep it as setup by the
+		 * bootloader.
+		 */
+		if (data->oc_pol_configured && data->oc_pol_active_low)
+			reg |= MX6_BM_OVER_CUR_POLARITY;
+		else if (data->oc_pol_configured)
+			reg &= ~MX6_BM_OVER_CUR_POLARITY;
 	}
 	writel(reg, usbmisc->base + data->index * 4);
 
@@ -444,9 +450,17 @@  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 */
-		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+	} else {
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+
+		/*
+		 * If the polarity is not configured keep it as setup by the
+		 * bootloader.
+		 */
+		if (data->oc_pol_configured && data->oc_pol_active_low)
+			reg |= MX6_BM_OVER_CUR_POLARITY;
+		else if (data->oc_pol_configured)
+			reg &= ~MX6_BM_OVER_CUR_POLARITY;
 	}
 	writel(reg, usbmisc->base);