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. >> >> >>> + } >> >> Side note for those interested in this driver: >> >>> if (termios->c_iflag & IXON) >>> flow |= SC16IS7XX_EFR_SWFLOW3_BIT; >>> if (termios->c_iflag & IXOFF) >>> flow |= SC16IS7XX_EFR_SWFLOW1_BIT; >> >> Looks like IXON and IXOFF auto soft flow control modes are backwards. >> >> IXON should enable comparison of rx with START/STOP chars, and IXOFF >> should enable flow control based on rx fifo levels. >> >> I would seriously consider not enabling soft flow control for >> IXOFF: only a remote with h/w-based soft flow control will be able to stop tx >> before overflowing this uart's rx fifo. >> > I haven't looked at XON/XOFF flow control yet. Still I see no problem in fixing HW flow-control in a patch of its own. Neither do I (which is why I commented with "Side note for those interested..."). > Several mail-threads about this driver are now active. Performance really s.... on the imx.6 we are currently using it at. I guess we'll see more patches in the future > >> >> Lastly, according to the datasheet, the soft flow control should not be >> enabled simultaneously with autoRTS/autoCTS, but I don't see any logic here >> that prevents that. However, I think this patch should fix this problem, which is as simple as: } else { .... } >> Regards, >> Peter Hurley >> >> >>> >>> + /* Always set enable enhanced */ >>> + flow |= SC16IS7XX_EFR_ENABLE_BIT; >>> + >>> sc16is7xx_port_write(port, SC16IS7XX_EFR_REG, flow); >>> regcache_cache_bypass(s->regmap, false); >> > -- 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
