Hi Maarten, On 03/14/2016 09:43 AM, Maarten Brock wrote: > ----- 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 Please fix your mail client or use a different one. None of your replies are threaded. >> 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. How? By dropping CRTSCTS in termios? No, that's exactly what I don't want; ioctl(TIOCMSET) should just work. >From the POV of userspace with CRTSCTS enabled, disabling RTS via ioctl(TIOCMSET) is halting input and reenabling RTS via ioctl(TIOCMSET) is resuming input. > Alternatively the driver could automagically disable autoRTS. But then it > should never reenable autoRTS as that AFAICT would override the forced setting > again. The driver would restore (rather than re-enable) the autoRTS state when RTS is raised. The 8250 driver does this with OMAP, and both OMAP and SC16IS7xx are based on the 16C750. > I see nothing broken though. As long as sc16is7xx_set_mctrl(~TIOCM_RTS) disables RTS regardless of autoRTS state. Regards, Peter Hurley -- 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
