On Tue, 17 Jan 2023 at 15:54, Evgeny Iakovlev <[email protected]> wrote: > > > On 1/17/2023 16:24, Peter Maydell wrote: > > On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev > > <[email protected]> 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. thanks -- PMM
