On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam
[off-list ref] wrote:

On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote:

quoted

On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam
[off-list ref] wrote:

quoted

Hi Rafael,


On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam
[off-list ref] wrote:

quoted

Hi Rafael,



On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote:

quoted

On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote:

quoted

From: Sricharan R <redacted>

Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.

Signed-off-by: Sricharan R <redacted>
Signed-off-by: Vivek Gautam <redacted>
Reviewed-by: Tomasz Figa <redacted>
Cc: Rafael J. Wysocki <redacted>
Cc: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---

   - Change since v11
     * Replaced DL_FLAG_AUTOREMOVE flag with
DL_FLAG_AUTOREMOVE_SUPPLIER.

   drivers/iommu/arm-smmu.c | 12 ++++++++++++
   1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 09265e206e2d..916cde4954d2 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c

@@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device

*dev)
         iommu_device_link(&smmu->iommu, dev);
   +     if (pm_runtime_enabled(smmu->dev) &&

Why does the creation of the link depend on whether or not runtime PM
is enabled for the MMU device?

The main purpose of this device link is to handle the runtime PM
synchronization
between the supplier (iommu) and consumer (client devices, such as
GPU/display).
Moreover, the runtime pm is conditionally enabled for smmu devices that
support
such [1].

Is there something you would like me to modify in this patch?

Not really, as long as you are sure that it is correct. :-)

You need to remember, however, that if you add system-wide PM
callbacks to the driver, the ordering between them and the client
device callbacks during system-wide suspend matters as well.  Don't
you need the link the ensure the correct system-wide suspend ordering
too?

The fact that currently we handle clocks only through runtime pm callbacks,
would it be better to call runtime pm put/get in system-wide PM callbacks.
This would be same as i mentioned in the other thread.

Well, my point is that there's no reason for system-wide suspend to
depend directly on runtime PM (which can be effectively disabled by
user space as mentioned for multiple times in this thread).

It simply is not efficient to let the clock run while the system as a
whole is sleeping (even if power/control has been set to "on" for this
particular device) and it should not be too hard to prevent that from
happening.  You may need an additional flag in the driver for that or
similar, but it definitely should be doable.

Now, that's my advice and I'm not the maintainer of that code, so it
is your call (as long as the maintainer agrees with it).

Thanks,
Rafael