| Message ID | 20200523232304.23976-3-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: some PM changes for cdns3 and xhci-plat | expand |
Hi, > -----Original Message----- > From: Peter Chen <peter.chen@nxp.com> > Sent: 2020年5月24日 7:23 > To: balbi@kernel.org; mathias.nyman@intel.com > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li > <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com> > Subject: [PATCH v2 2/9] usb: cdns3: add runtime PM support > > Introduce runtime PM and wakeup interrupt handler for cdns3, the runtime PM is default > off since other cdns3 has not implemented glue layer support for runtime PM. > > When the controller is in low power mode, the lpm flag will be set. > The interrupt triggered later than lpm flag is set considers as wakeup interrupt > and handled at cdns_drd_irq. Once the wakeup occurs, it first disables interrupt > to avoid later interrupt occurrence since the controller is in low power mode at > that time, and access registers may be invalid at that time. At wakeup handler, > it will call pm_runtime_get to wakeup controller, and at runtime resume handler, > it will enable interrupt again. > > The API platform_suspend is introduced for glue layer to implement platform specific > PM sequence. > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/cdns3/core.c | 119 +++++++++++++++++++++++++++++++------ > drivers/usb/cdns3/core.h | 14 +++++ > drivers/usb/cdns3/drd.c | 7 +++ > drivers/usb/cdns3/gadget.c | 4 ++ > drivers/usb/cdns3/host.c | 7 +++ > 5 files changed, 134 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index > bfd09aa98c12..5221c1306e64 100644 > --- a/drivers/usb/cdns3/core.c > +++ b/drivers/usb/cdns3/core.c > @@ -432,6 +432,7 @@ static int cdns3_probe(struct platform_device *pdev) > return -ENOMEM; > > cdns->dev = dev; > + cdns->pdata = dev_get_platdata(dev); > > platform_set_drvdata(pdev, cdns); > > @@ -525,9 +526,11 @@ static int cdns3_probe(struct platform_device *pdev) > if (ret) > goto err4; > > + spin_lock_init(&cdns->lock); > device_set_wakeup_capable(dev, true); > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > + pm_runtime_forbid(dev); > > /* > * The controller needs less time between bus and controller suspend, @@ -574,52 > +577,134 @@ static int cdns3_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM_SLEEP > +#ifdef CONFIG_PM __maybe_unused > > -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)? > > - 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 > + 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? > + } > + > + 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? > + spin_unlock_irqrestore(&cdns->lock, flags); > } > > + return cdns3_controller_suspend(dev, PMSG_SUSPEND); } > + > +static int cdns3_resume(struct device *dev) { > + int ret; > + > + ret = cdns3_controller_resume(dev, PMSG_RESUME); > + if (ret) > + return ret; > + > pm_runtime_disable(dev); > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > - return 0; > + return ret; > } > -#endif > +#endif /* CONFIG_PM_SLEEP */ > +#endif /* CONFIG_PM */ > > static const struct dev_pm_ops cdns3_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume) > + SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL) > }; > > #ifdef CONFIG_OF > diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h index > 1ad1f1fe61e9..9eb8aeea2d20 100644 > --- a/drivers/usb/cdns3/core.h > +++ b/drivers/usb/cdns3/core.h > @@ -38,6 +38,12 @@ struct cdns3_role_driver { }; > > #define CDNS3_XHCI_RESOURCES_NUM 2 > + > +struct cdns3_platform_data { > + int (*platform_suspend)(struct device *dev, > + bool suspend, bool wakeup); > +}; > + > /** > * struct cdns3 - Representation of Cadence USB3 DRD controller. > * @dev: pointer to Cadence device struct @@ -62,6 +68,10 @@ struct > cdns3_role_driver { > * This field based on firmware setting, kernel configuration > * and hardware configuration. > * @role_sw: pointer to role switch object. > + * @in_lpm: indicate the controller is in low power mode > + * @wakeup_int: wakeup interrupt occurs > + * @pdata: platform data from glue layer > + * @lock: spinlock structure > */ > struct cdns3 { > struct device *dev; > @@ -89,6 +99,10 @@ struct cdns3 { > struct mutex mutex; > enum usb_dr_mode dr_mode; > struct usb_role_switch *role_sw; > + bool in_lpm; > + bool wakeup_int; > + struct cdns3_platform_data *pdata; > + spinlock_t lock; > }; > > 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? Li Jun > + > if (cdns->dr_mode != USB_DR_MODE_OTG) > return ret; > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index > 559e5c01c2b0..ccb257d9daa6 100644 > --- a/drivers/usb/cdns3/gadget.c > +++ b/drivers/usb/cdns3/gadget.c > @@ -1766,9 +1766,13 @@ static void cdns3_check_usb_interrupt_proceed(struct > cdns3_device *priv_dev, static irqreturn_t cdns3_device_irq_handler(int irq, > void *data) { > struct cdns3_device *priv_dev = data; > + struct cdns3 *cdns = dev_get_drvdata(priv_dev->dev); > irqreturn_t ret = IRQ_NONE; > u32 reg; > > + if (cdns->in_lpm) > + return ret; > + > /* check USB device interrupt */ > reg = readl(&priv_dev->regs->usb_ists); > if (reg) { > diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c index > ad788bf3fe4f..b579ef15f4e0 100644 > --- a/drivers/usb/cdns3/host.c > +++ b/drivers/usb/cdns3/host.c > @@ -13,11 +13,13 @@ > #include "core.h" > #include "drd.h" > #include "host-export.h" > +#include <linux/usb/hcd.h> > > static int __cdns3_host_init(struct cdns3 *cdns) { > struct platform_device *xhci; > int ret; > + struct usb_hcd *hcd; > > cdns3_drd_switch_host(cdns, 1); > > @@ -43,6 +45,11 @@ static int __cdns3_host_init(struct cdns3 *cdns) > goto err1; > } > > + /* Glue needs to access xHCI region register for Power management */ > + hcd = platform_get_drvdata(xhci); > + if (hcd) > + cdns->xhci_regs = hcd->regs; > + > return 0; > err1: > platform_device_put(xhci); > -- > 2.17.1
> -----Original Message----- > From: Jun Li <jun.li@nxp.com> > Sent: 2020年5月24日 13:58 > To: Peter Chen <peter.chen@nxp.com>; balbi@kernel.org; mathias.nyman@intel.com > Cc: 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 ... > > +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. Ah, see gadget defined it. > this role->suspend() is only to be used in system suspend but not in runtime suspend? > > > + spin_unlock_irqrestore(&cdns->lock, flags); > > } > >
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
> -----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
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.
Hi Peter, On Sun, 2020-05-24 at 07:22 +0800, Peter Chen wrote: > Introduce runtime PM and wakeup interrupt handler for cdns3, > the runtime PM is default off since other cdns3 has not implemented > glue layer support for runtime PM. > > When the controller is in low power mode, the lpm flag will be set. > The interrupt triggered later than lpm flag is set considers as > wakeup interrupt and handled at cdns_drd_irq. Wonder to know that CPUs may be powered off in this case? Do you consider the case when usb controller is in low power mode, and also power off CPUs, then how to wakeup usb controller? Thanks > Once the wakeup > occurs, it first disables interrupt to avoid later interrupt > occurrence since the controller is in low power mode at that > time, and access registers may be invalid at that time. At wakeup > handler, it will call pm_runtime_get to wakeup controller, and > at runtime resume handler, it will enable interrupt again. > > The API platform_suspend is introduced for glue layer to implement > platform specific PM sequence. > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/cdns3/core.c | 119 +++++++++++++++++++++++++++++++------ > drivers/usb/cdns3/core.h | 14 +++++ > drivers/usb/cdns3/drd.c | 7 +++ > drivers/usb/cdns3/gadget.c | 4 ++ > drivers/usb/cdns3/host.c | 7 +++ > 5 files changed, 134 insertions(+), 17 deletions(-)
> > Introduce runtime PM and wakeup interrupt handler for cdns3, > > the runtime PM is default off since other cdns3 has not implemented > > glue layer support for runtime PM. > > > > When the controller is in low power mode, the lpm flag will be set. > > The interrupt triggered later than lpm flag is set considers as > > wakeup interrupt and handled at cdns_drd_irq. > Wonder to know that CPUs may be powered off in this case? Yes > Do you consider the case when usb controller is in low power mode, and > also power off CPUs, then how to wakeup usb controller? > About USB wakeup for system suspend, there are still some works need to do for this driver, will do later. For system wakeup, it is platform specific. If the main cores are powered off totally, and do not respond any wakeup events, it needs the other core to get this wakeup event, and power on main cores. After main cores are powered on, the wakeup interrupt status are there (if controller is not powered off), the controller driver could handle this wakeup event. Peter > Thanks > > > Once the wakeup > > occurs, it first disables interrupt to avoid later interrupt > > occurrence since the controller is in low power mode at that > > time, and access registers may be invalid at that time. At wakeup > > handler, it will call pm_runtime_get to wakeup controller, and > > at runtime resume handler, it will enable interrupt again. > > > > The API platform_suspend is introduced for glue layer to implement > > platform specific PM sequence. > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > --- > > drivers/usb/cdns3/core.c | 119 +++++++++++++++++++++++++++++++------ > > drivers/usb/cdns3/core.h | 14 +++++ > > drivers/usb/cdns3/drd.c | 7 +++ > > drivers/usb/cdns3/gadget.c | 4 ++ > > drivers/usb/cdns3/host.c | 7 +++ > > 5 files changed, 134 insertions(+), 17 deletions(-) >
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index bfd09aa98c12..5221c1306e64 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -432,6 +432,7 @@ static int cdns3_probe(struct platform_device *pdev) return -ENOMEM; cdns->dev = dev; + cdns->pdata = dev_get_platdata(dev); platform_set_drvdata(pdev, cdns); @@ -525,9 +526,11 @@ static int cdns3_probe(struct platform_device *pdev) if (ret) goto err4; + spin_lock_init(&cdns->lock); device_set_wakeup_capable(dev, true); pm_runtime_set_active(dev); pm_runtime_enable(dev); + pm_runtime_forbid(dev); /* * The controller needs less time between bus and controller suspend, @@ -574,52 +577,134 @@ static int cdns3_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM_SLEEP +#ifdef CONFIG_PM -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; - 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 */ + 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); + spin_unlock_irqrestore(&cdns->lock, flags); } + return cdns3_controller_suspend(dev, PMSG_SUSPEND); +} + +static int cdns3_resume(struct device *dev) +{ + int ret; + + ret = cdns3_controller_resume(dev, PMSG_RESUME); + if (ret) + return ret; + pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); - return 0; + return ret; } -#endif +#endif /* CONFIG_PM_SLEEP */ +#endif /* CONFIG_PM */ static const struct dev_pm_ops cdns3_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume) + SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL) }; #ifdef CONFIG_OF diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h index 1ad1f1fe61e9..9eb8aeea2d20 100644 --- a/drivers/usb/cdns3/core.h +++ b/drivers/usb/cdns3/core.h @@ -38,6 +38,12 @@ struct cdns3_role_driver { }; #define CDNS3_XHCI_RESOURCES_NUM 2 + +struct cdns3_platform_data { + int (*platform_suspend)(struct device *dev, + bool suspend, bool wakeup); +}; + /** * struct cdns3 - Representation of Cadence USB3 DRD controller. * @dev: pointer to Cadence device struct @@ -62,6 +68,10 @@ struct cdns3_role_driver { * This field based on firmware setting, kernel configuration * and hardware configuration. * @role_sw: pointer to role switch object. + * @in_lpm: indicate the controller is in low power mode + * @wakeup_int: wakeup interrupt occurs + * @pdata: platform data from glue layer + * @lock: spinlock structure */ struct cdns3 { struct device *dev; @@ -89,6 +99,10 @@ struct cdns3 { struct mutex mutex; enum usb_dr_mode dr_mode; struct usb_role_switch *role_sw; + bool in_lpm; + bool wakeup_int; + struct cdns3_platform_data *pdata; + spinlock_t lock; }; 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; + } + if (cdns->dr_mode != USB_DR_MODE_OTG) return ret; diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index 559e5c01c2b0..ccb257d9daa6 100644 --- a/drivers/usb/cdns3/gadget.c +++ b/drivers/usb/cdns3/gadget.c @@ -1766,9 +1766,13 @@ static void cdns3_check_usb_interrupt_proceed(struct cdns3_device *priv_dev, static irqreturn_t cdns3_device_irq_handler(int irq, void *data) { struct cdns3_device *priv_dev = data; + struct cdns3 *cdns = dev_get_drvdata(priv_dev->dev); irqreturn_t ret = IRQ_NONE; u32 reg; + if (cdns->in_lpm) + return ret; + /* check USB device interrupt */ reg = readl(&priv_dev->regs->usb_ists); if (reg) { diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c index ad788bf3fe4f..b579ef15f4e0 100644 --- a/drivers/usb/cdns3/host.c +++ b/drivers/usb/cdns3/host.c @@ -13,11 +13,13 @@ #include "core.h" #include "drd.h" #include "host-export.h" +#include <linux/usb/hcd.h> static int __cdns3_host_init(struct cdns3 *cdns) { struct platform_device *xhci; int ret; + struct usb_hcd *hcd; cdns3_drd_switch_host(cdns, 1); @@ -43,6 +45,11 @@ static int __cdns3_host_init(struct cdns3 *cdns) goto err1; } + /* Glue needs to access xHCI region register for Power management */ + hcd = platform_get_drvdata(xhci); + if (hcd) + cdns->xhci_regs = hcd->regs; + return 0; err1: platform_device_put(xhci);
Introduce runtime PM and wakeup interrupt handler for cdns3, the runtime PM is default off since other cdns3 has not implemented glue layer support for runtime PM. When the controller is in low power mode, the lpm flag will be set. The interrupt triggered later than lpm flag is set considers as wakeup interrupt and handled at cdns_drd_irq. Once the wakeup occurs, it first disables interrupt to avoid later interrupt occurrence since the controller is in low power mode at that time, and access registers may be invalid at that time. At wakeup handler, it will call pm_runtime_get to wakeup controller, and at runtime resume handler, it will enable interrupt again. The API platform_suspend is introduced for glue layer to implement platform specific PM sequence. Signed-off-by: Peter Chen <peter.chen@nxp.com> --- drivers/usb/cdns3/core.c | 119 +++++++++++++++++++++++++++++++------ drivers/usb/cdns3/core.h | 14 +++++ drivers/usb/cdns3/drd.c | 7 +++ drivers/usb/cdns3/gadget.c | 4 ++ drivers/usb/cdns3/host.c | 7 +++ 5 files changed, 134 insertions(+), 17 deletions(-)