[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Dave Aldridge]

If CONFIG_SYS_NS16550_MEM32 is defined then 32 bit memory
mapped access will be used to read/write the uart registers.

This is especially useful for SoC devices that implement 16550
compatible uarts but that have peripheral access width constraints.

Signed-off-by: Dave Aldridge <fovsoft@gmail.com>
---
Changes for v2:
    - Add endian support

Changes for v3:
    - Use in_be32()/out_be32() and in_le32/out_le32() functions
      to provide endian support

 drivers/serial/ns16550.c |    6 ++++++
 include/ns16550.h        |    2 ++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 8eeb48f..0174744 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -19,6 +19,12 @@
 #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
 #define serial_out(x,y)	outb(x,(ulong)y)
 #define serial_in(y)	inb((ulong)y)
+#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
+#define serial_out(x,y) out_be32(y,x)
+#define serial_in(y) 	in_be32(y)
+#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
+#define serial_out(x,y) out_le32(y,x)
+#define serial_in(y) 	in_le32(y)
 #else
 #define serial_out(x,y) writeb(x,y)
 #define serial_in(y) 	readb(y)
diff --git a/include/ns16550.h b/include/ns16550.h
index 9ea81e9..d4ffac9 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -23,6 +23,8 @@
 
 #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
 #error "Please define NS16550 registers size."
+#elif defined(CONFIG_SYS_NS16550_MEM32)
+#define UART_REG(x) unsigned int x;
 #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
 #define UART_REG(x)						   \
 	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
-- 
1.7.3.4

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Tabi Timur-B04825]

On Thu, Sep 1, 2011 at 6:40 AM, Dave Aldridge <fovsoft@gmail.com> wrote:

> ?#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
> ?#error "Please define NS16550 registers size."
> +#elif defined(CONFIG_SYS_NS16550_MEM32)
> +#define UART_REG(x) unsigned int x;

Shouldn't this be

#define UART_REG(x) u32 x

For one thing, I think you want a sized integer.  Secondly, does the
semicolon at the end belong there?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Dave Aldridge]

Hi Tabi Timur-B04825

On 01/09/11 16:39, Tabi Timur-B04825 wrote:
> On Thu, Sep 1, 2011 at 6:40 AM, Dave Aldridge <fovsoft@gmail.com> wrote:
> 
>>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>>  #error "Please define NS16550 registers size."
>> +#elif defined(CONFIG_SYS_NS16550_MEM32)
>> +#define UART_REG(x) unsigned int x;
> 
> Shouldn't this be
> 
> #define UART_REG(x) u32 x
> 
> For one thing, I think you want a sized integer.  Secondly, does the
> semicolon at the end belong there?
> 

You are correct I have made an assumption about the size on a int. I will fix
this to remove any doubt.

Thanks for the comments

Cheers

Dave

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Wolfgang Denk]

Dear Dave Aldridge,

In message <1314877212-31552-1-git-send-email-fovsoft@gmail.com> you wrote:
> If CONFIG_SYS_NS16550_MEM32 is defined then 32 bit memory
> mapped access will be used to read/write the uart registers.
> 
> This is especially useful for SoC devices that implement 16550
> compatible uarts but that have peripheral access width constraints.
> 
> Signed-off-by: Dave Aldridge <fovsoft@gmail.com>
...

> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -19,6 +19,12 @@
>  #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>  #define serial_out(x,y)	outb(x,(ulong)y)
>  #define serial_in(y)	inb((ulong)y)
> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
> +#define serial_out(x,y) out_be32(y,x)
> +#define serial_in(y) 	in_be32(y)
> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
> +#define serial_out(x,y) out_le32(y,x)
> +#define serial_in(y) 	in_le32(y)

Sorry for the dumb question, but in which way is REG_SIZE > 0 or
REG_SIZE < 0 connected to the endianess of the target system?

My understanding is that this only defines how byte wide registers
need to be padded, i. e. wether they are connected to the highest or
to the lowest byte lane.  THis has nothing to do with the endianess of
the system, and it appears wrong to me, to imply such a relation here.

Detlev, what do you think?

>  #else
>  #define serial_out(x,y) writeb(x,y)
>  #define serial_in(y) 	readb(y)
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 9ea81e9..d4ffac9 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -23,6 +23,8 @@
>  
>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>  #error "Please define NS16550 registers size."
> +#elif defined(CONFIG_SYS_NS16550_MEM32)
> +#define UART_REG(x) unsigned int x;

I think you should rather use an explicit 32 bit data type here;
"int" may be 64 bits on some systems.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I like your game but we have to change the rules."

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Dave Aldridge]

Hi Wolfgang

On 07/09/11 22:22, Wolfgang Denk wrote:
> Dear Dave Aldridge,
> 
> In message <1314877212-31552-1-git-send-email-fovsoft@gmail.com> you wrote:
>> If CONFIG_SYS_NS16550_MEM32 is defined then 32 bit memory
>> mapped access will be used to read/write the uart registers.
>>
>> This is especially useful for SoC devices that implement 16550
>> compatible uarts but that have peripheral access width constraints.
>>
>> Signed-off-by: Dave Aldridge <fovsoft@gmail.com>
> ...
> 
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -19,6 +19,12 @@
>>  #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>  #define serial_out(x,y)	outb(x,(ulong)y)
>>  #define serial_in(y)	inb((ulong)y)
>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
>> +#define serial_out(x,y) out_be32(y,x)
>> +#define serial_in(y) 	in_be32(y)
>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
>> +#define serial_out(x,y) out_le32(y,x)
>> +#define serial_in(y) 	in_le32(y)
> 
> Sorry for the dumb question, but in which way is REG_SIZE > 0 or
> REG_SIZE < 0 connected to the endianess of the target system?
> 
> My understanding is that this only defines how byte wide registers
> need to be padded, i. e. wether they are connected to the highest or
> to the lowest byte lane.  THis has nothing to do with the endianess of
> the system, and it appears wrong to me, to imply such a relation here.
> 
> Detlev, what do you think?
> 

No such thing as a dumb question. It is my understanding from the way that the
REG_SIZE macro is currently being used in the driver that the endianess of the
uart can be inferred in this way.

>>  #else
>>  #define serial_out(x,y) writeb(x,y)
>>  #define serial_in(y) 	readb(y)
>> diff --git a/include/ns16550.h b/include/ns16550.h
>> index 9ea81e9..d4ffac9 100644
>> --- a/include/ns16550.h
>> +++ b/include/ns16550.h
>> @@ -23,6 +23,8 @@
>>  
>>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>>  #error "Please define NS16550 registers size."
>> +#elif defined(CONFIG_SYS_NS16550_MEM32)
>> +#define UART_REG(x) unsigned int x;
> 
> I think you should rather use an explicit 32 bit data type here;
> "int" may be 64 bits on some systems.
> 
> Best regards,
> 
> Wolfgang Denk
> 

Cheers

Dave

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Detlev Zundel]

Hi,

> Dear Dave Aldridge,
>
> In message <1314877212-31552-1-git-send-email-fovsoft@gmail.com> you wrote:
>> If CONFIG_SYS_NS16550_MEM32 is defined then 32 bit memory
>> mapped access will be used to read/write the uart registers.
>> 
>> This is especially useful for SoC devices that implement 16550
>> compatible uarts but that have peripheral access width constraints.
>> 
>> Signed-off-by: Dave Aldridge <fovsoft@gmail.com>
> ...
>
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -19,6 +19,12 @@
>>  #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>  #define serial_out(x,y)	outb(x,(ulong)y)
>>  #define serial_in(y)	inb((ulong)y)
>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
>> +#define serial_out(x,y) out_be32(y,x)
>> +#define serial_in(y) 	in_be32(y)
>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
>> +#define serial_out(x,y) out_le32(y,x)
>> +#define serial_in(y) 	in_le32(y)
>
> Sorry for the dumb question, but in which way is REG_SIZE > 0 or
> REG_SIZE < 0 connected to the endianess of the target system?
>
> My understanding is that this only defines how byte wide registers
> need to be padded, i. e. wether they are connected to the highest or
> to the lowest byte lane.  THis has nothing to do with the endianess of
> the system, and it appears wrong to me, to imply such a relation here.
>
> Detlev, what do you think?

I _think_ that if we are concerned with the question of where a one-byte
entity is placed in relation to its embracing 32-bit unit, then this is
the definiton of endianness, right?  Actually the endianness of the
UART, and this is exactly what the interpretation of the sign of the
REG_SIZE macro is.

So if the patch works for Dave, he gets my

Acked-by: Detlev Zundel <dzu@denx.de>

on the latter version.

Cheers
  Detlev

-- 
The Buddha,  the Godhead,  resides quite as comfortably in the circuits of a
digital computer  or the gears of a cycle transmission as he does at the top
of a mountain or in the petals of a flower.  To think otherwise is to demean
the Buddha - which is to demean oneself.                 -- Robert M. Pirsig
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH v3] ns16550: change to allow 32 bit access to registers

[Dave Aldridge]

Hi Detlev

On 09/09/11 13:09, Detlev Zundel wrote:
> Hi,
> 
>> Dear Dave Aldridge,
>>
>> In message <1314877212-31552-1-git-send-email-fovsoft@gmail.com> you wrote:
>>> If CONFIG_SYS_NS16550_MEM32 is defined then 32 bit memory
>>> mapped access will be used to read/write the uart registers.
>>>
>>> This is especially useful for SoC devices that implement 16550
>>> compatible uarts but that have peripheral access width constraints.
>>>
>>> Signed-off-by: Dave Aldridge <fovsoft@gmail.com>
>> ...
>>
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -19,6 +19,12 @@
>>>  #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>>  #define serial_out(x,y)	outb(x,(ulong)y)
>>>  #define serial_in(y)	inb((ulong)y)
>>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
>>> +#define serial_out(x,y) out_be32(y,x)
>>> +#define serial_in(y) 	in_be32(y)
>>> +#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
>>> +#define serial_out(x,y) out_le32(y,x)
>>> +#define serial_in(y) 	in_le32(y)
>>
>> Sorry for the dumb question, but in which way is REG_SIZE > 0 or
>> REG_SIZE < 0 connected to the endianess of the target system?
>>
>> My understanding is that this only defines how byte wide registers
>> need to be padded, i. e. wether they are connected to the highest or
>> to the lowest byte lane.  THis has nothing to do with the endianess of
>> the system, and it appears wrong to me, to imply such a relation here.
>>
>> Detlev, what do you think?
> 
> I _think_ that if we are concerned with the question of where a one-byte
> entity is placed in relation to its embracing 32-bit unit, then this is
> the definiton of endianness, right?  Actually the endianness of the
> UART, and this is exactly what the interpretation of the sign of the
> REG_SIZE macro is.
> 
> So if the patch works for Dave, he gets my
> 
> Acked-by: Detlev Zundel <dzu@denx.de>
> 
> on the latter version.
> 

Just to confirm that the V4 patch does work for me.

> Cheers
>   Detlev
>