[PATCH] serial: 8250_dw: verify clock rate in dw8250_set_termios

[PATCH] serial: 8250_dw: verify clock rate in dw8250_set_termios

[asheplyakov]

From: Alexey Sheplyakov <asheplyakov@basealt.ru>

Refuse to change the clock rate if clk_round_rate() returns
a rate which is way too off (i.e. by more than 1/16 from the one
necessary for a given baud rate). In particular this happens if
the requested rate is below the minimum supported by the clock.

Fixes the UART console on BE-M1000 SoC. Without this patch the
console gets garbled immediately after loading the driver.
dw8250_set_termios tries to configure the baud rate (115200),
and calls clk_round_rate to figure out the supported rate closest
to 1843200 Hz (which is 115200 * 16). However the (SoC-specific)
clock driver returns 4705882 Hz. This frequency is way too off,
hence after setting it the console gets garbled.

Signed-off-by: Alexey Sheplyakov <asheplyakov@basealt.ru>
Signed-off-by: Vadim V. Vlasov <vadim.vlasov@elpitech.ru>

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/tty/serial/8250/8250_dw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1769808031c5..ec7e8169c983 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -329,14 +329,15 @@ dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
 static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 			       struct ktermios *old)
 {
-	unsigned long newrate = tty_termios_baud_rate(termios) * 16;
+	unsigned long baud = tty_termios_baud_rate(termios);
+	unsigned long newrate = baud * 16;
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	long rate;
 	int ret;
 
 	clk_disable_unprepare(d->clk);
 	rate = clk_round_rate(d->clk, newrate);
-	if (rate > 0) {
+	if (rate > 0 && rate >= baud * 15 && rate <= baud * 17) {
 		/*
 		 * Note that any clock-notifer worker will block in
 		 * serial8250_update_uartclk() until we are done.
-- 
2.32.0

Re: [PATCH] serial: 8250_dw: verify clock rate in dw8250_set_termios

[Andy Shevchenko]

On Tue, Jan 11, 2022 at 05:28:47PM +0400, asheplyakov@basealt.ru wrote:
> From: Alexey Sheplyakov <asheplyakov@basealt.ru>

Thanks for your patch, my comments below.

> Refuse to change the clock rate if clk_round_rate() returns
> a rate which is way too off (i.e. by more than 1/16 from the one
> necessary for a given baud rate). In particular this happens if
> the requested rate is below the minimum supported by the clock.
> 
> Fixes the UART console on BE-M1000 SoC. Without this patch the
> console gets garbled immediately after loading the driver.
> dw8250_set_termios tries to configure the baud rate (115200),
> and calls clk_round_rate to figure out the supported rate closest
> to 1843200 Hz (which is 115200 * 16). However the (SoC-specific)
> clock driver returns 4705882 Hz. This frequency is way too off,
> hence after setting it the console gets garbled.

So, the root cause is to understand _why_ the clock provider is uncapable
to fulfil the request. Any investigation has been conducted?

> Signed-off-by: Alexey Sheplyakov <asheplyakov@basealt.ru>
> Signed-off-by: Vadim V. Vlasov <vadim.vlasov@elpitech.ru>

> 

Should be no blank lines in the tag block, but...

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>

...I recommend to use --cc parameter to git send-email instead of polluting
commit message.

...

> -	if (rate > 0) {
> +	if (rate > 0 && rate >= baud * 15 && rate <= baud * 17) {

It doesn't fell like a correct fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: verify clock rate in dw8250_set_termios

[Alexey Sheplyakov]

Hello, Andy,

On Tue, Jan 11, 2022 at 08:30:28PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 05:28:47PM +0400, asheplyakov@basealt.ru wrote:
> > From: Alexey Sheplyakov <asheplyakov@basealt.ru>
> 
> Thanks for your patch, my comments below.

Thanks for the comments.
 
> > Refuse to change the clock rate if clk_round_rate() returns
> > a rate which is way too off (i.e. by more than 1/16 from the one
> > necessary for a given baud rate). In particular this happens if
> > the requested rate is below the minimum supported by the clock.
> > 
> > Fixes the UART console on BE-M1000 SoC. Without this patch the
> > console gets garbled immediately after loading the driver.
> > dw8250_set_termios tries to configure the baud rate (115200),
> > and calls clk_round_rate to figure out the supported rate closest
> > to 1843200 Hz (which is 115200 * 16). However the (SoC-specific)
> > clock driver returns 4705882 Hz. This frequency is way too off,
> > hence after setting it the console gets garbled.
> 
> So, the root cause is to understand _why_ the clock provider is uncapable
> to fulfil the request. Any investigation has been conducted?

Yes. On BE-M1000 SoC Linux has no direct control over (most) clocks.
The registers of CMU (clock management unit) are accessible only from
the secure world, therefore clocks are managed by the firmware (ARM-TF).
Linux' driver is a shim which calls into the firmware. And that
4705882 Hz is exactly what is returned by firmware.

> > Signed-off-by: Alexey Sheplyakov <asheplyakov@basealt.ru>
> > Signed-off-by: Vadim V. Vlasov <vadim.vlasov@elpitech.ru>
> 
> > 
> 
> Should be no blank lines in the tag block, but...
> 
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ...I recommend to use --cc parameter to git send-email instead of polluting
> commit message.

Got it.
 
> ...
> 
> > -	if (rate > 0) {
> > +	if (rate > 0 && rate >= baud * 15 && rate <= baud * 17) {
> 
> It doesn't fell like a correct fix.

What is the correct way to check if the rate returned by clk_round_rate
makes sense for the (new) baud rate? See, clk_round_rate is supposed to
give a supported rate which is closest to the requested one. Usually it
appears to be "close enough" to the requested rate (or an error), however
(as far as I understand) there is no a formal requirement (and it's up
to the driver to decide how close is "close enough").

Or should we just hope that the clock provider does the right thing?

Best regards,
   Alexey

Re: [PATCH] serial: 8250_dw: verify clock rate in dw8250_set_termios

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 12:12:58AM +0400, Alexey Sheplyakov wrote:
> On Tue, Jan 11, 2022 at 08:30:28PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 11, 2022 at 05:28:47PM +0400, asheplyakov@basealt.ru wrote:

> > > Refuse to change the clock rate if clk_round_rate() returns
> > > a rate which is way too off (i.e. by more than 1/16 from the one
> > > necessary for a given baud rate). In particular this happens if
> > > the requested rate is below the minimum supported by the clock.
> > > 
> > > Fixes the UART console on BE-M1000 SoC. Without this patch the
> > > console gets garbled immediately after loading the driver.
> > > dw8250_set_termios tries to configure the baud rate (115200),
> > > and calls clk_round_rate to figure out the supported rate closest
> > > to 1843200 Hz (which is 115200 * 16). However the (SoC-specific)
> > > clock driver returns 4705882 Hz. This frequency is way too off,
> > > hence after setting it the console gets garbled.
> > 
> > So, the root cause is to understand _why_ the clock provider is uncapable
> > to fulfil the request. Any investigation has been conducted?
> 
> Yes. On BE-M1000 SoC Linux has no direct control over (most) clocks.
> The registers of CMU (clock management unit) are accessible only from
> the secure world, therefore clocks are managed by the firmware (ARM-TF).
> Linux' driver is a shim which calls into the firmware. And that
> 4705882 Hz is exactly what is returned by firmware.

> > > -	if (rate > 0) {
> > > +	if (rate > 0 && rate >= baud * 15 && rate <= baud * 17) {
> > 
> > It doesn't fell like a correct fix.
> 
> What is the correct way to check if the rate returned by clk_round_rate
> makes sense for the (new) baud rate? See, clk_round_rate is supposed to
> give a supported rate which is closest to the requested one. Usually it
> appears to be "close enough" to the requested rate (or an error), however
> (as far as I understand) there is no a formal requirement (and it's up
> to the driver to decide how close is "close enough").
> 
> Or should we just hope that the clock provider does the right thing?

Does this hardware even work? I mean is it possible to change baud rate?

To me the proper solution sounds like the driver on this hardware should
not use CCF approach, i.e. use only DLAB for baud rate and fixed rate clock
of the bus or so.

-- 
With Best Regards,
Andy Shevchenko