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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]


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?

> 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.

> +		cdns->wakeup_pending = true;
> +		if ((cdns->role == USB_ROLE_HOST) && cdns->host_dev)
> +			pm_request_resume(&cdns->host_dev->dev);

nothing for peripheral 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?

> @@ -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()?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]