Re: [PATCH v6 2/3] usb: cdns3: add runtime PM support

[Peter Chen <peter.chen@nxp.com>]

On 20-08-21 14:15:55, Chunfeng Yun wrote:
> On Fri, 2020-08-21 at 10:48 +0800, Peter Chen wrote:
> > Introduce runtime PM and wakeup interrupt handler for cdns3,
> > the runtime PM is default off since other cdns3 may not
> > implement glue layer support for runtime PM.
> > 
> > One typical wakeup event use case is xHCI runtime suspend will clear
> > USBCMD.RS bit, after that the xHCI will not trigger any interrupts,
> > so its parent (cdns core device) needs to resume xHCI device when
> > any (wakeup) events occurs at host port.
> > 
> > When the controller is in low power mode, the lpm flag will be set.
> > The interrupt triggered later than lpm flag is set considers as
> > wakeup interrupt and handled at cdns_wakeup_irq. Once the wakeup
> > occurs, it first disables interrupt to avoid later interrupt
> > occurrence since the controller is in low power mode at that
> > time, and access registers may be invalid at that time. At wakeup
> > handler, it will call pm_request_resume to wakeup xHCI device, and
> > at runtime resume handler, it will enable interrupt again.
> > 
> > The API platform_suspend is introduced for glue layer to implement
> > platform specific PM sequence.
> > 
> > Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/core.c   | 153 ++++++++++++++++++++++++++++++++-----
> >  drivers/usb/cdns3/core.h   |  16 ++++
> >  drivers/usb/cdns3/drd.c    |   3 +
> >  drivers/usb/cdns3/gadget.c |   4 +
> >  drivers/usb/cdns3/host.c   |   7 ++
> >  5 files changed, 166 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index e56dbb6a898c..8e588eef38df 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -392,6 +392,30 @@ static void set_phy_power_off(struct cdns3 *cdns)
> >  	phy_power_off(cdns->usb2_phy);
> >  }
> >  
> > +/**
> > + * cdns3_wakeup_irq - interrupt handler for wakeup events
> > + *
> remove blank line?
> "Do not leave a blank line between the function description and the
> arguments, nor between the arguments"
> see kernel-doc.rst, Function parameters

Thanks, will change.

> >  	/*
> >  	 * The controller needs less time between bus and controller suspend,
> > @@ -559,52 +608,122 @@ static int cdns3_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -#ifdef CONFIG_PM_SLEEP
> > +#ifdef CONFIG_PM
> What about using __maybe_unused instead of #ifdef ?

It may be suitable for the number of function in power management scope
is less, say 4 functions for both runtime pm and system pm.

But for cdns3, it includes more functions at pm scope (will be more in
future after supporting more features), it may needs to add __maybe_unused
for all these functions if using your suggestion, so I prefer using MACRO.

-- 

Thanks,
Peter Chen