Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used

[James Hogan]

On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> > On 11 January 2017 at 19:48, Jason Uy <jason.uy@xxxxxxxxxxxx> wrote:
> > > In the most common use case, the Synopsys DW UART driver does not
> > > set the set_termios callback function. This prevents UPSTAT_AUTOCTS
> > > from being set when the UART flag CRTSCTS is set. As a result, the
> > > driver will use software flow control as opposed to hardware flow
> > > control.
> > > 
> > > To fix the problem, the set_termios callback function is set to the
> > > DW specific function. The logic to set UPSTAT_AUTOCTS is moved so
> > > that any clock error will not affect setting the hardware flow
> > > control.
> 
> > Bisection shows that this patch, commit
> > 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> > Cavium Octeon III based UTM-8 board (MIPS architecture).
> > 
> > I now get the following warning:
> 
> > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> 
> > Then it hangs and the watchdog restarts the machine.
> > 
> > Any ideas?
> 
> 1. Does it use clock on that platform?

btw, sorry for HTML email blocked from lists, gmail tricked me into it
:-(

I've now dug a little deeper. Essentially what is going on is:

1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns NULL
3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
   doesn't match, since !IS_ERR(NULL)
4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
   returns 0
6) dw8250_set_termios() thinks the frequency for that baud rate has been
   set successfully and writes 0 into uartclk
7) it all goes wrong from there...

The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:

> These calls will return error for platforms that don't select HAVE_CLK

And NULL isn't an error in this API.

Cc'ing some clk folk.

Cheers
James

Attachment: signature.asc
Description: Digital signature