Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support

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

On 20-05-25 12:23:36, Jun Li wrote:
> > > > +static int cdns3_controller_suspend(struct device *dev,
> > > > +pm_message_t
> > > > +msg)
> > > >  {
> > > >  	struct cdns3 *cdns = dev_get_drvdata(dev);
> > > > +	bool wakeup;
> > > >  	unsigned long flags;
> > > >
> > > > -	if (cdns->role == USB_ROLE_HOST)
> > > > +	if (cdns->in_lpm)
> > > >  		return 0;
> > > >
> > > > -	if (pm_runtime_status_suspended(dev))
> > > > -		pm_runtime_resume(dev);
> > > > +	if (PMSG_IS_AUTO(msg))
> > > > +		wakeup = true;
> > > > +	else
> > > > +		wakeup = false;
> > >
> > > wakeup = device_may_wakeup(dev)?
> > 
> > No, it is for wakeup indicator for both runtime and system. At runtime, the USB
> > wakeup needs to be enabled all time, not depends on /sys/.../power/wakeup.
> 
> I mean for system pm:
> if (PMSG_IS_AUTO(msg))
> 	wakeup = true;
> else
> 	wakeup = device_may_wakeup(dev);

Ok, I will change. In this patch series, it only supports
wakeup for runtime PM. For system PM wakeup support, I plan
to support later.

> > > > +	if (cdns->wakeup_int) {
> > > > +		cdns->wakeup_int = false;
> > > > +		if (cdns->role == USB_ROLE_HOST) {
> > > > +			/* Trigger xhci-plat.c runtime runtime */
> > > > +			pm_runtime_get(&cdns->host_dev->dev);
> > > > +			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
> > > > +			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
> > > > +			/* balence the pm_runtime_get at cdns3_drd_irq */
> > >
> > > %s/balence/balance
> > 
> > Ok
> > 
> > >
> > > > +			pm_runtime_mark_last_busy(cdns->dev);
> > > > +			pm_runtime_put_autosuspend(cdns->dev);
> > >
> > > A simple
> > > if (cdns->role == USB_ROLE_HOST)
> > >       pm_runtime_resume(&cdns->host_dev->dev);
> > > in low power event handler can't work?
> > 
> > I tried, the pm_runtime_resume is sync pm request, it will cause rpm_resume nested
> > because the cdns->dev runtime_status is resuming (it is during the runtime resume
> > stage).
> 
> if you only resume the cdns->host_dev->dev, I think there is no
> nested resume, the sequence is
> cdns-imx resume -> cdns->dev resume -> cdns->host_dev->dev resume.
> 

I only resume the cdns->host_dev->dev like you suggested for testing,
There is one more rpm resume for cdns->dev, see below:

cdns-imx resume (1) -> cdns->dev resume(2) -> cdns->host_dev->dev
resume(3) -> cdns->dev resume (4)

Trace log and rpm status like below:

root@imx8qmmek:~# cat /sys/kernel/debug/tracing/trace | grep xhci-hcd.1.auto
     kworker/0:0-5     [000] d..1   144.830873: rpm_resume: xhci-hcd.1.auto flags-0 cnt-0  dep-0  auto-1 p-0 irq-0 chi
ld-0
root@imx8qmmek:~# cat /sys/kernel/debug/tracing/trace | grep 5b130000.cdns3 
          <idle>-0     [000] d.h2   144.830155: rpm_resume: 5b130000.cdns3 flags-5 cnt-1  dep-0  auto-1 p-0 irq-0 chil
d-0
          <idle>-0     [000] dNh2   144.830166: rpm_return_int: rpm_resume+0x114/0x758:5b130000.cdns3 ret=0
     kworker/0:0-5     [000] d..1   144.830189: rpm_resume: 5b130000.cdns3 flags-2 cnt-1  dep-0  auto-1 p-0 irq-0 chil
d-0
     kworker/0:0-5     [000] d..1   144.830875: rpm_resume: 5b130000.cdns3 flags-0 cnt-2  dep-0  auto-1 p-0 irq-0 chil
d-0

imx8qmmek:~# cat /sys/bus/platform/devices/5b130000.cdns3/power/runtime_status
resuming

If one device is resuming, the rpm_resume will be scheduled, and wait
the previous resume finish.

> > > > +static int cdns3_suspend(struct device *dev) {
> > > > +	struct cdns3 *cdns = dev_get_drvdata(dev);
> > > > +	unsigned long flags;
> > > > +
> > > > +	if (pm_runtime_status_suspended(dev))
> > > > +		pm_runtime_resume(dev);
> > > > +
> > > > +	if (cdns->roles[cdns->role]->suspend) {
> > > > +		spin_lock_irqsave(&cdns->lock, flags);
> > > > +		cdns->roles[cdns->role]->suspend(cdns, false);
> > >
> > > Seems this hasn't been used, I did not find the implementation.
> > > this role->suspend() is only to be used in system suspend but not in runtime suspend?
> > 
> > I don't change it in this series, it is used for system PM.
> 
> OK, understand this is the original code.
> For imx runtime PM of this patch set, do you still expect this?
> (i.e cdns3_gadget_suspend() will not be called for gadget runtime
> suspend).
> 

This code may need to purify in future for current mainline code.
The main purpose of this code was to disconnect host and re-connect
host after resume during the system suspend/resume. At runtime,
we don't need such operation since the runtime suspend doesn't be
called if the connection is there.

> 
> > 
> > > >  int cdns3_hw_role_switch(struct cdns3 *cdns); diff --git
> > > > a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c index
> > > > 58089841ed52..292ea248c0ec 100644
> > > > --- a/drivers/usb/cdns3/drd.c
> > > > +++ b/drivers/usb/cdns3/drd.c
> > > > @@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> > > >  	struct cdns3 *cdns = data;
> > > >  	u32 reg;
> > > >
> > > > +	if (cdns->in_lpm) {
> > > > +		disable_irq_nosync(irq);
> > > > +		cdns->wakeup_int = true;
> > > > +		pm_runtime_get(cdns->dev);
> > > > +		return IRQ_HANDLED;
> > > > +	}
> > >
> > > All low power events go through otg irq?
> > 
> > drd.c is built into core, so no matter, peripheral(host)-only or drd configuration,
> > this drd interrupt handler are all called firstly.
> > 
> > One thing needs to change at next revision is I only consider the interrupt number
> > are the same for all three modules (peripheral/host/otg), I will cover the different
> > interrupt number use case.
> 
> For shared irq with the same number, disable_irq_nosync(irq)
> can really disable the irq?
> 

When the controller is in low power mode, the normal controller logic
(peripheral/host/otg) will not trigger interrupt. The
disable_irq_nosync here is mainly to disable interrupt for wakeup
interrupt, and wait async runtime resume to clear the status, then
re-enable the interrupt.

-- 

Thanks,
Peter Chen