[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Instead of special casing the different access patterns, use common
code with light macros sprinkled in to accomodate for the different
layouts of the register structure.

Note that this also changes the types of the registers for the
"positively packed (>1)" cases.  As the registers truly are unsigned
chars, this is surely the Right Thing, but it is a semantic change.
Note that for this case depending on the endianness on the bus, we may
see a change of behaviour.

Signed-off-by: Detlev Zundel <dzu@denx.de>
---
 include/ns16550.h |  130 +++++++++++++++--------------------------------------
 1 files changed, 37 insertions(+), 93 deletions(-)


Note, that I checked that the offsets are ok in the used cases
switching from the old to the new code.  They *do* shift however in
the positive packed cases, because the old code uses data types
different than unsigned char.  Note that doing this, I also noticed
that using "unsigned long" for 4 byte registers is also no longer true
on 64-bit architectures.  One more reason to change the code.

Apart from that the code was also compile tested on several
configurations using different REG_SIZES and compiles without
warnings.  The special interesting case of +4 was successfully tested
on CU824.



diff --git a/include/ns16550.h b/include/ns16550.h
index 87624bf..ce606b5 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -16,102 +16,46 @@
  * by Richard Danter (richard.danter at windriver.com), (C) 2005 Wind River Systems
  */
 
-#if (CONFIG_SYS_NS16550_REG_SIZE == 1)
-struct NS16550 {
-	unsigned char rbr;		/* 0 */
-	unsigned char ier;		/* 1 */
-	unsigned char fcr;		/* 2 */
-	unsigned char lcr;		/* 3 */
-	unsigned char mcr;		/* 4 */
-	unsigned char lsr;		/* 5 */
-	unsigned char msr;		/* 6 */
-	unsigned char scr;		/* 7 */
-#if defined(CONFIG_OMAP730)
-	unsigned char mdr1;		/* 8 */
-	unsigned char reg9;		/* 9 */
-	unsigned char regA;		/* A */
-	unsigned char regB;		/* B */
-	unsigned char regC;		/* C */
-	unsigned char regD;		/* D */
-	unsigned char regE;		/* E */
-	unsigned char regF;		/* F */
-	unsigned char reg10;		/* 10 */
-	unsigned char ssr;		/* 11*/
-#endif
-} __attribute__ ((packed));
-#elif (CONFIG_SYS_NS16550_REG_SIZE == 2)
-struct NS16550 {
-	unsigned short rbr;		/* 0 */
-	unsigned short ier;		/* 1 */
-	unsigned short fcr;		/* 2 */
-	unsigned short lcr;		/* 3 */
-	unsigned short mcr;		/* 4 */
-	unsigned short lsr;		/* 5 */
-	unsigned short msr;		/* 6 */
-	unsigned short scr;		/* 7 */
-} __attribute__ ((packed));
-#elif (CONFIG_SYS_NS16550_REG_SIZE == 4)
-struct NS16550 {
-	unsigned long rbr;		/* 0 r  */
-	unsigned long ier;		/* 1 rw */
-	unsigned long fcr;		/* 2 w  */
-	unsigned long lcr;		/* 3 rw */
-	unsigned long mcr;		/* 4 rw */
-	unsigned long lsr;		/* 5 r  */
-	unsigned long msr;		/* 6 r  */
-	unsigned long scr;		/* 7 rw */
-}; /* No need to pack an already aligned struct */
-#elif (CONFIG_SYS_NS16550_REG_SIZE == -4)
-struct NS16550 {
-	unsigned char rbr;		/* 0 */
-	int pad1:24;
-	unsigned char ier;		/* 1 */
-	int pad2:24;
-	unsigned char fcr;		/* 2 */
-	int pad3:24;
-	unsigned char lcr;		/* 3 */
-	int pad4:24;
-	unsigned char mcr;		/* 4 */
-	int pad5:24;
-	unsigned char lsr;		/* 5 */
-	int pad6:24;
-	unsigned char msr;		/* 6 */
-	int pad7:24;
-	unsigned char scr;		/* 7 */
-	int pad8:24;
-#if defined(CONFIG_OMAP)
-	unsigned char mdr1;		/* mode select reset TL16C750*/
-#endif
-#ifdef CONFIG_OMAP1510
-	int pad9:24;
-	unsigned long pad[10];
-	unsigned char osc_12m_sel;
-	int pad10:24;
-#endif
-} __attribute__ ((packed));
-#elif (CONFIG_SYS_NS16550_REG_SIZE == -8)
-struct NS16550 {
-	unsigned char rbr;		/* 0 */
-	unsigned char pad0[7];
-	unsigned char ier;		/* 1 */
-	unsigned char pad1[7];
-	unsigned char fcr;		/* 2 */
-	unsigned char pad2[7];
-	unsigned char lcr;		/* 3 */
-	unsigned char pad3[7];
-	unsigned char mcr;		/* 4 */
-	unsigned char pad4[7];
-	unsigned char lsr;		/* 5 */
-	unsigned char pad5[7];
-	unsigned char msr;		/* 6 */
-	unsigned char pad6[7];
-	unsigned char scr;		/* 7 */
-	unsigned char pad7[7];
-} __attribute__ ((packed));
-#else
+/*
+ * Note that the following macro magic uses the fact that the compiler
+ * will not allocate storage for arrays of size 0
+ */
+
+#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
 #error "Please define NS16550 registers size."
+#elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
+#define UART_REG(x)						   \
+	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
+	unsigned char x;
+#elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
+#define UART_REG(x)							\
+	unsigned char x;						\
+	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
 #endif
 
+struct NS16550 {
+	UART_REG(rbr);		/* 0 */
+	UART_REG(ier);		/* 1 */
+	UART_REG(fcr);		/* 2 */
+	UART_REG(lcr);		/* 3 */
+	UART_REG(mcr);		/* 4 */
+	UART_REG(lsr);		/* 5 */
+	UART_REG(msr);		/* 6 */
+	UART_REG(spr);		/* 7 */
+	UART_REG(mdr1);		/* 8 */
+	UART_REG(reg9);		/* 9 */
+	UART_REG(regA);		/* A */
+	UART_REG(regB);		/* B */
+	UART_REG(regC);		/* C */
+	UART_REG(regD);		/* D */
+	UART_REG(regE);		/* E */
+	UART_REG(uasr);		/* F */
+	UART_REG(scr);		/* 10*/
+	UART_REG(ssr);		/* 11*/
+	UART_REG(reg12);	/* 12*/
+	UART_REG(osc_12m_sel);	/* 13*/
+};
+
 #define thr rbr
 #define iir fcr
 #define dll rbr
-- 
1.6.0.6

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Detlev Zundel <dzu@denx.de> writes:

> Instead of special casing the different access patterns, use common
> code with light macros sprinkled in to accomodate for the different
> layouts of the register structure.
>
> Note that this also changes the types of the registers for the
> "positively packed (>1)" cases.  As the registers truly are unsigned
> chars, this is surely the Right Thing, but it is a semantic change.
> Note that for this case depending on the endianness on the bus, we may
> see a change of behaviour.
>
> Signed-off-by: Detlev Zundel <dzu@denx.de>
> ---
>  include/ns16550.h |  130 +++++++++++++++--------------------------------------
>  1 files changed, 37 insertions(+), 93 deletions(-)

I forgot to say, this patch depend on the previous cleanup patch sent
earlier today (Rename-common-ns16550-constants-with-UART_-prefix).

Cheers
  Detlev

-- 
1. What is the best thing about Unix?
A: The community.
2. What is the worst thing about Unix?
A: That there are so many communities.         (Rob Pike)
--
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] include/ns16550.h: Unify structure declaration for registers

[Wolfgang Denk]

Dear Detlev Zundel,

In message <1238769946-30370-1-git-send-email-dzu@denx.de> you wrote:
> Instead of special casing the different access patterns, use common
> code with light macros sprinkled in to accomodate for the different
> layouts of the register structure.
> 
> Note that this also changes the types of the registers for the
> "positively packed (>1)" cases.  As the registers truly are unsigned
> chars, this is surely the Right Thing, but it is a semantic change.
> Note that for this case depending on the endianness on the bus, we may
> see a change of behaviour.
> 
> Signed-off-by: Detlev Zundel <dzu@denx.de>
> ---
>  include/ns16550.h |  130 +++++++++++++++--------------------------------------
>  1 files changed, 37 insertions(+), 93 deletions(-)

Applied, thanks.

> Note, that I checked that the offsets are ok in the used cases
> switching from the old to the new code.  They *do* shift however in
> the positive packed cases, because the old code uses data types
> different than unsigned char.  Note that doing this, I also noticed
> that using "unsigned long" for 4 byte registers is also no longer true
> on 64-bit architectures.  One more reason to change the code.
> 
> Apart from that the code was also compile tested on several
> configurations using different REG_SIZES and compiles without
> warnings.  The special interesting case of +4 was successfully tested
> on CU824.


I guess when this code hits mainline (i. e. now) it will receive a
sufficient level of testing :-)

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
If A equals success, then the formula is A = X + Y + Z. X is work.  Y
is play. Z is keep your mouth shut.                 - Albert Einstein

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Detlev-san,

Detlev Zundel wrote:
> Instead of special casing the different access patterns, use common
> code with light macros sprinkled in to accomodate for the different
> layouts of the register structure.
> 
> Note that this also changes the types of the registers for the
> "positively packed (>1)" cases.  As the registers truly are unsigned
> chars, this is surely the Right Thing, but it is a semantic change.
> Note that for this case depending on the endianness on the bus, we may
> see a change of behaviour.
> 
> Signed-off-by: Detlev Zundel <dzu@denx.de>
> ---
>  include/ns16550.h |  130 +++++++++++++++--------------------------------------
>  1 files changed, 37 insertions(+), 93 deletions(-)
> 
> 
> Note, that I checked that the offsets are ok in the used cases
> switching from the old to the new code.  They *do* shift however in
> the positive packed cases, because the old code uses data types
> different than unsigned char.  Note that doing this, I also noticed
> that using "unsigned long" for 4 byte registers is also no longer true
> on 64-bit architectures.  One more reason to change the code.
> 
> Apart from that the code was also compile tested on several
> configurations using different REG_SIZES and compiles without
> warnings.  The special interesting case of +4 was successfully tested
> on CU824.

My hardware required 32-bit word access to NS16550 registers due to
byte-enable-lane reason (note that it's different from endian-ness).

I mean,

struct NS16550 {
    unsigned char rbr;
    unsigned char postpad_rbr[15];
        :
        :
};

if different from

struct NS16550 {
    unsigned long rbr;
    unsigned long postpad_rbr[3];
        :
        :
};

, at least for my hardware.

How do I supposed to configure UART in my board config file?

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Dear Shinya,

> Detlev Zundel wrote:
>> Instead of special casing the different access patterns, use common
>> code with light macros sprinkled in to accomodate for the different
>> layouts of the register structure.
>> 
>> Note that this also changes the types of the registers for the
>> "positively packed (>1)" cases.  As the registers truly are unsigned
>> chars, this is surely the Right Thing, but it is a semantic change.
>> Note that for this case depending on the endianness on the bus, we may
>> see a change of behaviour.
>> 
>> Signed-off-by: Detlev Zundel <dzu@denx.de>
>> ---
>>  include/ns16550.h |  130 +++++++++++++++--------------------------------------
>>  1 files changed, 37 insertions(+), 93 deletions(-)
>> 
>> 
>> Note, that I checked that the offsets are ok in the used cases
>> switching from the old to the new code.  They *do* shift however in
>> the positive packed cases, because the old code uses data types
>> different than unsigned char.  Note that doing this, I also noticed
>> that using "unsigned long" for 4 byte registers is also no longer true
>> on 64-bit architectures.  One more reason to change the code.
>> 
>> Apart from that the code was also compile tested on several
>> configurations using different REG_SIZES and compiles without
>> warnings.  The special interesting case of +4 was successfully tested
>> on CU824.
>
> My hardware required 32-bit word access to NS16550 registers due to
> byte-enable-lane reason (note that it's different from endian-ness).
>
> I mean,
>
> struct NS16550 {
>     unsigned char rbr;
>     unsigned char postpad_rbr[15];
>         :
>         :
> };
>
> if different from
>
> struct NS16550 {
>     unsigned long rbr;
>     unsigned long postpad_rbr[3];
>         :
>         :
> };
>
> , at least for my hardware.

To be honest, I did not expect such problems, as I saw no hints from
comments on why this code was needed.  Thinking afresh, it now makes at
least some sense why the code was.  It nevertheless was inconsistent, as
the word access was only done in an asymmetric way regarding the
REG_SIZES parameter.

> How do I supposed to configure UART in my board config file?

I'm not sure at all.  I believe you tested with 4 and -4 and it doesn't
work, right?

Now we have the problem that we have byte registers (after all, there
are only 8 bits significant even for your platform) which need to be
accessed as 32-bit entities (or 16 bit for other platforms maybe).

I don't see any way out here than to probably re-introduce different
data-types again - which I certainly do not like too much as the
registers stay 8 bit wide.

Does anyone else have a good idea here?

Thanks
  Detlev

-- 
"Oh, didn't you know, the Lord did the original programming of the universe in
COBOL." - "That's why the world is the evil work of Satan. A true divine being
would have used Scheme."  -  "And, if so, Jesus would have been crucified on a
big lambda symbol."  -- K. Chafin, K. Schilling & D. Hanley, on comp.lang.lisp
--
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] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Detlev Zundel wrote:
> To be honest, I did not expect such problems, as I saw no hints from
> comments on why this code was needed.  Thinking afresh, it now makes at
> least some sense why the code was.  It nevertheless was inconsistent, as
> the word access was only done in an asymmetric way regarding the
> REG_SIZES parameter.

I used to define locally "long + -16" variants.

>> How do I supposed to configure UART in my board config file?
> 
> I'm not sure at all.  I believe you tested with 4 and -4 and it doesn't
> work, right?

Right.

> Now we have the problem that we have byte registers (after all, there
> are only 8 bits significant even for your platform) which need to be
> accessed as 32-bit entities (or 16 bit for other platforms maybe).

This is why Linux 8250 driver supports not only UPIO_MEM but also
UPIO_MEM32.

> I don't see any way out here than to probably re-introduce different
> data-types again - which I certainly do not like too much as the
> registers stay 8 bit wide.

If there's no good alternatives, I think reverting is a good idea
because there must be other platforms affected by this change.

> Does anyone else have a good idea here?

Hm, how about introducing serial_{in,out} concepts from Linux?

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hello Shinya,

> Detlev Zundel wrote:
>> To be honest, I did not expect such problems, as I saw no hints from
>> comments on why this code was needed.  Thinking afresh, it now makes at
>> least some sense why the code was.  It nevertheless was inconsistent, as
>> the word access was only done in an asymmetric way regarding the
>> REG_SIZES parameter.
>
> I used to define locally "long + -16" variants.

As I said, I understand now why there were different data-types involved
although this was kind of non-obvious.  So I take it, you had a working
configuration with REG_SIZE = 4, correct?

Can you enlighten me, why exactly the 8-bit accesses do not work on your
hardware?  Is this because of a "too simplistic" address decoding logic?
What endianness is your CPU using?

You see, I still do not understand the problem completely.  There is a
board named ppmc7xx which uses REG_SIZE -8.  The comment says "64-bit
accesses to 8-bit port" and the definition is just like in the new
version with uchars and padding.  Why did that work?  Very likely the
comment is wrong, but still

>>> How do I supposed to configure UART in my board config file?
>> 
>> I'm not sure at all.  I believe you tested with 4 and -4 and it doesn't
>> work, right?
>
> Right.
>
>> Now we have the problem that we have byte registers (after all, there
>> are only 8 bits significant even for your platform) which need to be
>> accessed as 32-bit entities (or 16 bit for other platforms maybe).
>
> This is why Linux 8250 driver supports not only UPIO_MEM but also
> UPIO_MEM32.

I see.  Actually I was looking a lot at the Linux driver but was hoping
that we could away without introducing serial_{in,out}...
 
>> I don't see any way out here than to probably re-introduce different
>> data-types again - which I certainly do not like too much as the
>> registers stay 8 bit wide.
>
> If there's no good alternatives, I think reverting is a good idea
> because there must be other platforms affected by this change.

Reverting is not a long-term option if somebody wants to maintain the
source at all.  Did you see all the different layout structures with
fields being defined only in one of the 5 alternatives (of the possible
8)?  Bah...

>> Does anyone else have a good idea here?
>
> Hm, how about introducing serial_{in,out} concepts from Linux?

Maybe, but maybe we can also do some more cheat^B^B^B^B^B creative
coding.  I was think about something along those lines:

diff --git a/include/ns16550.h b/include/ns16550.h
index ce606b5..7924396 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -21,16 +21,20 @@
  * will not allocate storage for arrays of size 0
  */
 
+#if !defined(CONFIG_SYS_NS16550_REG_TYPE)
+#define UART_REG_TYPE unsigned char
+#endif
+
 #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
 #error "Please define NS16550 registers size."
 #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
-#define UART_REG(x)						   \
-	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
-	unsigned char x;
+#define UART_REG(x)							\
+	UART_REG_TYPE prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)]; \
+	UART_REG_TYPE x;
 #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
 #define UART_REG(x)							\
-	unsigned char x;						\
-	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
+	UART_REG_TYPE x;						\
+	UART_REG_TYPE postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)];
 #endif
 
 struct NS16550 {


Then you could do a

#define CONFIG_SYS_NS16550_REG_SIZE 4
#define CONFIG_SYS_NS16550_REG_TYPE unsigned long

This of course needs to be documented once it works ;)

What do you think?

Cheers
  Detlev

-- 
config LGUEST
        If unsure, say N.  If curious, say M.  If masochistic, say Y.
                                     -- linux/drivers/lguest/Kconfig
--
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@denx.de

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hello Shinya,

>> If there's no good alternatives, I think reverting is a good idea
>> because there must be other platforms affected by this change.

I just checked again - the "problematic" cases can only be REG_SIZE 2
and 4:

[dzu at pollux u-boot-testing (master)]$ grep CONFIG_SYS_NS16550_REG_SIZE include/configs/* | grep -v 'REG_SIZE.*\(1\|(-4)\|-4\|-8\)'
include/configs/AP1000.h:#define CONFIG_SYS_NS16550_REG_SIZE    4
include/configs/CU824.h:#define CONFIG_SYS_NS16550_REG_SIZE     4
include/configs/mcc200.h:#define CONFIG_SYS_NS16550_REG_SIZE    4
include/configs/ppmc7xx.h: * CONFIG_SYS_NS16550_REG_SIZE - 64-bit accesses to 8-bit port
include/configs/xilinx-ppc.h:#define CONFIG_SYS_NS16550_REG_SIZE 4

CU824 incidentally is the platform that I tested the patch on (PowerPC),
ppmc7xx is a false positive, so only three platforms remain.  I'm almost
100% sure that mcc200 also works, as it is also a PowerPC platform
comparable to CU824. Is AP1000 your platform?

We should really work to get these platforms in order, the fallout is
not as big as may have been expected...

Cheers
  Detlev

-- 
Summary [of object-oriented programming in Perl 5]
That's all about there is to it. Now you just need to go off and buy a
book about object-oriented design methodology, and bang  your forehead
with it for the next six months or so.    Larry Wall [Creator of Perl]
--
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] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Detlev Zundel wrote:
> As I said, I understand now why there were different data-types involved
> although this was kind of non-obvious.  So I take it, you had a working
> configuration with REG_SIZE = 4, correct?

I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
are located at 0, +0x10, +0x20, ..., .

In this case, I don't think REG_SIZE = 4/-4 works.  Let's see:

REG_SIZE = 4
------------
struct NS16550 {
        unsigned char prepad_rbr[3];
        unsigned char rbr;
        unsigned char prepad_ier[3];
        unsigned char ier;
        :
        :
};

REG_SIZE = -4
-------------
struct NS16550 {
        unsigned char rbr;
        unsigned char postpad_rbr[3];
        unsigned char ier;
        unsigned char postpad_ier[3];
        :
        :
};

because 16550 registers can be aligned in 16-bytes-interval.
And make things worse, REG_SIZE = 16/-16 also don't work.

REG_SIZE = 16
-------------
struct NS16550 {
        unsigned char prepad_rbr[15];
        unsigned char rbr;
        unsigned char prepad_ier[15];
        unsigned char ier;
        :
        :
};

REG_SIZE = -16
--------------
struct NS16550 {
        unsigned char rbr;
        unsigned char postpad_rbr[15];
        unsigned char ier;
        unsigned char postpad_ier[15];
        :
        :
};

What I need is something like this:

struct NS16550 {
        unsigned char prepad_rbr[3];
        unsigned char rbr;
        unsigned char postpad_rbr[12];
        :
        :
};

or this also might work,

struct NS16550 {
        unsigned long rbr;
        unsigned long pre_padrbr[3];
        :        ^^^^
        :
};

Makes sense?

> Can you enlighten me, why exactly the 8-bit accesses do not work on your
> hardware?  Is this because of a "too simplistic" address decoding logic?
> What endianness is your CPU using?

I don't know much about precise hardware logics, but the byte addresses
under 16-bytes-border are ignored.  I'm using a big-endian mips machine.

> I see.  Actually I was looking a lot at the Linux driver but was hoping
> that we could away without introducing serial_{in,out}...

In my horrible opinion, the combinations of base addres + reg_shift
+ iotype (char, long, or whatever), are simpler, more configurable,
more slid, easy to use, than what we used to have or what you
consolidated this time.

> diff --git a/include/ns16550.h b/include/ns16550.h
> index ce606b5..7924396 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -21,16 +21,20 @@
>   * will not allocate storage for arrays of size 0
>   */
>  
> +#if !defined(CONFIG_SYS_NS16550_REG_TYPE)
> +#define UART_REG_TYPE unsigned char
> +#endif
> +
>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>  #error "Please define NS16550 registers size."
>  #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
> -#define UART_REG(x)						   \
> -	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
> -	unsigned char x;
> +#define UART_REG(x)							\
> +	UART_REG_TYPE prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)]; \
> +	UART_REG_TYPE x;
>  #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
>  #define UART_REG(x)							\
> -	unsigned char x;						\
> -	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
> +	UART_REG_TYPE x;						\
> +	UART_REG_TYPE postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)];
>  #endif
>  
>  struct NS16550 {
> 
> 
> Then you could do a
> 
> #define CONFIG_SYS_NS16550_REG_SIZE 4
> #define CONFIG_SYS_NS16550_REG_TYPE unsigned long
> 
> This of course needs to be documented once it works ;)

Looks to me like playing with macros...

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Shinya Kuribayashi wrote:
> Detlev Zundel wrote:
>> As I said, I understand now why there were different data-types involved
>> although this was kind of non-obvious.  So I take it, you had a working
>> configuration with REG_SIZE = 4, correct?
> 
> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
> are located at 0, +0x10, +0x20, ..., .
> 
> In this case, I don't think REG_SIZE = 4/-4 works.  Let's see:
> 
> REG_SIZE = 4
> ------------
> struct NS16550 {
>         unsigned char prepad_rbr[3];
>         unsigned char rbr;
>         unsigned char prepad_ier[3];
>         unsigned char ier;
>         :
>         :
> };
> 
> REG_SIZE = -4
> -------------
> struct NS16550 {
>         unsigned char rbr;
>         unsigned char postpad_rbr[3];
>         unsigned char ier;
>         unsigned char postpad_ier[3];
>         :
>         :
> };
> 
> because 16550 registers can be aligned in 16-bytes-interval.

s/can be aligned/can not be aligned/

>> diff --git a/include/ns16550.h b/include/ns16550.h
>> index ce606b5..7924396 100644
>> --- a/include/ns16550.h
>> +++ b/include/ns16550.h
>> @@ -21,16 +21,20 @@
>>   * will not allocate storage for arrays of size 0
>>   */
>>  
>> +#if !defined(CONFIG_SYS_NS16550_REG_TYPE)
>> +#define UART_REG_TYPE unsigned char
>> +#endif
>> +
>>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>>  #error "Please define NS16550 registers size."
>>  #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
>> -#define UART_REG(x)						   \
>> -	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
>> -	unsigned char x;
>> +#define UART_REG(x)							\
>> +	UART_REG_TYPE prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)]; \
>> +	UART_REG_TYPE x;
>>  #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
>>  #define UART_REG(x)							\
>> -	unsigned char x;						\
>> -	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
>> +	UART_REG_TYPE x;						\
>> +	UART_REG_TYPE postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)];
>>  #endif
>>  
>>  struct NS16550 {
>>
>>
>> Then you could do a
>>
>> #define CONFIG_SYS_NS16550_REG_SIZE 4
>> #define CONFIG_SYS_NS16550_REG_TYPE unsigned long
>>
>> This of course needs to be documented once it works ;)
> 
> Looks to me like playing with macros...

Looks to me like playing with macros... but,
this is better than before, and would work for my machine.

I'm alo not interested in maintaining UART driver, so such work
reducing our maintenance cost is highly appreciated.  Thanks,

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hello Shinya,

> Detlev Zundel wrote:
>> As I said, I understand now why there were different data-types involved
>> although this was kind of non-obvious.  So I take it, you had a working
>> configuration with REG_SIZE = 4, correct?
>
> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
> are located at 0, +0x10, +0x20, ..., .

Ah, so you actually maintain an out-of-tree port.  How should I have
foreseen that I break something that I don't even have the code to?

> In this case, I don't think REG_SIZE = 4/-4 works.  Let's see:

No surely not.  My replies were based on the (wrong) assumption that
your board port was in U-Boot code.

> What I need is something like this:
>
> struct NS16550 {
>        unsigned char prepad_rbr[3];
>        unsigned char rbr;
>        unsigned char postpad_rbr[12];
>        :
>        :
> };
>
> or this also might work,
>
> struct NS16550 {
>        unsigned long rbr;
>        unsigned long pre_padrbr[3];
>        :        ^^^^
>        :
> };
>
> Makes sense?

Although I can see what you need, I would be lying if I said that this
makes sense to me.

>> Can you enlighten me, why exactly the 8-bit accesses do not work on your
>> hardware?  Is this because of a "too simplistic" address decoding logic?
>> What endianness is your CPU using?
>
> I don't know much about precise hardware logics, but the byte addresses
> under 16-bytes-border are ignored.  I'm using a big-endian mips machine.

This does not make much sense to me, sorry.

>> I see.  Actually I was looking a lot at the Linux driver but was hoping
>> that we could away without introducing serial_{in,out}...
>
> In my horrible opinion, the combinations of base addres + reg_shift
> + iotype (char, long, or whatever), are simpler, more configurable,
> more slid, easy to use, than what we used to have or what you
> consolidated this time.

You lost me here.

You truly consider

static unsigned int serial_in(struct uart_8250_port *up, int offset)
{
        unsigned int tmp;
        int ret, flags;
        offset = map_8250_in_reg(up, offset) << up->port.regshift;

        spin_lock_irqsave(&lbi_lock, flags);
        switch (up->port.iotype) {
        case UPIO_HUB6:
                outb(up->port.hub6 - 1 + offset, up->port.iobase);
                ret = inb(up->port.iobase + 1);
                break;

        case UPIO_MEM:
        case UPIO_DWAPB:
                ret = readb(up->port.membase + offset);
                break;

        case UPIO_RM9000:
        case UPIO_MEM32:
                ret = readl(up->port.membase + offset);
                break;

#ifdef CONFIG_SERIAL_8250_AU1X00
        case UPIO_AU:
                ret = __raw_readl(up->port.membase + offset);
                break;
#endif

        case UPIO_TSI:
                if (offset == UART_IIR) {
                        tmp = readl(up->port.membase + (UART_IIR & ~3));
                        ret = (tmp >> 16) & 0xff; /* UART_IIR % 4 == 2*/
                } else
                        ret = readb(up->port.membase + offset);
                break;

        default:
                ret = inb(up->port.iobase + offset);
                break;
        }
        spin_unlock_irqrestore(&lbi_lock, flags);
        return ret;
}

to be "simpler and more solid" readb(struct->field) (which is
effectively what we have in the current implementation)?  You consider
"more configurable" to be a good in its own?

If your answers to these questions are yes, then we have different ideas
of writing code.

>> diff --git a/include/ns16550.h b/include/ns16550.h
>> index ce606b5..7924396 100644
>> --- a/include/ns16550.h
>> +++ b/include/ns16550.h
>> @@ -21,16 +21,20 @@
>>   * will not allocate storage for arrays of size 0
>>   */
>>  +#if !defined(CONFIG_SYS_NS16550_REG_TYPE)
>> +#define UART_REG_TYPE unsigned char
>> +#endif
>> +
>>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>>  #error "Please define NS16550 registers size."
>>  #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
>> -#define UART_REG(x)						   \
>> -	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
>> -	unsigned char x;
>> +#define UART_REG(x)							\
>> +	UART_REG_TYPE prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)]; \
>> +	UART_REG_TYPE x;
>>  #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
>>  #define UART_REG(x)							\
>> -	unsigned char x;						\
>> -	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
>> +	UART_REG_TYPE x;						\
>> +	UART_REG_TYPE postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)];
>>  #endif
>>   struct NS16550 {
>>
>>
>> Then you could do a
>>
>> #define CONFIG_SYS_NS16550_REG_SIZE 4
>> #define CONFIG_SYS_NS16550_REG_TYPE unsigned long
>>
>> This of course needs to be documented once it works ;)
>
> Looks to me like playing with macros...

This is not playing.  I have better things to do if I want to play.
This was meant to be a solution for a problem which currently seems to
only exist in one special configuration, namely yours.

Best wishes
  Detlev

-- 
LISP has  jokingly been  described as  "the most  intelligent way to  misuse a
computer".  I think that  description a great  compliment because it transmits
the full  flavour of  liberation:  it has assisted a number of our most gifted
fellow humans in thinking previously impossible thoughts. - Edsger W. Dijkstra
--
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] include/ns16550.h: Unify structure declaration for registers

[Jerry Van Baren]

Detlev Zundel wrote:
> Hello Shinya,
> 
>> Detlev Zundel wrote:
>>> As I said, I understand now why there were different data-types involved
>>> although this was kind of non-obvious.  So I take it, you had a working
>>> configuration with REG_SIZE = 4, correct?
>> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
>> are located at 0, +0x10, +0x20, ..., .

16 byte stride.  That is seriously odd.

> Ah, so you actually maintain an out-of-tree port.  How should I have
> foreseen that I break something that I don't even have the code to?
> 
>> In this case, I don't think REG_SIZE = 4/-4 works.  Let's see:
> 
> No surely not.  My replies were based on the (wrong) assumption that
> your board port was in U-Boot code.
> 
>> What I need is something like this:
>>
>> struct NS16550 {
>>        unsigned char prepad_rbr[3];
>>        unsigned char rbr;
>>        unsigned char postpad_rbr[12];
>>        :
>>        :
>> };

This is showing a stride of 4 bytes, *not* 16.

>> or this also might work,
>>
>> struct NS16550 {
>>        unsigned long rbr;
>>        unsigned long pre_padrbr[3];
>>        :        ^^^^
>>        :
>> };

Again, a stride of 4 bytes, *not* 16.

>> Makes sense?
> 
> Although I can see what you need, I would be lying if I said that this
> makes sense to me.
> 
>>> Can you enlighten me, why exactly the 8-bit accesses do not work on your
>>> hardware?  Is this because of a "too simplistic" address decoding logic?
>>> What endianness is your CPU using?
>> I don't know much about precise hardware logics, but the byte addresses
>> under 16-bytes-border are ignored.  I'm using a big-endian mips machine.
> 
> This does not make much sense to me, sorry.

The "16" of the "16-bytes-border" statement confuses me too.

It sounds like Shinya has some pretty odd (read "broken") hardware that 
is decoding the registers with a 16 byte stride, although his example 
above shows a 4 byte stride (less broken).

I would further deduce his hardware does not support byte write 
operations (I've never seen hardware that didn't support byte reads). 
I've had hardware that did not support byte writes, so s/w needed to 
write a word instead (given Shinya's description, the extra bytes are 
"don't care").  (I've also dealt with flash connections that only 
supported 64 bit writes - PITA!).

My guess is his processor limitations prevent byte writes so he has to 
do 32bit (4byte) writes, but his hardware decoding results in a 16 byte 
stride.  The result is setting REG_SIZE to 4 gives him the r/w access he 
needs (32 bits), but fails the stride.  Setting it to 16 gives him the 
stride he needs, but a 16 byte register is nonsensical and breaks the 
software.  My guess is Shinya needs another customization dial (I'm 
making this up) "REG_STRIDE" = 16 as well as "REG_SIZE" = 4.

[snip]

Best regards,
gvb

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hello Shinya,

> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
> are located at 0, +0x10, +0x20, ..., .

Actually, come to think of it, I have never seen what you used to use,
as the REG_SIZE = -16 case was never in the official U-Boot sources.
Theoretically extending the "-4 to -8" step from the old code to
extrapolate to -16, I get exactly what my new version yields.

> Looks to me like playing with macros... but,
> this is better than before, and would work for my machine.

Thinking about it some more, I wonder about the following.  You said,
this would work for you:

struct NS16550 {
       unsigned long rbr;
       unsigned long postpad_rbr[3];
....

while

struct NS16550 {
       unsigned char rbr;
       unsigned char postpad_rbr[12];
...

doesn't.  If we regard only the "significant" 8-bits, the first layout
is congruent to the second shifted by 2 bytes (on big-endian machines).

So what about using +16 for your board and lower the base address by 2?
Does that work?  What is your base address?  Is that 64-bit aligned?

This is somewhat hypothetical and outright ugly, but I still want to
know if this works.

Thanks
  Detlev

-- 
#!/usr/bin/perl -l
print ((1 x shift) !~ /^(11+?)\1+$/ ? "prime" : "not prime");
--
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] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hi Jerry,

> Detlev Zundel wrote:
>> Hello Shinya,
>>
>>> Detlev Zundel wrote:
>>>> As I said, I understand now why there were different data-types involved
>>>> although this was kind of non-obvious.  So I take it, you had a working
>>>> configuration with REG_SIZE = 4, correct?
>>> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
>>> are located at 0, +0x10, +0x20, ..., .
>
> 16 byte stride.  That is seriously odd.

Isn't this "natural" for a 64-bitter?

>> Ah, so you actually maintain an out-of-tree port.  How should I have
>> foreseen that I break something that I don't even have the code to?
>>
>>> In this case, I don't think REG_SIZE = 4/-4 works.  Let's see:
>>
>> No surely not.  My replies were based on the (wrong) assumption that
>> your board port was in U-Boot code.
>>
>>> What I need is something like this:
>>>
>>> struct NS16550 {
>>>        unsigned char prepad_rbr[3];
>>>        unsigned char rbr;
>>>        unsigned char postpad_rbr[12];
>>>        :
>>>        :
>>> };
>
> This is showing a stride of 4 bytes, *not* 16.

Why 4 bytes?  3 + 1 + 12 = 16 as stated.

>>> or this also might work,
>>>
>>> struct NS16550 {
>>>        unsigned long rbr;
>>>        unsigned long pre_padrbr[3];
>>>        :        ^^^^
>>>        :
>>> };
>
> Again, a stride of 4 bytes, *not* 16.

4 * sizeof(unsigned long) = 16

>>> Makes sense?
>>
>> Although I can see what you need, I would be lying if I said that this
>> makes sense to me.
>>
>>>> Can you enlighten me, why exactly the 8-bit accesses do not work on your
>>>> hardware?  Is this because of a "too simplistic" address decoding logic?
>>>> What endianness is your CPU using?
>>> I don't know much about precise hardware logics, but the byte addresses
>>> under 16-bytes-border are ignored.  I'm using a big-endian mips machine.
>>
>> This does not make much sense to me, sorry.
>
> The "16" of the "16-bytes-border" statement confuses me too.
>
> It sounds like Shinya has some pretty odd (read "broken") hardware
> that is decoding the registers with a 16 byte stride, although his
> example above shows a 4 byte stride (less broken).

It's a 16-byte stride, although the register shows up neither at the
top, nor at the low end, but "slightly to the left", i.e. at offset 0x3
;)

> I would further deduce his hardware does not support byte write
> operations (I've never seen hardware that didn't support byte
> reads). I've had hardware that did not support byte writes, so s/w
> needed to write a word instead (given Shinya's description, the extra
> bytes are "don't care").  (I've also dealt with flash connections that
> only supported 64 bit writes - PITA!).
>
> My guess is his processor limitations prevent byte writes so he has to
> do 32bit (4byte) writes, but his hardware decoding results in a 16
> byte stride.  The result is setting REG_SIZE to 4 gives him the r/w
> access he needs (32 bits), but fails the stride.  Setting it to 16
> gives him the stride he needs, but a 16 byte register is nonsensical
> and breaks the software.  My guess is Shinya needs another
> customization dial (I'm making this up) "REG_STRIDE" = 16 as well as
> "REG_SIZE" = 4.

That's what my previous hack offered - REG_TYPE for the actual "size" of
accesses and REG_SIZE for the stride.  Now that you mention it, one
should probably use REG_TYPE and REG_STRIDE...

Cheers
  Detlev

-- 
Cyberwar is certainly not a myth. But you haven't seen it yet, despite
the attacks on Estonia. Cyberwar is warfare in cyberspace. And warfare
involves massive death and destruction. When you see it, you'll know it.
                           -- Bruce Schneier, Nov. 2007
--
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] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Detlev Zundel <dzu@denx.de> writes:

> So what about using +16 for your board and lower the base address by 2?

Ugh. Increasing by 2 would be more in line with my reasongin...

> Does that work?  What is your base address?  Is that 64-bit aligned?
>
> This is somewhat hypothetical and outright ugly, but I still want to
> know if this works.

Best wishes
  Detlev

-- 
This is  not the first  time my views  on some topic have  inspired in
someone the  desire to psychoanalyze me. Previous  experience leads me
to ask  about your couch. Is  it comfortable? Are its  springs in good
shape?                                     -- Jonh McCarthy
--
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] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hi Jerry,

>> 16 byte stride.  That is seriously odd.
>
> Isn't this "natural" for a 64-bitter?

No, of course not.  That would be still another generation of course.
Makes this look all the more weird.

Cheers
  Detlev

-- 
Each language has its purpose, however humble. Each language expresses
the Yin and Yang of software. Each language has its place within the Tao.

But do not program in COBOL if you can avoid it.
                           -- The Tao of Programming
--
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

-- 
I haven't lost my mind, I know exactly where I left it.
--
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] include/ns16550.h: Unify structure declaration for registers

[Jerry Van Baren]

Detlev Zundel wrote:
> Hi Jerry,
> 
>> Detlev Zundel wrote:
>>> Hello Shinya,
>>>
>>>> Detlev Zundel wrote:
>>>>> As I said, I understand now why there were different data-types involved
>>>>> although this was kind of non-obvious.  So I take it, you had a working
>>>>> configuration with REG_SIZE = 4, correct?
>>>> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
>>>> are located at 0, +0x10, +0x20, ..., .
>> 16 byte stride.  That is seriously odd.
> 
> Isn't this "natural" for a 64-bitter?

Yes.  I wasn't thinking of the processor as 64 bits.

[snip]

>>
>> It sounds like Shinya has some pretty odd (read "broken") hardware
>> that is decoding the registers with a 16 byte stride, although his
>> example above shows a 4 byte stride (less broken).
> 
> It's a 16-byte stride, although the register shows up neither at the
> top, nor at the low end, but "slightly to the left", i.e. at offset 0x3
> ;)

That is the big piece I didn't understand.  Thanks and sorry for the noise.

[snip]

Best regards
gvb

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Hi,

Detlev Zundel wrote:
> Thinking about it some more, I wonder about the following.  You said,
> this would work for you:
> 
> struct NS16550 {
>        unsigned long rbr;
>        unsigned long postpad_rbr[3];
> ....
> 
> while
> 
> struct NS16550 {
>        unsigned char rbr;
>        unsigned char postpad_rbr[12];
                                   15?
> ...
> 
> doesn't.  If we regard only the "significant" 8-bits, the first layout
> is congruent to the second shifted by 2 bytes (on big-endian machines).

I think you mean 'at offset 0x3', right?

My 16550 registers are like this:

        0    1    2    3
        +----+----+----+----+
0x00    |   reserved   |rbr |
        +----+----+----+----+
0x04    |     reserved      |
        +----+----+----+----+
0x08    |     reserved      |
        +----+----+----+----+
0x0c    |     reserved      |
        +----+----+----+----+
0x10    |   reserved   |ier |
        +----+----+----+----+
0x14    |     reserved      |
        +----+----+----+----+
0x18    |     reserved      |
        +----+----+----+----+
0x1c    |     reserved      |
        +----+----+----+----+
...

> So what about using +16 for your board and lower the base address by 2?
> Does that work?  What is your base address?  Is that 64-bit aligned?

With 64-bit (16 bytes) aligned base address, AND offset +0x3, AND
struct NS16550 configured by -16 (below)

struct NS16550 {
        unsigned char rbr;
        unsigned char postpad_rbr[15];
        unsigned char ier;
        unsigned char postpad_ier[15];
        ...
};

, then yes, I think this probably works for sane hardwares who can
handle byte read/write operations properly.

As for my hardware, however, this still doesn't work. My processor
(MIPS 4KEc) of couse supports byte read/write, on the other hand,
the address decoder at UART module can not handle byte addresses
properly; all byte read/write accesses with +1/+2/+3 offset, will
be round-down to +0.  Therefore, I can take 'offset +0x3' option.

This is the reasony I said 'my hardware requires 32-bit word access
to NS16550 registers'.

> This is somewhat hypothetical and outright ugly, but I still want to
> know if this works.

I know the address decoder of my UART hardware sucks, but it's not
unusual.

I'll reply to remaining mails shortly.

Thanks,

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Hi,

Detlev Zundel wrote:
>>> I see.  Actually I was looking a lot at the Linux driver but was hoping
>>> that we could away without introducing serial_{in,out}...
>> In my horrible opinion, the combinations of base addres + reg_shift
>> + iotype (char, long, or whatever), are simpler, more configurable,
>> more slid, easy to use, than what we used to have or what you
>> consolidated this time.
> 
> You lost me here.
> 
> You truly consider
> 
> static unsigned int serial_in(struct uart_8250_port *up, int offset)
[snip]
> }
> 
> to be "simpler and more solid" readb(struct->field) (which is
> effectively what we have in the current implementation)?  You consider
> "more configurable" to be a good in its own?

Yes.

> If your answers to these questions are yes, then we have different ideas
> of writing code.

Please make sure we don't need full serial_{in,out} port from Linux
as-is.  As suggested, the combinations of base addres + reg_shift +
iotype, are rather reasonable to support various kind of hardwares.

I mean we need something like this:

static unsigned int serial_in(struct uart_8250_port *up, int offset)
{
        unsigned int tmp;
        int ret;
        offset = map_8250_in_reg(up, offset) << up->port.regshift;

        switch (up->port.iotype) {
        case UPIO_MEM:
                ret = readb(up->port.membase + offset);
                break;

        case UPIO_MEM32:
                ret = readl(up->port.membase + offset);
                break;

        default:
                ret = inb(up->port.iobase + offset);
                break;
        }
        return ret;
}

Its implementation must be differed in U-Boot code, of course.


>>> diff --git a/include/ns16550.h b/include/ns16550.h
>>> index ce606b5..7924396 100644
>>> --- a/include/ns16550.h
>>> +++ b/include/ns16550.h
>>> @@ -21,16 +21,20 @@
>>>   * will not allocate storage for arrays of size 0
>>>   */
>>>  +#if !defined(CONFIG_SYS_NS16550_REG_TYPE)
>>> +#define UART_REG_TYPE unsigned char
>>> +#endif
>>> +
>>>  #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
>>>  #error "Please define NS16550 registers size."
>>>  #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
>>> -#define UART_REG(x)						   \
>>> -	unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
>>> -	unsigned char x;
>>> +#define UART_REG(x)							\
>>> +	UART_REG_TYPE prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)]; \
>>> +	UART_REG_TYPE x;
>>>  #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
>>>  #define UART_REG(x)							\
>>> -	unsigned char x;						\
>>> -	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
>>> +	UART_REG_TYPE x;						\
>>> +	UART_REG_TYPE postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - sizeof(UART_REG_TYPE)];
>>>  #endif
>>>   struct NS16550 {
>>>
>>>
>>> Then you could do a
>>>
>>> #define CONFIG_SYS_NS16550_REG_SIZE 4
>>> #define CONFIG_SYS_NS16550_REG_TYPE unsigned long
>>>
>>> This of course needs to be documented once it works ;)
>> Looks to me like playing with macros...
> 
> This is not playing.  I have better things to do if I want to play.
> This was meant to be a solution for a problem which currently seems to
> only exist in one special configuration, namely yours.

I admit the address decoder in my UART hardware is weird and needs
special configuration,  but this is not just for my case, it's not
unusual.

There're various kind of hardwares in the world, and there're many
U-Boot ports which can not be pushed to upstream for various reasons.
We can easily ignore such boards of course, but it would be very nice
for U-Boot if it could provide easy configurable drivers and could
support as many hardwares as possible.

Thanks for your time,

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Hi Jerry-san,

Jerry Van Baren wrote:
>>> I might be unclear. I used to use REG_SIZE = -16, as 16550 registers
>>> are located at 0, +0x10, +0x20, ..., .
> 
> 16 byte stride.  That is seriously odd.

Well, 8 or 16 byte stride is not so odd, IMHO.

>>> I don't know much about precise hardware logics, but the byte addresses
>>> under 16-bytes-border are ignored.  I'm using a big-endian mips machine.
>>
>> This does not make much sense to me, sorry.
> 
> The "16" of the "16-bytes-border" statement confuses me too.

Sorry for my poor vocabularies :-(

> It sounds like Shinya has some pretty odd (read "broken") hardware that 
> is decoding the registers with a 16 byte stride, although his example 
> above shows a 4 byte stride (less broken).

Let me reword:

* my UART registers are located with 16 byte stride.

* The address decoder in my UART block rounds +1/+2/+3 offsets down
  to zero offset.  Therefore we can't do byte read/write to ns16550
  registers properly; i.e. the return value of readb(x + 3) will be
  equal to readb(x).

> I would further deduce his hardware does not support byte write 
> operations (I've never seen hardware that didn't support byte reads). 
> I've had hardware that did not support byte writes, so s/w needed to 
> write a word instead (given Shinya's description, the extra bytes are 
> "don't care").  (I've also dealt with flash connections that only 
> supported 64 bit writes - PITA!).

Thanks for deducing :-)  Yes, I wanted to say 'don't care'.

> My guess is his processor limitations prevent byte writes so he has to 
> do 32bit (4byte) writes, but his hardware decoding results in a 16 byte 
> stride.  The result is setting REG_SIZE to 4 gives him the r/w access he 
> needs (32 bits), but fails the stride.  Setting it to 16 gives him the 
> stride he needs, but a 16 byte register is nonsensical and breaks the 
> software.  My guess is Shinya needs another customization dial (I'm 
> making this up) "REG_STRIDE" = 16 as well as "REG_SIZE" = 4.

Let me clarify:

* My processor MIPS 4KEc doesn't have limitations on byte accesses.

* My address decoder in UART block, can't handle +1/+2/+3 offsets
  properly.  This is the reason I need 32-bit word accesses.  And
  16 byte stride is not related here.

Thanks again for you translation,

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Shinya Kuribayashi wrote:
> As for my hardware, however, this still doesn't work. My processor
> (MIPS 4KEc) of couse supports byte read/write, on the other hand,
> the address decoder at UART module can not handle byte addresses
> properly; all byte read/write accesses with +1/+2/+3 offset, will
> be round-down to +0.  Therefore, I can take 'offset +0x3' option.

Oops,                              s/can take/can not take/.

> This is the reasony I said 'my hardware requires 32-bit word access
> to NS16550 registers'.

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hi Shinya,

>>>> I see.  Actually I was looking a lot at the Linux driver but was hoping
>>>> that we could away without introducing serial_{in,out}...
>>> In my horrible opinion, the combinations of base addres + reg_shift
>>> + iotype (char, long, or whatever), are simpler, more configurable,
>>> more slid, easy to use, than what we used to have or what you
>>> consolidated this time.
>>
>> You lost me here.
>>
>> You truly consider
>>
>> static unsigned int serial_in(struct uart_8250_port *up, int offset)
> [snip]
>> }
>>
>> to be "simpler and more solid" readb(struct->field) (which is
>> effectively what we have in the current implementation)?  You consider
>> "more configurable" to be a good in its own?
>
> Yes.

Wow.  As a rhetorical question - where do you actually draw the line if
you consider configurable to be a good in its own?  Shouldn't we then
have configuration options for UARTs who are attached bit-reversed on
the databus also?  And an option for a bit-shift in the data itself?

>> If your answers to these questions are yes, then we have different ideas
>> of writing code.
>
> Please make sure we don't need full serial_{in,out} port from Linux
> as-is.  As suggested, the combinations of base addres + reg_shift +
> iotype, are rather reasonable to support various kind of hardwares.
>
> I mean we need something like this:
>
> static unsigned int serial_in(struct uart_8250_port *up, int offset)
> {
>        unsigned int tmp;
>        int ret;
>        offset = map_8250_in_reg(up, offset) << up->port.regshift;
>
>        switch (up->port.iotype) {
>        case UPIO_MEM:
>                ret = readb(up->port.membase + offset);
>                break;
>
>        case UPIO_MEM32:
>                ret = readl(up->port.membase + offset);
>                break;
>
>        default:
>                ret = inb(up->port.iobase + offset);
>                break;
>        }
>        return ret;
> }
>
> Its implementation must be differed in U-Boot code, of course.

[...]
> I admit the address decoder in my UART hardware is weird and needs
> special configuration,  but this is not just for my case, it's not
> unusual.
>
> There're various kind of hardwares in the world, and there're many
> U-Boot ports which can not be pushed to upstream for various reasons.
> We can easily ignore such boards of course, but it would be very nice
> for U-Boot if it could provide easy configurable drivers and could
> support as many hardwares as possible.

Currently it seems that all in-tree boards can be accomodated with the
construct that I suggested.  I am not at all sure that we want code
which is only used by out-of-tree ports.

Post the port and we can rediscuss new code.

Cheers
  Detlev

-- 
[Linux] USB consoles was a  bad hack written on a drunken dare.   I'm still
constantly amazed that the thing even works at all, let alone the fact that
people are actually using it :) 
                            -- Greg KH <20090420225358.GC28697@kroah.com>
--
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] include/ns16550.h: Unify structure declaration for registers

[Scott Wood]

On Mon, May 04, 2009 at 05:40:37PM +0200, Detlev Zundel wrote:
> >> static unsigned int serial_in(struct uart_8250_port *up, int offset)
> > [snip]
> >> }
> >>
> >> to be "simpler and more solid" readb(struct->field) (which is
> >> effectively what we have in the current implementation)?  You consider
> >> "more configurable" to be a good in its own?

That's not "configuration", that's "separation of concerns".  Why
hardcode the bus mapping in code that only cares about the higher level
register interface?

> > Yes.
> 
> Wow.  As a rhetorical question - where do you actually draw the line if
> you consider configurable to be a good in its own?  Shouldn't we then
> have configuration options for UARTs who are attached bit-reversed on
> the databus also?  And an option for a bit-shift in the data itself?

If there's such hardware out there that needs to be supported, yes. :-)

-Scott

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Wolfgang Denk]

Dear Detlev,

In message <m2zldsanbe.fsf@ohwell.denx.de> you wrote:
> 
> >> to be "simpler and more solid" readb(struct->field) (which is
> >> effectively what we have in the current implementation)?  You consider
> >> "more configurable" to be a good in its own?
> >
> > Yes.
> 
> Wow.  As a rhetorical question - where do you actually draw the line if
> you consider configurable to be a good in its own?  Shouldn't we then

This is actually an easy question.

We draw the line such that

1) all boards that are supported in the mainline code continue to work
   at least not worse than they did before.
and
2) reasonable hardware configurations that can be foreseen are either
   supported out of the box or can be added with only little effort
   (i. e. without effort that is close to a redesign).

In the current situation, we have an out-of-tree port *plus* a
hardware design that can be considered broken.

Both are very good reasons not to spend special efforts to support
such a board.


> have configuration options for UARTs who are attached bit-reversed on
> the databus also?  And an option for a bit-shift in the data itself?

No, of course not.


> > I admit the address decoder in my UART hardware is weird and needs
> > special configuration,  but this is not just for my case, it's not
> > unusual.

Well, here I disagree with Shinya Kuribayashi - the fact that we don't
have *any* board with such requirements in mainline, and that it never
popped up before, is pretty clear proof that such a configuration is
highly unusual (or, with less diplomatic words, broken).

> > There're various kind of hardwares in the world, and there're many
> > U-Boot ports which can not be pushed to upstream for various reasons.

I can not imagine any reasons why a U-Boot port could not be pushed
upstream. It's GPL software after all...

> > We can easily ignore such boards of course, but it would be very nice
> > for U-Boot if it could provide easy configurable drivers and could
> > support as many hardwares as possible.
> 
> Currently it seems that all in-tree boards can be accomodated with the
> construct that I suggested.  I am not at all sure that we want code
> which is only used by out-of-tree ports.
> 
> Post the port and we can rediscuss new code.

Full ACK here.  It makes no sense to spend time and resources on sup-
porting out-of-tree ports on broken hardware.

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
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Shinya Kuribayashi]

Wolfgang Denk wrote:
>>> We can easily ignore such boards of course, but it would be very nice
>>> for U-Boot if it could provide easy configurable drivers and could
>>> support as many hardwares as possible.
>> Currently it seems that all in-tree boards can be accomodated with the
>> construct that I suggested.  I am not at all sure that we want code
>> which is only used by out-of-tree ports.
>>
>> Post the port and we can rediscuss new code.
> 
> Full ACK here.  It makes no sense to spend time and resources on sup-
> porting out-of-tree ports on broken hardware.

Ok, that's fine by me.

Thanks for your comments,

  Shinya

[U-Boot] [PATCH] include/ns16550.h: Unify structure declaration for registers

[Detlev Zundel]

Hi Shinya,

> Wolfgang Denk wrote:
>>>> We can easily ignore such boards of course, but it would be very nice
>>>> for U-Boot if it could provide easy configurable drivers and could
>>>> support as many hardwares as possible.
>>> Currently it seems that all in-tree boards can be accomodated with the
>>> construct that I suggested.  I am not at all sure that we want code
>>> which is only used by out-of-tree ports.
>>>
>>> Post the port and we can rediscuss new code.
>>
>> Full ACK here.  It makes no sense to spend time and resources on sup-
>> porting out-of-tree ports on broken hardware.
>
> Ok, that's fine by me.

That's by the way not much different to the state before my change.  You
always had local changes as there was never support for 16-byte strides.
This time I even posted a pretty short patch which - as you said - works
for your board.

Cheers
  Detlev

-- 
I shall be telling this with a sigh / Somewhere ages and ages hence: /
Two roads diverged in a wood, and I-- / I took the one less traveled by, /
And that has made all the difference.              -- Robert Frost
--
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