Re[2]: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems

=0D=0A Hello David,

 Thanks a lot for review.

 The general note to be made here is that the changes to the DTS file=20
made by this patch are necessary for a ppc440spe ADMA driver, which is=20
a not-completed arch/powerpc port from the arch/ppc branch, and which=20
uses DT (well, incorrectly) just to get interrupts. Otherwise, it's=20
just a platform device driver.

 We provided this ADMA driver just as the reference of driver, which=20
implements the RAID-6 related low-level stuff. ppc440spe ADMA in its=20
current state is far from ready for merging. We'll elaborate on its=20
cleaning up then (surely, taking into account all the comments made=20
from community). But, even now, the driver works, so we publish this=20
so interested people could use and test it.

 Some comments mixed in below.

On Tuesday, January 13, 2009 you wrote:

On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:

quoted

Adds the platform device definitions and the architecture specific suppo=

rt

quoted

routines for the ppc440spe adma driver.
=20
Any board equipped with PPC440SP(e) controller may utilize this driver.
=20

diff --git a/arch/powerpc/boot/dts/katmai.dts b/arch/powerpc/boot/dts/ka=

tmai.dts

quoted

index 077819b..f2f77c8 100644

--- a/arch/powerpc/boot/dts/katmai.dts
+++ b/arch/powerpc/boot/dts/katmai.dts

@@ -16,7 +16,7 @@

=20
 / {
      #address-cells =3D <2>;
-     #size-cells =3D <1>;
+     #size-cells =3D <2>;

You've changed the root level size-cells, but haven't updated the
sub-nodes (such as /memory) accordingly.

 Thanks, we'll fix this in the next version of this patch.

quoted

      model =3D "amcc,katmai";
      compatible =3D "amcc,katmai";
      dcr-parent =3D <&{/cpus/cpu@0}>;

@@ -392,6 +392,30 @@
                              0x0 0x0 0x0 0x3 &UIC3 0xa 0x4 /* swizzled =

int C */

quoted

                              0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled =

int D */>;

quoted

              };
+             DMA0: dma0 {

No 'compatible' property, which seems dubious.

 OK, we'll fix.

quoted

+                     interrupt-parent =3D <&DMA0>;
+                     interrupts =3D <0 1>;
+                     #interrupt-cells =3D <1>;
+                     #address-cells =3D <0>;
+                     #size-cells =3D <0>;
+                     interrupt-map =3D <
+                             0 &UIC0 0x14 4
+                             1 &UIC1 0x16 4>;
+             };
+             DMA1: dma1 {
+                     interrupt-parent =3D <&DMA1>;
+                     interrupts =3D <0 1>;
+                     #interrupt-cells =3D <1>;
+                     #address-cells =3D <0>;
+                     #size-cells =3D <0>;
+                     interrupt-map =3D <
+                             0 &UIC0 0x16 4
+                             1 &UIC1 0x16 4>;

Are these interrupt-maps correct?  The second interrupt from both dma
controllers is routed to the same line on UIC1?

 The map is correct:

- first interrupts are 'DMAx Command Status FIFO Needs Service';
- second interrupt is 'DMA Error', both DMA engines share common error IRQ.

quoted

+             };
+             xor {
+                     interrupt-parent =3D <&UIC1>;
+                     interrupts =3D <0x1f 4>;

What the hell is this thing?  No compatible property, nor even a
meaningful name.

 This is the XOR accelerator, the dedicated DMA engine of ppc440spe=20
equipped with the ability to do XOR operations in h/w. I guess, it=20
could be named like DMA2.

 Regards, Yuri

 --
 Yuri Tikhonov, Senior Software Engineer
 Emcraft Systems, www.emcraft.com