| Message ID | 1314877212-31552-1-git-send-email-fovsoft@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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?
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
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
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
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
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 >
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]; \
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(-)