[PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Kever Yang]

Hcd controller needs bus_suspend/resume, dwc2 controller make
root hub generate suspend/resume signal with hprt0 register
when work in host mode.
After the root hub enter suspend, we can make controller enter
low power state with PCGCTL register.

We also update the lx_state for hsotg state.

This patch has tested on rk3288 with suspend/resume.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2:
- update commit message
- make dwc2 suspend/resume sourcecode work

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

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 0a0e6f0..01a415b 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1471,6 +1471,30 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
 	}
 }
 
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
+{
+	u32 hprt0;
+
+	/* After clear the Stop PHY clock bit, we should wait for a moment
+	 * for PLL work stable with clock output.
+	 */
+	writel(0, hsotg->regs + PCGCTL);
+	usleep_range(2000, 4000);
+
+	hprt0 = dwc2_read_hprt0(hsotg);
+	hprt0 |= HPRT0_RES;
+	writel(hprt0, hsotg->regs + HPRT0);
+	hprt0 &= ~HPRT0_SUSP;
+	/* according to USB2.0 Spec 7.1.7.7, the host must send the resume
+	 * signal for at least 20ms
+	 */
+	usleep_range(20000, 25000);
+
+	hprt0 &= ~HPRT0_RES;
+	writel(hprt0, hsotg->regs + HPRT0);
+	hsotg->lx_state = DWC2_L0;
+}
+
 /* 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)
@@ -1516,17 +1540,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:
@@ -2299,6 +2313,44 @@ 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) ||
+		(hsotg->op_state == OTG_STATE_A_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);
+
+	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) ||
+		(hsotg->op_state == OTG_STATE_A_HOST)))
+		return 0;
+
+	if (hsotg->lx_state != DWC2_L2)
+		return 0;
+
+	hprt0 = dwc2_read_hprt0(hsotg);
+	if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
+		dwc2_port_resume(hsotg);
+
+	return 0;
+}
+
 /* Returns the current frame number */
 static int _dwc2_hcd_get_frame_number(struct usb_hcd *hcd)
 {
@@ -2669,6 +2721,10 @@ 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,
+#ifdef CONFIG_PM
+	.bus_suspend = _dwc2_hcd_suspend,
+	.bus_resume = _dwc2_hcd_resume,
+#endif
 };
 
 /*
-- 
1.9.1

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Romain Perier]

Hi Kever,


2014-11-06 2:30 GMT+01:00 Kever Yang <kever.yang@rock-chips.com>:
>
> +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) ||
> +               (hsotg->op_state == OTG_STATE_A_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);
> +
> +       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) ||
> +               (hsotg->op_state == OTG_STATE_A_HOST)))
> +               return 0;
> +
> +       if (hsotg->lx_state != DWC2_L2)
> +               return 0;
> +
> +       hprt0 = dwc2_read_hprt0(hsotg);
> +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
> +               dwc2_port_resume(hsotg);
> +
> +       return 0;
> +}

Could you also define these functions under #ifdef CONFIG_PM ?

Thanks.

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Felipe Balbi]

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

On Thu, Nov 06, 2014 at 06:21:42PM +0100, Romain Perier wrote:
> Hi Kever,
> 
> 
> 2014-11-06 2:30 GMT+01:00 Kever Yang <kever.yang@rock-chips.com>:
> >
> > +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) ||
> > +               (hsotg->op_state == OTG_STATE_A_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);
> > +
> > +       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) ||
> > +               (hsotg->op_state == OTG_STATE_A_HOST)))
> > +               return 0;
> > +
> > +       if (hsotg->lx_state != DWC2_L2)
> > +               return 0;
> > +
> > +       hprt0 = dwc2_read_hprt0(hsotg);
> > +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
> > +               dwc2_port_resume(hsotg);
> > +
> > +       return 0;
> > +}
> 
> Could you also define these functions under #ifdef CONFIG_PM ?

please don't. I'm actually considering ripping all ifdefs from all these
drivers and also stop using SIMPLE_DEV_PM_OPS or any of its friends.

There's really nobody today would would build a kernel with CONFIG_PM.

-- 
balbi

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

RE: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Paul Zimmerman]

> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, November 06, 2014 9:40 AM
> 
> On Thu, Nov 06, 2014 at 06:21:42PM +0100, Romain Perier wrote:
> >
> > 2014-11-06 2:30 GMT+01:00 Kever Yang <kever.yang@rock-chips.com>:
> > >
> > > +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) ||
> > > +               (hsotg->op_state == OTG_STATE_A_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);
> > > +
> > > +       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) ||
> > > +               (hsotg->op_state == OTG_STATE_A_HOST)))
> > > +               return 0;
> > > +
> > > +       if (hsotg->lx_state != DWC2_L2)
> > > +               return 0;
> > > +
> > > +       hprt0 = dwc2_read_hprt0(hsotg);
> > > +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
> > > +               dwc2_port_resume(hsotg);
> > > +
> > > +       return 0;
> > > +}
> >
> > Could you also define these functions under #ifdef CONFIG_PM ?
> 
> please don't. I'm actually considering ripping all ifdefs from all these
> drivers and also stop using SIMPLE_DEV_PM_OPS or any of its friends.
> 
> There's really nobody today would would build a kernel with CONFIG_PM.

I'm sure Felipe meant *without* CONFIG_PM.

Kever, in that case you should remove the #ifdef CONFIG_PM around the
.bus_suspend and .bus_resume assignments also, otherwise there will be
compiler warnings when built without CONFIG_PM. After that, you can
add my acked-by.

-- 
Paul

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Felipe Balbi]

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

Hi,

On Thu, Nov 06, 2014 at 06:35:53PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Thursday, November 06, 2014 9:40 AM
> > 
> > On Thu, Nov 06, 2014 at 06:21:42PM +0100, Romain Perier wrote:
> > >
> > > 2014-11-06 2:30 GMT+01:00 Kever Yang <kever.yang@rock-chips.com>:
> > > >
> > > > +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) ||
> > > > +               (hsotg->op_state == OTG_STATE_A_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);
> > > > +
> > > > +       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) ||
> > > > +               (hsotg->op_state == OTG_STATE_A_HOST)))
> > > > +               return 0;
> > > > +
> > > > +       if (hsotg->lx_state != DWC2_L2)
> > > > +               return 0;
> > > > +
> > > > +       hprt0 = dwc2_read_hprt0(hsotg);
> > > > +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
> > > > +               dwc2_port_resume(hsotg);
> > > > +
> > > > +       return 0;
> > > > +}
> > >
> > > Could you also define these functions under #ifdef CONFIG_PM ?
> > 
> > please don't. I'm actually considering ripping all ifdefs from all these
> > drivers and also stop using SIMPLE_DEV_PM_OPS or any of its friends.
> > 
> > There's really nobody today would would build a kernel with CONFIG_PM.
> 
> I'm sure Felipe meant *without* CONFIG_PM.

oops, that's correct.

> Kever, in that case you should remove the #ifdef CONFIG_PM around the
> .bus_suspend and .bus_resume assignments also, otherwise there will be
> compiler warnings when built without CONFIG_PM. After that, you can
> add my acked-by.

tks

-- 
balbi

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

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Julius Werner]

On Wed, Nov 5, 2014 at 5:30 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hcd controller needs bus_suspend/resume, dwc2 controller make
> root hub generate suspend/resume signal with hprt0 register
> when work in host mode.
> After the root hub enter suspend, we can make controller enter
> low power state with PCGCTL register.

You say you do this, but I don't actually see you doing it (for the
not-connected case)?

>
> We also update the lx_state for hsotg state.
>
> This patch has tested on rk3288 with suspend/resume.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
> Changes in v2:
> - update commit message
> - make dwc2 suspend/resume sourcecode work
>
>  drivers/usb/dwc2/hcd.c | 78 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 0a0e6f0..01a415b 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1471,6 +1471,30 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>         }
>  }
>
> +static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> +{
> +       u32 hprt0;
> +
> +       /* After clear the Stop PHY clock bit, we should wait for a moment
> +        * for PLL work stable with clock output.
> +        */
> +       writel(0, hsotg->regs + PCGCTL);
> +       usleep_range(2000, 4000);
> +
> +       hprt0 = dwc2_read_hprt0(hsotg);
> +       hprt0 |= HPRT0_RES;
> +       writel(hprt0, hsotg->regs + HPRT0);
> +       hprt0 &= ~HPRT0_SUSP;
> +       /* according to USB2.0 Spec 7.1.7.7, the host must send the resume
> +        * signal for at least 20ms
> +        */
> +       usleep_range(20000, 25000);
> +
> +       hprt0 &= ~HPRT0_RES;
> +       writel(hprt0, hsotg->regs + HPRT0);
> +       hsotg->lx_state = DWC2_L0;
> +}
> +
>  /* 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)
> @@ -1516,17 +1540,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);

I'm curious why this didn't change lx_state back to DWC2_L0 before...
Paul, do you know?

> +                       dwc2_port_resume(hsotg);
>                         break;
>
>                 case USB_PORT_FEAT_POWER:
> @@ -2299,6 +2313,44 @@ 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) ||
> +               (hsotg->op_state == OTG_STATE_A_HOST)))
> +               return 0;
> +
> +       if (hsotg->lx_state != DWC2_L0)

What if the port is in L1 state? I don't think the driver supports LPM
right now, but the DWC2_L1 enum is defined so it may one day in the
future. Let's maybe at least add a TODO.

> +               return 0;

In your original ChromiumOS version of this patch, you also set
PCGCTL_STOPPCLK here if the port was not connected. Is there a reason
that changed (does it not actually save power or something)?

> +
> +       hprt0 = dwc2_read_hprt0(hsotg);
> +       if (hprt0 & HPRT0_CONNSTS)
> +               dwc2_port_suspend(hsotg, 1);

The contract for bus_suspend() is that it will suspend all ports not
yet suspended, keep track of those ports and then only resume those in
bus_resume() (compare, for example, how XHCI keeps track of that with
xhci_bus_state.bus_suspended in xhci_bus_suspend/resume()). So you
need something here to remember whether this function suspended the
port or whether it had already been suspended, and then only resume
the port in bus_resume() in the former case. Note that
dwc2_port_suspend() changes lx_state to DWC_L2 (at least in the
version I'm looking at right now), so you can't just rely on that
unless you explicitly set it back to something else here.

> +
> +       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) ||
> +               (hsotg->op_state == OTG_STATE_A_HOST)))
> +               return 0;
> +
> +       if (hsotg->lx_state != DWC2_L2)
> +               return 0;
> +
> +       hprt0 = dwc2_read_hprt0(hsotg);
> +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
> +               dwc2_port_resume(hsotg);
> +
> +       return 0;
> +}
> +
>  /* Returns the current frame number */
>  static int _dwc2_hcd_get_frame_number(struct usb_hcd *hcd)
>  {
> @@ -2669,6 +2721,10 @@ 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,
> +#ifdef CONFIG_PM
> +       .bus_suspend = _dwc2_hcd_suspend,
> +       .bus_resume = _dwc2_hcd_resume,
> +#endif
>  };
>
>  /*
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Kever Yang]

Hi Julius,

On 11/07/2014 06:11 AM, Julius Werner wrote:
> On Wed, Nov 5, 2014 at 5:30 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hcd controller needs bus_suspend/resume, dwc2 controller make
>> root hub generate suspend/resume signal with hprt0 register
>> when work in host mode.
>> After the root hub enter suspend, we can make controller enter
>> low power state with PCGCTL register.
> You say you do this, but I don't actually see you doing it (for the
> not-connected case)?
Agree to add PCGCTL operation for the not-connected case.
>
>> We also update the lx_state for hsotg state.
>>
>> This patch has tested on rk3288 with suspend/resume.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - update commit message
>> - make dwc2 suspend/resume sourcecode work
>>
>>   drivers/usb/dwc2/hcd.c | 78 +++++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 67 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 0a0e6f0..01a415b 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -1471,6 +1471,30 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>          }
>>   }
>>
>> +static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
>> +{
>> +       u32 hprt0;
>> +
>> +       /* After clear the Stop PHY clock bit, we should wait for a moment
>> +        * for PLL work stable with clock output.
>> +        */
>> +       writel(0, hsotg->regs + PCGCTL);
>> +       usleep_range(2000, 4000);
>> +
>> +       hprt0 = dwc2_read_hprt0(hsotg);
>> +       hprt0 |= HPRT0_RES;
>> +       writel(hprt0, hsotg->regs + HPRT0);
>> +       hprt0 &= ~HPRT0_SUSP;
>> +       /* according to USB2.0 Spec 7.1.7.7, the host must send the resume
>> +        * signal for at least 20ms
>> +        */
>> +       usleep_range(20000, 25000);
>> +
>> +       hprt0 &= ~HPRT0_RES;
>> +       writel(hprt0, hsotg->regs + HPRT0);
>> +       hsotg->lx_state = DWC2_L0;
>> +}
>> +
>>   /* 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)
>> @@ -1516,17 +1540,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);
> I'm curious why this didn't change lx_state back to DWC2_L0 before...
> Paul, do you know?
>
>> +                       dwc2_port_resume(hsotg);
>>                          break;
>>
>>                  case USB_PORT_FEAT_POWER:
>> @@ -2299,6 +2313,44 @@ 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) ||
>> +               (hsotg->op_state == OTG_STATE_A_HOST)))
>> +               return 0;
>> +
>> +       if (hsotg->lx_state != DWC2_L0)
> What if the port is in L1 state? I don't think the driver supports LPM
> right now, but the DWC2_L1 enum is defined so it may one day in the
> future. Let's maybe at least add a TODO.
Added
>
>> +               return 0;
> In your original ChromiumOS version of this patch, you also set
> PCGCTL_STOPPCLK here if the port was not connected. Is there a reason
> that changed (does it not actually save power or something)?
>
>> +
>> +       hprt0 = dwc2_read_hprt0(hsotg);
>> +       if (hprt0 & HPRT0_CONNSTS)
>> +               dwc2_port_suspend(hsotg, 1);
> The contract for bus_suspend() is that it will suspend all ports not
> yet suspended, keep track of those ports and then only resume those in
> bus_resume() (compare, for example, how XHCI keeps track of that with
> xhci_bus_state.bus_suspended in xhci_bus_suspend/resume()). So you
> need something here to remember whether this function suspended the
> port or whether it had already been suspended, and then only resume
> the port in bus_resume() in the former case.
In fact, the dwc2 controller only support one port, so the hprt0
is the only one port we need to care.
> Note that
> dwc2_port_suspend() changes lx_state to DWC_L2 (at least in the
> version I'm looking at right now), so you can't just rely on that
> unless you explicitly set it back to something else here.
>
>> +
>> +       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) ||
>> +               (hsotg->op_state == OTG_STATE_A_HOST)))
>> +               return 0;
>> +
>> +       if (hsotg->lx_state != DWC2_L2)
>> +               return 0;
>> +
>> +       hprt0 = dwc2_read_hprt0(hsotg);
>> +       if ((hprt0 & HPRT0_CONNSTS) && (hprt0 & HPRT0_SUSP))
>> +               dwc2_port_resume(hsotg);
>> +
>> +       return 0;
>> +}
>> +
>>   /* Returns the current frame number */
>>   static int _dwc2_hcd_get_frame_number(struct usb_hcd *hcd)
>>   {
>> @@ -2669,6 +2721,10 @@ 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,
>> +#ifdef CONFIG_PM
>> +       .bus_suspend = _dwc2_hcd_suspend,
>> +       .bus_resume = _dwc2_hcd_resume,
>> +#endif
>>   };
>>
>>   /*
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2

[Julius Werner]

>> The contract for bus_suspend() is that it will suspend all ports not
>> yet suspended, keep track of those ports and then only resume those in
>> bus_resume() (compare, for example, how XHCI keeps track of that with
>> xhci_bus_state.bus_suspended in xhci_bus_suspend/resume()). So you
>> need something here to remember whether this function suspended the
>> port or whether it had already been suspended, and then only resume
>> the port in bus_resume() in the former case.
>
> In fact, the dwc2 controller only support one port, so the hprt0
> is the only one port we need to care.

Yes, I know, but that one port still needs to play by the rules the
USB core expects. All I'm saying is: if the port was already suspended
during bus_suspend(), then the next bus_resume() should not resume it.

The rest looks good to me now. But in order to get it really working,
I think we'll still need the actual driver.pm suspend/resume methods,
at least for the HCD_FLAG_HW_ACCESSIBLE and the
usb_root_hub_lost_power() handling (probably better in a separate
patch).