Re: [PATCH v3 5/5] hw/char/pl011: check if UART is enabled before RX or TX operation

[Evgeny Iakovlev]

On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
> On 20/1/23 16:54, Evgeny Iakovlev wrote:
> > UART should be enabled in general and have RX enabled specifically to be
> > able to receive data from peripheral device. Same goes for transmitting
> > data to peripheral device and a TXE flag.
> >
> > Check if UART CR register has EN and RXE or TXE bits enabled before
> > trying to receive or transmit data.
> >
> > Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/char/pl011.c | 22 +++++++++++++++++++---
> >   1 file changed, 19 insertions(+), 3 deletions(-)
>
> > +static inline bool pl011_can_transmit(PL011State *s)
> > +{
> > +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
> > +}
> > +
> >   static void pl011_write(void *opaque, hwaddr offset,
> >                           uint64_t value, unsigned size)
> >   {
> > @@ -221,7 +231,9 @@ static void pl011_write(void *opaque, hwaddr offset,
> >         switch (offset >> 2) {
> >       case 0: /* UARTDR */
> > -        /* ??? Check if transmitter is enabled.  */
> > +        if (!pl011_can_transmit(s)) {
> > +            break;
> > +        }
> >           ch = value;
> >           /* XXX this blocks entire thread. Rewrite to use
> >            * qemu_chr_fe_write and background I/O callbacks */
> > @@ -292,7 +304,11 @@ static int pl011_can_receive(void *opaque)
> >       PL011State *s = (PL011State *)opaque;
> >       int r;
> >   -    r = s->read_count < pl011_get_fifo_depth(s);
> > +    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
>
> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Thanks! There's already a pl011_can_receive though, its the 
pl011_can_transmit that's new :)


> > +        r = 0;
> > +    } else {
> > +        r = s->read_count < pl011_get_fifo_depth(s);
> > +    }
> >       trace_pl011_can_receive(s->lcr, s->read_count, r);
> >       return r;
> >   }