Re: [PATCH v6 2/3] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov <sorganov@gmail.com>]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > On Mon, Jul 22, 2019 at 10:22:10PM +0300, Sergey Organov wrote:
>> >> imx_uart_set_mctrl() happened to set UCR2_CTSC bit whenever TIOCM_RTS
>> >> was set, no matter if RTS/CTS handshake is enabled or not. Now fixed by
>> >> turning handshake on only when CRTSCTS bit for the port is set.
>> >>
>> >> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Oh, you added my Ack for patches 2 and 3, too, even before I looked
> again on those :-|
>
>> >> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
>> >> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >>  drivers/tty/serial/imx.c | 16 ++++++++++++++--
>> >>  1 file changed, 14 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> index 32f36d8..059ba35 100644
>> >> --- a/drivers/tty/serial/imx.c
>> >> +++ b/drivers/tty/serial/imx.c
>> >> @@ -974,10 +974,22 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> >>  	if (!(port->rs485.flags & SER_RS485_ENABLED)) {
>> >>  		u32 ucr2;
>> >>
>> >> +		/*
>> >> +		 * Turn off autoRTS if RTS is lowered and restore autoRTS
>> >> +		 * setting if RTS is raised.
>> >
>> > "lower" and "raising" are misleading here. I recommend sticking to
>> > "active" and "inactive".
>> 
>> This is copy-pasted from the 8250 driver. I'd prefer to leave it as is.
>
> I'd prefer to fix the 8250 accordingly. "raised" is just misleading
> because the handshaking signals are low-active and you always have to
> think if the logical or the physical signal is raising. "active" is
> clear in this regard.

You are free to do it in one bunch for all the drivers it appears in
then. For now I prefer consistency between drivers, even though I do
agree the wording could be improved.

>> >> +		 */
>> >>  		ucr2 = imx_uart_readl(sport, UCR2);
>> >>  		ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
>> >> -		if (mctrl & TIOCM_RTS)
>> >> -			ucr2 |= UCR2_CTS | UCR2_CTSC;
>> >> +		if (mctrl & TIOCM_RTS) {
>> >> +			ucr2 |= UCR2_CTS;
>> >> +			/*
>> >> +			 * UCR2_IRTS is unset if and only if the port is
>> >> +			 * configured for CRTSCTS, so we use inverted UCR2_IRTS
>> >> +			 * to get the state to restore to.
>> >> +			 */
>> >> +			if (!(ucr2 & UCR2_IRTS))
>> >> +				ucr2 |= UCR2_CTSC;
>> >> +		}
>> >
>> > If you teach imx_uart_rts_auto about IRTS this function could be reused
>> > here I think.
>> 
>> Yeah, but imx_uart_rts_auto_if_crtscts_and_rts_is_active() ? I feel
>> somewhat uncomfortable about that mixing of different purposes.
>> 
>> Besides, one of the purposes of these patch series was to get rid of
>> imx_uart_rts_auto() as its name is confusing in the context of existing
>> imx_uart_rts_active() and imx_uart_rts_inactive(), as I already
>> explained before.
>> 
>> We can rename the function to avoid confusion, add yet another check to
>> it, and call it from 2 places, but it's still questionable if it's an
>> improvement, and could be done as a follow-up step anyway. It will look
>> something like this then:
>> 
>>  -- >8 --
>> 
>>     serial: imx: factor out common code into new imx_uart_set_auto_rts()
>> 
>> 	Modified   drivers/tty/serial/imx.c
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index d9a73c7..c8b847e 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -954,6 +954,20 @@ static unsigned int imx_uart_get_mctrl(struct uart_port *port)
>>  	return ret;
>>  }
>> 
>> +/*
>> + * Compute and set auto RTS in 'ucr2' according to the current state of RTS
>> + * signal and CRTSCTS state of port configuration.
>> + *
>> + * Use inverted UCR2_IRTS to get the state of CRTSCTS, and don't let receiver
>> + * control RTS output if RTS is inactive.
>> + *
>> + */
>> +static void imx_uart_set_auto_rts(u32 *ucr2)
>> +{
>> +	if ((*ucr2 & UCR2_CTS) && !(*ucr2 & UCR2_IRTS))
>> +		*ucr2 |= UCR2_CTSC;
>> +}
>> +
>
> this looks fine and is what I intended to suggest. The comment isn't
> optimal yet, I'd write something like:
>
>   /*
>    * Enable hardware control of the RTS output iff handshaking is in use
>    * and software requested RTS to be active.
>    * "handshaking is in use" can be determined from the IRTS bit that is
>    * set when handshaking is not used. The requested state by software
>    * is represented in the CTS bit.
>    */

I don't like it as this function doesn't enable or disable anything. It
just computes needed state of one bit taking into account two
conditions.

Independent on comment, for more clarity, it could have been:

static void imx_uart_set_auto_rts(u32 *ucr2)
{
	if ((*ucr2 & UCR2_CTS) && !(*ucr2 & UCR2_IRTS))
		*ucr2 |= UCR2_CTSC;
	else
		*ucr2 &= ~UCR2_CTSC;
}

to be independent of the current state of UCR2_CTSC.

>
> IMHO go directly to imx_uart_set_auto_rts() before introducing the
> second open coding of its logic.

Actually, it looks clearer the way I've suggested, I think, as I didn't
in fact introduce second open coding of its logic, it's just we later
decided that 2 different logics (restore autoRTS on RTS raise, and set
autoRTS on user request) could be merged into single function.

-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel