Re: [PATCH v4 4/4] dma: pl330: add Power Management support

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On śro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote:
> On 16 September 2014 10:51, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:

[...]

> >
> > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> >         return 0;
> >  }
> >
> > +/*
> > + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver
> > + * will only disable/enable the clock in runtime PM suspend/resume.
> > + */
> > +static int __maybe_unused pl330_suspend(struct device *dev)
> > +{
> > +       struct amba_device *pcdev = to_amba_device(dev);
> > +
> > +       if (!pm_runtime_suspended(dev))
> > +               amba_pclk_disable(pcdev);
> 
> I would suggest to use pm_runtime_force_suspend() instead of the above.

Sure, I can change it but... (see below)

> 
> > +       amba_pclk_unprepare(pcdev);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused pl330_resume(struct device *dev)
> > +{
> > +       struct amba_device *pcdev = to_amba_device(dev);
> > +
> > +       amba_pclk_prepare(pcdev);
> > +       if (!pm_runtime_suspended(dev))
> > +               return amba_pclk_enable(pcdev);
> 
> The above if statement could be replaced with pm_runtime_force_resume().

But that would lead to runtime resuming the device even when it is not
needed. We don't have to fully wakeup the device during resume operation
when the device was runtime suspended before suspend.

> 
> Doing that, means the amba_pclk_enable|disable() don't need to be
> exported from the AMBA bus header, but entirely handled by the AMBA
> bus itself.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
> > +
> >  static int
> >  pl330_probe(struct amba_device *adev, const struct amba_id *id)
> >  {
> > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
> >                 pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan,
> >                 pcfg->num_peri, pcfg->num_events);
> >
> 
> You need pm_runtime_set_active() here as well.

This is done by amba/bus.c. Do I have to do it again?

> 
> > +       pm_runtime_irq_safe(&adev->dev);
> > +       pm_runtime_put_noidle(&adev->dev);
> 
> Why pm_runtime_put_noidle(), that seems like you might end up leaving
> the device in active state - unless you get some request. Likely not
> what you want?

Hmmm... I am sorry but I do not get the point here. It could be
pm_runtime_put() as well because initially we start with counter
incremented by amba/bus.c.


> A final question, have you considered using runtime PM autosuspend
> feature. Any reason to why not?

There is no reason, I just implemented the easier way. I was hoping for
such ideas of improvement. I'll add autosuspend.


Thanks for feedback, I really appreciate it!

Best regards,
Krzysztof

> 
> > +
> >         return 0;
> >  probe_err3:
> >         /* Idle the DMAC */
> > @@ -2764,6 +2830,8 @@ static int pl330_remove(struct amba_device *adev)
> >         struct pl330_dmac *pl330 = amba_get_drvdata(adev);
> >         struct dma_pl330_chan *pch, *_p;
> >
> > +       pm_runtime_get_noresume(pl330->ddma.dev);
> > +
> >         if (adev->dev.of_node)
> >                 of_dma_controller_free(adev->dev.of_node);
> >
> > @@ -2802,6 +2870,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