On 03/23/2016 08:47 AM, Maarten Brock wrote: > 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. Thanks for testing. > 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. You're misreading the 16C750 datasheet. MCR[5] == 1 and MCR[1] = 0 means autoRTS is disabled and RTS output is inactive. > I still do not consider this to be a broken state. Enabling autoRTS just > disables manual control of RTS. RTS output active when TIOCM_RTS is clear is broken. > Whether enabling/disabling autoRTS should be done through CRTSCTS is a > different issue. Sure, not enabling autoRTS at all is an option too. 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
