[1/6] usb: chipidea: Add dynamic pinctrl selection

[1/6] usb: chipidea: Add dynamic pinctrl selection

[Bjorn Andersson]

On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote:

> Some hardware implementations require to configure pins differently
> according to the USB role (host/device), this can be an update of the
> pins routing or a simple GPIO value change.
> 
> This patch introduces new optional "host" and "device" pinctrls.
> If these pinctrls are defined by the device, they are respectively
> selected on host/device role start.
> 
> If a default pinctrl exist, it is restored on host/device role stop.
> 

The implementation looks reasonable, but we're actually just toggling a
gpio using pinctrl states. Do you see any reason not to control this mux
through the gpio api?

Regards,
Bjorn

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/usb/chipidea/core.c  | 19 +++++++++++++++++++
>  drivers/usb/chipidea/host.c  |  9 +++++++++
>  drivers/usb/chipidea/udc.c   |  9 +++++++++
>  include/linux/usb/chipidea.h |  6 ++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 85fc6db..03e52fc 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -46,6 +46,7 @@
>  #include <linux/extcon.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/module.h>
>  #include <linux/idr.h>
>  #include <linux/interrupt.h>
> @@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev,
>  		else
>  			cable->connected = false;
>  	}
> +
> +	platdata->pctl = devm_pinctrl_get(dev);
> +	if (!IS_ERR(platdata->pctl)) {
> +		struct pinctrl_state *p;
> +
> +		p = pinctrl_lookup_state(platdata->pctl, "default");
> +		if (!IS_ERR(p))
> +			platdata->pins_default = p;
> +
> +		p = pinctrl_lookup_state(platdata->pctl, "host");
> +		if (!IS_ERR(p))
> +			platdata->pins_host = p;
> +
> +		p = pinctrl_lookup_state(platdata->pctl, "device");
> +		if (!IS_ERR(p))
> +			platdata->pins_device = p;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index af45aa32..55dbd49 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -13,6 +13,7 @@
>  #include <linux/usb/hcd.h>
>  #include <linux/usb/chipidea.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include "../host/ehci.h"
>  
> @@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci)
>  		}
>  	}
>  
> +	if (ci->platdata->pins_host)
> +		pinctrl_select_state(ci->platdata->pctl,
> +				     ci->platdata->pins_host);
> +
>  	ret = usb_add_hcd(hcd, 0, 0);
>  	if (ret) {
>  		goto disable_reg;
> @@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci)
>  	}
>  	ci->hcd = NULL;
>  	ci->otg.host = NULL;
> +
> +	if (ci->platdata->pins_host && ci->platdata->pins_default)
> +		pinctrl_select_state(ci->platdata->pctl,
> +				     ci->platdata->pins_default);
>  }
>  
>  
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 9852ec5..c04384e 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -19,6 +19,7 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg-fsm.h>
>  #include <linux/usb/chipidea.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include "ci.h"
>  #include "udc.h"
> @@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>  
>  static int udc_id_switch_for_device(struct ci_hdrc *ci)
>  {
> +	if (ci->platdata->pins_device)
> +		pinctrl_select_state(ci->platdata->pctl,
> +				     ci->platdata->pins_device);
> +
>  	if (ci->is_otg)
>  		/* Clear and enable BSV irq */
>  		hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> @@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
>  		hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
>  
>  	ci->vbus_active = 0;
> +
> +	if (ci->platdata->pins_device && ci->platdata->pins_default)
> +		pinctrl_select_state(ci->platdata->pctl,
> +				     ci->platdata->pins_default);
>  }
>  
>  /**
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 07f9936..63758c3 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -77,6 +77,12 @@ struct ci_hdrc_platform_data {
>  	struct ci_hdrc_cable		vbus_extcon;
>  	struct ci_hdrc_cable		id_extcon;
>  	u32			phy_clkgate_delay_us;
> +
> +	/* pins */
> +	struct pinctrl *pctl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_host;
> +	struct pinctrl_state *pins_device;
>  };
>  
>  /* Default offset of capability registers */
> -- 
> 2.7.4
>

[1/6] usb: chipidea: Add dynamic pinctrl selection

[Loic Poulain]

On 23 August 2018 at 12:11, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 21, 2018 at 4:57 PM Loic Poulain <loic.poulain@linaro.org> wrote:
>>
>> Some hardware implementations require to configure pins differently
>> according to the USB role (host/device), this can be an update of the
>> pins routing or a simple GPIO value change.
>>
>> This patch introduces new optional "host" and "device" pinctrls.
>> If these pinctrls are defined by the device, they are respectively
>> selected on host/device role start.
>>
>> If a default pinctrl exist, it is restored on host/device role stop.
>
>>  #include <linux/extcon.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>
> A nit: Even in this context it would be nice to keep *some* order.
>

Ok.

>>  #include <linux/module.h>
>>  #include <linux/idr.h>
>>  #include <linux/interrupt.h>
>
>> +               p = pinctrl_lookup_state(platdata->pctl, "default");
>> +               p = pinctrl_lookup_state(platdata->pctl, "host");
>> +               p = pinctrl_lookup_state(platdata->pctl, "device");
>
> I'm wondering if we have any rules applied to these names.

I suppose i have document this in the chipidea DT binding doc.

>
>>  #include <linux/usb/hcd.h>
>>  #include <linux/usb/chipidea.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <linux/pinctrl/consumer.h>
>
> Ditto about ordering.
>
>> +       if (ci->platdata->pins_host)
>> +               pinctrl_select_state(ci->platdata->pctl,
>> +                                    ci->platdata->pins_host);
>
> Hmm... Do we need to have a condition above?
>

Yes, since these pinctrls are optional and can be null.

>> +       if (ci->platdata->pins_host && ci->platdata->pins_default)
>> +               pinctrl_select_state(ci->platdata->pctl,
>> +                                    ci->platdata->pins_default);
>
> Ditto about conditional.
>
>>  #include <linux/usb/gadget.h>
>>  #include <linux/usb/otg-fsm.h>
>>  #include <linux/usb/chipidea.h>
>> +#include <linux/pinctrl/consumer.h>
>
> Ditto about ordering.
>
>> +       if (ci->platdata->pins_device)
>> +               pinctrl_select_state(ci->platdata->pctl,
>> +                                    ci->platdata->pins_device);
>
>
>> +       if (ci->platdata->pins_device && ci->platdata->pins_default)
>> +               pinctrl_select_state(ci->platdata->pctl,
>> +                                    ci->platdata->pins_default);
>
> Ditto about conditional.
>
> --
> With Best Regards,
> Andy Shevchenko

Regards,
Loic

[1/6] usb: chipidea: Add dynamic pinctrl selection

[Loic Poulain]

Hi Bjorn,

On 23 August 2018 at 16:53, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote:
>
>> Some hardware implementations require to configure pins differently
>> according to the USB role (host/device), this can be an update of the
>> pins routing or a simple GPIO value change.
>>
>> This patch introduces new optional "host" and "device" pinctrls.
>> If these pinctrls are defined by the device, they are respectively
>> selected on host/device role start.
>>
>> If a default pinctrl exist, it is restored on host/device role stop.
>>
>
> The implementation looks reasonable, but we're actually just toggling a
> gpio using pinctrl states. Do you see any reason not to control this mux
> through the gpio api?
>

Actually, two gpios (including hub reset one), but you're right.
Point is that these gpios are very specific to the Dragonboard layout and
not related to USB controller itself (external mux), so I'm not sure it makes
sense to control a 'mux' GPIO from the chipidea driver. That's why I used
pinctrl which is more generic and hides the underlying operations (a simple
GPIO toggling in our case).

But let me know if there is a better way.


Regards,
Loic

[1/6] usb: chipidea: Add dynamic pinctrl selection

[Andy Shevchenko]

On Tue, Aug 21, 2018 at 4:57 PM Loic Poulain <loic.poulain@linaro.org> wrote:
>
> Some hardware implementations require to configure pins differently
> according to the USB role (host/device), this can be an update of the
> pins routing or a simple GPIO value change.
>
> This patch introduces new optional "host" and "device" pinctrls.
> If these pinctrls are defined by the device, they are respectively
> selected on host/device role start.
>
> If a default pinctrl exist, it is restored on host/device role stop.

>  #include <linux/extcon.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>

A nit: Even in this context it would be nice to keep *some* order.

>  #include <linux/module.h>
>  #include <linux/idr.h>
>  #include <linux/interrupt.h>

> +               p = pinctrl_lookup_state(platdata->pctl, "default");
> +               p = pinctrl_lookup_state(platdata->pctl, "host");
> +               p = pinctrl_lookup_state(platdata->pctl, "device");

I'm wondering if we have any rules applied to these names.

>  #include <linux/usb/hcd.h>
>  #include <linux/usb/chipidea.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>

Ditto about ordering.

> +       if (ci->platdata->pins_host)
> +               pinctrl_select_state(ci->platdata->pctl,
> +                                    ci->platdata->pins_host);

Hmm... Do we need to have a condition above?

> +       if (ci->platdata->pins_host && ci->platdata->pins_default)
> +               pinctrl_select_state(ci->platdata->pctl,
> +                                    ci->platdata->pins_default);

Ditto about conditional.

>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg-fsm.h>
>  #include <linux/usb/chipidea.h>
> +#include <linux/pinctrl/consumer.h>

Ditto about ordering.

> +       if (ci->platdata->pins_device)
> +               pinctrl_select_state(ci->platdata->pctl,
> +                                    ci->platdata->pins_device);


> +       if (ci->platdata->pins_device && ci->platdata->pins_default)
> +               pinctrl_select_state(ci->platdata->pctl,
> +                                    ci->platdata->pins_default);

Ditto about conditional.

[1/6] usb: chipidea: Add dynamic pinctrl selection

[Loic Poulain]

Some hardware implementations require to configure pins differently
according to the USB role (host/device), this can be an update of the
pins routing or a simple GPIO value change.

This patch introduces new optional "host" and "device" pinctrls.
If these pinctrls are defined by the device, they are respectively
selected on host/device role start.

If a default pinctrl exist, it is restored on host/device role stop.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/usb/chipidea/core.c  | 19 +++++++++++++++++++
 drivers/usb/chipidea/host.c  |  9 +++++++++
 drivers/usb/chipidea/udc.c   |  9 +++++++++
 include/linux/usb/chipidea.h |  6 ++++++
 4 files changed, 43 insertions(+)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 85fc6db..03e52fc 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -46,6 +46,7 @@
 #include <linux/extcon.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/module.h>
 #include <linux/idr.h>
 #include <linux/interrupt.h>
@@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev,
 		else
 			cable->connected = false;
 	}
+
+	platdata->pctl = devm_pinctrl_get(dev);
+	if (!IS_ERR(platdata->pctl)) {
+		struct pinctrl_state *p;
+
+		p = pinctrl_lookup_state(platdata->pctl, "default");
+		if (!IS_ERR(p))
+			platdata->pins_default = p;
+
+		p = pinctrl_lookup_state(platdata->pctl, "host");
+		if (!IS_ERR(p))
+			platdata->pins_host = p;
+
+		p = pinctrl_lookup_state(platdata->pctl, "device");
+		if (!IS_ERR(p))
+			platdata->pins_device = p;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index af45aa32..55dbd49 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -13,6 +13,7 @@
 #include <linux/usb/hcd.h>
 #include <linux/usb/chipidea.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "../host/ehci.h"
 
@@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci)
 		}
 	}
 
+	if (ci->platdata->pins_host)
+		pinctrl_select_state(ci->platdata->pctl,
+				     ci->platdata->pins_host);
+
 	ret = usb_add_hcd(hcd, 0, 0);
 	if (ret) {
 		goto disable_reg;
@@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci)
 	}
 	ci->hcd = NULL;
 	ci->otg.host = NULL;
+
+	if (ci->platdata->pins_host && ci->platdata->pins_default)
+		pinctrl_select_state(ci->platdata->pctl,
+				     ci->platdata->pins_default);
 }
 
 
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 9852ec5..c04384e 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -19,6 +19,7 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg-fsm.h>
 #include <linux/usb/chipidea.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "ci.h"
 #include "udc.h"
@@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
 
 static int udc_id_switch_for_device(struct ci_hdrc *ci)
 {
+	if (ci->platdata->pins_device)
+		pinctrl_select_state(ci->platdata->pctl,
+				     ci->platdata->pins_device);
+
 	if (ci->is_otg)
 		/* Clear and enable BSV irq */
 		hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
@@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
 		hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
 
 	ci->vbus_active = 0;
+
+	if (ci->platdata->pins_device && ci->platdata->pins_default)
+		pinctrl_select_state(ci->platdata->pctl,
+				     ci->platdata->pins_default);
 }
 
 /**
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 07f9936..63758c3 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -77,6 +77,12 @@ struct ci_hdrc_platform_data {
 	struct ci_hdrc_cable		vbus_extcon;
 	struct ci_hdrc_cable		id_extcon;
 	u32			phy_clkgate_delay_us;
+
+	/* pins */
+	struct pinctrl *pctl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_host;
+	struct pinctrl_state *pins_device;
 };
 
 /* Default offset of capability registers */