[RFC PATCH 1/2] usb: dwc2: add clock manage for hcd

Discussion:

Kever Yang

2014-10-16 05:46:17 UTC

This patch move clock init out of gadget into platform,
make both hcd and gadget can use the clock

Signed-off-by: Kever Yang <kever.yang-***@public.gmane.org>
---

drivers/usb/dwc2/gadget.c | 16 ++--------------
drivers/usb/dwc2/hcd.c | 3 +++
drivers/usb/dwc2/platform.c | 30 ++++++++++++++++++------------
3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6ffbfc2..1943e52 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3408,20 +3408,12 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
hsotg->phyif = GUSBCFG_PHYIF8;
}

- hsotg->clk = devm_clk_get(dev, "otg");
- if (IS_ERR(hsotg->clk)) {
- dev_err(dev, "cannot get otg clock\n");
- return PTR_ERR(hsotg->clk);
- }
-
hsotg->gadget.max_speed = USB_SPEED_HIGH;
hsotg->gadget.ops = &s3c_hsotg_gadget_ops;
hsotg->gadget.name = dev_name(dev);

/* reset the system */

- clk_prepare_enable(hsotg->clk);
-
/* regulators */

for (i = 0; i < ARRAY_SIZE(hsotg->supplies); i++)
@@ -3431,7 +3423,7 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
hsotg->supplies);
if (ret) {
dev_err(dev, "failed to request supplies: %d\n", ret);
- goto err_clk;
+ goto out;
}

ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
@@ -3510,9 +3502,7 @@ err_ep_mem:
kfree(eps);
err_supplies:
s3c_hsotg_phy_disable(hsotg);
-err_clk:
- clk_disable_unprepare(hsotg->clk);
-
+out:
return ret;
}
EXPORT_SYMBOL_GPL(dwc2_gadget_init);
@@ -3532,8 +3522,6 @@ int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
usb_gadget_unregister_driver(hsotg->driver);
}

- clk_disable_unprepare(hsotg->clk);
-
return 0;
}
EXPORT_SYMBOL_GPL(s3c_hsotg_remove);
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index fa49c72..fddd923 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -46,6 +46,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/slab.h>
+#include <linux/clk.h>
#include <linux/usb.h>

#include <linux/usb/hcd.h>
@@ -2266,6 +2267,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
spin_lock_irqsave(&hsotg->lock, flags);

hcd->state = HC_STATE_RUNNING;
+ clk_enable(hsotg->clk);

if (dwc2_is_device_mode(hsotg)) {
spin_unlock_irqrestore(&hsotg->lock, flags);
@@ -2297,6 +2299,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
spin_lock_irqsave(&hsotg->lock, flags);
dwc2_hcd_stop(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);
+ clk_disable(hsotg->clk);

usleep_range(1000, 3000);
}
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 77c8417..16cdd1f 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -37,6 +37,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/clk.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
#include <linux/of_device.h>
@@ -122,6 +123,7 @@ static int dwc2_driver_remove(struct platform_device *dev)

dwc2_hcd_remove(hsotg);
s3c_hsotg_remove(hsotg);
+ clk_disable_unprepare(hsotg->clk);

return 0;
}
@@ -216,24 +218,28 @@ static int dwc2_driver_probe(struct platform_device *dev)
hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);

spin_lock_init(&hsotg->lock);
- retval = dwc2_gadget_init(hsotg, irq);
- if (retval) {
- /*
- * We will not fail the driver initialization for dual-role
- * if no clock node is supplied. However, all gadget
- * functionality will be disabled if a clock node is not
- * provided. Host functionality will continue.
- * TO-DO: make clock node a requirement for the HCD.
- */
- if (!IS_ERR(hsotg->clk))
- return retval;
+ hsotg->clk = devm_clk_get(dev, "otg");
+ if (IS_ERR(hsotg->clk)) {
+ dev_err(dev, "cannot get otg clock\n");
+ return PTR_ERR(hsotg->clk);
}
+
+ clk_prepare_enable(hsotg->clk);
+
+ retval = dwc2_gadget_init(hsotg, irq);
+ if (retval)
+ goto out;
+
retval = dwc2_hcd_init(hsotg, irq, params);
if (retval)
- return retval;
+ goto out;

platform_set_drvdata(dev, hsotg);

+out:
+ if (IS_ERR(retval))
+ clk_disable_unprepare(hsotg->clk);
+
return retval;
}

--
1.9.1

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

Kever Yang

2014-10-16 05:46:18 UTC

Permalink

This patch add suspend/resume for dwc2 hcd controller.

Signed-off-by: Kever Yang <kever.yang-***@public.gmane.org>
---

drivers/usb/dwc2/hcd.c | 71 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index fddd923..c1801d8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1474,6 +1474,23 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
}
}

+static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
+{
+ u32 hprt0;
+
+ writel(0, hsotg->regs + PCGCTL);
+ usleep_range(20000, 40000);
+
+ hprt0 = dwc2_read_hprt0(hsotg);
+ hprt0 |= HPRT0_RES;
+ writel(hprt0, hsotg->regs + HPRT0);
+ hprt0 &= ~HPRT0_SUSP;
+ usleep_range(100000, 150000);
+
+ hprt0 &= ~HPRT0_RES;
+ writel(hprt0, hsotg->regs + HPRT0);
+}
+
/* Handles hub class-specific requests */
static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
u16 wvalue, u16 windex, char *buf, u16 wlength)
@@ -1519,17 +1536,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
case USB_PORT_FEAT_SUSPEND:
dev_dbg(hsotg->dev,
"ClearPortFeature USB_PORT_FEAT_SUSPEND\n");
- writel(0, hsotg->regs + PCGCTL);
- usleep_range(20000, 40000);
-
- hprt0 = dwc2_read_hprt0(hsotg);
- hprt0 |= HPRT0_RES;
- writel(hprt0, hsotg->regs + HPRT0);
- hprt0 &= ~HPRT0_SUSP;
- usleep_range(100000, 150000);
-
- hprt0 &= ~HPRT0_RES;
- writel(hprt0, hsotg->regs + HPRT0);
+ dwc2_port_resume(hsotg);
break;

case USB_PORT_FEAT_POWER:
@@ -2304,6 +2311,45 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
usleep_range(1000, 3000);
}

+static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
+{
+ struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+ u32 hprt0;
+
+ if (hsotg->op_state != OTG_STATE_B_HOST)
+ return 0;
+
+ if (hsotg->lx_state != DWC2_L0)
+ return 0;
+
+ hprt0 = dwc2_read_hprt0(hsotg);
+ if (hprt0 | HPRT0_CONNSTS)
+ dwc2_port_suspend(hsotg, 1);
+
+ clk_disable(hsotg->clk);
+ return 0;
+}
+
+static int _dwc2_hcd_resume(struct usb_hcd *hcd)
+{
+ struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+ u32 hprt0;
+
+ if (hsotg->op_state != OTG_STATE_B_HOST)
+ return 0;
+
+ if (hsotg->lx_state != DWC2_L2)
+ return 0;
+
+ clk_enable(hsotg->clk);
+
+ hprt0 = dwc2_read_hprt0(hsotg);
+ if (hprt0 | HPRT0_CONNSTS | HPRT0_SUSP)
+ dwc2_port_resume(hsotg);
+
+ return 0;
+}
+
/* Returns the current frame number */
static int _dwc2_hcd_get_frame_number(struct usb_hcd *hcd)
{
@@ -2674,6 +2720,9 @@ static struct hc_driver dwc2_hc_driver = {
.hub_status_data = _dwc2_hcd_hub_status_data,
.hub_control = _dwc2_hcd_hub_control,
.clear_tt_buffer_complete = _dwc2_hcd_clear_tt_buffer_complete,
+
+ .bus_suspend = _dwc2_hcd_suspend,
+ .bus_resume = _dwc2_hcd_resume,
};

/*

Felipe Balbi

2014-10-16 13:30:47 UTC

Permalink

Post by Kever Yang
This patch add suspend/resume for dwc2 hcd controller.

adds

Post by Kever Yang
---
drivers/usb/dwc2/hcd.c | 71 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index fddd923..c1801d8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1474,6 +1474,23 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
}
}
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
+{
+ u32 hprt0;
+
+ writel(0, hsotg->regs + PCGCTL);
+ usleep_range(20000, 40000);

why this usleep_range() ? Is it documented somewhere ? How about adding
a comment explaining why you need to wait at least 20ms ?

Post by Kever Yang
+ hprt0 = dwc2_read_hprt0(hsotg);
+ hprt0 |= HPRT0_RES;
+ writel(hprt0, hsotg->regs + HPRT0);
+ hprt0 &= ~HPRT0_SUSP;
+ usleep_range(100000, 150000);

and another one here, why ?

Post by Kever Yang
+ hprt0 &= ~HPRT0_RES;
+ writel(hprt0, hsotg->regs + HPRT0);
+}
+
/* Handles hub class-specific requests */
static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
u16 wvalue, u16 windex, char *buf, u16 wlength)
@@ -1519,17 +1536,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
dev_dbg(hsotg->dev,
"ClearPortFeature USB_PORT_FEAT_SUSPEND\n");
- writel(0, hsotg->regs + PCGCTL);
- usleep_range(20000, 40000);
-
- hprt0 = dwc2_read_hprt0(hsotg);
- hprt0 |= HPRT0_RES;
- writel(hprt0, hsotg->regs + HPRT0);
- hprt0 &= ~HPRT0_SUSP;
- usleep_range(100000, 150000);
-
- hprt0 &= ~HPRT0_RES;
- writel(hprt0, hsotg->regs + HPRT0);
+ dwc2_port_resume(hsotg);
break;
@@ -2304,6 +2311,45 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
usleep_range(1000, 3000);
}
+static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
+{
+ struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+ u32 hprt0;
+
+ if (hsotg->op_state != OTG_STATE_B_HOST)
+ return 0;
+
+ if (hsotg->lx_state != DWC2_L0)
+ return 0;
+
+ hprt0 = dwc2_read_hprt0(hsotg);
+ if (hprt0 | HPRT0_CONNSTS)

did you mean:

if (hprt0 & HPRT0_CONNSTS) ??

Post by Kever Yang
+ dwc2_port_suspend(hsotg, 1);

always 1 ?

Post by Kever Yang
+ clk_disable(hsotg->clk);

this I keep getting wrong, but if ->bus_suspend() is really the one used
for actual system sleep, then you might want to fiddle with the clocks
only from the parent platform glue. I haven't really read the code to
make sure, however, so this might be a completely bogus comment :-)

Post by Kever Yang
+ return 0;
+}
+
+static int _dwc2_hcd_resume(struct usb_hcd *hcd)
+{
+ struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+ u32 hprt0;
+
+ if (hsotg->op_state != OTG_STATE_B_HOST)
+ return 0;
+
+ if (hsotg->lx_state != DWC2_L2)
+ return 0;
+
+ clk_enable(hsotg->clk);
+
+ hprt0 = dwc2_read_hprt0(hsotg);
+ if (hprt0 | HPRT0_CONNSTS | HPRT0_SUSP)

and here:

if ((hprt0 & HPRT0_CONNSTS) &&
(hprt0 & HPRT0_SUSP)) ???

--
balbi

Kever Yang

2014-10-16 16:51:38 UTC

Permalink

Hi Felipe,

Post by Felipe Balbi

Post by Kever Yang
This patch add suspend/resume for dwc2 hcd controller.

adds

why this usleep_range() ? Is it documented somewhere ? How about adding
a comment explaining why you need to wait at least 20ms ?

This is a straight copy of CLEAR_FEATURE(PORT_SUSPEND) code
in dwc2_hcd_hub_control(), maybe Paul knows the detail?

I will check with USB2.0 spec and DWC databook to figure out
how long the delay it should be.

Post by Felipe Balbi

Post by Kever Yang
+ hprt0 = dwc2_read_hprt0(hsotg);
+ hprt0 |= HPRT0_RES;
+ writel(hprt0, hsotg->regs + HPRT0);
+ hprt0 &= ~HPRT0_SUSP;
+ usleep_range(100000, 150000);

and another one here, why ?

if (hprt0 & HPRT0_CONNSTS) ??

Post by Kever Yang
+ dwc2_port_suspend(hsotg, 1);

always 1 ?

Post by Kever Yang
+ clk_disable(hsotg->clk);

if ((hprt0 & HPRT0_CONNSTS) &&
(hprt0 & HPRT0_SUSP)) ???

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

Felipe Balbi

2014-10-16 17:38:57 UTC

Permalink

Hi,

Post by Kever Yang

Post by Felipe Balbi

Post by Kever Yang
This patch add suspend/resume for dwc2 hcd controller.

adds

why this usleep_range() ? Is it documented somewhere ? How about adding
a comment explaining why you need to wait at least 20ms ?

This is a straight copy of CLEAR_FEATURE(PORT_SUSPEND) code
in dwc2_hcd_hub_control(), maybe Paul knows the detail?

Probably :-)

Post by Kever Yang
I will check with USB2.0 spec and DWC databook to figure out
how long the delay it should be.

Thank you, that helps.

--
balbi

Felipe Balbi

2014-10-16 13:23:50 UTC

Permalink

Hi,

Post by Kever Yang
This patch move clock init out of gadget into platform,
make both hcd and gadget can use the clock
---
drivers/usb/dwc2/gadget.c | 16 ++--------------
drivers/usb/dwc2/hcd.c | 3 +++
drivers/usb/dwc2/platform.c | 30 ++++++++++++++++++------------
3 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6ffbfc2..1943e52 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3408,20 +3408,12 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
hsotg->phyif = GUSBCFG_PHYIF8;
}
- hsotg->clk = devm_clk_get(dev, "otg");
- if (IS_ERR(hsotg->clk)) {
- dev_err(dev, "cannot get otg clock\n");
- return PTR_ERR(hsotg->clk);
- }
-
hsotg->gadget.max_speed = USB_SPEED_HIGH;
hsotg->gadget.ops = &s3c_hsotg_gadget_ops;
hsotg->gadget.name = dev_name(dev);
/* reset the system */
- clk_prepare_enable(hsotg->clk);
-
/* regulators */
for (i = 0; i < ARRAY_SIZE(hsotg->supplies); i++)
@@ -3431,7 +3423,7 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
hsotg->supplies);
if (ret) {
dev_err(dev, "failed to request supplies: %d\n", ret);
- goto err_clk;
+ goto out;
}
ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
kfree(eps);
s3c_hsotg_phy_disable(hsotg);
- clk_disable_unprepare(hsotg->clk);
-
return ret;
}
EXPORT_SYMBOL_GPL(dwc2_gadget_init);
@@ -3532,8 +3522,6 @@ int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
usb_gadget_unregister_driver(hsotg->driver);
}
- clk_disable_unprepare(hsotg->clk);
-
return 0;
}
EXPORT_SYMBOL_GPL(s3c_hsotg_remove);
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index fa49c72..fddd923 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -46,6 +46,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/slab.h>
+#include <linux/clk.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -2266,6 +2267,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
spin_lock_irqsave(&hsotg->lock, flags);
hcd->state = HC_STATE_RUNNING;
+ clk_enable(hsotg->clk);

with this you're moving clk_enable() from gadget to HCD. You might want
to leave this completely to the glue; at least for now.

--
balbi

Kever Yang

2014-10-16 16:41:27 UTC

Permalink

Hi Felipe,

Thank for comment.

Post by Felipe Balbi
Hi,

with this you're moving clk_enable() from gadget to HCD. You might want
to leave this completely to the glue; at least for now.

No, I moving devm_clk_get/clk_prerare_enable/clk_disable_unprepare
to platform, but not effect clk_enable/disable in the gadget.

I send this patch for comments if we can do it in this way.
If not, how should we manage clock in hcd and gadget?

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

Felipe Balbi

2014-10-16 17:37:47 UTC

Permalink

Post by Kever Yang
Hi Felipe,
Thank for comment.

Post by Felipe Balbi
Hi,

with this you're moving clk_enable() from gadget to HCD. You might want
to leave this completely to the glue; at least for now.

As a first cut, I'd say only platform glue manages it. You can, anyway,
"hide" clk_enable()/disable() under
->runtime_resume()/->runtime_suspend() and linux driver model will
guarantee that will be called in the correct order. E.g.:

->runtime_resume(parent);
for_each_child(parent)
->runtime_resume(child);

conversely on suspend:

for_each_child(parent)
->runtime_suspend(child);
->runtime_suspend(parent);

--
balbi