On Fri, Sep 01, 2017 at 06:50:24AM -0500, Russell Enderby wrote:
> PROBLEM ANALYSIS
> ================
> Problem occurs when changing the serial baud rate when setting up the
> new console.The bcm63xx driver does a disable and flush of the uart tx
> fifo while there is data still in the tx fifo.
>
> SOLUTION
> ========
> If the tx fifo still has data it is trying to send out, we need to wait
> until it is empty before disabling and flushing the uart.
While nice, you don't see formats like this in other commits in the
kernel, right?
Also, your subject is a bit odd, and long, and again, also not something
you see in the kernel history. How about:
[PATCH] serial: bcm63xx: fix timing issue
as it is, your subject implies that this is the case for all serial
connections, which it isn't.
>
> When we now go to change the uart parameters including speed we check
> if there is data currently in the tx fifo.If there is was mdelay(10)
> and check again.If it tries 3 times and still has data in it we just
> continue and sacrifice the tx fifo buffer.
>
> A cleaner and more preferred approach would be to remove :
> -spin_lock_irqsave()
> - bcm_uart_disable()
> - bcm_uart_flush()
>
> However it is not clear if the author put those in to fix another
> underlying issue.As a result this solution is a safer approach.
>
> BEFORE FIX
> [0.306000] 14e00520.serial: ttyS0 at MMIO 0x14e00520 (irq = 9,
> base_baud = 1687500) is a° 0.315000] console[ttyS0] enabled
>
> AFTER FIX
> [0.315000] 14e00520.serial: ttyS0 at MMIO 0x14e00520 (irq = 9,
> base_baud = 1687500) is a bcm63xx_uart [0.334000] console[ttyS0]
> enabled
I thought I said that these two log lines do not have to be wrapped, as
that's not the normal way for them. Only your text had to be, which you
did here, thanks.
>
> Signed - off - by : Russell Enderby <rte@xxxxxxx>
>
Why the ' ' here? And a blank line?
> Signed-off-by: Russell Enderby <rte@xxxxxxx>
Wait, you have two signed-off-by lines? Please
> ---
> drivers/tty/serial/bcm63xx_uart.c | 7 +++++++
> 1 file changed, 7 insertions(+)
As this is v2 of the patch, you need to say so in the subject, and put
what changed here below the --- line, as the documentation in the kernel
says to do.
>
> diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
> index 583c9a0..69cf55e 100644
> --- a/drivers/tty/serial/bcm63xx_uart.c
> +++ b/drivers/tty/serial/bcm63xx_uart.c
> @@ -507,9 +507,16 @@ static void bcm_uart_set_termios(struct uart_port *port,
> {
> unsigned int ctl, baud, quot, ier;
> unsigned long flags;
> + int tries;
>
> spin_lock_irqsave(&port->lock, flags);
>
> + /* Drain the hot tub fully before we power it off for the winter. */
> + for (tries = 3; !bcm_uart_tx_empty(port) && tries; tries--)
> + mdelay(10);
> + if (!tries)
> + dev_err(port->dev, "Unable to drain serial transmitter.\n");
What can a user do if they see this error?
And as it is coming from the console, and you just added more text to
the console about how the console isn't working, what's the odds that
this message is not going to go anywhere? :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
