ns16550: change to allow 32 bit access to registers

PATCH Superseded Dave Aldridge 4 participants 7 messages Sep 1, 2011 – Sep 21, 2011

Share & tools

Dave Aldridge

author

2011-09-01 11:40 UTC #0 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

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 <[email protected]>

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

drivers/serial/ns16550.c +6 -0

index 8eeb48f..0174744 100644
@@ -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)

include/ns16550.h +2 -0

index 9ea81e9..d4ffac9 100644
@@ -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]; \

Signature · 1 line

1.7.3.4

Tabi Timur-B04825

2011-09-01 15:39 UTC #1 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

include/ns16550.h line 27

Quoted 4 lines · Dave Aldridge

?#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0) ?#error "Please define NS16550 registers size."
include/ns16550.h +2 -0
+#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?

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Dave Aldridge

author

2011-09-01 16:00 UTC #2 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

Hi Tabi Timur-B04825

Quoted 9 lines · On 01/09/11 16:39, Tabi Timur-B04825 wrote:

include/ns16550.h line 27
Quoted 4 lines · Dave Aldridge
#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0) #error "Please define NS16550 registers size."
include/ns16550.h +2 -0
+#elif defined(CONFIG_SYS_NS16550_MEM32)
+#define UART_REG(x) unsigned int x;
Shouldn't this be

Quoted 5 lines

#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

Wolfgang Denk

2011-09-07 21:22 UTC #3 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

In reply to Dave Aldridge

Dear Dave Aldridge,

Quoted 7 lines · In message <[email protected]> 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 <[email protected]>

...

drivers/serial/ns16550.c line 27

Quoted 12 lines

drivers/serial/ns16550.c +6 -0

@@ -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?

include/ns16550.h line 27

Quoted 13 lines

drivers/serial/ns16550.c +0 -0

 #else
 #define serial_out(x,y) writeb(x,y)
 #define serial_in(y) 	readb(y)

include/ns16550.h +2 -0

index 9ea81e9..d4ffac9 100644
@@ -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

Signature · 4 lines

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

Dave Aldridge

author

2011-09-08 09:40 UTC #4 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

Hi Wolfgang

Quoted 28 lines · Wolfgang Denk

Dear Dave Aldridge,
Quoted 7 lines · In message <[email protected]> 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 <[email protected]>
...
drivers/serial/ns16550.c line 27
Quoted 12 lines
drivers/serial/ns16550.c +6 -0
@@ -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?

Quoted 7 lines

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.

Quoted 17 lines

include/ns16550.h line 27
Quoted 13 lines
drivers/serial/ns16550.c +0 -0
 #else
 #define serial_out(x,y) writeb(x,y)
 #define serial_in(y) 	readb(y)
include/ns16550.h +2 -0
index 9ea81e9..d4ffac9 100644
@@ -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.

Quoted 4 lines

Best regards,
Wolfgang Denk

Cheers

Dave

Detlev Zundel

2011-09-09 12:09 UTC #5 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

In reply to Wolfgang Denk

Hi,

Quoted 28 lines

Dear Dave Aldridge,
Quoted 7 lines · In message <[email protected]> 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 <[email protected]>
...
drivers/serial/ns16550.c line 27
Quoted 12 lines
drivers/serial/ns16550.c +6 -0
@@ -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?

Quoted 6 lines

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 <[email protected]>

on the latter version.

Cheers
Detlev

Signature · 8 lines

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

Dave Aldridge

author

2011-09-21 09:21 UTC #6 lore raw

PATCH v3 1/1 ns16550: change to allow 32 bit access to registers

Hi Detlev

Quoted 43 lines · Detlev Zundel

Hi,
Quoted 28 lines
Dear Dave Aldridge,
Quoted 7 lines · In message <[email protected]> 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 <[email protected]>
...
drivers/serial/ns16550.c line 27
Quoted 12 lines
drivers/serial/ns16550.c +6 -0
@@ -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?
Quoted 6 lines
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.

Quoted 6 lines

So if the patch works for Dave, he gets my
Acked-by Detlev Zundel <[email protected]>
on the latter version.

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

Quoted 3 lines

Cheers
Detlev