[v7,2/5] usb: dwc3: core: Host wake up support from system suspend

Commit Message

Sandeep Maheswaram April 28, 2021, 5:11 a.m. UTC

Avoiding phy powerdown when wakeup capable devices are connected
by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable
devices such as keyboard and mouse.

Signed-off-by: Sandeep Maheswaram <[e-mail]>
Reviewed-by: Matthias Kaehlcke <[e-mail]>
---
 drivers/usb/dwc3/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Felipe Balbi April 28, 2021, 9:59 a.m. UTC | #1

Hi,

Sandeep Maheswaram <[e-mail]> writes:
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking phy_power_off flag.
> Phy should be on to wake up the device from suspend using wakeup capable
> devices such as keyboard and mouse.
>
> Signed-off-by: Sandeep Maheswaram <[e-mail]>
> Reviewed-by: Matthias Kaehlcke <[e-mail]>
> ---
>  drivers/usb/dwc3/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b6e53d8..bb414c3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1738,7 +1738,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		dwc3_core_exit(dwc);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
> -		if (!PMSG_IS_AUTO(msg)) {
> +		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {

should be able to detect this generically, no? Shouldn't
device_may_wakeup() be valid here and give you the answer you want?

Sandeep Maheswaram May 3, 2021, 4:33 a.m. UTC | #2

On 4/28/2021 3:29 PM, Felipe Balbi wrote:
> Hi,
>
> Sandeep Maheswaram <[e-mail]> writes:
>> Avoiding phy powerdown when wakeup capable devices are connected
>> by checking phy_power_off flag.
>> Phy should be on to wake up the device from suspend using wakeup capable
>> devices such as keyboard and mouse.
>>
>> Signed-off-by: Sandeep Maheswaram <[e-mail]>
>> Reviewed-by: Matthias Kaehlcke <[e-mail]>
>> ---
>>   drivers/usb/dwc3/core.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index b6e53d8..bb414c3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1738,7 +1738,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   		dwc3_core_exit(dwc);
>>   		break;
>>   	case DWC3_GCTL_PRTCAP_HOST:
>> -		if (!PMSG_IS_AUTO(msg)) {
>> +		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
> should be able to detect this generically, no? Shouldn't
> device_may_wakeup() be valid here and give you the answer you want?

I think  device_may_wakeup() gives whether the controller is wake up 
capable or not.

But we want to keep phy powered on only when some wakeup capable devices 
(eg:keyboard ,mouse ) are connected to controller.

Felipe Balbi May 3, 2021, 11:20 a.m. UTC | #3

Hi,

Sandeep Maheswaram <[e-mail]> writes:
>> Sandeep Maheswaram <[e-mail]> writes:
>>> Avoiding phy powerdown when wakeup capable devices are connected
>>> by checking phy_power_off flag.
>>> Phy should be on to wake up the device from suspend using wakeup capable
>>> devices such as keyboard and mouse.
>>>
>>> Signed-off-by: Sandeep Maheswaram <[e-mail]>
>>> Reviewed-by: Matthias Kaehlcke <[e-mail]>
>>> ---
>>>   drivers/usb/dwc3/core.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index b6e53d8..bb414c3 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1738,7 +1738,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>   		dwc3_core_exit(dwc);
>>>   		break;
>>>   	case DWC3_GCTL_PRTCAP_HOST:
>>> -		if (!PMSG_IS_AUTO(msg)) {
>>> +		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
>> should be able to detect this generically, no? Shouldn't
>> device_may_wakeup() be valid here and give you the answer you want?
>
> I think  device_may_wakeup() gives whether the controller is wake up 
> capable or not.

Yes, but it's a bit more than that. Looking at devices.rst we read:

If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
prepared for generating hardware wakeup signals to trigger a system wakeup event
when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
might identify GPIO signals hooked up to a switch or other external hardware,
and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.

So, if there is a condition where $this device has to, somehow, deal
with wakeup, it should be configured accordingly. This ->phy_power_off
flag is telling us the same thing.

> But we want to keep phy powered on only when some wakeup capable devices 
> (eg:keyboard ,mouse ) are connected to controller.

Understood, it could be that we're missing some method for propagating
that state (i.e. keyboard with PM support) up to the parent device, but
that's no excuse to bypass driver boundaries. Wouldn't you agree?

Matthias Kaehlcke May 12, 2021, 9:47 p.m. UTC | #4

On Mon, May 03, 2021 at 02:20:23PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Sandeep Maheswaram <[e-mail]> writes:
> >> Sandeep Maheswaram <[e-mail]> writes:
> >>> Avoiding phy powerdown when wakeup capable devices are connected
> >>> by checking phy_power_off flag.
> >>> Phy should be on to wake up the device from suspend using wakeup capable
> >>> devices such as keyboard and mouse.
> >>>
> >>> Signed-off-by: Sandeep Maheswaram <[e-mail]>
> >>> Reviewed-by: Matthias Kaehlcke <[e-mail]>
> >>> ---
> >>>   drivers/usb/dwc3/core.c | 7 +++++--
> >>>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >>> index b6e53d8..bb414c3 100644
> >>> --- a/drivers/usb/dwc3/core.c
> >>> +++ b/drivers/usb/dwc3/core.c
> >>> @@ -1738,7 +1738,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >>>   		dwc3_core_exit(dwc);
> >>>   		break;
> >>>   	case DWC3_GCTL_PRTCAP_HOST:
> >>> -		if (!PMSG_IS_AUTO(msg)) {
> >>> +		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
> >> should be able to detect this generically, no? Shouldn't
> >> device_may_wakeup() be valid here and give you the answer you want?
> >
> > I think  device_may_wakeup() gives whether the controller is wake up 
> > capable or not.
> 
> Yes, but it's a bit more than that. Looking at devices.rst we read:
> 
> If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
> prepared for generating hardware wakeup signals to trigger a system wakeup event
> when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
> might identify GPIO signals hooked up to a switch or other external hardware,
> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
> 
> So, if there is a condition where $this device has to, somehow, deal
> with wakeup, it should be configured accordingly. This ->phy_power_off
> flag is telling us the same thing.
> 
> > But we want to keep phy powered on only when some wakeup capable devices 
> > (eg:keyboard ,mouse ) are connected to controller.
> 
> Understood, it could be that we're missing some method for propagating
> that state (i.e. keyboard with PM support) up to the parent device, but
> that's no excuse to bypass driver boundaries. Wouldn't you agree?

I'm not sure if device_may_wakeup() is really the right tool for the
job. This is the current implementation:

static inline bool device_may_wakeup(struct device *dev)
{
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

IIUC power.can_wakeup specifies whether the device is wakeup
capable, primarily in physical terms and indicating that the
driver is ready to handle wakeups, and power.wakeup represents
the policy which can be changed by userspace.

Supposing the hub is generally wakeup capable that flag
shouldn't be changed. Neither should be the policy based on
what is connected to the bus.

Sandeep Maheswaram May 26, 2021, 4:29 a.m. UTC | #5

Hi Felipe,

On 5/13/2021 3:17 AM, Matthias Kaehlcke wrote:
> On Mon, May 03, 2021 at 02:20:23PM +0300, Felipe Balbi wrote:
>> Hi,
>>
>> Sandeep Maheswaram <[e-mail]> writes:
>>>> Sandeep Maheswaram <[e-mail]> writes:
>>>>> Avoiding phy powerdown when wakeup capable devices are connected
>>>>> by checking phy_power_off flag.
>>>>> Phy should be on to wake up the device from suspend using wakeup capable
>>>>> devices such as keyboard and mouse.
>>>>>
>>>>> Signed-off-by: Sandeep Maheswaram <[e-mail]>
>>>>> Reviewed-by: Matthias Kaehlcke <[e-mail]>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 7 +++++--
>>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index b6e53d8..bb414c3 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1738,7 +1738,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>>>    		dwc3_core_exit(dwc);
>>>>>    		break;
>>>>>    	case DWC3_GCTL_PRTCAP_HOST:
>>>>> -		if (!PMSG_IS_AUTO(msg)) {
>>>>> +		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
>>>> should be able to detect this generically, no? Shouldn't
>>>> device_may_wakeup() be valid here and give you the answer you want?
>>> I think  device_may_wakeup() gives whether the controller is wake up
>>> capable or not.
>> Yes, but it's a bit more than that. Looking at devices.rst we read:
>>
>> If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
>> prepared for generating hardware wakeup signals to trigger a system wakeup event
>> when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
>> might identify GPIO signals hooked up to a switch or other external hardware,
>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
>>
>> So, if there is a condition where $this device has to, somehow, deal
>> with wakeup, it should be configured accordingly. This ->phy_power_off
>> flag is telling us the same thing.
>>
>>> But we want to keep phy powered on only when some wakeup capable devices
>>> (eg:keyboard ,mouse ) are connected to controller.
>> Understood, it could be that we're missing some method for propagating
>> that state (i.e. keyboard with PM support) up to the parent device, but
>> that's no excuse to bypass driver boundaries. Wouldn't you agree?
> I'm not sure if device_may_wakeup() is really the right tool for the
> job. This is the current implementation:
>
> static inline bool device_may_wakeup(struct device *dev)
> {
> 	return dev->power.can_wakeup && !!dev->power.wakeup;
> }
>
> IIUC power.can_wakeup specifies whether the device is wakeup
> capable, primarily in physical terms and indicating that the
> driver is ready to handle wakeups, and power.wakeup represents
> the policy which can be changed by userspace.
>
> Supposing the hub is generally wakeup capable that flag
> shouldn't be changed. Neither should be the policy based on
> what is connected to the bus.
Please suggest us how to proceed further with this patch.

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b6e53d8..bb414c3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1738,7 +1738,7 @@  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
 			dwc3_core_exit(dwc);
 			break;
 		}
@@ -1799,13 +1799,16 @@  static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
 			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 			break;
+		} else {
+			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 		}
+
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (dwc->dis_u2_susphy_quirk)