Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> Hello,
>
> On Thu, May 30, 2019 at 06:29:44PM +0300, Sergey Organov wrote:
>> imx_set_mctrl() stop fiddling with UCR2_CTSC bit
>>
>> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
>> ---
>> drivers/tty/serial/imx.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index e9e812a..6577552 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -967,9 +967,9 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> u32 ucr2;
>>
>> ucr2 = imx_uart_readl(sport, UCR2);
>> - ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
>> + ucr2 &= ~UCR2_CTS;
>> if (mctrl & TIOCM_RTS)
>> - ucr2 |= UCR2_CTS | UCR2_CTSC;
>> + ucr2 |= UCR2_CTS;
>> imx_uart_writel(sport, ucr2, UCR2);
>> }
>
> I'm sure this patch is wrong. And your change log fails to point out
> what you want to achieve.
Sorry, I somehow thought it's obvious when in fact it appears it isn't.
When user calls ioctl() to set/clear TIOCM_RTS bit from user space, the
RTS/CTS automatic handshake must not be affected. Without this patch
setting TIOCM_RTS to logic 0 turns off RTS/CTS handshake in hardware,
and setting it to logic 1 turns on RTS/CTS handshake in hardware, that
is wrong thing to do in both cases.
I'm sure the patch is the right thing to do here (see below for more
discussion). Just stop fiddling with RTS/CTS handshake bit in
set_mctrl(), and it will work as expected.
>
> Independant of your patch I discussed a problem in imx_uart_set_mctrl()
> with Sascha and Russell (both added to Cc:) earlier this week. In the
> current implementation there are actually two problems.
>
> Currently imx_uart_set_mctrl does:
>
> if TIOCM_RTS is set:
> let the receiver control the RTS signal
> else:
> set RTS inactive
Worse. In fact it breaks current UCR2_CTSC state on both branches of the
if, and thus breaks correspondence between CRTSCTS and UCR2_CTSC that
the driver should preserve at all times.
> The bigger problem is that if the UART is configured not to use
> handshaking (CRTSCTS unset) the mode "let the receiver control the RTS
> signal" should not be used.
Once again, that's exactly why we need to stop touching UCR2_CTSC in
set_mctrl().
> The smaller (and irrelevant for correctness) problem is that setting
> UCR2_CTS is a no-op when UCR2_CTSC is also set.
It's not a problem at all. When user wants to drive RTS manually, he
turns off RTS/CTS handshake. When RTS/CTS handshake is turned on,
setting/clearing TIOCM_RTS usually has no effect on the level of the RTS
pin.
>
> We think the right thing to do is:
>
> ucr2 = imx_uart_readl(sport, UCR2);
> ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
>
> if (mctrl & TIOCM_RTS) {
> if (sport->crtscts)
> /* let the receiver control RTS */
> ucr2 |= UCR2_CTSC;
> else
> /* Force RTS active */
> ucr2 |= UCR2_CTS;
> } else {
> /* Force RTS inactive, i.e. CTS=0, CTSC=0 */
> }
>
> imx_uart_writel(sport, ucr2, UCR2);
>
> but AFAICT this isn't tested yet to an end in the use case that Sascha
> currently has and so there isn't a complete patch available yet.
This is still wrong, as it turns off RTS/CTS handshake in hardware on
TIOCM_RTS=0. Once again, set_mctrl() should not touch UCR2_CTSC, -- it's
as simple as that.
I still think it's rather what I did in the patch above is the right
thing to do. It's simple and does the job, no surprises.
-- Sergey
