[PATCH 0/3] Add MX51 basic DT support

[PATCH 0/3] Add MX51 basic DT support

[Jason Liu]

This patchset adds FSL mx51 device tree support. This
is based on

git://git.secretlab.ca/git/linux-2.6 devicetree/test

This patch has been tested on MX51 babbage board and can
boot up succesfully to linux console with DT enabled.

Jason Liu (3):
  arm/dt: add basic mx51 device tree support
  arm/dt: add very basic dts file for babbage board
  serial/imx: parse from device tree support

 arch/arm/boot/dts/babbage.dts           |  117 +++++++++++++++++++++++++++++++
 arch/arm/mach-mx5/Kconfig               |    6 ++
 arch/arm/mach-mx5/Makefile              |    1 +
 arch/arm/mach-mx5/board-dt.c            |   64 +++++++++++++++++
 arch/arm/mach-mx5/clock-mx51-mx53.c     |   45 ++++++++++++-
 arch/arm/plat-mxc/include/mach/common.h |    1 +
 drivers/tty/serial/imx.c                |   81 +++++++++++++++++++---
 7 files changed, 304 insertions(+), 11 deletions(-)
 create mode 100644 arch/arm/boot/dts/babbage.dts
 create mode 100644 arch/arm/mach-mx5/board-dt.c
 mode change 100644 => 100755 drivers/tty/serial/imx.c

[PATCH 1/3] arm/dt: add basic mx51 device tree support

[Jason Liu]

Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/mach-mx5/Kconfig               |    6 +++
 arch/arm/mach-mx5/Makefile              |    1 +
 arch/arm/mach-mx5/board-dt.c            |   64 +++++++++++++++++++++++++++++++
 arch/arm/mach-mx5/clock-mx51-mx53.c     |   45 +++++++++++++++++++++-
 arch/arm/plat-mxc/include/mach/common.h |    1 +
 5 files changed, 116 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index de4fa99..5fbbd36 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -47,6 +47,12 @@ config MACH_MX51_BABBAGE
 	  u-boot. This includes specific configurations for the board and its
 	  peripherals.
 
+config MACH_MX51_DT
+       bool "Generic MX51 board (FDT support)"
+       select USE_OF
+       help
+         Support for generic Freescale MX51 boards using Flattened Device Tree.
+
 config MACH_MX51_3DS
 	bool "Support MX51PDK (3DS)"
 	select SOC_IMX51
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0d43be9..540697e 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_MACH_EUKREA_CPUIMX51SD) += board-cpuimx51sd.o
 obj-$(CONFIG_MACH_EUKREA_MBIMXSD51_BASEBOARD) += eukrea_mbimxsd-baseboard.o
 obj-$(CONFIG_MACH_MX51_EFIKAMX) += board-mx51_efikamx.o
 obj-$(CONFIG_MACH_MX50_RDP) += board-mx50_rdp.o
+obj-$(CONFIG_MACH_MX51_DT) += board-dt.o
diff --git a/arch/arm/mach-mx5/board-dt.c b/arch/arm/mach-mx5/board-dt.c
new file mode 100644
index 0000000..78ecd12
--- /dev/null
+++ b/arch/arm/mach-mx5/board-dt.c
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * 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/err.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/of_platform.h>
+#include <linux/of_fdt.h>
+
+#include <mach/common.h>
+#include <mach/hardware.h>
+#include <mach/imx-uart.h>
+#include <mach/iomux-mx51.h>
+
+#include <asm/irq.h>
+#include <asm/setup.h>
+#include <asm/mach-types.h>
+#include <asm/mach/arch.h>
+#include <asm/mach/time.h>
+
+#include "devices.h"
+
+static struct of_device_id mx51_dt_match_table[] __initdata = {
+	{ .compatible = "simple-bus", },
+	{}
+};
+
+static void __init mx51_dt_board_init(void)
+{
+	of_platform_bus_probe(NULL, mx51_dt_match_table, NULL);
+}
+
+static void __init mx51_dt_timer_init(void)
+{
+	mx51_clocks_init(32768, 24000000, 22579200, 0);
+	mx5_clk_dt_init();
+}
+
+static struct sys_timer mxc_timer = {
+	.init = mx51_dt_timer_init,
+};
+
+static const char * mx51_dt_board_compat[] = {
+	"fsl,mx51_babbage",
+	NULL
+};
+
+DT_MACHINE_START(MX51_DT, "Freescale MX51 (Flattened Device Tree)")
+	.boot_params  = PHYS_OFFSET + 0x100,
+	.map_io       = mx51_map_io,
+	.init_irq     = mx51_init_irq,
+	.init_machine = mx51_dt_board_init,
+	.dt_compat    = mx51_dt_board_compat,
+	.timer        = &mxc_timer,
+MACHINE_END
diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
index 0a19e75..b8a608e 100644
--- a/arch/arm/mach-mx5/clock-mx51-mx53.c
+++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
@@ -15,13 +15,19 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/clkdev.h>
-
+#include <linux/err.h>
 #include <asm/div64.h>
 
 #include <mach/hardware.h>
 #include <mach/common.h>
 #include <mach/clock.h>
 
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_clk.h>
+#endif /* CONFIG_OF */
+
 #include "crm_regs.h"
 
 /* External clock values passed-in by the board code */
@@ -1432,3 +1438,40 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
 		MX53_INT_GPT);
 	return 0;
 }
+
+#ifdef CONFIG_OF
+static struct clk* mx5_dt_clk_get(struct device_node *np,
+					const char *output_id, void *data)
+{
+	return data;
+}
+
+static __init void mx5_dt_scan_clks(void)
+{
+	struct device_node *node;
+	struct clk *clk;
+	const char *id;
+	int rc;
+
+	for_each_compatible_node(node, NULL, "clock") {
+		id = of_get_property(node, "clock-outputs", NULL);
+		if (!id)
+			continue;
+
+		clk = clk_get_sys(id, NULL);
+		if (IS_ERR(clk))
+			continue;
+
+		rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
+		if (rc) {
+			kfree(clk);
+			pr_err("error adding fixed clk %s\n", node->name);
+		}
+	}
+}
+
+void __init mx5_clk_dt_init(void)
+{
+	mx5_dt_scan_clks();
+}
+#endif
diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
index aea2cd3..a28e84a 100644
--- a/arch/arm/plat-mxc/include/mach/common.h
+++ b/arch/arm/plat-mxc/include/mach/common.h
@@ -58,4 +58,5 @@ extern void mxc91231_arch_reset(int, const char *);
 extern void mxc91231_prepare_idle(void);
 extern void mx51_efikamx_reset(void);
 extern int mx53_revision(void);
+extern void mx5_clk_dt_init(void);
 #endif
-- 
1.7.0.4

Re: [PATCH 1/3] arm/dt: add basic mx51 device tree support

[Shawn Guo]

Hi Jason,

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/arm/mach-mx5/Kconfig               |    6 +++
>  arch/arm/mach-mx5/Makefile              |    1 +
>  arch/arm/mach-mx5/board-dt.c            |   64 +++++++++++++++++++++++++++++++
>  arch/arm/mach-mx5/clock-mx51-mx53.c     |   45 +++++++++++++++++++++-
>  arch/arm/plat-mxc/include/mach/common.h |    1 +
>  5 files changed, 116 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> index de4fa99..5fbbd36 100644
> --- a/arch/arm/mach-mx5/Kconfig
> +++ b/arch/arm/mach-mx5/Kconfig
> @@ -47,6 +47,12 @@ config MACH_MX51_BABBAGE
>          u-boot. This includes specific configurations for the board and its
>          peripherals.
>
> +config MACH_MX51_DT
> +       bool "Generic MX51 board (FDT support)"
> +       select USE_OF
> +       help
> +         Support for generic Freescale MX51 boards using Flattened Device Tree.
> +

We may at least need to select SOC_IMX51 here, otherwise it does not
even build if MACH_MX51_DT is the only machine being selected (e.g.
none of any other mx51 machine being selected together).

And tab is being used throughout the file for indentation, but you
are using spaces here.

>  config MACH_MX51_3DS
>        bool "Support MX51PDK (3DS)"
>        select SOC_IMX51

-- 
Regards,
Shawn

Re: [PATCH 1/3] arm/dt: add basic mx51 device tree support

[Shawn Guo]

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/arm/mach-mx5/Kconfig               |    6 +++
>  arch/arm/mach-mx5/Makefile              |    1 +
>  arch/arm/mach-mx5/board-dt.c            |   64 +++++++++++++++++++++++++++++++
>  arch/arm/mach-mx5/clock-mx51-mx53.c     |   45 +++++++++++++++++++++-
>  arch/arm/plat-mxc/include/mach/common.h |    1 +
>  5 files changed, 116 insertions(+), 1 deletions(-)
>
[...]
> diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> index 0a19e75..b8a608e 100644
> --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> @@ -15,13 +15,19 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/clkdev.h>
> -
> +#include <linux/err.h>
>  #include <asm/div64.h>
>
>  #include <mach/hardware.h>
>  #include <mach/common.h>
>  #include <mach/clock.h>
>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_clk.h>
> +#endif /* CONFIG_OF */
> +
>  #include "crm_regs.h"
>
>  /* External clock values passed-in by the board code */
> @@ -1432,3 +1438,40 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
>                MX53_INT_GPT);
>        return 0;
>  }
> +
> +#ifdef CONFIG_OF
> +static struct clk* mx5_dt_clk_get(struct device_node *np,
> +                                       const char *output_id, void *data)
> +{
> +       return data;
> +}
> +
> +static __init void mx5_dt_scan_clks(void)
> +{
> +       struct device_node *node;
> +       struct clk *clk;
> +       const char *id;
> +       int rc;
> +
> +       for_each_compatible_node(node, NULL, "clock") {
> +               id = of_get_property(node, "clock-outputs", NULL);
> +               if (!id)
> +                       continue;
> +
> +               clk = clk_get_sys(id, NULL);
> +               if (IS_ERR(clk))
> +                       continue;
> +
> +               rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
> +               if (rc) {
> +                       kfree(clk);

In this particular implementation, kfree here may not be needed, as
all the "clk" are currently created in the static way.  And I'm trying
to change it to the dynamic way by scanning clock node from dt,
creating and registering the "clk" correspondingly.

> +                       pr_err("error adding fixed clk %s\n", node->name);
> +               }
> +       }
> +}
> +
> +void __init mx5_clk_dt_init(void)
> +{
> +       mx5_dt_scan_clks();
> +}
> +#endif

-- 
Regards,
Shawn

Re: [PATCH 1/3] arm/dt: add basic mx51 device tree support

[Jason Hui]

Hi, Shawn,

On Mon, Feb 28, 2011 at 2:48 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>  arch/arm/mach-mx5/Kconfig               |    6 +++
>>  arch/arm/mach-mx5/Makefile              |    1 +
>>  arch/arm/mach-mx5/board-dt.c            |   64 +++++++++++++++++++++++++++++++
>>  arch/arm/mach-mx5/clock-mx51-mx53.c     |   45 +++++++++++++++++++++-
>>  arch/arm/plat-mxc/include/mach/common.h |    1 +
>>  5 files changed, 116 insertions(+), 1 deletions(-)
>>
> [...]
>> diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
>> index 0a19e75..b8a608e 100644
>> --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
>> +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
>> @@ -15,13 +15,19 @@
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>>  #include <linux/clkdev.h>
>> -
>> +#include <linux/err.h>
>>  #include <asm/div64.h>
>>
>>  #include <mach/hardware.h>
>>  #include <mach/common.h>
>>  #include <mach/clock.h>
>>
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_clk.h>
>> +#endif /* CONFIG_OF */
>> +
>>  #include "crm_regs.h"
>>
>>  /* External clock values passed-in by the board code */
>> @@ -1432,3 +1438,40 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
>>                MX53_INT_GPT);
>>        return 0;
>>  }
>> +
>> +#ifdef CONFIG_OF
>> +static struct clk* mx5_dt_clk_get(struct device_node *np,
>> +                                       const char *output_id, void *data)
>> +{
>> +       return data;
>> +}
>> +
>> +static __init void mx5_dt_scan_clks(void)
>> +{
>> +       struct device_node *node;
>> +       struct clk *clk;
>> +       const char *id;
>> +       int rc;
>> +
>> +       for_each_compatible_node(node, NULL, "clock") {
>> +               id = of_get_property(node, "clock-outputs", NULL);
>> +               if (!id)
>> +                       continue;
>> +
>> +               clk = clk_get_sys(id, NULL);
>> +               if (IS_ERR(clk))
>> +                       continue;
>> +
>> +               rc = of_clk_add_provider(node, mx5_dt_clk_get, clk);
>> +               if (rc) {
>> +                       kfree(clk);
>
> In this particular implementation, kfree here may not be needed, as
> all the "clk" are currently created in the static way.  And I'm trying
> to change it to the dynamic way by scanning clock node from dt,
> creating and registering the "clk" correspondingly.

Yes, I will look into your dynamic way, but before the you real patch come up,
I will use the static one to do it currently.

>
>> +                       pr_err("error adding fixed clk %s\n", node->name);
>> +               }
>> +       }
>> +}
>> +
>> +void __init mx5_clk_dt_init(void)
>> +{
>> +       mx5_dt_scan_clks();
>> +}
>> +#endif
>
> --
> Regards,
> Shawn
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

[PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Jason Liu]

Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/babbage.dts |  117 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/babbage.dts b/arch/arm/boot/dts/babbage.dts
new file mode 100644
index 0000000..7ee26f1
--- /dev/null
+++ b/arch/arm/boot/dts/babbage.dts
@@ -0,0 +1,117 @@
+/dts-v1/;
+
+/ {
+	model = "mx51_babbage";
+	compatible = "fsl,mx51_babbage";
+	#address-cells = <0x1>;
+	#size-cells = <0x1>;
+	#interrupt-cells = <0x1>;
+	interrupt-parent = <0x1>;
+
+	memory {
+		device_type = "memory";
+		reg = <0x90000000 0x20000000>;
+	};
+
+	chosen {
+		bootargs = "console=ttymxc1,115200n8 debug earlyprintk";
+	};
+
+	soc {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		device_type = "soc";
+		compatible = "simple-bus";
+		ranges = <0x0 0x0 0xffffffff>;
+
+		tzic {
+			#address-cells = <0x0>;
+			#interrupt-cells = <0x1>;
+			interrupt-controller;
+			reg = <0xe0000000 0x1000>;
+			compatible = "fsl,tzic";
+			device_type = "tzic";
+			phandle = <0x1>;
+		};
+	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		uart_clk0: uart@0 {
+			compatible = "clock";
+			clock-outputs = "imx-uart.0";
+		};
+
+		uart_clk1: uart@1{
+			compatible = "clock";
+			clock-outputs = "imx-uart.1";
+		};
+
+		uart_clk2: uart@2{
+			compatible = "clock";
+			clock-outputs = "imx-uart.2";
+		};
+
+		fec_clk: @0{
+			compatible = "clock";
+			clock-outputs = "fec.0";
+		};
+	};
+
+	spba@70000000 {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		device_type = "soc";
+		compatible = "simple-bus";
+		ranges = <0x0 0x70000000 0x100000>;
+
+		imx-uart@C000 {
+			compatible = "imx-uart";
+			reg = <0xc000 0x1000>;
+			interrupts = <0x21>;
+			rts-cts;
+			uart-clock = < &uart_clk2 >, "uart";
+		};
+	};
+
+	aips@73f00000 {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		device_type = "soc";
+		compatible = "simple-bus";
+		ranges = <0x0 0x73f00000 0x100000>;
+
+		imx-uart@BC000 {
+			compatible = "imx-uart";
+			reg = <0xbc000 0x1000>;
+			interrupts = <0x1f>;
+			rts-cts;
+			uart-clock = < &uart_clk0 >, "uart";
+		};
+
+		imx-uart@C0000 {
+			compatible = "imx-uart";
+			reg = <0xc0000 0x1000>;
+			interrupts = <0x20>;
+			rts-cts;
+			uart-clock = <&uart_clk1>, "uart";
+		};
+	};
+
+	aips@83f00000 {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		device_type = "soc";
+		compatible = "simple-bus";
+		ranges = <0x0 0x83f00000 0x100000>;
+
+		fec@EC000 {
+			compatible = "fec";
+			reg = <0xec000 0x1000>;
+			interrupts = <0x57>;
+			fec_clk-clock = < &fec_clk >, "fec";
+		};
+	};
+};
-- 
1.7.0.4

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Shawn Guo]

Here is what I copied from booting-without-of.txt.

=====
  f) the /soc<SOCname> node

  This node is used to represent a system-on-a-chip (SoC) and must be
  present if the processor is a SoC. The top-level soc node contains
  information that is global to all devices on the SoC. The node name
  should contain a unit address for the SoC, which is the base address
  of the memory-mapped register set for the SoC. The name of an SoC
  node should start with "soc", and the remainder of the name should
  represent the part number for the soc.  For example, the MPC8540's
  soc node would be called "soc8540".

  Required properties:

    - ranges : Should be defined as specified in 1) to describe the
      translation of SoC addresses for memory mapped SoC registers.
    - bus-frequency: Contains the bus frequency for the SoC node.
      Typically, the value of this field is filled in by the boot
      loader.
    - compatible : Exact model of the SoC


  Recommended properties:

    - reg : This property defines the address and size of the
      memory-mapped registers that are used for the SOC node itself.
      It does not include the child device registers - these will be
      defined inside each child node.  The address specified in the
      "reg" property should match the unit address of the SOC node.
    - #address-cells : Address representation for "soc" devices.  The
      format of this field may vary depending on whether or not the
      device registers are memory mapped.  For memory mapped
      registers, this field represents the number of cells needed to
      represent the address of the registers.  For SOCs that do not
      use MMIO, a special address format should be defined that
      contains enough cells to represent the required information.
      See 1) above for more details on defining #address-cells.
    - #size-cells : Size representation for "soc" devices
    - #interrupt-cells : Defines the width of cells used to represent
       interrupts.  Typically this value is <2>, which includes a
       32-bit number that represents the interrupt number, and a
       32-bit number that represents the interrupt sense and level.
       This field is only needed if the SOC contains an interrupt
       controller.

  The SOC node may contain child nodes for each SOC device that the
  platform uses.  Nodes should not be created for devices which exist
  on the SOC but are not used by a particular platform. See chapter VI
  for more information on how to specify devices that are part of a SOC.

  Example SOC node for the MPC8540:

        soc8540@e0000000 {
                #address-cells = <1>;
                #size-cells = <1>;
                #interrupt-cells = <2>;
                device_type = "soc";
                ranges = <00000000 e0000000 00100000>
                reg = <e0000000 00003000>;
                bus-frequency = <0>;
        }
=====

I'm seeing the babbage.dts defined "soc" so differently than the
document requires.

> +       soc {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x0 0xffffffff>;
> +
> +               tzic {
> +                       #address-cells = <0x0>;
> +                       #interrupt-cells = <0x1>;
> +                       interrupt-controller;
> +                       reg = <0xe0000000 0x1000>;
> +                       compatible = "fsl,tzic";
> +                       device_type = "tzic";
> +                       phandle = <0x1>;
> +               };
> +       };

* The soc node name has neither part number nor base address
* Even required property "bus-frequency" is missing
* The document "Appendix A - Sample SOC node for MPC8540" defines
every device node under "soc", while babbage.dts has most devices
outside the "soc" node.

Are what the document describes requirements we must follow or just
suggestions we can take or not?

Besides the overall question above, some nitpicking embedded below ...

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/arm/boot/dts/babbage.dts |  117 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/babbage.dts b/arch/arm/boot/dts/babbage.dts
> new file mode 100644
> index 0000000..7ee26f1
> --- /dev/null
> +++ b/arch/arm/boot/dts/babbage.dts
> @@ -0,0 +1,117 @@
> +/dts-v1/;
> +
> +/ {
> +       model = "mx51_babbage";
> +       compatible = "fsl,mx51_babbage";
> +       #address-cells = <0x1>;
> +       #size-cells = <0x1>;
> +       #interrupt-cells = <0x1>;
> +       interrupt-parent = <0x1>;
> +
> +       memory {
> +               device_type = "memory";
> +               reg = <0x90000000 0x20000000>;
> +       };
> +
> +       chosen {
> +               bootargs = "console=ttymxc1,115200n8 debug earlyprintk";
> +       };
> +
> +       soc {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x0 0xffffffff>;
> +
> +               tzic {
> +                       #address-cells = <0x0>;
> +                       #interrupt-cells = <0x1>;
> +                       interrupt-controller;
> +                       reg = <0xe0000000 0x1000>;
> +                       compatible = "fsl,tzic";
> +                       device_type = "tzic";
> +                       phandle = <0x1>;
> +               };
> +       };
> +
> +       clocks {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +

Can we use hex value here to keep the consistency throughout the file?

> +               uart_clk0: uart@0 {
> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.0";
> +               };
> +
> +               uart_clk1: uart@1{

Can we put a space before "{" to keep the consistency throughout the file?

> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.1";
> +               };
> +
> +               uart_clk2: uart@2{

ditto

> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.2";
> +               };
> +
> +               fec_clk: @0{

ditto

> +                       compatible = "clock";
> +                       clock-outputs = "fec.0";
> +               };
> +       };
> +
> +       spba@70000000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x70000000 0x100000>;
> +
> +               imx-uart@C000 {

Use lowercase for all address/offset to keep consistency throughout the file?

> +                       compatible = "imx-uart";
> +                       reg = <0xc000 0x1000>;
> +                       interrupts = <0x21>;
> +                       rts-cts;
> +                       uart-clock = < &uart_clk2 >, "uart";

Remove the space after "<" and before ">" to keep consistency
throughout the file?

> +               };
> +       };
> +
> +       aips@73f00000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x73f00000 0x100000>;
> +
> +               imx-uart@BC000 {

ditto

> +                       compatible = "imx-uart";
> +                       reg = <0xbc000 0x1000>;
> +                       interrupts = <0x1f>;
> +                       rts-cts;
> +                       uart-clock = < &uart_clk0 >, "uart";

Remove the space after "<" and before ">"?

> +               };
> +
> +               imx-uart@C0000 {

Lowercase for offset?

> +                       compatible = "imx-uart";
> +                       reg = <0xc0000 0x1000>;
> +                       interrupts = <0x20>;
> +                       rts-cts;
> +                       uart-clock = <&uart_clk1>, "uart";
> +               };
> +       };
> +
> +       aips@83f00000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x83f00000 0x100000>;
> +
> +               fec@EC000 {

ditto

> +                       compatible = "fec";
> +                       reg = <0xec000 0x1000>;
> +                       interrupts = <0x57>;
> +                       fec_clk-clock = < &fec_clk >, "fec";
> +               };
> +       };
> +};
> --
> 1.7.0.4
>

-- 
Regards,
Shawn

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Grant Likely]

On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Here is what I copied from booting-without-of.txt.
>
> =====
>  f) the /soc<SOCname> node
>
>  This node is used to represent a system-on-a-chip (SoC) and must be
>  present if the processor is a SoC. The top-level soc node contains
>  information that is global to all devices on the SoC. The node name
>  should contain a unit address for the SoC, which is the base address
>  of the memory-mapped register set for the SoC. The name of an SoC
>  node should start with "soc", and the remainder of the name should
>  represent the part number for the soc.  For example, the MPC8540's
>  soc node would be called "soc8540".
>
>  Required properties:
>
>    - ranges : Should be defined as specified in 1) to describe the
>      translation of SoC addresses for memory mapped SoC registers.
>    - bus-frequency: Contains the bus frequency for the SoC node.
>      Typically, the value of this field is filled in by the boot
>      loader.
>    - compatible : Exact model of the SoC
>
>
>  Recommended properties:
>
>    - reg : This property defines the address and size of the
>      memory-mapped registers that are used for the SOC node itself.
>      It does not include the child device registers - these will be
>      defined inside each child node.  The address specified in the
>      "reg" property should match the unit address of the SOC node.
>    - #address-cells : Address representation for "soc" devices.  The
>      format of this field may vary depending on whether or not the
>      device registers are memory mapped.  For memory mapped
>      registers, this field represents the number of cells needed to
>      represent the address of the registers.  For SOCs that do not
>      use MMIO, a special address format should be defined that
>      contains enough cells to represent the required information.
>      See 1) above for more details on defining #address-cells.
>    - #size-cells : Size representation for "soc" devices
>    - #interrupt-cells : Defines the width of cells used to represent
>       interrupts.  Typically this value is <2>, which includes a
>       32-bit number that represents the interrupt number, and a
>       32-bit number that represents the interrupt sense and level.
>       This field is only needed if the SOC contains an interrupt
>       controller.
>
>  The SOC node may contain child nodes for each SOC device that the
>  platform uses.  Nodes should not be created for devices which exist
>  on the SOC but are not used by a particular platform. See chapter VI
>  for more information on how to specify devices that are part of a SOC.
>
>  Example SOC node for the MPC8540:
>
>        soc8540@e0000000 {
>                #address-cells = <1>;
>                #size-cells = <1>;
>                #interrupt-cells = <2>;
>                device_type = "soc";
>                ranges = <00000000 e0000000 00100000>
>                reg = <e0000000 00003000>;
>                bus-frequency = <0>;
>        }
> =====
>
> I'm seeing the babbage.dts defined "soc" so differently than the
> document requires.
>
>> +       soc {
>> +               #address-cells = <0x1>;
>> +               #size-cells = <0x1>;
>> +               device_type = "soc";
>> +               compatible = "simple-bus";
>> +               ranges = <0x0 0x0 0xffffffff>;
>> +
>> +               tzic {
>> +                       #address-cells = <0x0>;
>> +                       #interrupt-cells = <0x1>;
>> +                       interrupt-controller;
>> +                       reg = <0xe0000000 0x1000>;
>> +                       compatible = "fsl,tzic";
>> +                       device_type = "tzic";
>> +                       phandle = <0x1>;
>> +               };
>> +       };
>

The document is a little out of date in that it doesn't reflect all of
the current best practices.  It does need to be updated, but I'll
respond to the specific questions here...

> * The soc node name has neither part number nor base address

Part number isn't necessary because the 'compatible' property is
supposed to be used to identify the exact device.  Base address must
be part of any node that has a 'reg' property.  In this particular
example, neither is the case so it is okay.

> * Even required property "bus-frequency" is missing

bus-frequency isn't required.

> * The document "Appendix A - Sample SOC node for MPC8540" defines
> every device node under "soc", while babbage.dts has most devices
> outside the "soc" node.

The 'soc' node thing was something we started when new PowerPC SoC
machines were created.  However, it doesn't necessarily reflect the
best practice.  Ideally, the dt structure will match the physical
structure of the chip.  So, if a chip has multiple internal busses,
then the dts should probably have a node for each bus, and then each
device a child of the bus node.  However, having a single containing
'soc' node for all on-chip busses and devices is probably a good thing
to preserve.

>
> Are what the document describes requirements we must follow or just
> suggestions we can take or not?

Crafting a .dts file and device tree bindings are as much an art as a
science.  There are some things that aren't hard rules, but there are
reasons behind why most are done in a certain way.  The best way to
develop a new .dts file is to draft up your best effort, post it for
review, and listen to the comments that come back.  :-)

>
> Besides the overall question above, some nitpicking embedded below ...
>
> On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/babbage.dts |  117 +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 117 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/babbage.dts b/arch/arm/boot/dts/babbage.dts
>> new file mode 100644
>> index 0000000..7ee26f1
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/babbage.dts
>> @@ -0,0 +1,117 @@
>> +/dts-v1/;
>> +
>> +/ {
>> +       model = "mx51_babbage";

This is a human readable string.  You can write something like
"Freescale i.MX51 Babbage" here.

>> +       compatible = "fsl,mx51_babbage";

Use dash '-' instead of underscores '_' in compatible values.

>> +       #address-cells = <0x1>;
>> +       #size-cells = <0x1>;
>> +       #interrupt-cells = <0x1>;
>> +       interrupt-parent = <0x1>;
>> +
>> +       memory {
>> +               device_type = "memory";
>> +               reg = <0x90000000 0x20000000>;
>> +       };
>> +
>> +       chosen {
>> +               bootargs = "console=ttymxc1,115200n8 debug earlyprintk";
>> +       };
>> +
>> +       soc {
>> +               #address-cells = <0x1>;
>> +               #size-cells = <0x1>;
>> +               device_type = "soc";

Drop the "device_type" property.  It only makes sense for machines
with real Open Firmware.

>> +               compatible = "simple-bus";

Need to specify the specific chip here:

        compatible = "fsl,imx51-soc", "simple-bus";

>> +               ranges = <0x0 0x0 0xffffffff>;

one-to-one mapping of the full address range can be specified simply
with an empty ranges property:

        ranges;

>> +
>> +               tzic {

tzic@e0000000 {

>> +                       #address-cells = <0x0>;
>> +                       #interrupt-cells = <0x1>;
>> +                       interrupt-controller;
>> +                       reg = <0xe0000000 0x1000>;
>> +                       compatible = "fsl,tzic";

Specify the exact chip in the compatible property:

compatible = "fsl,imx51-tzic";

>> +                       device_type = "tzic";

Drop device type

>> +                       phandle = <0x1>;

Drop phandle; dtc adds phandles automatically.

>> +               };
>> +       };
>> +
>> +       clocks {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>
> Can we use hex value here to keep the consistency throughout the file?

For #address-cells and #size-cells use decimal integers.

>
>> +               uart_clk0: uart@0 {

@0 should only be specified if the node has a 'reg = <0>' property.
In this case it doesn't so either 'reg' should be added, or '@0'
should be removed.

>> +                       compatible = "clock";
>> +                       clock-outputs = "imx-uart.0";
>> +               };
>> +
>> +               uart_clk1: uart@1{
>
> Can we put a space before "{" to keep the consistency throughout the file?

Yes, please.

>
>> +                       compatible = "clock";
>> +                       clock-outputs = "imx-uart.1";
>> +               };
>> +
>> +               uart_clk2: uart@2{
>
> ditto
>
>> +                       compatible = "clock";
>> +                       clock-outputs = "imx-uart.2";
>> +               };
>> +
>> +               fec_clk: @0{

"@0" is not a valid node name.

>
> ditto
>
>> +                       compatible = "clock";
>> +                       clock-outputs = "fec.0";
>> +               };
>> +       };
>> +
>> +       spba@70000000 {
>> +               #address-cells = <0x1>;
>> +               #size-cells = <0x1>;
>> +               device_type = "soc";

Drop device type.

>> +               compatible = "simple-bus";

Specify the actual device here before "simple-bus"

>> +               ranges = <0x0 0x70000000 0x100000>;
>> +
>> +               imx-uart@C000 {
>
> Use lowercase for all address/offset to keep consistency throughout the file?

yes.

>
>> +                       compatible = "imx-uart";
>> +                       reg = <0xc000 0x1000>;
>> +                       interrupts = <0x21>;
>> +                       rts-cts;
>> +                       uart-clock = < &uart_clk2 >, "uart";
>
> Remove the space after "<" and before ">" to keep consistency
> throughout the file?
>
>> +               };
>> +       };
>> +
>> +       aips@73f00000 {
>> +               #address-cells = <0x1>;
>> +               #size-cells = <0x1>;
>> +               device_type = "soc";
>> +               compatible = "simple-bus";
>> +               ranges = <0x0 0x73f00000 0x100000>;
>> +
>> +               imx-uart@BC000 {
>
> ditto
>
>> +                       compatible = "imx-uart";
>> +                       reg = <0xbc000 0x1000>;
>> +                       interrupts = <0x1f>;
>> +                       rts-cts;
>> +                       uart-clock = < &uart_clk0 >, "uart";
>
> Remove the space after "<" and before ">"?
>
>> +               };
>> +
>> +               imx-uart@C0000 {
>
> Lowercase for offset?
>
>> +                       compatible = "imx-uart";
>> +                       reg = <0xc0000 0x1000>;
>> +                       interrupts = <0x20>;
>> +                       rts-cts;
>> +                       uart-clock = <&uart_clk1>, "uart";
>> +               };
>> +       };
>> +
>> +       aips@83f00000 {
>> +               #address-cells = <0x1>;
>> +               #size-cells = <0x1>;
>> +               device_type = "soc";
>> +               compatible = "simple-bus";
>> +               ranges = <0x0 0x83f00000 0x100000>;
>> +
>> +               fec@EC000 {
>
> ditto
>
>> +                       compatible = "fec";
>> +                       reg = <0xec000 0x1000>;
>> +                       interrupts = <0x57>;
>> +                       fec_clk-clock = < &fec_clk >, "fec";
>> +               };
>> +       };
>> +};
>> --
>> 1.7.0.4
>>
>
> --
> Regards,
> Shawn
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>



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

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Shawn Guo]

Hi Grant,

Thanks for the explanation in details.

One more minor doubt below ...

On Mon, Feb 21, 2011 at 10:10:24AM -0700, Grant Likely wrote:
> On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
[...]
> >> +               };
> >> +       };
> >> +
> >> +       clocks {
> >> +               #address-cells = <1>;
> >> +               #size-cells = <0>;
> >> +
> >
> > Can we use hex value here to keep the consistency throughout the file?
> 
> For #address-cells and #size-cells use decimal integers.
> 
If I run 'dtc -I dts -O dts -o babbage-dtc.dts babbage.dts', I see
babbage-dtc.dts have these two decimal values written into the hex.

-- 
Regards,
Shawn

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Grant Likely]

On Tue, Feb 22, 2011 at 10:13:10PM +0800, Shawn Guo wrote:
> Hi Grant,
> 
> Thanks for the explanation in details.
> 
> One more minor doubt below ...
> 
> On Mon, Feb 21, 2011 at 10:10:24AM -0700, Grant Likely wrote:
> > On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> [...]
> > >> +               };
> > >> +       };
> > >> +
> > >> +       clocks {
> > >> +               #address-cells = <1>;
> > >> +               #size-cells = <0>;
> > >> +
> > >
> > > Can we use hex value here to keep the consistency throughout the file?
> > 
> > For #address-cells and #size-cells use decimal integers.
> > 
> If I run 'dtc -I dts -O dts -o babbage-dtc.dts babbage.dts', I see
> babbage-dtc.dts have these two decimal values written into the hex.

Property values don't have any inherent type checking.  Once it is in
internal form, dtc has no way of knowing the preferred representation
and it just makes a guess.

When it comes to maintaining .dts files; humans like looking at
decimal numbers, so we use decimal for #address/#size-cells.

g.

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Shawn Guo]

Hi Grant,

On Mon, Feb 21, 2011 at 10:10:24AM -0700, Grant Likely wrote:
> On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
[...]
> >> +       clocks {

Do we already have a clock binding specification, which is followed
by this example?  Or the properties given here still can be discussed
and open to change?

Also to support dynamically creating and registering 'struct clk' per
dt clock nodes, I need to add a few properties to map stuff like
parent 'struct clk', con_id of 'struct clk_lookup'.  Is there any
specification to follow, or I can draft it per the needs of my work?

> >> +               #address-cells = <1>;
> >> +               #size-cells = <0>;
> >> +
> >
> > Can we use hex value here to keep the consistency throughout the file?
> 
> For #address-cells and #size-cells use decimal integers.
> 
> >
> >> +               uart_clk0: uart@0 {
> 
> @0 should only be specified if the node has a 'reg = <0>' property.
> In this case it doesn't so either 'reg' should be added, or '@0'
> should be removed.
> 
> >> +                       compatible = "clock";
> >> +                       clock-outputs = "imx-uart.0";
> >> +               };
> >> +
> >> +               uart_clk1: uart@1{
> >
> > Can we put a space before "{" to keep the consistency throughout the file?
> 
> Yes, please.
> 
> >
> >> +                       compatible = "clock";
> >> +                       clock-outputs = "imx-uart.1";
> >> +               };
> >> +
> >> +               uart_clk2: uart@2{
> >
> > ditto
> >
> >> +                       compatible = "clock";
> >> +                       clock-outputs = "imx-uart.2";
> >> +               };

-- 
Regards,
Shawn

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Grant Likely]

On Mon, Feb 28, 2011 at 10:32:01PM +0800, Shawn Guo wrote:
> Hi Grant,
> 
> On Mon, Feb 21, 2011 at 10:10:24AM -0700, Grant Likely wrote:
> > On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> [...]
> > >> +       clocks {
> 
> Do we already have a clock binding specification, which is followed
> by this example?  Or the properties given here still can be discussed
> and open to change?
> 
> Also to support dynamically creating and registering 'struct clk' per
> dt clock nodes, I need to add a few properties to map stuff like
> parent 'struct clk', con_id of 'struct clk_lookup'.  Is there any
> specification to follow, or I can draft it per the needs of my work?

The clock specifications are still in draft.  I'm not willing to lock
down on a binding until we've got some real implementations to back
them up.  If you need additional properties, go ahead an propose them.

Keep in mind when defining/extending bindings, that the dt is intended
to represent hardware layout, not Linux kernel internal implementation
details.  Try to avoid describing things in a linux-specific way.


> 
> > >> +               #address-cells = <1>;
> > >> +               #size-cells = <0>;
> > >> +
> > >
> > > Can we use hex value here to keep the consistency throughout the file?
> > 
> > For #address-cells and #size-cells use decimal integers.
> > 
> > >
> > >> +               uart_clk0: uart@0 {
> > 
> > @0 should only be specified if the node has a 'reg = <0>' property.
> > In this case it doesn't so either 'reg' should be added, or '@0'
> > should be removed.
> > 
> > >> +                       compatible = "clock";
> > >> +                       clock-outputs = "imx-uart.0";
> > >> +               };
> > >> +
> > >> +               uart_clk1: uart@1{
> > >
> > > Can we put a space before "{" to keep the consistency throughout the file?
> > 
> > Yes, please.
> > 
> > >
> > >> +                       compatible = "clock";
> > >> +                       clock-outputs = "imx-uart.1";
> > >> +               };
> > >> +
> > >> +               uart_clk2: uart@2{
> > >
> > > ditto
> > >
> > >> +                       compatible = "clock";
> > >> +                       clock-outputs = "imx-uart.2";
> > >> +               };
> 
> -- 
> Regards,
> Shawn
> 

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Jason Hui]

Hi, Grant and Shawn,

On Tue, Mar 1, 2011 at 2:09 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Mon, Feb 28, 2011 at 10:32:01PM +0800, Shawn Guo wrote:
>> Hi Grant,
>>
>> On Mon, Feb 21, 2011 at 10:10:24AM -0700, Grant Likely wrote:
>> > On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> [...]
>> > >> +       clocks {
>>
>> Do we already have a clock binding specification, which is followed
>> by this example?  Or the properties given here still can be discussed
>> and open to change?
>>
>> Also to support dynamically creating and registering 'struct clk' per
>> dt clock nodes, I need to add a few properties to map stuff like
>> parent 'struct clk', con_id of 'struct clk_lookup'.  Is there any
>> specification to follow, or I can draft it per the needs of my work?
>
> The clock specifications are still in draft.  I'm not willing to lock
> down on a binding until we've got some real implementations to back
> them up.  If you need additional properties, go ahead an propose them.
>
> Keep in mind when defining/extending bindings, that the dt is intended
> to represent hardware layout, not Linux kernel internal implementation
> details.  Try to avoid describing things in a linux-specific way.

I will consider all your review comments and come up with V2 patch soon.

>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

Re: [PATCH 2/3] arm/dt: add very basic dts file for babbage board

[Shawn Guo]

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/arm/boot/dts/babbage.dts |  117 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/babbage.dts b/arch/arm/boot/dts/babbage.dts
> new file mode 100644
> index 0000000..7ee26f1
> --- /dev/null
> +++ b/arch/arm/boot/dts/babbage.dts
> @@ -0,0 +1,117 @@
> +/dts-v1/;
> +
> +/ {
> +       model = "mx51_babbage";
> +       compatible = "fsl,mx51_babbage";
> +       #address-cells = <0x1>;
> +       #size-cells = <0x1>;
> +       #interrupt-cells = <0x1>;
> +       interrupt-parent = <0x1>;
> +
> +       memory {
> +               device_type = "memory";
> +               reg = <0x90000000 0x20000000>;
> +       };
> +
> +       chosen {
> +               bootargs = "console=ttymxc1,115200n8 debug earlyprintk";
> +       };

I was confused by this a little bit.  We used to have ttymxc0 than
ttymxc1 here for bootargs.  Per this dts file, we have the imx uart
nodes in order of imx-uart.2 (0x7000c000) --> imx-uart.0 (0x73fbc000)
--> mx-uart.1 (0x73fc0000).  That is why we have the following message
of of_platform_bus_probe().

of_platform_bus_probe()
 starting at: /
  match: /soc
   create child: /soc/tzic
  match: /spba@70000000
   create child: /spba@70000000/imx-uart@C000
  match: /aips@73f00000
   create child: /aips@73f00000/imx-uart@BC000
   create child: /aips@73f00000/imx-uart@C0000
  match: /aips@83f00000
   create child: /aips@83f00000/fec@EC000

That is to say imx-uart.2 will get probed as the first one before
imx-uart.0.  Meanwhile, the '[PATCH 3/3] serial/imx: parse from device
tree support' assumes it's the usual order, imx-uart.0 --> imx-uart.1
--> imx-uart.2.

======
+#ifdef CONFIG_OF
+static int serial_imx_probe_dt(struct imx_port *sport,
+               struct platform_device *pdev)
+{
+       struct device_node *node = pdev->dev.of_node;
+       static int line;
+
+       if (!node)
+               return -ENODEV;
+
+       if (of_get_property(node, "rts-cts", NULL))
+               sport->have_rtscts = 1;
+
+#ifdef CONFIG_IRDA
+       if (of_get_property(node, "irda", NULL))
+               sport->use_irda = 1;
+#endif
+       sport->port.line = line++;
+
+       return 0;
+}
+#else
+static int serial_imx_probe_dt(struct imx_port *sport,
+               struct platform_device *pdev)
+{
+       return -ENODEV;
+}
+#endif
+
[...]
@@ -1236,6 +1288,12 @@ static int serial_imx_probe(struct platform_device *pdev)
       if (!sport)
               return -ENOMEM;

+       ret = serial_imx_probe_dt(sport, pdev);
+       if (ret == -ENODEV)
+               ret = serial_imx_probe_pdata(sport, pdev);
+       if (ret)
+               goto free;
+
======

That's probably we have to tell console=ttymxc1 in bootargs, however
ttymxc0 hardware is actually being used.

> +
> +       soc {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x0 0xffffffff>;
> +
> +               tzic {
> +                       #address-cells = <0x0>;
> +                       #interrupt-cells = <0x1>;
> +                       interrupt-controller;
> +                       reg = <0xe0000000 0x1000>;
> +                       compatible = "fsl,tzic";
> +                       device_type = "tzic";
> +                       phandle = <0x1>;
> +               };
> +       };
> +
> +       clocks {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               uart_clk0: uart@0 {
> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.0";
> +               };
> +
> +               uart_clk1: uart@1{
> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.1";
> +               };
> +
> +               uart_clk2: uart@2{
> +                       compatible = "clock";
> +                       clock-outputs = "imx-uart.2";
> +               };
> +
> +               fec_clk: @0{
> +                       compatible = "clock";
> +                       clock-outputs = "fec.0";
> +               };
> +       };
> +
> +       spba@70000000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x70000000 0x100000>;
> +
> +               imx-uart@C000 {
> +                       compatible = "imx-uart";
> +                       reg = <0xc000 0x1000>;
> +                       interrupts = <0x21>;
> +                       rts-cts;
> +                       uart-clock = < &uart_clk2 >, "uart";
> +               };
> +       };
> +
> +       aips@73f00000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x73f00000 0x100000>;
> +
> +               imx-uart@BC000 {
> +                       compatible = "imx-uart";
> +                       reg = <0xbc000 0x1000>;
> +                       interrupts = <0x1f>;
> +                       rts-cts;
> +                       uart-clock = < &uart_clk0 >, "uart";
> +               };
> +
> +               imx-uart@C0000 {
> +                       compatible = "imx-uart";
> +                       reg = <0xc0000 0x1000>;
> +                       interrupts = <0x20>;
> +                       rts-cts;
> +                       uart-clock = <&uart_clk1>, "uart";
> +               };
> +       };
> +
> +       aips@83f00000 {
> +               #address-cells = <0x1>;
> +               #size-cells = <0x1>;
> +               device_type = "soc";
> +               compatible = "simple-bus";
> +               ranges = <0x0 0x83f00000 0x100000>;
> +
> +               fec@EC000 {
> +                       compatible = "fec";
> +                       reg = <0xec000 0x1000>;
> +                       interrupts = <0x57>;
> +                       fec_clk-clock = < &fec_clk >, "fec";
> +               };
> +       };
> +};
> --
> 1.7.0.4
>
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>



-- 
Regards,
Shawn

[PATCH 3/3] serial/imx: parse from device tree support

[Jason Liu]

Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/tty/serial/imx.c |   81 ++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
old mode 100644
new mode 100755
index dfcf4b1..3388599
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -52,6 +52,10 @@
 #include <asm/irq.h>
 #include <mach/hardware.h>
 #include <mach/imx-uart.h>
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/of_address.h>
+#endif /* CONFIG_OF */
 
 /* Register definitions */
 #define URXD0 0x0  /* Receiver Register */
@@ -1224,6 +1228,54 @@ static int serial_imx_resume(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static int serial_imx_probe_dt(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	static int line;
+
+	if (!node)
+		return -ENODEV;
+
+	if (of_get_property(node, "rts-cts", NULL))
+		sport->have_rtscts = 1;
+
+#ifdef CONFIG_IRDA
+	if (of_get_property(node, "irda", NULL))
+		sport->use_irda = 1;
+#endif
+	sport->port.line = line++;
+
+	return 0;
+}
+#else
+static int serial_imx_probe_dt(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#endif
+
+static int serial_imx_probe_pdata(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	struct imxuart_platform_data *pdata = pdev->dev.platform_data;
+
+	if (!pdata)
+		return 0;
+
+	if (pdata->flags & IMXUART_HAVE_RTSCTS)
+		sport->have_rtscts = 1;
+
+#ifdef CONFIG_IRDA
+	if (pdata->flags & IMXUART_IRDA)
+		sport->use_irda = 1;
+#endif
+	sport->port.line = pdev->id;
+
+	return 0;
+}
 static int serial_imx_probe(struct platform_device *pdev)
 {
 	struct imx_port *sport;
@@ -1236,6 +1288,12 @@ static int serial_imx_probe(struct platform_device *pdev)
 	if (!sport)
 		return -ENOMEM;
 
+	ret = serial_imx_probe_dt(sport, pdev);
+	if (ret == -ENODEV)
+		ret = serial_imx_probe_pdata(sport, pdev);
+	if (ret)
+		goto free;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		ret = -ENODEV;
@@ -1260,7 +1318,6 @@ static int serial_imx_probe(struct platform_device *pdev)
 	sport->port.fifosize = 32;
 	sport->port.ops = &imx_pops;
 	sport->port.flags = UPF_BOOT_AUTOCONF;
-	sport->port.line = pdev->id;
 	init_timer(&sport->timer);
 	sport->timer.function = imx_timeout;
 	sport->timer.data     = (unsigned long)sport;
@@ -1274,17 +1331,13 @@ static int serial_imx_probe(struct platform_device *pdev)
 
 	sport->port.uartclk = clk_get_rate(sport->clk);
 
-	imx_ports[pdev->id] = sport;
+	if (imx_ports[sport->port.line]) {
+		ret = -EBUSY;
+		goto clkput;
+	}
+	imx_ports[sport->port.line] = sport;
 
 	pdata = pdev->dev.platform_data;
-	if (pdata && (pdata->flags & IMXUART_HAVE_RTSCTS))
-		sport->have_rtscts = 1;
-
-#ifdef CONFIG_IRDA
-	if (pdata && (pdata->flags & IMXUART_IRDA))
-		sport->use_irda = 1;
-#endif
-
 	if (pdata && pdata->init) {
 		ret = pdata->init(pdev);
 		if (ret)
@@ -1336,6 +1389,11 @@ static int serial_imx_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id imx_uart_matches[] = {
+	{ .compatible = "imx-uart" },
+	{},
+};
+
 static struct platform_driver serial_imx_driver = {
 	.probe		= serial_imx_probe,
 	.remove		= serial_imx_remove,
@@ -1345,6 +1403,9 @@ static struct platform_driver serial_imx_driver = {
 	.driver		= {
 		.name	= "imx-uart",
 		.owner	= THIS_MODULE,
+#if defined(CONFIG_OF)
+		.of_match_table = imx_uart_matches,
+#endif
 	},
 };
 
-- 
1.7.0.4

Re: [PATCH 3/3] serial/imx: parse from device tree support

[Arnd Bergmann]

Hi Jason,

The patch looks good, but I noticed a few details that can be improved.

On Friday 18 February 2011, Jason Liu wrote:
> index dfcf4b1..3388599
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -52,6 +52,10 @@
>  #include <asm/irq.h>
>  #include <mach/hardware.h>
>  #include <mach/imx-uart.h>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#endif /* CONFIG_OF */
>  
>  /* Register definitions */
>  #define URXD0 0x0  /* Receiver Register */

There is generally no need to enclose any header incudes in #ifdef.
If there is a problem in the header when included without CONFIG_OF set,
that should be fixed in the header.

> @@ -1224,6 +1228,54 @@ static int serial_imx_resume(struct platform_device *dev)
>         return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static int serial_imx_probe_dt(struct imx_port *sport,
> +               struct platform_device *pdev)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       static int line;
> +
> +       if (!node)
> +               return -ENODEV;
> +
> +       if (of_get_property(node, "rts-cts", NULL))
> +               sport->have_rtscts = 1;
> +
> +#ifdef CONFIG_IRDA
> +       if (of_get_property(node, "irda", NULL))
> +               sport->use_irda = 1;
> +#endif
> +       sport->port.line = line++;
> +
> +       return 0;
> +}
> +#else
> +static int serial_imx_probe_dt(struct imx_port *sport,
> +               struct platform_device *pdev)
> +{
> +       return -ENODEV;
> +}
> +#endif

Similarly, there is no need to have the #ifdef CONFIG_IRDA here.
This one takes up a few bytes of probe code, but otherwise makes
the code more readable.

	Arnd

Re: [PATCH 3/3] serial/imx: parse from device tree support

[Shawn Guo]

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/tty/serial/imx.c |   81 ++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> old mode 100644
> new mode 100755

The file mode was changed to have 'x'?

-- 
Regards,
Shawn

Re: [PATCH 3/3] serial/imx: parse from device tree support

[Jason Hui]

Hi, Arnd, Shawn,

On Mon, Feb 28, 2011 at 6:35 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Signed-off-by: Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>  drivers/tty/serial/imx.c |   81 ++++++++++++++++++++++++++++++++++++++++------
>>  1 files changed, 71 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> old mode 100644
>> new mode 100755
>
> The file mode was changed to have 'x'?

Thanks for the review comments and I will come up with V2 patch soon.

BR,
Jason

>
> --
> Regards,
> Shawn
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

Re: [PATCH 0/3] Add MX51 basic DT support

[Rob Herring]

Jason,

On 02/18/2011 02:12 AM, Jason Liu wrote:
> This patchset adds FSL mx51 device tree support. This
> is based on
>
> git://git.secretlab.ca/git/linux-2.6 devicetree/test
>
> This patch has been tested on MX51 babbage board and can
> boot up succesfully to linux console with DT enabled.
>
> Jason Liu (3):
>    arm/dt: add basic mx51 device tree support
>    arm/dt: add very basic dts file for babbage board

This is clearly based on the initial version I wrote. Can you give me 
credit for it.

>    serial/imx: parse from device tree support

Jeremy Kerr wrote the initial version of this although it is quite 
heavily changed.

Original versions are here:

http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git;a=shortlog;h=refs/heads/mx51

Rob

RE: [PATCH 0/3] Add MX51 basic DT support

[Liu Hui-R64343]

Hi, Rob,

Best Regards,
Jason Liu


>-----Original Message-----
>From: Rob Herring [mailto:robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
>Sent: Friday, February 18, 2011 11:13 PM
>To: Liu Hui-R64343; Jeremy Kerr
>Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org;
>patches-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org
>Subject: Re: [PATCH 0/3] Add MX51 basic DT support
>
>Jason,
>
>On 02/18/2011 02:12 AM, Jason Liu wrote:
>> This patchset adds FSL mx51 device tree support. This is based on
>>
>> git://git.secretlab.ca/git/linux-2.6 devicetree/test
>>
>> This patch has been tested on MX51 babbage board and can boot up
>> succesfully to linux console with DT enabled.
>>
>> Jason Liu (3):
>>    arm/dt: add basic mx51 device tree support
>>    arm/dt: add very basic dts file for babbage board
>
>This is clearly based on the initial version I wrote. Can you give me credit for
>it.

OK, Sure,
>
>>    serial/imx: parse from device tree support
>
>Jeremy Kerr wrote the initial version of this although it is quite heavily
>changed.

Yes,  Will add it to the commit log.
>
>Original versions are here:
>
>http://kernel.ubuntu.com/git?p=jk/dt/linux-
>2.6.git;a=shortlog;h=refs/heads/mx51
>
>Rob

Re: [PATCH 0/3] Add MX51 basic DT support

[Shawn Guo]

Hi Jason,

On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> This patchset adds FSL mx51 device tree support. This
> is based on
>
> git://git.secretlab.ca/git/linux-2.6 devicetree/test
>
> This patch has been tested on MX51 babbage board and can
> boot up succesfully to linux console with DT enabled.
>
Is this supposed to see console login prompt or just console?  I'm
seeing fec related nodes in babbage.dts, but I can not get fec up
running.  Did I miss anything or fec is not with this patch set yet?

-- 
Regards,
Shawn

Re: [PATCH 0/3] Add MX51 basic DT support

[Jason Hui]

Hi, Shawn,

On Mon, Feb 21, 2011 at 4:17 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Jason,
>
> On 18 February 2011 16:12, Jason Liu <r64343-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> This patchset adds FSL mx51 device tree support. This
>> is based on
>>
>> git://git.secretlab.ca/git/linux-2.6 devicetree/test
>>
>> This patch has been tested on MX51 babbage board and can
>> boot up succesfully to linux console with DT enabled.
>>
> Is this supposed to see console login prompt or just console?  I'm
> seeing fec related nodes in babbage.dts, but I can not get fec up
> running.  Did I miss anything or fec is not with this patch set yet?

FEC is not support currently, I just use initramfs to boot kernel to console.

>
> --
> Regards,
> Shawn
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>