Re: [PATCH v7 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

[Tomasz Figa]

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