Skip to contentSkip to conversation

powerpc: DBox2 Board Support

PATCH/RFC Jochen Friedrich 5 participants 10 messages Dec 22, 2007 – Jan 2, 2008

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

This patch adds device tree source, default config and setup code for DBox2 devices.

---

arch/powerpc/boot/dts/dbox2.dts          |  263 ++++++++
arch/powerpc/configs/dbox2_defconfig     | 1042 ++++++++++++++++++++++++++++++
arch/powerpc/platforms/8xx/Kconfig       |    7 +
arch/powerpc/platforms/8xx/Makefile      |    1 +
arch/powerpc/platforms/8xx/dbox2.h       |   32 +
arch/powerpc/platforms/8xx/dbox2_setup.c |  225 +++++++
include/asm-powerpc/mpc8xx.h             |    4 +
7 files changed, 1574 insertions(+), 0 deletions(-)

create mode 100644 arch/powerpc/boot/dts/dbox2.dts create mode 100644 arch/powerpc/configs/dbox2_defconfig create mode 100644 arch/powerpc/platforms/8xx/dbox2.h create mode 100644 arch/powerpc/platforms/8xx/dbox2_setup.c

arch/powerpc/boot/dts/dbox2.dts new file +263 -0
new file mode 100644
index 0000000..8d91510
@@ -0,0 +1,263 @@
+/*
+ * DBOX2 Device Tree Source
+ *
+ * Copyright 2007 Jochen Friedrich <[email protected]>
+ *
+ * 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
+ * option) any later version.
+ */
+
+/ {
+	model = "Dbox2";
+	compatible = "betaresearch,dbox2";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		PowerPC,823@0 {
+			device_type = "cpu";
+			reg = <0>;
+			d-cache-line-size = <d#16>;
+			i-cache-line-size = <d#16>;
+			d-cache-size = <d#1024>;
+			i-cache-size = <d#2048>;
+			timebase-frequency = <0>;
+			bus-frequency = <0>;
+			clock-frequency = <0>;
+			interrupts = <f 2>;	// decrementer interrupt
+			interrupt-parent = <&PIC>;
+		};
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0 2000000>;
+	};
+
+	localbus@8000000 {
+		compatible = "betaresearch,dbox2-localbus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <8000000 18000000>;
+
+		ranges = <0 8000000 18000000>;
+
+		enx@0 {
+			compatible = "c-cube,enx";
+			reg = <0 3400 1000000 200000>;
+			interrupts = <2 2>;
+			interrupt-parent = <&PIC>;
+		};
+
+		gtx@0 {
+			compatible = "c-cube,gtx";
+			reg = <400000 3000 0 200000>;
+			interrupts = <2 2>;
+			interrupt-parent = <&PIC>;
+		};
+
+		fp@0 {
+			compatible = "betaresearch,dbox2-fp";
+			interrupts = <4 2>;
+			interrupt-parent = <&PIC>;
+			gpios = <0 e>;
+			gpio-parent = <&CPM1_PIO>;
+		};
+
+		fe@0 {
+			compatible = "betaresearch,dbox2-fe";
+			interrupts = <e 2>;
+			interrupt-parent = <&PIC>;
+		};
+
+		avia@2000000 {
+			compatible = "c-cube,avia";
+			reg = <2000000 200>;
+			interrupts = <8 2>;
+			interrupt-parent = <&PIC>;
+		};
+		cam@4000000 {
+			compatible = "betaresearch,dbox2-cam";
+			reg = <4000000 20000>;
+			interrupts = <6 2>;
+			interrupt-parent = <&PIC>;
+			gpios = <1 1c 1 1d 1 1e 1 1f>;
+			gpio-parent = <&CPM1_PIO>;
+		};
+
+		cam@4040000 {
+			compatible = "betaresearch,dbox2-cam";
+			reg = <4040000 20000>;
+			interrupts = <6 2>;
+			interrupt-parent = <&PIC>;
+			gpios = <1 1c 1 1d 1 1e 1 1f>;
+			gpio-parent = <&CPM1_PIO>;
+		};
+
+		flash@8000000 {
+			// Flash also has info about model needed by setup
+			compatible = "cfi-flash",
+				     "betaresearch,dbox2-config";
+			reg = <8000000 800000>;
+			bank-width = <4>;
+			device-width = <1>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			partition@0 {
+				label = "BR bootloader";
+				reg = <0 20000>;
+				read-only;
+			};
+			partition@20000 {
+				label = "FLFS (U-Boot)";
+				reg = <20000 20000>;
+			};
+			partition@40000 {
+				label = "rootfs";
+				reg = <40000 7c0000>;
+			};
+			ovpartition@20000 {
+				label = "Flash without bootloader";
+				reg = <20000 7e0000>;
+			};
+			ovpartition@0 {
+				label = "Complete Flash";
+				reg = <0 800000>;
+				read-only;
+			};
+		};
+	};
+
+	soc@ff000000 {
+		compatible = "fsl,mpc823",
+			     "fsl,pq1-soc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+		ranges = <0 ff000000 00004000>;
+		reg = <ff000000 00004000>;
+		bus-frequency = <0>;
+
+		PIC: pic@0 {
+			interrupt-controller;
+			#address-cells = <0>;
+			#interrupt-cells = <2>;
+			reg = <0 24>;
+			compatible = "fsl,mpc823-pic",
+				     "fsl,pq1-pic";
+		};
+
+		wdt@0 {
+			device_type = "watchdog";
+			compatible = "fsl,mpc823-wdt",
+				     "fsl,pq1-wdt";
+			reg = <0 10>;
+		};
+
+		cpm@9c0 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <9c0 40>;
+			command-proc = <9c0>;
+			interrupts = <0 2>;	// cpm error interrupt
+			interrupt-parent = <&CPM_PIC>;
+			compatible = "fsl,mpc823-cpm",
+				     "fsl,cpm1";
+
+			muram@2000 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 2000 2000>;
+
+				data@0 {
+					compatible = "fsl,cpm-muram-data";
+					reg = <0 1c00>;
+				};
+			};
+
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};
+
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";
+			};
+
+			brg@9f0 {
+				compatible = "fsl,mpc823-brg",
+					     "fsl,cpm1-brg",
+					     "fsl,cpm-brg";
+				reg = <9f0 10>;
+			};
+
+			CPM_PIC: pic@930 {
+				interrupt-controller;
+				#address-cells = <0>;
+				#interrupt-cells = <2>;
+				interrupts = <5 2 0 2>;
+				interrupt-parent = <&PIC>;
+				reg = <930 20>;
+				compatible = "fsl,mpc823-cpm-pic",
+					     "fsl,cpm1-pic";
+			};
+
+			serial@a80 {
+				device_type = "serial";
+				compatible = "fsl,mpc823-smc-uart",
+					     "fsl,cpm1-smc-uart";
+				reg = <a80 10 3e80 40>;
+				interrupts = <4 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fsl,cpm-brg = <1>;
+				fsl,cpm-command = <0090>;
+			};
+
+			serial@a90 {
+				device_type = "serial";
+				compatible = "fsl,mpc823-smc-uart",
+					     "fsl,cpm1-smc-uart";
+				reg = <a90 10 3f80 40>;
+				interrupts = <3 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fsl,cpm-brg = <2>;
+				fsl,cpm-command = <00d0>;
+			};
+
+			ethernet@a00 {
+				device_type = "network";
+				compatible = "fsl,mpc823-scc-enet",
+					     "fsl,cpm1-scc-enet";
+				reg = <a20 20 3d00 80>;
+				interrupts = <1d 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fixed-link = <1 0 d#10>;
+				fsl,cpm-command = <0040>;
+				linux,network-index = <0>;
+			};
+
+			i2c@860 {
+				compatible = "fsl,mpc823-i2c",
+					     "fsl,cpm1-i2c",
+					     "fsl,cpm-i2c";
+				reg = <860 20 3c80 30>;
+				interrupts = <10 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fsl,cpm-command = <0010>;
+			};
+		};
+	};
+	chosen {
+		linux,stdout-path = "/soc/cpm/serial@a80";
+	};
+};
arch/powerpc/configs/dbox2_defconfig new file +1042 -0

1,042 lines not on this page · raw message

arch/powerpc/platforms/8xx/Kconfig +0 -0
index bd28655..bcd3c40 100644
@@ -43,6 +43,13 @@ config PPC_EP88XC

This board is also resold by Freescale as the QUICCStart MPC885 Evaluation System and/or the CWH-PPC-885XN-VE.

+config DBOX2
+ bool "DBOX2"
+ select CPM1
+ select PPC_CPM_NEW_BINDING
+ help
+ This enables support for the Betaresearch DBOX2.
+
endchoice

menu "Freescale Ethernet driver platform-specific options"

arch/powerpc/platforms/8xx/Makefile +0 -0
index 8b70980..306df8a 100644
@@ -5,3 +5,4 @@ obj-$(CONFIG_PPC_8xx)	  += m8xx_setup.o
obj-$(CONFIG_MPC885ADS)   += mpc885ads_setup.o
obj-$(CONFIG_MPC86XADS)   += mpc86xads_setup.o
obj-$(CONFIG_PPC_EP88XC)  += ep88xc.o

+obj-$(CONFIG_DBOX2) += dbox2_setup.o

arch/powerpc/platforms/8xx/dbox2.h new file +32 -0
new file mode 100644
index 0000000..cf5fda9
@@ -0,0 +1,32 @@
+/*
+ * A collection of structures, addresses, and values associated with
+ * the DBox2.
+ *
+ * Author: (c) 2007 Jochen Friedrich
+ *
+ * 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.
+ */
+
+#ifdef __KERNEL__
+#ifndef __ASM_DBOX2_H__
+#define __ASM_DBOX2_H__
+
+#include <sysdev/fsl_soc.h>
+
+/* Vendor information in BR Bootloader
+ */
+
+#define DBOX2_VENDOR_OFFSET	(0x1ffe0)
+
+enum dbox2_mid {
+	DBOX2_MID_NOKIA       = 1,
+	DBOX2_MID_PHILIPS     = 2,
+	DBOX2_MID_SAGEM       = 3,
+};
+
+enum dbox2_mid dbox2_get_mid(void);
+
+#endif /* __ASM_DBOX2_H__ */
+#endif /* __KERNEL__ */
arch/powerpc/platforms/8xx/dbox2_setup.c new file +225 -0

225 lines not on this page · raw message

include/asm-powerpc/mpc8xx.h +0 -0
index 2be014b..b6fd7d6 100644
@@ -23,6 +23,10 @@

#include <platforms/8xx/mpc885ads.h>
#endif

+#if defined(CONFIG_DBOX2)
+#include <platforms/8xx/dbox2.h>
+#endif
+
#ifdef CONFIG_PCMCIA_M8XX
extern struct mpc8xx_pcmcia_ops m8xx_pcmcia_ops;
#endif

Signature · 1 line
1.5.3.7
#1 lore raw

arch/powerpc/platforms/8xx/dbox2.h line 16

Quoted 4 lines · On Sat, 22 Dec 2007 20:13:31 +0100 Jochen Friedrich <[email protected]> wrote:

+++ b/arch/powerpc/platforms/8xx/dbox2.h

arch/powerpc/platforms/8xx/dbox2.h new file +2 -0
+
+#include <sysdev/fsl_soc.h>

Nothing in this header files uses anything in that one, so don't include it.

arch/powerpc/platforms/8xx/dbox2_setup.c line 40

Quoted 3 lines

+++ b/arch/powerpc/platforms/8xx/dbox2_setup.c

arch/powerpc/platforms/8xx/dbox2_setup.c new file +2 -0
+
+char *dbox2_manuf_name[3] = {

static const.

arch/powerpc/platforms/8xx/dbox2_setup.c line 52

Quoted 12 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +12 -0
+	"Nokia",
+	"Philips",
+	"Sagem",
+};
+
+static enum dbox2_mid dbox2_manuf_id;
+
+struct cpm_pin {
+	int port, pin, flags;
+};
+
+static struct cpm_pin dbox2_pins[] = {

const?

arch/powerpc/platforms/8xx/dbox2_setup.c line 194

Quoted 4 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +4 -0
+static int __init dbox2_probe(void)
+{
+	unsigned long root = of_get_flat_dt_root();
+	return of_flat_dt_is_compatible(root, "betaresearch,dbox2");

You should include asm/prom.h to use the flattened device tree function.

Quoted 1 line

+static struct of_device_id __initdata of_bus_ids[] = {

__initdata

Signature · 3 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/
#2 lore raw
In reply to Jochen Friedrich
Quoted 2 lines · Jochen Friedrich

This patch adds device tree source, default config and setup code for DBox2 devices.

[snip]

Quoted 39 lines
arch/powerpc/boot/dts/dbox2.dts new file +33 -0
new file mode 100644
index 0000000..8d91510
@@ -0,0 +1,263 @@
+/*
+ * DBOX2 Device Tree Source
+ *
+ * Copyright 2007 Jochen Friedrich <[email protected]>
+ *
+ * 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
+ * option) any later version.
+ */
+
+/ {
+	model = "Dbox2";
+	compatible = "betaresearch,dbox2";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		PowerPC,823@0 {
+			device_type = "cpu";
+			reg = <0>;
+			d-cache-line-size = <d#16>;
+			i-cache-line-size = <d#16>;
+			d-cache-size = <d#1024>;
+			i-cache-size = <d#2048>;
+			timebase-frequency = <0>;
+			bus-frequency = <0>;
+			clock-frequency = <0>;
+			interrupts = <f 2>;	// decrementer interrupt
+			interrupt-parent = <&PIC>;

arch/powerpc/boot/dts/dbox2.dts line 43

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+		};
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0 2000000>;
+	};
+
+	localbus@8000000 {
+		compatible = "betaresearch,dbox2-localbus";

Is this bus interface really board specific? I would have thought the localbus bridge would have been part of the SoC in which case the compatible string should mention the 823 rather than the dbox.

arch/powerpc/boot/dts/dbox2.dts line 44

Quoted 1 line
arch/powerpc/boot/dts/dbox2.dts new file +1 -0
+		#address-cells = <1>;

It's also usual for these localbus things, to have #address-cells of 2, with the chipselect explicitly present as the first cell.

arch/powerpc/boot/dts/dbox2.dts line 51

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+		#size-cells = <1>;
+		reg = <8000000 18000000>;
+
+		ranges = <0 8000000 18000000>;
+
+		enx@0 {
+			compatible = "c-cube,enx";

What are these mysterious enx, gtx, etc. devices? Some comments might be nice.

[snip]

arch/powerpc/boot/dts/dbox2.dts line 105

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+		flash@8000000 {
+			// Flash also has info about model needed by setup
+			compatible = "cfi-flash",
+				     "betaresearch,dbox2-config";

compatible strings should be listed with most specific first, most general last. That would put "cfi-flash" after the dbox2 specific string, I believe.

[snip]

Quoted 11 lines
arch/powerpc/boot/dts/dbox2.dts new file +11 -0
+	soc@ff000000 {
+		compatible = "fsl,mpc823",
+			     "fsl,pq1-soc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+		ranges = <0 ff000000 00004000>;
+		reg = <ff000000 00004000>;
+		bus-frequency = <0>;
+
+		PIC: pic@0 {

arch/powerpc/boot/dts/dbox2.dts line 156

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+			interrupt-controller;
+			#address-cells = <0>;
+			#interrupt-cells = <2>;
+			reg = <0 24>;
+			compatible = "fsl,mpc823-pic",
+				     "fsl,pq1-pic";
+		};
+
+		wdt@0 {
+			device_type = "watchdog";

Drop this device_type.

Quoted 28 lines
arch/powerpc/boot/dts/dbox2.dts new file +28 -0
+			compatible = "fsl,mpc823-wdt",
+				     "fsl,pq1-wdt";
+			reg = <0 10>;
+		};
+
+		cpm@9c0 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <9c0 40>;
+			command-proc = <9c0>;
+			interrupts = <0 2>;	// cpm error interrupt
+			interrupt-parent = <&CPM_PIC>;
+			compatible = "fsl,mpc823-cpm",
+				     "fsl,cpm1";
+
+			muram@2000 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 2000 2000>;
+
+				data@0 {
+					compatible = "fsl,cpm-muram-data";
+					reg = <0 1c00>;
+				};
+			};
+
+			// Port D is LCD exclusive. Don't export as GPIO

arch/powerpc/boot/dts/dbox2.dts line 194

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};
+
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";

Is this representing an LCD controller, or the display itself. Either way I'm surprised there's something here in the SoC that has a compatible string that's not "fsl,something"

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

Hi David,

Quoted 11 lines

arch/powerpc/boot/dts/dbox2.dts line 43

Quoted 2 lines
arch/powerpc/boot/dts/dbox2.dts new file +2 -0
+	localbus@8000000 {
+		compatible = "betaresearch,dbox2-localbus";

Is this bus interface really board specific? I would have thought the localbus bridge would have been part of the SoC in which case the compatible string should mention the 823 rather than the dbox.

Review

Quoted 1 line
(quoted diff) +1 -0
+		#address-cells = <1>;

It's also usual for these localbus things, to have #address-cells of 2, with the chipselect explicitly present as the first cell.

The setup of the localbus is already done in the boot loader. However, i'll recode this to reflect the mapping of addresses to chipselects.

Quoted 10 lines

arch/powerpc/boot/dts/dbox2.dts line 51

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+		#size-cells = <1>;
+		reg = <8000000 18000000>;
+
+		ranges = <0 8000000 18000000>;
+
+		enx@0 {
+			compatible = "c-cube,enx";

What are these mysterious enx, gtx, etc. devices? Some comments might be nice.

Those are mostly devices for video processing (MPEG demux, on screen display, MPEG decoder, etc). Drivers (for ARCH=ppc) are available on cvs.tuxbox.org with GPL license. A work in progress port to ARCH=powerpc is at git://git.bocc.de/dbox2.git.

Quoted 7 lines
Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};
Quoted 8 lines

arch/powerpc/boot/dts/dbox2.dts line 194

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";

Is this representing an LCD controller, or the display itself. Either way I'm surprised there's something here in the SoC that has a compatible string that's not "fsl,something"

It's a LCD controller wired to PortD. PortD is used for four 1bit lines and one 8bit bus.

Would something like this be better?

CPM1_PIO: pio@970 {
	compatible = "fsl,cpm1-pario";
	reg = <970 180>;
	num-ports = <4>;
	#gpio-cells = <2>;
};
lcd@0 {
	compatible = "samsung,ks0713";
	gpio-parent = <&CPM1_PIO>;
	gpio-port = 3;
}

And then run gpio_request() for all lines on this port in the driver?

Thanks,
Jochen

#4 lore raw
Quoted 9 lines · Jochen Friedrich

Hi David,

Quoted 7 lines

arch/powerpc/boot/dts/dbox2.dts line 43

Quoted 2 lines
arch/powerpc/boot/dts/dbox2.dts new file +2 -0
+	localbus@8000000 {
+		compatible = "betaresearch,dbox2-localbus";

Is this bus interface really board specific? I would have thought the localbus bridge would have been part of the SoC in which case the compatible string should mention the 823 rather than the dbox.

Quoted 7 lines
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+		#address-cells = <1>;

It's also usual for these localbus things, to have #address-cells of 2, with the chipselect explicitly present as the first cell.

The setup of the localbus is already done in the boot loader. However, i'll recode this to reflect the mapping of addresses to chipselects.

Sure, that's normal. But to the greatest extent possible, the device tree should describe the hardware, not the configuration that's been set up by the firmware/bootloader. It's usually not possible to get all the way there, but encoding the chipselect information this way gets you closer. It means that if you later need to adapt the device tree to the same board with a firmware that does things differently, only the "ranges" property should need changing. Or, if you later need a kernel port to a version with no-firmware or minimal firmware that doesn't configure the bus, the necessary information is in the tree to configure the bridge.

Quoted 11 lines
Quoted 10 lines

arch/powerpc/boot/dts/dbox2.dts line 51

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+		#size-cells = <1>;
+		reg = <8000000 18000000>;
+
+		ranges = <0 8000000 18000000>;
+
+		enx@0 {
+			compatible = "c-cube,enx";

What are these mysterious enx, gtx, etc. devices? Some comments might be nice.

Quoted 3 lines

Those are mostly devices for video processing (MPEG demux, on screen display, MPEG decoder, etc). Drivers (for ARCH=ppc) are available on cvs.tuxbox.org with GPL license. A work in progress port to ARCH=powerpc is at git://git.bocc.de/dbox2.git.

Ok. Again, some comments so a casual read of the tree tells you roughly what they are would be nice.

Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};
Quoted 8 lines

arch/powerpc/boot/dts/dbox2.dts line 194

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";

Is this representing an LCD controller, or the display itself. Either way I'm surprised there's something here in the SoC that has a compatible string that's not "fsl,something"

Quoted 2 lines

It's a LCD controller wired to PortD. PortD is used for four 1bit lines and one 8bit bus.

I'm still kind of confused here. Does the pio@970 node above represent the PortD controller? If the LCD controller is accessed solely through PortD, then it should be a child of the PortD node.

At present, pio and lcd have overlapping reg resources which is certainly wrong.

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

Hi David,

Quoted 17 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
arch/powerpc/boot/dts/dbox2.dts new file +6 -0
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
Quoted 8 lines

arch/powerpc/boot/dts/dbox2.dts line 194

Quoted 5 lines
arch/powerpc/boot/dts/dbox2.dts new file +5 -0
+			};
+
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";

Is this representing an LCD controller, or the display itself. Either way I'm surprised there's something here in the SoC that has a compatible string that's not "fsl,something"

Quoted 2 lines

It's a LCD controller wired to PortD. PortD is used for four 1bit lines and one 8bit bus.

Quoted 7 lines

I'm still kind of confused here. Does the pio@970 node above represent the PortD controller? If the LCD controller is accessed solely through PortD, then it should be a child of the PortD node.

At present, pio and lcd have overlapping reg resources which is certainly wrong.

pio@970 represent 4 ports (A-D). Ports A-C are used as GPIO lines. Port D is exclusively used for the LCD, but it needs an 8bit accessor. Unfortunately, the GPIO API doesn't have such an accessor, so the LCD driver needs to access Port D itself and needs to make sure no other driver can access Port D via the GPIO API. The representation as child of pio seems reasonable.

Thanks,
Jochen

#6 lore raw
In reply to Jochen Friedrich
Quoted 6 lines
include/asm-powerpc/mpc8xx.h +0 -0
index 2be014b..b6fd7d6 100644

include/asm-powerpc/mpc8xx.h line 32

Quoted 10 lines
include/asm-powerpc/mpc8xx.h +4 -0
@@ -23,6 +23,10 @@
 #include <platforms/8xx/mpc885ads.h>
 #endif
 
+#if defined(CONFIG_DBOX2)
+#include <platforms/8xx/dbox2.h>
+#endif
+
 #ifdef CONFIG_PCMCIA_M8XX
 extern struct mpc8xx_pcmcia_ops m8xx_pcmcia_ops;
 #endif

is this really needed? if so why. we should work towards not having to do this going forward.

(I cant believe its needed since dbox2.h doesn't seem to have anything defined we'd need anywhere else).

- k

#7 lore raw
In reply to Jochen Friedrich
Quoted 10 lines · Jochen Friedrich
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+		gtx@0 {
+			compatible = "c-cube,gtx";
+			reg = <400000 3000 0 200000>;
+			interrupts = <2 2>;
+			interrupt-parent = <&PIC>;
+		};
+
+		fp@0 {
+			compatible = "betaresearch,dbox2-fp";
+			interrupts = <4 2>;

arch/powerpc/boot/dts/dbox2.dts line 76

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+			interrupt-parent = <&PIC>;
+			gpios = <0 e>;
+			gpio-parent = <&CPM1_PIO>;
+		};
+
+		fe@0 {
+			compatible = "betaresearch,dbox2-fe";
+			interrupts = <e 2>;
+			interrupt-parent = <&PIC>;
+		};

These unit addresses look wrong.

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+		cam@4000000 {
+			compatible = "betaresearch,dbox2-cam";
+			reg = <4000000 20000>;
+			interrupts = <6 2>;
+			interrupt-parent = <&PIC>;
+			gpios = <1 1c 1 1d 1 1e 1 1f>;
+			gpio-parent = <&CPM1_PIO>;

arch/powerpc/boot/dts/dbox2.dts line 100

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+		};
+
+		cam@4040000 {
+			compatible = "betaresearch,dbox2-cam";
+			reg = <4040000 20000>;
+			interrupts = <6 2>;
+			interrupt-parent = <&PIC>;
+			gpios = <1 1c 1 1d 1 1e 1 1f>;
+			gpio-parent = <&CPM1_PIO>;
+		};

Maybe gpios should have a length field? Or maybe we should just phandle to a separate node under the gpio controller node. What happens if a device sits on two different gpio-parents?

arch/powerpc/boot/dts/dbox2.dts line 105

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+		flash@8000000 {
+			// Flash also has info about model needed by setup
+			compatible = "cfi-flash",
+				     "betaresearch,dbox2-config";

What does dbox2-config mean?

arch/powerpc/boot/dts/dbox2.dts line 132

Quoted 9 lines
arch/powerpc/boot/dts/dbox2.dts new file +9 -0
+			ovpartition@20000 {
+				label = "Flash without bootloader";
+				reg = <20000 7e0000>;
+			};
+			ovpartition@0 {
+				label = "Complete Flash";
+				reg = <0 800000>;
+				read-only;
+			};

What is "ovpartition"?

arch/powerpc/boot/dts/dbox2.dts line 160

Quoted 6 lines
arch/powerpc/boot/dts/dbox2.dts new file +6 -0
+		wdt@0 {
+			device_type = "watchdog";
+			compatible = "fsl,mpc823-wdt",
+				     "fsl,pq1-wdt";
+			reg = <0 10>;
+		};

No device_type.

arch/powerpc/boot/dts/dbox2.dts line 167

Quoted 6 lines
arch/powerpc/boot/dts/dbox2.dts new file +6 -0
+		cpm@9c0 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <9c0 40>;
+			command-proc = <9c0>;

command-proc is obsolete.

arch/powerpc/boot/dts/dbox2.dts line 182

Quoted 10 lines
arch/powerpc/boot/dts/dbox2.dts new file +10 -0
+			muram@2000 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 2000 2000>;
+
+				data@0 {
+					compatible = "fsl,cpm-muram-data";
+					reg = <0 1c00>;
+				};
+			};

Should have compatible of fsl,cpm-muram on the muram node as well, even though the current code doesn't use it.

arch/powerpc/boot/dts/dbox2.dts line 190

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};

Why are we doing things differently just because all of the pins on this port happen to be directed to one device? If it's because the Linux GPIO API sucks, then either fix the API, or work around the suckage in software, not the device tree.

arch/powerpc/boot/dts/dbox2.dts line 195

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";
+			};

So some driver that matches on samsung,ks0713 has to know the details of the mpc8xx GPIO registers?

arch/powerpc/boot/dts/dbox2.dts line 202

Quoted 6 lines
arch/powerpc/boot/dts/dbox2.dts new file +6 -0
+			brg@9f0 {
+				compatible = "fsl,mpc823-brg",
+					     "fsl,cpm1-brg",
+					     "fsl,cpm-brg";
+				reg = <9f0 10>;
+			};

Should have clock-frequency in the brg node.

arch/powerpc/boot/dts/dbox2.dts line 257

Quoted 9 lines
arch/powerpc/boot/dts/dbox2.dts new file +9 -0
+			i2c@860 {
+				compatible = "fsl,mpc823-i2c",
+					     "fsl,cpm1-i2c",
+					     "fsl,cpm-i2c";
+				reg = <860 20 3c80 30>;
+				interrupts = <10 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fsl,cpm-command = <0010>;
+			};

Should have #address-cells and #size-cells.

arch/powerpc/platforms/8xx/dbox2_setup.c line 118

Quoted 5 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +5 -0
+enum dbox2_mid dbox2_get_mid(void)
+{
+	return dbox2_manuf_id;
+}
+EXPORT_SYMBOL_GPL(dbox2_get_mid);

Honestly, you're claiming derived-work status by calling a function that returns an integer, that was read directly from hardware?

Quoted 10 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +10 -0
+	if (dbox2_manuf_id == DBOX2_MID_NOKIA)
+		np = of_find_node_by_path("/localbus@8000000/enx@0");
+	else
+		np = of_find_node_by_path("/localbus@8000000/gtx@0");
+
+	if (np) {
+		of_detach_node(np);
+		of_node_put(np);
+	}
+

arch/powerpc/platforms/8xx/dbox2_setup.c line 189

Quoted 10 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +10 -0
+	if (dbox2_manuf_id == DBOX2_MID_PHILIPS)
+		np = of_find_node_by_path("/localbus@8000000/cam@4000000");
+	else
+		np = of_find_node_by_path("/localbus@8000000/cam@4040000");
+
+	if (np) {
+		of_detach_node(np);
+		of_node_put(np);
+	}
+}

I'd use separate device trees (I only did this kind of thing in mpc885ads because it's dip-switchable), but whatever...

include/asm-powerpc/mpc8xx.h line 28

Quoted 11 lines
include/asm-powerpc/mpc8xx.h +3 -0
index 2be014b..b6fd7d6 100644
@@ -23,6 +23,10 @@
 #include <platforms/8xx/mpc885ads.h>
 #endif
 
+#if defined(CONFIG_DBOX2)
+#include <platforms/8xx/dbox2.h>
+#endif

This shouldn't be needed.

-Scott

#8 lore raw

Hi Scott,

Quoted 1 line

These unit addresses look wrong.

I know. These have (hopefully) been fixed in v3.

Quoted 1 line

What does dbox2-config mean?

I used it to search for the vendor info byte in the flash. It has been moved to the boot wrapper in v3. Later this should all be moved to u-boot, IMHO.

Quoted 11 lines

arch/powerpc/boot/dts/dbox2.dts line 132

Quoted 9 lines
arch/powerpc/boot/dts/dbox2.dts new file +9 -0
+			ovpartition@20000 {
+				label = "Flash without bootloader";
+				reg = <20000 7e0000>;
+			};
+			ovpartition@0 {
+				label = "Complete Flash";
+				reg = <0 800000>;
+				read-only;
+			};

What is "ovpartition"?

Overlay partition. Is there a better way to specify this?

Quoted 13 lines

arch/powerpc/boot/dts/dbox2.dts line 190

Quoted 7 lines
arch/powerpc/boot/dts/dbox2.dts new file +7 -0
+			// Port D is LCD exclusive. Don't export as GPIO
+			CPM1_PIO: pio@970 {
+				compatible = "fsl,cpm1-pario";
+				reg = <970 180>;
+				num-ports = <3>;
+				#gpio-cells = <2>;
+			};

Why are we doing things differently just because all of the pins on this port happen to be directed to one device? If it's because the Linux GPIO API sucks, then either fix the API, or work around the suckage in software, not the device tree.

Quoted 7 lines

arch/powerpc/boot/dts/dbox2.dts line 195

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+			lcd@970 {
+				reg = <970 10>;
+				compatible = "samsung,ks0713";
+			};

So some driver that matches on samsung,ks0713 has to know the details of the mpc8xx GPIO registers?

The GPIO API only has an accessor for 1bit I/O. The LCD, however has for 1bit control lines and an 8bit port. The GPIO API currently is unable to handle this.

Quoted 11 lines

arch/powerpc/boot/dts/dbox2.dts line 257

Quoted 9 lines
arch/powerpc/boot/dts/dbox2.dts new file +9 -0
+			i2c@860 {
+				compatible = "fsl,mpc823-i2c",
+					     "fsl,cpm1-i2c",
+					     "fsl,cpm-i2c";
+				reg = <860 20 3c80 30>;
+				interrupts = <10 3>;
+				interrupt-parent = <&CPM_PIC>;
+				fsl,cpm-command = <0010>;
+			};

Should have #address-cells and #size-cells.

In the meantime, i use the i2c patches from Jon Smirl and have added these parameters.

Quoted 9 lines

arch/powerpc/platforms/8xx/dbox2_setup.c line 118

Quoted 6 lines
arch/powerpc/platforms/8xx/dbox2_setup.c new file +5 -0
+enum dbox2_mid dbox2_get_mid(void)
+{
+	return dbox2_manuf_id;
+}
+EXPORT_SYMBOL_GPL(dbox2_get_mid);

Honestly, you're claiming derived-work status by calling a function that returns an integer, that was read directly from hardware?

Nope. It was a big fat hint not to use this value :). It has been moved to the device tree in the mean time.

Quoted 2 lines

I'd use separate device trees (I only did this kind of thing in mpc885ads because it's dip-switchable), but whatever...

I don't really like having to use different images on different DBoxes. It's already too much to have two images (there are two different flash layouts). People tend to brick their box by flashing the wrong image.

Quoted 11 lines

include/asm-powerpc/mpc8xx.h line 28

Quoted 9 lines
include/asm-powerpc/mpc8xx.h +3 -0
@@ -23,6 +23,10 @@
 #include <platforms/8xx/mpc885ads.h>
 #endif
 
+#if defined(CONFIG_DBOX2)
+#include <platforms/8xx/dbox2.h>
+#endif

This shouldn't be needed.

Already gone in v3.

Quoted 1 line

What happens if a device sits on two different gpio-parents?

On the dbox2, there are even stranger devices. One is using iomaped address spaces, GPIO and an i2c control interface.
Another one uses GPIO and an i2c control interface. For now, they appear twice in the device tree (once under i2c and once under localbus or pio). Is there a better way to do this?

localbus@8000000 {
	cam@3,0 {
		compatible = "betaresearch,dbox2-cam";
		reg = <3 0 20000 6 0 20000>;
		interrupts = <6 2>;
		interrupt-parent = <&PIC>;
		gpios = <1 1c 1 1d 1 1e 1 1f>;
		gpio-parent = <&CPM1_PIO>;
	};
};
soc@ff000000 {
	cpm@9c0 {
		CPM1_PIO: pio@950 {
			fp@0,0 {
				compatible = "betaresearch,dbox2-fp";
				interrupts = <4 2>;
				interrupt-parent = <&PIC>;
				gpios = <0 e>;
				gpio-parent = <&CPM1_PIO>;
			};
		};
		i2c@860 {
			fp@30 {
				compatible = "betaresearch,dbox2-fp";
				reg = <30>;
			};
			cam@37 {
				compatible = "betaresearch,dbox2-cam";
				reg = <37>;
			};			
		};
	};
};

Thanks,
Jochen

#9 lore raw
Quoted 10 lines · Jochen Friedrich
Quoted 10 lines
Quoted 9 lines
arch/powerpc/boot/dts/dbox2.dts new file +9 -0
+            ovpartition@20000 {
+                label = "Flash without bootloader";
+                reg = <20000 7e0000>;
+            };
+            ovpartition@0 {
+                label = "Complete Flash";
+                reg = <0 800000>;
+                read-only;
+            };
Quoted 3 lines
Quoted 1 line

What is "ovpartition"?

Overlay partition. Is there a better way to specify this?

Not sure... what will the mtd code do with this?

Quoted 10 lines
Quoted 7 lines

arch/powerpc/boot/dts/dbox2.dts line 195

Quoted 4 lines
arch/powerpc/boot/dts/dbox2.dts new file +4 -0
+            lcd@970 {
+                reg = <970 10>;
+                compatible = "samsung,ks0713";
+            };

So some driver that matches on samsung,ks0713 has to know the details of the mpc8xx GPIO registers?

The GPIO API only has an accessor for 1bit I/O. The LCD, however has for 1bit control lines and an 8bit port. The GPIO API currently is unable to handle this.

Right, but we shouldn't weird up the device tree because of Linux's weaknesses. Ideally, the GPIO API should support wider I/O; in the meantime, there should be something in the board file that tells the ks0713 driver what to do.

Quoted 6 lines
Quoted 2 lines

I'd use separate device trees (I only did this kind of thing in mpc885ads because it's dip-switchable), but whatever...

I don't really like having to use different images on different DBoxes. It's already too much to have two images (there are two different flash layouts). People tend to brick their box by flashing the wrong image.

Fair enough.

Quoted 25 lines
localbus@8000000 {
    cam@3,0 {
        compatible = "betaresearch,dbox2-cam";
        reg = <3 0 20000 6 0 20000>;
        interrupts = <6 2>;
        interrupt-parent = <&PIC>;
        gpios = <1 1c 1 1d 1 1e 1 1f>;
        gpio-parent = <&CPM1_PIO>;
    };
};
soc@ff000000 {
    cpm@9c0 {
        CPM1_PIO: pio@950 {
            fp@0,0 {
                compatible = "betaresearch,dbox2-fp";
                interrupts = <4 2>;
                interrupt-parent = <&PIC>;
                gpios = <0 e>;
                gpio-parent = <&CPM1_PIO>;
            };
        };
        i2c@860 {
            fp@30 {
                compatible = "betaresearch,dbox2-fp";
                reg = <30>;
Quoted 8 lines
            };
            cam@37 {
                compatible = "betaresearch,dbox2-cam";
                reg = <37>;
            };           
        };
    };
};

I'd make the compatible name specific to the interface (e.g. "betaresearch,dbox2-cam-mmio", "betaresearch,dbox2-fp-i2c"), use reg rather than gpios/gpio-parent, and use phandle linkage between the different inteferfaces of a device. But yes, multiple nodes is the sanest way to do it.

-Scott