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

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

[Sricharan R]

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 symmu's
runtime to follow the master's runtime pm, so the smmu remains
powered only when the masters use it.

Took some reference from the exynos runtime patches [2].
Tested this with MDP, GPU, VENUS devices on apq8096-db820c board.

Previous version of the patchset [1].

[V2]
   * Split the patches little differently.

   * Addressed comments.

   * Removed the patch #4 [3] 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://www.spinics.net/lists/linux-arm-msm/msg23870.html
[2] https://lkml.org/lkml/2016/10/20/70
[3] https://patchwork.kernel.org/patch/9389717/

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

 .../devicetree/bindings/iommu/arm,smmu.txt         |  16 +++
 drivers/iommu/arm-smmu.c                           | 143 ++++++++++++++++++++-
 2 files changed, 158 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 V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan R]

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>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 16 ++++
 drivers/iommu/arm-smmu.c                           | 97 ++++++++++++++++++++++
 2 files changed, 113 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e862d148..2376828 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -60,6 +60,18 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+- 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:  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) :
@@ -84,6 +96,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 */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 091d4a6..cd2e3db 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>
 
@@ -387,6 +388,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 */
 };
@@ -444,6 +447,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 = smmu->num_clocks;
+
+	while (i--)
+		clk_disable_unprepare(smmu->clocks[i]);
+}
+
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
 	int i = 0;
@@ -1740,6 +1768,43 @@ 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 = 0;
+	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;
+	}
+
+	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;
@@ -2121,6 +2186,11 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		smmu->irqs[i] = irq;
 	}
 
+	err = arm_smmu_init_clocks(smmu);
+	if (err)
+		return err;
+
+	pm_runtime_enable(dev);
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -2182,10 +2252,37 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	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_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 V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

Hi,

On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
> +- 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.

Which SMMU implementations are those clock-names valid for?

The SMMU architecture specifications do not architect the clocks, which
are implemementation-specific.

AFAICT, this doesn't match MMU-400 or MMU-500.

> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
> +{
> +	const char *cname;
> +	struct property *prop;
> +	int i = 0;
> +	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;
> +	}
> +
> +	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;
> +}

I am very much not a fan of grabbing hold of resources that don't
necessarily match the binding, and we likely don't understand the use
of.

Either we know the names, and can manage them, or we don't, and cannot.

Thanks,
Mark.

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan]

Hi Mark,

>
>On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
>> +- 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.
>
>Which SMMU implementations are those clock-names valid for?
>
>The SMMU architecture specifications do not architect the clocks, which
>are implemementation-specific.
>
>AFAICT, this doesn't match MMU-400 or MMU-500.

Ok, should be more specific. Infact QCOM has MMU-500 and also
a smmu v2 implementation which is fully compatible with
"arm,smmu-v2", with the clocks being controlled by the soc's
clock controller. i was trying to define these clock bindings
so that its works across socs. So there are one or more interface
clocks which are required for the smmu's interface or the configuration
access and one or more clocks required for smmu's downstream
bus access. That was the reason i was trying to iterate over the
list of clocks down below.  But agree that the bindings should define
each of the clocks required separately.

So one way here is, define a separate compatible for QCOM's SMMU
implementation and define all the clock bindings as a part of it
and handle it in the same way in the driver. But just thinking if
it would scale well for any other soc that is compatible with
arm,smmu-v2 driver and wants to handle clocks in the future ?
 
Regards,
 Sricharan


>
>> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
>> +{
>> +	const char *cname;
>> +	struct property *prop;
>> +	int i = 0;
>> +	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;
>> +	}
>> +
>> +	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;
>> +}
>
>I am very much not a fan of grabbing hold of resources that don't
>necessarily match the binding, and we likely don't understand the use
>of.
>
>Either we know the names, and can manage them, or we don't, and cannot.

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

On Wed, Feb 08, 2017 at 04:23:17PM +0530, Sricharan wrote:
> >On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
> >> +- 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.
> >
> >Which SMMU implementations are those clock-names valid for?
> >
> >The SMMU architecture specifications do not architect the clocks, which
> >are implemementation-specific.
> >
> >AFAICT, this doesn't match MMU-400 or MMU-500.
> 
> Ok, should be more specific. Infact QCOM has MMU-500 and also
> a smmu v2 implementation which is fully compatible with
> "arm,smmu-v2", with the clocks being controlled by the soc's
> clock controller. i was trying to define these clock bindings
> so that its works across socs.

I don't think we can do that, if we don't know precisely what those
clocks are used for.

i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
would imply the set of clocks.

> So there are one or more interface clocks which are required for the
> smmu's interface or the configuration access and one or more clocks
> required for smmu's downstream bus access. That was the reason i was
> trying to iterate over the list of clocks down below.  But agree that
> the bindings should define each of the clocks required separately.

As above, I don't think the code should do this. It should only touch
the clocks it knows about.

> So one way here is, define a separate compatible for QCOM's SMMU
> implementation and define all the clock bindings as a part of it
> and handle it in the same way in the driver.

That would be my preference.

> But just thinking if it would scale well for any other soc that is
> compatible with arm,smmu-v2 driver and wants to handle clocks in the
> future ?

I don't think we can have our cake and eat it here. Either we handle the
clock management for each variant, or we don't do it at all. We have no
idea what requirements a future variant might have w.r.t. the management
of clocks we don't know about yet.

Thanks,
Mark.

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan]

Hi Mark,

>On Wed, Feb 08, 2017 at 04:23:17PM +0530, Sricharan wrote:
>> >On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
>> >> +- 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.
>> >
>> >Which SMMU implementations are those clock-names valid for?
>> >
>> >The SMMU architecture specifications do not architect the clocks, which
>> >are implemementation-specific.
>> >
>> >AFAICT, this doesn't match MMU-400 or MMU-500.
>>
>> Ok, should be more specific. Infact QCOM has MMU-500 and also
>> a smmu v2 implementation which is fully compatible with
>> "arm,smmu-v2", with the clocks being controlled by the soc's
>> clock controller. i was trying to define these clock bindings
>> so that its works across socs.
>
>I don't think we can do that, if we don't know precisely what those
>clocks are used for.
>
>i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
>would imply the set of clocks.
>

Ok, this was what i was trying to do for V3 and will actually put it
this way.

>> So there are one or more interface clocks which are required for the
>> smmu's interface or the configuration access and one or more clocks
>> required for smmu's downstream bus access. That was the reason i was
>> trying to iterate over the list of clocks down below.  But agree that
>> the bindings should define each of the clocks required separately.
>
>As above, I don't think the code should do this. It should only touch
>the clocks it knows about.
>

ok, after defining QCOM specific SMMU bindings, this would be become 
handling clocks specific to QCOM implementation as per its clock
bindings, which as i understand is what you suggest.

>> So one way here is, define a separate compatible for QCOM's SMMU
>> implementation and define all the clock bindings as a part of it
>> and handle it in the same way in the driver.
>
>That would be my preference.
>

ok.

>> But just thinking if it would scale well for any other soc that is
>> compatible with arm,smmu-v2 driver and wants to handle clocks in the
>> future ?
>
>I don't think we can have our cake and eat it here. Either we handle the
>clock management for each variant, or we don't do it at all. We have no
>idea what requirements a future variant might have w.r.t. the management
>of clocks we don't know about yet.
>

Right, at this point, this is first soc which adds the clocks in to the driver.
Feels if the clocks are initialized and enabled/disabled as a part of some
implementation specific callbacks, that would help always because that is
the part which is going to different for each implementation and patches 2,3
would remain common. Finally, as you have suggested will introduce new
SMMU binding in the case of QCOM and will try to handle clocks specifically for that
implementation and see how it looks.

Regards,
 Sricharan

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Robin Murphy]

On 08/02/17 12:30, Sricharan wrote:
> Hi Mark,
> 
>> On Wed, Feb 08, 2017 at 04:23:17PM +0530, Sricharan wrote:
>>>> On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
>>>>> +- 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.
>>>>
>>>> Which SMMU implementations are those clock-names valid for?
>>>>
>>>> The SMMU architecture specifications do not architect the clocks, which
>>>> are implemementation-specific.
>>>>
>>>> AFAICT, this doesn't match MMU-400 or MMU-500.
>>>
>>> Ok, should be more specific. Infact QCOM has MMU-500 and also
>>> a smmu v2 implementation which is fully compatible with
>>> "arm,smmu-v2", with the clocks being controlled by the soc's
>>> clock controller. i was trying to define these clock bindings
>>> so that its works across socs.
>>
>> I don't think we can do that, if we don't know precisely what those
>> clocks are used for.
>>
>> i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
>> would imply the set of clocks.
>>
> 
> Ok, this was what i was trying to do for V3 and will actually put it
> this way.

Clocks are not architectural, so it only makes sense to associate them
with an implementation-specific compatible string. There's also no
guarantee that different microarchitectures have equivalent internal
clock domains - I'm not sure if "the SMMU's underlying bus access" is
meant to refer to accesses *by* the SMMU, i.e. page table walks,
accesses *through* the SMMU by upstream masters, or both, and the
differences are rather significant. I'd also note that an MMU-500
configuration may have up to *33* clocks.

>>> So there are one or more interface clocks which are required for the
>>> smmu's interface or the configuration access and one or more clocks
>>> required for smmu's downstream bus access. That was the reason i was
>>> trying to iterate over the list of clocks down below.  But agree that
>>> the bindings should define each of the clocks required separately.
>>
>> As above, I don't think the code should do this. It should only touch
>> the clocks it knows about.
>>
> 
> ok, after defining QCOM specific SMMU bindings, this would be become 
> handling clocks specific to QCOM implementation as per its clock
> bindings, which as i understand is what you suggest.
> 
>>> So one way here is, define a separate compatible for QCOM's SMMU
>>> implementation and define all the clock bindings as a part of it
>>> and handle it in the same way in the driver.
>>
>> That would be my preference.
>>
> 
> ok.

Either way, the QCOM implementation deserves its own compatible if only
for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
behaviour WRT to IRQs as touched upon in the other thread).

Robin.

>>> But just thinking if it would scale well for any other soc that is
>>> compatible with arm,smmu-v2 driver and wants to handle clocks in the
>>> future ?
>>
>> I don't think we can have our cake and eat it here. Either we handle the
>> clock management for each variant, or we don't do it at all. We have no
>> idea what requirements a future variant might have w.r.t. the management
>> of clocks we don't know about yet.
>>
> 
> Right, at this point, this is first soc which adds the clocks in to the driver.
> Feels if the clocks are initialized and enabled/disabled as a part of some
> implementation specific callbacks, that would help always because that is
> the part which is going to different for each implementation and patches 2,3
> would remain common. Finally, as you have suggested will introduce new
> SMMU binding in the case of QCOM and will try to handle clocks specifically for that
> implementation and see how it looks.
> 
> Regards,
>  Sricharan
> 

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan]

Hi Robin,

>>>>> On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
>>>>>> +- 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.
>>>>>
>>>>> Which SMMU implementations are those clock-names valid for?
>>>>>
>>>>> The SMMU architecture specifications do not architect the clocks, which
>>>>> are implemementation-specific.
>>>>>
>>>>> AFAICT, this doesn't match MMU-400 or MMU-500.
>>>>
>>>> Ok, should be more specific. Infact QCOM has MMU-500 and also
>>>> a smmu v2 implementation which is fully compatible with
>>>> "arm,smmu-v2", with the clocks being controlled by the soc's
>>>> clock controller. i was trying to define these clock bindings
>>>> so that its works across socs.
>>>
>>> I don't think we can do that, if we don't know precisely what those
>>> clocks are used for.
>>>
>>> i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
>>> would imply the set of clocks.
>>>
>>
>> Ok, this was what i was trying to do for V3 and will actually put it
>> this way.
>
>Clocks are not architectural, so it only makes sense to associate them
>with an implementation-specific compatible string. There's also no

ok, it for this the QCOM specific implementation binding is tried(going to).

>guarantee that different microarchitectures have equivalent internal
>clock domains - I'm not sure if "the SMMU's underlying bus access" is
>meant to refer to accesses *by* the SMMU, i.e. page table walks,
>accesses *through* the SMMU by upstream masters, or both

In the above QCOM case, it is actually both. Its the same path for both the
page table walker and upstream masters.

>differences are rather significant. I'd also note that an MMU-500
>configuration may have up to *33* clocks.
>
>Either way, the QCOM implementation deserves its own compatible if only
>for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
>behaviour WRT to IRQs as touched upon in the other thread).
>

Ok, slightly unclear, so you mean then *clocks* are not good enough reason
to have a new compatible ?

Regards,
 Sricharan





>Robin.
>
>>>> But just thinking if it would scale well for any other soc that is
>>>> compatible with arm,smmu-v2 driver and wants to handle clocks in the
>>>> future ?
>>>
>>> I don't think we can have our cake and eat it here. Either we handle the
>>> clock management for each variant, or we don't do it at all. We have no
>>> idea what requirements a future variant might have w.r.t. the management
>>> of clocks we don't know about yet.
>>>
>>
>> Right, at this point, this is first soc which adds the clocks in to the driver.
>> Feels if the clocks are initialized and enabled/disabled as a part of some
>> implementation specific callbacks, that would help always because that is
>> the part which is going to different for each implementation and patches 2,3
>> would remain common. Finally, as you have suggested will introduce new
>> SMMU binding in the case of QCOM and will try to handle clocks specifically for that
>> implementation and see how it looks.
>>
>> Regards,
>>  Sricharan

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

On Wed, Feb 08, 2017 at 07:15:37PM +0530, Sricharan wrote:
> >Clocks are not architectural, so it only makes sense to associate them
> >with an implementation-specific compatible string. There's also no
> 
> ok, it for this the QCOM specific implementation binding is tried(going to).
> 
> >guarantee that different microarchitectures have equivalent internal
> >clock domains - I'm not sure if "the SMMU's underlying bus access" is
> >meant to refer to accesses *by* the SMMU, i.e. page table walks,
> >accesses *through* the SMMU by upstream masters, or both
> 
> In the above QCOM case, it is actually both. Its the same path for both the
> page table walker and upstream masters.
> 
> >differences are rather significant. I'd also note that an MMU-500
> >configuration may have up to *33* clocks.
> >
> >Either way, the QCOM implementation deserves its own compatible if only
> >for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
> >behaviour WRT to IRQs as touched upon in the other thread).
> >
> 
> Ok, slightly unclear, so you mean then *clocks* are not good enough reason
> to have a new compatible ?

I beleive Robin's point was even if the clocks didn't matter, there are
other reasons we should have the QCOM-specific compatible string.

So we should have one regardless.

Thanks,
Mark.

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Robin Murphy]

On 08/02/17 13:52, Mark Rutland wrote:
> On Wed, Feb 08, 2017 at 07:15:37PM +0530, Sricharan wrote:
>>> Clocks are not architectural, so it only makes sense to associate them
>>> with an implementation-specific compatible string. There's also no
>>
>> ok, it for this the QCOM specific implementation binding is tried(going to).
>>
>>> guarantee that different microarchitectures have equivalent internal
>>> clock domains - I'm not sure if "the SMMU's underlying bus access" is
>>> meant to refer to accesses *by* the SMMU, i.e. page table walks,
>>> accesses *through* the SMMU by upstream masters, or both
>>
>> In the above QCOM case, it is actually both. Its the same path for both the
>> page table walker and upstream masters.

Right, that's what I feared. As far as I can make out the current ARM
implementations, transactions passing through will require at least
TBUn_BCLK for the appropriate TBU, but would also need the page table
walker clocked with CCLK to resolve TLB misses. But then the programming
interface is also in the CCLK domain (not counting the incoming APB or
AXI clock for the actual slave port itself). Thus this 'generic' clock
binding already isn't compatible with MMU-40x/500.

>>> differences are rather significant. I'd also note that an MMU-500
>>> configuration may have up to *33* clocks.
>>>
>>> Either way, the QCOM implementation deserves its own compatible if only
>>> for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
>>> behaviour WRT to IRQs as touched upon in the other thread).
>>>
>>
>> Ok, slightly unclear, so you mean then *clocks* are not good enough reason
>> to have a new compatible ?
> 
> I beleive Robin's point was even if the clocks didn't matter, there are
> other reasons we should have the QCOM-specific compatible string.
> 
> So we should have one regardless.

Exactly.

Robin.

> 
> Thanks,
> Mark.
> 

[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Sricharan]

Hi Robin,

>-----Original Message-----
>From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Robin Murphy
>Sent: Wednesday, February 08, 2017 8:01 PM
>To: Mark Rutland <mark.rutland@arm.com>; Sricharan <sricharan@codeaurora.org>
>Cc: devicetree at vger.kernel.org; mathieu.poirier at linaro.org; linux-arm-msm at vger.kernel.org; joro at 8bytes.org;
>will.deacon at arm.com; iommu at lists.linux-foundation.org; robh+dt at kernel.org; sboyd at codeaurora.org; linux-arm-
>kernel at lists.infradead.org; m.szyprowski at samsung.com
>Subject: Re: [PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops
>
>On 08/02/17 13:52, Mark Rutland wrote:
>> On Wed, Feb 08, 2017 at 07:15:37PM +0530, Sricharan wrote:
>>>> Clocks are not architectural, so it only makes sense to associate them
>>>> with an implementation-specific compatible string. There's also no
>>>
>>> ok, it for this the QCOM specific implementation binding is tried(going to).
>>>
>>>> guarantee that different microarchitectures have equivalent internal
>>>> clock domains - I'm not sure if "the SMMU's underlying bus access" is
>>>> meant to refer to accesses *by* the SMMU, i.e. page table walks,
>>>> accesses *through* the SMMU by upstream masters, or both
>>>
>>> In the above QCOM case, it is actually both. Its the same path for both the
>>> page table walker and upstream masters.
>
>Right, that's what I feared. As far as I can make out the current ARM
>implementations, transactions passing through will require at least
>TBUn_BCLK for the appropriate TBU, but would also need the page table
>walker clocked with CCLK to resolve TLB misses. But then the programming
>interface is also in the CCLK domain (not counting the incoming APB or
>AXI clock for the actual slave port itself). Thus this 'generic' clock
>binding already isn't compatible with MMU-40x/500.
>

Right, this implementation's clock bindings are not going to compatible with
MMU-500. There is also another soc which integrates MMU-500.  So
will have to add the clock bindings for MMU-500 as well separately.
Also in MMU-500 i saw that there is a possibility where the clock-domain
can be shared between the TCU logic, programming interface and PTW read
channel.  Does this mean that the TCU-clock has to be 'ON' for both register
access and PTW, similar to above ?. So for MMU-500 clock bindings there
can be a CFG_CLK (optional and not required in shared case), TBUn_CLK and
TCU_CLK

Regards,
 Sricharan

>>>> differences are rather significant. I'd also note that an MMU-500
>>>> configuration may have up to *33* clocks.
>>>>
>>>> Either way, the QCOM implementation deserves its own compatible if only
>>>> for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
>>>> behaviour WRT to IRQs as touched upon in the other thread).
>>>>
>>>
>>> Ok, slightly unclear, so you mean then *clocks* are not good enough reason
>>> to have a new compatible ?
>>
>> I beleive Robin's point was even if the clocks didn't matter, there are
>> other reasons we should have the QCOM-specific compatible string.
>>
>> So we should have one regardless.
>
>Exactly.
>
>Robin.
>
>>
>> Thanks,
>> Mark.
>>
>
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel at lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sricharan R]

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>
---
 drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index cd2e3db..5170523 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1021,11 +1021,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	void __iomem *cb_base;
-	int irq;
+	int irq, ret;
 
 	if (!smmu)
 		return;
 
+	ret = pm_runtime_get_sync(smmu->dev);
+	if (ret)
+		dev_warn(smmu->dev, "runtime resume failed");
+
 	/*
 	 * Disable the context bank and free the page tables before freeing
 	 * it.
@@ -1040,6 +1044,10 @@ 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);
+
+	ret = pm_runtime_put_sync(smmu->dev);
+	if (ret)
+		dev_warn(smmu->dev, "runtime suspend failed");
 }
 
 static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
@@ -1508,10 +1516,18 @@ static int arm_smmu_add_device(struct device *dev)
 	while (i--)
 		cfg->smendx[i] = INVALID_SMENDX;
 
+	ret = pm_runtime_get_sync(smmu->dev);
+	if (ret)
+		goto out_free;
+
 	ret = arm_smmu_master_alloc_smes(dev);
 	if (ret)
 		goto out_free;
 
+	ret = pm_runtime_put_sync(smmu->dev);
+	if (ret)
+		goto out_free;
+
 	return 0;
 
 out_free:
@@ -1524,11 +1540,27 @@ static int arm_smmu_add_device(struct device *dev)
 static void arm_smmu_remove_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct arm_smmu_device *smmu = fwspec_smmu(fwspec);
+	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops)
 		return;
 
+	/*
+	 * 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)
+		dev_warn(smmu->dev, "runtime resume failed");
+
 	arm_smmu_master_free_smes(fwspec);
+
+	ret = pm_runtime_put_sync(smmu->dev);
+	if (ret)
+		dev_warn(smmu->dev, "runtime suspend failed");
 	iommu_group_remove_device(dev);
 	kfree(fwspec->iommu_priv);
 	iommu_fwspec_free(dev);
@@ -2190,7 +2222,12 @@ 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;
@@ -2217,9 +2254,11 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 
 	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
-	platform_set_drvdata(pdev, smmu);
 	arm_smmu_device_reset(smmu);
 	arm_smmu_test_smr_masks(smmu);
+	err = pm_runtime_put_sync(dev);
+	if (err)
+		return err;
 
 	/* Oh, for a proper bus abstraction */
 	if (!iommu_present(&platform_bus_type))
@@ -2249,6 +2288,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 V2 3/3] iommu/arm-smmu: Add the device_link between masters and smmu

[Sricharan R]

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 5170523..db4bb3c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1475,6 +1475,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 = NULL;
 	int i, ret;
 
 	if (using_legacy_binding) {
@@ -1528,6 +1529,16 @@ static int arm_smmu_add_device(struct device *dev)
 	if (ret)
 		goto out_free;
 
+	/*
+	 * 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_free:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation