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

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

[Sricharan R]

This series provides the support for turning on the arm-smmu's
clocks/powerdomains using runtime pm. This is done using the
recently introduced device links patches, which lets the symmu's
runtime to follow the master's runtime pm, so the smmu remains
powered only when the masters use it.

Also added a patch to do the context/save restore during suspend.
But a way to find if the context is really lost during suspend
has to be added in some way.

This is based on the device_link series [1].

Took some reference from the exynos runtime patches in post [2].

Tested this on a platform where context is not really lost,
but atleast the sequence to restore the context is verified.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1609.3/02637.html
[2] https://lkml.org/lkml/2016/10/20/70

Sricharan R (4):
  Docs: dt: document ARM SMMU clocks/powerdomains bindings
  iommu/arm-smmu: Add pm_runtime/sleep ops
  iommu/arm-smmu: Add context save restore support
  iommu/arm-smmu: Add the device_link between masters and smmu

 .../devicetree/bindings/iommu/arm,smmu.txt         |  12 ++
 drivers/iommu/arm-smmu.c                           | 168 ++++++++++++++++++++-
 2 files changed, 179 insertions(+), 1 deletion(-)

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

[PATCH 1/4] Docs: dt: document ARM SMMU clocks/powerdomains bindings

[Sricharan R]

Document the list of clocks and powerdomains required for the
smmu's register and bus access.

Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e862d148..ef465b0 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -46,6 +46,14 @@ conditions.
                   Care must be taken to ensure the set of matched IDs
                   does not result in conflicts.
 
+- clock-names:	  Should be a pair of "smmu_iface_clk" and "smmu_bus_clk"
+		  required for smmu's register group access and interface
+		  clk for the smmu's underlying bus access.
+
+- clocks:	  Phandles for respective clocks described by clock-names.
+
+- power-domains:  If required for turning on the smmu's clocks.
+
 ** System MMU optional properties:
 
 - dma-coherent  : Present if page table walks made by the SMMU are
@@ -84,6 +92,10 @@ conditions.
                              <0 36 4>,
                              <0 37 4>;
                 #iommu-cells = <1>;
+		clocks = <&mmcc SMMU_MDP_AHB_CLK>,
+			 <&mmcc SMMU_MDP_AXI_CLK>;
+		clock-names = "smmu_iface_clk",
+			      "smmu_bus_clk";
         };
 
         /* device with two stream IDs, 0 and 7 */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Sricharan R]

The smmu needs to be functional when the respective
master/s using it are active. As there is a device_link
between the master and smmu, the pm runtime ops for the smmu
gets invoked in sync with the master's runtime, thus the
smmu remains powered only when needed.

There are couple of places in the driver during probe,
master attach, where the smmu needs to be clocked without
the context of the master. So doing a pm_runtime_get/put sync
in those places separately.

Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 083489e4..45f2762 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -45,6 +45,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>
 
@@ -373,6 +374,8 @@ struct arm_smmu_device {
 	u32				num_global_irqs;
 	u32				num_context_irqs;
 	unsigned int			*irqs;
+	int                             num_clocks;
+	struct clk                      **clocks;
 
 	u32				cavium_id_base; /* Specific to Cavium */
 };
@@ -430,6 +433,31 @@ static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 	return container_of(dom, struct arm_smmu_domain, domain);
 }
 
+static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < smmu->num_clocks; ++i) {
+		ret = clk_prepare_enable(smmu->clocks[i]);
+		if (ret) {
+			dev_err(smmu->dev, "Couldn't enable clock #%d\n", i);
+			while (i--)
+				clk_disable_unprepare(smmu->clocks[i]);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
+{
+	int i;
+
+	for (i = 0; i < smmu->num_clocks; ++i)
+		clk_disable_unprepare(smmu->clocks[i]);
+}
+
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
 	int i = 0;
@@ -1187,11 +1215,13 @@ static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec)
 	int i, idx;
 
 	mutex_lock(&smmu->stream_map_mutex);
+	pm_runtime_get_sync(smmu->dev);
 	for_each_cfg_sme(fwspec, i, idx) {
 		if (arm_smmu_free_sme(smmu, idx))
 			arm_smmu_write_sme(smmu, idx);
 		cfg->smendx[i] = INVALID_SMENDX;
 	}
+	pm_runtime_put_sync(smmu->dev);
 	mutex_unlock(&smmu->stream_map_mutex);
 }
 
@@ -1424,9 +1454,11 @@ static int arm_smmu_add_device(struct device *dev)
 	while (i--)
 		cfg->smendx[i] = INVALID_SMENDX;
 
+	pm_runtime_get_sync(smmu->dev);
 	ret = arm_smmu_master_alloc_smes(dev);
 	if (ret)
 		goto out_free;
+	pm_runtime_put_sync(smmu->dev);
 
 	return 0;
 
@@ -1650,6 +1682,44 @@ static int arm_smmu_id_size_to_bits(int size)
 	}
 }
 
+static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
+{
+	const char *cname;
+	struct property *prop;
+	int i;
+	struct device *dev = smmu->dev;
+
+	smmu->num_clocks =
+		of_property_count_strings(dev->of_node, "clock-names");
+
+	if (smmu->num_clocks < 1)
+		return 0;
+
+	smmu->clocks = devm_kzalloc(dev,
+				    sizeof(*smmu->clocks) * smmu->num_clocks,
+				    GFP_KERNEL);
+
+	if (!smmu->clocks) {
+		dev_err(dev, "Failed to allocate memory for clocks\n");
+		return -ENODEV;
+	}
+
+	i = 0;
+	of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
+		struct clk *c = devm_clk_get(dev, cname);
+
+		if (IS_ERR(c)) {
+			dev_err(dev, "Couldn't get clock: %s", cname);
+			return -EPROBE_DEFER;
+		}
+
+		smmu->clocks[i] = c;
+		++i;
+	}
+
+	return 0;
+}
+
 static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
@@ -1968,6 +2038,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 		smmu->irqs[i] = irq;
 	}
 
+	err = arm_smmu_init_clocks(smmu);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, smmu);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -1996,8 +2073,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 
 	of_iommu_set_ops(dev->of_node, &arm_smmu_ops);
-	platform_set_drvdata(pdev, smmu);
 	arm_smmu_device_reset(smmu);
+	pm_runtime_put_sync(dev);
 
 	/* Oh, for a proper bus abstraction */
 	if (!iommu_present(&platform_bus_type))
@@ -2027,13 +2104,43 @@ 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;
+}
+
+#ifdef CONFIG_PM
+static int arm_smmu_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
+
+	return arm_smmu_enable_clocks(smmu);
+}
+
+static int arm_smmu_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
+
+	arm_smmu_disable_clocks(smmu);
+
 	return 0;
 }
+#endif
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_RUNTIME_PM_OPS(arm_smmu_suspend, arm_smmu_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
 		.name		= "arm-smmu",
 		.of_match_table	= of_match_ptr(arm_smmu_of_match),
+		.pm = &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_dt_probe,
 	.remove	= arm_smmu_device_remove,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/4] iommu/arm-smmu: Add context save restore support

[Sricharan R]

The smes registers and the context bank registers are
the ones that are needs to be saved and restored.
Fortunately the smes are already stored as a part
of the smmu device structure. So just write that
back. The data required to configure the context banks
are the master's domain data and pgtable cfgs.
So store them as a part of the context banks info
and just reconfigure the context banks on the restore
path.

Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 45f2762..578cdc2 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -328,6 +328,11 @@ struct arm_smmu_master_cfg {
 #define for_each_cfg_sme(fw, i, idx) \
 	for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i)
 
+struct cbinfo {
+	struct arm_smmu_domain		*domain;
+	struct io_pgtable_cfg		pgtbl_cfg;
+};
+
 struct arm_smmu_device {
 	struct device			*dev;
 
@@ -378,6 +383,9 @@ struct arm_smmu_device {
 	struct clk                      **clocks;
 
 	u32				cavium_id_base; /* Specific to Cavium */
+
+	/* For save/restore of context bank registers */
+	struct cbinfo                   *cb_saved_ctx;
 };
 
 enum arm_smmu_context_fmt {
@@ -972,6 +980,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 
 	/* Initialise the context bank with our page table cfg */
 	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
+	smmu->cb_saved_ctx[cfg->cbndx].domain = smmu_domain;
+	smmu->cb_saved_ctx[cfg->cbndx].pgtbl_cfg = pgtbl_cfg;
 
 	/*
 	 * Request context fault interrupt. Do this last to avoid the
@@ -1861,6 +1871,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	}
 	dev_notice(smmu->dev, "\t%u context banks (%u stage-2 only)\n",
 		   smmu->num_context_banks, smmu->num_s2_context_banks);
+
+	smmu->cb_saved_ctx = devm_kzalloc(smmu->dev,
+			     sizeof(struct cbinfo) * smmu->num_context_banks,
+			     GFP_KERNEL);
 	/*
 	 * Cavium CN88xx erratum #27704.
 	 * Ensure ASID and VMID allocation is unique across all SMMUs in
@@ -2115,8 +2129,44 @@ static int arm_smmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
+	struct arm_smmu_domain *domain = NULL;
+	struct io_pgtable_cfg *pgtbl_cfg = NULL;
+	struct arm_smmu_smr *smrs = smmu->smrs;
+	int i = 0, idx, cb, ret, pcb = 0;
+
+	ret = arm_smmu_enable_clocks(smmu);
+	if (ret)
+		return ret;
+
+	arm_smmu_device_reset(smmu);
 
-	return arm_smmu_enable_clocks(smmu);
+	/* Restore the smes and the context banks */
+	for (idx = 0; idx < smmu->num_mapping_groups; ++idx) {
+		mutex_lock(&smmu->stream_map_mutex);
+		if (!smrs[idx].valid) {
+			mutex_unlock(&smmu->stream_map_mutex);
+			continue;
+		}
+
+		arm_smmu_write_sme(smmu, idx);
+		cb = smmu->s2crs[idx].cbndx;
+		mutex_unlock(&smmu->stream_map_mutex);
+
+		if (!i || (cb != pcb)) {
+			domain = smmu->cb_saved_ctx[cb].domain;
+			pgtbl_cfg = &smmu->cb_saved_ctx[cb].pgtbl_cfg;
+
+			if (domain) {
+				mutex_lock(&domain->init_mutex);
+				arm_smmu_init_context_bank(domain, pgtbl_cfg);
+				mutex_unlock(&domain->init_mutex);
+			}
+		}
+		pcb = cb;
+		i++;
+	}
+
+	return 0;
 }
 
 static int arm_smmu_suspend(struct device *dev)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Sricharan R]

The device link between master and its smmu is added so that
the smmu gets runtime enabled/disabled when the master needs it.
This is done from add_device callback which gets called once
when the master is added to the smmu group.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 578cdc2..71ce4b6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1470,6 +1470,15 @@ static int arm_smmu_add_device(struct device *dev)
 		goto out_free;
 	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.
+	 */
+
+	device_link_add(dev, smmu->dev, DEVICE_LINK_AVAILABLE,
+			DEVICE_LINK_PM_RUNTIME);
+
 	return 0;
 
 out_free:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Mathieu Poirier]

On 21 October 2016 at 11:14, Sricharan R <sricharan@codeaurora.org> wrote:
> The smmu needs to be functional when the respective
> master/s using it are active. As there is a device_link
> between the master and smmu, the pm runtime ops for the smmu
> gets invoked in sync with the master's runtime, thus the
> smmu remains powered only when needed.
>
> There are couple of places in the driver during probe,
> master attach, where the smmu needs to be clocked without
> the context of the master. So doing a pm_runtime_get/put sync
> in those places separately.
>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 083489e4..45f2762 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -45,6 +45,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>
>
> @@ -373,6 +374,8 @@ struct arm_smmu_device {
>         u32                             num_global_irqs;
>         u32                             num_context_irqs;
>         unsigned int                    *irqs;
> +       int                             num_clocks;
> +       struct clk                      **clocks;
>
>         u32                             cavium_id_base; /* Specific to Cavium */
>  };
> @@ -430,6 +433,31 @@ static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>         return container_of(dom, struct arm_smmu_domain, domain);
>  }
>
> +static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
> +{
> +       int i, ret = 0;
> +
> +       for (i = 0; i < smmu->num_clocks; ++i) {
> +               ret = clk_prepare_enable(smmu->clocks[i]);
> +               if (ret) {
> +                       dev_err(smmu->dev, "Couldn't enable clock #%d\n", i);
> +                       while (i--)
> +                               clk_disable_unprepare(smmu->clocks[i]);
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
> +{
> +       int i;
> +
> +       for (i = 0; i < smmu->num_clocks; ++i)
> +               clk_disable_unprepare(smmu->clocks[i]);
> +}
> +
>  static void parse_driver_options(struct arm_smmu_device *smmu)
>  {
>         int i = 0;
> @@ -1187,11 +1215,13 @@ static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec)
>         int i, idx;
>
>         mutex_lock(&smmu->stream_map_mutex);
> +       pm_runtime_get_sync(smmu->dev);

Since this is generic code it is probably a good idea to check the
return value, same for _put_sync() below.

>         for_each_cfg_sme(fwspec, i, idx) {
>                 if (arm_smmu_free_sme(smmu, idx))
>                         arm_smmu_write_sme(smmu, idx);
>                 cfg->smendx[i] = INVALID_SMENDX;
>         }
> +       pm_runtime_put_sync(smmu->dev);
>         mutex_unlock(&smmu->stream_map_mutex);
>  }
>
> @@ -1424,9 +1454,11 @@ static int arm_smmu_add_device(struct device *dev)
>         while (i--)
>                 cfg->smendx[i] = INVALID_SMENDX;
>
> +       pm_runtime_get_sync(smmu->dev);
>         ret = arm_smmu_master_alloc_smes(dev);
>         if (ret)
>                 goto out_free;
> +       pm_runtime_put_sync(smmu->dev);
>
>         return 0;
>
> @@ -1650,6 +1682,44 @@ static int arm_smmu_id_size_to_bits(int size)
>         }
>  }
>
> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
> +{
> +       const char *cname;
> +       struct property *prop;
> +       int i;
> +       struct device *dev = smmu->dev;
> +
> +       smmu->num_clocks =
> +               of_property_count_strings(dev->of_node, "clock-names");
> +
> +       if (smmu->num_clocks < 1)
> +               return 0;
> +
> +       smmu->clocks = devm_kzalloc(dev,
> +                                   sizeof(*smmu->clocks) * smmu->num_clocks,
> +                                   GFP_KERNEL);
> +
> +       if (!smmu->clocks) {
> +               dev_err(dev, "Failed to allocate memory for clocks\n");
> +               return -ENODEV;
> +       }
> +
> +       i = 0;

Please do the initialisation above when you declare the variable.

> +       of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
> +               struct clk *c = devm_clk_get(dev, cname);
> +
> +               if (IS_ERR(c)) {
> +                       dev_err(dev, "Couldn't get clock: %s", cname);
> +                       return -EPROBE_DEFER;
> +               }
> +
> +               smmu->clocks[i] = c;
> +               ++i;
> +       }
> +
> +       return 0;
> +}
> +
>  static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  {
>         unsigned long size;
> @@ -1968,6 +2038,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>                 smmu->irqs[i] = irq;
>         }
>
> +       err = arm_smmu_init_clocks(smmu);
> +       if (err)
> +               return err;
> +
> +       platform_set_drvdata(pdev, smmu);
> +       pm_runtime_enable(dev);
> +       pm_runtime_get_sync(dev);
>         err = arm_smmu_device_cfg_probe(smmu);
>         if (err)
>                 return err;
> @@ -1996,8 +2073,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>         }
>
>         of_iommu_set_ops(dev->of_node, &arm_smmu_ops);
> -       platform_set_drvdata(pdev, smmu);
>         arm_smmu_device_reset(smmu);
> +       pm_runtime_put_sync(dev);
>
>         /* Oh, for a proper bus abstraction */
>         if (!iommu_present(&platform_bus_type))
> @@ -2027,13 +2104,43 @@ 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;
> +}
> +
> +#ifdef CONFIG_PM
> +static int arm_smmu_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
> +
> +       return arm_smmu_enable_clocks(smmu);
> +}
> +
> +static int arm_smmu_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
> +
> +       arm_smmu_disable_clocks(smmu);
> +
>         return 0;
>  }
> +#endif
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +       SET_RUNTIME_PM_OPS(arm_smmu_suspend, arm_smmu_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +};
>
>  static struct platform_driver arm_smmu_driver = {
>         .driver = {
>                 .name           = "arm-smmu",
>                 .of_match_table = of_match_ptr(arm_smmu_of_match),
> +               .pm = &arm_smmu_pm_ops,
>         },
>         .probe  = arm_smmu_device_dt_probe,
>         .remove = arm_smmu_device_remove,
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] iommu/arm-smmu: Add context save restore support

[Mathieu Poirier]

On 21 October 2016 at 11:14, Sricharan R <sricharan@codeaurora.org> wrote:
> The smes registers and the context bank registers are
> the ones that are needs to be saved and restored.
> Fortunately the smes are already stored as a part
> of the smmu device structure. So just write that
> back. The data required to configure the context banks
> are the master's domain data and pgtable cfgs.
> So store them as a part of the context banks info
> and just reconfigure the context banks on the restore
> path.
>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 45f2762..578cdc2 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -328,6 +328,11 @@ struct arm_smmu_master_cfg {
>  #define for_each_cfg_sme(fw, i, idx) \
>         for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i)
>
> +struct cbinfo {
> +       struct arm_smmu_domain          *domain;
> +       struct io_pgtable_cfg           pgtbl_cfg;
> +};
> +
>  struct arm_smmu_device {
>         struct device                   *dev;
>
> @@ -378,6 +383,9 @@ struct arm_smmu_device {
>         struct clk                      **clocks;
>
>         u32                             cavium_id_base; /* Specific to Cavium */
> +
> +       /* For save/restore of context bank registers */
> +       struct cbinfo                   *cb_saved_ctx;

It's not that clear to me this will become an array - better
documentation would help reviewing the code.

>  };
>
>  enum arm_smmu_context_fmt {
> @@ -972,6 +980,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
>         /* Initialise the context bank with our page table cfg */
>         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> +       smmu->cb_saved_ctx[cfg->cbndx].domain = smmu_domain;
> +       smmu->cb_saved_ctx[cfg->cbndx].pgtbl_cfg = pgtbl_cfg;
>
>         /*
>          * Request context fault interrupt. Do this last to avoid the
> @@ -1861,6 +1871,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>         }
>         dev_notice(smmu->dev, "\t%u context banks (%u stage-2 only)\n",
>                    smmu->num_context_banks, smmu->num_s2_context_banks);
> +
> +       smmu->cb_saved_ctx = devm_kzalloc(smmu->dev,
> +                            sizeof(struct cbinfo) * smmu->num_context_banks,
> +                            GFP_KERNEL);
>         /*
>          * Cavium CN88xx erratum #27704.
>          * Ensure ASID and VMID allocation is unique across all SMMUs in
> @@ -2115,8 +2129,44 @@ static int arm_smmu_resume(struct device *dev)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
>         struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
> +       struct arm_smmu_domain *domain = NULL;
> +       struct io_pgtable_cfg *pgtbl_cfg = NULL;
> +       struct arm_smmu_smr *smrs = smmu->smrs;
> +       int i = 0, idx, cb, ret, pcb = 0;
> +
> +       ret = arm_smmu_enable_clocks(smmu);
> +       if (ret)
> +               return ret;
> +
> +       arm_smmu_device_reset(smmu);
>
> -       return arm_smmu_enable_clocks(smmu);
> +       /* Restore the smes and the context banks */
> +       for (idx = 0; idx < smmu->num_mapping_groups; ++idx) {
> +               mutex_lock(&smmu->stream_map_mutex);
> +               if (!smrs[idx].valid) {
> +                       mutex_unlock(&smmu->stream_map_mutex);
> +                       continue;
> +               }
> +
> +               arm_smmu_write_sme(smmu, idx);
> +               cb = smmu->s2crs[idx].cbndx;
> +               mutex_unlock(&smmu->stream_map_mutex);
> +
> +               if (!i || (cb != pcb)) {
> +                       domain = smmu->cb_saved_ctx[cb].domain;
> +                       pgtbl_cfg = &smmu->cb_saved_ctx[cb].pgtbl_cfg;
> +
> +                       if (domain) {
> +                               mutex_lock(&domain->init_mutex);
> +                               arm_smmu_init_context_bank(domain, pgtbl_cfg);
> +                               mutex_unlock(&domain->init_mutex);
> +                       }
> +               }
> +               pcb = cb;
> +               i++;

What are you doing with variable 'i'?  Again, some comments would
greatly help with reviewing.

Thanks,
Mathieu

> +       }
> +
> +       return 0;
>  }
>
>  static int arm_smmu_suspend(struct device *dev)
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/4] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan]

Hi,

>On 21 October 2016 at 11:14, Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> The smmu needs to be functional when the respective
>> master/s using it are active. As there is a device_link
>> between the master and smmu, the pm runtime ops for the smmu
>> gets invoked in sync with the master's runtime, thus the
>> smmu remains powered only when needed.
>>
>> There are couple of places in the driver during probe,
>> master attach, where the smmu needs to be clocked without
>> the context of the master. So doing a pm_runtime_get/put sync
>> in those places separately.
>>
>> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 083489e4..45f2762 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -45,6 +45,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>
>>
>> @@ -373,6 +374,8 @@ struct arm_smmu_device {
>>         u32                             num_global_irqs;
>>         u32                             num_context_irqs;
>>         unsigned int                    *irqs;
>> +       int                             num_clocks;
>> +       struct clk                      **clocks;
>>
>>         u32                             cavium_id_base; /* Specific to Cavium */
>>  };
>> @@ -430,6 +433,31 @@ static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>>         return container_of(dom, struct arm_smmu_domain, domain);
>>  }
>>
>> +static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
>> +{
>> +       int i, ret = 0;
>> +
>> +       for (i = 0; i < smmu->num_clocks; ++i) {
>> +               ret = clk_prepare_enable(smmu->clocks[i]);
>> +               if (ret) {
>> +                       dev_err(smmu->dev, "Couldn't enable clock #%d\n", i);
>> +                       while (i--)
>> +                               clk_disable_unprepare(smmu->clocks[i]);
>> +                       break;
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < smmu->num_clocks; ++i)
>> +               clk_disable_unprepare(smmu->clocks[i]);
>> +}
>> +
>>  static void parse_driver_options(struct arm_smmu_device *smmu)
>>  {
>>         int i = 0;
>> @@ -1187,11 +1215,13 @@ static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec)
>>         int i, idx;
>>
>>         mutex_lock(&smmu->stream_map_mutex);
>> +       pm_runtime_get_sync(smmu->dev);
>
>Since this is generic code it is probably a good idea to check the
>return value, same for _put_sync() below.

 Ok, i will do it for V2.

>
>>         for_each_cfg_sme(fwspec, i, idx) {
>>                 if (arm_smmu_free_sme(smmu, idx))
>>                         arm_smmu_write_sme(smmu, idx);
>>                 cfg->smendx[i] = INVALID_SMENDX;
>>         }
>> +       pm_runtime_put_sync(smmu->dev);
>>         mutex_unlock(&smmu->stream_map_mutex);
>>  }
>>
>> @@ -1424,9 +1454,11 @@ static int arm_smmu_add_device(struct device *dev)
>>         while (i--)
>>                 cfg->smendx[i] = INVALID_SMENDX;
>>
>> +       pm_runtime_get_sync(smmu->dev);
>>         ret = arm_smmu_master_alloc_smes(dev);
>>         if (ret)
>>                 goto out_free;
>> +       pm_runtime_put_sync(smmu->dev);
>>
>>         return 0;
>>
>> @@ -1650,6 +1682,44 @@ static int arm_smmu_id_size_to_bits(int size)
>>         }
>>  }
>>
>> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
>> +{
>> +       const char *cname;
>> +       struct property *prop;
>> +       int i;
>> +       struct device *dev = smmu->dev;
>> +
>> +       smmu->num_clocks =
>> +               of_property_count_strings(dev->of_node, "clock-names");
>> +
>> +       if (smmu->num_clocks < 1)
>> +               return 0;
>> +
>> +       smmu->clocks = devm_kzalloc(dev,
>> +                                   sizeof(*smmu->clocks) * smmu->num_clocks,
>> +                                   GFP_KERNEL);
>> +
>> +       if (!smmu->clocks) {
>> +               dev_err(dev, "Failed to allocate memory for clocks\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       i = 0;
>
>Please do the initialisation above when you declare the variable.

     ok, will change it.

Regards,
 Sricharan

RE: [PATCH 3/4] iommu/arm-smmu: Add context save restore support

[Sricharan]

Hi,

>> The smes registers and the context bank registers are
>> back. The data required to configure the context banks
>> are the master's domain data and pgtable cfgs.
>> So store them as a part of the context banks info
>> the ones that are needs to be saved and restored.
>> Fortunately the smes are already stored as a part
>> of the smmu device structure. So just write that
>> and just reconfigure the context banks on the restore
>> path.
>>
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 45f2762..578cdc2 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -328,6 +328,11 @@ struct arm_smmu_master_cfg {
>>  #define for_each_cfg_sme(fw, i, idx) \
>>         for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i)
>>
>> +struct cbinfo {
>> +       struct arm_smmu_domain          *domain;
>> +       struct io_pgtable_cfg           pgtbl_cfg;
>> +};
>> +
>>  struct arm_smmu_device {
>>         struct device                   *dev;
>>
>> @@ -378,6 +383,9 @@ struct arm_smmu_device {
>>         struct clk                      **clocks;
>>
>>         u32                             cavium_id_base; /* Specific to Cavium */
>> +
>> +       /* For save/restore of context bank registers */
>> +       struct cbinfo                   *cb_saved_ctx;
>
>It's not that clear to me this will become an array - better
>documentation would help reviewing the code.

   ok, will add the doc for this.

>
>>  };
>>
>>  enum arm_smmu_context_fmt {
>> @@ -972,6 +980,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>
>>         /* Initialise the context bank with our page table cfg */
>>         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> +       smmu->cb_saved_ctx[cfg->cbndx].domain = smmu_domain;
>> +       smmu->cb_saved_ctx[cfg->cbndx].pgtbl_cfg = pgtbl_cfg;
>>
>>         /*
>>          * Request context fault interrupt. Do this last to avoid the
>> @@ -1861,6 +1871,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>         }
>>         dev_notice(smmu->dev, "\t%u context banks (%u stage-2 only)\n",
>>                    smmu->num_context_banks, smmu->num_s2_context_banks);
>> +
>> +       smmu->cb_saved_ctx = devm_kzalloc(smmu->dev,
>> +                            sizeof(struct cbinfo) * smmu->num_context_banks,
>> +                            GFP_KERNEL);
>>         /*
>>          * Cavium CN88xx erratum #27704.
>>          * Ensure ASID and VMID allocation is unique across all SMMUs in
>> @@ -2115,8 +2129,44 @@ static int arm_smmu_resume(struct device *dev)
>>  {
>>         struct platform_device *pdev = to_platform_device(dev);
>>         struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
>> +       struct arm_smmu_domain *domain = NULL;
>> +       struct io_pgtable_cfg *pgtbl_cfg = NULL;
>> +       struct arm_smmu_smr *smrs = smmu->smrs;
>> +       int i = 0, idx, cb, ret, pcb = 0;
>> +
>> +       ret = arm_smmu_enable_clocks(smmu);
>> +       if (ret)
>> +               return ret;
>> +
>> +       arm_smmu_device_reset(smmu);
>>
>> -       return arm_smmu_enable_clocks(smmu);
>> +       /* Restore the smes and the context banks */
>> +       for (idx = 0; idx < smmu->num_mapping_groups; ++idx) {
>> +               mutex_lock(&smmu->stream_map_mutex);
>> +               if (!smrs[idx].valid) {
>> +                       mutex_unlock(&smmu->stream_map_mutex);
>> +                       continue;
>> +               }
>> +
>> +               arm_smmu_write_sme(smmu, idx);
>> +               cb = smmu->s2crs[idx].cbndx;
>> +               mutex_unlock(&smmu->stream_map_mutex);
>> +
>> +               if (!i || (cb != pcb)) {
>> +                       domain = smmu->cb_saved_ctx[cb].domain;
>> +                       pgtbl_cfg = &smmu->cb_saved_ctx[cb].pgtbl_cfg;
>> +
>> +                       if (domain) {
>> +                               mutex_lock(&domain->init_mutex);
>> +                               arm_smmu_init_context_bank(domain, pgtbl_cfg);
>> +                               mutex_unlock(&domain->init_mutex);
>> +                       }
>> +               }
>> +               pcb = cb;
>> +               i++;
>
>What are you doing with variable 'i'?  Again, some comments would
>greatly help with reviewing.

       hmm, i was using  variable 'i' to pass through the 'if' first time
       even if cb != pcb, because pcb is uninitialized at that point.
        But i could just initialize 'pcb' with some -EINVAL and then
      avoid the extra 'i' itself.  Also check for cb != pcb was required
      because same context bank could be populated multiple times
      for different sids in sequence.  I will document these and send V2.
  
      Thanks for your time to review this.

Regards,
 Sricharan

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

[Marek Szyprowski]

Hi Sricharan,


On 2016-10-21 19:14, Sricharan R wrote:
> The device link between master and its smmu is added so that
> the smmu gets runtime enabled/disabled when the master needs it.
> This is done from add_device callback which gets called once
> when the master is added to the smmu group.
>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>   drivers/iommu/arm-smmu.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 578cdc2..71ce4b6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1470,6 +1470,15 @@ static int arm_smmu_add_device(struct device *dev)
>   		goto out_free;
>   	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.
> +	 */
> +
> +	device_link_add(dev, smmu->dev, DEVICE_LINK_AVAILABLE,
> +			DEVICE_LINK_PM_RUNTIME);

Please update to the latest version of Rafael's patches. In V5 the 
initial link
state is not needed anymore and there was an important fix for creating 
links
during master's driver probing, what happens after applying your IOMMU 
deferred
probe patchset.

> +
>   	return 0;
>   
>   out_free:

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Sricharan]

Hi Marek,

>
>On 2016-10-21 19:14, Sricharan R wrote:
>> The device link between master and its smmu is added so that
>> the smmu gets runtime enabled/disabled when the master needs it.
>> This is done from add_device callback which gets called once
>> when the master is added to the smmu group.
>>
>> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 578cdc2..71ce4b6 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1470,6 +1470,15 @@ static int arm_smmu_add_device(struct device *dev)
>>   		goto out_free;
>>   	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.
>> +	 */
>> +
>> +	device_link_add(dev, smmu->dev, DEVICE_LINK_AVAILABLE,
>> +			DEVICE_LINK_PM_RUNTIME);
>
>Please update to the latest version of Rafael's patches. In V5 the
>initial link
>state is not needed anymore and there was an important fix for creating
>links
>during master's driver probing, what happens after applying your IOMMU
>deferred
>probe patchset.

   Sure i will update to V5. I see that i am on V4.

Regards,
 Sricharan

Re: [PATCH 3/4] iommu/arm-smmu: Add context save restore support

[Robin Murphy]

On 21/10/16 18:14, Sricharan R wrote:
> The smes registers and the context bank registers are
> the ones that are needs to be saved and restored.
> Fortunately the smes are already stored as a part

"Fortunately"? Hey, that's by design! :P

I was actually planning to finish off suspend/resume beyond those
foundations laid by the big rework, so it's nice to see something already.

> of the smmu device structure. So just write that
> back. The data required to configure the context banks
> are the master's domain data and pgtable cfgs.
> So store them as a part of the context banks info
> and just reconfigure the context banks on the restore
> path.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 45f2762..578cdc2 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -328,6 +328,11 @@ struct arm_smmu_master_cfg {
>  #define for_each_cfg_sme(fw, i, idx) \
>  	for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i)
>  
> +struct cbinfo {
> +	struct arm_smmu_domain		*domain;
> +	struct io_pgtable_cfg		pgtbl_cfg;
> +};
> +
>  struct arm_smmu_device {
>  	struct device			*dev;
>  
> @@ -378,6 +383,9 @@ struct arm_smmu_device {
>  	struct clk                      **clocks;
>  
>  	u32				cavium_id_base; /* Specific to Cavium */
> +
> +	/* For save/restore of context bank registers */
> +	struct cbinfo                   *cb_saved_ctx;

I'd prefer to follow the same pattern for context banks as for the
stream map registers, i.e. keep just the smmu pointer and cbndx in the
arm_smmu_domain, and move the rest of the context bank state to the
arm_smmu_device. Yes, it requires rather more refactoring, but it's
going to result in cleaner and more obvious code in the end.

>  };
>  
>  enum arm_smmu_context_fmt {
> @@ -972,6 +980,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  
>  	/* Initialise the context bank with our page table cfg */
>  	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> +	smmu->cb_saved_ctx[cfg->cbndx].domain = smmu_domain;
> +	smmu->cb_saved_ctx[cfg->cbndx].pgtbl_cfg = pgtbl_cfg;
>  
>  	/*
>  	 * Request context fault interrupt. Do this last to avoid the
> @@ -1861,6 +1871,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	}
>  	dev_notice(smmu->dev, "\t%u context banks (%u stage-2 only)\n",
>  		   smmu->num_context_banks, smmu->num_s2_context_banks);
> +
> +	smmu->cb_saved_ctx = devm_kzalloc(smmu->dev,
> +			     sizeof(struct cbinfo) * smmu->num_context_banks,
> +			     GFP_KERNEL);
>  	/*
>  	 * Cavium CN88xx erratum #27704.
>  	 * Ensure ASID and VMID allocation is unique across all SMMUs in
> @@ -2115,8 +2129,44 @@ static int arm_smmu_resume(struct device *dev)
>  {
[...]
  	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
[...]
	int ret = arm_smmu_enable_clocks(smmu);
> +	if (ret)
> +		return ret;
> +
> +	arm_smmu_device_reset(smmu);

For reference, what I'm aiming for is for that to be the entire resume
function.

> -	return arm_smmu_enable_clocks(smmu);
> +	/* Restore the smes and the context banks */
> +	for (idx = 0; idx < smmu->num_mapping_groups; ++idx) {
> +		mutex_lock(&smmu->stream_map_mutex);

What are we taking this lock to protect against - can we resume one SMMU
multiple times in parallel?

> +		if (!smrs[idx].valid) {
> +			mutex_unlock(&smmu->stream_map_mutex);
> +			continue;

Nope, you can't skip anything, because there's no guarantee the SMRs (or
anything else) which are invalid in the saved state are also invalid in
the current hardware state. Consider the case of restoring from hibernate.

> +		}
> +
> +		arm_smmu_write_sme(smmu, idx);

arm_smmu_device_reset() already did that.

> +		cb = smmu->s2crs[idx].cbndx;
> +		mutex_unlock(&smmu->stream_map_mutex);
> +
> +		if (!i || (cb != pcb)) {

This took a while to figure out, and it seems like an unnecessary
micro-optimisation since it still doesn't actually prevent initialising
the same context bank multiple times (there's no guarantee that all
s2crs targeting that context are consecutive).

> +			domain = smmu->cb_saved_ctx[cb].domain;
> +			pgtbl_cfg = &smmu->cb_saved_ctx[cb].pgtbl_cfg;
> +
> +			if (domain) {
> +				mutex_lock(&domain->init_mutex);

Assuming the device link stuff works as I would expect it to (I've not
looked in detail), then shouldn't the SMMU be resumed before any of its
masters are, so what are we taking this lock to protect against?

Robin.

> +				arm_smmu_init_context_bank(domain, pgtbl_cfg);
> +				mutex_unlock(&domain->init_mutex);
> +			}
> +		}
> +		pcb = cb;
> +		i++;
> +	}
> +
> +	return 0;
>  }
>  
>  static int arm_smmu_suspend(struct device *dev)
>