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

[Andy Shevchenko]

Heiko, you might be interested in this as well.

On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> 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?

> 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...

So, it means we have need special care of NULL case here, and honestly,
I don't like it. But it seems the only feasible (quick) fix right now.

> 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.

Which is okay. I dunno what should be returned from clk_round_rate() if
clk is NULL. I would fix CLK framework, though I would like to gather
more details.

Btw, I hope you also noticed this one:

http://www.spinics.net/lists/linux-serial/msg25314.html

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy