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
