On Fri, Jan 07, 2022 at 09:30:48AM +0100, Jiri Slaby wrote: > On 02. 01. 22, 18:42, Lukas Wunner wrote: > > pl010_set_termios() briefly resets the CR register to zero. > > > > Where does this register write come from? > > > > The PL010 driver's IRQ handler ambauart_int() originally modified the CR > > register without holding the port spinlock. ambauart_set_termios() also > > modified that register. To prevent concurrent read-modify-writes by the > > IRQ handler and to prevent transmission while changing baudrate, > > ambauart_set_termios() had to disable interrupts. That is achieved by > > writing zero to the CR register. > > > > However in 2004 the PL010 driver was amended to acquire the port > > spinlock in the IRQ handler, obviating the need to disable interrupts in > > ->set_termios(): > > https://git.kernel.org/history/history/c/157c0342e591 > > > > That rendered the CR register write obsolete. Drop it. [...] > > --- a/drivers/tty/serial/amba-pl010.c > > +++ b/drivers/tty/serial/amba-pl010.c > > @@ -446,14 +446,11 @@ pl010_set_termios(struct uart_port *port, struct ktermios *termios, > > if ((termios->c_cflag & CREAD) == 0) > > uap->port.ignore_status_mask |= UART_DUMMY_RSR_RX; > > - /* first, disable everything */ > > old_cr = readb(uap->port.membase + UART010_CR) & ~UART010_CR_MSIE; > > if (UART_ENABLE_MS(port, termios->c_cflag)) > > old_cr |= UART010_CR_MSIE; > > - writel(0, uap->port.membase + UART010_CR); > > - > > Isn't the write of zero followed by the original CR value needed to actually > start the chip with the updated baud rate? Why do you think so? Such a requirement is not mentioned in the manual of the PL010 (the UARTCR register is documented on page 44): https://documentation-service.arm.com/static/5e8e246dfd977155116a54be The manual of the successor, PL011, does contain a note on page 62 which could be interpreted in the way you've written above. However, in reality this procedure appears to be unnecessary: "Program the control registers as follows: 1. Disable the UART. 2. Wait for the end of transmission or reception of the current character. 3. Flush the transmit FIFO by setting the FEN bit to 0 in the Line Control Register, UARTLCR_H on page 3-12. 4. Reprogram the UARTCR Register. 5. Enable the UART." https://documentation-service.arm.com/static/5e8e36c2fd977155116a90b5 > What are you trying to fix? This is not a fix, it's a cleanup. Zeroing the CR register was cargo-culted to the PL011 driver and appears to be not only unnecessary there but also harmful because it glitches RS-485 RTS deassertion. While digging in the git history to find out where the register write originates from, I've come to the conclusion that it is unnecessary in the PL010 driver as well. So it appears to be bit rot that can be removed. Of course, I only have a PL011 available for testing and not a PL010, so I cannot rule out 100% that the change does not cause regressions. Faced with the choice of letting the old driver bit rot or risk a regression, I chose the latter. I was hoping that Russell might remember the reason for the register write and cry foul if my change is not correct. Thanks, Lukas
