Date:   Thu, 8 Mar 2018 12:12:21 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Vivek Gautam <vivek.gautam@...eaurora.org>,
        Joerg Roedel <joro@...tes.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Will Deacon <will.deacon@....com>,
        Rob Clark <robdclark@...il.com>,
        "open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
        devicetree@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        jcrouse@...eaurora.org, Stephen Boyd <sboyd@...eaurora.org>,
        Sricharan R <sricharan@...eaurora.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Archit Taneja <architt@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v8 3/5] iommu/arm-smmu: Invoke pm_runtime during probe,
 add/remove device

On 08/03/18 04:33, Tomasz Figa wrote:
> On Thu, Mar 8, 2018 at 1:58 AM, Robin Murphy <robin.murphy@....com> wrote:
>> On 07/03/18 13:52, Tomasz Figa wrote:
>>>
>>> On Wed, Mar 7, 2018 at 9:38 PM, Robin Murphy <robin.murphy@....com> wrote:
>>>>
>>>> On 02/03/18 10:10, Vivek Gautam wrote:
>>>>>
>>>>>
>>>>> From: Sricharan R <sricharan@...eaurora.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@...eaurora.org>
>>>>> [vivek: Cleanup pm runtime calls]
>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.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.

Powered by blists - more mailing lists