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

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

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 <haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../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.

>  5 files changed, 492 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/marvell/boards.txt
>  create mode 100644 arch/arm/boot/dts/mmp2-brownstone.dts
>  create mode 100644 arch/arm/boot/dts/ttc-dkb.dts
>  create mode 100644 arch/arm/mach-mmp/boards.c
> 
> diff --git a/Documentation/devicetree/bindings/arm/marvell/boards.txt b/Documentation/devicetree/bindings/arm/marvell/boards.txt
> new file mode 100644
> index 0000000..219e134
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/marvell/boards.txt
> @@ -0,0 +1,7 @@
> +TTC(pxa910) "DKB" evalutation board
> +Required root node properties:
> +	- compatible = "mrvl,ttc-dkb", "mrvl,pxa910-dkb";
> +
> +mmp2(armada610) "Brownstone" evalutation board
> +Required root node properties:
> +	- compatible = "mrvl,mmp2-brownstone", "mrvl,armada610-brownstone";

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.

> diff --git a/arch/arm/boot/dts/mmp2-brownstone.dts b/arch/arm/boot/dts/mmp2-brownstone.dts
> new file mode 100644
> index 0000000..4e14388
> --- /dev/null
> +++ b/arch/arm/boot/dts/mmp2-brownstone.dts
> @@ -0,0 +1,242 @@
> +/dts-v1/;
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	model = "Marvell MMP2 Brownstone";
> +	compatible = "mrvl,mmp2-brownstone", "mrvl,armada610-brownstone";

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

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> diff --git a/arch/arm/mach-mmp/Makefile b/arch/arm/mach-mmp/Makefile
> index e7862ea..6c39bbd 100644
> --- a/arch/arm/mach-mmp/Makefile
> +++ b/arch/arm/mach-mmp/Makefile
> @@ -12,6 +12,9 @@ obj-$(CONFIG_CPU_PXA910)	+= pxa910.o irq-pxa168.o
>  obj-$(CONFIG_CPU_MMP2)		+= mmp2.o irq-mmp2.o
>  
>  # board support
> +ifeq ($(CONFIG_MMP_USE_OF),y)
> +obj-$(CONFIG_OF)		+= boards.o
> +else
>  obj-$(CONFIG_MACH_ASPENITE)	+= aspenite.o
>  obj-$(CONFIG_MACH_ZYLONITE2)	+= aspenite.o
>  obj-$(CONFIG_MACH_AVENGERS_LITE)+= avengers_lite.o
> @@ -21,3 +24,4 @@ obj-$(CONFIG_MACH_BROWNSTONE)	+= brownstone.o
>  obj-$(CONFIG_MACH_FLINT)	+= flint.o
>  obj-$(CONFIG_MACH_MARVELL_JASPER) += jasper.o
>  obj-$(CONFIG_MACH_TETON_BGA)	+= teton_bga.o
> +endif

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.

> diff --git a/arch/arm/mach-mmp/boards.c b/arch/arm/mach-mmp/boards.c
> new file mode 100644
> index 0000000..31c8e84
> --- /dev/null
> +++ b/arch/arm/mach-mmp/boards.c
> @@ -0,0 +1,159 @@
> +/*
> + *  linux/arch/arm/mach-mmp/boards.c
> + *
> + *  Support for the Multiple Marvell Development Platforms.
> + *
> + *  Copyright (C) 2009-2011 Marvell International Ltd.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  publishhed by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c/pxa-i2c.h>
> +
> +#include <asm/mach-types.h>
> +#include <asm/mach/arch.h>
> +
> +#include <mach/pxa910.h>
> +#include <mach/mmp2.h>
> +#include <mach/mfp-mmp2.h>
> +
> +#include "common.h"
> +
> +static struct of_device_id of_bus_ids[] __initdata = {
> +	{ .compatible = "simple-bus", },
> +	{},
> +};

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.