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

[Detlev Zundel <dzu@denx.de>]

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