[PATCH v6 0/6] iommu/arm-smmu: Add runtime pm/sleep support

[PATCH v6 0/6] iommu/arm-smmu: Add runtime pm/sleep support

[Vivek Gautam]

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.

It 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].

After much discussion [3] over the use of pm_runtime_get/put() in
.unmap op path for the arm-smmu, and after disussing over more than
a couple of approaches to address this, we are putting forward the
changes *without* using pm_runtime APIs in 'unmap'. Rather, letting
the client device take the control of powering on/off the connected
iommu through pm_runtime_get(put)_suppliers() APIs for the scnerios
when the iommu power can't be directly controlled by clients through
device links.
Rafael has agreed to export the suppliers APIs [4].

Hi Robin, Will,
please consider reviewing this series.

[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/9827825/
[4] https://patchwork.kernel.org/patch/10102445/

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 (3):
  base: power: runtime: Export pm_runtime_get/put_suppliers
  iommu/arm-smmu: Add support for qcom,smmu-v2 variant
  drm/msm: iommu: Replace runtime calls with runtime suppliers

 .../devicetree/bindings/iommu/arm,smmu.txt         |  43 +++++++
 drivers/base/power/runtime.c                       |   2 +
 drivers/gpu/drm/msm/msm_iommu.c                    |  16 +--
 drivers/iommu/arm-smmu.c                           | 124 ++++++++++++++++++++-
 4 files changed, 171 insertions(+), 14 deletions(-)

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

[PATCH v6 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers

[Vivek Gautam]

The device link allows the pm framework to tie the supplier and
consumer. So, whenever the consumer is powered-on the supplier
is powered-on first.

There are however cases in which the consumer wants to power-on
the supplier, but not itself.
E.g., A Graphics or multimedia driver wants to power-on the SMMU
to unmap a buffer and finish the TLB operations without powering
on itself. Some of these unmap requests are coming from the
user space when the controller itself is not powered-up, and it
can be huge penalty in terms of power and latency to power-up
the graphics/mm controllers.
There can be an argument that the supplier should handle this case
on its own and there should not be a need for the consumer to
power-on the supplier. But as discussed on the thread [1] about
ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
in atomic path in arm_smmu_unmap.

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

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 6e89b51ea3d9..06a2a88fe866 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1579,6 +1579,7 @@ void pm_runtime_get_suppliers(struct device *dev)
 
 	device_links_read_unlock(idx);
 }
+EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
 
 /**
  * pm_runtime_put_suppliers - Drop references to supplier devices.
@@ -1597,6 +1598,7 @@ void pm_runtime_put_suppliers(struct device *dev)
 
 	device_links_read_unlock(idx);
 }
+EXPORT_SYMBOL_GPL(pm_runtime_put_suppliers);
 
 void pm_runtime_new_link(struct device *dev)
 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v6 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

[Vivek Gautam]

From: Sricharan R <sricharan@codeaurora.org>

The smmu needs to be functional only when the respective
master's using it are active. The device_link feature
helps to track such functional dependencies, so that the
iommu gets powered when the master device enables itself
using pm_runtime. So by adapting the smmu driver for
runtime pm, above said dependency can be addressed.

This patch adds the pm runtime/sleep callbacks to the
driver and also the functions to parse the smmu clocks
from DT and enable them in resume/suspend.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
[vivek: Clock rework to request bulk of clocks]
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 78d4c6b8f1ba..21acffe91a1c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,6 +48,7 @@
 #include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -205,6 +206,9 @@ struct arm_smmu_device {
 	u32				num_global_irqs;
 	u32				num_context_irqs;
 	unsigned int			*irqs;
+	struct clk_bulk_data		*clocks;
+	int				num_clks;
+	const char * const		*clk_names;
 
 	u32				cavium_id_base; /* Specific to Cavium */
 
@@ -1685,6 +1689,25 @@ static int arm_smmu_id_size_to_bits(int size)
 	}
 }
 
+static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
+{
+	int i;
+	int num = smmu->num_clks;
+
+	if (num < 1)
+		return 0;
+
+	smmu->clocks = devm_kcalloc(smmu->dev, num,
+				    sizeof(*smmu->clocks), GFP_KERNEL);
+	if (!smmu->clocks)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++)
+		smmu->clocks[i].id = smmu->clk_names[i];
+
+	return devm_clk_bulk_get(smmu->dev, num, smmu->clocks);
+}
+
 static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
@@ -1897,10 +1920,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 struct arm_smmu_match_data {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char * const *clks;
+	int num_clks;
 };
 
 #define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
 
 ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
@@ -2001,6 +2026,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 	data = of_device_get_match_data(dev);
 	smmu->version = data->version;
 	smmu->model = data->model;
+	smmu->clk_names = data->clks;
+	smmu->num_clks = data->num_clks;
 
 	parse_driver_options(smmu);
 
@@ -2099,6 +2126,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		smmu->irqs[i] = irq;
 	}
 
+	err = arm_smmu_init_clocks(smmu);
+	if (err)
+		return err;
+
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -2197,7 +2228,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	return clk_bulk_prepare_enable(smmu->num_clks, smmu->clocks);
+}
+
+static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clocks);
+
+	return 0;
+}
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
+	SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
+			   arm_smmu_runtime_resume, NULL)
+};
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Vivek Gautam]

From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 21acffe91a1c..95478bfb182c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -914,11 +914,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;
+
 	/*
 	 * Disable the context bank and free the page tables before freeing
 	 * it.
@@ -933,6 +937,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 	__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
+
+	pm_runtime_put_sync(smmu->dev);
 }
 
 static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
@@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev)
 	while (i--)
 		cfg->smendx[i] = INVALID_SMENDX;
 
-	ret = arm_smmu_master_alloc_smes(dev);
+	ret = pm_runtime_get_sync(smmu->dev);
 	if (ret)
 		goto out_cfg_free;
 
+	ret = arm_smmu_master_alloc_smes(dev);
+	if (ret) {
+		pm_runtime_put_sync(smmu->dev);
+		goto out_cfg_free;
+	}
+
 	iommu_device_link(&smmu->iommu, dev);
 
+	pm_runtime_put_sync(smmu->dev);
+
 	return 0;
 
 out_cfg_free:
@@ -1428,7 +1442,7 @@ static void arm_smmu_remove_device(struct device *dev)
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct arm_smmu_master_cfg *cfg;
 	struct arm_smmu_device *smmu;
-
+	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops)
 		return;
@@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev)
 	cfg  = fwspec->iommu_priv;
 	smmu = cfg->smmu;
 
+	/*
+	 * The device link between the master device and
+	 * smmu is already purged at this point.
+	 * So enable the power to smmu explicitly.
+	 */
+
+	ret = pm_runtime_get_sync(smmu->dev);
+	if (ret)
+		return;
+
 	iommu_device_unlink(&smmu->iommu, dev);
 	arm_smmu_master_free_smes(fwspec);
+
+	pm_runtime_put_sync(smmu->dev);
+
 	iommu_group_remove_device(dev);
 	kfree(fwspec->iommu_priv);
 	iommu_fwspec_free(dev);
@@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	platform_set_drvdata(pdev, smmu);
+
+	pm_runtime_enable(dev);
+
+	err = pm_runtime_get_sync(dev);
+	if (err)
+		return err;
+
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -2171,9 +2206,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	platform_set_drvdata(pdev, smmu);
 	arm_smmu_device_reset(smmu);
 	arm_smmu_test_smr_masks(smmu);
+	pm_runtime_put_sync(dev);
 
 	/*
 	 * For ACPI and generic DT bindings, an SMMU will be probed before
@@ -2212,6 +2247,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 
 	/* Turn the thing off */
 	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+	pm_runtime_force_suspend(smmu->dev);
+
 	return 0;
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

From: Sricharan R <sricharan@codeaurora.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@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 95478bfb182c..33bbcfedb896 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct device_link *link;
 	int i, ret;
 
 	if (using_legacy_binding) {
@@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
 
 	pm_runtime_put_sync(smmu->dev);
 
+	/*
+	 * Establish the link between smmu and master, so that the
+	 * smmu gets runtime enabled/disabled as per the master's
+	 * needs.
+	 */
+	link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+	if (!link)
+		dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
+			 dev_name(smmu->dev), dev_name(dev));
+
 	return 0;
 
 out_cfg_free:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v6 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant

[Vivek Gautam]

qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
clock and power requirements. This smmu core is used with
multiple masters on msm8996, viz. mdss, video, etc.
Add bindings for the same.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 43 ++++++++++++++++++++++
 drivers/iommu/arm-smmu.c                           | 13 +++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 8a6ffce12af5..169222ae2706 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -17,10 +17,19 @@ conditions.
                         "arm,mmu-401"
                         "arm,mmu-500"
                         "cavium,smmu-v2"
+                        "qcom,<soc>-smmu-v2", "qcom,smmu-v2"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.
 
+                  A number of Qcom SoCs use qcom,smmu-v2 version of the IP.
+                  "qcom,<soc>-smmu-v2" represents a soc specific compatible
+                  string that should be present along with the "qcom,smmu-v2"
+                  to facilitate SoC specific clocks/power connections and to
+                  address specific bug fixes.
+                  An example string would be -
+                  "qcom,msm8996-smmu-v2", "qcom,smmu-v2".
+
 - reg           : Base address and size of the SMMU.
 
 - #global-interrupts : The number of global interrupts exposed by the
@@ -71,6 +80,23 @@ conditions.
                   or using stream matching with #iommu-cells = <2>, and
                   may be ignored if present in such cases.
 
+- clock-names:    Should be "bus", and "iface" for "qcom,smmu-v2"
+                  implementation.
+
+                  "bus" clock for "qcom,smmu-v2" is required for downstream
+                  bus access and for the smmu ptw.
+
+                  "iface" clock is required to access smmu's registers through
+                  the TCU's programming interface.
+
+- clocks:         Phandles for respective clocks described by clock-names.
+
+- power-domains:  Phandles to SMMU's power domain specifier. This is
+                  required even if SMMU belongs to the master's power
+                  domain, as the SMMU will have to be enabled and
+                  accessed before master gets enabled and linked to its
+                  SMMU.
+
 ** Deprecated properties:
 
 - mmu-masters (deprecated in favour of the generic "iommus" binding) :
@@ -137,3 +163,20 @@ conditions.
                 iommu-map = <0 &smmu3 0 0x400>;
                 ...
         };
+
+	/* Qcom's arm,smmu-v2 implementation */
+	smmu4: iommu {
+		compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+		reg = <0xd00000 0x10000>;
+
+		#global-interrupts = <1>;
+		interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 321 IRQ_TYPE_LEVEL_HIGH>;
+		#iommu-cells = <1>;
+		power-domains = <&mmcc MDSS_GDSC>;
+
+		clocks = <&mmcc SMMU_MDP_AXI_CLK>,
+			 <&mmcc SMMU_MDP_AHB_CLK>;
+		clock-names = "bus", "iface";
+	};
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 33bbcfedb896..2ade214c41bc 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -119,6 +119,7 @@ enum arm_smmu_implementation {
 	GENERIC_SMMU,
 	ARM_MMU500,
 	CAVIUM_SMMUV2,
+	QCOM_SMMUV2,
 };
 
 struct arm_smmu_s2cr {
@@ -1971,6 +1972,17 @@ struct arm_smmu_match_data {
 ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
 ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
 
+static const char * const qcom_smmuv2_clks[] = {
+	"bus", "iface",
+};
+
+static const struct arm_smmu_match_data qcom_smmuv2 = {
+	.version = ARM_SMMU_V2,
+	.model = QCOM_SMMUV2,
+	.clks = qcom_smmuv2_clks,
+	.num_clks = ARRAY_SIZE(qcom_smmuv2_clks),
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
 	{ .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 },
@@ -1978,6 +1990,7 @@ struct arm_smmu_match_data {
 	{ .compatible = "arm,mmu-401", .data = &arm_mmu401 },
 	{ .compatible = "arm,mmu-500", .data = &arm_mmu500 },
 	{ .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 },
+	{ .compatible = "qcom,smmu-v2", .data = &qcom_smmuv2 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v6 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers

[Vivek Gautam]

While handling the concerned iommu, there should not be a
need to power control the drm devices from iommu interface.
If these drm devices need to be powered around this time,
the respective drivers should take care of this.

Replace the pm_runtime_get/put_sync(<drm_device>) with
pm_runtime_get/put_suppliers(<drm_device>) calls, to power-up
the connected iommu through the device link interface.
In case the device link is not setup these get/put_suppliers()
calls will be a no-op, and the iommu driver should take care of
powering on its devices accordingly.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_iommu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index b23d33622f37..1ab629bbee69 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names,
 	struct msm_iommu *iommu = to_msm_iommu(mmu);
 	int ret;
 
-	pm_runtime_get_sync(mmu->dev);
+	pm_runtime_get_suppliers(mmu->dev);
 	ret = iommu_attach_device(iommu->domain, mmu->dev);
-	pm_runtime_put_sync(mmu->dev);
+	pm_runtime_put_suppliers(mmu->dev);
 
 	return ret;
 }
@@ -52,9 +52,9 @@ static void msm_iommu_detach(struct msm_mmu *mmu, const char * const *names,
 {
 	struct msm_iommu *iommu = to_msm_iommu(mmu);
 
-	pm_runtime_get_sync(mmu->dev);
+	pm_runtime_get_suppliers(mmu->dev);
 	iommu_detach_device(iommu->domain, mmu->dev);
-	pm_runtime_put_sync(mmu->dev);
+	pm_runtime_put_suppliers(mmu->dev);
 }
 
 static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
@@ -63,9 +63,9 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 	struct msm_iommu *iommu = to_msm_iommu(mmu);
 	size_t ret;
 
-//	pm_runtime_get_sync(mmu->dev);
+	pm_runtime_get_suppliers(mmu->dev);
 	ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot);
-//	pm_runtime_put_sync(mmu->dev);
+	pm_runtime_put_suppliers(mmu->dev);
 	WARN_ON(ret < 0);
 
 	return (ret == len) ? 0 : -EINVAL;
@@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova,
 {
 	struct msm_iommu *iommu = to_msm_iommu(mmu);
 
-	pm_runtime_get_sync(mmu->dev);
+	pm_runtime_get_suppliers(mmu->dev);
 	iommu_unmap(iommu->domain, iova, len);
-	pm_runtime_put_sync(mmu->dev);
+	pm_runtime_put_suppliers(mmu->dev);
 
 	return 0;
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v6 5/6] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

[Rob Herring]

On Fri, Jan 19, 2018 at 05:13:42PM +0530, Vivek Gautam wrote:
> qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
> clock and power requirements. This smmu core is used with
> multiple masters on msm8996, viz. mdss, video, etc.
> Add bindings for the same.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         | 43 ++++++++++++++++++++++
>  drivers/iommu/arm-smmu.c                           | 13 +++++++
>  2 files changed, 56 insertions(+)

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant

[Vivek Gautam]

On 1/30/2018 1:12 AM, Rob Herring wrote:
> On Fri, Jan 19, 2018 at 05:13:42PM +0530, Vivek Gautam wrote:
>> qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
>> clock and power requirements. This smmu core is used with
>> multiple masters on msm8996, viz. mdss, video, etc.
>> Add bindings for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>   .../devicetree/bindings/iommu/arm,smmu.txt         | 43 ++++++++++++++++++++++
>>   drivers/iommu/arm-smmu.c                           | 13 +++++++
>>   2 files changed, 56 insertions(+)
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks Rob.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v6 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

[Robin Murphy]

On 19/01/18 11:43, Vivek Gautam wrote:
> From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> The smmu needs to be functional only when the respective
> master's using it are active. The device_link feature
> helps to track such functional dependencies, so that the
> iommu gets powered when the master device enables itself
> using pm_runtime. So by adapting the smmu driver for
> runtime pm, above said dependency can be addressed.
> 
> This patch adds the pm runtime/sleep callbacks to the
> driver and also the functions to parse the smmu clocks
> from DT and enable them in resume/suspend.
> 
> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> [vivek: Clock rework to request bulk of clocks]
> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 78d4c6b8f1ba..21acffe91a1c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,6 +48,7 @@
>   #include <linux/of_iommu.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   
> @@ -205,6 +206,9 @@ struct arm_smmu_device {
>   	u32				num_global_irqs;
>   	u32				num_context_irqs;
>   	unsigned int			*irqs;
> +	struct clk_bulk_data		*clocks;
> +	int				num_clks;
> +	const char * const		*clk_names;

This seems unnecessary, as we use it a grand total of of once, during 
initialisation when we have the source data directly to hand. Just pass 
data->clks into arm_smmu_init_clks() as an additional argument.

Otherwise, I think this looks reasonable; it's about as unobtrusive as 
it's going to get.

Robin.

>   	u32				cavium_id_base; /* Specific to Cavium */
>   
> @@ -1685,6 +1689,25 @@ static int arm_smmu_id_size_to_bits(int size)
>   	}
>   }
>   
> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
> +{
> +	int i;
> +	int num = smmu->num_clks;
> +
> +	if (num < 1)
> +		return 0;
> +
> +	smmu->clocks = devm_kcalloc(smmu->dev, num,
> +				    sizeof(*smmu->clocks), GFP_KERNEL);
> +	if (!smmu->clocks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num; i++)
> +		smmu->clocks[i].id = smmu->clk_names[i];
> +
> +	return devm_clk_bulk_get(smmu->dev, num, smmu->clocks);
> +}
> +
>   static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>   {
>   	unsigned long size;
> @@ -1897,10 +1920,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>   struct arm_smmu_match_data {
>   	enum arm_smmu_arch_version version;
>   	enum arm_smmu_implementation model;
> +	const char * const *clks;
> +	int num_clks;
>   };
>   
>   #define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
>   
>   ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>   ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> @@ -2001,6 +2026,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>   	data = of_device_get_match_data(dev);
>   	smmu->version = data->version;
>   	smmu->model = data->model;
> +	smmu->clk_names = data->clks;
> +	smmu->num_clks = data->num_clks;
>   
>   	parse_driver_options(smmu);
>   
> @@ -2099,6 +2126,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		smmu->irqs[i] = irq;
>   	}
>   
> +	err = arm_smmu_init_clocks(smmu);
> +	if (err)
> +		return err;
> +
>   	err = arm_smmu_device_cfg_probe(smmu);
>   	if (err)
>   		return err;
> @@ -2197,7 +2228,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>   	return 0;
>   }
>   
> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	return clk_bulk_prepare_enable(smmu->num_clks, smmu->clocks);
> +}
> +
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clocks);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
> +	SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
> +			   arm_smmu_runtime_resume, NULL)
> +};
>   
>   static struct platform_driver arm_smmu_driver = {
>   	.driver	= {
> 

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

[Robin Murphy]

On 19/01/18 11:43, 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 21acffe91a1c..95478bfb182c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -914,11 +914,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;
> +
>   	/*
>   	 * Disable the context bank and free the page tables before freeing
>   	 * it.
> @@ -933,6 +937,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>   
>   	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>   	__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> +
> +	pm_runtime_put_sync(smmu->dev);
>   }
>   
>   static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev)
>   	while (i--)
>   		cfg->smendx[i] = INVALID_SMENDX;
>   
> -	ret = arm_smmu_master_alloc_smes(dev);
> +	ret = pm_runtime_get_sync(smmu->dev);
>   	if (ret)
>   		goto out_cfg_free;
>   
> +	ret = arm_smmu_master_alloc_smes(dev);
> +	if (ret) {
> +		pm_runtime_put_sync(smmu->dev);
> +		goto out_cfg_free;

Please keep to the existing pattern and put this on the cleanup path 
with a new label, rather than inline.

> +	}
> +
>   	iommu_device_link(&smmu->iommu, dev);
>   
> +	pm_runtime_put_sync(smmu->dev);
> +
>   	return 0;
>   
>   out_cfg_free:
> @@ -1428,7 +1442,7 @@ static void arm_smmu_remove_device(struct device *dev)
>   	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>   	struct arm_smmu_master_cfg *cfg;
>   	struct arm_smmu_device *smmu;
> -
> +	int ret;
>   
>   	if (!fwspec || fwspec->ops != &arm_smmu_ops)
>   		return;
> @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev)
>   	cfg  = fwspec->iommu_priv;
>   	smmu = cfg->smmu;
>   
> +	/*
> +	 * The device link between the master device and
> +	 * smmu is already purged at this point.
> +	 * So enable the power to smmu explicitly.
> +	 */

I don't understand this comment, especially since we don't even 
introduce device links until the following patch... :/

> +
> +	ret = pm_runtime_get_sync(smmu->dev);
> +	if (ret)
> +		return;
> +
>   	iommu_device_unlink(&smmu->iommu, dev);
>   	arm_smmu_master_free_smes(fwspec);
> +
> +	pm_runtime_put_sync(smmu->dev);
> +
>   	iommu_group_remove_device(dev);
>   	kfree(fwspec->iommu_priv);
>   	iommu_fwspec_free(dev);
> @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	if (err)
>   		return err;
>   
> +	platform_set_drvdata(pdev, smmu);
> +
> +	pm_runtime_enable(dev);
> +
> +	err = pm_runtime_get_sync(dev);
> +	if (err)
> +		return err;
> +
>   	err = arm_smmu_device_cfg_probe(smmu);
>   	if (err)
>   		return err;
> @@ -2171,9 +2206,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		return err;
>   	}
>   
> -	platform_set_drvdata(pdev, smmu);
>   	arm_smmu_device_reset(smmu);
>   	arm_smmu_test_smr_masks(smmu);
> +	pm_runtime_put_sync(dev);
>   
>   	/*
>   	 * For ACPI and generic DT bindings, an SMMU will be probed before
> @@ -2212,6 +2247,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>   
>   	/* Turn the thing off */
>   	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +	pm_runtime_force_suspend(smmu->dev);

Why do we need this? I guess it might be a Qualcomm-ism as I don't see 
anyone else calling it from .remove other than a couple of other qcom_* 
drivers. Given that we only get here during system shutdown (or the root 
user intentionally pissing about with driver unbinding), it doesn't seem 
like a point where power saving really matters all that much.

I'd also naively expect that anything this device was the last consumer 
off would get turned off by core code anyway once it's removed, but 
maybe things aren't that slick; I dunno :/

Robin.

> +
>   	return 0;
>   }
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Robin Murphy]

On 19/01/18 11:43, Vivek Gautam wrote:
> From: Sricharan R <sricharan@codeaurora.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.

Don't we need to balance this with a device_link_del() in .remove_device 
(like exynos-iommu does)?

Robin.

> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>   drivers/iommu/arm-smmu.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 95478bfb182c..33bbcfedb896 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
>   	struct arm_smmu_device *smmu;
>   	struct arm_smmu_master_cfg *cfg;
>   	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +	struct device_link *link;
>   	int i, ret;
>   
>   	if (using_legacy_binding) {
> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
>   
>   	pm_runtime_put_sync(smmu->dev);
>   
> +	/*
> +	 * Establish the link between smmu and master, so that the
> +	 * smmu gets runtime enabled/disabled as per the master's
> +	 * needs.
> +	 */
> +	link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
> +	if (!link)
> +		dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
> +			 dev_name(smmu->dev), dev_name(dev));
> +
>   	return 0;
>   
>   out_cfg_free:
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v6 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

[Vivek Gautam]

On 1/31/2018 5:53 PM, Robin Murphy wrote:
> On 19/01/18 11:43, Vivek Gautam wrote:
>> From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> The smmu needs to be functional only when the respective
>> master's using it are active. The device_link feature
>> helps to track such functional dependencies, so that the
>> iommu gets powered when the master device enables itself
>> using pm_runtime. So by adapting the smmu driver for
>> runtime pm, above said dependency can be addressed.
>>
>> This patch adds the pm runtime/sleep callbacks to the
>> driver and also the functions to parse the smmu clocks
>> from DT and enable them in resume/suspend.
>>
>> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Signed-off-by: Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> [vivek: Clock rework to request bulk of clocks]
>> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 55 
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 78d4c6b8f1ba..21acffe91a1c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -48,6 +48,7 @@
>>   #include <linux/of_iommu.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>>   @@ -205,6 +206,9 @@ struct arm_smmu_device {
>>       u32                num_global_irqs;
>>       u32                num_context_irqs;
>>       unsigned int            *irqs;
>> +    struct clk_bulk_data        *clocks;
>> +    int                num_clks;
>> +    const char * const        *clk_names;
>
> This seems unnecessary, as we use it a grand total of of once, during 
> initialisation when we have the source data directly to hand. Just 
> pass data->clks into arm_smmu_init_clks() as an additional argument.

Sure, will do that.
>
> Otherwise, I think this looks reasonable; it's about as unobtrusive as 
> it's going to get.

Thanks for reviewing.

regards
Vivek

>
> Robin.
>
>>       u32                cavium_id_base; /* Specific to Cavium */
>>   @@ -1685,6 +1689,25 @@ static int arm_smmu_id_size_to_bits(int size)
>>       }
>>   }
>>   +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
>> +{
>> +    int i;
>> +    int num = smmu->num_clks;
>> +
>> +    if (num < 1)
>> +        return 0;
>> +
>> +    smmu->clocks = devm_kcalloc(smmu->dev, num,
>> +                    sizeof(*smmu->clocks), GFP_KERNEL);
>> +    if (!smmu->clocks)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num; i++)
>> +        smmu->clocks[i].id = smmu->clk_names[i];
>> +
>> +    return devm_clk_bulk_get(smmu->dev, num, smmu->clocks);
>> +}
>> +
>>   static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>   {
>>       unsigned long size;
>> @@ -1897,10 +1920,12 @@ static int arm_smmu_device_cfg_probe(struct 
>> arm_smmu_device *smmu)
>>   struct arm_smmu_match_data {
>>       enum arm_smmu_arch_version version;
>>       enum arm_smmu_implementation model;
>> +    const char * const *clks;
>> +    int num_clks;
>>   };
>>     #define ARM_SMMU_MATCH_DATA(name, ver, imp)    \
>> -static struct arm_smmu_match_data name = { .version = ver, .model = 
>> imp }
>> +static const struct arm_smmu_match_data name = { .version = ver, 
>> .model = imp }
>>     ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>   ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>> @@ -2001,6 +2026,8 @@ static int arm_smmu_device_dt_probe(struct 
>> platform_device *pdev,
>>       data = of_device_get_match_data(dev);
>>       smmu->version = data->version;
>>       smmu->model = data->model;
>> +    smmu->clk_names = data->clks;
>> +    smmu->num_clks = data->num_clks;
>>         parse_driver_options(smmu);
>>   @@ -2099,6 +2126,10 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>           smmu->irqs[i] = irq;
>>       }
>>   +    err = arm_smmu_init_clocks(smmu);
>> +    if (err)
>> +        return err;
>> +
>>       err = arm_smmu_device_cfg_probe(smmu);
>>       if (err)
>>           return err;
>> @@ -2197,7 +2228,27 @@ static int __maybe_unused 
>> arm_smmu_pm_resume(struct device *dev)
>>       return 0;
>>   }
>>   -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>> +{
>> +    struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> +    return clk_bulk_prepare_enable(smmu->num_clks, smmu->clocks);
>> +}
>> +
>> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>> +{
>> +    struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> +    clk_bulk_disable_unprepare(smmu->num_clks, smmu->clocks);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dev_pm_ops arm_smmu_pm_ops = {
>> +    SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
>> +    SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
>> +               arm_smmu_runtime_resume, NULL)
>> +};
>>     static struct platform_driver arm_smmu_driver = {
>>       .driver    = {
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

Hi,


On 1/31/2018 6:39 PM, Robin Murphy wrote:
> On 19/01/18 11:43, Vivek Gautam wrote:
>> From: Sricharan R <sricharan@codeaurora.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.
>
> Don't we need to balance this with a device_link_del() in 
> .remove_device (like exynos-iommu does)?

Right. Will add device_link_del() call. Thanks for pointing out.

regards
Vivek

>
> Robin.
>
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 95478bfb182c..33bbcfedb896 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
>>       struct arm_smmu_device *smmu;
>>       struct arm_smmu_master_cfg *cfg;
>>       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +    struct device_link *link;
>>       int i, ret;
>>         if (using_legacy_binding) {
>> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device 
>> *dev)
>>         pm_runtime_put_sync(smmu->dev);
>>   +    /*
>> +     * Establish the link between smmu and master, so that the
>> +     * smmu gets runtime enabled/disabled as per the master's
>> +     * needs.
>> +     */
>> +    link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>> +    if (!link)
>> +        dev_warn(smmu->dev, "Unable to create device link between %s 
>> and %s\n",
>> +             dev_name(smmu->dev), dev_name(dev));
>> +
>>       return 0;
>>     out_cfg_free:
>>

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sricharan R]

Hi Robin,

On 1/31/2018 6:36 PM, Robin Murphy wrote:
> On 19/01/18 11:43, 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 21acffe91a1c..95478bfb182c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -914,11 +914,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;
>> +
>>       /*
>>        * Disable the context bank and free the page tables before freeing
>>        * it.
>> @@ -933,6 +937,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>         free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>>       __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
>> +
>> +    pm_runtime_put_sync(smmu->dev);
>>   }
>>     static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>> @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev)
>>       while (i--)
>>           cfg->smendx[i] = INVALID_SMENDX;
>>   -    ret = arm_smmu_master_alloc_smes(dev);
>> +    ret = pm_runtime_get_sync(smmu->dev);
>>       if (ret)
>>           goto out_cfg_free;
>>   +    ret = arm_smmu_master_alloc_smes(dev);
>> +    if (ret) {
>> +        pm_runtime_put_sync(smmu->dev);
>> +        goto out_cfg_free;
> 
> Please keep to the existing pattern and put this on the cleanup path with a new label, rather than inline.

 ok.

> 
>> +    }
>> +
>>       iommu_device_link(&smmu->iommu, dev);
>>   +    pm_runtime_put_sync(smmu->dev);
>> +
>>       return 0;
>>     out_cfg_free:
>> @@ -1428,7 +1442,7 @@ static void arm_smmu_remove_device(struct device *dev)
>>       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>       struct arm_smmu_master_cfg *cfg;
>>       struct arm_smmu_device *smmu;
>> -
>> +    int ret;
>>         if (!fwspec || fwspec->ops != &arm_smmu_ops)
>>           return;
>> @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev)
>>       cfg  = fwspec->iommu_priv;
>>       smmu = cfg->smmu;
>>   +    /*
>> +     * The device link between the master device and
>> +     * smmu is already purged at this point.
>> +     * So enable the power to smmu explicitly.
>> +     */
> 
> I don't understand this comment, especially since we don't even introduce device links until the following patch... :/
> 

 This is because the core device_del callback, does a device_links_purge for that device,
 before calling the remove_device notifier. As a result, have to explicitly turn on the
 power to iommu. Probably the comment should be removed, rest of the places we don't
 explain why we are turning on explicitly.

>> +
>> +    ret = pm_runtime_get_sync(smmu->dev);
>> +    if (ret)
>> +        return;
>> +
>>       iommu_device_unlink(&smmu->iommu, dev);
>>       arm_smmu_master_free_smes(fwspec);
>> +
>> +    pm_runtime_put_sync(smmu->dev);
>> +
>>       iommu_group_remove_device(dev);
>>       kfree(fwspec->iommu_priv);
>>       iommu_fwspec_free(dev);
>> @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>       if (err)
>>           return err;
>>   +    platform_set_drvdata(pdev, smmu);
>> +
>> +    pm_runtime_enable(dev);
>> +
>> +    err = pm_runtime_get_sync(dev);
>> +    if (err)
>> +        return err;
>> +
>>       err = arm_smmu_device_cfg_probe(smmu);
>>       if (err)
>>           return err;
>> @@ -2171,9 +2206,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>           return err;
>>       }
>>   -    platform_set_drvdata(pdev, smmu);
>>       arm_smmu_device_reset(smmu);
>>       arm_smmu_test_smr_masks(smmu);
>> +    pm_runtime_put_sync(dev);
>>         /*
>>        * For ACPI and generic DT bindings, an SMMU will be probed before
>> @@ -2212,6 +2247,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>>         /* Turn the thing off */
>>       writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>> +    pm_runtime_force_suspend(smmu->dev);
> 
> Why do we need this? I guess it might be a Qualcomm-ism as I don't see anyone else calling it from .remove other than a couple of other qcom_* drivers. Given that we only get here during system shutdown (or the root user intentionally pissing about with driver unbinding), it doesn't seem like a point where power saving really matters all that much.
> 
> I'd also naively expect that anything this device was the last consumer off would get turned off by core code anyway once it's removed, but maybe things aren't that slick; I dunno :/

 hmm, that should not be needed. with turning of all consumers taken care by device_link code before
 the supplier (iommu) remove gets called should ensure that. So the above force_suspend should
 not be needed/can be removed. But one more thing is, we do touch the register in the above code.
 So that should require a additional get/put sync around that writel.

Regards,
 Sricharan

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

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

[Vivek Gautam]

On 2/1/2018 5:03 PM, Sricharan R wrote:
> Hi Robin,
>
> On 1/31/2018 6:36 PM, Robin Murphy wrote:
>> On 19/01/18 11:43, Vivek Gautam wrote:
>>> From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>> [vivek: Cleanup pm runtime calls]
>>> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>> ---
>>>    drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 41 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 21acffe91a1c..95478bfb182c 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -914,11 +914,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;
>>> +
>>>        /*
>>>         * Disable the context bank and free the page tables before freeing
>>>         * it.
>>> @@ -933,6 +937,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>>          free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>>>        __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
>>> +
>>> +    pm_runtime_put_sync(smmu->dev);
>>>    }
>>>      static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>>> @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev)
>>>        while (i--)
>>>            cfg->smendx[i] = INVALID_SMENDX;
>>>    -    ret = arm_smmu_master_alloc_smes(dev);
>>> +    ret = pm_runtime_get_sync(smmu->dev);
>>>        if (ret)
>>>            goto out_cfg_free;
>>>    +    ret = arm_smmu_master_alloc_smes(dev);
>>> +    if (ret) {
>>> +        pm_runtime_put_sync(smmu->dev);
>>> +        goto out_cfg_free;
>> Please keep to the existing pattern and put this on the cleanup path with a new label, rather than inline.
>   ok.
>
>>> +    }
>>> +
>>>        iommu_device_link(&smmu->iommu, dev);
>>>    +    pm_runtime_put_sync(smmu->dev);
>>> +
>>>        return 0;
>>>      out_cfg_free:
>>> @@ -1428,7 +1442,7 @@ static void arm_smmu_remove_device(struct device *dev)
>>>        struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>>        struct arm_smmu_master_cfg *cfg;
>>>        struct arm_smmu_device *smmu;
>>> -
>>> +    int ret;
>>>          if (!fwspec || fwspec->ops != &arm_smmu_ops)
>>>            return;
>>> @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev)
>>>        cfg  = fwspec->iommu_priv;
>>>        smmu = cfg->smmu;
>>>    +    /*
>>> +     * The device link between the master device and
>>> +     * smmu is already purged at this point.
>>> +     * So enable the power to smmu explicitly.
>>> +     */
>> I don't understand this comment, especially since we don't even introduce device links until the following patch... :/
>>
>   This is because the core device_del callback, does a device_links_purge for that device,
>   before calling the remove_device notifier. As a result, have to explicitly turn on the
>   power to iommu. Probably the comment should be removed, rest of the places we don't
>   explain why we are turning on explicitly.

Yes, will remove the comment here.

>
>>> +
>>> +    ret = pm_runtime_get_sync(smmu->dev);
>>> +    if (ret)
>>> +        return;
>>> +
>>>        iommu_device_unlink(&smmu->iommu, dev);
>>>        arm_smmu_master_free_smes(fwspec);
>>> +
>>> +    pm_runtime_put_sync(smmu->dev);
>>> +
>>>        iommu_group_remove_device(dev);
>>>        kfree(fwspec->iommu_priv);
>>>        iommu_fwspec_free(dev);
>>> @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>        if (err)
>>>            return err;
>>>    +    platform_set_drvdata(pdev, smmu);
>>> +
>>> +    pm_runtime_enable(dev);
>>> +
>>> +    err = pm_runtime_get_sync(dev);
>>> +    if (err)
>>> +        return err;
>>> +
>>>        err = arm_smmu_device_cfg_probe(smmu);
>>>        if (err)
>>>            return err;
>>> @@ -2171,9 +2206,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>            return err;
>>>        }
>>>    -    platform_set_drvdata(pdev, smmu);
>>>        arm_smmu_device_reset(smmu);
>>>        arm_smmu_test_smr_masks(smmu);
>>> +    pm_runtime_put_sync(dev);
>>>          /*
>>>         * For ACPI and generic DT bindings, an SMMU will be probed before
>>> @@ -2212,6 +2247,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>>>          /* Turn the thing off */
>>>        writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>>> +    pm_runtime_force_suspend(smmu->dev);
>> Why do we need this? I guess it might be a Qualcomm-ism as I don't see anyone else calling it from .remove other than a couple of other qcom_* drivers. Given that we only get here during system shutdown (or the root user intentionally pissing about with driver unbinding), it doesn't seem like a point where power saving really matters all that much.
>>
>> I'd also naively expect that anything this device was the last consumer off would get turned off by core code anyway once it's removed, but maybe things aren't that slick; I dunno :/
>   hmm, that should not be needed. with turning of all consumers taken care by device_link code before
>   the supplier (iommu) remove gets called should ensure that. So the above force_suspend should
>   not be needed/can be removed. But one more thing is, we do touch the register in the above code.
>   So that should require a additional get/put sync around that writel.

Possibly we can replace the force_suspend() with a pm_runtime_disable() 
to complement pm_runtime_enable in the probe.
I will test the scenario where we are writing the SMMU register in 
.remove path.

regards
Vivek

>
> Regards,
>   Sricharan
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Sricharan R]

Hi Robin/Vivek,

On 2/1/2018 2:23 PM, Vivek Gautam wrote:
> Hi,
> 
> 
> On 1/31/2018 6:39 PM, Robin Murphy wrote:
>> On 19/01/18 11:43, Vivek Gautam wrote:
>>> From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.
>>
>> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)?
> 
> Right. Will add device_link_del() call. Thanks for pointing out.

 The reason for not adding device_link_del from .remove_device was, the core device_del 
 which calls the .remove_device from notifier, calls device_links_purge before that.
 That does the same thing as device_link_del. So by the time .remove_device is called,
 device_links for that device is already cleaned up. Vivek, you may want to check once that
 calling device_link_del from .remove_device has no effect, just to confirm once more.

Regards,
 Sricharan
 
> 
> regards
> Vivek
> 
>>
>> Robin.
>>
>>> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>> ---
>>>   drivers/iommu/arm-smmu.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 95478bfb182c..33bbcfedb896 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
>>>       struct arm_smmu_device *smmu;
>>>       struct arm_smmu_master_cfg *cfg;
>>>       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> +    struct device_link *link;
>>>       int i, ret;
>>>         if (using_legacy_binding) {
>>> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
>>>         pm_runtime_put_sync(smmu->dev);
>>>   +    /*
>>> +     * Establish the link between smmu and master, so that the
>>> +     * smmu gets runtime enabled/disabled as per the master's
>>> +     * needs.
>>> +     */
>>> +    link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>>> +    if (!link)
>>> +        dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
>>> +             dev_name(smmu->dev), dev_name(dev));
>>> +
>>>       return 0;
>>>     out_cfg_free:
>>>
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Robin Murphy]

On 02/02/18 05:40, Sricharan R wrote:
> Hi Robin/Vivek,
> 
> On 2/1/2018 2:23 PM, Vivek Gautam wrote:
>> Hi,
>>
>>
>> On 1/31/2018 6:39 PM, Robin Murphy wrote:
>>> On 19/01/18 11:43, Vivek Gautam wrote:
>>>> From: Sricharan R <sricharan@codeaurora.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.
>>>
>>> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)?
>>
>> Right. Will add device_link_del() call. Thanks for pointing out.
> 
>   The reason for not adding device_link_del from .remove_device was, the core device_del
>   which calls the .remove_device from notifier, calls device_links_purge before that.
>   That does the same thing as device_link_del. So by the time .remove_device is called,
>   device_links for that device is already cleaned up. Vivek, you may want to check once that
>   calling device_link_del from .remove_device has no effect, just to confirm once more.

There is at least one path in which .remove_device is not called via the 
notifier from device_del(), which is in the cleanup path of 
iommu_bus_init(). AFAICS any links created by .add_device during that 
process would be left dangling, because the device(s) would be live but 
otherwise disassociated from the IOMMU afterwards.

 From a maintenance perspective it's easier to have the call in its 
logical place even if it does nothing 99% of the time; that way we 
shouldn't have to keep an eye out for subtle changes in the power 
management code or driver core that might invalidate the device_del() 
reasoning above, and the power management guys shouldn't have to 
comprehend the internals of the IOMMU API to make sense of the 
unbalanced call if they ever want to change their API.

Thanks,
Robin.

Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Sricharan R]

Hi Robin,

On 2/2/2018 5:01 PM, Robin Murphy wrote:
> On 02/02/18 05:40, Sricharan R wrote:
>> Hi Robin/Vivek,
>>
>> On 2/1/2018 2:23 PM, Vivek Gautam wrote:
>>> Hi,
>>>
>>>
>>> On 1/31/2018 6:39 PM, Robin Murphy wrote:
>>>> On 19/01/18 11:43, Vivek Gautam wrote:
>>>>> From: Sricharan R <sricharan@codeaurora.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.
>>>>
>>>> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)?
>>>
>>> Right. Will add device_link_del() call. Thanks for pointing out.
>>
>>   The reason for not adding device_link_del from .remove_device was, the core device_del
>>   which calls the .remove_device from notifier, calls device_links_purge before that.
>>   That does the same thing as device_link_del. So by the time .remove_device is called,
>>   device_links for that device is already cleaned up. Vivek, you may want to check once that
>>   calling device_link_del from .remove_device has no effect, just to confirm once more.
> 
> There is at least one path in which .remove_device is not called via the notifier from device_del(), which is in the cleanup path of iommu_bus_init(). AFAICS any links created by .add_device during that process would be left dangling, because the device(s) would be live but otherwise disassociated from the IOMMU afterwards.
> 
> From a maintenance perspective it's easier to have the call in its logical place even if it does nothing 99% of the time; that way we shouldn't have to keep an eye out for subtle changes in the power management code or driver core that might invalidate the device_del() reasoning above, and the power management guys shouldn't have to comprehend the internals of the IOMMU API to make sense of the unbalanced call if they ever want to change their API.

 Ha, for a moment was thinking that with probe deferral add/remove_iommu_group in iommu_bus_init is dummy.
 But that may not be true for all Archs.
 Surely agree for the maintainability reason as well. Thanks.

Regards,
 Sricharan

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