[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