iommu/arm-smmu: misc features, new DT bindings

On some platforms with tight power constraints it is polite to only leave your clocks on for as long as you absolutely need them. Currently we assume that all clocks necessary for SMMU register access are always on.

Add some optional device tree properties to specify any clocks that are necessary for SMMU register access and turn them on and off as needed.

If no clocks are specified in the device tree things continue to work the way they always have: we assume all necessary clocks are always turned on.

Signed-off-by Mitchel Humpherys <[email protected]>

---
 .../devicetree/bindings/iommu/arm,smmu.txt         |  11 ++
 drivers/iommu/arm-smmu.c                           | 127 +++++++++++++++++++--
 2 files changed, 129 insertions(+), 9 deletions(-)

drivers/iommu/arm-smmu.c +36 -1

-static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
+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)
+		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 -ENODEV;
+		}
+
+		if (clk_get_rate(c) == 0) {
+			long rate = clk_round_rate(c, 1000);
+			clk_set_rate(c, rate);

On some platforms with tight power constraints it is polite to only leave your clocks on for as long as you absolutely need them. Currently we assume that all clocks necessary for SMMU register access are always on.

Add some optional device tree properties to specify any clocks that are necessary for SMMU register access and turn them on and off as needed.

If no clocks are specified in the device tree things continue to work the way they always have: we assume all necessary clocks are always turned on.

drivers/iommu/arm-smmu.c +14 -0

+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;
+		}
+	}
+

drivers/iommu/arm-smmu.c +9 -2

+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);

@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	dev_notice(dev, "registered %d master devices\n", i);
 
-	err = arm_smmu_device_cfg_probe(smmu);
+	err = arm_smmu_init_clocks(smmu);
 	if (err)
 		goto out_put_masters;
 
+	arm_smmu_enable_clocks(smmu);
+
+	err = arm_smmu_device_cfg_probe(smmu);
+	if (err)
+		goto out_disable_clocks;
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > 1 &&

@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			"found only %d context interrupt(s) but %d required\n",
 			smmu->num_context_irqs, smmu->num_context_banks);
 		err = -ENODEV;
-		goto out_put_masters;
+		goto out_disable_clocks;
 	}
 
 	for (i = 0; i < smmu->num_global_irqs; ++i) {

Quoted 5 lines · Mitchel Humpherys

On some platforms with tight power constraints it is polite to only leave your clocks on for as long as you absolutely need them. Currently we assume that all clocks necessary for SMMU register access are always on.

Quoted 6 lines

Add some optional device tree properties to specify any clocks that are necessary for SMMU register access and turn them on and off as needed.

If no clocks are specified in the device tree things continue to work the way they always have: we assume all necessary clocks are always turned on.

How does this interact with an SMMU in bypass mode?

[...]

Quoted 20 lines

drivers/iommu/arm-smmu.c +20 -0

+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;

drivers/iommu/arm-smmu.c static void __arm_smmu_free_bitmap(unsigned long *map, int idx) · line 618

Quoted 4 lines

drivers/iommu/arm-smmu.c +4 -0

+
+	for (i = 0; i < smmu->num_clocks; ++i)
+		clk_disable_unprepare(smmu->clocks[i]);
+}

What stops theses from racing with each other when there are multiple clocks? I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

drivers/iommu/arm-smmu.c static irqreturn_t arm_smmu_context_fault(int irq, void *dev) · line 675

Quoted 8 lines

/* Wait for any pending TLB invalidations to complete */ static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu) {

drivers/iommu/arm-smmu.c +1 -0

@@ -644,11 +672,15 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *cb_base;
 
+	arm_smmu_enable_clocks(smmu);

How can I get a context interrupt from an SMMU without its clocks enabled?

[...]

Quoted 34 lines

drivers/iommu/arm-smmu.c +9 -2

+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);

@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	dev_notice(dev, "registered %d master devices\n", i);
 
-	err = arm_smmu_device_cfg_probe(smmu);
+	err = arm_smmu_init_clocks(smmu);
 	if (err)
 		goto out_put_masters;
 
+	arm_smmu_enable_clocks(smmu);
+
+	err = arm_smmu_device_cfg_probe(smmu);
+	if (err)
+		goto out_disable_clocks;
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > 1 &&

@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			"found only %d context interrupt(s) but %d required\n",
 			smmu->num_context_irqs, smmu->num_context_banks);
 		err = -ENODEV;
-		goto out_put_masters;
+		goto out_disable_clocks;
 	}
 
 	for (i = 0; i < smmu->num_global_irqs; ++i) {

@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_unlock(&arm_smmu_devices_lock);
 

Review static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2167

Quoted 2 lines

(quoted diff) +1 -0

 	arm_smmu_device_reset(smmu);
+	arm_smmu_disable_clocks(smmu);

I wonder if this is really the right thing to do. Rather than the fine-grained clock enable/disable you have, why don't we just enable in domain_init and disable in domain_destroy, with refcounting for the clocks?

Hi Will,

Quoted 6 lines · Mitchel Humpherys

Quoted 5 lines

On some platforms with tight power constraints it is polite to only leave your clocks on for as long as you absolutely need them. Currently we assume that all clocks necessary for SMMU register access are always on.

Quoted 8 lines

Quoted 6 lines

Add some optional device tree properties to specify any clocks that are necessary for SMMU register access and turn them on and off as needed.

If no clocks are specified in the device tree things continue to work the way they always have: we assume all necessary clocks are always turned on.

How does this interact with an SMMU in bypass mode?

Do you mean if you have a platform that requires clock and power management but we leave the SMMU in bypass (i.e. no one calls into the SMMU driver) how are the clock/power managed?

Clients of the SMMU driver are required to vote for clocks and power when they know they need to use the SMMU. However, the clock and power needed to be on for the SMMU to service bus masters aren't necessarily the same as the ones needed to read/write registers...See below.

Quoted 20 lines

Quoted 20 lines

drivers/iommu/arm-smmu.c +20 -0

+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;

Quoted 8 lines

drivers/iommu/arm-smmu.c static void __arm_smmu_free_bitmap(unsigned long *map, int idx) · line 618

Quoted 4 lines

drivers/iommu/arm-smmu.c +4 -0

+
+	for (i = 0; i < smmu->num_clocks; ++i)
+		clk_disable_unprepare(smmu->clocks[i]);
+}

What stops theses from racing with each other when there are multiple clocks? I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

All the clock APIs are reference counted yes. Not sure what you mean by racing with each other? When you call to enable a clock the call does not return until the clock is already ON (or OFF).

Quoted 34 lines

Quoted 34 lines

drivers/iommu/arm-smmu.c +9 -2

+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);

@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	dev_notice(dev, "registered %d master devices\n", i);
 
-	err = arm_smmu_device_cfg_probe(smmu);
+	err = arm_smmu_init_clocks(smmu);
 	if (err)
 		goto out_put_masters;
 
+	arm_smmu_enable_clocks(smmu);
+
+	err = arm_smmu_device_cfg_probe(smmu);
+	if (err)
+		goto out_disable_clocks;
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > 1 &&

@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			"found only %d context interrupt(s) but %d required\n",
 			smmu->num_context_irqs, smmu->num_context_banks);
 		err = -ENODEV;
-		goto out_put_masters;
+		goto out_disable_clocks;
 	}
 
 	for (i = 0; i < smmu->num_global_irqs; ++i) {

@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_unlock(&arm_smmu_devices_lock);
 

Quoted 7 lines

Review static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2167

Quoted 2 lines

(quoted diff) +1 -0

 	arm_smmu_device_reset(smmu);
+	arm_smmu_disable_clocks(smmu);

I wonder if this is really the right thing to do. Rather than the fine-grained clock enable/disable you have, why don't we just enable in domain_init and disable in domain_destroy, with refcounting for the clocks?

So the whole point of all of this is that we try to save power. As Mitch wrote in the commit text we want to only leave the clock and power on for as short period of time as possible.

[adding Mike]

Quoted 1 line · Olav Haugan

Hi Will,

Hi Olav,

Quoted 16 lines · Will Deacon

Quoted 6 lines · Mitchel Humpherys

Quoted 5 lines

On some platforms with tight power constraints it is polite to only leave your clocks on for as long as you absolutely need them. Currently we assume that all clocks necessary for SMMU register access are always on.

Quoted 8 lines

Quoted 6 lines

Add some optional device tree properties to specify any clocks that are necessary for SMMU register access and turn them on and off as needed.

If no clocks are specified in the device tree things continue to work the way they always have: we assume all necessary clocks are always turned on.

How does this interact with an SMMU in bypass mode?

Quoted 8 lines

Do you mean if you have a platform that requires clock and power management but we leave the SMMU in bypass (i.e. no one calls into the SMMU driver) how are the clock/power managed?

Clients of the SMMU driver are required to vote for clocks and power when they know they need to use the SMMU. However, the clock and power needed to be on for the SMMU to service bus masters aren't necessarily the same as the ones needed to read/write registers...See below.

The case I'm thinking of is where a device masters through the IOMMU, but doesn't make use of any translations. In this case, its transactions will bypass the SMMU and I want to ensure that continues to happen, regardless of the power state of the SMMU.

Quoted 30 lines

Quoted 29 lines

Quoted 15 lines

drivers/iommu/arm-smmu.c +14 -0

+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;
+		}
+	}
+

drivers/iommu/arm-smmu.c static void __arm_smmu_free_bitmap(unsigned long *map, int idx) · line 618

Quoted 10 lines

drivers/iommu/arm-smmu.c +10 -0

+	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]);
+}

What stops theses from racing with each other when there are multiple clocks? I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

Quoted 3 lines

All the clock APIs are reference counted yes. Not sure what you mean by racing with each other? When you call to enable a clock the call does not return until the clock is already ON (or OFF).

I was thinking of an interrupt handler racing with normal code, but actually you balance the clk enable/disable in the interrupt handlers. However, it's not safe to call these clk functions from irq context anyway, since clk_prepare may sleep.

Quoted 43 lines

Quoted 42 lines

Quoted 33 lines

drivers/iommu/arm-smmu.c +9 -2

+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);

@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	dev_notice(dev, "registered %d master devices\n", i);
 
-	err = arm_smmu_device_cfg_probe(smmu);
+	err = arm_smmu_init_clocks(smmu);
 	if (err)
 		goto out_put_masters;
 
+	arm_smmu_enable_clocks(smmu);
+
+	err = arm_smmu_device_cfg_probe(smmu);
+	if (err)
+		goto out_disable_clocks;
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > 1 &&

@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			"found only %d context interrupt(s) but %d required\n",
 			smmu->num_context_irqs, smmu->num_context_banks);
 		err = -ENODEV;
-		goto out_put_masters;
+		goto out_disable_clocks;
 	}
 
 	for (i = 0; i < smmu->num_global_irqs; ++i) {

drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2167

Quoted 4 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_unlock(&arm_smmu_devices_lock);
 
 	arm_smmu_device_reset(smmu);
+	arm_smmu_disable_clocks(smmu);

I wonder if this is really the right thing to do. Rather than the fine-grained clock enable/disable you have, why don't we just enable in domain_init and disable in domain_destroy, with refcounting for the clocks?

Quoted 3 lines

So the whole point of all of this is that we try to save power. As Mitch wrote in the commit text we want to only leave the clock and power on for as short period of time as possible.

Understood, but if the clocks are going up and down like yo-yos, then it's not obvious that you end up saving any power at all. Have you tried measuring the power consumption with different granularities for the clocks?

The code you're proposing seems to take the approach of `we're going to access registers so enable the clocks, access the registers then disable the clocks', which is simple but may not be particularly effective.

Quoted 6 lines · Will Deacon

Quoted 4 lines · Olav Haugan

Clients of the SMMU driver are required to vote for clocks and power when they know they need to use the SMMU. However, the clock and power needed to be on for the SMMU to service bus masters aren't necessarily the same as the ones needed to read/write registers...See below.

Quoted 4 lines

The case I'm thinking of is where a device masters through the IOMMU, but doesn't make use of any translations. In this case, its transactions will bypass the SMMU and I want to ensure that continues to happen, regardless of the power state of the SMMU.

Then I assume the driver for such a device wouldn't be attaching to (or detaching from) the IOMMU, so we won't be touching it at all either way. Or am I missing something?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

What stops theses from racing with each other when there are multiple clocks? I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

All the clock APIs are reference counted yes. Not sure what you mean by racing with each other? When you call to enable a clock the call does not return until the clock is already ON (or OFF).

Quoted 4 lines

I was thinking of an interrupt handler racing with normal code, but actually you balance the clk enable/disable in the interrupt handlers. However, it's not safe to call these clk functions from irq context anyway, since clk_prepare may sleep.

Ah yes. You okay with moving to a threaded IRQ?

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2167

Quoted 5 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_unlock(&arm_smmu_devices_lock);
 
 	arm_smmu_device_reset(smmu);
+	arm_smmu_disable_clocks(smmu);

I wonder if this is really the right thing to do. Rather than the fine-grained clock enable/disable you have, why don't we just enable in domain_init and disable in domain_destroy, with refcounting for the clocks?

Quoted 8 lines

Quoted 3 lines

So the whole point of all of this is that we try to save power. As Mitch wrote in the commit text we want to only leave the clock and power on for as short period of time as possible.

Understood, but if the clocks are going up and down like yo-yos, then it's not obvious that you end up saving any power at all. Have you tried measuring the power consumption with different granularities for the clocks?

This has been profiled extensively and for some use cases it's a huge win. Unfortunately we don't have any numbers for public sharing :( but you can imagine a use case where some multimedia framework maps a bunch of buffers into an SMMU at the beginning of some interactive user session and doesn't unmap them until the (human) user decides they are done. This could be a long time, all the while these clocks could be off, saving power.

Quoted 3 lines

The code you're proposing seems to take the approach of `we're going to access registers so enable the clocks, access the registers then disable the clocks', which is simple but may not be particularly effective.

Yes, that's a good summary of the approach here. It has been effective in saving power for us in the past...

Quoted 12 lines · Mitchel Humpherys

Quoted 10 lines · Will Deacon

Quoted 4 lines · Olav Haugan

Clients of the SMMU driver are required to vote for clocks and power when they know they need to use the SMMU. However, the clock and power needed to be on for the SMMU to service bus masters aren't necessarily the same as the ones needed to read/write registers...See below.

The case I'm thinking of is where a device masters through the IOMMU, but doesn't make use of any translations. In this case, its transactions will bypass the SMMU and I want to ensure that continues to happen, regardless of the power state of the SMMU.

Quoted 3 lines

Then I assume the driver for such a device wouldn't be attaching to (or detaching from) the IOMMU, so we won't be touching it at all either way. Or am I missing something?

As long as its only the register file that gets powered down, then there's no issue. However, that's making assumptions about what these clocks are controlling. Is there a way for the driver to know which aspects of the device are controlled by which clock?

Quoted 14 lines

Quoted 13 lines

Quoted 8 lines

Quoted 4 lines

What stops theses from racing with each other when there are multiple clocks? I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

All the clock APIs are reference counted yes. Not sure what you mean by racing with each other? When you call to enable a clock the call does not return until the clock is already ON (or OFF).

I was thinking of an interrupt handler racing with normal code, but actually you balance the clk enable/disable in the interrupt handlers. However, it's not safe to call these clk functions from irq context anyway, since clk_prepare may sleep.

Quoted 1 line

Ah yes. You okay with moving to a threaded IRQ?

A threaded IRQ already makes sense for context interrupts (if anybody has a platform that can do stalls properly), but it seems a bit weird for the global fault handler. Is there no way to fix the race instead?

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2167

Quoted 6 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_unlock(&arm_smmu_devices_lock);
 
 	arm_smmu_device_reset(smmu);
+	arm_smmu_disable_clocks(smmu);

I wonder if this is really the right thing to do. Rather than the fine-grained clock enable/disable you have, why don't we just enable in domain_init and disable in domain_destroy, with refcounting for the clocks?

Quoted 8 lines

Quoted 3 lines

So the whole point of all of this is that we try to save power. As Mitch wrote in the commit text we want to only leave the clock and power on for as short period of time as possible.

Understood, but if the clocks are going up and down like yo-yos, then it's not obvious that you end up saving any power at all. Have you tried measuring the power consumption with different granularities for the clocks?

Quoted 7 lines

This has been profiled extensively and for some use cases it's a huge win. Unfortunately we don't have any numbers for public sharing :( but you can imagine a use case where some multimedia framework maps a bunch of buffers into an SMMU at the beginning of some interactive user session and doesn't unmap them until the (human) user decides they are done. This could be a long time, all the while these clocks could be off, saving power.

Ok, I can see that. I wonder whether we could wrap all of the iommu_ops functions with the clock enable/disable code, instead of putting it directly into the drivers?

Quoted 7 lines

Quoted 4 lines

The code you're proposing seems to take the approach of `we're going to access registers so enable the clocks, access the registers then disable the clocks', which is simple but may not be particularly effective.

Yes, that's a good summary of the approach here. It has been effective in saving power for us in the past...

Mike, do you have any experience with this sort of stuff?

Will

Quoted 13 lines · Will Deacon

Quoted 12 lines · Mitchel Humpherys

Quoted 10 lines · Will Deacon

Quoted 4 lines · Olav Haugan

Clients of the SMMU driver are required to vote for clocks and power when they know they need to use the SMMU. However, the clock and power needed to be on for the SMMU to service bus masters aren't necessarily the same as the ones needed to read/write registers...See below.

The case I'm thinking of is where a device masters through the IOMMU, but doesn't make use of any translations. In this case, its transactions will bypass the SMMU and I want to ensure that continues to happen, regardless of the power state of the SMMU.

Quoted 8 lines

Quoted 3 lines

Then I assume the driver for such a device wouldn't be attaching to (or detaching from) the IOMMU, so we won't be touching it at all either way. Or am I missing something?

As long as its only the register file that gets powered down, then there's no issue. However, that's making assumptions about what these clocks are controlling. Is there a way for the driver to know which aspects of the device are controlled by which clock?

Yes, folks should only be putting "config" clocks here. In our system, at least, the clocks for configuring the SMMU are different than those for using it. Maybe I should make a note about what "kinds" of clocks can be specified here in the bindings (i.e. only those that can be safely disabled while still allowing translations to occur)?

I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code...

On some power-constrained platforms it's useful to disable power when a device is not in use. Add support for specifying regulators for SMMUs and only leave power on as long as the SMMU is in use (attached).

Signed-off-by Mitchel Humpherys <[email protected]>

---
 .../devicetree/bindings/iommu/arm,smmu.txt         |   3 +
 drivers/iommu/arm-smmu.c                           | 102 ++++++++++++++++++---
 2 files changed, 93 insertions(+), 12 deletions(-)

drivers/iommu/arm-smmu.c +4 -0

@@ -2124,13 +2192,19 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	err = arm_smmu_init_regulators(smmu);
+	if (err)
+		goto out_put_masters;
+
 	err = arm_smmu_init_clocks(smmu);
 	if (err)

drivers/iommu/arm-smmu.c +0 -2

@@ -2163,8 +2237,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	list_add(&smmu->list, &arm_smmu_devices);
 	spin_unlock(&arm_smmu_devices_lock);
 
-	arm_smmu_device_reset(smmu);
-	arm_smmu_disable_clocks(smmu);
 	return 0;
 
 out_free_irqs:

On some power-constrained platforms it's useful to disable power when a device is not in use. Add support for specifying regulators for SMMUs and only leave power on as long as the SMMU is in use (attached).

Currently, we provide the iommu_ops.iova_to_phys service by doing a table walk in software to translate IO virtual addresses to physical addresses. On SMMUs that support it, it can be useful to ask the SMMU itself to do the translation. This can be used to warm the TLBs for an SMMU. It can also be useful for testing and hardware validation.

Since the address translation registers are optional on SMMUv2, only enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1 and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec.

drivers/iommu/arm-smmu.c +11 -0

+static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
+					dma_addr_t iova)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+	struct device *dev = smmu->dev;
+	void __iomem *cb_base;
+	int count = 0;
+	u64 phys;
+

Quoted 5 lines · Mitchel Humpherys

Currently, we provide the iommu_ops.iova_to_phys service by doing a table walk in software to translate IO virtual addresses to physical addresses. On SMMUs that support it, it can be useful to ask the SMMU itself to do the translation. This can be used to warm the TLBs for an SMMU. It can also be useful for testing and hardware validation.

I'm not really sold on the usefulness of this feature. If you want hardware validation features, I'd rather do something through debugfs, but your use-case for warming the TLBs is intriguing. Do you have an example use-case with performance figures?

Quoted 3 lines

Since the address translation registers are optional on SMMUv2, only enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1 and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec.

[...]

Quoted 15 lines

drivers/iommu/arm-smmu.c +15 -0

+static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
+					dma_addr_t iova)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+	struct device *dev = smmu->dev;
+	void __iomem *cb_base;
+	int count = 0;
+	u64 phys;
+
+	arm_smmu_enable_clocks(smmu);
+
+	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
+

drivers/iommu/arm-smmu.c static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, · line 1720

Quoted 6 lines

drivers/iommu/arm-smmu.c +6 -0

+	if (smmu->version == 1) {
+		u32 reg = iova & 0xFFFFF000;
+		writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
+	} else {
+		u64 reg = iova & 0xfffffffffffff000;
+		writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);

We don't have writeq for arch/arm/.

drivers/iommu/arm-smmu.c static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, · line 1723

Quoted 3 lines

drivers/iommu/arm-smmu.c +3 -0

+	}
+
+	mb();

Why?

Quoted 5 lines

drivers/iommu/arm-smmu.c +5 -0

+	while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) {
+		if (++count == ATSR_LOOP_TIMEOUT) {
+			dev_err(dev,
+				"iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n",
+				&iova, dev_name(dev));

drivers/iommu/arm-smmu.c static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, · line 1733

Quoted 5 lines

drivers/iommu/arm-smmu.c +5 -0

+			arm_smmu_disable_clocks(smmu);
+			return arm_smmu_iova_to_phys_soft(domain, iova);
+		}
+		cpu_relax();
+	}

Do you know what happened to Olav's patches to make this sort of code generic?

drivers/iommu/arm-smmu.c int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) · line 2076

Quoted 6 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		return -ENODEV;
 	}
 
+	if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) {

Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS is also clear.

Quoted 1 line · Will Deacon

We don't have writeq for arch/arm/.

Ah yes looks like this is an MSM-ism that never made it upstream since it wouldn't be guaranteed to be atomic. I'll make sure to do arm32 compiles on upstream kernels for future patches, sorry!

I guess we could use <asm-generic/io-64-nonatomic-lo-hi.h> but I can also re-work this to be two separate writel's.

Quoted 5 lines

drivers/iommu/arm-smmu.c static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, · line 1723

Quoted 3 lines

drivers/iommu/arm-smmu.c +3 -0

+	}
+
+	mb();

Why?

My thought was that if we start polling ATSR_ACTIVE prematurely (before the write to ATS1PR actually finishes) all heck could break loose? Not sure if that's a bogus assumption due to device memory being strongly ordered?

Quoted 5 lines

Quoted 5 lines

drivers/iommu/arm-smmu.c +5 -0

+	while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) {
+		if (++count == ATSR_LOOP_TIMEOUT) {
+			dev_err(dev,
+				"iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n",
+				&iova, dev_name(dev));

Quoted 8 lines

drivers/iommu/arm-smmu.c static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, · line 1733

Quoted 5 lines

drivers/iommu/arm-smmu.c +5 -0

+			arm_smmu_disable_clocks(smmu);
+			return arm_smmu_iova_to_phys_soft(domain, iova);
+		}
+		cpu_relax();
+	}

Do you know what happened to Olav's patches to make this sort of code generic?

I assume you're talking about this, right?

    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267943.html

Yeah looks like he never sent an update since it was part of a series that wasn't going to make it in (the qsmmu driver). I can always bring that patch (actually Matt Wagantall's patch) in here and rework this to use that.

Quoted 9 lines

drivers/iommu/arm-smmu.c int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) · line 2076

Quoted 6 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		return -ENODEV;
 	}
 
+	if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) {

Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS is also clear.

I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states:

In SMMUv2, the address translation registers are OPTIONAL. The address translation registers are implemented only when both:

o The SMMU_IDR0.S1TS bit is set to 1.
o The SMMU_IDR0.ATOSNS bit is set to 0.

I assume you're referring to section 9.6.1 of the same document:

ATOSNS, bit[26]
Address Translation Operations Not Supported. The possible values of this bit are:

0 Address translation operations are supported. Stage 1
translation is not supported, that is, the S1TS bit is set to 0.

1 Address translation operations are not supported. Stage 1
translation is supported, that is, the S1TS bit is set to 1.

If that really means that S1TS and ATOSNS always have the same value then Section 4.1.1 doesn't make any sense. Or am I missing something?

Quoted 27 lines · Mitchel Humpherys

Quoted 8 lines · Will Deacon

drivers/iommu/arm-smmu.c int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) · line 2076

Quoted 5 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		return -ENODEV;
 	}
 
+	if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) {

Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS is also clear.

I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states:

In SMMUv2, the address translation registers are OPTIONAL. The address translation registers are implemented only when both:

o The SMMU_IDR0.S1TS bit is set to 1.
o The SMMU_IDR0.ATOSNS bit is set to 0.

I assume you're referring to section 9.6.1 of the same document:

ATOSNS, bit[26]
Address Translation Operations Not Supported. The possible values of this bit are:

0 Address translation operations are supported. Stage 1
translation is not supported, that is, the S1TS bit is set to 0.

Quoted 5 lines

1 Address translation operations are not supported. Stage 1
translation is supported, that is, the S1TS bit is set to 1.

If that really means that S1TS and ATOSNS always have the same value then Section 4.1.1 doesn't make any sense. Or am I missing something?

I'll get this checked, as those two paragraphs don't make an awful lot of sense together.

Generic IOMMU device tree bindings were recently added in ["devicetree: Add generic IOMMU device tree bindings"]. Implement the bindings in the ARM SMMU driver.

See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings themselves.

Signed-off-by Mitchel Humpherys <[email protected]>

---
 drivers/iommu/arm-smmu.c | 87 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 23 deletions(-)

Generic IOMMU device tree bindings were recently added in ["devicetree: Add generic IOMMU device tree bindings"]. Implement the bindings in the ARM SMMU driver.

See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings themselves.

drivers/iommu/arm-smmu.c +19 -9

index 63c6707fad..22e25f3172 100644
@@ -538,25 +538,32 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 	return 0;
 }
 
+struct iommus_entry {
+	struct list_head list;
+	struct device_node *node;
+	u16 streamids[MAX_MASTER_STREAMIDS];
+	int num_sids;
+};
+
 static int register_smmu_master(struct arm_smmu_device *smmu,
-				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct iommus_entry *entry)
 {
 	int i;
 	struct arm_smmu_master *master;
+	struct device *dev = smmu->dev;
 
-	master = find_smmu_master(smmu, masterspec->np);
+	master = find_smmu_master(smmu, entry->node);
 	if (master) {
 		dev_err(dev,
 			"rejecting multiple registrations for master device %s\n",
-			masterspec->np->name);
+			entry->node->name);
 		return -EBUSY;
 	}
 
-	if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
+	if (entry->num_sids > MAX_MASTER_STREAMIDS) {
 		dev_err(dev,
 			"reached maximum number (%d) of stream IDs for master device %s\n",
-			MAX_MASTER_STREAMIDS, masterspec->np->name);
+			MAX_MASTER_STREAMIDS, entry->node->name);
 		return -ENOSPC;
 	}
 

@@ -564,15 +571,58 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	if (!master)
 		return -ENOMEM;
 
-	master->of_node			= masterspec->np;
-	master->cfg.num_streamids	= masterspec->args_count;
+	master->of_node			= entry->node;
+	master->cfg.num_streamids	= entry->num_sids;
 
 	for (i = 0; i < master->cfg.num_streamids; ++i)
-		master->cfg.streamids[i] = masterspec->args[i];
+		master->cfg.streamids[i] = entry->streamids[i];
 
 	return insert_smmu_master(smmu, master);
 }
 
+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+					int *num_masters)
+{

[adding Rob, Mark, Arnd, Thierry and Hiroshi]

Quoted 6 lines · Mitchel Humpherys

Generic IOMMU device tree bindings were recently added in ["devicetree: Add generic IOMMU device tree bindings"]. Implement the bindings in the ARM SMMU driver.

See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings themselves.

Could you look at moving the parsing code into a separate file please (maybe under lib/ ?). That way, other IOMMUs can use the same binding without the boilerplate having to be rewritten each time.

Quoted 59 lines

drivers/iommu/arm-smmu.c +16 -9

index 63c6707fad..22e25f3172 100644
@@ -538,25 +538,32 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 	return 0;
 }
 
+struct iommus_entry {
+	struct list_head list;
+	struct device_node *node;
+	u16 streamids[MAX_MASTER_STREAMIDS];
+	int num_sids;
+};
+
 static int register_smmu_master(struct arm_smmu_device *smmu,
-				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct iommus_entry *entry)
 {
 	int i;
 	struct arm_smmu_master *master;
+	struct device *dev = smmu->dev;
 
-	master = find_smmu_master(smmu, masterspec->np);
+	master = find_smmu_master(smmu, entry->node);
 	if (master) {
 		dev_err(dev,
 			"rejecting multiple registrations for master device %s\n",
-			masterspec->np->name);
+			entry->node->name);
 		return -EBUSY;
 	}
 
-	if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
+	if (entry->num_sids > MAX_MASTER_STREAMIDS) {
 		dev_err(dev,
 			"reached maximum number (%d) of stream IDs for master device %s\n",
-			MAX_MASTER_STREAMIDS, masterspec->np->name);
+			MAX_MASTER_STREAMIDS, entry->node->name);
 		return -ENOSPC;
 	}
 

@@ -564,15 +571,58 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	if (!master)
 		return -ENOMEM;
 
-	master->of_node			= masterspec->np;
-	master->cfg.num_streamids	= masterspec->args_count;
+	master->of_node			= entry->node;
+	master->cfg.num_streamids	= entry->num_sids;
 
 	for (i = 0; i < master->cfg.num_streamids; ++i)
-		master->cfg.streamids[i] = masterspec->args[i];
+		master->cfg.streamids[i] = entry->streamids[i];
 
 	return insert_smmu_master(smmu, master);
 }
 

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 592

Quoted 10 lines

drivers/iommu/arm-smmu.c +10 -0

+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+					int *num_masters)
+{
+	struct of_phandle_args iommuspec;
+	struct device_node *dn;
+
+	for_each_node_with_property(dn, "iommus") {
+		int arg_ind = 0;
+		struct iommus_entry *entry, *n;
+		LIST_HEAD(iommus);

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 595

Quoted 3 lines

drivers/iommu/arm-smmu.c +3 -0

+
+		while (!of_parse_phandle_with_args(dn, "iommus", "#iommu-cells",
+							arg_ind, &iommuspec)) {

We need to check that the phandle does indeed point at one of our SMMUs here, in case we have a system with multiple IOMMU types, all using the generic binding.

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 600

Quoted 5 lines

drivers/iommu/arm-smmu.c +5 -0

+			int i;
+
+			list_for_each_entry(entry, &iommus, list)
+				if (entry->node == dn)
+					break;

Oh, yuck, this is really nasty to parse...

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 601

Quoted 1 line

drivers/iommu/arm-smmu.c +1 -0

+			if (&entry->list == &iommus) {

Where is entry initialised the first time around?

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 605

Quoted 4 lines

drivers/iommu/arm-smmu.c +4 -0

+				entry = devm_kzalloc(smmu->dev, sizeof(*entry),
+						GFP_KERNEL);
+				if (!entry)
+					return -ENOMEM;

You need to of_node_put the guys you've got back from the phandle parsing code.

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 612

Quoted 7 lines

drivers/iommu/arm-smmu.c +7 -0

+				entry->node = dn;
+				list_add(&entry->list, &iommus);
+			}
+			entry->num_sids = iommuspec.args_count;
+			for (i = 0; i < entry->num_sids; ++i)
+				entry->streamids[i] = iommuspec.args[i];
+			arg_ind++;

Isn't this defined by #iommu-cells?

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 615

Quoted 3 lines

drivers/iommu/arm-smmu.c +3 -0

+		}
+
+		list_for_each_entry_safe(entry, n, &iommus, list) {

Why the _safe variant? This is a local list, right?

Will

Quoted 9 lines

drivers/iommu/arm-smmu.c +9 -0

+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+                                     int *num_masters)
+{
+     struct of_phandle_args iommuspec;
+     struct device_node *dn;
+
+     for_each_node_with_property(dn, "iommus") {
+             int arg_ind = 0;
+             struct iommus_entry *entry, *n;

drivers/iommu/arm-smmu.c static int register_smmu_master(struct arm_smmu_device *smmu, · line 595

Quoted 4 lines

drivers/iommu/arm-smmu.c +4 -0

+             LIST_HEAD(iommus);
+
+             while (!of_parse_phandle_with_args(dn, "iommus", "#iommu-cells",
+                                                     arg_ind, &iommuspec)) {

We need to check that the phandle does indeed point at one of our SMMUs here, in case we have a system with multiple IOMMU types, all using the generic binding.

Add a workaround for some buggy hardware that requires a TLB invalidate operation to occur at map time. Activate the feature with the qcom,smmu-invalidate-on-map boolean DT property.

Hi Mitch,

Quoted 3 lines · Mitchel Humpherys

Add a workaround for some buggy hardware that requires a TLB invalidate operation to occur at map time. Activate the feature with the qcom,smmu-invalidate-on-map boolean DT property.

I'm digging up an old thread here, but I've been working on a new page-table allocator for the SMMU and looked into implementing this workaround for you in there. When I do the TLBI on map after installing the new PTE, can I just invalidate the range mapped by that PTE, or does it need to be a full TLBI?

Quoted 9 lines · Will Deacon

Quoted 3 lines · Mitchel Humpherys

Add a workaround for some buggy hardware that requires a TLB invalidate operation to occur at map time. Activate the feature with the qcom,smmu-invalidate-on-map boolean DT property.

I'm digging up an old thread here, but I've been working on a new page-table allocator for the SMMU and looked into implementing this workaround for you in there. When I do the TLBI on map after installing the new PTE, can I just invalidate the range mapped by that PTE, or does it need to be a full TLBI?

I'm not totally sure on the history of the hardware errata but I believe it's just the range mapped by that pte. We use SMMU_CBn_TLBIVA in the our smmu driver.

However, let's actually just drop this... It's looking like the targets we have that will use the arm-smmu driver thankfully won't need this workaround. Thanks for keeping this in mind though :)

Ha, damn, then I don't have a user of the shiny new quirks field I added! I don't think I'll go as far as removing it altogether though...

Under certain conditions coherent hardware translation table walks can result in degraded performance. Add a new domain attribute to disable/enable this feature in generic code along with the domain attribute setter and getter to handle it in the ARM SMMU driver.

drivers/iommu/arm-smmu.c +4 -0

 	default:
 		return -ENODEV;
 	}

@@ -1922,6 +1935,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 				    enum iommu_attr attr, void *data)
 {
 	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:

@@ -1933,6 +1947,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
 
 		return 0;
+	case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+		cfg->htw_disable = *((bool *)data);
+		return 0;
 	default:
 		return -ENODEV;
 	}

include/linux/iommu.h +0 -0

index 0550286df4..8a6449857a 100644

Quoted 4 lines · Mitchel Humpherys

Under certain conditions coherent hardware translation table walks can result in degraded performance. Add a new domain attribute to disable/enable this feature in generic code along with the domain attribute setter and getter to handle it in the ARM SMMU driver.

Again, it would be nice to have some information about these cases and the performance issues that you are seeing.

Quoted 7 lines

drivers/iommu/arm-smmu.c +1 -0

@@ -1908,11 +1917,15 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 				    enum iommu_attr attr, void *data)
 {
 	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 

drivers/iommu/arm-smmu.c static int arm_smmu_domain_get_attr(struct iommu_domain *domain, · line 1928

Quoted 7 lines

drivers/iommu/arm-smmu.c +3 -0

 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:
 		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
 		return 0;
+	case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+		*((bool *)data) = cfg->htw_disable;
+		return 0;

I think I'd be more comfortable using int instead of bool for this, as it could well end up in the user ABI if vfio decides to make use of it. While we're here, let's also add an attributes bitmap to the arm_smmu_domain instead of having a bool in the arm_smmu_cfg.

Quoted 26 lines

drivers/iommu/arm-smmu.c +4 -0

 	default:
 		return -ENODEV;
 	}

@@ -1922,6 +1935,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 				    enum iommu_attr attr, void *data)
 {
 	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:

@@ -1933,6 +1947,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
 
 		return 0;
+	case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+		cfg->htw_disable = *((bool *)data);
+		return 0;
 	default:
 		return -ENODEV;
 	}

include/linux/iommu.h +0 -0

index 0550286df4..8a6449857a 100644

include/linux/iommu.h enum iommu_attr { · line 84

Quoted 5 lines

include/linux/iommu.h +1 -0

@@ -81,6 +81,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
+	DOMAIN_ATTR_COHERENT_HTW_DISABLE,

I wonder whether we should make this ARM-specific. Can you take a quick look to see if any of the other IOMMUs can potentially benefit from this?

This series is based on on Will's iommu/pci branch.

Quoted 1 line · Mitchel Humpherys

This series is based on on Will's iommu/pci branch.

Incredibly, I also neglected to base this on top of Olav's recent patch ("iommu/arm-smmu: Do not access non-existing SMR registers")! I will do that in v2 after review feedback.