[PATCH 0/3] Add basic device support for imx51 babbage

[PATCH 0/3] Add basic device support for imx51 babbage

[Shawn Guo]

This patch set adds the basic device tree support for imx51 babbage
board.  With uart and fec dt support added, the dt kernel can boot
into console with nfs root on babbage, so that we get a good base
to start playing dt and converting further drivers to use dt on
imx51.

It creates the platform imx51-dt for using the device tree, and
leaves existing board support files alone, so nothing should be
broken.  The plan is to add stuff step by step into imx51-dt to
get it support every existing imx51 boards, and then remove the
existing board files.

It works against Linus tree plus the dt infrastructure patches
posted by Grant Likely as part of the following series.

  [RFC PATCH 00/11] Full device tree support for ARM Versatile

Comments are appreciated.

Shawn Guo (3):
      serial/imx: add device tree support
      net/fec: add device tree support
      ARM: mx5: add basic device tree support for imx51 babbage

 Documentation/devicetree/bindings/net/fsl-fec.txt  |   14 +++
 .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
 arch/arm/boot/dts/imx51-babbage.dts                |   89 ++++++++++++++++++++
 arch/arm/mach-mx5/Kconfig                          |    8 ++
 arch/arm/mach-mx5/Makefile                         |    1 +
 arch/arm/mach-mx5/imx51-dt.c                       |   70 +++++++++++++++
 drivers/net/fec.c                                  |   28 ++++++
 drivers/tty/serial/imx.c                           |   81 ++++++++++++++++--
 8 files changed, 302 insertions(+), 10 deletions(-)

[PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

It adds device tree data parsing support for imx tty/serial driver.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
Signed-off-by: Jason Liu <jason.hui@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
 drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
 2 files changed, 92 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt

diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
new file mode 100644
index 0000000..7648e17
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
@@ -0,0 +1,21 @@
+* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
+
+Required properties:
+- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
+- reg : address and length of the register set for the device
+- interrupts : should contain uart interrupt
+- id : should be the port ID defined by soc
+
+Optional properties:
+- fsl,has-rts-cts : indicate it has rts-cts
+- fsl,irda-mode : support irda mode
+
+Example:
+
+uart@73fbc000 {
+	compatible = "fsl,imx51-uart", "fsl,imx-uart";
+	reg = <0x73fbc000 0x4000>;
+	interrupts = <31>;
+	id = <1>;
+	fsl,has-rts-cts;
+};
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a544731..2769353 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -45,6 +45,8 @@
 #include <linux/delay.h>
 #include <linux/rational.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1223,6 +1225,63 @@ 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;
+	const __be32 *line;
+
+	if (!node)
+		return -ENODEV;
+
+	line = of_get_property(node, "id", NULL);
+	if (!line)
+		return -ENODEV;
+
+	sport->port.line = be32_to_cpup(line) - 1;
+
+	if (of_get_property(node, "fsl,has-rts-cts", NULL))
+		sport->have_rtscts = 1;
+
+	if (of_get_property(node, "fsl,irda-mode", NULL))
+		sport->use_irda = 1;
+
+	return 0;
+}
+
+static struct of_device_id imx_uart_dt_ids[] = {
+	{ .compatible = "fsl,imx-uart" },
+	{},
+};
+#else
+static int serial_imx_probe_dt(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#define imx_uart_dt_ids NULL
+#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;
+
+	if (pdata->flags & IMXUART_IRDA)
+		sport->use_irda = 1;
+
+	sport->port.line = pdev->id;
+
+	return 0;
+}
+
 static int serial_imx_probe(struct platform_device *pdev)
 {
 	struct imx_port *sport;
@@ -1235,6 +1294,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;
@@ -1259,7 +1324,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;
@@ -1273,17 +1337,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)
@@ -1344,6 +1404,7 @@ static struct platform_driver serial_imx_driver = {
 	.driver		= {
 		.name	= "imx-uart",
 		.owner	= THIS_MODULE,
+		.of_match_table = imx_uart_dt_ids,
 	},
 };
 
-- 
1.7.4.1

[PATCH 2/3] net/fec: add device tree support

[Shawn Guo]

It adds device tree data parsing support for fec driver.

Signed-off-by: Jason Liu <jason.hui@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: David S. Miller <davem@davemloft.net>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |   14 ++++++++++
 drivers/net/fec.c                                 |   28 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
new file mode 100644
index 0000000..705111d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -0,0 +1,14 @@
+* Freescale Fast Ethernet Controller (FEC)
+
+Required properties:
+- compatible : should be "fsl,<soc>-fec", "fsl,fec"
+- reg : address and length of the register set for the device
+- interrupts : should contain fec interrupt
+
+Example:
+
+fec@83fec000 {
+	compatible = "fsl,imx51-fec", "fsl,fec";
+	reg = <0x83fec000 0x4000>;
+	interrupts = <87>;
+};
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 885d8ba..ef3d175 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -44,6 +44,8 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 #include <linux/fec.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <asm/cacheflush.h>
 
@@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = {
 	{ }
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id fec_dt_ids[] = {
+	{ .compatible = "fsl,fec", .data = &fec_devtype[0], },
+	{},
+};
+
+static const struct of_device_id *
+fec_get_of_device_id(struct platform_device *pdev)
+{
+	return of_match_device(fec_dt_ids, &pdev->dev);
+}
+#else
+#define fec_dt_ids NULL
+static inline struct of_device_id *
+fec_get_of_device_id(struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static unsigned char macaddr[ETH_ALEN];
 module_param_array(macaddr, byte, NULL, 0);
 MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
@@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	int i, irq, ret = 0;
 	struct resource *r;
+	const struct of_device_id *of_id;
+
+	of_id = fec_get_of_device_id(pdev);
+	if (of_id)
+		pdev->id_entry = (struct platform_device_id *) of_id->data;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
@@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = {
 #ifdef CONFIG_PM
 		.pm	= &fec_pm_ops,
 #endif
+		.of_match_table = fec_dt_ids,
 	},
 	.id_table = fec_devtype,
 	.probe	= fec_probe,
-- 
1.7.4.1

[PATCH 3/3] ARM: mx5: add basic device tree support for imx51 babbage

[Shawn Guo]

This patch adds the i.mx51 dt platform with uart and fec support.
It also adds the dts file imx51 babbage, so that we can have a dt
kernel on babbage booting into console with nfs root.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx51-babbage.dts |   89 +++++++++++++++++++++++++++++++++++
 arch/arm/mach-mx5/Kconfig           |    8 +++
 arch/arm/mach-mx5/Makefile          |    1 +
 arch/arm/mach-mx5/imx51-dt.c        |   70 +++++++++++++++++++++++++++
 4 files changed, 168 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx51-babbage.dts
 create mode 100644 arch/arm/mach-mx5/imx51-dt.c

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
new file mode 100644
index 0000000..7976932
--- /dev/null
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -0,0 +1,89 @@
+/*
+ * 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
+ */
+
+/dts-v1/;
+/include/ "skeleton.dtsi"
+
+/ {
+	model = "Freescale i.MX51 Babbage";
+	compatible = "fsl,imx51-babbage", "fsl,imx51";
+	interrupt-parent = <&tzic>;
+
+	chosen {
+		bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
+	};
+
+	memory {
+		reg = <0x90000000 0x20000000>;
+	};
+
+	tzic: tz-interrupt-controller@e0000000 {
+		compatible = "fsl,imx51-tzic", "fsl,tzic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0xe0000000 0x4000>;
+	};
+
+	aips@70000000 { /* aips-1 */
+		compatible = "fsl,aips-bus", "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x70000000 0x10000000>;
+		ranges;
+
+		spba {
+			compatible = "fsl,spba-bus", "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x70000000 0x40000>;
+			ranges;
+
+			uart@7000c000 {
+				compatible = "fsl,imx51-uart", "fsl,imx-uart";
+				reg = <0x7000c000 0x4000>;
+				interrupts = <33>;
+				id = <3>;
+				fsl,has-rts-cts;
+			};
+		};
+
+		uart@73fbc000 {
+			compatible = "fsl,imx51-uart", "fsl,imx-uart";
+			reg = <0x73fbc000 0x4000>;
+			interrupts = <31>;
+			id = <1>;
+			fsl,has-rts-cts;
+		};
+
+		uart@73fc0000 {
+			compatible = "fsl,imx51-uart", "fsl,imx-uart";
+			reg = <0x73fc0000 0x4000>;
+			interrupts = <32>;
+			id = <2>;
+			fsl,has-rts-cts;
+		};
+	};
+
+	aips@80000000 {	/* aips-2 */
+		compatible = "fsl,aips-bus", "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80000000 0x10000000>;
+		ranges;
+
+		fec@83fec000 {
+			compatible = "fsl,imx51-fec", "fsl,fec";
+			reg = <0x83fec000 0x4000>;
+			interrupts = <87>;
+		};
+	};
+};
diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index 799fbc4..8bdd0c4 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -62,6 +62,14 @@ endif # ARCH_MX50_SUPPORTED
 if ARCH_MX51
 comment "i.MX51 machines:"
 
+config MACH_IMX51_DT
+	bool "Support i.MX51 platforms from device tree"
+	select SOC_IMX51
+	select USE_OF
+	help
+	  Include support for Freescale i.MX51 based platforms
+	  using the device tree for discovery
+
 config MACH_MX51_BABBAGE
 	bool "Support MX51 BABBAGE platforms"
 	select SOC_IMX51
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0b9338c..47b483f 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -7,6 +7,7 @@ obj-y   := cpu.o mm.o clock-mx51-mx53.o devices.o ehci.o system.o
 obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
 
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpu_op-mx51.o
+obj-$(CONFIG_MACH_IMX51_DT) += imx51-dt.o
 obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
 obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
 obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o
diff --git a/arch/arm/mach-mx5/imx51-dt.c b/arch/arm/mach-mx5/imx51-dt.c
new file mode 100644
index 0000000..8bfdb91
--- /dev/null
+++ b/arch/arm/mach-mx5/imx51-dt.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ * 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/irq.h>
+#include <linux/of_platform.h>
+
+#include <asm/mach/arch.h>
+#include <asm/mach/time.h>
+
+#include <mach/common.h>
+#include <mach/mx51.h>
+
+/*
+ * Lookup table for attaching a specific name and platform_data pointer to
+ * devices as they get created by of_platform_populate().  Ideally this table
+ * would not exist, but the current clock implementation depends on some devices
+ * having a specific name.
+ */
+static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART1_BASE_ADDR, "imx-uart.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART2_BASE_ADDR, "imx-uart.1", NULL),
+	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART3_BASE_ADDR, "imx-uart.2", NULL),
+	OF_DEV_AUXDATA("fsl,imx51-fec", MX51_FEC_BASE_ADDR, "fec.0", NULL),
+	{}
+};
+
+static const struct of_device_id tzic_of_match[] __initconst = {
+	{ .compatible = "fsl,imx51-tzic", },
+	{}
+};
+
+static void __init imx51_dt_init(void)
+{
+	irq_domain_generate_simple(tzic_of_match, MX51_TZIC_BASE_ADDR, 0);
+
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     imx51_auxdata_lookup, NULL);
+}
+
+static void __init imx51_timer_init(void)
+{
+	mx51_clocks_init(32768, 24000000, 22579200, 0);
+}
+
+static struct sys_timer imx51_timer = {
+	.init = imx51_timer_init,
+};
+
+static const char *imx51_dt_board_compat[] __initdata = {
+	"fsl,imx51-babbage",
+	NULL
+};
+
+DT_MACHINE_START(IMX51_DT, "Freescale i.MX51 (Device Tree Support)")
+	.map_io		= mx51_map_io,
+	.init_early	= imx51_init_early,
+	.init_irq	= mx51_init_irq,
+	.timer		= &imx51_timer,
+	.init_machine	= imx51_dt_init,
+	.dt_compat	= imx51_dt_board_compat,
+MACHINE_END
-- 
1.7.4.1

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
> It adds device tree data parsing support for imx tty/serial driver.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
>  2 files changed, 92 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> 
> diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> new file mode 100644
> index 0000000..7648e17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> @@ -0,0 +1,21 @@
> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> +
> +Required properties:
> +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"

I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"

It's better to anchor these things on real silicon, or a real ip block
specification rather than something pseudo-generic.  Subsequent chips,
like the imx53, should simply claim compatibility with the older
fsl,imx51-uart.

(in essence, "fsl,imx51-uart" becomes the generic string without the
downside of having no obvious recourse when new silicon shows up that
is an imx part, but isn't compatible with the imx51 uart.

> +- reg : address and length of the register set for the device
> +- interrupts : should contain uart interrupt
> +- id : should be the port ID defined by soc
> +
> +Optional properties:
> +- fsl,has-rts-cts : indicate it has rts-cts
> +- fsl,irda-mode : support irda mode
> +
> +Example:
> +
> +uart@73fbc000 {
> +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +	reg = <0x73fbc000 0x4000>;
> +	interrupts = <31>;
> +	id = <1>;
> +	fsl,has-rts-cts;
> +};
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index a544731..2769353 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -45,6 +45,8 @@
>  #include <linux/delay.h>
>  #include <linux/rational.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -1223,6 +1225,63 @@ 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;
> +	const __be32 *line;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	line = of_get_property(node, "id", NULL);
> +	if (!line)
> +		return -ENODEV;
> +
> +	sport->port.line = be32_to_cpup(line) - 1;

Hmmm, I really would like to be rid of this.  Instead, if uarts must
be enumerated, the driver should look for a /aliases/uart* property
that matches the of_node.  Doing it that way is already established in
the OpenFirmware documentation, and it ensures there are no overlaps
in the global namespace.

We do need some infrastructure to make that easier though.  Would you
have time to help put that together?

> +
> +	if (of_get_property(node, "fsl,has-rts-cts", NULL))
> +		sport->have_rtscts = 1;
> +
> +	if (of_get_property(node, "fsl,irda-mode", NULL))
> +		sport->use_irda = 1;
> +
> +	return 0;
> +}
> +
> +static struct of_device_id imx_uart_dt_ids[] = {
> +	{ .compatible = "fsl,imx-uart" },
> +	{},
> +};
> +#else
> +static int serial_imx_probe_dt(struct imx_port *sport,
> +		struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +#define imx_uart_dt_ids NULL
> +#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;
> +
> +	if (pdata->flags & IMXUART_IRDA)
> +		sport->use_irda = 1;
> +
> +	sport->port.line = pdev->id;
> +
> +	return 0;
> +}
> +
>  static int serial_imx_probe(struct platform_device *pdev)
>  {
>  	struct imx_port *sport;
> @@ -1235,6 +1294,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;
> @@ -1259,7 +1324,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;
> @@ -1273,17 +1337,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)
> @@ -1344,6 +1404,7 @@ static struct platform_driver serial_imx_driver = {
>  	.driver		= {
>  		.name	= "imx-uart",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = imx_uart_dt_ids,
>  	},
>  };
>  
> -- 
> 1.7.4.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/3] serial/imx: add device tree support

[Arnd Bergmann]

On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
>
> +Required properties:
> +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> +- reg : address and length of the register set for the device
> +- interrupts : should contain uart interrupt
> +- id : should be the port ID defined by soc
> +
> +Optional properties:
> +- fsl,has-rts-cts : indicate it has rts-cts
> +- fsl,irda-mode : support irda mode
> +
> +Example:
> +
> +uart@73fbc000 {
> +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +	reg = <0x73fbc000 0x4000>;
> +	interrupts = <31>;
> +	id = <1>;
> +	fsl,has-rts-cts;
> +};

Should this also support the "clock-frequency" property that 8250-style
serial ports support [1]?

For the flow-control properties, should we name that more generic? The
same property certainly makes sense for other serial-ports if it does
here. OTOH, I'm not sure it's actually reliable, because it also depends
on whether the other side of the connection and the cable support hw flow
control.


	Arnd

[1] http://playground.sun.com/1275/bindings/devices/html/serial-1_0d.html#HDR9

Re: [PATCH 2/3] net/fec: add device tree support

[Grant Likely]

On Sat, Jun 18, 2011 at 11:19:13PM +0800, Shawn Guo wrote:
> It adds device tree data parsing support for fec driver.
> 
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt |   14 ++++++++++
>  drivers/net/fec.c                                 |   28 +++++++++++++++++++++
>  2 files changed, 42 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> new file mode 100644
> index 0000000..705111d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -0,0 +1,14 @@
> +* Freescale Fast Ethernet Controller (FEC)
> +
> +Required properties:
> +- compatible : should be "fsl,<soc>-fec", "fsl,fec"

Ditto to comment on last patch.  "fsl,fec" is to generic.
"fsl,imx51-soc" should be the generic value.

Otherwise looks okay to me, and I don't see any problem with queueing
it up for v3.1 with that change since it doesn't depend on any other
patches.

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

> +- reg : address and length of the register set for the device
> +- interrupts : should contain fec interrupt
> +
> +Example:
> +
> +fec@83fec000 {
> +	compatible = "fsl,imx51-fec", "fsl,fec";
> +	reg = <0x83fec000 0x4000>;
> +	interrupts = <87>;
> +};
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 885d8ba..ef3d175 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -44,6 +44,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/fec.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = {
>  	{ }
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id fec_dt_ids[] = {
> +	{ .compatible = "fsl,fec", .data = &fec_devtype[0], },
> +	{},
> +};
> +
> +static const struct of_device_id *
> +fec_get_of_device_id(struct platform_device *pdev)
> +{
> +	return of_match_device(fec_dt_ids, &pdev->dev);
> +}
> +#else
> +#define fec_dt_ids NULL
> +static inline struct of_device_id *
> +fec_get_of_device_id(struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static unsigned char macaddr[ETH_ALEN];
>  module_param_array(macaddr, byte, NULL, 0);
>  MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
>  	struct net_device *ndev;
>  	int i, irq, ret = 0;
>  	struct resource *r;
> +	const struct of_device_id *of_id;
> +
> +	of_id = fec_get_of_device_id(pdev);
> +	if (of_id)
> +		pdev->id_entry = (struct platform_device_id *) of_id->data;
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!r)
> @@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &fec_pm_ops,
>  #endif
> +		.of_match_table = fec_dt_ids,
>  	},
>  	.id_table = fec_devtype,
>  	.probe	= fec_probe,
> -- 
> 1.7.4.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/3] ARM: mx5: add basic device tree support for imx51 babbage

[Grant Likely]

On Sat, Jun 18, 2011 at 11:19:14PM +0800, Shawn Guo wrote:
> This patch adds the i.mx51 dt platform with uart and fec support.
> It also adds the dts file imx51 babbage, so that we can have a dt
> kernel on babbage booting into console with nfs root.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Looks like a good start to me.  I'll pick it up into devicetree/test
(probably on Monday).  Only think I see that needs changing is to
modify the compatible properties based on my comments on the first 2
patches.

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

g.

> ---
>  arch/arm/boot/dts/imx51-babbage.dts |   89 +++++++++++++++++++++++++++++++++++
>  arch/arm/mach-mx5/Kconfig           |    8 +++
>  arch/arm/mach-mx5/Makefile          |    1 +
>  arch/arm/mach-mx5/imx51-dt.c        |   70 +++++++++++++++++++++++++++
>  4 files changed, 168 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/boot/dts/imx51-babbage.dts
>  create mode 100644 arch/arm/mach-mx5/imx51-dt.c
> 
> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> new file mode 100644
> index 0000000..7976932
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx51-babbage.dts
> @@ -0,0 +1,89 @@
> +/*
> + * 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
> + */
> +
> +/dts-v1/;
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	model = "Freescale i.MX51 Babbage";
> +	compatible = "fsl,imx51-babbage", "fsl,imx51";
> +	interrupt-parent = <&tzic>;
> +
> +	chosen {
> +		bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
> +	};
> +
> +	memory {
> +		reg = <0x90000000 0x20000000>;
> +	};
> +
> +	tzic: tz-interrupt-controller@e0000000 {
> +		compatible = "fsl,imx51-tzic", "fsl,tzic";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0xe0000000 0x4000>;
> +	};
> +
> +	aips@70000000 { /* aips-1 */
> +		compatible = "fsl,aips-bus", "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x70000000 0x10000000>;
> +		ranges;
> +
> +		spba {
> +			compatible = "fsl,spba-bus", "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x70000000 0x40000>;
> +			ranges;
> +
> +			uart@7000c000 {
> +				compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +				reg = <0x7000c000 0x4000>;
> +				interrupts = <33>;
> +				id = <3>;
> +				fsl,has-rts-cts;
> +			};
> +		};
> +
> +		uart@73fbc000 {
> +			compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +			reg = <0x73fbc000 0x4000>;
> +			interrupts = <31>;
> +			id = <1>;
> +			fsl,has-rts-cts;
> +		};
> +
> +		uart@73fc0000 {
> +			compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +			reg = <0x73fc0000 0x4000>;
> +			interrupts = <32>;
> +			id = <2>;
> +			fsl,has-rts-cts;
> +		};
> +	};
> +
> +	aips@80000000 {	/* aips-2 */
> +		compatible = "fsl,aips-bus", "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80000000 0x10000000>;
> +		ranges;
> +
> +		fec@83fec000 {
> +			compatible = "fsl,imx51-fec", "fsl,fec";
> +			reg = <0x83fec000 0x4000>;
> +			interrupts = <87>;
> +		};
> +	};
> +};
> diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> index 799fbc4..8bdd0c4 100644
> --- a/arch/arm/mach-mx5/Kconfig
> +++ b/arch/arm/mach-mx5/Kconfig
> @@ -62,6 +62,14 @@ endif # ARCH_MX50_SUPPORTED
>  if ARCH_MX51
>  comment "i.MX51 machines:"
>  
> +config MACH_IMX51_DT
> +	bool "Support i.MX51 platforms from device tree"
> +	select SOC_IMX51
> +	select USE_OF
> +	help
> +	  Include support for Freescale i.MX51 based platforms
> +	  using the device tree for discovery
> +
>  config MACH_MX51_BABBAGE
>  	bool "Support MX51 BABBAGE platforms"
>  	select SOC_IMX51
> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
> index 0b9338c..47b483f 100644
> --- a/arch/arm/mach-mx5/Makefile
> +++ b/arch/arm/mach-mx5/Makefile
> @@ -7,6 +7,7 @@ obj-y   := cpu.o mm.o clock-mx51-mx53.o devices.o ehci.o system.o
>  obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
>  
>  obj-$(CONFIG_CPU_FREQ_IMX)    += cpu_op-mx51.o
> +obj-$(CONFIG_MACH_IMX51_DT) += imx51-dt.o
>  obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
>  obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
>  obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o
> diff --git a/arch/arm/mach-mx5/imx51-dt.c b/arch/arm/mach-mx5/imx51-dt.c
> new file mode 100644
> index 0000000..8bfdb91
> --- /dev/null
> +++ b/arch/arm/mach-mx5/imx51-dt.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + * 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/irq.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/mach/arch.h>
> +#include <asm/mach/time.h>
> +
> +#include <mach/common.h>
> +#include <mach/mx51.h>
> +
> +/*
> + * Lookup table for attaching a specific name and platform_data pointer to
> + * devices as they get created by of_platform_populate().  Ideally this table
> + * would not exist, but the current clock implementation depends on some devices
> + * having a specific name.
> + */
> +static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART1_BASE_ADDR, "imx-uart.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART2_BASE_ADDR, "imx-uart.1", NULL),
> +	OF_DEV_AUXDATA("fsl,imx51-uart", MX51_UART3_BASE_ADDR, "imx-uart.2", NULL),
> +	OF_DEV_AUXDATA("fsl,imx51-fec", MX51_FEC_BASE_ADDR, "fec.0", NULL),
> +	{}
> +};
> +
> +static const struct of_device_id tzic_of_match[] __initconst = {
> +	{ .compatible = "fsl,imx51-tzic", },
> +	{}
> +};
> +
> +static void __init imx51_dt_init(void)
> +{
> +	irq_domain_generate_simple(tzic_of_match, MX51_TZIC_BASE_ADDR, 0);
> +
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     imx51_auxdata_lookup, NULL);
> +}
> +
> +static void __init imx51_timer_init(void)
> +{
> +	mx51_clocks_init(32768, 24000000, 22579200, 0);
> +}
> +
> +static struct sys_timer imx51_timer = {
> +	.init = imx51_timer_init,
> +};
> +
> +static const char *imx51_dt_board_compat[] __initdata = {
> +	"fsl,imx51-babbage",
> +	NULL
> +};
> +
> +DT_MACHINE_START(IMX51_DT, "Freescale i.MX51 (Device Tree Support)")
> +	.map_io		= mx51_map_io,
> +	.init_early	= imx51_init_early,
> +	.init_irq	= mx51_init_irq,
> +	.timer		= &imx51_timer,
> +	.init_machine	= imx51_dt_init,
> +	.dt_compat	= imx51_dt_board_compat,
> +MACHINE_END
> -- 
> 1.7.4.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:
> On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
> >
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> > +- reg : address and length of the register set for the device
> > +- interrupts : should contain uart interrupt
> > +- id : should be the port ID defined by soc
> > +
> > +Optional properties:
> > +- fsl,has-rts-cts : indicate it has rts-cts
> > +- fsl,irda-mode : support irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > +	reg = <0x73fbc000 0x4000>;
> > +	interrupts = <31>;
> > +	id = <1>;
> > +	fsl,has-rts-cts;
> > +};
> 
> Should this also support the "clock-frequency" property that 8250-style
> serial ports support [1]?
> 
> For the flow-control properties, should we name that more generic? The
> same property certainly makes sense for other serial-ports if it does
> here. OTOH, I'm not sure it's actually reliable, because it also depends
> on whether the other side of the connection and the cable support hw flow
> control.

I'd like to see a few use cases before defining a common property.
That said, has-rts-cts does sound like a useful generic property.
Or maybe named "uart-has-rts-cts" to make it clear that it is a uart
specific binding?

g.

> 
> 
> 	Arnd
> 
> [1] http://playground.sun.com/1275/bindings/devices/html/serial-1_0d.html#HDR9
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] Add basic device support for imx51 babbage

[Grant Likely]

On Sat, Jun 18, 2011 at 11:19:11PM +0800, Shawn Guo wrote:
> This patch set adds the basic device tree support for imx51 babbage
> board.  With uart and fec dt support added, the dt kernel can boot
> into console with nfs root on babbage, so that we get a good base
> to start playing dt and converting further drivers to use dt on
> imx51.
> 
> It creates the platform imx51-dt for using the device tree, and
> leaves existing board support files alone, so nothing should be
> broken.  The plan is to add stuff step by step into imx51-dt to
> get it support every existing imx51 boards, and then remove the
> existing board files.
> 
> It works against Linus tree plus the dt infrastructure patches
> posted by Grant Likely as part of the following series.
> 
>   [RFC PATCH 00/11] Full device tree support for ARM Versatile
> 
> Comments are appreciated.
> 
> Shawn Guo (3):
>       serial/imx: add device tree support
>       net/fec: add device tree support
>       ARM: mx5: add basic device tree support for imx51 babbage

Good work!  Other than minor comments, this series looks awesome.

g.

> 
>  Documentation/devicetree/bindings/net/fsl-fec.txt  |   14 +++
>  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>  arch/arm/boot/dts/imx51-babbage.dts                |   89 ++++++++++++++++++++
>  arch/arm/mach-mx5/Kconfig                          |    8 ++
>  arch/arm/mach-mx5/Makefile                         |    1 +
>  arch/arm/mach-mx5/imx51-dt.c                       |   70 +++++++++++++++
>  drivers/net/fec.c                                  |   28 ++++++
>  drivers/tty/serial/imx.c                           |   81 ++++++++++++++++--
>  8 files changed, 302 insertions(+), 10 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] serial/imx: add device tree support

[Arnd Bergmann]

On Saturday 18 June 2011 18:26:55 Grant Likely wrote:
> On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:

> > Should this also support the "clock-frequency" property that 8250-style
> > serial ports support [1]?
> > 
> > For the flow-control properties, should we name that more generic? The
> > same property certainly makes sense for other serial-ports if it does
> > here. OTOH, I'm not sure it's actually reliable, because it also depends
> > on whether the other side of the connection and the cable support hw flow
> > control.
> 
> I'd like to see a few use cases before defining a common property.
> That said, has-rts-cts does sound like a useful generic property.
> Or maybe named "uart-has-rts-cts" to make it clear that it is a uart
> specific binding?
> 


Sounds ok to me.

	Arnd

Re: [PATCH 2/3] net/fec: add device tree support

[Arnd Bergmann]

On Saturday 18 June 2011 17:19:13 Shawn Guo wrote:
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> new file mode 100644
> index 0000000..705111d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -0,0 +1,14 @@
> +* Freescale Fast Ethernet Controller (FEC)
> +
> +Required properties:
> +- compatible : should be "fsl,<soc>-fec", "fsl,fec"
> +- reg : address and length of the register set for the device
> +- interrupts : should contain fec interrupt
> +
> +Example:
> +
> +fec@83fec000 {
> +	compatible = "fsl,imx51-fec", "fsl,fec";
> +	reg = <0x83fec000 0x4000>;
> +	interrupts = <87>;
> +};

How about also adding device_type="network" as required here, so you
inherit the attributes like "local-mac-address".

I would also suggest adding a call to of_get_mac_address() so you
can read the address out of the device tree when it is not configured
in hardware. Today, the driver relies on a module parameter or
platform_data on hardware with a mac address set.

The other information that is currently encoded in platform_data
is the phy mode. How about adding a property that enables RMII mode
when present?

	Arnd

Re: [PATCH 2/3] net/fec: add device tree support

[Grant Likely]

On Sat, Jun 18, 2011 at 12:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 18 June 2011 17:19:13 Shawn Guo wrote:
>> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> new file mode 100644
>> index 0000000..705111d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> @@ -0,0 +1,14 @@
>> +* Freescale Fast Ethernet Controller (FEC)
>> +
>> +Required properties:
>> +- compatible : should be "fsl,<soc>-fec", "fsl,fec"
>> +- reg : address and length of the register set for the device
>> +- interrupts : should contain fec interrupt
>> +
>> +Example:
>> +
>> +fec@83fec000 {
>> +     compatible = "fsl,imx51-fec", "fsl,fec";
>> +     reg = <0x83fec000 0x4000>;
>> +     interrupts = <87>;
>> +};
>
> How about also adding device_type="network" as required here, so you
> inherit the attributes like "local-mac-address".

local-mac-address should be used regardless.  "device_type" only makes
sense when a platform uses real OpenFirmware with the runtime services
api.  It should not be used with the flat tree.

> I would also suggest adding a call to of_get_mac_address() so you
> can read the address out of the device tree when it is not configured
> in hardware. Today, the driver relies on a module parameter or
> platform_data on hardware with a mac address set.

Yes, of_get_mac_address() is the right thing to do.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
> > It adds device tree data parsing support for imx tty/serial driver.
> > 
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
> >  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
> >  2 files changed, 92 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > new file mode 100644
> > index 0000000..7648e17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,21 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> 
> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
> 
> It's better to anchor these things on real silicon, or a real ip block
> specification rather than something pseudo-generic.  Subsequent chips,
> like the imx53, should simply claim compatibility with the older
> fsl,imx51-uart.
> 
> (in essence, "fsl,imx51-uart" becomes the generic string without the
> downside of having no obvious recourse when new silicon shows up that
> is an imx part, but isn't compatible with the imx51 uart.

Shouldn't that be the oldest SoC this core showed up? It might be an academic
question, but it would look a bit funny if mx27 got dt-support and would have a
imx51-uart? The first imx to have this core is the mx1. (Although there are
some cpu_is_mx1() calls used in the driver, but they are still available, or?)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
> > It adds device tree data parsing support for imx tty/serial driver.
> > 
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
> >  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
> >  2 files changed, 92 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > new file mode 100644
> > index 0000000..7648e17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,21 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> 
> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
> 
> It's better to anchor these things on real silicon, or a real ip block
> specification rather than something pseudo-generic.  Subsequent chips,
> like the imx53, should simply claim compatibility with the older
> fsl,imx51-uart.

It is a real IP block on all imx silicons (except imx23 and imx28
which are known as former stmp).

> 
> (in essence, "fsl,imx51-uart" becomes the generic string without the
> downside of having no obvious recourse when new silicon shows up that
> is an imx part, but isn't compatible with the imx51 uart.
> 
In this case, should imx1 the ancestor of imx family than imx51
becomes part of that generic string?  Claiming uart of imx1, imx21
and imx31 (senior than imx51) compatible with the imx51 uart seems
odd to me.

That said, IMO, "fsl,imx-uart" stands a real IP block specification
here and can be a perfect generic compatibility string to tell the
recourse of any imx silicon using this IP.

> > +- reg : address and length of the register set for the device
> > +- interrupts : should contain uart interrupt
> > +- id : should be the port ID defined by soc
> > +
> > +Optional properties:
> > +- fsl,has-rts-cts : indicate it has rts-cts
> > +- fsl,irda-mode : support irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > +	reg = <0x73fbc000 0x4000>;
> > +	interrupts = <31>;
> > +	id = <1>;
> > +	fsl,has-rts-cts;
> > +};
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index a544731..2769353 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -45,6 +45,8 @@
> >  #include <linux/delay.h>
> >  #include <linux/rational.h>
> >  #include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> >  
> >  #include <asm/io.h>
> >  #include <asm/irq.h>
> > @@ -1223,6 +1225,63 @@ 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;
> > +	const __be32 *line;
> > +
> > +	if (!node)
> > +		return -ENODEV;
> > +
> > +	line = of_get_property(node, "id", NULL);
> > +	if (!line)
> > +		return -ENODEV;
> > +
> > +	sport->port.line = be32_to_cpup(line) - 1;
> 
> Hmmm, I really would like to be rid of this.  Instead, if uarts must
> be enumerated, the driver should look for a /aliases/uart* property
> that matches the of_node.  Doing it that way is already established in
> the OpenFirmware documentation, and it ensures there are no overlaps
> in the global namespace.
> 

I just gave one more try to avoid using 'aliases', and you gave a
'no' again.  Now, I know how hard you are on this.  Okay, I start
thinking about your suggestion seriously :)

> We do need some infrastructure to make that easier though.  Would you
> have time to help put that together?
> 
Ok, I will give it a try.

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:
> On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
> >
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> > +- reg : address and length of the register set for the device
> > +- interrupts : should contain uart interrupt
> > +- id : should be the port ID defined by soc
> > +
> > +Optional properties:
> > +- fsl,has-rts-cts : indicate it has rts-cts
> > +- fsl,irda-mode : support irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > +	reg = <0x73fbc000 0x4000>;
> > +	interrupts = <31>;
> > +	id = <1>;
> > +	fsl,has-rts-cts;
> > +};
> 
> Should this also support the "clock-frequency" property that 8250-style
> serial ports support [1]?
> 
I would ignore it for a while until we have common clock api and
corresponding binding settled.  For now, I would have nothing clock
related parsed from device tree.

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Sat, Jun 18, 2011 at 10:26:55AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:
> > On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
> > >
> > > +Required properties:
> > > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> > > +- reg : address and length of the register set for the device
> > > +- interrupts : should contain uart interrupt
> > > +- id : should be the port ID defined by soc
> > > +
> > > +Optional properties:
> > > +- fsl,has-rts-cts : indicate it has rts-cts
> > > +- fsl,irda-mode : support irda mode
> > > +
> > > +Example:
> > > +
> > > +uart@73fbc000 {
> > > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > > +	reg = <0x73fbc000 0x4000>;
> > > +	interrupts = <31>;
> > > +	id = <1>;
> > > +	fsl,has-rts-cts;
> > > +};
> > 
> > Should this also support the "clock-frequency" property that 8250-style
> > serial ports support [1]?
> > 
> > For the flow-control properties, should we name that more generic? The
> > same property certainly makes sense for other serial-ports if it does
> > here. OTOH, I'm not sure it's actually reliable, because it also depends
> > on whether the other side of the connection and the cable support hw flow
> > control.
> 
> I'd like to see a few use cases before defining a common property.
> That said, has-rts-cts does sound like a useful generic property.
> Or maybe named "uart-has-rts-cts" to make it clear that it is a uart
> specific binding?
> 
I would keep the name as it is if you do not mind, since it's been
under uart node.

-- 
Regards,
Shawn

Re: [PATCH 2/3] net/fec: add device tree support

[Shawn Guo]

On Sat, Jun 18, 2011 at 10:22:20AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 11:19:13PM +0800, Shawn Guo wrote:
> > It adds device tree data parsing support for fec driver.
> > 
> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: David S. Miller <davem@davemloft.net>
> > ---
> >  Documentation/devicetree/bindings/net/fsl-fec.txt |   14 ++++++++++
> >  drivers/net/fec.c                                 |   28 +++++++++++++++++++++
> >  2 files changed, 42 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > new file mode 100644
> > index 0000000..705111d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -0,0 +1,14 @@
> > +* Freescale Fast Ethernet Controller (FEC)
> > +
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-fec", "fsl,fec"
> 
> Ditto to comment on last patch.  "fsl,fec" is to generic.
> "fsl,imx51-soc" should be the generic value.
> 

Ditto to the feedback on the last comment.  "fsl,imx51-fec" is not
a good one to be the compatibility string for imx27 and imx35 fec.

> Otherwise looks okay to me, and I don't see any problem with queueing
> it up for v3.1 with that change since it doesn't depend on any other
> patches.
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 

-- 
Regards,
Shawn

Re: [PATCH 2/3] net/fec: add device tree support

[Shawn Guo]

On Sat, Jun 18, 2011 at 08:27:22PM +0200, Arnd Bergmann wrote:
> On Saturday 18 June 2011 17:19:13 Shawn Guo wrote:
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > new file mode 100644
> > index 0000000..705111d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -0,0 +1,14 @@
> > +* Freescale Fast Ethernet Controller (FEC)
> > +
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-fec", "fsl,fec"
> > +- reg : address and length of the register set for the device
> > +- interrupts : should contain fec interrupt
> > +
> > +Example:
> > +
> > +fec@83fec000 {
> > +	compatible = "fsl,imx51-fec", "fsl,fec";
> > +	reg = <0x83fec000 0x4000>;
> > +	interrupts = <87>;
> > +};
> 
> How about also adding device_type="network" as required here, so you
> inherit the attributes like "local-mac-address".
> 
> I would also suggest adding a call to of_get_mac_address() so you
> can read the address out of the device tree when it is not configured
> in hardware. Today, the driver relies on a module parameter or
> platform_data on hardware with a mac address set.
> 
> The other information that is currently encoded in platform_data
> is the phy mode. How about adding a property that enables RMII mode
> when present?
> 
Ah, yes.  I missed that.  Will add support for local-mac-address and
phy-mode.  Thanks, Arnd.

-- 
Regards,
Shawn

Re: [PATCH 2/3] net/fec: add device tree support

[Greg Ungerer]

Hi Shawn,

On 06/19/2011 01:19 AM, Shawn Guo wrote:
> It adds device tree data parsing support for fec driver.
>
> Signed-off-by: Jason Liu<jason.hui@linaro.org>
> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
> Cc: David S. Miller<davem@davemloft.net>
> ---
>   Documentation/devicetree/bindings/net/fsl-fec.txt |   14 ++++++++++
>   drivers/net/fec.c                                 |   28 +++++++++++++++++++++
>   2 files changed, 42 insertions(+), 0 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
>
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> new file mode 100644
> index 0000000..705111d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -0,0 +1,14 @@
> +* Freescale Fast Ethernet Controller (FEC)
> +
> +Required properties:
> +- compatible : should be "fsl,<soc>-fec", "fsl,fec"
> +- reg : address and length of the register set for the device
> +- interrupts : should contain fec interrupt
> +
> +Example:
> +
> +fec@83fec000 {
> +	compatible = "fsl,imx51-fec", "fsl,fec";
> +	reg =<0x83fec000 0x4000>;
> +	interrupts =<87>;
> +};
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 885d8ba..ef3d175 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -44,6 +44,8 @@
>   #include<linux/platform_device.h>
>   #include<linux/phy.h>
>   #include<linux/fec.h>
> +#include<linux/of.h>
> +#include<linux/of_device.h>
>
>   #include<asm/cacheflush.h>
>
> @@ -78,6 +80,26 @@ static struct platform_device_id fec_devtype[] = {
>   	{ }
>   };
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id fec_dt_ids[] = {
> +	{ .compatible = "fsl,fec", .data =&fec_devtype[0], },
> +	{},
> +};
> +
> +static const struct of_device_id *
> +fec_get_of_device_id(struct platform_device *pdev)
> +{
> +	return of_match_device(fec_dt_ids,&pdev->dev);
> +}
> +#else
> +#define fec_dt_ids NULL
> +static inline struct of_device_id *
> +fec_get_of_device_id(struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>   static unsigned char macaddr[ETH_ALEN];
>   module_param_array(macaddr, byte, NULL, 0);
>   MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
>   	struct net_device *ndev;
>   	int i, irq, ret = 0;
>   	struct resource *r;
> +	const struct of_device_id *of_id;
> +
> +	of_id = fec_get_of_device_id(pdev);

fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This
use of it will break compilation when this is not defined.

Regards
Greg



> +	if (of_id)
> +		pdev->id_entry = (struct platform_device_id *) of_id->data;
>
>   	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!r)
> @@ -1531,6 +1558,7 @@ static struct platform_driver fec_driver = {
>   #ifdef CONFIG_PM
>   		.pm	=&fec_pm_ops,
>   #endif
> +		.of_match_table = fec_dt_ids,
>   	},
>   	.id_table = fec_devtype,
>   	.probe	= fec_probe,


-- 
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close,                            FAX:         +61 7 3891 3630
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [PATCH 2/3] net/fec: add device tree support

[Arnd Bergmann]

On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote:
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id fec_dt_ids[] = {
> > +     { .compatible = "fsl,fec", .data =&fec_devtype[0], },
> > +     {},
> > +};
> > +
> > +static const struct of_device_id *
> > +fec_get_of_device_id(struct platform_device *pdev)
> > +{
> > +     return of_match_device(fec_dt_ids,&pdev->dev);
> > +}
> > +#else
> > +#define fec_dt_ids NULL
> > +static inline struct of_device_id *
> > +fec_get_of_device_id(struct platform_device *pdev)
> > +{
> > +     return NULL;
> > +}
> > +#endif
> > +
> >   static unsigned char macaddr[ETH_ALEN];
> >   module_param_array(macaddr, byte, NULL, 0);
> >   MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> > @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
> >       struct net_device *ndev;
> >       int i, irq, ret = 0;
> >       struct resource *r;
> > +     const struct of_device_id *of_id;
> > +
> > +     of_id = fec_get_of_device_id(pdev);
> 
> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This
> use of it will break compilation when this is not defined.
> 


Why? Note the #else path defining an empty function.

	Arnd

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Sun, Jun 19, 2011 at 1:30 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
>> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
>> > It adds device tree data parsing support for imx tty/serial driver.
>> >
>> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
>> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
>> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> > ---
>> >  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>> >  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
>> >  2 files changed, 92 insertions(+), 10 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> > new file mode 100644
>> > index 0000000..7648e17
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> > @@ -0,0 +1,21 @@
>> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
>> > +
>> > +Required properties:
>> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
>>
>> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
>>
>> It's better to anchor these things on real silicon, or a real ip block
>> specification rather than something pseudo-generic.  Subsequent chips,
>> like the imx53, should simply claim compatibility with the older
>> fsl,imx51-uart.
>
> It is a real IP block on all imx silicons (except imx23 and imx28
> which are known as former stmp).
>
>>
>> (in essence, "fsl,imx51-uart" becomes the generic string without the
>> downside of having no obvious recourse when new silicon shows up that
>> is an imx part, but isn't compatible with the imx51 uart.
>>
> In this case, should imx1 the ancestor of imx family than imx51
> becomes part of that generic string?  Claiming uart of imx1, imx21
> and imx31 (senior than imx51) compatible with the imx51 uart seems
> odd to me.
>
> That said, IMO, "fsl,imx-uart" stands a real IP block specification
> here and can be a perfect generic compatibility string to tell the
> recourse of any imx silicon using this IP.

Yes, but which /version/ of the IP block?  Hardware designers are
notorious for changing hardware designs for newer silicon, sometimes
to add features, sometimes to fix bugs.  While I understand the
temptation to boil a compatible value down to a nice clean generic
string, doing so only works in a perfect world.  In the real world,
you still need to have some information about the specific
implementation.  I prefer this specifying it to the SoC name, but I've
been known to be convinced that specifying it to the ip-block name &
version in certain circumstances, like for IP blocks in an FPGA, or
some of the Freescale powerpc pXXXX SoCs which actually had an IP
block swapped out midway through the life of the chip.

Besides, encoding an soc or ip block version into the 'generic'
compatible values is not just good practice, it has *zero downside*.
That's the beauty of the compatible property semantics.  Any node can
claim compatibility with an existing device.  If no existing device
fits correctly, then the node simple does not claim compatibility.
Drivers can bind to any number of compatible strings, so it would be
just fine for the of_match_table list to include both "fsl,imx-21" and
"fsl,imx-51" (assuming that is the appropriate solution in this case).

>
>> > +- reg : address and length of the register set for the device
>> > +- interrupts : should contain uart interrupt
>> > +- id : should be the port ID defined by soc
>> > +
>> > +Optional properties:
>> > +- fsl,has-rts-cts : indicate it has rts-cts
>> > +- fsl,irda-mode : support irda mode
>> > +
>> > +Example:
>> > +
>> > +uart@73fbc000 {
>> > +   compatible = "fsl,imx51-uart", "fsl,imx-uart";
>> > +   reg = <0x73fbc000 0x4000>;
>> > +   interrupts = <31>;
>> > +   id = <1>;
>> > +   fsl,has-rts-cts;
>> > +};
>> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> > index a544731..2769353 100644
>> > --- a/drivers/tty/serial/imx.c
>> > +++ b/drivers/tty/serial/imx.c
>> > @@ -45,6 +45,8 @@
>> >  #include <linux/delay.h>
>> >  #include <linux/rational.h>
>> >  #include <linux/slab.h>
>> > +#include <linux/of.h>
>> > +#include <linux/of_address.h>
>> >
>> >  #include <asm/io.h>
>> >  #include <asm/irq.h>
>> > @@ -1223,6 +1225,63 @@ 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;
>> > +   const __be32 *line;
>> > +
>> > +   if (!node)
>> > +           return -ENODEV;
>> > +
>> > +   line = of_get_property(node, "id", NULL);
>> > +   if (!line)
>> > +           return -ENODEV;
>> > +
>> > +   sport->port.line = be32_to_cpup(line) - 1;
>>
>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>> be enumerated, the driver should look for a /aliases/uart* property
>> that matches the of_node.  Doing it that way is already established in
>> the OpenFirmware documentation, and it ensures there are no overlaps
>> in the global namespace.
>>
>
> I just gave one more try to avoid using 'aliases', and you gave a
> 'no' again.  Now, I know how hard you are on this.  Okay, I start
> thinking about your suggestion seriously :)

Ha ha ha.

>
>> We do need some infrastructure to make that easier though.  Would you
>> have time to help put that together?
>>
> Ok, I will give it a try.

Cool. We'll talk next week about it.

Re: [PATCH 2/3] net/fec: add device tree support

[Rob Herring]

On 06/19/2011 07:11 AM, Arnd Bergmann wrote:
> On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote:
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id fec_dt_ids[] = {
>>> +     { .compatible = "fsl,fec", .data =&fec_devtype[0], },
>>> +     {},
>>> +};
>>> +
>>> +static const struct of_device_id *
>>> +fec_get_of_device_id(struct platform_device *pdev)
>>> +{
>>> +     return of_match_device(fec_dt_ids,&pdev->dev);
>>> +}
>>> +#else
>>> +#define fec_dt_ids NULL
>>> +static inline struct of_device_id *
>>> +fec_get_of_device_id(struct platform_device *pdev)
>>> +{
>>> +     return NULL;
>>> +}
>>> +#endif
>>> +
>>>   static unsigned char macaddr[ETH_ALEN];
>>>   module_param_array(macaddr, byte, NULL, 0);
>>>   MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
>>>       struct net_device *ndev;
>>>       int i, irq, ret = 0;
>>>       struct resource *r;
>>> +     const struct of_device_id *of_id;
>>> +
>>> +     of_id = fec_get_of_device_id(pdev);
>>
>> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This
>> use of it will break compilation when this is not defined.
>>
> 
> 
> Why? Note the #else path defining an empty function.
> 

None of this is necessary in the first place. Just call of_match_device
directly. There is already an empty function to return NULL when
CONFIG_OF is not defined.

Rob

Re: [PATCH 1/3] serial/imx: add device tree support

[Rob Herring]

On 06/19/2011 10:05 AM, Grant Likely wrote:
> On Sun, Jun 19, 2011 at 1:30 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
>> On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
>>> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
>>>> It adds device tree data parsing support for imx tty/serial driver.
>>>>
>>>> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
>>>> Signed-off-by: Jason Liu <jason.hui@linaro.org>
>>>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>> ---
>>>>  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>>>>  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
>>>>  2 files changed, 92 insertions(+), 10 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>> new file mode 100644
>>>> index 0000000..7648e17
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>> @@ -0,0 +1,21 @@
>>>> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
>>>> +
>>>> +Required properties:
>>>> +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
>>>
>>> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
>>>
>>> It's better to anchor these things on real silicon, or a real ip block
>>> specification rather than something pseudo-generic.  Subsequent chips,
>>> like the imx53, should simply claim compatibility with the older
>>> fsl,imx51-uart.
>>
>> It is a real IP block on all imx silicons (except imx23 and imx28
>> which are known as former stmp).
>>
>>>
>>> (in essence, "fsl,imx51-uart" becomes the generic string without the
>>> downside of having no obvious recourse when new silicon shows up that
>>> is an imx part, but isn't compatible with the imx51 uart.
>>>
>> In this case, should imx1 the ancestor of imx family than imx51
>> becomes part of that generic string?  Claiming uart of imx1, imx21
>> and imx31 (senior than imx51) compatible with the imx51 uart seems
>> odd to me.
>>
>> That said, IMO, "fsl,imx-uart" stands a real IP block specification
>> here and can be a perfect generic compatibility string to tell the
>> recourse of any imx silicon using this IP.
> 
> Yes, but which /version/ of the IP block?  Hardware designers are
> notorious for changing hardware designs for newer silicon, sometimes
> to add features, sometimes to fix bugs.  While I understand the
> temptation to boil a compatible value down to a nice clean generic
> string, doing so only works in a perfect world.  In the real world,
> you still need to have some information about the specific
> implementation.  I prefer this specifying it to the SoC name, but I've
> been known to be convinced that specifying it to the ip-block name &
> version in certain circumstances, like for IP blocks in an FPGA, or
> some of the Freescale powerpc pXXXX SoCs which actually had an IP
> block swapped out midway through the life of the chip.
> 

There are definitely uart changes along the way with each generation.

> Besides, encoding an soc or ip block version into the 'generic'
> compatible values is not just good practice, it has *zero downside*.
> That's the beauty of the compatible property semantics.  Any node can
> claim compatibility with an existing device.  If no existing device
> fits correctly, then the node simple does not claim compatibility.
> Drivers can bind to any number of compatible strings, so it would be
> just fine for the of_match_table list to include both "fsl,imx-21" and
> "fsl,imx-51" (assuming that is the appropriate solution in this case).
> 

Don't you need uart or serial in here somewhere.

Rob

Re: [PATCH 2/3] net/fec: add device tree support

[Grant Likely]

On Sun, Jun 19, 2011 at 9:09 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 06/19/2011 07:11 AM, Arnd Bergmann wrote:
>> On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote:
>>>> +#ifdef CONFIG_OF
>>>> +static const struct of_device_id fec_dt_ids[] = {
>>>> +     { .compatible = "fsl,fec", .data =&fec_devtype[0], },
>>>> +     {},
>>>> +};
>>>> +
>>>> +static const struct of_device_id *
>>>> +fec_get_of_device_id(struct platform_device *pdev)
>>>> +{
>>>> +     return of_match_device(fec_dt_ids,&pdev->dev);
>>>> +}
>>>> +#else
>>>> +#define fec_dt_ids NULL
>>>> +static inline struct of_device_id *
>>>> +fec_get_of_device_id(struct platform_device *pdev)
>>>> +{
>>>> +     return NULL;
>>>> +}
>>>> +#endif
>>>> +
>>>>   static unsigned char macaddr[ETH_ALEN];
>>>>   module_param_array(macaddr, byte, NULL, 0);
>>>>   MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>>>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
>>>>       struct net_device *ndev;
>>>>       int i, irq, ret = 0;
>>>>       struct resource *r;
>>>> +     const struct of_device_id *of_id;
>>>> +
>>>> +     of_id = fec_get_of_device_id(pdev);
>>>
>>> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This
>>> use of it will break compilation when this is not defined.
>>>
>>
>>
>> Why? Note the #else path defining an empty function.
>>
>
> None of this is necessary in the first place. Just call of_match_device
> directly. There is already an empty function to return NULL when
> CONFIG_OF is not defined.

Heh, right you are.  I should have caught on to that.  :-)

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Sun, Jun 19, 2011 at 9:15 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 06/19/2011 10:05 AM, Grant Likely wrote:
>> On Sun, Jun 19, 2011 at 1:30 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
>>> On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
>>>> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
>>>>> It adds device tree data parsing support for imx tty/serial driver.
>>>>>
>>>>> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
>>>>> Signed-off-by: Jason Liu <jason.hui@linaro.org>
>>>>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>> ---
>>>>>  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>>>>>  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
>>>>>  2 files changed, 92 insertions(+), 10 deletions(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>>> new file mode 100644
>>>>> index 0000000..7648e17
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>>>>> @@ -0,0 +1,21 @@
>>>>> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
>>>>
>>>> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
>>>>
>>>> It's better to anchor these things on real silicon, or a real ip block
>>>> specification rather than something pseudo-generic.  Subsequent chips,
>>>> like the imx53, should simply claim compatibility with the older
>>>> fsl,imx51-uart.
>>>
>>> It is a real IP block on all imx silicons (except imx23 and imx28
>>> which are known as former stmp).
>>>
>>>>
>>>> (in essence, "fsl,imx51-uart" becomes the generic string without the
>>>> downside of having no obvious recourse when new silicon shows up that
>>>> is an imx part, but isn't compatible with the imx51 uart.
>>>>
>>> In this case, should imx1 the ancestor of imx family than imx51
>>> becomes part of that generic string?  Claiming uart of imx1, imx21
>>> and imx31 (senior than imx51) compatible with the imx51 uart seems
>>> odd to me.
>>>
>>> That said, IMO, "fsl,imx-uart" stands a real IP block specification
>>> here and can be a perfect generic compatibility string to tell the
>>> recourse of any imx silicon using this IP.
>>
>> Yes, but which /version/ of the IP block?  Hardware designers are
>> notorious for changing hardware designs for newer silicon, sometimes
>> to add features, sometimes to fix bugs.  While I understand the
>> temptation to boil a compatible value down to a nice clean generic
>> string, doing so only works in a perfect world.  In the real world,
>> you still need to have some information about the specific
>> implementation.  I prefer this specifying it to the SoC name, but I've
>> been known to be convinced that specifying it to the ip-block name &
>> version in certain circumstances, like for IP blocks in an FPGA, or
>> some of the Freescale powerpc pXXXX SoCs which actually had an IP
>> block swapped out midway through the life of the chip.
>>
>
> There are definitely uart changes along the way with each generation.
>
>> Besides, encoding an soc or ip block version into the 'generic'
>> compatible values is not just good practice, it has *zero downside*.
>> That's the beauty of the compatible property semantics.  Any node can
>> claim compatibility with an existing device.  If no existing device
>> fits correctly, then the node simple does not claim compatibility.
>> Drivers can bind to any number of compatible strings, so it would be
>> just fine for the of_match_table list to include both "fsl,imx-21" and
>> "fsl,imx-51" (assuming that is the appropriate solution in this case).
>>
>
> Don't you need uart or serial in here somewhere.

you are of course correct.  The examples should be "fsl,imx21-uart" &
"fsl,imx51-uart".  I was just writing too quickly.

g.

Re: [PATCH 2/3] net/fec: add device tree support

[Greg Ungerer]

On 19/06/11 22:11, Arnd Bergmann wrote:
> On Sunday 19 June 2011 13:39:32 Greg Ungerer wrote:
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id fec_dt_ids[] = {
>>> +     { .compatible = "fsl,fec", .data =&fec_devtype[0], },
>>> +     {},
>>> +};
>>> +
>>> +static const struct of_device_id *
>>> +fec_get_of_device_id(struct platform_device *pdev)
>>> +{
>>> +     return of_match_device(fec_dt_ids,&pdev->dev);
>>> +}
>>> +#else
>>> +#define fec_dt_ids NULL
>>> +static inline struct of_device_id *
>>> +fec_get_of_device_id(struct platform_device *pdev)
>>> +{
>>> +     return NULL;
>>> +}
>>> +#endif
>>> +
>>>    static unsigned char macaddr[ETH_ALEN];
>>>    module_param_array(macaddr, byte, NULL, 0);
>>>    MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>>> @@ -1363,6 +1385,11 @@ fec_probe(struct platform_device *pdev)
>>>        struct net_device *ndev;
>>>        int i, irq, ret = 0;
>>>        struct resource *r;
>>> +     const struct of_device_id *of_id;
>>> +
>>> +     of_id = fec_get_of_device_id(pdev);
>>
>> fec_get_of_device_id() is defined inside of "#ifdef CONFIG_OF". This
>> use of it will break compilation when this is not defined.
>>
>
>
> Why? Note the #else path defining an empty function.

Sorry, missed that :-)

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

Hi Grant,

I just gave a try to use aliases node for identify the device index.
Please take a look and let me know if it's what you expect.

On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
[...]
> > >  
> > > +#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;
> > > +	const __be32 *line;
> > > +
> > > +	if (!node)
> > > +		return -ENODEV;
> > > +
> > > +	line = of_get_property(node, "id", NULL);
> > > +	if (!line)
> > > +		return -ENODEV;
> > > +
> > > +	sport->port.line = be32_to_cpup(line) - 1;
> > 
> > Hmmm, I really would like to be rid of this.  Instead, if uarts must
> > be enumerated, the driver should look for a /aliases/uart* property
> > that matches the of_node.  Doing it that way is already established in
> > the OpenFirmware documentation, and it ensures there are no overlaps
> > in the global namespace.
> > 
> 
> I just gave one more try to avoid using 'aliases', and you gave a
> 'no' again.  Now, I know how hard you are on this.  Okay, I start
> thinking about your suggestion seriously :)
> 
> > We do need some infrastructure to make that easier though.  Would you
> > have time to help put that together?
> > 
> Ok, I will give it a try.
> 

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index da0381a..f4a5c3c 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -18,6 +18,12 @@
 	compatible = "fsl,imx51-babbage", "fsl,imx51";
 	interrupt-parent = <&tzic>;
 
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+	};
+
 	chosen {
 		bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
 	};
@@ -47,29 +53,29 @@
 			reg = <0x70000000 0x40000>;
 			ranges;
 
-			uart@7000c000 {
+			uart2: uart@7000c000 {
 				compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 				reg = <0x7000c000 0x4000>;
 				interrupts = <33>;
 				id = <3>;
-				fsl,has-rts-cts;
+				fsl,uart-has-rtscts;
 			};
 		};
 
-		uart@73fbc000 {
+		uart0: uart@73fbc000 {
 			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 			reg = <0x73fbc000 0x4000>;
 			interrupts = <31>;
 			id = <1>;
-			fsl,has-rts-cts;
+			fsl,uart-has-rtscts;
 		};
 
-		uart@73fc0000 {
+		uart1: uart@73fc0000 {
 			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 			reg = <0x73fc0000 0x4000>;
 			interrupts = <32>;
 			id = <2>;
-			fsl,has-rts-cts;
+			fsl,uart-has-rtscts;
 		};
 	};
 
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 632ebae..13df5d2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -737,6 +737,37 @@ err0:
 EXPORT_SYMBOL(of_parse_phandles_with_args);
 
 /**
+ *	of_get_device_index - Get device index by looking up "aliases" node
+ *	@np:	Pointer to device node that asks for device index
+ *	@name:	The device alias without index number
+ *
+ *	Returns the device index if find it, else returns -ENODEV.
+ */
+int of_get_device_index(struct device_node *np, const char *alias)
+{
+	struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
+	struct property *prop;
+	char name[32];
+	int index = 0;
+
+	if (!aliases)
+		return -ENODEV;
+
+	while (1) {
+		snprintf(name, sizeof(name), "%s%d", alias, index);
+		prop = of_find_property(aliases, name, NULL);
+		if (!prop)
+			return -ENODEV;
+		if (np == of_find_node_by_path(prop->value))
+			break;
+		index++;
+	}
+
+	return index;
+}
+EXPORT_SYMBOL(of_get_device_index);
+
+/**
  * prom_add_property - Add a property to a node
  */
 int prom_add_property(struct device_node *np, struct property *prop)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index da436e0..852668f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,
 	struct device_node *node = pdev->dev.of_node;
 	const struct of_device_id *of_id =
 			of_match_device(imx_uart_dt_ids, &pdev->dev);
-	const __be32 *line;
+	int line;
 
 	if (!node)
 		return -ENODEV;
 
-	line = of_get_property(node, "id", NULL);
-	if (!line)
+	line = of_get_device_index(node, "serial");
+	if (IS_ERR_VALUE(line))
 		return -ENODEV;
 
-	sport->port.line = be32_to_cpup(line) - 1;
+	sport->port.line = line;
 
-	if (of_get_property(node, "fsl,has-rts-cts", NULL))
+	if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
 		sport->have_rtscts = 1;
 
 	if (of_get_property(node, "fsl,irda-mode", NULL))
diff --git a/include/linux/of.h b/include/linux/of.h
index bfc0ed1..3153752 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct device_node **out_node, const void **out_args);
 
+extern int of_get_device_index(struct device_node *np, const char *alias);
+
 extern int of_machine_is_compatible(const char *compat);
 
 extern int prom_add_property(struct device_node* np, struct property* prop);

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Mitch Bradley]

I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries?  The properties could be the names of device special files and the values the corresponding node phandles.



On 6/21/2011 3:55 AM, Shawn Guo wrote:
> Hi Grant,
> 
> I just gave a try to use aliases node for identify the device index.
> Please take a look and let me know if it's what you expect.
> 
> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
> [...]
>>>>
>>>> +#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;
>>>> +	const __be32 *line;
>>>> +
>>>> +	if (!node)
>>>> +		return -ENODEV;
>>>> +
>>>> +	line = of_get_property(node, "id", NULL);
>>>> +	if (!line)
>>>> +		return -ENODEV;
>>>> +
>>>> +	sport->port.line = be32_to_cpup(line) - 1;
>>>
>>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>>> be enumerated, the driver should look for a /aliases/uart* property
>>> that matches the of_node.  Doing it that way is already established in
>>> the OpenFirmware documentation, and it ensures there are no overlaps
>>> in the global namespace.
>>>
>>
>> I just gave one more try to avoid using 'aliases', and you gave a
>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>> thinking about your suggestion seriously :)
>>
>>> We do need some infrastructure to make that easier though.  Would you
>>> have time to help put that together?
>>>
>> Ok, I will give it a try.
>>
> 
> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> index da0381a..f4a5c3c 100644
> --- a/arch/arm/boot/dts/imx51-babbage.dts
> +++ b/arch/arm/boot/dts/imx51-babbage.dts
> @@ -18,6 +18,12 @@
>   	compatible = "fsl,imx51-babbage", "fsl,imx51";
>   	interrupt-parent =<&tzic>;
> 
> +	aliases {
> +		serial0 =&uart0;
> +		serial1 =&uart1;
> +		serial2 =&uart2;
> +	};
> +
>   	chosen {
>   		bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
>   	};
> @@ -47,29 +53,29 @@
>   			reg =<0x70000000 0x40000>;
>   			ranges;
> 
> -			uart@7000c000 {
> +			uart2: uart@7000c000 {
>   				compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>   				reg =<0x7000c000 0x4000>;
>   				interrupts =<33>;
>   				id =<3>;
> -				fsl,has-rts-cts;
> +				fsl,uart-has-rtscts;
>   			};
>   		};
> 
> -		uart@73fbc000 {
> +		uart0: uart@73fbc000 {
>   			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>   			reg =<0x73fbc000 0x4000>;
>   			interrupts =<31>;
>   			id =<1>;
> -			fsl,has-rts-cts;
> +			fsl,uart-has-rtscts;
>   		};
> 
> -		uart@73fc0000 {
> +		uart1: uart@73fc0000 {
>   			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>   			reg =<0x73fc0000 0x4000>;
>   			interrupts =<32>;
>   			id =<2>;
> -			fsl,has-rts-cts;
> +			fsl,uart-has-rtscts;
>   		};
>   	};
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 632ebae..13df5d2 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -737,6 +737,37 @@ err0:
>   EXPORT_SYMBOL(of_parse_phandles_with_args);
> 
>   /**
> + *	of_get_device_index - Get device index by looking up "aliases" node
> + *	@np:	Pointer to device node that asks for device index
> + *	@name:	The device alias without index number
> + *
> + *	Returns the device index if find it, else returns -ENODEV.
> + */
> +int of_get_device_index(struct device_node *np, const char *alias)
> +{
> +	struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
> +	struct property *prop;
> +	char name[32];
> +	int index = 0;
> +
> +	if (!aliases)
> +		return -ENODEV;
> +
> +	while (1) {
> +		snprintf(name, sizeof(name), "%s%d", alias, index);
> +		prop = of_find_property(aliases, name, NULL);
> +		if (!prop)
> +			return -ENODEV;
> +		if (np == of_find_node_by_path(prop->value))
> +			break;
> +		index++;
> +	}
> +
> +	return index;
> +}
> +EXPORT_SYMBOL(of_get_device_index);
> +
> +/**
>    * prom_add_property - Add a property to a node
>    */
>   int prom_add_property(struct device_node *np, struct property *prop)
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index da436e0..852668f 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,
>   	struct device_node *node = pdev->dev.of_node;
>   	const struct of_device_id *of_id =
>   			of_match_device(imx_uart_dt_ids,&pdev->dev);
> -	const __be32 *line;
> +	int line;
> 
>   	if (!node)
>   		return -ENODEV;
> 
> -	line = of_get_property(node, "id", NULL);
> -	if (!line)
> +	line = of_get_device_index(node, "serial");
> +	if (IS_ERR_VALUE(line))
>   		return -ENODEV;
> 
> -	sport->port.line = be32_to_cpup(line) - 1;
> +	sport->port.line = line;
> 
> -	if (of_get_property(node, "fsl,has-rts-cts", NULL))
> +	if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>   		sport->have_rtscts = 1;
> 
>   	if (of_get_property(node, "fsl,irda-mode", NULL))
> diff --git a/include/linux/of.h b/include/linux/of.h
> index bfc0ed1..3153752 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct device_node *np,
>   	const char *list_name, const char *cells_name, int index,
>   	struct device_node **out_node, const void **out_args);
> 
> +extern int of_get_device_index(struct device_node *np, const char *alias);
> +
>   extern int of_machine_is_compatible(const char *compat);
> 
>   extern int prom_add_property(struct device_node* np, struct property* prop);
> 

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries?  The properties could be the names of device special files and the values the corresponding node phandles.

I've been trying /really/ hard to avoid doing something like that
because a lot of the time the desired Linux dev name is a
implementation detail, and a potentially unstable one at that.  If
Linux requires certain devices to have certain names because that is
how it hooks up clocks (which is the current situation on some
platforms), then I'd rather have Linux encode a lookup of the
preferred name, at least until the that particular implementation
detail goes away.

As for enumerating devices, I don't think this is a Linux-specific
thing.  In this case it is entirely reasonable to want to say /this
node/ is the second serial port, and /that node/ is the third, which
is information needed regardless of the client OS.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Russell King - ARM Linux]

On Tue, Jun 21, 2011 at 12:42:14PM -0600, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> > I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries?  The properties could be the names of device special files and the values the corresponding node phandles.
> 
> I've been trying /really/ hard to avoid doing something like that
> because a lot of the time the desired Linux dev name is a
> implementation detail, and a potentially unstable one at that.  If
> Linux requires certain devices to have certain names because that is
> how it hooks up clocks

As I keep on saying, we don't _have_ to have to match on device name.
If DT can come up with a better way to bind a clock to a particular
device/connection name then DT can provide its own clk_get()
implementation which does that.

clk_get() has the struct device, and therefore can get at the DT
information itself to read out whatever properties are required.  But,
we must not get away from the fact that clk_get()'s second argument
should _never_ be used as a unique clock name itself.

At the moment, until we do have some kind of DT based clk_get(), the
easiest way to move clk-API using drivers over is to use the device
name in the static clk_lookup tables.

It's all an implementation detail, one which is (thankfully) hidden
behind a proper API which will be _completely_ transparent to drivers
using the clk API in the _proper_ way.

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 12:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 21, 2011 at 12:42:14PM -0600, Grant Likely wrote:
>> On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote:
>> > I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries?  The properties could be the names of device special files and the values the corresponding node phandles.
>>
>> I've been trying /really/ hard to avoid doing something like that
>> because a lot of the time the desired Linux dev name is a
>> implementation detail, and a potentially unstable one at that.  If
>> Linux requires certain devices to have certain names because that is
>> how it hooks up clocks
>
> As I keep on saying, we don't _have_ to have to match on device name.
> If DT can come up with a better way to bind a clock to a particular
> device/connection name then DT can provide its own clk_get()
> implementation which does that.

Sorry, I overstated the situation.  My point is only that I don't want
encode how Linux currently views the world into the DT, because
implementation details can and do change.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Mitch Bradley]

On 6/21/2011 8:42 AM, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley<wmb@firmworks.com>  wrote:
>> I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries?  The properties could be the names of device special files and the values the corresponding node phandles.
>
> I've been trying /really/ hard to avoid doing something like that
> because a lot of the time the desired Linux dev name is a
> implementation detail, and a potentially unstable one at that.  If
> Linux requires certain devices to have certain names because that is
> how it hooks up clocks (which is the current situation on some
> platforms), then I'd rather have Linux encode a lookup of the
> preferred name, at least until the that particular implementation
> detail goes away.
>
> As for enumerating devices, I don't think this is a Linux-specific
> thing.  In this case it is entirely reasonable to want to say /this
> node/ is the second serial port, and /that node/ is the third, which
> is information needed regardless of the client OS.


This reminds me a little of the "slot-names" property defined by the PCI 
bus binding.  It was intended to correlate PCI device numbers with the 
corresponding externally-visible labeling of the slot, so that error 
messages could identify a slot using a name that had some meaning to a user.

The notion of "first" and "second" is, of course, rather difficult to 
define precisely.  What aspect of the device establishes the order?

The "slot-names" property was useful for systems from e.g. Sun, where 
one vendor controlled the whole system rollup.  It was almost useless 
for cases where the one vendor supplied the motherboard and others put 
it in various cases.

>
> g.
>
>

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Hi Grant,
>
> I just gave a try to use aliases node for identify the device index.
> Please take a look and let me know if it's what you expect.

Thanks Shawn.  This is definitely on track.  Comments below...

>
> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
> [...]
>> > >
>> > > +#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;
>> > > + const __be32 *line;
>> > > +
>> > > + if (!node)
>> > > +         return -ENODEV;
>> > > +
>> > > + line = of_get_property(node, "id", NULL);
>> > > + if (!line)
>> > > +         return -ENODEV;
>> > > +
>> > > + sport->port.line = be32_to_cpup(line) - 1;
>> >
>> > Hmmm, I really would like to be rid of this.  Instead, if uarts must
>> > be enumerated, the driver should look for a /aliases/uart* property
>> > that matches the of_node.  Doing it that way is already established in
>> > the OpenFirmware documentation, and it ensures there are no overlaps
>> > in the global namespace.
>> >
>>
>> I just gave one more try to avoid using 'aliases', and you gave a
>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>> thinking about your suggestion seriously :)
>>
>> > We do need some infrastructure to make that easier though.  Would you
>> > have time to help put that together?
>> >
>> Ok, I will give it a try.
>>
>
> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> index da0381a..f4a5c3c 100644
> --- a/arch/arm/boot/dts/imx51-babbage.dts
> +++ b/arch/arm/boot/dts/imx51-babbage.dts
> @@ -18,6 +18,12 @@
>        compatible = "fsl,imx51-babbage", "fsl,imx51";
>        interrupt-parent = <&tzic>;
>
> +       aliases {
> +               serial0 = &uart0;
> +               serial1 = &uart1;
> +               serial2 = &uart2;
> +       };
> +

Hmmm.  David Gibson had tossed out an idea of automatically generating
aliases from labels.  It may be time to revisit that idea.

David, perhaps using this format for a label should turn it into an
alias (prefix label with an asterisk):

        *thealias: i2c@12340000 { /*...*/ };

.... although that approach gets *really* hairy when considering that
different boards will want different enumeration.  How would one
override an automatic alias defined by an included .dts file?

>        chosen {
>                bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
>        };
> @@ -47,29 +53,29 @@
>                        reg = <0x70000000 0x40000>;
>                        ranges;
>
> -                       uart@7000c000 {
> +                       uart2: uart@7000c000 {
>                                compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                                reg = <0x7000c000 0x4000>;
>                                interrupts = <33>;
>                                id = <3>;
> -                               fsl,has-rts-cts;
> +                               fsl,uart-has-rtscts;
>                        };
>                };
>
> -               uart@73fbc000 {
> +               uart0: uart@73fbc000 {
>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                        reg = <0x73fbc000 0x4000>;
>                        interrupts = <31>;
>                        id = <1>;
> -                       fsl,has-rts-cts;
> +                       fsl,uart-has-rtscts;
>                };
>
> -               uart@73fc0000 {
> +               uart1: uart@73fc0000 {
>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                        reg = <0x73fc0000 0x4000>;
>                        interrupts = <32>;
>                        id = <2>;
> -                       fsl,has-rts-cts;
> +                       fsl,uart-has-rtscts;
>                };
>        };
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 632ebae..13df5d2 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -737,6 +737,37 @@ err0:
>  EXPORT_SYMBOL(of_parse_phandles_with_args);
>
>  /**
> + *     of_get_device_index - Get device index by looking up "aliases" node
> + *     @np:    Pointer to device node that asks for device index
> + *     @name:  The device alias without index number
> + *
> + *     Returns the device index if find it, else returns -ENODEV.
> + */
> +int of_get_device_index(struct device_node *np, const char *alias)
> +{
> +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
> +       struct property *prop;
> +       char name[32];
> +       int index = 0;
> +
> +       if (!aliases)
> +               return -ENODEV;
> +
> +       while (1) {
> +               snprintf(name, sizeof(name), "%s%d", alias, index);
> +               prop = of_find_property(aliases, name, NULL);
> +               if (!prop)
> +                       return -ENODEV;
> +               if (np == of_find_node_by_path(prop->value))
> +                       break;
> +               index++;
> +       }

Rather than parsing the alias strings everytime, it would probably be
better to preprocess all the properties in the aliases node and create
a lookup table of alias->node references that can be walked quickly
and trivially.

Also, when obtaining an enumeration for a device, you'll need to be
careful about what number gets returned.  If the node doesn't match a
given alias, but aliases do exist for other devices of like type, then
you need to be careful not to assign a number already assigned to
another device via an alias (this of course assumes the driver
supports dynamics enumeration, which many drivers will).  It would be

\> +
> +       return index;
> +}
> +EXPORT_SYMBOL(of_get_device_index);
> +
> +/**
>  * prom_add_property - Add a property to a node
>  */
>  int prom_add_property(struct device_node *np, struct property *prop)
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index da436e0..852668f 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,
>        struct device_node *node = pdev->dev.of_node;
>        const struct of_device_id *of_id =
>                        of_match_device(imx_uart_dt_ids, &pdev->dev);
> -       const __be32 *line;
> +       int line;
>
>        if (!node)
>                return -ENODEV;
>
> -       line = of_get_property(node, "id", NULL);
> -       if (!line)
> +       line = of_get_device_index(node, "serial");
> +       if (IS_ERR_VALUE(line))
>                return -ENODEV;

Personally, it an alias isn't present, then I'd dynamically assign a port id.

>
> -       sport->port.line = be32_to_cpup(line) - 1;
> +       sport->port.line = line;
>
> -       if (of_get_property(node, "fsl,has-rts-cts", NULL))
> +       if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>                sport->have_rtscts = 1;
>
>        if (of_get_property(node, "fsl,irda-mode", NULL))
> diff --git a/include/linux/of.h b/include/linux/of.h
> index bfc0ed1..3153752 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct device_node *np,
>        const char *list_name, const char *cells_name, int index,
>        struct device_node **out_node, const void **out_args);
>
> +extern int of_get_device_index(struct device_node *np, const char *alias);
> +
>  extern int of_machine_is_compatible(const char *compat);
>
>  extern int prom_add_property(struct device_node* np, struct property* prop);
>
> --
> Regards,
> Shawn
>
>



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

Re: [PATCH 1/3] serial/imx: add device tree support

[Mitch Bradley]

What is the problem with

aliases{
    serial0 = "/uart@7000c000";
}

Properties in the alias node are supposed to have string values.


On 6/21/2011 9:13 AM, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo<shawn.guo@freescale.com>  wrote:
>> Hi Grant,
>>
>> I just gave a try to use aliases node for identify the device index.
>> Please take a look and let me know if it's what you expect.
>
> Thanks Shawn.  This is definitely on track.  Comments below...
>
>>
>> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
>> [...]
>>>>>
>>>>> +#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;
>>>>> + const __be32 *line;
>>>>> +
>>>>> + if (!node)
>>>>> +         return -ENODEV;
>>>>> +
>>>>> + line = of_get_property(node, "id", NULL);
>>>>> + if (!line)
>>>>> +         return -ENODEV;
>>>>> +
>>>>> + sport->port.line = be32_to_cpup(line) - 1;
>>>>
>>>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>>>> be enumerated, the driver should look for a /aliases/uart* property
>>>> that matches the of_node.  Doing it that way is already established in
>>>> the OpenFirmware documentation, and it ensures there are no overlaps
>>>> in the global namespace.
>>>>
>>>
>>> I just gave one more try to avoid using 'aliases', and you gave a
>>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>>> thinking about your suggestion seriously :)
>>>
>>>> We do need some infrastructure to make that easier though.  Would you
>>>> have time to help put that together?
>>>>
>>> Ok, I will give it a try.
>>>
>>
>> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
>> index da0381a..f4a5c3c 100644
>> --- a/arch/arm/boot/dts/imx51-babbage.dts
>> +++ b/arch/arm/boot/dts/imx51-babbage.dts
>> @@ -18,6 +18,12 @@
>>         compatible = "fsl,imx51-babbage", "fsl,imx51";
>>         interrupt-parent =<&tzic>;
>>
>> +       aliases {
>> +               serial0 =&uart0;
>> +               serial1 =&uart1;
>> +               serial2 =&uart2;
>> +       };
>> +
>
> Hmmm.  David Gibson had tossed out an idea of automatically generating
> aliases from labels.  It may be time to revisit that idea.
>
> David, perhaps using this format for a label should turn it into an
> alias (prefix label with an asterisk):
>
>          *thealias: i2c@12340000 { /*...*/ };
>
> .... although that approach gets *really* hairy when considering that
> different boards will want different enumeration.  How would one
> override an automatic alias defined by an included .dts file?
>
>>         chosen {
>>                 bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
>>         };
>> @@ -47,29 +53,29 @@
>>                         reg =<0x70000000 0x40000>;
>>                         ranges;
>>
>> -                       uart@7000c000 {
>> +                       uart2: uart@7000c000 {
>>                                 compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>                                 reg =<0x7000c000 0x4000>;
>>                                 interrupts =<33>;
>>                                 id =<3>;
>> -                               fsl,has-rts-cts;
>> +                               fsl,uart-has-rtscts;
>>                         };
>>                 };
>>
>> -               uart@73fbc000 {
>> +               uart0: uart@73fbc000 {
>>                         compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>                         reg =<0x73fbc000 0x4000>;
>>                         interrupts =<31>;
>>                         id =<1>;
>> -                       fsl,has-rts-cts;
>> +                       fsl,uart-has-rtscts;
>>                 };
>>
>> -               uart@73fc0000 {
>> +               uart1: uart@73fc0000 {
>>                         compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>                         reg =<0x73fc0000 0x4000>;
>>                         interrupts =<32>;
>>                         id =<2>;
>> -                       fsl,has-rts-cts;
>> +                       fsl,uart-has-rtscts;
>>                 };
>>         };
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 632ebae..13df5d2 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -737,6 +737,37 @@ err0:
>>   EXPORT_SYMBOL(of_parse_phandles_with_args);
>>
>>   /**
>> + *     of_get_device_index - Get device index by looking up "aliases" node
>> + *     @np:    Pointer to device node that asks for device index
>> + *     @name:  The device alias without index number
>> + *
>> + *     Returns the device index if find it, else returns -ENODEV.
>> + */
>> +int of_get_device_index(struct device_node *np, const char *alias)
>> +{
>> +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
>> +       struct property *prop;
>> +       char name[32];
>> +       int index = 0;
>> +
>> +       if (!aliases)
>> +               return -ENODEV;
>> +
>> +       while (1) {
>> +               snprintf(name, sizeof(name), "%s%d", alias, index);
>> +               prop = of_find_property(aliases, name, NULL);
>> +               if (!prop)
>> +                       return -ENODEV;
>> +               if (np == of_find_node_by_path(prop->value))
>> +                       break;
>> +               index++;
>> +       }
>
> Rather than parsing the alias strings everytime, it would probably be
> better to preprocess all the properties in the aliases node and create
> a lookup table of alias->node references that can be walked quickly
> and trivially.
>
> Also, when obtaining an enumeration for a device, you'll need to be
> careful about what number gets returned.  If the node doesn't match a
> given alias, but aliases do exist for other devices of like type, then
> you need to be careful not to assign a number already assigned to
> another device via an alias (this of course assumes the driver
> supports dynamics enumeration, which many drivers will).  It would be
>
> \>  +
>> +       return index;
>> +}
>> +EXPORT_SYMBOL(of_get_device_index);
>> +
>> +/**
>>   * prom_add_property - Add a property to a node
>>   */
>>   int prom_add_property(struct device_node *np, struct property *prop)
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index da436e0..852668f 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,
>>         struct device_node *node = pdev->dev.of_node;
>>         const struct of_device_id *of_id =
>>                         of_match_device(imx_uart_dt_ids,&pdev->dev);
>> -       const __be32 *line;
>> +       int line;
>>
>>         if (!node)
>>                 return -ENODEV;
>>
>> -       line = of_get_property(node, "id", NULL);
>> -       if (!line)
>> +       line = of_get_device_index(node, "serial");
>> +       if (IS_ERR_VALUE(line))
>>                 return -ENODEV;
>
> Personally, it an alias isn't present, then I'd dynamically assign a port id.
>
>>
>> -       sport->port.line = be32_to_cpup(line) - 1;
>> +       sport->port.line = line;
>>
>> -       if (of_get_property(node, "fsl,has-rts-cts", NULL))
>> +       if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>>                 sport->have_rtscts = 1;
>>
>>         if (of_get_property(node, "fsl,irda-mode", NULL))
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index bfc0ed1..3153752 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct device_node *np,
>>         const char *list_name, const char *cells_name, int index,
>>         struct device_node **out_node, const void **out_args);
>>
>> +extern int of_get_device_index(struct device_node *np, const char *alias);
>> +
>>   extern int of_machine_is_compatible(const char *compat);
>>
>>   extern int prom_add_property(struct device_node* np, struct property* prop);
>>
>> --
>> Regards,
>> Shawn
>>
>>
>
>
>

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 1:35 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> What is the problem with
>
> aliases{
>   serial0 = "/uart@7000c000";
> }
>
> Properties in the alias node are supposed to have string values.

?

Not sure I follow.  Indeed, properties in the aliases node are string values.

Are you referring to how I was proposing some dtc syntax for
generating the alias strings?

g.

>
>
> On 6/21/2011 9:13 AM, Grant Likely wrote:
>>
>> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo<shawn.guo@freescale.com>
>>  wrote:
>>>
>>> Hi Grant,
>>>
>>> I just gave a try to use aliases node for identify the device index.
>>> Please take a look and let me know if it's what you expect.
>>
>> Thanks Shawn.  This is definitely on track.  Comments below...
>>
>>>
>>> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
>>> [...]
>>>>>>
>>>>>> +#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;
>>>>>> + const __be32 *line;
>>>>>> +
>>>>>> + if (!node)
>>>>>> +         return -ENODEV;
>>>>>> +
>>>>>> + line = of_get_property(node, "id", NULL);
>>>>>> + if (!line)
>>>>>> +         return -ENODEV;
>>>>>> +
>>>>>> + sport->port.line = be32_to_cpup(line) - 1;
>>>>>
>>>>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>>>>> be enumerated, the driver should look for a /aliases/uart* property
>>>>> that matches the of_node.  Doing it that way is already established in
>>>>> the OpenFirmware documentation, and it ensures there are no overlaps
>>>>> in the global namespace.
>>>>>
>>>>
>>>> I just gave one more try to avoid using 'aliases', and you gave a
>>>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>>>> thinking about your suggestion seriously :)
>>>>
>>>>> We do need some infrastructure to make that easier though.  Would you
>>>>> have time to help put that together?
>>>>>
>>>> Ok, I will give it a try.
>>>>
>>>
>>> diff --git a/arch/arm/boot/dts/imx51-babbage.dts
>>> b/arch/arm/boot/dts/imx51-babbage.dts
>>> index da0381a..f4a5c3c 100644
>>> --- a/arch/arm/boot/dts/imx51-babbage.dts
>>> +++ b/arch/arm/boot/dts/imx51-babbage.dts
>>> @@ -18,6 +18,12 @@
>>>        compatible = "fsl,imx51-babbage", "fsl,imx51";
>>>        interrupt-parent =<&tzic>;
>>>
>>> +       aliases {
>>> +               serial0 =&uart0;
>>> +               serial1 =&uart1;
>>> +               serial2 =&uart2;
>>> +       };
>>> +
>>
>> Hmmm.  David Gibson had tossed out an idea of automatically generating
>> aliases from labels.  It may be time to revisit that idea.
>>
>> David, perhaps using this format for a label should turn it into an
>> alias (prefix label with an asterisk):
>>
>>         *thealias: i2c@12340000 { /*...*/ };
>>
>> .... although that approach gets *really* hairy when considering that
>> different boards will want different enumeration.  How would one
>> override an automatic alias defined by an included .dts file?
>>
>>>        chosen {
>>>                bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3
>>> rootwait";
>>>        };
>>> @@ -47,29 +53,29 @@
>>>                        reg =<0x70000000 0x40000>;
>>>                        ranges;
>>>
>>> -                       uart@7000c000 {
>>> +                       uart2: uart@7000c000 {
>>>                                compatible = "fsl,imx51-uart",
>>> "fsl,imx21-uart";
>>>                                reg =<0x7000c000 0x4000>;
>>>                                interrupts =<33>;
>>>                                id =<3>;
>>> -                               fsl,has-rts-cts;
>>> +                               fsl,uart-has-rtscts;
>>>                        };
>>>                };
>>>
>>> -               uart@73fbc000 {
>>> +               uart0: uart@73fbc000 {
>>>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>                        reg =<0x73fbc000 0x4000>;
>>>                        interrupts =<31>;
>>>                        id =<1>;
>>> -                       fsl,has-rts-cts;
>>> +                       fsl,uart-has-rtscts;
>>>                };
>>>
>>> -               uart@73fc0000 {
>>> +               uart1: uart@73fc0000 {
>>>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>                        reg =<0x73fc0000 0x4000>;
>>>                        interrupts =<32>;
>>>                        id =<2>;
>>> -                       fsl,has-rts-cts;
>>> +                       fsl,uart-has-rtscts;
>>>                };
>>>        };
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index 632ebae..13df5d2 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -737,6 +737,37 @@ err0:
>>>  EXPORT_SYMBOL(of_parse_phandles_with_args);
>>>
>>>  /**
>>> + *     of_get_device_index - Get device index by looking up "aliases"
>>> node
>>> + *     @np:    Pointer to device node that asks for device index
>>> + *     @name:  The device alias without index number
>>> + *
>>> + *     Returns the device index if find it, else returns -ENODEV.
>>> + */
>>> +int of_get_device_index(struct device_node *np, const char *alias)
>>> +{
>>> +       struct device_node *aliases = of_find_node_by_name(NULL,
>>> "aliases");
>>> +       struct property *prop;
>>> +       char name[32];
>>> +       int index = 0;
>>> +
>>> +       if (!aliases)
>>> +               return -ENODEV;
>>> +
>>> +       while (1) {
>>> +               snprintf(name, sizeof(name), "%s%d", alias, index);
>>> +               prop = of_find_property(aliases, name, NULL);
>>> +               if (!prop)
>>> +                       return -ENODEV;
>>> +               if (np == of_find_node_by_path(prop->value))
>>> +                       break;
>>> +               index++;
>>> +       }
>>
>> Rather than parsing the alias strings everytime, it would probably be
>> better to preprocess all the properties in the aliases node and create
>> a lookup table of alias->node references that can be walked quickly
>> and trivially.
>>
>> Also, when obtaining an enumeration for a device, you'll need to be
>> careful about what number gets returned.  If the node doesn't match a
>> given alias, but aliases do exist for other devices of like type, then
>> you need to be careful not to assign a number already assigned to
>> another device via an alias (this of course assumes the driver
>> supports dynamics enumeration, which many drivers will).  It would be
>>
>> \>  +
>>>
>>> +       return index;
>>> +}
>>> +EXPORT_SYMBOL(of_get_device_index);
>>> +
>>> +/**
>>>  * prom_add_property - Add a property to a node
>>>  */
>>>  int prom_add_property(struct device_node *np, struct property *prop)
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index da436e0..852668f 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port
>>> *sport,
>>>        struct device_node *node = pdev->dev.of_node;
>>>        const struct of_device_id *of_id =
>>>                        of_match_device(imx_uart_dt_ids,&pdev->dev);
>>> -       const __be32 *line;
>>> +       int line;
>>>
>>>        if (!node)
>>>                return -ENODEV;
>>>
>>> -       line = of_get_property(node, "id", NULL);
>>> -       if (!line)
>>> +       line = of_get_device_index(node, "serial");
>>> +       if (IS_ERR_VALUE(line))
>>>                return -ENODEV;
>>
>> Personally, it an alias isn't present, then I'd dynamically assign a port
>> id.
>>
>>>
>>> -       sport->port.line = be32_to_cpup(line) - 1;
>>> +       sport->port.line = line;
>>>
>>> -       if (of_get_property(node, "fsl,has-rts-cts", NULL))
>>> +       if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>>>                sport->have_rtscts = 1;
>>>
>>>        if (of_get_property(node, "fsl,irda-mode", NULL))
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> index bfc0ed1..3153752 100644
>>> --- a/include/linux/of.h
>>> +++ b/include/linux/of.h
>>> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct
>>> device_node *np,
>>>        const char *list_name, const char *cells_name, int index,
>>>        struct device_node **out_node, const void **out_args);
>>>
>>> +extern int of_get_device_index(struct device_node *np, const char
>>> *alias);
>>> +
>>>  extern int of_machine_is_compatible(const char *compat);
>>>
>>>  extern int prom_add_property(struct device_node* np, struct property*
>>> prop);
>>>
>>> --
>>> Regards,
>>> Shawn
>>>
>>>
>>
>>
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



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

Re: [PATCH 1/3] serial/imx: add device tree support

[Mitch Bradley]

On 6/21/2011 9:38 AM, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 1:35 PM, Mitch Bradley<wmb@firmworks.com>  wrote:
>> What is the problem with
>>
>> aliases{
>>    serial0 = "/uart@7000c000";
>> }
>>
>> Properties in the alias node are supposed to have string values.
>
> ?
>
> Not sure I follow.  Indeed, properties in the aliases node are string values.
>
> Are you referring to how I was proposing some dtc syntax for
> generating the alias strings?


The point is that if you refer to the node explicitly by its string 
name, the need for a label disappears and the problem of overriding a 
default alias disappears (assuming that a later redefinition of a 
property takes precedence over an earlier one, as is the OFW convention).


>
> g.
>
>>
>>
>> On 6/21/2011 9:13 AM, Grant Likely wrote:
>>>
>>> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo<shawn.guo@freescale.com>
>>>   wrote:
>>>>
>>>> Hi Grant,
>>>>
>>>> I just gave a try to use aliases node for identify the device index.
>>>> Please take a look and let me know if it's what you expect.
>>>
>>> Thanks Shawn.  This is definitely on track.  Comments below...
>>>
>>>>
>>>> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
>>>> [...]
>>>>>>>
>>>>>>> +#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;
>>>>>>> + const __be32 *line;
>>>>>>> +
>>>>>>> + if (!node)
>>>>>>> +         return -ENODEV;
>>>>>>> +
>>>>>>> + line = of_get_property(node, "id", NULL);
>>>>>>> + if (!line)
>>>>>>> +         return -ENODEV;
>>>>>>> +
>>>>>>> + sport->port.line = be32_to_cpup(line) - 1;
>>>>>>
>>>>>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>>>>>> be enumerated, the driver should look for a /aliases/uart* property
>>>>>> that matches the of_node.  Doing it that way is already established in
>>>>>> the OpenFirmware documentation, and it ensures there are no overlaps
>>>>>> in the global namespace.
>>>>>>
>>>>>
>>>>> I just gave one more try to avoid using 'aliases', and you gave a
>>>>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>>>>> thinking about your suggestion seriously :)
>>>>>
>>>>>> We do need some infrastructure to make that easier though.  Would you
>>>>>> have time to help put that together?
>>>>>>
>>>>> Ok, I will give it a try.
>>>>>
>>>>
>>>> diff --git a/arch/arm/boot/dts/imx51-babbage.dts
>>>> b/arch/arm/boot/dts/imx51-babbage.dts
>>>> index da0381a..f4a5c3c 100644
>>>> --- a/arch/arm/boot/dts/imx51-babbage.dts
>>>> +++ b/arch/arm/boot/dts/imx51-babbage.dts
>>>> @@ -18,6 +18,12 @@
>>>>         compatible = "fsl,imx51-babbage", "fsl,imx51";
>>>>         interrupt-parent =<&tzic>;
>>>>
>>>> +       aliases {
>>>> +               serial0 =&uart0;
>>>> +               serial1 =&uart1;
>>>> +               serial2 =&uart2;
>>>> +       };
>>>> +
>>>
>>> Hmmm.  David Gibson had tossed out an idea of automatically generating
>>> aliases from labels.  It may be time to revisit that idea.
>>>
>>> David, perhaps using this format for a label should turn it into an
>>> alias (prefix label with an asterisk):
>>>
>>>          *thealias: i2c@12340000 { /*...*/ };
>>>
>>> .... although that approach gets *really* hairy when considering that
>>> different boards will want different enumeration.  How would one
>>> override an automatic alias defined by an included .dts file?
>>>
>>>>         chosen {
>>>>                 bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3
>>>> rootwait";
>>>>         };
>>>> @@ -47,29 +53,29 @@
>>>>                         reg =<0x70000000 0x40000>;
>>>>                         ranges;
>>>>
>>>> -                       uart@7000c000 {
>>>> +                       uart2: uart@7000c000 {
>>>>                                 compatible = "fsl,imx51-uart",
>>>> "fsl,imx21-uart";
>>>>                                 reg =<0x7000c000 0x4000>;
>>>>                                 interrupts =<33>;
>>>>                                 id =<3>;
>>>> -                               fsl,has-rts-cts;
>>>> +                               fsl,uart-has-rtscts;
>>>>                         };
>>>>                 };
>>>>
>>>> -               uart@73fbc000 {
>>>> +               uart0: uart@73fbc000 {
>>>>                         compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>>                         reg =<0x73fbc000 0x4000>;
>>>>                         interrupts =<31>;
>>>>                         id =<1>;
>>>> -                       fsl,has-rts-cts;
>>>> +                       fsl,uart-has-rtscts;
>>>>                 };
>>>>
>>>> -               uart@73fc0000 {
>>>> +               uart1: uart@73fc0000 {
>>>>                         compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>>                         reg =<0x73fc0000 0x4000>;
>>>>                         interrupts =<32>;
>>>>                         id =<2>;
>>>> -                       fsl,has-rts-cts;
>>>> +                       fsl,uart-has-rtscts;
>>>>                 };
>>>>         };
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index 632ebae..13df5d2 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -737,6 +737,37 @@ err0:
>>>>   EXPORT_SYMBOL(of_parse_phandles_with_args);
>>>>
>>>>   /**
>>>> + *     of_get_device_index - Get device index by looking up "aliases"
>>>> node
>>>> + *     @np:    Pointer to device node that asks for device index
>>>> + *     @name:  The device alias without index number
>>>> + *
>>>> + *     Returns the device index if find it, else returns -ENODEV.
>>>> + */
>>>> +int of_get_device_index(struct device_node *np, const char *alias)
>>>> +{
>>>> +       struct device_node *aliases = of_find_node_by_name(NULL,
>>>> "aliases");
>>>> +       struct property *prop;
>>>> +       char name[32];
>>>> +       int index = 0;
>>>> +
>>>> +       if (!aliases)
>>>> +               return -ENODEV;
>>>> +
>>>> +       while (1) {
>>>> +               snprintf(name, sizeof(name), "%s%d", alias, index);
>>>> +               prop = of_find_property(aliases, name, NULL);
>>>> +               if (!prop)
>>>> +                       return -ENODEV;
>>>> +               if (np == of_find_node_by_path(prop->value))
>>>> +                       break;
>>>> +               index++;
>>>> +       }
>>>
>>> Rather than parsing the alias strings everytime, it would probably be
>>> better to preprocess all the properties in the aliases node and create
>>> a lookup table of alias->node references that can be walked quickly
>>> and trivially.
>>>
>>> Also, when obtaining an enumeration for a device, you'll need to be
>>> careful about what number gets returned.  If the node doesn't match a
>>> given alias, but aliases do exist for other devices of like type, then
>>> you need to be careful not to assign a number already assigned to
>>> another device via an alias (this of course assumes the driver
>>> supports dynamics enumeration, which many drivers will).  It would be
>>>
>>> \>    +
>>>>
>>>> +       return index;
>>>> +}
>>>> +EXPORT_SYMBOL(of_get_device_index);
>>>> +
>>>> +/**
>>>>   * prom_add_property - Add a property to a node
>>>>   */
>>>>   int prom_add_property(struct device_node *np, struct property *prop)
>>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>>> index da436e0..852668f 100644
>>>> --- a/drivers/tty/serial/imx.c
>>>> +++ b/drivers/tty/serial/imx.c
>>>> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port
>>>> *sport,
>>>>         struct device_node *node = pdev->dev.of_node;
>>>>         const struct of_device_id *of_id =
>>>>                         of_match_device(imx_uart_dt_ids,&pdev->dev);
>>>> -       const __be32 *line;
>>>> +       int line;
>>>>
>>>>         if (!node)
>>>>                 return -ENODEV;
>>>>
>>>> -       line = of_get_property(node, "id", NULL);
>>>> -       if (!line)
>>>> +       line = of_get_device_index(node, "serial");
>>>> +       if (IS_ERR_VALUE(line))
>>>>                 return -ENODEV;
>>>
>>> Personally, it an alias isn't present, then I'd dynamically assign a port
>>> id.
>>>
>>>>
>>>> -       sport->port.line = be32_to_cpup(line) - 1;
>>>> +       sport->port.line = line;
>>>>
>>>> -       if (of_get_property(node, "fsl,has-rts-cts", NULL))
>>>> +       if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>>>>                 sport->have_rtscts = 1;
>>>>
>>>>         if (of_get_property(node, "fsl,irda-mode", NULL))
>>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>>> index bfc0ed1..3153752 100644
>>>> --- a/include/linux/of.h
>>>> +++ b/include/linux/of.h
>>>> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct
>>>> device_node *np,
>>>>         const char *list_name, const char *cells_name, int index,
>>>>         struct device_node **out_node, const void **out_args);
>>>>
>>>> +extern int of_get_device_index(struct device_node *np, const char
>>>> *alias);
>>>> +
>>>>   extern int of_machine_is_compatible(const char *compat);
>>>>
>>>>   extern int prom_add_property(struct device_node* np, struct property*
>>>> prop);
>>>>
>>>> --
>>>> Regards,
>>>> Shawn
>>>>
>>>>
>>>
>>>
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
>

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 4:08 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> On 6/21/2011 9:38 AM, Grant Likely wrote:
>>
>> On Tue, Jun 21, 2011 at 1:35 PM, Mitch Bradley<wmb@firmworks.com>  wrote:
>>>
>>> What is the problem with
>>>
>>> aliases{
>>>   serial0 = "/uart@7000c000";
>>> }
>>>
>>> Properties in the alias node are supposed to have string values.
>>
>> ?
>>
>> Not sure I follow.  Indeed, properties in the aliases node are string
>> values.
>>
>> Are you referring to how I was proposing some dtc syntax for
>> generating the alias strings?
>
>
> The point is that if you refer to the node explicitly by its string name,
> the need for a label disappears and the problem of overriding a default
> alias disappears (assuming that a later redefinition of a property takes
> precedence over an earlier one, as is the OFW convention).

Ah, we're having an impedance mismatch.  I'm thinking specifically
about the device tree compiler and some syntactic sugar for using the
label definition to generate /also/ create alias properties. The
hairiness is related to that and the way that dtc is implemented, not
with the final aliases themselves.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Segher Boessenkool]

> Ah, we're having an impedance mismatch.  I'm thinking specifically
> about the device tree compiler and some syntactic sugar for using the
> label definition to generate /also/ create alias properties. The
> hairiness is related to that and the way that dtc is implemented, not
> with the final aliases themselves.

You can generate DTC-style aliases from OFW-style aliases instead (or
as well), it has other advantages (like being more readable, and having
the aliases grouped together).


Segher

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Tue, Jun 21, 2011 at 4:52 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>> Ah, we're having an impedance mismatch.  I'm thinking specifically
>> about the device tree compiler and some syntactic sugar for using the
>> label definition to generate /also/ create alias properties. The
>> hairiness is related to that and the way that dtc is implemented, not
>> with the final aliases themselves.
>
> You can generate DTC-style aliases from OFW-style aliases instead (or
> as well), it has other advantages (like being more readable, and having
> the aliases grouped together).

There is no difference between OFW and DTC aliases as far as I'm aware.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> > Hi Grant,
> >
> > I just gave a try to use aliases node for identify the device index.
> > Please take a look and let me know if it's what you expect.
> 
> Thanks Shawn.  This is definitely on track.  Comments below...
> 
> >
> > On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
> > [...]
> >> > >
> >> > > +#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;
> >> > > + const __be32 *line;
> >> > > +
> >> > > + if (!node)
> >> > > +         return -ENODEV;
> >> > > +
> >> > > + line = of_get_property(node, "id", NULL);
> >> > > + if (!line)
> >> > > +         return -ENODEV;
> >> > > +
> >> > > + sport->port.line = be32_to_cpup(line) - 1;
> >> >
> >> > Hmmm, I really would like to be rid of this.  Instead, if uarts must
> >> > be enumerated, the driver should look for a /aliases/uart* property
> >> > that matches the of_node.  Doing it that way is already established in
> >> > the OpenFirmware documentation, and it ensures there are no overlaps
> >> > in the global namespace.
> >> >
> >>
> >> I just gave one more try to avoid using 'aliases', and you gave a
> >> 'no' again.  Now, I know how hard you are on this.  Okay, I start
> >> thinking about your suggestion seriously :)
> >>
> >> > We do need some infrastructure to make that easier though.  Would you
> >> > have time to help put that together?
> >> >
> >> Ok, I will give it a try.
> >>
> >
> > diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> > index da0381a..f4a5c3c 100644
> > --- a/arch/arm/boot/dts/imx51-babbage.dts
> > +++ b/arch/arm/boot/dts/imx51-babbage.dts
> > @@ -18,6 +18,12 @@
> >        compatible = "fsl,imx51-babbage", "fsl,imx51";
> >        interrupt-parent = <&tzic>;
> >
> > +       aliases {
> > +               serial0 = &uart0;
> > +               serial1 = &uart1;
> > +               serial2 = &uart2;
> > +       };
> > +
> 
> Hmmm.  David Gibson had tossed out an idea of automatically generating
> aliases from labels.  It may be time to revisit that idea.
> 
> David, perhaps using this format for a label should turn it into an
> alias (prefix label with an asterisk):
> 
>         *thealias: i2c@12340000 { /*...*/ };
> 
> .... although that approach gets *really* hairy when considering that
> different boards will want different enumeration.  How would one
> override an automatic alias defined by an included .dts file?
> 
Another dependency the patch has to wait for?  Or we can go ahead and
utilize the facility later when it gets ready?

> >        chosen {
> >                bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
> >        };
> > @@ -47,29 +53,29 @@
> >                        reg = <0x70000000 0x40000>;
> >                        ranges;
> >
> > -                       uart@7000c000 {
> > +                       uart2: uart@7000c000 {
> >                                compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> >                                reg = <0x7000c000 0x4000>;
> >                                interrupts = <33>;
> >                                id = <3>;
> > -                               fsl,has-rts-cts;
> > +                               fsl,uart-has-rtscts;
> >                        };
> >                };
> >
> > -               uart@73fbc000 {
> > +               uart0: uart@73fbc000 {
> >                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> >                        reg = <0x73fbc000 0x4000>;
> >                        interrupts = <31>;
> >                        id = <1>;
> > -                       fsl,has-rts-cts;
> > +                       fsl,uart-has-rtscts;
> >                };
> >
> > -               uart@73fc0000 {
> > +               uart1: uart@73fc0000 {
> >                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> >                        reg = <0x73fc0000 0x4000>;
> >                        interrupts = <32>;
> >                        id = <2>;
> > -                       fsl,has-rts-cts;
> > +                       fsl,uart-has-rtscts;
> >                };
> >        };
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 632ebae..13df5d2 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -737,6 +737,37 @@ err0:
> >  EXPORT_SYMBOL(of_parse_phandles_with_args);
> >
> >  /**
> > + *     of_get_device_index - Get device index by looking up "aliases" node
> > + *     @np:    Pointer to device node that asks for device index
> > + *     @name:  The device alias without index number
> > + *
> > + *     Returns the device index if find it, else returns -ENODEV.
> > + */
> > +int of_get_device_index(struct device_node *np, const char *alias)
> > +{
> > +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
> > +       struct property *prop;
> > +       char name[32];
> > +       int index = 0;
> > +
> > +       if (!aliases)
> > +               return -ENODEV;
> > +
> > +       while (1) {
> > +               snprintf(name, sizeof(name), "%s%d", alias, index);
> > +               prop = of_find_property(aliases, name, NULL);
> > +               if (!prop)
> > +                       return -ENODEV;
> > +               if (np == of_find_node_by_path(prop->value))
> > +                       break;
> > +               index++;
> > +       }
> 
> Rather than parsing the alias strings everytime, it would probably be
> better to preprocess all the properties in the aliases node and create
> a lookup table of alias->node references that can be walked quickly
> and trivially.
> 
Ok, I'm thinking about it.  Will probably ask something on details
later.

> Also, when obtaining an enumeration for a device, you'll need to be
> careful about what number gets returned.  If the node doesn't match a
> given alias, but aliases do exist for other devices of like type, then
> you need to be careful not to assign a number already assigned to
> another device via an alias (this of course assumes the driver
> supports dynamics enumeration, which many drivers will).  It would be
> 
Some words not finished?

> \> +
> > +       return index;
> > +}
> > +EXPORT_SYMBOL(of_get_device_index);
> > +
> > +/**
> >  * prom_add_property - Add a property to a node
> >  */
> >  int prom_add_property(struct device_node *np, struct property *prop)
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index da436e0..852668f 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,
> >        struct device_node *node = pdev->dev.of_node;
> >        const struct of_device_id *of_id =
> >                        of_match_device(imx_uart_dt_ids, &pdev->dev);
> > -       const __be32 *line;
> > +       int line;
> >
> >        if (!node)
> >                return -ENODEV;
> >
> > -       line = of_get_property(node, "id", NULL);
> > -       if (!line)
> > +       line = of_get_device_index(node, "serial");
> > +       if (IS_ERR_VALUE(line))
> >                return -ENODEV;
> 
> Personally, it an alias isn't present, then I'd dynamically assign a port id.
> 
We probably can not.  The driver works with being told the correct
port id which is defined by soc.  Instead of dynamically assigning
a port id, we have to tell the driver the exact hardware port id of
the device that is being probed.

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Wed, Jun 22, 2011 at 9:33 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:
>> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
>> > Hi Grant,
>> >
>> > I just gave a try to use aliases node for identify the device index.
>> > Please take a look and let me know if it's what you expect.
>>
>> Thanks Shawn.  This is definitely on track.  Comments below...
>>
>> >
>> > On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
>> > [...]
>> >> > >
>> >> > > +#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;
>> >> > > + const __be32 *line;
>> >> > > +
>> >> > > + if (!node)
>> >> > > +         return -ENODEV;
>> >> > > +
>> >> > > + line = of_get_property(node, "id", NULL);
>> >> > > + if (!line)
>> >> > > +         return -ENODEV;
>> >> > > +
>> >> > > + sport->port.line = be32_to_cpup(line) - 1;
>> >> >
>> >> > Hmmm, I really would like to be rid of this.  Instead, if uarts must
>> >> > be enumerated, the driver should look for a /aliases/uart* property
>> >> > that matches the of_node.  Doing it that way is already established in
>> >> > the OpenFirmware documentation, and it ensures there are no overlaps
>> >> > in the global namespace.
>> >> >
>> >>
>> >> I just gave one more try to avoid using 'aliases', and you gave a
>> >> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>> >> thinking about your suggestion seriously :)
>> >>
>> >> > We do need some infrastructure to make that easier though.  Would you
>> >> > have time to help put that together?
>> >> >
>> >> Ok, I will give it a try.
>> >>
>> >
>> > diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
>> > index da0381a..f4a5c3c 100644
>> > --- a/arch/arm/boot/dts/imx51-babbage.dts
>> > +++ b/arch/arm/boot/dts/imx51-babbage.dts
>> > @@ -18,6 +18,12 @@
>> >        compatible = "fsl,imx51-babbage", "fsl,imx51";
>> >        interrupt-parent = <&tzic>;
>> >
>> > +       aliases {
>> > +               serial0 = &uart0;
>> > +               serial1 = &uart1;
>> > +               serial2 = &uart2;
>> > +       };
>> > +
>>
>> Hmmm.  David Gibson had tossed out an idea of automatically generating
>> aliases from labels.  It may be time to revisit that idea.
>>
>> David, perhaps using this format for a label should turn it into an
>> alias (prefix label with an asterisk):
>>
>>         *thealias: i2c@12340000 { /*...*/ };
>>
>> .... although that approach gets *really* hairy when considering that
>> different boards will want different enumeration.  How would one
>> override an automatic alias defined by an included .dts file?
>>
> Another dependency the patch has to wait for?  Or we can go ahead and
> utilize the facility later when it gets ready?

No, this isn't something you need to wait for.  Just musing on future
enhancements.

>> Also, when obtaining an enumeration for a device, you'll need to be
>> careful about what number gets returned.  If the node doesn't match a
>> given alias, but aliases do exist for other devices of like type, then
>> you need to be careful not to assign a number already assigned to
>> another device via an alias (this of course assumes the driver
>> supports dynamics enumeration, which many drivers will).  It would be
>>
> Some words not finished?

heh, that happens sometimes.  I tend to be a bit scattered when
replying to email.  Just ignore the last sentence fragment.

>> > -       line = of_get_property(node, "id", NULL);
>> > -       if (!line)
>> > +       line = of_get_device_index(node, "serial");
>> > +       if (IS_ERR_VALUE(line))
>> >                return -ENODEV;
>>
>> Personally, it an alias isn't present, then I'd dynamically assign a port id.
>>
> We probably can not.  The driver works with being told the correct
> port id which is defined by soc.  Instead of dynamically assigning
> a port id, we have to tell the driver the exact hardware port id of
> the device that is being probed.

Are you sure?  It doesn't look like the driver behaviour uses id for
anything other than an index into the statically allocated serial port
instance table.  I don't see any change of behaviour based on the port
number anywhere.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Wed, Jun 22, 2011 at 09:52:11AM -0600, Grant Likely wrote:
[...]
> 
> >> > -       line = of_get_property(node, "id", NULL);
> >> > -       if (!line)
> >> > +       line = of_get_device_index(node, "serial");
> >> > +       if (IS_ERR_VALUE(line))
> >> >                return -ENODEV;
> >>
> >> Personally, it an alias isn't present, then I'd dynamically assign a port id.
> >>
> > We probably can not.  The driver works with being told the correct
> > port id which is defined by soc.  Instead of dynamically assigning
> > a port id, we have to tell the driver the exact hardware port id of
> > the device that is being probed.
> 
> Are you sure?  It doesn't look like the driver behaviour uses id for
> anything other than an index into the statically allocated serial port
> instance table.  I don't see any change of behaviour based on the port
> number anywhere.
> 
Sorry, I did not make this clear.  In serial_imx_probe(), the port
gets created and then saved as below.

	imx_ports[sport->port.line] = sport;

While in imx_console_setup(), it addresses the port as following.

	sport = imx_ports[co->index];

When users specify their console as ttymxc0, they mean they are
using the first i.mx uart hardware port, in turn ttymxc1 for the
second port ...

That said, imx_port[0] has to be the first hardware port, imx_port[1]
has to be the second one ...  That's why port id sport->port.line
can not be dynamically assigned, otherwise console may not work.

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Wed, Jun 22, 2011 at 6:12 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Wed, Jun 22, 2011 at 09:52:11AM -0600, Grant Likely wrote:
> [...]
>>
>> >> > -       line = of_get_property(node, "id", NULL);
>> >> > -       if (!line)
>> >> > +       line = of_get_device_index(node, "serial");
>> >> > +       if (IS_ERR_VALUE(line))
>> >> >                return -ENODEV;
>> >>
>> >> Personally, it an alias isn't present, then I'd dynamically assign a port id.
>> >>
>> > We probably can not.  The driver works with being told the correct
>> > port id which is defined by soc.  Instead of dynamically assigning
>> > a port id, we have to tell the driver the exact hardware port id of
>> > the device that is being probed.
>>
>> Are you sure?  It doesn't look like the driver behaviour uses id for
>> anything other than an index into the statically allocated serial port
>> instance table.  I don't see any change of behaviour based on the port
>> number anywhere.
>>
> Sorry, I did not make this clear.  In serial_imx_probe(), the port
> gets created and then saved as below.
>
>        imx_ports[sport->port.line] = sport;
>
> While in imx_console_setup(), it addresses the port as following.
>
>        sport = imx_ports[co->index];
>
> When users specify their console as ttymxc0, they mean they are
> using the first i.mx uart hardware port, in turn ttymxc1 for the
> second port ...
>
> That said, imx_port[0] has to be the first hardware port, imx_port[1]
> has to be the second one ...  That's why port id sport->port.line
> can not be dynamically assigned, otherwise console may not work.

My point still stands.  If there is no number specifically assigned to
a device, then numbering should be dynamic.  That's standard behaviour
for Linux device drivers, and I get nervous every time I see a driver
that make an assumption to the contrary, especially when there is no
good reason for it.  Besides, you still can ensure the console is
reliable by having an alias for the console device.

g.

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:
[...]
> >
> >  /**
> > + *     of_get_device_index - Get device index by looking up "aliases" node
> > + *     @np:    Pointer to device node that asks for device index
> > + *     @name:  The device alias without index number
> > + *
> > + *     Returns the device index if find it, else returns -ENODEV.
> > + */
> > +int of_get_device_index(struct device_node *np, const char *alias)
> > +{
> > +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
> > +       struct property *prop;
> > +       char name[32];
> > +       int index = 0;
> > +
> > +       if (!aliases)
> > +               return -ENODEV;
> > +
> > +       while (1) {
> > +               snprintf(name, sizeof(name), "%s%d", alias, index);
> > +               prop = of_find_property(aliases, name, NULL);
> > +               if (!prop)
> > +                       return -ENODEV;
> > +               if (np == of_find_node_by_path(prop->value))
> > +                       break;
> > +               index++;
> > +       }
> 
> Rather than parsing the alias strings everytime, it would probably be
> better to preprocess all the properties in the aliases node and create
> a lookup table of alias->node references that can be walked quickly
> and trivially.
> 
> Also, when obtaining an enumeration for a device, you'll need to be
> careful about what number gets returned.  If the node doesn't match a
> given alias, but aliases do exist for other devices of like type, then
> you need to be careful not to assign a number already assigned to
> another device via an alias (this of course assumes the driver
> supports dynamics enumeration, which many drivers will).  It would be
> 

Grant, please take a look at the second shot below.  Please let me
know what you think.

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index 7976932..f4a5c3c 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -18,6 +18,12 @@
 	compatible = "fsl,imx51-babbage", "fsl,imx51";
 	interrupt-parent = <&tzic>;
 
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+	};
+
 	chosen {
 		bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
 	};
@@ -47,29 +53,29 @@
 			reg = <0x70000000 0x40000>;
 			ranges;
 
-			uart@7000c000 {
-				compatible = "fsl,imx51-uart", "fsl,imx-uart";
+			uart2: uart@7000c000 {
+				compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 				reg = <0x7000c000 0x4000>;
 				interrupts = <33>;
 				id = <3>;
				fsl,has-rts-cts;
 			};
 		};
 
-		uart@73fbc000 {
-			compatible = "fsl,imx51-uart", "fsl,imx-uart";
+		uart0: uart@73fbc000 {
+			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 			reg = <0x73fbc000 0x4000>;
 			interrupts = <31>;
 			id = <1>;
			fsl,has-rts-cts;
 		};
 
-		uart@73fc0000 {
-			compatible = "fsl,imx51-uart", "fsl,imx-uart";
+		uart1: uart@73fc0000 {
+			compatible = "fsl,imx51-uart", "fsl,imx21-uart";
 			reg = <0x73fc0000 0x4000>;
 			interrupts = <32>;
 			id = <2>;
			fsl,has-rts-cts;
 		};
 	};
 
diff --git a/arch/arm/mach-mx5/imx51-dt.c b/arch/arm/mach-mx5/imx51-dt.c
index 8bfdb91..e6c7298 100644
--- a/arch/arm/mach-mx5/imx51-dt.c
+++ b/arch/arm/mach-mx5/imx51-dt.c
@@ -40,6 +40,8 @@ static const struct of_device_id tzic_of_match[] __initconst = {
 
 static void __init imx51_dt_init(void)
 {
+	of_scan_aliases();
+
 	irq_domain_generate_simple(tzic_of_match, MX51_TZIC_BASE_ADDR, 0);
 
 	of_platform_populate(NULL, of_default_bus_match_table,
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 632ebae..90349a2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -17,12 +17,27 @@
  *      as published by the Free Software Foundation; either version
  *      2 of the License, or (at your option) any later version.
  */
+#include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
 
+struct alias_devname {
+	char devname[32];
+	struct list_head link;
+	struct list_head head;
+};
+
+struct alias_devid {
+	int devid;
+	struct device_node *node;
+	struct list_head link;
+};
+
+static LIST_HEAD(aliases_lookup);
+
 struct device_node *allnodes;
 struct device_node *of_chosen;
 
@@ -922,3 +937,170 @@ out_unlock:
 }
 #endif /* defined(CONFIG_OF_DYNAMIC) */
 
+/*
+ * get_alias_dev_name_id - Get device name and id from alias name
+ *
+ * an:	The alias name passed in
+ * dn:	The pointer used to return device name
+ *
+ * Returns device id which should be the number at the end of alias
+ * name, otherwise returns -1.
+ */
+static int get_alias_name_id(char *an, char *dn)
+{
+	int len = strlen(an);
+	char *end = an + len;
+
+	while (isdigit(*--end))
+		len--;
+
+	end++;
+	strncpy(dn, an, len);
+	dn[len] = '\0';
+
+	return strlen(end) ? simple_strtol(end, NULL, 10) : -1;
+}
+
+/*
+ * get_an_available_devid - Get an available devid for the given devname
+ *
+ * adn:	The pointer to the given alias_devname
+ *
+ * Returns the available devid
+ */
+static int get_an_available_devid(struct alias_devname *adn)
+{
+	int devid = 0;
+	struct alias_devid *adi;
+
+	while (1) {
+		bool used = false;
+		list_for_each_entry(adi, &adn->head, link) {
+			if (adi->devid == devid) {
+				used = true;
+				break;
+			}
+		}
+
+		if (!used)
+			break;
+
+		devid++;
+	}
+
+	return devid;
+}
+
+/*
+ * of_scan_aliases - Scan all properties of aliases node and populate the
+ * 		     global lookup table with the device name and id info
+ *
+ * Returns the number of aliases properties found, or error code in error case.
+ */
+int of_scan_aliases(void)
+{
+	struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
+	struct property *pp;
+	struct alias_devname *adn;
+	struct alias_devid *adi;
+	int ret = 0;
+
+	if (!aliases) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	for (pp = aliases->properties; pp != NULL; pp = pp->next) {
+		bool found = false;
+		char devname[32];
+		int devid = get_alias_name_id(pp->name, devname);
+
+		/* We do not want to proceed this sentinel one */
+		if (!strcmp(pp->name, "name") && !strcmp(pp->value, "aliases"))
+			break;
+
+		/* See if the devname already exists */
+		list_for_each_entry(adn, &aliases_lookup, link) {
+			if (!strcmp(adn->devname, devname)) {
+				found = true;
+				break;
+			}
+		}
+
+		/*
+		 * Create the entry for this devname if not found,
+		 * and add it into aliases_lookup
+		 */
+		if (!found) {
+			adn = kzalloc(sizeof(*adn), GFP_KERNEL);
+			if (!adn) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			strcpy(adn->devname, devname);
+			INIT_LIST_HEAD(&adn->head);
+			list_add_tail(&adn->link, &aliases_lookup);
+		}
+
+		/*
+		 * Save the devid as one entry of the list for this
+		 * specified devname
+		 */
+		adi = kzalloc(sizeof(*adi), GFP_KERNEL);
+		if (!adi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		adi->devid = (devid == -1) ? get_an_available_devid(adn) :
+					     devid;
+		adi->node = of_find_node_by_path(pp->value);
+
+		list_add_tail(&adi->link, &adn->head);
+		ret++;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ *	of_get_device_id - Get device id by looking up "aliases" node
+ *	@np:	Pointer to device node that asks for device id
+ *	@name:	The device alias name
+ *
+ *	Returns the device id if find it, else returns -ENODEV.
+ */
+int of_get_device_id(struct device_node *np, const char *name)
+{
+	struct alias_devname *adn;
+	struct alias_devid *adi;
+	bool found = false;
+	int ret;
+
+	list_for_each_entry(adn, &aliases_lookup, link) {
+		if (!strcmp(adn->devname, name)) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	found = false;
+	list_for_each_entry(adi, &adn->head, link) {
+		if (np == adi->node) {
+			found = true;
+			break;
+		}
+	}
+
+	ret = found ? adi->devid : -ENODEV;
+out:
+	return ret;
+}
+EXPORT_SYMBOL(of_get_device_id);
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 2769353..062639e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1225,43 +1265,33 @@ static int serial_imx_resume(struct platform_device *dev)
 	return 0;
 }
 
 static int serial_imx_probe_dt(struct imx_port *sport,
 		struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
-	const __be32 *line;
+	int line;
 
 	if (!node)
 		return -ENODEV;
 
-	line = of_get_property(node, "id", NULL);
-	if (!line)
+	line = of_get_device_id(node, "serial");
+	if (IS_ERR_VALUE(line))
 		return -ENODEV;
 
-	sport->port.line = be32_to_cpup(line) - 1;
+	sport->port.line = line;
 
diff --git a/include/linux/of.h b/include/linux/of.h
index bfc0ed1..270c671 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -213,6 +213,9 @@ extern int of_parse_phandles_with_args(struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct device_node **out_node, const void **out_args);
 
+extern int of_scan_aliases(void);
+extern int of_get_device_id(struct device_node *np, const char *name);
+
 extern int of_machine_is_compatible(const char *compat);
 
 extern int prom_add_property(struct device_node* np, struct property* prop);

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Thu, Jun 23, 2011 at 12:38 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:
> [...]
>> >
>> >  /**
>> > + *     of_get_device_index - Get device index by looking up "aliases" node
>> > + *     @np:    Pointer to device node that asks for device index
>> > + *     @name:  The device alias without index number
>> > + *
>> > + *     Returns the device index if find it, else returns -ENODEV.
>> > + */
>> > +int of_get_device_index(struct device_node *np, const char *alias)
>> > +{
>> > +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
>> > +       struct property *prop;
>> > +       char name[32];
>> > +       int index = 0;
>> > +
>> > +       if (!aliases)
>> > +               return -ENODEV;
>> > +
>> > +       while (1) {
>> > +               snprintf(name, sizeof(name), "%s%d", alias, index);
>> > +               prop = of_find_property(aliases, name, NULL);
>> > +               if (!prop)
>> > +                       return -ENODEV;
>> > +               if (np == of_find_node_by_path(prop->value))
>> > +                       break;
>> > +               index++;
>> > +       }
>>
>> Rather than parsing the alias strings everytime, it would probably be
>> better to preprocess all the properties in the aliases node and create
>> a lookup table of alias->node references that can be walked quickly
>> and trivially.
>>
>> Also, when obtaining an enumeration for a device, you'll need to be
>> careful about what number gets returned.  If the node doesn't match a
>> given alias, but aliases do exist for other devices of like type, then
>> you need to be careful not to assign a number already assigned to
>> another device via an alias (this of course assumes the driver
>> supports dynamics enumeration, which many drivers will).  It would be
>>
>
> Grant, please take a look at the second shot below.  Please let me
> know what you think.

Hey Shawn, good progress.  Comments below.

Also, once you've got this sorted out, you'll need to break the
drivers/of/ bits out into a separate patch so I can apply it
separately.

g.

>
> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> index 7976932..f4a5c3c 100644
> --- a/arch/arm/boot/dts/imx51-babbage.dts
> +++ b/arch/arm/boot/dts/imx51-babbage.dts
> @@ -18,6 +18,12 @@
>        compatible = "fsl,imx51-babbage", "fsl,imx51";
>        interrupt-parent = <&tzic>;
>
> +       aliases {
> +               serial0 = &uart0;
> +               serial1 = &uart1;
> +               serial2 = &uart2;
> +       };
> +
>        chosen {
>                bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
>        };
> @@ -47,29 +53,29 @@
>                        reg = <0x70000000 0x40000>;
>                        ranges;
>
> -                       uart@7000c000 {
> -                               compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +                       uart2: uart@7000c000 {
> +                               compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                                reg = <0x7000c000 0x4000>;
>                                interrupts = <33>;
>                                id = <3>;
>                                fsl,has-rts-cts;
>                        };
>                };
>
> -               uart@73fbc000 {
> -                       compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +               uart0: uart@73fbc000 {
> +                       compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                        reg = <0x73fbc000 0x4000>;
>                        interrupts = <31>;
>                        id = <1>;
>                        fsl,has-rts-cts;
>                };
>
> -               uart@73fc0000 {
> -                       compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +               uart1: uart@73fc0000 {
> +                       compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>                        reg = <0x73fc0000 0x4000>;
>                        interrupts = <32>;
>                        id = <2>;
>                        fsl,has-rts-cts;
>                };
>        };
>
> diff --git a/arch/arm/mach-mx5/imx51-dt.c b/arch/arm/mach-mx5/imx51-dt.c
> index 8bfdb91..e6c7298 100644
> --- a/arch/arm/mach-mx5/imx51-dt.c
> +++ b/arch/arm/mach-mx5/imx51-dt.c
> @@ -40,6 +40,8 @@ static const struct of_device_id tzic_of_match[] __initconst = {
>
>  static void __init imx51_dt_init(void)
>  {
> +       of_scan_aliases();
> +

Instead of calling this from board code.  You can add the call
directly to the bottom of unflatten_device_tree() in drivers/of/fdt.c

>        irq_domain_generate_simple(tzic_of_match, MX51_TZIC_BASE_ADDR, 0);
>
>        of_platform_populate(NULL, of_default_bus_match_table,
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 632ebae..90349a2 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -17,12 +17,27 @@
>  *      as published by the Free Software Foundation; either version
>  *      2 of the License, or (at your option) any later version.
>  */
> +#include <linux/ctype.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/proc_fs.h>
>
> +struct alias_devname {
> +       char devname[32];
> +       struct list_head link;
> +       struct list_head head;
> +};
> +
> +struct alias_devid {
> +       int devid;
> +       struct device_node *node;
> +       struct list_head link;
> +};

Some LinuxDoc documentation on the meaning of these structures would
be helpful.  I'm not convinced that a two level lookup table is really
necessary.  A flat table containing alias, device_node pointer, and
possibly decoded devname and id is probably sufficient to get started.
 Also, I think it will still be useful to store a pointer to the
actual alias name in the alias_devid record.

> +
> +static LIST_HEAD(aliases_lookup);
> +
>  struct device_node *allnodes;
>  struct device_node *of_chosen;
>
> @@ -922,3 +937,170 @@ out_unlock:
>  }
>  #endif /* defined(CONFIG_OF_DYNAMIC) */
>
> +/*
> + * get_alias_dev_name_id - Get device name and id from alias name
> + *
> + * an: The alias name passed in
> + * dn: The pointer used to return device name

There is actually little point in decoding an alias to the device
name.  It is more useful to decode alias to the device_node pointer
which can be found with of_find_node_by_path().  I'd like to have a
lookup table generated which contains {const char *alias_name,
device_node *np} pairs.  It would also be useful for that table to
decode the 'id' from the end of the alias name when available.  Then,
given an alias stem and id (like imxuart and 2) the code could match
it to alias imxuart0 and look up the device_node associated with (I
could see this used by console setup code).  Alternately, driver probe
code could use its device_node pointer to lookup its alias, and if no
alias exists, then use the table to find an unused id (and possibly
even add an entry to the table when it allocates an id).

> + *
> + * Returns device id which should be the number at the end of alias
> + * name, otherwise returns -1.
> + */
> +static int get_alias_name_id(char *an, char *dn)

Even private static functions should have a prefix consistent with the
file.  In this case, all the functions should probably be something in
the form "of_alias_*()"

> +{
> +       int len = strlen(an);
> +       char *end = an + len;
> +
> +       while (isdigit(*--end))
> +               len--;

Clever!  :-)

> +
> +       end++;
> +       strncpy(dn, an, len);
> +       dn[len] = '\0';
> +
> +       return strlen(end) ? simple_strtol(end, NULL, 10) : -1;

Just to be pendantic: simple_strtoul()  :-)

> +}
> +
> +/*
> + * get_an_available_devid - Get an available devid for the given devname
> + *
> + * adn:        The pointer to the given alias_devname
> + *
> + * Returns the available devid
> + */
> +static int get_an_available_devid(struct alias_devname *adn)
> +{
> +       int devid = 0;
> +       struct alias_devid *adi;
> +
> +       while (1) {
> +               bool used = false;
> +               list_for_each_entry(adi, &adn->head, link) {
> +                       if (adi->devid == devid) {
> +                               used = true;
> +                               break;
> +                       }
> +               }
> +
> +               if (!used)
> +                       break;
> +
> +               devid++;
> +       }
> +
> +       return devid;
> +}
> +
> +/*
> + * of_scan_aliases - Scan all properties of aliases node and populate the
> + *                  global lookup table with the device name and id info
> + *
> + * Returns the number of aliases properties found, or error code in error case.
> + */

Use LinuxDoc format for documentation blocks.
Documentation/kernel-doc-nano-HOWTO.txt

> +int of_scan_aliases(void)
> +{
> +       struct device_node *aliases = of_find_node_by_name(NULL, "aliases");

Like the chosen node, it is useful to keep around a reference to the
aliases node.  There is other code that will use it that I hope to
merge soon.  You can add a global of_aliases pointer and initialized
it in unflatten_device_tree()

> +       struct property *pp;
> +       struct alias_devname *adn;
> +       struct alias_devid *adi;
> +       int ret = 0;
> +
> +       if (!aliases) {
> +               ret = -ENODEV;
> +               goto out;

The function hasn't done anything that needs unwinding yet. Just
return immediately.

> +       }
> +
> +       for (pp = aliases->properties; pp != NULL; pp = pp->next) {

A "for_each_property()" macro would be useful to have and use here.
Can you add one to include/linux/of.h?

> +               bool found = false;
> +               char devname[32];

Rather than a static sized string, I'd like this to be the actual size
of the string.  You can do this by making the name the last element of
the list and giving it a [0] length.  Then when memory is kzalloced
for it, the size of the devname can be added to the end:

struct alias_devname {
       struct list_head link;
       const char *alias;
       struct device_node *node;
       int alias_id;
       char alias_stem[0];
};

> +               int devid = get_alias_name_id(pp->name, devname);
> +
> +               /* We do not want to proceed this sentinel one */
> +               if (!strcmp(pp->name, "name") && !strcmp(pp->value, "aliases"))
> +                       break;

Skipping the 'name' property is good, but I don't think you need to
check the value.  You should also skip the "phandle" property.

> +
> +               /* See if the devname already exists */
> +               list_for_each_entry(adn, &aliases_lookup, link) {
> +                       if (!strcmp(adn->devname, devname)) {
> +                               found = true;
> +                               break;
> +                       }
> +               }
> +
> +               /*
> +                * Create the entry for this devname if not found,
> +                * and add it into aliases_lookup
> +                */
> +               if (!found) {
> +                       adn = kzalloc(sizeof(*adn), GFP_KERNEL);
> +                       if (!adn) {
> +                               ret = -ENOMEM;
> +                               goto out;
> +                       }
> +
> +                       strcpy(adn->devname, devname);
> +                       INIT_LIST_HEAD(&adn->head);
> +                       list_add_tail(&adn->link, &aliases_lookup);
> +               }
> +
> +               /*
> +                * Save the devid as one entry of the list for this
> +                * specified devname
> +                */
> +               adi = kzalloc(sizeof(*adi), GFP_KERNEL);
> +               if (!adi) {
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +
> +               adi->devid = (devid == -1) ? get_an_available_devid(adn) :
> +                                            devid;
> +               adi->node = of_find_node_by_path(pp->value);
> +
> +               list_add_tail(&adi->link, &adn->head);
> +               ret++;

Going to a single level lookup table will certainly simplify this function.

> +       }
> +
> +out:
> +       return ret;
> +}
> +
> +/**
> + *     of_get_device_id - Get device id by looking up "aliases" node
> + *     @np:    Pointer to device node that asks for device id
> + *     @name:  The device alias name
> + *
> + *     Returns the device id if find it, else returns -ENODEV.
> + */
> +int of_get_device_id(struct device_node *np, const char *name)
> +{
> +       struct alias_devname *adn;
> +       struct alias_devid *adi;
> +       bool found = false;
> +       int ret;
> +
> +       list_for_each_entry(adn, &aliases_lookup, link) {
> +               if (!strcmp(adn->devname, name)) {
> +                       found = true;
> +                       break;
> +               }
> +       }
> +
> +       if (!found) {
> +               ret = -ENODEV;
> +               goto out;
> +       }
> +
> +       found = false;
> +       list_for_each_entry(adi, &adn->head, link) {
> +               if (np == adi->node) {
> +                       found = true;
> +                       break;
> +               }
> +       }
> +
> +       ret = found ? adi->devid : -ENODEV;
> +out:
> +       return ret;
> +}
> +EXPORT_SYMBOL(of_get_device_id);
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 2769353..062639e 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1225,43 +1265,33 @@ static int serial_imx_resume(struct platform_device *dev)
>        return 0;
>  }
>
>  static int serial_imx_probe_dt(struct imx_port *sport,
>                struct platform_device *pdev)
>  {
>        struct device_node *node = pdev->dev.of_node;
> -       const __be32 *line;
> +       int line;
>
>        if (!node)
>                return -ENODEV;
>
> -       line = of_get_property(node, "id", NULL);
> -       if (!line)
> +       line = of_get_device_id(node, "serial");
> +       if (IS_ERR_VALUE(line))

if (line < 0) is a sufficient test.  I don't much like the IS_ERR_VALUE() macro.

>                return -ENODEV;
>
> -       sport->port.line = be32_to_cpup(line) - 1;
> +       sport->port.line = line;
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index bfc0ed1..270c671 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -213,6 +213,9 @@ extern int of_parse_phandles_with_args(struct device_node *np,
>        const char *list_name, const char *cells_name, int index,
>        struct device_node **out_node, const void **out_args);
>
> +extern int of_scan_aliases(void);
> +extern int of_get_device_id(struct device_node *np, const char *name);
> +
>  extern int of_machine_is_compatible(const char *compat);
>
>  extern int prom_add_property(struct device_node* np, struct property* prop);
>
> --
> Regards,
> Shawn
>
>



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

Re: [PATCH 1/3] serial/imx: add device tree support

[Shawn Guo]

On Thu, Jun 23, 2011 at 05:11:54PM -0600, Grant Likely wrote:
[...]
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 632ebae..90349a2 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -17,12 +17,27 @@
> >  *      as published by the Free Software Foundation; either version
> >  *      2 of the License, or (at your option) any later version.
> >  */
> > +#include <linux/ctype.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/slab.h>
> >  #include <linux/proc_fs.h>
> >
> > +struct alias_devname {
> > +       char devname[32];
> > +       struct list_head link;
> > +       struct list_head head;
> > +};
> > +
> > +struct alias_devid {
> > +       int devid;
> > +       struct device_node *node;
> > +       struct list_head link;
> > +};
> 
> Some LinuxDoc documentation on the meaning of these structures would
> be helpful.  I'm not convinced that a two level lookup table is really
> necessary.  A flat table containing alias, device_node pointer, and
> possibly decoded devname and id is probably sufficient to get started.
>  Also, I think it will still be useful to store a pointer to the
> actual alias name in the alias_devid record.
> 
I thought two level lookup with driver probe function passing in
stem name will get the matching faster.  But I agree with you that
a flat table may be sufficient to get started, since practically
the alias number will not be that huge.

> > +
> > +static LIST_HEAD(aliases_lookup);
> > +
> >  struct device_node *allnodes;
> >  struct device_node *of_chosen;
> >
> > @@ -922,3 +937,170 @@ out_unlock:
> >  }
> >  #endif /* defined(CONFIG_OF_DYNAMIC) */
> >
> > +/*
> > + * get_alias_dev_name_id - Get device name and id from alias name
> > + *
> > + * an: The alias name passed in
> > + * dn: The pointer used to return device name
> 
> There is actually little point in decoding an alias to the device
> name.  It is more useful to decode alias to the device_node pointer
> which can be found with of_find_node_by_path().  I'd like to have a
> lookup table generated which contains {const char *alias_name,
> device_node *np} pairs.  It would also be useful for that table to
> decode the 'id' from the end of the alias name when available.  Then,
> given an alias stem and id (like imxuart and 2) the code could match
> it to alias imxuart0 and look up the device_node associated with (I
> could see this used by console setup code).  Alternately, driver probe

Yes, this is definitely one way to match.  But it's not as handy as
the alternate one below, in terms of migrating the current platform
drivers that use pdev->id everywhere to dt.  For the imx console
setup example, we can get the device_node by matching alias stem and
id, but we have to address the port we need with another two
contains_of(), device_node -> dev -> port.

> code could use its device_node pointer to lookup its alias, and if no
> alias exists, then use the table to find an unused id (and possibly
> even add an entry to the table when it allocates an id).
> 
And this is easier for the current platform drivers to migrate to
dt, so I would keep purchasing this one.

[...]
> > +               int devid = get_alias_name_id(pp->name, devname);
> > +
> > +               /* We do not want to proceed this sentinel one */
> > +               if (!strcmp(pp->name, "name") && !strcmp(pp->value, "aliases"))
> > +                       break;
> 
> Skipping the 'name' property is good, but I don't think you need to
> check the value.  You should also skip the "phandle" property.
> 
Ok. I have not seen "phandle" property in aliases node though.  Can
you give me an example, so that I can make one up for testing?

-- 
Regards,
Shawn

Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely]

On Thu, Jun 23, 2011 at 9:49 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Thu, Jun 23, 2011 at 05:11:54PM -0600, Grant Likely wrote:
>> > +               int devid = get_alias_name_id(pp->name, devname);
>> > +
>> > +               /* We do not want to proceed this sentinel one */
>> > +               if (!strcmp(pp->name, "name") && !strcmp(pp->value, "aliases"))
>> > +                       break;
>>
>> Skipping the 'name' property is good, but I don't think you need to
>> check the value.  You should also skip the "phandle" property.
>>
> Ok. I have not seen "phandle" property in aliases node though.  Can
> you give me an example, so that I can make one up for testing?

It's a standard property that shows up automatically if any node
happens to have a phandle reference to another node.  The name will be
either "phandle" or "linux,phandle".  Search for "linux,phandle" in
drivers/of/fdt.c to see how that property is processed.

g.