Re: [PATCH v11 0/4] iommu/arm-smmu: Add runtime pm/sleep support

[Vivek Gautam]

Hi Robin,


On Mon, May 21, 2018 at 7:12 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> On 22/03/18 10:22, Vivek Gautam wrote:
>>
>> This series provides the support for turning on the arm-smmu's
>> clocks/power domains using runtime pm. This is done using the
>> recently introduced device links patches, which lets the smmu's
>> runtime to follow the master's runtime pm, so the smmu remains
>> powered only when the masters use it.
>> As not all implementations support clock/power gating, we are checking
>> for a valid 'smmu->dev's pm_domain' to conditionally enable the runtime
>> power management for such smmu implementations that can support it.
>>
>> This series also adds support for Qcom's arm-smmu-v2 variant that
>> has different clocks and power requirements.
>>
>> Took some reference from the exynos runtime patches [1].
>>
>> With conditional runtime pm now, we avoid touching dev->power.lock
>> in fastpaths for smmu implementations that don't need to do anything
>> useful with pm_runtime.
>> This lets us to use the much-argued pm_runtime_get_sync/put_sync()
>> calls in map/unmap callbacks so that the clients do not have to
>> worry about handling any of the arm-smmu's power.
>>
>> Previous version of this patch series is @ [5].
>>
>> [v11]
>>     * Some more cleanups for device link. We don't need an explicit
>>       delete for device link from the driver, but just set the flag
>>       DL_FLAG_AUTOREMOVE.
>>       device_link_add() API description says -
>>       "If the DL_FLAG_AUTOREMOVE is set, the link will be removed
>>       automatically when the consumer device driver unbinds."
>
>
> But then if the consumer subsequently rebinds, there's no opportunity to
> recreate the link, and PM will just quietly stop functioning properly. Given
> that it's a lot more reasonable for users to unload and reload modules (or
> otherwise switch drivers) for a consumer device than it is to unbind the
> SMMU driver or expect actual device hotplug, I'd rather do nothing and
> theoretically leak links than autoremove them incorrectly.
>
> AFAICS the previous discussion on this got as far as Lukas' suggestion of
> adding an equivalent flag for supplier-managed links (which FWIW sounds good
> to me), but seems to have stalled there.

Thanks for the review. I have posted a small change [1] for this.
Kindly take a look.

Best regards
Vivek

[1] https://patchwork.kernel.org/patch/10438109/

>
> Robin.
>
>
>>     * Addressed the comments for 'smmu' in arm_smmu_map/unmap().
>>     * Dropped the patch [10] that introduced device_link_del_dev() API.
>>
>> [v10]
>>     * Introduce device_link_del_dev() API to delete the link between
>>       given consumer and supplier devices. The users of device link
>>       do not need to store link pointer to delete the link later.
>>       They can straightaway use this API by passing consumer and
>>       supplier devices.
>>     * Made corresponding changes to arm-smmu driver patch handling the
>>       device links.
>>     * Dropped the patch [9] that was adding device_link_find() API to
>>       device core layer. device_link_del_dev() serves the purpose to
>>       directly delete the link between two given devices.
>>
>> [v9]
>>     * Removed 'rpm_supported' flag, instead checking on pm_domain
>>       to enable runtime pm.
>>     * Creating device link only when the runtime pm is enabled, as we
>>       don't need a device link besides managing the power dependency
>>       between supplier and consumer devices.
>>     * Introducing a patch to add device_link_find() API that finds
>>       and existing link between supplier and consumer devices.
>>       Also, made necessary change to device_link_add() to use this API.
>>     * arm_smmu_remove_device() now uses this device_link_find() to find
>>       the device link between smmu device and the master device, and then
>>       delete this link.
>>     * Dropped the destroy_domain_context() fix [8] as it was rather,
>>       introducing catastrophically bad problem by destroying
>>       'good dev's domain context.
>>     * Added 'Reviwed-by' tag for Tomasz's review.
>>
>> [v8]
>>     * Major change -
>>       - Added a flag 'rpm_supported' which each platform that supports
>>         runtime pm, can enable, and we enable runtime_pm over arm-smmu
>>         only when this flag is set.
>>       - Adding the conditional pm_runtime_get/put() calls to .map, .unmap
>>         and .attach_dev ops.
>>       - Dropped the patch [6] that exported pm_runtim_get/put_suupliers(),
>>         and also dropped the user driver patch [7] for these APIs.
>>
>>     * Clock code further cleanup
>>       - doing only clk_bulk_enable() and clk_bulk_disable() in runtime pm
>>         callbacks. We shouldn't be taking a slow path
>> (clk_prepare/unprepare())
>>         from these runtime pm callbacks. Thereby, moved clk_bulk_prepare()
>> to
>>         arm_smmu_device_probe(), and clk_bulk_unprepare() to
>>         arm_smmu_device_remove().
>>       - clk data filling to a common method arm_smmu_fill_clk_data() that
>>         fills the clock ids and number of clocks.
>>
>>     * Addressed other nits and comments
>>       - device_link_add() error path fixed.
>>       - Fix for checking negative error value from pm_runtime_get_sync().
>>       - Documentation redo.
>>
>>     * Added another patch fixing the error path in arm_smmu_attach_dev()
>>       to destroy allocated domain context.
>>
>> [v7]
>>     * Addressed review comments given by Robin Murphy -
>>       - Added device_link_del() in .remove_device path.
>>       - Error path cleanup in arm_smmu_add_device().
>>       - Added pm_runtime_get/put_sync() in .remove path, and replaced
>>          pm_runtime_force_suspend() with pm_runtime_disable().
>>       - clk_names cleanup in arm_smmu_init_clks()
>>     * Added 'Reviewed-by' given by Rob H.
>>
>> [V6]
>>     * Added Ack given by Rafael to first patch in the series.
>>     * Addressed Rob Herring's comment for adding soc specific compatible
>>       string as well besides 'qcom,smmu-v2'.
>>
>> [V5]
>>     * Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over
>>       the list [3] for the last patch series.
>>     * Added a patch to export pm_runtime_get/put_suppliers() APIs to the
>>       series as agreed with Rafael [4].
>>     * Added the related patch for msm drm iommu layer to use
>>       pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs.
>>     * Dropped arm-mmu500 clock patch since that would break existing
>>       platforms.
>>     * Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to
>> reflect
>>       the IP version rather than the platform on which it is used.
>>       The same IP is used across multiple platforms including msm8996,
>>       and sdm845 etc.
>>     * Using clock bulk APIs to handle the clocks available to the IP as
>>       suggested by Stephen Boyd.
>>     * The first patch in v4 version of the patch-series:
>>       ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has
>>       already made it to mainline.
>>
>> [V4]
>>     * Reworked the clock handling part. We now take clock names as data
>>       in the driver for supported compatible versions, and loop over them
>>       to get, enable, and disable the clocks.
>>     * Using qcom,msm8996 based compatibles for bindings instead of a
>> generic
>>       qcom compatible.
>>     * Refactor MMU500 patch to just add the necessary clock names data and
>>       corresponding bindings.
>>     * Added the pm_runtime_get/put() calls in .unmap iommu op (fix added
>> by
>>       Stanimir on top of previous patch version.
>>     * Added a patch to fix error path in arm_smmu_add_device()
>>     * Removed patch 3/5 of V3 patch series that added qcom,smmu-v2
>> bindings.
>>
>> [V3]
>>     * Reworked the patches to keep the clocks init/enabling function
>>       separately for each compatible.
>>
>>     * Added clocks bindings for MMU40x/500.
>>
>>     * Added a new compatible for qcom,smmu-v2 implementation and
>>       the clock bindings for the same.
>>
>>     * Rebased on top of 4.11-rc1
>>
>> [V2]
>>     * Split the patches little differently.
>>
>>     * Addressed comments.
>>
>>     * Removed the patch #4 [2] from previous post
>>       for arm-smmu context save restore. Planning to
>>       post this separately after reworking/addressing Robin's
>>       feedback.
>>
>>     * Reversed the sequence to disable clocks than enabling.
>>       This was required for those cases where the
>>       clocks are populated in a dependent order from DT.
>>
>> [1] https://lkml.org/lkml/2016/10/20/70
>> [2] https://patchwork.kernel.org/patch/9389717/
>> [3] https://patchwork.kernel.org/patch/10204925/
>> [4] https://patchwork.kernel.org/patch/10102445/
>> [5] https://lkml.org/lkml/2018/3/14/127
>> [6] https://patchwork.kernel.org/patch/10204945/
>> [7] https://patchwork.kernel.org/patch/10204925/
>> [8] https://patchwork.kernel.org/patch/10254105/
>> [9] https://patchwork.kernel.org/patch/10277975/
>> [10] https://patchwork.kernel.org/patch/10281613/
>>
>> Sricharan R (3):
>>    iommu/arm-smmu: Add pm_runtime/sleep ops
>>    iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
>>    iommu/arm-smmu: Add the device_link between masters and smmu
>>
>> Vivek Gautam (1):
>>    iommu/arm-smmu: Add support for qcom,smmu-v2 variant
>>
>>   .../devicetree/bindings/iommu/arm,smmu.txt         |  42 +++++
>>   drivers/iommu/arm-smmu.c                           | 178
>> +++++++++++++++++++--
>>   2 files changed, 210 insertions(+), 10 deletions(-)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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