[PATCH 0/4] usb: tegra: Replace nvidia,vbus-gpio property with vbus-supply

[PATCH 0/4] usb: tegra: Replace nvidia,vbus-gpio property with vbus-supply

[Mikko Perttunen]

This patchset removes the ehci-tegra device tree property nvidia,vbus-gpio
and adds support for the vbus-supply property to phy-tegra-usb.

Mikko Perttunen (4):
  usb: phy: tegra: Add support for device tree-based vbus regulator
    control
  usb: host: tegra: Remove direct vbus regulator control using GPIOs
  Documentation: devicetree: phy-tegra-usb: Add vbus-supply property for
    host mode PHYs
  ARM: dts: tegra20: Remove obsolete vbus-gpio properties

 .../bindings/usb/nvidia,tegra20-usb-phy.txt        |  3 ++
 arch/arm/boot/dts/tegra20-seaboard.dts             |  1 -
 arch/arm/boot/dts/tegra20-trimslice.dts            |  1 -
 arch/arm/boot/dts/tegra20-whistler.dts             |  2 --
 drivers/usb/host/ehci-tegra.c                      | 34 +---------------------
 drivers/usb/phy/phy-tegra-usb.c                    | 24 +++++++++++++++
 include/linux/usb/tegra_usb_phy.h                  |  1 +
 7 files changed, 29 insertions(+), 37 deletions(-)

-- 
1.8.1.5

[PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Mikko Perttunen]

After this patch, usb vbus regulators for tegra usb phy devices can be specified
with the device tree attribute vbus-supply = <&x> where x is a regulator defined
in the device tree.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/phy/phy-tegra-usb.c   | 24 ++++++++++++++++++++++++
 include/linux/usb/tegra_usb_phy.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index d7d6bd7..6122590 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -34,6 +34,7 @@
 #include <asm/mach-types.h>
 #include <linux/usb/ehci_def.h>
 #include <linux/usb/tegra_usb_phy.h>
+#include <linux/regulator/consumer.h>
 
 #define ULPI_VIEWPORT		0x170
 
@@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
 		return PTR_ERR(phy->pad_clk);
 	}
 
+	phy->vbus = devm_regulator_get(phy->dev, "vbus");
+	/* On some boards, the VBUS regulator doesn't need to be controlled */
+	if (IS_ERR(phy->vbus)) {
+		if (PTR_ERR(phy->vbus) == -ENODEV) {
+			dev_notice(phy->dev, "no vbus regulator");
+			phy->vbus = NULL;
+		} else {
+			return PTR_ERR(phy->vbus);
+		}
+	}
+
 	return 0;
 }
 
 static void utmip_pad_power_on(struct tegra_usb_phy *phy)
 {
 	unsigned long val, flags;
+	int err;
 	void __iomem *base = phy->pad_regs;
 
 	clk_prepare_enable(phy->pad_clk);
@@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy)
 	spin_unlock_irqrestore(&utmip_pad_lock, flags);
 
 	clk_disable_unprepare(phy->pad_clk);
+
+	if (phy->vbus) {
+		err = regulator_enable(phy->vbus);
+		if (err)
+			dev_err(phy->dev,
+				"failed to enable usb vbus regulator: %d\n",
+				err);
+	}
 }
 
 static int utmip_pad_power_off(struct tegra_usb_phy *phy)
@@ -306,6 +327,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy)
 
 	clk_disable_unprepare(phy->pad_clk);
 
+	if (phy->vbus)
+		regulator_disable(phy->vbus);
+
 	return 0;
 }
 
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index d2ca919..2b5fa94 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -55,6 +55,7 @@ struct tegra_usb_phy {
 	struct clk *clk;
 	struct clk *pll_u;
 	struct clk *pad_clk;
+	struct regulator *vbus;
 	enum tegra_usb_phy_mode mode;
 	void *config;
 	struct usb_phy *ulpi;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] usb: host: tegra: Remove direct vbus regulator control using GPIOs

[Mikko Perttunen]

The tegra ehci driver has enabled USB vbus regulators directly using
GPIOs and the device tree attribute nvidia,vbus-gpio. This is ugly
and causes error messages on boot when both the regulator driver
and the ehci driver want access to the same GPIO.

This patch removes this mechanism of vbus control, so that an actual
regulator interface can be used by the phy layer to control the
usb vbus regulator.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/host/ehci-tegra.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index a1cec77..51cf1d2 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -328,33 +328,6 @@ static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 	free_dma_aligned_buffer(urb);
 }
 
-static int setup_vbus_gpio(struct platform_device *pdev,
-			   struct tegra_ehci_platform_data *pdata)
-{
-	int err = 0;
-	int gpio;
-
-	gpio = pdata->vbus_gpio;
-	if (!gpio_is_valid(gpio))
-		gpio = of_get_named_gpio(pdev->dev.of_node,
-					 "nvidia,vbus-gpio", 0);
-	if (!gpio_is_valid(gpio))
-		return 0;
-
-	err = gpio_request(gpio, "vbus_gpio");
-	if (err) {
-		dev_err(&pdev->dev, "can't request vbus gpio %d", gpio);
-		return err;
-	}
-	err = gpio_direction_output(gpio, 1);
-	if (err) {
-		dev_err(&pdev->dev, "can't enable vbus\n");
-		return err;
-	}
-
-	return err;
-}
-
 static int tegra_ehci_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -382,14 +355,11 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	if (!pdev->dev.coherent_dma_mask)
 		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
-	setup_vbus_gpio(pdev, pdata);
-
 	hcd = usb_create_hcd(&tegra_ehci_hc_driver, &pdev->dev,
 					dev_name(&pdev->dev));
 	if (!hcd) {
 		dev_err(&pdev->dev, "Unable to create HCD\n");
-		err = -ENOMEM;
-		goto cleanup_vbus_gpio;
+		return -ENOMEM;
 	}
 	platform_set_drvdata(pdev, hcd);
 	ehci = hcd_to_ehci(hcd);
@@ -505,8 +475,6 @@ cleanup_clk_get:
 	clk_put(tegra->clk);
 cleanup_hcd_create:
 	usb_put_hcd(hcd);
-cleanup_vbus_gpio:
-	/* FIXME: Undo setup_vbus_gpio() here */
 	return err;
 }
 
-- 
1.8.1.5

[PATCH 3/4] Documentation: devicetree: phy-tegra-usb: Add vbus-supply property for host mode PHYs

[Mikko Perttunen]

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
index c4c9e9e..92348c9 100644
--- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
@@ -48,5 +48,8 @@ Optional properties:
       peripheral means it is device controller
       otg means it can operate as either ("on the go")
 
+Optional properties for dr_mode == host:
+  - vbus-supply: regulator for VBUS
+
 Required properties for dr_mode == otg:
   - vbus-supply: regulator for VBUS
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Mikko Perttunen]

USB VBUS regulators are now specified with the vbus-supply property
instead of vbus-gpio, so remove the obsolete properties.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-seaboard.dts  | 1 -
 arch/arm/boot/dts/tegra20-trimslice.dts | 1 -
 arch/arm/boot/dts/tegra20-whistler.dts  | 2 --
 3 files changed, 4 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index ab177b4..2ab4775 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -566,7 +566,6 @@
 
 	usb@c5000000 {
 		status = "okay";
-		nvidia,vbus-gpio = <&gpio TEGRA_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
 		dr_mode = "otg";
 	};
 
diff --git a/arch/arm/boot/dts/tegra20-trimslice.dts b/arch/arm/boot/dts/tegra20-trimslice.dts
index 1701599..bea4aaa 100644
--- a/arch/arm/boot/dts/tegra20-trimslice.dts
+++ b/arch/arm/boot/dts/tegra20-trimslice.dts
@@ -312,7 +312,6 @@
 
 	usb@c5000000 {
 		status = "okay";
-		nvidia,vbus-gpio = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
 	};
 
 	usb-phy@c5000000 {
diff --git a/arch/arm/boot/dts/tegra20-whistler.dts b/arch/arm/boot/dts/tegra20-whistler.dts
index ea078ab..e07f210 100644
--- a/arch/arm/boot/dts/tegra20-whistler.dts
+++ b/arch/arm/boot/dts/tegra20-whistler.dts
@@ -509,7 +509,6 @@
 
 	usb@c5000000 {
 		status = "okay";
-		nvidia,vbus-gpio = <&tca6416 0 GPIO_ACTIVE_HIGH>;
 	};
 
 	usb-phy@c5000000 {
@@ -519,7 +518,6 @@
 
 	usb@c5008000 {
 		status = "okay";
-		nvidia,vbus-gpio = <&tca6416 1 GPIO_ACTIVE_HIGH>;
 	};
 
 	usb-phy@c5008000 {
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

On Wed, Jun 26, 2013 at 12:59:41PM +0300, Mikko Perttunen wrote:
> USB VBUS regulators are now specified with the vbus-supply property
> instead of vbus-gpio, so remove the obsolete properties.
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra20-seaboard.dts  | 1 -
>  arch/arm/boot/dts/tegra20-trimslice.dts | 1 -
>  arch/arm/boot/dts/tegra20-whistler.dts  | 2 --
>  3 files changed, 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
> index ab177b4..2ab4775 100644
> --- a/arch/arm/boot/dts/tegra20-seaboard.dts
> +++ b/arch/arm/boot/dts/tegra20-seaboard.dts
> @@ -566,7 +566,6 @@
>  
>  	usb@c5000000 {
>  		status = "okay";
> -		nvidia,vbus-gpio = <&gpio TEGRA_GPIO(D, 0) GPIO_ACTIVE_HIGH>;

silly question but don't you need to put a phandle to the VBUS regulator
here ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Mikko Perttunen]

On Wed, 26 Jun 2013 13:17:58 +0300, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:

> * PGP Signed by an unknown key
>
> On Wed, Jun 26, 2013 at 12:59:41PM +0300, Mikko Perttunen wrote:
>> USB VBUS regulators are now specified with the vbus-supply property
>> instead of vbus-gpio, so remove the obsolete properties.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/tegra20-seaboard.dts  | 1 -
>>  arch/arm/boot/dts/tegra20-trimslice.dts | 1 -
>>  arch/arm/boot/dts/tegra20-whistler.dts  | 2 --
>>  3 files changed, 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts  
>> b/arch/arm/boot/dts/tegra20-seaboard.dts
>> index ab177b4..2ab4775 100644
>> --- a/arch/arm/boot/dts/tegra20-seaboard.dts
>> +++ b/arch/arm/boot/dts/tegra20-seaboard.dts
>> @@ -566,7 +566,6 @@
>>
>>  	usb@c5000000 {
>>  		status = "okay";
>> -		nvidia,vbus-gpio = <&gpio TEGRA_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
>
> silly question but don't you need to put a phandle to the VBUS regulator
> here ?
>

It's already there in the corresponding usb-phy node. They were added by
Venu Byravarasu in commit 4c94c8b5 "ARM: tegra: update device trees for
USB binding rework".

Re: [PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi,

On Wed, Jun 26, 2013 at 01:22:07PM +0300, Mikko Perttunen wrote:
> On Wed, 26 Jun 2013 13:17:58 +0300, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> 
> >* PGP Signed by an unknown key
> >
> >On Wed, Jun 26, 2013 at 12:59:41PM +0300, Mikko Perttunen wrote:
> >>USB VBUS regulators are now specified with the vbus-supply property
> >>instead of vbus-gpio, so remove the obsolete properties.
> >>
> >>Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>---
> >> arch/arm/boot/dts/tegra20-seaboard.dts  | 1 -
> >> arch/arm/boot/dts/tegra20-trimslice.dts | 1 -
> >> arch/arm/boot/dts/tegra20-whistler.dts  | 2 --
> >> 3 files changed, 4 deletions(-)
> >>
> >>diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts
> >>b/arch/arm/boot/dts/tegra20-seaboard.dts
> >>index ab177b4..2ab4775 100644
> >>--- a/arch/arm/boot/dts/tegra20-seaboard.dts
> >>+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
> >>@@ -566,7 +566,6 @@
> >>
> >> 	usb@c5000000 {
> >> 		status = "okay";
> >>-		nvidia,vbus-gpio = <&gpio TEGRA_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
> >
> >silly question but don't you need to put a phandle to the VBUS regulator
> >here ?
> >
> 
> It's already there in the corresponding usb-phy node. They were added by
> Venu Byravarasu in commit 4c94c8b5 "ARM: tegra: update device trees for
> USB binding rework".

aaa alright, my bad :-)

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Stephen Warren]

On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> After this patch, usb vbus regulators for tegra usb phy devices can be specified
> with the device tree attribute vbus-supply = <&x> where x is a regulator defined
> in the device tree.

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c

> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  		return PTR_ERR(phy->pad_clk);
>  	}
>  
> +	phy->vbus = devm_regulator_get(phy->dev, "vbus");
> +	/* On some boards, the VBUS regulator doesn't need to be controlled */
> +	if (IS_ERR(phy->vbus)) {
> +		if (PTR_ERR(phy->vbus) == -ENODEV) {
> +			dev_notice(phy->dev, "no vbus regulator");
> +			phy->vbus = NULL;
> +		} else {
> +			return PTR_ERR(phy->vbus);
> +		}
> +	}

I think this code should be added to some more core initialization
function; IIRC, there are separate utmip_pad_open() and some other
function for ULPI mode, and in the future there may be more for HSIC, etc.

For the error-handling, I think it'd be better to do:

* If property doesn't exist, set phy->vbus to some error value, e.g.
ERR_PTR(-ENODEV).
* If property does exist, call devm_regulator_get().
** If devm_regulator_get() returned any error, return it.

Or, does devm_regulator_get() return -ENODEV if-and-only-if the
vbus-supply DT property does not exist?

and ...

> @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>  	spin_unlock_irqrestore(&utmip_pad_lock, flags);
>  
>  	clk_disable_unprepare(phy->pad_clk);
> +
> +	if (phy->vbus) {

Here, check if (IS_ERR(phy->vbus) instead. The reason is if
devm_regulator_get() returns either a valid value or an error-pointer,
then NULL could in theory be a valid value (it's up the the regulator
API to determine that), and hence this code shouldn't assume that it can
use NULL to represent "no regulator".

Re: [PATCH 2/4] usb: host: tegra: Remove direct vbus regulator control using GPIOs

[Stephen Warren]

On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> The tegra ehci driver has enabled USB vbus regulators directly using
> GPIOs and the device tree attribute nvidia,vbus-gpio. This is ugly
> and causes error messages on boot when both the regulator driver
> and the ehci driver want access to the same GPIO.
> 
> This patch removes this mechanism of vbus control, so that an actual
> regulator interface can be used by the phy layer to control the
> usb vbus regulator.

This should be squashed into patch 1/4. The reason is that if just patch
1/4 is applied, then both ehci-tegra.c's legacy raw GPIO handling and
phy-tegra-usb.c's new regulator-based code will attempt to
gpio_request() the same GPIO, and one will fail, which will likely cause
the driver to fail to probe().

Re: [PATCH 3/4] Documentation: devicetree: phy-tegra-usb: Add vbus-supply property for host mode PHYs

[Stephen Warren]

On 06/26/2013 03:59 AM, Mikko Perttunen wrote:

Patch description?

> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt

> +Optional properties for dr_mode == host:
> +  - vbus-supply: regulator for VBUS
> +
>  Required properties for dr_mode == otg:
>    - vbus-supply: regulator for VBUS

I would define the property only once. Perhaps:

VBUS control (required dr_mode==otg, optional for dr_mode==host):
    - vbus-supply: regulator for VBUS

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Stephen Warren]

On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> USB VBUS regulators are now specified with the vbus-supply property
> instead of vbus-gpio, so remove the obsolete properties.

As far as patch scheduling goes, I might be simpler to send one series
to convert the driver to the new properties, then once that has been
merged into a kernel release, we can apply patches to remove all the
legacy properties from DT in a later kernel release. That will save the
requirement for any cross-subsystem merges.

Although, if the USB maintainers are OK with doing it all at once, and
we can make sure this series gets merged into a separate topic branch so
that I can take it into the Tegra tree to resolve any merge conflicts
with any other Tegra DT modifications if needed, then applying this all
at once is fine.

Re: [PATCH 4/4] ARM: dts: tegra20: Remove obsolete vbus-gpio properties

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

On Wed, Jun 26, 2013 at 11:19:45AM -0600, Stephen Warren wrote:
> On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> > USB VBUS regulators are now specified with the vbus-supply property
> > instead of vbus-gpio, so remove the obsolete properties.
> 
> As far as patch scheduling goes, I might be simpler to send one series
> to convert the driver to the new properties, then once that has been
> merged into a kernel release, we can apply patches to remove all the
> legacy properties from DT in a later kernel release. That will save the
> requirement for any cross-subsystem merges.
> 
> Although, if the USB maintainers are OK with doing it all at once, and
> we can make sure this series gets merged into a separate topic branch so
> that I can take it into the Tegra tree to resolve any merge conflicts
> with any other Tegra DT modifications if needed, then applying this all
> at once is fine.

I'd be fine with the merge from my side. It worked well in the previous
merge windows. Let's just not go crazy with it ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Mikko Perttunen]

On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>  
wrote:

> On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
>> After this patch, usb vbus regulators for tegra usb phy devices can be  
>> specified
>> with the device tree attribute vbus-supply = <&x> where x is a  
>> regulator defined
>> in the device tree.
>
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c  
>> b/drivers/usb/phy/phy-tegra-usb.c
>
>> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy  
>> *phy)
>>  		return PTR_ERR(phy->pad_clk);
>>  	}
>>
>> +	phy->vbus = devm_regulator_get(phy->dev, "vbus");
>> +	/* On some boards, the VBUS regulator doesn't need to be controlled */
>> +	if (IS_ERR(phy->vbus)) {
>> +		if (PTR_ERR(phy->vbus) == -ENODEV) {
>> +			dev_notice(phy->dev, "no vbus regulator");
>> +			phy->vbus = NULL;
>> +		} else {
>> +			return PTR_ERR(phy->vbus);
>> +		}
>> +	}
>
> I think this code should be added to some more core initialization
> function; IIRC, there are separate utmip_pad_open() and some other
> function for ULPI mode, and in the future there may be more for HSIC,  
> etc.

I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't
even list a pin for USB2 which is a ULPI/HSIC only controller.

>
> For the error-handling, I think it'd be better to do:
>
> * If property doesn't exist, set phy->vbus to some error value, e.g.
> ERR_PTR(-ENODEV).
> * If property does exist, call devm_regulator_get().
> ** If devm_regulator_get() returned any error, return it.
>
> Or, does devm_regulator_get() return -ENODEV if-and-only-if the
> vbus-supply DT property does not exist?

Yes, devm_regulator_get uses regulator_dev_lookup which returns -ENODEV
if of_reg_regulator returns NULL, and there is no other place a -ENODEV
could be returned from. Comment in regulator_dev_lookup:

	/*
	 * If we couldn't even get the node then it's
	 * not just that the device didn't register
	 * yet, there's no node and we'll never
	 * succeed.
	 */
	*ret = -ENODEV;

>
> and ...
>
>> @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct  
>> tegra_usb_phy *phy)
>>  	spin_unlock_irqrestore(&utmip_pad_lock, flags);
>>
>>  	clk_disable_unprepare(phy->pad_clk);
>> +
>> +	if (phy->vbus) {
>
> Here, check if (IS_ERR(phy->vbus) instead. The reason is if
> devm_regulator_get() returns either a valid value or an error-pointer,
> then NULL could in theory be a valid value (it's up the the regulator
> API to determine that), and hence this code shouldn't assume that it can
> use NULL to represent "no regulator".

I'll fix this along with the problems with the other patches.

- Mikko

Re: [PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Stephen Warren]

On 06/27/2013 01:20 AM, Mikko Perttunen wrote:
> On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren
> <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
>>> After this patch, usb vbus regulators for tegra usb phy devices can
>>> be specified
>>> with the device tree attribute vbus-supply = <&x> where x is a
>>> regulator defined
>>> in the device tree.
>>
>>> diff --git a/drivers/usb/phy/phy-tegra-usb.c
>>> b/drivers/usb/phy/phy-tegra-usb.c
>>
>>> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy
>>> *phy)
>>>          return PTR_ERR(phy->pad_clk);
>>>      }
>>>
>>> +    phy->vbus = devm_regulator_get(phy->dev, "vbus");
>>> +    /* On some boards, the VBUS regulator doesn't need to be
>>> controlled */
>>> +    if (IS_ERR(phy->vbus)) {
>>> +        if (PTR_ERR(phy->vbus) == -ENODEV) {
>>> +            dev_notice(phy->dev, "no vbus regulator");
>>> +            phy->vbus = NULL;
>>> +        } else {
>>> +            return PTR_ERR(phy->vbus);
>>> +        }
>>> +    }
>>
>> I think this code should be added to some more core initialization
>> function; IIRC, there are separate utmip_pad_open() and some other
>> function for ULPI mode, and in the future there may be more for HSIC,
>> etc.
> 
> I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't
> even list a pin for USB2 which is a ULPI/HSIC only controller.

IIUC, ULPI, UTMI, and HSIC are just different physical layers for the
USB data signals between Tegra and whatever is connected. Any of those
could be translated to regular USB ports on the board. Certainly, UTMI
is the common one for external ports, and we have Tegra boards with ULPI
PHYs that translate to regular external ports. I don't know about HSIC;
it's less common, but I see no reason one couldn't have a PHY that
translates it to an external USB socket just like the rest.

On Tegra, most of the USB signals aren't pinmux'd at all. There's also
aren't dedicated VBUS control pins, so you'd never find them in the
pinmux. Instead, a GPIO is used to gate a power signal, or perhaps in
some cases we might program a PMIC directly.

As such, I'm pretty sure we should just make VBUS handling entirely
independent from PHY type.

>> For the error-handling, I think it'd be better to do:
>>
>> * If property doesn't exist, set phy->vbus to some error value, e.g.
>> ERR_PTR(-ENODEV).
>> * If property does exist, call devm_regulator_get().
>> ** If devm_regulator_get() returned any error, return it.
>>
>> Or, does devm_regulator_get() return -ENODEV if-and-only-if the
>> vbus-supply DT property does not exist?
> 
> Yes, devm_regulator_get uses regulator_dev_lookup which returns -ENODEV
> if of_reg_regulator returns NULL, and there is no other place a -ENODEV
> could be returned from. Comment in regulator_dev_lookup:
> 
>     /*
>      * If we couldn't even get the node then it's
>      * not just that the device didn't register
>      * yet, there's no node and we'll never
>      * succeed.
>      */
>     *ret = -ENODEV;

I believe that of_parse_phandle() can fail for a lot more reasons than
just "property doesn't exist", (yes, I just checked: if the property has
too little data, it will also error out), so while "property doesn't
exist" will trigger a NULL then -ENODEV return, there are also other
error cases that return that value, so we should take care to
differentiate them, with an explicit check whether the property exists.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Peter De Schrijver]

On Thu, Jun 27, 2013 at 06:15:02PM +0200, Stephen Warren wrote:
> On 06/27/2013 01:20 AM, Mikko Perttunen wrote:
> > On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren
> > <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > 
> >> On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> >>> After this patch, usb vbus regulators for tegra usb phy devices can
> >>> be specified
> >>> with the device tree attribute vbus-supply = <&x> where x is a
> >>> regulator defined
> >>> in the device tree.
> >>
> >>> diff --git a/drivers/usb/phy/phy-tegra-usb.c
> >>> b/drivers/usb/phy/phy-tegra-usb.c
> >>
> >>> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy
> >>> *phy)
> >>>          return PTR_ERR(phy->pad_clk);
> >>>      }
> >>>
> >>> +    phy->vbus = devm_regulator_get(phy->dev, "vbus");
> >>> +    /* On some boards, the VBUS regulator doesn't need to be
> >>> controlled */
> >>> +    if (IS_ERR(phy->vbus)) {
> >>> +        if (PTR_ERR(phy->vbus) == -ENODEV) {
> >>> +            dev_notice(phy->dev, "no vbus regulator");
> >>> +            phy->vbus = NULL;
> >>> +        } else {
> >>> +            return PTR_ERR(phy->vbus);
> >>> +        }
> >>> +    }
> >>
> >> I think this code should be added to some more core initialization
> >> function; IIRC, there are separate utmip_pad_open() and some other
> >> function for ULPI mode, and in the future there may be more for HSIC,
> >> etc.
> > 
> > I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't
> > even list a pin for USB2 which is a ULPI/HSIC only controller.
> 
> IIUC, ULPI, UTMI, and HSIC are just different physical layers for the
> USB data signals between Tegra and whatever is connected. Any of those
> could be translated to regular USB ports on the board. Certainly, UTMI
> is the common one for external ports, and we have Tegra boards with ULPI
> PHYs that translate to regular external ports. I don't know about HSIC;
> it's less common, but I see no reason one couldn't have a PHY that
> translates it to an external USB socket just like the rest.
> 

AFAIK HSIC is meant to attach eg modems or other onboard peripherals to USB
without having the (power) overhead of a PHY on both host and device side.

Cheers,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html