On Fri, 1 Sep 2017, Greg KH wrote:
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.
[RE] Ok will change the subject to [PATCH v3] serial: bcm63xx: fix timing
issue. Also remove sections.
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.
[RE]: Will leave long lines with no wrap on console output.
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
[RE]: This was a bad formatting/cp/typo, my bad, will remove
duplicate.
---
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.
[RE]: Will do a new v2 patch (assume this patch will be rejected since it
has no v2 on it) should only be a diff between v1.
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? :)
[RE]: It is true that after this is a disable and flush so this will not
be seen. I will remove the if() and dev_err(). It will be a silent
failure.
Thanks,
Russell
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
