From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used Date: Fri, 03 Mar 2017 15:31:06 +0200 Message-ID: <1488547866.20145.74.camel@linux.intel.com> References: <1484164100-9805-1-git-send-email-jason.uy@broadcom.com> <1484164100-9805-2-git-send-email-jason.uy@broadcom.com> <1488394220.20145.68.camel@linux.intel.com> <20170303002129.GS996@jhogan-linux.le.imgtec.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20170303002129.GS996@jhogan-linux.le.imgtec.org> Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: James Hogan , Heiko Stuebner Cc: Jason Uy , Greg Kroah-Hartman , Jiri Slaby , Kefeng Wang , Noam Camus , Heikki Krogerus , Wang Hongcheng , linux-serial@vger.kernel.org, LKML , bcm-kernel-feedback-list@broadcom.com, Linux MIPS Mailing List , David Daney , Russell King , linux-clk@vger.kernel.org, Viresh Kumar List-Id: linux-serial@vger.kernel.org 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 > > > 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: > > > [] uart_get_baud_rate+0xfc/0x1f0 > > > [] serial8250_do_set_termios+0xb0/0x440 > > > [] uart_set_options+0xe8/0x190 > > > [] serial8250_console_setup+0x84/0x158 > > > [] univ8250_console_setup+0x54/0x70 > > > [] register_console+0x1c8/0x418 > > > [] uart_add_one_port+0x434/0x4b0 > > > [] serial8250_register_8250_port+0x2d8/0x440 > > > [] 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 Intel Finland Oy