[PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Add device tree support for Tegra30 IOMMU(SMMU).

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/arm/tegra/tegra30-smmu.txt |   19 +++++++++++++++++++
 arch/arm/boot/dts/tegra30.dtsi                     |   10 ++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
new file mode 100644
index 0000000..4bd8cd0
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
@@ -0,0 +1,19 @@
+NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
+
+Required properties:
+- compatible : "nvidia,tegra30-smmu"
+- reg : Should contain MC registers location and length
+- reg : Should contain AHB Arbitration registers and length
+- reg : Should contain virtual address space range
+- interrupts : Should contain MC General interrupt
+
+Example:
+	smmu: smmu@7000f000 {
+		compatible = "nvidia,tegra30-smmu";
+		reg = < 0x7000f000 0x400	/* controller registers */
+			0x6000c000 0x150	/* AHB Arbitration registers */
+			0x00001000 0x3ffff000 >;/* Virtual address space range
+						 * Exclude the 1st & last page
+						 */
+		interrupts = < 0 13 0x40 >;
+	};
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 62a7b39..c640a5b 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -183,4 +183,14 @@
 		reg = < 0x70000868 0xd0     /* Pad control registers */
 			0x70003000 0x3e0 >; /* Mux registers */
 	};
+
+	smmu: smmu@7000f000 {
+		compatible = "nvidia,tegra30-smmu";
+		reg = < 0x7000f000 0x400	/* controller registers */
+			0x6000c000 0x150	/* AHB Arbitration registers */
+			0x00001000 0x3ffff000 >;/* Virtual address space range
+						 * Exclude the 1st & last page
+						 */
+		interrupts = < 0 13 0x40 >;
+	};
 };
-- 
1.7.5.4

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2308 bytes --]

* Hiroshi Doyu wrote:
> From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Add device tree support for Tegra30 IOMMU(SMMU).
> 
> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/tegra/tegra30-smmu.txt |   19 +++++++++++++++++++
>  arch/arm/boot/dts/tegra30.dtsi                     |   10 ++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
> new file mode 100644
> index 0000000..4bd8cd0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
> @@ -0,0 +1,19 @@
> +NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
> +
> +Required properties:
> +- compatible : "nvidia,tegra30-smmu"
> +- reg : Should contain MC registers location and length
> +- reg : Should contain AHB Arbitration registers and length
> +- reg : Should contain virtual address space range
> +- interrupts : Should contain MC General interrupt
> +
> +Example:
> +	smmu: smmu@7000f000 {
> +		compatible = "nvidia,tegra30-smmu";
> +		reg = < 0x7000f000 0x400	/* controller registers */
> +			0x6000c000 0x150	/* AHB Arbitration registers */
> +			0x00001000 0x3ffff000 >;/* Virtual address space range
> +						 * Exclude the 1st & last page
> +						 */
> +		interrupts = < 0 13 0x40 >;
> +	};
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 62a7b39..c640a5b 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -183,4 +183,14 @@
>  		reg = < 0x70000868 0xd0     /* Pad control registers */
>  			0x70003000 0x3e0 >; /* Mux registers */
>  	};
> +
> +	smmu: smmu@7000f000 {
> +		compatible = "nvidia,tegra30-smmu";
> +		reg = < 0x7000f000 0x400	/* controller registers */
> +			0x6000c000 0x150	/* AHB Arbitration registers */
> +			0x00001000 0x3ffff000 >;/* Virtual address space range
> +						 * Exclude the 1st & last page
> +						 */
> +		interrupts = < 0 13 0x40 >;
> +	};
>  };

Why is the virtual address space range limited to 1 GiB? What is the reason
for the exclusion of the first and last pages?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

Hi Thierry,

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Subject: Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU
Date: Fri, 13 Apr 2012 13:03:32 +0200
Message-ID: <20120413110331.GA11605-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>

> * PGP Signed by an unknown key
> 
> * Hiroshi Doyu wrote:
> > From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Add device tree support for Tegra30 IOMMU(SMMU).
> > 
> > Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../devicetree/bindings/arm/tegra/tegra30-smmu.txt |   19 +++++++++++++++++++
> >  arch/arm/boot/dts/tegra30.dtsi                     |   10 ++++++++++
> >  2 files changed, 29 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
> > new file mode 100644
> > index 0000000..4bd8cd0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
> > @@ -0,0 +1,19 @@
> > +NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
> > +
> > +Required properties:
> > +- compatible : "nvidia,tegra30-smmu"
> > +- reg : Should contain MC registers location and length
> > +- reg : Should contain AHB Arbitration registers and length
> > +- reg : Should contain virtual address space range
> > +- interrupts : Should contain MC General interrupt
> > +
> > +Example:
> > +	smmu: smmu@7000f000 {
> > +		compatible = "nvidia,tegra30-smmu";
> > +		reg = < 0x7000f000 0x400	/* controller registers */
> > +			0x6000c000 0x150	/* AHB Arbitration registers */
> > +			0x00001000 0x3ffff000 >;/* Virtual address space range
> > +						 * Exclude the 1st & last page
> > +						 */
> > +		interrupts = < 0 13 0x40 >;
> > +	};
> > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> > index 62a7b39..c640a5b 100644
> > --- a/arch/arm/boot/dts/tegra30.dtsi
> > +++ b/arch/arm/boot/dts/tegra30.dtsi
> > @@ -183,4 +183,14 @@
> >  		reg = < 0x70000868 0xd0     /* Pad control registers */
> >  			0x70003000 0x3e0 >; /* Mux registers */
> >  	};
> > +
> > +	smmu: smmu@7000f000 {
> > +		compatible = "nvidia,tegra30-smmu";
> > +		reg = < 0x7000f000 0x400	/* controller registers */
> > +			0x6000c000 0x150	/* AHB Arbitration registers */
> > +			0x00001000 0x3ffff000 >;/* Virtual address space range
> > +						 * Exclude the 1st & last page
> > +						 */
> > +		interrupts = < 0 13 0x40 >;
> > +	};
> >  };
> 
> Why is the virtual address space range limited to 1 GiB? What is the reason
> for the exclusion of the first and last pages?

It's because physical RAM is located 2-4GB, and we may want to use
those area 1-1(V==P) mapping in some cases. This could be extended
with larger RAM without 1-1 mapping theoretically. So far 1GB seems to
be enough.

The 1st page for AVP vector, and the last one is required by another
H/W entity.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

* Hiroshi Doyu wrote:
> Thierry Reding wrote:
> > * Hiroshi Doyu wrote:
> > > +	smmu: smmu@7000f000 {
> > > +		compatible = "nvidia,tegra30-smmu";
> > > +		reg = < 0x7000f000 0x400	/* controller registers */
> > > +			0x6000c000 0x150	/* AHB Arbitration registers */
> > > +			0x00001000 0x3ffff000 >;/* Virtual address space range
> > > +						 * Exclude the 1st & last page
> > > +						 */
> > > +		interrupts = < 0 13 0x40 >;
> > > +	};
> > >  };
> > 
> > Why is the virtual address space range limited to 1 GiB? What is the reason
> > for the exclusion of the first and last pages?
> 
> It's because physical RAM is located 2-4GB, and we may want to use
> those area 1-1(V==P) mapping in some cases. This could be extended
> with larger RAM without 1-1 mapping theoretically. So far 1GB seems to
> be enough.

I'm thinking that this would be better off in a separate property so that
it's easier for boards to override it.

> The 1st page for AVP vector, and the last one is required by another
> H/W entity.

I would expect such peculiarities to be handled by the driver internally.
That way users wouldn't have to know or care about these kind of details.
If it can't be handled by the driver then at least it should be mentioned
explicitly in the binding documentation.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Stephen Warren]

On 04/13/2012 06:15 AM, Thierry Reding wrote:
> * Hiroshi Doyu wrote:
>> Thierry Reding wrote:
>>> * Hiroshi Doyu wrote:
>>>> +	smmu: smmu@7000f000 {
>>>> +		compatible = "nvidia,tegra30-smmu";
>>>> +		reg = < 0x7000f000 0x400	/* controller registers */
>>>> +			0x6000c000 0x150	/* AHB Arbitration registers */
>>>> +			0x00001000 0x3ffff000 >;/* Virtual address space range
>>>> +						 * Exclude the 1st & last page
>>>> +						 */
>>>> +		interrupts = < 0 13 0x40 >;
>>>> +	};
>>>>  };
>>>
>>> Why is the virtual address space range limited to 1 GiB? What is the reason
>>> for the exclusion of the first and last pages?
>>
>> It's because physical RAM is located 2-4GB, and we may want to use
>> those area 1-1(V==P) mapping in some cases. This could be extended
>> with larger RAM without 1-1 mapping theoretically. So far 1GB seems to
>> be enough.
> 
> I'm thinking that this would be better off in a separate property so that
> it's easier for boards to override it.

Yes, and for another reason: The entries in the reg property are
supposed to be within the bus address space that contains the device,
whereas here this 3rd reg entry is being used to define something about
an entirely unrelated address space - the virtual address space seen by
SMMU clients.

>> The 1st page for AVP vector, and the last one is required by another
>> H/W entity.
> 
> I would expect such peculiarities to be handled by the driver internally.
> That way users wouldn't have to know or care about these kind of details.
> If it can't be handled by the driver then at least it should be mentioned
> explicitly in the binding documentation.

I agree, but:

Why do those pages even need special handling? Doesn't the AVP get its
own address space ID, and then can set it up however it wants?

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU
Date: Fri, 13 Apr 2012 21:25:20 +0200
Message-ID: <4F887DA0.8030103-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 04/13/2012 06:15 AM, Thierry Reding wrote:
> > * Hiroshi Doyu wrote:
> >> Thierry Reding wrote:
> >>> * Hiroshi Doyu wrote:
> >>>> +	smmu: smmu@7000f000 {
> >>>> +		compatible = "nvidia,tegra30-smmu";
> >>>> +		reg = < 0x7000f000 0x400	/* controller registers */
> >>>> +			0x6000c000 0x150	/* AHB Arbitration registers */
> >>>> +			0x00001000 0x3ffff000 >;/* Virtual address space range
> >>>> +						 * Exclude the 1st & last page
> >>>> +						 */
> >>>> +		interrupts = < 0 13 0x40 >;
> >>>> +	};
> >>>>  };
> >>>
> >>> Why is the virtual address space range limited to 1 GiB? What is the reason
> >>> for the exclusion of the first and last pages?
> >>
> >> It's because physical RAM is located 2-4GB, and we may want to use
> >> those area 1-1(V==P) mapping in some cases. This could be extended
> >> with larger RAM without 1-1 mapping theoretically. So far 1GB seems to
> >> be enough.
> >
> > I'm thinking that this would be better off in a separate property so that
> > it's easier for boards to override it.
>
> Yes, and for another reason: The entries in the reg property are
> supposed to be within the bus address space that contains the device,
> whereas here this 3rd reg entry is being used to define something about
> an entirely unrelated address space - the virtual address space seen by
> SMMU clients.

What about using "dma-window" property to specify IOVA range in dtsi as below?

arch/powerpc/platforms/cell/iommu.c:

698 static int __init cell_iommu_get_window(struct device_node *np,
699                                          unsigned long *base,
700                                          unsigned long *size)
701 {
702         const void *dma_window;
703         unsigned long index;
704
705         /* Use ibm,dma-window if available, else, hard code ! */
706         dma_window = of_get_property(np, "ibm,dma-window", NULL);
707         if (dma_window == NULL) {          ^^^^^^^^^^^^^^
708                 *base = 0;
709                 *size = 0x80000000u;
710                 return -ENODEV;
711         }
712
713         of_parse_dma_window(np, dma_window, &index, base, size);
714         return 0;
715 }


> >> The 1st page for AVP vector, and the last one is required by another
> >> H/W entity.
> >
> > I would expect such peculiarities to be handled by the driver internally.
> > That way users wouldn't have to know or care about these kind of details.
> > If it can't be handled by the driver then at least it should be mentioned
> > explicitly in the binding documentation.
>
> I agree, but:
>
> Why do those pages even need special handling? Doesn't the AVP get its
> own address space ID, and then can set it up however it wants?

This sounds better than dealing with prefixed mappings at booting. At
least we need to pass # of ASIDs, and client should claim one of them.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Arnd Bergmann]

On Monday 16 April 2012, Hiroshi Doyu wrote:
> What about using "dma-window" property to specify IOVA range in dtsi as below?
> 
> arch/powerpc/platforms/cell/iommu.c:
> 
> 698 static int __init cell_iommu_get_window(struct device_node *np,
> 699                                          unsigned long *base,
> 700                                          unsigned long *size)
> 701 {
> 702         const void *dma_window;
> 703         unsigned long index;
> 704
> 705         /* Use ibm,dma-window if available, else, hard code ! */
> 706         dma_window = of_get_property(np, "ibm,dma-window", NULL);
> 707         if (dma_window == NULL) {          ^^^^^^^^^^^^^^
> 708                 *base = 0;
> 709                 *size = 0x80000000u;
> 710                 return -ENODEV;
> 711         }
> 712
> 713         of_parse_dma_window(np, dma_window, &index, base, size);
> 714         return 0;
> 715 }
> 

Yes, that's the right way to do it, but I would use the more generic
"dma-window" name rather than "ibm,dma-window", which was originally
introduced for pseries and for some reason copied into the cell qs2x
firmware.

	Arnd

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

On Mon, 16 Apr 2012 17:34:19 +0200
Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> On Monday 16 April 2012, Hiroshi Doyu wrote:
> > What about using "dma-window" property to specify IOVA range in dtsi as below?
> > 
> > arch/powerpc/platforms/cell/iommu.c:
> > 
> > 698 static int __init cell_iommu_get_window(struct device_node *np,
> > 699                                          unsigned long *base,
> > 700                                          unsigned long *size)
> > 701 {
> > 702         const void *dma_window;
> > 703         unsigned long index;
> > 704
> > 705         /* Use ibm,dma-window if available, else, hard code ! */
> > 706         dma_window = of_get_property(np, "ibm,dma-window", NULL);
> > 707         if (dma_window == NULL) {          ^^^^^^^^^^^^^^
> > 708                 *base = 0;
> > 709                 *size = 0x80000000u;
> > 710                 return -ENODEV;
> > 711         }
> > 712
> > 713         of_parse_dma_window(np, dma_window, &index, base, size);
> > 714         return 0;
> > 715 }
> > 
> 
> Yes, that's the right way to do it, but I would use the more generic
> "dma-window" name rather than "ibm,dma-window", which was originally
> introduced for pseries and for some reason copied into the cell qs2x
> firmware.

Which name is better, "dma-window" or "iova-window"?
Considering DMA IOMMU mapping API, "dma-window" may sound appropriate to me.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

* Hiroshi Doyu wrote:
> On Mon, 16 Apr 2012 17:34:19 +0200
> Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> 
> > On Monday 16 April 2012, Hiroshi Doyu wrote:
> > > What about using "dma-window" property to specify IOVA range in dtsi as below?
> > > 
> > > arch/powerpc/platforms/cell/iommu.c:
> > > 
> > > 698 static int __init cell_iommu_get_window(struct device_node *np,
> > > 699                                          unsigned long *base,
> > > 700                                          unsigned long *size)
> > > 701 {
> > > 702         const void *dma_window;
> > > 703         unsigned long index;
> > > 704
> > > 705         /* Use ibm,dma-window if available, else, hard code ! */
> > > 706         dma_window = of_get_property(np, "ibm,dma-window", NULL);
> > > 707         if (dma_window == NULL) {          ^^^^^^^^^^^^^^
> > > 708                 *base = 0;
> > > 709                 *size = 0x80000000u;
> > > 710                 return -ENODEV;
> > > 711         }
> > > 712
> > > 713         of_parse_dma_window(np, dma_window, &index, base, size);
> > > 714         return 0;
> > > 715 }
> > > 
> > 
> > Yes, that's the right way to do it, but I would use the more generic
> > "dma-window" name rather than "ibm,dma-window", which was originally
> > introduced for pseries and for some reason copied into the cell qs2x
> > firmware.
> 
> Which name is better, "dma-window" or "iova-window"?
> Considering DMA IOMMU mapping API, "dma-window" may sound appropriate to me.

But the window isn't necessarily used for DMA transfers. I was going to
propose "aperture", but that doesn't seem quite right since it is a very
GART-specific term. "iova-window" sounds like the best fit for what it
really is.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Subject: Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU
Date: Wed, 18 Apr 2012 07:41:40 +0200
Message-ID: <20120418054140.GA17506-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>

> * PGP Signed by an unknown key
> 
> * Hiroshi Doyu wrote:
> > On Mon, 16 Apr 2012 17:34:19 +0200
> > Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > 
> > > On Monday 16 April 2012, Hiroshi Doyu wrote:
> > > > What about using "dma-window" property to specify IOVA range in dtsi as below?
> > > > 
> > > > arch/powerpc/platforms/cell/iommu.c:
> > > > 
> > > > 698 static int __init cell_iommu_get_window(struct device_node *np,
> > > > 699                                          unsigned long *base,
> > > > 700                                          unsigned long *size)
> > > > 701 {
> > > > 702         const void *dma_window;
> > > > 703         unsigned long index;
> > > > 704
> > > > 705         /* Use ibm,dma-window if available, else, hard code ! */
> > > > 706         dma_window = of_get_property(np, "ibm,dma-window", NULL);
> > > > 707         if (dma_window == NULL) {          ^^^^^^^^^^^^^^
> > > > 708                 *base = 0;
> > > > 709                 *size = 0x80000000u;
> > > > 710                 return -ENODEV;
> > > > 711         }
> > > > 712
> > > > 713         of_parse_dma_window(np, dma_window, &index, base, size);
> > > > 714         return 0;
> > > > 715 }
> > > > 
> > > 
> > > Yes, that's the right way to do it, but I would use the more generic
> > > "dma-window" name rather than "ibm,dma-window", which was originally
> > > introduced for pseries and for some reason copied into the cell qs2x
> > > firmware.
> > 
> > Which name is better, "dma-window" or "iova-window"?
> > Considering DMA IOMMU mapping API, "dma-window" may sound appropriate to me.
> 
> But the window isn't necessarily used for DMA transfers. I was going to
> propose "aperture", but that doesn't seem quite right since it is a very
> GART-specific term. "iova-window" sounds like the best fit for what it
> really is.

If we can consider IOMMU as one of the implementation of DMA(Direct
Memory Access), the prefix "dma-(window)" may make sense here. Then,
we don't have to introduce a new concept "IO virtual address(iova)" in
addition to the existing "bus address"(?)

Anyway, either name would be ok for me;)

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Arnd Bergmann]

On Wednesday 18 April 2012, Hiroshi Doyu wrote:
> If we can consider IOMMU as one of the implementation of DMA(Direct
> Memory Access), the prefix "dma-(window)" may make sense here. Then,
> we don't have to introduce a new concept "IO virtual address(iova)" in
> addition to the existing "bus address"(?)
> 
> Anyway, either name would be ok for me;)

I would just use dma-window, without the "ibm," prefix but following the
same conventions. Note that the of_parse_dma_window function is currently
only defined in powerpc specific code, and should get moved to
drivers/of from arch/powerpc/kernel/prom_parse.c.

	Arnd

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Subject: Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU
Date: Wed, 18 Apr 2012 09:31:53 +0200
Message-ID: <201204180731.54064.arnd-r2nGTMty4D4@public.gmane.org>

> On Wednesday 18 April 2012, Hiroshi Doyu wrote:
> > If we can consider IOMMU as one of the implementation of DMA(Direct
> > Memory Access), the prefix "dma-(window)" may make sense here. Then,
> > we don't have to introduce a new concept "IO virtual address(iova)" in
> > addition to the existing "bus address"(?)
> > 
> > Anyway, either name would be ok for me;)
> 
> I would just use dma-window, without the "ibm," prefix but following the
> same conventions. Note that the of_parse_dma_window function is currently
> only defined in powerpc specific code, and should get moved to
> drivers/of from arch/powerpc/kernel/prom_parse.c.

Something like below?

At least, I verified that this works with "tegra-smmu".

>From 67c1dd493637c9e972d04e061a8e67049687021a Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 18 Apr 2012 12:09:03 +0300
Subject: [PATCH 1/1] dt: Add general DMA window parser

This code was stolen from:
	"arch/microblaze/kernel/prom_parse.c"
	"arch/powerpc/kernel/prom_parse.c"

Once "ibm," prefix is removed from dts file. This generic one could
replace the originals.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/Kconfig         |    4 ++++
 drivers/of/Makefile        |    1 +
 drivers/of/of_dma.c        |   35 +++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   10 ++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..3b0298b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,8 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DMA
+	depends on HAS_DMA
+	def_bool y
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..711ff5b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DMA)	+= of_dma.o
diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
new file mode 100644
index 0000000..1db1ccd
--- /dev/null
+++ b/drivers/of/of_dma.c
@@ -0,0 +1,35 @@
+/*
+ * Stealed from:
+ *	"arch/microblaze/kernel/prom_parse.c"
+ *	"arch/powerpc/kernel/prom_parse.c"
+ */
+
+#include <linux/of_address.h>
+
+void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
+		unsigned long *busno, unsigned long *phys, unsigned long *size)
+{
+	const u32 *dma_window;
+	u32 cells;
+	const unsigned char *prop;
+
+	dma_window = dma_window_prop;
+
+	/* busno is always one cell */
+	if (busno)
+		*busno = *(dma_window++);
+
+	prop = of_get_property(dn, "#dma-address-cells", NULL);
+	if (!prop)
+		prop = of_get_property(dn, "#address-cells", NULL);
+
+	cells = prop ? *(u32 *)prop : of_n_addr_cells(dn);
+	*phys = of_read_number(dma_window, cells);
+
+	dma_window += cells;
+
+	prop = of_get_property(dn, "#dma-size-cells", NULL);
+	cells = prop ? *(u32 *)prop : of_n_size_cells(dn);
+	*size = of_read_number(dma_window, cells);
+}
+
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..2a0f7c6 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -21,6 +21,10 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const u32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size);
+
 #ifndef pci_address_to_pio
 static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
@@ -48,6 +52,12 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+
+static inline void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size)
+{
+}
 #endif /* CONFIG_OF_ADDRESS */
 
 
-- 
1.7.5.4

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

* Hiroshi Doyu wrote:
> diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
> new file mode 100644
> index 0000000..1db1ccd
> --- /dev/null
> +++ b/drivers/of/of_dma.c
> @@ -0,0 +1,35 @@
> +/*
> + * Stealed from:

"Stolen from"

> + *	"arch/microblaze/kernel/prom_parse.c"
> + *	"arch/powerpc/kernel/prom_parse.c"
> + */
> +
> +#include <linux/of_address.h>
> +
> +void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
> +		unsigned long *busno, unsigned long *phys, unsigned long *size)
> +{
> +	const u32 *dma_window;

Should be __be32.

> +	u32 cells;
> +	const unsigned char *prop;
> +
> +	dma_window = dma_window_prop;
> +
> +	/* busno is always one cell */
> +	if (busno)
> +		*busno = *(dma_window++);

This needs endianness conversion:

	*busno = be32_to_cpup(dma_window++);

> +
> +	prop = of_get_property(dn, "#dma-address-cells", NULL);
> +	if (!prop)
> +		prop = of_get_property(dn, "#address-cells", NULL);
> +
> +	cells = prop ? *(u32 *)prop : of_n_addr_cells(dn);

Same here.

> +	*phys = of_read_number(dma_window, cells);
> +
> +	dma_window += cells;
> +
> +	prop = of_get_property(dn, "#dma-size-cells", NULL);
> +	cells = prop ? *(u32 *)prop : of_n_size_cells(dn);

And here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

Hi,

Try again with fixing Thierry's commnets, Thanks.

>From 9a632c24949e46df197a216ca95f684edd1db693 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 18 Apr 2012 12:09:03 +0300
Subject: [PATCH 1/1] dt: Add general DMA window parser

This code was stolen from:
	"arch/microblaze/kernel/prom_parse.c"
	"arch/powerpc/kernel/prom_parse.c"

Once "ibm," prefix is removed from dts file. This generic one could
replace the originals.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/Kconfig         |    4 ++++
 drivers/of/Makefile        |    1 +
 drivers/of/of_dma.c        |   35 +++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   10 ++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..3b0298b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,8 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DMA
+	depends on HAS_DMA
+	def_bool y
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..711ff5b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DMA)	+= of_dma.o
diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
new file mode 100644
index 0000000..a34db5a
--- /dev/null
+++ b/drivers/of/of_dma.c
@@ -0,0 +1,35 @@
+/*
+ * Stolen from:
+ *	"arch/microblaze/kernel/prom_parse.c"
+ *	"arch/powerpc/kernel/prom_parse.c"
+ */
+
+#include <linux/of_address.h>
+
+void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
+		unsigned long *busno, unsigned long *phys, unsigned long *size)
+{
+	const __be32 *dma_window;
+	u32 cells;
+	const unsigned char *prop;
+
+	dma_window = dma_window_prop;
+
+	/* busno is always one cell */
+	if (busno)
+		*busno = be32_to_cpup(dma_window++);
+
+	prop = of_get_property(dn, "#dma-address-cells", NULL);
+	if (!prop)
+		prop = of_get_property(dn, "#address-cells", NULL);
+
+	cells = prop ? *(__be32 *)prop : of_n_addr_cells(dn);
+	*phys = of_read_number(dma_window, cells);
+
+	dma_window += cells;
+
+	prop = of_get_property(dn, "#dma-size-cells", NULL);
+	cells = prop ? *(__be32 *)prop : of_n_size_cells(dn);
+	*size = of_read_number(dma_window, cells);
+}
+
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..2a0f7c6 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -21,6 +21,10 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const u32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size);
+
 #ifndef pci_address_to_pio
 static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
@@ -48,6 +52,12 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+
+static inline void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size)
+{
+}
 #endif /* CONFIG_OF_ADDRESS */
 
 
-- 
1.7.5.4

Re: [PATCH 1/1] dt: Add general DMA window parser

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 367 bytes --]

* Hiroshi Doyu wrote:
> +	cells = prop ? *(__be32 *)prop : of_n_addr_cells(dn);

I think this needs to be:

	cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);

Only casting isn't enough, you need the bytes to be swapped.

> +	cells = prop ? *(__be32 *)prop : of_n_size_cells(dn);

Similarly:

	cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Subject: Re: [PATCH 1/1] dt: Add general DMA window parser
Date: Wed, 18 Apr 2012 12:26:29 +0200
Message-ID: <20120418102629.GA14533-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>

> * PGP Signed by an unknown key
> 
> * Hiroshi Doyu wrote:
> > +	cells = prop ? *(__be32 *)prop : of_n_addr_cells(dn);
> 
> I think this needs to be:
> 
> 	cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
> 
> Only casting isn't enough, you need the bytes to be swapped.

Right. Try again. Sorry for spamming.

>From 1ee8a9b3a839456b170af74b11a4304bfda965c4 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 18 Apr 2012 12:09:03 +0300
Subject: [PATCH 1/1] dt: Add general DMA window parser

This code was stolen from:
	"arch/microblaze/kernel/prom_parse.c"
	"arch/powerpc/kernel/prom_parse.c"

Once "ibm," prefix is removed from dts file. This generic one could
replace the originals.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/Kconfig         |    4 ++++
 drivers/of/Makefile        |    1 +
 drivers/of/of_dma.c        |   35 +++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   10 ++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..3b0298b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,8 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DMA
+	depends on HAS_DMA
+	def_bool y
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..711ff5b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DMA)	+= of_dma.o
diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
new file mode 100644
index 0000000..45c9e88
--- /dev/null
+++ b/drivers/of/of_dma.c
@@ -0,0 +1,35 @@
+/*
+ * Stolen from:
+ *	"arch/microblaze/kernel/prom_parse.c"
+ *	"arch/powerpc/kernel/prom_parse.c"
+ */
+
+#include <linux/of_address.h>
+
+void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
+		unsigned long *busno, unsigned long *phys, unsigned long *size)
+{
+	const __be32 *dma_window;
+	u32 cells;
+	const unsigned char *prop;
+
+	dma_window = dma_window_prop;
+
+	/* busno is always one cell */
+	if (busno)
+		*busno = be32_to_cpup(dma_window++);
+
+	prop = of_get_property(dn, "#dma-address-cells", NULL);
+	if (!prop)
+		prop = of_get_property(dn, "#address-cells", NULL);
+
+	cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
+	*phys = of_read_number(dma_window, cells);
+
+	dma_window += cells;
+
+	prop = of_get_property(dn, "#dma-size-cells", NULL);
+	cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);
+	*size = of_read_number(dma_window, cells);
+}
+
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..2a0f7c6 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -21,6 +21,10 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const u32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size);
+
 #ifndef pci_address_to_pio
 static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
@@ -48,6 +52,12 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+
+static inline void of_parse_dma_window(struct device_node *dn,
+		       const void *dma_window_prop, unsigned long *busno,
+			       unsigned long *phys, unsigned long *size)
+{
+}
 #endif /* CONFIG_OF_ADDRESS */
 
 
-- 
1.7.5.4

Re: [PATCH 1/1] dt: Add general DMA window parser

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

* Hiroshi Doyu wrote:
> diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
> new file mode 100644
> index 0000000..45c9e88
> --- /dev/null
> +++ b/drivers/of/of_dma.c
> @@ -0,0 +1,35 @@
> +/*
> + * Stolen from:
> + *	"arch/microblaze/kernel/prom_parse.c"
> + *	"arch/powerpc/kernel/prom_parse.c"
> + */
> +
> +#include <linux/of_address.h>
> +
> +void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
> +		unsigned long *busno, unsigned long *phys, unsigned long *size)
> +{
> +	const __be32 *dma_window;
> +	u32 cells;
> +	const unsigned char *prop;

There's no need for this to be const unsigned char *, const void * will do
just as well.

> +
> +	dma_window = dma_window_prop;
> +
> +	/* busno is always one cell */
> +	if (busno)
> +		*busno = be32_to_cpup(dma_window++);
> +
> +	prop = of_get_property(dn, "#dma-address-cells", NULL);
> +	if (!prop)
> +		prop = of_get_property(dn, "#address-cells", NULL);
> +
> +	cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
> +	*phys = of_read_number(dma_window, cells);

This should probably fail gracefully if phys == NULL, similar to what you do
for busno.

> +
> +	dma_window += cells;
> +
> +	prop = of_get_property(dn, "#dma-size-cells", NULL);
> +	cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);
> +	*size = of_read_number(dma_window, cells);

Same here.

> +}
> +

And you might want to add a EXPORT_SYMBOL(of_parse_dma_window) here so the
function can be used from modules.

Sorry for having you go through another round. I should have looked more
closely before.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Subject: Re: [PATCH 1/1] dt: Add general DMA window parser
Date: Wed, 18 Apr 2012 12:54:23 +0200
Message-ID: <20120418105423.GA5667-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>

> * PGP Signed by an unknown key
> 
> * Hiroshi Doyu wrote:
> > diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
> > new file mode 100644
> > index 0000000..45c9e88
> > --- /dev/null
> > +++ b/drivers/of/of_dma.c
> > @@ -0,0 +1,35 @@
> > +/*
> > + * Stolen from:
> > + *	"arch/microblaze/kernel/prom_parse.c"
> > + *	"arch/powerpc/kernel/prom_parse.c"
> > + */
> > +
> > +#include <linux/of_address.h>
> > +
> > +void of_parse_dma_window(struct device_node *dn, const void *dma_window_prop,
> > +		unsigned long *busno, unsigned long *phys, unsigned long *size)
> > +{
> > +	const __be32 *dma_window;
> > +	u32 cells;
> > +	const unsigned char *prop;
> 
> There's no need for this to be const unsigned char *, const void * will do
> just as well.
> 
> > +
> > +	dma_window = dma_window_prop;
> > +
> > +	/* busno is always one cell */
> > +	if (busno)
> > +		*busno = be32_to_cpup(dma_window++);
> > +
> > +	prop = of_get_property(dn, "#dma-address-cells", NULL);
> > +	if (!prop)
> > +		prop = of_get_property(dn, "#address-cells", NULL);
> > +
> > +	cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
> > +	*phys = of_read_number(dma_window, cells);
> 
> This should probably fail gracefully if phys == NULL, similar to what you do
> for busno.
> 
> > +
> > +	dma_window += cells;
> > +
> > +	prop = of_get_property(dn, "#dma-size-cells", NULL);
> > +	cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);
> > +	*size = of_read_number(dma_window, cells);
> 
> Same here.
> 
> > +}
> > +
> 
> And you might want to add a EXPORT_SYMBOL(of_parse_dma_window) here so the
> function can be used from modules.
> 
> Sorry for having you go through another round. I should have looked more
> closely before.

No problem, I'll wait for another comments, and post again later. Thank you for reivew.

Re: [PATCH 1/1] dt: Add general DMA window parser

[Stephen Warren]

On 04/18/2012 04:19 AM, Hiroshi Doyu wrote:

> Subject: [PATCH 1/1] dt: Add general DMA window parser
> 
> This code was stolen from:
> 	"arch/microblaze/kernel/prom_parse.c"
> 	"arch/powerpc/kernel/prom_parse.c"
> 
> Once "ibm," prefix is removed from dts file. This generic one could
> replace the originals.

> +extern void of_parse_dma_window(struct device_node *dn,
> +		       const void *dma_window_prop, unsigned long *busno,
> +			       unsigned long *phys, unsigned long *size);

At least some other of_*() parsing functions take the property name
rather than the property pointer, and also take an index into the
property in order to support multiple entries in it. See for example
of_parse_phandle and of_get_named_gpio_flags. Should this new API be
similar? E.g.:

extern void of_parse_dma_window(struct device_node *np,
			const char *propname, int index,
			unsigned long *busno,
			unsigned long *phys, unsigned long *size);

Re: [PATCH 1/1] dt: Add general DMA window parser

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

* Stephen Warren wrote:
> On 04/18/2012 04:19 AM, Hiroshi Doyu wrote:
> 
> > Subject: [PATCH 1/1] dt: Add general DMA window parser
> > 
> > This code was stolen from:
> > 	"arch/microblaze/kernel/prom_parse.c"
> > 	"arch/powerpc/kernel/prom_parse.c"
> > 
> > Once "ibm," prefix is removed from dts file. This generic one could
> > replace the originals.
> 
> > +extern void of_parse_dma_window(struct device_node *dn,
> > +		       const void *dma_window_prop, unsigned long *busno,
> > +			       unsigned long *phys, unsigned long *size);
> 
> At least some other of_*() parsing functions take the property name
> rather than the property pointer, and also take an index into the
> property in order to support multiple entries in it. See for example
> of_parse_phandle and of_get_named_gpio_flags. Should this new API be
> similar? E.g.:
> 
> extern void of_parse_dma_window(struct device_node *np,
> 			const char *propname, int index,
> 			unsigned long *busno,
> 			unsigned long *phys, unsigned long *size);

In that case the function should return int for proper error handling.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

On Wed, 18 Apr 2012 21:39:45 +0200
Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:

> * PGP Signed by an unknown key
> 
> * Stephen Warren wrote:
> > On 04/18/2012 04:19 AM, Hiroshi Doyu wrote:
> > 
> > > Subject: [PATCH 1/1] dt: Add general DMA window parser
> > > 
> > > This code was stolen from:
> > > 	"arch/microblaze/kernel/prom_parse.c"
> > > 	"arch/powerpc/kernel/prom_parse.c"
> > > 
> > > Once "ibm," prefix is removed from dts file. This generic one could
> > > replace the originals.
> > 
> > > +extern void of_parse_dma_window(struct device_node *dn,
> > > +		       const void *dma_window_prop, unsigned long *busno,
> > > +			       unsigned long *phys, unsigned long *size);
> > 
> > At least some other of_*() parsing functions take the property name
> > rather than the property pointer, and also take an index into the
> > property in order to support multiple entries in it. See for example
> > of_parse_phandle and of_get_named_gpio_flags. Should this new API be
> > similar? E.g.:
> > 
> > extern void of_parse_dma_window(struct device_node *np,
> > 			const char *propname, int index,
> > 			unsigned long *busno,
> > 			unsigned long *phys, unsigned long *size);
> 
> In that case the function should return int for proper error handling.

Good point. I'll.

Re: [PATCH 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

On Wed, 18 Apr 2012 19:27:58 +0200
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 04/18/2012 04:19 AM, Hiroshi Doyu wrote:
> 
> > Subject: [PATCH 1/1] dt: Add general DMA window parser
> > 
> > This code was stolen from:
> > 	"arch/microblaze/kernel/prom_parse.c"
> > 	"arch/powerpc/kernel/prom_parse.c"
> > 
> > Once "ibm," prefix is removed from dts file. This generic one could
> > replace the originals.

It seems that the "ibm," prefix is not used in .dts files, but it
comes from the PAPR compliant firmware and cannot be changed.

The easiest solution would be to make the function understand both
variants.

> > +extern void of_parse_dma_window(struct device_node *dn,
> > +		       const void *dma_window_prop, unsigned long *busno,
> > +			       unsigned long *phys, unsigned long *size);
> 
> At least some other of_*() parsing functions take the property name
> rather than the property pointer, and also take an index into the
> property in order to support multiple entries in it. See for example
> of_parse_phandle and of_get_named_gpio_flags. Should this new API be
> similar? E.g.:
> 
> extern void of_parse_dma_window(struct device_node *np,
> 			const char *propname, int index,
> 			unsigned long *busno,
> 			unsigned long *phys, unsigned long *size);

At least, I can add the code checking the return value of this
function, which won't change the existing code.

Does anyone know the format of "ibm,dma-window"?

[PATCH v2 1/1] dt: Add general DMA window parser

[Hiroshi Doyu]

From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This code was based on:
	"arch/microblaze/kernel/prom_parse.c"
	"arch/powerpc/kernel/prom_parse.c"

"ibm," prefix could be supported with some modification.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2:
- Add several error checks. (Thierry Reding)
- Use property name given to func. (Stephen Warren)
- Support multiple entries. (Stephen Warren)

---
 drivers/of/Kconfig         |    4 ++
 drivers/of/Makefile        |    1 +
 drivers/of/of_dma.c        |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   13 ++++++++
 4 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..3b0298b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,8 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DMA
+	depends on HAS_DMA
+	def_bool y
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..711ff5b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DMA)	+= of_dma.o
diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
new file mode 100644
index 0000000..80bd7aa
--- /dev/null
+++ b/drivers/of/of_dma.c
@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Based on:
+ *	"arch/microblaze/kernel/prom_parse.c"
+ *	"arch/powerpc/kernel/prom_parse.c"
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/export.h>
+#include <linux/of_address.h>
+
+int of_parse_dma_window(struct device_node *dn,
+			const char *propname, int index,
+			unsigned long *busno,
+			dma_addr_t *addr, size_t *size)
+{
+	const __be32 *dma_window, *end;
+	int bytes, cur_index = 0;
+
+	if (!dn || !propname || !addr || !size)
+		return -EINVAL;
+
+	dma_window = of_get_property(dn, propname, &bytes);
+	if (!dma_window)
+		return -ENODEV;
+	end = dma_window + bytes / sizeof(*dma_window);
+
+	while (dma_window < end) {
+		u32 cells;
+		const void *prop;
+
+		/* busno is always one cell */
+		if (busno)
+			*busno = be32_to_cpup(dma_window++);
+
+		prop = of_get_property(dn, "#dma-address-cells", NULL);
+		if (!prop)
+			prop = of_get_property(dn, "#address-cells", NULL);
+
+		cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
+		if (!cells)
+			return -EINVAL;
+		*addr = of_read_number(dma_window, cells);
+		dma_window += cells;
+
+		prop = of_get_property(dn, "#dma-size-cells", NULL);
+		cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);
+		if (!cells)
+			return -EINVAL;
+		*size = of_read_number(dma_window, cells);
+		dma_window += cells;
+
+		if (cur_index++ == index)
+			break;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(of_parse_dma_window);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..1138cd3 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -21,6 +21,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const u32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern int of_parse_dma_window(struct device_node *dev,
+			       const char *propname, int index,
+			       unsigned long *busno,
+			       dma_addr_t *phys, size_t *size);
+
 #ifndef pci_address_to_pio
 static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
@@ -48,6 +53,14 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+
+extern int of_parse_dma_window(struct device_node *dev,
+			       const char *propname, int index,
+			       unsigned long *busno,
+			       dma_addr_t *phys, size_t *size)
+{
+	return 0;
+}
 #endif /* CONFIG_OF_ADDRESS */
 
 
-- 
1.7.5.4

Re: [PATCH v2 1/1] dt: Add general DMA window parser

[Stephen Warren]

On 04/23/2012 05:53 AM, Hiroshi Doyu wrote:
> From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This code was based on:
> 	"arch/microblaze/kernel/prom_parse.c"
> 	"arch/powerpc/kernel/prom_parse.c"
> 
> "ibm," prefix could be supported with some modification.

That'd probably be a good idea. If it isn't supported, then this code
can't be used as a replacement for the PPC and Microblaze code.

> +int of_parse_dma_window(struct device_node *dn,
> +			const char *propname, int index,
> +			unsigned long *busno,
> +			dma_addr_t *addr, size_t *size)
> +{
> +	const __be32 *dma_window, *end;
> +	int bytes, cur_index = 0;
> +
> +	if (!dn || !propname || !addr || !size)
> +		return -EINVAL;

Instead of erroring out if (!propname), perhaps:

if (!propname)
    propname = "dma-window";

so that only drivers using bindings before this common binding was
defined actually need to write out the property name?

> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
...
> +extern int of_parse_dma_window(struct device_node *dev,
> +			       const char *propname, int index,
> +			       unsigned long *busno,
> +			       dma_addr_t *phys, size_t *size);

This function has the same name as the PPC and Microblaze code it's
based on. Is that going to cause compile and link errors due to
duplicate symbol definitions?

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Stephen Warren]

On 04/13/2012 04:22 AM, Hiroshi Doyu wrote:
> From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Add device tree support for Tegra30 IOMMU(SMMU).

> +++ b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt

I personally like the documentation to be named after the full
compatible value, so nvidia,tegra30-smmu.txt.

> @@ -0,0 +1,19 @@
> +NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
> +
> +Required properties:
> +- compatible : "nvidia,tegra30-smmu"
> +- reg : Should contain MC registers location and length
> +- reg : Should contain AHB Arbitration registers and length
> +- reg : Should contain virtual address space range

This looks like 3 properties with the same name. It seems common to
write something more like:

reg : Should contain the register address and length for each fo the MC
and AHB arbitration registers.

But why does the SMMU driver expect to control the AHB arbitration
registers? They seem unrelated to the SMMU.

As I said in my previous email, I think the VA space size should be a
separate property.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU
Date: Fri, 13 Apr 2012 21:33:47 +0200
Message-ID: <4F887F9B.700-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 04/13/2012 04:22 AM, Hiroshi Doyu wrote:
> > From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Add device tree support for Tegra30 IOMMU(SMMU).
> 
> > +++ b/Documentation/devicetree/bindings/arm/tegra/tegra30-smmu.txt
> 
> I personally like the documentation to be named after the full
> compatible value, so nvidia,tegra30-smmu.txt.

OK

> > @@ -0,0 +1,19 @@
> > +NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
> > +
> > +Required properties:
> > +- compatible : "nvidia,tegra30-smmu"
> > +- reg : Should contain MC registers location and length
> > +- reg : Should contain AHB Arbitration registers and length
> > +- reg : Should contain virtual address space range
> 
> This looks like 3 properties with the same name. It seems common to
> write something more like:
> 
> reg : Should contain the register address and length for each fo the MC
> and AHB arbitration registers.

OK, this looks better.

> But why does the SMMU driver expect to control the AHB arbitration
> registers? They seem unrelated to the SMMU.

It's necessary to inform AHB that SMMU is ready to use. There's a
dedicated bit for SMMU in this AHB arbitration register.

> As I said in my previous email, I think the VA space size should be a
> separate property.

OK, I'll update.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Stephen Warren]

On 04/16/2012 04:10 AM, Hiroshi Doyu wrote:
> Stephen Warren wrote at Fri, 13 Apr 2012 21:33:47 +0200:
>> On 04/13/2012 04:22 AM, Hiroshi Doyu wrote:
>>> Add device tree support for Tegra30 IOMMU(SMMU).
...
>> But why does the SMMU driver expect to control the AHB arbitration
>> registers? They seem unrelated to the SMMU.
> 
> It's necessary to inform AHB that SMMU is ready to use. There's a
> dedicated bit for SMMU in this AHB arbitration register.

Shouldn't there be a dedicated driver for the AHB arbitration registers
that the SMMU driver calls into to achieve this? IIRC, the AHB
arbitration registers support much more than the SMMU enable, and if we
ever need to touch those other features, going to the SMMU driver to do
so probably wouldn't make sense.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Hiroshi Doyu]

On Mon, 16 Apr 2012 18:07:06 +0200
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 04/16/2012 04:10 AM, Hiroshi Doyu wrote:
> > Stephen Warren wrote at Fri, 13 Apr 2012 21:33:47 +0200:
> >> On 04/13/2012 04:22 AM, Hiroshi Doyu wrote:
> >>> Add device tree support for Tegra30 IOMMU(SMMU).
> ...
> >> But why does the SMMU driver expect to control the AHB arbitration
> >> registers? They seem unrelated to the SMMU.
> > 
> > It's necessary to inform AHB that SMMU is ready to use. There's a
> > dedicated bit for SMMU in this AHB arbitration register.
> 
> Shouldn't there be a dedicated driver for the AHB arbitration registers
> that the SMMU driver calls into to achieve this? IIRC, the AHB
> arbitration registers support much more than the SMMU enable, and if we
> ever need to touch those other features, going to the SMMU driver to do
> so probably wouldn't make sense.

Agree. Sounds like the way to go.

We haven't upstreamed AHB driver yet. So is it ok to remove AHB entry
from SMMU dt and add AHB function call when upstreaming AHB driver?
I'll work on that.

Re: [PATCH 1/1] arm/dts: Tegra30: Add device tree support for SMMU

[Stephen Warren]

On 04/17/2012 11:10 PM, Hiroshi Doyu wrote:
> On Mon, 16 Apr 2012 18:07:06 +0200
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> On 04/16/2012 04:10 AM, Hiroshi Doyu wrote:
>>> Stephen Warren wrote at Fri, 13 Apr 2012 21:33:47 +0200:
>>>> On 04/13/2012 04:22 AM, Hiroshi Doyu wrote:
>>>>> Add device tree support for Tegra30 IOMMU(SMMU).
>> ...
>>>> But why does the SMMU driver expect to control the AHB arbitration
>>>> registers? They seem unrelated to the SMMU.
>>>
>>> It's necessary to inform AHB that SMMU is ready to use. There's a
>>> dedicated bit for SMMU in this AHB arbitration register.
>>
>> Shouldn't there be a dedicated driver for the AHB arbitration registers
>> that the SMMU driver calls into to achieve this? IIRC, the AHB
>> arbitration registers support much more than the SMMU enable, and if we
>> ever need to touch those other features, going to the SMMU driver to do
>> so probably wouldn't make sense.
> 
> Agree. Sounds like the way to go.
> 
> We haven't upstreamed AHB driver yet. So is it ok to remove AHB entry
> from SMMU dt and add AHB function call when upstreaming AHB driver?
> I'll work on that.

I think that seems reasonable for now, yes. I assume you mean
implementing e.g. arch/arm/mach-tegra/tegra30-ahb.c, similar to how,
say, the fuse APIs work.