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

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

[Pavel Kiryukhin]

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

Signed-off-by: Pavel Kiryukhin <pkiryukhin@ru.mvista.com>
---
 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

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

[Stephen Neuendorffer]

> +       - 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

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

[Pavel Kiryukhin]

Stephen Neuendorffer wrote:
>> +       - 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=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

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

[Stephen Neuendorffer]

> -----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:
> >> +       - 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?
>=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
> > 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

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

[Milton Miller]

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.

> Signed-off-by: Pavel Kiryukhin <pkiryukhin at ru.mvista.com>
> ---
>  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).

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

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

[Grant Likely]

On Fri, Feb 15, 2008 at 6:40 AM, Pavel Kiryukhin
<pkiryukhin@ru.mvista.com> wrote:
> Add uart 16550 properties description to Xilinx portion of booting-without-of.txt
>
>  Signed-off-by: Pavel Kiryukhin <pkiryukhin@ru.mvista.com>
>  ---
>   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.

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

[Sergei Shtylyov]

Grant Likely wrote:

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

>> Signed-off-by: Pavel Kiryukhin <pkiryukhin@ru.mvista.com>
>> ---
>>  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

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

[Grant Likely]

On Fri, Feb 15, 2008 at 11:56 AM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> 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.

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

[Sergei Shtylyov]

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.

    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.

>> > 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).

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

>> > 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)

    Once again, I disagree. :-)

> Cheers,
> g.

WBR, Sergei

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

[Grant Likely]

On Fri, Feb 15, 2008 at 12:14 PM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> 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.

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

[Stephen Neuendorffer]

> > 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=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

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

[Sergei Shtylyov]

Hello.

Stephen Neuendorffer wrote:

>>>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...

> 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