On Tue, Feb 13, 2018 at 9:57 PM, Robin Murphy [off-list ref] wrote:

On 13/02/18 08:24, Tomasz Figa wrote:

quoted

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
[off-list ref] wrote:

quoted

From: Sricharan R <redacted>

The smmu device probe/remove and add/remove master device callbacks
gets called when the smmu is not linked to its master, that is without
the context of the master device. So calling runtime apis in those places
separately.

Signed-off-by: Sricharan R <redacted>
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam <redacted>
---
  drivers/iommu/arm-smmu.c | 42
++++++++++++++++++++++++++++++++++++++----
  1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9e2f917e16c2..c024f69c1682 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c

@@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct

iommu_domain *domain)
         struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
         struct arm_smmu_device *smmu = smmu_domain->smmu;
         struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
-       int irq;
+       int ret, irq;

         if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
                 return;

+       ret = pm_runtime_get_sync(smmu->dev);
+       if (ret)
+               return;

pm_runtime_get_sync() will return 0 if the device was powered off, 1
if it was already/still powered on or runtime PM is not compiled in,
or a negative value on error, so shouldn't the test be (ret < 0)?

Moreover, I'm actually wondering if it makes any sense to power up the
hardware just to program it and power it down again. In a system where
the IOMMU is located within a power domain, it would cause the IOMMU
block to lose its state anyway.

This is generally for the case where the SMMU internal state remains active,
but the programming interface needs to be powered up in order to access it.

That's true for Qualcomm SMMU, but I think that would be different for
existing users of the driver?

quoted

Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add
pm_runtime/sleep ops", perhaps it would make more sense to just
control the clocks independently of runtime PM? Then, runtime PM could
be used for real power management, e.g. really powering the block up
and down, for further power saving.

Unfortunately that ends up pretty much unmanageable, because there are
numerous different SMMU microarchitectures with fundamentally different
clock/power domain schemes (multiplied by individual SoC integration
possibilities). Since this is fundamentally a generic architectural driver,
adding explicit clock support would probably make the whole thing about 50%
clock code, with complicated decision trees around every hardware access
calculating which clocks are necessary for a given operation on a given
system. That maintainability aspect is why we've already nacked such a
fine-grained approach in the past.

Hmm, I think we are talking about different things here. My suggestion
would not add much more code to the driver than this patch does, calls
to arm_smmu_enable_clocks() instead of pm_runtime_get_sync() and
arm_smmu_disable_clocks() instead of pm_runtime_put(). The
implementation of both functions would be a simple call to clk_bulk_
API (possibly even no need to put this into functions, just call
directly).

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html