Patchwork [v2] usb: chipidea: imx: Allow OC polarity active low

login
register
mail settings
Submitter Matthew Starr
Date Nov. 30, 2018, 11:29 p.m.
Message ID <3C8219BAE02B894A9C69304A12E8AA06CA27C613@HED-Exchange.hed.local>
Download mbox | patch
Permalink /patch/669611/
State New
Headers show

Comments

Matthew Starr - Nov. 30, 2018, 11:29 p.m.
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.

Signed-off-by: Matthew Starr <mstarr@hedonline.com>
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.txt         |  4 ++--
 drivers/usb/chipidea/ci_hdrc_imx.c                   |  2 +-
 drivers/usb/chipidea/usbmisc_imx.c                   | 12 ++++++++----
 3 files changed, 11 insertions(+), 7 deletions(-)
kbuild test robot - Dec. 2, 2018, 5:57 a.m.
Hi Matthew,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.20-rc4 next-20181130]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matthew-Starr/usb-chipidea-imx-Allow-OC-polarity-active-low/20181202-132847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-x000-201848 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/usb/chipidea/usbmisc_imx.c: In function 'usbmisc_imx7d_init':
>> drivers/usb/chipidea/usbmisc_imx.c:452:2: error: expected '}' before 'else'
     else {
     ^~~~

vim +452 drivers/usb/chipidea/usbmisc_imx.c

   434	
   435	static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
   436	{
   437		struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
   438		unsigned long flags;
   439		u32 reg;
   440	
   441		if (data->index >= 1)
   442			return -EINVAL;
   443	
   444		spin_lock_irqsave(&usbmisc->lock, flags);
   445		reg = readl(usbmisc->base);
   446		if (data->disable_oc) {
   447			reg |= MX6_BM_OVER_CUR_DIS;
   448		} else if (data->oc_polarity == 1) {
   449			/* Low active */
   450			reg &= ~(MX6_BM_OVER_CUR_DIS);
   451			reg |= MX6_BM_OVER_CUR_POLARITY;
 > 452		else {
   453			reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
   454		}
   455		writel(reg, usbmisc->base);
   456	
   457		reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
   458		reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
   459		writel(reg | MX7D_USB_VBUS_WAKEUP_SOURCE_BVALID,
   460			 usbmisc->base + MX7D_USBNC_USB_CTRL2);
   461		spin_unlock_irqrestore(&usbmisc->lock, flags);
   462	
   463		usbmisc_imx7d_set_wakeup(data, false);
   464	
   465		return 0;
   466	}
   467	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Uwe Kleine-K├Ânig - Dec. 2, 2018, 9:12 p.m.
On Fri, Nov 30, 2018 at 11:29:13PM +0000, Matthew Starr wrote:
> 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.

I'd prefer to do the following instead: Keep over-current-active-high in
the binding and encourage users to specify (exactly) one of them by
issuing a warning if there is none and keep the bit as configured by the
bootloader (or by reset if the bootloader didn't touch it).

> Signed-off-by: Matthew Starr <mstarr@hedonline.com>
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt         |  4 ++--
>  drivers/usb/chipidea/ci_hdrc_imx.c                   |  2 +-
>  drivers/usb/chipidea/usbmisc_imx.c                   | 12 ++++++++----
>  3 files changed, 11 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..b39ba76b874f 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -341,10 +341,11 @@ 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);
> -	} else {
> +		/* Low active */
>  		reg &= ~(MX6_BM_OVER_CUR_DIS);
> +		reg |= MX6_BM_OVER_CUR_POLARITY;
> +	} else {
> +		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
>  	}
>  	writel(reg, usbmisc->base + data->index * 4);
>  
> @@ -445,7 +446,10 @@ 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 */
> +		/* Low active */
> +		reg &= ~(MX6_BM_OVER_CUR_DIS);
> +		reg |= MX6_BM_OVER_CUR_POLARITY;
> +	else {
>  		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);

You break machines here that use active low signaling but don't use the
new property (yet).

Also it's not nice to inverse the sematic of oc_polarity. Before it was
at least intuitively right that = 1 means active high.

As I improved my patch while being offline and before I saw your patch,
I'm sending out my patch as a better alternative.

Best regards
Uwe

Patch

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..b39ba76b874f 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -341,10 +341,11 @@  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);
-	} else {
+		/* Low active */
 		reg &= ~(MX6_BM_OVER_CUR_DIS);
+		reg |= MX6_BM_OVER_CUR_POLARITY;
+	} else {
+		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
 	}
 	writel(reg, usbmisc->base + data->index * 4);
 
@@ -445,7 +446,10 @@  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 */
+		/* Low active */
+		reg &= ~(MX6_BM_OVER_CUR_DIS);
+		reg |= MX6_BM_OVER_CUR_POLARITY;
+	else {
 		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
 	}
 	writel(reg, usbmisc->base);