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: d...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
