[PATCH 1/2] ARM: i.MX35: Add kernel oftree support
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Mon Aug 6 10:41:00 EDT 2012
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 at pengutronix.de>
> >
> > This patch adds basic support for imx35-based devices to the kernel.
> >
> > Signed-off-by: Steffen Trumtrar <s.trumtrar at pengutronix.de>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig at 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 at 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 at 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 at 40000000 { /* AIPS1 */
> > + compatible = "fsl,aips", "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0x40000000 0x100000>;
> > + ranges;
> > +
> > + i2c1: i2c at 43f80000 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> > + reg = <0x43f80000 0x4000>;
> > + interrupts = <10>;
> > + status = "disabled";
> > + };
> > +
> > + i2c3: i2c at 43f84000 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,imx35-i2c", "fsl,imx1-i2c";
> > + reg = <0x43f84000 0x4000>;
> > + interrupts = <3>;
> > + status = "disabled";
> > + };
> > +
> > + uart1: uart at 43f90000 {
>
> Name the node "serial".
ok
>
> > + compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > + reg = <0x43f90000 0x4000>;
> > + interrupts = <45>;
> > + status = "disabled";
> > + };
> > +
> > + uart2: uart at 43f94000 {
> > + compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > + reg = <0x43f94000 0x4000>;
> > + interrupts = <32>;
> > + status = "disabled";
> > + };
> > +
> > + i2c2: i2c at 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 at 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 at 50000000 {
> > + compatible = "fsl,spba-bus", "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0x50000000 0x100000>;
> > + ranges;
> > +
> > + uart3: uart at 5000c000 {
> > + compatible = "fsl,imx35-uart", "fsl,imx21-uart";
> > + reg = <0x5000c000 0x4000>;
> > + interrupts = <18>;
> > + status = "disabled";
> > + };
> > +
> > + fec: fec at 50038000 {
> > + compatible = "fsl,imx35-fec", "fsl,imx27-fec";
> > + reg = <0x50038000 0x4000>;
> > + interrupts = <57>;
> > + status = "disabled";
> > + };
> > + };
> > +
> > + aips at 53f00000 { /* AIPS2 */
> > + compatible = "fsl,aips", "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0x53f00000 0xffc0000>;
> > + ranges;
> > +
> > + gpio3: gpio at 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 at 53fb4000 {
> > + compatible = "fsl,mx35-sdhc", "fsl,esdhc";
>
> compatible = "fsl,imx35-esdhc";
>
> > + reg = <0x53fb4000 0x4000>;
> > + interrupts = <7>;
> > + status = "disabled";
> > + };
> > +
> > + esdhc2: esdhc at 53fb8000 {
> > + compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> > + reg = <0x53fb8000 0x4000>;
> > + interrupts = <8>;
> > + status = "disabled";
> > + };
> > +
> > + esdhc3: esdhc at 53fbc000 {
> > + compatible = "fsl,mx35-sdhc", "fsl,esdhc";
> > + reg = <0x53fbc000 0x4000>;
> > + interrupts = <9>;
> > + status = "disabled";
> > + };
> > +
> > + gpio1: gpio at 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 at 0x53fd0000 {
> > + compatible = "fsl,imx35-gpio", "fsl,imx31-gpio";
> > + reg = <0x53fd0000 0x4000>;
> > + interrupts = <51>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + };
> > +
> > + wdog at 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 at 53fe4000 {
> > + compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> > + reg = <0x53fe4000 0x1000>;
> > + interrupts = <43>;
> > + status = "disabled";
> > + };
> > + can at 53fe8000 {
> > + compatible = "fsl,imx35-flexcan", "fsl,p1010-flexcan";
> > + reg = <0x53fe8000 0x1000>;
> > + interrupts = <44>;
> > + status = "disabled";
> > + };
> > + };
> > + nand at 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
More information about the linux-arm-kernel
mailing list