Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Jan Kiszka <jan.kiszka@siemens.com>]

On 12.11.21 07:14, Su Bao Cheng wrote:
> On 2021/10/27 下午7:39, Lukas Wunner wrote:
>> On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
>>> This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.
>>>
>>> The `serial8250_do_set_mctrl` not only used by userspace ioctl but
>>> also the kernel itself.
>>>
>>> During tty_open, the uart_port_startup sets the MCR to 0, and then use
>>> set_mctrl to restore the MCR, so at this time, the MCR read does not
>>> reflect the desired value.
>>
>> I don't quite follow.  Where is uart_port_startup() setting the MCR to 0?
>> Are you referring to the call to uart_port_dtr_rts()?  That function should
>> set RTS correctly according to RS485 state, so I don't see where any
>> breakage may occur.
>>
>> What is the user-visible issue that you seek to fix with the revert?
>>
> 
> Sorry for the late response, the company exchange server does not work
> for me at this moment, so I have to use the private email instead.
> 
> The issue is observed on omap8250 hardware (CPU: AM6548). the use case
> is RS485 half-duplex (2 wire mode), in this mode the RTS pin is used to
> control the direction and is software controller via the MCR[1]
> register. The problem is that the RS485 transmitting is OK, but the
> receiving is not working. Similar issue also exists for the RS422, i.e.
> the 4-wire full-duplex mode of RS485, but this time the TX does not
> work, RX is fine.
> 
> The MCR is set to 0 at this line within uart_port_startup():
> 	retval = uport->ops->startup(uport);
> 
> On omap8250, the startup() points to omap_8250_startup(), within it:
> 	up->mcr = 0;
> 
> For software controlled RTS pin of RS485 half-duplex, when not in the
> transmitting, the MCR[1] should be constant to indicate the current
> direction is receiving. This is set in serial8250_em485_stop_tx().
> 
> So after this point of setting the MCR to 0, this up->mcr register
> mirror does not reflect the actual desired value anymore. Further
> checking against it leads to false result.
> 
> Another possible fix could be, instead of setting the mcr to 0 blindly,
> one could check if the current operating mode is RS485 half-duplex, and
> if so, mask the MCR[1], so that this bit is not changed. Because the
> MCR[1] will be changed to the correct value before TX in
> serial8250_em485_start_tx(), this change would not impact the transmitting.
> 

From this description, it seems like we have a rather fundamental
regression here. Was RS485-half-duplex / RS422 tested after this change,
Lukas?

A revert is just a workaround, I would say. But unless we have a quick
idea for the proper fix, it may be the option for stable at least.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux