[PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[John Linn]

The Xilinx 16550 uart core is not a standard 16550, because it uses
word-based addressing rather than byte-based addressing.  As a result,
it is not compatible with the open firmware 'ns16550' compatible
binding.  This code introduces new bindings, which pass the correct
register base and regshift properties to the uart driver to enable
this core to be used.  Doing this cleanly required some refactoring of
the existing code.

Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>
---
 drivers/serial/of_serial.c |   47 ++++++++++++++++++++++++++++++-------------
 1 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 2efb892..910c94f 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -20,13 +20,15 @@
 struct of_serial_info {
 	int type;
 	int line;
+	int regshift;
+	int regoffset;
 };
 
 /*
  * Fill a struct uart_port for a given device node
  */
 static int __devinit of_platform_serial_setup(struct of_device *ofdev,
-					int type, struct uart_port *port)
+					struct of_serial_info *info, struct uart_port *port)
 {
 	struct resource resource;
 	struct device_node *np = ofdev->node;
@@ -48,17 +50,16 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 	}
 
 	spin_lock_init(&port->lock);
-	port->mapbase = resource.start;
+	port->mapbase = resource.start + info->regoffset;
 	port->irq = irq_of_parse_and_map(np, 0);
 	port->iotype = UPIO_MEM;
-	port->type = type;
+	port->type = info->type;
+	port->regshift = info->regshift;
 	port->uartclk = *clk;
 	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
 		| UPF_FIXED_PORT;
 	port->dev = &ofdev->dev;
-	/* If current-speed was set, then try not to change it. */
-	if (spd)
-		port->custom_divisor = *clk / (16 * (*spd));
+	port->custom_divisor = *clk / (16 * (*spd));
 
 	return 0;
 }
@@ -81,8 +82,9 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
 	if (info == NULL)
 		return -ENOMEM;
 
-	port_type = (unsigned long)id->data;
-	ret = of_platform_serial_setup(ofdev, port_type, &port);
+	memcpy(info, id->data, sizeof(struct of_serial_info));
+	port_type = info->type;
+	ret = of_platform_serial_setup(ofdev, info, &port);
 	if (ret)
 		goto out;
 
@@ -100,7 +102,6 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
 	if (ret < 0)
 		goto out;
 
-	info->type = port_type;
 	info->line = ret;
 	ofdev->dev.driver_data = info;
 	return 0;
@@ -128,15 +129,33 @@ static int of_platform_serial_remove(struct of_device *ofdev)
 	return 0;
 }
 
+static struct of_serial_info __devinitdata ns8250_info = { .type = PORT_8250 };
+static struct of_serial_info __devinitdata ns16450_info = { .type = PORT_16450 };
+static struct of_serial_info __devinitdata ns16550_info = { .type = PORT_16550 };
+static struct of_serial_info __devinitdata ns16750_info = { .type = PORT_16750 };
+static struct of_serial_info __devinitdata xilinx_16550_info = {
+	.type = PORT_16550,
+	.regshift = 2,
+	.regoffset = 3,
+};
+static struct of_serial_info __devinitdata unknown_info = { .type = PORT_UNKNOWN };
+
 /*
  * A few common types, add more as needed.
  */
 static struct of_device_id __devinitdata of_platform_serial_table[] = {
-	{ .type = "serial", .compatible = "ns8250",   .data = (void *)PORT_8250, },
-	{ .type = "serial", .compatible = "ns16450",  .data = (void *)PORT_16450, },
-	{ .type = "serial", .compatible = "ns16550",  .data = (void *)PORT_16550, },
-	{ .type = "serial", .compatible = "ns16750",  .data = (void *)PORT_16750, },
-	{ .type = "serial",			      .data = (void *)PORT_UNKNOWN, },
+	{ .type = "serial", .compatible = "ns8250",   .data = &ns8250_info, },
+	{ .type = "serial", .compatible = "ns16450",  .data = &ns16450_info, },
+	{ .type = "serial", .compatible = "ns16550",  .data = &ns16550_info, },
+	{ .type = "serial", .compatible = "ns16750",  .data = &ns16750_info, },
+	{ .type = "serial", .compatible = "xlnx,opb-uart16550-1.00.c", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,opb-uart16550-1.00.d", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,opb-uart16550-1.00.e", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,plb-uart16550-1.00.c", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,xps-uart16550-1.00.a", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,xps-uart16550-2.00.a", .data = &xilinx_16550_info, },
+	{ .type = "serial", .compatible = "xlnx,xps-uart16550-2.00.b", .data = &xilinx_16550_info, },
+	{ .type = "serial",			      .data = &unknown_info, },
 	{ /* end of list */ },
 };
 
-- 
1.5.2.1

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com> wrote:
> The Xilinx 16550 uart core is not a standard 16550, because it uses
>  word-based addressing rather than byte-based addressing.  As a result,
>  it is not compatible with the open firmware 'ns16550' compatible
>  binding.  This code introduces new bindings, which pass the correct
>  register base and regshift properties to the uart driver to enable
>  this core to be used.  Doing this cleanly required some refactoring of
>  the existing code.

Personally, I'm not fond of this approach.  There is already some
traction to using the reg-shift property to specify spacing, and I
think it would be appropriate to also define a reg-offset property to
handle the +3 offset and then let the xilinx 16550 nodes use those.

More comments below.

>
>  Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
>  Signed-off-by: John Linn <john.linn@xilinx.com>
>  ---
>   drivers/serial/of_serial.c |   47 ++++++++++++++++++++++++++++++-------------
>   1 files changed, 33 insertions(+), 14 deletions(-)
>
>  diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
>  index 2efb892..910c94f 100644
>  --- a/drivers/serial/of_serial.c
>  +++ b/drivers/serial/of_serial.c
>  @@ -20,13 +20,15 @@
>   struct of_serial_info {
>         int type;
>         int line;
>  +       int regshift;
>  +       int regoffset;
>   };
>
>   /*
>   * Fill a struct uart_port for a given device node
>   */
>   static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>  -                                       int type, struct uart_port *port)
>  +                                       struct of_serial_info *info, struct uart_port *port)
>   {
>         struct resource resource;
>         struct device_node *np = ofdev->node;
>  @@ -48,17 +50,16 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>         }
>
>         spin_lock_init(&port->lock);
>  -       port->mapbase = resource.start;
>  +       port->mapbase = resource.start + info->regoffset;
>         port->irq = irq_of_parse_and_map(np, 0);
>         port->iotype = UPIO_MEM;
>  -       port->type = type;
>  +       port->type = info->type;
>  +       port->regshift = info->regshift;
>         port->uartclk = *clk;
>         port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
>                 | UPF_FIXED_PORT;
>         port->dev = &ofdev->dev;
>  -       /* If current-speed was set, then try not to change it. */
>  -       if (spd)
>  -               port->custom_divisor = *clk / (16 * (*spd));
>  +       port->custom_divisor = *clk / (16 * (*spd));

Oops, looks like you're undoing your previous patch here.

>
>         return 0;
>   }
>  @@ -81,8 +82,9 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
>         if (info == NULL)
>                 return -ENOMEM;
>
>  -       port_type = (unsigned long)id->data;
>  -       ret = of_platform_serial_setup(ofdev, port_type, &port);
>  +       memcpy(info, id->data, sizeof(struct of_serial_info));
>  +       port_type = info->type;
>  +       ret = of_platform_serial_setup(ofdev, info, &port);
>         if (ret)
>                 goto out;
>
>  @@ -100,7 +102,6 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
>         if (ret < 0)
>                 goto out;
>
>  -       info->type = port_type;
>         info->line = ret;
>         ofdev->dev.driver_data = info;
>         return 0;
>  @@ -128,15 +129,33 @@ static int of_platform_serial_remove(struct of_device *ofdev)
>         return 0;
>   }
>
>  +static struct of_serial_info __devinitdata ns8250_info = { .type = PORT_8250 };
>  +static struct of_serial_info __devinitdata ns16450_info = { .type = PORT_16450 };
>  +static struct of_serial_info __devinitdata ns16550_info = { .type = PORT_16550 };
>  +static struct of_serial_info __devinitdata ns16750_info = { .type = PORT_16750 };
>  +static struct of_serial_info __devinitdata xilinx_16550_info = {
>  +       .type = PORT_16550,
>  +       .regshift = 2,
>  +       .regoffset = 3,
>  +};
>  +static struct of_serial_info __devinitdata unknown_info = { .type = PORT_UNKNOWN };

In support of my argument; the fact that you need a table of data says
to me that this data should really be encoded in the device tree.  :-)

Cheers,
g.


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

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Paul Mackerras]

Grant Likely writes:

> Personally, I'm not fond of this approach.  There is already some
> traction to using the reg-shift property to specify spacing, and I
> think it would be appropriate to also define a reg-offset property to
> handle the +3 offset and then let the xilinx 16550 nodes use those.

Why do we need a reg-offset property when we can just add the offset
to the appropriate word(s) in the reg property?

Paul.

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Segher Boessenkool]

>> Personally, I'm not fond of this approach.  There is already some
>> traction to using the reg-shift property to specify spacing, and I
>> think it would be appropriate to also define a reg-offset property to
>> handle the +3 offset and then let the xilinx 16550 nodes use those.
>
> Why do we need a reg-offset property when we can just add the offset
> to the appropriate word(s) in the reg property?

Because if you do that, the "reg" property cannot describe the full
register block (it misses the first few bytes).  Not a huge problem
in practice, sure.


Segher

RE: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart16550.

[Stephen Neuendorffer]

I thought about this, and reconsidered it again after I finally figured
out how to get the Power.org website to relenquish the ePAPR spec, which
(FWIW) has reg-shift as an optional property of the ns16550 binding.

However, I'm not enamoured of it, since I doubt it's really good to be
doing ioremaps and memory management on unaligned blocks.  It seems more
logical to me to have the reg property define a range of aligned
addresses, and in this case, it happens that the driver never touches
some of those bytes.

In any event, the real point of the patch was to spark some discussion,
which we failed to get otherwise.. :)  I think any of these proposals
are workable...

Steve

> -----Original Message-----
> From: =
linuxppc-dev-bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org] On Behalf Of
Segher Boessenkool
> Sent: Friday, March 21, 2008 4:40 AM
> To: Paul Mackerras
> Cc: linuxppc-dev@ozlabs.org; John Linn
> Subject: Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for
Xilinx uart16550.
>=20
> >> Personally, I'm not fond of this approach.  There is already some
> >> traction to using the reg-shift property to specify spacing, and I
> >> think it would be appropriate to also define a reg-offset property
to
> >> handle the +3 offset and then let the xilinx 16550 nodes use those.
> >
> > Why do we need a reg-offset property when we can just add the offset
> > to the appropriate word(s) in the reg property?
>=20
> Because if you do that, the "reg" property cannot describe the full
> register block (it misses the first few bytes).  Not a huge problem
> in practice, sure.
>=20
>=20
> Segher
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart16550.

[Segher Boessenkool]

> I thought about this, and reconsidered it again after I finally figured
> out how to get the Power.org website to relenquish the ePAPR spec, 
> which
> (FWIW) has reg-shift as an optional property of the ns16550 binding.
>
> However, I'm not enamoured of it, since I doubt it's really good to be
> doing ioremaps and memory management on unaligned blocks.  It seems 
> more
> logical to me to have the reg property define a range of aligned
> addresses, and in this case, it happens that the driver never touches
> some of those bytes.

Yes, and that is (part of) why "reg-offset" might be needed /
might be a good idea.

> In any event, the real point of the patch was to spark some discussion,
> which we failed to get otherwise.. :)

You succeeded :-)


Segher

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Fri, Mar 21, 2008 at 3:21 AM, Paul Mackerras <paulus@samba.org> wrote:
> Grant Likely writes:
>
>  > Personally, I'm not fond of this approach.  There is already some
>  > traction to using the reg-shift property to specify spacing, and I
>  > think it would be appropriate to also define a reg-offset property to
>  > handle the +3 offset and then let the xilinx 16550 nodes use those.
>
>  Why do we need a reg-offset property when we can just add the offset
>  to the appropriate word(s) in the reg property?

Primarily because the device creates 32 byte registers starting at 0;
but they are also big-endian byte accessible so a byte read at offset
8 also works.  reg-offset seems to be a better description of the
hardware to me.

Cheers,
g.

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

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Grant Likely wrote:

>> > Personally, I'm not fond of this approach.  There is already some
>> > traction to using the reg-shift property to specify spacing, and I
>> > think it would be appropriate to also define a reg-offset property to
>> > handle the +3 offset and then let the xilinx 16550 nodes use those.

>> Why do we need a reg-offset property when we can just add the offset
>> to the appropriate word(s) in the reg property?

> Primarily because the device creates 32 byte registers starting at 0;
> but they are also big-endian byte accessible so a byte read at offset
> 8 also works.  reg-offset seems to be a better description of the
> hardware to me.

    Ugh... I was just going is it possible to access the chip registers as 
32-bit entities, and employ UPIO_MEM32 mode of 8250.c -- just to avoid that 
reg-offset wart. Now you're telling everybody that it's completely 
superfluous... :-)

> Cheers,
> g.

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Grant Likely wrote:

>> > Personally, I'm not fond of this approach.  There is already some
>> > traction to using the reg-shift property to specify spacing, and I
>> > think it would be appropriate to also define a reg-offset property to
>> > handle the +3 offset and then let the xilinx 16550 nodes use those.

>> Why do we need a reg-offset property when we can just add the offset
>> to the appropriate word(s) in the reg property?

> Primarily because the device creates 32 byte registers starting at 0;
> but they are also big-endian byte accessible so a byte read at offset
> 8 also works.

    Probably I misunderstood you: does it give the same result as offset 11?

> reg-offset seems to be a better description of the hardware to me.

    Have you considered using the existing "big-endian" property?

> Cheers,
> g.

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Mon, Mar 24, 2008 at 8:09 AM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Grant Likely wrote:
>
>  >> > Personally, I'm not fond of this approach.  There is already some
>  >> > traction to using the reg-shift property to specify spacing, and I
>  >> > think it would be appropriate to also define a reg-offset property to
>  >> > handle the +3 offset and then let the xilinx 16550 nodes use those.
>
>  >> Why do we need a reg-offset property when we can just add the offset
>  >> to the appropriate word(s) in the reg property?
>
>  > Primarily because the device creates 32 byte registers starting at 0;
>  > but they are also big-endian byte accessible so a byte read at offset
>  > 8 also works.
>
>     Probably I misunderstood you: does it give the same result as offset 11?

er; typo; oops.  A 32 bit read add offset 0 is the same as a byte read
at offset *3*.

>
>
>  > reg-offset seems to be a better description of the hardware to me.
>
>     Have you considered using the existing "big-endian" property?

No I haven't, but that would work too.  I'm happy with that if it
works for you.  If the property was defined, then the byte offset to
the first reg would be adjusted by 1^(reg-shift) - 1

Cheers,
g.

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

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Grant Likely wrote:

>> >> > Personally, I'm not fond of this approach.  There is already some
>> >> > traction to using the reg-shift property to specify spacing, and I
>> >> > think it would be appropriate to also define a reg-offset property to
>> >> > handle the +3 offset and then let the xilinx 16550 nodes use those.

>> >> Why do we need a reg-offset property when we can just add the offset
>> >> to the appropriate word(s) in the reg property?
>
>> > Primarily because the device creates 32 byte registers starting at 0;
>> > but they are also big-endian byte accessible so a byte read at offset
>> > 8 also works.

>>    Probably I misunderstood you: does it give the same result as offset 11?

> er; typo; oops.  A 32 bit read add offset 0 is the same as a byte read
> at offset *3*.

    Oh, well... unfortunately, we can't use UPIO_MEM32 "register model" in 
8250.c anyway since that makes use of readl()/writel() -- which treat the bus 
as bigendian on PPC... anyway, we would need at least a "reg-size" property, 
if not new "compatible"...

>> > reg-offset seems to be a better description of the hardware to me.

>>    Have you considered using the existing "big-endian" property?

> No I haven't, but that would work too.  I'm happy with that if it
> works for you.  If the property was defined, then the byte offset to
> the first reg would be adjusted by 1^(reg-shift) - 1

    You don't mean "xor" by ^, do you? :-O
    In fact, it should be <<...

> Cheers,
> g.

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Mon, Mar 24, 2008 at 10:15 AM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Grant Likely wrote:
>  >>    Probably I misunderstood you: does it give the same result as offset 11?
>
>  > er; typo; oops.  A 32 bit read add offset 0 is the same as a byte read
>  > at offset *3*.
>
>     Oh, well... unfortunately, we can't use UPIO_MEM32 "register model" in
>  8250.c anyway since that makes use of readl()/writel() -- which treat the bus
>  as bigendian on PPC... anyway, we would need at least a "reg-size" property,
>  if not new "compatible"...

:-) ... and the *driver* itself isn't any sort of issue; it's just
figuring out the best way to describe the hardware
accurately/appropriately so the binding can decide how to configure
the driver.

>  >>    Have you considered using the existing "big-endian" property?
>
>  > No I haven't, but that would work too.  I'm happy with that if it
>  > works for you.  If the property was defined, then the byte offset to
>  > the first reg would be adjusted by 1^(reg-shift) - 1
>
>     You don't mean "xor" by ^, do you? :-O
>     In fact, it should be <<...

heh; pseudocoding in the wrong language.  You, of course, are right.

Cheers,
g.

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

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Hi, I wrote:

>    Oh, well... unfortunately, we can't use UPIO_MEM32 "register model" 
> in 8250.c anyway since that makes use of readl()/writel() -- which treat 
> the bus as bigendian on PPC... anyway, we would need at least a 

    I was going to write "as little-endian"... :-<

> "reg-size" property, if not new "compatible"...

WBR, Sergei

RE: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[John Linn]

Thanks for all the input. I'd like to propose a solution hoping to
arrive at consensus.

We can define a "sparse16550" binding that uses reg-shift and reg-offset
properties in the device tree.=20

It seems like this is a common variable on the 16550 so that others
could also use it.  I like it because it specifies the differences
clearly from a normal 16550 yet is flexible enough to handle some other
variations also.

Thanks,
John

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]=20
Sent: Monday, March 24, 2008 11:04 AM
To: Sergei Shtylyov
Cc: Grant Likely; linuxppc-dev@ozlabs.org; Paul Mackerras; John Linn
Subject: Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx
uart 16550.

Hi, I wrote:

>    Oh, well... unfortunately, we can't use UPIO_MEM32 "register model"

> in 8250.c anyway since that makes use of readl()/writel() -- which
treat=20
> the bus as bigendian on PPC... anyway, we would need at least a=20

    I was going to write "as little-endian"... :-<

> "reg-size" property, if not new "compatible"...

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Hello.

Grant Likely wrote:

> On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com> wrote:

>>The Xilinx 16550 uart core is not a standard 16550, because it uses
>> word-based addressing rather than byte-based addressing.  As a result,
>> it is not compatible with the open firmware 'ns16550' compatible
>> binding.  This code introduces new bindings, which pass the correct
>> register base and regshift properties to the uart driver to enable
>> this core to be used.  Doing this cleanly required some refactoring of
>> the existing code.

> Personally, I'm not fond of this approach.  There is already some
> traction to using the reg-shift property to specify spacing, and I
> think it would be appropriate to also define a reg-offset property to
> handle the +3 offset and then let the xilinx 16550 nodes use those.

    That's making things only worse than the mere "reg-shift" idea. I think 
that both are totally wrong. Everything about the programming interface should 
be said in the "compatible" and possibly "model" properties. of_serial driver 
should recognize them and pass the necessary details to 8250.c. As for me, I'm 
strongly against plaguing the device tree with the *Linux driver 
implementation specifics* (despite I was trying this with MTD -- there it 
seemed somewhat more grounded :-).

> More comments below.

>> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
>> Signed-off-by: John Linn <john.linn@xilinx.com>

>> diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
>> index 2efb892..910c94f 100644
>> --- a/drivers/serial/of_serial.c
>> +++ b/drivers/serial/of_serial.c

[...]

>>        return 0;
>>  }
>> @@ -81,8 +82,9 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
>>        if (info == NULL)
>>                return -ENOMEM;
>>
>> -       port_type = (unsigned long)id->data;
>> -       ret = of_platform_serial_setup(ofdev, port_type, &port);
>> +       memcpy(info, id->data, sizeof(struct of_serial_info));
>> +       port_type = info->type;
>> +       ret = of_platform_serial_setup(ofdev, info, &port);
>>        if (ret)
>>                goto out;
>>
>> @@ -100,7 +102,6 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev,
>>        if (ret < 0)
>>                goto out;
>>
>> -       info->type = port_type;
>>        info->line = ret;
>>        ofdev->dev.driver_data = info;
>>        return 0;
>> @@ -128,15 +129,33 @@ static int of_platform_serial_remove(struct of_device *ofdev)
>>        return 0;
>>  }
>>
>> +static struct of_serial_info __devinitdata ns8250_info = { .type = PORT_8250 };
>> +static struct of_serial_info __devinitdata ns16450_info = { .type = PORT_16450 };
>> +static struct of_serial_info __devinitdata ns16550_info = { .type = PORT_16550 };
>> +static struct of_serial_info __devinitdata ns16750_info = { .type = PORT_16750 };
>> +static struct of_serial_info __devinitdata xilinx_16550_info = {
>> +       .type = PORT_16550,
>> +       .regshift = 2,
>> +       .regoffset = 3,

    I see that the data is already encoded in the driver itself, so I agree 
with the patch.

>> +};
>> +static struct of_serial_info __devinitdata unknown_info = { .type = PORT_UNKNOWN };

> In support of my argument; the fact that you need a table of data says
> to me that this data should really be encoded in the device tree.  :-)

    Not at all.

> Cheers,
> g.

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Segher Boessenkool]

>> Personally, I'm not fond of this approach.  There is already some
>> traction to using the reg-shift property to specify spacing, and I
>> think it would be appropriate to also define a reg-offset property to
>> handle the +3 offset and then let the xilinx 16550 nodes use those.
>
>    That's making things only worse than the mere "reg-shift" idea. I 
> think that both are totally wrong. Everything about the programming 
> interface should be said in the "compatible" and possibly "model" 
> properties.

No.  In effect, you are saying here that no device binding should define
any binding-specific properties.  This will just lead to combinatorial
explosion of "compatible" values.

That said, "reg-spacing"/"reg-shift"/"reg-offset" should *not* be
considered something generic; they are part of specific device
bindings.  Of course it is nice if various bindings use the same
names for the same concepts, but that's an orthogonal issue.

>  of_serial driver should recognize them and pass the necessary details 
> to 8250.c. As for me, I'm strongly against plaguing the device tree 
> with the *Linux driver implementation specifics*

"reg-*" has nothing to do with Linux device driver implementation
issues: it describes how a device is physically wired up!

> (despite I was trying this with MTD -- there it seemed somewhat more 
> grounded :-).

Quite the opposite, but let's not rehash that discussion.

>> In support of my argument; the fact that you need a table of data says
>> to me that this data should really be encoded in the device tree.  :-)
>
>    Not at all.

Not _necessarily_.  I agree with Grant here: for many of these devices
with byte-size registers, it is very common to find them with their
register banks wired up differently, and that is often the *only*
difference to the "normal" device.  In this situation, it makes a lot
of sense to describe that difference with "reg-*" properties.

In some other situations, it is better to create a new binding for
the device.


Segher

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

>>> Personally, I'm not fond of this approach.  There is already some
>>> traction to using the reg-shift property to specify spacing, and I
>>> think it would be appropriate to also define a reg-offset property to
>>> handle the +3 offset and then let the xilinx 16550 nodes use those.

>>    That's making things only worse than the mere "reg-shift" idea. I 
>> think that both are totally wrong. Everything about the programming 
>> interface should be said in the "compatible" and possibly "model" 
>> properties.

> No.  In effect, you are saying here that no device binding should define
> any binding-specific properties.  This will just lead to combinatorial
> explosion of "compatible" values.

> That said, "reg-spacing"/"reg-shift"/"reg-offset" should *not* be
> considered something generic; they are part of specific device
> bindings.  Of course it is nice if various bindings use the same
> names for the same concepts, but that's an orthogonal issue.

    The proposed use clearly would treat them as generic, since in the context 
of the Xilinx UART they're just not needed -- it's known beforehand and most 
probably fixed how/where the registers are mapped. There's just no need for 
such info in the device tree -- unless you're going to teach the *generic* 
driver to handle this specific (and possibly others alike) kind of a device.

>>  of_serial driver should recognize them and pass the necessary details 
>> to 8250.c. As for me, I'm strongly against plaguing the device tree 
>> with the *Linux driver implementation specifics*

> "reg-*" has nothing to do with Linux device driver implementation
> issues: it describes how a device is physically wired up!

    Hm... wasn't that you who were telling that use of "range" properties 
guarantees 1:1 correspondence of the upstream/downstream bus addresses (in 
their LSB part of course -- meaning that the device registers 0..x are seen by 
the CPU at addresses base+0..base+X?

>> (despite I was trying this with MTD -- there it seemed somewhat more 
>> grounded :-).

> Quite the opposite, but let's not rehash that discussion.

    I might be mixing with the February thread about this UART -- have 
alsready forgotten about it.

>>> In support of my argument; the fact that you need a table of data says
>>> to me that this data should really be encoded in the device tree.  :-)

>>    Not at all.

> Not _necessarily_.  I agree with Grant here: for many of these devices
> with byte-size registers, it is very common to find them with their
> register banks wired up differently, and that is often the *only*
> difference to the "normal" device.  In this situation, it makes a lot
> of sense to describe that difference with "reg-*" properties.

    Note that "compicated" mapping is not (necessarily) a property of the 
device itself but generally a property of the chip select circuit, i.e. 
external entity.

> In some other situations, it is better to create a new binding for
> the device.

> Segher

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Segher Boessenkool]

>    The proposed use clearly would treat them as generic, since in the 
> context of the Xilinx UART they're just not needed -- it's known 
> beforehand and most probably fixed how/where the registers are mapped. 
> There's just no need for such info in the device tree -- unless you're 
> going to teach the *generic* driver to handle this specific (and 
> possibly others alike) kind of a device.

I was under the impression that the "xilinx uart" was just a 16550 (or 
so)
with its registers wired up in a slightly unusual way.  If it's a 
completely
different device, of course you need a separate binding, and you might 
not
want reg-shift properties etc. there.

>> "reg-*" has nothing to do with Linux device driver implementation
>> issues: it describes how a device is physically wired up!
>
>    Hm... wasn't that you who were telling that use of "range" 
> properties guarantees 1:1 correspondence of the upstream/downstream 
> bus addresses (in their LSB part of course -- meaning that the device 
> registers 0..x are seen by the CPU at addresses base+0..base+X?

I have no idea what "ranges" has to do with this.  This device is not
a memory-mapped bus, it's a UART.

>>>> In support of my argument; the fact that you need a table of data 
>>>> says
>>>> to me that this data should really be encoded in the device tree.  
>>>> :-)
>
>>>    Not at all.
>
>> Not _necessarily_.  I agree with Grant here: for many of these devices
>> with byte-size registers, it is very common to find them with their
>> register banks wired up differently, and that is often the *only*
>> difference to the "normal" device.  In this situation, it makes a lot
>> of sense to describe that difference with "reg-*" properties.
>
>    Note that "compicated" mapping is not (necessarily) a property of 
> the device itself but generally a property of the chip select circuit, 
> i.e. external entity.

There is no difference insofar as the device tree is concerned.


Segher

RE: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart16550.

[Stephen Neuendorffer]

> -----Original Message-----
> From: =
linuxppc-dev-bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org] On Behalf Of
Segher Boessenkool
> Sent: Friday, March 21, 2008 9:46 AM
> To: Sergei Shtylyov
> Cc: linuxppc-dev@ozlabs.org; John Linn
> Subject: Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for
Xilinx uart16550.
>=20
> >    The proposed use clearly would treat them as generic, since in
the
> > context of the Xilinx UART they're just not needed -- it's known
> > beforehand and most probably fixed how/where the registers are
mapped.
> > There's just no need for such info in the device tree -- unless
you're
> > going to teach the *generic* driver to handle this specific (and
> > possibly others alike) kind of a device.
>=20
> I was under the impression that the "xilinx uart" was just a 16550 (or
> so)
> with its registers wired up in a slightly unusual way.  If it's a
> completely
> different device, of course you need a separate binding, and you might
> not
> want reg-shift properties etc. there.

It's a standard 16550, with registers in the standard order, but at
offsets 3,7,11, etc.

Steve

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Segher Boessenkool wrote:

>>    The proposed use clearly would treat them as generic, since in the 
>> context of the Xilinx UART they're just not needed -- it's known 
>> beforehand and most probably fixed how/where the registers are mapped. 
>> There's just no need for such info in the device tree -- unless you're 
>> going to teach the *generic* driver to handle this specific (and 
>> possibly others alike) kind of a device.

> I was under the impression that the "xilinx uart" was just a 16550 (or so)
> with its registers wired up in a slightly unusual way.  If it's a 
> completely
> different device, of course you need a separate binding, and you might not
> want reg-shift properties etc. there.

    It seems to be 16550 in the core, although coming in many flavors. :-)

>>> "reg-*" has nothing to do with Linux device driver implementation
>>> issues: it describes how a device is physically wired up!

>>    Hm... wasn't that you who were telling that use of "range" 
>> properties guarantees 1:1 correspondence of the upstream/downstream 
>> bus addresses (in their LSB part of course -- meaning that the device 
>> registers 0..x are seen by the CPU at addresses base+0..base+X?

> I have no idea what "ranges" has to do with this.

    Me neither. :-)

> This device is not a memory-mapped bus, it's a UART.

    IIRC, we were discussing MTD then, with an imaginary (for PPC) case of it 
being reverse-endian (actually happened on other archs)... IMHO, how the child 
bus addresses correspond to parent's one 1:1, doesn't imply that you can't 
have a device wired to the child bus a strange way, e.g. with byte-enables 
"reversed"...

>>>>> In support of my argument; the fact that you need a table of data says
>>>>> to me that this data should really be encoded in the device tree.  :-)

>>>>    Not at all.

>>> Not _necessarily_.  I agree with Grant here: for many of these devices
>>> with byte-size registers, it is very common to find them with their
>>> register banks wired up differently, and that is often the *only*
>>> difference to the "normal" device.  In this situation, it makes a lot
>>> of sense to describe that difference with "reg-*" properties.

>>    Note that "compicated" mapping is not (necessarily) a property of 
>> the device itself but generally a property of the chip select circuit, 
>> i.e. external entity.

> There is no difference insofar as the device tree is concerned.

    OK...

> Segher

WBR, Sergei

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Fri, Mar 21, 2008 at 7:00 AM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
>  Grant Likely wrote:
>  > Personally, I'm not fond of this approach.  There is already some
>  > traction to using the reg-shift property to specify spacing, and I
>  > think it would be appropriate to also define a reg-offset property to
>  > handle the +3 offset and then let the xilinx 16550 nodes use those.
>
>     That's making things only worse than the mere "reg-shift" idea. I think
>  that both are totally wrong. Everything about the programming interface should
>  be said in the "compatible" and possibly "model" properties. of_serial driver
>  should recognize them and pass the necessary details to 8250.c. As for me, I'm
>  strongly against plaguing the device tree with the *Linux driver
>  implementation specifics* (despite I was trying this with MTD -- there it
>  seemed somewhat more grounded :-).

Not true.  Compatible defines what the node is describing.  It is
perfectly valid for a compatible value definition to also defines some
additional properties that can be queried for interface details.
Xilinx is completely free to define a "xlnx,..." compatible value for
their ns16550 compatible device.  However, 'sparse' ns16550 devices
are a common and well known variation so I think it is valid and
reasonable to define a compatible binding for this case.

As for using a new binding like "sparse16550" instead of extending
"ns16550"; it is because reg-shift and reg-offset would be required
nodes and therefore is not compatible with drivers using the original
ns16550 binding.  Using a new namespace gives freedom to define the
required properties.

Cheers,
g.

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

Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Grant Likely wrote:

>> > Personally, I'm not fond of this approach.  There is already some
>> > traction to using the reg-shift property to specify spacing, and I
>> > think it would be appropriate to also define a reg-offset property to
>> > handle the +3 offset and then let the xilinx 16550 nodes use those.

>>    That's making things only worse than the mere "reg-shift" idea. I think
>> that both are totally wrong. Everything about the programming interface should
>> be said in the "compatible" and possibly "model" properties. of_serial driver
>> should recognize them and pass the necessary details to 8250.c. As for me, I'm
>> strongly against plaguing the device tree with the *Linux driver
>> implementation specifics* (despite I was trying this with MTD -- there it
>> seemed somewhat more grounded :-).

> Not true.  Compatible defines what the node is describing.  It is
> perfectly valid for a compatible value definition to also defines some
> additional properties that can be queried for interface details.
> Xilinx is completely free to define a "xlnx,..." compatible value for
> their ns16550 compatible device.  However, 'sparse' ns16550 devices
> are a common and well known variation so I think it is valid and
> reasonable to define a compatible binding for this case.

    We have been mostly talking about the 16550-compatible devices which 
external circuitry makes "sparse" so far. This is surely not a property of a 
16550 device to be "sparse" or not, although some say that this doesn't 
matter. :-)

> As for using a new binding like "sparse16550" instead of extending

    That "sparse16550" again... what if it's a superset of 16550 (not an 
uncommon case too), will you also define "sparce16650", "sparse16570", and so 
on? :-)

> "ns16550"; it is because reg-shift and reg-offset would be required
> nodes and therefore is not compatible with drivers using the original
> ns16550 binding.  Using a new namespace gives freedom to define the
> required properties.

    You'll have to define several namespaces I'm afraid...

> Cheers,
> g.

WBR, Sergei

RE: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart16550.

[Stephen Neuendorffer]

> -----Original Message-----
> From: =
linuxppc-dev-bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org] On Behalf Of =
Grant
Likely
> Sent: Thursday, March 20, 2008 5:19 PM
> To: John Linn
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for
Xilinx uart16550.
>=20
> On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com>
wrote:
> > The Xilinx 16550 uart core is not a standard 16550, because it uses
> >  word-based addressing rather than byte-based addressing.  As a
result,
> >  it is not compatible with the open firmware 'ns16550' compatible
> >  binding.  This code introduces new bindings, which pass the correct
> >  register base and regshift properties to the uart driver to enable
> >  this core to be used.  Doing this cleanly required some refactoring
of
> >  the existing code.
>=20
> Personally, I'm not fond of this approach.  There is already some
> traction to using the reg-shift property to specify spacing, and I
> think it would be appropriate to also define a reg-offset property to
> handle the +3 offset and then let the xilinx 16550 nodes use those.

Since the ePAPR ns16550 defines reg-shift, I don't think it makes sense
to have a separate binding for sparse16550, or for the xilinx16550
types.  Personally, I like having reg-offset better than
adding three to the reg space, but I leave that up to someone who is
much more involved in ePAPR.

> >  +static struct of_serial_info __devinitdata ns8250_info =3D { .type =
=3D
PORT_8250 };
> >  +static struct of_serial_info __devinitdata ns16450_info =3D { =
.type
=3D PORT_16450 };
> >  +static struct of_serial_info __devinitdata ns16550_info =3D { =
.type
=3D PORT_16550 };
> >  +static struct of_serial_info __devinitdata ns16750_info =3D { =
.type
=3D PORT_16750 };
> >  +static struct of_serial_info __devinitdata xilinx_16550_info =3D {
> >  +       .type =3D PORT_16550,
> >  +       .regshift =3D 2,
> >  +       .regoffset =3D 3,
> >  +};
> >  +static struct of_serial_info __devinitdata unknown_info =3D { =
.type
=3D PORT_UNKNOWN };
>=20
> In support of my argument; the fact that you need a table of data says
> to me that this data should really be encoded in the device tree.  :-)

The table of data was always there to bind the UART types, I just added
some more info to it.

Steve

[PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[John Linn]

The Xilinx 16550 uart core is not a standard 16550, because it uses
word-based addressing rather than byte-based addressing.  As a result,
it is not compatible with the open firmware 'ns16550' compatible
binding.

This code adds the Xilinx uart 16550 to the serial console
of the boot. A new initialization function for the Xilinx 16550 uart
is added to the ns16550 driver. It sets up the correct register base
and regshift properties specific to the Xilinx 16550.

Signed-off-by: John Linn <john.linn@xilinx.com>
---
 arch/powerpc/boot/ns16550.c |   24 ++++++++++++++++++++++++
 arch/powerpc/boot/ops.h     |    1 +
 arch/powerpc/boot/serial.c  |    8 ++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index f8f1b2f..5385255 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -77,3 +77,27 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
 
 	return 0;
 }
+
+int xilinx16550_console_init(void *devp, struct serial_console_data *scdp)
+{
+	int n;
+	unsigned long reg_phys;
+
+	n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
+	if (n != sizeof(reg_base)) {
+		if (!dt_xlate_reg(devp, 0, &reg_phys, NULL))
+			return -1;
+
+		reg_base = (void *)reg_phys + 3;
+	}
+
+	reg_shift = 2;
+
+	scdp->open = ns16550_open;
+	scdp->putc = ns16550_putc;
+	scdp->getc = ns16550_getc;
+	scdp->tstc = ns16550_tstc;
+	scdp->close = NULL;
+
+	return 0;
+}
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index a180b65..d8b864b 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -86,6 +86,7 @@ int mpsc_console_init(void *devp, struct serial_console_data *scdp);
 int cpm_console_init(void *devp, struct serial_console_data *scdp);
 int mpc5200_psc_console_init(void *devp, struct serial_console_data *scdp);
 int uartlite_console_init(void *devp, struct serial_console_data *scdp);
+int xilinx16550_console_init(void *devp, struct serial_console_data *scdp);
 void *simple_alloc_init(char *base, unsigned long heap_size,
 			unsigned long granularity, unsigned long max_allocs);
 extern void flush_cache(void *, unsigned long);
diff --git a/arch/powerpc/boot/serial.c b/arch/powerpc/boot/serial.c
index 7fa5078..fb5c91e 100644
--- a/arch/powerpc/boot/serial.c
+++ b/arch/powerpc/boot/serial.c
@@ -131,6 +131,14 @@ int serial_console_init(void)
 	else if (dt_is_compatible(devp, "xlnx,opb-uartlite-1.00.b") ||
                  dt_is_compatible(devp, "xlnx,xps-uartlite-1.00.a"))
 		rc = uartlite_console_init(devp, &serial_cd);
+	else if (dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.c") ||
+		 dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.d") ||
+		 dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.e") ||
+		 dt_is_compatible(devp, "xlnx,plb-uart16550-1.00.c") ||
+		 dt_is_compatible(devp, "xlnx,xps-uart16550-1.00.a") ||
+		 dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.a") ||
+		 dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.b"))
+		rc = xilinx16550_console_init(devp, &serial_cd);
 
 	/* Add other serial console driver calls here */
 
-- 
1.5.2.1

Re: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[Grant Likely]

On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com> wrote:
> The Xilinx 16550 uart core is not a standard 16550, because it uses
>  word-based addressing rather than byte-based addressing.  As a result,
>  it is not compatible with the open firmware 'ns16550' compatible
>  binding.
>
>  This code adds the Xilinx uart 16550 to the serial console
>  of the boot. A new initialization function for the Xilinx 16550 uart
>  is added to the ns16550 driver. It sets up the correct register base
>  and regshift properties specific to the Xilinx 16550.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>
>  --- a/arch/powerpc/boot/ops.h
>  +++ b/arch/powerpc/boot/ops.h
>  @@ -86,6 +86,7 @@ int mpsc_console_init(void *devp, struct serial_console_data *scdp);
>   int cpm_console_init(void *devp, struct serial_console_data *scdp);
>   int mpc5200_psc_console_init(void *devp, struct serial_console_data *scdp);
>   int uartlite_console_init(void *devp, struct serial_console_data *scdp);
>  +int xilinx16550_console_init(void *devp, struct serial_console_data *scdp);
>   void *simple_alloc_init(char *base, unsigned long heap_size,
>                         unsigned long granularity, unsigned long max_allocs);
>   extern void flush_cache(void *, unsigned long);
>  diff --git a/arch/powerpc/boot/serial.c b/arch/powerpc/boot/serial.c
>  index 7fa5078..fb5c91e 100644
>  --- a/arch/powerpc/boot/serial.c
>  +++ b/arch/powerpc/boot/serial.c
>  @@ -131,6 +131,14 @@ int serial_console_init(void)
>         else if (dt_is_compatible(devp, "xlnx,opb-uartlite-1.00.b") ||
>                   dt_is_compatible(devp, "xlnx,xps-uartlite-1.00.a"))
>                 rc = uartlite_console_init(devp, &serial_cd);
>  +       else if (dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.c") ||
>  +                dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.d") ||
>  +                dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.e") ||
>  +                dt_is_compatible(devp, "xlnx,plb-uart16550-1.00.c") ||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-1.00.a") ||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.a") ||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.b"))
>  +               rc = xilinx16550_console_init(devp, &serial_cd);

I'll review this whole patch set today, but I need to make this
comment now.  Adding to this ever increasing list of xilinx device
versions is not sustainable.  A common base compatible value needs to
be chosen instead.  Way back in the mists of time something line
"sparse16550" was suggested and I think it is a good one.  xilinx uart
nodes should claim compatibility with their specific version *and*
"sparse16550".  Then all this code should only match with
"sparse16550" and documentation should be added to
Documentation/powerpc/booting-without-of.txt to describe what
sparse16550 means.

Cheers,
g.

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

RE: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[John Linn]

You're right.  Rick and I were just talking about could we not check the
versions but just make sure the device is the right type independent of
the versions.

This issue will be true for all the Xilinx drivers.

I think FPGA IP is a little different in that the hardware can change
much more often than in hard devices.

-- John

-----Original Message-----
From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of
Grant Likely
Sent: Thursday, March 20, 2008 8:55 AM
To: John Linn
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart
16550.

On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com> wrote:
> The Xilinx 16550 uart core is not a standard 16550, because it uses
>  word-based addressing rather than byte-based addressing.  As a
result,
>  it is not compatible with the open firmware 'ns16550' compatible
>  binding.
>
>  This code adds the Xilinx uart 16550 to the serial console
>  of the boot. A new initialization function for the Xilinx 16550 uart
>  is added to the ns16550 driver. It sets up the correct register base
>  and regshift properties specific to the Xilinx 16550.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>
>  --- a/arch/powerpc/boot/ops.h
>  +++ b/arch/powerpc/boot/ops.h
>  @@ -86,6 +86,7 @@ int mpsc_console_init(void *devp, struct
serial_console_data *scdp);
>   int cpm_console_init(void *devp, struct serial_console_data *scdp);
>   int mpc5200_psc_console_init(void *devp, struct serial_console_data
*scdp);
>   int uartlite_console_init(void *devp, struct serial_console_data
*scdp);
>  +int xilinx16550_console_init(void *devp, struct serial_console_data
*scdp);
>   void *simple_alloc_init(char *base, unsigned long heap_size,
>                         unsigned long granularity, unsigned long
max_allocs);
>   extern void flush_cache(void *, unsigned long);
>  diff --git a/arch/powerpc/boot/serial.c b/arch/powerpc/boot/serial.c
>  index 7fa5078..fb5c91e 100644
>  --- a/arch/powerpc/boot/serial.c
>  +++ b/arch/powerpc/boot/serial.c
>  @@ -131,6 +131,14 @@ int serial_console_init(void)
>         else if (dt_is_compatible(devp, "xlnx,opb-uartlite-1.00.b") ||
>                   dt_is_compatible(devp, "xlnx,xps-uartlite-1.00.a"))
>                 rc =3D uartlite_console_init(devp, &serial_cd);
>  +       else if (dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.c")
||
>  +                dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.d")
||
>  +                dt_is_compatible(devp, "xlnx,opb-uart16550-1.00.e")
||
>  +                dt_is_compatible(devp, "xlnx,plb-uart16550-1.00.c")
||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-1.00.a")
||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.a")
||
>  +                dt_is_compatible(devp, "xlnx,xps-uart16550-2.00.b"))
>  +               rc =3D xilinx16550_console_init(devp, &serial_cd);

I'll review this whole patch set today, but I need to make this
comment now.  Adding to this ever increasing list of xilinx device
versions is not sustainable.  A common base compatible value needs to
be chosen instead.  Way back in the mists of time something line
"sparse16550" was suggested and I think it is a good one.  xilinx uart
nodes should claim compatibility with their specific version *and*
"sparse16550".  Then all this code should only match with
"sparse16550" and documentation should be added to
Documentation/powerpc/booting-without-of.txt to describe what
sparse16550 means.

Cheers,
g.

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

Re: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[Grant Likely]

On Thu, Mar 20, 2008 at 10:15 AM, John Linn <John.Linn@xilinx.com> wrote:
> You're right.  Rick and I were just talking about could we not check the
>  versions but just make sure the device is the right type independent of
>  the versions.
>
>  This issue will be true for all the Xilinx drivers.
>
>  I think FPGA IP is a little different in that the hardware can change
>  much more often than in hard devices.

Exactly, so all your compatible properties should look something like
one of the following:

compatible = "xlnx,xps-uart16550-2.00.b", "spares16550";
 - or -
compatible = "xlnx,xps-uart16550-2.00.b", "xlnx,opb-uart16550-1.00c",
"sparse16550";
 - or -
compatible = "xlnx,xps-uart16550-2.00.b", "xlnx,opb-uart16550-1.00c";

Notice that the exact version is *always* there.  Followed by versions
that it is exactly backwards compatible with.  As I said, I personally
like the string "sparse16550", but if other people complain about it
then pick the earliest version of the part that it is backwards
compatible with and use that version string (like in my third example
above).

The reason "xlnx,uart16550" isn't a good idea is because it isn't a
real part thing.  It is a made up idea of what the thing is, but it
isn't grounded in a particular implementation.  (Note that even
ns16550 is a real part).  The problem with 'virtual' compatible
properties is that because they aren't grounded; the temptation is to
shift their definition over time as newer parts come out that behave
differently.  It is really hard to shift the definition of something
that is exactly specified.

Side note; While I do like sparse16550, it may also not be a good idea
for the same reason that "xlnx,uart16550" is not a good idea.
However, the risk is much lower because there is already a fair amount
of precedence of 'sparse' 16550 devices in the wild and their behavior
is well known.

Cheers,
g.

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

Re: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[Grant Likely]

On Thu, Mar 20, 2008 at 11:55 AM, John Linn <John.Linn@xilinx.com> wrote:
> Hi Grant,
>
>  We could create a xilinx name, "xlnx,uart16550", and only use that
>  without the versions.
>

I'm okay with this as long as you don't drop the versioned property
also.  ie. Each compatible node should contain 2 strings.  The exact
version and the generic string.

My preference is something even more generic like "sparse16550" simply
because there is precedence for such a thing before Xilinx came along
(specifically, an 16550 device with reg shift and offset values).
Since it is a common variant and If you document exactly what it
means, then it should be okay to create a compatible property without
the 'xlnx,' prefix.

Cheers,
g.

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

Re: [PATCH 3/3] [POWERPC] Xilinx: boot support for Xilinx uart 16550.

[Grant Likely]

On Thu, Mar 20, 2008 at 8:43 AM, John Linn <john.linn@xilinx.com> wrote:
> The Xilinx 16550 uart core is not a standard 16550, because it uses
>  word-based addressing rather than byte-based addressing.  As a result,
>  it is not compatible with the open firmware 'ns16550' compatible
>  binding.
>
>  This code adds the Xilinx uart 16550 to the serial console
>  of the boot. A new initialization function for the Xilinx 16550 uart
>  is added to the ns16550 driver. It sets up the correct register base
>  and regshift properties specific to the Xilinx 16550.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>
>  ---
>   arch/powerpc/boot/ns16550.c |   24 ++++++++++++++++++++++++
>   arch/powerpc/boot/ops.h     |    1 +
>   arch/powerpc/boot/serial.c  |    8 ++++++++
>   3 files changed, 33 insertions(+), 0 deletions(-)
>
>  diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
>  index f8f1b2f..5385255 100644
>  --- a/arch/powerpc/boot/ns16550.c
>  +++ b/arch/powerpc/boot/ns16550.c
>  @@ -77,3 +77,27 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>
>         return 0;
>   }
>  +
>  +int xilinx16550_console_init(void *devp, struct serial_console_data *scdp)
>  +{
>  +       int n;
>  +       unsigned long reg_phys;
>  +
>  +       n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
>  +       if (n != sizeof(reg_base)) {
>  +               if (!dt_xlate_reg(devp, 0, &reg_phys, NULL))
>  +                       return -1;
>  +
>  +               reg_base = (void *)reg_phys + 3;
>  +       }
>  +
>  +       reg_shift = 2;
>  +
>  +       scdp->open = ns16550_open;
>  +       scdp->putc = ns16550_putc;
>  +       scdp->getc = ns16550_getc;
>  +       scdp->tstc = ns16550_tstc;
>  +       scdp->close = NULL;
>  +
>  +       return 0;

This is mostly duplicated code.  xilinx16550_console_init should
simply call ns16550_console_init and then modify reg_base/reg_shift
after it returns.

However, if something like sparse16550 is used, then the need for this
function disappears.  In fact, ns16550_console_init already supports
the reg-shift property.  It just doesn't know about the 3 byte offset.

Cheers,
g.



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