* [PATCH 0/2] Add device tree probe support for tty/serial/imx
@ 2011-07-03 7:55 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:55 UTC (permalink / raw)
To: linux-serial; +Cc: linux-arm-kernel, devicetree-discuss, patches
The first patch removes the use of cpu_is_mx1(). It has no dependency
on other patches, and can be merged right away after review gets done.
I assume it will go through Sascha's tree with Alan's ack, since it
touches quite some arch codes. Please let me know if it's not the case.
The second patch adds device tree probe, and depends on the patch
below.
http://article.gmane.org/gmane.linux.drivers.devicetree/6128
Grant, how does the above patch look to you? I have been seeing
a number of drivers depending on it when migrating to device tree.
It will be good if we can git it settled soon.
Shawn Guo (2):
serial/imx: get rid of the uses of cpu_is_mx1()
serial/imx: add device tree probe support
.../bindings/tty/serial/fsl-imx-uart.txt | 19 +++
arch/arm/mach-imx/clock-imx1.c | 6 +-
arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
drivers/tty/serial/imx.c | 131 +++++++++++++++++--
4 files changed, 140 insertions(+), 18 deletions(-)
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 0/2] Add device tree probe support for tty/serial/imx
@ 2011-07-03 7:55 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:55 UTC (permalink / raw)
To: linux-arm-kernel
The first patch removes the use of cpu_is_mx1(). It has no dependency
on other patches, and can be merged right away after review gets done.
I assume it will go through Sascha's tree with Alan's ack, since it
touches quite some arch codes. Please let me know if it's not the case.
The second patch adds device tree probe, and depends on the patch
below.
http://article.gmane.org/gmane.linux.drivers.devicetree/6128
Grant, how does the above patch look to you? I have been seeing
a number of drivers depending on it when migrating to device tree.
It will be good if we can git it settled soon.
Shawn Guo (2):
serial/imx: get rid of the uses of cpu_is_mx1()
serial/imx: add device tree probe support
.../bindings/tty/serial/fsl-imx-uart.txt | 19 +++
arch/arm/mach-imx/clock-imx1.c | 6 +-
arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
drivers/tty/serial/imx.c | 131 +++++++++++++++++--
4 files changed, 140 insertions(+), 18 deletions(-)
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-03 7:55 ` Shawn Guo
@ 2011-07-03 7:55 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:55 UTC (permalink / raw)
To: linux-serial
Cc: linux-arm-kernel, devicetree-discuss, patches, Shawn Guo,
Sascha Hauer, Alan Cox
The patch removes all the uses of cpu_is_mx1(). Instead, it uses
the .id_table of platform_driver to distinguish the uart device type.
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
arch/arm/mach-imx/clock-imx1.c | 6 +-
arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
3 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
index dcc4172..4aabeb2 100644
--- a/arch/arm/mach-imx/clock-imx1.c
+++ b/arch/arm/mach-imx/clock-imx1.c
@@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
_REGISTER_CLOCK(NULL, "mma", mma_clk)
_REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
_REGISTER_CLOCK(NULL, "gpt", gpt_clk)
- _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
- _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
- _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
_REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
_REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
_REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
index cfce8c9..477271a 100644
--- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
+++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
@@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
},
};
- return imx_add_platform_device("imx-uart", data->id, res,
+ return imx_add_platform_device("imx1-uart", data->id, res,
ARRAY_SIZE(res), pdata, sizeof(*pdata));
}
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 22fe801..983f3bd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -48,7 +48,6 @@
#include <asm/io.h>
#include <asm/irq.h>
-#include <mach/hardware.h>
#include <mach/imx-uart.h>
/* Register definitions */
@@ -67,7 +66,8 @@
#define UBMR 0xa8 /* BRM Modulator Register */
#define UBRC 0xac /* Baud Rate Count Register */
#define MX2_ONEMS 0xb0 /* One Millisecond register */
-#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
+#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
+#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
/* UART Control Register Bit Fields.*/
#define URXD_CHARRDY (1<<15)
@@ -181,6 +181,17 @@
#define UART_NR 8
+enum imx_uart_type {
+ IMX1_UART,
+ IMX2_UART,
+};
+
+/* device type dependent stuff */
+struct imx_uart_data {
+ unsigned uts_reg;
+ enum imx_uart_type devtype;
+};
+
struct imx_port {
struct uart_port port;
struct timer_list timer;
@@ -192,6 +203,7 @@ struct imx_port {
unsigned int irda_inv_tx:1;
unsigned short trcv_delay; /* transceiver delay */
struct clk *clk;
+ struct imx_uart_data *devdata;
};
#ifdef CONFIG_IRDA
@@ -200,6 +212,33 @@ struct imx_port {
#define USE_IRDA(sport) (0)
#endif
+#define UTS (sport->devdata->uts_reg)
+#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
+#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
+
+static struct imx_uart_data imx_uart_devdata[] = {
+ [IMX1_UART] = {
+ .uts_reg = MX1_UTS,
+ .devtype = IMX1_UART,
+ },
+ [IMX2_UART] = {
+ .uts_reg = MX2_UTS,
+ .devtype = IMX2_UART,
+ },
+};
+
+static struct platform_device_id imx_uart_devtype[] = {
+ {
+ .name = "imx1-uart",
+ .driver_data = IMX1_UART,
+ }, {
+ .name = "imx-uart",
+ .driver_data = IMX2_UART,
+ }, {
+ /* sentinel */
+ }
+};
+
/*
* Handle any change of modem status signal since we were last called.
*/
@@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
}
}
- if (!cpu_is_mx1()) {
+ if (IS_IMX2_UART()) {
temp = readl(sport->port.membase + UCR3);
temp |= MX2_UCR3_RXDMUXSEL;
writel(temp, sport->port.membase + UCR3);
@@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
writel(num, sport->port.membase + UBIR);
writel(denom, sport->port.membase + UBMR);
- if (!cpu_is_mx1())
+ if (IS_IMX2_UART())
writel(sport->port.uartclk / div / 1000,
sport->port.membase + MX2_ONEMS);
@@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
old_ucr2 = readl(sport->port.membase + UCR2);
- if (cpu_is_mx1())
+ if (IS_IMX1_UART())
ucr1 |= MX1_UCR1_UARTCLKEN;
ucr1 |= UCR1_UARTEN;
ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
@@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
init_timer(&sport->timer);
sport->timer.function = imx_timeout;
sport->timer.data = (unsigned long)sport;
+ sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
sport->clk = clk_get(&pdev->dev, "uart");
if (IS_ERR(sport->clk)) {
@@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
.suspend = serial_imx_suspend,
.resume = serial_imx_resume,
+ .id_table = imx_uart_devtype,
.driver = {
.name = "imx-uart",
.owner = THIS_MODULE,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-03 7:55 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:55 UTC (permalink / raw)
To: linux-arm-kernel
The patch removes all the uses of cpu_is_mx1(). Instead, it uses
the .id_table of platform_driver to distinguish the uart device type.
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
arch/arm/mach-imx/clock-imx1.c | 6 +-
arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
3 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
index dcc4172..4aabeb2 100644
--- a/arch/arm/mach-imx/clock-imx1.c
+++ b/arch/arm/mach-imx/clock-imx1.c
@@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
_REGISTER_CLOCK(NULL, "mma", mma_clk)
_REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
_REGISTER_CLOCK(NULL, "gpt", gpt_clk)
- _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
- _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
- _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
+ _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
_REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
_REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
_REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
index cfce8c9..477271a 100644
--- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
+++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
@@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
},
};
- return imx_add_platform_device("imx-uart", data->id, res,
+ return imx_add_platform_device("imx1-uart", data->id, res,
ARRAY_SIZE(res), pdata, sizeof(*pdata));
}
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 22fe801..983f3bd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -48,7 +48,6 @@
#include <asm/io.h>
#include <asm/irq.h>
-#include <mach/hardware.h>
#include <mach/imx-uart.h>
/* Register definitions */
@@ -67,7 +66,8 @@
#define UBMR 0xa8 /* BRM Modulator Register */
#define UBRC 0xac /* Baud Rate Count Register */
#define MX2_ONEMS 0xb0 /* One Millisecond register */
-#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
+#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
+#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
/* UART Control Register Bit Fields.*/
#define URXD_CHARRDY (1<<15)
@@ -181,6 +181,17 @@
#define UART_NR 8
+enum imx_uart_type {
+ IMX1_UART,
+ IMX2_UART,
+};
+
+/* device type dependent stuff */
+struct imx_uart_data {
+ unsigned uts_reg;
+ enum imx_uart_type devtype;
+};
+
struct imx_port {
struct uart_port port;
struct timer_list timer;
@@ -192,6 +203,7 @@ struct imx_port {
unsigned int irda_inv_tx:1;
unsigned short trcv_delay; /* transceiver delay */
struct clk *clk;
+ struct imx_uart_data *devdata;
};
#ifdef CONFIG_IRDA
@@ -200,6 +212,33 @@ struct imx_port {
#define USE_IRDA(sport) (0)
#endif
+#define UTS (sport->devdata->uts_reg)
+#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
+#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
+
+static struct imx_uart_data imx_uart_devdata[] = {
+ [IMX1_UART] = {
+ .uts_reg = MX1_UTS,
+ .devtype = IMX1_UART,
+ },
+ [IMX2_UART] = {
+ .uts_reg = MX2_UTS,
+ .devtype = IMX2_UART,
+ },
+};
+
+static struct platform_device_id imx_uart_devtype[] = {
+ {
+ .name = "imx1-uart",
+ .driver_data = IMX1_UART,
+ }, {
+ .name = "imx-uart",
+ .driver_data = IMX2_UART,
+ }, {
+ /* sentinel */
+ }
+};
+
/*
* Handle any change of modem status signal since we were last called.
*/
@@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
}
}
- if (!cpu_is_mx1()) {
+ if (IS_IMX2_UART()) {
temp = readl(sport->port.membase + UCR3);
temp |= MX2_UCR3_RXDMUXSEL;
writel(temp, sport->port.membase + UCR3);
@@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
writel(num, sport->port.membase + UBIR);
writel(denom, sport->port.membase + UBMR);
- if (!cpu_is_mx1())
+ if (IS_IMX2_UART())
writel(sport->port.uartclk / div / 1000,
sport->port.membase + MX2_ONEMS);
@@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
old_ucr2 = readl(sport->port.membase + UCR2);
- if (cpu_is_mx1())
+ if (IS_IMX1_UART())
ucr1 |= MX1_UCR1_UARTCLKEN;
ucr1 |= UCR1_UARTEN;
ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
@@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
init_timer(&sport->timer);
sport->timer.function = imx_timeout;
sport->timer.data = (unsigned long)sport;
+ sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
sport->clk = clk_get(&pdev->dev, "uart");
if (IS_ERR(sport->clk)) {
@@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
.suspend = serial_imx_suspend,
.resume = serial_imx_resume,
+ .id_table = imx_uart_devtype,
.driver = {
.name = "imx-uart",
.owner = THIS_MODULE,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
2011-07-03 7:55 ` Shawn Guo
@ 2011-07-03 7:56 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:56 UTC (permalink / raw)
To: linux-serial
Cc: linux-arm-kernel, devicetree-discuss, patches, Shawn Guo,
Jeremy Kerr, Jason Liu, Sascha Hauer, Alan Cox, Grant Likely
It adds device tree probe 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>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
.../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
drivers/tty/serial/imx.c | 82 +++++++++++++++++---
2 files changed, 91 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..a9c0406
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
@@ -0,0 +1,19 @@
+* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
+
+Required properties:
+- compatible : Should be "fsl,<soc>-uart"
+- reg : Address and length of the register set for the device
+- interrupts : Should contain uart interrupt
+
+Optional properties:
+- fsl,uart-has-rtscts : Indicate the uart has rts and cts
+- fsl,irda-mode : Indicate the uart supports irda mode
+
+Example:
+
+uart@73fbc000 {
+ compatible = "fsl,imx51-uart", "fsl,imx21-uart";
+ reg = <0x73fbc000 0x4000>;
+ interrupts = <31>;
+ fsl,uart-has-rtscts;
+};
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 983f3bd..483c447 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_device.h>
#include <asm/io.h>
#include <asm/irq.h>
@@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
}
};
+static struct of_device_id imx_uart_dt_ids[] = {
+ { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
+ { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
+ { /* sentinel */ },
+};
+
/*
* Handle any change of modem status signal since we were last called.
*/
@@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
+ const struct of_device_id *of_id =
+ of_match_device(imx_uart_dt_ids, &pdev->dev);
+
+ if (!np)
+ return -ENODEV;
+
+ pdev->id = of_alias_get_id(np, "serial");
+ if (pdev->id < 0)
+ return -ENODEV;
+
+ if (of_get_property(np, "fsl,uart-has-rtscts", NULL))
+ sport->have_rtscts = 1;
+
+ if (of_get_property(np, "fsl,irda-mode", NULL))
+ sport->use_irda = 1;
+
+ sport->devdata = of_id->data;
+
+ return 0;
+}
+#else
+static inline int serial_imx_probe_dt(struct imx_port *sport,
+ struct platform_device *pdev)
+{
+ return -ENODEV;
+}
+#endif
+
+static int serial_imx_probe_pdata(struct imx_port *sport,
+ struct platform_device *pdev)
+{
+ struct imxuart_platform_data *pdata = pdev->dev.platform_data;
+
+ if (!pdata)
+ return 0;
+
+ if (pdata->flags & IMXUART_HAVE_RTSCTS)
+ sport->have_rtscts = 1;
+
+ if (pdata->flags & IMXUART_IRDA)
+ sport->use_irda = 1;
+
+ sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
+
+ return 0;
+}
+
static int serial_imx_probe(struct platform_device *pdev)
{
struct imx_port *sport;
@@ -1273,6 +1333,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;
@@ -1301,7 +1367,6 @@ static int serial_imx_probe(struct platform_device *pdev)
init_timer(&sport->timer);
sport->timer.function = imx_timeout;
sport->timer.data = (unsigned long)sport;
- sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
sport->clk = clk_get(&pdev->dev, "uart");
if (IS_ERR(sport->clk)) {
@@ -1312,17 +1377,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)
@@ -1384,6 +1445,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-03 7:56 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-03 7:56 UTC (permalink / raw)
To: linux-arm-kernel
It adds device tree probe 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>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
.../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
drivers/tty/serial/imx.c | 82 +++++++++++++++++---
2 files changed, 91 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..a9c0406
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
@@ -0,0 +1,19 @@
+* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
+
+Required properties:
+- compatible : Should be "fsl,<soc>-uart"
+- reg : Address and length of the register set for the device
+- interrupts : Should contain uart interrupt
+
+Optional properties:
+- fsl,uart-has-rtscts : Indicate the uart has rts and cts
+- fsl,irda-mode : Indicate the uart supports irda mode
+
+Example:
+
+uart at 73fbc000 {
+ compatible = "fsl,imx51-uart", "fsl,imx21-uart";
+ reg = <0x73fbc000 0x4000>;
+ interrupts = <31>;
+ fsl,uart-has-rtscts;
+};
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 983f3bd..483c447 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_device.h>
#include <asm/io.h>
#include <asm/irq.h>
@@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
}
};
+static struct of_device_id imx_uart_dt_ids[] = {
+ { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
+ { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
+ { /* sentinel */ },
+};
+
/*
* Handle any change of modem status signal since we were last called.
*/
@@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
+ const struct of_device_id *of_id =
+ of_match_device(imx_uart_dt_ids, &pdev->dev);
+
+ if (!np)
+ return -ENODEV;
+
+ pdev->id = of_alias_get_id(np, "serial");
+ if (pdev->id < 0)
+ return -ENODEV;
+
+ if (of_get_property(np, "fsl,uart-has-rtscts", NULL))
+ sport->have_rtscts = 1;
+
+ if (of_get_property(np, "fsl,irda-mode", NULL))
+ sport->use_irda = 1;
+
+ sport->devdata = of_id->data;
+
+ return 0;
+}
+#else
+static inline int serial_imx_probe_dt(struct imx_port *sport,
+ struct platform_device *pdev)
+{
+ return -ENODEV;
+}
+#endif
+
+static int serial_imx_probe_pdata(struct imx_port *sport,
+ struct platform_device *pdev)
+{
+ struct imxuart_platform_data *pdata = pdev->dev.platform_data;
+
+ if (!pdata)
+ return 0;
+
+ if (pdata->flags & IMXUART_HAVE_RTSCTS)
+ sport->have_rtscts = 1;
+
+ if (pdata->flags & IMXUART_IRDA)
+ sport->use_irda = 1;
+
+ sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
+
+ return 0;
+}
+
static int serial_imx_probe(struct platform_device *pdev)
{
struct imx_port *sport;
@@ -1273,6 +1333,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;
@@ -1301,7 +1367,6 @@ static int serial_imx_probe(struct platform_device *pdev)
init_timer(&sport->timer);
sport->timer.function = imx_timeout;
sport->timer.data = (unsigned long)sport;
- sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
sport->clk = clk_get(&pdev->dev, "uart");
if (IS_ERR(sport->clk)) {
@@ -1312,17 +1377,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)
@@ -1384,6 +1445,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-03 7:55 ` Shawn Guo
@ 2011-07-03 21:10 ` Grant Likely
-1 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-03 21:10 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-serial, patches, devicetree-discuss, linux-arm-kernel,
Sascha Hauer, Alan Cox
On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> the .id_table of platform_driver to distinguish the uart device type.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
> arch/arm/mach-imx/clock-imx1.c | 6 +-
> arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> index dcc4172..4aabeb2 100644
> --- a/arch/arm/mach-imx/clock-imx1.c
> +++ b/arch/arm/mach-imx/clock-imx1.c
> @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> _REGISTER_CLOCK(NULL, "mma", mma_clk)
> _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> index cfce8c9..477271a 100644
> --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> },
> };
>
> - return imx_add_platform_device("imx-uart", data->id, res,
> + return imx_add_platform_device("imx1-uart", data->id, res,
> ARRAY_SIZE(res), pdata, sizeof(*pdata));
> }
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 22fe801..983f3bd 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -48,7 +48,6 @@
>
> #include <asm/io.h>
> #include <asm/irq.h>
> -#include <mach/hardware.h>
> #include <mach/imx-uart.h>
>
> /* Register definitions */
> @@ -67,7 +66,8 @@
> #define UBMR 0xa8 /* BRM Modulator Register */
> #define UBRC 0xac /* Baud Rate Count Register */
> #define MX2_ONEMS 0xb0 /* One Millisecond register */
> -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
>
> /* UART Control Register Bit Fields.*/
> #define URXD_CHARRDY (1<<15)
> @@ -181,6 +181,17 @@
>
> #define UART_NR 8
>
> +enum imx_uart_type {
> + IMX1_UART,
> + IMX2_UART,
> +};
> +
> +/* device type dependent stuff */
> +struct imx_uart_data {
> + unsigned uts_reg;
> + enum imx_uart_type devtype;
> +};
> +
> struct imx_port {
> struct uart_port port;
> struct timer_list timer;
> @@ -192,6 +203,7 @@ struct imx_port {
> unsigned int irda_inv_tx:1;
> unsigned short trcv_delay; /* transceiver delay */
> struct clk *clk;
> + struct imx_uart_data *devdata;
> };
>
> #ifdef CONFIG_IRDA
> @@ -200,6 +212,33 @@ struct imx_port {
> #define USE_IRDA(sport) (0)
> #endif
>
> +#define UTS (sport->devdata->uts_reg)
> +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> +
> +static struct imx_uart_data imx_uart_devdata[] = {
> + [IMX1_UART] = {
> + .uts_reg = MX1_UTS,
> + .devtype = IMX1_UART,
> + },
> + [IMX2_UART] = {
> + .uts_reg = MX2_UTS,
> + .devtype = IMX2_UART,
> + },
> +};
> +
> +static struct platform_device_id imx_uart_devtype[] = {
> + {
> + .name = "imx1-uart",
> + .driver_data = IMX1_UART,
> + }, {
> + .name = "imx-uart",
> + .driver_data = IMX2_UART,
Rather than using driver_data as an index, and having a separate
table, you can use it as a pointer to the imx_uard_data structure.
That's why driver_data is declared as a kernel_ulong_t. The only
reason it isn't a void* (AFAIK) is that mod_devicetable.h is exported
to userspace.
> + }, {
> + /* sentinel */
> + }
> +};
> +
> /*
> * Handle any change of modem status signal since we were last called.
> */
> @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> }
> }
>
> - if (!cpu_is_mx1()) {
> + if (IS_IMX2_UART()) {
The logic is getting reversed here, is this really what you want to
do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
> temp = readl(sport->port.membase + UCR3);
> temp |= MX2_UCR3_RXDMUXSEL;
> writel(temp, sport->port.membase + UCR3);
> @@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
> writel(num, sport->port.membase + UBIR);
> writel(denom, sport->port.membase + UBMR);
>
> - if (!cpu_is_mx1())
> + if (IS_IMX2_UART())
> writel(sport->port.uartclk / div / 1000,
> sport->port.membase + MX2_ONEMS);
>
> @@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
> ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
> old_ucr2 = readl(sport->port.membase + UCR2);
>
> - if (cpu_is_mx1())
> + if (IS_IMX1_UART())
> ucr1 |= MX1_UCR1_UARTCLKEN;
> ucr1 |= UCR1_UARTEN;
> ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
> @@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
> init_timer(&sport->timer);
> sport->timer.function = imx_timeout;
> sport->timer.data = (unsigned long)sport;
> + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
>
> sport->clk = clk_get(&pdev->dev, "uart");
> if (IS_ERR(sport->clk)) {
> @@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
>
> .suspend = serial_imx_suspend,
> .resume = serial_imx_resume,
> + .id_table = imx_uart_devtype,
> .driver = {
> .name = "imx-uart",
> .owner = THIS_MODULE,
> --
> 1.7.4.1
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-03 21:10 ` Grant Likely
0 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-03 21:10 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> the .id_table of platform_driver to distinguish the uart device type.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
> arch/arm/mach-imx/clock-imx1.c | 6 +-
> arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> index dcc4172..4aabeb2 100644
> --- a/arch/arm/mach-imx/clock-imx1.c
> +++ b/arch/arm/mach-imx/clock-imx1.c
> @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> _REGISTER_CLOCK(NULL, "mma", mma_clk)
> _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> index cfce8c9..477271a 100644
> --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> },
> };
>
> - return imx_add_platform_device("imx-uart", data->id, res,
> + return imx_add_platform_device("imx1-uart", data->id, res,
> ARRAY_SIZE(res), pdata, sizeof(*pdata));
> }
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 22fe801..983f3bd 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -48,7 +48,6 @@
>
> #include <asm/io.h>
> #include <asm/irq.h>
> -#include <mach/hardware.h>
> #include <mach/imx-uart.h>
>
> /* Register definitions */
> @@ -67,7 +66,8 @@
> #define UBMR 0xa8 /* BRM Modulator Register */
> #define UBRC 0xac /* Baud Rate Count Register */
> #define MX2_ONEMS 0xb0 /* One Millisecond register */
> -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
>
> /* UART Control Register Bit Fields.*/
> #define URXD_CHARRDY (1<<15)
> @@ -181,6 +181,17 @@
>
> #define UART_NR 8
>
> +enum imx_uart_type {
> + IMX1_UART,
> + IMX2_UART,
> +};
> +
> +/* device type dependent stuff */
> +struct imx_uart_data {
> + unsigned uts_reg;
> + enum imx_uart_type devtype;
> +};
> +
> struct imx_port {
> struct uart_port port;
> struct timer_list timer;
> @@ -192,6 +203,7 @@ struct imx_port {
> unsigned int irda_inv_tx:1;
> unsigned short trcv_delay; /* transceiver delay */
> struct clk *clk;
> + struct imx_uart_data *devdata;
> };
>
> #ifdef CONFIG_IRDA
> @@ -200,6 +212,33 @@ struct imx_port {
> #define USE_IRDA(sport) (0)
> #endif
>
> +#define UTS (sport->devdata->uts_reg)
> +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> +
> +static struct imx_uart_data imx_uart_devdata[] = {
> + [IMX1_UART] = {
> + .uts_reg = MX1_UTS,
> + .devtype = IMX1_UART,
> + },
> + [IMX2_UART] = {
> + .uts_reg = MX2_UTS,
> + .devtype = IMX2_UART,
> + },
> +};
> +
> +static struct platform_device_id imx_uart_devtype[] = {
> + {
> + .name = "imx1-uart",
> + .driver_data = IMX1_UART,
> + }, {
> + .name = "imx-uart",
> + .driver_data = IMX2_UART,
Rather than using driver_data as an index, and having a separate
table, you can use it as a pointer to the imx_uard_data structure.
That's why driver_data is declared as a kernel_ulong_t. The only
reason it isn't a void* (AFAIK) is that mod_devicetable.h is exported
to userspace.
> + }, {
> + /* sentinel */
> + }
> +};
> +
> /*
> * Handle any change of modem status signal since we were last called.
> */
> @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> }
> }
>
> - if (!cpu_is_mx1()) {
> + if (IS_IMX2_UART()) {
The logic is getting reversed here, is this really what you want to
do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
> temp = readl(sport->port.membase + UCR3);
> temp |= MX2_UCR3_RXDMUXSEL;
> writel(temp, sport->port.membase + UCR3);
> @@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
> writel(num, sport->port.membase + UBIR);
> writel(denom, sport->port.membase + UBMR);
>
> - if (!cpu_is_mx1())
> + if (IS_IMX2_UART())
> writel(sport->port.uartclk / div / 1000,
> sport->port.membase + MX2_ONEMS);
>
> @@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
> ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
> old_ucr2 = readl(sport->port.membase + UCR2);
>
> - if (cpu_is_mx1())
> + if (IS_IMX1_UART())
> ucr1 |= MX1_UCR1_UARTCLKEN;
> ucr1 |= UCR1_UARTEN;
> ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
> @@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
> init_timer(&sport->timer);
> sport->timer.function = imx_timeout;
> sport->timer.data = (unsigned long)sport;
> + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
>
> sport->clk = clk_get(&pdev->dev, "uart");
> if (IS_ERR(sport->clk)) {
> @@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
>
> .suspend = serial_imx_suspend,
> .resume = serial_imx_resume,
> + .id_table = imx_uart_devtype,
> .driver = {
> .name = "imx-uart",
> .owner = THIS_MODULE,
> --
> 1.7.4.1
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-03 7:56 ` Shawn Guo
@ 2011-07-03 21:14 ` Grant Likely
-1 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-03 21:14 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-serial, linux-arm-kernel, devicetree-discuss, patches,
Jeremy Kerr, Jason Liu, Sascha Hauer, Alan Cox
On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> It adds device tree probe 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>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
Minor comments below, but otherwise looks okay to me. Feel free to
add the following when you respin with the first patch:
Acked-by: Grant Likely <grant.likely@secretlab.ca>
I don't see any reason preventing this from being ready to be queued
up for v3.1
g.
> ---
> .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> 2 files changed, 91 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..a9c0406
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> @@ -0,0 +1,19 @@
> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-uart"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain uart interrupt
> +
> +Optional properties:
> +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> +- fsl,irda-mode : Indicate the uart supports irda mode
> +
> +Example:
> +
> +uart@73fbc000 {
> + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> + reg = <0x73fbc000 0x4000>;
> + interrupts = <31>;
> + fsl,uart-has-rtscts;
> +};
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 983f3bd..483c447 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_device.h>
>
> #include <asm/io.h>
> #include <asm/irq.h>
> @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> }
> };
>
> +static struct of_device_id imx_uart_dt_ids[] = {
> + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> + { /* sentinel */ },
Nit: drop the ',' from the last entry so that nobody inadvertently
adds another entry to the list.
> +};
> +
> /*
> * Handle any change of modem status signal since we were last called.
> */
> @@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
> + const struct of_device_id *of_id =
> + of_match_device(imx_uart_dt_ids, &pdev->dev);
> +
> + if (!np)
> + return -ENODEV;
> +
> + pdev->id = of_alias_get_id(np, "serial");
> + if (pdev->id < 0)
> + return -ENODEV;
If you're bailing here, it would be friendly to spit out a klog
message saying why the driver has bailed.
> + if (of_get_property(np, "fsl,uart-has-rtscts", NULL))
> + sport->have_rtscts = 1;
> +
> + if (of_get_property(np, "fsl,irda-mode", NULL))
> + sport->use_irda = 1;
> +
> + sport->devdata = of_id->data;
> +
> + return 0;
> +}
> +#else
> +static inline int serial_imx_probe_dt(struct imx_port *sport,
> + struct platform_device *pdev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +static int serial_imx_probe_pdata(struct imx_port *sport,
> + struct platform_device *pdev)
> +{
> + struct imxuart_platform_data *pdata = pdev->dev.platform_data;
> +
> + if (!pdata)
> + return 0;
> +
> + if (pdata->flags & IMXUART_HAVE_RTSCTS)
> + sport->have_rtscts = 1;
> +
> + if (pdata->flags & IMXUART_IRDA)
> + sport->use_irda = 1;
> +
> + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
> +
> + return 0;
> +}
> +
> static int serial_imx_probe(struct platform_device *pdev)
> {
> struct imx_port *sport;
> @@ -1273,6 +1333,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;
> @@ -1301,7 +1367,6 @@ static int serial_imx_probe(struct platform_device *pdev)
> init_timer(&sport->timer);
> sport->timer.function = imx_timeout;
> sport->timer.data = (unsigned long)sport;
> - sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
>
> sport->clk = clk_get(&pdev->dev, "uart");
> if (IS_ERR(sport->clk)) {
> @@ -1312,17 +1377,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)
> @@ -1384,6 +1445,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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-03 21:14 ` Grant Likely
0 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-03 21:14 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> It adds device tree probe 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>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
Minor comments below, but otherwise looks okay to me. Feel free to
add the following when you respin with the first patch:
Acked-by: Grant Likely <grant.likely@secretlab.ca>
I don't see any reason preventing this from being ready to be queued
up for v3.1
g.
> ---
> .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> 2 files changed, 91 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..a9c0406
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> @@ -0,0 +1,19 @@
> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-uart"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain uart interrupt
> +
> +Optional properties:
> +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> +- fsl,irda-mode : Indicate the uart supports irda mode
> +
> +Example:
> +
> +uart at 73fbc000 {
> + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> + reg = <0x73fbc000 0x4000>;
> + interrupts = <31>;
> + fsl,uart-has-rtscts;
> +};
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 983f3bd..483c447 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_device.h>
>
> #include <asm/io.h>
> #include <asm/irq.h>
> @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> }
> };
>
> +static struct of_device_id imx_uart_dt_ids[] = {
> + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> + { /* sentinel */ },
Nit: drop the ',' from the last entry so that nobody inadvertently
adds another entry to the list.
> +};
> +
> /*
> * Handle any change of modem status signal since we were last called.
> */
> @@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
> + const struct of_device_id *of_id =
> + of_match_device(imx_uart_dt_ids, &pdev->dev);
> +
> + if (!np)
> + return -ENODEV;
> +
> + pdev->id = of_alias_get_id(np, "serial");
> + if (pdev->id < 0)
> + return -ENODEV;
If you're bailing here, it would be friendly to spit out a klog
message saying why the driver has bailed.
> + if (of_get_property(np, "fsl,uart-has-rtscts", NULL))
> + sport->have_rtscts = 1;
> +
> + if (of_get_property(np, "fsl,irda-mode", NULL))
> + sport->use_irda = 1;
> +
> + sport->devdata = of_id->data;
> +
> + return 0;
> +}
> +#else
> +static inline int serial_imx_probe_dt(struct imx_port *sport,
> + struct platform_device *pdev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +static int serial_imx_probe_pdata(struct imx_port *sport,
> + struct platform_device *pdev)
> +{
> + struct imxuart_platform_data *pdata = pdev->dev.platform_data;
> +
> + if (!pdata)
> + return 0;
> +
> + if (pdata->flags & IMXUART_HAVE_RTSCTS)
> + sport->have_rtscts = 1;
> +
> + if (pdata->flags & IMXUART_IRDA)
> + sport->use_irda = 1;
> +
> + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
> +
> + return 0;
> +}
> +
> static int serial_imx_probe(struct platform_device *pdev)
> {
> struct imx_port *sport;
> @@ -1273,6 +1333,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;
> @@ -1301,7 +1367,6 @@ static int serial_imx_probe(struct platform_device *pdev)
> init_timer(&sport->timer);
> sport->timer.function = imx_timeout;
> sport->timer.data = (unsigned long)sport;
> - sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
>
> sport->clk = clk_get(&pdev->dev, "uart");
> if (IS_ERR(sport->clk)) {
> @@ -1312,17 +1377,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)
> @@ -1384,6 +1445,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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-03 21:10 ` Grant Likely
@ 2011-07-04 2:19 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 2:19 UTC (permalink / raw)
To: Grant Likely
Cc: Shawn Guo, patches, devicetree-discuss, linux-arm-kernel,
linux-serial, Sascha Hauer, Alan Cox
Hi Grant,
Thanks for the review.
On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > the .id_table of platform_driver to distinguish the uart device type.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > ---
> > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > 3 files changed, 50 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > index dcc4172..4aabeb2 100644
> > --- a/arch/arm/mach-imx/clock-imx1.c
> > +++ b/arch/arm/mach-imx/clock-imx1.c
> > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > index cfce8c9..477271a 100644
> > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > },
> > };
> >
> > - return imx_add_platform_device("imx-uart", data->id, res,
> > + return imx_add_platform_device("imx1-uart", data->id, res,
> > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > }
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 22fe801..983f3bd 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -48,7 +48,6 @@
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > -#include <mach/hardware.h>
> > #include <mach/imx-uart.h>
> >
> > /* Register definitions */
> > @@ -67,7 +66,8 @@
> > #define UBMR 0xa8 /* BRM Modulator Register */
> > #define UBRC 0xac /* Baud Rate Count Register */
> > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> >
> > /* UART Control Register Bit Fields.*/
> > #define URXD_CHARRDY (1<<15)
> > @@ -181,6 +181,17 @@
> >
> > #define UART_NR 8
> >
> > +enum imx_uart_type {
> > + IMX1_UART,
> > + IMX2_UART,
> > +};
> > +
> > +/* device type dependent stuff */
> > +struct imx_uart_data {
> > + unsigned uts_reg;
> > + enum imx_uart_type devtype;
> > +};
> > +
> > struct imx_port {
> > struct uart_port port;
> > struct timer_list timer;
> > @@ -192,6 +203,7 @@ struct imx_port {
> > unsigned int irda_inv_tx:1;
> > unsigned short trcv_delay; /* transceiver delay */
> > struct clk *clk;
> > + struct imx_uart_data *devdata;
> > };
> >
> > #ifdef CONFIG_IRDA
> > @@ -200,6 +212,33 @@ struct imx_port {
> > #define USE_IRDA(sport) (0)
> > #endif
> >
> > +#define UTS (sport->devdata->uts_reg)
> > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > +
> > +static struct imx_uart_data imx_uart_devdata[] = {
> > + [IMX1_UART] = {
> > + .uts_reg = MX1_UTS,
> > + .devtype = IMX1_UART,
> > + },
> > + [IMX2_UART] = {
> > + .uts_reg = MX2_UTS,
> > + .devtype = IMX2_UART,
> > + },
> > +};
> > +
> > +static struct platform_device_id imx_uart_devtype[] = {
> > + {
> > + .name = "imx1-uart",
> > + .driver_data = IMX1_UART,
> > + }, {
> > + .name = "imx-uart",
> > + .driver_data = IMX2_UART,
>
> Rather than using driver_data as an index, and having a separate
> table, you can use it as a pointer to the imx_uard_data structure.
> That's why driver_data is declared as a kernel_ulong_t. The only
> reason it isn't a void* (AFAIK) is that mod_devicetable.h is exported
> to userspace.
>
Yes, I thought about it. I happened to choose saving type cast
(twice) over making two tables (platform_device_id and of_device_id)
look consistent. But I do not have a strong position on that, so I
respect your comment since you do :)
I will make the change on other patches where the comment apples.
> > + }, {
> > + /* sentinel */
> > + }
> > +};
> > +
> > /*
> > * Handle any change of modem status signal since we were last called.
> > */
> > @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> > }
> > }
> >
> > - if (!cpu_is_mx1()) {
> > + if (IS_IMX2_UART()) {
>
> The logic is getting reversed here, is this really what you want to
> do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
>
Maybe not. I actually made a small improvement here. The body of
the 'if' is really IMX2 specific code, so it makes more sense to use
IS_IMX2_UART() than !IS_IMX1_UART().
Regards,
Shawn
> > temp = readl(sport->port.membase + UCR3);
> > temp |= MX2_UCR3_RXDMUXSEL;
> > writel(temp, sport->port.membase + UCR3);
> > @@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
> > writel(num, sport->port.membase + UBIR);
> > writel(denom, sport->port.membase + UBMR);
> >
> > - if (!cpu_is_mx1())
> > + if (IS_IMX2_UART())
> > writel(sport->port.uartclk / div / 1000,
> > sport->port.membase + MX2_ONEMS);
> >
> > @@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
> > ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
> > old_ucr2 = readl(sport->port.membase + UCR2);
> >
> > - if (cpu_is_mx1())
> > + if (IS_IMX1_UART())
> > ucr1 |= MX1_UCR1_UARTCLKEN;
> > ucr1 |= UCR1_UARTEN;
> > ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
> > @@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
> > init_timer(&sport->timer);
> > sport->timer.function = imx_timeout;
> > sport->timer.data = (unsigned long)sport;
> > + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
> >
> > sport->clk = clk_get(&pdev->dev, "uart");
> > if (IS_ERR(sport->clk)) {
> > @@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
> >
> > .suspend = serial_imx_suspend,
> > .resume = serial_imx_resume,
> > + .id_table = imx_uart_devtype,
> > .driver = {
> > .name = "imx-uart",
> > .owner = THIS_MODULE,
> > --
> > 1.7.4.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 2:19 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 2:19 UTC (permalink / raw)
To: linux-arm-kernel
Hi Grant,
Thanks for the review.
On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > the .id_table of platform_driver to distinguish the uart device type.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > ---
> > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > 3 files changed, 50 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > index dcc4172..4aabeb2 100644
> > --- a/arch/arm/mach-imx/clock-imx1.c
> > +++ b/arch/arm/mach-imx/clock-imx1.c
> > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > index cfce8c9..477271a 100644
> > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > },
> > };
> >
> > - return imx_add_platform_device("imx-uart", data->id, res,
> > + return imx_add_platform_device("imx1-uart", data->id, res,
> > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > }
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 22fe801..983f3bd 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -48,7 +48,6 @@
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > -#include <mach/hardware.h>
> > #include <mach/imx-uart.h>
> >
> > /* Register definitions */
> > @@ -67,7 +66,8 @@
> > #define UBMR 0xa8 /* BRM Modulator Register */
> > #define UBRC 0xac /* Baud Rate Count Register */
> > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> >
> > /* UART Control Register Bit Fields.*/
> > #define URXD_CHARRDY (1<<15)
> > @@ -181,6 +181,17 @@
> >
> > #define UART_NR 8
> >
> > +enum imx_uart_type {
> > + IMX1_UART,
> > + IMX2_UART,
> > +};
> > +
> > +/* device type dependent stuff */
> > +struct imx_uart_data {
> > + unsigned uts_reg;
> > + enum imx_uart_type devtype;
> > +};
> > +
> > struct imx_port {
> > struct uart_port port;
> > struct timer_list timer;
> > @@ -192,6 +203,7 @@ struct imx_port {
> > unsigned int irda_inv_tx:1;
> > unsigned short trcv_delay; /* transceiver delay */
> > struct clk *clk;
> > + struct imx_uart_data *devdata;
> > };
> >
> > #ifdef CONFIG_IRDA
> > @@ -200,6 +212,33 @@ struct imx_port {
> > #define USE_IRDA(sport) (0)
> > #endif
> >
> > +#define UTS (sport->devdata->uts_reg)
> > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > +
> > +static struct imx_uart_data imx_uart_devdata[] = {
> > + [IMX1_UART] = {
> > + .uts_reg = MX1_UTS,
> > + .devtype = IMX1_UART,
> > + },
> > + [IMX2_UART] = {
> > + .uts_reg = MX2_UTS,
> > + .devtype = IMX2_UART,
> > + },
> > +};
> > +
> > +static struct platform_device_id imx_uart_devtype[] = {
> > + {
> > + .name = "imx1-uart",
> > + .driver_data = IMX1_UART,
> > + }, {
> > + .name = "imx-uart",
> > + .driver_data = IMX2_UART,
>
> Rather than using driver_data as an index, and having a separate
> table, you can use it as a pointer to the imx_uard_data structure.
> That's why driver_data is declared as a kernel_ulong_t. The only
> reason it isn't a void* (AFAIK) is that mod_devicetable.h is exported
> to userspace.
>
Yes, I thought about it. I happened to choose saving type cast
(twice) over making two tables (platform_device_id and of_device_id)
look consistent. But I do not have a strong position on that, so I
respect your comment since you do :)
I will make the change on other patches where the comment apples.
> > + }, {
> > + /* sentinel */
> > + }
> > +};
> > +
> > /*
> > * Handle any change of modem status signal since we were last called.
> > */
> > @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> > }
> > }
> >
> > - if (!cpu_is_mx1()) {
> > + if (IS_IMX2_UART()) {
>
> The logic is getting reversed here, is this really what you want to
> do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
>
Maybe not. I actually made a small improvement here. The body of
the 'if' is really IMX2 specific code, so it makes more sense to use
IS_IMX2_UART() than !IS_IMX1_UART().
Regards,
Shawn
> > temp = readl(sport->port.membase + UCR3);
> > temp |= MX2_UCR3_RXDMUXSEL;
> > writel(temp, sport->port.membase + UCR3);
> > @@ -923,7 +962,7 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
> > writel(num, sport->port.membase + UBIR);
> > writel(denom, sport->port.membase + UBMR);
> >
> > - if (!cpu_is_mx1())
> > + if (IS_IMX2_UART())
> > writel(sport->port.uartclk / div / 1000,
> > sport->port.membase + MX2_ONEMS);
> >
> > @@ -1062,7 +1101,7 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
> > ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
> > old_ucr2 = readl(sport->port.membase + UCR2);
> >
> > - if (cpu_is_mx1())
> > + if (IS_IMX1_UART())
> > ucr1 |= MX1_UCR1_UARTCLKEN;
> > ucr1 |= UCR1_UARTEN;
> > ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
> > @@ -1262,6 +1301,7 @@ static int serial_imx_probe(struct platform_device *pdev)
> > init_timer(&sport->timer);
> > sport->timer.function = imx_timeout;
> > sport->timer.data = (unsigned long)sport;
> > + sport->devdata = &imx_uart_devdata[pdev->id_entry->driver_data];
> >
> > sport->clk = clk_get(&pdev->dev, "uart");
> > if (IS_ERR(sport->clk)) {
> > @@ -1340,6 +1380,7 @@ static struct platform_driver serial_imx_driver = {
> >
> > .suspend = serial_imx_suspend,
> > .resume = serial_imx_resume,
> > + .id_table = imx_uart_devtype,
> > .driver = {
> > .name = "imx-uart",
> > .owner = THIS_MODULE,
> > --
> > 1.7.4.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-03 21:14 ` Grant Likely
@ 2011-07-04 2:36 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 2:36 UTC (permalink / raw)
To: Grant Likely
Cc: Shawn Guo, patches, devicetree-discuss, Jason Liu, Alan Cox,
linux-serial, Jeremy Kerr, Sascha Hauer, linux-arm-kernel
On Sun, Jul 03, 2011 at 03:14:34PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > It adds device tree probe 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>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
>
> Minor comments below, but otherwise looks okay to me. Feel free to
> add the following when you respin with the first patch:
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
> I don't see any reason preventing this from being ready to be queued
> up for v3.1
>
> g.
>
> > ---
> > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > 2 files changed, 91 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..a9c0406
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,19 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-uart"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain uart interrupt
> > +
> > +Optional properties:
> > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > +- fsl,irda-mode : Indicate the uart supports irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > + reg = <0x73fbc000 0x4000>;
> > + interrupts = <31>;
> > + fsl,uart-has-rtscts;
> > +};
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 983f3bd..483c447 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_device.h>
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > }
> > };
> >
> > +static struct of_device_id imx_uart_dt_ids[] = {
> > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > + { /* sentinel */ },
>
> Nit: drop the ',' from the last entry so that nobody inadvertently
> adds another entry to the list.
>
Ok, will make the change on other patches where the comment applies.
> > +};
> > +
> > /*
> > * Handle any change of modem status signal since we were last called.
> > */
> > @@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
> > + const struct of_device_id *of_id =
> > + of_match_device(imx_uart_dt_ids, &pdev->dev);
> > +
> > + if (!np)
> > + return -ENODEV;
> > +
> > + pdev->id = of_alias_get_id(np, "serial");
> > + if (pdev->id < 0)
> > + return -ENODEV;
>
> If you're bailing here, it would be friendly to spit out a klog
> message saying why the driver has bailed.
>
Ok, will make the change on other patches where the comment applies.
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 2:36 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 2:36 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 03, 2011 at 03:14:34PM -0600, Grant Likely wrote:
> On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > It adds device tree probe 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>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
>
> Minor comments below, but otherwise looks okay to me. Feel free to
> add the following when you respin with the first patch:
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
> I don't see any reason preventing this from being ready to be queued
> up for v3.1
>
> g.
>
> > ---
> > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > 2 files changed, 91 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..a9c0406
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,19 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-uart"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain uart interrupt
> > +
> > +Optional properties:
> > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > +- fsl,irda-mode : Indicate the uart supports irda mode
> > +
> > +Example:
> > +
> > +uart at 73fbc000 {
> > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > + reg = <0x73fbc000 0x4000>;
> > + interrupts = <31>;
> > + fsl,uart-has-rtscts;
> > +};
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 983f3bd..483c447 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_device.h>
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > }
> > };
> >
> > +static struct of_device_id imx_uart_dt_ids[] = {
> > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > + { /* sentinel */ },
>
> Nit: drop the ',' from the last entry so that nobody inadvertently
> adds another entry to the list.
>
Ok, will make the change on other patches where the comment applies.
> > +};
> > +
> > /*
> > * Handle any change of modem status signal since we were last called.
> > */
> > @@ -1261,6 +1269,58 @@ 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 *np = pdev->dev.of_node;
> > + const struct of_device_id *of_id =
> > + of_match_device(imx_uart_dt_ids, &pdev->dev);
> > +
> > + if (!np)
> > + return -ENODEV;
> > +
> > + pdev->id = of_alias_get_id(np, "serial");
> > + if (pdev->id < 0)
> > + return -ENODEV;
>
> If you're bailing here, it would be friendly to spit out a klog
> message saying why the driver has bailed.
>
Ok, will make the change on other patches where the comment applies.
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-04 2:19 ` Shawn Guo
@ 2011-07-04 5:38 ` Grant Likely
-1 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-04 5:38 UTC (permalink / raw)
To: Shawn Guo
Cc: Shawn Guo, patches, devicetree-discuss, linux-arm-kernel,
linux-serial, Sascha Hauer, Alan Cox
On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> > > }
> > > }
> > >
> > > - if (!cpu_is_mx1()) {
> > > + if (IS_IMX2_UART()) {
> >
> > The logic is getting reversed here, is this really what you want to
> > do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
> >
> Maybe not. I actually made a small improvement here. The body of
> the 'if' is really IMX2 specific code, so it makes more sense to use
> IS_IMX2_UART() than !IS_IMX1_UART().
Okay, it would probably be worth mentioning this change in the commit text.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 5:38 ` Grant Likely
0 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-04 5:38 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > @@ -689,7 +728,7 @@ static int imx_startup(struct uart_port *port)
> > > }
> > > }
> > >
> > > - if (!cpu_is_mx1()) {
> > > + if (IS_IMX2_UART()) {
> >
> > The logic is getting reversed here, is this really what you want to
> > do? I would think you'd want to preserve the !IS_IMX1_UART() logic.
> >
> Maybe not. I actually made a small improvement here. The body of
> the 'if' is really IMX2 specific code, so it makes more sense to use
> IS_IMX2_UART() than !IS_IMX1_UART().
Okay, it would probably be worth mentioning this change in the commit text.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-03 7:56 ` Shawn Guo
@ 2011-07-04 6:58 ` Sascha Hauer
-1 siblings, 0 replies; 34+ messages in thread
From: Sascha Hauer @ 2011-07-04 6:58 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-serial, linux-arm-kernel, devicetree-discuss, patches,
Jeremy Kerr, Jason Liu, Alan Cox, Grant Likely
On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> It adds device tree probe 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>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> 2 files changed, 91 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..a9c0406
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> @@ -0,0 +1,19 @@
> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-uart"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain uart interrupt
> +
> +Optional properties:
> +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> +- fsl,irda-mode : Indicate the uart supports irda mode
> +
> +Example:
> +
> +uart@73fbc000 {
> + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> + reg = <0x73fbc000 0x4000>;
> + interrupts = <31>;
> + fsl,uart-has-rtscts;
> +};
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 983f3bd..483c447 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_device.h>
>
> #include <asm/io.h>
> #include <asm/irq.h>
> @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> }
> };
>
> +static struct of_device_id imx_uart_dt_ids[] = {
> + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> + { /* sentinel */ },
> +};
Isn't this table a bit too small? What about the other SoCs?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 6:58 ` Sascha Hauer
0 siblings, 0 replies; 34+ messages in thread
From: Sascha Hauer @ 2011-07-04 6:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> It adds device tree probe 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>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> 2 files changed, 91 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..a9c0406
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> @@ -0,0 +1,19 @@
> +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-uart"
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain uart interrupt
> +
> +Optional properties:
> +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> +- fsl,irda-mode : Indicate the uart supports irda mode
> +
> +Example:
> +
> +uart at 73fbc000 {
> + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> + reg = <0x73fbc000 0x4000>;
> + interrupts = <31>;
> + fsl,uart-has-rtscts;
> +};
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 983f3bd..483c447 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_device.h>
>
> #include <asm/io.h>
> #include <asm/irq.h>
> @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> }
> };
>
> +static struct of_device_id imx_uart_dt_ids[] = {
> + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> + { /* sentinel */ },
> +};
Isn't this table a bit too small? What about the other SoCs?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-04 2:19 ` Shawn Guo
@ 2011-07-04 7:33 ` Uwe Kleine-König
-1 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 7:33 UTC (permalink / raw)
To: Shawn Guo
Cc: Grant Likely, patches, devicetree-discuss, Alan Cox,
linux-serial, Shawn Guo, Sascha Hauer, linux-arm-kernel
On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> Hi Grant,
>
> Thanks for the review.
>
> On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> > On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > > the .id_table of platform_driver to distinguish the uart device type.
> > >
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > ---
> > > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > > 3 files changed, 50 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > > index dcc4172..4aabeb2 100644
> > > --- a/arch/arm/mach-imx/clock-imx1.c
> > > +++ b/arch/arm/mach-imx/clock-imx1.c
> > > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > index cfce8c9..477271a 100644
> > > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > > },
> > > };
> > >
> > > - return imx_add_platform_device("imx-uart", data->id, res,
> > > + return imx_add_platform_device("imx1-uart", data->id, res,
> > > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > > }
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 22fe801..983f3bd 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -48,7 +48,6 @@
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > -#include <mach/hardware.h>
> > > #include <mach/imx-uart.h>
> > >
> > > /* Register definitions */
> > > @@ -67,7 +66,8 @@
> > > #define UBMR 0xa8 /* BRM Modulator Register */
> > > #define UBRC 0xac /* Baud Rate Count Register */
> > > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> > >
> > > /* UART Control Register Bit Fields.*/
> > > #define URXD_CHARRDY (1<<15)
> > > @@ -181,6 +181,17 @@
> > >
> > > #define UART_NR 8
> > >
> > > +enum imx_uart_type {
> > > + IMX1_UART,
> > > + IMX2_UART,
> > > +};
> > > +
> > > +/* device type dependent stuff */
> > > +struct imx_uart_data {
> > > + unsigned uts_reg;
> > > + enum imx_uart_type devtype;
> > > +};
> > > +
> > > struct imx_port {
> > > struct uart_port port;
> > > struct timer_list timer;
> > > @@ -192,6 +203,7 @@ struct imx_port {
> > > unsigned int irda_inv_tx:1;
> > > unsigned short trcv_delay; /* transceiver delay */
> > > struct clk *clk;
> > > + struct imx_uart_data *devdata;
> > > };
> > >
> > > #ifdef CONFIG_IRDA
> > > @@ -200,6 +212,33 @@ struct imx_port {
> > > #define USE_IRDA(sport) (0)
> > > #endif
> > >
> > > +#define UTS (sport->devdata->uts_reg)
> > > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > > +
> > > +static struct imx_uart_data imx_uart_devdata[] = {
> > > + [IMX1_UART] = {
> > > + .uts_reg = MX1_UTS,
> > > + .devtype = IMX1_UART,
> > > + },
> > > + [IMX2_UART] = {
> > > + .uts_reg = MX2_UTS,
> > > + .devtype = IMX2_UART,
> > > + },
> > > +};
> > > +
> > > +static struct platform_device_id imx_uart_devtype[] = {
> > > + {
> > > + .name = "imx1-uart",
> > > + .driver_data = IMX1_UART,
> > > + }, {
> > > + .name = "imx-uart",
> > > + .driver_data = IMX2_UART,
It feels strange to introduce IMX2 today meaning
i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
if the only relevant change is that the UTS register is at a different
location, I'd prefer to make it as follows:
#define IMX_UART_UTS_AT_D0 1
{
.name = "imx1-uart",
.driver_data = IMX_UART_UTS_AT_D0,
}, {
.name = "imx-uart",
.driver_data = 0,
}
and then do
u32 imx_uart_read_UTS(...)
{
if (get_driver_data() & IMX_UART_UTS_AT_D0)
return read(0xd0);
else
return read(0xb4);
}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 7:33 ` Uwe Kleine-König
0 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 7:33 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> Hi Grant,
>
> Thanks for the review.
>
> On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> > On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > > the .id_table of platform_driver to distinguish the uart device type.
> > >
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > ---
> > > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > > 3 files changed, 50 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > > index dcc4172..4aabeb2 100644
> > > --- a/arch/arm/mach-imx/clock-imx1.c
> > > +++ b/arch/arm/mach-imx/clock-imx1.c
> > > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > index cfce8c9..477271a 100644
> > > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > > },
> > > };
> > >
> > > - return imx_add_platform_device("imx-uart", data->id, res,
> > > + return imx_add_platform_device("imx1-uart", data->id, res,
> > > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > > }
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 22fe801..983f3bd 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -48,7 +48,6 @@
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > -#include <mach/hardware.h>
> > > #include <mach/imx-uart.h>
> > >
> > > /* Register definitions */
> > > @@ -67,7 +66,8 @@
> > > #define UBMR 0xa8 /* BRM Modulator Register */
> > > #define UBRC 0xac /* Baud Rate Count Register */
> > > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> > >
> > > /* UART Control Register Bit Fields.*/
> > > #define URXD_CHARRDY (1<<15)
> > > @@ -181,6 +181,17 @@
> > >
> > > #define UART_NR 8
> > >
> > > +enum imx_uart_type {
> > > + IMX1_UART,
> > > + IMX2_UART,
> > > +};
> > > +
> > > +/* device type dependent stuff */
> > > +struct imx_uart_data {
> > > + unsigned uts_reg;
> > > + enum imx_uart_type devtype;
> > > +};
> > > +
> > > struct imx_port {
> > > struct uart_port port;
> > > struct timer_list timer;
> > > @@ -192,6 +203,7 @@ struct imx_port {
> > > unsigned int irda_inv_tx:1;
> > > unsigned short trcv_delay; /* transceiver delay */
> > > struct clk *clk;
> > > + struct imx_uart_data *devdata;
> > > };
> > >
> > > #ifdef CONFIG_IRDA
> > > @@ -200,6 +212,33 @@ struct imx_port {
> > > #define USE_IRDA(sport) (0)
> > > #endif
> > >
> > > +#define UTS (sport->devdata->uts_reg)
> > > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > > +
> > > +static struct imx_uart_data imx_uart_devdata[] = {
> > > + [IMX1_UART] = {
> > > + .uts_reg = MX1_UTS,
> > > + .devtype = IMX1_UART,
> > > + },
> > > + [IMX2_UART] = {
> > > + .uts_reg = MX2_UTS,
> > > + .devtype = IMX2_UART,
> > > + },
> > > +};
> > > +
> > > +static struct platform_device_id imx_uart_devtype[] = {
> > > + {
> > > + .name = "imx1-uart",
> > > + .driver_data = IMX1_UART,
> > > + }, {
> > > + .name = "imx-uart",
> > > + .driver_data = IMX2_UART,
It feels strange to introduce IMX2 today meaning
i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
if the only relevant change is that the UTS register is at a different
location, I'd prefer to make it as follows:
#define IMX_UART_UTS_AT_D0 1
{
.name = "imx1-uart",
.driver_data = IMX_UART_UTS_AT_D0,
}, {
.name = "imx-uart",
.driver_data = 0,
}
and then do
u32 imx_uart_read_UTS(...)
{
if (get_driver_data() & IMX_UART_UTS_AT_D0)
return read(0xd0);
else
return read(0xb4);
}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-04 6:58 ` Sascha Hauer
@ 2011-07-04 7:38 ` Uwe Kleine-König
-1 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 7:38 UTC (permalink / raw)
To: Sascha Hauer
Cc: Shawn Guo, patches, devicetree-discuss, Jason Liu, Grant Likely,
Alan Cox, linux-serial, Jeremy Kerr, linux-arm-kernel
On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > It adds device tree probe 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>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > ---
> > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > 2 files changed, 91 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..a9c0406
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,19 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-uart"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain uart interrupt
> > +
> > +Optional properties:
> > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > +- fsl,irda-mode : Indicate the uart supports irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > + reg = <0x73fbc000 0x4000>;
> > + interrupts = <31>;
> > + fsl,uart-has-rtscts;
> > +};
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 983f3bd..483c447 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_device.h>
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > }
> > };
> >
> > +static struct of_device_id imx_uart_dt_ids[] = {
> > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > + { /* sentinel */ },
> > +};
>
> Isn't this table a bit too small? What about the other SoCs?
I'd expect that using fsl,imx21-uart on imx51 would result in a working
port, but it feels stange and if that's the intention the example above
needs fixing.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 7:38 ` Uwe Kleine-König
0 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 7:38 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > It adds device tree probe 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>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > ---
> > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > 2 files changed, 91 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..a9c0406
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > @@ -0,0 +1,19 @@
> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<soc>-uart"
> > +- reg : Address and length of the register set for the device
> > +- interrupts : Should contain uart interrupt
> > +
> > +Optional properties:
> > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > +- fsl,irda-mode : Indicate the uart supports irda mode
> > +
> > +Example:
> > +
> > +uart at 73fbc000 {
> > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > + reg = <0x73fbc000 0x4000>;
> > + interrupts = <31>;
> > + fsl,uart-has-rtscts;
> > +};
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 983f3bd..483c447 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_device.h>
> >
> > #include <asm/io.h>
> > #include <asm/irq.h>
> > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > }
> > };
> >
> > +static struct of_device_id imx_uart_dt_ids[] = {
> > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > + { /* sentinel */ },
> > +};
>
> Isn't this table a bit too small? What about the other SoCs?
I'd expect that using fsl,imx21-uart on imx51 would result in a working
port, but it feels stange and if that's the intention the example above
needs fixing.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-04 7:33 ` Uwe Kleine-König
@ 2011-07-04 8:43 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 8:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Grant Likely, patches, devicetree-discuss, Alan Cox,
linux-serial, Shawn Guo, Sascha Hauer, linux-arm-kernel
On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > Hi Grant,
> >
> > Thanks for the review.
> >
> > On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> > > On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > > > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > > > the .id_table of platform_driver to distinguish the uart device type.
> > > >
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > > ---
> > > > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > > > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > > > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > > > 3 files changed, 50 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > > > index dcc4172..4aabeb2 100644
> > > > --- a/arch/arm/mach-imx/clock-imx1.c
> > > > +++ b/arch/arm/mach-imx/clock-imx1.c
> > > > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > > > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > > > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > > > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > > > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > > > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > > > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > > > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > > > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > > > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > > > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > index cfce8c9..477271a 100644
> > > > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > > > },
> > > > };
> > > >
> > > > - return imx_add_platform_device("imx-uart", data->id, res,
> > > > + return imx_add_platform_device("imx1-uart", data->id, res,
> > > > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > > > }
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 22fe801..983f3bd 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -48,7 +48,6 @@
> > > >
> > > > #include <asm/io.h>
> > > > #include <asm/irq.h>
> > > > -#include <mach/hardware.h>
> > > > #include <mach/imx-uart.h>
> > > >
> > > > /* Register definitions */
> > > > @@ -67,7 +66,8 @@
> > > > #define UBMR 0xa8 /* BRM Modulator Register */
> > > > #define UBRC 0xac /* Baud Rate Count Register */
> > > > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > > > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > > > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > > > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> > > >
> > > > /* UART Control Register Bit Fields.*/
> > > > #define URXD_CHARRDY (1<<15)
> > > > @@ -181,6 +181,17 @@
> > > >
> > > > #define UART_NR 8
> > > >
> > > > +enum imx_uart_type {
> > > > + IMX1_UART,
> > > > + IMX2_UART,
> > > > +};
> > > > +
> > > > +/* device type dependent stuff */
> > > > +struct imx_uart_data {
> > > > + unsigned uts_reg;
> > > > + enum imx_uart_type devtype;
> > > > +};
> > > > +
> > > > struct imx_port {
> > > > struct uart_port port;
> > > > struct timer_list timer;
> > > > @@ -192,6 +203,7 @@ struct imx_port {
> > > > unsigned int irda_inv_tx:1;
> > > > unsigned short trcv_delay; /* transceiver delay */
> > > > struct clk *clk;
> > > > + struct imx_uart_data *devdata;
> > > > };
> > > >
> > > > #ifdef CONFIG_IRDA
> > > > @@ -200,6 +212,33 @@ struct imx_port {
> > > > #define USE_IRDA(sport) (0)
> > > > #endif
> > > >
> > > > +#define UTS (sport->devdata->uts_reg)
> > > > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > > > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > > > +
> > > > +static struct imx_uart_data imx_uart_devdata[] = {
> > > > + [IMX1_UART] = {
> > > > + .uts_reg = MX1_UTS,
> > > > + .devtype = IMX1_UART,
> > > > + },
> > > > + [IMX2_UART] = {
> > > > + .uts_reg = MX2_UTS,
> > > > + .devtype = IMX2_UART,
> > > > + },
> > > > +};
> > > > +
> > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > + {
> > > > + .name = "imx1-uart",
> > > > + .driver_data = IMX1_UART,
> > > > + }, {
> > > > + .name = "imx-uart",
> > > > + .driver_data = IMX2_UART,
> It feels strange to introduce IMX2 today meaning
> i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> if the only relevant change is that the UTS register is at a different
No, it's not the only relevant change. The patch also changes a couple of
'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
have the same UART block which was firstly introduced on IMX2.
It's actually a change respecting the existing code, which defines
MX2_ONEMS for all imx except imx1.
--
Regards,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 8:43 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 8:43 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > Hi Grant,
> >
> > Thanks for the review.
> >
> > On Sun, Jul 03, 2011 at 03:10:07PM -0600, Grant Likely wrote:
> > > On Sun, Jul 03, 2011 at 03:55:59PM +0800, Shawn Guo wrote:
> > > > The patch removes all the uses of cpu_is_mx1(). Instead, it uses
> > > > the .id_table of platform_driver to distinguish the uart device type.
> > > >
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > > ---
> > > > arch/arm/mach-imx/clock-imx1.c | 6 +-
> > > > arch/arm/plat-mxc/devices/platform-imx-uart.c | 2 +-
> > > > drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++--
> > > > 3 files changed, 50 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
> > > > index dcc4172..4aabeb2 100644
> > > > --- a/arch/arm/mach-imx/clock-imx1.c
> > > > +++ b/arch/arm/mach-imx/clock-imx1.c
> > > > @@ -587,9 +587,9 @@ static struct clk_lookup lookups[] __initdata = {
> > > > _REGISTER_CLOCK(NULL, "mma", mma_clk)
> > > > _REGISTER_CLOCK("imx_udc.0", NULL, usbd_clk)
> > > > _REGISTER_CLOCK(NULL, "gpt", gpt_clk)
> > > > - _REGISTER_CLOCK("imx-uart.0", NULL, uart_clk)
> > > > - _REGISTER_CLOCK("imx-uart.1", NULL, uart_clk)
> > > > - _REGISTER_CLOCK("imx-uart.2", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.0", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.1", NULL, uart_clk)
> > > > + _REGISTER_CLOCK("imx1-uart.2", NULL, uart_clk)
> > > > _REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
> > > > _REGISTER_CLOCK("imx1-cspi.0", NULL, spi_clk)
> > > > _REGISTER_CLOCK("imx1-cspi.1", NULL, spi_clk)
> > > > diff --git a/arch/arm/plat-mxc/devices/platform-imx-uart.c b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > index cfce8c9..477271a 100644
> > > > --- a/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > +++ b/arch/arm/plat-mxc/devices/platform-imx-uart.c
> > > > @@ -152,7 +152,7 @@ struct platform_device *__init imx_add_imx_uart_3irq(
> > > > },
> > > > };
> > > >
> > > > - return imx_add_platform_device("imx-uart", data->id, res,
> > > > + return imx_add_platform_device("imx1-uart", data->id, res,
> > > > ARRAY_SIZE(res), pdata, sizeof(*pdata));
> > > > }
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 22fe801..983f3bd 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -48,7 +48,6 @@
> > > >
> > > > #include <asm/io.h>
> > > > #include <asm/irq.h>
> > > > -#include <mach/hardware.h>
> > > > #include <mach/imx-uart.h>
> > > >
> > > > /* Register definitions */
> > > > @@ -67,7 +66,8 @@
> > > > #define UBMR 0xa8 /* BRM Modulator Register */
> > > > #define UBRC 0xac /* Baud Rate Count Register */
> > > > #define MX2_ONEMS 0xb0 /* One Millisecond register */
> > > > -#define UTS (cpu_is_mx1() ? 0xd0 : 0xb4) /* UART Test Register */
> > > > +#define MX1_UTS 0xd0 /* UART Test Register on mx1 */
> > > > +#define MX2_UTS 0xb4 /* UART Test Register on mx2 and later */
> > > >
> > > > /* UART Control Register Bit Fields.*/
> > > > #define URXD_CHARRDY (1<<15)
> > > > @@ -181,6 +181,17 @@
> > > >
> > > > #define UART_NR 8
> > > >
> > > > +enum imx_uart_type {
> > > > + IMX1_UART,
> > > > + IMX2_UART,
> > > > +};
> > > > +
> > > > +/* device type dependent stuff */
> > > > +struct imx_uart_data {
> > > > + unsigned uts_reg;
> > > > + enum imx_uart_type devtype;
> > > > +};
> > > > +
> > > > struct imx_port {
> > > > struct uart_port port;
> > > > struct timer_list timer;
> > > > @@ -192,6 +203,7 @@ struct imx_port {
> > > > unsigned int irda_inv_tx:1;
> > > > unsigned short trcv_delay; /* transceiver delay */
> > > > struct clk *clk;
> > > > + struct imx_uart_data *devdata;
> > > > };
> > > >
> > > > #ifdef CONFIG_IRDA
> > > > @@ -200,6 +212,33 @@ struct imx_port {
> > > > #define USE_IRDA(sport) (0)
> > > > #endif
> > > >
> > > > +#define UTS (sport->devdata->uts_reg)
> > > > +#define IS_IMX1_UART() (sport->devdata->devtype == IMX1_UART)
> > > > +#define IS_IMX2_UART() (sport->devdata->devtype == IMX2_UART)
> > > > +
> > > > +static struct imx_uart_data imx_uart_devdata[] = {
> > > > + [IMX1_UART] = {
> > > > + .uts_reg = MX1_UTS,
> > > > + .devtype = IMX1_UART,
> > > > + },
> > > > + [IMX2_UART] = {
> > > > + .uts_reg = MX2_UTS,
> > > > + .devtype = IMX2_UART,
> > > > + },
> > > > +};
> > > > +
> > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > + {
> > > > + .name = "imx1-uart",
> > > > + .driver_data = IMX1_UART,
> > > > + }, {
> > > > + .name = "imx-uart",
> > > > + .driver_data = IMX2_UART,
> It feels strange to introduce IMX2 today meaning
> i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> if the only relevant change is that the UTS register is at a different
No, it's not the only relevant change. The patch also changes a couple of
'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
have the same UART block which was firstly introduced on IMX2.
It's actually a change respecting the existing code, which defines
MX2_ONEMS for all imx except imx1.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-04 7:38 ` Uwe Kleine-König
@ 2011-07-04 9:05 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 9:05 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sascha Hauer, patches, devicetree-discuss, Jason Liu,
linux-arm-kernel, linux-serial, Jeremy Kerr, Alan Cox
On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > It adds device tree probe 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>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > ---
> > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > 2 files changed, 91 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..a9c0406
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > @@ -0,0 +1,19 @@
> > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-uart"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain uart interrupt
> > > +
> > > +Optional properties:
> > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > +
> > > +Example:
> > > +
> > > +uart@73fbc000 {
> > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > + reg = <0x73fbc000 0x4000>;
> > > + interrupts = <31>;
> > > + fsl,uart-has-rtscts;
> > > +};
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 983f3bd..483c447 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_device.h>
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > }
> > > };
> > >
> > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > + { /* sentinel */ },
> > > +};
> >
> > Isn't this table a bit too small? What about the other SoCs?
> I'd expect that using fsl,imx21-uart on imx51 would result in a working
> port, but it feels stange and if that's the intention the example above
> needs fixing.
>
Here is what I have in dts file for uart. The 'compatible' tells
that it's a imx51-uart and compatible with imx21-uart.
uart0: uart@73fbc000 {
compatible = "fsl,imx51-uart", "fsl,imx21-uart";
reg = <0x73fbc000 0x4000>;
interrupts = <31>;
fsl,uart-has-rtscts;
};
I did not buy it until I got convinced by Grant here [1]. We do not
need this long table until there are some soc specific thing to
distinguish. Currently, they are all compatible with imx21-uart and
share imx_uart_devdata[IMX2_UART].
{ .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx25-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx27-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx31-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx35-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx50-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx51-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx53-uart", .data = &imx_uart_devdata[IMX2_UART], },
[1] http://permalink.gmane.org/gmane.linux.network/198935
--
Regards,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 9:05 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 9:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > It adds device tree probe 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>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > ---
> > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > 2 files changed, 91 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..a9c0406
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > @@ -0,0 +1,19 @@
> > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-uart"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain uart interrupt
> > > +
> > > +Optional properties:
> > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > +
> > > +Example:
> > > +
> > > +uart at 73fbc000 {
> > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > + reg = <0x73fbc000 0x4000>;
> > > + interrupts = <31>;
> > > + fsl,uart-has-rtscts;
> > > +};
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 983f3bd..483c447 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_device.h>
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > }
> > > };
> > >
> > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > + { /* sentinel */ },
> > > +};
> >
> > Isn't this table a bit too small? What about the other SoCs?
> I'd expect that using fsl,imx21-uart on imx51 would result in a working
> port, but it feels stange and if that's the intention the example above
> needs fixing.
>
Here is what I have in dts file for uart. The 'compatible' tells
that it's a imx51-uart and compatible with imx21-uart.
uart0: uart at 73fbc000 {
compatible = "fsl,imx51-uart", "fsl,imx21-uart";
reg = <0x73fbc000 0x4000>;
interrupts = <31>;
fsl,uart-has-rtscts;
};
I did not buy it until I got convinced by Grant here [1]. We do not
need this long table until there are some soc specific thing to
distinguish. Currently, they are all compatible with imx21-uart and
share imx_uart_devdata[IMX2_UART].
{ .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx25-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx27-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx31-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx35-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx50-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx51-uart", .data = &imx_uart_devdata[IMX2_UART], },
{ .compatible = "fsl,imx53-uart", .data = &imx_uart_devdata[IMX2_UART], },
[1] http://permalink.gmane.org/gmane.linux.network/198935
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-04 9:05 ` Shawn Guo
@ 2011-07-04 9:10 ` Uwe Kleine-König
-1 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 9:10 UTC (permalink / raw)
To: Shawn Guo
Cc: Sascha Hauer, patches, devicetree-discuss, Jason Liu,
linux-arm-kernel, linux-serial, Jeremy Kerr, Alan Cox
On Mon, Jul 04, 2011 at 05:05:27PM +0800, Shawn Guo wrote:
> On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-König wrote:
> > On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > > It adds device tree probe 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>
> > > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > ---
> > > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > > 2 files changed, 91 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..a9c0406
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > > @@ -0,0 +1,19 @@
> > > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "fsl,<soc>-uart"
> > > > +- reg : Address and length of the register set for the device
> > > > +- interrupts : Should contain uart interrupt
> > > > +
> > > > +Optional properties:
> > > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > > +
> > > > +Example:
> > > > +
> > > > +uart@73fbc000 {
> > > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > > + reg = <0x73fbc000 0x4000>;
> > > > + interrupts = <31>;
> > > > + fsl,uart-has-rtscts;
> > > > +};
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 983f3bd..483c447 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_device.h>
> > > >
> > > > #include <asm/io.h>
> > > > #include <asm/irq.h>
> > > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > > }
> > > > };
> > > >
> > > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > > + { /* sentinel */ },
> > > > +};
> > >
> > > Isn't this table a bit too small? What about the other SoCs?
> > I'd expect that using fsl,imx21-uart on imx51 would result in a working
> > port, but it feels stange and if that's the intention the example above
> > needs fixing.
> >
> Here is what I have in dts file for uart. The 'compatible' tells
> that it's a imx51-uart and compatible with imx21-uart.
>
> uart0: uart@73fbc000 {
> compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> reg = <0x73fbc000 0x4000>;
> interrupts = <31>;
> fsl,uart-has-rtscts;
> };
/me learned something new about dt. Sorry for the noise.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 9:10 ` Uwe Kleine-König
0 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 9:10 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 05:05:27PM +0800, Shawn Guo wrote:
> On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > > It adds device tree probe 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>
> > > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > ---
> > > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > > 2 files changed, 91 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..a9c0406
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > > @@ -0,0 +1,19 @@
> > > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "fsl,<soc>-uart"
> > > > +- reg : Address and length of the register set for the device
> > > > +- interrupts : Should contain uart interrupt
> > > > +
> > > > +Optional properties:
> > > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > > +
> > > > +Example:
> > > > +
> > > > +uart at 73fbc000 {
> > > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > > + reg = <0x73fbc000 0x4000>;
> > > > + interrupts = <31>;
> > > > + fsl,uart-has-rtscts;
> > > > +};
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 983f3bd..483c447 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_device.h>
> > > >
> > > > #include <asm/io.h>
> > > > #include <asm/irq.h>
> > > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > > }
> > > > };
> > > >
> > > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > > + { /* sentinel */ },
> > > > +};
> > >
> > > Isn't this table a bit too small? What about the other SoCs?
> > I'd expect that using fsl,imx21-uart on imx51 would result in a working
> > port, but it feels stange and if that's the intention the example above
> > needs fixing.
> >
> Here is what I have in dts file for uart. The 'compatible' tells
> that it's a imx51-uart and compatible with imx21-uart.
>
> uart0: uart at 73fbc000 {
> compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> reg = <0x73fbc000 0x4000>;
> interrupts = <31>;
> fsl,uart-has-rtscts;
> };
/me learned something new about dt. Sorry for the noise.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-04 8:43 ` Shawn Guo
@ 2011-07-04 9:28 ` Uwe Kleine-König
-1 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 9:28 UTC (permalink / raw)
To: Shawn Guo
Cc: Grant Likely, patches, devicetree-discuss, Alan Cox,
linux-serial, Shawn Guo, Sascha Hauer, linux-arm-kernel
Hello Shawn,
On Mon, Jul 04, 2011 at 04:43:25PM +0800, Shawn Guo wrote:
> On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-König wrote:
> > On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > > + {
> > > > > + .name = "imx1-uart",
> > > > > + .driver_data = IMX1_UART,
> > > > > + }, {
> > > > > + .name = "imx-uart",
> > > > > + .driver_data = IMX2_UART,
> > It feels strange to introduce IMX2 today meaning
> > i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> > if the only relevant change is that the UTS register is at a different
>
> No, it's not the only relevant change. The patch also changes a couple of
> 'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
>
> The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
> 31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
> have the same UART block which was firstly introduced on IMX2.
This is ugly. IMHO something like imx_uart_v2 would be much better.
For a driver that supports all i.MX generations
if (IS_IMX2_UART())
is simply misleading. Even if you had documented at the definition
of imx_uart_type (and/or IS_IMX2_UART) that IMX2_UART means everything
currently known but MX1, just looking at the if condition yields wrong
expectations.
if (cpu_is_imx1())
is much better (in this aspect). (At least until Freescale comes up with
a new SOC in the mx1 family that includes a completely new UART block
:-)
With
if (somehow_get_current_driver_data() & SOME_FLAG)
do_the(things, that, "are only needed with SOME_FLAG");
you can have both, even local understandable code and no dependency on
the cpu_is_... stuff.
> It's actually a change respecting the existing code, which defines
> MX2_ONEMS for all imx except imx1.
IMHO that's a bad excuse. Take it the other way 'round: You can do
better than the existing code (and even fix it on the go if you're
motivated).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 9:28 ` Uwe Kleine-König
0 siblings, 0 replies; 34+ messages in thread
From: Uwe Kleine-König @ 2011-07-04 9:28 UTC (permalink / raw)
To: linux-arm-kernel
Hello Shawn,
On Mon, Jul 04, 2011 at 04:43:25PM +0800, Shawn Guo wrote:
> On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > > + {
> > > > > + .name = "imx1-uart",
> > > > > + .driver_data = IMX1_UART,
> > > > > + }, {
> > > > > + .name = "imx-uart",
> > > > > + .driver_data = IMX2_UART,
> > It feels strange to introduce IMX2 today meaning
> > i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> > if the only relevant change is that the UTS register is at a different
>
> No, it's not the only relevant change. The patch also changes a couple of
> 'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
>
> The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
> 31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
> have the same UART block which was firstly introduced on IMX2.
This is ugly. IMHO something like imx_uart_v2 would be much better.
For a driver that supports all i.MX generations
if (IS_IMX2_UART())
is simply misleading. Even if you had documented at the definition
of imx_uart_type (and/or IS_IMX2_UART) that IMX2_UART means everything
currently known but MX1, just looking at the if condition yields wrong
expectations.
if (cpu_is_imx1())
is much better (in this aspect). (At least until Freescale comes up with
a new SOC in the mx1 family that includes a completely new UART block
:-)
With
if (somehow_get_current_driver_data() & SOME_FLAG)
do_the(things, that, "are only needed with SOME_FLAG");
you can have both, even local understandable code and no dependency on
the cpu_is_... stuff.
> It's actually a change respecting the existing code, which defines
> MX2_ONEMS for all imx except imx1.
IMHO that's a bad excuse. Take it the other way 'round: You can do
better than the existing code (and even fix it on the go if you're
motivated).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
2011-07-04 9:28 ` Uwe Kleine-König
@ 2011-07-04 10:26 ` Shawn Guo
-1 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 10:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Grant Likely, patches, devicetree-discuss, Alan Cox,
linux-serial, Shawn Guo, Sascha Hauer, linux-arm-kernel
On Mon, Jul 04, 2011 at 11:28:05AM +0200, Uwe Kleine-König wrote:
> Hello Shawn,
>
> On Mon, Jul 04, 2011 at 04:43:25PM +0800, Shawn Guo wrote:
> > On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > > > + {
> > > > > > + .name = "imx1-uart",
> > > > > > + .driver_data = IMX1_UART,
> > > > > > + }, {
> > > > > > + .name = "imx-uart",
> > > > > > + .driver_data = IMX2_UART,
> > > It feels strange to introduce IMX2 today meaning
> > > i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> > > if the only relevant change is that the UTS register is at a different
> >
> > No, it's not the only relevant change. The patch also changes a couple of
> > 'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
> >
> > The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
> > 31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
> > have the same UART block which was firstly introduced on IMX2.
> This is ugly. IMHO something like imx_uart_v2 would be much better.
I will be very careful to define version number for IP block while
hardware people do not. When some day they do, we will probably have
the version number we defined today conflicting with the one they
will define.
> For a driver that supports all i.MX generations
>
> if (IS_IMX2_UART())
>
> is simply misleading. Even if you had documented at the definition
> of imx_uart_type (and/or IS_IMX2_UART) that IMX2_UART means everything
> currently known but MX1, just looking at the if condition yields wrong
> expectations.
>
To address your concern, I will rename IMX2_UART to IMX_UART.
--
Regards,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()
@ 2011-07-04 10:26 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2011-07-04 10:26 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 11:28:05AM +0200, Uwe Kleine-K?nig wrote:
> Hello Shawn,
>
> On Mon, Jul 04, 2011 at 04:43:25PM +0800, Shawn Guo wrote:
> > On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-K?nig wrote:
> > > On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > > > > +static struct platform_device_id imx_uart_devtype[] = {
> > > > > > + {
> > > > > > + .name = "imx1-uart",
> > > > > > + .driver_data = IMX1_UART,
> > > > > > + }, {
> > > > > > + .name = "imx-uart",
> > > > > > + .driver_data = IMX2_UART,
> > > It feels strange to introduce IMX2 today meaning
> > > i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
> > > if the only relevant change is that the UTS register is at a different
> >
> > No, it's not the only relevant change. The patch also changes a couple of
> > 'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.
> >
> > The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
> > 31,35,50,51,53}. It actually means i.MX{21,25,27,31,35,50,51,53} all
> > have the same UART block which was firstly introduced on IMX2.
> This is ugly. IMHO something like imx_uart_v2 would be much better.
I will be very careful to define version number for IP block while
hardware people do not. When some day they do, we will probably have
the version number we defined today conflicting with the one they
will define.
> For a driver that supports all i.MX generations
>
> if (IS_IMX2_UART())
>
> is simply misleading. Even if you had documented at the definition
> of imx_uart_type (and/or IS_IMX2_UART) that IMX2_UART means everything
> currently known but MX1, just looking at the if condition yields wrong
> expectations.
>
To address your concern, I will rename IMX2_UART to IMX_UART.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] serial/imx: add device tree probe support
2011-07-04 7:38 ` Uwe Kleine-König
@ 2011-07-04 15:33 ` Grant Likely
-1 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-04 15:33 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sascha Hauer, Shawn Guo, patches, devicetree-discuss, Jason Liu,
Alan Cox, linux-serial, Jeremy Kerr, linux-arm-kernel
On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > It adds device tree probe 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>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > ---
> > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > 2 files changed, 91 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..a9c0406
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > @@ -0,0 +1,19 @@
> > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-uart"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain uart interrupt
> > > +
> > > +Optional properties:
> > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > +
> > > +Example:
> > > +
> > > +uart@73fbc000 {
> > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > + reg = <0x73fbc000 0x4000>;
> > > + interrupts = <31>;
> > > + fsl,uart-has-rtscts;
> > > +};
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 983f3bd..483c447 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_device.h>
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > }
> > > };
> > >
> > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > + { /* sentinel */ },
> > > +};
> >
> > Isn't this table a bit too small? What about the other SoCs?
> I'd expect that using fsl,imx21-uart on imx51 would result in a working
> port, but it feels stange and if that's the intention the example above
> needs fixing.
If the newer parts claim compatibility with the older, then have a
terse of_device_id table is just fine. ie, the compatible property
for an imx51 would be:
compatible = "fsl,imx51-uart", "fsl,imx21-uart";
It would only be wrong if the device tree node omitted the
"fsl,imx51-uart" value from the list.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] serial/imx: add device tree probe support
@ 2011-07-04 15:33 ` Grant Likely
0 siblings, 0 replies; 34+ messages in thread
From: Grant Likely @ 2011-07-04 15:33 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 04, 2011 at 09:38:26AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jul 04, 2011 at 08:58:20AM +0200, Sascha Hauer wrote:
> > On Sun, Jul 03, 2011 at 03:56:00PM +0800, Shawn Guo wrote:
> > > It adds device tree probe 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>
> > > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > ---
> > > .../bindings/tty/serial/fsl-imx-uart.txt | 19 +++++
> > > drivers/tty/serial/imx.c | 82 +++++++++++++++++---
> > > 2 files changed, 91 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..a9c0406
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
> > > @@ -0,0 +1,19 @@
> > > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<soc>-uart"
> > > +- reg : Address and length of the register set for the device
> > > +- interrupts : Should contain uart interrupt
> > > +
> > > +Optional properties:
> > > +- fsl,uart-has-rtscts : Indicate the uart has rts and cts
> > > +- fsl,irda-mode : Indicate the uart supports irda mode
> > > +
> > > +Example:
> > > +
> > > +uart at 73fbc000 {
> > > + compatible = "fsl,imx51-uart", "fsl,imx21-uart";
> > > + reg = <0x73fbc000 0x4000>;
> > > + interrupts = <31>;
> > > + fsl,uart-has-rtscts;
> > > +};
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 983f3bd..483c447 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_device.h>
> > >
> > > #include <asm/io.h>
> > > #include <asm/irq.h>
> > > @@ -239,6 +241,12 @@ static struct platform_device_id imx_uart_devtype[] = {
> > > }
> > > };
> > >
> > > +static struct of_device_id imx_uart_dt_ids[] = {
> > > + { .compatible = "fsl,imx1-uart", .data = &imx_uart_devdata[IMX1_UART], },
> > > + { .compatible = "fsl,imx21-uart", .data = &imx_uart_devdata[IMX2_UART], },
> > > + { /* sentinel */ },
> > > +};
> >
> > Isn't this table a bit too small? What about the other SoCs?
> I'd expect that using fsl,imx21-uart on imx51 would result in a working
> port, but it feels stange and if that's the intention the example above
> needs fixing.
If the newer parts claim compatibility with the older, then have a
terse of_device_id table is just fine. ie, the compatible property
for an imx51 would be:
compatible = "fsl,imx51-uart", "fsl,imx21-uart";
It would only be wrong if the device tree node omitted the
"fsl,imx51-uart" value from the list.
g.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2011-07-04 15:33 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-03 7:55 [PATCH 0/2] Add device tree probe support for tty/serial/imx Shawn Guo
2011-07-03 7:55 ` Shawn Guo
2011-07-03 7:55 ` [PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1() Shawn Guo
2011-07-03 7:55 ` Shawn Guo
2011-07-03 21:10 ` Grant Likely
2011-07-03 21:10 ` Grant Likely
2011-07-04 2:19 ` Shawn Guo
2011-07-04 2:19 ` Shawn Guo
2011-07-04 5:38 ` Grant Likely
2011-07-04 5:38 ` Grant Likely
2011-07-04 7:33 ` Uwe Kleine-König
2011-07-04 7:33 ` Uwe Kleine-König
2011-07-04 8:43 ` Shawn Guo
2011-07-04 8:43 ` Shawn Guo
2011-07-04 9:28 ` Uwe Kleine-König
2011-07-04 9:28 ` Uwe Kleine-König
2011-07-04 10:26 ` Shawn Guo
2011-07-04 10:26 ` Shawn Guo
2011-07-03 7:56 ` [PATCH 2/2] serial/imx: add device tree probe support Shawn Guo
2011-07-03 7:56 ` Shawn Guo
2011-07-03 21:14 ` Grant Likely
2011-07-03 21:14 ` Grant Likely
2011-07-04 2:36 ` Shawn Guo
2011-07-04 2:36 ` Shawn Guo
2011-07-04 6:58 ` Sascha Hauer
2011-07-04 6:58 ` Sascha Hauer
2011-07-04 7:38 ` Uwe Kleine-König
2011-07-04 7:38 ` Uwe Kleine-König
2011-07-04 9:05 ` Shawn Guo
2011-07-04 9:05 ` Shawn Guo
2011-07-04 9:10 ` Uwe Kleine-König
2011-07-04 9:10 ` Uwe Kleine-König
2011-07-04 15:33 ` Grant Likely
2011-07-04 15:33 ` Grant Likely
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.