Hi Marek,
On Thu, Dec 13, 2018 at 03:27:51AM +0100, Marek Vasut wrote:
> > I wonder whether the issue may be the JZ4780 UART not automatically
> > resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
> > doesn't explicitly say it'll happen & the programming example it gives
> > says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
> > the kernel has been doing for at least the whole git era I wouldn't be
> > surprised if other devices are bitten by the change as people start
> > trying 4.20 on them.
>
> The patch you're complaining about is doing exactly that -- it sets
> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
> any other bits in FCR.
No - your patch does that *only* if the FIFO enable bit is set, and
presumes that clearing FIFOs is a no-op when they're disabled. I don't
believe that's true on my system. I also believe that not touching the
FIFO enable bit is problematic, since some callers clearly expect that
to be cleared.
> > @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> > if (p->capabilities & UART_CAP_FIFO) {
> > /*
> > * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
> > - * In case ENABLE_FIFO is not set, there is nothing to flush
> > - * so just return. Furthermore, on certain implementations of
> > - * the 8250 core, the FCR[7:3] bits may only be changed under
> > - * specific conditions and changing them if those conditions
> > - * are not met can have nasty side effects. One such core is
> > - * the 8250-omap present in TI AM335x.
> > + * On certain implementations of the 8250 core, the FCR[7:3]
> > + * bits may only be changed under specific conditions and
> > + * changing them if those conditions are not met can have nasty
> > + * side effects. One such core is the 8250-omap present in TI
> > + * AM335x.
> > */
> > fcr = serial_in(p, UART_FCR);
> > + serial_out(p, UART_FCR, fcr | clr_mask);
> > + serial_out(p, UART_FCR, fcr & ~clr);
>
> Note that, if I understand the patch correctly, this will _not_ restore
> the original (broken) behavior. The original behavior cleared the entire
> FCR at the end, which cleared even bits that were not supposed to be
> cleared .
It will restore the original behaviour with regards to disabling the
FIFOs, so long as callers that expect that use
serial8250_clear_and_disable_fifos().
> This patch will make things worse by retaining the clr_mask set in the
> FCR, thus the FCR[2:1] will be set when you return from this function.
> That cannot be right ?
You're mistaken - clr_mask (ie. the FIFO reset bits) get cleared again
by the second call to serial_out(), I just did it without modifying the
fcr variable - ie. we write the original fcr value again at the end, but
with UART_FCR_ENABLE_FIFO cleared in the
serial8250_clear_and_disable_fifos() case. It would probably be clearer
with the clr argument renamed disable or something like that.
Thanks,
Paul
