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.
--
Thanks,
Peter Chen
