On Wed, Mar 28, 2012 at 01:58:04PM +0800, Guo Shawn-R65073 wrote:

On Fri, Mar 23, 2012 at 10:31:10PM +0800, Dong Aisheng wrote:

quoted

From: Dong Aisheng <redacted>

...

quoted

+/dts-v1/;
+/include/ "imx28.dtsi"
+
+/ {
+	model = "Freescale i.MX28 Evaluation Kit";
+	compatible = "fsl,imx28-evk", "fsl,imx28";
+
+	memory {
+		device_type = "memory";

This is already in skeleton.dtsi included by imx28.dtsi.

Correct.

quoted

+/include/ "skeleton.dtsi"
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;

These two are already in skeleton.dtsi.

will remove.

quoted

+			icoll: interrupt-controller at 80000000 {
+				compatible = "fsl,imx28-icoll";

I would expect it be:

	compatible = "fsl,imx28-icoll", "fsl,mxs-icoll";

So it can be matched by both imx23 and imx28.

Yes, i can change like that.

quoted

+				#interrupt-cells = <1>;
+			};
+

diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index c57f996..c776aef 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig

@@ -17,6 +17,14 @@ config SOC_IMX28
 
 comment "MXS platforms:"
 
+config MACH_MXS_DT
+	bool "Support MXS platforms from device tree"
+	select SOC_IMX28
+	select USE_OF
+	help
+	  Include support for Freescale MXS platforms(i.MX23 and i.MX28)
+	  using the device tree for discovery
+

If I build mxs_defconfig with only MACH_MXS_DT enabled, I got

  LD      .tmp_vmlinux1
arch/arm/mach-mxs/built-in.o: In function `mxs_add_amba_device':
arch/arm/mach-mxs/devices.c:89: undefined reference to `amba_device_register'

It looks ideally we do not need to compile devices.c and devices/* for only dt.

It's caused by missing "select ARM_AMBA".  For non-dt build, it gets
selected under "config MXS_HAVE_AMBA_DUART" (mach-mxs/devices/Kconfig).

I intend to fix it in the following way.

I agree with this temporary fix.

Correct.

quoted

+
+static int __init imx28_icoll_add_irq_domain(struct device_node *np,

mxs_icoll_add_irq_domain

quoted

+				struct device_node *interrupt_parent)
+{
+	irq_domain_add_simple(np, 0);
+
+	return 0;
+}
+
+static const struct of_device_id mxs_irq_match[] __initconst = {
+	{ .compatible = "fsl,imx28-icoll", .data = imx28_icoll_add_irq_domain, },

"fsl,mxs-icoll"

Will change.

quoted

+	{ /* sentinel */ }
+};
+
+static void __init mxs_dt_init_irq(void)
+{
+	icoll_init_irq();
+	of_irq_init(mxs_irq_match);
+}
+
+static void __init imx28_timer_init(void)
+{
+	mx28_clocks_init();
+}
+
+static struct sys_timer imx28_timer = {
+	.init = imx28_timer_init,
+};
+
+static void __init imx28_machine_init(void)

mxs_init_machine(), so that imx23 can use it later and have the
function name somehow aligned with hook name .init_machine.

I can do it, but, as icoll, that means we're doing things by assuming
no difference between mx23 and mx28 before we really start mx23 dt work.
However, i think at least of_platform_populate should be common.
So i agree to change to mxs_init_machine right now.
If any difference we may change accordingly latter.

quoted

+{
+       of_platform_populate(NULL, of_default_bus_match_table,
+		NULL, NULL);
+}
+
+static const char *imx28_dt_compat[] __initdata = {

mxs_dt_compat

If changed like that, is it reasonable for mx23 to use this
compatible string list?
I planed to have separate compatible string for mx23 and mx28.

quoted

+	"fsl,imx28",
+	"fsl,imx28-evk",

I would have the list sorted from the most specific to the most
general.  That said, it's better to have "fsl,imx28" sorted after
"fsl,imx28-evk".

I prefer to keep the basic one first, then for future boards support
we just add them below rather than insert above the basic one "fsl,imx28".
However, it's really not a big deal.
If you persist to do like that, i can also do it.

quoted

+	NULL,
+};
+
+DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
+	.map_io		= mx28_map_io,
+	.init_irq	= mxs_dt_init_irq,
+	.timer		= &imx28_timer,
+	.init_machine	= imx28_machine_init,
+	.dt_compat	= imx28_dt_compat,
+	.restart	= mxs_restart,
+MACHINE_END
-- 
1.7.0.4

Regards
Dong Aisheng