[PATCH 0/4] usb: dwc3/xhci: Enable runtime power management

[PATCH 0/4] usb: dwc3/xhci: Enable runtime power management

[Vivek Gautam]

This patch-series enables runtime power management on xhci-plat,
dwc3-core, dwc3-exynos as well as on samsung-usb3 type PHY.
This allows usb 3.0 host ports to be power managed at runtime.
We also turn off the PHY ref_clk PLL, which supplies reference clock
to USB3 type phy, when ports are not in use.

This patch-series is based on 'usb-next' with follwing patches:

usb: phy: samsung: Introducing usb phy driver for hsotg
usb: phy: s3c-hsotg: adding phy driver support

usb: phy: samsung: Add support to set pmu isolation
ARM: EXYNOS: Update & move usb-phy types to generic include layer
usb: phy: samsung: Add host phy support to samsung-phy driver
USB: ehci-s5p: Add phy driver support
USB: ohci-exynos: Add phy driver support
usb: phy: samsung: Remove __devinit, __devexit_p and __exit annotations

[PATCH v4 1/2] usb: phy: samsung: Common out the generic stuff
[PATCH v4 2/2] usb: phy: samsung: Add PHY support for USB 3.0 controller

Vivek Gautam (4):
  usb: xhci: Enable runtime pm in xhci-plat
  USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend
  usb: dwc3: exynos: Enable runtime power management
  usb: phy: samsung: Enable runtime power management on samsung-usb3

 drivers/usb/dwc3/core.c          |    4 +-
 drivers/usb/dwc3/dwc3-exynos.c   |   47 +++++++++++++++++
 drivers/usb/host/xhci-plat.c     |    9 +++
 drivers/usb/phy/samsung-usb3.c   |  107 +++++++++++++++++++++++++++++++++++--
 drivers/usb/phy/samsung-usbphy.c |   26 +++++++++
 drivers/usb/phy/samsung-usbphy.h |    1 +
 6 files changed, 187 insertions(+), 7 deletions(-)

-- 
1.7.6.5

[PATCH 1/4] usb: xhci: Enable runtime pm in xhci-plat

[Vivek Gautam]

By enabling runtime pm in this driver is allows users of xhci-plat to
enter into runtime pm.  This is not full runtime pm support (AKA
xhci-plat doesn't actually power anything off when in runtime suspend
mode) but just basic enablement.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/host/xhci-plat.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index df90fe5..6ad7cfb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -139,6 +139,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		goto dealloc_usb2_hcd;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_forbid(&pdev->dev);
+
 	/*
 	 * Set the xHCI pointer before xhci_plat_setup() (aka hcd_driver.reset)
 	 * is called by usb_add_hcd().
@@ -149,10 +153,13 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto put_usb3_hcd;
 
+	pm_runtime_allow(&pdev->dev);
+
 	return 0;
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
+	pm_runtime_disable(&pdev->dev);
 
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
@@ -174,6 +181,8 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
+	pm_runtime_disable(&dev->dev);
+
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
-- 
1.7.6.5

[PATCH 2/4] USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend

[Vivek Gautam]

The current code in the dwc3 probe effectively disables runtime pm
from ever working because it calls a get() that was never put() until
device removal.  Change the runtime pm code to match the standard
formula and allow runtime pm to function.

Note that this doesn't enable full runtime pm on the DWC3 device in
that the port isn't put into a lower power mode when not used.
However it does allow users of dwc3 (like dwc3-exynos) to do some
amount of runtime power management.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/dwc3/core.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3a4004a..59c2494 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -453,6 +453,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (of_get_property(node, "tx-fifo-resize", NULL))
 		dwc->needs_fifo_resize = true;
 
+	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	pm_runtime_forbid(dev);
@@ -517,6 +518,7 @@ static int dwc3_probe(struct platform_device *pdev)
 		goto err2;
 	}
 
+	pm_runtime_put(dev);
 	pm_runtime_allow(dev);
 
 	return 0;
@@ -543,6 +545,7 @@ err1:
 
 err0:
 	dwc3_free_event_buffers(dwc);
+	pm_runtime_disable(&pdev->dev);
 
 	return ret;
 }
@@ -554,7 +557,6 @@ static int dwc3_remove(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	dwc3_debugfs_exit(dwc);
-- 
1.7.6.5

Re: [PATCH 2/4] USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend

[Felipe Balbi]

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

On Mon, Jan 28, 2013 at 05:12:26PM +0530, Vivek Gautam wrote:
> The current code in the dwc3 probe effectively disables runtime pm
> from ever working because it calls a get() that was never put() until
> device removal.  Change the runtime pm code to match the standard
> formula and allow runtime pm to function.
> 
> Note that this doesn't enable full runtime pm on the DWC3 device in
> that the port isn't put into a lower power mode when not used.
> However it does allow users of dwc3 (like dwc3-exynos) to do some
> amount of runtime power management.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc3/core.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 3a4004a..59c2494 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -453,6 +453,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (of_get_property(node, "tx-fifo-resize", NULL))
>  		dwc->needs_fifo_resize = true;
>  
> +	pm_runtime_set_active(dev);

this usage of pm_runtime_set_active() actually makes me a bit scared. At
least OMAP starts with the device switched off, so this will probably
break OMAP at least. OTOH, calling ->runtime_resume() during probe()
might not make that much sense after all, but the way OMAP is
implemented, we won't get clocks turned on if this ->runtime_resume()
method isn't called.

/me starts to wonder whether OMAP implementation is flakey and what
should be done here...

-- 
balbi

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

Re: [PATCH 2/4] USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend

[Vivek Gautam]

Hi Felipe,


On Mon, Jan 28, 2013 at 5:15 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Jan 28, 2013 at 05:12:26PM +0530, Vivek Gautam wrote:
>> The current code in the dwc3 probe effectively disables runtime pm
>> from ever working because it calls a get() that was never put() until
>> device removal.  Change the runtime pm code to match the standard
>> formula and allow runtime pm to function.
>>
>> Note that this doesn't enable full runtime pm on the DWC3 device in
>> that the port isn't put into a lower power mode when not used.
>> However it does allow users of dwc3 (like dwc3-exynos) to do some
>> amount of runtime power management.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/usb/dwc3/core.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 3a4004a..59c2494 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -453,6 +453,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>       if (of_get_property(node, "tx-fifo-resize", NULL))
>>               dwc->needs_fifo_resize = true;
>>
>> +     pm_runtime_set_active(dev);
>
> this usage of pm_runtime_set_active() actually makes me a bit scared. At
> least OMAP starts with the device switched off, so this will probably
> break OMAP at least.

I am fine with dropping pm_runtime_set_active(), actually thought
to put device in active state so that as and when system finds it idle,
force into suspend state.

I fact should i drop  pm_runtime_set_active() calls from other places too
(xhci-plat, dwc3-exynos, and samsung-usb3 phy) and call get_sync() alongwith
enable() ?

> OTOH, calling ->runtime_resume() during probe()
> might not make that much sense after all, but the way OMAP is
> implemented, we won't get clocks turned on if this ->runtime_resume()
> method isn't called.
>
> /me starts to wonder whether OMAP implementation is flakey and what
> should be done here...
>



-- 
Thanks & Regards
Vivek

Re: [PATCH 2/4] USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend

[Felipe Balbi]

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

Hi,

sorry for the delay

On Mon, Jan 28, 2013 at 07:06:56PM +0530, Vivek Gautam wrote:
> Hi Felipe,
> 
> 
> On Mon, Jan 28, 2013 at 5:15 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Mon, Jan 28, 2013 at 05:12:26PM +0530, Vivek Gautam wrote:
> >> The current code in the dwc3 probe effectively disables runtime pm
> >> from ever working because it calls a get() that was never put() until
> >> device removal.  Change the runtime pm code to match the standard
> >> formula and allow runtime pm to function.
> >>
> >> Note that this doesn't enable full runtime pm on the DWC3 device in
> >> that the port isn't put into a lower power mode when not used.
> >> However it does allow users of dwc3 (like dwc3-exynos) to do some
> >> amount of runtime power management.
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >> ---
> >>  drivers/usb/dwc3/core.c |    4 +++-
> >>  1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index 3a4004a..59c2494 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -453,6 +453,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >>       if (of_get_property(node, "tx-fifo-resize", NULL))
> >>               dwc->needs_fifo_resize = true;
> >>
> >> +     pm_runtime_set_active(dev);
> >
> > this usage of pm_runtime_set_active() actually makes me a bit scared. At
> > least OMAP starts with the device switched off, so this will probably
> > break OMAP at least.
> 
> I am fine with dropping pm_runtime_set_active(), actually thought
> to put device in active state so that as and when system finds it idle,
> force into suspend state.
> 
> I fact should i drop  pm_runtime_set_active() calls from other places too
> (xhci-plat, dwc3-exynos, and samsung-usb3 phy) and call get_sync() alongwith
> enable() ?

that's correct, make sure it works fine for you ;-)

-- 
balbi

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

Re: [PATCH 2/4] USB: dwc3: Adjust runtime pm the dwc3 driver to allow runtime suspend

[Vivek Gautam]

Hi Felipe,


On Wed, Feb 27, 2013 at 1:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> sorry for the delay
>

That's alright ;-)

> On Mon, Jan 28, 2013 at 07:06:56PM +0530, Vivek Gautam wrote:
>> Hi Felipe,
>>
>>
>> On Mon, Jan 28, 2013 at 5:15 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Mon, Jan 28, 2013 at 05:12:26PM +0530, Vivek Gautam wrote:
>> >> The current code in the dwc3 probe effectively disables runtime pm
>> >> from ever working because it calls a get() that was never put() until
>> >> device removal.  Change the runtime pm code to match the standard
>> >> formula and allow runtime pm to function.
>> >>
>> >> Note that this doesn't enable full runtime pm on the DWC3 device in
>> >> that the port isn't put into a lower power mode when not used.
>> >> However it does allow users of dwc3 (like dwc3-exynos) to do some
>> >> amount of runtime power management.
>> >>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> >> ---
>> >>  drivers/usb/dwc3/core.c |    4 +++-
>> >>  1 files changed, 3 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> >> index 3a4004a..59c2494 100644
>> >> --- a/drivers/usb/dwc3/core.c
>> >> +++ b/drivers/usb/dwc3/core.c
>> >> @@ -453,6 +453,7 @@ static int dwc3_probe(struct platform_device *pdev)
>> >>       if (of_get_property(node, "tx-fifo-resize", NULL))
>> >>               dwc->needs_fifo_resize = true;
>> >>
>> >> +     pm_runtime_set_active(dev);
>> >
>> > this usage of pm_runtime_set_active() actually makes me a bit scared. At
>> > least OMAP starts with the device switched off, so this will probably
>> > break OMAP at least.
>>
>> I am fine with dropping pm_runtime_set_active(), actually thought
>> to put device in active state so that as and when system finds it idle,
>> force into suspend state.
>>
>> I fact should i drop  pm_runtime_set_active() calls from other places too
>> (xhci-plat, dwc3-exynos, and samsung-usb3 phy) and call get_sync() alongwith
>> enable() ?
>
> that's correct, make sure it works fine for you ;-)
>
Yeah sure.


-- 
Thanks & Regards
Vivek

[PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Vivek Gautam]

Enabling runtime power management on dwc3-exynos to save
power and allow its PHY's power to be managed at runtime.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/dwc3/dwc3-exynos.c |   47 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index aae5328..c51e8c1 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -157,11 +157,15 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err4;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 err4:
 	clk_disable(clk);
 	clk_put(clk);
+	pm_runtime_disable(&pdev->dev);
 err3:
 	platform_device_put(dwc3);
 err1:
@@ -174,6 +178,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+
 	platform_device_unregister(exynos->dwc3);
 	platform_device_unregister(exynos->usb2_phy);
 	platform_device_unregister(exynos->usb3_phy);
@@ -186,6 +192,46 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dwc3_exynos_runtime_suspend(struct device *dev)
+{
+	struct dwc3_exynos	*exynos = dev_get_drvdata(dev);
+	struct platform_device	*pdev_dwc = exynos->dwc3;
+	struct dwc3		*dwc = NULL;
+
+	dwc = platform_get_drvdata(pdev_dwc);
+
+	if (!dwc)
+		return 0;
+
+	pm_runtime_put_sync(dwc->usb3_phy->dev);
+
+	clk_disable(exynos->clk);
+
+	return 0;
+}
+static int dwc3_exynos_runtime_resume(struct device *dev)
+{
+	struct dwc3_exynos	*exynos = dev_get_drvdata(dev);
+	struct platform_device	*pdev_dwc = exynos->dwc3;
+	struct dwc3		*dwc = NULL;
+
+	dwc = platform_get_drvdata(pdev_dwc);
+
+	clk_enable(exynos->clk);
+
+	if (!dwc)
+		return 0;
+
+	pm_runtime_get_sync(dwc->usb3_phy->dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dwc3_exynos_pm_ops = {
+	SET_RUNTIME_PM_OPS(dwc3_exynos_runtime_suspend,
+				dwc3_exynos_runtime_resume, NULL)
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id exynos_dwc3_match[] = {
 	{ .compatible = "samsung,exynos-dwc3" },
@@ -200,6 +246,7 @@ static struct platform_driver dwc3_exynos_driver = {
 	.driver		= {
 		.name	= "exynos-dwc3",
 		.of_match_table = of_match_ptr(exynos_dwc3_match),
+		.pm	= &dwc3_exynos_pm_ops,
 	},
 };
 
-- 
1.7.6.5

Re: [PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Felipe Balbi]

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

On Mon, Jan 28, 2013 at 05:12:27PM +0530, Vivek Gautam wrote:
> Enabling runtime power management on dwc3-exynos to save
> power and allow its PHY's power to be managed at runtime.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c |   47 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index aae5328..c51e8c1 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -157,11 +157,15 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  		goto err4;
>  	}
>  
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	return 0;
>  
>  err4:
>  	clk_disable(clk);
>  	clk_put(clk);
> +	pm_runtime_disable(&pdev->dev);
>  err3:
>  	platform_device_put(dwc3);
>  err1:
> @@ -174,6 +178,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>  {
>  	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
>  
> +	pm_runtime_disable(&pdev->dev);
> +
>  	platform_device_unregister(exynos->dwc3);
>  	platform_device_unregister(exynos->usb2_phy);
>  	platform_device_unregister(exynos->usb3_phy);
> @@ -186,6 +192,46 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int dwc3_exynos_runtime_suspend(struct device *dev)
> +{
> +	struct dwc3_exynos	*exynos = dev_get_drvdata(dev);
> +	struct platform_device	*pdev_dwc = exynos->dwc3;
> +	struct dwc3		*dwc = NULL;
> +
> +	dwc = platform_get_drvdata(pdev_dwc);
> +
> +	if (!dwc)
> +		return 0;
> +
> +	pm_runtime_put_sync(dwc->usb3_phy->dev);
> +
> +	clk_disable(exynos->clk);
> +
> +	return 0;
> +}
> +static int dwc3_exynos_runtime_resume(struct device *dev)
> +{
> +	struct dwc3_exynos	*exynos = dev_get_drvdata(dev);
> +	struct platform_device	*pdev_dwc = exynos->dwc3;
> +	struct dwc3		*dwc = NULL;
> +
> +	dwc = platform_get_drvdata(pdev_dwc);
> +
> +	clk_enable(exynos->clk);
> +
> +	if (!dwc)
> +		return 0;
> +
> +	pm_runtime_get_sync(dwc->usb3_phy->dev);

dude, this is wrong :-)

look at this:

pm_runtime_get() -> dwc3_exynos_runtime_resume() ->
pm_runtime_get_sync() -> dwc3_exynos_runtime_resume() -> ...

only your clock enalbe should do

-- 
balbi

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

Re: [PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Vivek Gautam]

Hi Balbi,


On Mon, Jan 28, 2013 at 5:17 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Jan 28, 2013 at 05:12:27PM +0530, Vivek Gautam wrote:
>> Enabling runtime power management on dwc3-exynos to save
>> power and allow its PHY's power to be managed at runtime.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> ---
>>  drivers/usb/dwc3/dwc3-exynos.c |   47 ++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> index aae5328..c51e8c1 100644
>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> @@ -157,11 +157,15 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>               goto err4;
>>       }
>>
>> +     pm_runtime_set_active(&pdev->dev);
>> +     pm_runtime_enable(&pdev->dev);
>> +
>>       return 0;
>>
>>  err4:
>>       clk_disable(clk);
>>       clk_put(clk);
>> +     pm_runtime_disable(&pdev->dev);
>>  err3:
>>       platform_device_put(dwc3);
>>  err1:
>> @@ -174,6 +178,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>  {
>>       struct dwc3_exynos      *exynos = platform_get_drvdata(pdev);
>>
>> +     pm_runtime_disable(&pdev->dev);
>> +
>>       platform_device_unregister(exynos->dwc3);
>>       platform_device_unregister(exynos->usb2_phy);
>>       platform_device_unregister(exynos->usb3_phy);
>> @@ -186,6 +192,46 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +static int dwc3_exynos_runtime_suspend(struct device *dev)
>> +{
>> +     struct dwc3_exynos      *exynos = dev_get_drvdata(dev);
>> +     struct platform_device  *pdev_dwc = exynos->dwc3;
>> +     struct dwc3             *dwc = NULL;
>> +
>> +     dwc = platform_get_drvdata(pdev_dwc);
>> +
>> +     if (!dwc)
>> +             return 0;
>> +
>> +     pm_runtime_put_sync(dwc->usb3_phy->dev);
>> +
>> +     clk_disable(exynos->clk);
>> +
>> +     return 0;
>> +}
>> +static int dwc3_exynos_runtime_resume(struct device *dev)
>> +{
>> +     struct dwc3_exynos      *exynos = dev_get_drvdata(dev);
>> +     struct platform_device  *pdev_dwc = exynos->dwc3;
>> +     struct dwc3             *dwc = NULL;
>> +
>> +     dwc = platform_get_drvdata(pdev_dwc);
>> +
>> +     clk_enable(exynos->clk);
>> +
>> +     if (!dwc)
>> +             return 0;
>> +
>> +     pm_runtime_get_sync(dwc->usb3_phy->dev);
>
> dude, this is wrong :-)
>
> look at this:
>
> pm_runtime_get() -> dwc3_exynos_runtime_resume() ->
> pm_runtime_get_sync() -> dwc3_exynos_runtime_resume() -> ...
>
> only your clock enalbe should do
>

We want to wake up "dwc->usb3_phy" so tried to call pm_runtime_get_sync()
with "dwc->usb3_phy->dev".
Missing something ? :-(




-- 
Thanks & Regards
Vivek

Re: [PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Felipe Balbi]

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

Hi,

On Mon, Jan 28, 2013 at 05:28:30PM +0530, Vivek Gautam wrote:
> >> +static int dwc3_exynos_runtime_resume(struct device *dev)
> >> +{
> >> +     struct dwc3_exynos      *exynos = dev_get_drvdata(dev);
> >> +     struct platform_device  *pdev_dwc = exynos->dwc3;
> >> +     struct dwc3             *dwc = NULL;
> >> +
> >> +     dwc = platform_get_drvdata(pdev_dwc);
> >> +
> >> +     clk_enable(exynos->clk);
> >> +
> >> +     if (!dwc)
> >> +             return 0;
> >> +
> >> +     pm_runtime_get_sync(dwc->usb3_phy->dev);
> >
> > dude, this is wrong :-)
> >
> > look at this:
> >
> > pm_runtime_get() -> dwc3_exynos_runtime_resume() ->
> > pm_runtime_get_sync() -> dwc3_exynos_runtime_resume() -> ...
> >
> > only your clock enalbe should do
> >
> 
> We want to wake up "dwc->usb3_phy" so tried to call pm_runtime_get_sync()
> with "dwc->usb3_phy->dev".
> Missing something ? :-(

oh, my bad. That's the PHY... But we can't really do that for samsung
only. It needs to be done generically for the entire dwc3 core driver,
and for that we need to introduce usb_phy_autopm_get(),
usb_phy_autopm_get_sync() and friends.

Then, from dwc_probe() we call:

phy = usb_get_phy();
usb_phy_autopm_enable(phy);
usb_phy_autopm_get_sync(phy);

or something similar ;-) Bottom line, you shouldn't fiddle with phy->dev
directly.

-- 
balbi

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

Re: [PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Vivek Gautam]

On Mon, Jan 28, 2013 at 5:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Jan 28, 2013 at 05:28:30PM +0530, Vivek Gautam wrote:
>> >> +static int dwc3_exynos_runtime_resume(struct device *dev)
>> >> +{
>> >> +     struct dwc3_exynos      *exynos = dev_get_drvdata(dev);
>> >> +     struct platform_device  *pdev_dwc = exynos->dwc3;
>> >> +     struct dwc3             *dwc = NULL;
>> >> +
>> >> +     dwc = platform_get_drvdata(pdev_dwc);
>> >> +
>> >> +     clk_enable(exynos->clk);
>> >> +
>> >> +     if (!dwc)
>> >> +             return 0;
>> >> +
>> >> +     pm_runtime_get_sync(dwc->usb3_phy->dev);
>> >
>> > dude, this is wrong :-)
>> >
>> > look at this:
>> >
>> > pm_runtime_get() -> dwc3_exynos_runtime_resume() ->
>> > pm_runtime_get_sync() -> dwc3_exynos_runtime_resume() -> ...
>> >
>> > only your clock enalbe should do
>> >
>>
>> We want to wake up "dwc->usb3_phy" so tried to call pm_runtime_get_sync()
>> with "dwc->usb3_phy->dev".
>> Missing something ? :-(
>
> oh, my bad. That's the PHY... But we can't really do that for samsung
> only. It needs to be done generically for the entire dwc3 core driver,
> and for that we need to introduce usb_phy_autopm_get(),
> usb_phy_autopm_get_sync() and friends.
>

aah!! Ok. I definitely missed that part. :-(

> Then, from dwc_probe() we call:
>
> phy = usb_get_phy();
> usb_phy_autopm_enable(phy);
> usb_phy_autopm_get_sync(phy);
>
> or something similar ;-) Bottom line, you shouldn't fiddle with phy->dev
> directly.
>

Ok, the core should actually be handling the 'phy' not the glue layers.
Right ?

Will try putting these helper functions in place and come up with a
solution. :-)


-- 
Thanks & Regards
Vivek

Re: [PATCH 3/4] usb: dwc3: exynos: Enable runtime power management

[Felipe Balbi]

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

On Mon, Jan 28, 2013 at 05:57:04PM +0530, Vivek Gautam wrote:
> On Mon, Jan 28, 2013 at 5:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Jan 28, 2013 at 05:28:30PM +0530, Vivek Gautam wrote:
> >> >> +static int dwc3_exynos_runtime_resume(struct device *dev)
> >> >> +{
> >> >> +     struct dwc3_exynos      *exynos = dev_get_drvdata(dev);
> >> >> +     struct platform_device  *pdev_dwc = exynos->dwc3;
> >> >> +     struct dwc3             *dwc = NULL;
> >> >> +
> >> >> +     dwc = platform_get_drvdata(pdev_dwc);
> >> >> +
> >> >> +     clk_enable(exynos->clk);
> >> >> +
> >> >> +     if (!dwc)
> >> >> +             return 0;
> >> >> +
> >> >> +     pm_runtime_get_sync(dwc->usb3_phy->dev);
> >> >
> >> > dude, this is wrong :-)
> >> >
> >> > look at this:
> >> >
> >> > pm_runtime_get() -> dwc3_exynos_runtime_resume() ->
> >> > pm_runtime_get_sync() -> dwc3_exynos_runtime_resume() -> ...
> >> >
> >> > only your clock enalbe should do
> >> >
> >>
> >> We want to wake up "dwc->usb3_phy" so tried to call pm_runtime_get_sync()
> >> with "dwc->usb3_phy->dev".
> >> Missing something ? :-(
> >
> > oh, my bad. That's the PHY... But we can't really do that for samsung
> > only. It needs to be done generically for the entire dwc3 core driver,
> > and for that we need to introduce usb_phy_autopm_get(),
> > usb_phy_autopm_get_sync() and friends.
> >
> 
> aah!! Ok. I definitely missed that part. :-(
> 
> > Then, from dwc_probe() we call:
> >
> > phy = usb_get_phy();
> > usb_phy_autopm_enable(phy);
> > usb_phy_autopm_get_sync(phy);
> >
> > or something similar ;-) Bottom line, you shouldn't fiddle with phy->dev
> > directly.
> >
> 
> Ok, the core should actually be handling the 'phy' not the glue layers.
> Right ?

that's right, thanks for doing this ;-)

> Will try putting these helper functions in place and come up with a
> solution. :-)

awesome ;-)

cheers

-- 
balbi

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

[PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Vivek Gautam]

Enabling runtime power management support on samsung-usb3 phy
and further adding support to turn off the PHY ref_clk PLL.
It thereby requires PHY ref_clk to be switched between internal
core clock and external PLL clock.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/phy/samsung-usb3.c   |  107 +++++++++++++++++++++++++++++++++++--
 drivers/usb/phy/samsung-usbphy.c |   26 +++++++++
 drivers/usb/phy/samsung-usbphy.h |    1 +
 3 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/phy/samsung-usb3.c b/drivers/usb/phy/samsung-usb3.c
index 29e1321..4dbef15 100644
--- a/drivers/usb/phy/samsung-usb3.c
+++ b/drivers/usb/phy/samsung-usb3.c
@@ -22,8 +22,10 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/usb/samsung_usb_phy.h>
 #include <linux/platform_data/samsung-usbphy.h>
 
@@ -32,7 +34,7 @@
 /*
  * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
  */
-static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
+static u32 samsung_usb3_phy_set_refclk_int(struct samsung_usbphy *sphy)
 {
 	u32 reg;
 	u32 refclk;
@@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
 	return reg;
 }
 
-static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
+/*
+ * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
+ */
+static u32 samsung_usb3_phy_set_refclk_ext(void)
+{
+	u32 reg;
+
+	reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
+		PHYCLKRST_FSEL_PAD_100MHZ |
+		PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
+
+	return reg;
+}
+
+static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy,
+							bool use_ext_clk)
 {
 	void __iomem *regs = sphy->regs;
 	u32 phyparam0;
@@ -80,7 +97,11 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
 
 	phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
 	/* Select PHY CLK source */
-	phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
+	if (use_ext_clk)
+		phyparam0 |= PHYPARAM0_REF_USE_PAD;
+	else
+		phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
+
 	/* Set Loss-of-Signal Detector sensitivity */
 	phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
 	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
@@ -115,7 +136,10 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
 	/* UTMI Power Control */
 	writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
 
-	phyclkrst = samsung_usb3_phy_set_refclk(sphy);
+	if (use_ext_clk)
+		phyclkrst = samsung_usb3_phy_set_refclk_ext();
+	else
+		phyclkrst = samsung_usb3_phy_set_refclk_int(sphy);
 
 	phyclkrst |= PHYCLKRST_PORTRESET |
 			/* Digital power supply in normal operating mode */
@@ -163,7 +187,7 @@ static void samsung_exynos5_usb3_phy_disable(struct samsung_usbphy *sphy)
 	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
 }
 
-static int samsung_usb3_phy_init(struct usb_phy *phy)
+static int samsung_exynos5_usb3_phy_init(struct usb_phy *phy, bool use_ext_clk)
 {
 	struct samsung_usbphy *sphy;
 	unsigned long flags;
@@ -187,7 +211,7 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
 	samsung_usbphy_set_isolation(sphy, false);
 
 	/* Initialize usb phy registers */
-	samsung_exynos5_usb3_phy_enable(sphy);
+	samsung_exynos5_usb3_phy_enable(sphy, use_ext_clk);
 
 	spin_unlock_irqrestore(&sphy->lock, flags);
 
@@ -198,6 +222,34 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
 }
 
 /*
+ * Switch  between internal core clock and external oscillator clock
+ * for PHY reference clock
+ */
+static int samsung_exynos5_usb3phy_clk_switch(struct usb_phy *phy,
+						bool use_ext_clk)
+{
+	/*
+	 * This will switch PHY refclk from internal core clock
+	 * to external PLL clock when device is in use and vice versa
+	 * when device plunge into runtime suspend mode.
+	 */
+	return samsung_exynos5_usb3_phy_init(phy, use_ext_clk);
+}
+
+/*
+ * The function passed to the usb driver for phy initialization
+ */
+static int samsung_usb3_phy_init(struct usb_phy *phy)
+{
+	/*
+	 * We start with using PHY refclk from external PLL,
+	 * once runtime suspend for the device is called this
+	 * will change to internal core clock
+	 */
+	return  samsung_exynos5_usb3_phy_init(phy, true);
+}
+
+/*
  * The function passed to the usb driver for phy shutdown
  */
 static void samsung_usb3_phy_shutdown(struct usb_phy *phy)
@@ -287,6 +339,9 @@ static int samsung_usb3_phy_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sphy);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB3);
 }
 
@@ -296,6 +351,8 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	pm_runtime_disable(&pdev->dev);
+
 	if (sphy->pmuregs)
 		iounmap(sphy->pmuregs);
 	if (sphy->sysreg)
@@ -304,6 +361,42 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int samsung_usb3_phy_runtime_suspend(struct device *dev)
+{
+	struct samsung_usbphy *sphy = dev_get_drvdata(dev);
+
+	samsung_exynos5_usb3phy_clk_switch(&sphy->phy, false);
+
+	if (gpio_is_valid(sphy->phyclk_gpio))
+		gpio_set_value(sphy->phyclk_gpio, 0);
+
+	return 0;
+}
+
+static int samsung_usb3_phy_runtime_resume(struct device *dev)
+{
+	struct samsung_usbphy *sphy = dev_get_drvdata(dev);
+
+	if (gpio_is_valid(sphy->phyclk_gpio)) {
+		gpio_set_value(sphy->phyclk_gpio, 1);
+		/*
+		 * PI6C557-03 clock generator needs 3ms typically to stabilise,
+		 * but the datasheet doesn't list max.  We'll sleep for 10ms
+		 * and cross our fingers that it's enough.
+		 */
+		usleep_range(10000, 20000);
+	}
+
+	samsung_exynos5_usb3phy_clk_switch(&sphy->phy, true);
+
+	return 0;
+}
+
+static const struct dev_pm_ops samsung_usb3_phy_pm_ops = {
+	SET_RUNTIME_PM_OPS(samsung_usb3_phy_runtime_suspend,
+				samsung_usb3_phy_runtime_resume, NULL)
+};
+
 static struct samsung_usbphy_drvdata usb3_phy_exynos5 = {
 	.cpu_type		= TYPE_EXYNOS5250,
 	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
@@ -338,7 +431,9 @@ static struct platform_driver samsung_usb3_phy_driver = {
 		.name	= "samsung-usb3-phy",
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(samsung_usbphy_dt_match),
+		.pm	= &samsung_usb3_phy_pm_ops,
 	},
+
 };
 
 module_platform_driver(samsung_usb3_phy_driver);
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 7782a43..fb17b84 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/usb/samsung_usb_phy.h>
 
 #include "samsung-usbphy.h"
@@ -33,6 +34,7 @@
 int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
 {
 	struct device_node *usbphy_sys;
+	int ret;
 
 	/* Getting node for system controller interface for usb-phy */
 	usbphy_sys = of_get_child_by_name(sphy->dev->of_node, "usbphy-sys");
@@ -57,6 +59,30 @@ int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
 	if (sphy->sysreg == NULL)
 		dev_warn(sphy->dev, "Can't get usb-phy sysreg cfg register\n");
 
+	/* Getting PHY clk gpio here to enable/disable PHY clock PLL, if any */
+	sphy->phyclk_gpio = of_get_named_gpio(sphy->dev->of_node,
+						"samsung,phyclk-gpio", 0);
+	/*
+	 * We don't want to return error code here in case we don't get the
+	 * PHY clock gpio, some PHYs may not have it.
+	 */
+	if (gpio_is_valid(sphy->phyclk_gpio)) {
+		ret = gpio_request_one(sphy->phyclk_gpio, GPIOF_INIT_HIGH,
+						"samsung_usb_phy_clock_en");
+		if (ret) {
+			/*
+			 * We don't want to return error code here,
+			 * sometimes either of usb2 phy or usb3 phy may not
+			 * have the PHY clock gpio.
+			 */
+			dev_err(sphy->dev, "can't request phyclk gpio %d\n",
+						sphy->phyclk_gpio);
+			sphy->phyclk_gpio = -EINVAL;
+		}
+	} else {
+		dev_warn(sphy->dev, "Can't get usb-phy clock gpio\n");
+	}
+
 	of_node_put(usbphy_sys);
 
 	return 0;
diff --git a/drivers/usb/phy/samsung-usbphy.h b/drivers/usb/phy/samsung-usbphy.h
index f7e657d..1921ab0 100644
--- a/drivers/usb/phy/samsung-usbphy.h
+++ b/drivers/usb/phy/samsung-usbphy.h
@@ -300,6 +300,7 @@ struct samsung_usbphy {
 	enum samsung_usb_phy_type phy_type;
 	atomic_t	phy_usage;
 	spinlock_t	lock;
+	int		phyclk_gpio;
 };
 
 #define phy_to_sphy(x)		container_of((x), struct samsung_usbphy, phy)
-- 
1.7.6.5

Re: [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Felipe Balbi]

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

Hi,

On Mon, Jan 28, 2013 at 05:12:28PM +0530, Vivek Gautam wrote:
> Enabling runtime power management support on samsung-usb3 phy
> and further adding support to turn off the PHY ref_clk PLL.
> It thereby requires PHY ref_clk to be switched between internal
> core clock and external PLL clock.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>

this needs to be broken down a bit. I can see three patches at least:
add support for external clock, add support for phy gpio powerdown and
add runtime pm ;-)

> ---
>  drivers/usb/phy/samsung-usb3.c   |  107 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/phy/samsung-usbphy.c |   26 +++++++++
>  drivers/usb/phy/samsung-usbphy.h |    1 +
>  3 files changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/phy/samsung-usb3.c b/drivers/usb/phy/samsung-usb3.c
> index 29e1321..4dbef15 100644
> --- a/drivers/usb/phy/samsung-usb3.c
> +++ b/drivers/usb/phy/samsung-usb3.c
> @@ -22,8 +22,10 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> +#include <linux/gpio.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/usb/samsung_usb_phy.h>
>  #include <linux/platform_data/samsung-usbphy.h>
>  
> @@ -32,7 +34,7 @@
>  /*
>   * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
>   */
> -static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
> +static u32 samsung_usb3_phy_set_refclk_int(struct samsung_usbphy *sphy)
>  {
>  	u32 reg;
>  	u32 refclk;
> @@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
>  	return reg;
>  }
>  
> -static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
> +/*
> + * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
> + */
> +static u32 samsung_usb3_phy_set_refclk_ext(void)
> +{
> +	u32 reg;
> +
> +	reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
> +		PHYCLKRST_FSEL_PAD_100MHZ |
> +		PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
> +
> +	return reg;
> +}

I wonder if you really need this small function (likewise for
set_refclk_int()). They don't do much, so you could just inline them on
the only caller.

> @@ -80,7 +97,11 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>  
>  	phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
>  	/* Select PHY CLK source */
> -	phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
> +	if (use_ext_clk)
> +		phyparam0 |= PHYPARAM0_REF_USE_PAD;
> +	else
> +		phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
> +
>  	/* Set Loss-of-Signal Detector sensitivity */
>  	phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
>  	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
> @@ -115,7 +136,10 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>  	/* UTMI Power Control */
>  	writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
>  
> -	phyclkrst = samsung_usb3_phy_set_refclk(sphy);
> +	if (use_ext_clk)
> +		phyclkrst = samsung_usb3_phy_set_refclk_ext();
> +	else
> +		phyclkrst = samsung_usb3_phy_set_refclk_int(sphy);
>  
>  	phyclkrst |= PHYCLKRST_PORTRESET |
>  			/* Digital power supply in normal operating mode */
> @@ -163,7 +187,7 @@ static void samsung_exynos5_usb3_phy_disable(struct samsung_usbphy *sphy)
>  	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
>  }
>  
> -static int samsung_usb3_phy_init(struct usb_phy *phy)
> +static int samsung_exynos5_usb3_phy_init(struct usb_phy *phy, bool use_ext_clk)
>  {
>  	struct samsung_usbphy *sphy;
>  	unsigned long flags;
> @@ -187,7 +211,7 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
>  	samsung_usbphy_set_isolation(sphy, false);
>  
>  	/* Initialize usb phy registers */
> -	samsung_exynos5_usb3_phy_enable(sphy);
> +	samsung_exynos5_usb3_phy_enable(sphy, use_ext_clk);
>  
>  	spin_unlock_irqrestore(&sphy->lock, flags);
>  
> @@ -198,6 +222,34 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
>  }
>  
>  /*
> + * Switch  between internal core clock and external oscillator clock
> + * for PHY reference clock
> + */
> +static int samsung_exynos5_usb3phy_clk_switch(struct usb_phy *phy,
> +						bool use_ext_clk)
> +{
> +	/*
> +	 * This will switch PHY refclk from internal core clock
> +	 * to external PLL clock when device is in use and vice versa
> +	 * when device plunge into runtime suspend mode.
> +	 */
> +	return samsung_exynos5_usb3_phy_init(phy, use_ext_clk);
> +}
> +
> +/*
> + * The function passed to the usb driver for phy initialization
> + */
> +static int samsung_usb3_phy_init(struct usb_phy *phy)
> +{
> +	/*
> +	 * We start with using PHY refclk from external PLL,
> +	 * once runtime suspend for the device is called this
> +	 * will change to internal core clock
> +	 */
> +	return  samsung_exynos5_usb3_phy_init(phy, true);
> +}
> +
> +/*
>   * The function passed to the usb driver for phy shutdown
>   */
>  static void samsung_usb3_phy_shutdown(struct usb_phy *phy)
> @@ -287,6 +339,9 @@ static int samsung_usb3_phy_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sphy);
>  
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB3);
>  }
>  
> @@ -296,6 +351,8 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
>  
>  	usb_remove_phy(&sphy->phy);
>  
> +	pm_runtime_disable(&pdev->dev);

before disabling, shouldn't you make sure the IP is turned off by
calling:

if (!pm_runtime_suspend(&pdev->dev))
	pm_runtime_put_sync(&pdev->dev);

??

> @@ -304,6 +361,42 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int samsung_usb3_phy_runtime_suspend(struct device *dev)
> +{
> +	struct samsung_usbphy *sphy = dev_get_drvdata(dev);
> +
> +	samsung_exynos5_usb3phy_clk_switch(&sphy->phy, false);
> +
> +	if (gpio_is_valid(sphy->phyclk_gpio))
> +		gpio_set_value(sphy->phyclk_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int samsung_usb3_phy_runtime_resume(struct device *dev)
> +{
> +	struct samsung_usbphy *sphy = dev_get_drvdata(dev);
> +
> +	if (gpio_is_valid(sphy->phyclk_gpio)) {
> +		gpio_set_value(sphy->phyclk_gpio, 1);
> +		/*
> +		 * PI6C557-03 clock generator needs 3ms typically to stabilise,
> +		 * but the datasheet doesn't list max.  We'll sleep for 10ms
> +		 * and cross our fingers that it's enough.
> +		 */
> +		usleep_range(10000, 20000);
> +	}
> +
> +	samsung_exynos5_usb3phy_clk_switch(&sphy->phy, true);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops samsung_usb3_phy_pm_ops = {
> +	SET_RUNTIME_PM_OPS(samsung_usb3_phy_runtime_suspend,
> +				samsung_usb3_phy_runtime_resume, NULL)
> +};

you need to wrap this with #ifdef CONFIG_PM_RUNTIME. So it would look
better as:

#ifdef CONFIG_PM_RUNTIME
suspend()
resume()
#define DEV_PM_OPS	(&samsung_usb3_phy_pm_ops)
#else
#define DEV_PM_OPS	NULL
#endif

> +
>  static struct samsung_usbphy_drvdata usb3_phy_exynos5 = {
>  	.cpu_type		= TYPE_EXYNOS5250,
>  	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
> @@ -338,7 +431,9 @@ static struct platform_driver samsung_usb3_phy_driver = {
>  		.name	= "samsung-usb3-phy",
>  		.owner	= THIS_MODULE,
>  		.of_match_table = of_match_ptr(samsung_usbphy_dt_match),
> +		.pm	= &samsung_usb3_phy_pm_ops,

and here you have:

	.pm = DEV_PM_OPS,

-- 
balbi

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

Re: [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Vivek Gautam]

Hi Felipe,


On Mon, Jan 28, 2013 at 5:39 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Jan 28, 2013 at 05:12:28PM +0530, Vivek Gautam wrote:
>> Enabling runtime power management support on samsung-usb3 phy
>> and further adding support to turn off the PHY ref_clk PLL.
>> It thereby requires PHY ref_clk to be switched between internal
>> core clock and external PLL clock.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>
> this needs to be broken down a bit. I can see three patches at least:
> add support for external clock, add support for phy gpio powerdown and
> add runtime pm ;-)
>

Alright, will break this into required number of patches.

>> ---
>>  drivers/usb/phy/samsung-usb3.c   |  107 +++++++++++++++++++++++++++++++++++--
>>  drivers/usb/phy/samsung-usbphy.c |   26 +++++++++
>>  drivers/usb/phy/samsung-usbphy.h |    1 +
>>  3 files changed, 128 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/phy/samsung-usb3.c b/drivers/usb/phy/samsung-usb3.c
>> index 29e1321..4dbef15 100644
>> --- a/drivers/usb/phy/samsung-usb3.c
>> +++ b/drivers/usb/phy/samsung-usb3.c
>> @@ -22,8 +22,10 @@
>>  #include <linux/clk.h>
>>  #include <linux/delay.h>
>>  #include <linux/err.h>
>> +#include <linux/gpio.h>
>>  #include <linux/io.h>
>>  #include <linux/of.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/usb/samsung_usb_phy.h>
>>  #include <linux/platform_data/samsung-usbphy.h>
>>
>> @@ -32,7 +34,7 @@
>>  /*
>>   * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
>>   */
>> -static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
>> +static u32 samsung_usb3_phy_set_refclk_int(struct samsung_usbphy *sphy)
>>  {
>>       u32 reg;
>>       u32 refclk;
>> @@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
>>       return reg;
>>  }
>>
>> -static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>> +/*
>> + * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
>> + */
>> +static u32 samsung_usb3_phy_set_refclk_ext(void)
>> +{
>> +     u32 reg;
>> +
>> +     reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
>> +             PHYCLKRST_FSEL_PAD_100MHZ |
>> +             PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
>> +
>> +     return reg;
>> +}
>
> I wonder if you really need this small function (likewise for
> set_refclk_int()). They don't do much, so you could just inline them on
> the only caller.
>

Created this just to keep symmetry, ;-)
will move this in the caller only.

>> @@ -80,7 +97,11 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>>
>>       phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
>>       /* Select PHY CLK source */
>> -     phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
>> +     if (use_ext_clk)
>> +             phyparam0 |= PHYPARAM0_REF_USE_PAD;
>> +     else
>> +             phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
>> +
>>       /* Set Loss-of-Signal Detector sensitivity */
>>       phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
>>       phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
>> @@ -115,7 +136,10 @@ static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>>       /* UTMI Power Control */
>>       writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
>>
>> -     phyclkrst = samsung_usb3_phy_set_refclk(sphy);
>> +     if (use_ext_clk)
>> +             phyclkrst = samsung_usb3_phy_set_refclk_ext();
>> +     else
>> +             phyclkrst = samsung_usb3_phy_set_refclk_int(sphy);
>>
>>       phyclkrst |= PHYCLKRST_PORTRESET |
>>                       /* Digital power supply in normal operating mode */
>> @@ -163,7 +187,7 @@ static void samsung_exynos5_usb3_phy_disable(struct samsung_usbphy *sphy)
>>       writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
>>  }
>>
>> -static int samsung_usb3_phy_init(struct usb_phy *phy)
>> +static int samsung_exynos5_usb3_phy_init(struct usb_phy *phy, bool use_ext_clk)
>>  {
>>       struct samsung_usbphy *sphy;
>>       unsigned long flags;
>> @@ -187,7 +211,7 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
>>       samsung_usbphy_set_isolation(sphy, false);
>>
>>       /* Initialize usb phy registers */
>> -     samsung_exynos5_usb3_phy_enable(sphy);
>> +     samsung_exynos5_usb3_phy_enable(sphy, use_ext_clk);
>>
>>       spin_unlock_irqrestore(&sphy->lock, flags);
>>
>> @@ -198,6 +222,34 @@ static int samsung_usb3_phy_init(struct usb_phy *phy)
>>  }
>>
>>  /*
>> + * Switch  between internal core clock and external oscillator clock
>> + * for PHY reference clock
>> + */
>> +static int samsung_exynos5_usb3phy_clk_switch(struct usb_phy *phy,
>> +                                             bool use_ext_clk)
>> +{
>> +     /*
>> +      * This will switch PHY refclk from internal core clock
>> +      * to external PLL clock when device is in use and vice versa
>> +      * when device plunge into runtime suspend mode.
>> +      */
>> +     return samsung_exynos5_usb3_phy_init(phy, use_ext_clk);
>> +}
>> +
>> +/*
>> + * The function passed to the usb driver for phy initialization
>> + */
>> +static int samsung_usb3_phy_init(struct usb_phy *phy)
>> +{
>> +     /*
>> +      * We start with using PHY refclk from external PLL,
>> +      * once runtime suspend for the device is called this
>> +      * will change to internal core clock
>> +      */
>> +     return  samsung_exynos5_usb3_phy_init(phy, true);
>> +}
>> +
>> +/*
>>   * The function passed to the usb driver for phy shutdown
>>   */
>>  static void samsung_usb3_phy_shutdown(struct usb_phy *phy)
>> @@ -287,6 +339,9 @@ static int samsung_usb3_phy_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, sphy);
>>
>> +     pm_runtime_set_active(&pdev->dev);
>> +     pm_runtime_enable(&pdev->dev);
>> +
>>       return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB3);
>>  }
>>
>> @@ -296,6 +351,8 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
>>
>>       usb_remove_phy(&sphy->phy);
>>
>> +     pm_runtime_disable(&pdev->dev);
>
> before disabling, shouldn't you make sure the IP is turned off by
> calling:
>
> if (!pm_runtime_suspend(&pdev->dev))
>         pm_runtime_put_sync(&pdev->dev);
>
> ??
>
True, will amend this.

>> @@ -304,6 +361,42 @@ static int samsung_usb3_phy_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +static int samsung_usb3_phy_runtime_suspend(struct device *dev)
>> +{
>> +     struct samsung_usbphy *sphy = dev_get_drvdata(dev);
>> +
>> +     samsung_exynos5_usb3phy_clk_switch(&sphy->phy, false);
>> +
>> +     if (gpio_is_valid(sphy->phyclk_gpio))
>> +             gpio_set_value(sphy->phyclk_gpio, 0);
>> +
>> +     return 0;
>> +}
>> +
>> +static int samsung_usb3_phy_runtime_resume(struct device *dev)
>> +{
>> +     struct samsung_usbphy *sphy = dev_get_drvdata(dev);
>> +
>> +     if (gpio_is_valid(sphy->phyclk_gpio)) {
>> +             gpio_set_value(sphy->phyclk_gpio, 1);
>> +             /*
>> +              * PI6C557-03 clock generator needs 3ms typically to stabilise,
>> +              * but the datasheet doesn't list max.  We'll sleep for 10ms
>> +              * and cross our fingers that it's enough.
>> +              */
>> +             usleep_range(10000, 20000);
>> +     }
>> +
>> +     samsung_exynos5_usb3phy_clk_switch(&sphy->phy, true);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct dev_pm_ops samsung_usb3_phy_pm_ops = {
>> +     SET_RUNTIME_PM_OPS(samsung_usb3_phy_runtime_suspend,
>> +                             samsung_usb3_phy_runtime_resume, NULL)
>> +};
>
> you need to wrap this with #ifdef CONFIG_PM_RUNTIME. So it would look
> better as:
>
> #ifdef CONFIG_PM_RUNTIME
> suspend()
> resume()
> #define DEV_PM_OPS      (&samsung_usb3_phy_pm_ops)
> #else
> #define DEV_PM_OPS      NULL
> #endif
>

Yeah, this one looks much better :-)
Will amend this as suggested.

>> +
>>  static struct samsung_usbphy_drvdata usb3_phy_exynos5 = {
>>       .cpu_type               = TYPE_EXYNOS5250,
>>       .devphy_en_mask         = EXYNOS_USBPHY_ENABLE,
>> @@ -338,7 +431,9 @@ static struct platform_driver samsung_usb3_phy_driver = {
>>               .name   = "samsung-usb3-phy",
>>               .owner  = THIS_MODULE,
>>               .of_match_table = of_match_ptr(samsung_usbphy_dt_match),
>> +             .pm     = &samsung_usb3_phy_pm_ops,
>
> and here you have:
>
>         .pm = DEV_PM_OPS,
>

sure,



-- 
Thanks & Regards
Vivek

Re: [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Felipe Balbi]

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

Hi,

On Mon, Jan 28, 2013 at 06:34:15PM +0530, Vivek Gautam wrote:
> >> @@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
> >>       return reg;
> >>  }
> >>
> >> -static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
> >> +/*
> >> + * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
> >> + */
> >> +static u32 samsung_usb3_phy_set_refclk_ext(void)
> >> +{
> >> +     u32 reg;
> >> +
> >> +     reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
> >> +             PHYCLKRST_FSEL_PAD_100MHZ |
> >> +             PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
> >> +
> >> +     return reg;
> >> +}
> >
> > I wonder if you really need this small function (likewise for
> > set_refclk_int()). They don't do much, so you could just inline them on
> > the only caller.
> >
> 
> Created this just to keep symmetry, ;-)
> will move this in the caller only.

you can have a patch before this series moving the refclk_int() to the
caller, then you will have symmetry ;-)

-- 
balbi

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

Re: [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Vivek Gautam]

Hi Felipe,


On Mon, Jan 28, 2013 at 6:37 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Jan 28, 2013 at 06:34:15PM +0530, Vivek Gautam wrote:
>> >> @@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
>> >>       return reg;
>> >>  }
>> >>
>> >> -static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
>> >> +/*
>> >> + * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
>> >> + */
>> >> +static u32 samsung_usb3_phy_set_refclk_ext(void)
>> >> +{
>> >> +     u32 reg;
>> >> +
>> >> +     reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
>> >> +             PHYCLKRST_FSEL_PAD_100MHZ |
>> >> +             PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
>> >> +
>> >> +     return reg;
>> >> +}
>> >
>> > I wonder if you really need this small function (likewise for
>> > set_refclk_int()). They don't do much, so you could just inline them on
>> > the only caller.
>> >
>>
>> Created this just to keep symmetry, ;-)
>> will move this in the caller only.
>
> you can have a patch before this series moving the refclk_int() to the
> caller, then you will have symmetry ;-)
>

refclk_int() was rather slightly a big chunk :-(
as available in below patch as samsung_usb3_phy_set_refclk() :
[PATCH v4 2/2] usb: phy: samsung: Add PHY support for USB 3.0 controller
http://www.mail-archive.com/linux-usb@vger.kernel.org/msg13796.html

Will try to figure best possible way and amend.


-- 
Thanks & Regards
Vivek

Re: [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb3

[Felipe Balbi]

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

On Mon, Jan 28, 2013 at 06:54:42PM +0530, Vivek Gautam wrote:
> Hi Felipe,
> 
> 
> On Mon, Jan 28, 2013 at 6:37 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Jan 28, 2013 at 06:34:15PM +0530, Vivek Gautam wrote:
> >> >> @@ -65,7 +67,22 @@ static u32 samsung_usb3_phy_set_refclk(struct samsung_usbphy *sphy)
> >> >>       return reg;
> >> >>  }
> >> >>
> >> >> -static int samsung_exynos5_usb3_phy_enable(struct samsung_usbphy *sphy)
> >> >> +/*
> >> >> + * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
> >> >> + */
> >> >> +static u32 samsung_usb3_phy_set_refclk_ext(void)
> >> >> +{
> >> >> +     u32 reg;
> >> >> +
> >> >> +     reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
> >> >> +             PHYCLKRST_FSEL_PAD_100MHZ |
> >> >> +             PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
> >> >> +
> >> >> +     return reg;
> >> >> +}
> >> >
> >> > I wonder if you really need this small function (likewise for
> >> > set_refclk_int()). They don't do much, so you could just inline them on
> >> > the only caller.
> >> >
> >>
> >> Created this just to keep symmetry, ;-)
> >> will move this in the caller only.
> >
> > you can have a patch before this series moving the refclk_int() to the
> > caller, then you will have symmetry ;-)
> >
> 
> refclk_int() was rather slightly a big chunk :-(
> as available in below patch as samsung_usb3_phy_set_refclk() :
> [PATCH v4 2/2] usb: phy: samsung: Add PHY support for USB 3.0 controller
> http://www.mail-archive.com/linux-usb@vger.kernel.org/msg13796.html
> 
> Will try to figure best possible way and amend.

oh, right... In that case, keep the way it is. Ignore my comment ;-)

-- 
balbi

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