[PATCH] booting-without-of: add Xilinx uart 16550.

Add uart 16550 properties description to Xilinx portion of booting-without-of.txt

Signed-off-by: Pavel Kiryukhin <redacted>
---
 Documentation/powerpc/booting-without-of.txt |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 7b4e8a7..dd77bbc 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt

@@ -2575,10 +2575,22 @@ platforms are moved over to use the flattened-device-tree model.
 
       Xilinx uartlite devices are simple fixed speed serial ports.
 
-      Requred properties:
+      Required properties:
        - current-speed : Baud rate of uartlite
 
-      v) Xilinx hwicap
+      v) Xilinx Uart 16550
+
+      Xilinx uart 16550 device registers are compatible with all standard 16540 
+      and 16550 UARTs.
+
+      Required properties:
+       - current-speed : Baud rate of uart.
+       - clock-frequency : Baud rate generator reference clock. May be driven 
+         by OPB_Clk (100 MHz).
+       - reg-shift : registers offset shift (standard uart_port field). 
+         Property is optional if regshift is zero.
+
+      vi) Xilinx hwicap
 
 		Xilinx hwicap devices provide access to the configuration logic
 		of the FPGA through the Internal Configuration Access Port

-- 
1.5.4.1

+       - reg-shift : registers offset shift (standard uart_port

field).

+         Property is optional if regshift is zero.

I was hoping to get an idea of what is required here, or when I might
use it?

It looks like the ARCH=3Dppc code instantiates this with a reg-shift of
2...  Is this the expected value?  When would it be not zero? or not
two?

Steve

Stephen Neuendorffer wrote:

quoted

+       - reg-shift : registers offset shift (standard uart_port

field).

quoted

+         Property is optional if regshift is zero.

I was hoping to get an idea of what is required here, or when I might
use it?

It looks like the ARCH=ppc code instantiates this with a reg-shift of
2...  Is this the expected value?  

Yes, reg-shift = 2 should be set for Xilinx 16550 uart. 
Should I add this to patch?

BTW regshift=2 is hardcoded for uartlite.

When would it be not zero? or not
two?

Sorry, it seems I don't follow here. 
--
Regards,
Pavel

-----Original Message-----
From: Pavel Kiryukhin [mailto:pkiryukhin@ru.mvista.com]
Sent: Friday, February 15, 2008 9:41 AM
To: Stephen Neuendorffer
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH] booting-without-of: add Xilinx uart 16550.
=20
Stephen Neuendorffer wrote:

quoted

quoted

+       - reg-shift : registers offset shift (standard uart_port

field).

quoted

+         Property is optional if regshift is zero.

I was hoping to get an idea of what is required here, or when I

might

quoted

use it?

It looks like the ARCH=3Dppc code instantiates this with a reg-shift

of

quoted

2...  Is this the expected value?

=20
Yes, reg-shift =3D 2 should be set for Xilinx 16550 uart.
Should I add this to patch?

Yes, please!
=20

BTW regshift=3D2 is hardcoded for uartlite.
=20

quoted

When would it be not zero? or not
two?

Sorry, it seems I don't follow here.

I think all that needs to get added is 'For the Xilinx uart 16550 cores,
reg-shift must be set to 2'

Steve

On Sat Feb 16 00:40:01 EST 2008, Pavel Kiryukhin pkiryukhin wrote:

Add uart 16550 properties description to Xilinx portion of 
booting-without-of.txt

This patch description is a bit weak.  How about adding what properties 
are being added.  Also, as described below, it's going to become more 
than just adding a property.

Freqency of the reference clock generating the baud rate.  (Sometimes 
this is connected to the OBP_Clk which is usually 100MHz).   This is 
not the documentation for the macro, its the documentation for the 
device tree.

+       - reg-shift : registers offset shift (standard uart_port 
field).
+         Property is optional if regshift is zero.

As Steven said, you must define what the property means (don't say that 
the value should be 2 on chip x, describe how to determine the correct 
value and how to use it).  "When this property is present, the address 
bits selecting the different 16550 registers are shifted left this many 
bits."

Also, since this is not register compatible with previous the previous 
ns16550 binding, we need to use a new compatible string. 
ns16550-shifted or ns16550-offset would be ok with me.  When we define 
this node, refer to 16550, and state that we expect nodes that have a 
zero shfit should include the ns16550 string.

Looking in the bigger context, this binding is not Xilinx specific; but 
I don't see an obviousy better place and am not going to do more than 
mention that issue.

milton

On Fri, Feb 15, 2008 at 6:40 AM, Pavel Kiryukhin
[off-list ref] wrote:

Add uart 16550 properties description to Xilinx portion of booting-without-of.txt

 Signed-off-by: Pavel Kiryukhin [off-list ref]
 ---
  Documentation/powerpc/booting-without-of.txt |   16 ++++++++++++++--
  1 files changed, 14 insertions(+), 2 deletions(-)

 diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
 index 7b4e8a7..dd77bbc 100644
 --- a/Documentation/powerpc/booting-without-of.txt
 +++ b/Documentation/powerpc/booting-without-of.txt
 @@ -2575,10 +2575,22 @@ platforms are moved over to use the flattened-device-tree model.

       Xilinx uartlite devices are simple fixed speed serial ports.

 -      Requred properties:
 +      Required properties:
        - current-speed : Baud rate of uartlite

 -      v) Xilinx hwicap
 +      v) Xilinx Uart 16550
 +
 +      Xilinx uart 16550 device registers are compatible with all standard 16540
 +      and 16550 UARTs.

Not strictly true; the xilinx uart is *almost* compatible with the
ns16550.  The same driver can be made to work, but it is not register
level compatible so we cannot claim compatible="ns16550".  We need a
new compatible property for 16550 like devices with a reg shift and
offset.  Instead of attempting to come up with a generic description
of this, I recommend just naming it after the actual device instance;
something like compatible="xlnx,opb-uart16550";

Cheers,
g.

Grant Likely wrote:

quoted

Add uart 16550 properties description to Xilinx portion of booting-without-of.txt

quoted

Signed-off-by: Pavel Kiryukhin <redacted>
---
 Documentation/powerpc/booting-without-of.txt |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 7b4e8a7..dd77bbc 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt

@@ -2575,10 +2575,22 @@ platforms are moved over to use the flattened-device-tree model.

      Xilinx uartlite devices are simple fixed speed serial ports.

-      Requred properties:
+      Required properties:
       - current-speed : Baud rate of uartlite

-      v) Xilinx hwicap
+      v) Xilinx Uart 16550
+
+      Xilinx uart 16550 device registers are compatible with all standard 16540
+      and 16550 UARTs.

Not strictly true; the xilinx uart is *almost* compatible with the
ns16550.  The same driver can be made to work, but it is not register
level compatible so we cannot claim compatible="ns16550".

    How much incompatible it is with 16550? Does that only extend to register 
stride of 4 instead of 1 -- if so, it should be considered compatible since
the same chip can be into address space mapped with stride of 1, 2, or 4, or 
whatever power of 2. If it has some actual register difference, like e. g. 
DLAB not existing and the divisor latch mapped to a separate register rather 
than 0..1, then indeed, new compatible property must be defined.

We need a new compatible property for 16550 like devices with a reg shift and
offset.

    No, we don't strictly need it if all incompatibilty is constrained to how 
the same 16550 registers mapped to address space which is a function of the 
address decoder, not the chip itself. Well, that's of course based in 8250.c's 
ability to handle different strides -- an imaginary driver could only handle 
1:1 chip mapping.

Instead of attempting to come up with a generic description
of this, I recommend just naming it after the actual device instance;
something like compatible="xlnx,opb-uart16550";

    Well, that means that we'll need a to add a code which "glues" the chip to 
8250.c driver... well, of_serial.c could be that glue layer if we add to it 
the ability to recognize Xilinx UART... well, legacy_serial.c could be taught 
that trick too...
    Well, we could also add the new compatible, but still claim "ns16550" 
compatibility...

Cheers,
g.

WBR, Sergei

On Fri, Feb 15, 2008 at 11:56 AM, Sergei Shtylyov
[off-list ref] wrote:

Grant Likely wrote:

 >> +      Xilinx uart 16550 device registers are compatible with all standard 16540
 >> +      and 16550 UARTs.


 > Not strictly true; the xilinx uart is *almost* compatible with the
 > ns16550.  The same driver can be made to work, but it is not register
 > level compatible so we cannot claim compatible="ns16550".

    How much incompatible it is with 16550? Does that only extend to register
 stride of 4 instead of 1 -- if so, it should be considered compatible since
 the same chip can be into address space mapped with stride of 1, 2, or 4, or
 whatever power of 2. If it has some actual register difference, like e. g.
 DLAB not existing and the divisor latch mapped to a separate register rather
 than 0..1, then indeed, new compatible property must be defined.

The definition of compatible (from the OpenFirmware) docs is that the
*device* is register level compatible.   That includes register
locations and offsets.  The registers are not at the same location,
therefore it is not compatible.

However, the *driver* can be easily made compatible with the devices.
We just teach the driver to bind against both "ns16550" and whatever
value is chosen for these reg-shifted devices.  Not a big deal.

 > We need a new compatible property for 16550 like devices with a reg shift and
 > offset.

    No, we don't strictly need it if all incompatibilty is constrained to how
 the same 16550 registers mapped to address space which is a function of the
 address decoder, not the chip itself. Well, that's of course based in 8250.c's
 ability to handle different strides -- an imaginary driver could only handle
 1:1 chip mapping.

compatible also covers bus binding when it is a memory mapped device.
Otherwise you need another node between the bus and the ns16550 type
device that does translation from the wide stride (regshift=2) to the
ns16550 register definitions (regshift=0).

 > Instead of attempting to come up with a generic description
 > of this, I recommend just naming it after the actual device instance;
 > something like compatible="xlnx,opb-uart16550";

    Well, that means that we'll need a to add a code which "glues" the chip to
 8250.c driver... well, of_serial.c could be that glue layer if we add to it
 the ability to recognize Xilinx UART... well, legacy_serial.c could be taught
 that trick too...
    Well, we could also add the new compatible, but still claim "ns16550"
 compatibility...

No, we cannot because it is not register level compatible (and once
again, that definition includes the stride between registers)

Cheers,
g.

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

Grant Likely wrote:

quoted

quoted

quoted

+      Xilinx uart 16550 device registers are compatible with all standard 16540
+      and 16550 UARTs.

quoted

quoted

Not strictly true; the xilinx uart is *almost* compatible with the
ns16550.  The same driver can be made to work, but it is not register
level compatible so we cannot claim compatible="ns16550".

quoted

   How much incompatible it is with 16550? Does that only extend to register
stride of 4 instead of 1 -- if so, it should be considered compatible since
the same chip can be into address space mapped with stride of 1, 2, or 4, or
whatever power of 2. If it has some actual register difference, like e. g.
DLAB not existing and the divisor latch mapped to a separate register rather
than 0..1, then indeed, new compatible property must be defined.

The definition of compatible (from the OpenFirmware) docs is that the
*device* is register level compatible.   That includes register
locations and offsets.

    I'd disagree here: register offsets are the function of the chip select, 
not a chip itself -- you can possible wire chip selects to 16550 so that the 
registers would be spaced by 4 bytes.

The registers are not at the same location, therefore it is not compatible.

However, the *driver* can be easily made compatible with the devices.
We just teach the driver to bind against both "ns16550" and whatever
value is chosen for these reg-shifted devices.  Not a big deal.

    You're going to "teach" 8250.c? Good luck. :-)
    IMO we can only teach a glue layer which "passes" UARTs to 8250.c via 
platform devices.

quoted

quoted

We need a new compatible property for 16550 like devices with a reg shift and
offset.

quoted

   No, we don't strictly need it if all incompatibilty is constrained to how
the same 16550 registers mapped to address space which is a function of the
address decoder, not the chip itself. Well, that's of course based in 8250.c's
ability to handle different strides -- an imaginary driver could only handle
1:1 chip mapping.

compatible also covers bus binding when it is a memory mapped device.
Otherwise you need another node between the bus and the ns16550 type
device that does translation from the wide stride (regshift=2) to the
ns16550 register definitions (regshift=0).

    The chip can be connected to the bus (via chip select circuitry) in 
different ways, therefore we need a "glue" node for that circuitry?

quoted

quoted

Instead of attempting to come up with a generic description
of this, I recommend just naming it after the actual device instance;
something like compatible="xlnx,opb-uart16550";

quoted

   Well, that means that we'll need a to add a code which "glues" the chip to
8250.c driver... well, of_serial.c could be that glue layer if we add to it
the ability to recognize Xilinx UART... well, legacy_serial.c could be taught
that trick too...
   Well, we could also add the new compatible, but still claim "ns16550"
compatibility...

No, we cannot because it is not register level compatible (and once
again, that definition includes the stride between registers)

    Once again, I disagree. :-)

Cheers,
g.

WBR, Sergei

On Fri, Feb 15, 2008 at 12:14 PM, Sergei Shtylyov
[off-list ref] wrote:

Grant Likely wrote:
 > The registers are not at the same location, therefore it is not compatible.

 > However, the *driver* can be easily made compatible with the devices.
 > We just teach the driver to bind against both "ns16550" and whatever
 > value is chosen for these reg-shifted devices.  Not a big deal.

    You're going to "teach" 8250.c? Good luck. :-)

:-P

As you already know, 8250.c already knows how to handle reg shifts.
But that is not what this conversation is about.

    IMO we can only teach a glue layer which "passes" UARTs to 8250.c via
 platform devices.

That's right.  This discussion is only about the device tree binding.
It is not about the core driver.

 > compatible also covers bus binding when it is a memory mapped device.
 > Otherwise you need another node between the bus and the ns16550 type
 > device that does translation from the wide stride (regshift=2) to the
 > ns16550 register definitions (regshift=0).

    The chip can be connected to the bus (via chip select circuitry) in
 different ways, therefore we need a "glue" node for that circuitry?

I'm not arguing that we want a glue node.  What I'm arguing is that
"compatible=ns16550" has a very specific meaning that has become a
defacto standard over the years.  To add a new interpretation of that
meaning is problematic.  Instead, if reg shift is needed then use a
different value for compatible.

It's not like we're talking about something that is hard to do.  It is
simply being explicit in the device tree layout.  Historically
"ns16550" means no reg shift, so don't use it for devices that have a
reg shift.

Cheers,
g.

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

quoted

Instead of attempting to come up with a generic description
of this, I recommend just naming it after the actual device

instance;

quoted

something like compatible=3D"xlnx,opb-uart16550";

=20
    Well, that means that we'll need a to add a code which "glues" the

chip to

8250.c driver... well, of_serial.c could be that glue layer if we add

to it

the ability to recognize Xilinx UART... well, legacy_serial.c could be

taught

that trick too...
    Well, we could also add the new compatible, but still claim

"ns16550"

compatibility...

This actually makes more sense to me...  I'd rather have the code set
the reg-shift than have it explicitly set in the device tree anyway.
The compatibility set should include (at the least):

      opb_uart16550_v1_00_c
      opb_uart16550_v1_00_d
      opb_uart16550_v1_00_e
      plb_uart16550_v1_00_c
      xps_uart16550_v1_00_a

I think this is somewhat independent of Sergei's arguments that generic
ns16550 devices should allow having a reg-shift set....

Steve

Hello.

Stephen Neuendorffer wrote:

quoted

quoted

Instead of attempting to come up with a generic description
of this, I recommend just naming it after the actual device instance;

quoted

quoted

something like compatible="xlnx,opb-uart16550";

quoted

   Well, that means that we'll need a to add a code which "glues" the chip to
8250.c driver... well, of_serial.c could be that glue layer if we add to it

quoted

the ability to recognize Xilinx UART... well, legacy_serial.c could be taught

quoted

that trick too...
   Well, we could also add the new compatible, but still claim "ns16550"

quoted

compatibility...

This actually makes more sense to me...  I'd rather have the code set
the reg-shift than have it explicitly set in the device tree anyway.
The compatibility set should include (at the least):

      opb_uart16550_v1_00_c
      opb_uart16550_v1_00_d
      opb_uart16550_v1_00_e
      plb_uart16550_v1_00_c
      xps_uart16550_v1_00_a

    Sounds like too much? Couldn't this be handled via the "model" prop?

I think this is somewhat independent of Sergei's arguments that generic
ns16550 devices should allow having a reg-shift set....

Steve

WBR, Sergei