On 20-08-27 16:16:03, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen <peter.chen@xxxxxxx> writes:
> > Introduce runtime PM and wakeup interrupt handler for cdns3,
> > the runtime PM is default off since other cdns3 may not
> > implement glue layer support for runtime PM.
> >
> > One typical wakeup event use case is xHCI runtime suspend will clear
> > USBCMD.RS bit, after that the xHCI will not trigger any interrupts,
> > so its parent (cdns core device) needs to resume xHCI device when
> > any (wakeup) events occurs at host port.
> >
> > 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_wakeup_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_request_resume to wakeup xHCI device, 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.
>
> can't you rely on parent->child relationship here?
Sorry, what do you mean? It is the glue layer needed API for power
management, and pass through the platform_data when create the core
device.
>
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index e56dbb6a898c..faee5ec5fc20 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -392,6 +392,29 @@ static void set_phy_power_off(struct cdns3 *cdns)
> > phy_power_off(cdns->usb2_phy);
> > }
> >
> > +/**
> > + * cdns3_wakeup_irq - interrupt handler for wakeup events
> > + * @irq: irq number for cdns3 core device
> > + * @data: structure of cdns3
> > + *
> > + * Returns IRQ_HANDLED or IRQ_NONE
> > + */
> > +static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
> > +{
> > + struct cdns3 *cdns = data;
> > +
> > + if (cdns->in_lpm) {
> > + disable_irq_nosync(irq);
>
> why do you need to call disable_irq_nosync()? interrupts are already
> disabled.
The interrupt is pending if it is not cleared, but clear the interrupt
status needs clock which will be done at .runtime_pm_resume and will
be scheduled later. Disable it could avoid the pending'ed interrupt
entering interrupt handler again.
We have similar design at: dwc3/gadget.c and chipidea/core.c
>
> > + cdns->wakeup_pending = true;
> > + if ((cdns->role == USB_ROLE_HOST) && cdns->host_dev)
> > + pm_request_resume(&cdns->host_dev->dev);
>
> nothing for peripheral mode?
CDNS3 device is only at device mode when the connection is there, and if
the connection is there, it will not enter low power mode.
>
> > + return IRQ_HANDLED;
> > + }
> > +
> > + return IRQ_NONE;
> > +}
> > +
> > /**
> > * cdns3_probe - probe for cdns3 core device
> > * @pdev: Pointer to cdns3 core platform device
> > @@ -418,6 +441,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);
> >
> > @@ -466,6 +490,15 @@ static int cdns3_probe(struct platform_device *pdev)
> >
> > cdns->otg_res = *res;
> >
> > + cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> > + if (cdns->wakeup_irq == -EPROBE_DEFER)
> > + return cdns->wakeup_irq;
> > +
> > + if (cdns->wakeup_irq < 0) {
>
> should be <= 0, no?
Good catch, will change.
>
> > @@ -502,6 +535,19 @@ static int cdns3_probe(struct platform_device *pdev)
> > goto err3;
> > }
> >
> > + if (cdns->wakeup_irq) {
> > + ret = devm_request_threaded_irq(cdns->dev, cdns->wakeup_irq,
> > + cdns3_wakeup_irq,
> > + NULL,
>
> if the thread handler is NULL, why don't you use devm_request_irq()?
Good catch, will change.
--
Thanks,
Peter Chen
