On 18 September 2014 12:09, Krzysztof Kozlowski [off-list ref] wrote:

On śro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote:

quoted

On 16 September 2014 10:51, Krzysztof Kozlowski [off-list ref] wrote:

[...]

quoted

quoted

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

quoted

quoted

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

That's true - but that's not a big issue I believe. I think the below
problems are worse. :-)

1) You don't disable runtime PM, thus not preventing the runtime PM
callbacks from being invoked.
2) You don't update the runtime PM status, thus not reflecting the
actual state of the device.

Both the above may cause unbalanced clk_enable|disable calls.

Don't get me wrong, I certainly think the "don't do runtime PM resume
during system PM resume" is interesting to solve. But that it's a
common problem and should be supported by the runtime PM API somehow
instead. I have been thinking of extending the
pm_runtime_force_resume() API to support this, let's see when that
happens. :-)

quoted

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.

quoted

+
+       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?

Ohh, you are right! No need to add it!

quoted

quoted

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

The "no_idle" version will prevent the device from going inactive,
unless a new cycle of pm_runtime_get() to pm_runtime_put() happens,
which there are no guarantees of.

Kind regards
Uffe