From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Hogan Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used Date: Sat, 4 Mar 2017 00:11:30 +0000 Message-ID: <20170304001130.GV996@jhogan-linux.le.imgtec.org> 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> <1488547866.20145.74.camel@linux.intel.com> <3e45a0582dd91dab1a6e9bd6f4339e12@mail.gmail.com> <20170303230712.GT996@jhogan-linux.le.imgtec.org> <69272eeb863f39633c51b0312c9ccd2f@mail.gmail.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="O9T4zNOkGnr0n+A/" Return-path: Content-Disposition: inline In-Reply-To: <69272eeb863f39633c51b0312c9ccd2f@mail.gmail.com> Sender: linux-clk-owner@vger.kernel.org To: Jason Uy Cc: Ray Jui , Andy Shevchenko , Heiko Stuebner , 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 --O9T4zNOkGnr0n+A/ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Jason, On Fri, Mar 03, 2017 at 04:02:56PM -0800, Jason Uy wrote: > HI James, >=20 > Maybe instead of that, we should do this instead >=20 > If ((IS_ERR(d->clk) && PTR_ERR(d->clk)) || !old) IS_ERR(x) matches a range of non-zero negative values, so it implies PTR_ERR(x) already, so it doesn't change anything. Cheers James >=20 > Note that this is what is done in the probe function of the dw driver. >=20 > Regards, > Jason >=20 > -----Original Message----- > From: James Hogan [mailto:james.hogan@imgtec.com] > Sent: March-03-17 3:07 PM > To: Jason Uy > Cc: Ray Jui ; Andy Shevchenko > ; Heiko Stuebner ; Gr= eg > Kroah-Hartman ; Jiri Slaby ; > Kefeng Wang ; Noam Camus ; > Heikki Krogerus ; Wang Hongcheng > ; linux-serial@vger.kernel.org; LKML > ; bcm-kernel-feedback-list@broadcom.com; Li= nux > MIPS Mailing List ; David Daney > ; Russell King ; > linux-clk@vger.kernel.org; Viresh Kumar > Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control = to > be used >=20 > Hi Jason, >=20 > On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote: > > James, > > > > Can you verify that changing the code to the following fixes your probl= em? > > > > if (IS_ERR_OR_NULL(d->clk) || !old) > > goto out; >=20 > It does, however I'm not at all convinced it is correct. clk_get either > returns a valid opaque clock cookie that can be passed to other clock > functions (which includes NULL), or ERR_PTR(-errno), which IS_ERR() should > catch for errors. >=20 > According to this thread: >=20 > https://lists.gt.net/linux/kernel/2102623 >=20 > we should stick to the clk API and use IS_ERR() rather than > IS_ERR_OR_NULL(), but shouldn't be blindly accepting the result of > clk_get_rate() (or I suppose clk_round_rate()), but rather checking for t= he > value 0 and handling that case as "we don't have a usable clock from the = clk > api, fall back to something else". >=20 > Cheers > James >=20 > > > > Regards, > > Jason > > > > -----Original Message----- > > From: Ray Jui [mailto:ray.jui@broadcom.com] > > Sent: March-03-17 9:34 AM > > To: Andy Shevchenko ; 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 > > Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow > > control to be used > > > > Hi Andy/Jason, > > > > On 3/3/2017 5:31 AM, Andy Shevchenko wrote: > > > 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=3Dn (Octeon doesn't select it) > > >> 2) The CONFIG_HAVE_CLK=3Dn 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=3Dn implementation of clk_round_rate() returns > > >> 0 > > >> 5) The CONFIG_HAVE_CLK=3Dn 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. > > > > I agree. I think it should have been: > > > > if (IS_ERR_OR_NULL(d->clk) || !old) > > goto out; > > > > I think it makes sense to validate to make sure the 'clk' pointer is > > valid before proceeding any further down below (regardless of how well > > or how not well the clock framework handles it). > > > > Thanks, > > > > Ray > > > > > > > >> The CONFIG_HAVE_CLK=3Dn 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 > > > --O9T4zNOkGnr0n+A/ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYugYyAAoJEGwLaZPeOHZ6UlUP/38BfF1UYDxx4a7HBrhHZPZW V5QasOZgwCaNub6twcX5SMvX2jJQZb8Rwgkaz6CfMpZnq0WYTTGYjVFjKE9PeTJI qno2Fm+w/nk8/YuYQVcWsqG9UzZa9tts5NnNKxPbpyRntKoZH90LP84sdTkRwZoy sA7mOapLI2WNNr3yPZjQVGwTi7S7IJ0ZvbC+trnih2/F2L3taPMHiRbYcVfMBttk NJSWD3ybvQIGgJtEKaKTNxbSZzdXO5g0bMBHfniPveapVGqSKXdUEJYYkSALhyo8 y2ZF+uIRyj3LxO8OHIvJXLk2ktqwXExmKbsP3vj1G0s8uC9zDFIUfPTATIWPGpbg Lj/eGNkxWAWeEsFNz8GoqicpCEwwGlbnm/qPAzzxUzFKm94En9wxYOtWRINlTYe9 8ndCZMJuhQYx1zIxL00V62YOUThLJsGFfwrAB6U0Yo94koPfkvDqi3UUx76i9/1F MAh9S53z/JkaKO2ZO8rpqx9CWqxkcl2y1Ft7dp7o8L2eOhDqllL4uJFAk2V2p9G/ 7ivyjYhlIPrvrhCkyVUQYwJy0iLqBIeyUMUGs0CYNjv6cSIuoDiv3sYFxRY8UIJc K/YkqnMWfivw2Sulmw8bEoEsKLRS8Z8ufOsqOfCEVqaT1J795+fcXELIUsx8iB7v EdAtXtU8GdxI8YuYjUKv =gj8k -----END PGP SIGNATURE----- --O9T4zNOkGnr0n+A/--