On Tue, Feb 13, 2018 at 9:57 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote: > On 13/02/18 08:24, Tomasz Figa wrote: >> >> Hi Vivek, >> >> Thanks for the patch. Please see my comments inline. >> >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >> <vivek.gautam@xxxxxxxxxxxxxx> wrote: >>> >>> From: Sricharan R <sricharan@xxxxxxxxxxxxxx> >>> >>> 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 <sricharan@xxxxxxxxxxxxxx> >>> [vivek: Cleanup pm runtime calls] >>> Signed-off-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> >>> --- >>> 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? > >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
