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

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

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.

> 
> > 
> > -	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).

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.

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

Peter

-- 

Thanks,
Peter Chen