[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

This patch adds a description of the device tree binding for the ARM
System MMU architecture.

Cc: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu.txt

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
new file mode 100644
index 0000000..56de07c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -0,0 +1,66 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+
+                  depending on the particular implementation and/or the
+                  version of the architecture implemented.
+
+- reg           : Base address and size of the SMMU.
+
+- #global-interrupts : The number of global interrupts exposed by the
+                       device.
+
+- interrupts    : Interrupt list, with the first #global-irqs entries
+                  corresponding to the global interrupts and any
+                  following entries corresponding to context interrupts,
+                  specified in order of their indexing by the SMMU.
+
+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation.
+
+- stream-ids    : A list of 32-bit values corresponding to the StreamIDs
+                  for the devices listed in the mmu-masters property.
+                  This list must be same length as mmu-masters, so
+                  masters with multiple stream-ids will have multiple
+                  entries in mmu-masters.
+
+** System MMU optional properties:
+
+- smmu-parent   : When multiple SMMUs are chained together, this
+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.
+
+Example:
+
+        smmu {
+                compatible = "arm,smmu-v1";
+                reg = <0xba5e0000 0x10000>;
+                #global-interrupts = <2>;
+                interrupts = <0 32 4>,
+                             <0 33 4>,
+                             <0 34 4>, /* This is the first context interrupt */
+                             <0 35 4>,
+                             <0 36 4>,
+                             <0 37 4>;
+                mmu-masters = <&dma0>,
+                              <&dma0>,
+                              <&dma1>;
+                stream-ids  = <0xd01d>,
+                              <0xd01e>,
+                              <0xd11c>;
+        };
-- 
1.8.0

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

This patch adds a description of the device tree binding for the ARM
System MMU architecture.

Cc: Rob Herring <robherring2@gmail.com>
Cc: Andreas Herrmann <andreas.herrmann@calxeda.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu.txt

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
new file mode 100644
index 0000000..56de07c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -0,0 +1,66 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+
+                  depending on the particular implementation and/or the
+                  version of the architecture implemented.
+
+- reg           : Base address and size of the SMMU.
+
+- #global-interrupts : The number of global interrupts exposed by the
+                       device.
+
+- interrupts    : Interrupt list, with the first #global-irqs entries
+                  corresponding to the global interrupts and any
+                  following entries corresponding to context interrupts,
+                  specified in order of their indexing by the SMMU.
+
+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation.
+
+- stream-ids    : A list of 32-bit values corresponding to the StreamIDs
+                  for the devices listed in the mmu-masters property.
+                  This list must be same length as mmu-masters, so
+                  masters with multiple stream-ids will have multiple
+                  entries in mmu-masters.
+
+** System MMU optional properties:
+
+- smmu-parent   : When multiple SMMUs are chained together, this
+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.
+
+Example:
+
+        smmu {
+                compatible = "arm,smmu-v1";
+                reg = <0xba5e0000 0x10000>;
+                #global-interrupts = <2>;
+                interrupts = <0 32 4>,
+                             <0 33 4>,
+                             <0 34 4>, /* This is the first context interrupt */
+                             <0 35 4>,
+                             <0 36 4>,
+                             <0 37 4>;
+                mmu-masters = <&dma0>,
+                              <&dma0>,
+                              <&dma1>;
+                stream-ids  = <0xd01d>,
+                              <0xd01e>,
+                              <0xd11c>;
+        };
-- 
1.8.0

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

Hi Will,

so far, I thought, that this proposal is fine. After I have tried to
make use of the binding I have some points that might need further
disucssion.

Olav already asked (in another thread) how to model the mapping of
stream IDs to contexts for master devices that support multiple
contexts.  I doubt that this is fully covered yet.

I also think that it is more useful to move the stream-id property to
the device node of a master device. (It's a characteristic of the
master device not of the SMMU.) Currently with multiple stream IDs per
master device you have repeated entries in the mmu-master property.

But all that is needed is to point (once) to each mmu-master in the
SMMU device node. Then you should be able to look up the corresponding
stream IDs in the device node for each mmu-master.


Regars,
Andreas


On Fri, Apr 12, 2013 at 02:02:07PM -0400, Will Deacon wrote:
> This patch adds a description of the device tree binding for the ARM
> System MMU architecture.
> 
> Cc: Rob Herring <robherring2@gmail.com>
> Cc: Andreas Herrmann <andreas.herrmann@calxeda.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu.txt
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> new file mode 100644
> index 0000000..56de07c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -0,0 +1,66 @@
> +* ARM System MMU Architecture Implementation
> +
> +ARM SoCs may contain an implementation of the ARM System Memory
> +Management Unit Architecture, which can be used to provide 1 or 2 stages
> +of address translation to bus masters external to the CPU.
> +
> +The SMMU may also raise interrupts in response to various fault
> +conditions.
> +
> +** System MMU required properties:
> +
> +- compatible    : Should be one of:
> +
> +                        "arm,smmu-v1"
> +                        "arm,smmu-v2"
> +                        "arm,mmu-400"
> +
> +                  depending on the particular implementation and/or the
> +                  version of the architecture implemented.
> +
> +- reg           : Base address and size of the SMMU.
> +
> +- #global-interrupts : The number of global interrupts exposed by the
> +                       device.
> +
> +- interrupts    : Interrupt list, with the first #global-irqs entries
> +                  corresponding to the global interrupts and any
> +                  following entries corresponding to context interrupts,
> +                  specified in order of their indexing by the SMMU.
> +
> +- mmu-masters   : A list of phandles to device nodes representing bus
> +                  masters for which the SMMU can provide a translation.
> +
> +- stream-ids    : A list of 32-bit values corresponding to the StreamIDs
> +                  for the devices listed in the mmu-masters property.
> +                  This list must be same length as mmu-masters, so
> +                  masters with multiple stream-ids will have multiple
> +                  entries in mmu-masters.
> +
> +** System MMU optional properties:
> +
> +- smmu-parent   : When multiple SMMUs are chained together, this
> +                  property can be used to provide a phandle to the
> +                  parent SMMU (that is the next SMMU on the path going
> +                  from the mmu-masters towards memory) node for this
> +                  SMMU.
> +
> +Example:
> +
> +        smmu {
> +                compatible = "arm,smmu-v1";
> +                reg = <0xba5e0000 0x10000>;
> +                #global-interrupts = <2>;
> +                interrupts = <0 32 4>,
> +                             <0 33 4>,
> +                             <0 34 4>, /* This is the first context interrupt */
> +                             <0 35 4>,
> +                             <0 36 4>,
> +                             <0 37 4>;
> +                mmu-masters = <&dma0>,
> +                              <&dma0>,
> +                              <&dma1>;
> +                stream-ids  = <0xd01d>,
> +                              <0xd01e>,
> +                              <0xd11c>;
> +        };
> -- 
> 1.8.0
> 

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

Hi Will,

so far, I thought, that this proposal is fine. After I have tried to
make use of the binding I have some points that might need further
disucssion.

Olav already asked (in another thread) how to model the mapping of
stream IDs to contexts for master devices that support multiple
contexts.  I doubt that this is fully covered yet.

I also think that it is more useful to move the stream-id property to
the device node of a master device. (It's a characteristic of the
master device not of the SMMU.) Currently with multiple stream IDs per
master device you have repeated entries in the mmu-master property.

But all that is needed is to point (once) to each mmu-master in the
SMMU device node. Then you should be able to look up the corresponding
stream IDs in the device node for each mmu-master.


Regars,
Andreas


On Fri, Apr 12, 2013 at 02:02:07PM -0400, Will Deacon wrote:
> This patch adds a description of the device tree binding for the ARM
> System MMU architecture.
> 
> Cc: Rob Herring <robherring2@gmail.com>
> Cc: Andreas Herrmann <andreas.herrmann@calxeda.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu.txt
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> new file mode 100644
> index 0000000..56de07c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -0,0 +1,66 @@
> +* ARM System MMU Architecture Implementation
> +
> +ARM SoCs may contain an implementation of the ARM System Memory
> +Management Unit Architecture, which can be used to provide 1 or 2 stages
> +of address translation to bus masters external to the CPU.
> +
> +The SMMU may also raise interrupts in response to various fault
> +conditions.
> +
> +** System MMU required properties:
> +
> +- compatible    : Should be one of:
> +
> +                        "arm,smmu-v1"
> +                        "arm,smmu-v2"
> +                        "arm,mmu-400"
> +
> +                  depending on the particular implementation and/or the
> +                  version of the architecture implemented.
> +
> +- reg           : Base address and size of the SMMU.
> +
> +- #global-interrupts : The number of global interrupts exposed by the
> +                       device.
> +
> +- interrupts    : Interrupt list, with the first #global-irqs entries
> +                  corresponding to the global interrupts and any
> +                  following entries corresponding to context interrupts,
> +                  specified in order of their indexing by the SMMU.
> +
> +- mmu-masters   : A list of phandles to device nodes representing bus
> +                  masters for which the SMMU can provide a translation.
> +
> +- stream-ids    : A list of 32-bit values corresponding to the StreamIDs
> +                  for the devices listed in the mmu-masters property.
> +                  This list must be same length as mmu-masters, so
> +                  masters with multiple stream-ids will have multiple
> +                  entries in mmu-masters.
> +
> +** System MMU optional properties:
> +
> +- smmu-parent   : When multiple SMMUs are chained together, this
> +                  property can be used to provide a phandle to the
> +                  parent SMMU (that is the next SMMU on the path going
> +                  from the mmu-masters towards memory) node for this
> +                  SMMU.
> +
> +Example:
> +
> +        smmu {
> +                compatible = "arm,smmu-v1";
> +                reg = <0xba5e0000 0x10000>;
> +                #global-interrupts = <2>;
> +                interrupts = <0 32 4>,
> +                             <0 33 4>,
> +                             <0 34 4>, /* This is the first context interrupt */
> +                             <0 35 4>,
> +                             <0 36 4>,
> +                             <0 37 4>;
> +                mmu-masters = <&dma0>,
> +                              <&dma0>,
> +                              <&dma1>;
> +                stream-ids  = <0xd01d>,
> +                              <0xd01e>,
> +                              <0xd11c>;
> +        };
> -- 
> 1.8.0
> 

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:
> Hi Will,

Hi Andreas,

> so far, I thought, that this proposal is fine. After I have tried to
> make use of the binding I have some points that might need further
> disucssion.

Sure, although I've been using the binding without issues so it would be
interesting to understand your use-case and why it's raising problems.

> Olav already asked (in another thread) how to model the mapping of
> stream IDs to contexts for master devices that support multiple
> contexts.  I doubt that this is fully covered yet.

Yeah, I've been meaning to reply to that. Given the way that the IOMMU API
is structure in Linux, I don't think having multiple stream IDs per (struct)
device, where each StreamID points at a different context (and therefore
address space) makes much sense. It also doesn't solve the more general
problem where StreamIDs for a device might have different SMMUs downstream.

To solve this, I think it is better to treat the device as having multiple
struct device instances, and managing the mappings in the device driver.

For most devices, I'd expect a single context to be enough. This is
certainly the case for device virtualisation at stage-2 and DMA at stage-1.

> I also think that it is more useful to move the stream-id property to
> the device node of a master device. (It's a characteristic of the
> master device not of the SMMU.) Currently with multiple stream IDs per
> master device you have repeated entries in the mmu-master property.

The problem with that approach is how to handle StreamID remastering. This
can and will happen, so the StreamID for a device is actually a property of
both the device *and* a particular point in the bus topology. Putting this
information in the device nodes will drag topology information all over the
place and I don't think it will make things clearer to read or easier to parse.

> But all that is needed is to point (once) to each mmu-master in the
> SMMU device node. Then you should be able to look up the corresponding
> stream IDs in the device node for each mmu-master.

Again, you also need to tie in topology information if you go down this
route.

Will

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:
> Hi Will,

Hi Andreas,

> so far, I thought, that this proposal is fine. After I have tried to
> make use of the binding I have some points that might need further
> disucssion.

Sure, although I've been using the binding without issues so it would be
interesting to understand your use-case and why it's raising problems.

> Olav already asked (in another thread) how to model the mapping of
> stream IDs to contexts for master devices that support multiple
> contexts.  I doubt that this is fully covered yet.

Yeah, I've been meaning to reply to that. Given the way that the IOMMU API
is structure in Linux, I don't think having multiple stream IDs per (struct)
device, where each StreamID points at a different context (and therefore
address space) makes much sense. It also doesn't solve the more general
problem where StreamIDs for a device might have different SMMUs downstream.

To solve this, I think it is better to treat the device as having multiple
struct device instances, and managing the mappings in the device driver.

For most devices, I'd expect a single context to be enough. This is
certainly the case for device virtualisation at stage-2 and DMA at stage-1.

> I also think that it is more useful to move the stream-id property to
> the device node of a master device. (It's a characteristic of the
> master device not of the SMMU.) Currently with multiple stream IDs per
> master device you have repeated entries in the mmu-master property.

The problem with that approach is how to handle StreamID remastering. This
can and will happen, so the StreamID for a device is actually a property of
both the device *and* a particular point in the bus topology. Putting this
information in the device nodes will drag topology information all over the
place and I don't think it will make things clearer to read or easier to parse.

> But all that is needed is to point (once) to each mmu-master in the
> SMMU device node. Then you should be able to look up the corresponding
> stream IDs in the device node for each mmu-master.

Again, you also need to tie in topology information if you go down this
route.

Will

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:
> > Hi Will,
> 
> Hi Andreas,
> 
> > so far, I thought, that this proposal is fine. After I have tried to
> > make use of the binding I have some points that might need further
> > disucssion.
> 
> Sure, although I've been using the binding without issues so it would be
> interesting to understand your use-case and why it's raising problems.
> 
> > Olav already asked (in another thread) how to model the mapping of
> > stream IDs to contexts for master devices that support multiple
> > contexts.  I doubt that this is fully covered yet.
> 
> Yeah, I've been meaning to reply to that. Given the way that the IOMMU API
> is structure in Linux, I don't think having multiple stream IDs per (struct)
> device, where each StreamID points at a different context (and therefore
> address space) makes much sense. It also doesn't solve the more general
> problem where StreamIDs for a device might have different SMMUs downstream.
> 
> To solve this, I think it is better to treat the device as having multiple
> struct device instances, and managing the mappings in the device driver.

Ok.

> For most devices, I'd expect a single context to be enough. This is
> certainly the case for device virtualisation at stage-2 and DMA at stage-1.

Yep, that's the usual case.
(Just thought what other scenarios there are.)

So the DT binding is for the most common (single-context) use case only.
(And everything else needs special device driver treatment.)

> > I also think that it is more useful to move the stream-id property to
> > the device node of a master device. (It's a characteristic of the
> > master device not of the SMMU.) Currently with multiple stream IDs per
> > master device you have repeated entries in the mmu-master property.
> 
> The problem with that approach is how to handle StreamID remastering. This
> can and will happen, so the StreamID for a device is actually a property of
> both the device *and* a particular point in the bus topology. Putting this
> information in the device nodes will drag topology information all over the
> place and I don't think it will make things clearer to read or easier to parse.

Ok, good point, didn't think about that.
And agreed, adding remastered StreamIDs as a property to a device node is odd.

> > But all that is needed is to point (once) to each mmu-master in the
> > SMMU device node. Then you should be able to look up the corresponding
> > stream IDs in the device node for each mmu-master.
> 
> Again, you also need to tie in topology information if you go down this
> route.


Thanks,
Andreas

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:
> > Hi Will,
> 
> Hi Andreas,
> 
> > so far, I thought, that this proposal is fine. After I have tried to
> > make use of the binding I have some points that might need further
> > disucssion.
> 
> Sure, although I've been using the binding without issues so it would be
> interesting to understand your use-case and why it's raising problems.
> 
> > Olav already asked (in another thread) how to model the mapping of
> > stream IDs to contexts for master devices that support multiple
> > contexts.  I doubt that this is fully covered yet.
> 
> Yeah, I've been meaning to reply to that. Given the way that the IOMMU API
> is structure in Linux, I don't think having multiple stream IDs per (struct)
> device, where each StreamID points at a different context (and therefore
> address space) makes much sense. It also doesn't solve the more general
> problem where StreamIDs for a device might have different SMMUs downstream.
> 
> To solve this, I think it is better to treat the device as having multiple
> struct device instances, and managing the mappings in the device driver.

Ok.

> For most devices, I'd expect a single context to be enough. This is
> certainly the case for device virtualisation at stage-2 and DMA at stage-1.

Yep, that's the usual case.
(Just thought what other scenarios there are.)

So the DT binding is for the most common (single-context) use case only.
(And everything else needs special device driver treatment.)

> > I also think that it is more useful to move the stream-id property to
> > the device node of a master device. (It's a characteristic of the
> > master device not of the SMMU.) Currently with multiple stream IDs per
> > master device you have repeated entries in the mmu-master property.
> 
> The problem with that approach is how to handle StreamID remastering. This
> can and will happen, so the StreamID for a device is actually a property of
> both the device *and* a particular point in the bus topology. Putting this
> information in the device nodes will drag topology information all over the
> place and I don't think it will make things clearer to read or easier to parse.

Ok, good point, didn't think about that.
And agreed, adding remastered StreamIDs as a property to a device node is odd.

> > But all that is needed is to point (once) to each mmu-master in the
> > SMMU device node. Then you should be able to look up the corresponding
> > stream IDs in the device node for each mmu-master.
> 
> Again, you also need to tie in topology information if you go down this
> route.


Thanks,
Andreas

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:

[snip]

> > > I also think that it is more useful to move the stream-id property to
> > > the device node of a master device. (It's a characteristic of the
> > > master device not of the SMMU.) Currently with multiple stream IDs per
> > > master device you have repeated entries in the mmu-master property.
> > 
> > The problem with that approach is how to handle StreamID remastering. This
> > can and will happen, so the StreamID for a device is actually a property of
> > both the device *and* a particular point in the bus topology. Putting this
> > information in the device nodes will drag topology information all over the
> > place and I don't think it will make things clearer to read or easier to parse.
> 
> Ok, good point, didn't think about that.
> And agreed, adding remastered StreamIDs as a property to a device node is odd.
> 
> > > But all that is needed is to point (once) to each mmu-master in the
> > > SMMU device node. Then you should be able to look up the corresponding
> > > stream IDs in the device node for each mmu-master.
> > 
> > Again, you also need to tie in topology information if you go down this
> > route.

I still don't like the approach of having two independend lists that
must be in sync to associate a master with its stream-ids.

Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:

     	 smmu {
		...
                mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
                stream-ids =	<0>, <1>, <2>, <3>,
				<4>, <5>, <6>, <7>,
		                <8>, <9>, <0xa>, <0xb>;
	}

Couldn't we use of_phandle_args for this purpose? So your example

+        smmu {
		 ...
+                mmu-masters = <&dma0>,
+                              <&dma0>,
+                              <&dma1>;
+                stream-ids  = <0xd01d>,
+                              <0xd01e>,
+                              <0xd11c>;
+        };

would look like

	dma0 {
		...
		#stream-id-cells = <2>
		...
	}

	dma1 {
		...
		#stream-id-cells = <1>
		...
	}

        smmu {
		...
		mmu-masters = <&dma0 0xd01d 0xd01e
			       &dma1 0xd11c>,
       };

and my example would be converted to

	smmu {
		...
                mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
			       &dma4 6 7 &dma5 8 &dma6 9
			       &dma7 0xa &dma8 0xb>
		...
	}

where each master has #stream-id-cells property with value 1 or 2.
And if dma4 has #stream-id-cells = <1>, the parsing code quickly
notifies us about an error whereas such an error can't be noticed
right from the beginning with the two-list-approach. In this example
stream-id 6 belongs to dma3 which was completely ommitted in both
descriptions.

Of course usage of of_phandle_args would restrict the number of
stream-ids per master to 8 (which is currently used as
MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
practice or do you expect to have more than 8 stream-ids per master
(ie. per struct device in Linux)?


Andreas

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > On Mon, May 13, 2013 at 10:50:20AM +0100, Andreas Herrmann wrote:

[snip]

> > > I also think that it is more useful to move the stream-id property to
> > > the device node of a master device. (It's a characteristic of the
> > > master device not of the SMMU.) Currently with multiple stream IDs per
> > > master device you have repeated entries in the mmu-master property.
> > 
> > The problem with that approach is how to handle StreamID remastering. This
> > can and will happen, so the StreamID for a device is actually a property of
> > both the device *and* a particular point in the bus topology. Putting this
> > information in the device nodes will drag topology information all over the
> > place and I don't think it will make things clearer to read or easier to parse.
> 
> Ok, good point, didn't think about that.
> And agreed, adding remastered StreamIDs as a property to a device node is odd.
> 
> > > But all that is needed is to point (once) to each mmu-master in the
> > > SMMU device node. Then you should be able to look up the corresponding
> > > stream IDs in the device node for each mmu-master.
> > 
> > Again, you also need to tie in topology information if you go down this
> > route.

I still don't like the approach of having two independend lists that
must be in sync to associate a master with its stream-ids.

Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:

     	 smmu {
		...
                mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
                stream-ids =	<0>, <1>, <2>, <3>,
				<4>, <5>, <6>, <7>,
		                <8>, <9>, <0xa>, <0xb>;
	}

Couldn't we use of_phandle_args for this purpose? So your example

+        smmu {
		 ...
+                mmu-masters = <&dma0>,
+                              <&dma0>,
+                              <&dma1>;
+                stream-ids  = <0xd01d>,
+                              <0xd01e>,
+                              <0xd11c>;
+        };

would look like

	dma0 {
		...
		#stream-id-cells = <2>
		...
	}

	dma1 {
		...
		#stream-id-cells = <1>
		...
	}

        smmu {
		...
		mmu-masters = <&dma0 0xd01d 0xd01e
			       &dma1 0xd11c>,
       };

and my example would be converted to

	smmu {
		...
                mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
			       &dma4 6 7 &dma5 8 &dma6 9
			       &dma7 0xa &dma8 0xb>
		...
	}

where each master has #stream-id-cells property with value 1 or 2.
And if dma4 has #stream-id-cells = <1>, the parsing code quickly
notifies us about an error whereas such an error can't be noticed
right from the beginning with the two-list-approach. In this example
stream-id 6 belongs to dma3 which was completely ommitted in both
descriptions.

Of course usage of of_phandle_args would restrict the number of
stream-ids per master to 8 (which is currently used as
MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
practice or do you expect to have more than 8 stream-ids per master
(ie. per struct device in Linux)?


Andreas

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

Hi Andreas,

On Fri, May 17, 2013 at 09:16:39PM +0100, Andreas Herrmann wrote:
> On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> > On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > > Again, you also need to tie in topology information if you go down this
> > > route.
> 
> I still don't like the approach of having two independend lists that
> must be in sync to associate a master with its stream-ids.
> 
> Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:
> 
>      	 smmu {
> 		...
>                 mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
> 			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
> 			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
>                 stream-ids =	<0>, <1>, <2>, <3>,
> 				<4>, <5>, <6>, <7>,
> 		                <8>, <9>, <0xa>, <0xb>;
> 	}
> 
> Couldn't we use of_phandle_args for this purpose? So your example
> 
> +        smmu {
> 		 ...
> +                mmu-masters = <&dma0>,
> +                              <&dma0>,
> +                              <&dma1>;
> +                stream-ids  = <0xd01d>,
> +                              <0xd01e>,
> +                              <0xd11c>;
> +        };
> 
> would look like
> 
> 	dma0 {
> 		...
> 		#stream-id-cells = <2>
> 		...
> 	}
> 
> 	dma1 {
> 		...
> 		#stream-id-cells = <1>
> 		...
> 	}
> 
>         smmu {
> 		...
> 		mmu-masters = <&dma0 0xd01d 0xd01e
> 			       &dma1 0xd11c>,
>        };
> 
> and my example would be converted to
> 
> 	smmu {
> 		...
>                 mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
> 			       &dma4 6 7 &dma5 8 &dma6 9
> 			       &dma7 0xa &dma8 0xb>
> 		...
> 	}

That also looks fine to me, although I'd like to write the parsing code in
my driver before I commit to anything!

> Of course usage of of_phandle_args would restrict the number of
> stream-ids per master to 8 (which is currently used as
> MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
> practice or do you expect to have more than 8 stream-ids per master
> (ie. per struct device in Linux)?

Actually, I think that could be a problem. It doesn't sound unlikely that
multi-channel DMA controllers could have:

	- Separate instruction fetch streamid per channel
	- Separate read/write streamids per channel

so 8 does sound a bit small to me. How difficult would it be to bump that
number in the future if we needed to?

Will

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

Hi Andreas,

On Fri, May 17, 2013 at 09:16:39PM +0100, Andreas Herrmann wrote:
> On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> > On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > > Again, you also need to tie in topology information if you go down this
> > > route.
> 
> I still don't like the approach of having two independend lists that
> must be in sync to associate a master with its stream-ids.
> 
> Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:
> 
>      	 smmu {
> 		...
>                 mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
> 			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
> 			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
>                 stream-ids =	<0>, <1>, <2>, <3>,
> 				<4>, <5>, <6>, <7>,
> 		                <8>, <9>, <0xa>, <0xb>;
> 	}
> 
> Couldn't we use of_phandle_args for this purpose? So your example
> 
> +        smmu {
> 		 ...
> +                mmu-masters = <&dma0>,
> +                              <&dma0>,
> +                              <&dma1>;
> +                stream-ids  = <0xd01d>,
> +                              <0xd01e>,
> +                              <0xd11c>;
> +        };
> 
> would look like
> 
> 	dma0 {
> 		...
> 		#stream-id-cells = <2>
> 		...
> 	}
> 
> 	dma1 {
> 		...
> 		#stream-id-cells = <1>
> 		...
> 	}
> 
>         smmu {
> 		...
> 		mmu-masters = <&dma0 0xd01d 0xd01e
> 			       &dma1 0xd11c>,
>        };
> 
> and my example would be converted to
> 
> 	smmu {
> 		...
>                 mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
> 			       &dma4 6 7 &dma5 8 &dma6 9
> 			       &dma7 0xa &dma8 0xb>
> 		...
> 	}

That also looks fine to me, although I'd like to write the parsing code in
my driver before I commit to anything!

> Of course usage of of_phandle_args would restrict the number of
> stream-ids per master to 8 (which is currently used as
> MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
> practice or do you expect to have more than 8 stream-ids per master
> (ie. per struct device in Linux)?

Actually, I think that could be a problem. It doesn't sound unlikely that
multi-channel DMA controllers could have:

	- Separate instruction fetch streamid per channel
	- Separate read/write streamids per channel

so 8 does sound a bit small to me. How difficult would it be to bump that
number in the future if we needed to?

Will

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> Hi Andreas,
> 
> On Fri, May 17, 2013 at 09:16:39PM +0100, Andreas Herrmann wrote:
> > On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> > > On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > > > Again, you also need to tie in topology information if you go down this
> > > > route.
> > 
> > I still don't like the approach of having two independend lists that
> > must be in sync to associate a master with its stream-ids.
> > 
> > Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:
> > 
> >      	 smmu {
> > 		...
> >                 mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
> > 			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
> > 			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
> >                 stream-ids =	<0>, <1>, <2>, <3>,
> > 				<4>, <5>, <6>, <7>,
> > 		                <8>, <9>, <0xa>, <0xb>;
> > 	}
> > 
> > Couldn't we use of_phandle_args for this purpose? So your example
> > 
> > +        smmu {
> > 		 ...
> > +                mmu-masters = <&dma0>,
> > +                              <&dma0>,
> > +                              <&dma1>;
> > +                stream-ids  = <0xd01d>,
> > +                              <0xd01e>,
> > +                              <0xd11c>;
> > +        };
> > 
> > would look like
> > 
> > 	dma0 {
> > 		...
> > 		#stream-id-cells = <2>
> > 		...
> > 	}
> > 
> > 	dma1 {
> > 		...
> > 		#stream-id-cells = <1>
> > 		...
> > 	}
> > 
> >         smmu {
> > 		...
> > 		mmu-masters = <&dma0 0xd01d 0xd01e
> > 			       &dma1 0xd11c>,
> >        };
> > 
> > and my example would be converted to
> > 
> > 	smmu {
> > 		...
> >                 mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
> > 			       &dma4 6 7 &dma5 8 &dma6 9
> > 			       &dma7 0xa &dma8 0xb>
> > 		...
> > 	}
> 
> That also looks fine to me, although I'd like to write the parsing code in
> my driver before I commit to anything!
> 
> > Of course usage of of_phandle_args would restrict the number of
> > stream-ids per master to 8 (which is currently used as
> > MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
> > practice or do you expect to have more than 8 stream-ids per master
> > (ie. per struct device in Linux)?
> 
> Actually, I think that could be a problem. It doesn't sound unlikely that
> multi-channel DMA controllers could have:
> 
> 	- Separate instruction fetch streamid per channel
> 	- Separate read/write streamids per channel
> 
> so 8 does sound a bit small to me. How difficult would it be to bump that
> number in the future if we needed to?

Seems it was introduced with commit 15c9a0acc3f7873db4b7d35d016729b2dc229b49
(of: create of_phandle_args to simplify return of phandle parsing data)
by Grant Likely.

The macro is primarily used in struct of_phandle_args. I don't believe
that increasing the number of args (e.g. doubling it if necessary)
would cause objections.

struct of_phandle_args is used as argument for various (parsing
functions). In several functions objects of that type are on the
stack.

And there is a private data structure in gpiolib-of.c which also
contains an object of that type:

  struct gg_data {
        enum of_gpio_flags *flags;
        struct of_phandle_args gpiospec;
        int out_gpio;
  };

I don't expect that stack overflows or significant blowup of kernel
size will be casued by a moderate bump of MAX_PHANDLE_ARGS. I think
doubling it or even increasing it to 32 will not cause issues.

(Of course its impossible to increase it to the theoretical maximum of
32k (15-bit) stream-ids. But 32k stream-ids for just one Linux device?
I'd say practically irrelevant.)


Andreas

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> Hi Andreas,
> 
> On Fri, May 17, 2013 at 09:16:39PM +0100, Andreas Herrmann wrote:
> > On Mon, May 13, 2013 at 12:41:47PM +0200, Andreas Herrmann wrote:
> > > On Mon, May 13, 2013 at 05:58:46AM -0400, Will Deacon wrote:
> > > > Again, you also need to tie in topology information if you go down this
> > > > route.
> > 
> > I still don't like the approach of having two independend lists that
> > must be in sync to associate a master with its stream-ids.
> > 
> > Why? Say you have 8 masters for an SMMU with 1 or 2 stream-ids each:
> > 
> >      	 smmu {
> > 		...
> >                 mmu-masters = <&dma0>, <&dma0>, <&dma1>, <&dma1>,
> > 			      <&dma2>, <&dma2>, <&dma4>, <&dma4>,
> > 			      <&dma5>, <&dma6>, <&dma7>, <&dma8>;
> >                 stream-ids =	<0>, <1>, <2>, <3>,
> > 				<4>, <5>, <6>, <7>,
> > 		                <8>, <9>, <0xa>, <0xb>;
> > 	}
> > 
> > Couldn't we use of_phandle_args for this purpose? So your example
> > 
> > +        smmu {
> > 		 ...
> > +                mmu-masters = <&dma0>,
> > +                              <&dma0>,
> > +                              <&dma1>;
> > +                stream-ids  = <0xd01d>,
> > +                              <0xd01e>,
> > +                              <0xd11c>;
> > +        };
> > 
> > would look like
> > 
> > 	dma0 {
> > 		...
> > 		#stream-id-cells = <2>
> > 		...
> > 	}
> > 
> > 	dma1 {
> > 		...
> > 		#stream-id-cells = <1>
> > 		...
> > 	}
> > 
> >         smmu {
> > 		...
> > 		mmu-masters = <&dma0 0xd01d 0xd01e
> > 			       &dma1 0xd11c>,
> >        };
> > 
> > and my example would be converted to
> > 
> > 	smmu {
> > 		...
> >                 mmu-masters = <&dma0 0 1 &dma1 2 3 &dma2 4 5
> > 			       &dma4 6 7 &dma5 8 &dma6 9
> > 			       &dma7 0xa &dma8 0xb>
> > 		...
> > 	}
> 
> That also looks fine to me, although I'd like to write the parsing code in
> my driver before I commit to anything!
> 
> > Of course usage of of_phandle_args would restrict the number of
> > stream-ids per master to 8 (which is currently used as
> > MAX_PHANDLE_ARGS). But I don't think that this is a restriction in
> > practice or do you expect to have more than 8 stream-ids per master
> > (ie. per struct device in Linux)?
> 
> Actually, I think that could be a problem. It doesn't sound unlikely that
> multi-channel DMA controllers could have:
> 
> 	- Separate instruction fetch streamid per channel
> 	- Separate read/write streamids per channel
> 
> so 8 does sound a bit small to me. How difficult would it be to bump that
> number in the future if we needed to?

Seems it was introduced with commit 15c9a0acc3f7873db4b7d35d016729b2dc229b49
(of: create of_phandle_args to simplify return of phandle parsing data)
by Grant Likely.

The macro is primarily used in struct of_phandle_args. I don't believe
that increasing the number of args (e.g. doubling it if necessary)
would cause objections.

struct of_phandle_args is used as argument for various (parsing
functions). In several functions objects of that type are on the
stack.

And there is a private data structure in gpiolib-of.c which also
contains an object of that type:

  struct gg_data {
        enum of_gpio_flags *flags;
        struct of_phandle_args gpiospec;
        int out_gpio;
  };

I don't expect that stack overflows or significant blowup of kernel
size will be casued by a moderate bump of MAX_PHANDLE_ARGS. I think
doubling it or even increasing it to 32 will not cause issues.

(Of course its impossible to increase it to the theoretical maximum of
32k (15-bit) stream-ids. But 32k stream-ids for just one Linux device?
I'd say practically irrelevant.)


Andreas

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

On Tue, May 21, 2013 at 11:25:01AM +0100, Andreas Herrmann wrote:
> On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> > That also looks fine to me, although I'd like to write the parsing code in
> > my driver before I commit to anything!

Right, I hacked that up this afternoon and it seems to be working ok. I'll
post a revised binding soon (hopefully I'll include my driver this time too!).

Will

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Will Deacon]

On Tue, May 21, 2013 at 11:25:01AM +0100, Andreas Herrmann wrote:
> On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> > That also looks fine to me, although I'd like to write the parsing code in
> > my driver before I commit to anything!

Right, I hacked that up this afternoon and it seems to be working ok. I'll
post a revised binding soon (hopefully I'll include my driver this time too!).

Will

Re: [PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Tue, May 21, 2013 at 01:33:58PM -0400, Will Deacon wrote:
> On Tue, May 21, 2013 at 11:25:01AM +0100, Andreas Herrmann wrote:
> > On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> > > That also looks fine to me, although I'd like to write the parsing code in
> > > my driver before I commit to anything!
> 
> Right, I hacked that up this afternoon and it seems to be working ok. I'll
> post a revised binding soon (hopefully I'll include my driver this time too!).

Cool.


Thanks,

Andreas

[PATCH v2] documentation: iommu: add description of ARM System MMU binding

[Andreas Herrmann]

On Tue, May 21, 2013 at 01:33:58PM -0400, Will Deacon wrote:
> On Tue, May 21, 2013 at 11:25:01AM +0100, Andreas Herrmann wrote:
> > On Mon, May 20, 2013 at 06:18:41AM -0400, Will Deacon wrote:
> > > That also looks fine to me, although I'd like to write the parsing code in
> > > my driver before I commit to anything!
> 
> Right, I hacked that up this afternoon and it seems to be working ok. I'll
> post a revised binding soon (hopefully I'll include my driver this time too!).

Cool.


Thanks,

Andreas