arm-smmu: add master mask

Quoted 2 lines

[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

Quoted 3 lines

[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

Quoted 1 line

[Gupta Nipun] I am not an expert in this, but I do not see any reason for 'of_for_each_phandle' to not work

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 <[email protected]>

---
  drivers/iommu/arm-smmu.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 45 insertions(+), 1 deletion(-)

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

index 9345a3f..14438f8 100644
@@ -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);

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

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

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).

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

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.

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.

From a user perspective I would personally consider it a bit easier to

Quoted 8 lines · Robin Murphy

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 =)

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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

From: Robin Murphy <[email protected]>
Date: Wed, Jun 8, 2016 at 6:11 AM
Subject: Re: [PATCH 2/2] arm-smmu: Add possibillity to mask streamIDs
To: Thommy Jakobsson <[email protected]>
Cc: will.deacon at arm.com, linux-arm-kernel at lists.infradead.org

Quoted 12 lines · Thommy Jakobsson

Quoted 9 lines · Robin Murphy

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).

Quoted 9 lines

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.

Quoted 7 lines

Quoted 2 lines

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!

Quoted 6 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 7 lines

Quoted 3 lines

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.

Quoted 9 lines

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.

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.

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

Signed-off-by Thommy Jakobsson <[email protected]>

---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)