From: Peter Chen <peter.chen@nxp.com>
To: Jun Li <jun.li@nxp.com>
Cc: "balbi@kernel.org" <balbi@kernel.org>,
"mathias.nyman@intel.com" <mathias.nyman@intel.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
dl-linux-imx <linux-imx@nxp.com>,
"pawell@cadence.com" <pawell@cadence.com>,
"rogerq@ti.com" <rogerq@ti.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support
Date: Mon, 25 May 2020 02:57:07 +0000 [thread overview]
Message-ID: <20200525025731.GA16948@b29397-desktop> (raw)
In-Reply-To: <VE1PR04MB652836D56801C440F3E3928689B20@VE1PR04MB6528.eurprd04.prod.outlook.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
next prev parent reply other threads:[~2020-05-25 2:57 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-23 23:22 [PATCH v2 0/9] usb: some PM changes for cdns3 and xhci-plat Peter Chen
2020-05-23 23:22 ` [PATCH v2 1/9] usb: cdns3: introduce cdns3_set_phy_power API Peter Chen
2020-05-24 5:56 ` Jun Li
2020-05-23 23:22 ` [PATCH v2 2/9] usb: cdns3: add runtime PM support Peter Chen
2020-05-24 5:57 ` Jun Li
2020-05-24 6:43 ` Jun Li
2020-05-25 2:57 ` Peter Chen [this message]
2020-05-25 12:23 ` Jun Li
2020-05-26 2:03 ` Peter Chen
2020-06-03 8:51 ` Chunfeng Yun
2020-06-03 9:27 ` Peter Chen
2020-05-23 23:22 ` [PATCH v2 3/9] usb: cdns3: imx: add glue layer runtime pm implementation Peter Chen
2020-05-24 6:55 ` Jun Li
2020-05-25 2:59 ` Peter Chen
2020-05-25 4:53 ` kbuild test robot
2020-05-25 6:31 ` Peter Chen
2020-05-25 8:25 ` kbuild test robot
2020-05-23 23:22 ` [PATCH v2 4/9] usb: host: xhci-plat: add platform data support Peter Chen
2020-05-29 10:21 ` Mathias Nyman
2020-05-23 23:23 ` [PATCH v2 5/9] usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv Peter Chen
2020-05-29 10:22 ` Mathias Nyman
2020-05-23 23:23 ` [PATCH v2 6/9] usb: host: xhci-plat: delete the unnecessary code Peter Chen
2020-05-24 7:40 ` Jun Li
2020-05-29 10:23 ` Mathias Nyman
2020-05-23 23:23 ` [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization Peter Chen
2020-05-24 7:40 ` Jun Li
2020-05-25 3:03 ` Peter Chen
2020-05-25 12:30 ` Jun Li
2020-05-26 0:26 ` Peter Chen
2020-05-29 13:10 ` Mathias Nyman
2020-05-29 13:15 ` Peter Chen
2020-05-23 23:23 ` [PATCH v2 8/9] usb: cdns3: host: add .suspend_quirk for xhci-plat.c Peter Chen
2020-05-23 23:23 ` [PATCH v2 9/9] usb: cdns3: host: add xhci_plat_priv's flag skip_phy_initialization Peter Chen
2020-05-29 7:22 ` [PATCH v2 0/9] usb: some PM changes for cdns3 and xhci-plat Peter Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200525025731.GA16948@b29397-desktop \
--to=peter.chen@nxp.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jun.li@nxp.com \
--cc=linux-imx@nxp.com \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@intel.com \
--cc=pawell@cadence.com \
--cc=rogerq@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).