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

[Sandeep Maheswaram]

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 <sanm@xxxxxxxxxxxxxx> writes:
> > > > Sandeep Maheswaram <sanm@xxxxxxxxxxxxxx> 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 <sanm@xxxxxxxxxxxxxx>
> > > > > Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > > > > ---
> > > > >    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.