[PATCH 0/2] arm-smmu: add master mask

[PATCH 0/2] arm-smmu: add master mask

[Thommy Jakobsson]

This patchset makes it possible to manually set a mask for a master.
Default behaviour when not specifying a mask is to keep current behaviour.
If masks are specified for a master, all of its stream IDs needs to have a
mask.

Platforms with a large number of IDs per master starts to show up. Its
impractical to specify all available IDs in some cases. Specific example
is the ZynqMPSoC, where the streamid have 6 bits from the AXI ID. So 2^6
IDs for each PL port, which there are 6 of.

Thommy Jakobsson (2):
  arm-smmu: doc: add binding for stream ID mask
  arm-smmu: Add possibillity to mask streamIDs

 .../devicetree/bindings/iommu/arm,smmu.txt         | 15 +++++++-
 drivers/iommu/arm-smmu.c                           | 43 +++++++++++++++++++++-
 2 files changed, 55 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/2] arm-smmu: doc: add binding for stream ID mask

[Thommy Jakobsson]

Add documentation for new device tree binding, mmu-masters-mask.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 19fe6f2..cabd1dc 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -56,6 +56,13 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+- mmu-masters-mask : An optional list that specifies a mask per StreamID.
+                  A master included in this liss, have to specify masks
+                  for all of its StreamIDs. A master not included in the
+                  list will default to mask 0 (i.e entire ID is used).
+                  This can for example be used in systems where parts of
+                  the StreamID is dynamic.
+
 Example:
 
         smmu {
@@ -70,9 +77,13 @@ Example:
                              <0 37 4>;
 
                 /*
-                 * Two DMA controllers, the first with two StreamIDs (0xd01d
+                 * Three DMA controllers, the first with two StreamIDs (0xd01d
                  * and 0xd01e) and the second with only one (0xd11c).
+                 * The third uses a mask to use all StreamIDs that starts with
+                 * 0xd8
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
-                              <&dma1 0xd11c>;
+                              <&dma1 0xd11c>,
+                              <&dma2 0xd800>;
+                mmu-masters-mask = <&dma2 0x07FF>
         };
-- 
1.9.1

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

Use DT bindings to allow masks to be added for a masters streamIDs. This
is necessary for devices that have multiple IDs, or use dynamic signals to
build up the ID. For example the ZynqMPSoC that mirrors 6bits from the
AXI ID into the streamid.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/iommu/arm-smmu.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9345a3f..14438f8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -305,6 +305,7 @@ struct arm_smmu_smr {
 struct arm_smmu_master_cfg {
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	u16				streammasks[MAX_MASTER_STREAMIDS];
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -554,6 +555,38 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);
 }
 
+static int register_smmu_master_mask(struct arm_smmu_device *smmu,
+				     struct device *dev,
+				     struct of_phandle_args *masterspec)
+{
+	int i;
+	struct arm_smmu_master *master;
+
+	master = find_smmu_master(smmu, masterspec->np);
+	if (!master) {
+		dev_err(dev,
+			"mask phandle to %s, but no ID registred\n",
+			masterspec->np->name);
+		return -EINVAL;
+	}
+
+	if (masterspec->args_count != master->cfg.num_streamids) {
+		dev_err(dev,
+			"Different number of ids %d and masks %d not supported for master device %s\n",
+			master->cfg.num_streamids,
+			masterspec->args_count,
+			masterspec->np->name);
+		return -ENOSPC;
+	}
+
+	for (i = 0; i < master->cfg.num_streamids; ++i) {
+		u16 streammask = masterspec->args[i];
+
+		master->cfg.streamids[i] = streammask;
+	}
+	return 0;
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -1106,7 +1139,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 
 		smrs[i] = (struct arm_smmu_smr) {
 			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
+			.mask	= cfg->streammasks[i],
 			.id	= cfg->streamids[i],
 		};
 	}
@@ -1972,6 +2005,17 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters-mask",
+					   "#stream-id-cells", i,
+					   &masterspec)) {
+		err = register_smmu_master_mask(smmu, dev, &masterspec);
+		if (err) {
+			dev_err(dev, "failed to add mask for%s\n",
+				masterspec.np->name);
+			goto out_put_masters;
+		}
+
+	}
 	kfree(masterspec);
 
 	parse_driver_options(smmu);
-- 
1.9.1

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Nipun Gupta]

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Thommy Jakobsson
Sent: Tuesday, May 31, 2016 2:10 AM
To: linux-arm-kernel at lists.infradead.org
Cc: robin.murphy at arm.com; Thommy Jakobsson <thommyj@gmail.com>; will.deacon at arm.com
Subject: [PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

Use DT bindings to allow masks to be added for a masters streamIDs. This is necessary for devices that have multiple IDs, or use dynamic signals to build up the ID. For example the ZynqMPSoC that mirrors 6bits from the AXI ID into the streamid.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/iommu/arm-smmu.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 9345a3f..14438f8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -305,6 +305,7 @@ struct arm_smmu_smr {  struct arm_smmu_master_cfg {
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	u16				streammasks[MAX_MASTER_STREAMIDS];
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -554,6 +555,38 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);  }
 
+static int register_smmu_master_mask(struct arm_smmu_device *smmu,
+				     struct device *dev,
+				     struct of_phandle_args *masterspec) {
+	int i;
+	struct arm_smmu_master *master;
+
+	master = find_smmu_master(smmu, masterspec->np);
+	if (!master) {
+		dev_err(dev,
+			"mask phandle to %s, but no ID registred\n",
+			masterspec->np->name);
+		return -EINVAL;
+	}
+
+	if (masterspec->args_count != master->cfg.num_streamids) {
+		dev_err(dev,
+			"Different number of ids %d and masks %d not supported for master device %s\n",
+			master->cfg.num_streamids,
+			masterspec->args_count,
+			masterspec->np->name);
+		return -ENOSPC;
+	}
+
+	for (i = 0; i < master->cfg.num_streamids; ++i) {
+		u16 streammask = masterspec->args[i];
+
+		master->cfg.streamids[i] = streammask;
[Gupta Nipun] Should this not be 'master->cfg. streammasks[i] = streammask'

+	}
+	return 0;
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)  {
 	struct arm_smmu_device *smmu;
@@ -1106,7 +1139,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 
 		smrs[i] = (struct arm_smmu_smr) {
 			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
+			.mask	= cfg->streammasks[i],
 			.id	= cfg->streamids[i],
 		};
 	}
@@ -1972,6 +2005,17 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters-mask",
+					   "#stream-id-cells", i,
+					   &masterspec)) {
[Gupta Nipun] I should start from 0 and should be incremented in the while loop.

+		err = register_smmu_master_mask(smmu, dev, &masterspec);
+		if (err) {
+			dev_err(dev, "failed to add mask for%s\n",
+				masterspec.np->name);
+			goto out_put_masters;
+		}
+
+	}
 	kfree(masterspec);
 
 	parse_driver_options(smmu);
--
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

>>[Gupta Nipun] I should start from 0 and should be incremented in the while loop.
>>[Gupta Nipun] Should this not be 'master->cfg.streammasks[i] = streammask
Good catch Gupta, thanks. Merging to latest linus tree obviously went
wrong. Should of_for_each_phandle work?
I had problems with that so I backed out and moved back to while
of_parse_phandle_with_args. But parsing
of mmu-masters seems to use of_for_each_phandle

BR,
Thommy Jakobsson

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Nipun Gupta]

-----Original Message-----
From: Thommy Jakobsson [mailto:thommyj at gmail.com] 
Sent: Friday, June 03, 2016 2:32 PM
To: Nipun Gupta <nipun.gupta@nxp.com>
Cc: linux-arm-kernel at lists.infradead.org; robin.murphy at arm.com; will.deacon at arm.com
Subject: Re: [PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

>>[Gupta Nipun] I should start from 0 and should be incremented in the while loop.
>>[Gupta Nipun] Should this not be 'master->cfg.streammasks[i] = 
>>streammask
Good catch Gupta, thanks. Merging to latest linus tree obviously went wrong. Should of_for_each_phandle work?
I had problems with that so I backed out and moved back to while of_parse_phandle_with_args. But parsing of mmu-masters seems to use of_for_each_phandle
[Gupta Nipun] I am not an expert in this, but I do not see any reason for 'of_for_each_phandle' to not work.

BR,
Thommy Jakobsson

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

>>[Gupta Nipun] I am not an expert in this, but I do not see any reason for 'of_for_each_phandle' to not work
Did a fetch and tried again, now it works on target. Not sure if the
fetch did magic or if I just did wrong last time.
New patch uploaded with of_for_each_phandle instead

BR,
Thommy Jakobsson

[PATCH v2 0/2] arm-smmu: add master mask

[Thommy Jakobsson]

This patchset makes it possible to manually set a mask for a master.
Default behaviour when not specifying a mask is to keep current behaviour.
If masks are specified for a master, all of its stream IDs needs to have a
mask.

Platforms with a large number of IDs per master starts to show up. Its
impractical to specify all available IDs in some cases. Specific example
is the ZynqMPSoC, where the streamid have 6 bits from the AXI ID. So 2^6
IDs for each PL port, which there are 6 of.

Thommy Jakobsson (2):
  arm-smmu: doc: add binding for stream ID mask
  arm-smmu: Add possibillity to mask streamIDs

 .../devicetree/bindings/iommu/arm,smmu.txt         | 15 ++++++-
 drivers/iommu/arm-smmu.c                           | 52 +++++++++++++++++++++-
 2 files changed, 63 insertions(+), 4 deletions(-)

--

version 2
 - fix bug of mask not beeing setup properly
 - changed to using of_for_each_phandle for DT mask parse
1.9.1

[PATCH v2 1/2] arm-smmu: doc: add binding for stream ID mask

[Thommy Jakobsson]

Add documentation for new device tree binding, mmu-masters-mask.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 19fe6f2..cabd1dc 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -56,6 +56,13 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+- mmu-masters-mask : An optional list that specifies a mask per StreamID.
+                  A master included in this liss, have to specify masks
+                  for all of its StreamIDs. A master not included in the
+                  list will default to mask 0 (i.e entire ID is used).
+                  This can for example be used in systems where parts of
+                  the StreamID is dynamic.
+
 Example:
 
         smmu {
@@ -70,9 +77,13 @@ Example:
                              <0 37 4>;
 
                 /*
-                 * Two DMA controllers, the first with two StreamIDs (0xd01d
+                 * Three DMA controllers, the first with two StreamIDs (0xd01d
                  * and 0xd01e) and the second with only one (0xd11c).
+                 * The third uses a mask to use all StreamIDs that starts with
+                 * 0xd8
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
-                              <&dma1 0xd11c>;
+                              <&dma1 0xd11c>,
+                              <&dma2 0xd800>;
+                mmu-masters-mask = <&dma2 0x07FF>
         };
-- 
1.9.1

[PATCH v2 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

Use DT bindings to allow masks to be added for a masters streamIDs. This
is necessary for devices that have multiple IDs, or use dynamic signals to
build up the ID. For example the ZynqMPSoC that mirrors 6bits from the
AXI ID into the streamid.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/iommu/arm-smmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9345a3f..f325cf7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -305,6 +305,7 @@ struct arm_smmu_smr {
 struct arm_smmu_master_cfg {
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	u16				streammasks[MAX_MASTER_STREAMIDS];
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -554,6 +555,38 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);
 }
 
+static int register_smmu_master_mask(struct arm_smmu_device *smmu,
+				     struct device *dev,
+				     struct arm_smmu_phandle_args *masterspec)
+{
+	int i;
+	struct arm_smmu_master *master;
+
+	master = find_smmu_master(smmu, masterspec->np);
+	if (!master) {
+		dev_err(dev,
+			"mask phandle to %s, but no ID registred\n",
+			masterspec->np->name);
+		return -EINVAL;
+	}
+
+	if (masterspec->args_count != master->cfg.num_streamids) {
+		dev_err(dev,
+			"Different number of ids %d and masks %d not supported for master device %s\n",
+			master->cfg.num_streamids,
+			masterspec->args_count,
+			masterspec->np->name);
+		return -ENOSPC;
+	}
+
+	for (i = 0; i < master->cfg.num_streamids; ++i) {
+		u16 streammask = masterspec->args[i];
+
+		master->cfg.streammasks[i] = streammask;
+	}
+	return 0;
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -1106,7 +1139,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 
 		smrs[i] = (struct arm_smmu_smr) {
 			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
+			.mask	= cfg->streammasks[i],
 			.id	= cfg->streamids[i],
 		};
 	}
@@ -1969,9 +2002,24 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 		i++;
 	}
-
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters-mask", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec->args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec->np		= of_node_get(it.node);
+		masterspec->args_count	= count;
+
+		err = register_smmu_master_mask(smmu, dev, masterspec);
+		if (err) {
+			dev_err(dev, "failed to add mask for%s\n",
+				masterspec->np->name);
+			kfree(masterspec);
+			goto out_put_masters;
+		}
+
+	}
 	kfree(masterspec);
 
 	parse_driver_options(smmu);
-- 
1.9.1

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Robin Murphy]

On 30/05/16 21:40, Thommy Jakobsson wrote:
> Use DT bindings to allow masks to be added for a masters streamIDs. This
> is necessary for devices that have multiple IDs, or use dynamic signals to
> build up the ID. For example the ZynqMPSoC that mirrors 6bits from the
> AXI ID into the streamid.

That rationale doesn't hold, since "necessary" is clearly untrue - 
devices with multiple IDs work just fine even with the current behaviour 
of the driver provided there are sufficient SMRs. The third option of 
generating a mask dynamically from a set of IDs is also perfectly 
possible (which reminds me I must dig up the code I prototyped a while 
back...), although an explicitly specified mask can still be beneficial 
for optimal allocation in certain cases (by being able to express the 
non-existence of IDs outside the set).

> Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
> ---
>   drivers/iommu/arm-smmu.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 9345a3f..14438f8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -305,6 +305,7 @@ struct arm_smmu_smr {
>   struct arm_smmu_master_cfg {
>   	int				num_streamids;
>   	u16				streamids[MAX_MASTER_STREAMIDS];
> +	u16				streammasks[MAX_MASTER_STREAMIDS];
>   	struct arm_smmu_smr		*smrs;
>   };
>
> @@ -554,6 +555,38 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   	return insert_smmu_master(smmu, master);
>   }
>
> +static int register_smmu_master_mask(struct arm_smmu_device *smmu,
> +				     struct device *dev,
> +				     struct of_phandle_args *masterspec)
> +{
> +	int i;
> +	struct arm_smmu_master *master;
> +
> +	master = find_smmu_master(smmu, masterspec->np);
> +	if (!master) {
> +		dev_err(dev,
> +			"mask phandle to %s, but no ID registred\n",
> +			masterspec->np->name);
> +		return -EINVAL;
> +	}
> +
> +	if (masterspec->args_count != master->cfg.num_streamids) {
> +		dev_err(dev,
> +			"Different number of ids %d and masks %d not supported for master device %s\n",

But this directly contradicts the example in the binding doc? (Which 
incidentally wasn't sent to the devicetree mailing list.)

> +			master->cfg.num_streamids,
> +			masterspec->args_count,
> +			masterspec->np->name);
> +		return -ENOSPC;
> +	}
> +
> +	for (i = 0; i < master->cfg.num_streamids; ++i) {
> +		u16 streammask = masterspec->args[i];
> +
> +		master->cfg.streamids[i] = streammask;

What if the DT is wrong and the SMMU doesn't support stream matching at 
all, or the mask uses unimplemented SMR bits? Bogus stream IDs or stream 
match conflicts are annoyingly subtle to debug.

> +	}
> +	return 0;
> +}
> +
>   static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
>   {
>   	struct arm_smmu_device *smmu;
> @@ -1106,7 +1139,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
>
>   		smrs[i] = (struct arm_smmu_smr) {
>   			.idx	= idx,
> -			.mask	= 0, /* We don't currently share SMRs */
> +			.mask	= cfg->streammasks[i],
>   			.id	= cfg->streamids[i],
>   		};
>   	}
> @@ -1972,6 +2005,17 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>
>   	dev_notice(dev, "registered %d master devices\n", i);
>
> +	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters-mask",
> +					   "#stream-id-cells", i,
> +					   &masterspec)) {
> +		err = register_smmu_master_mask(smmu, dev, &masterspec);
> +		if (err) {
> +			dev_err(dev, "failed to add mask for%s\n",
> +				masterspec.np->name);
> +			goto out_put_masters;
> +		}
> +
> +	}
>   	kfree(masterspec);
>
>   	parse_driver_options(smmu);
>

Either way, if you insist on wanting to build on the legacy binding that 
doesn't work for PCI, can't integrate with DMA mapping, and everyone 
else would rather get away from in favour of the generic IOMMU 
bindings[1], there's still no need for yet another binding to support. 
You can easily extend "mmu-masters" to encode an SMR mask in the upper 
halfword of a cell without ambiguity and without breaking compatibility 
with existing DTs.

Robin.

[1]The latest of many attempts being mine: 
http://thread.gmane.org/gmane.linux.kernel.iommu/12454

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

On 6 June 2016 at 14:47, Robin Murphy <robin.murphy@arm.com> wrote:
> That rationale doesn't hold, since "necessary" is clearly untrue - devices
> with multiple IDs work just fine even with the current behaviour of the
> driver provided there are sufficient SMRs. The third option of generating a
> mask dynamically from a set of IDs is also perfectly possible (which reminds
> me I must dig up the code I prototyped a while back...), although an
> explicitly specified mask can still be beneficial for optimal allocation in
> certain cases (by being able to express the non-existence of IDs outside the
> set).
Thanks for your time Robin. I was probably unclear in my comment. You're
right and devices with multiple IDs do work already today. Its more the second
part that you talk about that limits me, "provided there are
sufficient SMRs". In
the ZynqMPSoC, 6 of the AXI ID bits are copied into the stream id for FPGA
IPs, so I have 2^6 IDs per master and there is 6 masters. Besides the FPGA
there is also ethernet and dma masters (also gpu and sata, but I don't use
those). The SMMU implementation only has 48 register groups to play with, so
without mask I'm screwed =)

> But this directly contradicts the example in the binding doc? (Which
> incidentally wasn't sent to the devicetree mailing list.)
Not sure that I follow, what contradicts? My intention was that for masters that
explicitly is configured with a mask, all of its IDs should have a mask. Without
specifying a mask it should be same behavior as before. The doc example
shows a master, dma2, with one ID and one mask.

> What if the DT is wrong and the SMMU doesn't support stream matching at all,
> or the mask uses unimplemented SMR bits? Bogus stream IDs or stream match
> conflicts are annoyingly subtle to debug.
Point taken.

> Either way, if you insist on wanting to build on the legacy binding that
> doesn't work for PCI, can't integrate with DMA mapping, and everyone else
> would rather get away from in favour of the generic IOMMU bindings[1],
Generic IOMMU bindings would be great, but I'm not sure that it solves
my problem
with too many IDs vs SMRs. For a smaller

> there's still no need for yet another binding to support. You can easily
> extend "mmu-masters" to encode an SMR mask in the upper halfword of a cell
> without ambiguity and without breaking compatibility with existing DTs.
How do you mean breaking compatibility with existing DTs? It was my
intention not
to do so but instead adding an optional mask for cases when actually needed.
>From a user perspective I would personally consider it a bit easier to
understand
with separate bindings for mask and ID, but to be honest I haven't worked that
much with DT bindings before (which is also why I missed sending it to the DT
mailinglist). So if you see a problem with an increasing mass of
bindings to support,
and consider using the upper halfword of the ID-cell a better way,
then sure I'll buy
that.

BR,
Thommy Jakobsson

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Robin Murphy]

On 08/06/16 08:47, Thommy Jakobsson wrote:
> On 6 June 2016 at 14:47, Robin Murphy <robin.murphy@arm.com> wrote:
>> That rationale doesn't hold, since "necessary" is clearly untrue - devices
>> with multiple IDs work just fine even with the current behaviour of the
>> driver provided there are sufficient SMRs. The third option of generating a
>> mask dynamically from a set of IDs is also perfectly possible (which reminds
>> me I must dig up the code I prototyped a while back...), although an
>> explicitly specified mask can still be beneficial for optimal allocation in
>> certain cases (by being able to express the non-existence of IDs outside the
>> set).
> Thanks for your time Robin. I was probably unclear in my comment. You're
> right and devices with multiple IDs do work already today. Its more the second
> part that you talk about that limits me, "provided there are
> sufficient SMRs". In
> the ZynqMPSoC, 6 of the AXI ID bits are copied into the stream id for FPGA
> IPs, so I have 2^6 IDs per master and there is 6 masters. Besides the FPGA
> there is also ethernet and dma masters (also gpu and sata, but I don't use
> those). The SMMU implementation only has 48 register groups to play with, so
> without mask I'm screwed =)

Indeed, I wasn't implying you don't need it, merely nitpicking that the 
justification was worded badly.

>> But this directly contradicts the example in the binding doc? (Which
>> incidentally wasn't sent to the devicetree mailing list.)
> Not sure that I follow, what contradicts? My intention was that for masters that
> explicitly is configured with a mask, all of its IDs should have a mask. Without
> specifying a mask it should be same behavior as before. The doc example
> shows a master, dma2, with one ID and one mask.

Oh, wait, it's only comparing the number of arguments to each phandle, 
not the whole property length, my mistake - sorry!

>> What if the DT is wrong and the SMMU doesn't support stream matching at all,
>> or the mask uses unimplemented SMR bits? Bogus stream IDs or stream match
>> conflicts are annoyingly subtle to debug.
> Point taken.
>
>> Either way, if you insist on wanting to build on the legacy binding that
>> doesn't work for PCI, can't integrate with DMA mapping, and everyone else
>> would rather get away from in favour of the generic IOMMU bindings[1],
> Generic IOMMU bindings would be great, but I'm not sure that it solves
> my problem
> with too many IDs vs SMRs. For a smaller

My point was that the generic bindings are also trivially easy to extend 
arbitrarily by simply increasing "#iommu-cells". In that patch series 
I'm very deliberately laying the groundwork for supporting an SMR mask 
in the second cell ;)

>> there's still no need for yet another binding to support. You can easily
>> extend "mmu-masters" to encode an SMR mask in the upper halfword of a cell
>> without ambiguity and without breaking compatibility with existing DTs.
> How do you mean breaking compatibility with existing DTs? It was my
> intention not
> to do so but instead adding an optional mask for cases when actually needed.

I didn't mean that your proposal would break anything - it clearly won't 
- but that if we were to redefine "mmu-masters" such that for for 
stream-matching SMMUs each cell represents an SMR to match rather than 
just an ID, it wouldn't actually make any difference to the behaviour on 
existing systems.

>  From a user perspective I would personally consider it a bit easier to
> understand
> with separate bindings for mask and ID, but to be honest I haven't worked that
> much with DT bindings before (which is also why I missed sending it to the DT
> mailinglist). So if you see a problem with an increasing mass of
> bindings to support,
> and consider using the upper halfword of the ID-cell a better way,
> then sure I'll buy
> that.

Either way, I'm currently in the process of reworking the SMMUv2 generic 
bindings series, having got the SMMUv3 patches out of the way, so I'll 
make sure that at least basic masking support actually makes the cut 
this time round.

Robin.

>
> BR,
> Thommy Jakobsson
>

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Stuart Yoder]

> From: Robin Murphy <robin.murphy@arm.com>
> Date: Wed, Jun 8, 2016 at 6:11 AM
> Subject: Re: [PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs
> To: Thommy Jakobsson <thommyj@gmail.com>
> Cc: will.deacon at arm.com, linux-arm-kernel at lists.infradead.org
> 
> 
> On 08/06/16 08:47, Thommy Jakobsson wrote:
> >
> > On 6 June 2016 at 14:47, Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> That rationale doesn't hold, since "necessary" is clearly untrue - devices
> >> with multiple IDs work just fine even with the current behaviour of the
> >> driver provided there are sufficient SMRs. The third option of generating a
> >> mask dynamically from a set of IDs is also perfectly possible (which reminds
> >> me I must dig up the code I prototyped a while back...), although an
> >> explicitly specified mask can still be beneficial for optimal allocation in
> >> certain cases (by being able to express the non-existence of IDs outside the
> >> set).
> >
> > Thanks for your time Robin. I was probably unclear in my comment. You're
> > right and devices with multiple IDs do work already today. Its more the second
> > part that you talk about that limits me, "provided there are
> > sufficient SMRs". In
> > the ZynqMPSoC, 6 of the AXI ID bits are copied into the stream id for FPGA
> > IPs, so I have 2^6 IDs per master and there is 6 masters. Besides the FPGA
> > there is also ethernet and dma masters (also gpu and sata, but I don't use
> > those). The SMMU implementation only has 48 register groups to play with, so
> > without mask I'm screwed =)
> 
> 
> Indeed, I wasn't implying you don't need it, merely nitpicking that
> the justification was worded badly.
> 
> >> But this directly contradicts the example in the binding doc? (Which
> >> incidentally wasn't sent to the devicetree mailing list.)
> >
> > Not sure that I follow, what contradicts? My intention was that for masters that
> > explicitly is configured with a mask, all of its IDs should have a mask. Without
> > specifying a mask it should be same behavior as before. The doc example
> > shows a master, dma2, with one ID and one mask.
> 
> 
> Oh, wait, it's only comparing the number of arguments to each phandle,
> not the whole property length, my mistake - sorry!
> 
> >> What if the DT is wrong and the SMMU doesn't support stream matching at all,
> >> or the mask uses unimplemented SMR bits? Bogus stream IDs or stream match
> >> conflicts are annoyingly subtle to debug.
> >
> > Point taken.
> >
> >> Either way, if you insist on wanting to build on the legacy binding that
> >> doesn't work for PCI, can't integrate with DMA mapping, and everyone else
> >> would rather get away from in favour of the generic IOMMU bindings[1],
> >
> > Generic IOMMU bindings would be great, but I'm not sure that it solves
> > my problem
> > with too many IDs vs SMRs. For a smaller
> 
> 
> My point was that the generic bindings are also trivially easy to
> extend arbitrarily by simply increasing "#iommu-cells". In that patch
> series I'm very deliberately laying the groundwork for supporting an
> SMR mask in the second cell ;)
> 
> >> there's still no need for yet another binding to support. You can easily
> >> extend "mmu-masters" to encode an SMR mask in the upper halfword of a cell
> >> without ambiguity and without breaking compatibility with existing DTs.
> >
> > How do you mean breaking compatibility with existing DTs? It was my
> > intention not
> > to do so but instead adding an optional mask for cases when actually needed.
> 
> 
> I didn't mean that your proposal would break anything - it clearly
> won't - but that if we were to redefine "mmu-masters" such that for
> for stream-matching SMMUs each cell represents an SMR to match rather
> than just an ID, it wouldn't actually make any difference to the
> behaviour on existing systems.
> 
> >  From a user perspective I would personally consider it a bit easier to
> > understand
> > with separate bindings for mask and ID, but to be honest I haven't worked that
> > much with DT bindings before (which is also why I missed sending it to the DT
> > mailinglist). So if you see a problem with an increasing mass of
> > bindings to support,
> > and consider using the upper halfword of the ID-cell a better way,
> > then sure I'll buy
> > that.
> 
> 
> Either way, I'm currently in the process of reworking the SMMUv2
> generic bindings series, having got the SMMUv3 patches out of the way,
> so I'll make sure that at least basic masking support actually makes
> the cut this time round.

One comment on the explicit masking support-- keep in mind that there will
be bus types in addition to platform and pci.  We are working on
fsl-mc bus support for the SMMU driver and an SMR mask will be needed
there as well.  I assume that some kind of bus-specific function
will be needed for the SMMU driver to get platform and PCI SMR mask
values because those 2 busses are quite different.  And we can use
something similar for fsl-mc, but wanted to mention that this is 
coming.

Thanks,
Stuart

[PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

On 8 June 2016 at 13:11, Robin Murphy <robin.murphy@arm.com> wrote:

> Either way, I'm currently in the process of reworking the SMMUv2 generic
> bindings series, having got the SMMUv3 patches out of the way, so I'll make
> sure that at least basic masking support actually makes the cut this time
> round.
Thanks, since I'm kind of keen of getting a mask I'll send in a patch
with updates suggested =) If you want to use it use it, otherwise I'll
wait for your binding series.

BR,
Thommy Jakobsson

[PATCH v3 0/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

This patchset makes it possible to manually set a mask for a master.
Default behaviour when not specifying a mask is to keep current behaviour.
A mask is specified by using upper halw word of the StreamID cell in DT.

Thommy Jakobsson (2):
  arm-smmu: doc: add SMMU mask documentation
  arm-smmu: Add possibillity to mask streamIDs

 .../devicetree/bindings/iommu/arm,smmu.txt         | 23 +++++++++++++++++++---
 drivers/iommu/arm-smmu.c                           | 18 +++++++++++++++--
 2 files changed, 36 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] arm-smmu: doc: add SMMU mask documentation

[Thommy Jakobsson]

Add documentation on how to use a mask for a StreamId.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 19fe6f2..faf32d8b 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -37,7 +37,15 @@ conditions.
 
 - mmu-masters   : A list of phandles to device nodes representing bus
                   masters for which the SMMU can provide a translation
-                  and their corresponding StreamIDs (see example below).
+                  and their corresponding StreamIDs.
+
+                  Upper halfword of the StreamID-cell is describing an
+                  (optional) mask. If supported by the SMMU, it will use
+                  the following comparison when matching
+                  (incoming StreamID & !mask) == (DT StreamId & !mask)
+                  If no mask is given, a default mask of 0 is used (i.e
+                  entire ID is used for matching). See example below.
+
                   Each device node linked from this list must have a
                   "#stream-id-cells" property, indicating the number of
                   StreamIDs associated with it.
@@ -55,6 +63,12 @@ conditions.
                   SMMU configuration registers. In this case non-secure
                   aliases of secure registers have to be used during
                   SMMU configuration.
+- mmu-masters-mask : An optional list that specifies a mask per StreamID.
+                  A master included in this liss, have to specify masks
+                  for all of its StreamIDs. A master not included in the
+                  list will default to mask 0 (i.e entire ID is used).
+                  This can for example be used in systems where parts of
+                  the StreamID is dynamic.
 
 Example:
 
@@ -70,9 +84,12 @@ Example:
                              <0 37 4>;
 
                 /*
-                 * Two DMA controllers, the first with two StreamIDs (0xd01d
+                 * Three DMA controllers, the first with two StreamIDs (0xd01d
                  * and 0xd01e) and the second with only one (0xd11c).
+                 * The third uses a mask to use all StreamIDs that starts with
+                 * 0xd8
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
-                              <&dma1 0xd11c>;
+                              <&dma1 0xd11c>,
+                              <&dma2 0x07ffd800>;
         };
-- 
1.9.1

[PATCH v3 1/2] arm-smmu: doc: add SMMU mask documentation

[Will Deacon]

On Thu, Jun 09, 2016 at 12:43:20PM +0200, Thommy Jakobsson wrote:
> Add documentation on how to use a mask for a StreamId.
> 
> Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

We should *really* be moving people over to the generic IOMMU bindings
rather than extending this one.

Will

[PATCH v3 2/2] arm-smmu: Add possibillity to mask streamIDs

[Thommy Jakobsson]

Use upper half word in StreamID cell from DT as a StreamID mask. This is
useful for reducing number of SMRs used for a master with many StreamIDs.
For example the ZynqMPSoC mirrors 6bits from the AXI ID into the StreamID
for FPGA blocks, but only allows 48 register groups for stream matching.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/iommu/arm-smmu.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9345a3f..ffedd96 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -305,6 +305,7 @@ struct arm_smmu_smr {
 struct arm_smmu_master_cfg {
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	u16				streammasks[MAX_MASTER_STREAMIDS];
 	struct arm_smmu_smr		*smrs;
 };
 
@@ -347,6 +348,7 @@ struct arm_smmu_device {
 
 	u32				num_mapping_groups;
 	DECLARE_BITMAP(smr_map, ARM_SMMU_MAX_SMRS);
+	u16				supported_mask;
 
 	unsigned long			va_size;
 	unsigned long			ipa_size;
@@ -540,7 +542,10 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	master->cfg.num_streamids	= masterspec->args_count;
 
 	for (i = 0; i < master->cfg.num_streamids; ++i) {
-		u16 streamid = masterspec->args[i];
+		u16 streamid = (masterspec->args[i] >> SMR_ID_SHIFT) &
+				SMR_ID_MASK;
+		u16 mask     = (masterspec->args[i] >> SMR_MASK_SHIFT) &
+				SMR_MASK_MASK;
 
 		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
 		     (streamid >= smmu->num_mapping_groups)) {
@@ -550,6 +555,14 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 			return -ERANGE;
 		}
 		master->cfg.streamids[i] = streamid;
+
+		if ((smmu->supported_mask | mask) != smmu->supported_mask) {
+			dev_err(dev,
+				"unsupported mask, 0x%04x used for master device %s\n",
+				mask, masterspec->np->name);
+			return -ERANGE;
+		}
+		master->cfg.streammasks[i] = mask;
 	}
 	return insert_smmu_master(smmu, master);
 }
@@ -1106,7 +1119,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 
 		smrs[i] = (struct arm_smmu_smr) {
 			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
+			.mask	= cfg->streammasks[i],
 			.id	= cfg->streamids[i],
 		};
 	}
@@ -1735,6 +1748,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 				mask, sid);
 			return -ENODEV;
 		}
+		smmu->supported_mask = mask;
 
 		dev_notice(smmu->dev,
 			   "\tstream matching with %u register groups, mask 0x%x",
-- 
1.9.1