Hi Scott,

Thanks for you feedback.

On 01/17/2014 12:35 AM, Scott Wood wrote:

On Thu, 2014-01-16 at 14:38 +0100, Valentin Longchamp wrote:

quoted

This patch introduces the support for Keymile's kmp204x reference
design. This design is based on Freescale's P2040/P2041 SoC.

The peripherals used by this design are:
- SPI NOR Flash as bootloader medium
- NAND Flash with a ubi partition
- 2 PCIe busses (hosts 1 and 3)
- 3 FMAN Ethernet devices (FMAN1 DTSEC1/2/5)
- 4 Local Bus windows, with one dedicated to the QRIO reset/power mgmt
  FPGA
- 2 HW I2C busses
- last but not least, the mandatory serial port

The patch also adds a defconfig file for this reference design and a DTS
file for the kmcoge4 board which is the first one based on this
reference design.

To try to avoid code duplication, the support was added directly to the
corenet_generic.c file.

Signed-off-by: Valentin Longchamp <redacted>
---
 arch/powerpc/boot/dts/kmcoge4.dts             | 165 ++++++++++++++++++
 arch/powerpc/configs/85xx/kmp204x_defconfig   | 231 ++++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/Kconfig           |  14 ++
 arch/powerpc/platforms/85xx/Makefile          |   1 +
 arch/powerpc/platforms/85xx/corenet_generic.c |  52 ++++++
 5 files changed, 463 insertions(+)
 create mode 100644 arch/powerpc/boot/dts/kmcoge4.dts
 create mode 100644 arch/powerpc/configs/85xx/kmp204x_defconfig

diff --git a/arch/powerpc/boot/dts/kmcoge4.dts b/arch/powerpc/boot/dts/kmcoge4.dts
new file mode 100644
index 0000000..c10df6d
--- /dev/null
+++ b/arch/powerpc/boot/dts/kmcoge4.dts

@@ -0,0 +1,165 @@
+/*
+ * Keymile kmcoge4 Device Tree Source, based on the P2041RDB DTS
+ *
+ * (C) Copyright 2014
+ * Valentin Longchamp, Keymile AG, valentin.longchamp@keymile.com
+ *
+ * Copyright 2011 Freescale Semiconductor Inc.
+ *
+ * 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.
+ */
+
+/include/ "fsl/p2041si-pre.dtsi"
+
+/ {
+	model = "keymile,kmcoge4";
+	compatible = "keymile,kmp204x";

Don't put wildcards in compatible.

Well it's a wildcard in the sense that we support both the p2040 and the p2041,
but it's also the name of the plaftorm, similarly to names like '85xx' or 'tqm85xx'.

quoted

+	soc: soc@ffe000000 {
+		ranges = <0x00000000 0xf 0xfe000000 0x1000000>;
+		reg = <0xf 0xfe000000 0 0x00001000>;
+		spi@110000 {
+			flash@0 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				compatible = "spansion,s25fl256s1";
+				reg = <0>;
+				spi-max-frequency = <20000000>; /* input clock */
+				partition@u-boot {
+					label = "u-boot";
+					reg = <0x00000000 0x00100000>;
+					read-only;
+				};
+				partition@env {
+					label = "env";
+					reg = <0x00100000 0x00010000>;
+				};
+				partition@envred {
+					label = "envred";
+					reg = <0x00110000 0x00010000>;
+				};
+				partition@fman {
+					label = "fman-ucode";
+					reg = <0x00120000 0x00010000>;
+					read-only;
+				};
+			};

I realize it's common practice, but it would be good to get away from
putting partition layouts in the dts file.  Alternatives include using
mtdparts on the command line, or having U-Boot put the partition info
into the dtb based on the mtdparts environment variable (there is
existing code for this).

I agree that u-boot also has to know about the addresses because it also
accesses these partitions.

But I think it is clearer to have this in the device tree: I try to keep the
kernel command line small and I don't like having u-boot "fixing" the dtb at
runtime.

quoted

+			zl30343@1 {
+				compatible = "gen,spidev";

Node names are supposed to be generic.  Compatibles are supposed to be
specific.

That's a very specific device for which we only have a userspace driver and for
which we must use the generic kernel spidev driver. That's why the node name is
so specific and the compatible field very generic.

quoted

+	lbc: localbus@ffe124000 {
+		reg = <0xf 0xfe124000 0 0x1000>;
+		ranges = <0 0 0xf 0xffa00000 0x00040000		/* LB 0 */
+			  1 0 0xf 0xfb000000 0x00010000		/* LB 1 */
+			  2 0 0xf 0xd0000000 0x10000000		/* LB 2 */
+			  3 0 0xf 0xe0000000 0x10000000>;	/* LB 3 */
+
+		nand@0,0 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "fsl,elbc-fcm-nand";
+			reg = <0 0 0x40000>;
+
+			partition@0 {
+				label = "ubi0";
+				reg = <0x0 0x8000000>;
+			};
+		};
+	};

No nodes for those other chipselects?

Well, there are nodes, but they are internally developed FPGAs and the drivers
are not mainlined that's why I removed the nodes.

quoted

diff --git a/arch/powerpc/configs/85xx/kmp204x_defconfig b/arch/powerpc/configs/85xx/kmp204x_defconfig
new file mode 100644
index 0000000..3bbf4fa
--- /dev/null
+++ b/arch/powerpc/configs/85xx/kmp204x_defconfig

Why does this board need its own defconfig?

Apart from the different drivers and FS that we use (or don't use) on the
system, the most notable differences are:
- lowmem must be set to a bigger size so that we can ioremap the the total
memory requested for all of our PCIe devices
- CGROUPS is enabled because that's a mandatory feature for our systems
- NAND_ECC_BCH is enabled because it is used on all of our NAND devices

quoted

diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index fbd871e..8e84e1c 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c

@@ -122,6 +122,7 @@ static const char * const hv_boards[] __initconst = {
 	NULL
 };
 
+#ifdef CONFIG_CORENET_GENERIC

corenet_generic.c without CONFIG_CORENET_GENERIC?

It would be possible with the CONFIG_KMP204X introduced with the patch. But this
is linked with the below discussion.

quoted

 /*
  * Called very early, device-tree isn't unflattened
  */

@@ -180,3 +181,54 @@ machine_arch_initcall(corenet_generic, corenet_gen_publish_devices);
 #ifdef CONFIG_SWIOTLB
 machine_arch_initcall(corenet_generic, swiotlb_setup_bus_notifier);
 #endif
+#endif
+
+#ifdef CONFIG_KMP204X
+/*
+ * Called very early, device-tree isn't unflattened
+ */
+static int __init kmp204x_generic_probe(void)
+{
+	unsigned long root = of_get_flat_dt_root();
+
+	return of_flat_dt_is_compatible(root, "keymile,kmp204x");
+}
+
+
+/*
+ * Setup the architecture
+ */
+void __init kmp204x_gen_setup_arch(void)
+{
+	mpc85xx_smp_init();
+
+	swiotlb_detect_4g();
+
+	pr_info("%s platform from Keymile\n", ppc_md.name);
+}
+
+define_machine(kmp204x) {
+	.name			= "kmp204x",
+	.probe			= kmp204x_generic_probe,
+	.setup_arch		= kmp204x_gen_setup_arch,
+	.init_IRQ		= corenet_gen_pic_init,
+#ifdef CONFIG_PCI
+	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+#endif
+	.get_irq		= mpic_get_coreint_irq,
+	.restart		= fsl_rstcr_restart,
+	.calibrate_decr		= generic_calibrate_decr,
+	.progress		= udbg_progress,
+#ifdef CONFIG_PPC64
+	.power_save		= book3e_idle,
+#else
+	.power_save		= e500_idle,
+#endif
+};
+
+machine_arch_initcall(kmp204x, corenet_gen_publish_devices);
+
+#ifdef CONFIG_SWIOTLB
+machine_arch_initcall(kmp204x, swiotlb_setup_bus_notifier);
+#endif
+#endif

The whole point of corenet_generic.c is to avoid duplicating all of this
stuff.

Can't you just use corenet_generic as-is other than adding the
compatible to boards[]?  If not, explain why and put it in a different
file.

That's a valid point and I have to admit I have hesitated about that. I have
mostly based my work on the FSL SDK where every single board has a "dedicated" file.

I agree that I do nothing different than the corenet_generic does and adding my
platform to the boards[] would be the same and you are right, I should use that
and avoid code duplication.

The only thing that would "bother" me is thus the pr_info print from
*_gen_setup_arch(), it would be nice if somehow we could differentiate it or at
least make it more generic since the kmp204x boards are not strictly boards from
Freescale.

Best regards

Valentin