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: Thu, 17 Mar 2016 02:22:16 +0100 Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver > 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 > > > >> 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 > >> > >>>> 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 > >>>>>> > >>>>>> On 03/08/2016 04:30 AM, Kim Bøndergaard Poulsen wrote: > >>>>>>> Following patch fixes HW flowcontrol issues with UARTs based in > >>>>>>> SC16IS7XX > >>>>>>> > >>>>>> > >>>>>> 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 I'll try to explain my point. You claim that the SC16IS7xx is broken if it ignores MCR[1] when EFR[6] is set. I asked where it is documented that it may not do this. Now you point to above documentation which states nothing about autoRTS or CRTSCTS. It states that setting TIOCM_RTS must unconditionally drive RTS active. It also states that clearing TIOCM_RTS must unconditionally drive RTS inactive. This means that this function *should* disable autoRTS *without* restoring. As I understand it this is not what you wanted. This documentation doesn't even say a word what CRTSCTS should do to RTS: set_termios(port,termios,oldtermios) Change the port parameters, including word length, parity, stop bits. Update read_status_mask and ignore_status_mask to indicate the types of events we are interested in receiving. Relevant termios->c_cflag bits are: CRTSCTS - if set, enable CTS status change reporting So my point is that something that has no documented behavior cannot be broken. Since autoRTS is not mentioned at all, it would be my assumption that enabling autoRTS would fully take over the RTS line. This indeed means that userspace should have to disable CRTSCTS unconditionally to get ioctl(TIOCMSET) to work properly. Or if the documentation should be taken literally, ioctl(TIOCMSET) should disable autoRTS without automatic re-enabling. I also see a risk in disabling and restoring. What if the user calls disableRTS twice, without an intermediate enableRTS? Now back to the datasheet. Rereading it I think it does exactly what you want: MCR[1] == 0: RTS inactive MCR[1] == 1 && EFR[6] == 0: RTS active MCR[1] == 1 && EFR[6] == 1: RTS depends on buffer level I think it is like this because the clause "If Auto RTS is enabled, the RTS output is controlled by hardware flow control." is in the same paragraph as "logic 1". I have not yet had the time to actually verify this on a SC16IS740, the only device I have at hand. I'll get back to you when I have. 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
