| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4C4E9064.6070001@stericsson.com>
Date: Tue, 27 Jul 2010 09:53:08 +0200
From: Philippe Langlais <philippe.langlais@...ricsson.com>
To: Alan Cox <alan@...rguk.ukuu.org.uk>
Cc: "gregkh@...e.de" <gregkh@...e.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ludovic BARRE <ludovic.barre@...ricsson.com>,
Vincent Guittot <vincent.guittot@...ricsson.com>
Subject: Re: patch "U6715 8250 serial like driver" added to gregkh-2.6 tree
Hi,
On 07/23/10 10:01, Alan Cox wrote:
> On Thu, 22 Jul 2010 16:57:57 -0700
> <gregkh@...e.de> 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,
>> + .flags = UART_CAP_FIFO | UART_CAP_AFE,
>> +#else
>> .fifo_size = 16,
>> .tx_loadsz = 16,
>> - .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>> .flags = UART_CAP_FIFO,
>> +#endif
>> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>> },
>>
> 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;
+#else
+ up->port.type = PORT_U6_16550A;
+#endif
>
>
>> @@ -2268,6 +2274,13 @@ serial8250_set_termios(struct uart_port
>> /*
>> * Ask the core to calculate the divisor for us.
>> */
>> +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
>> + baud = uart_get_baud_rate(port, termios, old, 0,
>> + CONFIG_SERIAL_8250_CUSTOM_MAX_BAUDRATE);
>> + /* Calculate the new uart clock frequency if it is tunable */
>> + port->uartclk = serial8250_get_custom_clock(port, baud);
>> +#endif
>> +
>> 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;
>> +#endif
>>
> 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));
>>
> pr_warn()
>
> 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.
>
OK
>
>
>> +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 ?
>
Yes
>
>
>> +config SERIAL_8250_CUSTOM_CLOCK
>> + 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.
>
Yes
>
>
>> +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
>> +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);
>> +#endif
>>
> This shouldn't need ifdefs in the header
>
Yes
Regards
Philippe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists