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

login
register
mail settings
Submitter Mikko Perttunen
Date June 26, 2013, 9:59 a.m.
Message ID <1372240781-1017-2-git-send-email-mperttunen@nvidia.com>
Download mbox | patch
Permalink /patch/254685/
State Superseded, archived
Headers show

Comments

Mikko Perttunen - June 26, 2013, 9:59 a.m.
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@nvidia.com>
---
 drivers/usb/phy/phy-tegra-usb.c   | 24 ++++++++++++++++++++++++
 include/linux/usb/tegra_usb_phy.h |  1 +
 2 files changed, 25 insertions(+)
Stephen Warren - June 26, 2013, 5:14 p.m.
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".
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen - June 27, 2013, 7:20 a.m.
On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren <swarren@wwwdotorg.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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - June 27, 2013, 4:15 p.m.
On 06/27/2013 01:20 AM, Mikko Perttunen wrote:
> On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren
> <swarren@wwwdotorg.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-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter De Schrijver - June 28, 2013, 7:17 a.m.
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@wwwdotorg.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-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;