[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