[PATCH 0/2] dwc3: exynos: Device tree fixes

[PATCH 0/2] dwc3: exynos: Device tree fixes

[Vivek Gautam]

This patch-set modifies dwc3-exynos as per latest bindings
available for dwc3. Now the dwc3 core also has device support,
there's no need to add platform device for core in glue layers.
This change has come as a result of discussion happened in:
[PATCH RFC] usb: dwc3: Get PHY from platform specific dwc3 dt node.

Additionally, now that Samsung exynos series has moved to common
clock framework, we use clock_prepare_enable() and clock_disable_unprepare()
APIs.
Some cleanup is also done here.

Based on 'usb-next' plus Felipe's 'testing' branch patches;
(some 194 patches in fact ;-), on top of 3.9rc2 tag).
Also based on: "usb: dwc3: set dma_mask for dwc3_omap device" by Kishon
in which DMA mask for dwc3-core is being set from its parent.

Vivek Gautam (2):
  usb: dwc3: exynos: Use of_platform API to create dwc3 core pdev
  usb: dwc3: exynos: use clk_prepare_enable and clk_disable_unprepare

 drivers/usb/dwc3/dwc3-exynos.c |   54 ++++++++++++++--------------------------
 1 files changed, 19 insertions(+), 35 deletions(-)

-- 
1.7.6.5

[PATCH 1/2] usb: dwc3: exynos: Use of_platform API to create dwc3 core pdev

[Vivek Gautam]

Used of_platform_populate() to create dwc3 core platform_device
from device tree data. Additionally some cleanup is also done.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/usb/dwc3/dwc3-exynos.c |   46 +++++++++++++---------------------------
 1 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index e12e452..66ca9ac 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -22,9 +22,9 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/nop-usb-xceiv.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 
 struct dwc3_exynos {
-	struct platform_device	*dwc3;
 	struct platform_device	*usb2_phy;
 	struct platform_device	*usb3_phy;
 	struct device		*dev;
@@ -90,17 +90,17 @@ static u64 dwc3_exynos_dma_mask = DMA_BIT_MASK(32);
 
 static int dwc3_exynos_probe(struct platform_device *pdev)
 {
-	struct platform_device	*dwc3;
 	struct dwc3_exynos	*exynos;
 	struct clk		*clk;
 	struct device		*dev = &pdev->dev;
+	struct device_node	*node = dev->of_node;
 
 	int			ret = -ENOMEM;
 
 	exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL);
 	if (!exynos) {
 		dev_err(dev, "not enough memory\n");
-		return -ENOMEM;
+		goto err1;
 	}
 
 	/*
@@ -108,21 +108,15 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	 * Since shared usb code relies on it, set it here for now.
 	 * Once we move to full device tree support this will vanish off.
 	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &dwc3_exynos_dma_mask;
+	if (!dev->dma_mask)
+		dev->dma_mask = &dwc3_exynos_dma_mask;
 
 	platform_set_drvdata(pdev, exynos);
 
 	ret = dwc3_exynos_register_phys(exynos);
 	if (ret) {
 		dev_err(dev, "couldn't register PHYs\n");
-		return ret;
-	}
-
-	dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
-	if (!dwc3) {
-		dev_err(dev, "couldn't allocate dwc3 device\n");
-		return -ENOMEM;
+		goto err1;
 	}
 
 	clk = devm_clk_get(dev, "usbdrd30");
@@ -132,27 +126,20 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err1;
 	}
 
-	dma_set_coherent_mask(&dwc3->dev, dev->coherent_dma_mask);
-
-	dwc3->dev.parent = dev;
-	dwc3->dev.dma_mask = dev->dma_mask;
-	dwc3->dev.dma_parms = dev->dma_parms;
-	exynos->dwc3	= dwc3;
 	exynos->dev	= dev;
 	exynos->clk	= clk;
 
 	clk_enable(exynos->clk);
 
-	ret = platform_device_add_resources(dwc3, pdev->resource,
-			pdev->num_resources);
-	if (ret) {
-		dev_err(dev, "couldn't add resources to dwc3 device\n");
-		goto err2;
-	}
-
-	ret = platform_device_add(dwc3);
-	if (ret) {
-		dev_err(dev, "failed to register dwc3 device\n");
+	if (node) {
+		ret = of_platform_populate(node, NULL, NULL, dev);
+		if (ret) {
+			dev_err(dev, "failed to add dwc3 core\n");
+			goto err2;
+		}
+	} else {
+		dev_err(dev, "no device node, failed to add dwc3 core\n");
+		ret = -ENODEV;
 		goto err2;
 	}
 
@@ -161,8 +148,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 err2:
 	clk_disable(clk);
 err1:
-	platform_device_put(dwc3);
-
 	return ret;
 }
 
@@ -170,7 +155,6 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
 
-	platform_device_unregister(exynos->dwc3);
 	platform_device_unregister(exynos->usb2_phy);
 	platform_device_unregister(exynos->usb3_phy);
 
-- 
1.7.6.5

[PATCH 2/2] usb: dwc3: exynos: use clk_prepare_enable and clk_disable_unprepare

[Vivek Gautam]

Convert clk_enable/clk_disable to clk_prepare_enable/clk_disable_unprepare
calls as required by common clock framework.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/usb/dwc3/dwc3-exynos.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 66ca9ac..b03f609 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -129,7 +129,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	exynos->dev	= dev;
 	exynos->clk	= clk;
 
-	clk_enable(exynos->clk);
+	clk_prepare_enable(exynos->clk);
 
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, dev);
@@ -146,7 +146,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	return 0;
 
 err2:
-	clk_disable(clk);
+	clk_disable_unprepare(clk);
 err1:
 	return ret;
 }
@@ -158,7 +158,7 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 	platform_device_unregister(exynos->usb2_phy);
 	platform_device_unregister(exynos->usb3_phy);
 
-	clk_disable(exynos->clk);
+	clk_disable_unprepare(exynos->clk);
 
 	return 0;
 }
-- 
1.7.6.5

Re: [PATCH 1/2] usb: dwc3: exynos: Use of_platform API to create dwc3 core pdev

[Felipe Balbi]

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

Hi,

On Thu, Mar 14, 2013 at 04:14:57PM +0530, Vivek Gautam wrote:
> @@ -170,7 +155,6 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>  {
>  	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
>  
> -	platform_device_unregister(exynos->dwc3);

don't you want to do what Kishon did here and have:

static int dwc3_exynos_remove_child(struct device *dev, void *unused)
{
	struct platform_device	*pdev = to_platform_device(dev);

	platform_device_unregister(pdev);

	return 0;
}

device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);

???

-- 
balbi

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

Re: [PATCH 2/2] usb: dwc3: exynos: use clk_prepare_enable and clk_disable_unprepare

[Felipe Balbi]

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

Hi,

On Thu, Mar 14, 2013 at 04:14:58PM +0530, Vivek Gautam wrote:
> Convert clk_enable/clk_disable to clk_prepare_enable/clk_disable_unprepare
> calls as required by common clock framework.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> CC: Felipe Balbi <balbi@ti.com>
> CC: Kukjin Kim <kgene.kim@samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 66ca9ac..b03f609 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -129,7 +129,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	exynos->dev	= dev;
>  	exynos->clk	= clk;
>  
> -	clk_enable(exynos->clk);
> +	clk_prepare_enable(exynos->clk);

eventually we need to pass this clock handling to dwc3/core.c. Just make
sure it's optional since not all platforms need it.

I guess the best way would be to handle clocks via
->runtime_suspend()/->runtime_resume() ??

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: exynos: Use of_platform API to create dwc3 core pdev

[Vivek Gautam]

On Thu, Mar 14, 2013 at 4:21 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Mar 14, 2013 at 04:14:57PM +0530, Vivek Gautam wrote:
>> @@ -170,7 +155,6 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>  {
>>       struct dwc3_exynos      *exynos = platform_get_drvdata(pdev);
>>
>> -     platform_device_unregister(exynos->dwc3);
>
> don't you want to do what Kishon did here and have:
>
> static int dwc3_exynos_remove_child(struct device *dev, void *unused)
> {
>         struct platform_device  *pdev = to_platform_device(dev);
>
>         platform_device_unregister(pdev);
>
>         return 0;
> }
>
> device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
>
> ???

Hmm, right. We need to do that. :-)

>
> --
> balbi



-- 
Thanks & Regards
Vivek

[PATCH v2 1/2] usb: dwc3: exynos: Use of_platform API to create dwc3 core pdev

[Vivek Gautam]

Used of_platform_populate() to create dwc3 core platform_device
from device tree data. Additionally some cleanup is also done.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kukjin Kim <kgene.kim@samsung.com>
---

Changes from v1:
 - Added method to unregister dwc3 core from dwc3_exynos_remove()
   using device_for_each_child() API, which we missed to do.

 drivers/usb/dwc3/dwc3-exynos.c |   56 ++++++++++++++++++----------------------
 1 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index e12e452..f77ec75 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -22,9 +22,9 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/nop-usb-xceiv.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 
 struct dwc3_exynos {
-	struct platform_device	*dwc3;
 	struct platform_device	*usb2_phy;
 	struct platform_device	*usb3_phy;
 	struct device		*dev;
@@ -86,21 +86,30 @@ err1:
 	return ret;
 }
 
+static int dwc3_exynos_remove_child(struct device *dev, void *unused)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	platform_device_unregister(pdev);
+
+	return 0;
+}
+
 static u64 dwc3_exynos_dma_mask = DMA_BIT_MASK(32);
 
 static int dwc3_exynos_probe(struct platform_device *pdev)
 {
-	struct platform_device	*dwc3;
 	struct dwc3_exynos	*exynos;
 	struct clk		*clk;
 	struct device		*dev = &pdev->dev;
+	struct device_node	*node = dev->of_node;
 
 	int			ret = -ENOMEM;
 
 	exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL);
 	if (!exynos) {
 		dev_err(dev, "not enough memory\n");
-		return -ENOMEM;
+		goto err1;
 	}
 
 	/*
@@ -108,21 +117,15 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	 * Since shared usb code relies on it, set it here for now.
 	 * Once we move to full device tree support this will vanish off.
 	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &dwc3_exynos_dma_mask;
+	if (!dev->dma_mask)
+		dev->dma_mask = &dwc3_exynos_dma_mask;
 
 	platform_set_drvdata(pdev, exynos);
 
 	ret = dwc3_exynos_register_phys(exynos);
 	if (ret) {
 		dev_err(dev, "couldn't register PHYs\n");
-		return ret;
-	}
-
-	dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
-	if (!dwc3) {
-		dev_err(dev, "couldn't allocate dwc3 device\n");
-		return -ENOMEM;
+		goto err1;
 	}
 
 	clk = devm_clk_get(dev, "usbdrd30");
@@ -132,27 +135,20 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err1;
 	}
 
-	dma_set_coherent_mask(&dwc3->dev, dev->coherent_dma_mask);
-
-	dwc3->dev.parent = dev;
-	dwc3->dev.dma_mask = dev->dma_mask;
-	dwc3->dev.dma_parms = dev->dma_parms;
-	exynos->dwc3	= dwc3;
 	exynos->dev	= dev;
 	exynos->clk	= clk;
 
 	clk_enable(exynos->clk);
 
-	ret = platform_device_add_resources(dwc3, pdev->resource,
-			pdev->num_resources);
-	if (ret) {
-		dev_err(dev, "couldn't add resources to dwc3 device\n");
-		goto err2;
-	}
-
-	ret = platform_device_add(dwc3);
-	if (ret) {
-		dev_err(dev, "failed to register dwc3 device\n");
+	if (node) {
+		ret = of_platform_populate(node, NULL, NULL, dev);
+		if (ret) {
+			dev_err(dev, "failed to add dwc3 core\n");
+			goto err2;
+		}
+	} else {
+		dev_err(dev, "no device node, failed to add dwc3 core\n");
+		ret = -ENODEV;
 		goto err2;
 	}
 
@@ -161,8 +157,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 err2:
 	clk_disable(clk);
 err1:
-	platform_device_put(dwc3);
-
 	return ret;
 }
 
@@ -170,9 +164,9 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
 
-	platform_device_unregister(exynos->dwc3);
 	platform_device_unregister(exynos->usb2_phy);
 	platform_device_unregister(exynos->usb3_phy);
+	device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
 
 	clk_disable(exynos->clk);
 
-- 
1.7.6.5

Re: [PATCH 2/2] usb: dwc3: exynos: use clk_prepare_enable and clk_disable_unprepare

[Vivek Gautam]

Hi,


On Thu, Mar 14, 2013 at 4:23 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Mar 14, 2013 at 04:14:58PM +0530, Vivek Gautam wrote:
>> Convert clk_enable/clk_disable to clk_prepare_enable/clk_disable_unprepare
>> calls as required by common clock framework.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> CC: Felipe Balbi <balbi@ti.com>
>> CC: Kukjin Kim <kgene.kim@samsung.com>
>> ---
>>  drivers/usb/dwc3/dwc3-exynos.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> index 66ca9ac..b03f609 100644
>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> @@ -129,7 +129,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>       exynos->dev     = dev;
>>       exynos->clk     = clk;
>>
>> -     clk_enable(exynos->clk);
>> +     clk_prepare_enable(exynos->clk);
>
> eventually we need to pass this clock handling to dwc3/core.c. Just make
> sure it's optional since not all platforms need it.
>
True, as of now i could see only exynos platform getting a device
clock for dwc3-glue.
So, if not all platforms need to do this, why should we plan to move
this to dwc3/core.c ?

> I guess the best way would be to handle clocks via
> ->runtime_suspend()/->runtime_resume() ??

Right, but there was a doubt actually if you can please clear that.
In device probe, after enabling runtime_pm we would need to
'pm_runtime_get_sync' the device.
Thereby, in runtime_resume the clocks will be enabled.
Now as soon as the device probe finishes, the device will go in
suspend state, calling runtime_suspend
and the clocks would be disabled.
Now would it be possible for the controller to detect any connect/disconnect.

Pardon me if there is something very silly i am missing out.

>
> --
> balbi



-- 
Thanks & Regards
Vivek

Re: [PATCH 2/2] usb: dwc3: exynos: use clk_prepare_enable and clk_disable_unprepare

[Felipe Balbi]

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

Hi,

On Fri, Mar 15, 2013 at 11:36:00AM +0530, Vivek Gautam wrote:
> >> Convert clk_enable/clk_disable to clk_prepare_enable/clk_disable_unprepare
> >> calls as required by common clock framework.
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> CC: Felipe Balbi <balbi@ti.com>
> >> CC: Kukjin Kim <kgene.kim@samsung.com>
> >> ---
> >>  drivers/usb/dwc3/dwc3-exynos.c |    6 +++---
> >>  1 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >> index 66ca9ac..b03f609 100644
> >> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >> @@ -129,7 +129,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>       exynos->dev     = dev;
> >>       exynos->clk     = clk;
> >>
> >> -     clk_enable(exynos->clk);
> >> +     clk_prepare_enable(exynos->clk);
> >
> > eventually we need to pass this clock handling to dwc3/core.c. Just make
> > sure it's optional since not all platforms need it.
> >
> True, as of now i could see only exynos platform getting a device
> clock for dwc3-glue.
> So, if not all platforms need to do this, why should we plan to move
> this to dwc3/core.c ?

what if dwc3.ko's probe fail ? Clock will be left enabled ;-)

> > I guess the best way would be to handle clocks via
> > ->runtime_suspend()/->runtime_resume() ??
> 
> Right, but there was a doubt actually if you can please clear that.
> In device probe, after enabling runtime_pm we would need to
> 'pm_runtime_get_sync' the device.
> Thereby, in runtime_resume the clocks will be enabled.
> Now as soon as the device probe finishes, the device will go in
> suspend state, calling runtime_suspend
> and the clocks would be disabled.
> Now would it be possible for the controller to detect any
> connect/disconnect.

it depends on how you have configured your core in coreConsultant and
how you're implementing the actual IP. If you have retention flip-flops
then gating clocks (but not cutting Vcc) will not loose context and, if
PHYs are still enabled, you will see new connect events.

But that part of PM optimization has to be done as a last step, as it
tends to break things apart.

-- 
balbi

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