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

[PATCH 3/3][POWERPC][V2] 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 adressing. With
additional properties it is compatible with the open firmware
'ns16550' compatible binding.

This code updates the ns16550 driver to use the reg-offset property
so that the Xilinx UART 16550 can be used with it. The reg-shift
was already being handled.

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

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index f8f1b2f..da9d2c2 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
 {
 	int n;
 	unsigned long reg_phys;
+	u32 reg_offset;
 
 	n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
 	if (n != sizeof(reg_base)) {
@@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
 		reg_base = (void *)reg_phys;
 	}
 
+	n = getprop(devp, "reg-offset", &reg_offset, sizeof(reg_offset));
+	if (n == sizeof(reg_offset))
+		reg_base += reg_offset;
+
 	n = getprop(devp, "reg-shift", &reg_shift, sizeof(reg_shift));
 	if (n != sizeof(reg_shift))
 		reg_shift = 0;
-- 
1.5.2.1

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

[Grant Likely]

On Wed, Apr 2, 2008 at 10:52 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 adressing. With
>  additional properties it is compatible with the open firmware
>  'ns16550' compatible binding.
>
>  This code updates the ns16550 driver to use the reg-offset property
>  so that the Xilinx UART 16550 can be used with it. The reg-shift
>  was already being handled.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

>  ---
>   arch/powerpc/boot/ns16550.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
>  diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
>  index f8f1b2f..da9d2c2 100644
>  --- a/arch/powerpc/boot/ns16550.c
>  +++ b/arch/powerpc/boot/ns16550.c
>  @@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>   {
>         int n;
>         unsigned long reg_phys;
>  +       u32 reg_offset;
>
>         n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
>         if (n != sizeof(reg_base)) {
>  @@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>                 reg_base = (void *)reg_phys;
>         }
>
>  +       n = getprop(devp, "reg-offset", &reg_offset, sizeof(reg_offset));
>  +       if (n == sizeof(reg_offset))
>  +               reg_base += reg_offset;
>  +
>         n = getprop(devp, "reg-shift", &reg_shift, sizeof(reg_shift));
>         if (n != sizeof(reg_shift))
>                 reg_shift = 0;
>  --
>  1.5.2.1
>
>
>
>



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

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

[David Gibson]

On Wed, Apr 02, 2008 at 09:52:14AM -0700, John Linn wrote:
> The Xilinx 16550 uart core is not a standard 16550 because it uses
> word-based addressing rather than byte-based adressing. With
> additional properties it is compatible with the open firmware
> 'ns16550' compatible binding.
> 
> This code updates the ns16550 driver to use the reg-offset property
> so that the Xilinx UART 16550 can be used with it. The reg-shift
> was already being handled.
> 
> Signed-off-by: John Linn <john.linn@xilinx.com>
> ---
>  arch/powerpc/boot/ns16550.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
> index f8f1b2f..da9d2c2 100644
> --- a/arch/powerpc/boot/ns16550.c
> +++ b/arch/powerpc/boot/ns16550.c
> @@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>  {
>  	int n;
>  	unsigned long reg_phys;
> +	u32 reg_offset;
>  
>  	n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
>  	if (n != sizeof(reg_base)) {
> @@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>  		reg_base = (void *)reg_phys;
>  	}
>  
> +	n = getprop(devp, "reg-offset", &reg_offset, sizeof(reg_offset));
> +	if (n == sizeof(reg_offset))
> +		reg_base += reg_offset;

Uh... how does the behaviour of reg-offset differ from just bumping
the address in "reg"?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Grant Likely]

On Wed, Apr 2, 2008 at 6:02 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Wed, Apr 02, 2008 at 09:52:14AM -0700, John Linn wrote:
>  > The Xilinx 16550 uart core is not a standard 16550 because it uses
>  > word-based addressing rather than byte-based adressing. With
>  > additional properties it is compatible with the open firmware
>  > 'ns16550' compatible binding.
>  >
>  > This code updates the ns16550 driver to use the reg-offset property
>  > so that the Xilinx UART 16550 can be used with it. The reg-shift
>  > was already being handled.
>  >
>  > Signed-off-by: John Linn <john.linn@xilinx.com>
>  > ---
>  >  arch/powerpc/boot/ns16550.c |    5 +++++
>  >  1 files changed, 5 insertions(+), 0 deletions(-)
>  >
>  > diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
>  > index f8f1b2f..da9d2c2 100644
>  > --- a/arch/powerpc/boot/ns16550.c
>  > +++ b/arch/powerpc/boot/ns16550.c
>  > @@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>  >  {
>  >       int n;
>  >       unsigned long reg_phys;
>  > +     u32 reg_offset;
>  >
>  >       n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
>  >       if (n != sizeof(reg_base)) {
>  > @@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp)
>  >               reg_base = (void *)reg_phys;
>  >       }
>  >
>  > +     n = getprop(devp, "reg-offset", &reg_offset, sizeof(reg_offset));
>  > +     if (n == sizeof(reg_offset))
>  > +             reg_base += reg_offset;
>
>  Uh... how does the behaviour of reg-offset differ from just bumping
>  the address in "reg"?

Mostly because the registers are actually 32 bit registers that can be
accessed with 32bit reads at offset 0.  Using this property keeps the
reg property describing the real address range.

Cheers,
g.

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

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

[Johann Baudy]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

Hi John,

I've a small question about ns16550 boot support.
I can't see any code related to either "clock-frequency" or "current-speed"
key words.
Such actions are performed in of_serial.c to get appropriate baud rate
(update of UART_DLL and UART_DLM).

Is it needed or is it a misunderstanding?
Personally, I have added this below debug code (dirty :)) to make it work.

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index f8f1b2f..850e223 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -26,6 +26,9 @@
 #define UART_MSR    6    /* In:  Modem Status Register */
 #define UART_SCR    7    /* I/O: Scratch Register */

+#define UART_LCR_DLAB 0x80      /* Divisor latch access */
+#define UART_LCR_8_DATA_BITS 0x03
+
 static unsigned char *reg_base;
 static u32 reg_shift;

@@ -56,6 +59,10 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
 {
     int n;
     unsigned long reg_phys;
+  unsigned long divisor=0;
+  unsigned long spd=0, clk=0;
+
+

     n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
     if (n != sizeof(reg_base)) {
@@ -75,5 +82,23 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
     scdp->tstc = ns16550_tstc;
     scdp->close = NULL;

+    n = getprop(devp, "current-speed", &spd, sizeof(spd));
+
+
+    n = getprop(devp, "clock-frequency", &clk, sizeof(clk));
+
+
+  if(spd&&clk)
+  {
+
+     divisor = clk / (spd * 16UL);
+
+     out_8(reg_base + (UART_FCR << reg_shift), 0x06);
+     out_8(reg_base + (UART_LCR << reg_shift),
UART_LCR_8_DATA_BITS|UART_LCR_DLAB);
+     out_8(reg_base + (UART_DLL << reg_shift), divisor&0xff);
+     out_8(reg_base + (UART_DLM << reg_shift), (divisor>>8)&0xff);
+     out_8(reg_base + (UART_LCR << reg_shift), UART_LCR_8_DATA_BITS);
+
+  }
     return 0;

Best regards,
Johann

On Wed, Apr 2, 2008 at 4:52 PM, 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 adressing. With
> additional properties it is compatible with the open firmware
> 'ns16550' compatible binding.
>
> This code updates the ns16550 driver to use the reg-offset property
> so that the Xilinx UART 16550 can be used with it. The reg-shift
> was already being handled.
>
> Signed-off-by: John Linn <john.linn@xilinx.com>
> ---
>  arch/powerpc/boot/ns16550.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
> index f8f1b2f..da9d2c2 100644
> --- a/arch/powerpc/boot/ns16550.c
> +++ b/arch/powerpc/boot/ns16550.c
> @@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct
> serial_console_data *scdp)
>  {
>        int n;
>        unsigned long reg_phys;
> +       u32 reg_offset;
>
>        n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
>        if (n != sizeof(reg_base)) {
> @@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct
> serial_console_data *scdp)
>                reg_base = (void *)reg_phys;
>        }
>
> +       n = getprop(devp, "reg-offset", &reg_offset, sizeof(reg_offset));
> +       if (n == sizeof(reg_offset))
> +               reg_base += reg_offset;
> +
>        n = getprop(devp, "reg-shift", &reg_shift, sizeof(reg_shift));
>        if (n != sizeof(reg_shift))
>                reg_shift = 0;
> --
> 1.5.2.1
>
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



-- 
Johann Baudy
johaahn@gmail.com

[-- Attachment #2: Type: text/html, Size: 5376 bytes --]

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

[John Linn]

[-- Attachment #1: Type: text/plain, Size: 4539 bytes --]

Hi Johann,

 

You're right about it being missing. I have another patch for virtex
specific initialization that handles that.  I have not submitted it yet
as I was trying to get these patches thru the system.

 

I have been told in this forum that the bootstrap loader should not be
setting up the baud rate and that normally the boot loader does it.

 

In the case of FPGAs, we don't always use a boot loader so we need this
to happen in the bootstrap loader.  I can forward that patch to you if
you're interested before it goes to this group.

 

Thanks,

John

 

________________________________

From: Johann Baudy [mailto:johaahn@gmail.com] 
Sent: Thursday, April 03, 2008 7:23 AM
To: John Linn
Cc: linuxppc-dev@ozlabs.org; grant.likely@secretlab.ca
Subject: Re: [PATCH 3/3][POWERPC][V2] Xilinx: boot support for Xilinx
uart 16550.

 

Hi John,

I've a small question about ns16550 boot support.
I can't see any code related to either "clock-frequency" or
"current-speed" key words.
Such actions are performed in of_serial.c to get appropriate baud rate
(update of UART_DLL and UART_DLM).

Is it needed or is it a misunderstanding? 
Personally, I have added this below debug code (dirty :)) to make it
work.

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index f8f1b2f..850e223 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -26,6 +26,9 @@
 #define UART_MSR    6    /* In:  Modem Status Register */
 #define UART_SCR    7    /* I/O: Scratch Register */
 
+#define UART_LCR_DLAB 0x80      /* Divisor latch access */
+#define UART_LCR_8_DATA_BITS 0x03
+
 static unsigned char *reg_base;
 static u32 reg_shift;
 
@@ -56,6 +59,10 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
 {
     int n;
     unsigned long reg_phys;
+  unsigned long divisor=0;
+  unsigned long spd=0, clk=0;
+
+
 
     n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
     if (n != sizeof(reg_base)) {
@@ -75,5 +82,23 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
     scdp->tstc = ns16550_tstc;
     scdp->close = NULL;
 
+    n = getprop(devp, "current-speed", &spd, sizeof(spd));
+
+
+    n = getprop(devp, "clock-frequency", &clk, sizeof(clk));
+
+
+  if(spd&&clk)
+  {
+
+     divisor = clk / (spd * 16UL);
+
+     out_8(reg_base + (UART_FCR << reg_shift), 0x06);
+     out_8(reg_base + (UART_LCR << reg_shift),
UART_LCR_8_DATA_BITS|UART_LCR_DLAB);
+     out_8(reg_base + (UART_DLL << reg_shift), divisor&0xff);
+     out_8(reg_base + (UART_DLM << reg_shift), (divisor>>8)&0xff);
+     out_8(reg_base + (UART_LCR << reg_shift), UART_LCR_8_DATA_BITS);
+
+  }
     return 0;
 
Best regards,
Johann

On Wed, Apr 2, 2008 at 4:52 PM, 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 adressing. With
additional properties it is compatible with the open firmware
'ns16550' compatible binding.

This code updates the ns16550 driver to use the reg-offset property
so that the Xilinx UART 16550 can be used with it. The reg-shift
was already being handled.

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

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index f8f1b2f..da9d2c2 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -56,6 +56,7 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
 {
       int n;
       unsigned long reg_phys;
+       u32 reg_offset;

       n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
       if (n != sizeof(reg_base)) {
@@ -65,6 +66,10 @@ int ns16550_console_init(void *devp, struct
serial_console_data *scdp)
               reg_base = (void *)reg_phys;
       }

+       n = getprop(devp, "reg-offset", &reg_offset,
sizeof(reg_offset));
+       if (n == sizeof(reg_offset))
+               reg_base += reg_offset;
+
       n = getprop(devp, "reg-shift", &reg_shift, sizeof(reg_shift));
       if (n != sizeof(reg_shift))
               reg_shift = 0;
--
1.5.2.1



_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev




-- 
Johann Baudy
johaahn@gmail.com 


[-- Attachment #2: Type: text/html, Size: 15859 bytes --]

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

[Grant Likely]

On Thu, Apr 3, 2008 at 7:28 AM, John Linn <John.Linn@xilinx.com> wrote:
> Hi Johann,
>
> You're right about it being missing. I have another patch for virtex
> specific initialization that handles that.  I have not submitted it yet as I
> was trying to get these patches thru the system.
>
> I have been told in this forum that the bootstrap loader should not be
> setting up the baud rate and that normally the boot loader does it.
>
> In the case of FPGAs, we don't always use a boot loader so we need this to
> happen in the bootstrap loader.  I can forward that patch to you if you're
> interested before it goes to this group.
>

Yes, the reason for not fiddling with the clock rate divisor at this
point is that it increases the chance of getting at least *something*
out the serial port if the boot goes bad.  For example, if the device
tree has the clock rate listed incorrectly and the wrapper sets the
serial port baud rate then the serial port will become unusable for
bootwrapper debug.

So, in the FPGA case or other no-firmware situations there needs to be
a place to do this.  Thinking on it further, I suppose it really does
belong in the serial driver, but it needs to be protected so that only
board ports that explicitly request it will do baud rate setup.
Perhaps ns16550_console_init() should call a __weak function that can
be overridden by a board port with a version that returns 1 instead of
0.  (off the top of my head; there may be better approaches).

Cheers,
g.

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

[PATCH 2/3][POWERPC][V2] 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. With
additional properties it is compatible with the open firmware
'ns16550' compatible binding.

This code updates the of_serial driver to handle the reg-offset
and reg-shift properties to enable this core to be used.

Signed-off-by: John Linn <john.linn@xilinx.com>
---
 Documentation/powerpc/booting-without-of.txt |   11 +++++++++++
 drivers/serial/of_serial.c                   |   15 +++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 87f4d84..af112d9 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -2539,6 +2539,17 @@ platforms are moved over to use the flattened-device-tree model.
                       differ between different families.  May be
                       'virtex2p', 'virtex4', or 'virtex5'.
 
+      iv) Xilinx Uart 16550
+
+      Xilinx UART 16550 devices are very similar to the NS16550 such that they
+      use the ns16550 binding with properties to specify register spacing and
+      an offset from the base address.
+
+      Requred properties:
+       - clock-frequency : Frequency of the clock input
+       - reg-offset : A value of 3 is required
+       - reg-shift : A value of 2 is required
+
    More devices will be defined as this spec matures.
 
 VII - Specifying interrupt information for devices
diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 2efb892..af9ed48 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -30,7 +30,7 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 {
 	struct resource resource;
 	struct device_node *np = ofdev->node;
-	const unsigned int *clk, *spd;
+	const unsigned int *clk, *spd, *reg_offset, *reg_shift;
 	int ret;
 
 	memset(port, 0, sizeof *port);
@@ -48,7 +48,18 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 	}
 
 	spin_lock_init(&port->lock);
-	port->mapbase = resource.start;
+
+	reg_offset = of_get_property(np, "reg-offset", NULL);
+	reg_shift = of_get_property(np, "reg-shift", NULL);
+
+	if (!reg_offset) 
+		port->mapbase = resource.start;
+	else 		
+		port->mapbase = resource.start + *reg_offset;	
+
+	if (reg_shift)
+		port->regshift = *reg_shift;
+
 	port->irq = irq_of_parse_and_map(np, 0);
 	port->iotype = UPIO_MEM;
 	port->type = type;
-- 
1.5.2.1

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

[Grant Likely]

On Wed, Apr 2, 2008 at 10:52 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. With
>  additional properties it is compatible with the open firmware
>  'ns16550' compatible binding.
>
>  This code updates the of_serial driver to handle the reg-offset
>  and reg-shift properties to enable this core to be used.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>

Comments below...

>  ---
>   Documentation/powerpc/booting-without-of.txt |   11 +++++++++++
>   drivers/serial/of_serial.c                   |   15 +++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
>  diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
>  index 87f4d84..af112d9 100644
>  --- a/Documentation/powerpc/booting-without-of.txt
>  +++ b/Documentation/powerpc/booting-without-of.txt
>  @@ -2539,6 +2539,17 @@ platforms are moved over to use the flattened-device-tree model.
>                        differ between different families.  May be
>                        'virtex2p', 'virtex4', or 'virtex5'.
>
>  +      iv) Xilinx Uart 16550
>  +
>  +      Xilinx UART 16550 devices are very similar to the NS16550 such that they
>  +      use the ns16550 binding with properties to specify register spacing and
>  +      an offset from the base address.
>  +
>  +      Requred properties:
>  +       - clock-frequency : Frequency of the clock input
>  +       - reg-offset : A value of 3 is required
>  +       - reg-shift : A value of 2 is required
>  +
>     More devices will be defined as this spec matures.
>
>   VII - Specifying interrupt information for devices
>  diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
>  index 2efb892..af9ed48 100644
>  --- a/drivers/serial/of_serial.c
>  +++ b/drivers/serial/of_serial.c
>  @@ -30,7 +30,7 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>   {
>         struct resource resource;
>         struct device_node *np = ofdev->node;
>  -       const unsigned int *clk, *spd;
>  +       const unsigned int *clk, *spd, *reg_offset, *reg_shift;

These should really be u32's I believe; on 64 bit architectures this
will misbehave (not an immediate practical problem, but it's best to
be explicit about these things).

>         int ret;
>
>         memset(port, 0, sizeof *port);
>  @@ -48,7 +48,18 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>         }
>
>         spin_lock_init(&port->lock);
>  -       port->mapbase = resource.start;
>  +
>  +       reg_offset = of_get_property(np, "reg-offset", NULL);
>  +       reg_shift = of_get_property(np, "reg-shift", NULL);
>  +
>  +       if (!reg_offset)
>  +               port->mapbase = resource.start;
>  +       else
>  +               port->mapbase = resource.start + *reg_offset;
>  +
>  +       if (reg_shift)
>  +               port->regshift = *reg_shift;
>  +

This is a little unsafe since it doesn't check the property size, I'd
do the following instead:

port->mapbase = resource.start

/* Check for shifted address mapping */
prop = of_get_property(np, "reg-offset", &prop_size);
if (prop && (prop_size == sizeof(u32))
        port->mapbase += *prop;

/* Check for registers offset within the devices address range */
prop = of_get_property(np, "reg-shift", &prop_size);
if (prop && (prop_size == sizeof(u32)))
        port->regshift = *prop;

Cheers,
g.


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

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

[John Linn]

Sounds good, those are easy changes and make sense.

Since I'm a newbie, I don't know any better sometimes when I copy other
code that may not be as safe. =20

The same thing, of_get_property(np, "current-speed", NULL);, is done
right above my code I added. =20

Should the other code in the driver using the same method be fixed, or
just my patch?

Thanks for your patience,
John

-----Original Message-----
From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of
Grant Likely
Sent: Wednesday, April 02, 2008 12:00 PM
To: John Linn
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 2/3][POWERPC][V2] Xilinx: of_serial support for
Xilinx uart 16550.

On Wed, Apr 2, 2008 at 10:52 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. With
>  additional properties it is compatible with the open firmware
>  'ns16550' compatible binding.
>
>  This code updates the of_serial driver to handle the reg-offset
>  and reg-shift properties to enable this core to be used.
>
>  Signed-off-by: John Linn <john.linn@xilinx.com>

Comments below...

>  ---
>   Documentation/powerpc/booting-without-of.txt |   11 +++++++++++
>   drivers/serial/of_serial.c                   |   15 +++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
>  diff --git a/Documentation/powerpc/booting-without-of.txt
b/Documentation/powerpc/booting-without-of.txt
>  index 87f4d84..af112d9 100644
>  --- a/Documentation/powerpc/booting-without-of.txt
>  +++ b/Documentation/powerpc/booting-without-of.txt
>  @@ -2539,6 +2539,17 @@ platforms are moved over to use the
flattened-device-tree model.
>                        differ between different families.  May be
>                        'virtex2p', 'virtex4', or 'virtex5'.
>
>  +      iv) Xilinx Uart 16550
>  +
>  +      Xilinx UART 16550 devices are very similar to the NS16550 such
that they
>  +      use the ns16550 binding with properties to specify register
spacing and
>  +      an offset from the base address.
>  +
>  +      Requred properties:
>  +       - clock-frequency : Frequency of the clock input
>  +       - reg-offset : A value of 3 is required
>  +       - reg-shift : A value of 2 is required
>  +
>     More devices will be defined as this spec matures.
>
>   VII - Specifying interrupt information for devices
>  diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
>  index 2efb892..af9ed48 100644
>  --- a/drivers/serial/of_serial.c
>  +++ b/drivers/serial/of_serial.c
>  @@ -30,7 +30,7 @@ static int __devinit
of_platform_serial_setup(struct of_device *ofdev,
>   {
>         struct resource resource;
>         struct device_node *np =3D ofdev->node;
>  -       const unsigned int *clk, *spd;
>  +       const unsigned int *clk, *spd, *reg_offset, *reg_shift;

These should really be u32's I believe; on 64 bit architectures this
will misbehave (not an immediate practical problem, but it's best to
be explicit about these things).

>         int ret;
>
>         memset(port, 0, sizeof *port);
>  @@ -48,7 +48,18 @@ static int __devinit
of_platform_serial_setup(struct of_device *ofdev,
>         }
>
>         spin_lock_init(&port->lock);
>  -       port->mapbase =3D resource.start;
>  +
>  +       reg_offset =3D of_get_property(np, "reg-offset", NULL);
>  +       reg_shift =3D of_get_property(np, "reg-shift", NULL);
>  +
>  +       if (!reg_offset)
>  +               port->mapbase =3D resource.start;
>  +       else
>  +               port->mapbase =3D resource.start + *reg_offset;
>  +
>  +       if (reg_shift)
>  +               port->regshift =3D *reg_shift;
>  +

This is a little unsafe since it doesn't check the property size, I'd
do the following instead:

port->mapbase =3D resource.start

/* Check for shifted address mapping */
prop =3D of_get_property(np, "reg-offset", &prop_size);
if (prop && (prop_size =3D=3D sizeof(u32))
        port->mapbase +=3D *prop;

/* Check for registers offset within the devices address range */
prop =3D of_get_property(np, "reg-shift", &prop_size);
if (prop && (prop_size =3D=3D sizeof(u32)))
        port->regshift =3D *prop;

Cheers,
g.


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

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

[Grant Likely]

On Wed, Apr 2, 2008 at 12:20 PM, John Linn <John.Linn@xilinx.com> wrote:
> Sounds good, those are easy changes and make sense.
>
>  Since I'm a newbie, I don't know any better sometimes when I copy other
>  code that may not be as safe.
>
>  The same thing, of_get_property(np, "current-speed", NULL);, is done
>  right above my code I added.
>
>  Should the other code in the driver using the same method be fixed, or
>  just my patch?

It would be good to fix the other code, but not in this patch.  Write
another patch to fix that.

Cheers,
g.

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

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

[Sergei Shtylyov]

Hello.

John Linn wrote:

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

> This code updates the of_serial driver to handle the reg-offset
> and reg-shift properties to enable this core to be used.

> Signed-off-by: John Linn <john.linn@xilinx.com>

> diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> index 87f4d84..af112d9 100644
> --- a/Documentation/powerpc/booting-without-of.txt
> +++ b/Documentation/powerpc/booting-without-of.txt
> @@ -2539,6 +2539,17 @@ platforms are moved over to use the flattened-device-tree model.
>                        differ between different families.  May be
>                        'virtex2p', 'virtex4', or 'virtex5'.
>  
> +      iv) Xilinx Uart 16550
> +
> +      Xilinx UART 16550 devices are very similar to the NS16550 such that they
> +      use the ns16550 binding with properties to specify register spacing and
> +      an offset from the base address.
> +
> +      Requred properties:
> +       - clock-frequency : Frequency of the clock input
> +       - reg-offset : A value of 3 is required

    I'm proposing you to use the already existing "big-endian" property ISO 
"reg-offset" (used in the  nodes describing OpenPIC, for example).

WBR, Sergei

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

[Stephen Neuendorffer]

I don't think big-endian has the same context as reg-shift/reg-offset.
The OpenPOC is fundamentally a 32 bit device, but ns16550 is not...  If
we were talking about a 32 bit device, then I'd probably agree with you,
but in this case, the reg-shift (and to some extent) reg-offset have
been used before and probably make more sense, I think.

Steve

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]
> Sent: Wednesday, April 02, 2008 1:20 PM
> To: John Linn
> Cc: linuxppc-dev@ozlabs.org; grant.likely@secretlab.ca
> Subject: Re: [PATCH 2/3][POWERPC][V2] Xilinx: of_serial support for
Xilinx uart 16550.
>=20
> Hello.
>=20
> John Linn wrote:
>=20
> > The Xilinx 16550 uart core is not a standard 16550 because it uses
> > word-based addressing rather than byte-based addressing. With
> > additional properties it is compatible with the open firmware
> > 'ns16550' compatible binding.
>=20
> > This code updates the of_serial driver to handle the reg-offset
> > and reg-shift properties to enable this core to be used.
>=20
> > Signed-off-by: John Linn <john.linn@xilinx.com>
>=20
> > diff --git a/Documentation/powerpc/booting-without-of.txt
b/Documentation/powerpc/booting-without-
> of.txt
> > index 87f4d84..af112d9 100644
> > --- a/Documentation/powerpc/booting-without-of.txt
> > +++ b/Documentation/powerpc/booting-without-of.txt
> > @@ -2539,6 +2539,17 @@ platforms are moved over to use the
flattened-device-tree model.
> >                        differ between different families.  May be
> >                        'virtex2p', 'virtex4', or 'virtex5'.
> >
> > +      iv) Xilinx Uart 16550
> > +
> > +      Xilinx UART 16550 devices are very similar to the NS16550
such that they
> > +      use the ns16550 binding with properties to specify register
spacing and
> > +      an offset from the base address.
> > +
> > +      Requred properties:
> > +       - clock-frequency : Frequency of the clock input
> > +       - reg-offset : A value of 3 is required
>=20
>     I'm proposing you to use the already existing "big-endian"
property ISO
> "reg-offset" (used in the  nodes describing OpenPIC, for example).
>=20
> WBR, Sergei

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

[Sergei Shtylyov]

Hello.

Stephen Neuendorffer wrote:

> I don't think big-endian has the same context as reg-shift/reg-offset.

    The "big-endian" is about how the byte addresses are laid out, so the 
context is the same -- in this case, it would determine where each UART 
register is located within the address stride specified by "reg-shift".
It'll alwaay be at offset 0 or (2 << reg-shift) - 1 (unless some vendor goes 
and implements something with "middle-endian" layout of course :-)

> The OpenPOC is fundamentally a 32 bit device, but ns16550 is not...  If

    So what?

> we were talking about a 32 bit device, then I'd probably agree with you,

    There are 16550 clones that *are* 32-bit.

> but in this case, the reg-shift

    I'm not arguing about "reg-shift" already -- look like it's been spec'ed. :-)

> (and to some extent) reg-offset have
> been used before and probably make more sense, I think.

    The "reg-offset" has been used before? Where?

> Steve

WBR, Sergei