Skip to contentSkip to conversation

powerpc/85xx: Add support for the "socrates" board (MPC8544)

PATCH Applied Wolfgang Grandegger 5 participants 23 messages Mar 19, 2009 – Apr 2, 2009

Subsystems (3)
Device tree bindings PowerPC Linux for PowerPC embedded PPC85XX
lore
Share & tools
lore

Wolfgang Grandegger

author
#0 lore raw

This patch adds support for the "socrates" board based on the MPC8544. Supported are Ethernet, serial console, I2C, I2C-based RTC and temperature sensors, NOR and NAND flash, PCI, USB, CAN and Lime display controller.

The multiplexing of FPGA interrupts onto PowerPC interrupt lines is supported through our own fpga_pic interrupt controller driver.

For example the SJA1000 controller is level low sensitive connected to fpga_pic line 2 and is routed to the second (of three) irq lines to the CPU:

    can@3,100 {
            compatible = "philips,sja1000";
            reg = <3 0x100 0x80>;
            interrupts = <2 2>;
            interrupts = <2 8 1>;   // number, type, routing
            interrupt-parent = <&fpga_pic>;
    };
Signed-off-by Sergei Poselenov <[email protected]>
Signed-off-by Yuri Tikhonov <[email protected]>
Signed-off-by Ilya Yanok <[email protected]>
Signed-off-by Wolfgang Grandegger <[email protected]>
Signed-off-by Anatolij Gustschin <[email protected]>
Signed-off-by Dmitry Rakhchev <[email protected]>
---
 arch/powerpc/boot/Makefile                      |    1 
 arch/powerpc/boot/dts/socrates.dts              |  337 +++++
 arch/powerpc/boot/wrapper                       |    2 
 arch/powerpc/configs/85xx/socrates_defconfig    | 1410 ++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/Kconfig             |    6 
 arch/powerpc/platforms/85xx/Makefile            |    2 
 arch/powerpc/platforms/85xx/socrates.c          |  156 ++
 arch/powerpc/platforms/85xx/socrates_fpga_pic.c |  320 +++++
 arch/powerpc/platforms/85xx/socrates_fpga_pic.h |   16 
 9 files changed, 2249 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/boot/dts/socrates.dts
 create mode 100644 arch/powerpc/configs/85xx/socrates_defconfig
 create mode 100644 arch/powerpc/platforms/85xx/socrates.c
 create mode 100644 arch/powerpc/platforms/85xx/socrates_fpga_pic.c
 create mode 100644 arch/powerpc/platforms/85xx/socrates_fpga_pic.h
 create mode 100644 drivers/mtd/nand/socrates_nand.c
arch/powerpc/boot/Makefile +1 -0
Index: linux-2.6/arch/powerpc/boot/Makefile
===================================================================
@@ -267,6 +267,7 @@ image-$(CONFIG_TQM8555)			+= cuImage.tqm
 image-$(CONFIG_TQM8560)			+= cuImage.tqm8560
 image-$(CONFIG_SBC8548)			+= cuImage.sbc8548
 image-$(CONFIG_SBC8560)			+= cuImage.sbc8560
+image-$(CONFIG_SOCRATES)		+= cuImage.socrates
 image-$(CONFIG_KSI8560)			+= cuImage.ksi8560
 
 # Board ports in arch/powerpc/platform/embedded6xx/Kconfig
arch/powerpc/boot/dts/socrates.dts new file +337 -0

337 lines not on this page · raw message

arch/powerpc/boot/wrapper +1 -1
Index: linux-2.6/arch/powerpc/boot/wrapper
===================================================================
@@ -183,7 +183,7 @@ cuboot*)
     *-tqm8541|*-mpc8560*|*-tqm8560|*-tqm8555|*-ksi8560*)
         platformo=$object/cuboot-85xx-cpm2.o
         ;;
-    *-mpc85*|*-tqm85*|*-sbc85*)
+    *-mpc85*|*-tqm85*|*-sbc85*|*-socrates)
         platformo=$object/cuboot-85xx.o
         ;;
     esac
arch/powerpc/configs/85xx/socrates_defconfig new file +1410 -0

1,410 lines not on this page · raw message

arch/powerpc/platforms/85xx/Kconfig +6 -0
Index: linux-2.6/arch/powerpc/platforms/85xx/Kconfig
===================================================================
@@ -51,6 +51,12 @@ config MPC85xx_DS
 	help
 	  This option enables support for the MPC85xx DS (MPC8544 DS) board
 
+config SOCRATES
+	bool "Socrates"
+	select DEFAULT_UIMAGE
+	help
+	  This option enables support for the Socrates board.
+
 config KSI8560
         bool "Emerson KSI8560"
         select DEFAULT_UIMAGE
arch/powerpc/platforms/85xx/Makefile +2 -0
Index: linux-2.6/arch/powerpc/platforms/85xx/Makefile
===================================================================
@@ -13,4 +13,6 @@ obj-$(CONFIG_STX_GP3)	  += stx_gp3.o
 obj-$(CONFIG_TQM85xx)	  += tqm85xx.o
 obj-$(CONFIG_SBC8560)     += sbc8560.o
 obj-$(CONFIG_SBC8548)     += sbc8548.o
+obj-$(CONFIG_SOCRATES)    += socrates.o
+obj-$(CONFIG_SOCRATES)    += socrates_fpga_pic.o
 obj-$(CONFIG_KSI8560)	  += ksi8560.o
arch/powerpc/platforms/85xx/socrates.c new file +156 -0

156 lines not on this page · raw message

arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +320 -0

320 lines not on this page · raw message

arch/powerpc/platforms/85xx/socrates_fpga_pic.h new file +16 -0
Index: linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.h
===================================================================
@@ -0,0 +1,16 @@
+/*
+ *  Copyright (C) 2008 Ilya Yanok, Emcraft Systems
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef SOCRATES_FPGA_PIC_H
+#define SOCRATES_FPGA_PIC_H
+
+void socrates_fpga_pic_init(struct device_node *pic);
+
+#endif
#1 lore raw
Quoted 11 lines · Wolfgang Grandegger

This patch adds support for the "socrates" board based on the MPC8544. Supported are Ethernet, serial console, I2C, I2C-based RTC and temperature sensors, NOR and NAND flash, PCI, USB, CAN and Lime display controller.

The multiplexing of FPGA interrupts onto PowerPC interrupt lines is supported through our own fpga_pic interrupt controller driver.

For example the SJA1000 controller is level low sensitive connected to fpga_pic line 2 and is routed to the second (of three) irq lines to the CPU:

A few minor device tree nits.

arch/powerpc/boot/dts/socrates.dts line 60

Quoted 9 lines
arch/powerpc/boot/dts/socrates.dts new file +9 -0
+	soc8544@e0000000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+
+		ranges = <0x00000000 0xe0000000 0x00100000>;
+		reg = <0xe0000000 0x00001000>;	// CCSRBAR 1M
+		bus-frequency = <0>;		// Filled in by U-Boot
+		compatible = "fsl,socrates-immr", "simple-bus";

This should probably refer to 8544 instead of socrates. Unless you really do have a board-specific version of the SoC...

[snip]

arch/powerpc/boot/dts/socrates.dts line 131

Quoted 11 lines
arch/powerpc/boot/dts/socrates.dts new file +11 -0
+		mdio@24520 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "fsl,gianfar-mdio";
+			reg = <0x24520 0x20>;
+
+			phy0: ethernet-phy@0 {
+				interrupt-parent = <&mpic>;
+				interrupts = <0 1>;
+				reg = <0>;
+				device_type = "ethernet-phy";

No device_type here.

arch/powerpc/boot/dts/socrates.dts line 141

Quoted 10 lines
arch/powerpc/boot/dts/socrates.dts new file +10 -0
+			};
+			phy1: ethernet-phy@1 {
+				interrupt-parent = <&mpic>;
+				interrupts = <0 1>;
+				reg = <1>;
+				device_type = "ethernet-phy";
+			};
+			tbi0: tbi-phy@11 {
+				reg = <0x11>;
+				device_type = "tbi-phy";

Or here.

[snip]

arch/powerpc/boot/dts/socrates.dts line 225

Quoted 4 lines
arch/powerpc/boot/dts/socrates.dts new file +4 -0
+	localbus {
+		compatible = "fsl,socrates-localbus",
+		             "fsl,mpc85xx-localbus",
+		             "fsl,pq3-localbus";

Is this bus controller part of the SoC? If so, the first compatible string probably shouldn't be there (as for the soc/immr node itself).

[snip[

arch/powerpc/boot/dts/socrates.dts line 271

Quoted 2 lines
arch/powerpc/boot/dts/socrates.dts new file +2 -0
+		display@2,0 {
+			compatible = "fujitsu,lime";

This compat string looks slightly worryingly non-specific.

Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
author
#2 lore raw
Quoted 1 line · David Gibson

On Thu, Mar 19, 2009 at 04:26:44PM +0100, Wolfgang Grandegger wrote:

[snip]

Quoted 5 lines

[snip[

arch/powerpc/boot/dts/socrates.dts line 271

Quoted 2 lines
arch/powerpc/boot/dts/socrates.dts new file +2 -0
+		display@2,0 {
+			compatible = "fujitsu,lime";

This compat string looks slightly worryingly non-specific.

The node is for the Lime graphic controller from Fujitsu. What does worry you?

Wolfgang.

author
#3 lore raw
In reply to David Gibson
Quoted 15 lines · David Gibson
Quoted 11 lines · Wolfgang Grandegger

This patch adds support for the "socrates" board based on the MPC8544. Supported are Ethernet, serial console, I2C, I2C-based RTC and temperature sensors, NOR and NAND flash, PCI, USB, CAN and Lime display controller.

The multiplexing of FPGA interrupts onto PowerPC interrupt lines is supported through our own fpga_pic interrupt controller driver.

For example the SJA1000 controller is level low sensitive connected to fpga_pic line 2 and is routed to the second (of three) irq lines to the CPU:

A few minor device tree nits.

Quoted 12 lines

arch/powerpc/boot/dts/socrates.dts line 60

Quoted 9 lines
arch/powerpc/boot/dts/socrates.dts new file +9 -0
+	soc8544@e0000000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+
+		ranges = <0x00000000 0xe0000000 0x00100000>;
+		reg = <0xe0000000 0x00001000>;	// CCSRBAR 1M
+		bus-frequency = <0>;		// Filled in by U-Boot
+		compatible = "fsl,socrates-immr", "simple-bus";

This should probably refer to 8544 instead of socrates. Unless you really do have a board-specific version of the SoC...

OK, that should then be:

   +		compatible = "fsl,mpc8544-immr", "simple-bus";

It should always be with the prefix "mpc", I guess. But some of the following compatible names are without that prefix:

$ grep -h '\-l2-cache-controller' *
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,mpc8536-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,8541-l2-cache-controller";
			compatible = "fsl,8544-l2-cache-controller";
			compatible = "fsl,8548-l2-cache-controller";
			compatible = "fsl,8555-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,8568-l2-cache-controller";
			compatible = "fsl,mpc8572-l2-cache-controller";
			compatible = "fsl,mpc8572-l2-cache-controller";
			compatible = "fsl,mpc8572-l2-cache-controller";
			compatible = "fsl,8548-l2-cache-controller";
			compatible = "fsl,8560-l2-cache-controller";
			compatible = "fsl,mpc8544-l2-cache-controller";
			compatible = "fsl,mpc8544-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,mpc8548-l2-cache-controller";
			compatible = "fsl,mpc8548-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";
			compatible = "fsl,8540-l2-cache-controller";

That's confusing.

Wolfgang.

#4 lore raw
In reply to Wolfgang Grandegger
Quoted 40 lines · Wolfgang Grandegger
arch/powerpc/boot/dts/socrates.dts new file +40 -0
+		mdio@24520 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "fsl,gianfar-mdio";
+			reg = <0x24520 0x20>;
+
+			phy0: ethernet-phy@0 {
+				interrupt-parent = <&mpic>;
+				interrupts = <0 1>;
+				reg = <0>;
+				device_type = "ethernet-phy";
+			};
+			phy1: ethernet-phy@1 {
+				interrupt-parent = <&mpic>;
+				interrupts = <0 1>;
+				reg = <1>;
+				device_type = "ethernet-phy";
+			};
+			tbi0: tbi-phy@11 {
+				reg = <0x11>;
+				device_type = "tbi-phy";
+			};
+		};
+
+		mdio@26520 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "fsl,gianfar-tbi";
+			reg = <0x26520 0x20>;
+
+			tbi1: tbi-phy@11 {
+				reg = <0x11>;
+				device_type = "tbi-phy";
+			};
+		};
+
+		enet0: ethernet@24000 {
+			cell-index = <0>;
+			device_type = "network";
+			model = "eTSEC";

arch/powerpc/boot/dts/socrates.dts line 170

Quoted 10 lines
arch/powerpc/boot/dts/socrates.dts new file +10 -0
+			compatible = "gianfar";
+			reg = <0x24000 0x1000>;
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			interrupts = <29 2 30 2 34 2>;
+			interrupt-parent = <&mpic>;
+			phy-handle = <&phy0>;
+			tbi-handle = <&tbi0>;
+			phy-connection-type = "rgmii-id";
+		};
+

See Anton's recent post in moving the mdio node under the ethernet. Please match.

- k

author
#5 lore raw

Hi Kumar,

Quoted 48 lines · Kumar Gala
Quoted 45 lines · Wolfgang Grandegger
arch/powerpc/boot/dts/socrates.dts new file +45 -0
+        mdio@24520 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "fsl,gianfar-mdio";
+            reg = <0x24520 0x20>;
+
+            phy0: ethernet-phy@0 {
+                interrupt-parent = <&mpic>;
+                interrupts = <0 1>;
+                reg = <0>;
+                device_type = "ethernet-phy";
+            };
+            phy1: ethernet-phy@1 {
+                interrupt-parent = <&mpic>;
+                interrupts = <0 1>;
+                reg = <1>;
+                device_type = "ethernet-phy";
+            };
+            tbi0: tbi-phy@11 {
+                reg = <0x11>;
+                device_type = "tbi-phy";
+            };
+        };
+
+        mdio@26520 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "fsl,gianfar-tbi";
+            reg = <0x26520 0x20>;
+
+            tbi1: tbi-phy@11 {
+                reg = <0x11>;
+                device_type = "tbi-phy";
+            };
+        };
+
+        enet0: ethernet@24000 {
+            cell-index = <0>;
+            device_type = "network";
+            model = "eTSEC";
+            compatible = "gianfar";
+            reg = <0x24000 0x1000>;
+            local-mac-address = [ 00 00 00 00 00 00 ];
+            interrupts = <29 2 30 2 34 2>;
+            interrupt-parent = <&mpic>;
Quoted 8 lines

arch/powerpc/boot/dts/socrates.dts line 170

Quoted 5 lines
arch/powerpc/boot/dts/socrates.dts new file +5 -0
+            phy-handle = <&phy0>;
+            tbi-handle = <&tbi0>;
+            phy-connection-type = "rgmii-id";
+        };
+

See Anton's recent post in moving the mdio node under the ethernet. Please match.

I have fixed it for the socrates board. Is there a GIT tree with that modifications which I can base my patches on, also for the one for the TQM8548 posted recently:

http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069364.html

Wolfgang.

#6 lore raw
In reply to Wolfgang Grandegger

I agree 100% with David's comments, and I have some additional ones below.

On Thu, Mar 19, 2009 at 9:26 AM, Wolfgang Grandegger <[email protected]> wr= ote:

Quoted 4 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Drop device_type here too.

Quoted 2 lines

+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 ranges =3D <0x00000000 0xe0000000 0x0010000=

0>;

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0xe0000000 0x00001000>; =A0// CCSR=

BAR 1M

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 bus-frequency =3D <0>; =A0 =A0 =A0 =A0 =A0 =

=A0// Filled in by U-Boot

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,socrates-immr", "simple=

-bus";

As David said, fix this to be SoC specific, not board specific.

Quoted 3 lines

+ =A0 =A0 =A0 localbus {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,socrates-localbus",
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"fsl,mpc85xx-loc=

albus",

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"fsl,pq3-localbu=

s";

1st entry shouldn't be there.
2nd entry should specify exact chip
3rd entry I don't like much, but others may debate me on it Also, add "simple-bus" to this list. (important for a later comment)

Quoted 6 lines

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <2>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0xe0005000 0x40>;
+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 ranges =3D <0 0 0xfc000000 0x04000000
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 2 0 0xc8000000 0x040000=

00

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 3 0 0xc0000000 0x001000=

00

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 >; /* Overwritten by U-Boot=

*/

Just curious, why is U-Boot overwriting the ranges property?

Quoted 2 lines

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 fpga_pic: fpga-pic@3,10 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "abb,socrate=

s-fpga-pic";

Is 'abb' the companies' stock ticker symbol? If not, then use the real name and not an abbreviation.

Quoted 2 lines

Index: linux-2.6/arch/powerpc/boot/wrapper =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Quoted 10 lines
arch/powerpc/boot/wrapper +0 -0
@@ -183,7 +183,7 @@ cuboot*)
 =A0 =A0 *-tqm8541|*-mpc8560*|*-tqm8560|*-tqm8555|*-ksi8560*)

=A0 =A0 =A0 =A0 platformo=3D$object/cuboot-85xx-cpm2.o =A0 =A0 =A0 =A0 ;;

- =A0 =A0*-mpc85*|*-tqm85*|*-sbc85*)
+ =A0 =A0*-mpc85*|*-tqm85*|*-sbc85*|*-socrates)

=A0 =A0 =A0 =A0 platformo=3D$object/cuboot-85xx.o
=A0 =A0 =A0 =A0 ;;

Is this a new or old platform? Can U-Boot on the board boot with a uImage + dtb instead of a cuImage? I'd prefer to avoid adding new cuImages to the wrapper script if at all possible.

Quoted 2 lines

Index: linux-2.6/arch/powerpc/configs/85xx/socrates_defconfig =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

arch/powerpc/configs/85xx/socrates_defconfig

Quoted 2 lines
arch/powerpc/configs/85xx/socrates_defconfig new file +0 -0

Is a socrates-specific defconfig really warranted?

arch/powerpc/platforms/85xx/Makefile

Quoted 8 lines
arch/powerpc/platforms/85xx/Makefile +0 -0
@@ -13,4 +13,6 @@ obj-$(CONFIG_STX_GP3) =A0 +=3D stx_gp3.o

=A0obj-$(CONFIG_TQM85xx) =A0 =A0+=3D tqm85xx.o
=A0obj-$(CONFIG_SBC8560) =A0 =A0 +=3D sbc8560.o
=A0obj-$(CONFIG_SBC8548) =A0 =A0 +=3D sbc8548.o

arch/powerpc/platforms/85xx/Makefile +2 -0
+obj-$(CONFIG_SOCRATES) =A0 =A0+=3D socrates.o
+obj-$(CONFIG_SOCRATES) =A0 =A0+=3D socrates_fpga_pic.o

The pic stuff isn't all that big. Personally I'd roll it all into the socrates.c file.

arch/powerpc/platforms/85xx/socrates.c

Quoted 11 lines
arch/powerpc/platforms/85xx/socrates.c new file +9 -0
+static void __init socrates_pic_init(void)
+{
+ =A0 =A0 =A0 struct mpic *mpic;
+ =A0 =A0 =A0 struct resource r;
+ =A0 =A0 =A0 struct device_node *np;
+
+ =A0 =A0 =A0 np =3D of_find_node_by_type(NULL, "open-pic");
+ =A0 =A0 =A0 if (!np) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "Could not find open-pic no=

de\n");

Quoted 5 lines

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 if (of_address_to_resource(np, 0, &r)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "Could not map mpic registe=

r space\n");

Quoted 6 lines

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(np);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 mpic =3D mpic_alloc(np, r.start,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 MPIC_PRIMARY | MPIC_WANTS_R=

ESET | MPIC_BIG_ENDIAN,

Quoted 5 lines

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 0, 256, " OpenPIC =A0");
+ =A0 =A0 =A0 BUG_ON(mpic =3D=3D NULL);
+ =A0 =A0 =A0 of_node_put(np);
+
+ =A0 =A0 =A0 mpic_init(mpic);

Heh, this is a block of code cloned between all the 85xx boards it seems. Smells like a small refactoring candidate. This isn't really a critique of this patch, but I noticed it so I thought I'd mention it.

Quoted 4 lines
arch/powerpc/platforms/85xx/socrates.c new file +4 -0
+static void socrates_show_cpuinfo(struct seq_file *m)
+{
+ =A0 =A0 =A0 uint pvid, svid, phid1;
+ =A0 =A0 =A0 uint memsize =3D total_memory;

arch/powerpc/platforms/85xx/socrates.c

Quoted 10 lines
arch/powerpc/platforms/85xx/socrates.c new file +10 -0
+
+ =A0 =A0 =A0 pvid =3D mfspr(SPRN_PVR);
+ =A0 =A0 =A0 svid =3D mfspr(SPRN_SVR);
+
+ =A0 =A0 =A0 seq_printf(m, "PVR\t\t: 0x%x\n", pvid);
+ =A0 =A0 =A0 seq_printf(m, "SVR\t\t: 0x%x\n", svid);
+
+ =A0 =A0 =A0 /* Display cpu Pll setting */
+ =A0 =A0 =A0 phid1 =3D mfspr(SPRN_HID1);
+ =A0 =A0 =A0 seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3=

f));

Quoted 3 lines

+
+ =A0 =A0 =A0 /* Display the amount of memory */
+ =A0 =A0 =A0 seq_printf(m, "Memory\t\t: %d MB\n", memsize / (1024 * 1024=

));

Quoted 1 line

+}

Another block of duplicated code. In fact, many platforms have dropped the cpuinfo hook entirely and just use the default output.

arch/powerpc/platforms/85xx/socrates.c

Quoted 5 lines
arch/powerpc/platforms/85xx/socrates.c new file +5 -0
+
+static struct of_device_id __initdata of_bus_ids[] =3D {
+ =A0 =A0 =A0 { .name =3D "soc", },
+ =A0 =A0 =A0 { .type =3D "soc", },
+ =A0 =A0 =A0 { .name =3D "localbus", },

Drop these three lines. It is considered bad form now to bind on either name or type for flattened device trees. Instead add one { .compatible =3D "simple-bus", }, entry and make sure the immr and localbus nodes include "simple-bus" in the compatible string.

arch/powerpc/platforms/85xx/socrates.c line 131

Quoted 10 lines

+ =A0 =A0 =A0 {},

arch/powerpc/platforms/85xx/socrates.c new file +9 -0
+};
+
+static int __init declare_of_platform_devices(void)
+{
+ =A0 =A0 =A0 of_platform_bus_probe(NULL, of_bus_ids, NULL);
+
+ =A0 =A0 =A0 return 0;
+}
+machine_device_initcall(socrates, declare_of_platform_devices);

Don't add an initcall for this. Instead assign declar_of_platform_devices to the .init member of in the define_machine() block below.

Quoted 2 lines

Index: linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Quoted 26 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +23 -0
@@ -0,0 +1,320 @@
+/*
+ * =A0Copyright (C) 2008 Ilya Yanok, Emcraft Systems
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/irq.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+
+#define SOCRATES_FPGA_NUM_IRQS 9
+
+#define FPGA_PIC_IRQCFG =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(0x0)
+#define FPGA_PIC_IRQMASK(n) =A0 =A0(0x4 + 0x4 * (n))
+
+#define SOCRATES_FPGA_IRQ_MASK ((1 << SOCRATES_FPGA_NUM_IRQS) - 1)
+
+struct socrates_fpga_irq_info {
+ =A0 =A0 =A0 unsigned int irq_line;

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 33

Quoted 10 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +10 -0
+ =A0 =A0 =A0 int type;
+};
+
+/*
+ * Interrupt routing and type table
+ *
+ * IRQ_TYPE_NONE means the interrupt type is configurable,
+ * otherwise it's fixed to the specified value.
+ */
+static struct socrates_fpga_irq_info fpga_irqs[SOCRATES_FPGA_NUM_IRQS] =

=3D {

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 45

Quoted 12 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +12 -0
+ =A0 =A0 =A0 [0] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [1] =3D {0, IRQ_TYPE_LEVEL_HIGH},
+ =A0 =A0 =A0 [2] =3D {0, IRQ_TYPE_LEVEL_LOW},
+ =A0 =A0 =A0 [3] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [4] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [5] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [6] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [7] =3D {0, IRQ_TYPE_NONE},
+ =A0 =A0 =A0 [8] =3D {0, IRQ_TYPE_LEVEL_HIGH},
+};
+
+#define socrates_fpga_irq_to_hw(virq) =A0 =A0((unsigned int)irq_map[virq=

].hwirq)

Quoted 19 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +19 -0
+
+static DEFINE_SPINLOCK(socrates_fpga_pic_lock);
+
+static void __iomem *socrates_fpga_pic_iobase;
+static struct irq_host *socrates_fpga_pic_irq_host;
+static unsigned int socrates_fpga_irqs[3];
+
+static inline uint32_t socrates_fpga_pic_read(int reg)
+{
+ =A0 =A0 =A0 return in_be32(socrates_fpga_pic_iobase + reg);
+}
+
+static inline void socrates_fpga_pic_write(int reg, uint32_t val)
+{
+ =A0 =A0 =A0 out_be32(socrates_fpga_pic_iobase + reg, val);
+}
+
+static inline unsigned int socrates_fpga_pic_get_irq(unsigned int irq)
+{

arch/powerpc/platforms/85xx/socrates_fpga_pic.c

Quoted 10 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +10 -0
+ =A0 =A0 =A0 uint32_t cause;
+ =A0 =A0 =A0 unsigned long flags;
+ =A0 =A0 =A0 int i;
+
+ =A0 =A0 =A0 for (i =3D 0; i < 3; i++) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (irq =3D=3D socrates_fpga_irqs[i])
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
+ =A0 =A0 =A0 }
+ =A0 =A0 =A0 if (i =3D=3D 3)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NO_IRQ;

This is interesting. What does it mean? It would be helpful to have a theory of operation blurb in this file for stuff like this..

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 76

Quoted 2 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +2 -0
+static int socrates_fpga_pic_host_xlate(struct irq_host *h,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct device_node *ct, u32 *intspec, unsig=

ned int intsize,

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 77

Quoted 1 line
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 irq_hw_number_t *out_hwirq, unsigned int *o=

ut_flags)

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 79

Quoted 2 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +2 -0
+{
+ =A0 =A0 =A0 struct socrates_fpga_irq_info *fpga_irq =3D &fpga_irqs[ints=

pec[0]];

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 85

Quoted 6 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +6 -0
+
+ =A0 =A0 =A0 *out_hwirq =3D intspec[0];
+ =A0 =A0 =A0 if =A0(fpga_irq->type =3D=3D IRQ_TYPE_NONE) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* type is configurable */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (intspec[1] !=3D IRQ_TYPE_LEVEL_LOW &&
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 intspec[1] !=3D IRQ_TYPE_LEVEL_HIGH=

) {

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 86

Quoted 1 line
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING "FPGA P=

IC: invalid irq type, "

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 87

Quoted 1 line
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"setting def=

ault active low\n");

Nit: pr_warn() perhaps? And same through the rest of the file.

Cheers,
g.

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

author
#7 lore raw
Quoted 1 line · Grant Likely

I agree 100% with David's comments, and I have some additional ones below.

OK.

Quoted 21 lines · Wolfgang Grandegger

arch/powerpc/boot/dts/socrates.dts line 55

Quoted 4 lines
arch/powerpc/boot/dts/socrates.dts new file +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

arch/powerpc/boot/dts/socrates.dts line 60

Quoted 5 lines
arch/powerpc/boot/dts/socrates.dts new file +5 -0
+
+               ranges = <0x00000000 0xe0000000 0x00100000>;
+               reg = <0xe0000000 0x00001000>;  // CCSRBAR 1M
+               bus-frequency = <0>;            // Filled in by U-Boot
+               compatible = "fsl,socrates-immr", "simple-bus";

As David said, fix this to be SoC specific, not board specific.

arch/powerpc/boot/dts/socrates.dts line 225

Quoted 4 lines
arch/powerpc/boot/dts/socrates.dts new file +4 -0
+       localbus {
+               compatible = "fsl,socrates-localbus",
+                            "fsl,mpc85xx-localbus",
+                            "fsl,pq3-localbus";
Quoted 4 lines

1st entry shouldn't be there.
2nd entry should specify exact chip
3rd entry I don't like much, but others may debate me on it Also, add "simple-bus" to this list. (important for a later comment)

OK.

Quoted 11 lines

arch/powerpc/boot/dts/socrates.dts line 233

Quoted 9 lines
arch/powerpc/boot/dts/socrates.dts new file +8 -0
+               #address-cells = <2>;
+               #size-cells = <1>;
+               reg = <0xe0005000 0x40>;
+
+               ranges = <0 0 0xfc000000 0x04000000
+                         2 0 0xc8000000 0x04000000
+                         3 0 0xc0000000 0x00100000
+                       >; /* Overwritten by U-Boot */

Just curious, why is U-Boot overwriting the ranges property?

Because there are boards without FPGA or graphic controller on the local bus.

Quoted 5 lines

arch/powerpc/boot/dts/socrates.dts line 278

Quoted 2 lines
arch/powerpc/boot/dts/socrates.dts new file +2 -0
+               fpga_pic: fpga-pic@3,10 {
+                       compatible = "abb,socrates-fpga-pic";

Is 'abb' the companies' stock ticker symbol? If not, then use the real name and not an abbreviation.

Yes.

Quoted 8 lines
Quoted 8 lines

Index: linux-2.6/arch/powerpc/boot/wrapper
===================================================================

arch/powerpc/boot/wrapper +0 -0
@@ -183,7 +183,7 @@ cuboot*)
    *-tqm8541|*-mpc8560*|*-tqm8560|*-tqm8555|*-ksi8560*)
        platformo=$object/cuboot-85xx-cpm2.o
        ;;
Quoted 8 lines

arch/powerpc/boot/wrapper cuboot*) · line 188

Quoted 4 lines
arch/powerpc/boot/wrapper +1 -1
-    *-mpc85*|*-tqm85*|*-sbc85*)
+    *-mpc85*|*-tqm85*|*-sbc85*|*-socrates)
        platformo=$object/cuboot-85xx.o
        ;;

Is this a new or old platform? Can U-Boot on the board boot with a uImage + dtb instead of a cuImage? I'd prefer to avoid adding new cuImages to the wrapper script if at all possible.

It's a new platform. Therefore I will drop cuboot support.

Quoted 7 lines

arch/powerpc/configs/85xx/socrates_defconfig

Quoted 4 lines

Index: linux-2.6/arch/powerpc/configs/85xx/socrates_defconfig
===================================================================

arch/powerpc/configs/85xx/socrates_defconfig new file +0 -0

Is a socrates-specific defconfig really warranted?

Quoted 11 lines

arch/powerpc/platforms/85xx/Makefile obj-$(CONFIG_STX_GP3) += stx_gp3.o · line 17

Quoted 8 lines
arch/powerpc/platforms/85xx/Makefile +2 -0
@@ -13,4 +13,6 @@ obj-$(CONFIG_STX_GP3)   += stx_gp3.o
 obj-$(CONFIG_TQM85xx)    += tqm85xx.o
 obj-$(CONFIG_SBC8560)     += sbc8560.o
 obj-$(CONFIG_SBC8548)     += sbc8548.o
+obj-$(CONFIG_SOCRATES)    += socrates.o
+obj-$(CONFIG_SOCRATES)    += socrates_fpga_pic.o

The pic stuff isn't all that big. Personally I'd roll it all into the socrates.c file.

Well,

  $ wc -l socrates_fpga_pic.c
  156 socrates.c
  320 socrates_fpga_pic.c

Personally, I'd prefer to separate the pic from the board init code, especially with that size.

Quoted 46 lines
Quoted 17 lines
arch/powerpc/platforms/85xx/socrates.c new file +15 -0
+static void __init socrates_pic_init(void)
+{
+       struct mpic *mpic;
+       struct resource r;
+       struct device_node *np;
+
+       np = of_find_node_by_type(NULL, "open-pic");
+       if (!np) {
+               printk(KERN_ERR "Could not find open-pic node\n");
+               return;
+       }
+
+       if (of_address_to_resource(np, 0, &r)) {
+               printk(KERN_ERR "Could not map mpic register space\n");
+               of_node_put(np);

arch/powerpc/platforms/85xx/socrates.c line 70

Quoted 10 lines
arch/powerpc/platforms/85xx/socrates.c new file +10 -0
+               return;
+       }
+
+       mpic = mpic_alloc(np, r.start,
+                       MPIC_PRIMARY | MPIC_WANTS_RESET | MPIC_BIG_ENDIAN,
+                       0, 256, " OpenPIC  ");
+       BUG_ON(mpic == NULL);
+       of_node_put(np);
+
+       mpic_init(mpic);

Heh, this is a block of code cloned between all the 85xx boards it seems. Smells like a small refactoring candidate. This isn't really a critique of this patch, but I noticed it so I thought I'd mention it.

Quoted 13 lines
arch/powerpc/platforms/85xx/socrates.c new file +13 -0
+static void socrates_show_cpuinfo(struct seq_file *m)
+{
+       uint pvid, svid, phid1;
+       uint memsize = total_memory;
+
+       pvid = mfspr(SPRN_PVR);
+       svid = mfspr(SPRN_SVR);
+
+       seq_printf(m, "PVR\t\t: 0x%x\n", pvid);
+       seq_printf(m, "SVR\t\t: 0x%x\n", svid);
+
+       /* Display cpu Pll setting */
+       phid1 = mfspr(SPRN_HID1);
Quoted 8 lines

arch/powerpc/platforms/85xx/socrates.c line 116

Quoted 5 lines
arch/powerpc/platforms/85xx/socrates.c new file +5 -0
+       seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3f));
+
+       /* Display the amount of memory */
+       seq_printf(m, "Memory\t\t: %d MB\n", memsize / (1024 * 1024));
+}

Another block of duplicated code. In fact, many platforms have dropped the cpuinfo hook entirely and just use the default output.

I can drop it for this board as well, no problem.

Quoted 11 lines

arch/powerpc/platforms/85xx/socrates.c line 121

Quoted 5 lines
arch/powerpc/platforms/85xx/socrates.c new file +5 -0
+
+static struct of_device_id __initdata of_bus_ids[] = {
+       { .name = "soc", },
+       { .type = "soc", },
+       { .name = "localbus", },

Drop these three lines. It is considered bad form now to bind on either name or type for flattened device trees. Instead add one { .compatible = "simple-bus", }, entry and make sure the immr and localbus nodes include "simple-bus" in the compatible string.

Quoted 14 lines

arch/powerpc/platforms/85xx/socrates.c line 131

Quoted 10 lines
arch/powerpc/platforms/85xx/socrates.c new file +10 -0
+       {},
+};
+
+static int __init declare_of_platform_devices(void)
+{
+       of_platform_bus_probe(NULL, of_bus_ids, NULL);
+
+       return 0;
+}
+machine_device_initcall(socrates, declare_of_platform_devices);

Don't add an initcall for this. Instead assign declar_of_platform_devices to the .init member of in the define_machine() block below.

OK.

Quoted 74 lines
Quoted 74 lines

Index: linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c
===================================================================

arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +69 -0
@@ -0,0 +1,320 @@
+/*
+ *  Copyright (C) 2008 Ilya Yanok, Emcraft Systems
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/irq.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+
+#define SOCRATES_FPGA_NUM_IRQS 9
+
+#define FPGA_PIC_IRQCFG                (0x0)
+#define FPGA_PIC_IRQMASK(n)    (0x4 + 0x4 * (n))
+
+#define SOCRATES_FPGA_IRQ_MASK ((1 << SOCRATES_FPGA_NUM_IRQS) - 1)
+
+struct socrates_fpga_irq_info {
+       unsigned int irq_line;
+       int type;
+};
+
+/*
+ * Interrupt routing and type table
+ *
+ * IRQ_TYPE_NONE means the interrupt type is configurable,
+ * otherwise it's fixed to the specified value.
+ */
+static struct socrates_fpga_irq_info fpga_irqs[SOCRATES_FPGA_NUM_IRQS] = {
+       [0] = {0, IRQ_TYPE_NONE},
+       [1] = {0, IRQ_TYPE_LEVEL_HIGH},
+       [2] = {0, IRQ_TYPE_LEVEL_LOW},
+       [3] = {0, IRQ_TYPE_NONE},
+       [4] = {0, IRQ_TYPE_NONE},
+       [5] = {0, IRQ_TYPE_NONE},
+       [6] = {0, IRQ_TYPE_NONE},
+       [7] = {0, IRQ_TYPE_NONE},
+       [8] = {0, IRQ_TYPE_LEVEL_HIGH},
+};
+
+#define socrates_fpga_irq_to_hw(virq)    ((unsigned int)irq_map[virq].hwirq)
+
+static DEFINE_SPINLOCK(socrates_fpga_pic_lock);
+
+static void __iomem *socrates_fpga_pic_iobase;
+static struct irq_host *socrates_fpga_pic_irq_host;
+static unsigned int socrates_fpga_irqs[3];
+
+static inline uint32_t socrates_fpga_pic_read(int reg)
+{
+       return in_be32(socrates_fpga_pic_iobase + reg);
+}
+
+static inline void socrates_fpga_pic_write(int reg, uint32_t val)
+{
+       out_be32(socrates_fpga_pic_iobase + reg, val);
+}
+
+static inline unsigned int socrates_fpga_pic_get_irq(unsigned int irq)
+{
+       uint32_t cause;
+       unsigned long flags;
+       int i;
+
+       for (i = 0; i < 3; i++) {
Quoted 8 lines

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 74

Quoted 5 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +5 -0
+               if (irq == socrates_fpga_irqs[i])
+                       break;
+       }
+       if (i == 3)
+               return NO_IRQ;

This is interesting. What does it mean? It would be helpful to have a theory of operation blurb in this file for stuff like this..

Just three interrupt lines are routed to the MPIC. A BUG_ON would be more appropriate, though.

Quoted 7 lines
Quoted 7 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +7 -0
+static int socrates_fpga_pic_host_xlate(struct irq_host *h,
+               struct device_node *ct, u32 *intspec, unsigned int intsize,
+               irq_hw_number_t *out_hwirq, unsigned int *out_flags)
+{
+       struct socrates_fpga_irq_info *fpga_irq = &fpga_irqs[intspec[0]];
+
+       *out_hwirq = intspec[0];
Quoted 8 lines

arch/powerpc/platforms/85xx/socrates_fpga_pic.c line 260

Quoted 6 lines
arch/powerpc/platforms/85xx/socrates_fpga_pic.c new file +6 -0
+       if  (fpga_irq->type == IRQ_TYPE_NONE) {
+               /* type is configurable */
+               if (intspec[1] != IRQ_TYPE_LEVEL_LOW &&
+                   intspec[1] != IRQ_TYPE_LEVEL_HIGH) {
+                       printk(KERN_WARNING "FPGA PIC: invalid irq type, "
+                              "setting default active low\n");

Nit: pr_warn() perhaps? And same through the rest of the file.

Yep, will fix the not commented issues as well.

Wolfgang.

author
#8 lore raw
In reply to Grant Likely
Quoted 9 lines · Grant Likely

I agree 100% with David's comments, and I have some additional ones below.

arch/powerpc/boot/dts/socrates.dts line 55

Quoted 4 lines · Wolfgang Grandegger
arch/powerpc/boot/dts/socrates.dts new file +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

Wolfgang.

#9 lore raw

On Tue, Mar 31, 2009 at 3:35 AM, Wolfgang Grandegger <[email protected]> wr= ote:

Quoted 2 lines
Quoted 1 line · Grant Likely

I agree 100% with David's comments, and I have some additional ones belo=

w.

Quoted 2 lines
Quoted 2 lines

On Thu, Mar 19, 2009 at 9:26 AM, Wolfgang Grandegger <[email protected]>=

Quoted 7 lines · wrote:
Quoted 6 lines
Quoted 4 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible =3D "fsl,mpc8544-immr"; (immr =3D=3D Internally Memory Mapped R= egisters)

Many other boards already do this.

g.

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

author
#10 lore raw
Quoted 19 lines · Grant Likely
Quoted 9 lines · Wolfgang Grandegger
Quoted 8 lines · Grant Likely

I agree 100% with David's comments, and I have some additional ones below.

arch/powerpc/boot/dts/socrates.dts line 55

Quoted 4 lines · Wolfgang Grandegger
arch/powerpc/boot/dts/socrates.dts new file +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

Quoted 5 lines

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr"; (immr == Internally Memory Mapped Registers)

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Unfortunately, other 85xx functions search for the device type "soc" as well. Therefore I think we must keep the devices type "soc" for the time being. Kumar?

Wolfgang.

#11 lore raw

On Tue, Mar 31, 2009 at 7:36 AM, Wolfgang Grandegger <[email protected]> wr= ote:

Quoted 2 lines
Quoted 1 line · Grant Likely

On Tue, Mar 31, 2009 at 3:35 AM, Wolfgang Grandegger <[email protected]>=

Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · Grant Likely

I agree 100% with David's comments, and I have some additional ones be=

low.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

On Thu, Mar 19, 2009 at 9:26 AM, Wolfgang Grandegger <[email protected]=

Quoted 11 lines · m> wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
Quoted 4 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a commo=

n

Quoted 1 line
Quoted 1 line
Quoted 1 line

function for the MPC52xx as well, but it's not obvious to me how to fin=

d

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible =3D "fsl,mpc8544-immr"; =A0(immr =3D=3D Internally Memory Map=

ped Registers)

Quoted 7 lines
Quoted 2 lines

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

Quoted 3 lines

Unfortunately, other 85xx functions search for the device type "soc" as well. Therefore I think we must keep the devices type "soc" for the time being. Kumar?

Fix them! :-)

Most troublesome places are where of_find_node_by_type() is used (I see two cases in fsl_soc.c). I added the of_find_matching_node() function specifically to rework code that was only matching on a single compatible or type value. See usage of mpc52xx_bus_ids in arch/powerpc/platforms/52xx/mpc52xx_common.c for an example.

It is a pain, but I think it is important to be reducing device_type usage as we hit against them.

g.

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

#12 lore raw

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr";  (immr == Internally Memory Mapped Registers)

Many other boards already do this.

Quoted 7 lines
Quoted 4 lines

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type = "soc".

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
#13 lore raw
Quoted 8 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broke=

n

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a co=

mmon

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

function for the MPC52xx as well, but it's not obvious to me how to =

find

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

the SOC node without the device type property.

SoC node should have a compatible property, just like everything else=

.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

compatible =3D "fsl,mpc8544-immr"; =A0(immr =3D=3D Internally Memory =

Mapped Registers)

Quoted 23 lines
Quoted 10 lines
Quoted 7 lines
Quoted 2 lines

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

Quoted 6 lines

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type =3D "soc".

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

I'm totally fine with fsl,soc *providing* that it is documented as to exactly what it describes, what properties are expected, and how they are used. Since fsl,soc is not tied to a specific piece of silicon I want to guard against the definition of "fsl,soc" drifting over time.

g.

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

author
#14 lore raw
In reply to Anton Vorontsov
Quoted 39 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 5 lines
Quoted 5 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr"; (immr == Internally Memory Mapped Registers)

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

Quoted 6 lines

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type = "soc".

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Wolfgang.

#15 lore raw

On Wed, Apr 1, 2009 at 1:36 AM, Wolfgang Grandegger <[email protected]> wro= te:

Quoted 9 lines
Quoted 8 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broke=

n

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a co=

mmon

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

function for the MPC52xx as well, but it's not obvious to me how to =

find

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

the SOC node without the device type property.

SoC node should have a compatible property, just like everything else=

.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

compatible =3D "fsl,mpc8544-immr"; =A0(immr =3D=3D Internally Memory =

Mapped Registers)

Quoted 25 lines
Quoted 25 lines
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type =3D "soc".

Quoted 5 lines
Quoted 2 lines

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Yes, I know this. I'm not asking you to fix all the other boards, but make sure that it is not required for the new board.

g.

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

author
#16 lore raw
Quoted 44 lines · Grant Likely
Quoted 43 lines · Wolfgang Grandegger
Quoted 42 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 5 lines
Quoted 5 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr"; (immr == Internally Memory Mapped Registers)

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type = "soc".

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Yes, I know this. I'm not asking you to fix all the other boards, but make sure that it is not required for the new board.

Hm, I'm confused, if we want to fix this issue we need first to

- fix all functions in fsl_soc.c searching for the compatible string "fsl,soc" instead of the device type "soc" (or both for backward compatibility).

- fix U-Boot to find the SOC node by looking for "fsl,soc" to insert the proper bus-frequency, at least.

That affects *all* boards using CONFIG_FSL_SOC and requires an up-to-date version of U-Boot for new kernels :-(. If that is fixed, I can remove the "device_type = "soc";" from socrates.dts (and may more), but not right now. Or have I missed something?

Wolfgang.

#17 lore raw
Quoted 60 lines · Wolfgang Grandegger
Quoted 45 lines · Grant Likely

On Wed, Apr 1, 2009 at 1:36 AM, Wolfgang Grandegger <[email protected]

Quoted 44 lines

wrote:

Quoted 42 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 5 lines
Quoted 5 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Quoted 8 lines

Grrr, I just realized that removing the devices type "soc" has broken fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common function for the MPC52xx as well, but it's not obvious to me how to find the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr"; (immr == Internally Memory Mapped Registers)

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be just fine to replace device_type = "soc".

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Yes, I know this. I'm not asking you to fix all the other boards, but make sure that it is not required for the new board.

Hm, I'm confused, if we want to fix this issue we need first to

- fix all functions in fsl_soc.c searching for the compatible string "fsl,soc" instead of the device type "soc" (or both for backward compatibility).

Quoted 8 lines

- fix U-Boot to find the SOC node by looking for "fsl,soc" to insert the
proper bus-frequency, at least.

That affects *all* boards using CONFIG_FSL_SOC and requires an up-to-date version of U-Boot for new kernels :-(. If that is fixed, I can remove the "device_type = "soc";" from socrates.dts (and may more), but not right now. Or have I missed something?

I presume the intent is not to break old u-boots w/new kernels, but to make it so new .dts don't require device_type = soc in them if using new kernels.

- k

#18 lore raw

On Wed, Apr 1, 2009 at 7:27 AM, Kumar Gala <[email protected]> wrot= e:

Quoted 18 lines · Wolfgang Grandegger
Quoted 16 lines
Quoted 14 lines · Grant Likely
Quoted 11 lines · Wolfgang Grandegger
Quoted 9 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

+ =A0 =A0 =A0 soc8544@e0000000 {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_type =3D "soc";

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broken
fsl_get_sys_freq(). See:

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L=

80

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

We need a quick fix and we could take the occasion to establish a common
function for the MPC52xx as well, but it's not obvious to me how =

to

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines

find
the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible =3D "fsl,mpc8544-immr"; =A0(immr =3D=3D Internally Memo=

ry Mapped

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

Registers)

Many other boards already do this.

Yes, it does, but searching for the SOC node is not straight-forwar=

d

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

because there is no common compatibility string but many CPU-specif=

ic

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what =

it

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line

means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc=

"

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

compatible in favour of something more elegant, but "fsl,soc" should =

be

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

just fine to replace device_type =3D "soc".

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Yes, I know this. =A0I'm not asking you to fix all the other boards, bu=

t

Quoted 8 lines
Quoted 8 lines
Quoted 1 line

make sure that it is not required for the new board.

Hm, I'm confused, if we want to fix this issue we need first to

- fix all functions in fsl_soc.c searching for the compatible string =A0"fsl,soc" instead of the device type "soc" (or both for backward =A0compatibility).

Quoted 6 lines
Quoted 6 lines

- fix U-Boot to find the SOC node by looking for "fsl,soc" to insert the =A0proper bus-frequency, at least.

That affects *all* boards using CONFIG_FSL_SOC and requires an up-to-date version of U-Boot for new kernels :-(. If that is fixed, I can remove the "device_type =3D "soc";" from socrates.dts (and may more)=

,

Quoted 3 lines
Quoted 1 line

but not right now. Or have I missed something?

I presume the intent is not to break old u-boots w/new kernels, but to ma=

ke

Quoted 1 line

it so new .dts don't require device_type =3D soc in them if using new ker=

nels.

Exactly.

g.

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

author
#19 lore raw
In reply to Kumar Gala

Hi Kumar,

Quoted 75 lines · Kumar Gala
Quoted 72 lines · Wolfgang Grandegger
Quoted 54 lines · Grant Likely
Quoted 52 lines · Wolfgang Grandegger
Quoted 51 lines · Anton Vorontsov

On Tue, Mar 31, 2009 at 09:05:28AM -0600, Grant Likely wrote:
[...]

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 9 lines
Quoted 5 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

Grrr, I just realized that removing the devices type "soc" has broken
fsl_get_sys_freq(). See:

Quoted 8 lines

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

We need a quick fix and we could take the occasion to establish a common
function for the MPC52xx as well, but it's not obvious to me how to find
the SOC node without the device type property.

SoC node should have a compatible property, just like everything else.

compatible = "fsl,mpc8544-immr"; (immr == Internally Memory Mapped Registers)

Many other boards already do this.

Quoted 8 lines
Quoted 5 lines

Yes, it does, but searching for the SOC node is not straight-forward because there is no common compatibility string but many CPU-specific
compatibility strings, e.g. "fsl,mpc8560-immr", etc. Have I missed something?

Choose a new value ("fsl,mpc-immr" perhaps?), document exactly what it
means, and add add it to the end of the compatible list.

As Scott Wood once pointed out, IMMR does not exists for MPC85xx parts. There it's called CCSR.

See this thread:

http://www.mail-archive.com/[email protected]/msg12665.html

I still think that
"fsl,mpc83NN-immr", "fsl,soc", "simple-bus" for 83xx and
"fsl,mpc85NN-ccsr", "fsl,soc", "simple-bus" for 85xx

would be OK, at least to start with. We can always deprecate "fsl,soc" compatible in favour of something more elegant, but "fsl,soc" should be
just fine to replace device_type = "soc".

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Also, there is another good thing about "fsl,soc" -- U-Boot already finds it for 83xx CPUs. ;-)

Ugh! I just realize the full impact of removing device type "soc". It will break compatibility with U-Boot for many boards. Is it worth it?

Yes, I know this. I'm not asking you to fix all the other boards, but make sure that it is not required for the new board.

Hm, I'm confused, if we want to fix this issue we need first to

- fix all functions in fsl_soc.c searching for the compatible string "fsl,soc" instead of the device type "soc" (or both for backward compatibility).

- fix U-Boot to find the SOC node by looking for "fsl,soc" to insert the proper bus-frequency, at least.

Quoted 8 lines
Quoted 4 lines

That affects *all* boards using CONFIG_FSL_SOC and requires an up-to-date version of U-Boot for new kernels :-(. If that is fixed, I can remove the "device_type = "soc";" from socrates.dts (and may more), but not right now. Or have I missed something?

I presume the intent is not to break old u-boots w/new kernels, but to make it so new .dts don't require device_type = soc in them if using new kernels.

Socrates is a new board and it has no problem using an up-to-date version of U-Boot. The socrates.dts file in your "next" tree already has the "device_type = "soc" removed. We need to add "fsl,soc" to the "compatible" property and update fsl_get_sys_freq(), etc. to search for it as well. And U-Boot should use the same name to fixup the frequencies. Are you already working on that issue? Should I provide patches?

Wolfgang.

#20 lore raw
Quoted 9 lines · Wolfgang Grandegger

Socrates is a new board and it has no problem using an up-to-date version of U-Boot. The socrates.dts file in your "next" tree already has the "device_type = "soc" removed. We need to add "fsl,soc" to the "compatible" property and update fsl_get_sys_freq(), etc. to search for it as well. And U-Boot should use the same name to fixup the frequencies. Are you already working on that issue? Should I provide patches?

I'm not working on it at all. I'm also guessing at this point the Socrates support is in Linus tree :)

- k

author
#21 lore raw
Quoted 13 lines · Kumar Gala
Quoted 9 lines · Wolfgang Grandegger

Socrates is a new board and it has no problem using an up-to-date version of U-Boot. The socrates.dts file in your "next" tree already has the "device_type = "soc" removed. We need to add "fsl,soc" to the "compatible" property and update fsl_get_sys_freq(), etc. to search for it as well. And U-Boot should use the same name to fixup the frequencies. Are you already working on that issue? Should I provide patches?

Quoted 2 lines

I'm not working on it at all. I'm also guessing at this point the Socrates support is in Linus tree :)

Yes, I know, but it does not work properly because the "device_type = "soc" is missing :-(. I did not realize that immediately, sorry. Could you please apply the fix below re-adding the 'device_type = "soc";' line for the time being.

Thanks.

Wolfgang.

[PATCH] powerpc/85xx: Re-add the device_type soc to socrates.dts

The device_type "soc" is still required for MPC85xx boards.

Signed-off-by Wolfgang Grandegger <[email protected]>
---
 arch/powerpc/boot/dts/socrates.dts |    1 +
 1 file changed, 1 insertion(+)

Index: powerpc/arch/powerpc/boot/dts/socrates.dts
===================================================================
--- powerpc.orig/arch/powerpc/boot/dts/socrates.dts 2009-04-01
15:26:09.000000000 +0200
+++ powerpc/arch/powerpc/boot/dts/socrates.dts 2009-04-02
16:37:09.838719521 +0200
@@ -52,6 +52,7 @@
soc8544@e0000000 {
#address-cells = <1>;
#size-cells = <1>;
+ device_type = "soc";

ranges = <0x00000000 0xe0000000 0x00100000>;
reg = <0xe0000000 0x00001000>; // CCSRBAR 1M

Applied
#22 lore raw
Quoted 8 lines · Wolfgang Grandegger

[PATCH] powerpc/85xx: Re-add the device_type soc to socrates.dts

The device_type "soc" is still required for MPC85xx boards.

Signed-off-by Wolfgang Grandegger <[email protected]>

---
arch/powerpc/boot/dts/socrates.dts | 1 +

1 file changed, 1 insertion(+)

applied

- k