Re: [PATCH v2 3/4] hw/char/pl011: better handling of FIFO flags on LCR reset
On 1/19/2023 14:30, Peter Maydell wrote:
On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
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 guest-visible flag update will happen
only on next data read or write attempt. As a result of that any guest
that expects RXFE flag to be set (and RXFF to be cleared) after resetting
FIFO will never see that happen.
Signed-off-by: Evgeny Iakovlev
---
hw/char/pl011.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 404d52a3b8..3184949d69 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -87,6 +87,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
}
+static inline void pl011_reset_pipe(PL011State *s)
+{
+s->read_count = 0;
+s->read_pos = 0;
+s->flags = PL011_FLAG_RXFE | PL011_FLAG_TXFE;
Should this really be resetting all the other flags to 0 ?
I think we should set/clear only the FIFO related flags, and
leave the others alone. We don't yet implement the
modem-status signals, but if/when we ever do, clearing them
would be the wrong thing here.
(Reset still needs to reset all the flag register bits.)
Right, i thought about it, but as you mention we only use FIFO flags
currently. Still, your suggestion about future changes makes sense.
thanks
-- PMM
Re: [PATCH v2 3/4] hw/char/pl011: better handling of FIFO flags on LCR reset
On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
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 guest-visible flag update will happen
> only on next data read or write attempt. As a result of that any guest
> that expects RXFE flag to be set (and RXFF to be cleared) after resetting
> FIFO will never see that happen.
>
> Signed-off-by: Evgeny Iakovlev
> ---
> hw/char/pl011.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 404d52a3b8..3184949d69 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -87,6 +87,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
> return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
> }
>
> +static inline void pl011_reset_pipe(PL011State *s)
> +{
> +s->read_count = 0;
> +s->read_pos = 0;
> +s->flags = PL011_FLAG_RXFE | PL011_FLAG_TXFE;
Should this really be resetting all the other flags to 0 ?
I think we should set/clear only the FIFO related flags, and
leave the others alone. We don't yet implement the
modem-status signals, but if/when we ever do, clearing them
would be the wrong thing here.
(Reset still needs to reset all the flag register bits.)
thanks
-- PMM
