Hello again Peter, ----- Original Message ----- From: Maarten Brock [mailto:m.brock@xxxxxxxxxxxxx] To: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx], Kim Bøndergaard Poulsen [mailto:Kim.BondergaardPoulsen@xxxxxxxxx], linux-serial@xxxxxxxxxxxxxxx, joshc@xxxxxx, kubakici@xxxxx Cc: gregkh@xxxxxxxxxxxxxxxxxxx Sent: Sat, 19 Mar 2016 10:42:47 +0100 Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver > 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 As promised I tested what the SC16IS740 does. It is not as I described last, but as you and I already assumed in the first place. When auto-RTS is enabled with EFR[6] == 1 then MCR[1] is ignored. Then I looked up the TL16C750 datasheet since you mentioned this SC16IS7xx should be based on the 16C750. The TL16C750 uses MCR[1] to enable auto-RTS when MCR[5] is set. So with MCR[5] == 1 and MCR[1] == 0 there is nothing left that can control RTS: autoRTS is disabled and MCR[1] has a different effect. And with MCR[5] == 1 and MCR[1] == 1 RTS is fully controlled by the FIFO. So effectively this one also disables manual control of RTS when autoRTS is enabled. I still do not consider this to be a broken state. Enabling autoRTS just disables manual control of RTS. Whether enabling/disabling autoRTS should be done through CRTSCTS is a different issue. 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
