[PATCH v2] update crypto node definition and device tree instances

[PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

delete obsolete device-type property, delete model property
(use compatible property instead), prepend "fsl," to Freescale
specific properties. Add nodes to device trees that are missing them,
and fix broken property values in other trees.

Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
---
changes from v1: b-w-of.txt:

-    - compatible : Should contain entries for all compatible SEC versions,
-      high to low, e.g., "fsl,sec2.1", "fsl,sec2.0"
+    - compatible : Should contain entries for this and backward compatible
+      SEC versions, high to low, e.g., "fsl,sec2.1", "fsl,sec2.0"

 Documentation/powerpc/booting-without-of.txt |   51 +++++++++++++-------------
 arch/powerpc/boot/dts/mpc8272ads.dts         |   21 ++++-------
 arch/powerpc/boot/dts/mpc8313erdb.dts        |   15 +++-----
 arch/powerpc/boot/dts/mpc8315erdb.dts        |   15 ++++----
 arch/powerpc/boot/dts/mpc832x_mds.dts        |   15 +++-----
 arch/powerpc/boot/dts/mpc832x_rdb.dts        |   15 +++-----
 arch/powerpc/boot/dts/mpc8349emitx.dts       |   12 +++----
 arch/powerpc/boot/dts/mpc8349emitxgp.dts     |   12 +++----
 arch/powerpc/boot/dts/mpc834x_mds.dts        |   15 +++-----
 arch/powerpc/boot/dts/mpc836x_mds.dts        |   13 +++----
 arch/powerpc/boot/dts/mpc8377_mds.dts        |   13 +++----
 arch/powerpc/boot/dts/mpc8377_rdb.dts        |   14 +++----
 arch/powerpc/boot/dts/mpc8378_mds.dts        |   13 +++----
 arch/powerpc/boot/dts/mpc8378_rdb.dts        |   14 +++----
 arch/powerpc/boot/dts/mpc8379_mds.dts        |   13 +++----
 arch/powerpc/boot/dts/mpc8379_rdb.dts        |   14 +++----
 arch/powerpc/boot/dts/mpc8541cds.dts         |   11 ++++++
 arch/powerpc/boot/dts/mpc8544ds.dts          |   11 ++++++
 arch/powerpc/boot/dts/mpc8548cds.dts         |   11 ++++++
 arch/powerpc/boot/dts/mpc8555cds.dts         |   11 ++++++
 arch/powerpc/boot/dts/mpc8568mds.dts         |   14 +++----
 arch/powerpc/boot/dts/mpc8572ds.dts          |   12 ++++++
 arch/powerpc/boot/dts/sbc8349.dts            |   14 +++-----
 arch/powerpc/boot/dts/sbc8548.dts            |   11 ++++++
 arch/powerpc/boot/dts/tqm8541.dts            |   11 ++++++
 arch/powerpc/boot/dts/tqm8555.dts            |   11 ++++++
 26 files changed, 213 insertions(+), 169 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index b68684d..196e694 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1472,9 +1472,8 @@ platforms are moved over to use the flattened-device-tree model.
 
    Required properties:
 
-    - device_type : Should be "crypto"
-    - model : Model of the device.  Should be "SEC1" or "SEC2"
-    - compatible : Should be "talitos"
+    - compatible : Should contain entries for this and backward compatible
+      SEC versions, high to low, e.g., "fsl,sec2.1", "fsl,sec2.0"
     - reg : Offset and length of the register set for the device
     - interrupts : <a b> where a is the interrupt number and b is a
       field that represents an encoding of the sense and level
@@ -1483,27 +1482,29 @@ platforms are moved over to use the flattened-device-tree model.
       controller you have.
     - interrupt-parent : the phandle for the interrupt controller that
       services interrupts for this device.
-    - num-channels : An integer representing the number of channels
+    - fsl,num-channels : An integer representing the number of channels
       available.
-    - channel-fifo-len : An integer representing the number of
+    - fsl,channel-fifo-len : An integer representing the number of
       descriptor pointers each channel fetch fifo can hold.
-    - exec-units-mask : The bitmask representing what execution units
+    - fsl,exec-units-mask : The bitmask representing what execution units
       (EUs) are available. It's a single 32-bit cell. EU information
       should be encoded following the SEC's Descriptor Header Dword
       EU_SEL0 field documentation, i.e. as follows:
 
-        bit 0 = reserved - should be 0
-        bit 1 = set if SEC has the ARC4 EU (AFEU)
-        bit 2 = set if SEC has the DES/3DES EU (DEU)
-        bit 3 = set if SEC has the message digest EU (MDEU)
-        bit 4 = set if SEC has the random number generator EU (RNG)
-        bit 5 = set if SEC has the public key EU (PKEU)
-        bit 6 = set if SEC has the AES EU (AESU)
-        bit 7 = set if SEC has the Kasumi EU (KEU)
+        bit 0  = reserved - should be 0
+        bit 1  = set if SEC has the ARC4 EU (AFEU)
+        bit 2  = set if SEC has the DES/3DES EU (DEU)
+        bit 3  = set if SEC has the message digest EU (MDEU/MDEU-A)
+        bit 4  = set if SEC has the random number generator EU (RNG)
+        bit 5  = set if SEC has the public key EU (PKEU)
+        bit 6  = set if SEC has the AES EU (AESU)
+        bit 7  = set if SEC has the Kasumi EU (KEU)
+        bit 8  = set if SEC has the CRC EU (CRCU)
+        bit 11 = set if SEC has the message digest EU extended alg set (MDEU-B)
 
-      bits 8 through 31 are reserved for future SEC EUs.
+      remaining bits are reserved for future SEC EUs.
 
-    - descriptor-types-mask : The bitmask representing what descriptors
+    - fsl,descriptor-types-mask : The bitmask representing what descriptors
       are available. It's a single 32-bit cell. Descriptor type
       information should be encoded following the SEC's Descriptor
       Header Dword DESC_TYPE field documentation, i.e. as follows:
@@ -1527,16 +1528,14 @@ platforms are moved over to use the flattened-device-tree model.
 
        /* MPC8548E */
        crypto@30000 {
-               device_type = "crypto";
-               model = "SEC2";
-               compatible = "talitos";
-               reg = <30000 10000>;
-               interrupts = <1d 3>;
-               interrupt-parent = <40000>;
-               num-channels = <4>;
-               channel-fifo-len = <18>;
-               exec-units-mask = <000000fe>;
-               descriptor-types-mask = <012b0ebf>;
+               compatible = "fsl,sec2.1", "fsl,sec2.0";
+               reg = <0x30000 0x10000>;
+               interrupts = <29 2>;
+               interrupt-parent = <&mpic>;
+               fsl,num-channels = <4>;
+               fsl,channel-fifo-len = <24>;
+               fsl,exec-units-mask = <0xfe>;
+               fsl,descriptor-types-mask = <0x12b0ebf>;
        };
 
    h) Board Control and Status (BCSR)
diff --git a/arch/powerpc/boot/dts/mpc8272ads.dts b/arch/powerpc/boot/dts/mpc8272ads.dts
index 46e2da3..9a3881d 100644
--- a/arch/powerpc/boot/dts/mpc8272ads.dts
+++ b/arch/powerpc/boot/dts/mpc8272ads.dts
@@ -226,22 +226,15 @@
 			compatible = "fsl,mpc8272-pic", "fsl,cpm2-pic";
 		};
 
-/* May need to remove if on a part without crypto engine */
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "fsl,mpc8272-talitos-sec2",
-			             "fsl,talitos-sec2",
-			             "fsl,talitos",
-			             "talitos";
-			reg = <0x30000 0x10000>;
-			interrupts = <11 8>;
+			compatible = "fsl,sec1.0";
+			reg = <0x40000 0x13000>;
+			interrupts = <47 0x8>;
 			interrupt-parent = <&PIC>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x7e>;
-/* desc mask is for rev1.x, we need runtime fixup for >=2.x */
-			descriptor-types-mask = <0x1010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x1010415>;
 		};
 	};
 
diff --git a/arch/powerpc/boot/dts/mpc8313erdb.dts b/arch/powerpc/boot/dts/mpc8313erdb.dts
index e1f0dca..03edb86 100644
--- a/arch/powerpc/boot/dts/mpc8313erdb.dts
+++ b/arch/powerpc/boot/dts/mpc8313erdb.dts
@@ -219,17 +219,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
-			reg = <0x30000 0x7000>;
+			compatible = "fsl,sec2.2", "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 2.2 */
-			num-channels = <1>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000004c>;
-			descriptor-types-mask = <0x0122003f>;
+			fsl,num-channels = <1>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x4c>;
+			fsl,descriptor-types-mask = <0x0122003f>;
 		};
 
 		/* IPIC
diff --git a/arch/powerpc/boot/dts/mpc8315erdb.dts b/arch/powerpc/boot/dts/mpc8315erdb.dts
index d7a1ece..e433303 100644
--- a/arch/powerpc/boot/dts/mpc8315erdb.dts
+++ b/arch/powerpc/boot/dts/mpc8315erdb.dts
@@ -206,17 +206,16 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			device_type = "crypto";
-			compatible = "talitos";
+			compatible = "fsl,sec3.3", "fsl,sec3.1", "fsl,sec3.0",
+				     "fsl,sec2.4", "fsl,sec2.2", "fsl,sec2.1",
+				     "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x97c>;
+			fsl,descriptor-types-mask = <0x3ab0abf>;
 		};
 
 		sata@18000 {
diff --git a/arch/powerpc/boot/dts/mpc832x_mds.dts b/arch/powerpc/boot/dts/mpc832x_mds.dts
index 539e02f..5462845 100644
--- a/arch/powerpc/boot/dts/mpc832x_mds.dts
+++ b/arch/powerpc/boot/dts/mpc832x_mds.dts
@@ -115,17 +115,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
-			reg = <0x30000 0x7000>;
+			compatible = "fsl,sec2.2", "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 2.2 */
-			num-channels = <1>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000004c>;
-			descriptor-types-mask = <0x0122003f>;
+			fsl,num-channels = <1>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x4c>;
+			fsl,descriptor-types-mask = <0x0122003f>;
 		};
 
 		ipic: pic@700 {
diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index 179c81c..7fb50ab 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -93,17 +93,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
-			reg = <0x30000 0x7000>;
+			compatible = "fsl,sec2.2", "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&pic>;
-			/* Rev. 2.2 */
-			num-channels = <1>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000004c>;
-			descriptor-types-mask = <0x0122003f>;
+			fsl,num-channels = <1>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x4c>;
+			fsl,descriptor-types-mask = <0x0122003f>;
 		};
 
 		pic:pic@700 {
diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index 9426676..f7a850c 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -178,16 +178,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
+			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000007e>;
-			descriptor-types-mask = <0x01010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
 		};
 
 		ipic: pic@700 {
diff --git a/arch/powerpc/boot/dts/mpc8349emitxgp.dts b/arch/powerpc/boot/dts/mpc8349emitxgp.dts
index f81d735..b866b47 100644
--- a/arch/powerpc/boot/dts/mpc8349emitxgp.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitxgp.dts
@@ -151,16 +151,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
+			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000007e>;
-			descriptor-types-mask = <0x01010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
 		};
 
 		ipic: pic@700 {
diff --git a/arch/powerpc/boot/dts/mpc834x_mds.dts b/arch/powerpc/boot/dts/mpc834x_mds.dts
index 0199c5c..e0d0753 100644
--- a/arch/powerpc/boot/dts/mpc834x_mds.dts
+++ b/arch/powerpc/boot/dts/mpc834x_mds.dts
@@ -193,20 +193,15 @@
 			interrupt-parent = <&ipic>;
 		};
 
-		/* May need to remove if on a part without crypto engine */
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
+			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000007e>;
-			/* desc mask is for rev2.0,
-			 * we need runtime fixup for >2.0 */
-			descriptor-types-mask = <0x01010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
 		};
 
 		/* IPIC
diff --git a/arch/powerpc/boot/dts/mpc836x_mds.dts b/arch/powerpc/boot/dts/mpc836x_mds.dts
index 8160ff2..69e6564 100644
--- a/arch/powerpc/boot/dts/mpc836x_mds.dts
+++ b/arch/powerpc/boot/dts/mpc836x_mds.dts
@@ -119,17 +119,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
+			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000007e>;
-			/* desc mask is for rev1.x, we need runtime fixup for >=2.x */
-			descriptor-types-mask = <0x01010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
 		};
 
 		ipic: pic@700 {
diff --git a/arch/powerpc/boot/dts/mpc8377_mds.dts b/arch/powerpc/boot/dts/mpc8377_mds.dts
index fea5925..76149b7 100644
--- a/arch/powerpc/boot/dts/mpc8377_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8377_mds.dts
@@ -234,16 +234,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		sdhc@2e000 {
diff --git a/arch/powerpc/boot/dts/mpc8377_rdb.dts b/arch/powerpc/boot/dts/mpc8377_rdb.dts
index 5bc09ad..58d2472 100644
--- a/arch/powerpc/boot/dts/mpc8377_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8377_rdb.dts
@@ -219,17 +219,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			device_type = "crypto";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		sata@18000 {
diff --git a/arch/powerpc/boot/dts/mpc8378_mds.dts b/arch/powerpc/boot/dts/mpc8378_mds.dts
index 1d6ea08..07a69a6 100644
--- a/arch/powerpc/boot/dts/mpc8378_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8378_mds.dts
@@ -234,16 +234,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		sdhc@2e000 {
diff --git a/arch/powerpc/boot/dts/mpc8378_rdb.dts b/arch/powerpc/boot/dts/mpc8378_rdb.dts
index 711f9a3..6f845d3 100644
--- a/arch/powerpc/boot/dts/mpc8378_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8378_rdb.dts
@@ -219,17 +219,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			device_type = "crypto";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		/* IPIC
diff --git a/arch/powerpc/boot/dts/mpc8379_mds.dts b/arch/powerpc/boot/dts/mpc8379_mds.dts
index 6f78a9f..4d3bab0 100644
--- a/arch/powerpc/boot/dts/mpc8379_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8379_mds.dts
@@ -234,16 +234,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		sdhc@2e000 {
diff --git a/arch/powerpc/boot/dts/mpc8379_rdb.dts b/arch/powerpc/boot/dts/mpc8379_rdb.dts
index c11ceb7..71619c4 100644
--- a/arch/powerpc/boot/dts/mpc8379_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8379_rdb.dts
@@ -219,17 +219,15 @@
 		};
 
 		crypto@30000 {
-			model = "SEC3";
-			device_type = "crypto";
-			compatible = "talitos";
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			/* Rev. 3.0 geometry */
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x000001fe>;
-			descriptor-types-mask = <0x03ab0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
 		};
 
 		sata@18000 {
diff --git a/arch/powerpc/boot/dts/mpc8541cds.dts b/arch/powerpc/boot/dts/mpc8541cds.dts
index 66192aa..4e7b051 100644
--- a/arch/powerpc/boot/dts/mpc8541cds.dts
+++ b/arch/powerpc/boot/dts/mpc8541cds.dts
@@ -148,6 +148,17 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/mpc8544ds.dts b/arch/powerpc/boot/dts/mpc8544ds.dts
index 6cf533f..7d3829d 100644
--- a/arch/powerpc/boot/dts/mpc8544ds.dts
+++ b/arch/powerpc/boot/dts/mpc8544ds.dts
@@ -210,6 +210,17 @@
 			fsl,has-rstcr;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0xfe>;
+			fsl,descriptor-types-mask = <0x12b0ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index 205598d..8029407 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -208,6 +208,17 @@
 			fsl,has-rstcr;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0xfe>;
+			fsl,descriptor-types-mask = <0x12b0ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/mpc8555cds.dts b/arch/powerpc/boot/dts/mpc8555cds.dts
index 7c9d0b1..3a91a03 100644
--- a/arch/powerpc/boot/dts/mpc8555cds.dts
+++ b/arch/powerpc/boot/dts/mpc8555cds.dts
@@ -148,6 +148,17 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/mpc8568mds.dts b/arch/powerpc/boot/dts/mpc8568mds.dts
index d7af8db..c81c18c 100644
--- a/arch/powerpc/boot/dts/mpc8568mds.dts
+++ b/arch/powerpc/boot/dts/mpc8568mds.dts
@@ -190,16 +190,14 @@
 		};
 
 		crypto@30000 {
-			device_type = "crypto";
-			model = "SEC2";
-			compatible = "talitos";
-			reg = <0x30000 0xf000>;
+			compatible = "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
 			interrupts = <45 2>;
 			interrupt-parent = <&mpic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0xfe>;
-			descriptor-types-mask = <0x12b0ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0xfe>;
+			fsl,descriptor-types-mask = <0x12b0ebf>;
 		};
 
 		mpic: pic@40000 {
diff --git a/arch/powerpc/boot/dts/mpc8572ds.dts b/arch/powerpc/boot/dts/mpc8572ds.dts
index a444e6a..fe732f1 100644
--- a/arch/powerpc/boot/dts/mpc8572ds.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds.dts
@@ -239,6 +239,18 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
+				     "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2 58 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x9fe>;
+			fsl,descriptor-types-mask = <0x3ab0ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/sbc8349.dts b/arch/powerpc/boot/dts/sbc8349.dts
index 3839d4b..6013ed8 100644
--- a/arch/powerpc/boot/dts/sbc8349.dts
+++ b/arch/powerpc/boot/dts/sbc8349.dts
@@ -186,19 +186,15 @@
 			interrupt-parent = <&ipic>;
 		};
 
-		/* May need to remove if on a part without crypto engine */
 		crypto@30000 {
-			model = "SEC2";
-			compatible = "talitos";
+			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
 			interrupts = <11 0x8>;
 			interrupt-parent = <&ipic>;
-			num-channels = <4>;
-			channel-fifo-len = <24>;
-			exec-units-mask = <0x0000007e>;
-			/* desc mask is for rev2.0,
-			 * we need runtime fixup for >2.0 */
-			descriptor-types-mask = <0x01010ebf>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
 		};
 
 		/* IPIC
diff --git a/arch/powerpc/boot/dts/sbc8548.dts b/arch/powerpc/boot/dts/sbc8548.dts
index d252e38..04a0a55 100644
--- a/arch/powerpc/boot/dts/sbc8548.dts
+++ b/arch/powerpc/boot/dts/sbc8548.dts
@@ -263,6 +263,17 @@
 			fsl,has-rstcr;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.1", "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0xfe>;
+			fsl,descriptor-types-mask = <0x12b0ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/tqm8541.dts b/arch/powerpc/boot/dts/tqm8541.dts
index 8241ae3..7ca9a53 100644
--- a/arch/powerpc/boot/dts/tqm8541.dts
+++ b/arch/powerpc/boot/dts/tqm8541.dts
@@ -159,6 +159,17 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/boot/dts/tqm8555.dts b/arch/powerpc/boot/dts/tqm8555.dts
index 5a57cef..22105ef 100644
--- a/arch/powerpc/boot/dts/tqm8555.dts
+++ b/arch/powerpc/boot/dts/tqm8555.dts
@@ -159,6 +159,17 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		crypto@30000 {
+			compatible = "fsl,sec2.0";
+			reg = <0x30000 0x10000>;
+			interrupts = <45 2>;
+			interrupt-parent = <&mpic>;
+			fsl,num-channels = <4>;
+			fsl,channel-fifo-len = <24>;
+			fsl,exec-units-mask = <0x7e>;
+			fsl,descriptor-types-mask = <0x01010ebf>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
-- 
1.5.6

Re: [PATCH v2] update crypto node definition and device tree instances

[Grant Likely]

On Fri, Jun 27, 2008 at 9:52 AM, Kim Phillips
<kim.phillips@freescale.com> wrote:
> delete obsolete device-type property, delete model property
> (use compatible property instead), prepend "fsl," to Freescale
> specific properties. Add nodes to device trees that are missing them,
> and fix broken property values in other trees.
>
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>

Are there any drivers in mainline which use this node?  Do these
changes break backwards compatibility with already deployed boards?
(I know; this patch doesn't change any code; but I'm asking whether
the driver will need to support both old and new bindings).

> ---
> diff --git a/arch/powerpc/boot/dts/mpc8272ads.dts b/arch/powerpc/boot/dts/mpc8272ads.dts
> index 46e2da3..9a3881d 100644
> --- a/arch/powerpc/boot/dts/mpc8272ads.dts
> +++ b/arch/powerpc/boot/dts/mpc8272ads.dts
> @@ -226,22 +226,15 @@
>                        compatible = "fsl,mpc8272-pic", "fsl,cpm2-pic";
>                };
>
> -/* May need to remove if on a part without crypto engine */
>                crypto@30000 {
> -                       device_type = "crypto";
> -                       model = "SEC2";
> -                       compatible = "fsl,mpc8272-talitos-sec2",
> -                                    "fsl,talitos-sec2",
> -                                    "fsl,talitos",
> -                                    "talitos";
> +                       compatible = "fsl,sec1.0";

Should really be encoding the SoC model in the compatible property.
At the very least, compatible should be:
compatible = "fsl,mpc8272-sec", "fsl,sec1.0";

I'm really don't like "fsl,sec1.0" or any of the variants as a
compatible property either because it can easily be abused (it's not
anchored to a specific physical part so the meaning can shift over
time); but that is another argument and it is well documented in other
email threads (http://thread.gmane.org/gmane.linux.ports.ppc64.devel/38977/focus=39147)

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2] update crypto node definition and device tree instances

[Segher Boessenkool]

> I'm really don't like "fsl,sec1.0" or any of the variants as a
> compatible property either because it can easily be abused (it's not
> anchored to a specific physical part so the meaning can shift over
> time); but that is another argument and it is well documented in other
> email threads  
> (http://thread.gmane.org/gmane.linux.ports.ppc64.devel/38977/ 
> focus=39147)

Also, these made-up names make you do more work: you'll need to
write up a binding for them, explaining exactly what a 1.0 device
etc. is (or at least point to documentation for it).  If you use
a name that refers to some device that people can easily google
for documentation, you can skip this (well, you might need to
write a binding anyway; but at least you won't have to explain
what the device _is_).

Using actual model names also reduces the namespace pollution
(hopefully Freescale will not create some other MPC8272 device
ever, so "fsl,mpc8272-whatever" will never be a nice name to
use for any other device; OTOH, it's likely that Freescale will
create some other device called "SEC" (there are only so many
TLAs, after all), so "fsl,sec-n.m" isn't as future-proof.


Segher

Re: [PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

On Fri, 27 Jun 2008 22:29:59 -0700
"Grant Likely" <grant.likely@secretlab.ca> wrote:

> On Fri, Jun 27, 2008 at 9:52 AM, Kim Phillips
> <kim.phillips@freescale.com> wrote:
> > delete obsolete device-type property, delete model property
> > (use compatible property instead), prepend "fsl," to Freescale
> > specific properties. Add nodes to device trees that are missing them,
> > and fix broken property values in other trees.
> >
> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> 
> Are there any drivers in mainline which use this node?  Do these

there's one sitting on kernel.org's cryptodev-2.6 tree.

> changes break backwards compatibility with already deployed boards?
> (I know; this patch doesn't change any code; but I'm asking whether
> the driver will need to support both old and new bindings).

Since the driver won't go in until 2.6.27 (and hopefully this patch),
I'm guessing no.  There are out-of-tree drivers that will need to be
updated to handle the new bindings, however.

> > ---
> > diff --git a/arch/powerpc/boot/dts/mpc8272ads.dts b/arch/powerpc/boot/dts/mpc8272ads.dts
> > index 46e2da3..9a3881d 100644
> > --- a/arch/powerpc/boot/dts/mpc8272ads.dts
> > +++ b/arch/powerpc/boot/dts/mpc8272ads.dts
> > @@ -226,22 +226,15 @@
> >                        compatible = "fsl,mpc8272-pic", "fsl,cpm2-pic";
> >                };
> >
> > -/* May need to remove if on a part without crypto engine */
> >                crypto@30000 {
> > -                       device_type = "crypto";
> > -                       model = "SEC2";
> > -                       compatible = "fsl,mpc8272-talitos-sec2",
> > -                                    "fsl,talitos-sec2",
> > -                                    "fsl,talitos",
> > -                                    "talitos";
> > +                       compatible = "fsl,sec1.0";
> 
> Should really be encoding the SoC model in the compatible property.
> At the very least, compatible should be:
> compatible = "fsl,mpc8272-sec", "fsl,sec1.0";
> 
> I'm really don't like "fsl,sec1.0" or any of the variants as a
> compatible property either because it can easily be abused (it's not
> anchored to a specific physical part so the meaning can shift over
> time); but that is another argument and it is well documented in other
> email threads (http://thread.gmane.org/gmane.linux.ports.ppc64.devel/38977/focus=39147)

I don't follow - afaict, the thread you are referring to had a
more generic name (no version number for the device).  Meanwhile, the
"fsl,secX.Y" designation /is/ anchored to one or more specific parts
and thus its meaning will not shift over time.  In fact, different
revisions of the mpc834x have different revisions of the SEC device
(the node gets up-revved in u-boot), so compatible = "fsl,mpc8349-sec"
is, in fact, ambiguous.

Kim

Re: [PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

On Sun, 29 Jun 2008 01:37:12 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > I'm really don't like "fsl,sec1.0" or any of the variants as a
> > compatible property either because it can easily be abused (it's not
> > anchored to a specific physical part so the meaning can shift over
> > time); but that is another argument and it is well documented in other
> > email threads  
> > (http://thread.gmane.org/gmane.linux.ports.ppc64.devel/38977/ 
> > focus=39147)
> 
> Also, these made-up names make you do more work: you'll need to

who said they were made up?

> write up a binding for them, explaining exactly what a 1.0 device
> etc. is (or at least point to documentation for it).  If you use
> a name that refers to some device that people can easily google
> for documentation, you can skip this (well, you might need to
> write a binding anyway; but at least you won't have to explain
> what the device _is_).

documentation is available in the usual places, and it specifically
points out which SEC version it references.  Plus, as I mentioned
before, a lot of the differences between the SEC versions are miniscule
feature bits scattered across the programming model. 

> Using actual model names also reduces the namespace pollution
> (hopefully Freescale will not create some other MPC8272 device
> ever, so "fsl,mpc8272-whatever" will never be a nice name to
> use for any other device; OTOH, it's likely that Freescale will
> create some other device called "SEC" (there are only so many
> TLAs, after all), so "fsl,sec-n.m" isn't as future-proof.

I doubt that; the SEC has been around for about a decade now and that
hasn't happened.  The SEC is on par with the TSEC ethernet controller
as far as this goes.

Kim

Re: [PATCH v2] update crypto node definition and device tree instances

[Segher Boessenkool]

>> Also, these made-up names make you do more work: you'll need to
>
> who said they were made up?

I did.  These names do not refer to some physical part you can buy.

>> write up a binding for them, explaining exactly what a 1.0 device
>> etc. is (or at least point to documentation for it).  If you use
>> a name that refers to some device that people can easily google
>> for documentation, you can skip this (well, you might need to
>> write a binding anyway; but at least you won't have to explain
>> what the device _is_).
>
> documentation is available in the usual places, and it specifically
> points out which SEC version it references.

I can't find a manual online for "freescale sec"; googling
for "freescale sec-1.0" finds a manual for the PowerQUICC I;
is that the right one?  I don't know, so the binding needs
to explain it to me.

Going from SoC name -> SEC version is easy, but the other way around
not so.

Anyway, minor stuff.

> Plus, as I mentioned
> before, a lot of the differences between the SEC versions are miniscule
> feature bits scattered across the programming model.

I don't see how this is relevant, sorry.

>> Using actual model names also reduces the namespace pollution
>> (hopefully Freescale will not create some other MPC8272 device
>> ever, so "fsl,mpc8272-whatever" will never be a nice name to
>> use for any other device; OTOH, it's likely that Freescale will
>> create some other device called "SEC" (there are only so many
>> TLAs, after all), so "fsl,sec-n.m" isn't as future-proof.
>
> I doubt that; the SEC has been around for about a decade now and that
> hasn't happened.

You'll have to admit a three-letter acronym is a bigger namespace
squatter than a nice long name is.  But it's your namespace, I don't
care.

i tried googling for "freescale sec" to find any other devices called
SEC, but that didn't work out.  What is "insider trading"?  ;-)


Segher

Re: [PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

On Mon, 30 Jun 2008 18:55:34 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >> Also, these made-up names make you do more work: you'll need to
> >
> > who said they were made up?
> 
> I did.  These names do not refer to some physical part you can buy.

right, they refer to devices in multiple physical parts you can buy.
Part-you-can-buy documentation clearly indicates the SEC version in
that part, in the form "SEC X.Y", i.e, it's not something made up
that's not already in freescale documentation.

> >> write up a binding for them, explaining exactly what a 1.0 device
> >> etc. is (or at least point to documentation for it).  If you use
> >> a name that refers to some device that people can easily google
> >> for documentation, you can skip this (well, you might need to
> >> write a binding anyway; but at least you won't have to explain
> >> what the device _is_).
> >
> > documentation is available in the usual places, and it specifically
> > points out which SEC version it references.
> 
> I can't find a manual online for "freescale sec"; googling
> for "freescale sec-1.0" finds a manual for the PowerQUICC I;
> is that the right one?  I don't know, so the binding needs
> to explain it to me.

the binding shouldn't be responsible for google's shortcomings (that
hit is correct, btw).

> Going from SoC name -> SEC version is easy, but the other way around
> not so.
> 
> Anyway, minor stuff.

sounds like you're pointing out a lack of "SEC versions guide"
documentation of Freescale..

> > Plus, as I mentioned
> > before, a lot of the differences between the SEC versions are miniscule
> > feature bits scattered across the programming model.
> 
> I don't see how this is relevant, sorry.
> 
I'm under the impression that listing the differences (assuming they're
easily obtainable) would lead to unnecessary b-w-of bloat.

> >> Using actual model names also reduces the namespace pollution
> >> (hopefully Freescale will not create some other MPC8272 device
> >> ever, so "fsl,mpc8272-whatever" will never be a nice name to
> >> use for any other device; OTOH, it's likely that Freescale will
> >> create some other device called "SEC" (there are only so many
> >> TLAs, after all), so "fsl,sec-n.m" isn't as future-proof.
> >
> > I doubt that; the SEC has been around for about a decade now and that
> > hasn't happened.
> 
> You'll have to admit a three-letter acronym is a bigger namespace
> squatter than a nice long name is.  But it's your namespace, I don't
> care.
> 
> i tried googling for "freescale sec" to find any other devices called
> SEC, but that didn't work out.  What is "insider trading"?  ;-)

I don't know what google does; I'd search freescale documentation
directly.

Kim

Re: [PATCH v2] update crypto node definition and device tree instances

[Segher Boessenkool]

>>>> Also, these made-up names make you do more work: you'll need to
>>>
>>> who said they were made up?
>>
>> I did.  These names do not refer to some physical part you can buy.
>
> right, they refer to devices in multiple physical parts you can buy.
> Part-you-can-buy documentation clearly indicates the SEC version in
> that part, in the form "SEC X.Y", i.e, it's not something made up
> that's not already in freescale documentation.

Yes.  As a side note, since there are multiple devices that contain
e.g. a sec-1.0, it would be prudent to describe the exact incarnation
in the device tree, like "mpc8272-sec" or something, in either "model"
or "compatible", just in case a problem shows up with one of them.

>>>> write up a binding for them, explaining exactly what a 1.0 device
>>>> etc. is (or at least point to documentation for it).  If you use
>>>> a name that refers to some device that people can easily google
>>>> for documentation, you can skip this (well, you might need to
>>>> write a binding anyway; but at least you won't have to explain
>>>> what the device _is_).
>>>
>>> documentation is available in the usual places, and it specifically
>>> points out which SEC version it references.
>>
>> I can't find a manual online for "freescale sec"; googling
>> for "freescale sec-1.0" finds a manual for the PowerQUICC I;
>> is that the right one?  I don't know, so the binding needs
>> to explain it to me.
>
> the binding shouldn't be responsible for google's shortcomings

The binding needs to describe what device it is for.  I am a stupid
user, just like most users, so if the binding doesn't tell me I turn
to google.  Don't blame them for not finding it; the binding should
have told me in the first place!

> (that hit is correct, btw).

Okay, cool.

>> Going from SoC name -> SEC version is easy, but the other way around
>> not so.
>>
>> Anyway, minor stuff.
>
> sounds like you're pointing out a lack of "SEC versions guide"
> documentation of Freescale..

Yes, that would have helped.

>>> Plus, as I mentioned
>>> before, a lot of the differences between the SEC versions are 
>>> miniscule
>>> feature bits scattered across the programming model.
>>
>> I don't see how this is relevant, sorry.
>>
> I'm under the impression that listing the differences (assuming they're
> easily obtainable) would lead to unnecessary b-w-of bloat.

The binding at a minimum should describe how to identify each
unique version from the device tree, no matter how miniscule
those differences are.  Just a specific "compatible" value will
do.

> I don't know what google does; I'd search freescale documentation
> directly.

Or the binding could just bloody say what it is talking about in the
first place, heh.


Anyway, how about we do something constructive?  If you still want to
use "fsl,sec-N.M" names, that's fine with me.  Each specific device
tree needs to still say which exact device it contains, so an entry
would look like e.g.

	compatible = "fsl,mpc8272-sec", "fsl,sec-3.0";

and the driver can just probe for "fsl,sec-3.0" if it doesn't need
to know about the exact version; but it _can_ use it if it _does_
need to know.


Segher

Re: [PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

On Mon, 30 Jun 2008 23:19:05 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >>>> Also, these made-up names make you do more work: you'll need to
> >>>
> >>> who said they were made up?
> >>
> >> I did.  These names do not refer to some physical part you can buy.
> >
> > right, they refer to devices in multiple physical parts you can buy.
> > Part-you-can-buy documentation clearly indicates the SEC version in
> > that part, in the form "SEC X.Y", i.e, it's not something made up
> > that's not already in freescale documentation.
> 
> Yes.  As a side note, since there are multiple devices that contain
> e.g. a sec-1.0, it would be prudent to describe the exact incarnation
> in the device tree, like "mpc8272-sec" or something, in either "model"

but 'fsl,sec-X.Y' /does/ describe the exact incarnation, whereas
'fsl,mpc8349-sec' /does not/.  "fsl,mpc8349-sec' might mean the SEC 2.1
or the SEC 2.4, it depends on the revision of the mpc8349.

> or "compatible", just in case a problem shows up with one of them.

I thought 'model' was superseded by 'compatible'; that's why it's taken
out here, along with device_type.

> >>>> write up a binding for them, explaining exactly what a 1.0 device
> >>>> etc. is (or at least point to documentation for it).  If you use
> >>>> a name that refers to some device that people can easily google
> >>>> for documentation, you can skip this (well, you might need to
> >>>> write a binding anyway; but at least you won't have to explain
> >>>> what the device _is_).
> >>>
> >>> documentation is available in the usual places, and it specifically
> >>> points out which SEC version it references.
> >>
> >> I can't find a manual online for "freescale sec"; googling
> >> for "freescale sec-1.0" finds a manual for the PowerQUICC I;
> >> is that the right one?  I don't know, so the binding needs
> >> to explain it to me.
> >
> > the binding shouldn't be responsible for google's shortcomings
> 
> The binding needs to describe what device it is for.  I am a stupid
> user, just like most users, so if the binding doesn't tell me I turn
> to google.  Don't blame them for not finding it; the binding should
> have told me in the first place!

Again, I don't see how google's results are pertinent in this
discussion.  The fact that Freescale doesn't publish a separate SEC
manual is not what this patch is trying to address.

btw, the title for the binding is:

  g) Freescale SOC SEC Security Engines

Is that what you are looking for?  If not, what precisely?  a list of
all the parts?  There's an SEC in every mpc8[35]xxE!

> >>> Plus, as I mentioned
> >>> before, a lot of the differences between the SEC versions are 
> >>> miniscule
> >>> feature bits scattered across the programming model.
> >>
> >> I don't see how this is relevant, sorry.
> >>
> > I'm under the impression that listing the differences (assuming they're
> > easily obtainable) would lead to unnecessary b-w-of bloat.
> 
> The binding at a minimum should describe how to identify each
> unique version from the device tree, no matter how miniscule
> those differences are.  Just a specific "compatible" value will
> do.

I'm at a loss; isn't that what this patch does?

> > I don't know what google does; I'd search freescale documentation
> > directly.
> 
> Or the binding could just bloody say what it is talking about in the
> first place, heh.

Again, I'm at a loss here.  Can you give a specific example of what
you're looking for here?

> Anyway, how about we do something constructive?  If you still want to
> use "fsl,sec-N.M" names, that's fine with me.  Each specific device
> tree needs to still say which exact device it contains, so an entry
> would look like e.g.
> 
> 	compatible = "fsl,mpc8272-sec", "fsl,sec-3.0";
> 
> and the driver can just probe for "fsl,sec-3.0" if it doesn't need
> to know about the exact version; but it _can_ use it if it _does_
> need to know.

Currently the driver matches on "fsl,sec2.0", and if needs be, will
call of_device_is_compatible with the version number that introduces
the feature it wants to implement.

Kim

Re: [PATCH v2] update crypto node definition and device tree instances

[Segher Boessenkool]

>> Yes.  As a side note, since there are multiple devices that contain
>> e.g. a sec-1.0, it would be prudent to describe the exact incarnation
>> in the device tree, like "mpc8272-sec" or something, in either "model"
>
> but 'fsl,sec-X.Y' /does/ describe the exact incarnation,

No it doesn't.  If it's on a different SoC, it can have different
bugs.  It might be _meant_ to be exactly the same, but that's not
the same thing.

> whereas
> 'fsl,mpc8349-sec' /does not/.  "fsl,mpc8349-sec' might mean the SEC 2.1
> or the SEC 2.4, it depends on the revision of the mpc8349.

Oh, nasty.  That just means you'll need to put the revision of
the 8349 in there as well, though -- "fsl,mpc8349-rev2-sec" or
something.

>> or "compatible", just in case a problem shows up with one of them.
>
> I thought 'model' was superseded by 'compatible';

"model" is still a valid property.  "model" shouldn't be needed
to find which device driver to use, but any specific device driver
can use it just fine.

> that's why it's taken out here, along with device_type.

"device_type" simply isn't useful for flat device trees (in pretty
much all cases), since it describes the OF programming model of a
device, and almost none of that applies to flat trees.  So yeah,
taking that out is a good thing (esp. in a case like this, where
it isn't defined anywhere what device_type "crypto" means).

>>>> I can't find a manual online for "freescale sec"; googling
>>>> for "freescale sec-1.0" finds a manual for the PowerQUICC I;
>>>> is that the right one?  I don't know, so the binding needs
>>>> to explain it to me.
>>>
>>> the binding shouldn't be responsible for google's shortcomings
>>
>> The binding needs to describe what device it is for.  I am a stupid
>> user, just like most users, so if the binding doesn't tell me I turn
>> to google.  Don't blame them for not finding it; the binding should
>> have told me in the first place!
>
> Again, I don't see how google's results are pertinent in this
> discussion.

It's not about google.  It's about a user who needs to find out
what a certain "compatible" entry means, or who needs to find out
what value to use for a certain device.

> btw, the title for the binding is:
>
>   g) Freescale SOC SEC Security Engines
>
> Is that what you are looking for?

I'm not looking for the binding, I know where it is, thanks; I'm
looking for information in the binding that tells me what "compatible"
value means what.

> If not, what precisely?  a list of
> all the parts?  There's an SEC in every mpc8[35]xxE!

You could do a list of all, sure.  You could also say what a
"compatible" value looks like, and give some representative
examples.

>> The binding at a minimum should describe how to identify each
>> unique version from the device tree, no matter how miniscule
>> those differences are.  Just a specific "compatible" value will
>> do.
>
> I'm at a loss; isn't that what this patch does?

I lost the patch, sorry.  I came into this thread at the point where
Grant said that "fsl,sec1.0" is a horrible "compatible" value.

> Currently the driver matches on "fsl,sec2.0", and if needs be, will
> call of_device_is_compatible with the version number that introduces
> the feature it wants to implement.

That's okay I suppose.  Each device tree still should put the exact
version of the chip in there as well.


Segher

Re: [PATCH v2] update crypto node definition and device tree instances

[Kim Phillips]

On Tue, 1 Jul 2008 01:27:58 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >> Yes.  As a side note, since there are multiple devices that contain
> >> e.g. a sec-1.0, it would be prudent to describe the exact incarnation
> >> in the device tree, like "mpc8272-sec" or something, in either "model"
> >
> > but 'fsl,sec-X.Y' /does/ describe the exact incarnation,
> 
> No it doesn't.  If it's on a different SoC, it can have different
> bugs.  It might be _meant_ to be exactly the same, but that's not
> the same thing.

It might not be to you, but to me and others it's true; some of these
SoCs truly share the exact same SEC block.

> > whereas
> > 'fsl,mpc8349-sec' /does not/.  "fsl,mpc8349-sec' might mean the SEC 2.1
> > or the SEC 2.4, it depends on the revision of the mpc8349.
> 
> Oh, nasty.  That just means you'll need to put the revision of
> the 8349 in there as well, though -- "fsl,mpc8349-rev2-sec" or
> something.

so according to this and your above statement, it sounds like the scheme
you're proposing either warrants a separate device tree for each
revision of each SoC, with "fsl,mpcXXXX-revY.Z-whatever" property
values, or forces u-boot to fixup all compatible =
"fsl,mpcXXXX-whatever" properties with to a
"fsl,mpcXXXX-revY.Z-whatever" replacement, regardless whether that
"whatever" device is an SEC device or not.  That can easily lead to
considerable bloat, esp. since SoC-specific data (such as mpcXXXX and
its revision) belong as a single occurence in the SoC node (and can be
reached via of_get_parent).

If a driver comes to the point where it actually, specifically cares
about a specific erratum on a specific revision of a specific SoC, then
it can easily look up the ID register of the device (not SoC) it's
driving and modify the precise point of its behaviour change.  That's a
much simpler, direct solution, plus it's its responsibility as a device
driver to know and handle minor details like this anyway.

> >> or "compatible", just in case a problem shows up with one of them.
> >
> > I thought 'model' was superseded by 'compatible';
> 
> "model" is still a valid property.  "model" shouldn't be needed
> to find which device driver to use, but any specific device driver
> can use it just fine.
> 
sure, but it doesn't provide anything extra, and afaict, it doesn't
come with conveniences such as of_device_is_compatible.

> > that's why it's taken out here, along with device_type.
> 
> "device_type" simply isn't useful for flat device trees (in pretty
> much all cases), since it describes the OF programming model of a
> device, and almost none of that applies to flat trees.  So yeah,
> taking that out is a good thing (esp. in a case like this, where
> it isn't defined anywhere what device_type "crypto" means).

wunderbar.

> >>>> I can't find a manual online for "freescale sec"; googling
> >>>> for "freescale sec-1.0" finds a manual for the PowerQUICC I;
> >>>> is that the right one?  I don't know, so the binding needs
> >>>> to explain it to me.
> >>>
> >>> the binding shouldn't be responsible for google's shortcomings
> >>
> >> The binding needs to describe what device it is for.  I am a stupid
> >> user, just like most users, so if the binding doesn't tell me I turn
> >> to google.  Don't blame them for not finding it; the binding should
> >> have told me in the first place!
> >
> > Again, I don't see how google's results are pertinent in this
> > discussion.
> 
> It's not about google.  It's about a user who needs to find out
> what a certain "compatible" entry means, or who needs to find out

this goes back to the lack of a single piece of documentation that goes
over SEC version differences in detail, which I don't have.  I have seen
somewhat-useful bits in later revisions of the SEC that I know the
driver could use, e.g., a hardware integrity check feature in revs 2.1
and above.

> what value to use for a certain device.

one good thing about freescale parts documentation is that it clearly
informs you of what SEC version you're on.

> > btw, the title for the binding is:
> >
> >   g) Freescale SOC SEC Security Engines
> >
> > Is that what you are looking for?
> 
> I'm not looking for the binding, I know where it is, thanks; I'm
> looking for information in the binding that tells me what "compatible"
> value means what.

ok, I don't have that specific level of data.  sorry.

> > If not, what precisely?  a list of
> > all the parts?  There's an SEC in every mpc8[35]xxE!
> 
> You could do a list of all, sure.  You could also say what a

I'd rather use expressions.

> "compatible" value looks like, and give some representative
> examples.

um, there is one in the patch already.

> >> The binding at a minimum should describe how to identify each
> >> unique version from the device tree, no matter how miniscule
> >> those differences are.  Just a specific "compatible" value will
> >> do.
> >
> > I'm at a loss; isn't that what this patch does?
> 
> I lost the patch, sorry.  I came into this thread at the point where
> Grant said that "fsl,sec1.0" is a horrible "compatible" value.
> 
> > Currently the driver matches on "fsl,sec2.0", and if needs be, will
> > call of_device_is_compatible with the version number that introduces
> > the feature it wants to implement.
> 
> That's okay I suppose.  Each device tree still should put the exact
> version of the chip in there as well.

see above.

Kim