[RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Antony Pavlov]

This patch retrieves and configures the vbus control gpio via
the device tree.

This patch is based on a ehci-s5p.c commit fd81d59c90d38661
("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index bd7082f2..0d95ced 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -28,6 +28,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
@@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
 	.power_off =		ehci_platform_power_off,
 };
 
+static void setup_vbus_gpio(struct device *dev)
+{
+	int err;
+	int gpio;
+
+	if (!dev->of_node)
+		return;
+
+	gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
+	if (!gpio_is_valid(gpio))
+		return;
+
+	err = devm_gpio_request_one(dev, gpio,
+				GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
+				"ehci_vbus_gpio");
+	if (err)
+		dev_err(dev, "can't request ehci vbus gpio %d", gpio);
+}
+
 static int ehci_platform_probe(struct platform_device *dev)
 {
 	struct usb_hcd *hcd;
@@ -174,6 +194,8 @@ static int ehci_platform_probe(struct platform_device *dev)
 		return irq;
 	}
 
+	setup_vbus_gpio(&dev->dev);
+
 	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
 			     dev_name(&dev->dev));
 	if (!hcd)
-- 
2.7.0

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Marek Vasut]

On Tuesday, February 09, 2016 at 09:13:53 AM, Antony Pavlov wrote:
> This patch retrieves and configures the vbus control gpio via
> the device tree.
> 
> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

I think this patch will not be needed if you switch the ar9331 to chipidea hdrc
driver. There is CI HDRC in the ar9331.

Best regards,
Marek Vasut

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Alban]

On Tue,  9 Feb 2016 11:13:53 +0300
Antony Pavlov <antonynpavlov@gmail.com> wrote:

> This patch retrieves and configures the vbus control gpio via
> the device tree.

Wouldn't using a regulator be better than hard coding the GPIO case?

Alban

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Antony Pavlov]

On Tue, 9 Feb 2016 23:15:20 +0100
Alban <albeu@free.fr> wrote:

> On Tue,  9 Feb 2016 11:13:53 +0300
> Antony Pavlov <antonynpavlov@gmail.com> wrote:
> 
> > This patch retrieves and configures the vbus control gpio via
> > the device tree.
> 
> Wouldn't using a regulator be better than hard coding the GPIO case?
> 

Marek Vasut has noted that it is possible to use compatible = "chipidea,usb2",
which makes it possible to connect fixed vbus regulator via "vbus-supply" property.

I'll try to use fixed-regulator in RFC v6.

-- 
Best regards,
  Antony Pavlov

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Alan Stern]

On Tue, 9 Feb 2016, Antony Pavlov wrote:

> This patch retrieves and configures the vbus control gpio via
> the device tree.
> 
> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index bd7082f2..0d95ced 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -28,6 +28,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset.h>
> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
>  	.power_off =		ehci_platform_power_off,
>  };
>  
> +static void setup_vbus_gpio(struct device *dev)
> +{
> +	int err;
> +	int gpio;
> +
> +	if (!dev->of_node)
> +		return;
> +
> +	gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
> +	if (!gpio_is_valid(gpio))
> +		return;
> +
> +	err = devm_gpio_request_one(dev, gpio,
> +				GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
> +				"ehci_vbus_gpio");
> +	if (err)
> +		dev_err(dev, "can't request ehci vbus gpio %d", gpio);

I don't understand this.  If you get an error here, what's the point of 
allowing the probe to continue?  Shouldn't you return an error code so 
the probe will fail?

Alan Stern

> +}
> +
>  static int ehci_platform_probe(struct platform_device *dev)
>  {
>  	struct usb_hcd *hcd;
> @@ -174,6 +194,8 @@ static int ehci_platform_probe(struct platform_device *dev)
>  		return irq;
>  	}
>  
> +	setup_vbus_gpio(&dev->dev);
> +
>  	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
>  			     dev_name(&dev->dev));
>  	if (!hcd)
> 

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Marek Vasut]

On 02/18/2016 05:12 PM, Alan Stern wrote:
> On Tue, 9 Feb 2016, Antony Pavlov wrote:
> 
>> This patch retrieves and configures the vbus control gpio via
>> the device tree.
>>
>> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
>> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
>>
>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-usb@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index bd7082f2..0d95ced 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/reset.h>
>> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
>>  	.power_off =		ehci_platform_power_off,
>>  };
>>  
>> +static void setup_vbus_gpio(struct device *dev)
>> +{
>> +	int err;
>> +	int gpio;
>> +
>> +	if (!dev->of_node)
>> +		return;
>> +
>> +	gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
>> +	if (!gpio_is_valid(gpio))
>> +		return;
>> +
>> +	err = devm_gpio_request_one(dev, gpio,
>> +				GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
>> +				"ehci_vbus_gpio");
>> +	if (err)
>> +		dev_err(dev, "can't request ehci vbus gpio %d", gpio);
> 
> I don't understand this.  If you get an error here, what's the point of 
> allowing the probe to continue?  Shouldn't you return an error code so 
> the probe will fail?

The idea is I believe that if there is no vbus gpio specified, the port
might just not have vbus control, so the probe can continue. But this
patch is irrelevant anyway, since Alexey will switch to CI HDRC driver
and use standard regulator, as it should be done.

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Antony Pavlov]

On Thu, 18 Feb 2016 11:12:43 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 9 Feb 2016, Antony Pavlov wrote:
> 
> > This patch retrieves and configures the vbus control gpio via
> > the device tree.
> > 
> > This patch is based on a ehci-s5p.c commit fd81d59c90d38661
> > ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
> > 
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> > index bd7082f2..0d95ced 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> > @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
> >  	.power_off =		ehci_platform_power_off,
> >  };
> >  
> > +static void setup_vbus_gpio(struct device *dev)
> > +{
> > +	int err;
> > +	int gpio;
> > +
> > +	if (!dev->of_node)
> > +		return;
> > +
> > +	gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
> > +	if (!gpio_is_valid(gpio))
> > +		return;
> > +
> > +	err = devm_gpio_request_one(dev, gpio,
> > +				GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
> > +				"ehci_vbus_gpio");
> > +	if (err)
> > +		dev_err(dev, "can't request ehci vbus gpio %d", gpio);
>
>
> I don't understand this.  If you get an error here, what's the point of 
> allowing the probe to continue?  Shouldn't you return an error code so 
> the probe will fail?

Please ignore the 'usb: ehci: add vbus-gpio parameter' patch!

In the new AR9331 patchseries I use chipidea USB driver (thanks to Marek for the suggestion)
in the AR9331 dtsi-file:

        usb: usb@1b000100 {
                compatible = "chipidea,usb2";
                reg = <0x1b000000 0x200>;

                interrupt-parent = <&cpuintc>;
                interrupts = <3>;
                resets = <&rst 5>;

                phy-names = "usb-phy";
                phys = <&usb_phy>;

                status = "disabled";
        };


so I use regulator in the TL-MR3020 board dts file:

        reg_usb_vbus: reg_usb_vbus {
                compatible = "regulator-fixed";
                regulator-name = "usb_vbus";
                regulator-min-microvolt = <5000000>;
                regulator-max-microvolt = <5000000>;
                gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
                enable-active-high;
        };

&usb {
        dr_mode = "host";
        vbus-supply = <&reg_usb_vbus>;
        status = "okay";
};

As a result there is no need in adding vbus-gpio parameter to ehci anymore!

> > +}
> > +
> >  static int ehci_platform_probe(struct platform_device *dev)
> >  {
> >  	struct usb_hcd *hcd;
> > @@ -174,6 +194,8 @@ static int ehci_platform_probe(struct platform_device *dev)
> >  		return irq;
> >  	}
> >  
> > +	setup_vbus_gpio(&dev->dev);
> > +
> >  	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
> >  			     dev_name(&dev->dev));
> >  	if (!hcd)
> > 
> 


-- 
-- 
Best regards,
  Antony Pavlov

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Sergei Shtylyov]

On 02/18/2016 09:06 PM, Antony Pavlov wrote:

>>> This patch retrieves and configures the vbus control gpio via
>>> the device tree.
>>>
>>> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
>>> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
>>>
>>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>   drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index bd7082f2..0d95ced 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -28,6 +28,7 @@
>>>   #include <linux/io.h>
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/reset.h>
>>> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
>>>   	.power_off =		ehci_platform_power_off,
>>>   };
>>>
>>> +static void setup_vbus_gpio(struct device *dev)
>>> +{
>>> +	int err;
>>> +	int gpio;
>>> +
>>> +	if (!dev->of_node)
>>> +		return;
>>> +
>>> +	gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
>>> +	if (!gpio_is_valid(gpio))
>>> +		return;
>>> +
>>> +	err = devm_gpio_request_one(dev, gpio,
>>> +				GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
>>> +				"ehci_vbus_gpio");
>>> +	if (err)
>>> +		dev_err(dev, "can't request ehci vbus gpio %d", gpio);
>>
>>
>> I don't understand this.  If you get an error here, what's the point of
>> allowing the probe to continue?  Shouldn't you return an error code so
>> the probe will fail?
>
> Please ignore the 'usb: ehci: add vbus-gpio parameter' patch!
>
> In the new AR9331 patchseries I use chipidea USB driver (thanks to Marek for the suggestion)
> in the AR9331 dtsi-file:
>
>          usb: usb@1b000100 {
>                  compatible = "chipidea,usb2";
>                  reg = <0x1b000000 0x200>;
>
>                  interrupt-parent = <&cpuintc>;
>                  interrupts = <3>;
>                  resets = <&rst 5>;
>
>                  phy-names = "usb-phy";
>                  phys = <&usb_phy>;
>
>                  status = "disabled";
>          };
>
>
> so I use regulator in the TL-MR3020 board dts file:
>
>          reg_usb_vbus: reg_usb_vbus {
>                  compatible = "regulator-fixed";
>                  regulator-name = "usb_vbus";
>                  regulator-min-microvolt = <5000000>;

    Not 0?

>                  regulator-max-microvolt = <5000000>;
>                  gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;

    Where's the switch if both voltages are equal?

>                  enable-active-high;
>          };
>
> &usb {
>          dr_mode = "host";
>          vbus-supply = <&reg_usb_vbus>;
>          status = "okay";
> };
>
> As a result there is no need in adding vbus-gpio parameter to ehci anymore!
[...]

MBR, Sergei

Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter

[Antony Pavlov]

On Thu, 18 Feb 2016 21:31:20 +0300
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

> On 02/18/2016 09:06 PM, Antony Pavlov wrote:
[...]
> > so I use regulator in the TL-MR3020 board dts file:
> >
> >          reg_usb_vbus: reg_usb_vbus {
> >                  compatible = "regulator-fixed";
> >                  regulator-name = "usb_vbus";
> >                  regulator-min-microvolt = <5000000>;
> 
>     Not 0?
> 
> >                  regulator-max-microvolt = <5000000>;
> >                  gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
> 
>     Where's the switch if both voltages are equal?

Here is a quote from linux/Documentation/devicetree/bindings/regulator/fixed-regulator.txt

        Any property defined as part of the core regulator
        binding, defined in regulator.txt, can also be used.
        However a fixed voltage regulator is expected to have the
        regulator-min-microvolt and regulator-max-microvolt
        to be the same.

Moreover please see this of_get_fixed_voltage_config() code fragment
(please see linux/drivers/regulator/fixed.c for details):

	if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
		config->microvolts = init_data->constraints.min_uV;
	} else {
		dev_err(dev,
			 "Fixed regulator specified with variable voltages\n");
		return ERR_PTR(-EINVAL);
	}

-- 
Best regards,
  Antony Pavlov