serial: 8250: Fix reporting real baudrate value in c_ospeed field

PATCH In review Pali Rohár 2 participants 3 messages Sep 27, 2021 TTY layer and serial drivers

lore

Share & tools

lore

Pali Rohár

author

2021-09-27 09:38 UTC #0 lore raw

PATCH 1/1 serial: 8250: Fix reporting real baudrate value in c_ospeed field

In most cases it is not possible to set exact baudrate value to hardware.

So fix reporting real baudrate value which was set to hardware via c_ospeed termios field. It can be retrieved by ioctl(TCGETS2) from userspace.

Real baudrate value is calculated from chosen hardware divisor and base clock. It is implemented in a new function serial8250_compute_baud_rate() which is inverse of serial8250_get_divisor() function.

With this change is fixed also UART timeout value (it is updated via uart_update_timeout() function), which is calculated from the now fixed baudrate value too.

Signed-off-by Pali Rohár <[email protected]>

Cc [email protected]

---

I have tested this change on device with 8250 compatible UART. I have not tested it on PORT_NPCM nor on UPF_MAGIC_MULTIPLIER hardware, as I do not have such.

Tested device has 250 MHz base clock for 8250 UART. When I set baudrate to 115200, then ioctl(TCGETS2) reported B115200 CBAUD flag and value 114890 in c_ospeed field.

This result is correct as HW for baudrate 115200 is using UART divisor 136, which results in correct reported baudrate: 250000000/(16*136) = 114890

---
 drivers/tty/serial/8250/8250_port.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/tty/serial/8250/8250_port.c +17 -0

index 66374704747e..dc6900b2daa8 100644
@@ -2584,6 +2584,19 @@ static unsigned int serial8250_get_divisor(struct uart_port *port,
 	return serial8250_do_get_divisor(port, baud, frac);
 }
 
+static unsigned int serial8250_compute_baud_rate(struct uart_port *port,
+						 unsigned int quot)
+{
+	if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8001)
+		return port->uartclk / 4;
+	else if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8002)
+		return port->uartclk / 8;
+	else if (port->type == PORT_NPCM)
+		return DIV_ROUND_CLOSEST(port->uartclk - 2 * (quot + 2), 16 * (quot + 2));
+	else
+		return DIV_ROUND_CLOSEST(port->uartclk, 16 * quot);
+}
+
 static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
 					    tcflag_t c_cflag)
 {

@@ -2714,11 +2727,14 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 
 	baud = serial8250_get_baud_rate(port, termios, NULL);
 	quot = serial8250_get_divisor(port, baud, &frac);
+	baud = serial8250_compute_baud_rate(port, quot);
 
 	serial8250_rpm_get(up);
 	spin_lock_irqsave(&port->lock, flags);
 
 	uart_update_timeout(port, termios->c_cflag, baud);
+	if (tty_termios_baud_rate(termios))
+		tty_termios_encode_baud_rate(termios, baud, baud);
 
 	serial8250_set_divisor(port, baud, quot, frac);
 	serial_port_out(port, UART_LCR, up->lcr);

@@ -2750,6 +2766,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	baud = serial8250_get_baud_rate(port, termios, old);
 	quot = serial8250_get_divisor(port, baud, &frac);
+	baud = serial8250_compute_baud_rate(port, quot);
 
 	/*
 	 * Ok, we're now changing the port state.  Do it with

Signature · 1 line

2.20.1

Greg Kroah-Hartman

maintainer

2021-09-27 09:55 UTC #1 lore raw

Quoted 1 line · Pali Rohár

In most cases it is not possible to set exact baudrate value to hardware.

Why not?

Quoted 10 lines

So fix reporting real baudrate value which was set to hardware via c_ospeed termios field. It can be retrieved by ioctl(TCGETS2) from userspace.
Real baudrate value is calculated from chosen hardware divisor and base clock. It is implemented in a new function serial8250_compute_baud_rate() which is inverse of serial8250_get_divisor() function.
With this change is fixed also UART timeout value (it is updated via uart_update_timeout() function), which is calculated from the now fixed baudrate value too.

I can not parse this sentence, sorry. Can you try to rephrase it differently?

Quoted 9 lines

Signed-off-by Pali Rohár <[email protected]>
Cc [email protected]
---
I have tested this change on device with 8250 compatible UART. I have not tested it on PORT_NPCM nor on UPF_MAGIC_MULTIPLIER hardware, as I do not have such.

Quoted 6 lines

Tested device has 250 MHz base clock for 8250 UART. When I set baudrate to 115200, then ioctl(TCGETS2) reported B115200 CBAUD flag and value 114890 in c_ospeed field.
This result is correct as HW for baudrate 115200 is using UART divisor 136, which results in correct reported baudrate: 250000000/(16*136) = 114890

So is this going to break all the userspace tools that set a baud rate and then read it back and get a different number than what they thought they set?

That feels very dangerous. Why does this matter?

Quoted 14 lines

---
 drivers/tty/serial/8250/8250_port.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/tty/serial/8250/8250_port.c +2 -0

index 66374704747e..dc6900b2daa8 100644
@@ -2584,6 +2584,19 @@ static unsigned int serial8250_get_divisor(struct uart_port *port,
 	return serial8250_do_get_divisor(port, baud, frac);
 }
 
+static unsigned int serial8250_compute_baud_rate(struct uart_port *port,
+						 unsigned int quot)

drivers/tty/serial/8250/8250_port.c static unsigned int serial8250_get_divisor(struct uart_port *port, · line 2598

Quoted 10 lines

drivers/tty/serial/8250/8250_port.c +10 -0

+{
+	if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8001)
+		return port->uartclk / 4;
+	else if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8002)
+		return port->uartclk / 8;
+	else if (port->type == PORT_NPCM)
+		return DIV_ROUND_CLOSEST(port->uartclk - 2 * (quot + 2), 16 * (quot + 2));
+	else
+		return DIV_ROUND_CLOSEST(port->uartclk, 16 * quot);
+}

Where did these formulas come from?

thanks,

greg k-h

Pali Rohár

author

2021-09-27 10:21 UTC #2 lore raw

Quoted 4 lines · On Monday 27 September 2021 11:55:43 Greg Kroah-Hartman wrote:

Quoted 1 line · Pali Rohár
In most cases it is not possible to set exact baudrate value to hardware.
Why not?

8250 allows to set only divisor and the final baudrate which hw use is:

baudrate = clock / (16*divisor)

In most cases base UART clock is fixed, so you can set only baudrates for divisors 1, 2, 3, ..., N.

Quoted 7 lines

Quoted 7 lines
So fix reporting real baudrate value which was set to hardware via c_ospeed termios field. It can be retrieved by ioctl(TCGETS2) from userspace.
Real baudrate value is calculated from chosen hardware divisor and base clock. It is implemented in a new function serial8250_compute_baud_rate() which is inverse of serial8250_get_divisor() function.

Quoted 6 lines

Quoted 3 lines
With this change is fixed also UART timeout value (it is updated via uart_update_timeout() function), which is calculated from the now fixed baudrate value too.
I can not parse this sentence, sorry. Can you try to rephrase it differently?

Kernel uses UART timeout value (stored in port->timeout variable) for e.g. testing if TX fifo is empty. And uart_update_timeout() function calculates this timeout from baudrate. Hence this change fixes UART timeout.

Quoted 13 lines

Quoted 13 lines
Signed-off-by Pali Rohár <[email protected]>
Cc [email protected]
---
I have tested this change on device with 8250 compatible UART. I have not tested it on PORT_NPCM nor on UPF_MAGIC_MULTIPLIER hardware, as I do not have such.
Tested device has 250 MHz base clock for 8250 UART. When I set baudrate to 115200, then ioctl(TCGETS2) reported B115200 CBAUD flag and value 114890 in c_ospeed field.

Quoted 6 lines

Quoted 2 lines
This result is correct as HW for baudrate 115200 is using UART divisor 136, which results in correct reported baudrate: 250000000/(16*136) = 114890
So is this going to break all the userspace tools that set a baud rate and then read it back and get a different number than what they thought they set?

As I wrote in above test, CBAUD Bnnn flag (in my test case B115200) was still set. So userspace is not going to be broken. Kernel functions in drivers/tty/tty_baudrate.c reports back to userspace original baudrate value if is in 2% tolerance.

So, in case userspace ask for baudrate value which is not possible to set (with 2% tolerance) then kernel would report in CBAUD flag different value. But in this case UART transfer does not work as 2-3% is the threshold when transmitter and receiver are de-synced and are not able correctly receive any byte.

So with this change, userspace is notified in case kernel was not able to set baudrate value which userspace asked and can report to user reason why UART transfer does not work.

All above was about CBAUD flag, as this is the only supported kernel flag by glibc.

Extended ioctl(TCGETS2) provides additional field c_ospeed, for which glibc does not have API yet. This field is supposed to report current real baudrate value, not rounded to nearest Bnnn value.

And more serial drivers already reports it. So if you set baudrate 115200 then more serial drivers in c_ospeed report real value (e.g. 114890). This is very useful for userspace as it can decide if the current baudrate value is suitable for its limits. E.g. you may construct UART receiver which would be able to detect data even if they are only in 5% tolerance. So you set baudrate on transmitter and then check if value set by kernel to hw is in tolerance or not.

Quoted 1 line

That feels very dangerous. Why does this matter?

Current problem with this driver is that if userspace ask for baudrate B and for value B there is no divisor D, then this drivers sets some nearest divisor D' to hw. But divisor D' drivers different baudrate and userspace did not receive any information that asked B is not possible to set.

So, I do not think that this is dangerous. It just starts reporting correct value via TCGETS2 API, like some other serial drivers are already doing.

Quoted 12 lines

---
 drivers/tty/serial/8250/8250_port.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/tty/serial/8250/8250_port.c +0 -0

index 66374704747e..dc6900b2daa8 100644
@@ -2584,6 +2584,19 @@ static unsigned int serial8250_get_divisor(struct uart_port *port,
 	return serial8250_do_get_divisor(port, baud, frac);
 }

Quoted 14 lines

drivers/tty/serial/8250/8250_port.c static unsigned int serial8250_get_divisor(struct uart_port *port, · line 2598

Quoted 12 lines

drivers/tty/serial/8250/8250_port.c +12 -0

+static unsigned int serial8250_compute_baud_rate(struct uart_port *port,
+						 unsigned int quot)
+{
+	if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8001)
+		return port->uartclk / 4;
+	else if ((port->flags & UPF_MAGIC_MULTIPLIER) && quot == 0x8002)
+		return port->uartclk / 8;
+	else if (port->type == PORT_NPCM)
+		return DIV_ROUND_CLOSEST(port->uartclk - 2 * (quot + 2), 16 * (quot + 2));
+	else
+		return DIV_ROUND_CLOSEST(port->uartclk, 16 * quot);
+}

Where did these formulas come from?

I wrote it in commit message. I constructed inverse of function serial8250_get_divisor() which calculated divisor for UART HW, which is what configures baudrate.

Quoted 3 lines

thanks,
greg k-h