Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again

[Marek Vasut]

On 12/13/2018 04:33 AM, Paul Burton wrote:
> Hi Marek,

Hello Paul,

> 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.

Why would you disable FIFO to clear it ? This doesn't make sense to me,
the FIFO must be operational for it to be cleared. Moreover, it
contradicts your previous statement, "the programming example it gives
says to explicitly clear the FIFOs using FCR[2:1]" .

What exactly does your programming example for the JZ4780 say about the
FIFO clearing ? And does it work in that example ?

>>> @@ -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().

Does your platform need the FIFO to be explicitly disabled for it to be
cleared, can you confirm that ? And if so, does this apply to other
platforms with 8250 UART or is this specific to JZ4780 implementation of
the UART block ?

I just remembered U-Boot has this patch for JZ4780 UART [1], which
touches the FCR UME bit, so the FCR behavior on JZ4780 does differ from
the generic 8250 core. Could it be that this is what you're hitting ?

[1]
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=0b060eefd951fc111ecb77c7b1932b158e6a4f2c;hp=79fd9281880974f076c5b4b354b57faa6e0cc146

>> 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.

Uhh, OK, thanks for clarifying.

-- 
Best regards,
Marek Vasut