Patchwork [1/2] ARM: i.MX35: Add kernel oftree support

login
register
mail settings
Submitter Uwe Kleine-König
Date Aug. 2, 2012, 3:56 p.m.
Message ID <1343922986-32469-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/174776/
State New
Headers show

Comments

Uwe Kleine-König - Aug. 2, 2012, 3:56 p.m.
From: Steffen Trumtrar <s.trumtrar@pengutronix.de>

This patch adds basic support for imx35-based devices to the kernel.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/boot/dts/imx35.dtsi            |  246 +++++++++++++++++++++++++++++++
 arch/arm/configs/imx_v6_v7_defconfig    |    1 +
 arch/arm/mach-imx/Kconfig               |   13 ++
 arch/arm/mach-imx/Makefile              |    1 +
 arch/arm/mach-imx/imx35-dt.c            |  119 +++++++++++++++
 arch/arm/plat-mxc/include/mach/common.h |    2 +
 6 files changed, 382 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx35.dtsi
 create mode 100644 arch/arm/mach-imx/imx35-dt.c
Sascha Hauer - Aug. 3, 2012, 1:05 p.m.
On Thu, Aug 02, 2012 at 05:56:25PM +0200, Uwe Kleine-König wrote:
> From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> This patch adds basic support for imx35-based devices to the kernel.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx35.dtsi            |  246 +++++++++++++++++++++++++++++++
>  arch/arm/configs/imx_v6_v7_defconfig    |    1 +
>  arch/arm/mach-imx/Kconfig               |   13 ++
>  arch/arm/mach-imx/Makefile              |    1 +
>  arch/arm/mach-imx/imx35-dt.c            |  119 +++++++++++++++
>  arch/arm/plat-mxc/include/mach/common.h |    2 +
>  6 files changed, 382 insertions(+)
>  create mode 100644 arch/arm/boot/dts/imx35.dtsi
>  create mode 100644 arch/arm/mach-imx/imx35-dt.c
> 

> diff --git a/arch/arm/mach-imx/imx35-dt.c b/arch/arm/mach-imx/imx35-dt.c
> new file mode 100644
> index 0000000..8b9cc84
> --- /dev/null
> +++ b/arch/arm/mach-imx/imx35-dt.c
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar, Pengutronix
> + *
> + * based on imx27-dt.c
> + *
> + * 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 published by the
> + * Free Software Foundation.
> + */
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/machine.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/time.h>
> +#include <mach/common.h>
> +#include <mach/mx35.h>
> +
> +static const struct of_dev_auxdata imx35_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART1_BASE_ADDR, "imx21-uart.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART2_BASE_ADDR, "imx21-uart.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART3_BASE_ADDR, "imx21-uart.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-fec", MX35_FEC_BASE_ADDR, "imx27-fec.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-wdt", MX35_WDOG_BASE_ADDR, "imx2-wdt.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C1_BASE_ADDR, "imx-i2c.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C2_BASE_ADDR, "imx-i2c.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C3_BASE_ADDR, "imx-i2c.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC1_BASE_ADDR, "sdhci-esdhc-imx35.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC2_BASE_ADDR, "sdhci-esdhc-imx35.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC3_BASE_ADDR, "sdhci-esdhc-imx35.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-nand", MX35_NFC_BASE_ADDR, "mxc_nand.0", NULL),
> +	{ /* sentinel */ }
> +};
> +
> +static int __init imx35_avic_add_irq_domain(struct device_node *np,
> +				struct device_node *interrupt_parent)
> +{
> +	irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> +	return 0;
> +}
> +
> +static int __init imx35_gpio_add_irq_domain(struct device_node *np,
> +				struct device_node *interrupt_parent)
> +{
> +	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +
> +	gpio_irq_base -= 32;
> +	irq_domain_add_legacy(np, 32, gpio_irq_base, 0, &irq_domain_simple_ops,
> +				NULL);
> +
> +	return 0;
> +}

The gpio/irq drivers now call irq_domain_add_legacy themselves.

> +
> +static const struct of_device_id imx35_irq_match[] __initconst = {
> +	{ .compatible = "fsl,imx35-avic", .data = imx35_avic_add_irq_domain, },
> +	{ .compatible = "fsl,imx35-gpio", .data = imx35_gpio_add_irq_domain, },
> +	{ /* sentinel */ }
> +};
> +
> +static const struct of_device_id imx35_of_mixed_devices_match[] __initconst = {
> +	{ /* END OF LIST */ }
> +};
> +
> +static void __init of_mixed_device_hook(void)
> +{
> +	struct device_node *np;
> +	const struct of_device_id *of_id;
> +
> +	np = of_find_matching_node(NULL, imx35_of_mixed_devices_match);
> +	if (np) {
> +		void (*func)(void);
> +
> +		of_id = of_match_node(imx35_of_mixed_devices_match, np);
> +		func = of_id->data;
> +		func();
> +	}
> +}

What is the intention of this? I suppose currently it does nothing,
right?

Sascha
Shawn Guo - Aug. 3, 2012, 1:12 p.m.
On Thu, Aug 02, 2012 at 05:56:25PM +0200, Uwe Kleine-König wrote:
> From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> This patch adds basic support for imx35-based devices to the kernel.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx35.dtsi            |  246 +++++++++++++++++++++++++++++++
>  arch/arm/configs/imx_v6_v7_defconfig    |    1 +
>  arch/arm/mach-imx/Kconfig               |   13 ++
>  arch/arm/mach-imx/Makefile              |    1 +
>  arch/arm/mach-imx/imx35-dt.c            |  119 +++++++++++++++
>  arch/arm/plat-mxc/include/mach/common.h |    2 +
>  6 files changed, 382 insertions(+)
>  create mode 100644 arch/arm/boot/dts/imx35.dtsi
>  create mode 100644 arch/arm/mach-imx/imx35-dt.c
> 
> diff --git a/arch/arm/boot/dts/imx35.dtsi b/arch/arm/boot/dts/imx35.dtsi
> new file mode 100644
> index 0000000..d0c6456
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx35.dtsi
> @@ -0,0 +1,246 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar, Pengutronix
> + *
> + * based on imx27.dtsi
> + *
> + * 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 published by the
> + * Free Software Foundation.
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	aliases {
> +		serial0 = &uart1;
> +		serial1 = &uart2;
> +		serial2 = &uart3;
> +	};
> +
> +	avic: avic-interrupt-controller@68000000 {
> +		compatible = "fsl,imx35-avic", "fsl,avic";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0x68000000 0x10000000>;
> +	};
> +
> +	clocks {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ckil {
> +			compatible = "fsl,imx-ckil", "fixed-clock";
> +			clock-frequency = <32768>;
> +		};
> +
> +		osc {
> +			compatible = "fsl,imx-osc", "fixed-clock";
> +			clock-frequency = <24000000>;
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu@0 {
> +			device_type = "cpu";

This is not really necessary, IIRC.

> +			compatible = "fsl,imx35", "arm,arm1136jfs";
> +			reg = <0>;
> +			d-cache-size = <0x4000>;	// L1, 16K
> +			i-cache-size = <0x4000>;	// L1, 16K

Are these two common bindings?  Documented in
Documentation/devicetree/bindings for somewhere?

> +			bus-frequency = <0>;		// from bootloader

Ditto.  I'm not even sure how this works.  Does your bootloader
really populates this property?  Which bus?

> +		};
> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		interrupt-parent = <&avic>;
> +		ranges;
> +
> +		aips@40000000 { /* AIPS1 */
> +			compatible = "fsl,aips", "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x40000000 0x100000>;
> +			ranges;
> +
> +			i2c1: i2c@43f80000 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> +				reg = <0x43f80000 0x4000>;
> +				interrupts = <10>;
> +				status = "disabled";
> +			};
> +
> +			i2c3: i2c@43f84000 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> +				reg = <0x43f84000 0x4000>;
> +				interrupts = <3>;
> +				status = "disabled";
> +			};
> +
> +			uart1: uart@43f90000 {

Name the node "serial".

> +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> +				reg = <0x43f90000 0x4000>;
> +				interrupts = <45>;
> +				status = "disabled";
> +			};
> +
> +			uart2: uart@43f94000 {
> +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> +				reg = <0x43f94000 0x4000>;
> +				interrupts = <32>;
> +				status = "disabled";
> +			};
> +
> +			i2c2: i2c@43f98000 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> +				reg = <0x43f98000 0x4000>;
> +				interrupts = <4>;
> +				status = "disabled";
> +			};

We generally have a new line between nodes.

> +			iomuxc@43fac000 {
> +				compatible = "fsl,imx35-iomuxc";
> +				reg = <0x43fac000 0x4000>;
> +
> +				i2c3 {
> +					pinctrl_i2c3_1: i2c3grp-1 {
> +						fsl,pins = <773 0x1c0 /* MX35_PAD_ATA_DATA12__I2C3_SCL */
> +							777 0x1c0>; /* MX35_PAD_ATA_DATA13__I2C3_SDA */
> +					};
> +				};
> +
> +				can1 {
> +					pinctrl_can1_1: can1grp-1 {
> +						fsl,pins = <223 0x1c0 /* MX35_PAD_I2C2_CLK__CAN1_TXCAN */
> +							228 0x1c0>; /* MX35_PAD_I2C2_DAT__CAN1_RXCAN */
> +					};
> +				};
> +				can2 {
> +					pinctrl_can2_1: can2grp-1 {
> +						fsl,pins = <291 0x1c0 /* MX35_PAD_TX5_RX0__CAN2_TXCAN */
> +							298 0x1c0>; /* MX35_PAD_TX4_RX1__CAN2_RXCAN */
> +					};
> +				};
> +
> +			};
> +		};
> +
> +		spba@50000000 {
> +			compatible = "fsl,spba-bus", "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x50000000 0x100000>;
> +			ranges;
> +
> +			uart3: uart@5000c000 {
> +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> +				reg = <0x5000c000 0x4000>;
> +				interrupts = <18>;
> +				status = "disabled";
> +			};
> +
> +			fec: fec@50038000 {
> +				compatible = "fsl,imx35-fec", "fsl,imx27-fec";
> +				reg = <0x50038000 0x4000>;
> +				interrupts = <57>;
> +				status = "disabled";
> +			};
> +		};
> +
> +		aips@53f00000 { /* AIPS2 */
> +			compatible = "fsl,aips", "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x53f00000 0xffc0000>;
> +			ranges;
> +
> +			gpio3: gpio@0x53fa4000 {
> +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";

compatible = "fsl,imx35-gpio";

> +				reg = <0x53fa4000 0x4000>;
> +				interrupts = <56>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				interrupt-controller;
> +				#interrupt-cells = <1>;

It should be 2 now.

> +			};
> +
> +			esdhc1: esdhc@53fb4000 {
> +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";

compatible = "fsl,imx35-esdhc";

> +				reg = <0x53fb4000 0x4000>;
> +				interrupts = <7>;
> +				status = "disabled";
> +			};
> +
> +			esdhc2: esdhc@53fb8000 {
> +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> +				reg = <0x53fb8000 0x4000>;
> +				interrupts = <8>;
> +				status = "disabled";
> +			};
> +
> +			esdhc3: esdhc@53fbc000 {
> +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> +				reg = <0x53fbc000 0x4000>;
> +				interrupts = <9>;
> +				status = "disabled";
> +			};
> +
> +			gpio1: gpio@0x53fcc000 {
> +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> +				reg = <0x53fcc000 0x4000>;
> +				interrupts = <52>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +
> +			gpio2: gpio@0x53fd0000 {
> +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> +				reg = <0x53fd0000 0x4000>;
> +				interrupts = <51>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +
> +			wdog@53fdc000 {
> +				compatible = "fsl,imx35-wdt", "fsl,imx2-wdt";
> +				reg = <0x53fdc000 0x4000>;
> +				interrupts = <55>;
> +				status = "disabled";

Drop status to have wdog such completely SoC internal stuff enabled by
default.

> +			};
> +
> +			can@53fe4000 {
> +				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> +				reg = <0x53fe4000 0x1000>;
> +				interrupts = <43>;
> +				status = "disabled";
> +			};
> +			can@53fe8000 {
> +				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> +				reg = <0x53fe8000 0x1000>;
> +				interrupts = <44>;
> +				status = "disabled";
> +			};
> +		};
> +		nand@bb000000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			compatible = "fsl,imx35-nand", "fsl,imx25-nand";
> +			reg = <0xbb000000 0x2000>;
> +			interrupts = <33>;
> +			status = "disabled";
> +		};

Why nand device isn't under any bus but soc directly?

> +	};
> +};
> diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> index b1d3675..9389ab6 100644
> --- a/arch/arm/configs/imx_v6_v7_defconfig
> +++ b/arch/arm/configs/imx_v6_v7_defconfig
> @@ -26,6 +26,7 @@ CONFIG_MACH_ARMADILLO5X0=y
>  CONFIG_MACH_KZM_ARM11_01=y
>  CONFIG_MACH_PCM043=y
>  CONFIG_MACH_MX35_3DS=y
> +CONFIG_MACH_IMX35_DT=y
>  CONFIG_MACH_VPR200=y
>  CONFIG_MACH_IMX51_DT=y
>  CONFIG_MACH_MX51_3DS=y
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index eff4db5..95f9d6a 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -644,6 +644,19 @@ config MACH_VPR200
>  	  Include support for VPR200 platform. This includes specific
>  	  configurations for the board and its peripherals.
>  
> +config MACH_IMX35_DT
> +	bool "Support i.MX35 platforms from device tree"
> +	select SOC_IMX35
> +	select USE_OF

USE_OF is selected by ARCH_MXC now.

> +	select HAVE_CAN_FLEXCAN if CAN

Move it to "config SOC_IMX35".

> +	select IMX_HAVE_PLATFORM_MXC_NAND
> +	select IMX_HAVE_PLATFORM_SPI_IMX
> +	select IMX_HAVE_PLATFORM_IMX2_WDT

These are unreasonable.

> +	select PINCTRL

Need to select PINCTRL_IMX35 also?

And they should be under "config SOC_IMX35".

> +	help
> +	  Include support for Freescale i.MX35 based platforms
> +	  using the device tree for discovery
> +
>  comment "i.MX5 platforms:"
>  
>  config MACH_MX50_RDP
> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> index ff29421..97d01b5 100644
> --- a/arch/arm/mach-imx/Makefile
> +++ b/arch/arm/mach-imx/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_MACH_MX35_3DS) += mach-mx35_3ds.o
>  obj-$(CONFIG_MACH_EUKREA_CPUIMX35SD) += mach-cpuimx35.o
>  obj-$(CONFIG_MACH_EUKREA_MBIMXSD35_BASEBOARD) += eukrea_mbimxsd35-baseboard.o
>  obj-$(CONFIG_MACH_VPR200) += mach-vpr200.o
> +obj-$(CONFIG_MACH_IMX35_DT) += imx35-dt.o
>  
>  obj-$(CONFIG_DEBUG_LL) += lluart.o
>  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> diff --git a/arch/arm/mach-imx/imx35-dt.c b/arch/arm/mach-imx/imx35-dt.c
> new file mode 100644
> index 0000000..8b9cc84
> --- /dev/null
> +++ b/arch/arm/mach-imx/imx35-dt.c
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar, Pengutronix
> + *
> + * based on imx27-dt.c
> + *
> + * 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 published by the
> + * Free Software Foundation.
> + */
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/machine.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/time.h>
> +#include <mach/common.h>
> +#include <mach/mx35.h>
> +
> +static const struct of_dev_auxdata imx35_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART1_BASE_ADDR, "imx21-uart.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART2_BASE_ADDR, "imx21-uart.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART3_BASE_ADDR, "imx21-uart.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-fec", MX35_FEC_BASE_ADDR, "imx27-fec.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-wdt", MX35_WDOG_BASE_ADDR, "imx2-wdt.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C1_BASE_ADDR, "imx-i2c.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C2_BASE_ADDR, "imx-i2c.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C3_BASE_ADDR, "imx-i2c.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC1_BASE_ADDR, "sdhci-esdhc-imx35.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC2_BASE_ADDR, "sdhci-esdhc-imx35.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC3_BASE_ADDR, "sdhci-esdhc-imx35.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx35-nand", MX35_NFC_BASE_ADDR, "mxc_nand.0", NULL),
> +	{ /* sentinel */ }
> +};

If these auxdata are here only for clk lookup, we can save them by
having DT specific clkdev lookup in clock driver.

> +
> +static int __init imx35_avic_add_irq_domain(struct device_node *np,
> +				struct device_node *interrupt_parent)
> +{
> +	irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> +	return 0;
> +}
> +
> +static int __init imx35_gpio_add_irq_domain(struct device_node *np,
> +				struct device_node *interrupt_parent)
> +{
> +	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +
> +	gpio_irq_base -= 32;
> +	irq_domain_add_legacy(np, 32, gpio_irq_base, 0, &irq_domain_simple_ops,
> +				NULL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx35_irq_match[] __initconst = {
> +	{ .compatible = "fsl,imx35-avic", .data = imx35_avic_add_irq_domain, },
> +	{ .compatible = "fsl,imx35-gpio", .data = imx35_gpio_add_irq_domain, },
> +	{ /* sentinel */ }
> +};

All these are not needed now.

> +
> +static const struct of_device_id imx35_of_mixed_devices_match[] __initconst = {
> +	{ /* END OF LIST */ }
> +};
> +
> +static void __init of_mixed_device_hook(void)
> +{
> +	struct device_node *np;
> +	const struct of_device_id *of_id;
> +
> +	np = of_find_matching_node(NULL, imx35_of_mixed_devices_match);
> +	if (np) {
> +		void (*func)(void);
> +
> +		of_id = of_match_node(imx35_of_mixed_devices_match, np);
> +		func = of_id->data;
> +		func();
> +	}
> +}

We have this on other imx DT machines for hooking the IOMUX setup done
in non-DT board files.  Since you have imx35 pinctrl support in place.
What is above hook for then?

> +
> +static void __init imx35_dt_init(void)
> +{
> +	of_irq_init(imx35_irq_match);
> +
> +	/* Some mx35 have problems when the cache is not initialized
> +	 * FIXME: ultimatly this belongs in the oftree definition
> +	 */
> +	imx3_init_l2x0();
> +	pinctrl_provide_dummies();
> +
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     imx35_auxdata_lookup, NULL);
> +
> +	of_mixed_device_hook();
> +}
> +
> +static void __init imx35_timer_init(void)
> +{
> +	mx35_clocks_init();
> +}
> +
> +static struct sys_timer imx35_timer = {
> +	.init = imx35_timer_init,
> +};
> +
> +static const char *imx35_dt_board_compat[] __initdata = {
> +	"fsl,imx35",
> +	NULL
> +};
> +
> +DT_MACHINE_START(IMX35_DT, "Freescale i.MX35 (Device Tree Support)")
> +	.map_io		= mx35_map_io,
> +	.init_early	= imx35_init_early,
> +	.init_irq	= mx35_init_irq,
> +	.handle_irq	= imx35_handle_irq,
> +	.timer		= &imx35_timer,
> +	.init_machine	= imx35_dt_init,
> +	.dt_compat	= imx35_dt_board_compat,
> +	.restart	= mxc_restart,
> +MACHINE_END
> diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
> index e429ca1..643b0aa 100644
> --- a/arch/arm/plat-mxc/include/mach/common.h
> +++ b/arch/arm/plat-mxc/include/mach/common.h
> @@ -50,6 +50,7 @@ extern void imx25_soc_init(void);
>  extern void imx27_soc_init(void);
>  extern void imx31_soc_init(void);
>  extern void imx35_soc_init(void);
> +extern void imx3_init_l2x0(void);
>  extern void imx50_soc_init(void);
>  extern void imx51_soc_init(void);
>  extern void imx53_soc_init(void);
> @@ -67,6 +68,7 @@ extern int mx51_clocks_init(unsigned long ckil, unsigned long osc,
>  extern int mx53_clocks_init(unsigned long ckil, unsigned long osc,
>  			unsigned long ckih1, unsigned long ckih2);
>  extern int mx27_clocks_init_dt(void);
> +extern int mx35_clocks_init_dt(void);

It does not necessarily belong this driver.

Regards,
Shawn

>  extern int mx51_clocks_init_dt(void);
>  extern int mx53_clocks_init_dt(void);
>  extern int mx6q_clocks_init(void);
> -- 
> 1.7.10.4
>
Uwe Kleine-König - Aug. 6, 2012, 2:41 p.m.
Hello,

On Fri, Aug 03, 2012 at 09:12:32PM +0800, Shawn Guo wrote:
> On Thu, Aug 02, 2012 at 05:56:25PM +0200, Uwe Kleine-König wrote:
> > From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > 
> > This patch adds basic support for imx35-based devices to the kernel.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/boot/dts/imx35.dtsi            |  246 +++++++++++++++++++++++++++++++
> >  arch/arm/configs/imx_v6_v7_defconfig    |    1 +
> >  arch/arm/mach-imx/Kconfig               |   13 ++
> >  arch/arm/mach-imx/Makefile              |    1 +
> >  arch/arm/mach-imx/imx35-dt.c            |  119 +++++++++++++++
> >  arch/arm/plat-mxc/include/mach/common.h |    2 +
> >  6 files changed, 382 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/imx35.dtsi
> >  create mode 100644 arch/arm/mach-imx/imx35-dt.c
> > 
> > diff --git a/arch/arm/boot/dts/imx35.dtsi b/arch/arm/boot/dts/imx35.dtsi
> > new file mode 100644
> > index 0000000..d0c6456
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/imx35.dtsi
> > @@ -0,0 +1,246 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar, Pengutronix
> > + *
> > + * based on imx27.dtsi
> > + *
> > + * 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 published by the
> > + * Free Software Foundation.
> > + */
> > +
> > +/include/ "skeleton.dtsi"
> > +
> > +/ {
> > +	aliases {
> > +		serial0 = &uart1;
> > +		serial1 = &uart2;
> > +		serial2 = &uart3;
I will add gpio aliases here in the next round.

> > +	};
> > +
> > +	avic: avic-interrupt-controller@68000000 {
> > +		compatible = "fsl,imx35-avic", "fsl,avic";
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		reg = <0x68000000 0x10000000>;
> > +	};
> > +
> > +	clocks {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		ckil {
> > +			compatible = "fsl,imx-ckil", "fixed-clock";
> > +			clock-frequency = <32768>;
> > +		};
> > +
> > +		osc {
> > +			compatible = "fsl,imx-osc", "fixed-clock";
> > +			clock-frequency = <24000000>;
> > +		};
> > +	};
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		cpu@0 {
> > +			device_type = "cpu";
> 
> This is not really necessary, IIRC.
ok, can drop that.
> 
> > +			compatible = "fsl,imx35", "arm,arm1136jfs";
> > +			reg = <0>;
> > +			d-cache-size = <0x4000>;	// L1, 16K
> > +			i-cache-size = <0x4000>;	// L1, 16K
> 
> Are these two common bindings?  Documented in
> Documentation/devicetree/bindings for somewhere?
Don't know, that part wass written by Steffen. I will look into the
other imx.dtsi files and make it use the same idioms.
 
> > +			bus-frequency = <0>;		// from bootloader
> 
> Ditto.  I'm not even sure how this works.  Does your bootloader
> really populates this property?  Which bus?
No it doesn't. As above ...

> > +		};
> > +	};
> > +
> > +	soc {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "simple-bus";
> > +		interrupt-parent = <&avic>;
> > +		ranges;
> > +
> > +		aips@40000000 { /* AIPS1 */
> > +			compatible = "fsl,aips", "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x40000000 0x100000>;
> > +			ranges;
> > +
> > +			i2c1: i2c@43f80000 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> > +				reg = <0x43f80000 0x4000>;
> > +				interrupts = <10>;
> > +				status = "disabled";
> > +			};
> > +
> > +			i2c3: i2c@43f84000 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> > +				reg = <0x43f84000 0x4000>;
> > +				interrupts = <3>;
> > +				status = "disabled";
> > +			};
> > +
> > +			uart1: uart@43f90000 {
> 
> Name the node "serial".
ok

> 
> > +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > +				reg = <0x43f90000 0x4000>;
> > +				interrupts = <45>;
> > +				status = "disabled";
> > +			};
> > +
> > +			uart2: uart@43f94000 {
> > +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > +				reg = <0x43f94000 0x4000>;
> > +				interrupts = <32>;
> > +				status = "disabled";
> > +			};
> > +
> > +			i2c2: i2c@43f98000 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> > +				reg = <0x43f98000 0x4000>;
> > +				interrupts = <4>;
> > +				status = "disabled";
> > +			};
> 
> We generally have a new line between nodes.
fine for me.

> > +			iomuxc@43fac000 {
> > +				compatible = "fsl,imx35-iomuxc";
> > +				reg = <0x43fac000 0x4000>;
> > +
> > +				i2c3 {
> > +					pinctrl_i2c3_1: i2c3grp-1 {
> > +						fsl,pins = <773 0x1c0 /* MX35_PAD_ATA_DATA12__I2C3_SCL */
> > +							777 0x1c0>; /* MX35_PAD_ATA_DATA13__I2C3_SDA */
> > +					};
> > +				};
> > +
> > +				can1 {
> > +					pinctrl_can1_1: can1grp-1 {
> > +						fsl,pins = <223 0x1c0 /* MX35_PAD_I2C2_CLK__CAN1_TXCAN */
> > +							228 0x1c0>; /* MX35_PAD_I2C2_DAT__CAN1_RXCAN */
> > +					};
> > +				};
> > +				can2 {
> > +					pinctrl_can2_1: can2grp-1 {
> > +						fsl,pins = <291 0x1c0 /* MX35_PAD_TX5_RX0__CAN2_TXCAN */
> > +							298 0x1c0>; /* MX35_PAD_TX4_RX1__CAN2_RXCAN */
> > +					};
> > +				};
> > +
> > +			};
> > +		};
> > +
> > +		spba@50000000 {
> > +			compatible = "fsl,spba-bus", "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x50000000 0x100000>;
> > +			ranges;
> > +
> > +			uart3: uart@5000c000 {
> > +				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > +				reg = <0x5000c000 0x4000>;
> > +				interrupts = <18>;
> > +				status = "disabled";
> > +			};
> > +
> > +			fec: fec@50038000 {
> > +				compatible = "fsl,imx35-fec", "fsl,imx27-fec";
> > +				reg = <0x50038000 0x4000>;
> > +				interrupts = <57>;
> > +				status = "disabled";
> > +			};
> > +		};
> > +
> > +		aips@53f00000 { /* AIPS2 */
> > +			compatible = "fsl,aips", "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x53f00000 0xffc0000>;
> > +			ranges;
> > +
> > +			gpio3: gpio@0x53fa4000 {
> > +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> 
> compatible = "fsl,imx35-gpio";
> 
> > +				reg = <0x53fa4000 0x4000>;
> > +				interrupts = <56>;
> > +				gpio-controller;
> > +				#gpio-cells = <2>;
> > +				interrupt-controller;
> > +				#interrupt-cells = <1>;
> 
> It should be 2 now.
> 
> > +			};
> > +
> > +			esdhc1: esdhc@53fb4000 {
> > +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> 
> compatible = "fsl,imx35-esdhc";
> 
> > +				reg = <0x53fb4000 0x4000>;
> > +				interrupts = <7>;
> > +				status = "disabled";
> > +			};
> > +
> > +			esdhc2: esdhc@53fb8000 {
> > +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> > +				reg = <0x53fb8000 0x4000>;
> > +				interrupts = <8>;
> > +				status = "disabled";
> > +			};
> > +
> > +			esdhc3: esdhc@53fbc000 {
> > +				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> > +				reg = <0x53fbc000 0x4000>;
> > +				interrupts = <9>;
> > +				status = "disabled";
> > +			};
> > +
> > +			gpio1: gpio@0x53fcc000 {
> > +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> > +				reg = <0x53fcc000 0x4000>;
> > +				interrupts = <52>;
> > +				gpio-controller;
> > +				#gpio-cells = <2>;
> > +				interrupt-controller;
> > +				#interrupt-cells = <1>;
> > +			};
> > +
> > +			gpio2: gpio@0x53fd0000 {
> > +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> > +				reg = <0x53fd0000 0x4000>;
> > +				interrupts = <51>;
> > +				gpio-controller;
> > +				#gpio-cells = <2>;
> > +				interrupt-controller;
> > +				#interrupt-cells = <1>;
> > +			};
> > +
> > +			wdog@53fdc000 {
> > +				compatible = "fsl,imx35-wdt", "fsl,imx2-wdt";
> > +				reg = <0x53fdc000 0x4000>;
> > +				interrupts = <55>;
> > +				status = "disabled";
> 
> Drop status to have wdog such completely SoC internal stuff enabled by
> default.
Ah I remember to have seen the corresponding patches. OK.
 
> > +			};
> > +
> > +			can@53fe4000 {
> > +				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> > +				reg = <0x53fe4000 0x1000>;
> > +				interrupts = <43>;
> > +				status = "disabled";
> > +			};
> > +			can@53fe8000 {
> > +				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> > +				reg = <0x53fe8000 0x1000>;
> > +				interrupts = <44>;
> > +				status = "disabled";
> > +			};
> > +		};
> > +		nand@bb000000 {
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +
> > +			compatible = "fsl,imx35-nand", "fsl,imx25-nand";
> > +			reg = <0xbb000000 0x2000>;
> > +			interrupts = <33>;
> > +			status = "disabled";
> > +		};
> 
> Why nand device isn't under any bus but soc directly?
I copied that from imx27.dtsi (where I introduced it myself before).
It's not obvious to me which bus this is a part of from reading the
hardware manual. Neither for i.MX27 nor i.MX35.
> 
> > +	};
> > +};
> > diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> > index b1d3675..9389ab6 100644
> > --- a/arch/arm/configs/imx_v6_v7_defconfig
> > +++ b/arch/arm/configs/imx_v6_v7_defconfig
> > @@ -26,6 +26,7 @@ CONFIG_MACH_ARMADILLO5X0=y
> >  CONFIG_MACH_KZM_ARM11_01=y
> >  CONFIG_MACH_PCM043=y
> >  CONFIG_MACH_MX35_3DS=y
> > +CONFIG_MACH_IMX35_DT=y
> >  CONFIG_MACH_VPR200=y
> >  CONFIG_MACH_IMX51_DT=y
> >  CONFIG_MACH_MX51_3DS=y
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index eff4db5..95f9d6a 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -644,6 +644,19 @@ config MACH_VPR200
> >  	  Include support for VPR200 platform. This includes specific
> >  	  configurations for the board and its peripherals.
> >  
> > +config MACH_IMX35_DT
> > +	bool "Support i.MX35 platforms from device tree"
> > +	select SOC_IMX35
> > +	select USE_OF
> 
> USE_OF is selected by ARCH_MXC now.
ah right.

> 
> > +	select HAVE_CAN_FLEXCAN if CAN
> 
> Move it to "config SOC_IMX35".
hmm, do we want CAN_FLEXCAN to be selectable for machines that don't
have can although they are based on i.MX35?

> 
> > +	select IMX_HAVE_PLATFORM_MXC_NAND
> > +	select IMX_HAVE_PLATFORM_SPI_IMX
> > +	select IMX_HAVE_PLATFORM_IMX2_WDT
> 
> These are unreasonable.
These are needed to be able to select the corresponding drivers.

> 
> > +	select PINCTRL
> 
> Need to select PINCTRL_IMX35 also?
good idea.

> And they should be under "config SOC_IMX35".
Even though currently the pincontrol driver isn't used for them?
 
> > +	help
> > +	  Include support for Freescale i.MX35 based platforms
> > +	  using the device tree for discovery
> > +
> >  comment "i.MX5 platforms:"
> >  
> >  config MACH_MX50_RDP
> > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> > index ff29421..97d01b5 100644
> > --- a/arch/arm/mach-imx/Makefile
> > +++ b/arch/arm/mach-imx/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_MACH_MX35_3DS) += mach-mx35_3ds.o
> >  obj-$(CONFIG_MACH_EUKREA_CPUIMX35SD) += mach-cpuimx35.o
> >  obj-$(CONFIG_MACH_EUKREA_MBIMXSD35_BASEBOARD) += eukrea_mbimxsd35-baseboard.o
> >  obj-$(CONFIG_MACH_VPR200) += mach-vpr200.o
> > +obj-$(CONFIG_MACH_IMX35_DT) += imx35-dt.o
> >  
> >  obj-$(CONFIG_DEBUG_LL) += lluart.o
> >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> > diff --git a/arch/arm/mach-imx/imx35-dt.c b/arch/arm/mach-imx/imx35-dt.c
> > new file mode 100644
> > index 0000000..8b9cc84
> > --- /dev/null
> > +++ b/arch/arm/mach-imx/imx35-dt.c
> > @@ -0,0 +1,119 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar, Pengutronix
> > + *
> > + * based on imx27-dt.c
> > + *
> > + * 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 published by the
> > + * Free Software Foundation.
> > + */
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pinctrl/machine.h>
> > +#include <asm/mach/arch.h>
> > +#include <asm/mach/time.h>
> > +#include <mach/common.h>
> > +#include <mach/mx35.h>
> > +
> > +static const struct of_dev_auxdata imx35_auxdata_lookup[] __initconst = {
> > +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART1_BASE_ADDR, "imx21-uart.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART2_BASE_ADDR, "imx21-uart.1", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART3_BASE_ADDR, "imx21-uart.2", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-fec", MX35_FEC_BASE_ADDR, "imx27-fec.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-wdt", MX35_WDOG_BASE_ADDR, "imx2-wdt.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C1_BASE_ADDR, "imx-i2c.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C2_BASE_ADDR, "imx-i2c.1", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C3_BASE_ADDR, "imx-i2c.2", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC1_BASE_ADDR, "sdhci-esdhc-imx35.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC2_BASE_ADDR, "sdhci-esdhc-imx35.1", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC3_BASE_ADDR, "sdhci-esdhc-imx35.2", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx35-nand", MX35_NFC_BASE_ADDR, "mxc_nand.0", NULL),
> > +	{ /* sentinel */ }
> > +};
> 
> If these auxdata are here only for clk lookup, we can save them by
> having DT specific clkdev lookup in clock driver.
ok.

> 
> > +
> > +static int __init imx35_avic_add_irq_domain(struct device_node *np,
> > +				struct device_node *interrupt_parent)
> > +{
> > +	irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> > +	return 0;
> > +}
> > +
> > +static int __init imx35_gpio_add_irq_domain(struct device_node *np,
> > +				struct device_node *interrupt_parent)
> > +{
> > +	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> > +
> > +	gpio_irq_base -= 32;
> > +	irq_domain_add_legacy(np, 32, gpio_irq_base, 0, &irq_domain_simple_ops,
> > +				NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx35_irq_match[] __initconst = {
> > +	{ .compatible = "fsl,imx35-avic", .data = imx35_avic_add_irq_domain, },
> > +	{ .compatible = "fsl,imx35-gpio", .data = imx35_gpio_add_irq_domain, },
> > +	{ /* sentinel */ }
> > +};
> 
> All these are not needed now.
ok.

> 
> > +
> > +static const struct of_device_id imx35_of_mixed_devices_match[] __initconst = {
> > +	{ /* END OF LIST */ }
> > +};
> > +
> > +static void __init of_mixed_device_hook(void)
> > +{
> > +	struct device_node *np;
> > +	const struct of_device_id *of_id;
> > +
> > +	np = of_find_matching_node(NULL, imx35_of_mixed_devices_match);
> > +	if (np) {
> > +		void (*func)(void);
> > +
> > +		of_id = of_match_node(imx35_of_mixed_devices_match, np);
> > +		func = of_id->data;
> > +		func();
> > +	}
> > +}
> 
> We have this on other imx DT machines for hooking the IOMUX setup done
> in non-DT board files.  Since you have imx35 pinctrl support in place.
> What is above hook for then?
I need it to configure and export some gpios.

> > +static void __init imx35_dt_init(void)
> > +{
> > +	of_irq_init(imx35_irq_match);
> > +
> > +	/* Some mx35 have problems when the cache is not initialized
> > +	 * FIXME: ultimatly this belongs in the oftree definition
> > +	 */
> > +	imx3_init_l2x0();
> > +	pinctrl_provide_dummies();
> > +
> > +	of_platform_populate(NULL, of_default_bus_match_table,
> > +			     imx35_auxdata_lookup, NULL);
> > +
> > +	of_mixed_device_hook();
> > +}
> > +
> > +static void __init imx35_timer_init(void)
> > +{
> > +	mx35_clocks_init();
> > +}
> > +
> > +static struct sys_timer imx35_timer = {
> > +	.init = imx35_timer_init,
> > +};
> > +
> > +static const char *imx35_dt_board_compat[] __initdata = {
> > +	"fsl,imx35",
> > +	NULL
> > +};
> > +
> > +DT_MACHINE_START(IMX35_DT, "Freescale i.MX35 (Device Tree Support)")
> > +	.map_io		= mx35_map_io,
> > +	.init_early	= imx35_init_early,
> > +	.init_irq	= mx35_init_irq,
> > +	.handle_irq	= imx35_handle_irq,
> > +	.timer		= &imx35_timer,
> > +	.init_machine	= imx35_dt_init,
> > +	.dt_compat	= imx35_dt_board_compat,
> > +	.restart	= mxc_restart,
> > +MACHINE_END
> > diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
> > index e429ca1..643b0aa 100644
> > --- a/arch/arm/plat-mxc/include/mach/common.h
> > +++ b/arch/arm/plat-mxc/include/mach/common.h
> > @@ -50,6 +50,7 @@ extern void imx25_soc_init(void);
> >  extern void imx27_soc_init(void);
> >  extern void imx31_soc_init(void);
> >  extern void imx35_soc_init(void);
> > +extern void imx3_init_l2x0(void);
> >  extern void imx50_soc_init(void);
> >  extern void imx51_soc_init(void);
> >  extern void imx53_soc_init(void);
> > @@ -67,6 +68,7 @@ extern int mx51_clocks_init(unsigned long ckil, unsigned long osc,
> >  extern int mx53_clocks_init(unsigned long ckil, unsigned long osc,
> >  			unsigned long ckih1, unsigned long ckih2);
> >  extern int mx27_clocks_init_dt(void);
> > +extern int mx35_clocks_init_dt(void);
> 
> It does not necessarily belong this driver.
I cannot parse this sentence. Can you please try to explain again your
objections here?

Thanks
Uwe
Shawn Guo - Aug. 7, 2012, 2:44 a.m.
On Mon, Aug 06, 2012 at 04:41:00PM +0200, Uwe Kleine-König wrote:
> > > +		nand@bb000000 {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +
> > > +			compatible = "fsl,imx35-nand", "fsl,imx25-nand";
> > > +			reg = <0xbb000000 0x2000>;
> > > +			interrupts = <33>;
> > > +			status = "disabled";
> > > +		};
> > 
> > Why nand device isn't under any bus but soc directly?
> I copied that from imx27.dtsi (where I introduced it myself before).
> It's not obvious to me which bus this is a part of from reading the
> hardware manual. Neither for i.MX27 nor i.MX35.

See i.MX35 RM "Chapter 17 External Memory Interface (EMI)",
"Memory Map/Register Definition" section.

> > > +	select HAVE_CAN_FLEXCAN if CAN
> > 
> > Move it to "config SOC_IMX35".
> hmm, do we want CAN_FLEXCAN to be selectable for machines that don't
> have can although they are based on i.MX35?
> 
I just recall it has been done by commit 610578a (ARM: imx: enable
flexcan on imx25, imx35, imx53, imx6q).

> > 
> > > +	select IMX_HAVE_PLATFORM_MXC_NAND
> > > +	select IMX_HAVE_PLATFORM_SPI_IMX
> > > +	select IMX_HAVE_PLATFORM_IMX2_WDT
> > 
> > These are unreasonable.
> These are needed to be able to select the corresponding drivers.
> 
The driver Kconfig needs fixing then.

> > 
> > > +	select PINCTRL
> > 
> > Need to select PINCTRL_IMX35 also?
> good idea.
> 
> > And they should be under "config SOC_IMX35".
> Even though currently the pincontrol driver isn't used for them?
>  
No pressure for now.  But in the end, it will be like that.

> > > +static const struct of_device_id imx35_of_mixed_devices_match[] __initconst = {
> > > +	{ /* END OF LIST */ }
> > > +};
> > > +
> > > +static void __init of_mixed_device_hook(void)
> > > +{
> > > +	struct device_node *np;
> > > +	const struct of_device_id *of_id;
> > > +
> > > +	np = of_find_matching_node(NULL, imx35_of_mixed_devices_match);
> > > +	if (np) {
> > > +		void (*func)(void);
> > > +
> > > +		of_id = of_match_node(imx35_of_mixed_devices_match, np);
> > > +		func = of_id->data;
> > > +		func();
> > > +	}
> > > +}
> > 
> > We have this on other imx DT machines for hooking the IOMUX setup done
> > in non-DT board files.  Since you have imx35 pinctrl support in place.
> > What is above hook for then?
> I need it to configure and export some gpios.
> 
Add it when you add users?  So that we can know it's really the only
way to go.

> > > +extern int mx35_clocks_init_dt(void);
> > 
> > It does not necessarily belong this driver.
> I cannot parse this sentence. Can you please try to explain again your
> objections here?
> 
I see neither the implementation nor the users for mx35_clocks_init_dt().
Uwe Kleine-König - Aug. 8, 2012, 8:59 a.m.
Hello Shawn,

On Fri, Aug 03, 2012 at 09:12:32PM +0800, Shawn Guo wrote:
> On Thu, Aug 02, 2012 at 05:56:25PM +0200, Uwe Kleine-König wrote:
> > +			gpio3: gpio@0x53fa4000 {
> > +				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> 
> compatible = "fsl,imx35-gpio";
$ git grep fsl,imx3[15]-gpio drivers/gpio
drivers/gpio/gpio-mxc.c:        { .compatible = "fsl,imx31-gpio", .data = &mxc_gpio_devtype[IMX31_GPIO], },

I guess that means I have to keep "fsl,imx31-gpio", right?

Best regards
Uwe
Shawn Guo - Aug. 8, 2012, 2:43 p.m.
On Wed, Aug 08, 2012 at 10:59:56AM +0200, Uwe Kleine-König wrote:
> $ git grep fsl,imx3[15]-gpio drivers/gpio
> drivers/gpio/gpio-mxc.c:        { .compatible = "fsl,imx31-gpio", .data = &mxc_gpio_devtype[IMX31_GPIO], },
> 
Try it on v3.6-rc1.

Patch

diff --git a/arch/arm/boot/dts/imx35.dtsi b/arch/arm/boot/dts/imx35.dtsi
new file mode 100644
index 0000000..d0c6456
--- /dev/null
+++ b/arch/arm/boot/dts/imx35.dtsi
@@ -0,0 +1,246 @@ 
+/*
+ * Copyright 2012 Steffen Trumtrar, Pengutronix
+ *
+ * based on imx27.dtsi
+ *
+ * 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 published by the
+ * Free Software Foundation.
+ */
+
+/include/ "skeleton.dtsi"
+
+/ {
+	aliases {
+		serial0 = &uart1;
+		serial1 = &uart2;
+		serial2 = &uart3;
+	};
+
+	avic: avic-interrupt-controller@68000000 {
+		compatible = "fsl,imx35-avic", "fsl,avic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0x68000000 0x10000000>;
+	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ckil {
+			compatible = "fsl,imx-ckil", "fixed-clock";
+			clock-frequency = <32768>;
+		};
+
+		osc {
+			compatible = "fsl,imx-osc", "fixed-clock";
+			clock-frequency = <24000000>;
+		};
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "fsl,imx35", "arm,arm1136jfs";
+			reg = <0>;
+			d-cache-size = <0x4000>;	// L1, 16K
+			i-cache-size = <0x4000>;	// L1, 16K
+			bus-frequency = <0>;		// from bootloader
+		};
+	};
+
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&avic>;
+		ranges;
+
+		aips@40000000 { /* AIPS1 */
+			compatible = "fsl,aips", "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x40000000 0x100000>;
+			ranges;
+
+			i2c1: i2c@43f80000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
+				reg = <0x43f80000 0x4000>;
+				interrupts = <10>;
+				status = "disabled";
+			};
+
+			i2c3: i2c@43f84000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
+				reg = <0x43f84000 0x4000>;
+				interrupts = <3>;
+				status = "disabled";
+			};
+
+			uart1: uart@43f90000 {
+				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
+				reg = <0x43f90000 0x4000>;
+				interrupts = <45>;
+				status = "disabled";
+			};
+
+			uart2: uart@43f94000 {
+				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
+				reg = <0x43f94000 0x4000>;
+				interrupts = <32>;
+				status = "disabled";
+			};
+
+			i2c2: i2c@43f98000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
+				reg = <0x43f98000 0x4000>;
+				interrupts = <4>;
+				status = "disabled";
+			};
+			iomuxc@43fac000 {
+				compatible = "fsl,imx35-iomuxc";
+				reg = <0x43fac000 0x4000>;
+
+				i2c3 {
+					pinctrl_i2c3_1: i2c3grp-1 {
+						fsl,pins = <773 0x1c0 /* MX35_PAD_ATA_DATA12__I2C3_SCL */
+							777 0x1c0>; /* MX35_PAD_ATA_DATA13__I2C3_SDA */
+					};
+				};
+
+				can1 {
+					pinctrl_can1_1: can1grp-1 {
+						fsl,pins = <223 0x1c0 /* MX35_PAD_I2C2_CLK__CAN1_TXCAN */
+							228 0x1c0>; /* MX35_PAD_I2C2_DAT__CAN1_RXCAN */
+					};
+				};
+				can2 {
+					pinctrl_can2_1: can2grp-1 {
+						fsl,pins = <291 0x1c0 /* MX35_PAD_TX5_RX0__CAN2_TXCAN */
+							298 0x1c0>; /* MX35_PAD_TX4_RX1__CAN2_RXCAN */
+					};
+				};
+
+			};
+		};
+
+		spba@50000000 {
+			compatible = "fsl,spba-bus", "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x50000000 0x100000>;
+			ranges;
+
+			uart3: uart@5000c000 {
+				compatible = "fsl,imx35-uart", "fsl,imx21-uart";
+				reg = <0x5000c000 0x4000>;
+				interrupts = <18>;
+				status = "disabled";
+			};
+
+			fec: fec@50038000 {
+				compatible = "fsl,imx35-fec", "fsl,imx27-fec";
+				reg = <0x50038000 0x4000>;
+				interrupts = <57>;
+				status = "disabled";
+			};
+		};
+
+		aips@53f00000 { /* AIPS2 */
+			compatible = "fsl,aips", "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x53f00000 0xffc0000>;
+			ranges;
+
+			gpio3: gpio@0x53fa4000 {
+				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
+				reg = <0x53fa4000 0x4000>;
+				interrupts = <56>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
+			esdhc1: esdhc@53fb4000 {
+				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
+				reg = <0x53fb4000 0x4000>;
+				interrupts = <7>;
+				status = "disabled";
+			};
+
+			esdhc2: esdhc@53fb8000 {
+				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
+				reg = <0x53fb8000 0x4000>;
+				interrupts = <8>;
+				status = "disabled";
+			};
+
+			esdhc3: esdhc@53fbc000 {
+				compatible = "fsl,mx35-sdhc", "fsl,esdhc";
+				reg = <0x53fbc000 0x4000>;
+				interrupts = <9>;
+				status = "disabled";
+			};
+
+			gpio1: gpio@0x53fcc000 {
+				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
+				reg = <0x53fcc000 0x4000>;
+				interrupts = <52>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
+			gpio2: gpio@0x53fd0000 {
+				compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
+				reg = <0x53fd0000 0x4000>;
+				interrupts = <51>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
+			wdog@53fdc000 {
+				compatible = "fsl,imx35-wdt", "fsl,imx2-wdt";
+				reg = <0x53fdc000 0x4000>;
+				interrupts = <55>;
+				status = "disabled";
+			};
+
+			can@53fe4000 {
+				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
+				reg = <0x53fe4000 0x1000>;
+				interrupts = <43>;
+				status = "disabled";
+			};
+			can@53fe8000 {
+				compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
+				reg = <0x53fe8000 0x1000>;
+				interrupts = <44>;
+				status = "disabled";
+			};
+		};
+		nand@bb000000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			compatible = "fsl,imx35-nand", "fsl,imx25-nand";
+			reg = <0xbb000000 0x2000>;
+			interrupts = <33>;
+			status = "disabled";
+		};
+	};
+};
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index b1d3675..9389ab6 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -26,6 +26,7 @@  CONFIG_MACH_ARMADILLO5X0=y
 CONFIG_MACH_KZM_ARM11_01=y
 CONFIG_MACH_PCM043=y
 CONFIG_MACH_MX35_3DS=y
+CONFIG_MACH_IMX35_DT=y
 CONFIG_MACH_VPR200=y
 CONFIG_MACH_IMX51_DT=y
 CONFIG_MACH_MX51_3DS=y
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index eff4db5..95f9d6a 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -644,6 +644,19 @@  config MACH_VPR200
 	  Include support for VPR200 platform. This includes specific
 	  configurations for the board and its peripherals.
 
+config MACH_IMX35_DT
+	bool "Support i.MX35 platforms from device tree"
+	select SOC_IMX35
+	select USE_OF
+	select HAVE_CAN_FLEXCAN if CAN
+	select IMX_HAVE_PLATFORM_MXC_NAND
+	select IMX_HAVE_PLATFORM_SPI_IMX
+	select IMX_HAVE_PLATFORM_IMX2_WDT
+	select PINCTRL
+	help
+	  Include support for Freescale i.MX35 based platforms
+	  using the device tree for discovery
+
 comment "i.MX5 platforms:"
 
 config MACH_MX50_RDP
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index ff29421..97d01b5 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -64,6 +64,7 @@  obj-$(CONFIG_MACH_MX35_3DS) += mach-mx35_3ds.o
 obj-$(CONFIG_MACH_EUKREA_CPUIMX35SD) += mach-cpuimx35.o
 obj-$(CONFIG_MACH_EUKREA_MBIMXSD35_BASEBOARD) += eukrea_mbimxsd35-baseboard.o
 obj-$(CONFIG_MACH_VPR200) += mach-vpr200.o
+obj-$(CONFIG_MACH_IMX35_DT) += imx35-dt.o
 
 obj-$(CONFIG_DEBUG_LL) += lluart.o
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
diff --git a/arch/arm/mach-imx/imx35-dt.c b/arch/arm/mach-imx/imx35-dt.c
new file mode 100644
index 0000000..8b9cc84
--- /dev/null
+++ b/arch/arm/mach-imx/imx35-dt.c
@@ -0,0 +1,119 @@ 
+/*
+ * Copyright 2012 Steffen Trumtrar, Pengutronix
+ *
+ * based on imx27-dt.c
+ *
+ * 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 published by the
+ * Free Software Foundation.
+ */
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/machine.h>
+#include <asm/mach/arch.h>
+#include <asm/mach/time.h>
+#include <mach/common.h>
+#include <mach/mx35.h>
+
+static const struct of_dev_auxdata imx35_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART1_BASE_ADDR, "imx21-uart.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART2_BASE_ADDR, "imx21-uart.1", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-uart", MX35_UART3_BASE_ADDR, "imx21-uart.2", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-fec", MX35_FEC_BASE_ADDR, "imx27-fec.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-wdt", MX35_WDOG_BASE_ADDR, "imx2-wdt.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C1_BASE_ADDR, "imx-i2c.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C2_BASE_ADDR, "imx-i2c.1", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-i2c", MX35_I2C3_BASE_ADDR, "imx-i2c.2", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC1_BASE_ADDR, "sdhci-esdhc-imx35.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC2_BASE_ADDR, "sdhci-esdhc-imx35.1", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-sdhc", MX35_ESDHC3_BASE_ADDR, "sdhci-esdhc-imx35.2", NULL),
+	OF_DEV_AUXDATA("fsl,imx35-nand", MX35_NFC_BASE_ADDR, "mxc_nand.0", NULL),
+	{ /* sentinel */ }
+};
+
+static int __init imx35_avic_add_irq_domain(struct device_node *np,
+				struct device_node *interrupt_parent)
+{
+	irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
+	return 0;
+}
+
+static int __init imx35_gpio_add_irq_domain(struct device_node *np,
+				struct device_node *interrupt_parent)
+{
+	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
+
+	gpio_irq_base -= 32;
+	irq_domain_add_legacy(np, 32, gpio_irq_base, 0, &irq_domain_simple_ops,
+				NULL);
+
+	return 0;
+}
+
+static const struct of_device_id imx35_irq_match[] __initconst = {
+	{ .compatible = "fsl,imx35-avic", .data = imx35_avic_add_irq_domain, },
+	{ .compatible = "fsl,imx35-gpio", .data = imx35_gpio_add_irq_domain, },
+	{ /* sentinel */ }
+};
+
+static const struct of_device_id imx35_of_mixed_devices_match[] __initconst = {
+	{ /* END OF LIST */ }
+};
+
+static void __init of_mixed_device_hook(void)
+{
+	struct device_node *np;
+	const struct of_device_id *of_id;
+
+	np = of_find_matching_node(NULL, imx35_of_mixed_devices_match);
+	if (np) {
+		void (*func)(void);
+
+		of_id = of_match_node(imx35_of_mixed_devices_match, np);
+		func = of_id->data;
+		func();
+	}
+}
+
+static void __init imx35_dt_init(void)
+{
+	of_irq_init(imx35_irq_match);
+
+	/* Some mx35 have problems when the cache is not initialized
+	 * FIXME: ultimatly this belongs in the oftree definition
+	 */
+	imx3_init_l2x0();
+	pinctrl_provide_dummies();
+
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     imx35_auxdata_lookup, NULL);
+
+	of_mixed_device_hook();
+}
+
+static void __init imx35_timer_init(void)
+{
+	mx35_clocks_init();
+}
+
+static struct sys_timer imx35_timer = {
+	.init = imx35_timer_init,
+};
+
+static const char *imx35_dt_board_compat[] __initdata = {
+	"fsl,imx35",
+	NULL
+};
+
+DT_MACHINE_START(IMX35_DT, "Freescale i.MX35 (Device Tree Support)")
+	.map_io		= mx35_map_io,
+	.init_early	= imx35_init_early,
+	.init_irq	= mx35_init_irq,
+	.handle_irq	= imx35_handle_irq,
+	.timer		= &imx35_timer,
+	.init_machine	= imx35_dt_init,
+	.dt_compat	= imx35_dt_board_compat,
+	.restart	= mxc_restart,
+MACHINE_END
diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
index e429ca1..643b0aa 100644
--- a/arch/arm/plat-mxc/include/mach/common.h
+++ b/arch/arm/plat-mxc/include/mach/common.h
@@ -50,6 +50,7 @@  extern void imx25_soc_init(void);
 extern void imx27_soc_init(void);
 extern void imx31_soc_init(void);
 extern void imx35_soc_init(void);
+extern void imx3_init_l2x0(void);
 extern void imx50_soc_init(void);
 extern void imx51_soc_init(void);
 extern void imx53_soc_init(void);
@@ -67,6 +68,7 @@  extern int mx51_clocks_init(unsigned long ckil, unsigned long osc,
 extern int mx53_clocks_init(unsigned long ckil, unsigned long osc,
 			unsigned long ckih1, unsigned long ckih2);
 extern int mx27_clocks_init_dt(void);
+extern int mx35_clocks_init_dt(void);
 extern int mx51_clocks_init_dt(void);
 extern int mx53_clocks_init_dt(void);
 extern int mx6q_clocks_init(void);