lkml.org 
[lkml]   [2018]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v8 3/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
From
Date
On 08/03/18 04:33, Tomasz Figa wrote:
> On Thu, Mar 8, 2018 at 1:58 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 07/03/18 13:52, Tomasz Figa wrote:
>>>
>>> On Wed, Mar 7, 2018 at 9:38 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 02/03/18 10:10, Vivek Gautam wrote:
>>>>>
>>>>>
>>>>> From: Sricharan R <sricharan@codeaurora.org>
>>>>>
>>>>> 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@codeaurora.org>
>>>>> [vivek: Cleanup pm runtime calls]
>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>>>> ---
>>>>> drivers/iommu/arm-smmu.c | 96
>>>>> ++++++++++++++++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 88 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>>>> index c8b16f53f597..3d6a1875431f 100644
>>>>> --- a/drivers/iommu/arm-smmu.c
>>>>> +++ b/drivers/iommu/arm-smmu.c
>>>>> @@ -209,6 +209,8 @@ struct arm_smmu_device {
>>>>> struct clk_bulk_data *clks;
>>>>> int num_clks;
>>>>> + bool rpm_supported;
>>>>> +
>>>>
>>>>
>>>>
>>>> Can we not automatically infer this from whether clocks and/or power
>>>> domains
>>>> are specified or not, then just use pm_runtime_enabled() as the fast-path
>>>> check as Tomasz originally proposed?
>>>
>>>
>>> I wouldn't tie this to presence of clocks, since as a next step we
>>> would want to actually control the clocks separately. (As far as I
>>> understand, on QCom SoCs we might want to have runtime PM active for
>>> the translation to work, but clocks gated whenever access to SMMU
>>> registers is not needed.) Moreover, you might still have some super
>>> high scale thousand-core systems that require clocks to be
>>> prepare-enabled, but runtime PM would be undesirable for the reasons
>>> we discussed before.
>>>
>>>>
>>>> I worry that relying on statically-defined matchdata is just going to
>>>> blow
>>>> up the driver and DT binding into a maintenance nightmare; I really don't
>>>> want to start needing separate definitions for e.g.
>>>> "arm,juno-etr-mmu-401"
>>>> and "arm,juno-hdlcd-mmu-401" just because one otherwise-identical
>>>> instance
>>>> within the SoC is in a separate controllable power domain while the
>>>> others
>>>> aren't.
>>>
>>>
>>> I don't see a reason why both couldn't just have RPM supported
>>> regardless of whether there is a real power domain. It would
>>> effectively be just a no-op for those that don't have one.
>>
>>
>> Because you're then effectively defining "compatible" values for the sake of
>> attaching software policy to them, rather than actually describing different
>> hardware implementations.
>>
>> The fact that RPM can't do anything meaningful unless relevant clock/power
>> aspects *are* described, however, means that we shouldn't need additional
>> information redundant with that. Much like the fact that we don't *already*
>> have an "arm,juno-hdlcd-mmu-401" compatible to account for those being
>> integrated such that IDR0.CTTW has the wrong value, since the presence or
>> not of the "dma-coherent" property already describes the truth in that
>> regard.
>
> Fair enough.
>
>>
>>> IMHO the
>>> only reason to avoid having the RPM enabled is the scalability issue
>>> we discussed before.
>>
>>
>> Yes, but that's kind of my point; in reality high throughput/minimal latency
>> and aggressive power management are more or less mutually exclusive. Mobile
>> SoCs with fine-grained clock trees and power domains won't have multiple
>> 40GBe/NVMf/whatever links running flat out in parallel; conversely
>> networking/infrastructure/server SoCs aren't designed around saving every
>> last microamp of leakage current - even in the (fairly unlikely) case of the
>> interconnect clocks being software-gateable at all I would be very surprised
>> if that were ever exposed directly to Linux (FWIW I believe ACPI essentially
>> *requires* clocks to be abstracted behind firmware).
>>
>> Realistically then, explicit clocks are only expected on systems which care
>> about power management. We can always revisit that assumption if anything
>> crazy where it isn't the case ever becomes non-theoretical, but for now it's
>> one I'm entirely comfortable with. If on the other hand it turns out that we
>> can rely on just a power domain being present wherever we want RPM, making
>> clocks moot, then all the better.
>
> Alright. Since Qcom would be the only user of clock and power handling
> for the time being, I think checking power domain presence could work
> for us. +/- the fact that clocks need to be handled even if power
> domain is not present, but we should normally always have both.

Great! (the issue of Qcom-specific clock handling is a separate argument
which I don't feel like reigniting just now...)

> Now we need a way to do the check. Perhaps for the time being it would
> be enough to just check for the power-domains property in DT?

AFAICS, it might be as simple as arm_smmu_probe() doing this:

/*
* We want to avoid touching dev->power.lock in fastpaths unless
* it's really going to do something useful - pm_runtime_enabled()
* can serve as an ideal proxy for that decision.
*/
if (dev->pm_domain)
pm_runtime_enable(dev);

or maybe even just gate all the calls with "if (smmu->dev.pm_domain)"
directly (like pcie-mediatek does), but I'm not sure which would be
conceptually cleaner.

Robin.

\
 
 \ /
  Last update: 2018-03-08 13:13    [W:0.076 / U:1.100 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site