diff mbox

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

Message ID 1308410354-21387-2-git-send-email-shawn.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Guo June 18, 2011, 3:19 p.m. UTC
It adds device tree data parsing support for imx tty/serial driver.

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

Comments

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

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

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

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

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

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

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

> +
> +	if (of_get_property(node, "fsl,has-rts-cts", NULL))
> +		sport->have_rtscts = 1;
> +
> +	if (of_get_property(node, "fsl,irda-mode", NULL))
> +		sport->use_irda = 1;
> +
> +	return 0;
> +}
> +
> +static struct of_device_id imx_uart_dt_ids[] = {
> +	{ .compatible = "fsl,imx-uart" },
> +	{},
> +};
> +#else
> +static int serial_imx_probe_dt(struct imx_port *sport,
> +		struct platform_device *pdev)
> +{
> +	return -ENODEV;
> +}
> +#define imx_uart_dt_ids NULL
> +#endif
> +
> +static int serial_imx_probe_pdata(struct imx_port *sport,
> +		struct platform_device *pdev)
> +{
> +	struct imxuart_platform_data *pdata = pdev->dev.platform_data;
> +
> +	if (!pdata)
> +		return 0;
> +
> +	if (pdata->flags & IMXUART_HAVE_RTSCTS)
> +		sport->have_rtscts = 1;
> +
> +	if (pdata->flags & IMXUART_IRDA)
> +		sport->use_irda = 1;
> +
> +	sport->port.line = pdev->id;
> +
> +	return 0;
> +}
> +
>  static int serial_imx_probe(struct platform_device *pdev)
>  {
>  	struct imx_port *sport;
> @@ -1235,6 +1294,12 @@ static int serial_imx_probe(struct platform_device *pdev)
>  	if (!sport)
>  		return -ENOMEM;
>  
> +	ret = serial_imx_probe_dt(sport, pdev);
> +	if (ret == -ENODEV)
> +		ret = serial_imx_probe_pdata(sport, pdev);
> +	if (ret)
> +		goto free;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>  		ret = -ENODEV;
> @@ -1259,7 +1324,6 @@ static int serial_imx_probe(struct platform_device *pdev)
>  	sport->port.fifosize = 32;
>  	sport->port.ops = &imx_pops;
>  	sport->port.flags = UPF_BOOT_AUTOCONF;
> -	sport->port.line = pdev->id;
>  	init_timer(&sport->timer);
>  	sport->timer.function = imx_timeout;
>  	sport->timer.data     = (unsigned long)sport;
> @@ -1273,17 +1337,13 @@ static int serial_imx_probe(struct platform_device *pdev)
>  
>  	sport->port.uartclk = clk_get_rate(sport->clk);
>  
> -	imx_ports[pdev->id] = sport;
> +	if (imx_ports[sport->port.line]) {
> +		ret = -EBUSY;
> +		goto clkput;
> +	}
> +	imx_ports[sport->port.line] = sport;
>  
>  	pdata = pdev->dev.platform_data;
> -	if (pdata && (pdata->flags & IMXUART_HAVE_RTSCTS))
> -		sport->have_rtscts = 1;
> -
> -#ifdef CONFIG_IRDA
> -	if (pdata && (pdata->flags & IMXUART_IRDA))
> -		sport->use_irda = 1;
> -#endif
> -
>  	if (pdata && pdata->init) {
>  		ret = pdata->init(pdev);
>  		if (ret)
> @@ -1344,6 +1404,7 @@ static struct platform_driver serial_imx_driver = {
>  	.driver		= {
>  		.name	= "imx-uart",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = imx_uart_dt_ids,
>  	},
>  };
>  
> -- 
> 1.7.4.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Arnd Bergmann June 18, 2011, 4:21 p.m. UTC | #2
On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
>
> +Required properties:
> +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> +- reg : address and length of the register set for the device
> +- interrupts : should contain uart interrupt
> +- id : should be the port ID defined by soc
> +
> +Optional properties:
> +- fsl,has-rts-cts : indicate it has rts-cts
> +- fsl,irda-mode : support irda mode
> +
> +Example:
> +
> +uart@73fbc000 {
> +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> +	reg = <0x73fbc000 0x4000>;
> +	interrupts = <31>;
> +	id = <1>;
> +	fsl,has-rts-cts;
> +};

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

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


	Arnd

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

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

g.

> 
> 
> 	Arnd
> 
> [1] http://playground.sun.com/1275/bindings/devices/html/serial-1_0d.html#HDR9
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Arnd Bergmann June 18, 2011, 6:11 p.m. UTC | #4
On Saturday 18 June 2011 18:26:55 Grant Likely wrote:
> On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:

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


Sounds ok to me.

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

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

Regards,

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

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

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

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

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

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

> We do need some infrastructure to make that easier though.  Would you
> have time to help put that together?
> 
Ok, I will give it a try.
Shawn Guo June 19, 2011, 7:40 a.m. UTC | #7
On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:
> On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
> >
> > +Required properties:
> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> > +- reg : address and length of the register set for the device
> > +- interrupts : should contain uart interrupt
> > +- id : should be the port ID defined by soc
> > +
> > +Optional properties:
> > +- fsl,has-rts-cts : indicate it has rts-cts
> > +- fsl,irda-mode : support irda mode
> > +
> > +Example:
> > +
> > +uart@73fbc000 {
> > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > +	reg = <0x73fbc000 0x4000>;
> > +	interrupts = <31>;
> > +	id = <1>;
> > +	fsl,has-rts-cts;
> > +};
> 
> Should this also support the "clock-frequency" property that 8250-style
> serial ports support [1]?
> 
I would ignore it for a while until we have common clock api and
corresponding binding settled.  For now, I would have nothing clock
related parsed from device tree.
Shawn Guo June 19, 2011, 7:41 a.m. UTC | #8
On Sat, Jun 18, 2011 at 10:26:55AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 06:21:46PM +0200, Arnd Bergmann wrote:
> > On Saturday 18 June 2011 17:19:12 Shawn Guo wrote:
> > >
> > > +Required properties:
> > > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
> > > +- reg : address and length of the register set for the device
> > > +- interrupts : should contain uart interrupt
> > > +- id : should be the port ID defined by soc
> > > +
> > > +Optional properties:
> > > +- fsl,has-rts-cts : indicate it has rts-cts
> > > +- fsl,irda-mode : support irda mode
> > > +
> > > +Example:
> > > +
> > > +uart@73fbc000 {
> > > +	compatible = "fsl,imx51-uart", "fsl,imx-uart";
> > > +	reg = <0x73fbc000 0x4000>;
> > > +	interrupts = <31>;
> > > +	id = <1>;
> > > +	fsl,has-rts-cts;
> > > +};
> > 
> > Should this also support the "clock-frequency" property that 8250-style
> > serial ports support [1]?
> > 
> > For the flow-control properties, should we name that more generic? The
> > same property certainly makes sense for other serial-ports if it does
> > here. OTOH, I'm not sure it's actually reliable, because it also depends
> > on whether the other side of the connection and the cable support hw flow
> > control.
> 
> I'd like to see a few use cases before defining a common property.
> That said, has-rts-cts does sound like a useful generic property.
> Or maybe named "uart-has-rts-cts" to make it clear that it is a uart
> specific binding?
> 
I would keep the name as it is if you do not mind, since it's been
under uart node.
Grant Likely June 19, 2011, 3:05 p.m. UTC | #9
On Sun, Jun 19, 2011 at 1:30 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Sat, Jun 18, 2011 at 10:19:34AM -0600, Grant Likely wrote:
>> On Sat, Jun 18, 2011 at 11:19:12PM +0800, Shawn Guo wrote:
>> > It adds device tree data parsing support for imx tty/serial driver.
>> >
>> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
>> > Signed-off-by: Jason Liu <jason.hui@linaro.org>
>> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> > ---
>> >  .../bindings/tty/serial/fsl-imx-uart.txt           |   21 +++++
>> >  drivers/tty/serial/imx.c                           |   81 +++++++++++++++++---
>> >  2 files changed, 92 insertions(+), 10 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> > new file mode 100644
>> > index 0000000..7648e17
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
>> > @@ -0,0 +1,21 @@
>> > +* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
>> > +
>> > +Required properties:
>> > +- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
>>
>> I'd make this "fsl,<soc>-uart", "fsl,imx51-uart"
>>
>> It's better to anchor these things on real silicon, or a real ip block
>> specification rather than something pseudo-generic.  Subsequent chips,
>> like the imx53, should simply claim compatibility with the older
>> fsl,imx51-uart.
>
> It is a real IP block on all imx silicons (except imx23 and imx28
> which are known as former stmp).
>
>>
>> (in essence, "fsl,imx51-uart" becomes the generic string without the
>> downside of having no obvious recourse when new silicon shows up that
>> is an imx part, but isn't compatible with the imx51 uart.
>>
> In this case, should imx1 the ancestor of imx family than imx51
> becomes part of that generic string?  Claiming uart of imx1, imx21
> and imx31 (senior than imx51) compatible with the imx51 uart seems
> odd to me.
>
> That said, IMO, "fsl,imx-uart" stands a real IP block specification
> here and can be a perfect generic compatibility string to tell the
> recourse of any imx silicon using this IP.

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

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

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

Ha ha ha.

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

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

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

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

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

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

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

g.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
new file mode 100644
index 0000000..7648e17
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/fsl-imx-uart.txt
@@ -0,0 +1,21 @@ 
+* Freescale i.MX Universal Asynchronous Receiver/Transmitter (UART)
+
+Required properties:
+- compatible : should be "fsl,<soc>-uart", "fsl,imx-uart"
+- reg : address and length of the register set for the device
+- interrupts : should contain uart interrupt
+- id : should be the port ID defined by soc
+
+Optional properties:
+- fsl,has-rts-cts : indicate it has rts-cts
+- fsl,irda-mode : support irda mode
+
+Example:
+
+uart@73fbc000 {
+	compatible = "fsl,imx51-uart", "fsl,imx-uart";
+	reg = <0x73fbc000 0x4000>;
+	interrupts = <31>;
+	id = <1>;
+	fsl,has-rts-cts;
+};
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a544731..2769353 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -45,6 +45,8 @@ 
 #include <linux/delay.h>
 #include <linux/rational.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1223,6 +1225,63 @@  static int serial_imx_resume(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static int serial_imx_probe_dt(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	const __be32 *line;
+
+	if (!node)
+		return -ENODEV;
+
+	line = of_get_property(node, "id", NULL);
+	if (!line)
+		return -ENODEV;
+
+	sport->port.line = be32_to_cpup(line) - 1;
+
+	if (of_get_property(node, "fsl,has-rts-cts", NULL))
+		sport->have_rtscts = 1;
+
+	if (of_get_property(node, "fsl,irda-mode", NULL))
+		sport->use_irda = 1;
+
+	return 0;
+}
+
+static struct of_device_id imx_uart_dt_ids[] = {
+	{ .compatible = "fsl,imx-uart" },
+	{},
+};
+#else
+static int serial_imx_probe_dt(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#define imx_uart_dt_ids NULL
+#endif
+
+static int serial_imx_probe_pdata(struct imx_port *sport,
+		struct platform_device *pdev)
+{
+	struct imxuart_platform_data *pdata = pdev->dev.platform_data;
+
+	if (!pdata)
+		return 0;
+
+	if (pdata->flags & IMXUART_HAVE_RTSCTS)
+		sport->have_rtscts = 1;
+
+	if (pdata->flags & IMXUART_IRDA)
+		sport->use_irda = 1;
+
+	sport->port.line = pdev->id;
+
+	return 0;
+}
+
 static int serial_imx_probe(struct platform_device *pdev)
 {
 	struct imx_port *sport;
@@ -1235,6 +1294,12 @@  static int serial_imx_probe(struct platform_device *pdev)
 	if (!sport)
 		return -ENOMEM;
 
+	ret = serial_imx_probe_dt(sport, pdev);
+	if (ret == -ENODEV)
+		ret = serial_imx_probe_pdata(sport, pdev);
+	if (ret)
+		goto free;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		ret = -ENODEV;
@@ -1259,7 +1324,6 @@  static int serial_imx_probe(struct platform_device *pdev)
 	sport->port.fifosize = 32;
 	sport->port.ops = &imx_pops;
 	sport->port.flags = UPF_BOOT_AUTOCONF;
-	sport->port.line = pdev->id;
 	init_timer(&sport->timer);
 	sport->timer.function = imx_timeout;
 	sport->timer.data     = (unsigned long)sport;
@@ -1273,17 +1337,13 @@  static int serial_imx_probe(struct platform_device *pdev)
 
 	sport->port.uartclk = clk_get_rate(sport->clk);
 
-	imx_ports[pdev->id] = sport;
+	if (imx_ports[sport->port.line]) {
+		ret = -EBUSY;
+		goto clkput;
+	}
+	imx_ports[sport->port.line] = sport;
 
 	pdata = pdev->dev.platform_data;
-	if (pdata && (pdata->flags & IMXUART_HAVE_RTSCTS))
-		sport->have_rtscts = 1;
-
-#ifdef CONFIG_IRDA
-	if (pdata && (pdata->flags & IMXUART_IRDA))
-		sport->use_irda = 1;
-#endif
-
 	if (pdata && pdata->init) {
 		ret = pdata->init(pdev);
 		if (ret)
@@ -1344,6 +1404,7 @@  static struct platform_driver serial_imx_driver = {
 	.driver		= {
 		.name	= "imx-uart",
 		.owner	= THIS_MODULE,
+		.of_match_table = imx_uart_dt_ids,
 	},
 };