Re: patch "U6715 8250 serial like driver" added to gregkh-2.6 tree

From: Philippe Langlais
Date: Tue Jul 27 2010 - 03:54:46 EST


On 07/23/10 10:01, Alan Cox wrote:
On Thu, 22 Jul 2010 16:57:57 -0700
<gregkh@xxxxxxx> wrote:

This is a note to let you know that I've just added the patch titled
NAK this

I've nakked several similar patches already from people trying to
convolute the 8250 driver even further, doubly so when the patch
redefines properties of existing chips according to a compile time option

[PORT_16550A] = {
.name = "16550A",
+#if defined(CONFIG_SERIAL_8250_U6XXX)
+ .fifo_size = 64,
+ .tx_loadsz = 64,
.fifo_size = 16,
.tx_loadsz = 16,
.flags = UART_CAP_FIFO,
Define yourself a new port type
I agree, but the problem is to recognize our 16550A IP vs others.
Until now I can't find a differentiator with other 16550A and this IP
is only use in our U6XXX SoCs family.
To avoid mis recognition can I patch autoconfig_16550a() in 8250.c like this:

static void autoconfig_16550a(struct uart_8250_port *up)
unsigned char status1, status2;
unsigned int iersave;

+#if !defined(CONFIG_PLAT_U6XXX)
up->port.type = PORT_16550A;
+ up->port.type = PORT_U6_16550A;

@@ -2268,6 +2274,13 @@ serial8250_set_termios(struct uart_port
* Ask the core to calculate the divisor for us.
+ baud = uart_get_baud_rate(port, termios, old, 0,
+ /* Calculate the new uart clock frequency if it is tunable */
+ port->uartclk = serial8250_get_custom_clock(port, baud);
baud = uart_get_baud_rate(port, termios, old,
port->uartclk / 16 / 0xffff,
port->uartclk / 16);

I think what you need to keep this clean is to create a
port->set_termios() for the 8250 port (akin to
port->serial_in/serial_out) which does the specials you need each end and
calls the standard serial8250_set_termios)
I don't think so, call a function which compute clock each time
we access UART registers isn't optimal, another function to overload ?
@@ -2298,6 +2311,13 @@ serial8250_set_termios(struct uart_port
up->mcr&= ~UART_MCR_AFE;
if (termios->c_cflag& CRTSCTS)
up->mcr |= UART_MCR_AFE;
+#if defined(CONFIG_SERIAL_8250_U6XXX)
+ /**
+ * When AFE is active, let the HW handle the stop/restart TX
+ * upon CTS change. It reacts much quicker than the SW driver.
+ */
+ port->flags&= ~ASYNC_CTS_FLOW;
Why do you need to clear this flag when doing so ?
I don't know if it's useful, I'll withdraw this & make tests.

+unsigned int serial8250_enable_clock(struct uart_port *port)
+ struct u6_uart *uart_u6 = port->private_data;
+ if (!uart_u6)
+ return uart_enable_clock(port);
+ if (IS_ERR(uart_u6->uartClk)) {
+ printk(KERN_WARNING "%s - uart clock failed error:%ld\n",
+ __func__, PTR_ERR(uart_u6->uartClk));

Also as the functions are specific to the u6175 can you use u6715_ names
so any printk, traceback and the like is obvious in where to look.

+unsigned int serial8250_get_custom_clock(struct uart_port *port,
+ unsigned int baud)
+ switch (baud) {
+ case 3250000:
+ return 52000000;
+ case 2000000:
+ return 32000000;
+ case 1843200:
+ return 29491200;
+ case 921600:
+ return 14745600;
+ default:
+ return 7372800;
+ }
Baud rates are arbitary values, so surely this should doing range checks ?

+ bool "Support serial ports with tunable input clock frequency"
+ depends on SERIAL_8250_EXTENDED&& SERIAL_8250_U6XXX
+ default y
+ help
+ Say Y here if your platform has specific registers to change UART clock frequency.
Except the patch you've posted is actually a patch for a U6715 not
generic tunable input clock support, so this Kconfig option/help will
confuse people. Better to describe it as U6715 support.

+unsigned int serial8250_enable_clock(struct uart_port *port);
+unsigned int serial8250_disable_clock(struct uart_port *port);
+unsigned int serial8250_get_custom_clock(struct uart_port *port,
+ unsigned int baud);
+void serial8250_set_custom_clock(struct uart_port *port);
This shouldn't need ifdefs in the header

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at