Re: [PATCH 3/7] ARM: mmp: support DT on both dkb and brownstone

On Tue, Jul 19, 2011 at 10:24:46AM +0800, Haojian Zhuang wrote:

Add new boards.c to support both TTC-DKB and MMP2-BROWNSTONE. While
CONFIG_MMP_USE_OF is selected, original ttc_dkb.c and brownstone.c won't be
compiled.

While everything moving to DT in ARCH-MMP, original ttc_dkb.c and brownstone.c
will be abandoned.

Signed-off-by: Haojian Zhuang <redacted>
---
 .../devicetree/bindings/arm/marvell/boards.txt     |    7 +
 arch/arm/boot/dts/mmp2-brownstone.dts              |  242 ++++++++++++++++++++
 arch/arm/boot/dts/ttc-dkb.dts                      |   80 +++++++
 arch/arm/mach-mmp/Makefile                         |    4 +
 arch/arm/mach-mmp/boards.c                         |  159 +++++++++++++

I would suggest arch/arm/mach-mmp/board-dt.c.  That is the pattern
that has been used for several other socs.

What do these individual values mean?  Typically a board should have 2
values, the exact name of the board/system, and the exact name of the
SoC used by the board.  The board.c file will normally match on the
soc value.

Missing #address-cells and #size-cells in the root node.

If you add an interrupt-parent = <&mmp_intc>; property here, then it
can be removed from all the nodes below because child nodes inherit
interrupt-parent from their parent node.

+
+	memory {
+		reg = <0x00000000 0x20000000>;
+	};
+
+	chosen {
+		bootargs = "console=ttyS2,38400 root=/dev/nfs nfsroot=192.168.1.100:192.168.1.101::255.255.255.0::eth0:on";
+		linux,stdout-path = &uart2;
+	};
+
+	soc@d4000000 {
+		compatible = "mrvl,mmp2", "mrvl,armada610", "simple-bus";
+		device_type = "soc";

Drop device_type

The else/endif clause breaks multiplatform kernels.  There is no need
to do this.  It should be possible to select both DT and non-DT
platforms.

Just use of_default_bus_match_table.

+
+static struct of_dev_auxdata ttc_dkb_auxdata_lookup[] __initdata = {
+	{}
+};

If the auxdata table is empty, then pass NULL to
of_platform_populate().

+
+static void __init ttc_dkb_init(void)
+{
+	if (of_platform_populate(NULL, of_bus_ids, ttc_dkb_auxdata_lookup,
+		NULL) < 0)
+		BUG();

I wouldn't even bother with checking the return code.  If it fails,
we're borked anyway.

+}
+
+static const char *ttc_dkb_dt_match[] __initdata = {
+	"mrvl,ttc-dkb",
+	NULL,
+};
+
+#ifdef CONFIG_CPU_PXA910
+MACHINE_START(TTC_DKB, "PXA910-based TTC-DKB Development Platform")

DT_MACHINE_START()

+	.map_io		= mmp_map_io,
+	.init_irq	= mmp_init_intc,
+	.timer		= &pxa910_timer,
+	.init_machine	= ttc_dkb_init,
+	.dt_compat	= ttc_dkb_dt_match,
+MACHINE_END
+#endif
+
+static unsigned long brownstone_pin_config[] __initdata = {
+	/* UART1 */
+	GPIO29_UART1_RXD,
+	GPIO30_UART1_TXD,
+
+	/* UART3 */
+	GPIO51_UART3_RXD,
+	GPIO52_UART3_TXD,
+
+	/* DFI */
+	GPIO168_DFI_D0,
+	GPIO167_DFI_D1,
+	GPIO166_DFI_D2,
+	GPIO165_DFI_D3,
+	GPIO107_DFI_D4,
+	GPIO106_DFI_D5,
+	GPIO105_DFI_D6,
+	GPIO104_DFI_D7,
+	GPIO111_DFI_D8,
+	GPIO164_DFI_D9,
+	GPIO163_DFI_D10,
+	GPIO162_DFI_D11,
+	GPIO161_DFI_D12,
+	GPIO110_DFI_D13,
+	GPIO109_DFI_D14,
+	GPIO108_DFI_D15,
+	GPIO143_ND_nCS0,
+	GPIO144_ND_nCS1,
+	GPIO147_ND_nWE,
+	GPIO148_ND_nRE,
+	GPIO150_ND_ALE,
+	GPIO149_ND_CLE,
+	GPIO112_ND_RDY0,
+	GPIO160_ND_RDY1,
+
+	/* PMIC */
+	PMIC_PMIC_INT | MFP_LPM_EDGE_FALL,
+
+	/* MMC0 */
+	GPIO131_MMC1_DAT3 | MFP_PULL_HIGH,
+	GPIO132_MMC1_DAT2 | MFP_PULL_HIGH,
+	GPIO133_MMC1_DAT1 | MFP_PULL_HIGH,
+	GPIO134_MMC1_DAT0 | MFP_PULL_HIGH,
+	GPIO136_MMC1_CMD | MFP_PULL_HIGH,
+	GPIO139_MMC1_CLK,
+	GPIO140_MMC1_CD | MFP_PULL_LOW,
+	GPIO141_MMC1_WP | MFP_PULL_LOW,
+
+	/* MMC1 */
+	GPIO37_MMC2_DAT3 | MFP_PULL_HIGH,
+	GPIO38_MMC2_DAT2 | MFP_PULL_HIGH,
+	GPIO39_MMC2_DAT1 | MFP_PULL_HIGH,
+	GPIO40_MMC2_DAT0 | MFP_PULL_HIGH,
+	GPIO41_MMC2_CMD | MFP_PULL_HIGH,
+	GPIO42_MMC2_CLK,
+
+	/* MMC2 */
+	GPIO165_MMC3_DAT7 | MFP_PULL_HIGH,
+	GPIO162_MMC3_DAT6 | MFP_PULL_HIGH,
+	GPIO166_MMC3_DAT5 | MFP_PULL_HIGH,
+	GPIO163_MMC3_DAT4 | MFP_PULL_HIGH,
+	GPIO167_MMC3_DAT3 | MFP_PULL_HIGH,
+	GPIO164_MMC3_DAT2 | MFP_PULL_HIGH,
+	GPIO168_MMC3_DAT1 | MFP_PULL_HIGH,
+	GPIO111_MMC3_DAT0 | MFP_PULL_HIGH,
+	GPIO112_MMC3_CMD | MFP_PULL_HIGH,
+	GPIO151_MMC3_CLK,
+
+	/* 5V regulator */
+	GPIO89_GPIO,
+};
+
+static struct of_dev_auxdata brownstone_auxdata_lookup[] __initdata = {
+	{}
+};

Ditto here.  Drop empty auxdata.

+
+static void __init brownstone_init(void)
+{
+	mfp_config(ARRAY_AND_SIZE(brownstone_pin_config));
+
+	if (of_platform_populate(NULL, of_bus_ids, brownstone_auxdata_lookup,
+		NULL) < 0)
+		BUG();
+}
+
+static const char *brownstone_dt_match[] __initdata = {
+	"mrvl,mmp2-brownstone",
+	NULL,
+};
+
+#ifdef CONFIG_CPU_MMP2
+MACHINE_START(BROWNSTONE, "Brownstone Development Platform")
+	.map_io		= mmp_map_io,
+	.init_irq	= mmp_init_intc,
+	.timer		= &mmp2_timer,
+	.init_machine	= brownstone_init,
+	.dt_compat	= brownstone_dt_match,
+MACHINE_END
+#endif

Brownstone and dkb should share the same DT_MACHINE_START() section.
The only difference is the pinmux setup which is eventually going to
be populated from the DT, right?

g.