On 1/17/23 5:02 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
On Tue, 17 Jan 2023 at 15:54, Evgeny Iakovlev
<eiakov...@linux.microsoft.com> wrote:
>
>
> On 1/17/2023 16:24, Peter Maydell wrote:
>> On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
>> <eiakov...@linux.microsoft.com> wrote:
>>> Current FIFO handling code does not reset RXFE/RXFF flags when guest
>>> resets FIFO by writing to UARTLCR register, although internal FIFO state
>>> is reset to 0 read count. Actual flag update will happen only on next
>>> read or write to UART. As a result of that any guest that expects RXFE
>>> flag to be set (and RXFF to be cleared) after resetting FIFO will just
>>> hang.
>>>
>>> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
>>> depth handling code based on current FIFO mode.
>> This patch is doing multiple things at once ("also" in a
>> commit message is often a sign of that) and I think it
>> would be helpful to split it. There are three things here:
>> * refactorings which aren't making any real code change
>> (eg calling pl011_get_fifo_depth() instead of doing the
>> "if LCR bit set then 16 else 1" inline)
>
>
> Yeah, tbh i also though i should do it..
>
>
>> * the fix to the actual bug
>> * changes to the handling of the FIFO which should in theory
>> not be visible to the guest (I think it now when the FIFO
>> is disabled always puts the incoming character in read_fifo[0],
>> whereas previously it would allow read_pos to increment all
>> the way around the FIFO even though we only keep one character
>> at a time).
>
>
> That last part i don't understand. If by changes to the FIFO you are
> referring to the flags handling, then it will be visible to the guest by
> design, since that's what I'm fixing. Could you maybe explain that one
> again? :)
I meant the bit where the existing code for the FIFO-disabled
case puts incoming characters in s->read_fifo[s->read_pos] and
reads from UARTDR always increment s->read_pos; whereas the
changed code always puts characters in s->read_fifo[0] and
avoids incrementing read_pos. I think your version is more
sensible (and also matches what the TRM claims the hardware
is doing), although the guest-visible behaviour doesn't change.
I see, thanks. I tried separating this particular piece into its own commit,
but i just feel like it makes the part that's left pointless, because
pl011_get_fifo_depth replaces mostly just those 2 occurences anyway.. I've
added a paragraph to a commit message to document a functional change. Let me
know if that's not good enough. I've sent out a v2 with the rest of comments
addressed + a reset method for PL011.
thanks
-- PMM
