On 19/01/18 16:02, Andy Shevchenko wrote:
> The commit
>
> de9e33bdfa22 ("serial: 8250_dw: Improve clock rate setting")
>
> obviously tries to cure symptoms, and not a root cause.
>
> The root cause is the non-flexible rate calculation inside the
> corresponding clock driver.
The issue is the clock generating hardware in this particular platform
is only capable of generating finite clock rates from a fixed divider.
So when dw8250_set_termios() requests baud * 16 it may get something
quite different - e.g. it requests 1.8432MHz for a baud rate of 115,200
and gets back 3MHz from clk_round_rate() as this is the nearest
achievable. Before de9e33bdfa22, dw8250_set_termios() then sets the
clock to this rate which obviously doesn't work.
> What we need is to provide maximum UART
> divisor value to the clock driver to allow it do the job transparently
> to the caller.
So the clock driver should know that this clock is attached to a UART
with a fixed divider, and when 1.8432MHz is requested in the above
example it returns say 24MHz from clk_round_rate() because it knows that
the UART can divide this down to something within an acceptable range of
the requested rate for correct operation?
> Since from the initial commit message I have got no clue which clock
> driver actually needs to be amended, I leave this exercise to the people
> who know better the case.
>
> Moreover, it seems [1] the fix introduced a regression. And possible
> even one more [2].
>
> Taking above, revert the commit de9e33bdfa22 for now.
>
> [1]: https://www.spinics.net/lists/linux-serial/msg28872.html
> [2]: https://github.com/Dunedan/mbp-2016-linux/issues/29#issuecomment-357583782
>
> Cc: Ed Blake <ed.blake@xxxxxxxxxxx>
> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Cc: Lukas Wunner <lukas@xxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/8250/8250_dw.c | 30 ++++++++++++------------------
> 1 file changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 7ab5c2643019..693fa66e7ec9 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -245,31 +245,25 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
> struct ktermios *old)
> {
> unsigned int baud = tty_termios_baud_rate(termios);
> - unsigned int target_rate, min_rate, max_rate;
> struct dw8250_data *d = p->private_data;
> long rate;
> - int i, ret;
> + int ret;
>
> if (IS_ERR(d->clk) || !old)
> goto out;
>
> - /* Find a clk rate within +/-1.6% of an integer multiple of baudx16 */
> - target_rate = baud * 16;
> - min_rate = target_rate - (target_rate >> 6);
> - max_rate = target_rate + (target_rate >> 6);
> -
> - for (i = 1; i <= UART_DIV_MAX; i++) {
> - rate = clk_round_rate(d->clk, i * target_rate);
> - if (rate >= i * min_rate && rate <= i * max_rate)
> - break;
> - }
> - if (i <= UART_DIV_MAX) {
> - clk_disable_unprepare(d->clk);
> + clk_disable_unprepare(d->clk);
> + rate = clk_round_rate(d->clk, baud * 16);
> + if (rate < 0)
> + ret = rate;
> + else if (rate == 0)
> + ret = -ENOENT;
> + else
> ret = clk_set_rate(d->clk, rate);
> - clk_prepare_enable(d->clk);
> - if (!ret)
> - p->uartclk = rate;
> - }
> + clk_prepare_enable(d->clk);
> +
> + if (!ret)
> + p->uartclk = rate;
>
> out:
> p->status &= ~UPSTAT_AUTOCTS;
--
Ed
--
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
