LinuxLists.cc - [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

[permalink] [raw]

Subject: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

Facilitate getting required 3.3V and 1.0V VDD supply for
DWC3 controller on Exynos.

With patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,

certain perripherals will now need to ensure that,
they request VDD regulators in their drivers, and enable
them so as to make them working.

Signed-off-by: Vivek Gautam <[email protected]>
Cc: Anton Tikhomirov <[email protected]>
---

Based on 'usb-next' branch of Greg's USB tree.
Also cleanly applies on 'next' branch of Balbi's USB tree.

drivers/usb/dwc3/dwc3-exynos.c | 51 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 28c8ad7..c9d9102 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -27,6 +27,7 @@
#include <linux/usb/usb_phy_gen_xceiv.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/regulator/consumer.h>

struct dwc3_exynos {
struct platform_device *usb2_phy;
@@ -34,6 +35,8 @@ struct dwc3_exynos {
struct device *dev;

struct clk *clk;
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};

static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
@@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct platform_device *pdev)

clk_prepare_enable(exynos->clk);

+ exynos->vdd33 = devm_regulator_get(dev, "vdd33");
+ if (IS_ERR(exynos->vdd33)) {
+ ret = PTR_ERR(exynos->vdd33);
+ goto err2;
+ }
+ ret = regulator_enable(exynos->vdd33);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD33 supply\n");
+ goto err2;
+ }
+
+ exynos->vdd10 = devm_regulator_get(dev, "vdd10");
+ if (IS_ERR(exynos->vdd10)) {
+ ret = PTR_ERR(exynos->vdd10);
+ goto err3;
+ }
+ ret = regulator_enable(exynos->vdd10);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD10 supply\n");
+ goto err3;
+ }
+
if (node) {
ret = of_platform_populate(node, NULL, NULL, dev);
if (ret) {
dev_err(dev, "failed to add dwc3 core\n");
- goto err2;
+ goto err4;
}
} else {
dev_err(dev, "no device node, failed to add dwc3 core\n");
ret = -ENODEV;
- goto err2;
+ goto err4;
}

return 0;

+err4:
+ regulator_disable(exynos->vdd10);
+err3:
+ regulator_disable(exynos->vdd33);
err2:
clk_disable_unprepare(clk);
err1:
@@ -174,6 +203,9 @@ static int dwc3_exynos_remove(struct platform_device *pdev)

clk_disable_unprepare(exynos->clk);

+ regulator_disable(exynos->vdd33);
+ regulator_disable(exynos->vdd10);
+
return 0;
}

@@ -192,12 +224,27 @@ static int dwc3_exynos_suspend(struct device *dev)

clk_disable(exynos->clk);

+ regulator_disable(exynos->vdd33);
+ regulator_disable(exynos->vdd10);
+
return 0;
}

static int dwc3_exynos_resume(struct device *dev)
{
struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regulator_enable(exynos->vdd33);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD33 supply\n");
+ return ret;
+ }
+ ret = regulator_enable(exynos->vdd10);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD10 supply\n");
+ return ret;
+ }

clk_enable(exynos->clk);

--
1.7.10.4

2014-04-21 12:18:04

[permalink] [raw]

Subject: [PATCH 2/3] usb: ehci-exynos: Make provision for vdd regulators

Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.

With patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,

certain perripherals will now need to ensure that,
they request VDD regulators in their drivers, and enable
them so as to make them working.

Signed-off-by: Vivek Gautam <[email protected]>
Cc: Jingoo Han <[email protected]>
---

Based on 'usb-next' branch of Greg's usb tree.

drivers/usb/host/ehci-exynos.c | 47 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index d1d8c47..a3ca8cc 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -20,6 +20,7 @@
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/usb/phy.h>
#include <linux/usb/samsung_usb_phy.h>
#include <linux/usb.h>
@@ -46,6 +47,8 @@ struct exynos_ehci_hcd {
struct clk *clk;
struct usb_phy *phy;
struct usb_otg *otg;
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};

#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
@@ -112,6 +115,28 @@ static int exynos_ehci_probe(struct platform_device *pdev)
exynos_ehci->otg = phy->otg;
}

+ exynos_ehci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
+ if (IS_ERR(exynos_ehci->vdd33)) {
+ err = PTR_ERR(exynos_ehci->vdd33);
+ goto fail_regulator1;
+ }
+ err = regulator_enable(exynos_ehci->vdd33);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
+ goto fail_regulator1;
+ }
+
+ exynos_ehci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
+ if (IS_ERR(exynos_ehci->vdd10)) {
+ err = PTR_ERR(exynos_ehci->vdd10);
+ goto fail_regulator2;
+ }
+ err = regulator_enable(exynos_ehci->vdd10);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
+ goto fail_regulator2;
+ }
+
skip_phy:

exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost");
@@ -178,6 +203,10 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci->clk);
fail_clk:
+ regulator_disable(exynos_ehci->vdd10);
+fail_regulator2:
+ regulator_disable(exynos_ehci->vdd33);
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -197,6 +226,9 @@ static int exynos_ehci_remove(struct platform_device *pdev)

clk_disable_unprepare(exynos_ehci->clk);

+ regulator_disable(exynos_ehci->vdd10);
+ regulator_disable(exynos_ehci->vdd33);
+
usb_put_hcd(hcd);

return 0;
@@ -221,6 +253,9 @@ static int exynos_ehci_suspend(struct device *dev)

clk_disable_unprepare(exynos_ehci->clk);

+ regulator_disable(exynos_ehci->vdd10);
+ regulator_disable(exynos_ehci->vdd33);
+
return rc;
}

@@ -228,6 +263,18 @@ static int exynos_ehci_resume(struct device *dev)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+ int ret;
+
+ ret = regulator_enable(exynos_ehci->vdd33);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD33 supply\n");
+ return ret;
+ }
+ ret = regulator_enable(exynos_ehci->vdd10);
+ if (ret) {
+ dev_err(dev, "Failed to enable VDD10 supply\n");
+ return ret;
+ }

clk_prepare_enable(exynos_ehci->clk);

--
1.7.10.4

2014-04-23 09:27:02

[permalink] [raw]

Subject: RE: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

Hello,

> -----Original Message-----
> From: Vivek Gautam [mailto:[email protected]] On Behalf Of
> Vivek Gautam
> Sent: Monday, April 21, 2014 9:17 PM
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Vivek
> Gautam; Anton Tikhomirov
> Subject: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd
> regulators
>
> Facilitate getting required 3.3V and 1.0V VDD supply for
> DWC3 controller on Exynos.
>
> With patches for regulators' nodes merged in 3.15:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>
> certain perripherals will now need to ensure that,
> they request VDD regulators in their drivers, and enable
> them so as to make them working.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> Cc: Anton Tikhomirov <[email protected]>
> ---
>
> Based on 'usb-next' branch of Greg's USB tree.
> Also cleanly applies on 'next' branch of Balbi's USB tree.
>
> drivers/usb/dwc3/dwc3-exynos.c | 51
> ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
> exynos.c
> index 28c8ad7..c9d9102 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -27,6 +27,7 @@
> #include <linux/usb/usb_phy_gen_xceiv.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> +#include <linux/regulator/consumer.h>
>
> struct dwc3_exynos {
> struct platform_device *usb2_phy;
> @@ -34,6 +35,8 @@ struct dwc3_exynos {
> struct device *dev;
>
> struct clk *clk;
> + struct regulator *vdd33;
> + struct regulator *vdd10;
> };
>
> static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
> @@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct
> platform_device *pdev)
>
> clk_prepare_enable(exynos->clk);
>
> + exynos->vdd33 = devm_regulator_get(dev, "vdd33");
> + if (IS_ERR(exynos->vdd33)) {
> + ret = PTR_ERR(exynos->vdd33);
> + goto err2;

Is regulator property mandatory for dwc3-exynos? If it is not
and device tree doesn't provide it, dwc3-exynos driver probe shouldn't
fail here.

> + }
> + ret = regulator_enable(exynos->vdd33);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD33 supply\n");
> + goto err2;
> + }
> +

Thanks

2014-04-23 09:52:06

[permalink] [raw]

Subject: Re: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

Hi Anton,

On Wed, Apr 23, 2014 at 2:56 PM, Anton Tikhomirov
<[email protected]> wrote:
> Hello,
>
>> -----Original Message-----
>> From: Vivek Gautam [mailto:[email protected]] On Behalf Of
>> Vivek Gautam
>> Sent: Monday, April 21, 2014 9:17 PM
>> To: [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]; Vivek
>> Gautam; Anton Tikhomirov
>> Subject: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd
>> regulators
>>
>> Facilitate getting required 3.3V and 1.0V VDD supply for
>> DWC3 controller on Exynos.
>>
>> With patches for regulators' nodes merged in 3.15:
>> c8c253f ARM: dts: Add regulator entries to smdk5420
>> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>>
>> certain perripherals will now need to ensure that,
>> they request VDD regulators in their drivers, and enable
>> them so as to make them working.
>>
>> Signed-off-by: Vivek Gautam <[email protected]>
>> Cc: Anton Tikhomirov <[email protected]>
>> ---
>>
>> Based on 'usb-next' branch of Greg's USB tree.
>> Also cleanly applies on 'next' branch of Balbi's USB tree.
>>
>> drivers/usb/dwc3/dwc3-exynos.c | 51
>> ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
>> exynos.c
>> index 28c8ad7..c9d9102 100644
>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> @@ -27,6 +27,7 @@
>> #include <linux/usb/usb_phy_gen_xceiv.h>
>> #include <linux/of.h>
>> #include <linux/of_platform.h>
>> +#include <linux/regulator/consumer.h>
>>
>> struct dwc3_exynos {
>> struct platform_device *usb2_phy;
>> @@ -34,6 +35,8 @@ struct dwc3_exynos {
>> struct device *dev;
>>
>> struct clk *clk;
>> + struct regulator *vdd33;
>> + struct regulator *vdd10;
>> };
>>
>> static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
>> @@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct
>> platform_device *pdev)
>>
>> clk_prepare_enable(exynos->clk);
>>
>> + exynos->vdd33 = devm_regulator_get(dev, "vdd33");
>> + if (IS_ERR(exynos->vdd33)) {
>> + ret = PTR_ERR(exynos->vdd33);
>> + goto err2;
>
> Is regulator property mandatory for dwc3-exynos? If it is not
> and device tree doesn't provide it, dwc3-exynos driver probe shouldn't
> fail here.

These are the VDD regulators (from PMIC ldo supplies), in absence of
which the controller will not be powered up.
So doesn't it make sense to stop the probe when these are not supplied
by device tree ?

[snip]

2014-04-23 10:57:42

[permalink] [raw]

Subject: RE: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

Hi,

> Hi Anton,
>
>
> On Wed, Apr 23, 2014 at 2:56 PM, Anton Tikhomirov
> <[email protected]> wrote:
> > Hello,
> >
> >> -----Original Message-----
> >> From: Vivek Gautam [mailto:[email protected]] On Behalf Of
> >> Vivek Gautam
> >> Sent: Monday, April 21, 2014 9:17 PM
> >> To: [email protected]; [email protected]
> >> Cc: [email protected]; [email protected]; linux-
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> Vivek
> >> Gautam; Anton Tikhomirov
> >> Subject: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd
> >> regulators
> >>
> >> Facilitate getting required 3.3V and 1.0V VDD supply for
> >> DWC3 controller on Exynos.
> >>
> >> With patches for regulators' nodes merged in 3.15:
> >> c8c253f ARM: dts: Add regulator entries to smdk5420
> >> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> >>
> >> certain perripherals will now need to ensure that,
> >> they request VDD regulators in their drivers, and enable
> >> them so as to make them working.
> >>
> >> Signed-off-by: Vivek Gautam <[email protected]>
> >> Cc: Anton Tikhomirov <[email protected]>
> >> ---
> >>
> >> Based on 'usb-next' branch of Greg's USB tree.
> >> Also cleanly applies on 'next' branch of Balbi's USB tree.
> >>
> >> drivers/usb/dwc3/dwc3-exynos.c | 51
> >> ++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 49 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
> >> exynos.c
> >> index 28c8ad7..c9d9102 100644
> >> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >> @@ -27,6 +27,7 @@
> >> #include <linux/usb/usb_phy_gen_xceiv.h>
> >> #include <linux/of.h>
> >> #include <linux/of_platform.h>
> >> +#include <linux/regulator/consumer.h>
> >>
> >> struct dwc3_exynos {
> >> struct platform_device *usb2_phy;
> >> @@ -34,6 +35,8 @@ struct dwc3_exynos {
> >> struct device *dev;
> >>
> >> struct clk *clk;
> >> + struct regulator *vdd33;
> >> + struct regulator *vdd10;
> >> };
> >>
> >> static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
> >> @@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct
> >> platform_device *pdev)
> >>
> >> clk_prepare_enable(exynos->clk);
> >>
> >> + exynos->vdd33 = devm_regulator_get(dev, "vdd33");
> >> + if (IS_ERR(exynos->vdd33)) {
> >> + ret = PTR_ERR(exynos->vdd33);
> >> + goto err2;
> >
> > Is regulator property mandatory for dwc3-exynos? If it is not
> > and device tree doesn't provide it, dwc3-exynos driver probe
> shouldn't
> > fail here.
>
> These are the VDD regulators (from PMIC ldo supplies), in absence of
> which the controller will not be powered up.
> So doesn't it make sense to stop the probe when these are not supplied
> by device tree ?

Agree. Just curious, is there special reason for this change except making
things right?

2014-04-23 11:14:05

[permalink] [raw]

Subject: Re: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

Hi,

On Wed, Apr 23, 2014 at 4:27 PM, Anton Tikhomirov
<[email protected]> wrote:
> Hi,
>
>> Hi Anton,
>>
>>
>> On Wed, Apr 23, 2014 at 2:56 PM, Anton Tikhomirov
>> <[email protected]> wrote:
>> > Hello,
>> >
>> >> -----Original Message-----
>> >> From: Vivek Gautam [mailto:[email protected]] On Behalf Of
>> >> Vivek Gautam
>> >> Sent: Monday, April 21, 2014 9:17 PM
>> >> To: [email protected]; [email protected]
>> >> Cc: [email protected]; [email protected]; linux-
>> >> [email protected]; [email protected];
>> >> [email protected]; [email protected]; [email protected];
>> Vivek
>> >> Gautam; Anton Tikhomirov
>> >> Subject: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd
>> >> regulators
>> >>
>> >> Facilitate getting required 3.3V and 1.0V VDD supply for
>> >> DWC3 controller on Exynos.
>> >>
>> >> With patches for regulators' nodes merged in 3.15:
>> >> c8c253f ARM: dts: Add regulator entries to smdk5420
>> >> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> >>
>> >> certain perripherals will now need to ensure that,
>> >> they request VDD regulators in their drivers, and enable
>> >> them so as to make them working.
>> >>
>> >> Signed-off-by: Vivek Gautam <[email protected]>
>> >> Cc: Anton Tikhomirov <[email protected]>
>> >> ---
>> >>
>> >> Based on 'usb-next' branch of Greg's USB tree.
>> >> Also cleanly applies on 'next' branch of Balbi's USB tree.
>> >>
>> >> drivers/usb/dwc3/dwc3-exynos.c | 51
>> >> ++++++++++++++++++++++++++++++++++++++--
>> >> 1 file changed, 49 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
>> >> exynos.c
>> >> index 28c8ad7..c9d9102 100644
>> >> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> >> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> >> @@ -27,6 +27,7 @@
>> >> #include <linux/usb/usb_phy_gen_xceiv.h>
>> >> #include <linux/of.h>
>> >> #include <linux/of_platform.h>
>> >> +#include <linux/regulator/consumer.h>
>> >>
>> >> struct dwc3_exynos {
>> >> struct platform_device *usb2_phy;
>> >> @@ -34,6 +35,8 @@ struct dwc3_exynos {
>> >> struct device *dev;
>> >>
>> >> struct clk *clk;
>> >> + struct regulator *vdd33;
>> >> + struct regulator *vdd10;
>> >> };
>> >>
>> >> static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
>> >> @@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct
>> >> platform_device *pdev)
>> >>
>> >> clk_prepare_enable(exynos->clk);
>> >>
>> >> + exynos->vdd33 = devm_regulator_get(dev, "vdd33");
>> >> + if (IS_ERR(exynos->vdd33)) {
>> >> + ret = PTR_ERR(exynos->vdd33);
>> >> + goto err2;
>> >
>> > Is regulator property mandatory for dwc3-exynos? If it is not
>> > and device tree doesn't provide it, dwc3-exynos driver probe
>> shouldn't
>> > fail here.
>>
>> These are the VDD regulators (from PMIC ldo supplies), in absence of
>> which the controller will not be powered up.
>> So doesn't it make sense to stop the probe when these are not supplied
>> by device tree ?
>
> Agree. Just curious, is there special reason for this change except making
> things right?

Yea, actually after the patch (which got merged in 3.15 rc1)
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,

the USB stops working, and that's because the regulators related to
usb are not turned on by default (ldo12 and ldo15 to be specific).
So we need to enable those regulators, which ofcourse the driver
should be doing, if i am not wrong.
Similar is the reason for EHCI, and OHCI exynos patches in this series.

I shall be sending the dt patch for this soon.

--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

2014-04-23 12:31:54

[permalink] [raw]

Subject: Re: [PATCH 3/3] usb: dwc3-exynos: Make provision for vdd regulators

On Wednesday, April 23, 2014 8:06 PM, Vivek Gautam wrote:
> On Wednesday, April 23, 2014 7:58 PM, Anton Tikhomirov wrote:
> > On Wednesday, April 23, 2014 6:52 PM, Vivek Gautam wrote:
> >> On Wednesday, April 23, 2014 6:27 PM, Anton Tikhomirov wrote:
> >> > On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
> >> >>
> >> >> Facilitate getting required 3.3V and 1.0V VDD supply for
> >> >> DWC3 controller on Exynos.
> >> >>
> >> >> With patches for regulators' nodes merged in 3.15:
> >> >> c8c253f ARM: dts: Add regulator entries to smdk5420
> >> >> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> >> >>
> >> >> certain perripherals will now need to ensure that,
> >> >> they request VDD regulators in their drivers, and enable
> >> >> them so as to make them working.
> >> >>
> >> >> Signed-off-by: Vivek Gautam <[email protected]>
> >> >> Cc: Anton Tikhomirov <[email protected]>
> >> >> ---
> >> >>
> >> >> Based on 'usb-next' branch of Greg's USB tree.
> >> >> Also cleanly applies on 'next' branch of Balbi's USB tree.
> >> >>
> >> >> drivers/usb/dwc3/dwc3-exynos.c | 51
> >> >> ++++++++++++++++++++++++++++++++++++++--
> >> >> 1 file changed, 49 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
> >> >> exynos.c
> >> >> index 28c8ad7..c9d9102 100644
> >> >> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >> >> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >> >> @@ -27,6 +27,7 @@
> >> >> #include <linux/usb/usb_phy_gen_xceiv.h>
> >> >> #include <linux/of.h>
> >> >> #include <linux/of_platform.h>
> >> >> +#include <linux/regulator/consumer.h>
> >> >>
> >> >> struct dwc3_exynos {
> >> >> struct platform_device *usb2_phy;
> >> >> @@ -34,6 +35,8 @@ struct dwc3_exynos {
> >> >> struct device *dev;
> >> >>
> >> >> struct clk *clk;
> >> >> + struct regulator *vdd33;
> >> >> + struct regulator *vdd10;
> >> >> };
> >> >>
> >> >> static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
> >> >> @@ -144,20 +147,46 @@ static int dwc3_exynos_probe(struct
> >> >> platform_device *pdev)
> >> >>
> >> >> clk_prepare_enable(exynos->clk);
> >> >>
> >> >> + exynos->vdd33 = devm_regulator_get(dev, "vdd33");
> >> >> + if (IS_ERR(exynos->vdd33)) {
> >> >> + ret = PTR_ERR(exynos->vdd33);
> >> >> + goto err2;
> >> >
> >> > Is regulator property mandatory for dwc3-exynos? If it is not
> >> > and device tree doesn't provide it, dwc3-exynos driver probe
> >> shouldn't
> >> > fail here.
> >>
> >> These are the VDD regulators (from PMIC ldo supplies), in absence of
> >> which the controller will not be powered up.
> >> So doesn't it make sense to stop the probe when these are not supplied
> >> by device tree ?
> >
> > Agree. Just curious, is there special reason for this change except making
> > things right?
>
> Yea, actually after the patch (which got merged in 3.15 rc1)
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>
> the USB stops working, and that's because the regulators related to
> usb are not turned on by default (ldo12 and ldo15 to be specific).
> So we need to enable those regulators, which ofcourse the driver
> should be doing, if i am not wrong.
> Similar is the reason for EHCI, and OHCI exynos patches in this series.

Oh, I see. Thank you for your explanation.

>
> I shall be sending the dt patch for this soon.

OK, I will wait for your patch.

Best regards,
Jingoo Han

>
>
> --
> Best Regards
> Vivek Gautam
> Samsung R&D Institute, Bangalore
> India

2014-04-23 12:43:49

[permalink] [raw]

Subject: Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
>
> Facilitate getting required 3.3V and 1.0V VDD supply for
> OHCI controller on Exynos.
>
> With patches for regulators' nodes merged in 3.15:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>
> certain perripherals will now need to ensure that,
> they request VDD regulators in their drivers, and enable
> them so as to make them working.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> Cc: Jingoo Han <[email protected]>

Acked-by: Jingoo Han <[email protected]>

Best regards,
Jingoo Han

> ---
>
> Based on 'usb-next' branch of Greg's usb tree.
>
> drivers/usb/host/ohci-exynos.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 68588d8..e2e72a8 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -18,6 +18,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/usb/phy.h>
> #include <linux/usb/samsung_usb_phy.h>
> #include <linux/usb.h>
> @@ -37,6 +38,8 @@ struct exynos_ohci_hcd {
> struct clk *clk;
> struct usb_phy *phy;
> struct usb_otg *otg;
> + struct regulator *vdd33;
> + struct regulator *vdd10;
> };
>
> static void exynos_ohci_phy_enable(struct platform_device *pdev)
> @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device *pdev)
> exynos_ohci->otg = phy->otg;
> }
>
> + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> + if (IS_ERR(exynos_ohci->vdd33)) {
> + err = PTR_ERR(exynos_ohci->vdd33);
> + goto fail_regulator1;
> + }
> + err = regulator_enable(exynos_ohci->vdd33);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> + goto fail_regulator1;
> + }
> +
> + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> + if (IS_ERR(exynos_ohci->vdd10)) {
> + err = PTR_ERR(exynos_ohci->vdd10);
> + goto fail_regulator2;
> + }
> + err = regulator_enable(exynos_ohci->vdd10);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> + goto fail_regulator2;
> + }
> +
> skip_phy:
> exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
>
> @@ -154,6 +179,10 @@ fail_add_hcd:
> fail_io:
> clk_disable_unprepare(exynos_ohci->clk);
> fail_clk:
> + regulator_disable(exynos_ohci->vdd10);
> +fail_regulator2:
> + regulator_disable(exynos_ohci->vdd33);
> +fail_regulator1:
> usb_put_hcd(hcd);
> return err;
> }
> @@ -172,6 +201,9 @@ static int exynos_ohci_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(exynos_ohci->clk);
>
> + regulator_disable(exynos_ohci->vdd10);
> + regulator_disable(exynos_ohci->vdd33);
> +
> usb_put_hcd(hcd);
>
> return 0;
> @@ -208,6 +240,9 @@ static int exynos_ohci_suspend(struct device *dev)
>
> clk_disable_unprepare(exynos_ohci->clk);
>
> + regulator_disable(exynos_ohci->vdd10);
> + regulator_disable(exynos_ohci->vdd33);
> +
> spin_unlock_irqrestore(&ohci->lock, flags);
>
> return 0;
> @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> struct platform_device *pdev = to_platform_device(dev);
> + int ret;
> +
> + ret = regulator_enable(exynos_ohci->vdd33);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD33 supply\n");
> + return ret;
> + }
> + ret = regulator_enable(exynos_ohci->vdd10);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD10 supply\n");
> + return ret;
> + }
>
> clk_prepare_enable(exynos_ohci->clk);
>
> --
> 1.7.10.4

2014-04-23 12:44:44

[permalink] [raw]

Subject: Re: [PATCH 2/3] usb: ehci-exynos: Make provision for vdd regulators

On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
>
> Facilitate getting required 3.3V and 1.0V VDD supply for
> EHCI controller on Exynos.
>
> With patches for regulators' nodes merged in 3.15:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>
> certain perripherals will now need to ensure that,
> they request VDD regulators in their drivers, and enable
> them so as to make them working.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> Cc: Jingoo Han <[email protected]>

Acked-by: Jingoo Han <[email protected]>

Best regards,
Jingoo Han

> ---
>
> Based on 'usb-next' branch of Greg's usb tree.
>
> drivers/usb/host/ehci-exynos.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index d1d8c47..a3ca8cc 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -20,6 +20,7 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/usb/phy.h>
> #include <linux/usb/samsung_usb_phy.h>
> #include <linux/usb.h>
> @@ -46,6 +47,8 @@ struct exynos_ehci_hcd {
> struct clk *clk;
> struct usb_phy *phy;
> struct usb_otg *otg;
> + struct regulator *vdd33;
> + struct regulator *vdd10;
> };
>
> #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
> @@ -112,6 +115,28 @@ static int exynos_ehci_probe(struct platform_device *pdev)
> exynos_ehci->otg = phy->otg;
> }
>
> + exynos_ehci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> + if (IS_ERR(exynos_ehci->vdd33)) {
> + err = PTR_ERR(exynos_ehci->vdd33);
> + goto fail_regulator1;
> + }
> + err = regulator_enable(exynos_ehci->vdd33);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> + goto fail_regulator1;
> + }
> +
> + exynos_ehci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> + if (IS_ERR(exynos_ehci->vdd10)) {
> + err = PTR_ERR(exynos_ehci->vdd10);
> + goto fail_regulator2;
> + }
> + err = regulator_enable(exynos_ehci->vdd10);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> + goto fail_regulator2;
> + }
> +
> skip_phy:
>
> exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost");
> @@ -178,6 +203,10 @@ fail_add_hcd:
> fail_io:
> clk_disable_unprepare(exynos_ehci->clk);
> fail_clk:
> + regulator_disable(exynos_ehci->vdd10);
> +fail_regulator2:
> + regulator_disable(exynos_ehci->vdd33);
> +fail_regulator1:
> usb_put_hcd(hcd);
> return err;
> }
> @@ -197,6 +226,9 @@ static int exynos_ehci_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(exynos_ehci->clk);
>
> + regulator_disable(exynos_ehci->vdd10);
> + regulator_disable(exynos_ehci->vdd33);
> +
> usb_put_hcd(hcd);
>
> return 0;
> @@ -221,6 +253,9 @@ static int exynos_ehci_suspend(struct device *dev)
>
> clk_disable_unprepare(exynos_ehci->clk);
>
> + regulator_disable(exynos_ehci->vdd10);
> + regulator_disable(exynos_ehci->vdd33);
> +
> return rc;
> }
>
> @@ -228,6 +263,18 @@ static int exynos_ehci_resume(struct device *dev)
> {
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> + int ret;
> +
> + ret = regulator_enable(exynos_ehci->vdd33);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD33 supply\n");
> + return ret;
> + }
> + ret = regulator_enable(exynos_ehci->vdd10);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD10 supply\n");
> + return ret;
> + }
>
> clk_prepare_enable(exynos_ehci->clk);
>
> --
> 1.7.10.4

2014-04-24 00:18:10

[permalink] [raw]

Subject: RE: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

Hi,

> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of Vivek Gautam
> Sent: Monday, April 21, 2014 9:17 PM
>
> Facilitate getting required 3.3V and 1.0V VDD supply for
> OHCI controller on Exynos.
>
> With patches for regulators' nodes merged in 3.15:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>
> certain perripherals will now need to ensure that,
> they request VDD regulators in their drivers, and enable
> them so as to make them working.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> Cc: Jingoo Han <[email protected]>
> ---
>
> Based on 'usb-next' branch of Greg's usb tree.
>
> drivers/usb/host/ohci-exynos.c | 47
> ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
> exynos.c
> index 68588d8..e2e72a8 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -18,6 +18,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/usb/phy.h>
> #include <linux/usb/samsung_usb_phy.h>
> #include <linux/usb.h>
> @@ -37,6 +38,8 @@ struct exynos_ohci_hcd {
> struct clk *clk;
> struct usb_phy *phy;
> struct usb_otg *otg;
> + struct regulator *vdd33;
> + struct regulator *vdd10;
> };
>
> static void exynos_ohci_phy_enable(struct platform_device *pdev)
> @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device
> *pdev)
> exynos_ohci->otg = phy->otg;
> }
>
> + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> + if (IS_ERR(exynos_ohci->vdd33)) {
> + err = PTR_ERR(exynos_ohci->vdd33);
> + goto fail_regulator1;
> + }
> + err = regulator_enable(exynos_ohci->vdd33);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> + goto fail_regulator1;
> + }
> +
> + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> + if (IS_ERR(exynos_ohci->vdd10)) {
> + err = PTR_ERR(exynos_ohci->vdd10);
> + goto fail_regulator2;
> + }
> + err = regulator_enable(exynos_ohci->vdd10);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> + goto fail_regulator2;
> + }
> +

Do we need to skip regulator settings together with PHY configuration
in case of exynos5440?

> skip_phy:
> exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
>
> @@ -154,6 +179,10 @@ fail_add_hcd:
> fail_io:
> clk_disable_unprepare(exynos_ohci->clk);
> fail_clk:
> + regulator_disable(exynos_ohci->vdd10);
> +fail_regulator2:
> + regulator_disable(exynos_ohci->vdd33);
> +fail_regulator1:
> usb_put_hcd(hcd);
> return err;
> }
> @@ -172,6 +201,9 @@ static int exynos_ohci_remove(struct
> platform_device *pdev)
>
> clk_disable_unprepare(exynos_ohci->clk);
>
> + regulator_disable(exynos_ohci->vdd10);
> + regulator_disable(exynos_ohci->vdd33);
> +
> usb_put_hcd(hcd);
>
> return 0;
> @@ -208,6 +240,9 @@ static int exynos_ohci_suspend(struct device *dev)
>
> clk_disable_unprepare(exynos_ohci->clk);
>
> + regulator_disable(exynos_ohci->vdd10);
> + regulator_disable(exynos_ohci->vdd33);
> +
> spin_unlock_irqrestore(&ohci->lock, flags);
>
> return 0;
> @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> struct platform_device *pdev = to_platform_device(dev);
> + int ret;
> +
> + ret = regulator_enable(exynos_ohci->vdd33);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD33 supply\n");
> + return ret;

Not sure, but shall we continue resuming and do everything
we can in case of error? At least it will avoid
WARN_ON(clk->enable_count == 0) on next system suspend.

> + }
> + ret = regulator_enable(exynos_ohci->vdd10);
> + if (ret) {
> + dev_err(dev, "Failed to enable VDD10 supply\n");
> + return ret;
> + }
>
> clk_prepare_enable(exynos_ohci->clk);
>
> --

Thanks

2014-04-24 00:32:38

[permalink] [raw]

Subject: Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

On Thursday, April 24, 2014 9:18 AM, Anton Tikhomirov wrote:
> On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
> >
> > Facilitate getting required 3.3V and 1.0V VDD supply for
> > OHCI controller on Exynos.
> >
> > With patches for regulators' nodes merged in 3.15:
> > c8c253f ARM: dts: Add regulator entries to smdk5420
> > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> >
> > certain perripherals will now need to ensure that,
> > they request VDD regulators in their drivers, and enable
> > them so as to make them working.
> >
> > Signed-off-by: Vivek Gautam <[email protected]>
> > Cc: Jingoo Han <[email protected]>
> > ---
> >
> > Based on 'usb-next' branch of Greg's usb tree.
> >
> > drivers/usb/host/ohci-exynos.c | 47
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
> > exynos.c
> > index 68588d8..e2e72a8 100644
> > --- a/drivers/usb/host/ohci-exynos.c
> > +++ b/drivers/usb/host/ohci-exynos.c
> > @@ -18,6 +18,7 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/usb/phy.h>
> > #include <linux/usb/samsung_usb_phy.h>
> > #include <linux/usb.h>
> > @@ -37,6 +38,8 @@ struct exynos_ohci_hcd {
> > struct clk *clk;
> > struct usb_phy *phy;
> > struct usb_otg *otg;
> > + struct regulator *vdd33;
> > + struct regulator *vdd10;
> > };
> >
> > static void exynos_ohci_phy_enable(struct platform_device *pdev)
> > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device
> > *pdev)
> > exynos_ohci->otg = phy->otg;
> > }
> >
> > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> > + if (IS_ERR(exynos_ohci->vdd33)) {
> > + err = PTR_ERR(exynos_ohci->vdd33);
> > + goto fail_regulator1;
> > + }
> > + err = regulator_enable(exynos_ohci->vdd33);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> > + goto fail_regulator1;
> > + }
> > +
> > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> > + if (IS_ERR(exynos_ohci->vdd10)) {
> > + err = PTR_ERR(exynos_ohci->vdd10);
> > + goto fail_regulator2;
> > + }
> > + err = regulator_enable(exynos_ohci->vdd10);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> > + goto fail_regulator2;
> > + }
> > +
>
> Do we need to skip regulator settings together with PHY configuration
> in case of exynos5440?

Oh, right. In the case of exynos5440, regulator settings is not
necessary. Vivek, would you fix it in order skip regulator settings
in exynos5440? It also applies to ehci-exynos.

Best regards,
Jingoo Han

>
> > skip_phy:
> > exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
> >
> > @@ -154,6 +179,10 @@ fail_add_hcd:
> > fail_io:
> > clk_disable_unprepare(exynos_ohci->clk);
> > fail_clk:
> > + regulator_disable(exynos_ohci->vdd10);
> > +fail_regulator2:
> > + regulator_disable(exynos_ohci->vdd33);
> > +fail_regulator1:
> > usb_put_hcd(hcd);
> > return err;
> > }
> > @@ -172,6 +201,9 @@ static int exynos_ohci_remove(struct
> > platform_device *pdev)
> >
> > clk_disable_unprepare(exynos_ohci->clk);
> >
> > + regulator_disable(exynos_ohci->vdd10);
> > + regulator_disable(exynos_ohci->vdd33);
> > +
> > usb_put_hcd(hcd);
> >
> > return 0;
> > @@ -208,6 +240,9 @@ static int exynos_ohci_suspend(struct device *dev)
> >
> > clk_disable_unprepare(exynos_ohci->clk);
> >
> > + regulator_disable(exynos_ohci->vdd10);
> > + regulator_disable(exynos_ohci->vdd33);
> > +
> > spin_unlock_irqrestore(&ohci->lock, flags);
> >
> > return 0;
> > @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
> > struct usb_hcd *hcd = dev_get_drvdata(dev);
> > struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> > struct platform_device *pdev = to_platform_device(dev);
> > + int ret;
> > +
> > + ret = regulator_enable(exynos_ohci->vdd33);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable VDD33 supply\n");
> > + return ret;
>
> Not sure, but shall we continue resuming and do everything
> we can in case of error? At least it will avoid
> WARN_ON(clk->enable_count == 0) on next system suspend.
>
> > + }
> > + ret = regulator_enable(exynos_ohci->vdd10);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable VDD10 supply\n");
> > + return ret;
> > + }
> >
> > clk_prepare_enable(exynos_ohci->clk);
> >
> > --
>
> Thanks

2014-04-24 00:38:11

[permalink] [raw]

Subject: RE: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

Hi,

> Hi,
>
> > -----Original Message-----
> > From: [email protected] [mailto:linux-usb-
> > [email protected]] On Behalf Of Vivek Gautam
> > Sent: Monday, April 21, 2014 9:17 PM
> >
> > Facilitate getting required 3.3V and 1.0V VDD supply for
> > OHCI controller on Exynos.
> >
> > With patches for regulators' nodes merged in 3.15:
> > c8c253f ARM: dts: Add regulator entries to smdk5420
> > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> >
> > certain perripherals will now need to ensure that,
> > they request VDD regulators in their drivers, and enable
> > them so as to make them working.
> >
> > Signed-off-by: Vivek Gautam <[email protected]>
> > Cc: Jingoo Han <[email protected]>
> > ---
> >
> > Based on 'usb-next' branch of Greg's usb tree.
> >
> > drivers/usb/host/ohci-exynos.c | 47
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
> > exynos.c
> > index 68588d8..e2e72a8 100644
> > --- a/drivers/usb/host/ohci-exynos.c
> > +++ b/drivers/usb/host/ohci-exynos.c
> > @@ -18,6 +18,7 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/usb/phy.h>
> > #include <linux/usb/samsung_usb_phy.h>
> > #include <linux/usb.h>
> > @@ -37,6 +38,8 @@ struct exynos_ohci_hcd {
> > struct clk *clk;
> > struct usb_phy *phy;
> > struct usb_otg *otg;
> > + struct regulator *vdd33;
> > + struct regulator *vdd10;
> > };
> >
> > static void exynos_ohci_phy_enable(struct platform_device *pdev)
> > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct
> platform_device
> > *pdev)
> > exynos_ohci->otg = phy->otg;
> > }
> >
> > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> > + if (IS_ERR(exynos_ohci->vdd33)) {
> > + err = PTR_ERR(exynos_ohci->vdd33);
> > + goto fail_regulator1;
> > + }
> > + err = regulator_enable(exynos_ohci->vdd33);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> > + goto fail_regulator1;
> > + }
> > +
> > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> > + if (IS_ERR(exynos_ohci->vdd10)) {
> > + err = PTR_ERR(exynos_ohci->vdd10);
> > + goto fail_regulator2;
> > + }
> > + err = regulator_enable(exynos_ohci->vdd10);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> > + goto fail_regulator2;
> > + }
> > +
>
> Do we need to skip regulator settings together with PHY configuration
> in case of exynos5440?
>
> > skip_phy:
> > exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
> >
> > @@ -154,6 +179,10 @@ fail_add_hcd:
> > fail_io:
> > clk_disable_unprepare(exynos_ohci->clk);
> > fail_clk:
> > + regulator_disable(exynos_ohci->vdd10);
> > +fail_regulator2:
> > + regulator_disable(exynos_ohci->vdd33);
> > +fail_regulator1:
> > usb_put_hcd(hcd);
> > return err;
> > }
> > @@ -172,6 +201,9 @@ static int exynos_ohci_remove(struct
> > platform_device *pdev)
> >
> > clk_disable_unprepare(exynos_ohci->clk);
> >
> > + regulator_disable(exynos_ohci->vdd10);
> > + regulator_disable(exynos_ohci->vdd33);
> > +
> > usb_put_hcd(hcd);
> >
> > return 0;
> > @@ -208,6 +240,9 @@ static int exynos_ohci_suspend(struct device *dev)
> >
> > clk_disable_unprepare(exynos_ohci->clk);
> >
> > + regulator_disable(exynos_ohci->vdd10);
> > + regulator_disable(exynos_ohci->vdd33);
> > +
> > spin_unlock_irqrestore(&ohci->lock, flags);
> >
> > return 0;
> > @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
> > struct usb_hcd *hcd = dev_get_drvdata(dev);
> > struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> > struct platform_device *pdev = to_platform_device(dev);
> > + int ret;
> > +
> > + ret = regulator_enable(exynos_ohci->vdd33);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable VDD33 supply\n");
> > + return ret;
>
> Not sure, but shall we continue resuming and do everything
> we can in case of error? At least it will avoid
> WARN_ON(clk->enable_count == 0) on next system suspend.

On the other hand, if power is not applied to the controller,
register access in ohci_resume() may lead to undefined behavior.
What's your opinion?

>
> > + }
> > + ret = regulator_enable(exynos_ohci->vdd10);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable VDD10 supply\n");
> > + return ret;
> > + }
> >
> > clk_prepare_enable(exynos_ohci->clk);
> >
> > --
>
> Thanks

Thanks

2014-04-24 01:26:20

[permalink] [raw]

Subject: Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

On Thursday, April 24, 2014 9:33 AM, Jingoo Han wrote:
> On Thursday, April 24, 2014 9:18 AM, Anton Tikhomirov wrote:
> > On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
> > >
> > > Facilitate getting required 3.3V and 1.0V VDD supply for
> > > OHCI controller on Exynos.
> > >
> > > With patches for regulators' nodes merged in 3.15:
> > > c8c253f ARM: dts: Add regulator entries to smdk5420
> > > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> > >
> > > certain perripherals will now need to ensure that,
> > > they request VDD regulators in their drivers, and enable
> > > them so as to make them working.
> > >
> > > Signed-off-by: Vivek Gautam <[email protected]>
> > > Cc: Jingoo Han <[email protected]>
> > > ---
> > >
> > > Based on 'usb-next' branch of Greg's usb tree.
> > >
> > > drivers/usb/host/ohci-exynos.c | 47
> > > ++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
> > > exynos.c
> > > index 68588d8..e2e72a8 100644
> > > --- a/drivers/usb/host/ohci-exynos.c
> > > +++ b/drivers/usb/host/ohci-exynos.c
> > > @@ -18,6 +18,7 @@
> > > #include <linux/module.h>
> > > #include <linux/of.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > #include <linux/usb/phy.h>
> > > #include <linux/usb/samsung_usb_phy.h>
> > > #include <linux/usb.h>
> > > @@ -37,6 +38,8 @@ struct exynos_ohci_hcd {
> > > struct clk *clk;
> > > struct usb_phy *phy;
> > > struct usb_otg *otg;
> > > + struct regulator *vdd33;
> > > + struct regulator *vdd10;
> > > };
> > >
> > > static void exynos_ohci_phy_enable(struct platform_device *pdev)
> > > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device
> > > *pdev)
> > > exynos_ohci->otg = phy->otg;
> > > }
> > >
> > > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> > > + if (IS_ERR(exynos_ohci->vdd33)) {
> > > + err = PTR_ERR(exynos_ohci->vdd33);
> > > + goto fail_regulator1;
> > > + }
> > > + err = regulator_enable(exynos_ohci->vdd33);
> > > + if (err) {
> > > + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> > > + goto fail_regulator1;
> > > + }
> > > +
> > > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> > > + if (IS_ERR(exynos_ohci->vdd10)) {
> > > + err = PTR_ERR(exynos_ohci->vdd10);
> > > + goto fail_regulator2;
> > > + }
> > > + err = regulator_enable(exynos_ohci->vdd10);
> > > + if (err) {
> > > + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> > > + goto fail_regulator2;
> > > + }
> > > +
> >
> > Do we need to skip regulator settings together with PHY configuration
> > in case of exynos5440?
>
> Oh, right. In the case of exynos5440, regulator settings is not
> necessary. Vivek, would you fix it in order skip regulator settings
> in exynos5440? It also applies to ehci-exynos.

Sorry, in the case of exynos5440, this patch already skips
regulator settings.

In the case of exynos5440, there is no need to set PHY setting
and regulator setting.

How about making regulator setting "optional"?
Then, regulator setting can be done, only when regulator
is supported.

exynos_ohci_probe()
exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
if (IS_ERR(exynos_ohci->vdd33)) {
dev_err(&pdev->dev, "Failed to get VDD33 supply\n");
} else {
err = regulator_enable(exynos_ohci->vdd33);
if (err) {
dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
goto fail_regulator1;
}
}

exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
if (IS_ERR(exynos_ohci->vdd10)) {
dev_err(&pdev->dev, "Failed to get VDD10 supply\n");
} else {
err = regulator_enable(exynos_ohci->vdd10);
if (err) {
dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
goto fail_regulator2;
}
}

In this case, suspend/resume can be fixed as below.

exynos_ohci_suspend()
if (exynos_ohci->vdd10)
regulator_disable(exynos_ohci->vdd10);
if (exynos_ohci->vdd33)
regulator_disable(exynos_ohci->vdd33);

exynos_ohci_resume()

if (exynos_ohci->vdd33) {
ret = regulator_enable(exynos_ohci->vdd33);
if (ret) {
dev_err(dev, "Failed to enable VDD33 supply\n");
return ret;
}
}
if (exynos_ohci->vdd10) {
ret = regulator_enable(exynos_ohci->vdd10);
if (ret) {
dev_err(dev, "Failed to enable VDD10 supply\n");
return ret;
}
}

Best regards,
Jingoo Han

>
> >
> > > skip_phy:
> > > exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
> > >
> > > @@ -154,6 +179,10 @@ fail_add_hcd:
> > > fail_io:
> > > clk_disable_unprepare(exynos_ohci->clk);
> > > fail_clk:
> > > + regulator_disable(exynos_ohci->vdd10);
> > > +fail_regulator2:
> > > + regulator_disable(exynos_ohci->vdd33);
> > > +fail_regulator1:
> > > usb_put_hcd(hcd);
> > > return err;
> > > }
> > > @@ -172,6 +201,9 @@ static int exynos_ohci_remove(struct
> > > platform_device *pdev)
> > >
> > > clk_disable_unprepare(exynos_ohci->clk);
> > >
> > > + regulator_disable(exynos_ohci->vdd10);
> > > + regulator_disable(exynos_ohci->vdd33);
> > > +
> > > usb_put_hcd(hcd);
> > >
> > > return 0;
> > > @@ -208,6 +240,9 @@ static int exynos_ohci_suspend(struct device *dev)
> > >
> > > clk_disable_unprepare(exynos_ohci->clk);
> > >
> > > + regulator_disable(exynos_ohci->vdd10);
> > > + regulator_disable(exynos_ohci->vdd33);
> > > +
> > > spin_unlock_irqrestore(&ohci->lock, flags);
> > >
> > > return 0;
> > > @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
> > > struct usb_hcd *hcd = dev_get_drvdata(dev);
> > > struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> > > struct platform_device *pdev = to_platform_device(dev);
> > > + int ret;
> > > +
> > > + ret = regulator_enable(exynos_ohci->vdd33);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable VDD33 supply\n");
> > > + return ret;
> >
> > Not sure, but shall we continue resuming and do everything
> > we can in case of error? At least it will avoid
> > WARN_ON(clk->enable_count == 0) on next system suspend.
> >
> > > + }
> > > + ret = regulator_enable(exynos_ohci->vdd10);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable VDD10 supply\n");
> > > + return ret;
> > > + }
> > >
> > > clk_prepare_enable(exynos_ohci->clk);
> > >
> > > --
> >
> > Thanks

2014-04-24 06:33:01

[permalink] [raw]

Subject: Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

Hi,

On Thu, Apr 24, 2014 at 6:08 AM, Anton Tikhomirov
<[email protected]> wrote:
> Hi,
>
>> Hi,
>>
>> > -----Original Message-----
>> > From: [email protected] [mailto:linux-usb-
>> > [email protected]] On Behalf Of Vivek Gautam
>> > Sent: Monday, April 21, 2014 9:17 PM
>> >
>> > Facilitate getting required 3.3V and 1.0V VDD supply for
>> > OHCI controller on Exynos.
>> >
>> > With patches for regulators' nodes merged in 3.15:
>> > c8c253f ARM: dts: Add regulator entries to smdk5420
>> > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> >
>> > certain perripherals will now need to ensure that,
>> > they request VDD regulators in their drivers, and enable
>> > them so as to make them working.
>> >
>> > Signed-off-by: Vivek Gautam <[email protected]>
>> > Cc: Jingoo Han <[email protected]>
>> > ---
>> >
>> > Based on 'usb-next' branch of Greg's usb tree.
>> >
>> > drivers/usb/host/ohci-exynos.c | 47
>> > ++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 47 insertions(+)
>> >
>> > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
>> > exynos.c
>> > index 68588d8..e2e72a8 100644
>> > --- a/drivers/usb/host/ohci-exynos.c
>> > +++ b/drivers/usb/host/ohci-exynos.c

[snip]

>> > static void exynos_ohci_phy_enable(struct platform_device *pdev)
>> > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct
>> platform_device
>> > *pdev)
>> > exynos_ohci->otg = phy->otg;
>> > }
>> >
>> > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
>> > + if (IS_ERR(exynos_ohci->vdd33)) {
>> > + err = PTR_ERR(exynos_ohci->vdd33);
>> > + goto fail_regulator1;
>> > + }
>> > + err = regulator_enable(exynos_ohci->vdd33);
>> > + if (err) {
>> > + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
>> > + goto fail_regulator1;
>> > + }
>> > +
>> > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
>> > + if (IS_ERR(exynos_ohci->vdd10)) {
>> > + err = PTR_ERR(exynos_ohci->vdd10);
>> > + goto fail_regulator2;
>> > + }
>> > + err = regulator_enable(exynos_ohci->vdd10);
>> > + if (err) {
>> > + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
>> > + goto fail_regulator2;
>> > + }
>> > +
>>
>> Do we need to skip regulator settings together with PHY configuration
>> in case of exynos5440?
>>
>> > skip_phy:
>> > exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
>> >

[snip]

>> > @@ -218,6 +253,18 @@ static int exynos_ohci_resume(struct device *dev)
>> > struct usb_hcd *hcd = dev_get_drvdata(dev);
>> > struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> > struct platform_device *pdev = to_platform_device(dev);
>> > + int ret;
>> > +
>> > + ret = regulator_enable(exynos_ohci->vdd33);
>> > + if (ret) {
>> > + dev_err(dev, "Failed to enable VDD33 supply\n");
>> > + return ret;
>>
>> Not sure, but shall we continue resuming and do everything
>> we can in case of error? At least it will avoid
>> WARN_ON(clk->enable_count == 0) on next system suspend.

True, i will modify it.
>
> On the other hand, if power is not applied to the controller,
> register access in ohci_resume() may lead to undefined behavior.
> What's your opinion?

AFA i think, it is obvious that the regulators would not work without
a VDD supply.
So the question is, do we have VDD LDOs available on all Exynos
systems for usb ?
>From the schemata of Exynos5250 and Exynos5420 boards, i can see the
required VDD LDOs
for USB. Unfortunately at present i don't have a Exynos4* schemata.

If regulator is not a mandatory, then we can make it option similar to
what Jingoo has also suggested
in subsequent mail.

[snip]

--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

2014-04-24 06:40:30

[permalink] [raw]

Subject: Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

Hi Jingoo,

On Thu, Apr 24, 2014 at 6:56 AM, Jingoo Han <[email protected]> wrote:
> On Thursday, April 24, 2014 9:33 AM, Jingoo Han wrote:
>> On Thursday, April 24, 2014 9:18 AM, Anton Tikhomirov wrote:
>> > On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
>> > >
>> > > Facilitate getting required 3.3V and 1.0V VDD supply for
>> > > OHCI controller on Exynos.
>> > >
>> > > With patches for regulators' nodes merged in 3.15:
>> > > c8c253f ARM: dts: Add regulator entries to smdk5420
>> > > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> > >
>> > > certain perripherals will now need to ensure that,
>> > > they request VDD regulators in their drivers, and enable
>> > > them so as to make them working.
>> > >
>> > > Signed-off-by: Vivek Gautam <[email protected]>
>> > > Cc: Jingoo Han <[email protected]>
>> > > ---
>> > >
>> > > Based on 'usb-next' branch of Greg's usb tree.
>> > >
>> > > drivers/usb/host/ohci-exynos.c | 47
>> > > ++++++++++++++++++++++++++++++++++++++++
>> > > 1 file changed, 47 insertions(+)
>> > >
>> > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
>> > > exynos.c
>> > > index 68588d8..e2e72a8 100644
>> > > --- a/drivers/usb/host/ohci-exynos.c
>> > > +++ b/drivers/usb/host/ohci-exynos.c

[snip]

>> > > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device
>> > > *pdev)
>> > > exynos_ohci->otg = phy->otg;
>> > > }
>> > >
>> > > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
>> > > + if (IS_ERR(exynos_ohci->vdd33)) {
>> > > + err = PTR_ERR(exynos_ohci->vdd33);
>> > > + goto fail_regulator1;
>> > > + }
>> > > + err = regulator_enable(exynos_ohci->vdd33);
>> > > + if (err) {
>> > > + dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
>> > > + goto fail_regulator1;
>> > > + }
>> > > +
>> > > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
>> > > + if (IS_ERR(exynos_ohci->vdd10)) {
>> > > + err = PTR_ERR(exynos_ohci->vdd10);
>> > > + goto fail_regulator2;
>> > > + }
>> > > + err = regulator_enable(exynos_ohci->vdd10);
>> > > + if (err) {
>> > > + dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
>> > > + goto fail_regulator2;
>> > > + }
>> > > +
>> >
>> > Do we need to skip regulator settings together with PHY configuration
>> > in case of exynos5440?
>>
>> Oh, right. In the case of exynos5440, regulator settings is not
>> necessary. Vivek, would you fix it in order skip regulator settings
>> in exynos5440? It also applies to ehci-exynos.
>
> Sorry, in the case of exynos5440, this patch already skips
> regulator settings.
>
> In the case of exynos5440, there is no need to set PHY setting
> and regulator setting.

Right, in case of exynos5440, we are skipping PHY setting and regulator setting.
Actually i had missed taking into account 5440, so just curious. Do we
really not need a regulator
settings for Exynos5440 ?

>
> How about making regulator setting "optional"?
> Then, regulator setting can be done, only when regulator
> is supported.

True, so with Exynos5440 not needing the regulator, we should make the
regulator settings optional.

>
> exynos_ohci_probe()
> exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
> if (IS_ERR(exynos_ohci->vdd33)) {
> dev_err(&pdev->dev, "Failed to get VDD33 supply\n");
> } else {
> err = regulator_enable(exynos_ohci->vdd33);
> if (err) {
> dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
> goto fail_regulator1;
> }
> }
>
> exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
> if (IS_ERR(exynos_ohci->vdd10)) {
> dev_err(&pdev->dev, "Failed to get VDD10 supply\n");
> } else {
> err = regulator_enable(exynos_ohci->vdd10);
> if (err) {
> dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
> goto fail_regulator2;
> }
> }
>
> In this case, suspend/resume can be fixed as below.
>
> exynos_ohci_suspend()
> if (exynos_ohci->vdd10)
> regulator_disable(exynos_ohci->vdd10);
> if (exynos_ohci->vdd33)
> regulator_disable(exynos_ohci->vdd33);
>
> exynos_ohci_resume()
>
> if (exynos_ohci->vdd33) {
> ret = regulator_enable(exynos_ohci->vdd33);
> if (ret) {
> dev_err(dev, "Failed to enable VDD33 supply\n");
> return ret;
> }
> }
> if (exynos_ohci->vdd10) {
> ret = regulator_enable(exynos_ohci->vdd10);
> if (ret) {
> dev_err(dev, "Failed to enable VDD10 supply\n");
> return ret;
> }
> }

Thanks for the suggestion.
I will make the required changes, and post the patchset again.

[snip]

--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

2014-04-24 06:57:37