[PATCH v1 0/5] dt: add basic imx28 support

[PATCH v1 0/5] dt: add basic imx28 support

[Dong Aisheng]

This patch series adds basic imx28 dt support including fec, mmc and dma.

Tested on mx28evk.

TODO:
Convert the remaining devices to support dt.
mxs-auart, mxs-gpio, i2c, flexcan, saif, rtc, pwm, fb.

Dong Aisheng (5):
  ARM: imx28: add basic dt support
  mmc: mxs-mmc: add dt probe support
  ARM: imx28evk: add mmc dt support
  dma: mxs-dma: add dt probe support
  ARM: mxs: add mxs dma dt support

 Documentation/devicetree/bindings/arm/fsl.txt      |    4 +
 .../devicetree/bindings/dma/fsl-mxs-dma.txt        |   17 +++
 .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++
 arch/arm/boot/dts/imx28-evk.dts                    |   45 +++++++
 arch/arm/boot/dts/imx28.dtsi                       |  131 ++++++++++++++++++++
 arch/arm/mach-mxs/Kconfig                          |    9 ++
 arch/arm/mach-mxs/Makefile                         |    1 +
 arch/arm/mach-mxs/devices-mx23.h                   |    2 +
 arch/arm/mach-mxs/devices-mx28.h                   |    2 +
 arch/arm/mach-mxs/devices/platform-dma.c           |    3 +-
 arch/arm/mach-mxs/imx28-dt.c                       |   71 +++++++++++
 arch/arm/mach-mxs/include/mach/devices-common.h    |    3 +
 arch/arm/mach-mxs/mach-apx4devkit.c                |    1 +
 arch/arm/mach-mxs/mach-m28evk.c                    |    1 +
 arch/arm/mach-mxs/mach-mx23evk.c                   |    1 +
 arch/arm/mach-mxs/mach-mx28evk.c                   |    1 +
 arch/arm/mach-mxs/mach-stmp378x_devb.c             |    1 +
 arch/arm/mach-mxs/mach-tx28.c                      |    1 +
 drivers/dma/mxs-dma.c                              |   44 +++++--
 drivers/mmc/host/mxs-mmc.c                         |   82 ++++++++++++-
 20 files changed, 424 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
 create mode 100644 Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
 create mode 100644 arch/arm/boot/dts/imx28-evk.dts
 create mode 100644 arch/arm/boot/dts/imx28.dtsi
 create mode 100644 arch/arm/mach-mxs/imx28-dt.c

[PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

This patch includes basic dt support which can boot via nfs rootfs.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 Documentation/devicetree/bindings/arm/fsl.txt |    4 +
 arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
 arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
 arch/arm/mach-mxs/Kconfig                     |    9 +++
 arch/arm/mach-mxs/Makefile                    |    1 +
 arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
 6 files changed, 200 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
index 54bddda..9f21faf 100644
--- a/Documentation/devicetree/bindings/arm/fsl.txt
+++ b/Documentation/devicetree/bindings/arm/fsl.txt
@@ -1,6 +1,10 @@
 Freescale i.MX Platforms Device Tree Bindings
 -----------------------------------------------
 
+i.MX28 Evaluation Kit
+Required root node properties:
+    - compatible = "fsl,imx28-evk", "fsl,imx28";
+
 i.MX51 Babbage Board
 Required root node properties:
     - compatible = "fsl,imx51-babbage", "fsl,imx51";
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
new file mode 100644
index 0000000..9758dc4
--- /dev/null
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+/include/ "imx28.dtsi"
+
+/ {
+	model = "Freescale i.MX28 Evaluation Kit";
+	compatible = "fsl,imx28-evk", "fsl,imx28";
+
+	memory {
+		device_type = "memory";
+		reg = <0x40000000 0x08000000>;
+	};
+
+	ahb@80080000 {
+		fec@800f0000 {
+			phy-mode = "rmii";
+			local-mac-address = [00 04 9F 01 7D 5B];
+			status = "okay";
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
new file mode 100644
index 0000000..acf0dab
--- /dev/null
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/include/ "skeleton.dtsi"
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	interrupt-parent = <&icoll>;
+
+	aliases {
+		serial0 = &uart1;
+	};
+
+	cpus {
+		cpu@0 {
+			compatible = "arm,arm926ejs";
+		};
+	};
+
+	apb@80000000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80000000 0x80000>;
+		ranges;
+
+		apbh@80000000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x80000000 0x3c900>;
+			ranges;
+
+			icoll: interrupt-controller@80000000 {
+				compatible = "fsl,imx28-icoll";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				reg = <0x80000000 0x2000>;
+			};
+		};
+
+		apbx@80040000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x80040000 0x40000>;
+			ranges;
+
+			uart1: uart@80074000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x80074000 0x2000>;
+				interrupts = <47>;
+			};
+		};
+	};
+
+	ahb@80080000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80080000 0x80000>;
+		ranges;
+
+		fec@800f0000 {
+			compatible = "fsl,imx28-fec";
+			reg = <0x800f0000 0x4000>;
+			interrupts = <101>;
+			status = "disabled";
+		};
+
+		fec@800f4000 {
+			compatible = "fsl,imx28-fec";
+			reg = <0x800f4000 0x4000>;
+			interrupts = <102>;
+			status = "disabled";
+		};
+
+	};
+};
diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index c57f996..6ab66af 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -17,6 +17,15 @@ config SOC_IMX28
 
 comment "MXS platforms:"
 
+config MACH_IMX28_DT
+	bool "Support i.MX28 platforms from device tree"
+	select SOC_IMX28
+	select USE_OF
+	select MACH_MX28EVK
+	help
+	  Include support for Freescale i.MX28 based platforms
+	  using the device tree for discovery
+
 config MACH_STMP378X_DEVB
 	bool "Support STMP378x_devb Platform"
 	select SOC_IMX23
diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
index 908bf9a..25a4122 100644
--- a/arch/arm/mach-mxs/Makefile
+++ b/arch/arm/mach-mxs/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PM) += pm.o
 obj-$(CONFIG_SOC_IMX23) += clock-mx23.o
 obj-$(CONFIG_SOC_IMX28) += clock-mx28.o
 
+obj-$(CONFIG_MACH_IMX28_DT) += imx28-dt.o
 obj-$(CONFIG_MACH_STMP378X_DEVB) += mach-stmp378x_devb.o
 obj-$(CONFIG_MACH_MX23EVK) += mach-mx23evk.o
 obj-$(CONFIG_MACH_MX28EVK) += mach-mx28evk.o
diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
new file mode 100644
index 0000000..78d1129
--- /dev/null
+++ b/arch/arm/mach-mxs/imx28-dt.c
@@ -0,0 +1,67 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/init.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <asm/mach/arch.h>
+#include <asm/mach/time.h>
+#include <mach/common.h>
+#include <mach/mx28.h>
+
+static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
+	{ /* sentinel */ }
+};
+
+static void __init imx28_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     imx28_auxdata_lookup, NULL);
+}
+
+static const struct of_device_id icoll_of_match[] __initconst = {
+	{ .compatible = "fsl,imx28-icoll", },
+	{},
+};
+
+static void __init imx28_dt_init_irq(void)
+{
+	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
+	mx28_init_irq();
+}
+
+static void __init imx28_timer_init(void)
+{
+	mx28_clocks_init();
+}
+
+static struct sys_timer imx28_timer = {
+	.init = imx28_timer_init,
+};
+
+static const char *imx28_dt_compat[] __initdata = {
+	"fsl,imx28-evk",
+	NULL,
+};
+
+DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
+	.map_io		= mx28_map_io,
+	.init_irq	= imx28_dt_init_irq,
+	.timer		= &imx28_timer,
+	.init_machine	= imx28_init,
+	.dt_compat	= imx28_dt_compat,
+	.restart	= mxs_restart,
+MACHINE_END
-- 
1.7.0.4

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Rob Herring]

On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@
>  Freescale i.MX Platforms Device Tree Bindings
>  -----------------------------------------------
>  
> +i.MX28 Evaluation Kit
> +Required root node properties:
> +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> +
>  i.MX51 Babbage Board
>  Required root node properties:
>      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> new file mode 100644
> index 0000000..9758dc4
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28-evk.dts
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/dts-v1/;
> +/include/ "imx28.dtsi"
> +
> +/ {
> +	model = "Freescale i.MX28 Evaluation Kit";
> +	compatible = "fsl,imx28-evk", "fsl,imx28";
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x40000000 0x08000000>;
> +	};
> +
> +	ahb@80080000 {
> +		fec@800f0000 {

Use generic names: ethernet@800f0000

> +			phy-mode = "rmii";
> +			local-mac-address = [00 04 9F 01 7D 5B];
> +			status = "okay";
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> new file mode 100644
> index 0000000..acf0dab
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&icoll>;
> +
> +	aliases {
> +		serial0 = &uart1;
> +	};
> +
> +	cpus {
> +		cpu@0 {
> +			compatible = "arm,arm926ejs";
> +		};
> +	};
> +
> +	apb@80000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80000000 0x80000>;
> +		ranges;
> +
> +		apbh@80000000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80000000 0x3c900>;
> +			ranges;
> +
> +			icoll: interrupt-controller@80000000 {
> +				compatible = "fsl,imx28-icoll";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +				reg = <0x80000000 0x2000>;
> +			};
> +		};
> +
> +		apbx@80040000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80040000 0x40000>;
> +			ranges;
> +
> +			uart1: uart@80074000 {

Use generic names: uart1: serial@...

> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x80074000 0x2000>;

This is really only 0x1000 in length.

> +				interrupts = <47>;
> +			};
> +		};
> +	};
> +
> +	ahb@80080000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80080000 0x80000>;
> +		ranges;
> +
> +		fec@800f0000 {
> +			compatible = "fsl,imx28-fec";
> +			reg = <0x800f0000 0x4000>;

This too IIRC.

> +			interrupts = <101>;
> +			status = "disabled";
> +		};
> +
> +		fec@800f4000 {
> +			compatible = "fsl,imx28-fec";
> +			reg = <0x800f4000 0x4000>;
> +			interrupts = <102>;
> +			status = "disabled";
> +		};
> +
> +	};
> +};
> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> index c57f996..6ab66af 100644
> --- a/arch/arm/mach-mxs/Kconfig
> +++ b/arch/arm/mach-mxs/Kconfig
> @@ -17,6 +17,15 @@ config SOC_IMX28
>  
>  comment "MXS platforms:"
>  
> +config MACH_IMX28_DT

Perhaps this should be more generic like MACH_MXS_DT to eventually
support MX23 as well?

> +	bool "Support i.MX28 platforms from device tree"
> +	select SOC_IMX28
> +	select USE_OF
> +	select MACH_MX28EVK

This should not be selected here.

> +	help
> +	  Include support for Freescale i.MX28 based platforms
> +	  using the device tree for discovery
> +
>  config MACH_STMP378X_DEVB
>  	bool "Support STMP378x_devb Platform"
>  	select SOC_IMX23
> diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> index 908bf9a..25a4122 100644
> --- a/arch/arm/mach-mxs/Makefile
> +++ b/arch/arm/mach-mxs/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PM) += pm.o
>  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o
>  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o
>  
> +obj-$(CONFIG_MACH_IMX28_DT) += imx28-dt.o
>  obj-$(CONFIG_MACH_STMP378X_DEVB) += mach-stmp378x_devb.o
>  obj-$(CONFIG_MACH_MX23EVK) += mach-mx23evk.o
>  obj-$(CONFIG_MACH_MX28EVK) += mach-mx28evk.o
> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> new file mode 100644
> index 0000000..78d1129
> --- /dev/null
> +++ b/arch/arm/mach-mxs/imx28-dt.c
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.

It's 2012 now.

> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/time.h>
> +#include <mach/common.h>
> +#include <mach/mx28.h>
> +
> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),

Why do you need the old names?

> +	{ /* sentinel */ }
> +};
> +
> +static void __init imx28_init(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     imx28_auxdata_lookup, NULL);
> +}
> +
> +static const struct of_device_id icoll_of_match[] __initconst = {
> +	{ .compatible = "fsl,imx28-icoll", },
> +	{},
> +};
> +
> +static void __init imx28_dt_init_irq(void)
> +{
> +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);

Please do "proper" domain support for the interrupt controller and use
of_irq_init.

> +	mx28_init_irq();
> +}
> +
> +static void __init imx28_timer_init(void)
> +{
> +	mx28_clocks_init();
> +}
> +
> +static struct sys_timer imx28_timer = {
> +	.init = imx28_timer_init,
> +};
> +
> +static const char *imx28_dt_compat[] __initdata = {
> +	"fsl,imx28-evk",
> +	NULL,
> +};
> +
> +DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
> +	.map_io		= mx28_map_io,
> +	.init_irq	= imx28_dt_init_irq,
> +	.timer		= &imx28_timer,
> +	.init_machine	= imx28_init,
> +	.dt_compat	= imx28_dt_compat,
> +	.restart	= mxs_restart,
> +MACHINE_END

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Zach Sadecki]

On 03/13/2012 09:35 AM, Rob Herring wrote:
> +	ahb@80080000 {
> +		fec@800f0000 {
> Use generic names: ethernet@800f0000
Generic is good, but consistency is better, IMHO.  grepping existing dts 
files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
>> +			uart1: uart@80074000 {
> Use generic names: uart1: serial@...
Same comment here, but unfortunately there is already inconsistency in 
existing files...  25 instances of "serial@" and 35 instances of "uart@"

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Grant Likely]

On Tue, 13 Mar 2012 09:59:39 -0500, Zach Sadecki <zsadecki@itwatchdogs.com> wrote:
> On 03/13/2012 09:35 AM, Rob Herring wrote:
> > +	ahb@80080000 {
> > +		fec@800f0000 {
> > Use generic names: ethernet@800f0000
> Generic is good, but consistency is better, IMHO.  grepping existing dts 
> files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
> >> +			uart1: uart@80074000 {
> > Use generic names: uart1: serial@...
> Same comment here, but unfortunately there is already inconsistency in 
> existing files...  25 instances of "serial@" and 35 instances of "uart@"

No, Rob is correct.  The generic names recommended practice is well
established and documented.  Expand your grep search to include
arch/powerpc/bot/dts/*.

See section 2.2.2 of ePAPR[1]

[1]https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Shawn Guo]

On Tue, Mar 13, 2012 at 11:28:07AM -0600, Grant Likely wrote:
> On Tue, 13 Mar 2012 09:59:39 -0500, Zach Sadecki <zsadecki@itwatchdogs.com> wrote:
> > On 03/13/2012 09:35 AM, Rob Herring wrote:
> > > +	ahb@80080000 {
> > > +		fec@800f0000 {
> > > Use generic names: ethernet@800f0000
> > Generic is good, but consistency is better, IMHO.  grepping existing dts 
> > files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
> > >> +			uart1: uart@80074000 {
> > > Use generic names: uart1: serial@...
> > Same comment here, but unfortunately there is already inconsistency in 
> > existing files...  25 instances of "serial@" and 35 instances of "uart@"
> 
> No, Rob is correct.  The generic names recommended practice is well
> established and documented.  Expand your grep search to include
> arch/powerpc/bot/dts/*.
> 
> See section 2.2.2 of ePAPR[1]
> 
> [1]https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf
> 
I will probably need to patch imx5 and imx6 dts files with the
inconsistency fixed.

-- 
Regards,
Shawn

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
....
> > +				interrupts = <47>;
> > +			};
> > +		};
> > +	};
> > +
> > +	ahb@80080000 {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		reg = <0x80080000 0x80000>;
> > +		ranges;
> > +
> > +		fec@800f0000 {
> > +			compatible = "fsl,imx28-fec";
> > +			reg = <0x800f0000 0x4000>;
> 
> This too IIRC.
> 
The size is 16KB/0x4000 in iMX28 spec. :)

> > +			interrupts = <101>;
> > +			status = "disabled";
> > +		};
> > +
> > +		fec@800f4000 {
> > +			compatible = "fsl,imx28-fec";
> > +			reg = <0x800f4000 0x4000>;
> > +			interrupts = <102>;
> > +			status = "disabled";
> > +		};
> > +
> > +	};
> > +};
> > diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> > index c57f996..6ab66af 100644
> > --- a/arch/arm/mach-mxs/Kconfig
> > +++ b/arch/arm/mach-mxs/Kconfig
> > @@ -17,6 +17,15 @@ config SOC_IMX28
> >  
> >  comment "MXS platforms:"
> >  
> > +config MACH_IMX28_DT
> 
> Perhaps this should be more generic like MACH_MXS_DT to eventually
> support MX23 as well?
> 
I just did like the imx ways.
But yes if we see the need when do imx23 dt support.

> > +	bool "Support i.MX28 platforms from device tree"
> > +	select SOC_IMX28
> > +	select USE_OF
> > +	select MACH_MX28EVK
> 
> This should not be selected here.
> 
Like other imx soc dt board files, the imx28-dt.c may need to use some bits
like pinmux in mx28evk.c board file.

Yes, currently i can remove it since it is using uboot pinmux setting.
But when add other devices support which is not covered in u-boot like flexcan,
i may need to use non-dt board's pinmux setting.
So maybe we can keep it first and remove it when totally not dependent
on non-dt files.

> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <asm/mach/arch.h>
> > +#include <asm/mach/time.h>
> > +#include <mach/common.h>
> > +#include <mach/mx28.h>
> > +
> > +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> > +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> 
> Why do you need the old names?
> 
To keep align with the old clocks.
See arch/arm/mach-mxs/clock-mx28.c:
_REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
_REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
Is there any better way to avoid this?

> > +	{ /* sentinel */ }
> > +};
> > +
> > +static void __init imx28_init(void)
> > +{
> > +	of_platform_populate(NULL, of_default_bus_match_table,
> > +			     imx28_auxdata_lookup, NULL);
> > +}
> > +
> > +static const struct of_device_id icoll_of_match[] __initconst = {
> > +	{ .compatible = "fsl,imx28-icoll", },
> > +	{},
> > +};
> > +
> > +static void __init imx28_dt_init_irq(void)
> > +{
> > +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
> 
> Please do "proper" domain support for the interrupt controller and use
> of_irq_init.
> 
Ok, i will see it.

For others, will fix them all.
Thanks for the review.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Marek Vasut]

Dear Dong Aisheng,

> On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> > On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > This patch includes basic dt support which can boot via nfs rootfs.
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > >  arch/arm/boot/dts/imx28.dtsi                  |   88
> > >  +++++++++++++++++++++++++ arch/arm/mach-mxs/Kconfig                  
> > >    |    9 +++
> > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > >  arch/arm/mach-mxs/imx28-dt.c                  |   67
> > >  +++++++++++++++++++ 6 files changed, 200 insertions(+), 0
> > >  deletions(-)
> 
> ....
> 
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > > +	};
> > > +
> > > +	ahb@80080000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80080000 0x80000>;
> > > +		ranges;
> > > +
> > > +		fec@800f0000 {
> > > +			compatible = "fsl,imx28-fec";
> > > +			reg = <0x800f0000 0x4000>;
> > 
> > This too IIRC.
> 
> The size is 16KB/0x4000 in iMX28 spec. :)

Agreed, the memory map is slightly non-standard.

> 
> > > +			interrupts = <101>;
> > > +			status = "disabled";
> > > +		};
> > > +
> > > +		fec@800f4000 {
> > > +			compatible = "fsl,imx28-fec";
> > > +			reg = <0x800f4000 0x4000>;
> > > +			interrupts = <102>;
> > > +			status = "disabled";
> > > +		};
> > > +
> > > +	};
> > > +};
> > > diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> > > index c57f996..6ab66af 100644
> > > --- a/arch/arm/mach-mxs/Kconfig
> > > +++ b/arch/arm/mach-mxs/Kconfig
> > > @@ -17,6 +17,15 @@ config SOC_IMX28
> > > 
> > >  comment "MXS platforms:"
> > > 
> > > +config MACH_IMX28_DT
> > 
> > Perhaps this should be more generic like MACH_MXS_DT to eventually
> > support MX23 as well?
> 
> I just did like the imx ways.
> But yes if we see the need when do imx23 dt support.

Let's do it all in one swipe, there's not so much difference between these two 
chips.
> 
> > > +	bool "Support i.MX28 platforms from device tree"
> > > +	select SOC_IMX28
> > > +	select USE_OF
> > > +	select MACH_MX28EVK
> > 
> > This should not be selected here.
> 
> Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> like pinmux in mx28evk.c board file.
> 
> Yes, currently i can remove it since it is using uboot pinmux setting.
> But when add other devices support which is not covered in u-boot like
> flexcan, i may need to use non-dt board's pinmux setting.
> So maybe we can keep it first and remove it when totally not dependent
> on non-dt files.

I'd be all in for the "decremental approach" -- keep in whatever is needed and 
remove is as it becomes redundant.

> 
> > > + * The code contained herein is licensed under the GNU General Public
> > > + * License. You may obtain a copy of the GNU General Public License
> > > + * Version 2 or later at the following locations:
> > > + *
> > > + * http://www.opensource.org/licenses/gpl-license.html
> > > + * http://www.gnu.org/copyleft/gpl.html
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/irqdomain.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/of_platform.h>
> > > +#include <asm/mach/arch.h>
> > > +#include <asm/mach/time.h>
> > > +#include <mach/common.h>
> > > +#include <mach/mx28.h>
> > > +
> > > +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst
> > > = { +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart",
> > > NULL), +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR,
> > > "imx28-fec.0", NULL), +	OF_DEV_AUXDATA("fsl,imx28-fec",
> > > MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> > 
> > Why do you need the old names?
> 
> To keep align with the old clocks.
> See arch/arm/mach-mxs/clock-mx28.c:
> _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> Is there any better way to avoid this?
> 
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +static void __init imx28_init(void)
> > > +{
> > > +	of_platform_populate(NULL, of_default_bus_match_table,
> > > +			     imx28_auxdata_lookup, NULL);
> > > +}
> > > +
> > > +static const struct of_device_id icoll_of_match[] __initconst = {
> > > +	{ .compatible = "fsl,imx28-icoll", },
> > > +	{},
> > > +};
> > > +
> > > +static void __init imx28_dt_init_irq(void)
> > > +{
> > > +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
> > 
> > Please do "proper" domain support for the interrupt controller and use
> > of_irq_init.
> 
> Ok, i will see it.
> 
> For others, will fix them all.
> Thanks for the review.
> 
> Regards
> Dong Aisheng


Best regards,
Marek Vasut

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Rob Herring]

On 03/14/2012 01:23 AM, Dong Aisheng wrote:
> On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
>> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
>>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>>
>>> This patch includes basic dt support which can boot via nfs rootfs.
>>>
>>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>>>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>>>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>>>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>>>  arch/arm/mach-mxs/Makefile                    |    1 +
>>>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>>>  6 files changed, 200 insertions(+), 0 deletions(-)
> ....
>>> +				interrupts = <47>;
>>> +			};
>>> +		};
>>> +	};
>>> +
>>> +	ahb@80080000 {
>>> +		compatible = "simple-bus";
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		reg = <0x80080000 0x80000>;
>>> +		ranges;
>>> +
>>> +		fec@800f0000 {
>>> +			compatible = "fsl,imx28-fec";
>>> +			reg = <0x800f0000 0x4000>;
>>
>> This too IIRC.
>>
> The size is 16KB/0x4000 in iMX28 spec. :)

Yes, but the module only uses <4KB. You waste virtual memory space by
mapping all the unused area. It's not really an issue when you have
<512MB of RAM, but does become an issue.


>>> +			interrupts = <101>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		fec@800f4000 {
>>> +			compatible = "fsl,imx28-fec";
>>> +			reg = <0x800f4000 0x4000>;
>>> +			interrupts = <102>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +	};
>>> +};
>>> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
>>> index c57f996..6ab66af 100644
>>> --- a/arch/arm/mach-mxs/Kconfig
>>> +++ b/arch/arm/mach-mxs/Kconfig
>>> @@ -17,6 +17,15 @@ config SOC_IMX28
>>>  
>>>  comment "MXS platforms:"
>>>  
>>> +config MACH_IMX28_DT
>>
>> Perhaps this should be more generic like MACH_MXS_DT to eventually
>> support MX23 as well?
>>
> I just did like the imx ways.
> But yes if we see the need when do imx23 dt support.
> 
>>> +	bool "Support i.MX28 platforms from device tree"
>>> +	select SOC_IMX28
>>> +	select USE_OF
>>> +	select MACH_MX28EVK
>>
>> This should not be selected here.
>>
> Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> like pinmux in mx28evk.c board file.
> 
> Yes, currently i can remove it since it is using uboot pinmux setting.
> But when add other devices support which is not covered in u-boot like flexcan,
> i may need to use non-dt board's pinmux setting.
> So maybe we can keep it first and remove it when totally not dependent
> on non-dt files.

Okay either way, but my vote would be add it as needed.

>>> + * The code contained herein is licensed under the GNU General Public
>>> + * License. You may obtain a copy of the GNU General Public License
>>> + * Version 2 or later at the following locations:
>>> + *
>>> + * http://www.opensource.org/licenses/gpl-license.html
>>> + * http://www.gnu.org/copyleft/gpl.html
>>> + */
>>> +
>>> +#include <linux/init.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/of_platform.h>
>>> +#include <asm/mach/arch.h>
>>> +#include <asm/mach/time.h>
>>> +#include <mach/common.h>
>>> +#include <mach/mx28.h>
>>> +
>>> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
>>> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
>>
>> Why do you need the old names?
>>
> To keep align with the old clocks.
> See arch/arm/mach-mxs/clock-mx28.c:
> _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> Is there any better way to avoid this?

Yes, you can add more clock look-ups with the DT name.

Rob

> 
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static void __init imx28_init(void)
>>> +{
>>> +	of_platform_populate(NULL, of_default_bus_match_table,
>>> +			     imx28_auxdata_lookup, NULL);
>>> +}
>>> +
>>> +static const struct of_device_id icoll_of_match[] __initconst = {
>>> +	{ .compatible = "fsl,imx28-icoll", },
>>> +	{},
>>> +};
>>> +
>>> +static void __init imx28_dt_init_irq(void)
>>> +{
>>> +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
>>
>> Please do "proper" domain support for the interrupt controller and use
>> of_irq_init.
>>
> Ok, i will see it.
> 
> For others, will fix them all.
> Thanks for the review.
> 
> Regards
> Dong Aisheng
> 

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 09:05:34PM +0800, Rob Herring wrote:
> On 03/14/2012 01:23 AM, Dong Aisheng wrote:
> > On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> >> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> >>>
> >>> This patch includes basic dt support which can boot via nfs rootfs.
> >>>
> >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >>>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >>>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >>>  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >>>  arch/arm/mach-mxs/Makefile                    |    1 +
> >>>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >>>  6 files changed, 200 insertions(+), 0 deletions(-)
> > ....
> >>> +				interrupts = <47>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +
> >>> +	ahb@80080000 {
> >>> +		compatible = "simple-bus";
> >>> +		#address-cells = <1>;
> >>> +		#size-cells = <1>;
> >>> +		reg = <0x80080000 0x80000>;
> >>> +		ranges;
> >>> +
> >>> +		fec@800f0000 {
> >>> +			compatible = "fsl,imx28-fec";
> >>> +			reg = <0x800f0000 0x4000>;
> >>
> >> This too IIRC.
> >>
> > The size is 16KB/0x4000 in iMX28 spec. :)
> 
> Yes, but the module only uses <4KB. You waste virtual memory space by
> mapping all the unused area. It's not really an issue when you have
> <512MB of RAM, but does become an issue.
> 
Yes, it seems an issue of spec.
And all other places are using the same size.
The simplest way now may be just keep align with spec first.

> >>> +			interrupts = <101>;
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +		fec@800f4000 {
> >>> +			compatible = "fsl,imx28-fec";
> >>> +			reg = <0x800f4000 0x4000>;
> >>> +			interrupts = <102>;
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +	};
> >>> +};
> >>> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> >>> index c57f996..6ab66af 100644
> >>> --- a/arch/arm/mach-mxs/Kconfig
> >>> +++ b/arch/arm/mach-mxs/Kconfig
> >>> @@ -17,6 +17,15 @@ config SOC_IMX28
> >>>  
> >>>  comment "MXS platforms:"
> >>>  
> >>> +config MACH_IMX28_DT
> >>
> >> Perhaps this should be more generic like MACH_MXS_DT to eventually
> >> support MX23 as well?
> >>
> > I just did like the imx ways.
> > But yes if we see the need when do imx23 dt support.
> > 
> >>> +	bool "Support i.MX28 platforms from device tree"
> >>> +	select SOC_IMX28
> >>> +	select USE_OF
> >>> +	select MACH_MX28EVK
> >>
> >> This should not be selected here.
> >>
> > Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> > like pinmux in mx28evk.c board file.
> > 
> > Yes, currently i can remove it since it is using uboot pinmux setting.
> > But when add other devices support which is not covered in u-boot like flexcan,
> > i may need to use non-dt board's pinmux setting.
> > So maybe we can keep it first and remove it when totally not dependent
> > on non-dt files.
> 
> Okay either way, but my vote would be add it as needed.
> 
Your vote is ok to me.

> >>> + * The code contained herein is licensed under the GNU General Public
> >>> + * License. You may obtain a copy of the GNU General Public License
> >>> + * Version 2 or later at the following locations:
> >>> + *
> >>> + * http://www.opensource.org/licenses/gpl-license.html
> >>> + * http://www.gnu.org/copyleft/gpl.html
> >>> + */
> >>> +
> >>> +#include <linux/init.h>
> >>> +#include <linux/irqdomain.h>
> >>> +#include <linux/of_irq.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <asm/mach/arch.h>
> >>> +#include <asm/mach/time.h>
> >>> +#include <mach/common.h>
> >>> +#include <mach/mx28.h>
> >>> +
> >>> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> >>> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> >>
> >> Why do you need the old names?
> >>
> > To keep align with the old clocks.
> > See arch/arm/mach-mxs/clock-mx28.c:
> > _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> > _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> > Is there any better way to avoid this?
> 
> Yes, you can add more clock look-ups with the DT name.
> 
Got it.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Grant Likely]

On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@
>  Freescale i.MX Platforms Device Tree Bindings
>  -----------------------------------------------
>  
> +i.MX28 Evaluation Kit
> +Required root node properties:
> +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> +
>  i.MX51 Babbage Board
>  Required root node properties:
>      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> new file mode 100644
> index 0000000..9758dc4
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28-evk.dts
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/dts-v1/;
> +/include/ "imx28.dtsi"
> +
> +/ {
> +	model = "Freescale i.MX28 Evaluation Kit";
> +	compatible = "fsl,imx28-evk", "fsl,imx28";
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x40000000 0x08000000>;
> +	};
> +
> +	ahb@80080000 {
> +		fec@800f0000 {
> +			phy-mode = "rmii";
> +			local-mac-address = [00 04 9F 01 7D 5B];

Generally a bad idea to put a specific mac address into the device tree.
Better to fill it with zeros.  Otherwise all the dev boards will end up using
the same value.

> +			status = "okay";
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> new file mode 100644
> index 0000000..acf0dab
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&icoll>;
> +
> +	aliases {
> +		serial0 = &uart1;
> +	};
> +
> +	cpus {
> +		cpu@0 {
> +			compatible = "arm,arm926ejs";
> +		};
> +	};
> +
> +	apb@80000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80000000 0x80000>;
> +		ranges;
> +
> +		apbh@80000000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80000000 0x3c900>;
> +			ranges;
> +
> +			icoll: interrupt-controller@80000000 {
> +				compatible = "fsl,imx28-icoll";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +				reg = <0x80000000 0x2000>;
> +			};
> +		};
> +
> +		apbx@80040000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80040000 0x40000>;
> +			ranges;
> +
> +			uart1: uart@80074000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x80074000 0x2000>;
> +				interrupts = <47>;
> +			};
> +		};

What is the purpose of the apbh and apbx busses?  Will more device nodes
get added to them later, or does each only contain a single device?

g.

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Shawn Guo]

On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
...
> > +	apb@80000000 {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		reg = <0x80000000 0x80000>;
> > +		ranges;
> > +
> > +		apbh@80000000 {
> > +			compatible = "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x80000000 0x3c900>;
> > +			ranges;
> > +
> > +			icoll: interrupt-controller@80000000 {
> > +				compatible = "fsl,imx28-icoll";
> > +				interrupt-controller;
> > +				#interrupt-cells = <1>;
> > +				reg = <0x80000000 0x2000>;
> > +			};
> > +		};
> > +
> > +		apbx@80040000 {
> > +			compatible = "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x80040000 0x40000>;
> > +			ranges;
> > +
> > +			uart1: uart@80074000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x80074000 0x2000>;
> > +				interrupts = <47>;
> > +			};
> > +		};
> 
> What is the purpose of the apbh and apbx busses?  Will more device nodes
> get added to them later, or does each only contain a single device?
> 
Since our ultimate goal is to convert mach-mxs over to device tree,
I would also suggest we have all the hardware blocks defined in dts
from the beginning.

-- 
Regards,
Shawn

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Marek Vasut]

Dear Shawn Guo,

> On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
> ...
> 
> > > +	apb@80000000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80000000 0x80000>;
> > > +		ranges;
> > > +
> > > +		apbh@80000000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80000000 0x3c900>;
> > > +			ranges;
> > > +
> > > +			icoll: interrupt-controller@80000000 {
> > > +				compatible = "fsl,imx28-icoll";
> > > +				interrupt-controller;
> > > +				#interrupt-cells = <1>;
> > > +				reg = <0x80000000 0x2000>;
> > > +			};
> > > +		};
> > > +
> > > +		apbx@80040000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80040000 0x40000>;
> > > +			ranges;
> > > +
> > > +			uart1: uart@80074000 {
> > > +				compatible = "arm,pl011", "arm,primecell";
> > > +				reg = <0x80074000 0x2000>;
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > 
> > What is the purpose of the apbh and apbx busses?  Will more device nodes
> > get added to them later, or does each only contain a single device?
> 
> Since our ultimate goal is to convert mach-mxs over to device tree,
> I would also suggest we have all the hardware blocks defined in dts
> from the beginning.

Agreed.

Best regards,
Marek Vasut

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 01:41:20PM +0800, Shawn Guo wrote:
> On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
> ...
> > > +	apb@80000000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80000000 0x80000>;
> > > +		ranges;
> > > +
> > > +		apbh@80000000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80000000 0x3c900>;
> > > +			ranges;
> > > +
> > > +			icoll: interrupt-controller@80000000 {
> > > +				compatible = "fsl,imx28-icoll";
> > > +				interrupt-controller;
> > > +				#interrupt-cells = <1>;
> > > +				reg = <0x80000000 0x2000>;
> > > +			};
> > > +		};
> > > +
> > > +		apbx@80040000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80040000 0x40000>;
> > > +			ranges;
> > > +
> > > +			uart1: uart@80074000 {
> > > +				compatible = "arm,pl011", "arm,primecell";
> > > +				reg = <0x80074000 0x2000>;
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > 
> > What is the purpose of the apbh and apbx busses?  Will more device nodes
> > get added to them later, or does each only contain a single device?
> > 
> Since our ultimate goal is to convert mach-mxs over to device tree,
> I would also suggest we have all the hardware blocks defined in dts
> from the beginning.
> 
Also agree.
Will add them all in later patches.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > index 54bddda..9f21faf 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > @@ -1,6 +1,10 @@
> >  Freescale i.MX Platforms Device Tree Bindings
> >  -----------------------------------------------
> >  
> > +i.MX28 Evaluation Kit
> > +Required root node properties:
> > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > +
> >  i.MX51 Babbage Board
> >  Required root node properties:
> >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > new file mode 100644
> > index 0000000..9758dc4
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > @@ -0,0 +1,31 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/dts-v1/;
> > +/include/ "imx28.dtsi"
> > +
> > +/ {
> > +	model = "Freescale i.MX28 Evaluation Kit";
> > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > +
> > +	memory {
> > +		device_type = "memory";
> > +		reg = <0x40000000 0x08000000>;
> > +	};
> > +
> > +	ahb@80080000 {
> > +		fec@800f0000 {
> > +			phy-mode = "rmii";
> > +			local-mac-address = [00 04 9F 01 7D 5B];
> 
> Generally a bad idea to put a specific mac address into the device tree.
> Better to fill it with zeros.  Otherwise all the dev boards will end up using
> the same value.
> 
Yes, this issue also exists on other platfroms like mx6q.
One way is to dynamically get mac address by reading otp register as non-dt does
like:
static int __init mx28evk_fec_get_mac(void)
{
        int i;
        u32 val;
        const u32 *ocotp = mxs_get_ocotp();

        if (!ocotp)
                return -ETIMEDOUT;

        /*
         * OCOTP only stores the last 4 octets for each mac address,
         * so hard-code Freescale OUI (00:04:9f) here.
         */
        for (i = 0; i < 2; i++) {
                val = ocotp[i];
                mx28_fec_pdata[i].mac[0] = 0x00;
                mx28_fec_pdata[i].mac[1] = 0x04;
                mx28_fec_pdata[i].mac[2] = 0x9f;
                mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
                mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
                mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
        }

        return 0;
}

But it seems this needs pass mac address to fec driver via platforom data which is
not friendly to dt.

Another way may be changing fec driver to get the fixed part of mac address(first
two bytes) from device tree and read the left dynamical part from otp which i'm not
sure is better enough.

BTW, filling with zeros seems not work since it's invalid mac address.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[s.hauer]

On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > This patch includes basic dt support which can boot via nfs rootfs.
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> > >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> > >  6 files changed, 200 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > > index 54bddda..9f21faf 100644
> > > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > > @@ -1,6 +1,10 @@
> > >  Freescale i.MX Platforms Device Tree Bindings
> > >  -----------------------------------------------
> > >  
> > > +i.MX28 Evaluation Kit
> > > +Required root node properties:
> > > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > > +
> > >  i.MX51 Babbage Board
> > >  Required root node properties:
> > >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > > new file mode 100644
> > > index 0000000..9758dc4
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > > @@ -0,0 +1,31 @@
> > > +/*
> > > + * Copyright 2012 Freescale Semiconductor, Inc.
> > > + *
> > > + * The code contained herein is licensed under the GNU General Public
> > > + * License. You may obtain a copy of the GNU General Public License
> > > + * Version 2 or later at the following locations:
> > > + *
> > > + * http://www.opensource.org/licenses/gpl-license.html
> > > + * http://www.gnu.org/copyleft/gpl.html
> > > + */
> > > +
> > > +/dts-v1/;
> > > +/include/ "imx28.dtsi"
> > > +
> > > +/ {
> > > +	model = "Freescale i.MX28 Evaluation Kit";
> > > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > > +
> > > +	memory {
> > > +		device_type = "memory";
> > > +		reg = <0x40000000 0x08000000>;
> > > +	};
> > > +
> > > +	ahb@80080000 {
> > > +		fec@800f0000 {
> > > +			phy-mode = "rmii";
> > > +			local-mac-address = [00 04 9F 01 7D 5B];
> > 
> > Generally a bad idea to put a specific mac address into the device tree.
> > Better to fill it with zeros.  Otherwise all the dev boards will end up using
> > the same value.
> > 
> Yes, this issue also exists on other platfroms like mx6q.
> One way is to dynamically get mac address by reading otp register as non-dt does
> like:
> static int __init mx28evk_fec_get_mac(void)
> {
>         int i;
>         u32 val;
>         const u32 *ocotp = mxs_get_ocotp();
> 
>         if (!ocotp)
>                 return -ETIMEDOUT;
> 
>         /*
>          * OCOTP only stores the last 4 octets for each mac address,
>          * so hard-code Freescale OUI (00:04:9f) here.
>          */
>         for (i = 0; i < 2; i++) {
>                 val = ocotp[i];
>                 mx28_fec_pdata[i].mac[0] = 0x00;
>                 mx28_fec_pdata[i].mac[1] = 0x04;
>                 mx28_fec_pdata[i].mac[2] = 0x9f;
>                 mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
>                 mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
>                 mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
>         }
> 
>         return 0;
> }
> 
> But it seems this needs pass mac address to fec driver via platforom data which is
> not friendly to dt.
> 
> Another way may be changing fec driver to get the fixed part of mac address(first
> two bytes) from device tree and read the left dynamical part from otp which i'm not
> sure is better enough.
> 
> BTW, filling with zeros seems not work since it's invalid mac address.

Yes, that's the idea of using this value...

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > > 
> > > > This patch includes basic dt support which can boot via nfs rootfs.
> > > > 
> > > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > > >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> > > >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> > > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > > >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> > > >  6 files changed, 200 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > > > index 54bddda..9f21faf 100644
> > > > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > > > @@ -1,6 +1,10 @@
> > > >  Freescale i.MX Platforms Device Tree Bindings
> > > >  -----------------------------------------------
> > > >  
> > > > +i.MX28 Evaluation Kit
> > > > +Required root node properties:
> > > > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > > > +
> > > >  i.MX51 Babbage Board
> > > >  Required root node properties:
> > > >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > > > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > > > new file mode 100644
> > > > index 0000000..9758dc4
> > > > --- /dev/null
> > > > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > > > @@ -0,0 +1,31 @@
> > > > +/*
> > > > + * Copyright 2012 Freescale Semiconductor, Inc.
> > > > + *
> > > > + * The code contained herein is licensed under the GNU General Public
> > > > + * License. You may obtain a copy of the GNU General Public License
> > > > + * Version 2 or later at the following locations:
> > > > + *
> > > > + * http://www.opensource.org/licenses/gpl-license.html
> > > > + * http://www.gnu.org/copyleft/gpl.html
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +/include/ "imx28.dtsi"
> > > > +
> > > > +/ {
> > > > +	model = "Freescale i.MX28 Evaluation Kit";
> > > > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > > > +
> > > > +	memory {
> > > > +		device_type = "memory";
> > > > +		reg = <0x40000000 0x08000000>;
> > > > +	};
> > > > +
> > > > +	ahb@80080000 {
> > > > +		fec@800f0000 {
> > > > +			phy-mode = "rmii";
> > > > +			local-mac-address = [00 04 9F 01 7D 5B];
> > > 
> > > Generally a bad idea to put a specific mac address into the device tree.
> > > Better to fill it with zeros.  Otherwise all the dev boards will end up using
> > > the same value.
> > > 
> > Yes, this issue also exists on other platfroms like mx6q.
> > One way is to dynamically get mac address by reading otp register as non-dt does
> > like:
> > static int __init mx28evk_fec_get_mac(void)
> > {
> >         int i;
> >         u32 val;
> >         const u32 *ocotp = mxs_get_ocotp();
> > 
> >         if (!ocotp)
> >                 return -ETIMEDOUT;
> > 
> >         /*
> >          * OCOTP only stores the last 4 octets for each mac address,
> >          * so hard-code Freescale OUI (00:04:9f) here.
> >          */
> >         for (i = 0; i < 2; i++) {
> >                 val = ocotp[i];
> >                 mx28_fec_pdata[i].mac[0] = 0x00;
> >                 mx28_fec_pdata[i].mac[1] = 0x04;
> >                 mx28_fec_pdata[i].mac[2] = 0x9f;
> >                 mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> >                 mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> >                 mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> >         }
> > 
> >         return 0;
> > }
> > 
> > But it seems this needs pass mac address to fec driver via platforom data which is
> > not friendly to dt.
> > 
> > Another way may be changing fec driver to get the fixed part of mac address(first
> > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > sure is better enough.
> > 
> > BTW, filling with zeros seems not work since it's invalid mac address.
> 
> Yes, that's the idea of using this value...
> 
But comparing to use an invalid value, why not just do not define that
local-mac-address property?

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
[...]
> > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > not friendly to dt.
> > > 
> > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > sure is better enough.
> > > 
> > > BTW, filling with zeros seems not work since it's invalid mac address.
> > 
> > Yes, that's the idea of using this value...
> > 
> But comparing to use an invalid value, why not just do not define that
> local-mac-address property?
> 
Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
which is contrary to coding a "valid" default value for it somewhere!
The owner of a certain MAC address range (OUI) is responsible for
ensuring the uniqueness. Thus only they may assign a specific MAC
address to a specific device.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> [...]
> > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > not friendly to dt.
> > > > 
> > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > sure is better enough.
> > > > 
> > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > 
> > > Yes, that's the idea of using this value...
> > > 
> > But comparing to use an invalid value, why not just do not define that
> > local-mac-address property?
> > 
> Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> which is contrary to coding a "valid" default value for it somewhere!
> The owner of a certain MAC address range (OUI) is responsible for
> ensuring the uniqueness. Thus only they may assign a specific MAC
> address to a specific device.
> 
Yes, you're correct.
So i propose to read the MAC address from MX28 OTP in fec driver instead of
define it in device tree in my last mail.
http://www.spinics.net/lists/arm-kernel/msg165040.html
Do you have any comment on that way?

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > Hi,
> > 
> > Dong Aisheng writes:
> > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > [...]
> > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > not friendly to dt.
> > > > > 
> > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > sure is better enough.
> > > > > 
> > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > 
> > > > Yes, that's the idea of using this value...
> > > > 
> > > But comparing to use an invalid value, why not just do not define that
> > > local-mac-address property?
> > > 
> > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > which is contrary to coding a "valid" default value for it somewhere!
> > The owner of a certain MAC address range (OUI) is responsible for
> > ensuring the uniqueness. Thus only they may assign a specific MAC
> > address to a specific device.
> > 
> Yes, you're correct.
> So i propose to read the MAC address from MX28 OTP in fec driver instead of
> define it in device tree in my last mail.
> http://www.spinics.net/lists/arm-kernel/msg165040.html
> Do you have any comment on that way?
> 
That patch sets the OUI to the Freescale owned MAC range. Thus only
Freescale products may be able to use the driver.

Anyway there is no definite spec how the MAC address(es) are stored
in the fuse map. Thus reading the MAC from there is more or less
platform specific.

Currently I'm setting up the MAC address for our TX28 from the fuses
in the platform code passed via platform_data, but that will obviously
not work with DT.

The correct way would probably be to pass the MAC from the bootloader
via a DT blob. But that would require all bootloaders to be updated
first to support DTS. :(

An intermediate solution could be using OF_DEV_AUXDATA to pass a
platform_data struct that may contain a MAC address set up by the
platform code.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > [...]
> > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > not friendly to dt.
> > > > > > 
> > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > sure is better enough.
> > > > > > 
> > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > 
> > > > > Yes, that's the idea of using this value...
> > > > > 
> > > > But comparing to use an invalid value, why not just do not define that
> > > > local-mac-address property?
> > > > 
> > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > which is contrary to coding a "valid" default value for it somewhere!
> > > The owner of a certain MAC address range (OUI) is responsible for
> > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > address to a specific device.
> > > 
> > Yes, you're correct.
> > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > define it in device tree in my last mail.
> > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > Do you have any comment on that way?
> > 
> That patch sets the OUI to the Freescale owned MAC range. Thus only
> Freescale products may be able to use the driver.
> 
No.
My proposal is only set the fixed part(first two octets) in board dts file,
each board knows it's value, and read the left 4 octets from OCOTP dynamically.
Obviously the freeescale board dts file is only for FSL platforms.
Other platforms can define their fix part of MAC address in their dts file.

> Anyway there is no definite spec how the MAC address(es) are stored
> in the fuse map. Thus reading the MAC from there is more or less
> platform specific.
> 
It's just provide one more option since there are customers storing the MAC
in the fuse map.

> Currently I'm setting up the MAC address for our TX28 from the fuses
> in the platform code passed via platform_data, but that will obviously
> not work with DT.
> 
Non-dt can also use my proposal, then you only need to pass the fixed part of
MAC via platfrom data, the left will be read by fec driver.

> The correct way would probably be to pass the MAC from the bootloader
> via a DT blob. But that would require all bootloaders to be updated
> first to support DTS. :(
> 
But uboot will face the same problem that can not define a valid MAC
in dts, did i undertand correctly?

> An intermediate solution could be using OF_DEV_AUXDATA to pass a
> platform_data struct that may contain a MAC address set up by the
> platform code.
> 
Yes, it's a way but i'm afraid will not get accepted by dt people.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
[...]
> My proposal is only set the fixed part(first two octets) in board dts file,
> each board knows it's value, and read the left 4 octets from OCOTP dynamically.
>
The OUI part is three bytes, not two! But anyway, since there is no
definition on how the MAC has to be stored in OCOTP there is no way
for the driver to know how to interpret the data it may read from
there.

> > Anyway there is no definite spec how the MAC address(es) are stored
> > in the fuse map. Thus reading the MAC from there is more or less
> > platform specific.
> > 
> It's just provide one more option since there are customers storing the MAC
> in the fuse map.
> 
How would the driver know, whether and how the customer has stored the
MAC address in OCOTP? E.g. on our TX28 modules the FULL MAC is stored
in the CUSTOM registers in OCOTP.

> > Currently I'm setting up the MAC address for our TX28 from the fuses
> > in the platform code passed via platform_data, but that will obviously
> > not work with DT.
> > 
> Non-dt can also use my proposal, then you only need to pass the fixed part of
> MAC via platfrom data, the left will be read by fec driver.
> 
Reading the MAC from fuses is platform sepcific. The driver cannot
know how the MAC is stored there and needs to rely on platform
specific code to do this.

> > The correct way would probably be to pass the MAC from the bootloader
> > via a DT blob. But that would require all bootloaders to be updated
> > first to support DTS. :(
> > 
> But uboot will face the same problem that can not define a valid MAC
> in dts, did i undertand correctly?
> 
That's why I said "would require all bootloaders to be updated first
to support DTS".


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Fri, Mar 16, 2012 at 03:48:13PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> [...]
> > My proposal is only set the fixed part(first two octets) in board dts file,
> > each board knows it's value, and read the left 4 octets from OCOTP dynamically.
> >
> The OUI part is three bytes, not two! But anyway, since there is no
> definition on how the MAC has to be stored in OCOTP there is no way
> for the driver to know how to interpret the data it may read from
> there.
> 
I may miss this, i will double check it.
Thanks for the info. :-)

> > > Anyway there is no definite spec how the MAC address(es) are stored
> > > in the fuse map. Thus reading the MAC from there is more or less
> > > platform specific.
> > > 
> > It's just provide one more option since there are customers storing the MAC
> > in the fuse map.
> > 
> How would the driver know, whether and how the customer has stored the
> MAC address in OCOTP? E.g. on our TX28 modules the FULL MAC is stored
> in the CUSTOM registers in OCOTP.
> 
> > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > in the platform code passed via platform_data, but that will obviously
> > > not work with DT.
> > > 
> > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > MAC via platfrom data, the left will be read by fec driver.
> > 
> Reading the MAC from fuses is platform sepcific. The driver cannot
> know how the MAC is stored there and needs to rely on platform
> specific code to do this.
> 
> > > The correct way would probably be to pass the MAC from the bootloader
> > > via a DT blob. But that would require all bootloaders to be updated
> > > first to support DTS. :(
> > > 
> > But uboot will face the same problem that can not define a valid MAC
> > in dts, did i undertand correctly?
> > 
> That's why I said "would require all bootloaders to be updated first
> to support DTS".

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Grant Likely writes:
> On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > > > Hi,
> > > > > 
> > > > > Dong Aisheng writes:
> > > > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > > > [...]
> > > > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > > > not friendly to dt.
> > > > > > > > 
> > > > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > > > sure is better enough.
> > > > > > > > 
> > > > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > > > 
> > > > > > > Yes, that's the idea of using this value...
> > > > > > > 
> > > > > > But comparing to use an invalid value, why not just do not define that
> > > > > > local-mac-address property?
> > > > > > 
> > > > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > > > which is contrary to coding a "valid" default value for it somewhere!
> > > > > The owner of a certain MAC address range (OUI) is responsible for
> > > > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > > > address to a specific device.
> > > > > 
> > > > Yes, you're correct.
> > > > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > > > define it in device tree in my last mail.
> > > > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > > > Do you have any comment on that way?
> > > > 
> > > That patch sets the OUI to the Freescale owned MAC range. Thus only
> > > Freescale products may be able to use the driver.
> > > 
> > No.
> > My proposal is only set the fixed part(first two octets) in board dts file,
> > each board knows it's value, and read the left 4 octets from OCOTP dynamically.
> > Obviously the freeescale board dts file is only for FSL platforms.
> > Other platforms can define their fix part of MAC address in their dts file.
> > 
> > > Anyway there is no definite spec how the MAC address(es) are stored
> > > in the fuse map. Thus reading the MAC from there is more or less
> > > platform specific.
> > > 
> > It's just provide one more option since there are customers storing the MAC
> > in the fuse map.
> 
> That should be straight forward to support; have a property that
> specifies the method used for fetching/calculating the MAC.
> 
Executable code stored inside a DT blob? ;)

> > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > in the platform code passed via platform_data, but that will obviously
> > > not work with DT.
> > > 
> > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > MAC via platfrom data, the left will be read by fec driver.
> > 
> > > The correct way would probably be to pass the MAC from the bootloader
> > > via a DT blob. But that would require all bootloaders to be updated
> > > first to support DTS. :(
> > > 
> > But uboot will face the same problem that can not define a valid MAC
> > in dts, did i undertand correctly?
> > 
> > > An intermediate solution could be using OF_DEV_AUXDATA to pass a
> > > platform_data struct that may contain a MAC address set up by the
> > > platform code.
> > > 
> > Yes, it's a way but i'm afraid will not get accepted by dt people.
> 
> The problem remains; where does the platform code get the MAC address
> from?  The kernel has to devise it from *somewhere* and the best place
> for that should be in the MAC driver itself.
> 
The platform code knows how and where the MAC is stored on a specific
platform. The driver has no business in knowing platform details like
this. Thus only platform code (or the bootloader) can provide the MAC
to the driver.

> I've got no problem with the idea of only specifying the OUI in the DT
> and the driver extracting the remaining 3 bytes from the hardware.
> 
> In principle, that's the design intent for DT systems anyway; it
> describes things which cannot be probed from the hardware, or tells
> the OS how the data is encoded in the hardware.
> 
If all bootloaders were aware of DT this wouldn't be a problem,
because they could place the correct MAC inside the DT blob right
away.
But in the mean time (AKA forever) we need some other solution.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Grant Likely writes:
> On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > Grant Likely writes:
> > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > > > > Dong Aisheng writes:
> > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > > > Anyway there is no definite spec how the MAC address(es) are stored
> > > > > in the fuse map. Thus reading the MAC from there is more or less
> > > > > platform specific.
> > > > > 
> > > > It's just provide one more option since there are customers storing the MAC
> > > > in the fuse map.
> > > 
> > > That should be straight forward to support; have a property that
> > > specifies the method used for fetching/calculating the MAC.
> > > 
> > Executable code stored inside a DT blob? ;)
> 
> I know you're joking here, but I'm going to answer seriously
> anyway... Absolutely not.  What I'm suggesting is a property that
> specifies the method used to determine the mac address.  Something
> like (off the top of my head):
> 
> 	local-mac-address = [01 02 03 00 00 00];
> 	local-mac-mask = [0xff 0xff 0xff 0 0 0];
> 	mac-encoding = "append-serial-number";
> 
That still does not specify where the remaining part of the MAC is
stored and how it should be retrieved.

> > > > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > > > in the platform code passed via platform_data, but that will obviously
> > > > > not work with DT.
> > > > > 
> > > > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > > > MAC via platfrom data, the left will be read by fec driver.
> > > > 
> > > > > The correct way would probably be to pass the MAC from the bootloader
> > > > > via a DT blob. But that would require all bootloaders to be updated
> > > > > first to support DTS. :(
> > > > > 
> > > > But uboot will face the same problem that can not define a valid MAC
> > > > in dts, did i undertand correctly?
> > > > 
> > > > > An intermediate solution could be using OF_DEV_AUXDATA to pass a
> > > > > platform_data struct that may contain a MAC address set up by the
> > > > > platform code.
> > > > > 
> > > > Yes, it's a way but i'm afraid will not get accepted by dt people.
> > > 
> > > The problem remains; where does the platform code get the MAC address
> > > from?  The kernel has to devise it from *somewhere* and the best place
> > > for that should be in the MAC driver itself.
> > > 
> > The platform code knows how and where the MAC is stored on a specific
> > platform. The driver has no business in knowing platform details like
> > this. Thus only platform code (or the bootloader) can provide the MAC
> > to the driver.
> 
> Okay, if so then it would be wise to have a reliable function for the
> MAC driver to call to lookup it's address as determined by platform
> code.  Alternately, the platform code can write the correct mac
> address into the device tree node at init time (see
> prom_update_property() and prom_add_property()).
> 
That sounds good. Didn't know about those functions. That could be
used to mimic the current behaviour of supplying the MAC via
platform_data.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
>> Hi,
>>
>> Grant Likely writes:
>> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
>> > > Grant Likely writes:
>> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
>> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
>> > > > > > Dong Aisheng writes:
>> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
>> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
>> > > > > > in the fuse map. Thus reading the MAC from there is more or less
>> > > > > > platform specific.
>> > > > > >
>> > > > > It's just provide one more option since there are customers storing the MAC
>> > > > > in the fuse map.
>> > > >
>> > > > That should be straight forward to support; have a property that
>> > > > specifies the method used for fetching/calculating the MAC.
>> > > >
>> > > Executable code stored inside a DT blob? ;)
>> >
>> > I know you're joking here, but I'm going to answer seriously
>> > anyway... Absolutely not.  What I'm suggesting is a property that
>> > specifies the method used to determine the mac address.  Something
>> > like (off the top of my head):
>> >
>> >     local-mac-address = [01 02 03 00 00 00];
>> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
>> >     mac-encoding = "append-serial-number";
>> >
>> That still does not specify where the remaining part of the MAC is
>> stored and how it should be retrieved.
>
> I'm suggesting that you define a string that means something specific;
> that hopefully can be shared by multiple platforms.  For example,
> "append-serial-number" might mean start with the values selected by
> AND of local-mac-address and local-mac-mask, and OR in the board's
> serial number.  You would need to define something that worked if this
> was the solution you used.
>
I'm intend to Grant's this suggestion.
This can be shared by all other imx platforms which stores mac address in
fuse map and this is commonly used by customer.
Then we do not need keep writing repeat code for different platforms
via platform
data.

For Lothar's question, we can add a property fuse_mac_offset to indicate
read mac from fuse map and where to read it.
For how many bytes to read, we can calculate it from the local-mac-mask.
For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
as 3 bypes.

Then we have three properties:
local-mac-address = [01 02 03 00 00 00];
local-mac-mask = [0xff 0xff 0xff 0 0 0];
fuse_mac_offset = <1>;

In fec driver, the final mac address can be got by:
local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)

Lathar,
Do you think if this is ok?

>> > Okay, if so then it would be wise to have a reliable function for the
>> > MAC driver to call to lookup it's address as determined by platform
>> > code.  Alternately, the platform code can write the correct mac
>> > address into the device tree node at init time (see
>> > prom_update_property() and prom_add_property()).
>> >
>> That sounds good. Didn't know about those functions. That could be
>> used to mimic the current behaviour of supplying the MAC via
>> platform_data.
>
> I'm okay with doing this; but make sure you remove the bogus
> local-mac-address from the .dts file.
>
This is a backup solution to me if the above solution you suggested can not
work.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> >> Hi,
> >>
> >> Grant Likely writes:
> >> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> >> > > Grant Likely writes:
> >> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> >> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> >> > > > > > Dong Aisheng writes:
> >> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> >> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
> >> > > > > > in the fuse map. Thus reading the MAC from there is more or less
> >> > > > > > platform specific.
> >> > > > > >
> >> > > > > It's just provide one more option since there are customers storing the MAC
> >> > > > > in the fuse map.
> >> > > >
> >> > > > That should be straight forward to support; have a property that
> >> > > > specifies the method used for fetching/calculating the MAC.
> >> > > >
> >> > > Executable code stored inside a DT blob? ;)
> >> >
> >> > I know you're joking here, but I'm going to answer seriously
> >> > anyway... Absolutely not.  What I'm suggesting is a property that
> >> > specifies the method used to determine the mac address.  Something
> >> > like (off the top of my head):
> >> >
> >> >     local-mac-address = [01 02 03 00 00 00];
> >> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
> >> >     mac-encoding = "append-serial-number";
> >> >
> >> That still does not specify where the remaining part of the MAC is
> >> stored and how it should be retrieved.
> >
> > I'm suggesting that you define a string that means something specific;
> > that hopefully can be shared by multiple platforms.  For example,
> > "append-serial-number" might mean start with the values selected by
> > AND of local-mac-address and local-mac-mask, and OR in the board's
> > serial number.  You would need to define something that worked if this
> > was the solution you used.
> >
> I'm intend to Grant's this suggestion.
> This can be shared by all other imx platforms which stores mac address in
> fuse map and this is commonly used by customer.
> Then we do not need keep writing repeat code for different platforms
> via platform
> data.
> 
> For Lothar's question, we can add a property fuse_mac_offset to indicate
> read mac from fuse map and where to read it.
>
That's not really enough. The format (big-endian vs. little-endian)
may also differ. But it's not really necessary, if the solution with
prom_update_property() works, since then platform code has full
control over the MAC.

> For how many bytes to read, we can calculate it from the local-mac-mask.
> For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
> as 3 bypes.
> 
> Then we have three properties:
> local-mac-address = [01 02 03 00 00 00];
> local-mac-mask = [0xff 0xff 0xff 0 0 0];
> fuse_mac_offset = <1>;
> 
> In fec driver, the final mac address can be got by:
> local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)
> 
> Lathar,
> Do you think if this is ok?
>
No. IMO the FEC driver itself should not read any fuses.
Reading fuses is a SoC specific thing and the FEC driver should not
depend on the idiosyncrasies of some SoC wrt. anything else but the
implementation of the FEC itself.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Tue, Mar 20, 2012 at 09:17:48PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > >> Hi,
> > >>
> > >> Grant Likely writes:
> > >> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > >> > > Grant Likely writes:
> > >> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > >> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > >> > > > > > Dong Aisheng writes:
> > >> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > >> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
> > >> > > > > > in the fuse map. Thus reading the MAC from there is more or less
> > >> > > > > > platform specific.
> > >> > > > > >
> > >> > > > > It's just provide one more option since there are customers storing the MAC
> > >> > > > > in the fuse map.
> > >> > > >
> > >> > > > That should be straight forward to support; have a property that
> > >> > > > specifies the method used for fetching/calculating the MAC.
> > >> > > >
> > >> > > Executable code stored inside a DT blob? ;)
> > >> >
> > >> > I know you're joking here, but I'm going to answer seriously
> > >> > anyway... Absolutely not.  What I'm suggesting is a property that
> > >> > specifies the method used to determine the mac address.  Something
> > >> > like (off the top of my head):
> > >> >
> > >> >     local-mac-address = [01 02 03 00 00 00];
> > >> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
> > >> >     mac-encoding = "append-serial-number";
> > >> >
> > >> That still does not specify where the remaining part of the MAC is
> > >> stored and how it should be retrieved.
> > >
> > > I'm suggesting that you define a string that means something specific;
> > > that hopefully can be shared by multiple platforms.  For example,
> > > "append-serial-number" might mean start with the values selected by
> > > AND of local-mac-address and local-mac-mask, and OR in the board's
> > > serial number.  You would need to define something that worked if this
> > > was the solution you used.
> > >
> > I'm intend to Grant's this suggestion.
> > This can be shared by all other imx platforms which stores mac address in
> > fuse map and this is commonly used by customer.
> > Then we do not need keep writing repeat code for different platforms
> > via platform
> > data.
> > 
> > For Lothar's question, we can add a property fuse_mac_offset to indicate
> > read mac from fuse map and where to read it.
> >
> That's not really enough. The format (big-endian vs. little-endian)
> may also differ. But it's not really necessary, if the solution with
> prom_update_property() works, since then platform code has full
> control over the MAC.
> 
> > For how many bytes to read, we can calculate it from the local-mac-mask.
> > For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
> > as 3 bypes.
> > 
> > Then we have three properties:
> > local-mac-address = [01 02 03 00 00 00];
> > local-mac-mask = [0xff 0xff 0xff 0 0 0];
> > fuse_mac_offset = <1>;
> > 
> > In fec driver, the final mac address can be got by:
> > local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)
> > 
> > Lathar,
> > Do you think if this is ok?
> >
> No. IMO the FEC driver itself should not read any fuses.
> Reading fuses is a SoC specific thing and the FEC driver should not
> depend on the idiosyncrasies of some SoC wrt. anything else but the
> implementation of the FEC itself.
> 
Yes, i think i'm agree with this point.
Okay, i will do that with prom_update_property.
Thanks for suggestion.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Shawn Guo]

On Thu, Mar 15, 2012 at 12:22:04PM +0100, Lothar Waßmann wrote:
...
> An intermediate solution could be using OF_DEV_AUXDATA to pass a
> platform_data struct that may contain a MAC address set up by the
> platform code.
> 
+1

-- 
Regards,
Shawn

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[s.hauer]

On Thu, Mar 15, 2012 at 06:59:28PM +0800, Dong Aisheng wrote:
> On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > Hi,
> > 
> > Dong Aisheng writes:
> > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > [...]
> > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > not friendly to dt.
> > > > > 
> > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > sure is better enough.
> > > > > 
> > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > 
> > > > Yes, that's the idea of using this value...
> > > > 
> > > But comparing to use an invalid value, why not just do not define that
> > > local-mac-address property?
> > > 
> > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > which is contrary to coding a "valid" default value for it somewhere!
> > The owner of a certain MAC address range (OUI) is responsible for
> > ensuring the uniqueness. Thus only they may assign a specific MAC
> > address to a specific device.
> > 
> Yes, you're correct.
> So i propose to read the MAC address from MX28 OTP in fec driver instead of
> define it in device tree in my last mail.
> http://www.spinics.net/lists/arm-kernel/msg165040.html
> Do you have any comment on that way?

Yes, you could do that. Otherwise it's perfectly fine to pass the MAC
address in the device tree, it's just not ok to put specific values
into the kernel source. You could leave the values blank and let the
bootloader fill them in. Or you could fill in valid values before you
compile the devicetree, but be prepared for surprises once you have
two boards in your network and use the devicetree for both.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Thu, Mar 15, 2012 at 07:24:19PM +0800, s.hauer@pengutronix.de wrote:
> On Thu, Mar 15, 2012 at 06:59:28PM +0800, Dong Aisheng wrote:
> > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > [...]
> > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > not friendly to dt.
> > > > > > 
> > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > sure is better enough.
> > > > > > 
> > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > 
> > > > > Yes, that's the idea of using this value...
> > > > > 
> > > > But comparing to use an invalid value, why not just do not define that
> > > > local-mac-address property?
> > > > 
> > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > which is contrary to coding a "valid" default value for it somewhere!
> > > The owner of a certain MAC address range (OUI) is responsible for
> > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > address to a specific device.
> > > 
> > Yes, you're correct.
> > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > define it in device tree in my last mail.
> > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > Do you have any comment on that way?
> 
> Yes, you could do that. Otherwise it's perfectly fine to pass the MAC
> address in the device tree, it's just not ok to put specific values
> into the kernel source. You could leave the values blank and let the
Yes, correct.

> bootloader fill them in. Or you could fill in valid values before you
> compile the devicetree, but be prepared for surprises once you have
> two boards in your network and use the devicetree for both.
> 

Thanks for the info.
I will submit that patch for discussion.

Regards
Dong Aisheng

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Sascha Hauer]

On Tue, Mar 13, 2012 at 04:47:04PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@

> +
> +static const char *imx28_dt_compat[] __initdata = {
> +	"fsl,imx28-evk",
> +	NULL,
> +};
> +

I suggest to add a generic "fsl,imx28" entry here. It helps people to
start an unpatched kernel on their boards.

Sascha

> +DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
> +	.map_io		= mx28_map_io,
> +	.init_irq	= imx28_dt_init_irq,
> +	.timer		= &imx28_timer,
> +	.init_machine	= imx28_init,
> +	.dt_compat	= imx28_dt_compat,
> +	.restart	= mxs_restart,
> +MACHINE_END
> -- 
> 1.7.0.4
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/5] ARM: imx28: add basic dt support

[Dong Aisheng]

On Thu, Mar 15, 2012 at 03:41:23AM +0800, Sascha Hauer wrote:
> On Tue, Mar 13, 2012 at 04:47:04PM +0800, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > index 54bddda..9f21faf 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > @@ -1,6 +1,10 @@
> 
> > +
> > +static const char *imx28_dt_compat[] __initdata = {
> > +	"fsl,imx28-evk",
> > +	NULL,
> > +};
> > +
> 
> I suggest to add a generic "fsl,imx28" entry here. It helps people to
> start an unpatched kernel on their boards.
> 
Good point.
Thanks for reminder.

Regards
Dong Aisheng

[PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

---
The patch is still using a private way for dma part binding
since the common dma binding is still under discussion.
http://www.spinics.net/lists/linux-omap/msg65528.html

Will update to use common dma binding when it hits mainline.
---
 .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
 drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
new file mode 100644
index 0000000..adc1142
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
@@ -0,0 +1,23 @@
+* FREESCALE MXS MMC peripheral
+
+Required properties:
+- compatible : Should be "fsl,<chip>-mmc"
+- reg : Should contain registers location and length
+- interrupts : Should contain interrupt.
+  The format is <irq_err irq_dma>.
+- dma_channel: Should contain the dma channel it uses
+
+Optional properties:
+- wp-gpios : Specify GPIOs for write protection
+- slot-4bit: Specify 4 bit mode support
+- slot-8bit: Specify 8 bit and 4 bit mode support
+
+Examples:
+mmc1: ssp@80010000 {
+	compatible = "fsl,imx28-mmc";
+	reg = <0x80010000 2000>;
+	/* <irq_err irq_dma> */
+	interrupts = <96 82>;
+	dma_channel = <0>;
+	slot-8bit;
+};
diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 382c835..6cf2d17 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -38,6 +38,10 @@
 #include <linux/gpio.h>
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
 
 #include <mach/mxs.h>
 #include <mach/common.h>
@@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan, void *param)
 	return true;
 }
 
+#ifdef CONFIG_OF
+static struct resource * __devinit mxs_mmc_get_of_dmares(
+				struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *dmares;
+	int ret;
+
+	if (!np)
+		return NULL;
+
+	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);
+	dmares->flags = IORESOURCE_DMA;
+	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to get dmares from dt\n");
+		return NULL;
+	}
+	dmares->end = dmares->start;
+
+	return dmares;
+}
+
+static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
+				struct mxs_mmc_platform_data **ppdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mxs_mmc_platform_data *pdata = *ppdata;
+
+	if (!np)
+		return -ENODEV;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+
+	if (of_get_property(np, "slot-8bit", NULL))
+		pdata->flags |= SLOTF_8_BIT_CAPABLE;
+
+	if (of_get_property(np, "slot-4bit", NULL))
+		pdata->flags |= SLOTF_4_BIT_CAPABLE;
+
+	pdata->wp_gpio = of_get_named_gpio(np, "wp-gpios", 0);
+
+	dev_dbg(&pdev->dev, "wp-gpios %d flags %d\n", pdata->wp_gpio,
+		pdata->flags);
+
+	return 0;
+}
+#else
+static struct resource * __devinit mxs_mmc_get_of_dmares(
+				struct platform_device *pdev)
+{
+	return NULL;
+}
+static inline int mxs_mmc_get_of_property(struct platform_device *pdev,
+			struct mxs_mmc_platform_data *pdata)
+{
+	return -ENODEV;
+}
+#endif
+
 static int mxs_mmc_probe(struct platform_device *pdev)
 {
 	struct mxs_mmc_host *host;
 	struct mmc_host *mmc;
 	struct resource *iores, *dmares, *r;
-	struct mxs_mmc_platform_data *pdata;
+	struct mxs_mmc_platform_data *pdata = NULL;
 	int ret = 0, irq_err, irq_dma;
 	dma_cap_mask_t mask;
 
 	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+	dmares = mxs_mmc_get_of_dmares(pdev);
+	if (dmares == NULL)
+		dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
 	irq_err = platform_get_irq(pdev, 0);
 	irq_dma = platform_get_irq(pdev, 1);
 	if (!iores || !dmares || irq_err < 0 || irq_dma < 0)
@@ -740,7 +806,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
 	mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
 		    MMC_CAP_SDIO_IRQ | MMC_CAP_NEEDS_POLL;
 
-	pdata =	mmc_dev(host->mmc)->platform_data;
+	mxs_mmc_get_of_property(pdev, &pdata);
+	if (pdata == NULL)
+		pdata =	mmc_dev(host->mmc)->platform_data;
 	if (pdata) {
 		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
 			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
@@ -851,6 +919,13 @@ static const struct dev_pm_ops mxs_mmc_pm_ops = {
 };
 #endif
 
+static const struct of_device_id mxs_mmc_dt_ids[] = {
+	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
+	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids);
+
 static struct platform_driver mxs_mmc_driver = {
 	.probe		= mxs_mmc_probe,
 	.remove		= mxs_mmc_remove,
@@ -860,6 +935,7 @@ static struct platform_driver mxs_mmc_driver = {
 #ifdef CONFIG_PM
 		.pm	= &mxs_mmc_pm_ops,
 #endif
+		.of_match_table = mxs_mmc_dt_ids,
 	},
 };
 
-- 
1.7.0.4

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Grant Likely]

On Tue, 13 Mar 2012 16:47:05 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> ---
> The patch is still using a private way for dma part binding
> since the common dma binding is still under discussion.
> http://www.spinics.net/lists/linux-omap/msg65528.html
> 
> Will update to use common dma binding when it hits mainline.
> ---
>  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
>  drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
>  2 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> new file mode 100644
> index 0000000..adc1142
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> @@ -0,0 +1,23 @@
> +* FREESCALE MXS MMC peripheral
> +
> +Required properties:
> +- compatible : Should be "fsl,<chip>-mmc"
> +- reg : Should contain registers location and length
> +- interrupts : Should contain interrupt.
> +  The format is <irq_err irq_dma>.
> +- dma_channel: Should contain the dma channel it uses

Don't use '_' in property names.

The is a generic dma binding being drafted that uses a phandle to the dma
controller and the ability to encode channel numbers.  You may want to take
a look at it.

> +
> +Optional properties:
> +- wp-gpios : Specify GPIOs for write protection
> +- slot-4bit: Specify 4 bit mode support
> +- slot-8bit: Specify 8 bit and 4 bit mode support
> +
> +Examples:
> +mmc1: ssp@80010000 {
> +	compatible = "fsl,imx28-mmc";
> +	reg = <0x80010000 2000>;
> +	/* <irq_err irq_dma> */
> +	interrupts = <96 82>;
> +	dma_channel = <0>;
> +	slot-8bit;
> +};
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 382c835..6cf2d17 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -38,6 +38,10 @@
>  #include <linux/gpio.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
>  
>  #include <mach/mxs.h>
>  #include <mach/common.h>
> @@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan, void *param)
>  	return true;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct resource * __devinit mxs_mmc_get_of_dmares(
> +				struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *dmares;
> +	int ret;
> +
> +	if (!np)
> +		return NULL;
> +
> +	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);

devm_kzalloc()

> +	dmares->flags = IORESOURCE_DMA;
> +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> +		return NULL;
> +	}
> +	dmares->end = dmares->start;
> +
> +	return dmares;
> +}
> +
> +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> +				struct mxs_mmc_platform_data **ppdata)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct mxs_mmc_platform_data *pdata = *ppdata;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);

Ditto

Fix up those comments and you can add my:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> +
> +	if (of_get_property(np, "slot-8bit", NULL))
> +		pdata->flags |= SLOTF_8_BIT_CAPABLE;
> +
> +	if (of_get_property(np, "slot-4bit", NULL))
> +		pdata->flags |= SLOTF_4_BIT_CAPABLE;
> +
> +	pdata->wp_gpio = of_get_named_gpio(np, "wp-gpios", 0);
> +
> +	dev_dbg(&pdev->dev, "wp-gpios %d flags %d\n", pdata->wp_gpio,
> +		pdata->flags);
> +
> +	return 0;
> +}
> +#else
> +static struct resource * __devinit mxs_mmc_get_of_dmares(
> +				struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +static inline int mxs_mmc_get_of_property(struct platform_device *pdev,
> +			struct mxs_mmc_platform_data *pdata)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int mxs_mmc_probe(struct platform_device *pdev)
>  {
>  	struct mxs_mmc_host *host;
>  	struct mmc_host *mmc;
>  	struct resource *iores, *dmares, *r;
> -	struct mxs_mmc_platform_data *pdata;
> +	struct mxs_mmc_platform_data *pdata = NULL;
>  	int ret = 0, irq_err, irq_dma;
>  	dma_cap_mask_t mask;
>  
>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
> +	dmares = mxs_mmc_get_of_dmares(pdev);
> +	if (dmares == NULL)
> +		dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>  	irq_err = platform_get_irq(pdev, 0);
>  	irq_dma = platform_get_irq(pdev, 1);
>  	if (!iores || !dmares || irq_err < 0 || irq_dma < 0)
> @@ -740,7 +806,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
>  	mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
>  		    MMC_CAP_SDIO_IRQ | MMC_CAP_NEEDS_POLL;
>  
> -	pdata =	mmc_dev(host->mmc)->platform_data;
> +	mxs_mmc_get_of_property(pdev, &pdata);
> +	if (pdata == NULL)
> +		pdata =	mmc_dev(host->mmc)->platform_data;
>  	if (pdata) {
>  		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
>  			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> @@ -851,6 +919,13 @@ static const struct dev_pm_ops mxs_mmc_pm_ops = {
>  };
>  #endif
>  
> +static const struct of_device_id mxs_mmc_dt_ids[] = {
> +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids);
> +
>  static struct platform_driver mxs_mmc_driver = {
>  	.probe		= mxs_mmc_probe,
>  	.remove		= mxs_mmc_remove,
> @@ -860,6 +935,7 @@ static struct platform_driver mxs_mmc_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &mxs_mmc_pm_ops,
>  #endif
> +		.of_match_table = mxs_mmc_dt_ids,
>  	},
>  };
>  
> -- 
> 1.7.0.4
> 
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 01:42:38AM +0800, Grant Likely wrote:
> On Tue, 13 Mar 2012 16:47:05 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > ---
> > The patch is still using a private way for dma part binding
> > since the common dma binding is still under discussion.
> > http://www.spinics.net/lists/linux-omap/msg65528.html
> > 
> > Will update to use common dma binding when it hits mainline.
> > ---
> >  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
> >  drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
> >  2 files changed, 102 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > new file mode 100644
> > index 0000000..adc1142
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > @@ -0,0 +1,23 @@
> > +* FREESCALE MXS MMC peripheral
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<chip>-mmc"
> > +- reg : Should contain registers location and length
> > +- interrupts : Should contain interrupt.
> > +  The format is <irq_err irq_dma>.
> > +- dma_channel: Should contain the dma channel it uses
> 
> Don't use '_' in property names.
> 
Yes, my mistake.

> The is a generic dma binding being drafted that uses a phandle to the dma
> controller and the ability to encode channel numbers.  You may want to take
> a look at it.
> 
I will look at it.
Thanks for the reminder.

...
> > +	dmares->flags = IORESOURCE_DMA;
> > +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> > +		return NULL;
> > +	}
> > +	dmares->end = dmares->start;
> > +
> > +	return dmares;
> > +}
> > +
> > +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> > +				struct mxs_mmc_platform_data **ppdata)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct mxs_mmc_platform_data *pdata = *ppdata;
> > +
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> 
> Ditto
> 
Got it.

> Fix up those comments and you can add my:
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
Thanks.

Regards
Dong Aisheng

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Marek Vasut]

Dear Dong Aisheng,

> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> ---
> The patch is still using a private way for dma part binding
> since the common dma binding is still under discussion.
> http://www.spinics.net/lists/linux-omap/msg65528.html
> 
> Will update to use common dma binding when it hits mainline.
> ---
>  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
>  drivers/mmc/host/mxs-mmc.c                         |   82
> +++++++++++++++++++- 2 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt new file mode
> 100644
> index 0000000..adc1142
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> @@ -0,0 +1,23 @@
> +* FREESCALE MXS MMC peripheral
> +
> +Required properties:
> +- compatible : Should be "fsl,<chip>-mmc"
> +- reg : Should contain registers location and length
> +- interrupts : Should contain interrupt.
> +  The format is <irq_err irq_dma>.
> +- dma_channel: Should contain the dma channel it uses
> +
> +Optional properties:
> +- wp-gpios : Specify GPIOs for write protection
> +- slot-4bit: Specify 4 bit mode support
> +- slot-8bit: Specify 8 bit and 4 bit mode support
> +
> +Examples:
> +mmc1: ssp@80010000 {
> +	compatible = "fsl,imx28-mmc";
> +	reg = <0x80010000 2000>;
> +	/* <irq_err irq_dma> */
> +	interrupts = <96 82>;
> +	dma_channel = <0>;
> +	slot-8bit;
> +};
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 382c835..6cf2d17 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -38,6 +38,10 @@
>  #include <linux/gpio.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
> 
>  #include <mach/mxs.h>
>  #include <mach/common.h>
> @@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan,
> void *param) return true;
>  }
> 
> +#ifdef CONFIG_OF
> +static struct resource * __devinit mxs_mmc_get_of_dmares(
> +				struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *dmares;
> +	int ret;
> +
> +	if (!np)
> +		return NULL;
> +
> +	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);
> +	dmares->flags = IORESOURCE_DMA;
> +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> +		return NULL;
> +	}
> +	dmares->end = dmares->start;
> +
> +	return dmares;
> +}
> +
> +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> +				struct mxs_mmc_platform_data **ppdata)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct mxs_mmc_platform_data *pdata = *ppdata;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +
> +	if (of_get_property(np, "slot-8bit", NULL))
> +		pdata->flags |= SLOTF_8_BIT_CAPABLE;
> +
> +	if (of_get_property(np, "slot-4bit", NULL))
> +		pdata->flags |= SLOTF_4_BIT_CAPABLE;
> +
> +	pdata->wp_gpio = of_get_named_gpio(np, "wp-gpios", 0);
> +
> +	dev_dbg(&pdev->dev, "wp-gpios %d flags %d\n", pdata->wp_gpio,
> +		pdata->flags);
> +
> +	return 0;
> +}
> +#else
> +static struct resource * __devinit mxs_mmc_get_of_dmares(
> +				struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +static inline int mxs_mmc_get_of_property(struct platform_device *pdev,
> +			struct mxs_mmc_platform_data *pdata)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int mxs_mmc_probe(struct platform_device *pdev)
>  {
>  	struct mxs_mmc_host *host;
>  	struct mmc_host *mmc;
>  	struct resource *iores, *dmares, *r;
> -	struct mxs_mmc_platform_data *pdata;
> +	struct mxs_mmc_platform_data *pdata = NULL;
>  	int ret = 0, irq_err, irq_dma;
>  	dma_cap_mask_t mask;
> 
>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
> +	dmares = mxs_mmc_get_of_dmares(pdev);
> +	if (dmares == NULL)
> +		dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>  	irq_err = platform_get_irq(pdev, 0);
>  	irq_dma = platform_get_irq(pdev, 1);
>  	if (!iores || !dmares || irq_err < 0 || irq_dma < 0)
> @@ -740,7 +806,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
>  	mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
>  		    MMC_CAP_SDIO_IRQ | MMC_CAP_NEEDS_POLL;
> 
> -	pdata =	mmc_dev(host->mmc)->platform_data;
> +	mxs_mmc_get_of_property(pdev, &pdata);
> +	if (pdata == NULL)
> +		pdata =	mmc_dev(host->mmc)->platform_data;
>  	if (pdata) {
>  		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
>  			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> @@ -851,6 +919,13 @@ static const struct dev_pm_ops mxs_mmc_pm_ops = {
>  };
>  #endif
> 
> +static const struct of_device_id mxs_mmc_dt_ids[] = {
> +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },

Do you really need two distinct ones here?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids);
> +
>  static struct platform_driver mxs_mmc_driver = {
>  	.probe		= mxs_mmc_probe,
>  	.remove		= mxs_mmc_remove,
> @@ -860,6 +935,7 @@ static struct platform_driver mxs_mmc_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &mxs_mmc_pm_ops,
>  #endif
> +		.of_match_table = mxs_mmc_dt_ids,
>  	},
>  };

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 01:58:25PM +0800, Marek Vasut wrote:
> Dear Dong Aisheng,
> 
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
........

> > +static const struct of_device_id mxs_mmc_dt_ids[] = {
> > +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> > +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> 
> Do you really need two distinct ones here?
> 
Hmm, my original purpose is to put soc difference data in .data
to remove cpu_is_* function calls in the driver later.
Do you think if any issue?

Regards
Dong Aisheng

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Marek Vasut]

Dear Dong Aisheng,

> On Wed, Mar 14, 2012 at 01:58:25PM +0800, Marek Vasut wrote:
> > Dear Dong Aisheng,
> > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> ........
> 
> > > +static const struct of_device_id mxs_mmc_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> > > +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> > 
> > Do you really need two distinct ones here?
> 
> Hmm, my original purpose is to put soc difference data in .data
> to remove cpu_is_* function calls in the driver later.
> Do you think if any issue?
> 

Well, what's the difference between the interfaces on mx233 and mx28? Is it 
something that can't be encoded otherwise? I think they're not so different.

Best regards,
Marek Vasut

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[s.hauer]

On Wed, Mar 14, 2012 at 08:09:22AM +0100, Marek Vasut wrote:
> Dear Dong Aisheng,
> 
> > On Wed, Mar 14, 2012 at 01:58:25PM +0800, Marek Vasut wrote:
> > > Dear Dong Aisheng,
> > > 
> > > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > ........
> > 
> > > > +static const struct of_device_id mxs_mmc_dt_ids[] = {
> > > > +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> > > > +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> > > 
> > > Do you really need two distinct ones here?
> > 
> > Hmm, my original purpose is to put soc difference data in .data
> > to remove cpu_is_* function calls in the driver later.
> > Do you think if any issue?
> > 
> 
> Well, what's the difference between the interfaces on mx233 and mx28? Is it 
> something that can't be encoded otherwise? I think they're not so different.

They differ in the register layout. Matching two different compatible
strings is the right way here I think.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 08:09:22AM +0100, Marek Vasut wrote:
> Dear Dong Aisheng,
> 
> > On Wed, Mar 14, 2012 at 01:58:25PM +0800, Marek Vasut wrote:
> > > Dear Dong Aisheng,
> > > 
> > > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > ........
> > 
> > > > +static const struct of_device_id mxs_mmc_dt_ids[] = {
> > > > +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> > > > +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> > > 
> > > Do you really need two distinct ones here?
> > 
> > Hmm, my original purpose is to put soc difference data in .data
> > to remove cpu_is_* function calls in the driver later.
> > Do you think if any issue?
> > 
> 
> Well, what's the difference between the interfaces on mx233 and mx28? Is it 
> something that can't be encoded otherwise? I think they're not so different.
> 
Not much difference except the one register offset and ip version.
See:
#define SSP_VERSION_LATEST      4
#define ssp_is_old()            (host->version < SSP_VERSION_LATEST)
..
#define HW_SSP_VERSION (cpu_is_mx23() ? 0x110 : 0x130)
The ip version can be handled in driver, but for offset...
it depends on cpu_is_* macro.
Putting the HW_SSP_VERSION offset difference in .data can eliminate the need
of cpu_is_*.

Despite of that, since they're two devices,
i guess it's ok to put two compatible string there, right?
Or you thought just put one as below?
{ .compatible = "fsl,mxs-mmc", .data = NULL, },

Regards
Dong Aisheng

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Marek Vasut]

Dear Dong Aisheng,

> On Wed, Mar 14, 2012 at 08:09:22AM +0100, Marek Vasut wrote:
> > Dear Dong Aisheng,
> > 
> > > On Wed, Mar 14, 2012 at 01:58:25PM +0800, Marek Vasut wrote:
> > > > Dear Dong Aisheng,
> > > > 
> > > > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > ........
> > > 
> > > > > +static const struct of_device_id mxs_mmc_dt_ids[] = {
> > > > > +	{ .compatible = "fsl,imx23-mmc", .data = NULL, },
> > > > > +	{ .compatible = "fsl,imx28-mmc", .data = NULL, },
> > > > 
> > > > Do you really need two distinct ones here?
> > > 
> > > Hmm, my original purpose is to put soc difference data in .data
> > > to remove cpu_is_* function calls in the driver later.
> > > Do you think if any issue?
> > 
> > Well, what's the difference between the interfaces on mx233 and mx28? Is
> > it something that can't be encoded otherwise? I think they're not so
> > different.
> 
> Not much difference except the one register offset and ip version.
> See:
> #define SSP_VERSION_LATEST      4
> #define ssp_is_old()            (host->version < SSP_VERSION_LATEST)
> ..
> #define HW_SSP_VERSION (cpu_is_mx23() ? 0x110 : 0x130)
> The ip version can be handled in driver, but for offset...
> it depends on cpu_is_* macro.
> Putting the HW_SSP_VERSION offset difference in .data can eliminate the
> need of cpu_is_*.
> 
> Despite of that, since they're two devices,
> i guess it's ok to put two compatible string there, right?
> Or you thought just put one as below?
> { .compatible = "fsl,mxs-mmc", .data = NULL, },
> 
No, I understand now /wrt the register layout.

Best regards,
Marek Vasut

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:47 Tue 13 Mar     , Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> ---
> The patch is still using a private way for dma part binding
> since the common dma binding is still under discussion.
> http://www.spinics.net/lists/linux-omap/msg65528.html
> 
> Will update to use common dma binding when it hits mainline.
> ---
>  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
>  drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
>  2 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> new file mode 100644
> index 0000000..adc1142
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> @@ -0,0 +1,23 @@
> +* FREESCALE MXS MMC peripheral
> +
> +Required properties:
> +- compatible : Should be "fsl,<chip>-mmc"
> +- reg : Should contain registers location and length
> +- interrupts : Should contain interrupt.
> +  The format is <irq_err irq_dma>.
> +- dma_channel: Should contain the dma channel it uses
> +
> +Optional properties:
> +- wp-gpios : Specify GPIOs for write protection
> +- slot-4bit: Specify 4 bit mode support
> +- slot-8bit: Specify 8 bit and 4 bit mode support
> +
> +Examples:
> +mmc1: ssp@80010000 {
> +	compatible = "fsl,imx28-mmc";
> +	reg = <0x80010000 2000>;
> +	/* <irq_err irq_dma> */
> +	interrupts = <96 82>;
> +	dma_channel = <0>;
> +	slot-8bit;
> +};
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 382c835..6cf2d17 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -38,6 +38,10 @@
>  #include <linux/gpio.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
>  
>  #include <mach/mxs.h>
>  #include <mach/common.h>
> @@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan, void *param)
>  	return true;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct resource * __devinit mxs_mmc_get_of_dmares(
> +				struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *dmares;
> +	int ret;
> +
> +	if (!np)
> +		return NULL;
> +
> +	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);
> +	dmares->flags = IORESOURCE_DMA;
> +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> +		return NULL;
> +	}
> +	dmares->end = dmares->start;
> +
> +	return dmares;
> +}
> +
> +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> +				struct mxs_mmc_platform_data **ppdata)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct mxs_mmc_platform_data *pdata = *ppdata;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +
> +	if (of_get_property(np, "slot-8bit", NULL))
> +		pdata->flags |= SLOTF_8_BIT_CAPABLE;
> +
> +	if (of_get_property(np, "slot-4bit", NULL))
> +		pdata->flags |= SLOTF_4_BIT_CAPABLE;
it will conflit if both binding are set use a number instead

Best Regards,
J.

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 03:23:43PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:47 Tue 13 Mar     , Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > ---
> > The patch is still using a private way for dma part binding
> > since the common dma binding is still under discussion.
> > http://www.spinics.net/lists/linux-omap/msg65528.html
> > 
> > Will update to use common dma binding when it hits mainline.
> > ---
> >  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
> >  drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
> >  2 files changed, 102 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > new file mode 100644
> > index 0000000..adc1142
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > @@ -0,0 +1,23 @@
> > +* FREESCALE MXS MMC peripheral
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<chip>-mmc"
> > +- reg : Should contain registers location and length
> > +- interrupts : Should contain interrupt.
> > +  The format is <irq_err irq_dma>.
> > +- dma_channel: Should contain the dma channel it uses
> > +
> > +Optional properties:
> > +- wp-gpios : Specify GPIOs for write protection
> > +- slot-4bit: Specify 4 bit mode support
> > +- slot-8bit: Specify 8 bit and 4 bit mode support
> > +
> > +Examples:
> > +mmc1: ssp@80010000 {
> > +	compatible = "fsl,imx28-mmc";
> > +	reg = <0x80010000 2000>;
> > +	/* <irq_err irq_dma> */
> > +	interrupts = <96 82>;
> > +	dma_channel = <0>;
> > +	slot-8bit;
> > +};
> > diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> > index 382c835..6cf2d17 100644
> > --- a/drivers/mmc/host/mxs-mmc.c
> > +++ b/drivers/mmc/host/mxs-mmc.c
> > @@ -38,6 +38,10 @@
> >  #include <linux/gpio.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/slab.h>
> >  
> >  #include <mach/mxs.h>
> >  #include <mach/common.h>
> > @@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan, void *param)
> >  	return true;
> >  }
> >  
> > +#ifdef CONFIG_OF
> > +static struct resource * __devinit mxs_mmc_get_of_dmares(
> > +				struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct resource *dmares;
> > +	int ret;
> > +
> > +	if (!np)
> > +		return NULL;
> > +
> > +	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);
> > +	dmares->flags = IORESOURCE_DMA;
> > +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> > +		return NULL;
> > +	}
> > +	dmares->end = dmares->start;
> > +
> > +	return dmares;
> > +}
> > +
> > +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> > +				struct mxs_mmc_platform_data **ppdata)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct mxs_mmc_platform_data *pdata = *ppdata;
> > +
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +
> > +	if (of_get_property(np, "slot-8bit", NULL))
> > +		pdata->flags |= SLOTF_8_BIT_CAPABLE;
> > +
> > +	if (of_get_property(np, "slot-4bit", NULL))
> > +		pdata->flags |= SLOTF_4_BIT_CAPABLE;
> it will conflit if both binding are set use a number instead
> 
Hmm, i did not see conflict, can you explain more?
The "slot-8bit" includes the support for 4bit.(see binding doc)
Even user define them two property in dt by mistake, it does not cause conflict.
See:
if (pdata) {
	if (pdata->flags & SLOTF_8_BIT_CAPABLE)
		mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
	if (pdata->flags & SLOTF_4_BIT_CAPABLE)
		mmc->caps |= MMC_CAP_4_BIT_DATA;
}

Regards
Dong Aisheng

Re: [PATCH v1 2/5] mmc: mxs-mmc: add dt probe support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:09 Wed 14 Mar     , Dong Aisheng wrote:
> On Wed, Mar 14, 2012 at 03:23:43PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 16:47 Tue 13 Mar     , Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > ---
> > > The patch is still using a private way for dma part binding
> > > since the common dma binding is still under discussion.
> > > http://www.spinics.net/lists/linux-omap/msg65528.html
> > > 
> > > Will update to use common dma binding when it hits mainline.
> > > ---
> > >  .../devicetree/bindings/mmc/fsl-mxs-mmc.txt        |   23 ++++++
> > >  drivers/mmc/host/mxs-mmc.c                         |   82 +++++++++++++++++++-
> > >  2 files changed, 102 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > > new file mode 100644
> > > index 0000000..adc1142
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mmc/fsl-mxs-mmc.txt
> > > @@ -0,0 +1,23 @@
> > > +* FREESCALE MXS MMC peripheral
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<chip>-mmc"
> > > +- reg : Should contain registers location and length
> > > +- interrupts : Should contain interrupt.
> > > +  The format is <irq_err irq_dma>.
> > > +- dma_channel: Should contain the dma channel it uses
> > > +
> > > +Optional properties:
> > > +- wp-gpios : Specify GPIOs for write protection
> > > +- slot-4bit: Specify 4 bit mode support
> > > +- slot-8bit: Specify 8 bit and 4 bit mode support
> > > +
> > > +Examples:
> > > +mmc1: ssp@80010000 {
> > > +	compatible = "fsl,imx28-mmc";
> > > +	reg = <0x80010000 2000>;
> > > +	/* <irq_err irq_dma> */
> > > +	interrupts = <96 82>;
> > > +	dma_channel = <0>;
> > > +	slot-8bit;
> > > +};
> > > diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> > > index 382c835..6cf2d17 100644
> > > --- a/drivers/mmc/host/mxs-mmc.c
> > > +++ b/drivers/mmc/host/mxs-mmc.c
> > > @@ -38,6 +38,10 @@
> > >  #include <linux/gpio.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_gpio.h>
> > > +#include <linux/slab.h>
> > >  
> > >  #include <mach/mxs.h>
> > >  #include <mach/common.h>
> > > @@ -673,17 +677,79 @@ static bool mxs_mmc_dma_filter(struct dma_chan *chan, void *param)
> > >  	return true;
> > >  }
> > >  
> > > +#ifdef CONFIG_OF
> > > +static struct resource * __devinit mxs_mmc_get_of_dmares(
> > > +				struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct resource *dmares;
> > > +	int ret;
> > > +
> > > +	if (!np)
> > > +		return NULL;
> > > +
> > > +	dmares = kzalloc(sizeof(*dmares), GFP_KERNEL);
> > > +	dmares->flags = IORESOURCE_DMA;
> > > +	ret = of_property_read_u32(np, "dma_channel", &dmares->start);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "unable to get dmares from dt\n");
> > > +		return NULL;
> > > +	}
> > > +	dmares->end = dmares->start;
> > > +
> > > +	return dmares;
> > > +}
> > > +
> > > +static int __devinit mxs_mmc_get_of_property(struct platform_device *pdev,
> > > +				struct mxs_mmc_platform_data **ppdata)
> > > +{
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct mxs_mmc_platform_data *pdata = *ppdata;
> > > +
> > > +	if (!np)
> > > +		return -ENODEV;
> > > +
> > > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +
> > > +	if (of_get_property(np, "slot-8bit", NULL))
> > > +		pdata->flags |= SLOTF_8_BIT_CAPABLE;
> > > +
> > > +	if (of_get_property(np, "slot-4bit", NULL))
> > > +		pdata->flags |= SLOTF_4_BIT_CAPABLE;
> > it will conflit if both binding are set use a number instead
> > 
> Hmm, i did not see conflict, can you explain more?
> The "slot-8bit" includes the support for 4bit.(see binding doc)
> Even user define them two property in dt by mistake, it does not cause conflict.
> See:
> if (pdata) {
> 	if (pdata->flags & SLOTF_8_BIT_CAPABLE)
> 		mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> 	if (pdata->flags & SLOTF_4_BIT_CAPABLE)
> 		mmc->caps |= MMC_CAP_4_BIT_DATA;
> }

this is mmc specifc sohould have a generic binding

Best Regards,
J.

[PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
 arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-mxs/imx28-dt.c    |    2 ++
 3 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index 9758dc4..61350ab 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -21,6 +21,20 @@
 		reg = <0x40000000 0x08000000>;
 	};
 
+        apb@80000000 {
+                apbh@80000000 {
+			mmc1: ssp@80010000 {
+				slot-8bit;
+				status = "okay";
+			};
+
+			mmc2: ssp@80012000 {
+				slot-8bit;
+				status = "okay";
+			};
+		};
+	};
+
 	ahb@80080000 {
 		fec@800f0000 {
 			phy-mode = "rmii";
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index acf0dab..71c7bfb 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -46,6 +46,39 @@
 				#interrupt-cells = <1>;
 				reg = <0x80000000 0x2000>;
 			};
+
+			ssp@80010000 {
+				compatible = "fsl,imx28-mmc";
+				reg = <0x80010000 2000>;
+				/* <irq_err irq_dma> */
+				interrupts = <96 82>;
+				dma_channel = <0>;
+				status = "disabled";
+			};
+
+			ssp@80012000 {
+				compatible = "fsl,imx28-mmc";
+				reg = <0x80012000 2000>;
+				interrupts = <97 83>;
+				dma_channel = <1>;
+				status = "disabled";
+			};
+
+			ssp@80014000 {
+				compatible = "fsl,imx28-mmc";
+				reg = <0x80014000 2000>;
+				interrupts = <98 84>;
+				dma_channel = <2>;
+				status = "disabled";
+			};
+
+			ssp@80016000 {
+				compatible = "fsl,imx28-mmc";
+				reg = <0x80016000 2000>;
+				interrupts = <99 85>;
+				dma_channel = <3>;
+				status = "disabled";
+			};
 		};
 
 		apbx@80040000 {
diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
index 78d1129..429b88e 100644
--- a/arch/arm/mach-mxs/imx28-dt.c
+++ b/arch/arm/mach-mxs/imx28-dt.c
@@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
 	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
 	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
 	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
 	{ /* sentinel */ }
 };
 
-- 
1.7.0.4

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Rob Herring]

On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
>  3 files changed, 49 insertions(+), 0 deletions(-)

snip

> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> index 78d1129..429b88e 100644
> --- a/arch/arm/mach-mxs/imx28-dt.c
> +++ b/arch/arm/mach-mxs/imx28-dt.c
> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),

Why is this needed?

>  	{ /* sentinel */ }
>  };
>  

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Sascha Hauer]

On Tue, Mar 13, 2012 at 09:39:30AM -0500, Rob Herring wrote:
> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
> >  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
> >  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
> >  3 files changed, 49 insertions(+), 0 deletions(-)
> 
> snip
> 
> > diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> > index 78d1129..429b88e 100644
> > --- a/arch/arm/mach-mxs/imx28-dt.c
> > +++ b/arch/arm/mach-mxs/imx28-dt.c
> > @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> >  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> >  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> >  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
> 
> Why is this needed?

These are needed for the drivers which have still the mxs-mmc.* names
to find their clocks. Alternatively we could also add the appropriate
clocks to the clock file. Don't know if that's better though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Rob Herring]

On 03/13/2012 11:52 AM, Sascha Hauer wrote:
> On Tue, Mar 13, 2012 at 09:39:30AM -0500, Rob Herring wrote:
>> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
>>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>>
>>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>>> ---
>>>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
>>>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
>>>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
>>>  3 files changed, 49 insertions(+), 0 deletions(-)
>>
>> snip
>>
>>> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
>>> index 78d1129..429b88e 100644
>>> --- a/arch/arm/mach-mxs/imx28-dt.c
>>> +++ b/arch/arm/mach-mxs/imx28-dt.c
>>> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
>>>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
>>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
>>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
>>
>> Why is this needed?
> 
> These are needed for the drivers which have still the mxs-mmc.* names
> to find their clocks. Alternatively we could also add the appropriate
> clocks to the clock file. Don't know if that's better though.

Ah, yes I should have known that... If clk lookup is all that's needed,
I'd suggest adding the necessary clk lookups either here or in the clock
code. Not much difference, but at least it removes the use of
*_BASE_ADDR defines.

Rob

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:45 Tue 13 Mar     , Rob Herring wrote:
> On 03/13/2012 11:52 AM, Sascha Hauer wrote:
> > On Tue, Mar 13, 2012 at 09:39:30AM -0500, Rob Herring wrote:
> >> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> >>>
> >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> >>> ---
> >>>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
> >>>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
> >>>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
> >>>  3 files changed, 49 insertions(+), 0 deletions(-)
> >>
> >> snip
> >>
> >>> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> >>> index 78d1129..429b88e 100644
> >>> --- a/arch/arm/mach-mxs/imx28-dt.c
> >>> +++ b/arch/arm/mach-mxs/imx28-dt.c
> >>> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> >>>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
> >>
> >> Why is this needed?
> > 
> > These are needed for the drivers which have still the mxs-mmc.* names
> > to find their clocks. Alternatively we could also add the appropriate
> > clocks to the clock file. Don't know if that's better though.
> 
> Ah, yes I should have known that... If clk lookup is all that's needed,
> I'd suggest adding the necessary clk lookups either here or in the clock
> code. Not much difference, but at least it removes the use of
> *_BASE_ADDR defines.
Agreed we do this on AT91

Best Regards,
J.

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 03:30:35PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:45 Tue 13 Mar     , Rob Herring wrote:
> > On 03/13/2012 11:52 AM, Sascha Hauer wrote:
> > > On Tue, Mar 13, 2012 at 09:39:30AM -0500, Rob Herring wrote:
> > >> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> > >>>
> > >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > >>> ---
> > >>>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
> > >>>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
> > >>>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
> > >>>  3 files changed, 49 insertions(+), 0 deletions(-)
> > >>
> > >> snip
> > >>
> > >>> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> > >>> index 78d1129..429b88e 100644
> > >>> --- a/arch/arm/mach-mxs/imx28-dt.c
> > >>> +++ b/arch/arm/mach-mxs/imx28-dt.c
> > >>> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> > >>>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> > >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> > >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> > >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> > >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
> > >>
> > >> Why is this needed?
> > > 
> > > These are needed for the drivers which have still the mxs-mmc.* names
> > > to find their clocks. Alternatively we could also add the appropriate
> > > clocks to the clock file. Don't know if that's better though.
> > 
> > Ah, yes I should have known that... If clk lookup is all that's needed,
> > I'd suggest adding the necessary clk lookups either here or in the clock
> > code. Not much difference, but at least it removes the use of
> > *_BASE_ADDR defines.
> Agreed we do this on AT91
> 
I'm willing to do like that.
But i'm new to this, can you help point our where the code i can reference
in Rob suggested way?

Regards
Dong Aisheng

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:20 Wed 14 Mar     , Dong Aisheng wrote:
> On Wed, Mar 14, 2012 at 03:30:35PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:45 Tue 13 Mar     , Rob Herring wrote:
> > > On 03/13/2012 11:52 AM, Sascha Hauer wrote:
> > > > On Tue, Mar 13, 2012 at 09:39:30AM -0500, Rob Herring wrote:
> > > >> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > > >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> > > >>>
> > > >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > >>> ---
> > > >>>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
> > > >>>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
> > > >>>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
> > > >>>  3 files changed, 49 insertions(+), 0 deletions(-)
> > > >>
> > > >> snip
> > > >>
> > > >>> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> > > >>> index 78d1129..429b88e 100644
> > > >>> --- a/arch/arm/mach-mxs/imx28-dt.c
> > > >>> +++ b/arch/arm/mach-mxs/imx28-dt.c
> > > >>> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> > > >>>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> > > >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> > > >>>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> > > >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> > > >>> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
> > > >>
> > > >> Why is this needed?
> > > > 
> > > > These are needed for the drivers which have still the mxs-mmc.* names
> > > > to find their clocks. Alternatively we could also add the appropriate
> > > > clocks to the clock file. Don't know if that's better though.
> > > 
> > > Ah, yes I should have known that... If clk lookup is all that's needed,
> > > I'd suggest adding the necessary clk lookups either here or in the clock
> > > code. Not much difference, but at least it removes the use of
> > > *_BASE_ADDR defines.
> > Agreed we do this on AT91
> > 
> I'm willing to do like that.
> But i'm new to this, can you help point our where the code i can reference
> in Rob suggested way?
mach-at91

Best Regards,
J.

Re: [PATCH v1 3/5] ARM: imx28evk: add mmc dt support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:47 Tue 13 Mar     , Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  arch/arm/boot/dts/imx28-evk.dts |   14 ++++++++++++++
>  arch/arm/boot/dts/imx28.dtsi    |   33 +++++++++++++++++++++++++++++++++
>  arch/arm/mach-mxs/imx28-dt.c    |    2 ++
>  3 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> index 9758dc4..61350ab 100644
> --- a/arch/arm/boot/dts/imx28-evk.dts
> +++ b/arch/arm/boot/dts/imx28-evk.dts
> @@ -21,6 +21,20 @@
>  		reg = <0x40000000 0x08000000>;
>  	};
>  
> +        apb@80000000 {
> +                apbh@80000000 {
> +			mmc1: ssp@80010000 {
> +				slot-8bit;
> +				status = "okay";
> +			};
> +
> +			mmc2: ssp@80012000 {
> +				slot-8bit;
> +				status = "okay";
> +			};
> +		};
> +	};
> +
>  	ahb@80080000 {
>  		fec@800f0000 {
>  			phy-mode = "rmii";
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> index acf0dab..71c7bfb 100644
> --- a/arch/arm/boot/dts/imx28.dtsi
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -46,6 +46,39 @@
>  				#interrupt-cells = <1>;
>  				reg = <0x80000000 0x2000>;
>  			};
> +
> +			ssp@80010000 {
> +				compatible = "fsl,imx28-mmc";
> +				reg = <0x80010000 2000>;
> +				/* <irq_err irq_dma> */
> +				interrupts = <96 82>;
> +				dma_channel = <0>;
> +				status = "disabled";
> +			};
> +
> +			ssp@80012000 {
> +				compatible = "fsl,imx28-mmc";
> +				reg = <0x80012000 2000>;
> +				interrupts = <97 83>;
> +				dma_channel = <1>;
> +				status = "disabled";
> +			};
> +
> +			ssp@80014000 {
> +				compatible = "fsl,imx28-mmc";
> +				reg = <0x80014000 2000>;
> +				interrupts = <98 84>;
> +				dma_channel = <2>;
> +				status = "disabled";
> +			};
> +
> +			ssp@80016000 {
> +				compatible = "fsl,imx28-mmc";
> +				reg = <0x80016000 2000>;
> +				interrupts = <99 85>;
> +				dma_channel = <3>;
> +				status = "disabled";
> +			};
>  		};
>  
>  		apbx@80040000 {
> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> index 78d1129..429b88e 100644
> --- a/arch/arm/mach-mxs/imx28-dt.c
> +++ b/arch/arm/mach-mxs/imx28-dt.c
> @@ -23,6 +23,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
>  	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
>  	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
do we really need to continue to add OF_DEV_AUXDATA

instead add a clkdev entry

IIRC OF_DEV_AUXDATA was a temporary solution, we need to stop to use it more
and more

Best Reards,
J.

[PATCH v1 4/5] dma: mxs-dma: add dt probe support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 .../devicetree/bindings/dma/fsl-mxs-dma.txt        |   17 ++++++++
 drivers/dma/mxs-dma.c                              |   44 +++++++++++++------
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
new file mode 100644
index 0000000..cfa1730
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
@@ -0,0 +1,17 @@
+* Freescale MXS DMA
+
+Required properties:
+- compatible : Should be "fsl,mxs-dma-apbh" or "fsl,mxs-dma-apbx"
+- reg : Should contain registers location and length
+
+Examples:
+
+dma-apbh@80004000 {
+	compatible = "fsl,mxs-dma-apbh";
+	reg = <0x80004000 2000>;
+};
+
+dma-apbx@80024000 {
+	compatible = "fsl,mxs-dma-apbx";
+	reg = <0x80024000 2000>;
+};
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index b06cd4c..45e8d46 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -22,6 +22,9 @@
 #include <linux/platform_device.h>
 #include <linux/dmaengine.h>
 #include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <asm/irq.h>
 #include <mach/mxs.h>
@@ -130,6 +133,25 @@ struct mxs_dma_engine {
 	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
 };
 
+static struct platform_device_id mxs_dma_type[] = {
+	{
+		.name = "mxs-dma-apbh",
+		.driver_data = MXS_DMA_APBH,
+	}, {
+		.name = "mxs-dma-apbx",
+		.driver_data = MXS_DMA_APBX,
+	}, {
+		/* end of list */
+	}
+};
+
+static const struct of_device_id mxs_dma_dt_ids[] = {
+	{ .compatible = "fsl,mxs-dma-apbh", .data = &mxs_dma_type[0], },
+	{ .compatible = "fsl,mxs-dma-apbx", .data = &mxs_dma_type[1], },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_dma_dt_ids);
+
 static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 {
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
@@ -587,8 +609,8 @@ err_out:
 
 static int __init mxs_dma_probe(struct platform_device *pdev)
 {
-	const struct platform_device_id *id_entry =
-				platform_get_device_id(pdev);
+	const struct platform_device_id *id_entry;
+	const struct of_device_id *of_id;
 	struct mxs_dma_engine *mxs_dma;
 	struct resource *iores;
 	int ret, i;
@@ -597,6 +619,11 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
 	if (!mxs_dma)
 		return -ENOMEM;
 
+	of_id = of_match_device(mxs_dma_dt_ids, &pdev->dev);
+	if (of_id)
+		id_entry = of_id->data;
+	else
+		id_entry = platform_get_device_id(pdev);
 	mxs_dma->dev_id = id_entry->driver_data;
 
 	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -679,21 +706,10 @@ err_request_region:
 	return ret;
 }
 
-static struct platform_device_id mxs_dma_type[] = {
-	{
-		.name = "mxs-dma-apbh",
-		.driver_data = MXS_DMA_APBH,
-	}, {
-		.name = "mxs-dma-apbx",
-		.driver_data = MXS_DMA_APBX,
-	}, {
-		/* end of list */
-	}
-};
-
 static struct platform_driver mxs_dma_driver = {
 	.driver		= {
 		.name	= "mxs-dma",
+		.of_match_table = mxs_dma_dt_ids,
 	},
 	.id_table	= mxs_dma_type,
 };
-- 
1.7.0.4

Re: [PATCH v1 4/5] dma: mxs-dma: add dt probe support

[Huang Shijie]

Hi Aisheng:
> From: Dong Aisheng<dong.aisheng@linaro.org>
>
> Signed-off-by: Dong Aisheng<dong.aisheng@linaro.org>
> ---
>   .../devicetree/bindings/dma/fsl-mxs-dma.txt        |   17 ++++++++
>   drivers/dma/mxs-dma.c                              |   44 +++++++++++++------
>   2 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
> new file mode 100644
> index 0000000..cfa1730
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
> @@ -0,0 +1,17 @@
> +* Freescale MXS DMA
> +
> +Required properties:
> +- compatible : Should be "fsl,mxs-dma-apbh" or "fsl,mxs-dma-apbx"
> +- reg : Should contain registers location and length
> +
> +Examples:
> +
> +dma-apbh@80004000 {
> +	compatible = "fsl,mxs-dma-apbh";
> +	reg =<0x80004000 2000>;
> +};
> +
> +dma-apbx@80024000 {
> +	compatible = "fsl,mxs-dma-apbx";
> +	reg =<0x80024000 2000>;
> +};
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index b06cd4c..45e8d46 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -22,6 +22,9 @@
>   #include<linux/platform_device.h>
>   #include<linux/dmaengine.h>
>   #include<linux/delay.h>
> +#include<linux/module.h>
> +#include<linux/of.h>
> +#include<linux/of_device.h>
>
>   #include<asm/irq.h>
>   #include<mach/mxs.h>
> @@ -130,6 +133,25 @@ struct mxs_dma_engine {
>   	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
>   };
>
> +static struct platform_device_id mxs_dma_type[] = {
> +	{
> +		.name = "mxs-dma-apbh",
> +		.driver_data = MXS_DMA_APBH,
> +	}, {
> +		.name = "mxs-dma-apbx",
> +		.driver_data = MXS_DMA_APBX,
> +	}, {
> +		/* end of list */
> +	}
> +};
> +
I think you should use the platform_device_id to distinguish different 
archs.
In the mx6q,  you will meet some compiler error for the macro cpu_is_mx23().

BR
Huang Shijie



> +static const struct of_device_id mxs_dma_dt_ids[] = {
> +	{ .compatible = "fsl,mxs-dma-apbh", .data =&mxs_dma_type[0], },
> +	{ .compatible = "fsl,mxs-dma-apbx", .data =&mxs_dma_type[1], },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_dma_dt_ids);
> +
>   static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
>   {
>   	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> @@ -587,8 +609,8 @@ err_out:
>
>   static int __init mxs_dma_probe(struct platform_device *pdev)
>   {
> -	const struct platform_device_id *id_entry =
> -				platform_get_device_id(pdev);
> +	const struct platform_device_id *id_entry;
> +	const struct of_device_id *of_id;
>   	struct mxs_dma_engine *mxs_dma;
>   	struct resource *iores;
>   	int ret, i;
> @@ -597,6 +619,11 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
>   	if (!mxs_dma)
>   		return -ENOMEM;
>
> +	of_id = of_match_device(mxs_dma_dt_ids,&pdev->dev);
> +	if (of_id)
> +		id_entry = of_id->data;
> +	else
> +		id_entry = platform_get_device_id(pdev);
>   	mxs_dma->dev_id = id_entry->driver_data;
>
>   	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -679,21 +706,10 @@ err_request_region:
>   	return ret;
>   }
>
> -static struct platform_device_id mxs_dma_type[] = {
> -	{
> -		.name = "mxs-dma-apbh",
> -		.driver_data = MXS_DMA_APBH,
> -	}, {
> -		.name = "mxs-dma-apbx",
> -		.driver_data = MXS_DMA_APBX,
> -	}, {
> -		/* end of list */
> -	}
> -};
> -
>   static struct platform_driver mxs_dma_driver = {
>   	.driver		= {
>   		.name	= "mxs-dma",
> +		.of_match_table = mxs_dma_dt_ids,
>   	},
>   	.id_table	= mxs_dma_type,
>   };

Re: [PATCH v1 4/5] dma: mxs-dma: add dt probe support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 03:54:37PM +0800, Huang Shijie-B32955 wrote:
> Hi Aisheng:
> > From: Dong Aisheng<dong.aisheng@linaro.org>
> >
> > Signed-off-by: Dong Aisheng<dong.aisheng@linaro.org>
> > ---
> >   .../devicetree/bindings/dma/fsl-mxs-dma.txt        |   17 ++++++++
> >   drivers/dma/mxs-dma.c                              |   44 +++++++++++++------
> >   2 files changed, 47 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
> > new file mode 100644
> > index 0000000..cfa1730
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/fsl-mxs-dma.txt
> > @@ -0,0 +1,17 @@
> > +* Freescale MXS DMA
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,mxs-dma-apbh" or "fsl,mxs-dma-apbx"
> > +- reg : Should contain registers location and length
> > +
> > +Examples:
> > +
> > +dma-apbh@80004000 {
> > +	compatible = "fsl,mxs-dma-apbh";
> > +	reg =<0x80004000 2000>;
> > +};
> > +
> > +dma-apbx@80024000 {
> > +	compatible = "fsl,mxs-dma-apbx";
> > +	reg =<0x80024000 2000>;
> > +};
> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > index b06cd4c..45e8d46 100644
> > --- a/drivers/dma/mxs-dma.c
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -22,6 +22,9 @@
> >   #include<linux/platform_device.h>
> >   #include<linux/dmaengine.h>
> >   #include<linux/delay.h>
> > +#include<linux/module.h>
> > +#include<linux/of.h>
> > +#include<linux/of_device.h>
> >
> >   #include<asm/irq.h>
> >   #include<mach/mxs.h>
> > @@ -130,6 +133,25 @@ struct mxs_dma_engine {
> >   	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
> >   };
> >
> > +static struct platform_device_id mxs_dma_type[] = {
> > +	{
> > +		.name = "mxs-dma-apbh",
> > +		.driver_data = MXS_DMA_APBH,
> > +	}, {
> > +		.name = "mxs-dma-apbx",
> > +		.driver_data = MXS_DMA_APBX,
> > +	}, {
> > +		/* end of list */
> > +	}
> > +};
> > +
> I think you should use the platform_device_id to distinguish different 
> archs.
> In the mx6q,  you will meet some compiler error for the macro cpu_is_mx23().
> 
Yes, i will remove cpu_is_*() in mxs-mmc driver by using device id.

Regards
Dong Aisheng

[PATCH v1 5/5] ARM: mxs: add mxs dma dt support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Originally the dma device will be added by default via
arch_initcall(mxs_add_mxs_dma);

Now change to call it explicitly in board file to avoid conflict
with device tree support.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 arch/arm/boot/dts/imx28.dtsi                    |   12 +++++++++++-
 arch/arm/mach-mxs/devices-mx23.h                |    2 ++
 arch/arm/mach-mxs/devices-mx28.h                |    2 ++
 arch/arm/mach-mxs/devices/platform-dma.c        |    3 +--
 arch/arm/mach-mxs/imx28-dt.c                    |    2 ++
 arch/arm/mach-mxs/include/mach/devices-common.h |    3 +++
 arch/arm/mach-mxs/mach-apx4devkit.c             |    1 +
 arch/arm/mach-mxs/mach-m28evk.c                 |    1 +
 arch/arm/mach-mxs/mach-mx23evk.c                |    1 +
 arch/arm/mach-mxs/mach-mx28evk.c                |    1 +
 arch/arm/mach-mxs/mach-stmp378x_devb.c          |    1 +
 arch/arm/mach-mxs/mach-tx28.c                   |    1 +
 12 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 71c7bfb..f0322e9 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -47,6 +47,11 @@
 				reg = <0x80000000 0x2000>;
 			};
 
+			dma-apbh@80004000 {
+				compatible = "fsl,mxs-dma-apbh";
+				reg = <0x80004000 2000>;
+			};
+
 			ssp@80010000 {
 				compatible = "fsl,imx28-mmc";
 				reg = <0x80010000 2000>;
@@ -79,7 +84,12 @@
 				dma_channel = <3>;
 				status = "disabled";
 			};
-		};
+
+			dma-apbx@80024000 {
+				compatible = "fsl,mxs-dma-apbx";
+				reg = <0x80024000 2000>;
+			};
+                };
 
 		apbx@80040000 {
 			compatible = "simple-bus";
diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h
index 3fa651d..721c91a 100644
--- a/arch/arm/mach-mxs/devices-mx23.h
+++ b/arch/arm/mach-mxs/devices-mx23.h
@@ -31,3 +31,5 @@ struct platform_device *__init mx23_add_mxsfb(
 		const struct mxsfb_platform_data *pdata);
 
 struct platform_device *__init mx23_add_rtc_stmp3xxx(void);
+
+#define mx23_add_dma()	mxs_add_mxs_dma()
diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h
index 4f50094..c0cfd4e 100644
--- a/arch/arm/mach-mxs/devices-mx28.h
+++ b/arch/arm/mach-mxs/devices-mx28.h
@@ -51,3 +51,5 @@ extern const struct mxs_saif_data mx28_saif_data[] __initconst;
 	mxs_add_saif(&mx28_saif_data[id], pdata)
 
 struct platform_device *__init mx28_add_rtc_stmp3xxx(void);
+
+#define mx28_add_dma()	mxs_add_mxs_dma()
diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c
index 6a0202b..57f851e 100644
--- a/arch/arm/mach-mxs/devices/platform-dma.c
+++ b/arch/arm/mach-mxs/devices/platform-dma.c
@@ -30,7 +30,7 @@ static struct platform_device *__init mxs_add_dma(const char *devid,
 				DMA_BIT_MASK(32));
 }
 
-static int __init mxs_add_mxs_dma(void)
+int __init mxs_add_mxs_dma(void)
 {
 	char *apbh = "mxs-dma-apbh";
 	char *apbx = "mxs-dma-apbx";
@@ -47,4 +47,3 @@ static int __init mxs_add_mxs_dma(void)
 
 	return 0;
 }
-arch_initcall(mxs_add_mxs_dma);
diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
index 429b88e..1c4d317 100644
--- a/arch/arm/mach-mxs/imx28-dt.c
+++ b/arch/arm/mach-mxs/imx28-dt.c
@@ -25,6 +25,8 @@ static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
 	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
 	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP0_BASE_ADDR, "mxs-mmc.0", NULL),
 	OF_DEV_AUXDATA("fsl,imx28-mmc", MX28_SSP1_BASE_ADDR, "mxs-mmc.1", NULL),
+	OF_DEV_AUXDATA("fsl,mxs-dma-apbh", MX28_APBH_DMA_BASE_ADDR, "mxs-dma-apbh", NULL),
+	OF_DEV_AUXDATA("fsl,mxs-dma-apbx", MX28_APBX_DMA_BASE_ADDR, "mxs-dma-apbx", NULL),
 	{ /* sentinel */ }
 };
 
diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
index dc369c1..87939a2 100644
--- a/arch/arm/mach-mxs/include/mach/devices-common.h
+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
@@ -106,3 +106,6 @@ struct mxs_saif_data {
 struct platform_device *__init mxs_add_saif(
 		const struct mxs_saif_data *data,
 		const struct mxs_saif_platform_data *pdata);
+
+/* dma */
+int __init mxs_add_mxs_dma(void);
diff --git a/arch/arm/mach-mxs/mach-apx4devkit.c b/arch/arm/mach-mxs/mach-apx4devkit.c
index 48a7fab..92f47e9 100644
--- a/arch/arm/mach-mxs/mach-apx4devkit.c
+++ b/arch/arm/mach-mxs/mach-apx4devkit.c
@@ -210,6 +210,7 @@ static void __init apx4devkit_init(void)
 	mxs_iomux_setup_multiple_pads(apx4devkit_pads,
 			ARRAY_SIZE(apx4devkit_pads));
 
+	mx28_add_dma();
 	mx28_add_duart();
 	mx28_add_auart0();
 	mx28_add_auart1();
diff --git a/arch/arm/mach-mxs/mach-m28evk.c b/arch/arm/mach-mxs/mach-m28evk.c
index 06d7996..9a74ce6 100644
--- a/arch/arm/mach-mxs/mach-m28evk.c
+++ b/arch/arm/mach-mxs/mach-m28evk.c
@@ -321,6 +321,7 @@ static void __init m28evk_init(void)
 {
 	mxs_iomux_setup_multiple_pads(m28evk_pads, ARRAY_SIZE(m28evk_pads));
 
+	mx28_add_dma();
 	mx28_add_duart();
 	mx28_add_auart0();
 	mx28_add_auart3();
diff --git a/arch/arm/mach-mxs/mach-mx23evk.c b/arch/arm/mach-mxs/mach-mx23evk.c
index 5ea1c57..cabc884 100644
--- a/arch/arm/mach-mxs/mach-mx23evk.c
+++ b/arch/arm/mach-mxs/mach-mx23evk.c
@@ -143,6 +143,7 @@ static void __init mx23evk_init(void)
 
 	mxs_iomux_setup_multiple_pads(mx23evk_pads, ARRAY_SIZE(mx23evk_pads));
 
+	mx23_add_dma();
 	mx23_add_duart();
 	mx23_add_auart0();
 
diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index e386c14..4b645eb 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -415,6 +415,7 @@ static void __init mx28evk_init(void)
 
 	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
 
+	mx28_add_dma();
 	mx28_add_duart();
 	mx28_add_auart0();
 	mx28_add_auart3();
diff --git a/arch/arm/mach-mxs/mach-stmp378x_devb.c b/arch/arm/mach-mxs/mach-stmp378x_devb.c
index a626c07..a391b9e 100644
--- a/arch/arm/mach-mxs/mach-stmp378x_devb.c
+++ b/arch/arm/mach-mxs/mach-stmp378x_devb.c
@@ -88,6 +88,7 @@ static void __init stmp378x_dvb_init(void)
 	mxs_iomux_setup_multiple_pads(stmp378x_dvb_pads,
 			ARRAY_SIZE(stmp378x_dvb_pads));
 
+	mx23_add_dma();
 	mx23_add_duart();
 	mx23_add_auart0();
 	mx23_add_rtc_stmp3xxx();
diff --git a/arch/arm/mach-mxs/mach-tx28.c b/arch/arm/mach-mxs/mach-tx28.c
index 2c0862e..e17e262 100644
--- a/arch/arm/mach-mxs/mach-tx28.c
+++ b/arch/arm/mach-mxs/mach-tx28.c
@@ -149,6 +149,7 @@ static void __init tx28_stk5v3_init(void)
 	mxs_iomux_setup_multiple_pads(tx28_stk5v3_pads,
 			ARRAY_SIZE(tx28_stk5v3_pads));
 
+	mx28_add_dma();
 	mx28_add_duart(); /* UART1 */
 	mx28_add_auart(1); /* UART2 */
 
-- 
1.7.0.4

Re: [PATCH v1 5/5] ARM: mxs: add mxs dma dt support

[Huang Shijie]

Hi,
> +			dma-apbh@80004000 {
> +				compatible = "fsl,mxs-dma-apbh";
> +				reg =<0x80004000 2000>;
> +			};
> +
Why you do not add the `interrupt` for the it?
the gpmi-nand needs to parse the interrupt out.

BR
Huang Shijie

Re: [PATCH v1 5/5] ARM: mxs: add mxs dma dt support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 03:58:23PM +0800, Huang Shijie-B32955 wrote:
> Hi,
> > +			dma-apbh@80004000 {
> > +				compatible = "fsl,mxs-dma-apbh";
> > +				reg =<0x80004000 2000>;
> > +			};
> > +
> Why you do not add the `interrupt` for the it?
> the gpmi-nand needs to parse the interrupt out.
> 
The non-dt way also does not have interrupt resource for dma device.
It seems the channel interrupt resource is handled in each client device.
I'm not familiar with gpmi-nand, but I guess you may need to handle
it in gpmi driver, just like mxs-mmc driver.

Regards
Dong Aisheng

Re: [PATCH v1 5/5] ARM: mxs: add mxs dma dt support

[Huang Shijie]

Hi Aisheng:
> The non-dt way also does not have interrupt resource for dma device.
> It seems the channel interrupt resource is handled in each client device.
> I'm not familiar with gpmi-nand, but I guess you may need to handle
> it in gpmi driver, just like mxs-mmc driver.
>
ok, got it.

thanks
Huang Shijie

Re: [PATCH v1 0/5] dt: add basic imx28 support

[Marek Vasut]

Dear Dong Aisheng,

> This patch series adds basic imx28 dt support including fec, mmc and dma.
> 
> Tested on mx28evk.
> 
> TODO:
> Convert the remaining devices to support dt.
> mxs-auart, mxs-gpio, i2c, flexcan, saif, rtc, pwm, fb.
> 
> Dong Aisheng (5):
>   ARM: imx28: add basic dt support
>   mmc: mxs-mmc: add dt probe support
>   ARM: imx28evk: add mmc dt support
>   dma: mxs-dma: add dt probe support
>   ARM: mxs: add mxs dma dt support
> 
Can you keep me in Cc on new versions of these patches? Thanks!

Best regards,
Marek Vasut

Re: [PATCH v1 0/5] dt: add basic imx28 support

[Dong Aisheng]

On Wed, Mar 14, 2012 at 07:01:06AM +0100, Marek Vasut wrote:
> Dear Dong Aisheng,
> 
> > This patch series adds basic imx28 dt support including fec, mmc and dma.
> > 
> > Tested on mx28evk.
> > 
> > TODO:
> > Convert the remaining devices to support dt.
> > mxs-auart, mxs-gpio, i2c, flexcan, saif, rtc, pwm, fb.
> > 
> > Dong Aisheng (5):
> >   ARM: imx28: add basic dt support
> >   mmc: mxs-mmc: add dt probe support
> >   ARM: imx28evk: add mmc dt support
> >   dma: mxs-dma: add dt probe support
> >   ARM: mxs: add mxs dma dt support
> > 
> Can you keep me in Cc on new versions of these patches? Thanks!
> 
Of course yes.
Thanks for your review.

Regards
Dong Aisheng