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

[Jun Li <jun.li@nxp.com>]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月25日 10:57
> To: Jun Li <jun.li@nxp.com>
> Cc: balbi@kernel.org; mathias.nyman@intel.com; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; pawell@cadence.com; rogerq@ti.com;
> gregkh@linuxfoundation.org
> Subject: Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support
> 
> On 20-05-24 05:57:53, Jun Li wrote:
> > >
> > > -#ifdef CONFIG_PM_SLEEP
> > > +#ifdef CONFIG_PM
> >
> > __maybe_unused
> 
> Since there are several functions will be used in PM routine, it needs to add
> '__maybe_unused' for every functions, So, I choose to use MACRO directly.
> 
> >
> > >
> > > -static int cdns3_suspend(struct device *dev)
> > > +static int cdns3_set_platform_suspend(struct device *dev,
> > > +		bool suspend, bool wakeup)
> > > +{
> > > +	struct cdns3 *cdns = dev_get_drvdata(dev);
> > > +	int ret = 0;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&cdns->lock, flags);
> > > +	if (cdns->pdata && cdns->pdata->platform_suspend)
> > > +		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
> > > +
> > > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +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);
> 
> >
> > >
> > > -	if (cdns->roles[cdns->role]->suspend) {
> > > -		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
> > > -		cdns->roles[cdns->role]->suspend(cdns, false);
> > > -		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
> > > -	}
> > > +	cdns3_set_platform_suspend(cdns->dev, true, wakeup);
> > > +	cdns3_set_phy_power(cdns, false);
> > > +	spin_lock_irqsave(&cdns->lock, flags);
> > > +	cdns->in_lpm = true;
> > > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > >
> > >  	return 0;
> > >  }
> > >
> > > -static int cdns3_resume(struct device *dev)
> > > +static int cdns3_controller_resume(struct device *dev, pm_message_t
> > > +msg)
> > >  {
> > >  	struct cdns3 *cdns = dev_get_drvdata(dev);
> > > +	int ret;
> > >  	unsigned long flags;
> > >
> > > -	if (cdns->role == USB_ROLE_HOST)
> > > +	if (!cdns->in_lpm)
> > >  		return 0;
> > >
> > > -	if (cdns->roles[cdns->role]->resume) {
> > > -		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
> > > +	ret = cdns3_set_phy_power(cdns, true);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	cdns3_set_platform_suspend(cdns->dev, false, false);
> > > +
> > > +	spin_lock_irqsave(&cdns->lock, flags);
> > > +	if (cdns->roles[cdns->role]->resume && !PMSG_IS_AUTO(msg))
> > >  		cdns->roles[cdns->role]->resume(cdns, false);
> > > -		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
> > > +
> > > +	cdns->in_lpm = false;
> > > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > > +	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.

> 
> Using pm_request_resume could work since it is async pm, but below are still needed
> to balance pm_runtime_get at wakeup interrupt handler.
> 
> +			pm_runtime_mark_last_busy(cdns->dev);
> +			pm_runtime_put_autosuspend(cdns->dev);
> 
> >
> > > +		}
> > > +
> > > +		enable_irq(cdns->otg_irq);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int cdns3_runtime_suspend(struct device *dev) {
> > > +	return cdns3_controller_suspend(dev, PMSG_AUTO_SUSPEND); }
> > > +
> > > +static int cdns3_runtime_resume(struct device *dev) {
> > > +	return cdns3_controller_resume(dev, PMSG_AUTO_RESUME); } #ifdef
> > > +CONFIG_PM_SLEEP
> > > +
> > > +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).

Li Jun

> 
> > >  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?

Li Jun
> 
> Peter
> 
> --
> 
> Thanks,
> Peter Chen