----- Original Message ----- From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx] To: Kim Bøndergaard Poulsen [mailto:Kim.BondergaardPoulsen@xxxxxxxxx], linux-serial@xxxxxxxxxxxxxxx [mailto:linux-serial@xxxxxxxxxxxxxxx], joshc@xxxxxx [mailto:joshc@xxxxxx], kubakici@xxxxx [mailto:kubakici@xxxxx] Cc: gregkh@xxxxxxxxxxxxxxxxxxx [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] Sent: Mon, 14 Mar 2016 16:25:37 +0100 Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver > On 03/14/2016 02:07 AM, Kim Bøndergaard Poulsen wrote: > > > > > >> -----Original Message----- > >> From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx] > >> Sent: 11. marts 2016 02:44 > >> To: Kim Bøndergaard Poulsen; linux-serial@xxxxxxxxxxxxxxx; joshc@xxxxxx; > >> kubakici@xxxxx > >> Cc: gregkh@xxxxxxxxxxxxxxxxxxx > >> Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver > >> > >> Hi Kim, > >> > >> On 03/08/2016 04:30 AM, Kim Bøndergaard Poulsen wrote: > >>> Following patch fixes HW flowcontrol issues with UARTs based in > >>> SC16IS7XX > >>> > >>> I've only tested the patch with an SC16IS750. > >>> > >>> commit 9390e7e2952cac5e0119a656a0d8a4e8efe33d07 > >>> Author: Kim Bøndergaard <kibo@xxxxxxxxx> > >>> Date: Tue Mar 8 12:31:21 2016 +0100 > >>> > >>> sc16is7xx: Fix HW flow control > >>> > >>> Following problems fixed: > >>> > >>> In sc16is7xx_set_baud() what ever was already set in > >>> EFR.CTS and EFT.RTS was cleared. > >>> > >>> In sc16is7xx_set_termios() enhanced functions where disabled > >>> > >>> In sc16is6xx_set_termios() upper layer is informed that this driver > >>> support automatic HW flowcontrol. Without this knowledge the two > >>> layers eventually ended up in different states, causing > >>> communication to stop > >> > >> Missing Signed-off-by: > > > > I'll add it > > > >> > >>> > >>> diff --git a/drivers/tty/serial/sc16is7xx.c > >>> b/drivers/tty/serial/sc16is7xx.c index 13f8d5f..5a51024 100644 > >>> --- a/drivers/tty/serial/sc16is7xx.c > >>> +++ b/drivers/tty/serial/sc16is7xx.c > >>> @@ -511,8 +511,9 @@ static int sc16is7xx_set_baud(struct uart_port > >>> *port, int baud) > >>> > >>> /* Enable enhanced features */ > >>> regcache_cache_bypass(s->regmap, true); > >>> - sc16is7xx_port_write(port, SC16IS7XX_EFR_REG, > >>> - SC16IS7XX_EFR_ENABLE_BIT); > >>> + sc16is7xx_port_update(port, SC16IS7XX_EFR_REG, > >>> + SC16IS7XX_EFR_ENABLE_BIT, > >>> + SC16IS7XX_EFR_ENABLE_BIT); > >>> regcache_cache_bypass(s->regmap, false); > >>> > >>> /* Put LCR back to the normal mode */ @@ -901,14 +902,19 @@ > >>> static void sc16is7xx_set_termios(struct uart_port *port, > >>> regcache_cache_bypass(s->regmap, true); > >>> sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios- > >>> c_cc[VSTART]); > >>> sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios- > >>> c_cc[VSTOP]); > >>> - if (termios->c_cflag & CRTSCTS) > >>> + if (termios->c_cflag & CRTSCTS) { > >>> flow |= SC16IS7XX_EFR_AUTOCTS_BIT | > >>> SC16IS7XX_EFR_AUTORTS_BIT; > >>> + port->status |= UPSTAT_AUTOCTS; > >> > >> This is correct. > >> > >> > >> However a quick read of the SC16IS740 datasheet suggests that RTS > >> handling is broken when autoRTS is enabled. > >> > > > > > > Where do you see that? > > http://www.nxp.com/documents/data_sheet/SC16IS740_750_760.pdf > > page 27/63, 8.6 Modem Control Register, > "If Auto RTS is enabled, the RTS output is controlled by hardware flow > control" > > It's possible I'm misreading that, in that the statement only applies if > MCR[1] == 1. > > > Like written I've only tested at SC16IS750 but the datasheet I've used > cover the whole range 740/750/760 > > I think I have to get that information to understand your next statements. > Can't really see if it relates to my fix. > > If MCR[1] == 0 disables RTS even when autoRTS is enabled, then please ignore > my comments regarding TIOCMSET ioctl and set_mctrl() method below. > > > > >> AFAICT, RTS state in MCR is _ignored_ when autoRTS is enabled. > >> > >> This means that if userspace has specifically disabled RTS with TIOCMSET > >> ioctl and CRTSCTS is enabled, input will not be disabled. > >> > >> To make this work properly requires disabling/restoring autoRTS in the > >> set_mctrl() method when TIOCM_RTS is changed. > >> > >> > >>> + } If the device uses autoRTS and then userspace tries to force RTS one should expect this not to work. Userspace should first disable autoRTS. Alternatively the driver could automagically disable autoRTS. But then it should never reenable autoRTS as that AFAICT would override the forced setting again. I see nothing broken though. Maarten -- 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
