Hello. Thomas Koeller wrote:
+ [PORT_RM9000] = { + .name = "RM9000", + .fifo_size = 16, + .tx_loadsz = 16, + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10, + .flags = UART_CAP_FIFO, + }, };
What was the point of introducing the separate port type if its settings are the same as for PORT_16550A?
I was under the impression that for every serial port hardware that is not exactly identical to one of the standard types, a distinct port type is required to be able to write code that takes care of its peculiarities. But from what you have written so far I conclude that this was a misconception.
Yes, I see one case where the distinct port type serves its purpose -- iotype could have been used instead anyway.
@@ -289,6 +296,36 @@ static inline int map_8250_out_reg(struc return au_io_out_map[offset]; } +#elif defined (CONFIG_SERIAL_8250_RM9K) + +static const u8 + regmap_in[8] = { + [UART_RX] = 0x00, + [UART_IER] = 0x0c, + [UART_IIR] = 0x14, + [UART_LCR] = 0x1c, + [UART_MCR] = 0x20, + [UART_LSR] = 0x24, + [UART_MSR] = 0x28, + [UART_SCR] = 0x2c + }, + regmap_out[8] = { + [UART_TX] = 0x04, + [UART_IER] = 0x0c, + [UART_FCR] = 0x18, + [UART_LCR] = 0x1c, + [UART_MCR] = 0x20, + [UART_LSR] = 0x24, + [UART_MSR] = 0x28, + [UART_SCR] = 0x2c + };
I guess you're using regshift == 0?
Yes.
Well, regshift of 2 seems more fitting for the 32-bit registers. This is not principal but using 0 regshift don't actually buy anything -- the shift will be perfomed anyway.
+ +#define map_8250_in_reg(up, offset) \ + (((up)->port.type == PORT_RM9000) ? regmap_in[offset] : (offset)) +#define map_8250_out_reg(up, offset) \ + (((up)->port.type == PORT_RM9000) ? regmap_out[offset] : (offset)) + +
Why you're not using specific iotype for RM9000 UARTs?
Because I did not realize that this was necessary. The device registers are
This is strange as you had an opposite example before your eyes.
ioremapped, and so the standard UPIO_MEM32 seemed the right thing to use. I
It is not.
will return to this topic further down.
So, read on... :-)
@@ -374,21 +411,21 @@ #define serial_inp(up, offset) serial_i
[...]
-#ifdef CONFIG_SERIAL_8250_AU1X00 +#if defined (CONFIG_SERIAL_8250_AU1X00) /* Au1x00 haven't got a standard divisor latch */ -static int serial_dl_read(struct uart_8250_port *up) +static unsigned int serial_dl_read(struct uart_8250_port *up) { if (up->port.iotype == UPIO_AU) return __raw_readl(up->port.membase + 0x28); @@ -396,13 +433,26 @@ static int serial_dl_read(struct uart_82 return _serial_dl_read(up); } -static void serial_dl_write(struct uart_8250_port *up, int value) +static void serial_dl_write(struct uart_8250_port *up, unsigned int value) { if (up->port.iotype == UPIO_AU) __raw_writel(value, up->port.membase + 0x28); else _serial_dl_write(up, value); } +#elif defined (CONFIG_SERIAL_8250_RM9K) +static inline unsigned int serial_dl_read(struct uart_8250_port *up) +{ + return + ((readl(up->port.membase + 0x10) << 8) | + (readl(up->port.membase + 0x08) & 0xff)) & 0xffff; +} + +static inline void serial_dl_write(struct uart_8250_port *up, unsigned int value) +{ + writel(value, up->port.membase + 0x08); + writel(value >> 8, up->port.membase + 0x10); +}
And why this doesn't check for up->port.type == PORT_RM9000 first? This way it won't work with any compatible UARTs anymore. This is wrong.
Because it is inside a conditional block already. I now realize that even if the driver is configured for some special silicon it still has to support the standard types, something that escaped me when I started to write the code.
Yes. And it's after you have yourself pointed out the compatibility issues. :-)
@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po */ static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p) { - unsigned char old_dll, old_dlm, old_lcr; - unsigned int id; + unsigned char old_lcr; + unsigned int id, old_dl; old_lcr = serial_inp(p, UART_LCR); serial_outp(p, UART_LCR, UART_LCR_DLAB); + old_dl = _serial_dl_read(p); - old_dll = serial_inp(p, UART_DLL); - old_dlm = serial_inp(p, UART_DLM); - - serial_outp(p, UART_DLL, 0); - serial_outp(p, UART_DLM, 0); - - id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8; + serial_dl_write(p, 0); + id = serial_dl_read(p); - serial_outp(p, UART_DLL, old_dll); - serial_outp(p, UART_DLM, old_dlm); + serial_dl_write(p, old_dl); serial_outp(p, UART_LCR, old_lcr); return id;
Not sure the autoconfig code was intended for half-compatible UARTs. Note that it sets up->port.type as its result. However, your change seems correct, it just have nothing to do with RM9000.
Should I factor out this part and create a separate patch for it?
Now this is up to Russel. :-)
As a side note, I think that the code that sets DLAB before and resets it after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.
BTW, I guess for RM9000 it should be also reserved?
@@ -1138,8 +1183,11 @@ static void serial8250_start_tx(struct u if (up->bugs & UART_BUG_TXEN) { unsigned char lsr, iir; lsr = serial_in(up, UART_LSR); - iir = serial_in(up, UART_IIR); - if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) + iir = serial_in(up, UART_IIR) & 0x0f; + if ((up->port.type == PORT_RM9000) ? + (lsr & UART_LSR_THRE && + (iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) : + (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)) transmit_chars(up); } }
It would be good to clarify why this is needed...
The RM9000 serial h/w also needs to be kicked if a transmitter holding register empty interrupt is pending. Oh, and no need to tell me, I realize that I have to deal with the standard case here as well...
Does RM9000 have UART_BUG_TXEN flag set? Note that this code will only execute in such case.
I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w
No, it's intended as just a method of accessing device registers.
implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine? In my
UPIO_MEM32 doesn't actually cover your case as it corresponds to the UART with the fully 8250-compatible register set, just having 32-bit registers instead of the usual 8-bit ones. RM9000 is clearly not fully compatible to 8250 in regard to the register addresses since it has RX/TX regs, FCR and the divisor latch mapped to the separate addresses, just like Alchemy UART. And I stressed that it's the main issue with this
UART's compatibility to 8250 in my first followup.
case, UPIO_AU might be the best choice,
Alchemy UARTs have *different* address mapping, so UPIO_AU clearly *cannot* be used for RM9000 UART.
as __raw_readl() and __raw_writel() are insensitive to CONFIG_SWAP_IO_SPACE, and that is what I want.
Why you've used readl() and writel() then, may I ask? :-)
Would I still need to invent UPIO_RM9K,
Yes.
just to have a distinct iotype, and be able to do 'if (up->port.iotype == UPIO_RM9K)'
A good "just to".
where I now use 'if (up->port.type == PORT_RM9000)'? That seems a bit weird.
Why?
Thomas
WBR, Sergei
