Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support

[Ulf Hansson]

On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> wrote:
> Add system suspend/resume capabilities to the pl330 driver so the amba
> bus clock could be also unprepared to conserve energy.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> ---
>  drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c3bd3584f261..fd71777fc565 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
>         return 0;
>  }
>
> +/*
> + * Runtime PM callbacks are provided by amba/bus.c driver.
> + *
> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> + * bus driver will only disable/enable the clock in runtime PM callbacks.
> + */
> +static int __maybe_unused pl330_suspend(struct device *dev)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +
> +       if (!pm_runtime_status_suspended(dev)) {

No, this wont work. Consider this scenario:

pm_runtime_status_suspended() returns "true", which means you consider
the device to be runtime PM suspended, thus you will unprepare the
clock below.

Later, a call to pm_runtime_get_sync() is invoked from "somewhere" and
before this driver's system PM resume callback has been invoked. That
will trigger the runtime PM resume callback to be invoked, causing
clock unbalance issues.

You need a pm_runtime_disable() prior checking the runtime PM status
using pm_runtime_status_suspended(). That will prevent this from
happen.

I guess you may see where this is leading. I think
pm_runtime_force_suspend|resume() is well suited for this situation.

> +               /* amba did not disable the clock */
> +               amba_pclk_disable(pcdev);
> +       }
> +       amba_pclk_unprepare(pcdev);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused pl330_resume(struct device *dev)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +       int ret;
> +
> +       ret = amba_pclk_prepare(pcdev);
> +       if (ret)
> +               return ret;
> +
> +       if (!pm_runtime_status_suspended(dev))
> +               ret = amba_pclk_enable(pcdev);
> +
> +       return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
> +
>  static int
>  pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  {
> @@ -2853,6 +2889,7 @@ static struct amba_driver pl330_driver = {
>         .drv = {
>                 .owner = THIS_MODULE,
>                 .name = "dma-pl330",
> +               .pm = &pl330_pm,
>         },
>         .id_table = pl330_ids,
>         .probe = pl330_probe,
> --
> 1.9.1
>

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/