Hi Andy,

On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote:

On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:

quoted

Some platforms can be designed in a way so the UART port reference clock
might be asynchronously changed at some point. In Baikal-T1 SoC this may
happen due to the reference clock being shared between two UART ports, on
the Allwinner SoC the reference clock is derived from the CPU clock, so
any CPU frequency change should get to be known/reflected by/in the UART
controller as well. But it's not enough to just update the
uart_port->uartclk field of the corresponding UART port, the 8250
controller reference clock divisor should be altered so to preserve
current baud rate setting. All of these things is done in a coherent
way by calling the serial8250_update_uartclk() method provided in this
patch. Though note that it isn't supposed to be called from within the
UART port callbacks because the locks using to the protect the UART port
data are already taken in there.

...

quoted

+/*
+ * Note in order to avoid the tty port mutex deadlock don't use the next method
+ * within the uart port callbacks. Primarily it's supposed to be utilized to
+ * handle a sudden reference clock rate change.
+ */
+void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned int baud, quot, frac = 0;
+	struct ktermios *termios;
+	unsigned long flags;
+
+	mutex_lock(&port->state->port.mutex);
+
+	if (port->uartclk == uartclk)
+		goto out_lock;
+
+	port->uartclk = uartclk;
+	termios = &port->state->port.tty->termios;
+
+	baud = serial8250_get_baud_rate(port, termios, NULL);
+	quot = serial8250_get_divisor(port, baud, &frac);
+
+	serial8250_rpm_get(up);
+	spin_lock_irqsave(&port->lock, flags);
+
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	serial8250_set_divisor(port, baud, quot, frac);
+	serial_port_out(port, UART_LCR, up->lcr);
+	serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+	serial8250_rpm_put(up);
+
+out_lock:
+	mutex_unlock(&port->state->port.mutex);

While looking for something else I have stumbled over this function.
My Q is, since it has some duplications with
serial8250_do_set_termios(), can we actually call the latter (or
derevative that can be called in both) in the above code instead of
duplicating some lines?

	if (port UART clock has to be updated)
	  call (unlocked version of) serial8250_do_set_termios()

Serge, what do you think?

What an old thread you've digged out.)

Well, AFAIR I didn't create a common baud-rate/clock-update method
because the baud-rate change was just a two stages action:
1. calculate divisor+quot couple based on the new clock,
2. update the divisor+quot (+ update the timeout).
The first stage didn't need to have the IRQsafe lock being held and
the runtime-PM being enabled, meanwhile the later one needed those.
So unless the nested locking or try-lock-based pattern is implemented
each stage required dedicated function introduced, which would have
been an overkill for that. But even if I got to implement the
try-lock-based solution with a single function containing both stages
I still couldn't avoid having the serial8250_get_baud_rate() and
serial8250_get_divisor() methods executed in the atomic context, which
isn't required for them and which would needlessly pro-long the CPU
executing with the IRQs disabled. As you well know it's better to
speed up the atomic context execution as much as possible. 

Secondly I didn't know much about the tty/serial subsystem internals
back then. So I was afraid to break some parts I didn't aware of if
the baud-rate/ref-clock change code had some implicit dependencies
from the surrounding code and vice-versa (like the LCR DLAB flag
state).

Finally frankly it didn't seem like that much worth bothering about.
Basically AFAICS there were only four methods which invocation I
would have needed to move to a separate function:

serial8250_get_baud_rate();
serial8250_get_divisor();
// spin-lock
uart_update_timeout(port, termios->c_cflag, baud);
serial8250_set_divisor(port, baud, quot, frac);
// spin-unlock

So I decided to take a simplest and safest path, and created a
dedicated method for the just the ref-clock updates case leaving the
baud-rate change task implemented in the framework of the standard
serial8250_do_set_termios() method.


Regarding doing vise-versa and calling the serial8250_do_set_termios()
method from serial8250_update_uartclk() instead. To be honest I didn't
consider that option. That might work though, but AFAICS the
serial8250_do_set_termios() function will do much more than it's
required in case if the ref-clock has changed.

-Serge(y)

quoted

+}

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel