[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