Re: [PATCH v7 2/3] usb: cdns3: add runtime PM support

[Peter Chen <peter.chen@nxp.com>]

On 20-08-27 16:16:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> 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