lkml.org 
[lkml]   [2010]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: 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@suse.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


    \
     
     \ /
      Last update: 2010-07-27 09:57    [W:0.082 / U:29.640 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site