On 19.11.21 09:00, Jan Kiszka wrote: > 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 > Digging a bit deeper: One issue of the original patch was definitely that it tried to sanitized mctrl inside serial8250_do_set_mctrl, rather than serial8250_set_mctrl. That caused alternative set_mctrl handler to become out of sync /wrt mctrl. Just look at omap8250_set_mctrl, how it will work on the unsanitized value. That may or may not yet explain all issues Baocheng is seeing with the patch. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
