On 18 February 2011 16:12, Jason Liu r64343@freescale.com wrote:

Signed-off-by: Jason Liu r64343@freescale.com

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

On Mon, Feb 21, 2011 at 2:46 AM, Shawn Guo shawn.guo@linaro.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@freescale.com wrote:

...

Signed-off-by: Jason Liu r64343@freescale.com

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@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev

On 18 February 2011 16:12, Jason Liu r64343@freescale.com wrote:

Signed-off-by: Jason Liu r64343@freescale.com

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@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev

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