Hi Maarten, On 03/14/2016 11:19 AM, Maarten Brock wrote: > Hello Peter, > > ----- Original Message ----- > From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx] > To: Maarten Brock [mailto:m.brock@xxxxxxxxxxxxx], Kim Bøndergaard Poulsen [mailto:Kim.BondergaardPoulsen@xxxxxxxxx], linux-serial@xxxxxxxxxxxxxxx, joshc@xxxxxx, kubakici@xxxxx > Cc: gregkh@xxxxxxxxxxxxxxxxxxx > Sent: Mon, 14 Mar 2016 18:12:23 +0100 > Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver > > >> 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. >> > It is already cumbersome enough to use Kerio Mini to please this mailing list > not to send HTTP replies. Wow. Sorry for huge inconvenience. Picking through a bunch of different threads that are actually all the same thread is way better. > I cannot force my company to use a different email client. > >> >>>> 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 > > Can I assume then that this behavior that you describe is documented somewhere > as the way it is supposed to work. Documentation/serial/driver: set_mctrl(port, mctrl) This function sets the modem control lines for port described by 'port' to the state described by mctrl. The relevant bits of mctrl are: - TIOCM_RTS RTS signal. - TIOCM_DTR DTR signal. - TIOCM_OUT1 OUT1 signal. - TIOCM_OUT2 OUT2 signal. - TIOCM_LOOP Set the port into loopback mode. If the appropriate bit is set, the signal should be driven active. If the bit is clear, the signal should be driven inactive. Locking: port->lock taken. Interrupts: locally disabled. This call must not sleep Also, what is your point? That userspace should have to disable CRTSCTS unconditionally to get ioctl(TIOCMSET) to work properly? That's just stupid. Finally, other in-tree drivers use tiocmset() to signal RTS directly; see hci_ldisc.c and others. So like I said, if MCR[1] == 0 disables RTS even when autoRTS is enabled then no driver changes are required. But if MCR[1] is ignored when autoRTS is enabled, then this driver will need to disable/restore autoRTS in sc16is7xx_set_mctrl(). 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
