[PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Valentine Barshak]

AMCC Sequoia board DTS

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
---
 arch/powerpc/boot/dts/sequoia.dts |  292 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 292 insertions(+)

diff -ruN linux.orig/arch/powerpc/boot/dts/sequoia.dts linux/arch/powerpc/boot/dts/sequoia.dts
--- linux.orig/arch/powerpc/boot/dts/sequoia.dts	1970-01-01 03:00:00.000000000 +0300
+++ linux/arch/powerpc/boot/dts/sequoia.dts	2007-07-27 20:44:26.000000000 +0400
@@ -0,0 +1,292 @@
+/*
+ * Device Tree Source for AMCC Sequoia
+ *
+ * Based on Bamboo code by Josh Boyer <jwboyer@linux.vnet.ibm.com>
+ * Copyright (c) 2006, 2007 IBM Corp.
+ *
+ * FIXME: Draft only!
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without
+ * any warranty of any kind, whether express or implied.
+ *
+ * To build:
+ *   dtc -I dts -O asm -o bamboo.S -b 0 sequoia.dts
+ *   dtc -I dts -O dtb -o bamboo.dtb -b 0 sequoia.dts
+ */
+
+/ {
+	#address-cells = <2>;
+	#size-cells = <1>;
+	model = "amcc,sequoia";
+	compatible = "amcc,sequoia";
+	dcr-parent = <&/cpus/PowerPC,440EPx@0>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		PowerPC,440EPx@0 {
+			device_type = "cpu";
+			reg = <0>;
+			clock-frequency = <0>; /* Filled in by zImage */
+			timebase-frequency = <0>; /* Filled in by zImage */
+			i-cache-line-size = <20>;
+			d-cache-line-size = <20>;
+			i-cache-size = <8000>;
+			d-cache-size = <8000>;
+			dcr-controller;
+			dcr-access-method = "native";
+		};
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0 0 0>; /* Filled in by zImage */
+	};
+
+	UIC0: interrupt-controller0 {
+		compatible = "ibm,uic-440gp","ibm,uic";
+		interrupt-controller;
+		cell-index = <0>;
+		dcr-reg = <0c0 009>;
+		#address-cells = <0>;
+		#size-cells = <0>;
+		#interrupt-cells = <2>;
+	};
+
+	UIC1: interrupt-controller1 {
+		compatible = "ibm,uic-440gp","ibm,uic";
+		interrupt-controller;
+		cell-index = <1>;
+		dcr-reg = <0d0 009>;
+		#address-cells = <0>;
+		#size-cells = <0>;
+		#interrupt-cells = <2>;
+		interrupts = <1e 4 1f 4>; /* cascade */
+		interrupt-parent = <&UIC0>;
+	};
+
+	UIC2: interrupt-controller2 {
+		compatible = "ibm,uic-440gp","ibm,uic";
+		interrupt-controller;
+		cell-index = <2>;
+		dcr-reg = <0e0 009>;
+		#address-cells = <0>;
+		#size-cells = <0>;
+		#interrupt-cells = <2>;
+		interrupts = <1c 4 1d 4>; /* cascade */
+		interrupt-parent = <&UIC0>;
+	};
+
+	SDR0: sdr {
+		compatible = "ibm,sdr-440ep";
+		dcr-reg = <00e 002>;
+	};
+
+	CPR0: cpr {
+		compatible = "ibm,cpr-440ep";
+		dcr-reg = <00c 002>;
+	};
+
+	plb {
+		compatible = "ibm,plb-440gp", "ibm,plb4";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges;
+		clock-frequency = <0>; /* Filled in by zImage */
+
+		SDRAM0: sdram {
+			device_type = "memory-controller";
+			compatible = "ibm,sdram-44x-ddr2denali";
+			dcr-reg = <010 2>;
+		};
+
+		DMA0: dma {
+			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
+			dcr-reg = <100 027>;
+		};
+
+		MAL0: mcmal {
+			compatible = "ibm,mcmal-440spe", "ibm,mcmal2";
+			dcr-reg = <180 62>;
+			num-tx-chans = <4>;
+			num-rx-chans = <4>;
+			interrupt-parent = <&MAL0>;
+			interrupts = <0 1 2 3 4>;
+			#interrupt-cells = <1>;
+			#address-cells = <0>;
+			#size-cells = <0>;
+			interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
+					/*RXEOB*/ 1 &UIC0 b 4
+					/*SERR*/  2 &UIC1 0 4
+					/*TXDE*/  3 &UIC1 1 4
+					/*RXDE*/  4 &UIC1 2 4>;
+			interrupt-map-mask = <ffffffff>;
+		};
+
+		POB0: opb {
+		  	compatible = "ibm,opb-440gp", "ibm,opb";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			/* Bamboo is oddball in the 44x world and doesn't use the ERPN
+			 * bits.
+			 */
+		  	ranges = <00000000 1 00000000 80000000
+			          80000000 1 80000000 80000000>;
+		  	interrupt-parent = <&UIC1>;
+		  	interrupts = <7 4>;
+		  	clock-frequency = <0>; /* Filled in by zImage */
+
+			EBC0: ebc {
+				compatible = "ibm,ebc-440gp";
+				dcr-reg = <012 2>;
+				#address-cells = <2>;
+				#size-cells = <1>;
+				clock-frequency = <0>; /* Filled in by zImage */
+				interrupts = <5 1>;
+				interrupt-parent = <&UIC1>;
+
+				nor_flash@0,0 {
+					device_type = "rom";
+					compatible = "direct-mapped";
+					probe-type = "CFI";
+					bank-width = <2>;
+					partitions = <	0	180000
+							180000	200000
+							380000	3aa0000
+							3e20000	140000
+							3f60000	40000
+							3fa0000	60000>;
+					partition-names = "Kernel", "ramdisk", "file system",
+								"kozio", "env", "u-boot";
+					reg = <0 000000 4000000>;
+				};
+
+			};
+
+			UART0: serial@ef600300 {
+		   		device_type = "serial";
+		   		compatible = "ns16550";
+		   		reg = <ef600300 8>;
+		   		virtual-reg = <ef600300>;
+		   		clock-frequency = <0>; /* Filled in by zImage */
+		   		current-speed = <1c200>;
+		   		interrupt-parent = <&UIC0>;
+		   		interrupts = <0 4>;
+	   		};
+
+			UART1: serial@ef600400 {
+		   		device_type = "serial";
+		   		compatible = "ns16550";
+		   		reg = <ef600400 8>;
+		   		virtual-reg = <ef600400>;
+		   		clock-frequency = <0>;
+		   		current-speed = <0>;
+		   		interrupt-parent = <&UIC0>;
+		   		interrupts = <1 4>;
+	   		};
+
+			UART2: serial@ef600500 {
+		   		device_type = "serial";
+		   		compatible = "ns16550";
+		   		reg = <ef600500 8>;
+		   		virtual-reg = <ef600500>;
+		   		clock-frequency = <0>;
+		   		current-speed = <0>;
+		   		interrupt-parent = <&UIC1>;
+		   		interrupts = <3 4>;
+	   		};
+
+			UART3: serial@ef600600 {
+		   		device_type = "serial";
+		   		compatible = "ns16550";
+		   		reg = <ef600600 8>;
+		   		virtual-reg = <ef600600>;
+		   		clock-frequency = <0>;
+		   		current-speed = <0>;
+		   		interrupt-parent = <&UIC1>;
+		   		interrupts = <4 4>;
+	   		};
+
+			IIC0: i2c@ef600700 {
+				device_type = "i2c";
+				compatible = "ibm,iic-440gp", "ibm,iic";
+				reg = <ef600700 14>;
+				interrupt-parent = <&UIC0>;
+				interrupts = <2 4>;
+			};
+
+			IIC1: i2c@ef600800 {
+				device_type = "i2c";
+				compatible = "ibm,iic-44gp", "ibm,iic";
+				reg = <ef600800 14>;
+				interrupt-parent = <&UIC0>;
+				interrupts = <7 4>;
+			};
+
+			ZMII0: emac-zmii@ef600d00 {
+				device_type = "zmii-interface";
+				compatible = "ibm,zmii-440gp", "ibm,zmii";
+				reg = <ef600d00 c>;
+			};
+
+			EMAC0: ethernet@ef600e00 {
+				linux,network-index = <0>;
+				device_type = "network";
+				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
+				interrupt-parent = <&EMAC0>;
+				interrupts = <0 1>;
+				#interrupt-cells = <1>;
+				#address-cells = <0>;
+				#size-cells = <0>;
+				interrupt-map = </*Status*/ 0 &UIC0 18 4
+						/*Wake*/  1 &UIC1 1d 4>;
+				reg = <ef600e00 70>;
+				local-mac-address = [000000000000];
+				mal-device = <&MAL0>;
+				mal-tx-channel = <0 1>;
+				mal-rx-channel = <0>;
+				cell-index = <0>;
+				max-frame-size = <5dc>;
+				rx-fifo-size = <1000>;
+				tx-fifo-size = <800>;
+				phy-mode = "rmii";
+				phy-map = <00000000>;
+				zmii-device = <&ZMII0>;
+				zmii-channel = <0>;
+			};
+
+			EMAC1: ethernet@ef600f00 {
+				linux,network-index = <1>;
+				device_type = "network";
+				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
+				interrupt-parent = <&EMAC1>;
+				interrupts = <0 1>;
+				#interrupt-cells = <1>;
+				#address-cells = <0>;
+				#size-cells = <0>;
+				interrupt-map = </*Status*/ 0 &UIC0 19 4
+						/*Wake*/  1 &UIC1 1f 4>;
+				reg = <ef600f00 70>;
+				local-mac-address = [000000000000];
+				mal-device = <&MAL0>;
+				mal-tx-channel = <2 3>;
+				mal-rx-channel = <1>;
+				cell-index = <1>;
+				max-frame-size = <5dc>;
+				rx-fifo-size = <1000>;
+				tx-fifo-size = <800>;
+				phy-mode = "rmii";
+				phy-map = <00000000>;
+				zmii-device = <&ZMII0>;
+				zmii-channel = <1>;
+			};
+		};
+	};
+
+	chosen {
+		linux,stdout-path = "/plb/opb/serial@ef600300";
+		bootargs = "console=ttyS0,115200";
+	};
+};

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Jul 30, 2007 at 07:06:48PM +0400, Valentine Barshak wrote:
> AMCC Sequoia board DTS
> 
> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
> ---
>  arch/powerpc/boot/dts/sequoia.dts |  292 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 292 insertions(+)
> 
> diff -ruN linux.orig/arch/powerpc/boot/dts/sequoia.dts linux/arch/powerpc/boot/dts/sequoia.dts
> --- linux.orig/arch/powerpc/boot/dts/sequoia.dts	1970-01-01 03:00:00.000000000 +0300
> +++ linux/arch/powerpc/boot/dts/sequoia.dts	2007-07-27 20:44:26.000000000 +0400
> @@ -0,0 +1,292 @@
> +/*
> + * Device Tree Source for AMCC Sequoia
> + *
> + * Based on Bamboo code by Josh Boyer <jwboyer@linux.vnet.ibm.com>
> + * Copyright (c) 2006, 2007 IBM Corp.
> + *
> + * FIXME: Draft only!
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without
> + * any warranty of any kind, whether express or implied.
> + *
> + * To build:
> + *   dtc -I dts -O asm -o bamboo.S -b 0 sequoia.dts
> + *   dtc -I dts -O dtb -o bamboo.dtb -b 0 sequoia.dts

Needs updating to remove the bamboo references.  In fact we can
probably get rid of this "To build" comment that's been copied to just
about every dts ever.

> + */
> +
> +/ {
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	model = "amcc,sequoia";
> +	compatible = "amcc,sequoia";
> +	dcr-parent = <&/cpus/PowerPC,440EPx@0>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		PowerPC,440EPx@0 {
> +			device_type = "cpu";
> +			reg = <0>;
> +			clock-frequency = <0>; /* Filled in by zImage */
> +			timebase-frequency = <0>; /* Filled in by zImage */
> +			i-cache-line-size = <20>;
> +			d-cache-line-size = <20>;
> +			i-cache-size = <8000>;
> +			d-cache-size = <8000>;
> +			dcr-controller;
> +			dcr-access-method = "native";
> +		};
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0 0 0>; /* Filled in by zImage */
> +	};
> +
> +	UIC0: interrupt-controller0 {
> +		compatible = "ibm,uic-440gp","ibm,uic";

The first compatible entry should always be the precise model, so in
this case "ibm,uic-440epx".  If it is (supposed to be) identical to
the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
since I believe all the UICs are supposed to operate the same, I think
that's implicit in the "ibm,uic" entry.

This goes for all the entries below where you list "ibm,....-440gp" or
or "ibm,....-440ep" or "ibm,.....-440spe" first instead of
"ibm,....-440epx".

> +		interrupt-controller;
> +		cell-index = <0>;
> +		dcr-reg = <0c0 009>;
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	UIC1: interrupt-controller1 {
> +		compatible = "ibm,uic-440gp","ibm,uic";
> +		interrupt-controller;
> +		cell-index = <1>;
> +		dcr-reg = <0d0 009>;
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <2>;
> +		interrupts = <1e 4 1f 4>; /* cascade */
> +		interrupt-parent = <&UIC0>;
> +	};
> +
> +	UIC2: interrupt-controller2 {
> +		compatible = "ibm,uic-440gp","ibm,uic";
> +		interrupt-controller;
> +		cell-index = <2>;
> +		dcr-reg = <0e0 009>;
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <2>;
> +		interrupts = <1c 4 1d 4>; /* cascade */
> +		interrupt-parent = <&UIC0>;
> +	};
> +
> +	SDR0: sdr {

What is the SDR?

> +		compatible = "ibm,sdr-440ep";
> +		dcr-reg = <00e 002>;
> +	};
> +
> +	CPR0: cpr {

And the CPR?

> +		compatible = "ibm,cpr-440ep";
> +		dcr-reg = <00c 002>;
> +	};
> +
> +	plb {
> +		compatible = "ibm,plb-440gp", "ibm,plb4";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges;
> +		clock-frequency = <0>; /* Filled in by zImage */
> +
> +		SDRAM0: sdram {
> +			device_type = "memory-controller";
> +			compatible = "ibm,sdram-44x-ddr2denali";

Should have a precise -440epx compatible, as well as the more general one.

> +			dcr-reg = <010 2>;
> +		};
> +
> +		DMA0: dma {
> +			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
> +			dcr-reg = <100 027>;
> +		};
> +
> +		MAL0: mcmal {
> +			compatible = "ibm,mcmal-440spe", "ibm,mcmal2";
> +			dcr-reg = <180 62>;
> +			num-tx-chans = <4>;
> +			num-rx-chans = <4>;
> +			interrupt-parent = <&MAL0>;
> +			interrupts = <0 1 2 3 4>;
> +			#interrupt-cells = <1>;
> +			#address-cells = <0>;
> +			#size-cells = <0>;
> +			interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
> +					/*RXEOB*/ 1 &UIC0 b 4
> +					/*SERR*/  2 &UIC1 0 4
> +					/*TXDE*/  3 &UIC1 1 4
> +					/*RXDE*/  4 &UIC1 2 4>;
> +			interrupt-map-mask = <ffffffff>;
> +		};
> +
> +		POB0: opb {
> +		  	compatible = "ibm,opb-440gp", "ibm,opb";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			/* Bamboo is oddball in the 44x world and doesn't use the ERPN
> +			 * bits.
> +			 */

Comment is for Bamboo and does not match the ranges property below.

> +		  	ranges = <00000000 1 00000000 80000000
> +			          80000000 1 80000000 80000000>;
> +		  	interrupt-parent = <&UIC1>;
> +		  	interrupts = <7 4>;
> +		  	clock-frequency = <0>; /* Filled in by zImage */
> +
> +			EBC0: ebc {
> +				compatible = "ibm,ebc-440gp";
> +				dcr-reg = <012 2>;
> +				#address-cells = <2>;
> +				#size-cells = <1>;
> +				clock-frequency = <0>; /* Filled in by zImage */
> +				interrupts = <5 1>;
> +				interrupt-parent = <&UIC1>;
> +
> +				nor_flash@0,0 {
> +					device_type = "rom";
> +					compatible = "direct-mapped";
> +					probe-type = "CFI";

This flash binding needs to be replaced, but I guess that's not really
your problem.

> +					bank-width = <2>;
> +					partitions = <	0	180000
> +							180000	200000
> +							380000	3aa0000
> +							3e20000	140000
> +							3f60000	40000
> +							3fa0000	60000>;
> +					partition-names = "Kernel", "ramdisk", "file system",
> +								"kozio", "env", "u-boot";
> +					reg = <0 000000 4000000>;
> +				};
> +
> +			};
> +
> +			UART0: serial@ef600300 {
> +		   		device_type = "serial";
> +		   		compatible = "ns16550";
> +		   		reg = <ef600300 8>;
> +		   		virtual-reg = <ef600300>;
> +		   		clock-frequency = <0>; /* Filled in by zImage */
> +		   		current-speed = <1c200>;
> +		   		interrupt-parent = <&UIC0>;
> +		   		interrupts = <0 4>;
> +	   		};
> +
> +			UART1: serial@ef600400 {
> +		   		device_type = "serial";
> +		   		compatible = "ns16550";
> +		   		reg = <ef600400 8>;
> +		   		virtual-reg = <ef600400>;
> +		   		clock-frequency = <0>;
> +		   		current-speed = <0>;
> +		   		interrupt-parent = <&UIC0>;
> +		   		interrupts = <1 4>;
> +	   		};
> +
> +			UART2: serial@ef600500 {
> +		   		device_type = "serial";
> +		   		compatible = "ns16550";
> +		   		reg = <ef600500 8>;
> +		   		virtual-reg = <ef600500>;
> +		   		clock-frequency = <0>;
> +		   		current-speed = <0>;
> +		   		interrupt-parent = <&UIC1>;
> +		   		interrupts = <3 4>;
> +	   		};
> +
> +			UART3: serial@ef600600 {
> +		   		device_type = "serial";
> +		   		compatible = "ns16550";
> +		   		reg = <ef600600 8>;
> +		   		virtual-reg = <ef600600>;
> +		   		clock-frequency = <0>;
> +		   		current-speed = <0>;
> +		   		interrupt-parent = <&UIC1>;
> +		   		interrupts = <4 4>;
> +	   		};
> +
> +			IIC0: i2c@ef600700 {
> +				device_type = "i2c";
> +				compatible = "ibm,iic-440gp", "ibm,iic";
> +				reg = <ef600700 14>;
> +				interrupt-parent = <&UIC0>;
> +				interrupts = <2 4>;
> +			};
> +
> +			IIC1: i2c@ef600800 {
> +				device_type = "i2c";
> +				compatible = "ibm,iic-44gp", "ibm,iic";
> +				reg = <ef600800 14>;
> +				interrupt-parent = <&UIC0>;
> +				interrupts = <7 4>;
> +			};
> +
> +			ZMII0: emac-zmii@ef600d00 {
> +				device_type = "zmii-interface";
> +				compatible = "ibm,zmii-440gp", "ibm,zmii";
> +				reg = <ef600d00 c>;
> +			};
> +
> +			EMAC0: ethernet@ef600e00 {
> +				linux,network-index = <0>;
> +				device_type = "network";
> +				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";

"ibm,emac-axon" is definitely wrong, since this isn't an Axon chip.

> +				interrupt-parent = <&EMAC0>;
> +				interrupts = <0 1>;
> +				#interrupt-cells = <1>;
> +				#address-cells = <0>;
> +				#size-cells = <0>;
> +				interrupt-map = </*Status*/ 0 &UIC0 18 4
> +						/*Wake*/  1 &UIC1 1d 4>;
> +				reg = <ef600e00 70>;
> +				local-mac-address = [000000000000];
> +				mal-device = <&MAL0>;
> +				mal-tx-channel = <0 1>;
> +				mal-rx-channel = <0>;
> +				cell-index = <0>;
> +				max-frame-size = <5dc>;
> +				rx-fifo-size = <1000>;
> +				tx-fifo-size = <800>;
> +				phy-mode = "rmii";
> +				phy-map = <00000000>;
> +				zmii-device = <&ZMII0>;
> +				zmii-channel = <0>;
> +			};
> +
> +			EMAC1: ethernet@ef600f00 {
> +				linux,network-index = <1>;
> +				device_type = "network";
> +				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
> +				interrupt-parent = <&EMAC1>;
> +				interrupts = <0 1>;
> +				#interrupt-cells = <1>;
> +				#address-cells = <0>;
> +				#size-cells = <0>;
> +				interrupt-map = </*Status*/ 0 &UIC0 19 4
> +						/*Wake*/  1 &UIC1 1f 4>;
> +				reg = <ef600f00 70>;
> +				local-mac-address = [000000000000];
> +				mal-device = <&MAL0>;
> +				mal-tx-channel = <2 3>;
> +				mal-rx-channel = <1>;
> +				cell-index = <1>;
> +				max-frame-size = <5dc>;
> +				rx-fifo-size = <1000>;
> +				tx-fifo-size = <800>;
> +				phy-mode = "rmii";
> +				phy-map = <00000000>;
> +				zmii-device = <&ZMII0>;
> +				zmii-channel = <1>;
> +			};
> +		};
> +	};
> +
> +	chosen {
> +		linux,stdout-path = "/plb/opb/serial@ef600300";
> +		bootargs = "console=ttyS0,115200";
> +	};
> +};
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> +	UIC0: interrupt-controller0 {
>> +		compatible = "ibm,uic-440gp","ibm,uic";
>
> The first compatible entry should always be the precise model, so in
> this case "ibm,uic-440epx".

This isn't really _required_, but it is a very good idea in
almost all cases (the exception is for very generic or legacy
devices).

> If it is (supposed to be) identical to
> the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> since I believe all the UICs are supposed to operate the same, I think
> that's implicit in the "ibm,uic" entry.

Sure, but there is no harm in having the better qualified 440gp
name in there as well -- bytes are cheap :-)

>> +	SDR0: sdr {
>
> What is the SDR?
>
>> +		compatible = "ibm,sdr-440ep";
>> +		dcr-reg = <00e 002>;
>> +	};
>> +
>> +	CPR0: cpr {
>
> And the CPR?

Yeah, better names please -- if possible, something that someone
without knowledge of this SoC will understand what it is.

>> +				nor_flash@0,0 {
>> +					device_type = "rom";
>> +					compatible = "direct-mapped";
>> +					probe-type = "CFI";
>
> This flash binding needs to be replaced, but I guess that's not really
> your problem.

Yeah, that's my problem, thanks for the prod :-)


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Wed, Aug 01, 2007 at 06:57:33AM +0200, Segher Boessenkool wrote:
> >> +	UIC0: interrupt-controller0 {
> >> +		compatible = "ibm,uic-440gp","ibm,uic";
> >
> > The first compatible entry should always be the precise model, so in
> > this case "ibm,uic-440epx".
> 
> This isn't really _required_, but it is a very good idea in
> almost all cases (the exception is for very generic or legacy
> devices).

Well, yes.  That's a "should" not a "must" in rfc-speak.

> > If it is (supposed to be) identical to
> > the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> > since I believe all the UICs are supposed to operate the same, I think
> > that's implicit in the "ibm,uic" entry.
> 
> Sure, but there is no harm in having the better qualified 440gp
> name in there as well -- bytes are cheap :-)
> 
> >> +	SDR0: sdr {
> >
> > What is the SDR?
> >
> >> +		compatible = "ibm,sdr-440ep";
> >> +		dcr-reg = <00e 002>;
> >> +	};
> >> +
> >> +	CPR0: cpr {
> >
> > And the CPR?
> 
> Yeah, better names please -- if possible, something that someone
> without knowledge of this SoC will understand what it is.

I think the names are probably ok - I'm assuming they're in keeping
with the convention I've used of using the same names / abbreviations
as in the CPU user manual.  I'm asking just for my own information,
although a comment might not be a bad idea.

> >> +				nor_flash@0,0 {
> >> +					device_type = "rom";
> >> +					compatible = "direct-mapped";
> >> +					probe-type = "CFI";
> >
> > This flash binding needs to be replaced, but I guess that's not really
> > your problem.
> 
> Yeah, that's my problem, thanks for the prod :-)

Also mine.  I've been home sick the last couple of days, but by way of
a sharper prod, see my draft work below.  It patches both
booting-without-of.txt with a revised binding, and implements it in
the physmap_of driver (which needs renaming, but that's another
story).  It also revises the ebony device tree as an example.

This is certainly not complete - it defines none of the extra
properties that JEDEC chips need (although the mtd drivers'
defaults/probing seem to cope for ebony).  And there are various other
ommisions.  Still, it's a starting point - something precise for you
to flame Segher :-p.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Wed, Aug 01, 2007 at 03:04:22PM +1000, David Gibson wrote:
> On Wed, Aug 01, 2007 at 06:57:33AM +0200, Segher Boessenkool wrote:
> > >> +	UIC0: interrupt-controller0 {
> > >> +		compatible = "ibm,uic-440gp","ibm,uic";
> > >
> > > The first compatible entry should always be the precise model, so in
> > > this case "ibm,uic-440epx".
> > 
> > This isn't really _required_, but it is a very good idea in
> > almost all cases (the exception is for very generic or legacy
> > devices).
> 
> Well, yes.  That's a "should" not a "must" in rfc-speak.
> 
> > > If it is (supposed to be) identical to
> > > the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> > > since I believe all the UICs are supposed to operate the same, I think
> > > that's implicit in the "ibm,uic" entry.
> > 
> > Sure, but there is no harm in having the better qualified 440gp
> > name in there as well -- bytes are cheap :-)
> > 
> > >> +	SDR0: sdr {
> > >
> > > What is the SDR?
> > >
> > >> +		compatible = "ibm,sdr-440ep";
> > >> +		dcr-reg = <00e 002>;
> > >> +	};
> > >> +
> > >> +	CPR0: cpr {
> > >
> > > And the CPR?
> > 
> > Yeah, better names please -- if possible, something that someone
> > without knowledge of this SoC will understand what it is.
> 
> I think the names are probably ok - I'm assuming they're in keeping
> with the convention I've used of using the same names / abbreviations
> as in the CPU user manual.  I'm asking just for my own information,
> although a comment might not be a bad idea.
> 
> > >> +				nor_flash@0,0 {
> > >> +					device_type = "rom";
> > >> +					compatible = "direct-mapped";
> > >> +					probe-type = "CFI";
> > >
> > > This flash binding needs to be replaced, but I guess that's not really
> > > your problem.
> > 
> > Yeah, that's my problem, thanks for the prod :-)
> 
> Also mine.  I've been home sick the last couple of days, but by way of
> a sharper prod, see my draft work below.  It patches both
> booting-without-of.txt with a revised binding, and implements it in
> the physmap_of driver (which needs renaming, but that's another
> story).  It also revises the ebony device tree as an example.
> 
> This is certainly not complete - it defines none of the extra
> properties that JEDEC chips need (although the mtd drivers'
> defaults/probing seem to cope for ebony).  And there are various other
> ommisions.  Still, it's a starting point - something precise for you
> to flame Segher :-p.

Duh, forgot to attach the actual patch.  Here it is:

Index: working-2.6/Documentation/powerpc/booting-without-of.txt
===================================================================
--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
@@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
 		};
 	};
 
-    j) Flash chip nodes
+   j) CFI or JEDEC memory-mapped NOR flash
 
     Flash chips (Memory Technology Devices) are often used for solid state
     file systems on embedded devices.
 
-    Required properties:
+     - compatible : should contain the specific model of flash chip(s) used
+       followed by either "cfi-flash" or "jedec-flash"
+     - reg : Address range of the flash chip
+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
+       times the number of interleaved chips.
+     - device-width : (optional) Width of a single flash chip.  If omitted,
+       assumed to be equal to 'bank-width'.
+
 
-     - device_type : has to be "rom"
-     - compatible : Should specify what this flash device is compatible with.
-       Currently, this is most likely to be "direct-mapped" (which
-       corresponds to the MTD physmap mapping driver).
-     - reg : Offset and length of the register set (or memory mapping) for
-       the device.
-     - bank-width : Width of the flash data bus in bytes. Required
-       for the NOR flashes (compatible == "direct-mapped" and others) ONLY.
-
-    Recommended properties :
-
-     - partitions : Several pairs of 32-bit values where the first value is
-       partition's offset from the start of the device and the second one is
-       partition size in bytes with LSB used to signify a read only
-       partition (so, the partition size should always be an even number).
-     - partition-names : The list of concatenated zero terminated strings
-       representing the partition names.
-     - probe-type : The type of probe which should be done for the chip
-       (JEDEC vs CFI actually). Valid ONLY for NOR flashes.
-
-   Example:
-
- 	flash@ff000000 {
- 		device_type = "rom";
- 		compatible = "direct-mapped";
- 		probe-type = "CFI";
- 		reg = <ff000000 01000000>;
- 		bank-width = <4>;
- 		partitions = <00000000 00f80000
- 			      00f80000 00080001>;
- 		partition-names = "fs\0firmware";
- 	};
+    Flash partitions
+     - reg :
+     - read-only : (optional)
 
    k) Global Utilities Block
 
Index: working-2.6/drivers/mtd/maps/physmap_of.c
===================================================================
--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
@@ -4,6 +4,9 @@
  * Copyright (C) 2006 MontaVista Software Inc.
  * Author: Vitaly Wool <vwool@ru.mvista.com>
  *
+ * Revised to handle newer style flash binding by:
+ *   Copyright (C) 2007 David Gibson, IBM Corporation.
+ *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
  * Free Software Foundation;  either version 2 of the  License, or (at your
@@ -30,17 +33,72 @@ struct physmap_flash_info {
 	struct map_info		map;
 	struct resource		*res;
 #ifdef CONFIG_MTD_PARTITIONS
-	int			nr_parts;
 	struct mtd_partition	*parts;
 #endif
 };
 
-static const char *rom_probe_types[] = { "cfi_probe", "jedec_probe", "map_rom", NULL };
 #ifdef CONFIG_MTD_PARTITIONS
-static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
-#endif
+static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
+						 struct of_device *dev)
+{
+	static const char *part_probe_types[]
+		= { "cmdlinepart", "RedBoot", NULL };
+	struct device_node *dp = dev->node, *pp;
+	int nr_parts, i, err;
+
+	/* First look for RedBoot table or partitions on the command
+	 * line, these take precedence over device tree information */
+	nr_parts = parse_mtd_partitions(info->mtd, part_probe_types,
+					&info->parts, 0);
+	if (nr_parts > 0) {
+		add_mtd_partitions(info->mtd, info->parts, err);
+		return 0;
+	}
+
+	/* First count the subnodes */
+	nr_parts = 0;
+	for (pp = dp->child; pp; pp = pp->sibling)
+		nr_parts++;
+
+	info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition), GFP_KERNEL);
+	if (!info->parts) {
+		printk(KERN_ERR "Can't allocate the flash partition data!\n");
+		return -ENOMEM;
+	}
+
+	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
+		const u32 *reg;
+		const char *name;
+		const void *ro;
+		int len;
+
+		reg = of_get_property(pp, "reg", &len);
+		if (! reg || (len != 2*sizeof(u32))) {
+			dev_err(&dev->dev, "Invalid 'reg' on %s\n",
+				dp->full_name);
+			err = -EINVAL;
+			goto fail;
+		}
+		info->parts[i].offset = reg[0];
+		info->parts[i].size = reg[1];
+
+		name = of_get_property(pp, "name", &len);
+		info->parts[i].name = name;
+
+		ro = of_get_property(pp, "read-only", &len);
+		if (ro)
+			info->parts[i].mask_flags = MTD_WRITEABLE;
+	}
+
+	add_mtd_partitions(info->mtd, info->parts, nr_parts);
+	return 0;
+
+ fail:
+	kfree(info->parts);
+	info->parts = NULL;
+	return err;
+}
 
-#ifdef CONFIG_MTD_PARTITIONS
 static int parse_flash_partitions(struct device_node *node,
 		struct mtd_partition **parts)
 {
@@ -79,7 +137,14 @@ static int parse_flash_partitions(struct
 err:
 	return retval;
 }
-#endif
+#else /* MTD_PARTITIONS */
+static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
+						 struct device_node *dev)
+{
+	add_mtd_device(info->mtd);
+	return 0;
+}
+#endif /* MTD_PARTITIONS */
 
 static int of_physmap_remove(struct of_device *dev)
 {
@@ -92,7 +157,7 @@ static int of_physmap_remove(struct of_d
 
 	if (info->mtd != NULL) {
 #ifdef CONFIG_MTD_PARTITIONS
-		if (info->nr_parts) {
+		if (info->parts) {
 			del_mtd_partitions(info->mtd);
 			kfree(info->parts);
 		} else {
@@ -115,13 +180,45 @@ static int of_physmap_remove(struct of_d
 	return 0;
 }
 
+/* Helper function to handle probing of the obsolete "direct-mapped"
+ * compatible binding, which has an extra "probe-type" property
+ * describing the type of flash probe necessary. */
+static struct mtd_info * __devinit obsolete_probe(struct of_device *dev,
+						  struct map_info *map)
+{
+	struct device_node *dp = dev->node;
+	const char *of_probe;
+	struct mtd_info *mtd;
+	static const char *rom_probe_types[]
+		= { "cfi_probe", "jedec_probe", "map_rom"};
+	int i;
+
+	of_probe = of_get_property(dp, "probe-type", NULL);
+	if (!of_probe) {
+		for (i = 0; i < ARRAY_SIZE(rom_probe_types); i++) {
+			mtd = do_map_probe(rom_probe_types[i], map);
+			if (mtd)
+				return mtd;
+		}
+		return NULL;
+	} else if (strcmp(of_probe, "CFI") == 0) {
+		return do_map_probe("cfi_probe", map);
+	} else if (strcmp(of_probe, "JEDEC") == 0) {
+		return do_map_probe("jedec_probe", map);
+	} else {
+ 		if (strcmp(of_probe, "ROM") != 0)
+			dev_dbg(&dev->dev, "obsolete_probe: don't know probe type "
+				"'%s', mapping as rom\n", of_probe);
+		return do_map_probe("mtd_rom", map);
+	}
+}
+
 static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
 {
 	struct device_node *dp = dev->node;
 	struct resource res;
 	struct physmap_flash_info *info;
-	const char **probe_type;
-	const char *of_probe;
+	const char *probe_type = (const char *)match->data;
 	const u32 *width;
 	int err;
 
@@ -174,21 +271,11 @@ static int __devinit of_physmap_probe(st
 
 	simple_map_init(&info->map);
 
-	of_probe = of_get_property(dp, "probe-type", NULL);
-	if (of_probe == NULL) {
-		probe_type = rom_probe_types;
-		for (; info->mtd == NULL && *probe_type != NULL; probe_type++)
-			info->mtd = do_map_probe(*probe_type, &info->map);
-	} else if (!strcmp(of_probe, "CFI"))
-		info->mtd = do_map_probe("cfi_probe", &info->map);
-	else if (!strcmp(of_probe, "JEDEC"))
-		info->mtd = do_map_probe("jedec_probe", &info->map);
-	else {
- 		if (strcmp(of_probe, "ROM"))
-			dev_dbg(&dev->dev, "map_probe: don't know probe type "
-			"'%s', mapping as rom\n", of_probe);
-		info->mtd = do_map_probe("mtd_rom", &info->map);
-	}
+	if (probe_type)
+		info->mtd = do_map_probe(probe_type, &info->map);
+	else
+		info->mtd = obsolete_probe(dev, &info->map);
+
 	if (info->mtd == NULL) {
 		dev_err(&dev->dev, "map_probe failed\n");
 		err = -ENXIO;
@@ -196,18 +283,7 @@ static int __devinit of_physmap_probe(st
 	}
 	info->mtd->owner = THIS_MODULE;
 
-#ifdef CONFIG_MTD_PARTITIONS
-	err = parse_mtd_partitions(info->mtd, part_probe_types, &info->parts, 0);
-	if (err > 0) {
-		add_mtd_partitions(info->mtd, info->parts, err);
-	} else if ((err = parse_flash_partitions(dp, &info->parts)) > 0) {
-		dev_info(&dev->dev, "Using OF partition information\n");
-		add_mtd_partitions(info->mtd, info->parts, err);
-		info->nr_parts = err;
-	} else
-#endif
-
-	add_mtd_device(info->mtd);
+	handle_of_flash_partitions(info, dev);
 	return 0;
 
 err_out:
@@ -221,6 +297,14 @@ err_out:
 
 static struct of_device_id of_physmap_match[] = {
 	{
+		.compatible	= "cfi-flash",
+		.data		= (void *)"cfi_probe",
+	},
+	{
+		.compatible	= "jedec-flash",
+		.data		= (void *)"jedec_probe",
+	},
+	{
 		.type		= "rom",
 		.compatible	= "direct-mapped"
 	},
Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
@@ -142,13 +142,16 @@
 				interrupt-parent = <&UIC1>;
 
 				small-flash@0,80000 {
-					device_type = "rom";
-					compatible = "direct-mapped";
-					probe-type = "JEDEC";
+					compatible = "jedec-flash";
 					bank-width = <1>;
-					partitions = <0 80000>;
-					partition-names = "OpenBIOS";
+//					partitions = <0 80000>;
+//					partition-names = "OpenBIOS";
 					reg = <0 80000 80000>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					OpenBIOS@0 {
+						reg = <0 80000>;
+					};
 				};
 
 				ds1743@1,0 {
@@ -158,14 +161,20 @@
 				};
 
 				large-flash@2,0 {
-					device_type = "rom";
-					compatible = "direct-mapped";
-					probe-type = "JEDEC";
+					compatible = "jedec-flash";
 					bank-width = <1>;
-					partitions = <0 380000
-						      380000 80000>;
-					partition-names = "fs", "firmware";
+//					partitions = <0 380000
+//						      380000 80000>;
+//					partition-names = "fs", "firmware";
 					reg = <2 0 400000>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+					fs@0 {
+						reg = <0 380000>;
+					};
+					firmware@380000 {
+						reg = <380000 80000>;
+					};
 				};
 
 				ir@3,0 {


-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Valentine Barshak]

David Gibson wrote:
> On Mon, Jul 30, 2007 at 07:06:48PM +0400, Valentine Barshak wrote:
>> AMCC Sequoia board DTS
>>
>> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>> ---
>>  arch/powerpc/boot/dts/sequoia.dts |  292 ++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 292 insertions(+)
>>
>> diff -ruN linux.orig/arch/powerpc/boot/dts/sequoia.dts linux/arch/powerpc/boot/dts/sequoia.dts
>> --- linux.orig/arch/powerpc/boot/dts/sequoia.dts	1970-01-01 03:00:00.000000000 +0300
>> +++ linux/arch/powerpc/boot/dts/sequoia.dts	2007-07-27 20:44:26.000000000 +0400
>> @@ -0,0 +1,292 @@
>> +/*
>> + * Device Tree Source for AMCC Sequoia
>> + *
>> + * Based on Bamboo code by Josh Boyer <jwboyer@linux.vnet.ibm.com>
>> + * Copyright (c) 2006, 2007 IBM Corp.
>> + *
>> + * FIXME: Draft only!
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without
>> + * any warranty of any kind, whether express or implied.
>> + *
>> + * To build:
>> + *   dtc -I dts -O asm -o bamboo.S -b 0 sequoia.dts
>> + *   dtc -I dts -O dtb -o bamboo.dtb -b 0 sequoia.dts
> 
> Needs updating to remove the bamboo references.  In fact we can
> probably get rid of this "To build" comment that's been copied to just
> about every dts ever.
> 

Old copy/paste stuff :)

>> + */
>> +
>> +/ {
>> +	#address-cells = <2>;
>> +	#size-cells = <1>;
>> +	model = "amcc,sequoia";
>> +	compatible = "amcc,sequoia";
>> +	dcr-parent = <&/cpus/PowerPC,440EPx@0>;
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		PowerPC,440EPx@0 {
>> +			device_type = "cpu";
>> +			reg = <0>;
>> +			clock-frequency = <0>; /* Filled in by zImage */
>> +			timebase-frequency = <0>; /* Filled in by zImage */
>> +			i-cache-line-size = <20>;
>> +			d-cache-line-size = <20>;
>> +			i-cache-size = <8000>;
>> +			d-cache-size = <8000>;
>> +			dcr-controller;
>> +			dcr-access-method = "native";
>> +		};
>> +	};
>> +
>> +	memory {
>> +		device_type = "memory";
>> +		reg = <0 0 0>; /* Filled in by zImage */
>> +	};
>> +
>> +	UIC0: interrupt-controller0 {
>> +		compatible = "ibm,uic-440gp","ibm,uic";
> 
> The first compatible entry should always be the precise model, so in
> this case "ibm,uic-440epx".  If it is (supposed to be) identical to
> the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> since I believe all the UICs are supposed to operate the same, I think
> that's implicit in the "ibm,uic" entry.
> 
> This goes for all the entries below where you list "ibm,....-440gp" or
> or "ibm,....-440ep" or "ibm,.....-440spe" first instead of
> "ibm,....-440epx".
> 

OK, I'll correct it.

>> +		interrupt-controller;
>> +		cell-index = <0>;
>> +		dcr-reg = <0c0 009>;
>> +		#address-cells = <0>;
>> +		#size-cells = <0>;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	UIC1: interrupt-controller1 {
>> +		compatible = "ibm,uic-440gp","ibm,uic";
>> +		interrupt-controller;
>> +		cell-index = <1>;
>> +		dcr-reg = <0d0 009>;
>> +		#address-cells = <0>;
>> +		#size-cells = <0>;
>> +		#interrupt-cells = <2>;
>> +		interrupts = <1e 4 1f 4>; /* cascade */
>> +		interrupt-parent = <&UIC0>;
>> +	};
>> +
>> +	UIC2: interrupt-controller2 {
>> +		compatible = "ibm,uic-440gp","ibm,uic";
>> +		interrupt-controller;
>> +		cell-index = <2>;
>> +		dcr-reg = <0e0 009>;
>> +		#address-cells = <0>;
>> +		#size-cells = <0>;
>> +		#interrupt-cells = <2>;
>> +		interrupts = <1c 4 1d 4>; /* cascade */
>> +		interrupt-parent = <&UIC0>;
>> +	};
>> +
>> +	SDR0: sdr {
> 
> What is the SDR?

SDR are System Device Control Registers (chip ID, pin function and stuff).
They are accessed by using the configuration address and data (CFGADDR 
and CFGDATA) registers.

> 
>> +		compatible = "ibm,sdr-440ep";
>> +		dcr-reg = <00e 002>;
>> +	};
>> +
>> +	CPR0: cpr {
> 
> And the CPR?

CPR are Clock/Power-On Reset configuration registers.
They are also accessed by using the configuration address and data 
(CFGADDR and CFGDATA) registers.

> 
>> +		compatible = "ibm,cpr-440ep";
>> +		dcr-reg = <00c 002>;
>> +	};
>> +
>> +	plb {
>> +		compatible = "ibm,plb-440gp", "ibm,plb4";
>> +		#address-cells = <2>;
>> +		#size-cells = <1>;
>> +		ranges;
>> +		clock-frequency = <0>; /* Filled in by zImage */
>> +
>> +		SDRAM0: sdram {
>> +			device_type = "memory-controller";
>> +			compatible = "ibm,sdram-44x-ddr2denali";
> 
> Should have a precise -440epx compatible, as well as the more general one.
OK, got it.
> 
>> +			dcr-reg = <010 2>;
>> +		};
>> +
>> +		DMA0: dma {
>> +			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
>> +			dcr-reg = <100 027>;
>> +		};
>> +
>> +		MAL0: mcmal {
>> +			compatible = "ibm,mcmal-440spe", "ibm,mcmal2";
>> +			dcr-reg = <180 62>;
>> +			num-tx-chans = <4>;
>> +			num-rx-chans = <4>;
>> +			interrupt-parent = <&MAL0>;
>> +			interrupts = <0 1 2 3 4>;
>> +			#interrupt-cells = <1>;
>> +			#address-cells = <0>;
>> +			#size-cells = <0>;
>> +			interrupt-map = </*TXEOB*/ 0 &UIC0 a 4
>> +					/*RXEOB*/ 1 &UIC0 b 4
>> +					/*SERR*/  2 &UIC1 0 4
>> +					/*TXDE*/  3 &UIC1 1 4
>> +					/*RXDE*/  4 &UIC1 2 4>;
>> +			interrupt-map-mask = <ffffffff>;
>> +		};
>> +
>> +		POB0: opb {
>> +		  	compatible = "ibm,opb-440gp", "ibm,opb";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			/* Bamboo is oddball in the 44x world and doesn't use the ERPN
>> +			 * bits.
>> +			 */
> 
> Comment is for Bamboo and does not match the ranges property below.
Oops :)
> 
>> +		  	ranges = <00000000 1 00000000 80000000
>> +			          80000000 1 80000000 80000000>;
>> +		  	interrupt-parent = <&UIC1>;
>> +		  	interrupts = <7 4>;
>> +		  	clock-frequency = <0>; /* Filled in by zImage */
>> +
>> +			EBC0: ebc {
>> +				compatible = "ibm,ebc-440gp";
>> +				dcr-reg = <012 2>;
>> +				#address-cells = <2>;
>> +				#size-cells = <1>;
>> +				clock-frequency = <0>; /* Filled in by zImage */
>> +				interrupts = <5 1>;
>> +				interrupt-parent = <&UIC1>;
>> +
>> +				nor_flash@0,0 {
>> +					device_type = "rom";
>> +					compatible = "direct-mapped";
>> +					probe-type = "CFI";
> 
> This flash binding needs to be replaced, but I guess that's not really
> your problem.
> 
>> +					bank-width = <2>;
>> +					partitions = <	0	180000
>> +							180000	200000
>> +							380000	3aa0000
>> +							3e20000	140000
>> +							3f60000	40000
>> +							3fa0000	60000>;
>> +					partition-names = "Kernel", "ramdisk", "file system",
>> +								"kozio", "env", "u-boot";
>> +					reg = <0 000000 4000000>;
>> +				};
>> +
>> +			};
>> +
>> +			UART0: serial@ef600300 {
>> +		   		device_type = "serial";
>> +		   		compatible = "ns16550";
>> +		   		reg = <ef600300 8>;
>> +		   		virtual-reg = <ef600300>;
>> +		   		clock-frequency = <0>; /* Filled in by zImage */
>> +		   		current-speed = <1c200>;
>> +		   		interrupt-parent = <&UIC0>;
>> +		   		interrupts = <0 4>;
>> +	   		};
>> +
>> +			UART1: serial@ef600400 {
>> +		   		device_type = "serial";
>> +		   		compatible = "ns16550";
>> +		   		reg = <ef600400 8>;
>> +		   		virtual-reg = <ef600400>;
>> +		   		clock-frequency = <0>;
>> +		   		current-speed = <0>;
>> +		   		interrupt-parent = <&UIC0>;
>> +		   		interrupts = <1 4>;
>> +	   		};
>> +
>> +			UART2: serial@ef600500 {
>> +		   		device_type = "serial";
>> +		   		compatible = "ns16550";
>> +		   		reg = <ef600500 8>;
>> +		   		virtual-reg = <ef600500>;
>> +		   		clock-frequency = <0>;
>> +		   		current-speed = <0>;
>> +		   		interrupt-parent = <&UIC1>;
>> +		   		interrupts = <3 4>;
>> +	   		};
>> +
>> +			UART3: serial@ef600600 {
>> +		   		device_type = "serial";
>> +		   		compatible = "ns16550";
>> +		   		reg = <ef600600 8>;
>> +		   		virtual-reg = <ef600600>;
>> +		   		clock-frequency = <0>;
>> +		   		current-speed = <0>;
>> +		   		interrupt-parent = <&UIC1>;
>> +		   		interrupts = <4 4>;
>> +	   		};
>> +
>> +			IIC0: i2c@ef600700 {
>> +				device_type = "i2c";
>> +				compatible = "ibm,iic-440gp", "ibm,iic";
>> +				reg = <ef600700 14>;
>> +				interrupt-parent = <&UIC0>;
>> +				interrupts = <2 4>;
>> +			};
>> +
>> +			IIC1: i2c@ef600800 {
>> +				device_type = "i2c";
>> +				compatible = "ibm,iic-44gp", "ibm,iic";
>> +				reg = <ef600800 14>;
>> +				interrupt-parent = <&UIC0>;
>> +				interrupts = <7 4>;
>> +			};
>> +
>> +			ZMII0: emac-zmii@ef600d00 {
>> +				device_type = "zmii-interface";
>> +				compatible = "ibm,zmii-440gp", "ibm,zmii";
>> +				reg = <ef600d00 c>;
>> +			};
>> +
>> +			EMAC0: ethernet@ef600e00 {
>> +				linux,network-index = <0>;
>> +				device_type = "network";
>> +				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
> 
> "ibm,emac-axon" is definitely wrong, since this isn't an Axon chip.

The chip uses EMACX_STACR_STAC_READ and EMACX_STACR_STAC_WRITE bits with 
mdio_read/mdio_write commands. These are only enabled if the chip is 
axon-compatible.
---
	if (device_is_compatible(np, "ibm,emac-axon"))
		dev->features |= EMAC_FTR_HAS_AXON_STACR
---

> 
>> +				interrupt-parent = <&EMAC0>;
>> +				interrupts = <0 1>;
>> +				#interrupt-cells = <1>;
>> +				#address-cells = <0>;
>> +				#size-cells = <0>;
>> +				interrupt-map = </*Status*/ 0 &UIC0 18 4
>> +						/*Wake*/  1 &UIC1 1d 4>;
>> +				reg = <ef600e00 70>;
>> +				local-mac-address = [000000000000];
>> +				mal-device = <&MAL0>;
>> +				mal-tx-channel = <0 1>;
>> +				mal-rx-channel = <0>;
>> +				cell-index = <0>;
>> +				max-frame-size = <5dc>;
>> +				rx-fifo-size = <1000>;
>> +				tx-fifo-size = <800>;
>> +				phy-mode = "rmii";
>> +				phy-map = <00000000>;
>> +				zmii-device = <&ZMII0>;
>> +				zmii-channel = <0>;
>> +			};
>> +
>> +			EMAC1: ethernet@ef600f00 {
>> +				linux,network-index = <1>;
>> +				device_type = "network";
>> +				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
>> +				interrupt-parent = <&EMAC1>;
>> +				interrupts = <0 1>;
>> +				#interrupt-cells = <1>;
>> +				#address-cells = <0>;
>> +				#size-cells = <0>;
>> +				interrupt-map = </*Status*/ 0 &UIC0 19 4
>> +						/*Wake*/  1 &UIC1 1f 4>;
>> +				reg = <ef600f00 70>;
>> +				local-mac-address = [000000000000];
>> +				mal-device = <&MAL0>;
>> +				mal-tx-channel = <2 3>;
>> +				mal-rx-channel = <1>;
>> +				cell-index = <1>;
>> +				max-frame-size = <5dc>;
>> +				rx-fifo-size = <1000>;
>> +				tx-fifo-size = <800>;
>> +				phy-mode = "rmii";
>> +				phy-map = <00000000>;
>> +				zmii-device = <&ZMII0>;
>> +				zmii-channel = <1>;
>> +			};
>> +		};
>> +	};
>> +
>> +	chosen {
>> +		linux,stdout-path = "/plb/opb/serial@ef600300";
>> +		bootargs = "console=ttyS0,115200";
>> +	};
>> +};
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@ozlabs.org
>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>
> 
Thanks for review,
Valentine.

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Wed, Aug 01, 2007 at 06:13:04PM +0400, Valentine Barshak wrote:
> David Gibson wrote:
> > On Mon, Jul 30, 2007 at 07:06:48PM +0400, Valentine Barshak wrote:
[snip]
> >> +	SDR0: sdr {
> > 
> > What is the SDR?
> 
> SDR are System Device Control Registers (chip ID, pin function and stuff).
> They are accessed by using the configuration address and data (CFGADDR 
> and CFGDATA) registers.

Ok.

> >> +		compatible = "ibm,sdr-440ep";
> >> +		dcr-reg = <00e 002>;
> >> +	};
> >> +
> >> +	CPR0: cpr {
> > 
> > And the CPR?
> 
> CPR are Clock/Power-On Reset configuration registers.
> They are also accessed by using the configuration address and data 
> (CFGADDR and CFGDATA) registers.

Ok, so the rough equivalent of the 440GP's CPC registers (but with an
entirely different register layout).

[snip]
> >> +			EMAC0: ethernet@ef600e00 {
> >> +				linux,network-index = <0>;
> >> +				device_type = "network";
> >> +				compatible = "ibm,emac-440spe", "ibm,emac4", "ibm,emac-axon";
> > 
> > "ibm,emac-axon" is definitely wrong, since this isn't an Axon chip.
> 
> The chip uses EMACX_STACR_STAC_READ and EMACX_STACR_STAC_WRITE bits with 
> mdio_read/mdio_write commands. These are only enabled if the chip is 
> axon-compatible.
> ---
> 	if (device_is_compatible(np, "ibm,emac-axon"))
> 		dev->features |= EMAC_FTR_HAS_AXON_STACR
> ---

Then the driver needs fixing; I'll talk to Ben and look into it.
We'll need something in the device tree to indicate this, but it
shouldn't be "emac-axon" in compatible.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>Also mine.  I've been home sick the last couple of days, but by way of
>>a sharper prod, see my draft work below.  It patches both
>>booting-without-of.txt with a revised binding, and implements it in
>>the physmap_of driver (which needs renaming, but that's another
>>story).  It also revises the ebony device tree as an example.

>>This is certainly not complete - it defines none of the extra
>>properties that JEDEC chips need (although the mtd drivers'
>>defaults/probing seem to cope for ebony).  And there are various other
>>ommisions.  Still, it's a starting point - something precise for you
>>to flame Segher :-p.

    Let me flame you a bit for starters ;-)

> Duh, forgot to attach the actual patch.  Here it is:

> Index: working-2.6/Documentation/powerpc/booting-without-of.txt
> ===================================================================
> --- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> +++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> @@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
>  		};
>  	};
>  
> -    j) Flash chip nodes
> +   j) CFI or JEDEC memory-mapped NOR flash
>  
>      Flash chips (Memory Technology Devices) are often used for solid state
>      file systems on embedded devices.
>  
> -    Required properties:
> +     - compatible : should contain the specific model of flash chip(s) used
> +       followed by either "cfi-flash" or "jedec-flash"

    This "compatible" prop (and the node in whole) doesn't say a thing about 
how the flash is mapped into the CPU address space.  I strongly disagree that 
this node provides enough info to select a driver. :-/

> +     - reg : Address range of the flash chip
> +     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
> +       times the number of interleaved chips.
> +     - device-width : (optional) Width of a single flash chip.  If omitted,
> +       assumed to be equal to 'bank-width'.

    Why then not just introduce the "interleave" prop and obsolete the
"bank-width"?

> -   Example:
> -
> - 	flash@ff000000 {
> - 		device_type = "rom";
> - 		compatible = "direct-mapped";
> - 		probe-type = "CFI";
> - 		reg = <ff000000 01000000>;
> - 		bank-width = <4>;
> - 		partitions = <00000000 00f80000
> - 			      00f80000 00080001>;
> - 		partition-names = "fs\0firmware";
> - 	};

    Why delete the example? :-/

> +    Flash partitions
> +     - reg :
> +     - read-only : (optional)

    All that would look nice but partition is even less of a device than the
original "rom" node was... well, who cares now? :-)

> Index: working-2.6/drivers/mtd/maps/physmap_of.c
> ===================================================================
> --- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
> +++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
[...]
> @@ -30,17 +33,72 @@ struct physmap_flash_info {
>  	struct map_info		map;
>  	struct resource		*res;
>  #ifdef CONFIG_MTD_PARTITIONS
> -	int			nr_parts;
>  	struct mtd_partition	*parts;
>  #endif
>  };
>  
> -static const char *rom_probe_types[] = { "cfi_probe", "jedec_probe", "map_rom", NULL };
>  #ifdef CONFIG_MTD_PARTITIONS
> -static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
> -#endif
> +static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
> +						 struct of_device *dev)
> +{
> +	static const char *part_probe_types[]
> +		= { "cmdlinepart", "RedBoot", NULL };
> +	struct device_node *dp = dev->node, *pp;
> +	int nr_parts, i, err;
> +
> +	/* First look for RedBoot table or partitions on the command
> +	 * line, these take precedence over device tree information */
> +	nr_parts = parse_mtd_partitions(info->mtd, part_probe_types,
> +					&info->parts, 0);
> +	if (nr_parts > 0) {
> +		add_mtd_partitions(info->mtd, info->parts, err);
> +		return 0;
> +	}
> +
> +	/* First count the subnodes */
> +	nr_parts = 0;
> +	for (pp = dp->child; pp; pp = pp->sibling)
> +		nr_parts++;
> +
> +	info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition), GFP_KERNEL);
> +	if (!info->parts) {
> +		printk(KERN_ERR "Can't allocate the flash partition data!\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
> +		const u32 *reg;
> +		const char *name;
> +		const void *ro;

    We hardly need the above 2 variables.

> +		int len;
> +
> +		reg = of_get_property(pp, "reg", &len);
> +		if (! reg || (len != 2*sizeof(u32))) {

    Kill the space after !, please.

> +			dev_err(&dev->dev, "Invalid 'reg' on %s\n",
> +				dp->full_name);
> +			err = -EINVAL;
> +			goto fail;
> +		}
> +		info->parts[i].offset = reg[0];
> +		info->parts[i].size = reg[1];
> +
> +		name = of_get_property(pp, "name", &len);
> +		info->parts[i].name = name;
> +
> +		ro = of_get_property(pp, "read-only", &len);
> +		if (ro)
> +			info->parts[i].mask_flags = MTD_WRITEABLE;
> +	}
> +
> +	add_mtd_partitions(info->mtd, info->parts, nr_parts);
> +	return 0;
> +
> + fail:
> +	kfree(info->parts);
> +	info->parts = NULL;
> +	return err;
> +}

    Oh, I see that the new partition representation have really simplified
parsing -- this function is hardly shorter than the old one... :-P

> -#ifdef CONFIG_MTD_PARTITIONS
>  static int parse_flash_partitions(struct device_node *node,
>  		struct mtd_partition **parts)
>  {
> @@ -79,7 +137,14 @@ static int parse_flash_partitions(struct
>  err:
>  	return retval;

    Could get rid of that useless label and goto's in this function, while at
it...

>  }
> -#endif
> +#else /* MTD_PARTITIONS */
> +static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
> +						 struct device_node *dev)
> +{
> +	add_mtd_device(info->mtd);
> +	return 0;
> +}
> +#endif /* MTD_PARTITIONS */
>  
>  static int of_physmap_remove(struct of_device *dev)
>  {
> @@ -115,13 +180,45 @@ static int of_physmap_remove(struct of_d
>  	return 0;
>  }
>  
> +/* Helper function to handle probing of the obsolete "direct-mapped"
> + * compatible binding, which has an extra "probe-type" property
> + * describing the type of flash probe necessary. */

    Too early to obsolete it, I'm afraid... :-)

> +static struct mtd_info * __devinit obsolete_probe(struct of_device *dev,
> +						  struct map_info *map)
> +{
> +	struct device_node *dp = dev->node;
> +	const char *of_probe;
> +	struct mtd_info *mtd;
> +	static const char *rom_probe_types[]
> +		= { "cfi_probe", "jedec_probe", "map_rom"};
> +	int i;
> +
> +	of_probe = of_get_property(dp, "probe-type", NULL);
> +	if (!of_probe) {
> +		for (i = 0; i < ARRAY_SIZE(rom_probe_types); i++) {
> +			mtd = do_map_probe(rom_probe_types[i], map);
> +			if (mtd)
> +				return mtd;
> +		}
> +		return NULL;
> +	} else if (strcmp(of_probe, "CFI") == 0) {
> +		return do_map_probe("cfi_probe", map);
> +	} else if (strcmp(of_probe, "JEDEC") == 0) {
> +		return do_map_probe("jedec_probe", map);
> +	} else {
> + 		if (strcmp(of_probe, "ROM") != 0)
> +			dev_dbg(&dev->dev, "obsolete_probe: don't know probe type "
> +				"'%s', mapping as rom\n", of_probe);
> +		return do_map_probe("mtd_rom", map);
> +	}
> +}
> +
>  static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
>  {
>  	struct device_node *dp = dev->node;
>  	struct resource res;
>  	struct physmap_flash_info *info;
> -	const char **probe_type;
> -	const char *of_probe;
> +	const char *probe_type = (const char *)match->data;
>  	const u32 *width;
>  	int err;
[...]
> @@ -221,6 +297,14 @@ err_out:
>  
>  static struct of_device_id of_physmap_match[] = {
>  	{
> +		.compatible	= "cfi-flash",
> +		.data		= (void *)"cfi_probe",
> +	},
> +	{
> +		.compatible	= "jedec-flash",
> +		.data		= (void *)"jedec_probe",
> +	},
> +	{

    This would also trigger on non-linearly mapped CFI or JEDEC flashes which 
is not a good idea -- however, as we're using the MTD probing code anyway, it 
will just fail, so it's not luckily is not a fatal design flaw.

>  		.type		= "rom",
>  		.compatible	= "direct-mapped"
>  	},
> Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
> ===================================================================
> --- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> +++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
[...]
> @@ -158,14 +161,20 @@
>  				};
>  
>  				large-flash@2,0 {

    Hmm... what does @2,0 mean? :-O

> -					device_type = "rom";
> -					compatible = "direct-mapped";
> -					probe-type = "JEDEC";
> +					compatible = "jedec-flash";
>  					bank-width = <1>;
> -					partitions = <0 380000
> -						      380000 80000>;
> -					partition-names = "fs", "firmware";
> +//					partitions = <0 380000
> +//						      380000 80000>;
> +//					partition-names = "fs", "firmware";
>  					reg = <2 0 400000>;
> +					#address-cells = <1>;
> +					#size-cells = <1>;

    Heh...

> +					fs@0 {
> +						reg = <0 380000>;
> +					};
> +					firmware@380000 {
> +						reg = <380000 80000>;

   I guess the "firmware" should have a "read-only" prop...

> +					};
>  				};

    So, I don't see what we're really winning with your changes...

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Josh Boyer]

On Wed, 1 Aug 2007 12:08:36 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jul 30, 2007 at 07:06:48PM +0400, Valentine Barshak wrote:
> > AMCC Sequoia board DTS
> > 
> > Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
> > ---
> >  arch/powerpc/boot/dts/sequoia.dts |  292
> > ++++++++++++++++++++++++++++++++++++++ 1 files changed, 292
> > insertions(+)
> > 
> > diff -ruN linux.orig/arch/powerpc/boot/dts/sequoia.dts
> > linux/arch/powerpc/boot/dts/sequoia.dts ---
> > linux.orig/arch/powerpc/boot/dts/sequoia.dts	1970-01-01
> > 03:00:00.000000000 +0300 +++
> > linux/arch/powerpc/boot/dts/sequoia.dts	2007-07-27
> > 20:44:26.000000000 +0400 @@ -0,0 +1,292 @@ +/*
> > + * Device Tree Source for AMCC Sequoia
> > + *
> > + * Based on Bamboo code by Josh Boyer <jwboyer@linux.vnet.ibm.com>
> > + * Copyright (c) 2006, 2007 IBM Corp.
> > + *
> > + * FIXME: Draft only!
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2.  This program is licensed "as is" without
> > + * any warranty of any kind, whether express or implied.
> > + *
> > + * To build:
> > + *   dtc -I dts -O asm -o bamboo.S -b 0 sequoia.dts
> > + *   dtc -I dts -O dtb -o bamboo.dtb -b 0 sequoia.dts
> 
> Needs updating to remove the bamboo references.  In fact we can
> probably get rid of this "To build" comment that's been copied to just
> about every dts ever.
> 
> > + */
> > +
> > +/ {
> > +	#address-cells = <2>;
> > +	#size-cells = <1>;
> > +	model = "amcc,sequoia";
> > +	compatible = "amcc,sequoia";
> > +	dcr-parent = <&/cpus/PowerPC,440EPx@0>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		PowerPC,440EPx@0 {
> > +			device_type = "cpu";
> > +			reg = <0>;
> > +			clock-frequency = <0>; /* Filled in by
> > zImage */
> > +			timebase-frequency = <0>; /* Filled in by
> > zImage */
> > +			i-cache-line-size = <20>;
> > +			d-cache-line-size = <20>;
> > +			i-cache-size = <8000>;
> > +			d-cache-size = <8000>;
> > +			dcr-controller;
> > +			dcr-access-method = "native";
> > +		};
> > +	};
> > +
> > +	memory {
> > +		device_type = "memory";
> > +		reg = <0 0 0>; /* Filled in by zImage */
> > +	};
> > +
> > +	UIC0: interrupt-controller0 {
> > +		compatible = "ibm,uic-440gp","ibm,uic";
> 
> The first compatible entry should always be the precise model, so in
> this case "ibm,uic-440epx".  If it is (supposed to be) identical to
> the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> since I believe all the UICs are supposed to operate the same, I think
> that's implicit in the "ibm,uic" entry.

Most UICs are the same.  There are some oddball chips that either hide
particular registers because they are unused, or they change the
addressing stride.  I'm not sure that is a common enough case to worry
about now though.

josh

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Josh Boyer]

On Wed, 1 Aug 2007 15:04:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 01, 2007 at 06:57:33AM +0200, Segher Boessenkool wrote:
> > >> +	UIC0: interrupt-controller0 {
> > >> +		compatible = "ibm,uic-440gp","ibm,uic";
> > >
> > > The first compatible entry should always be the precise model, so
> > > in this case "ibm,uic-440epx".
> > 
> > This isn't really _required_, but it is a very good idea in
> > almost all cases (the exception is for very generic or legacy
> > devices).
> 
> Well, yes.  That's a "should" not a "must" in rfc-speak.
> 
> > > If it is (supposed to be) identical to
> > > the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry,
> > > but since I believe all the UICs are supposed to operate the
> > > same, I think that's implicit in the "ibm,uic" entry.
> > 
> > Sure, but there is no harm in having the better qualified 440gp
> > name in there as well -- bytes are cheap :-)
> > 
> > >> +	SDR0: sdr {
> > >
> > > What is the SDR?
> > >
> > >> +		compatible = "ibm,sdr-440ep";
> > >> +		dcr-reg = <00e 002>;
> > >> +	};
> > >> +
> > >> +	CPR0: cpr {
> > >
> > > And the CPR?
> > 
> > Yeah, better names please -- if possible, something that someone
> > without knowledge of this SoC will understand what it is.
> 
> I think the names are probably ok - I'm assuming they're in keeping
> with the convention I've used of using the same names / abbreviations
> as in the CPU user manual.  I'm asking just for my own information,
> although a comment might not be a bad idea.

Yes, they are the names used in the user manual.  I'd prefer to keep
them as is.

And yeah, they are similar to the macros found on 440GP.  But not the
same, hence a different name.

josh

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Josh Boyer]

On Wed, 1 Aug 2007 15:47:51 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> Duh, forgot to attach the actual patch.  Here it is:

So, no signed-off-by.  Intentional, as it's for comments only?

Also, could you break this out into a separate thread when you do
submit it please?  Will make a few people's lives easier since this has
nothing to do with 440EPx really :)

josh

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Thu, Aug 02, 2007 at 03:18:33PM -0500, Josh Boyer wrote:
> On Wed, 1 Aug 2007 15:47:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > Duh, forgot to attach the actual patch.  Here it is:
> 
> So, no signed-off-by.  Intentional, as it's for comments only?

Pretty much.

> Also, could you break this out into a separate thread when you do
> submit it please?  Will make a few people's lives easier since this has
> nothing to do with 440EPx really :)

Sure.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Thu, Aug 02, 2007 at 07:23:00PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> David Gibson wrote:
> 
> >>Also mine.  I've been home sick the last couple of days, but by way of
> >>a sharper prod, see my draft work below.  It patches both
> >>booting-without-of.txt with a revised binding, and implements it in
> >>the physmap_of driver (which needs renaming, but that's another
> >>story).  It also revises the ebony device tree as an example.
> 
> >>This is certainly not complete - it defines none of the extra
> >>properties that JEDEC chips need (although the mtd drivers'
> >>defaults/probing seem to cope for ebony).  And there are various other
> >>ommisions.  Still, it's a starting point - something precise for you
> >>to flame Segher :-p.
> 
>     Let me flame you a bit for starters ;-)

Well, ok then.

> > Duh, forgot to attach the actual patch.  Here it is:
> 
> > Index: working-2.6/Documentation/powerpc/booting-without-of.txt
> > ===================================================================
> > --- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> > +++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> > @@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
> >  		};
> >  	};
> >  
> > -    j) Flash chip nodes
> > +   j) CFI or JEDEC memory-mapped NOR flash
> >  
> >      Flash chips (Memory Technology Devices) are often used for solid state
> >      file systems on embedded devices.
> >  
> > -    Required properties:
> > +     - compatible : should contain the specific model of flash chip(s) used
> > +       followed by either "cfi-flash" or "jedec-flash"
> 
>     This "compatible" prop (and the node in whole) doesn't say a
> thing about how the flash is mapped into the CPU address space.  I
> strongly disagree that this node provides enough info to select a
> driver. :-/

To be honest, I'm not sure that describing the mapping is really the
job of the compatible property.  That the flash is mapped into the
address space is kind of implicit in the way the reg interacts with
the parents' ranges property.

Can you describe some of the options for *not* direct mapped flash
chips - I can't reasonably come up with a way of describing the
distinction when I've never seen NOR flash other than direct mapped.

> > +     - reg : Address range of the flash chip
> > +     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
> > +       times the number of interleaved chips.
> > +     - device-width : (optional) Width of a single flash chip.  If omitted,
> > +       assumed to be equal to 'bank-width'.
> 
>     Why then not just introduce the "interleave" prop and obsolete the
> "bank-width"?

Because they're equivalent information, and bank-width is what the
code ends up actually caring about.  I don't see any reason to prefer
giving the interleave.

> > -   Example:
> > -
> > - 	flash@ff000000 {
> > - 		device_type = "rom";
> > - 		compatible = "direct-mapped";
> > - 		probe-type = "CFI";
> > - 		reg = <ff000000 01000000>;
> > - 		bank-width = <4>;
> > - 		partitions = <00000000 00f80000
> > - 			      00f80000 00080001>;
> > - 		partition-names = "fs\0firmware";
> > - 	};
> 
>     Why delete the example? :-/

Because it's the old style, and I haven't gotten around to writing a
new example for the new style.

> > +    Flash partitions
> > +     - reg :
> > +     - read-only : (optional)
> 
>     All that would look nice but partition is even less of a device than the
> original "rom" node was... well, who cares now? :-)
> 
> > Index: working-2.6/drivers/mtd/maps/physmap_of.c
> > ===================================================================
> > --- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
> > +++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
> [...]
> > @@ -30,17 +33,72 @@ struct physmap_flash_info {
> >  	struct map_info		map;
> >  	struct resource		*res;
> >  #ifdef CONFIG_MTD_PARTITIONS
> > -	int			nr_parts;
> >  	struct mtd_partition	*parts;
> >  #endif
> >  };
> >  
> > -static const char *rom_probe_types[] = { "cfi_probe", "jedec_probe", "map_rom", NULL };
> >  #ifdef CONFIG_MTD_PARTITIONS
> > -static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
> > -#endif
> > +static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
> > +						 struct of_device *dev)
> > +{
> > +	static const char *part_probe_types[]
> > +		= { "cmdlinepart", "RedBoot", NULL };
> > +	struct device_node *dp = dev->node, *pp;
> > +	int nr_parts, i, err;
> > +
> > +	/* First look for RedBoot table or partitions on the command
> > +	 * line, these take precedence over device tree information */
> > +	nr_parts = parse_mtd_partitions(info->mtd, part_probe_types,
> > +					&info->parts, 0);
> > +	if (nr_parts > 0) {
> > +		add_mtd_partitions(info->mtd, info->parts, err);
> > +		return 0;
> > +	}
> > +
> > +	/* First count the subnodes */
> > +	nr_parts = 0;
> > +	for (pp = dp->child; pp; pp = pp->sibling)
> > +		nr_parts++;
> > +
> > +	info->parts = kzalloc(nr_parts * sizeof(struct mtd_partition), GFP_KERNEL);
> > +	if (!info->parts) {
> > +		printk(KERN_ERR "Can't allocate the flash partition data!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
> > +		const u32 *reg;
> > +		const char *name;
> > +		const void *ro;
> 
>     We hardly need the above 2 variables.

They're all used...

> > +		int len;
> > +
> > +		reg = of_get_property(pp, "reg", &len);
> > +		if (! reg || (len != 2*sizeof(u32))) {
> 
>     Kill the space after !, please.

Done.

> > +			dev_err(&dev->dev, "Invalid 'reg' on %s\n",
> > +				dp->full_name);
> > +			err = -EINVAL;
> > +			goto fail;
> > +		}
> > +		info->parts[i].offset = reg[0];
> > +		info->parts[i].size = reg[1];
> > +
> > +		name = of_get_property(pp, "name", &len);
> > +		info->parts[i].name = name;
> > +
> > +		ro = of_get_property(pp, "read-only", &len);
> > +		if (ro)
> > +			info->parts[i].mask_flags = MTD_WRITEABLE;
> > +	}
> > +
> > +	add_mtd_partitions(info->mtd, info->parts, nr_parts);
> > +	return 0;
> > +
> > + fail:
> > +	kfree(info->parts);
> > +	info->parts = NULL;
> > +	return err;
> > +}
> 
>     Oh, I see that the new partition representation have really simplified
> parsing -- this function is hardly shorter than the old one... :-P

They new format isn't supposed to be simpler to parse: it's supposed
to be more flexible if we ever need to add more per-partition
information than just the offset / size / read-only.

> > -#ifdef CONFIG_MTD_PARTITIONS
> >  static int parse_flash_partitions(struct device_node *node,
> >  		struct mtd_partition **parts)
> >  {
> > @@ -79,7 +137,14 @@ static int parse_flash_partitions(struct
> >  err:
> >  	return retval;
> 
>     Could get rid of that useless label and goto's in this function, while at
> it...

Yes, haven't really touched this function yet - it needs to be cleaned
up and re-integrated as the fallback method to parse device trees with
the old-style partition description.

> >  }
> > -#endif
> > +#else /* MTD_PARTITIONS */
> > +static int __devinit handle_of_flash_partitions(struct physmap_flash_info *info,
> > +						 struct device_node *dev)
> > +{
> > +	add_mtd_device(info->mtd);
> > +	return 0;
> > +}
> > +#endif /* MTD_PARTITIONS */
> >  
> >  static int of_physmap_remove(struct of_device *dev)
> >  {
> > @@ -115,13 +180,45 @@ static int of_physmap_remove(struct of_d
> >  	return 0;
> >  }
> >  
> > +/* Helper function to handle probing of the obsolete "direct-mapped"
> > + * compatible binding, which has an extra "probe-type" property
> > + * describing the type of flash probe necessary. */
> 
>     Too early to obsolete it, I'm afraid... :-)

Hrm..  obsolete != deprecate.  The names designed to discourage use of
it for anything new, not to indicate that it's about to go away.

> > +static struct mtd_info * __devinit obsolete_probe(struct of_device *dev,
> > +						  struct map_info *map)
> > +{
> > +	struct device_node *dp = dev->node;
> > +	const char *of_probe;
> > +	struct mtd_info *mtd;
> > +	static const char *rom_probe_types[]
> > +		= { "cfi_probe", "jedec_probe", "map_rom"};
> > +	int i;
> > +
> > +	of_probe = of_get_property(dp, "probe-type", NULL);
> > +	if (!of_probe) {
> > +		for (i = 0; i < ARRAY_SIZE(rom_probe_types); i++) {
> > +			mtd = do_map_probe(rom_probe_types[i], map);
> > +			if (mtd)
> > +				return mtd;
> > +		}
> > +		return NULL;
> > +	} else if (strcmp(of_probe, "CFI") == 0) {
> > +		return do_map_probe("cfi_probe", map);
> > +	} else if (strcmp(of_probe, "JEDEC") == 0) {
> > +		return do_map_probe("jedec_probe", map);
> > +	} else {
> > + 		if (strcmp(of_probe, "ROM") != 0)
> > +			dev_dbg(&dev->dev, "obsolete_probe: don't know probe type "
> > +				"'%s', mapping as rom\n", of_probe);
> > +		return do_map_probe("mtd_rom", map);
> > +	}
> > +}
> > +
> >  static int __devinit of_physmap_probe(struct of_device *dev, const struct of_device_id *match)
> >  {
> >  	struct device_node *dp = dev->node;
> >  	struct resource res;
> >  	struct physmap_flash_info *info;
> > -	const char **probe_type;
> > -	const char *of_probe;
> > +	const char *probe_type = (const char *)match->data;
> >  	const u32 *width;
> >  	int err;
> [...]
> > @@ -221,6 +297,14 @@ err_out:
> >  
> >  static struct of_device_id of_physmap_match[] = {
> >  	{
> > +		.compatible	= "cfi-flash",
> > +		.data		= (void *)"cfi_probe",
> > +	},
> > +	{
> > +		.compatible	= "jedec-flash",
> > +		.data		= (void *)"jedec_probe",
> > +	},
> > +	{
> 
>     This would also trigger on non-linearly mapped CFI or JEDEC
> flashes which is not a good idea -- however, as we're using the MTD
> probing code anyway, it will just fail, so it's not luckily is not a
> fatal design flaw.

Well, if there's nothing else to distinguish them.  Which there isn't
yet, but will need to be: see above about incomplete - helpful
suggestions about how to describe the mapping would be more useful
than merely pointing out the lack.

> >  		.type		= "rom",
> >  		.compatible	= "direct-mapped"
> >  	},
> > Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
> > ===================================================================
> > --- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> > +++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> [...]
> > @@ -158,14 +161,20 @@
> >  				};
> >  
> >  				large-flash@2,0 {
> 
>     Hmm... what does @2,0 mean? :-O

EBC chip select 2, offset 0.

> > -					device_type = "rom";
> > -					compatible = "direct-mapped";
> > -					probe-type = "JEDEC";
> > +					compatible = "jedec-flash";
> >  					bank-width = <1>;
> > -					partitions = <0 380000
> > -						      380000 80000>;
> > -					partition-names = "fs", "firmware";
> > +//					partitions = <0 380000
> > +//						      380000 80000>;
> > +//					partition-names = "fs", "firmware";
> >  					reg = <2 0 400000>;
> > +					#address-cells = <1>;
> > +					#size-cells = <1>;
> 
>     Heh...

Yeah, that bit's a bit ugly, I'll grant you.

> > +					fs@0 {
> > +						reg = <0 380000>;
> > +					};
> > +					firmware@380000 {
> > +						reg = <380000 80000>;
> 
>    I guess the "firmware" should have a "read-only" prop...

Possibly.

> > +					};
> >  				};
> 
>     So, I don't see what we're really winning with your changes...

"direct-mapped" is simply not a sufficiently specific compatible
property, no two ways about it.  This spec still needs more specific
description of some properties, at least for JEDEC flashes.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>>Index: working-2.6/Documentation/powerpc/booting-without-of.txt
>>>===================================================================
>>>--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
>>>+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
>>>@@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
>>> 		};
>>> 	};
>>> 
>>>-    j) Flash chip nodes
>>>+   j) CFI or JEDEC memory-mapped NOR flash
>>> 
>>>     Flash chips (Memory Technology Devices) are often used for solid state
>>>     file systems on embedded devices.

>>>-    Required properties:
>>>+     - compatible : should contain the specific model of flash chip(s) used
>>>+       followed by either "cfi-flash" or "jedec-flash"

>>    This "compatible" prop (and the node in whole) doesn't say a
>>thing about how the flash is mapped into the CPU address space.  I
>>strongly disagree that this node provides enough info to select a
>>driver. :-/

> To be honest, I'm not sure that describing the mapping is really the
> job of the compatible property.  That the flash is mapped into the
> address space is kind of implicit in the way the reg interacts with
> the parents' ranges property.

    Ah, I keep forgetting about implied 1:1 parent/child address 
correspondence... :-<
    But does it really imply how the (low) address bits of the *child* bus 
("ebc" in this case) are connected to the chip?  I don't think so...

> Can you describe some of the options for *not* direct mapped flash
> chips - I can't reasonably come up with a way of describing the
> distinction when I've never seen NOR flash other than direct mapped.

    You're lucky to live in the single-endian world.  Once you're in bi-endian 
one, all kinds of strange mappings become possible.  I've seen the MIPS 
mapping driver which required swapping flash bytes in s/w in BE mode, i.e. 
couldn't be served by physmap, yet that's not all...  here's the code of its 
map read*() methods:

static __u8 xxx_map_read8(struct map_info *map, unsigned long offs)
{
         u16 val;

         val = readw(map->map_priv_1 + (offs & ~1));
         if (offs & 1)
                 return ((val >> 8) & 0xff);
         else
                 return (val & 0xff);
}

static __u16 bcm947xx_map_read16(struct map_info *map, unsigned long offs)
{
         return readw(map->map_priv_1 + offs);
}

static __u32 bcm947xx_map_read32(struct map_info *map, unsigned long offs)
{
         return readl(map->map_priv_1 + offs);
}

... while the simple map (used by physmap) has just __raw_read*() for those?

>>>+     - reg : Address range of the flash chip
>>>+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
>>>+       times the number of interleaved chips.
>>>+     - device-width : (optional) Width of a single flash chip.  If omitted,
>>>+       assumed to be equal to 'bank-width'.

>>    Why then not just introduce the "interleave" prop and obsolete the
>>"bank-width"?

> Because they're equivalent information, and bank-width is what the
> code ends up actually caring about.  I don't see any reason to prefer
> giving the interleave.

    Well, "device-width" is not the thing that we care about either. ;-)

>>>Index: working-2.6/drivers/mtd/maps/physmap_of.c
>>>===================================================================
>>>--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
>>>+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
[...]
>>>+	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
>>>+		const u32 *reg;
>>>+		const char *name;
>>>+		const void *ro;
>>
>>    We hardly need the above 2 variables.

> They're all used...

    I meant that there's no necessity in them.

[...]

>>    Oh, I see that the new partition representation have really simplified
>>parsing -- this function is hardly shorter than the old one... :-P

> They new format isn't supposed to be simpler to parse: it's supposed
> to be more flexible if we ever need to add more per-partition
> information than just the offset / size / read-only.

    Well, if we ever need that indeed... :-)

[...]

>>>@@ -221,6 +297,14 @@ err_out:
>>> 
>>> static struct of_device_id of_physmap_match[] = {
>>> 	{
>>>+		.compatible	= "cfi-flash",
>>>+		.data		= (void *)"cfi_probe",
>>>+	},
>>>+	{
>>>+		.compatible	= "jedec-flash",
>>>+		.data		= (void *)"jedec_probe",
>>>+	},
>>>+	{

>>    This would also trigger on non-linearly mapped CFI or JEDEC
>>flashes which is not a good idea -- however, as we're using the MTD
>>probing code anyway, it will just fail, so it's not luckily is not a
>>fatal design flaw.

> Well, if there's nothing else to distinguish them.  Which there isn't
> yet, but will need to be: see above about incomplete - helpful
> suggestions about how to describe the mapping would be more useful
> than merely pointing out the lack.

    I was thinking about adding "direct-mapped" prop... but maybe adding 
"ranges" to the parent node (that's "ebc") would indeed ensure that the flash 
is mapped 1:1 to the EBC's parent bus also.

>>>Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
>>>===================================================================
>>>--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
>>>+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000

>>[...]

>>>@@ -158,14 +161,20 @@
>>> 				};

>>> 				large-flash@2,0 {

>>    Hmm... what does @2,0 mean? :-O

> EBC chip select 2, offset 0.

    Well, so this node is under some kind of local bus node -- that's good.
Didn't know that the spec allows commas after @...

>>>-					device_type = "rom";
>>>-					compatible = "direct-mapped";
>>>-					probe-type = "JEDEC";
>>>+					compatible = "jedec-flash";
>>> 					bank-width = <1>;
>>>-					partitions = <0 380000
>>>-						      380000 80000>;
>>>-					partition-names = "fs", "firmware";
>>>+//					partitions = <0 380000
>>>+//						      380000 80000>;
>>>+//					partition-names = "fs", "firmware";
>>> 					reg = <2 0 400000>;
>>>+					#address-cells = <1>;
>>>+					#size-cells = <1>;

>>    Heh...

> Yeah, that bit's a bit ugly, I'll grant you.

    Don't we need "ranges" here, at least from the formal PoV -- as the parent 
and child address spaces differ? I know the physmap_of parser doesn't care but 
still...

>>>+					};
>>> 				};

>>    So, I don't see what we're really winning with your changes...

> "direct-mapped" is simply not a sufficiently specific compatible
> property, no two ways about it.

    Yes, for example "direct-mapped-cfi" and "direct-mapped-jedec" would have 
been better...

> This spec still needs more specific
> description of some properties, at least for JEDEC flashes.

    Yes, of course...

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

David Gibson wrote:

> Duh, forgot to attach the actual patch.  Here it is:

> Index: working-2.6/Documentation/powerpc/booting-without-of.txt
> ===================================================================
> --- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> +++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> @@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
>  		};
>  	};
>  
> -    j) Flash chip nodes
> +   j) CFI or JEDEC memory-mapped NOR flash
>  
>      Flash chips (Memory Technology Devices) are often used for solid state
>      file systems on embedded devices.
>  
> -    Required properties:
> +     - compatible : should contain the specific model of flash chip(s) used
> +       followed by either "cfi-flash" or "jedec-flash"

    Duh, have nearly forgotten to complain about "-flash" suffix.  Isn't it 
superfluous?

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Fri, Aug 03, 2007 at 07:47:43PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> David Gibson wrote:
> 
> >>>Index: working-2.6/Documentation/powerpc/booting-without-of.txt
> >>>===================================================================
> >>>--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> >>>+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> >>>@@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
> >>> 		};
> >>> 	};
> >>> 
> >>>-    j) Flash chip nodes
> >>>+   j) CFI or JEDEC memory-mapped NOR flash
> >>> 
> >>>     Flash chips (Memory Technology Devices) are often used for solid state
> >>>     file systems on embedded devices.
> 
> >>>-    Required properties:
> >>>+     - compatible : should contain the specific model of flash chip(s) used
> >>>+       followed by either "cfi-flash" or "jedec-flash"
> 
> >>    This "compatible" prop (and the node in whole) doesn't say a
> >>thing about how the flash is mapped into the CPU address space.  I
> >>strongly disagree that this node provides enough info to select a
> >>driver. :-/
> 
> > To be honest, I'm not sure that describing the mapping is really the
> > job of the compatible property.  That the flash is mapped into the
> > address space is kind of implicit in the way the reg interacts with
> > the parents' ranges property.
> 
>     Ah, I keep forgetting about implied 1:1 parent/child address 
> correspondence... :-<
>     But does it really imply how the (low) address bits of the *child* bus 
> ("ebc" in this case) are connected to the chip?  I don't think so...
> 
> > Can you describe some of the options for *not* direct mapped flash
> > chips - I can't reasonably come up with a way of describing the
> > distinction when I've never seen NOR flash other than direct mapped.
> 
>     You're lucky to live in the single-endian world.  Once you're in bi-endian 
> one, all kinds of strange mappings become possible.  I've seen the MIPS 
> mapping driver which required swapping flash bytes in s/w in BE mode, i.e. 
> couldn't be served by physmap, yet that's not all...  here's the code of its 
> map read*() methods:

Aha!  Ok, now I understand the sorts of situations you're talking
about.  By "not direct mapped", I thought you were talking about some
kind of access via address/data registers on some indirect bus
controller, rather than weird variations on endianness and
bit-swizzling.

Hrm.. this is a property of how the flash is wired onto the bus,
rather than of the flash chips themselves, so I'm not entirely sure
where description of it belongs.

Simplest option seems to me to add a property "endianness" or
"bit-swizzling" or something which can be defined to describe some odd
connections.  If absent we'd default to direct mapping.  Segher, is
that idea going to cause you to scream?

> >>>+     - reg : Address range of the flash chip
> >>>+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
> >>>+       times the number of interleaved chips.
> >>>+     - device-width : (optional) Width of a single flash chip.  If omitted,
> >>>+       assumed to be equal to 'bank-width'.
> 
> >>    Why then not just introduce the "interleave" prop and obsolete the
> >>"bank-width"?
> 
> > Because they're equivalent information, and bank-width is what the
> > code ends up actually caring about.  I don't see any reason to prefer
> > giving the interleave.
> 
>     Well, "device-width" is not the thing that we care about either. ;-)

Well, yes but we need to encode it somehow.  Segher preferred
device-width to interleave, because it's closer to a description of
the actual hardware, rather than an abstration decribing its wiring.

In other words I *still* don't see any reason to prefer giving the
interleave.

> >>>Index: working-2.6/drivers/mtd/maps/physmap_of.c
> >>>===================================================================
> >>>--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
> >>>+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
> [...]
> >>>+	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
> >>>+		const u32 *reg;
> >>>+		const char *name;
> >>>+		const void *ro;
> >>
> >>    We hardly need the above 2 variables.
> 
> > They're all used...
> 
>     I meant that there's no necessity in them.

By which you mean....?

> >>    Oh, I see that the new partition representation have really simplified
> >>parsing -- this function is hardly shorter than the old one... :-P
> 
> > They new format isn't supposed to be simpler to parse: it's supposed
> > to be more flexible if we ever need to add more per-partition
> > information than just the offset / size / read-only.
> 
>     Well, if we ever need that indeed... :-)
> 
> [...]
> 
> >>>@@ -221,6 +297,14 @@ err_out:
> >>> 
> >>> static struct of_device_id of_physmap_match[] = {
> >>> 	{
> >>>+		.compatible	= "cfi-flash",
> >>>+		.data		= (void *)"cfi_probe",
> >>>+	},
> >>>+	{
> >>>+		.compatible	= "jedec-flash",
> >>>+		.data		= (void *)"jedec_probe",
> >>>+	},
> >>>+	{
> 
> >>    This would also trigger on non-linearly mapped CFI or JEDEC
> >>flashes which is not a good idea -- however, as we're using the MTD
> >>probing code anyway, it will just fail, so it's not luckily is not a
> >>fatal design flaw.
> 
> > Well, if there's nothing else to distinguish them.  Which there isn't
> > yet, but will need to be: see above about incomplete - helpful
> > suggestions about how to describe the mapping would be more useful
> > than merely pointing out the lack.
> 
>     I was thinking about adding "direct-mapped" prop... but maybe adding 
> "ranges" to the parent node (that's "ebc") would indeed ensure that the flash 
> is mapped 1:1 to the EBC's parent bus also.

The ebc already has a ranges property.  But that can't describe the
sorts of bit-swizzling you're talking about.

> >>>Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
> >>>===================================================================
> >>>--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> >>>+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> 
> >>[...]
> 
> >>>@@ -158,14 +161,20 @@
> >>> 				};
> 
> >>> 				large-flash@2,0 {
> 
> >>    Hmm... what does @2,0 mean? :-O
> 
> > EBC chip select 2, offset 0.
> 
>     Well, so this node is under some kind of local bus node -- that's good.
> Didn't know that the spec allows commas after @...

Well, now you do.  I believe USB usually uses that format, too.

> >>>-					device_type = "rom";
> >>>-					compatible = "direct-mapped";
> >>>-					probe-type = "JEDEC";
> >>>+					compatible = "jedec-flash";
> >>> 					bank-width = <1>;
> >>>-					partitions = <0 380000
> >>>-						      380000 80000>;
> >>>-					partition-names = "fs", "firmware";
> >>>+//					partitions = <0 380000
> >>>+//						      380000 80000>;
> >>>+//					partition-names = "fs", "firmware";
> >>> 					reg = <2 0 400000>;
> >>>+					#address-cells = <1>;
> >>>+					#size-cells = <1>;
> 
> >>    Heh...
> 
> > Yeah, that bit's a bit ugly, I'll grant you.
> 
>     Don't we need "ranges" here, at least from the formal PoV -- as the parent 
> and child address spaces differ? I know the physmap_of parser doesn't care but 
> still...

That's one I've wondered about.  To describe the partitions address
space as lying (ultimately) in the physical address space, which in a
sense it does, yes we'd need a ranges property here.  But we also have
a 'reg' at the top level which would overlap with that hypothetical
ranges which would be weird.  Or we could exclude the top-level reg,
but then that's a pain if we do want to map the flash as a whole.

So I left out ranges, on the grounds that there isn't actually
anything at present which will attempt to access flash partitions
"generically" as a device tree device. 

I'm not sold on this approach, but I haven't heard you give a better
argument yet.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Fri, Aug 03, 2007 at 08:29:07PM +0400, Sergei Shtylyov wrote:
> David Gibson wrote:
> 
> > Duh, forgot to attach the actual patch.  Here it is:
> 
> > Index: working-2.6/Documentation/powerpc/booting-without-of.txt
> > ===================================================================
> > --- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> > +++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
> > @@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
> >  		};
> >  	};
> >  
> > -    j) Flash chip nodes
> > +   j) CFI or JEDEC memory-mapped NOR flash
> >  
> >      Flash chips (Memory Technology Devices) are often used for solid state
> >      file systems on embedded devices.
> >  
> > -    Required properties:
> > +     - compatible : should contain the specific model of flash chip(s) used
> > +       followed by either "cfi-flash" or "jedec-flash"
> 
>     Duh, have nearly forgotten to complain about "-flash" suffix.  Isn't it 
> superfluous?

For CFI, I guess so.  But don't JEDEC standardise other things as well
as flash?  I think "-flash" makes the description a bit more obvious,
but I'll be swayed if a few other people chime in with opinions on this.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

Hello.

David Gibson wrote:

>>>>>Index: working-2.6/Documentation/powerpc/booting-without-of.txt
>>>>>===================================================================
>>>>>--- working-2.6.orig/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
>>>>>+++ working-2.6/Documentation/powerpc/booting-without-of.txt	2007-07-30 17:07:14.000000000 +1000
>>>>>@@ -1757,45 +1757,23 @@ platforms are moved over to use the flat
>>>>>		};
>>>>>	};
>>>>>
>>>>>-    j) Flash chip nodes
>>>>>+   j) CFI or JEDEC memory-mapped NOR flash
>>>>>
>>>>>    Flash chips (Memory Technology Devices) are often used for solid state
>>>>>    file systems on embedded devices.
>>
>>>>>-    Required properties:
>>>>>+     - compatible : should contain the specific model of flash chip(s) used
>>>>>+       followed by either "cfi-flash" or "jedec-flash"

>>>>   This "compatible" prop (and the node in whole) doesn't say a
>>>>thing about how the flash is mapped into the CPU address space.  I
>>>>strongly disagree that this node provides enough info to select a
>>>>driver. :-/

>>>To be honest, I'm not sure that describing the mapping is really the
>>>job of the compatible property.  That the flash is mapped into the
>>>address space is kind of implicit in the way the reg interacts with
>>>the parents' ranges property.

>>    Ah, I keep forgetting about implied 1:1 parent/child address 
>>correspondence... :-<
>>    But does it really imply how the (low) address bits of the *child* bus 
>>("ebc" in this case) are connected to the chip?  I don't think so...

>>>Can you describe some of the options for *not* direct mapped flash
>>>chips - I can't reasonably come up with a way of describing the
>>>distinction when I've never seen NOR flash other than direct mapped.

>>    You're lucky to live in the single-endian world.  Once you're in bi-endian 
>>one, all kinds of strange mappings become possible.  I've seen the MIPS 

    Well, not necessarily -- 16-bit only accesses are always possibly (no 
dount this would be a strange mapping however)...

>>mapping driver which required swapping flash bytes in s/w in BE mode, i.e. 
>>couldn't be served by physmap, yet that's not all...  here's the code of its 
>>map read*() methods:

> Aha!  Ok, now I understand the sorts of situations you're talking
> about.  By "not direct mapped", I thought you were talking about some
> kind of access via address/data registers on some indirect bus
> controller, rather than weird variations on endianness and
> bit-swizzling.

    No, that would be just too ridiculous for a NOR flash -- I hope. :-)

> Hrm.. this is a property of how the flash is wired onto the bus,
> rather than of the flash chips themselves, 

    Indeed.

> so I'm not entirely sure where description of it belongs.

    So, you're saying that the 1:1 address correspondence rule stops to apply 
here?

> Simplest option seems to me to add a property "endianness" or

    And we even have a precedent of "big-endian" prop in the MPIC nodes 
(although I'm not sure why it's needed there).

> "bit-swizzling" or something which can be defined to describe some odd
> connections.  If absent we'd default to direct mapping.  Segher, is
> that idea going to cause you to scream?

    Actually, checking for the presence of all possible perverted mapping 
props doesn't seem a good idea -- it's simpler to check for the presence of 
one prop (like "direct-mapped" I was thinking about, or maybe "simple-map").

>>>>>+     - reg : Address range of the flash chip
>>>>>+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
>>>>>+       times the number of interleaved chips.
>>>>>+     - device-width : (optional) Width of a single flash chip.  If omitted,
>>>>>+       assumed to be equal to 'bank-width'.

>>>>   Why then not just introduce the "interleave" prop and obsolete the
>>>>"bank-width"?

>>>Because they're equivalent information, and bank-width is what the
>>>code ends up actually caring about.  I don't see any reason to prefer
>>>giving the interleave.

>>    Well, "device-width" is not the thing that we care about either. ;-)

> Well, yes but we need to encode it somehow.  Segher preferred
> device-width to interleave, because it's closer to a description of
> the actual hardware, rather than an abstration decribing its wiring.

> In other words I *still* don't see any reason to prefer giving the
> interleave.

    I wasn't talking of "interleave" preference over "device-width", I was 
talking about obsoleting "bank-width" with this pair.

>>>>>Index: working-2.6/drivers/mtd/maps/physmap_of.c
>>>>>===================================================================
>>>>>--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
>>>>>+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000

>>[...]

>>>>>+	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
>>>>>+		const u32 *reg;
>>>>>+		const char *name;
>>>>>+		const void *ro;

>>>>   We hardly need the above 2 variables.

>>>They're all used...

>>    I meant that there's no necessity in them.

> By which you mean....?

    They're only written to once, and then read immediately which might be 
easily collapsed into a single statement.

>>[...]

>>>>>@@ -221,6 +297,14 @@ err_out:
>>>>>
>>>>>static struct of_device_id of_physmap_match[] = {
>>>>>	{
>>>>>+		.compatible	= "cfi-flash",
>>>>>+		.data		= (void *)"cfi_probe",
>>>>>+	},
>>>>>+	{
>>>>>+		.compatible	= "jedec-flash",
>>>>>+		.data		= (void *)"jedec_probe",
>>>>>+	},
>>>>>+	{

>>>>   This would also trigger on non-linearly mapped CFI or JEDEC
>>>>flashes which is not a good idea -- however, as we're using the MTD
>>>>probing code anyway, it will just fail, so it's not luckily is not a
>>>>fatal design flaw.

>>>Well, if there's nothing else to distinguish them.  Which there isn't
>>>yet, but will need to be: see above about incomplete - helpful
>>>suggestions about how to describe the mapping would be more useful
>>>than merely pointing out the lack.

>>    I was thinking about adding "direct-mapped" prop... but maybe adding 
>>"ranges" to the parent node (that's "ebc") would indeed ensure that the flash 
>>is mapped 1:1 to the EBC's parent bus also.

> The ebc already has a ranges property.  But that can't describe the

    Not in the device tree that started that thread -- I haven't seen another.

> sorts of bit-swizzling you're talking about.

    Let's hear what Segher says (if he's not yet tired of all this :-).

>>>>>Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
>>>>>===================================================================
>>>>>--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
>>>>>+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000

>>>>[...]

>>>>>@@ -158,14 +161,20 @@
>>>>>				};

>>>>>				large-flash@2,0 {

>>>>   Hmm... what does @2,0 mean? :-O

>>>EBC chip select 2, offset 0.

>>    Well, so this node is under some kind of local bus node -- that's good.
>>Didn't know that the spec allows commas after @...

> Well, now you do.  I believe USB usually uses that format, too.

    USB what, hosts or devices?

>>>>>-					device_type = "rom";
>>>>>-					compatible = "direct-mapped";
>>>>>-					probe-type = "JEDEC";
>>>>>+					compatible = "jedec-flash";
>>>>>					bank-width = <1>;
>>>>>-					partitions = <0 380000
>>>>>-						      380000 80000>;
>>>>>-					partition-names = "fs", "firmware";
>>>>>+//					partitions = <0 380000
>>>>>+//						      380000 80000>;
>>>>>+//					partition-names = "fs", "firmware";
>>>>>					reg = <2 0 400000>;
>>>>>+					#address-cells = <1>;
>>>>>+					#size-cells = <1>;

>>>>   Heh...

>>>Yeah, that bit's a bit ugly, I'll grant you.

>>    Don't we need "ranges" here, at least from the formal PoV -- as the parent 
>>and child address spaces differ? I know the physmap_of parser doesn't care but 
>>still...

> That's one I've wondered about.  To describe the partitions address
> space as lying (ultimately) in the physical address space, which in a
> sense it does, yes we'd need a ranges property here.  But we also have
> a 'reg' at the top level which would overlap with that hypothetical
> ranges which would be weird.  Or we could exclude the top-level reg,
> but then that's a pain if we do want to map the flash as a whole.

    Hm, right... the option here would be to always have at least one 
partition and no "reg" property in the MTD node itself... or have "reg" with 
no partition and "ranges" if partitions are there... :-)

> So I left out ranges, on the grounds that there isn't actually
> anything at present which will attempt to access flash partitions
> "generically" as a device tree device. 

> I'm not sold on this approach, but I haven't heard you give a better
> argument yet.

   Well, that was mostly thoretic speculation...

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>> Yeah, better names please -- if possible, something that someone
>>> without knowledge of this SoC will understand what it is.
>>
>> I think the names are probably ok - I'm assuming they're in keeping
>> with the convention I've used of using the same names / abbreviations
>> as in the CPU user manual.  I'm asking just for my own information,
>> although a comment might not be a bad idea.

Fine with me -- I personally prefer "system-device-controller"
and "clock-power-controller" or similar, but that is mostly a
matter of taste.  As long as it's human readable it's fine.

> -    Required properties:
> +     - compatible : should contain the specific model of flash 
> chip(s) used

"if known".

> +       followed by either "cfi-flash" or "jedec-flash"


> +    Flash partitions
> +     - reg :
> +     - read-only : (optional)

I'll hold off commenting on this until you've finish writing it,
you probably know my opinion about it anyway :-)

One thing though -- what _exactly_ does "read-only" signify?


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> +     - compatible : should contain the specific model of flash 
>> chip(s) used
>> +       followed by either "cfi-flash" or "jedec-flash"
>
>    This "compatible" prop (and the node in whole) doesn't say a thing 
> about how the flash is mapped into the CPU address space.

...and it shouldn't.  That's what "ranges" properties in all
the various (grand-)parents of the node are for.

> I strongly disagree that this node provides enough info to select a 
> driver. :-/

If Linux needs more information than what the device _is_, but
also needs information about how it is _used_ on some particular
hardware, to select a driver; then it can bloody well do so.
Nowhere is it said that an OS can _only_ use "compatible" properties
to do its driver selection.

>> +     - reg : Address range of the flash chip
>> +     - bank-width : Width (in bytes) of the flash bank.  Equal to 
>> the device width
>> +       times the number of interleaved chips.
>> +     - device-width : (optional) Width of a single flash chip.  If 
>> omitted,
>> +       assumed to be equal to 'bank-width'.
>
>    Why then not just introduce the "interleave" prop and obsolete the
> "bank-width"?

Because "interleave" is overly complicated and still doesn't handle
all cases.  Also, it's a more confusing name.

The goal here is to handle 98% (or just 90% even) of all cases in
as simple a way as possible; everything else can get special handling
later.

>> +    Flash partitions
>> +     - reg :
>> +     - read-only : (optional)
>
>    All that would look nice but partition is even less of a device 
> than the
> original "rom" node was... well, who cares now? :-)

Some partitions can be useful for the firmware itself, or for
early boot stages; those should be described in the device
tree in some way.  And yes, you certainly can consider an
(aligned) flash partition to be a subdevice of the whole flash
bank.

>    Oh, I see that the new partition representation have really 
> simplified
> parsing -- this function is hardly shorter than the old one... :-P

Neither simplifying machine-parsing nor compacting the kernel
code are a goal at all, I don't see why you bring this up.

>>   static struct of_device_id of_physmap_match[] = {
>>  	{
>> +		.compatible	= "cfi-flash",
>> +		.data		= (void *)"cfi_probe",
>> +	},
>> +	{
>> +		.compatible	= "jedec-flash",
>> +		.data		= (void *)"jedec_probe",
>> +	},
>> +	{
>
>    This would also trigger on non-linearly mapped CFI or JEDEC flashes

No, it wouldn't.

>>   				large-flash@2,0 {
>
>    Hmm... what does @2,0 mean? :-O
>
>>  					reg = <2 0 400000>;

"2,0" is the text representation for the unit address <2 0>
on this bus.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>> +	UIC0: interrupt-controller0 {
>>> +		compatible = "ibm,uic-440gp","ibm,uic";
>>
>> The first compatible entry should always be the precise model, so in
>> this case "ibm,uic-440epx".  If it is (supposed to be) identical to
>> the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
>> since I believe all the UICs are supposed to operate the same, I think
>> that's implicit in the "ibm,uic" entry.
>
> Most UICs are the same.  There are some oddball chips that either hide
> particular registers because they are unused, or they change the
> addressing stride.  I'm not sure that is a common enough case to worry
> about now though.

You only need to worry about the oddball cases in the device
trees for a device that uses one off those.

It is prudent to put the exact name of the device you're
working with in there whenever possible, in case you later
discover it has some quirks.  If that doesn't happen, the
kernel can happily keep probing on the more generic name.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>> +     - compatible : should contain the specific model of flash 
>>> chip(s) used
>>> +       followed by either "cfi-flash" or "jedec-flash"
>>
>>     This "compatible" prop (and the node in whole) doesn't say a
>> thing about how the flash is mapped into the CPU address space.  I
>> strongly disagree that this node provides enough info to select a
>> driver. :-/
>
> To be honest, I'm not sure that describing the mapping is really the
> job of the compatible property.  That the flash is mapped into the
> address space is kind of implicit in the way the reg interacts with
> the parents' ranges property.

Exactly.

> Can you describe some of the options for *not* direct mapped flash
> chips - I can't reasonably come up with a way of describing the
> distinction when I've never seen NOR flash other than direct mapped.

What, you've never seen SPI flash or IIC flash?  :-)

Not that these are handled by the MTD framework AFAIK.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> To be honest, I'm not sure that describing the mapping is really the
>> job of the compatible property.  That the flash is mapped into the
>> address space is kind of implicit in the way the reg interacts with
>> the parents' ranges property.
>
>     Ah, I keep forgetting about implied 1:1 parent/child address
> correspondence... :-<

It's not implied, it is explicitly specified.

>     But does it really imply how the (low) address bits of the *child* 
> bus
> ("ebc" in this case) are connected to the chip?  I don't think so...

The currently proposed binding assumes linear mapping.

Strange setups will need to extend this binding; this
does mean that the burden for that is not on the "normal"
case.

There are so many weird ways in which people wire up memory
devices that there is no chance to define a "generic" binding
that works for all in less than 400 pages of documentation.

>     Well, "device-width" is not the thing that we care about either. 
> ;-)

You need to know the device-width to drive the chip (and no,
it isn't enough to know the exact chip model even) -- why do
you say you don't care about it?

>>>    Hmm... what does @2,0 mean? :-O
>
>> EBC chip select 2, offset 0.
>
>     Well, so this node is under some kind of local bus node -- that's 
> good.
> Didn't know that the spec allows commas after @...

Most characters are allowed in the unit-address...  The following
is just fine: "my-secret-base@the-moon".  ISA uses letters to
distinguish between its different address spaces, for example.

There is a direct correspondence between the first "reg"
address and the text representation of the unit address;
this correspondence is bus-dependent however.

David, can multiple devices sit on the same chip-select
on EBC, or on the same "minor" address?  If not, you can
simplify your unit address representation.

>> "direct-mapped" is simply not a sufficiently specific compatible
>> property, no two ways about it.
>
>     Yes, for example "direct-mapped-cfi" and "direct-mapped-jedec" 
> would have
> been better...

Nah.  These memory devices are meant to sit at some address/data
bus; whether it is direct-mapped or not is obvious from the node
hierarchy the flash node hangs under already; let's make the simple
case simple and the complicated cases possible.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> +     - compatible : should contain the specific model of flash 
>> chip(s) used
>> +       followed by either "cfi-flash" or "jedec-flash"
>
>    Duh, have nearly forgotten to complain about "-flash" suffix.  
> Isn't it superfluous?

No, it describes what kind of thing this is.  "cfi" and "jedec"
don't say much: "cfi" could equally well mean the "cute friendly imp",
and who knows what kind of JEDEC-interface device this is supposed
to indicate.  "cfi-flash" and "jedec-flash" on the other hand are
sufficiently clear, even if they aren't completely specific; this
is a very common device so some leeway is in order.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

> Aha!  Ok, now I understand the sorts of situations you're talking
> about.  By "not direct mapped", I thought you were talking about some
> kind of access via address/data registers on some indirect bus
> controller, rather than weird variations on endianness and
> bit-swizzling.
>
> Hrm.. this is a property of how the flash is wired onto the bus,
> rather than of the flash chips themselves, so I'm not entirely sure
> where description of it belongs.
>
> Simplest option seems to me to add a property "endianness" or
> "bit-swizzling" or something which can be defined to describe some odd
> connections.  If absent we'd default to direct mapping.  Segher, is
> that idea going to cause you to scream?

No, that's fine with me.  I would recommend either using a
_good_ _descriptive_ name for such a property describing the
swizzling, if this swizzling is common; or just put the whole
bloody weirdo address permutation into some nice big array,
something like

address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;

>>     Well, "device-width" is not the thing that we care about either. 
>> ;-)
>
> Well, yes but we need to encode it somehow.  Segher preferred
> device-width to interleave, because it's closer to a description of
> the actual hardware, rather than an abstration decribing its wiring.
>
> In other words I *still* don't see any reason to prefer giving the
> interleave.

You *need* to know the device width.  You *need* to know
the bank width.  And for 99% of the cases you don't need
to know anything more since most hardware designs aren't
insane.  Case closed I'd say.


>> Didn't know that the spec allows commas after @...
>
> Well, now you do.  I believe USB usually uses that format, too.

PCI too, and it even has an official binding :-)

>>     Don't we need "ranges" here, at least from the formal PoV -- as 
>> the parent
>> and child address spaces differ? I know the physmap_of parser doesn't 
>> care but
>> still...
>
> That's one I've wondered about.  To describe the partitions address
> space as lying (ultimately) in the physical address space, which in a
> sense it does, yes we'd need a ranges property here.  But we also have
> a 'reg' at the top level which would overlap with that hypothetical
> ranges which would be weird.  Or we could exclude the top-level reg,
> but then that's a pain if we do want to map the flash as a whole.
>
> So I left out ranges, on the grounds that there isn't actually
> anything at present which will attempt to access flash partitions
> "generically" as a device tree device.

It looks good to me like this.

In a real OF, the "register" access for the flash partition
node would be handled by its parent node, which would know
to do the direct-mapping thing (at least mapping it to _its_
parent, which typically asks the nodes further up, etc.)

For the kernel world, we should just document it in the binding.

> I'm not sold on this approach, but I haven't heard you give a better
> argument yet.

I haven't heard or thought of anything better either.  Using "ranges"
is conceptually wrong, even ignoring the technical problems that come
with it.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>> +     - compatible : should contain the specific model of flash 
>>> chip(s) used
>>> +       followed by either "cfi-flash" or "jedec-flash"
>>
>>     Duh, have nearly forgotten to complain about "-flash" suffix.  
>> Isn't it
>> superfluous?
>
> For CFI, I guess so.  But don't JEDEC standardise other things as well
> as flash?  I think "-flash" makes the description a bit more obvious,
> but I'll be swayed if a few other people chime in with opinions on 
> this.

How about I'll just veto making the names any shorter.  Problem
solved :-)


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> "bit-swizzling" or something which can be defined to describe some odd
>> connections.  If absent we'd default to direct mapping.  Segher, is
>> that idea going to cause you to scream?
>
>     Actually, checking for the presence of all possible perverted 
> mapping
> props doesn't seem a good idea -- it's simpler to check for the 
> presence of
> one prop (like "direct-mapped" I was thinking about, or maybe 
> "simple-map").

There shouldn't be many "perverted" properties -- really weird
mappings can just do a special "compatible" value, the standard
kernel driver can't handle them either, anyway.

A few extra property lookups are dirt cheap.  This isn't
speed-critical code anyway.

>> Well, now you do.  I believe USB usually uses that format, too.
>
>     USB what, hosts or devices?

The unit-address of a device is specified in its parent's
address space.  USB hosts don't typically have a USB bus as
parent ;-)  (but they can still have a comma in the unit
address.  Confused?  Don't be :-) ).

>     Hm, right... the option here would be to always have at least one
> partition and no "reg" property in the MTD node itself... or have 
> "reg" with
> no partition and "ranges" if partitions are there... :-)

No way.  "reg" is needed *no matter what* -- I'll not list the
other problems with that proposal.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Benjamin Herrenschmidt]

On Mon, 2007-08-06 at 22:37 +0400, Sergei Shtylyov wrote:
> 
>     Actually, checking for the presence of all possible perverted
> mapping 
> props doesn't seem a good idea -- it's simpler to check for the
> presence of 
> one prop (like "direct-mapped" I was thinking about, or maybe
> "simple-map").

Or more simply... if it's not a direct mapping, it doesn't have a ranges
property and can't be walked down by conventional means.

Ben.

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>     Actually, checking for the presence of all possible perverted
>> mapping
>> props doesn't seem a good idea -- it's simpler to check for the
>> presence of
>> one prop (like "direct-mapped" I was thinking about, or maybe
>> "simple-map").
>
> Or more simply... if it's not a direct mapping, it doesn't have a 
> ranges
> property and can't be walked down by conventional means.

That establishes that the flash address space is mapped 1-1 into
the host address space; however it doesn't say anything about
swizzling or interleaving or endianness etc., which are important
for NOR flash memory because of how the command sets work.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 10:37:14PM +0400, Sergei Shtylyov wrote:
> Hello.

[snip]
> >>>Can you describe some of the options for *not* direct mapped flash
> >>>chips - I can't reasonably come up with a way of describing the
> >>>distinction when I've never seen NOR flash other than direct mapped.
> 
> >>    You're lucky to live in the single-endian world.  Once you're in bi-endian 
> >>one, all kinds of strange mappings become possible.  I've seen the MIPS 
> 
>     Well, not necessarily -- 16-bit only accesses are always possibly (no 
> dount this would be a strange mapping however)...

I have no idea what you're getting at here.

> >>mapping driver which required swapping flash bytes in s/w in BE mode, i.e. 
> >>couldn't be served by physmap, yet that's not all...  here's the code of its 
> >>map read*() methods:
> 
> > Aha!  Ok, now I understand the sorts of situations you're talking
> > about.  By "not direct mapped", I thought you were talking about some
> > kind of access via address/data registers on some indirect bus
> > controller, rather than weird variations on endianness and
> > bit-swizzling.
> 
>     No, that would be just too ridiculous for a NOR flash -- I hope. :-)

Heh.  In my experience, very little is so ridiculous that some
embedded vendor won't do it.

> > Hrm.. this is a property of how the flash is wired onto the bus,
> > rather than of the flash chips themselves, 
> 
>     Indeed.
> 
> > so I'm not entirely sure where description of it belongs.
> 
>     So, you're saying that the 1:1 address correspondence rule stops to apply 
> here?

Well.. it all depends what exactly you consider the address space of
the flash bank.  By which I mean the whole shmozzle represented by the
device node, not the individual flash chips.  It's not immediately
obvious whether or not that should include any swizzling done by the
bus wiring.

It would be possible, I guess, to define a 'swizzled-ranges' property
or something which allows child devices to be embedded in the parent's
address range in a not-direct way.  However, the swizzling on the
flash bank is really a property of the flash bank, not of the parent
bus - requiring it to be encoded in the parent is pretty yucky -
especially if the flash bank is just part of a larger chunk of bus
address space, defined by a single large ranges entry in the parent.

> > Simplest option seems to me to add a property "endianness" or
> 
>     And we even have a precedent of "big-endian" prop in the MPIC nodes 
> (although I'm not sure why it's needed there).
> 
> > "bit-swizzling" or something which can be defined to describe some odd
> > connections.  If absent we'd default to direct mapping.  Segher, is
> > that idea going to cause you to scream?
> 
>     Actually, checking for the presence of all possible perverted mapping 
> props doesn't seem a good idea -- it's simpler to check for the presence of 
> one prop (like "direct-mapped" I was thinking about, or maybe "simple-map").
> 
> >>>>>+     - reg : Address range of the flash chip
> >>>>>+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
> >>>>>+       times the number of interleaved chips.
> >>>>>+     - device-width : (optional) Width of a single flash chip.  If omitted,
> >>>>>+       assumed to be equal to 'bank-width'.
> 
> >>>>   Why then not just introduce the "interleave" prop and obsolete the
> >>>>"bank-width"?
> 
> >>>Because they're equivalent information, and bank-width is what the
> >>>code ends up actually caring about.  I don't see any reason to prefer
> >>>giving the interleave.
> 
> >>    Well, "device-width" is not the thing that we care about either. ;-)
> 
> > Well, yes but we need to encode it somehow.  Segher preferred
> > device-width to interleave, because it's closer to a description of
> > the actual hardware, rather than an abstration decribing its wiring.
> 
> > In other words I *still* don't see any reason to prefer giving the
> > interleave.
> 
>     I wasn't talking of "interleave" preference over "device-width", I was 
> talking about obsoleting "bank-width" with this pair.

Whatever.  You haven't given any argument to prefer interleave over
either device-width or bank-width.

> >>>>>Index: working-2.6/drivers/mtd/maps/physmap_of.c
> >>>>>===================================================================
> >>>>>--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
> >>>>>+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000
> 
> >>[...]
> 
> >>>>>+	for (pp = dp->child, i = 0 ; pp; pp = pp->sibling, i++) {
> >>>>>+		const u32 *reg;
> >>>>>+		const char *name;
> >>>>>+		const void *ro;
> 
> >>>>   We hardly need the above 2 variables.
> 
> >>>They're all used...
> 
> >>    I meant that there's no necessity in them.
> 
> > By which you mean....?
> 
>     They're only written to once, and then read immediately which might be 
> easily collapsed into a single statement.

Ah, yes, ok.  Changed.

> >>[...]
> 
> >>>>>@@ -221,6 +297,14 @@ err_out:
> >>>>>
> >>>>>static struct of_device_id of_physmap_match[] = {
> >>>>>	{
> >>>>>+		.compatible	= "cfi-flash",
> >>>>>+		.data		= (void *)"cfi_probe",
> >>>>>+	},
> >>>>>+	{
> >>>>>+		.compatible	= "jedec-flash",
> >>>>>+		.data		= (void *)"jedec_probe",
> >>>>>+	},
> >>>>>+	{
> 
> >>>>   This would also trigger on non-linearly mapped CFI or JEDEC
> >>>>flashes which is not a good idea -- however, as we're using the MTD
> >>>>probing code anyway, it will just fail, so it's not luckily is not a
> >>>>fatal design flaw.
> 
> >>>Well, if there's nothing else to distinguish them.  Which there isn't
> >>>yet, but will need to be: see above about incomplete - helpful
> >>>suggestions about how to describe the mapping would be more useful
> >>>than merely pointing out the lack.
> 
> >>    I was thinking about adding "direct-mapped" prop... but maybe adding 
> >>"ranges" to the parent node (that's "ebc") would indeed ensure that the flash 
> >>is mapped 1:1 to the EBC's parent bus also.
> 
> > The ebc already has a ranges property.  But that can't describe the
> 
>     Not in the device tree that started that thread -- I haven't seen another.

Ah sorry.  The ranges property isn't in the dts, it's added by the
bootwrapper based on the EBC register contents.

> > sorts of bit-swizzling you're talking about.
> 
>     Let's hear what Segher says (if he's not yet tired of all this :-).
> 
> >>>>>Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
> >>>>>===================================================================
> >>>>>--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> >>>>>+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
> 
> >>>>[...]
> 
> >>>>>@@ -158,14 +161,20 @@
> >>>>>				};
> 
> >>>>>				large-flash@2,0 {
> 
> >>>>   Hmm... what does @2,0 mean? :-O
> 
> >>>EBC chip select 2, offset 0.
> 
> >>    Well, so this node is under some kind of local bus node -- that's good.
> >>Didn't know that the spec allows commas after @...
> 
> > Well, now you do.  I believe USB usually uses that format, too.
> 
>     USB what, hosts or devices?

Devices.

> >>>>>-					device_type = "rom";
> >>>>>-					compatible = "direct-mapped";
> >>>>>-					probe-type = "JEDEC";
> >>>>>+					compatible = "jedec-flash";
> >>>>>					bank-width = <1>;
> >>>>>-					partitions = <0 380000
> >>>>>-						      380000 80000>;
> >>>>>-					partition-names = "fs", "firmware";
> >>>>>+//					partitions = <0 380000
> >>>>>+//						      380000 80000>;
> >>>>>+//					partition-names = "fs", "firmware";
> >>>>>					reg = <2 0 400000>;
> >>>>>+					#address-cells = <1>;
> >>>>>+					#size-cells = <1>;
> 
> >>>>   Heh...
> 
> >>>Yeah, that bit's a bit ugly, I'll grant you.
> 
> >>    Don't we need "ranges" here, at least from the formal PoV -- as the parent 
> >>and child address spaces differ? I know the physmap_of parser doesn't care but 
> >>still...
> 
> > That's one I've wondered about.  To describe the partitions address
> > space as lying (ultimately) in the physical address space, which in a
> > sense it does, yes we'd need a ranges property here.  But we also have
> > a 'reg' at the top level which would overlap with that hypothetical
> > ranges which would be weird.  Or we could exclude the top-level reg,
> > but then that's a pain if we do want to map the flash as a whole.
> 
>     Hm, right... the option here would be to always have at least one 
> partition and no "reg" property in the MTD node itself... or have "reg" with 
> no partition and "ranges" if partitions are there... :-)

Eck.

> > So I left out ranges, on the grounds that there isn't actually
> > anything at present which will attempt to access flash partitions
> > "generically" as a device tree device. 
> 
> > I'm not sold on this approach, but I haven't heard you give a better
> > argument yet.
> 
>    Well, that was mostly thoretic speculation...
> 
> WBR, Sergei
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Tue, Aug 07, 2007 at 08:15:23AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-08-06 at 22:37 +0400, Sergei Shtylyov wrote:
> > 
> >     Actually, checking for the presence of all possible perverted
> > mapping 
> > props doesn't seem a good idea -- it's simpler to check for the
> > presence of 
> > one prop (like "direct-mapped" I was thinking about, or maybe
> > "simple-map").
> 
> Or more simply... if it's not a direct mapping, it doesn't have a ranges
> property and can't be walked down by conventional means.

But the ranges property is in the parent.  And if the flash bank is
only one of several devices in the same address space, it might be
messy to exclude the flash range from the parent's ranges property.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 10:20:01PM +0200, Segher Boessenkool wrote:
> >>> +     - compatible : should contain the specific model of flash 
> >>> chip(s) used
> >>> +       followed by either "cfi-flash" or "jedec-flash"
> >>
> >>     This "compatible" prop (and the node in whole) doesn't say a
> >> thing about how the flash is mapped into the CPU address space.  I
> >> strongly disagree that this node provides enough info to select a
> >> driver. :-/
> >
> > To be honest, I'm not sure that describing the mapping is really the
> > job of the compatible property.  That the flash is mapped into the
> > address space is kind of implicit in the way the reg interacts with
> > the parents' ranges property.
> 
> Exactly.
> 
> > Can you describe some of the options for *not* direct mapped flash
> > chips - I can't reasonably come up with a way of describing the
> > distinction when I've never seen NOR flash other than direct mapped.
> 
> What, you've never seen SPI flash or IIC flash?  :-)

Nope.  Colour me ignorant :-p.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 09:59:24PM +0200, Segher Boessenkool wrote:
> >>> Yeah, better names please -- if possible, something that someone
> >>> without knowledge of this SoC will understand what it is.
> >>
> >> I think the names are probably ok - I'm assuming they're in keeping
> >> with the convention I've used of using the same names / abbreviations
> >> as in the CPU user manual.  I'm asking just for my own information,
> >> although a comment might not be a bad idea.
> 
> Fine with me -- I personally prefer "system-device-controller"
> and "clock-power-controller" or similar, but that is mostly a
> matter of taste.  As long as it's human readable it's fine.

Actually, it occurs to me that I've only sometimes been using that
convention for the names:  basically just for the weirdo chip control
devices that don't have a more widespread generic name.  I *have* been
strictly keeping to that convention for the labels in the dts (which
is why the PLB<->OPB bridge node is labelled POB rather than OPB, for
example).

> > -    Required properties:
> > +     - compatible : should contain the specific model of flash 
> > chip(s) used
> 
> "if known".

Added.

> > +       followed by either "cfi-flash" or "jedec-flash"
> 
> 
> > +    Flash partitions
> > +     - reg :
> > +     - read-only : (optional)
> 
> I'll hold off commenting on this until you've finish writing it,
> you probably know my opinion about it anyway :-)

Heh.. actually I was kind of hoping for your input on what's still
missing.  For example, I don't know what the necessary extra
properties for JEDEC chips are.

> One thing though -- what _exactly_ does "read-only" signify?

That's... a good question.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 10:35:57PM +0200, Segher Boessenkool wrote:
> >> To be honest, I'm not sure that describing the mapping is really the
> >> job of the compatible property.  That the flash is mapped into the
> >> address space is kind of implicit in the way the reg interacts with
> >> the parents' ranges property.
> >
> >     Ah, I keep forgetting about implied 1:1 parent/child address
> > correspondence... :-<
> 
> It's not implied, it is explicitly specified.
> 
> >     But does it really imply how the (low) address bits of the *child* 
> > bus
> > ("ebc" in this case) are connected to the chip?  I don't think so...
> 
> The currently proposed binding assumes linear mapping.
> 
> Strange setups will need to extend this binding; this
> does mean that the burden for that is not on the "normal"
> case.
> 
> There are so many weird ways in which people wire up memory
> devices that there is no chance to define a "generic" binding
> that works for all in less than 400 pages of documentation.
> 
> >     Well, "device-width" is not the thing that we care about either. 
> > ;-)
> 
> You need to know the device-width to drive the chip (and no,
> it isn't enough to know the exact chip model even) -- why do
> you say you don't care about it?
> 
> >>>    Hmm... what does @2,0 mean? :-O
> >
> >> EBC chip select 2, offset 0.
> >
> >     Well, so this node is under some kind of local bus node -- that's 
> > good.
> > Didn't know that the spec allows commas after @...
> 
> Most characters are allowed in the unit-address...  The following
> is just fine: "my-secret-base@the-moon".  ISA uses letters to
> distinguish between its different address spaces, for example.

Yeah, I should probably make dtc a bit more flexible about accepting
that, too.  At present, it only takes hex digits and ',', since those
are the common character.

> There is a direct correspondence between the first "reg"
> address and the text representation of the unit address;
> this correspondence is bus-dependent however.
> 
> David, can multiple devices sit on the same chip-select
> on EBC, or on the same "minor" address?  If not, you can
> simplify your unit address representation.

As far as I know, multiple devices could sit on the same chip select:
provided there was enough address decoding logic in or around the
devices, and that there existing bus timing parameters which would
work with all the devices on a chip select (or "bank" in the
terminology of the EBC bridge documentation).

Devices on different banks can certainly have the same address/offset
within the bank - e.g. on Ebony most of the devices are at offset 0.
The OPB address range for each bank is separately programmable in the
EBC bridge DCRs.

(Incidentally, this is why I created the binding in this way, rather
than just using the firmware established addresses in OPB space, which
are usually fixed for a particular board/platform.  This way provides
enough information that, if necessary, the kernel or another client
can reprogram the EBC from scratch to access the various devices
present.  Well.. actually fully reprogramming would also need the the
bus timing parameters, which I was thinking of adding information
before, but I haven't gotten to it yet.)

> >> "direct-mapped" is simply not a sufficiently specific compatible
> >> property, no two ways about it.
> >
> >     Yes, for example "direct-mapped-cfi" and "direct-mapped-jedec" 
> > would have
> > been better...
> 
> Nah.  These memory devices are meant to sit at some address/data
> bus; whether it is direct-mapped or not is obvious from the node
> hierarchy the flash node hangs under already; let's make the simple
> case simple and the complicated cases possible.
> 
> 
> Segher
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 10:15:39PM +0200, Segher Boessenkool wrote:
> >>> +	UIC0: interrupt-controller0 {
> >>> +		compatible = "ibm,uic-440gp","ibm,uic";
> >>
> >> The first compatible entry should always be the precise model, so in
> >> this case "ibm,uic-440epx".  If it is (supposed to be) identical to
> >> the UIC in the 440GP, it can also have an "ibm,uic-440gp" entry, but
> >> since I believe all the UICs are supposed to operate the same, I think
> >> that's implicit in the "ibm,uic" entry.
> >
> > Most UICs are the same.  There are some oddball chips that either hide
> > particular registers because they are unused, or they change the
> > addressing stride.  I'm not sure that is a common enough case to worry
> > about now though.
> 
> You only need to worry about the oddball cases in the device
> trees for a device that uses one off those.
> 
> It is prudent to put the exact name of the device you're
> working with in there whenever possible, in case you later
> discover it has some quirks.  If that doesn't happen, the
> kernel can happily keep probing on the more generic name.

Right.  In that case I suggest the sequoia just specify:
	compatible = "ibm,uic-440epx", "ibm,uic";
Because we have no particular reason to think that the 440EPx version
of UIC is any more similar to the 440GP version of UIC than it is to
the "normal" UIC.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Mon, Aug 06, 2007 at 10:54:33PM +0200, Segher Boessenkool wrote:
> > Aha!  Ok, now I understand the sorts of situations you're talking
> > about.  By "not direct mapped", I thought you were talking about some
> > kind of access via address/data registers on some indirect bus
> > controller, rather than weird variations on endianness and
> > bit-swizzling.
> >
> > Hrm.. this is a property of how the flash is wired onto the bus,
> > rather than of the flash chips themselves, so I'm not entirely sure
> > where description of it belongs.
> >
> > Simplest option seems to me to add a property "endianness" or
> > "bit-swizzling" or something which can be defined to describe some odd
> > connections.  If absent we'd default to direct mapping.  Segher, is
> > that idea going to cause you to scream?
> 
> No, that's fine with me.  I would recommend either using a
> _good_ _descriptive_ name for such a property describing the
> swizzling, if this swizzling is common; or just put the whole
> bloody weirdo address permutation into some nice big array,
> something like
> 
> address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;

Yes, I was contemplating something like that.

[snip]
> > So I left out ranges, on the grounds that there isn't actually
> > anything at present which will attempt to access flash partitions
> > "generically" as a device tree device.
> 
> It looks good to me like this.
> 
> In a real OF, the "register" access for the flash partition
> node would be handled by its parent node, which would know
> to do the direct-mapping thing (at least mapping it to _its_
> parent, which typically asks the nodes further up, etc.)
> 
> For the kernel world, we should just document it in the binding.
> 
> > I'm not sold on this approach, but I haven't heard you give a better
> > argument yet.
> 
> I haven't heard or thought of anything better either.  Using "ranges"
> is conceptually wrong, even ignoring the technical problems that come
> with it.

Why is "ranges" conceptually wrong?

To be honest this looks rather to me like another case where having
overlapping 'reg' and 'ranges' would actually make sense.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Scott Wood]

On Tue, Aug 07, 2007 at 01:28:06PM +1000, David Gibson wrote:
> It would be possible, I guess, to define a 'swizzled-ranges' property
> or something which allows child devices to be embedded in the parent's
> address range in a not-direct way.  However, the swizzling on the
> flash bank is really a property of the flash bank, not of the parent
> bus - requiring it to be encoded in the parent is pretty yucky -
> especially if the flash bank is just part of a larger chunk of bus
> address space, defined by a single large ranges entry in the parent.

It's more a property of the connection between the bus and the flash
chips, and that connection could be described as its own "bus" node,
something like:

localbus {
	#address-cells = <1>;
	#size-cells = <1>;
	ranges;

	random-sane-device@ff000000 {
		reg = <ff000000 800000>;
		...
	};
	
	freaky-swizzle-bus@ff800000 {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "with-enough-lsd";
		swizzle-bytes = <4>;
		swizzle-ranges = <0 ff800000 00800000 2 3 0 1>;
		
		flash@0 {
			compatible = "cfi-flash";
			reg = <0 800000>;
			bank-width = <4>;
			device-width = <2>;
		};
	};
};

Similar intermediary buses could be used for flashes with indirect
access (SPI and such).

-Scott

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>>>> Yeah, better names please -- if possible, something that someone
>>>>> without knowledge of this SoC will understand what it is.
>>>>
>>>> I think the names are probably ok - I'm assuming they're in keeping
>>>> with the convention I've used of using the same names / 
>>>> abbreviations
>>>> as in the CPU user manual.  I'm asking just for my own information,
>>>> although a comment might not be a bad idea.
>>
>> Fine with me -- I personally prefer "system-device-controller"
>> and "clock-power-controller" or similar, but that is mostly a
>> matter of taste.  As long as it's human readable it's fine.
>
> Actually, it occurs to me that I've only sometimes been using that
> convention for the names:  basically just for the weirdo chip control
> devices that don't have a more widespread generic name.

It's pretty darn hard to make good sensible "generic names" for
some of the devices on a SoC; using the acronym that's in the
documentation for that SoC certainly isn't the worst choice.

>>> +    Flash partitions
>>> +     - reg :
>>> +     - read-only : (optional)
>>
>> I'll hold off commenting on this until you've finish writing it,
>> you probably know my opinion about it anyway :-)
>
> Heh.. actually I was kind of hoping for your input on what's still
> missing.  For example, I don't know what the necessary extra
> properties for JEDEC chips are.

I meant for the "partitions" stuff only.

For the JEDEC chips, we need a "vendor-id" and "device-id"
property at least (or similar names -- whatever is general
practice for this); both are a single byte, encoded as with
encode-int.

>> One thing though -- what _exactly_ does "read-only" signify?
>
> That's... a good question.

Yeah.  It seems to me that the way it is currently used is
pure policy enforcement, which doesn't belong in the device
tree.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>> Aha!  Ok, now I understand the sorts of situations you're talking
>>> about.  By "not direct mapped", I thought you were talking about some
>>> kind of access via address/data registers on some indirect bus
>>> controller, rather than weird variations on endianness and
>>> bit-swizzling.
>>
>>     No, that would be just too ridiculous for a NOR flash -- I hope. 
>> :-)
>
> Heh.  In my experience, very little is so ridiculous that some
> embedded vendor won't do it.

True -- but if you can't map the NOR into the CPU address space,
there are cheaper alternatives.  Most embedded vendors care about
that, too ;-)

>>     So, you're saying that the 1:1 address correspondence rule stops 
>> to apply
>> here?
>
> Well.. it all depends what exactly you consider the address space of
> the flash bank.  By which I mean the whole shmozzle represented by the
> device node, not the individual flash chips.  It's not immediately
> obvious whether or not that should include any swizzling done by the
> bus wiring.

The parent device/bus shouldn't care, from its viewpoint the flash
bank is just one linear hunk of address space.  For reading or
writing the flash bank appears linear to the CPU as well (at least
that's the only thing our current proposed binding supports); the
only thing that gets "interesting" is sending commands to the flash
chips.

> It would be possible, I guess, to define a 'swizzled-ranges' property
> or something which allows child devices to be embedded in the parent's
> address range in a not-direct way.

Let's not even consider this please.

> However, the swizzling on the
> flash bank is really a property of the flash bank,

Yeah, it's the "fine structure" of the flash bank.  Something
only the flash driver has to deal with.  So no contaminating the
parent device node, thank you.

>>> Simplest option seems to me to add a property "endianness" or
>>
>>     And we even have a precedent of "big-endian" prop in the MPIC 
>> nodes
>> (although I'm not sure why it's needed there).

A single register read (32-bit registers) gives a big-endian value
on some MPIC implementations, and wrong-endian on others.  The
device driver really needs to know -- it really should just figure
it out from the "compatible" property though.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;
>
> Yes, I was contemplating something like that.

Let's not define this until we need it though :-)

>> I haven't heard or thought of anything better either.  Using "ranges"
>> is conceptually wrong, even ignoring the technical problems that come
>> with it.
>
> Why is "ranges" conceptually wrong?

The flash partitions aren't separate devices sitting on a
"flash bus", they are "sub-devices" of their parent.

> To be honest this looks rather to me like another case where having
> overlapping 'reg' and 'ranges' would actually make sense.

It never makes sense.  You should give the "master" device
the full "reg" range it covers, and have it define its own
address space; "sub-devices" can carve out their little hunk
from that.  You don't want more than one device owning the
same address range in the same address space.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> Most characters are allowed in the unit-address...  The following
>> is just fine: "my-secret-base@the-moon".  ISA uses letters to
>> distinguish between its different address spaces, for example.
>
> Yeah, I should probably make dtc a bit more flexible about accepting
> that, too.  At present, it only takes hex digits and ',', since those
> are the common character.

Sounds good.  And then the legacy ISA devices in existing
DTS files should be changed to say @i60 instead of @60, etc.
(@60 is correct since the default is legacy I/O space, but
it's good the be more verbose in those cases).

>> David, can multiple devices sit on the same chip-select
>> on EBC, or on the same "minor" address?  If not, you can
>> simplify your unit address representation.
>
> As far as I know, multiple devices could sit on the same chip select:
> provided there was enough address decoding logic in or around the
> devices, and that there existing bus timing parameters which would
> work with all the devices on a chip select (or "bank" in the
> terminology of the EBC bridge documentation).

Ah, that's what multiple banks are for!

> Devices on different banks can certainly have the same address/offset
> within the bank - e.g. on Ebony most of the devices are at offset 0.
> The OPB address range for each bank is separately programmable in the
> EBC bridge DCRs.

Okay, seems like the <bank,offset> representation is the simplest
possible, then.  Good.  <rubber stamp>

> (Incidentally, this is why I created the binding in this way, rather
> than just using the firmware established addresses in OPB space, which
> are usually fixed for a particular board/platform.  This way provides
> enough information that, if necessary, the kernel or another client
> can reprogram the EBC from scratch to access the various devices
> present.  Well.. actually fully reprogramming would also need the the
> bus timing parameters, which I was thinking of adding information
> before, but I haven't gotten to it yet.)

It gives a full "as simple as possible but no simpler" description
of the hardware, so it's just fine independent of whether you want
to reprogram the EBC or not.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> It would be possible, I guess, to define a 'swizzled-ranges' property
>> or something which allows child devices to be embedded in the parent's
>> address range in a not-direct way.  However, the swizzling on the
>> flash bank is really a property of the flash bank, not of the parent
>> bus - requiring it to be encoded in the parent is pretty yucky -
>> especially if the flash bank is just part of a larger chunk of bus
>> address space, defined by a single large ranges entry in the parent.
>
> It's more a property of the connection between the bus and the flash
> chips, and that connection could be described as its own "bus" node,
> something like:

But it's not a bus in reality.  There is no need to introduce
a fake bus here, it won't help anything AFAICS.

> Similar intermediary buses could be used for flashes with indirect
> access (SPI and such).

There are perfectly good mechanisms already for describing
those, too (you make a device node for the controller, and
it defines its own address space).


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Tue, Aug 07, 2007 at 06:43:35PM +0200, Segher Boessenkool wrote:
> >>> Aha!  Ok, now I understand the sorts of situations you're talking
> >>> about.  By "not direct mapped", I thought you were talking about some
> >>> kind of access via address/data registers on some indirect bus
> >>> controller, rather than weird variations on endianness and
> >>> bit-swizzling.
> >>
> >>     No, that would be just too ridiculous for a NOR flash -- I hope. 
> >> :-)
> >
> > Heh.  In my experience, very little is so ridiculous that some
> > embedded vendor won't do it.
> 
> True -- but if you can't map the NOR into the CPU address space,
> there are cheaper alternatives.  Most embedded vendors care about
> that, too ;-)
> 
> >>     So, you're saying that the 1:1 address correspondence rule stops 
> >> to apply
> >> here?
> >
> > Well.. it all depends what exactly you consider the address space of
> > the flash bank.  By which I mean the whole shmozzle represented by the
> > device node, not the individual flash chips.  It's not immediately
> > obvious whether or not that should include any swizzling done by the
> > bus wiring.
> 
> The parent device/bus shouldn't care, from its viewpoint the flash
> bank is just one linear hunk of address space.  For reading or
> writing the flash bank appears linear to the CPU as well (at least
> that's the only thing our current proposed binding supports); the
> only thing that gets "interesting" is sending commands to the flash
> chips.
> 
> > It would be possible, I guess, to define a 'swizzled-ranges' property
> > or something which allows child devices to be embedded in the parent's
> > address range in a not-direct way.
> 
> Let's not even consider this please.
> 
> > However, the swizzling on the
> > flash bank is really a property of the flash bank,
> 
> Yeah, it's the "fine structure" of the flash bank.  Something
> only the flash driver has to deal with.  So no contaminating the
> parent device node, thank you.

Sounds like we're in agreement, then.


-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Tue, Aug 07, 2007 at 06:58:20PM +0200, Segher Boessenkool wrote:
> >> Most characters are allowed in the unit-address...  The following
> >> is just fine: "my-secret-base@the-moon".  ISA uses letters to
> >> distinguish between its different address spaces, for example.
> >
> > Yeah, I should probably make dtc a bit more flexible about accepting
> > that, too.  At present, it only takes hex digits and ',', since those
> > are the common character.
> 
> Sounds good.  And then the legacy ISA devices in existing
> DTS files should be changed to say @i60 instead of @60, etc.
> (@60 is correct since the default is legacy I/O space, but
> it's good the be more verbose in those cases).

Ok, I'll look into that.  No promises that it will be real soon,
though.

> >> David, can multiple devices sit on the same chip-select
> >> on EBC, or on the same "minor" address?  If not, you can
> >> simplify your unit address representation.
> >
> > As far as I know, multiple devices could sit on the same chip select:
> > provided there was enough address decoding logic in or around the
> > devices, and that there existing bus timing parameters which would
> > work with all the devices on a chip select (or "bank" in the
> > terminology of the EBC bridge documentation).
> 
> Ah, that's what multiple banks are for!

Yes.

> > Devices on different banks can certainly have the same address/offset
> > within the bank - e.g. on Ebony most of the devices are at offset 0.
> > The OPB address range for each bank is separately programmable in the
> > EBC bridge DCRs.
> 
> Okay, seems like the <bank,offset> representation is the simplest
> possible, then.  Good.  <rubber stamp>

Excellent.  I should really do a proper write-up for b-o-f.txt, I
guess.

> > (Incidentally, this is why I created the binding in this way, rather
> > than just using the firmware established addresses in OPB space, which
> > are usually fixed for a particular board/platform.  This way provides
> > enough information that, if necessary, the kernel or another client
> > can reprogram the EBC from scratch to access the various devices
> > present.  Well.. actually fully reprogramming would also need the the
> > bus timing parameters, which I was thinking of adding information
> > before, but I haven't gotten to it yet.)
> 
> It gives a full "as simple as possible but no simpler" description
> of the hardware, so it's just fine independent of whether you want
> to reprogram the EBC or not.

That was the idea.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Tue, Aug 07, 2007 at 06:51:04PM +0200, Segher Boessenkool wrote:
> >> address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;
> >
> > Yes, I was contemplating something like that.
> 
> Let's not define this until we need it though :-)

Indeed.

> >> I haven't heard or thought of anything better either.  Using "ranges"
> >> is conceptually wrong, even ignoring the technical problems that come
> >> with it.
> >
> > Why is "ranges" conceptually wrong?
> 
> The flash partitions aren't separate devices sitting on a
> "flash bus", they are "sub-devices" of their parent.

Well, yes, but nonetheless the partitions show up as part of the
overall physical address space.  How do you encode that other than in
'ranges'?

> > To be honest this looks rather to me like another case where having
> > overlapping 'reg' and 'ranges' would actually make sense.
> 
> It never makes sense.  You should give the "master" device
> the full "reg" range it covers, and have it define its own
> address space; "sub-devices" can carve out their little hunk
> from that.  You don't want more than one device owning the
> same address range in the same address space.

Why not?  After all, the physical address ranges of the flash
partitions really do overlap with that of the flash device as a whole.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Tue, Aug 07, 2007 at 06:33:01PM +0200, Segher Boessenkool wrote:
> >>>>> Yeah, better names please -- if possible, something that someone
> >>>>> without knowledge of this SoC will understand what it is.
> >>>>
> >>>> I think the names are probably ok - I'm assuming they're in keeping
> >>>> with the convention I've used of using the same names / 
> >>>> abbreviations
> >>>> as in the CPU user manual.  I'm asking just for my own information,
> >>>> although a comment might not be a bad idea.
> >>
> >> Fine with me -- I personally prefer "system-device-controller"
> >> and "clock-power-controller" or similar, but that is mostly a
> >> matter of taste.  As long as it's human readable it's fine.
> >
> > Actually, it occurs to me that I've only sometimes been using that
> > convention for the names:  basically just for the weirdo chip control
> > devices that don't have a more widespread generic name.
> 
> It's pretty darn hard to make good sensible "generic names" for
> some of the devices on a SoC; using the acronym that's in the
> documentation for that SoC certainly isn't the worst choice.

My thoughts exactly.

> >>> +    Flash partitions
> >>> +     - reg :
> >>> +     - read-only : (optional)
> >>
> >> I'll hold off commenting on this until you've finish writing it,
> >> you probably know my opinion about it anyway :-)
> >
> > Heh.. actually I was kind of hoping for your input on what's still
> > missing.  For example, I don't know what the necessary extra
> > properties for JEDEC chips are.
> 
> I meant for the "partitions" stuff only.
> 
> For the JEDEC chips, we need a "vendor-id" and "device-id"
> property at least (or similar names -- whatever is general
> practice for this); both are a single byte, encoded as with
> encode-int.

Ok... should those really be separate properties, or should that go in
compatible, i.e. something like:
	compatible = "amd,XXXXXX", "jedec,a4-b7", "jedec-flash";

> >> One thing though -- what _exactly_ does "read-only" signify?
> >
> > That's... a good question.
> 
> Yeah.  It seems to me that the way it is currently used is
> pure policy enforcement, which doesn't belong in the device
> tree.

Well.. not really policy enforcement, but a policy hint.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>>> I haven't heard or thought of anything better either.  Using 
>>>> "ranges"
>>>> is conceptually wrong, even ignoring the technical problems that 
>>>> come
>>>> with it.
>>>
>>> Why is "ranges" conceptually wrong?
>>
>> The flash partitions aren't separate devices sitting on a
>> "flash bus", they are "sub-devices" of their parent.
>
> Well, yes, but nonetheless the partitions show up as part of the
> overall physical address space.  How do you encode that other than in
> 'ranges'?

All that address space shows up in the flash node already.  To
access the partitions you have to go via that "master" node
anyway (some commands have to be sent to address 0 on the flash,
or similar).

It is a very nice feature to not only be able to translate addresses
"up" the device tree, but also "down".

Also, it mimics reality, just like a good OF citizen should:
those partitions aren't actually devices at all, so they
certainly shouldn't be assigned a part of the host address
space.

>>> To be honest this looks rather to me like another case where having
>>> overlapping 'reg' and 'ranges' would actually make sense.
>>
>> It never makes sense.  You should give the "master" device
>> the full "reg" range it covers, and have it define its own
>> address space; "sub-devices" can carve out their little hunk
>> from that.  You don't want more than one device owning the
>> same address range in the same address space.
>
> Why not?  After all, the physical address ranges of the flash
> partitions really do overlap with that of the flash device as a whole.

They don't overlap, a partition is a proper subset of the flash.
Which as usual is shown as "reg" in the child node and #a,#s in
the parent node.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>> For the JEDEC chips, we need a "vendor-id" and "device-id"
>> property at least (or similar names -- whatever is general
>> practice for this); both are a single byte, encoded as with
>> encode-int.
>
> Ok... should those really be separate properties, or should that go in
> compatible, i.e. something like:
> 	compatible = "amd,XXXXXX", "jedec,a4-b7", "jedec-flash";

Good question.  I think we want the separate bytes, if nothing
else then just for the benefit of drivers that have their own
table of those already.  But the "compatible" thing also has
its merits of course.  I'll ask some flash gurus about what's
special about JEDEC flash, and maybe even read a datasheet or
two.  We're in no hurry right, CFI flash is lots more common
nowadays ;-)

>>>> One thing though -- what _exactly_ does "read-only" signify?
>>>
>>> That's... a good question.
>>
>> Yeah.  It seems to me that the way it is currently used is
>> pure policy enforcement, which doesn't belong in the device
>> tree.
>
> Well.. not really policy enforcement, but a policy hint.

So it most likely doesn't belong there.  How the OS userland
wants to mount those partitions, if at all, and if they even
contain a filesystem -- that's all its own business and belongs
in /etc/fstab or whatever the newfangled thing is.

On most flash chips, you can actually write-protect some
sectors; "read-only" sounds more like that.  Although
"write-protected" would be a better name.

Or maybe "read-only" is useful and I just don't see why.  In
that case, please figure out what its semantics are :-)


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Thu, Aug 09, 2007 at 09:53:41PM +0200, Segher Boessenkool wrote:
> >>>> I haven't heard or thought of anything better either.  Using 
> >>>> "ranges"
> >>>> is conceptually wrong, even ignoring the technical problems that 
> >>>> come
> >>>> with it.
> >>>
> >>> Why is "ranges" conceptually wrong?
> >>
> >> The flash partitions aren't separate devices sitting on a
> >> "flash bus", they are "sub-devices" of their parent.
> >
> > Well, yes, but nonetheless the partitions show up as part of the
> > overall physical address space.  How do you encode that other than in
> > 'ranges'?
> 
> All that address space shows up in the flash node already.  To
> access the partitions you have to go via that "master" node
> anyway (some commands have to be sent to address 0 on the flash,
> or similar).

Hrm, I suppose.  Although for read-only access that's not relevant.

> It is a very nice feature to not only be able to translate addresses
> "up" the device tree, but also "down".

I don't follow, sorry.

> Also, it mimics reality, just like a good OF citizen should:
> those partitions aren't actually devices at all, so they
> certainly shouldn't be assigned a part of the host address
> space.

Hrm.  I'm not entirely convinced that the distinction between
"actually a device" and "not actually a device" is really as clear cut
as you imply.

> >>> To be honest this looks rather to me like another case where having
> >>> overlapping 'reg' and 'ranges' would actually make sense.
> >>
> >> It never makes sense.  You should give the "master" device
> >> the full "reg" range it covers, and have it define its own
> >> address space; "sub-devices" can carve out their little hunk
> >> from that.  You don't want more than one device owning the
> >> same address range in the same address space.
> >
> > Why not?  After all, the physical address ranges of the flash
> > partitions really do overlap with that of the flash device as a whole.
> 
> They don't overlap, a partition is a proper subset of the flash.

A proper subset is a form of overlapping (indeed cases of being a
proper subset form a proper subset of cases of overlapping, to be
gratuitously meta-set-theoretic).

> Which as usual is shown as "reg" in the child node and #a,#s in
> the parent node.

That in no way encodes that the child addresses are a subset of the
parent address space.  Instead #a and #s establish a new, separate
address space for the children, and without 'ranges', there's no
information about any sort of connection, overlapping, proper-subset
or otherwise, with the parent address space.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[David Gibson]

On Thu, Aug 09, 2007 at 10:00:47PM +0200, Segher Boessenkool wrote:
> >> For the JEDEC chips, we need a "vendor-id" and "device-id"
> >> property at least (or similar names -- whatever is general
> >> practice for this); both are a single byte, encoded as with
> >> encode-int.
> >
> > Ok... should those really be separate properties, or should that go in
> > compatible, i.e. something like:
> > 	compatible = "amd,XXXXXX", "jedec,a4-b7", "jedec-flash";
> 
> Good question.  I think we want the separate bytes, if nothing
> else then just for the benefit of drivers that have their own
> table of those already.  But the "compatible" thing also has
> its merits of course.  

Ok.  Well, I guess there's no reason we can't have both.  Separate
properties for convenient access, and a compatible encoding for
unified matching.

>  I'll ask some flash gurus about what's
> special about JEDEC flash, and maybe even read a datasheet or
> two.  We're in no hurry right, CFI flash is lots more common
> nowadays ;-)

Ok.

> >>>> One thing though -- what _exactly_ does "read-only" signify?
> >>>
> >>> That's... a good question.
> >>
> >> Yeah.  It seems to me that the way it is currently used is
> >> pure policy enforcement, which doesn't belong in the device
> >> tree.
> >
> > Well.. not really policy enforcement, but a policy hint.
> 
> So it most likely doesn't belong there.  How the OS userland
> wants to mount those partitions, if at all, and if they even
> contain a filesystem -- that's all its own business and belongs
> in /etc/fstab or whatever the newfangled thing is.

Hrm.  You can argue the same about all the partition information, but
where else does it go.  Encoding suggested platform policy into the
device tree isn't fabulous, but it beats hardcoded per-platform flash
maps.

> On most flash chips, you can actually write-protect some
> sectors; "read-only" sounds more like that.  Although
> "write-protected" would be a better name.
> 
> Or maybe "read-only" is useful and I just don't see why.  In
> that case, please figure out what its semantics are :-)

Heh, well, in practice the semantics are that it passes a read-only
flag through to the mtd layer.  I guess we need to ask the physmap_of
author what he thought the read-only bit in the old binding should be
used for.

-- 
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

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>>>> Why is "ranges" conceptually wrong?
>>>>
>>>> The flash partitions aren't separate devices sitting on a
>>>> "flash bus", they are "sub-devices" of their parent.
>>>
>>> Well, yes, but nonetheless the partitions show up as part of the
>>> overall physical address space.  How do you encode that other than in
>>> 'ranges'?
>>
>> All that address space shows up in the flash node already.  To
>> access the partitions you have to go via that "master" node
>> anyway (some commands have to be sent to address 0 on the flash,
>> or similar).
>
> Hrm, I suppose.  Although for read-only access that's not relevant.

Sure, for just accessing the data, all you need is a mapping
and off you go.

>> It is a very nice feature to not only be able to translate addresses
>> "up" the device tree, but also "down".
>
> I don't follow, sorry.

Given a host address, it is nice if you can uniquely identify
the device node you access via it.

>> Also, it mimics reality, just like a good OF citizen should:
>> those partitions aren't actually devices at all, so they
>> certainly shouldn't be assigned a part of the host address
>> space.
>
> Hrm.  I'm not entirely convinced that the distinction between
> "actually a device" and "not actually a device" is really as clear cut
> as you imply.

A flash partition isn't an actual device, you cannot do all
needed operations on it without using the actual flash device
as well, even if the partition is neatly aligned on flash
blocks / flash sectors (and the proposed binding doesn't require
that; there is no need to, either, I think).

>>>>> To be honest this looks rather to me like another case where having
>>>>> overlapping 'reg' and 'ranges' would actually make sense.
>>>>
>>>> It never makes sense.  You should give the "master" device
>>>> the full "reg" range it covers, and have it define its own
>>>> address space; "sub-devices" can carve out their little hunk
>>>> from that.  You don't want more than one device owning the
>>>> same address range in the same address space.
>>>
>>> Why not?  After all, the physical address ranges of the flash
>>> partitions really do overlap with that of the flash device as a 
>>> whole.
>>
>> They don't overlap, a partition is a proper subset of the flash.
>
> A proper subset is a form of overlapping (indeed cases of being a
> proper subset form a proper subset of cases of overlapping, to be
> gratuitously meta-set-theoretic).

Yeah well, depends on your viewpoint.

>> Which as usual is shown as "reg" in the child node and #a,#s in
>> the parent node.
>
> That in no way encodes that the child addresses are a subset of the
> parent address space.

It does, via the semantics of the flash binding: its child nodes
are partitions on the flash device.

> Instead #a and #s establish a new, separate
> address space for the children, and without 'ranges', there's no
> information about any sort of connection, overlapping, proper-subset
> or otherwise, with the parent address space.

Such information is there implicitly, per the binding.


Segher

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

Hello.

    I'm having sort of vacation, mostly away from computer, hence my belated 
reply...

David Gibson wrote:

>>>>>Can you describe some of the options for *not* direct mapped flash
>>>>>chips - I can't reasonably come up with a way of describing the
>>>>>distinction when I've never seen NOR flash other than direct mapped.

>>>>   You're lucky to live in the single-endian world.  Once you're in bi-endian 
>>>>one, all kinds of strange mappings become possible.  I've seen the MIPS 

>>    Well, not necessarily -- 16-bit only accesses are always possibly (no 
>>dount this would be a strange mapping however)...

> I have no idea what you're getting at here.

    The code fragment that I've posted before -- it used readw() in read8() 
method of the mapping.

>>>>mapping driver which required swapping flash bytes in s/w in BE mode, i.e. 
>>>>couldn't be served by physmap, yet that's not all...  here's the code of its 
>>>>map read*() methods:

>>>Aha!  Ok, now I understand the sorts of situations you're talking
>>>about.  By "not direct mapped", I thought you were talking about some
>>>kind of access via address/data registers on some indirect bus
>>>controller, rather than weird variations on endianness and
>>>bit-swizzling.

>>    No, that would be just too ridiculous for a NOR flash -- I hope. :-)

> Heh.  In my experience, very little is so ridiculous that some
> embedded vendor won't do it.

    Right. :-)

>>>Hrm.. this is a property of how the flash is wired onto the bus,
>>>rather than of the flash chips themselves, 

>>    Indeed.

>>>so I'm not entirely sure where description of it belongs.

>>    So, you're saying that the 1:1 address correspondence rule stops to apply 
>>here?

> Well.. it all depends what exactly you consider the address space of
> the flash bank.  By which I mean the whole shmozzle represented by the
> device node, not the individual flash chips.  It's not immediately
> obvious whether or not that should include any swizzling done by the
> bus wiring.

> It would be possible, I guess, to define a 'swizzled-ranges' property
> or something which allows child devices to be embedded in the parent's
> address range in a not-direct way.  However, the swizzling on the
> flash bank is really a property of the flash bank, not of the parent
> bus - requiring it to be encoded in the parent is pretty yucky -
> especially if the flash bank is just part of a larger chunk of bus
> address space, defined by a single large ranges entry in the parent.

[...]

>>>>>>>+     - reg : Address range of the flash chip
>>>>>>>+     - bank-width : Width (in bytes) of the flash bank.  Equal to the device width
>>>>>>>+       times the number of interleaved chips.
>>>>>>>+     - device-width : (optional) Width of a single flash chip.  If omitted,
>>>>>>>+       assumed to be equal to 'bank-width'.

>>>>>>  Why then not just introduce the "interleave" prop and obsolete the
>>>>>>"bank-width"?

>>>>>Because they're equivalent information, and bank-width is what the
>>>>>code ends up actually caring about.  I don't see any reason to prefer
>>>>>giving the interleave.

>>>>   Well, "device-width" is not the thing that we care about either. ;-)

>>>Well, yes but we need to encode it somehow.  Segher preferred
>>>device-width to interleave, because it's closer to a description of
>>>the actual hardware, rather than an abstration decribing its wiring.

>>>In other words I *still* don't see any reason to prefer giving the
>>>interleave.

    Well, for example, enable "Flash chip driver advanced configuration 
options" (appers in the "RAM/ROM/Flash chip drivers menu" when you enable CFI 
probe, and it has a choice of bus widths 8 to 256 bits (here buswidth == bank 
width) and *interleave factors* 1 to 8...

>>    I wasn't talking of "interleave" preference over "device-width", I was 
>>talking about obsoleting "bank-width" with this pair.

> Whatever.  You haven't given any argument to prefer interleave over
> either device-width or bank-width.

    Consistency? The bank width is a product of the chip width and interleave 
factor.  However, I don't insist. The MTD core itself has a different view on 
the CFI flash geometry: bank width (which is a property of a mapping driver) 
and interleave (which is a property of a chip driver).

>>>>>>>Index: working-2.6/drivers/mtd/maps/physmap_of.c
>>>>>>>===================================================================
>>>>>>>--- working-2.6.orig/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:11.000000000 +1000
>>>>>>>+++ working-2.6/drivers/mtd/maps/physmap_of.c	2007-07-30 17:07:14.000000000 +1000

>>>>[...]

>>>>>>>@@ -221,6 +297,14 @@ err_out:
>>>>>>>
>>>>>>>static struct of_device_id of_physmap_match[] = {
>>>>>>>	{
>>>>>>>+		.compatible	= "cfi-flash",
>>>>>>>+		.data		= (void *)"cfi_probe",
>>>>>>>+	},
>>>>>>>+	{
>>>>>>>+		.compatible	= "jedec-flash",
>>>>>>>+		.data		= (void *)"jedec_probe",
>>>>>>>+	},
>>>>>>>+	{

>>>>>>  This would also trigger on non-linearly mapped CFI or JEDEC
>>>>>>flashes which is not a good idea -- however, as we're using the MTD
>>>>>>probing code anyway, it will just fail, so it's not luckily is not a
>>>>>>fatal design flaw.

>>>>>Well, if there's nothing else to distinguish them.  Which there isn't
>>>>>yet, but will need to be: see above about incomplete - helpful
>>>>>suggestions about how to describe the mapping would be more useful
>>>>>than merely pointing out the lack.

>>>>   I was thinking about adding "direct-mapped" prop... but maybe adding 
>>>>"ranges" to the parent node (that's "ebc") would indeed ensure that the flash 
>>>>is mapped 1:1 to the EBC's parent bus also.

>>>The ebc already has a ranges property.  But that can't describe the

>>    Not in the device tree that started that thread -- I haven't seen another.

> Ah sorry.  The ranges property isn't in the dts, it's added by the
> bootwrapper based on the EBC register contents.

    Cool. :-)

>>>sorts of bit-swizzling you're talking about.

>>    Let's hear what Segher says (if he's not yet tired of all this :-).

>>>>>>>Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
>>>>>>>===================================================================
>>>>>>>--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000
>>>>>>>+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-07-30 17:07:14.000000000 +1000

>>>>>>[...]

>>>>>>>@@ -158,14 +161,20 @@
>>>>>>>				};

>>>>>>>				large-flash@2,0 {

>>>>>>  Hmm... what does @2,0 mean? :-O

>>>>>EBC chip select 2, offset 0.

>>>>   Well, so this node is under some kind of local bus node -- that's good.
>>>>Didn't know that the spec allows commas after @...

>>>Well, now you do.  I believe USB usually uses that format, too.

>>    USB what, hosts or devices?

> Devices.

    And what the comma-separated numbers mean there?

[...]

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Sergei Shtylyov]

Segher Boessenkool wrote:

>>>address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;

>>Yes, I was contemplating something like that.

> Let's not define this until we need it though :-)

    Let's ot even think of it, since this will end up in a "catch all" driver, 
and yet this may be not enough when the flash doesn't support 8-but R/W, for 
example (I've already quoted it...

>>>I haven't heard or thought of anything better either.  Using "ranges"
>>>is conceptually wrong, even ignoring the technical problems that come
>>>with it.
>>Why is "ranges" conceptually wrong?

> The flash partitions aren't separate devices sitting on a

    Yeah, that's why I decided not to go that from the very start... though 
wait: I didn't do this simply because they'renot devices.
That lead me to interesting question: do device tree have something for the 
disk partitions?

> "flash bus", they are "sub-devices" of their parent.

    They're quite an abstaction of a device -- althogh Linux treats them as 
separate devices indeed.

>>To be honest this looks rather to me like another case where having
>>overlapping 'reg' and 'ranges' would actually make sense.

> It never makes sense.  You should give the "master" device
> the full "reg" range it covers, and have it define its own
> address space; "sub-devices" can carve out their little hunk
> from that.  You don't want more than one device owning the
> same address range in the same address space.

    So, no "ranges" prop in MTD node is necessary? Phew... :-)

> Segher

WBR, Sergei

Re: [PATCH 2/6] PowerPC 440EPx: Sequoia DTS

[Segher Boessenkool]

>>>> address-permutation = <0 1 3 2 4 5 7 6 e f d c a b 9 8>;
>
>>> Yes, I was contemplating something like that.
>
>> Let's not define this until we need it though :-)
>
>    Let's ot even think of it,

It is good to think about it, for the simple reason that it
validates whether the current design is future-proof or not.

> since this will end up in a "catch all" driver,

Yeah, we shouldn't _define_ anything like this, not until
it is needed anyway.

> and yet this may be not enough when the flash doesn't support 8-but 
> R/W, for example (I've already quoted it...

Yeah.  There is no need to future-proof to insane designs anyway;
whatever can not fit in the "generic" framework can bloody well
just do its own binding, no need to pollute the generic thing.

>>>> I haven't heard or thought of anything better either.  Using 
>>>> "ranges"
>>>> is conceptually wrong, even ignoring the technical problems that 
>>>> come
>>>> with it.
>>> Why is "ranges" conceptually wrong?
>
>> The flash partitions aren't separate devices sitting on a
>
>    Yeah, that's why I decided not to go that from the very start... 
> though wait: I didn't do this simply because they'renot devices.
> That lead me to interesting question: do device tree have something 
> for the disk partitions?

Some do.  Most don't.  There is no standardised binding I know of.

The big huge difference here is that disks typically do contain
partitioning information on the disk itself, and flash doesn't.

>> "flash bus", they are "sub-devices" of their parent.
>
>    They're quite an abstaction of a device -- althogh Linux treats 
> them as separate devices indeed.

Sure, it's a pseudo-device.  Nothing new there.

>>> To be honest this looks rather to me like another case where having
>>> overlapping 'reg' and 'ranges' would actually make sense.
>
>> It never makes sense.  You should give the "master" device
>> the full "reg" range it covers, and have it define its own
>> address space; "sub-devices" can carve out their little hunk
>> from that.  You don't want more than one device owning the
>> same address range in the same address space.
>
>    So, no "ranges" prop in MTD node is necessary? Phew... :-)

Yeah, it would be positively harmful.  They are pseudo-devices
only, the kernel device driver needs to always access the real
device.


Segher