Date:   Wed, 18 Jul 2018 19:01:28 +0530
From:   Vivek Gautam <vivek.gautam@...eaurora.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Mark Rutland <mark.rutland@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Linux PM <linux-pm@...r.kernel.org>,
        sboyd@...nel.org, linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Will Deacon <will.deacon@....com>,
        open list <linux-kernel@...r.kernel.org>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <iommu@...ts.linux-foundation.org>,
        "robh+dt" <robh+dt@...nel.org>, Lukas Wunner <lukas@...ner.de>,
        freedreno <freedreno@...ts.freedesktop.org>
Subject: Re: [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between
 masters and smmu

On Wed, Jul 18, 2018 at 6:13 PM, Robin Murphy <robin.murphy@....com> wrote:
> On 18/07/18 10:30, Vivek Gautam wrote:
>>
>> On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@...ysocki.net>
>> wrote:
>>>
>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote:
>>>>
>>>> From: Sricharan R <sricharan@...eaurora.org>
>>>>
>>>> Finally add the device link between the master device and
>>>> smmu, so that the smmu gets runtime enabled/disabled only when the
>>>> master needs it. This is done from add_device callback which gets
>>>> called once when the master is added to the smmu.
>>>>
>>>> Signed-off-by: Sricharan R <sricharan@...eaurora.org>
>>>> Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.org>
>>>> Reviewed-by: Tomasz Figa <tfiga@...omium.org>
>>>> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
>>>> Cc: Lukas Wunner <lukas@...ner.de>
>>>> ---
>>>>
>>>>   - Change since v11
>>>>     * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER.
>>>>
>>>>   drivers/iommu/arm-smmu.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>>> index 09265e206e2d..916cde4954d2 100644
>>>> --- a/drivers/iommu/arm-smmu.c
>>>> +++ b/drivers/iommu/arm-smmu.c
>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device
>>>> *dev)
>>>>
>>>>        iommu_device_link(&smmu->iommu, dev);
>>>>
>>>> +     if (pm_runtime_enabled(smmu->dev) &&
>>>
>>>
>>> Why does the creation of the link depend on whether or not runtime PM
>>> is enabled for the MMU device?
>>>
>>> What about system-wide PM and system shutdown?  Are they always
>>> guaranteed
>>> to happen in the right order without the link?
>>
>>
>> Hi Robin,
>>
>> As Rafael pointed, we should the device link creation should not depend on
>> runtime PM being enabled or not, as we would also want to guarantee
>> that system wide PM callbacks are called in the right order for smmu
>> and clients.
>>
>> Does this change of removing the check for pm_runtime_enabled() from here
>> looks okay to you?
>
>
> FWIW the existing system PM ops make no claim to be perfect, and I wouldn't
> be at all surprised if it was only by coincidence that my devices happened
> to put on the relevant lists in the right order to start with. If we no
> longer need to worry about explicit device_link housekeeping in the SMMU
> driver, then creating them unconditionally sounds like the sensible thing to
> do. I'd be inclined to treat failure as non-fatal like we do for the sysfs
> link, though, since it's another thing that correct SMMU operation doesn't
> actually depend on (at this point we don't necessarily know if this consumer
> even has a driver at all).

Thanks. I will then respin the patch taking care of treating failure
as non-fatal.

>> FYI, as discussed in the first patch [1] of this series, I will add a
>> system wide
>> suspend callback - arm_smmu_pm_suspend, that would do clock disable, and
>> will
>> add corresponding clock enable calls in arm_smmu_pm_resume().
>
>
> OK, I still don't really understand the finer points of how system PM and
> runtime PM interact, but if making it robust is just a case of calling the
> runtime suspend/resume hooks as appropriate from the system ones, that
> sounds reasonable.

Sure. Thanks.

Best regards
Vivek

>
> Robin.
>
>>
>> [1] https://lore.kernel.org/patchwork/patch/960460/
>>
>>
>> Best regards
>> Vivek
>>
>>>
>>>> +         !device_link_add(dev, smmu->dev,
>>>> +                     DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER))
>>>> {
>>>> +             dev_err(smmu->dev, "Unable to add link to the consumer
>>>> %s\n",
>>>> +                     dev_name(dev));
>>>> +             ret = -ENODEV;
>>>> +             goto out_unlink;
>>>> +     }
>>>> +
>>>>        return 0;
>>>>
>>>> +out_unlink:
>>>> +     iommu_device_unlink(&smmu->iommu, dev);
>>>> +     arm_smmu_master_free_smes(fwspec);
>>>>   out_cfg_free:
>>>>        kfree(cfg);
>>>>   out_free:
>>>>
>>>
>>>
>>
>>
>>
> _______________________________________________
> iommu mailing list
> iommu@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists