| Message ID | 20240719181041.49545-11-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Do not receive characters when UART or receiver are disabled. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > hw/char/pl011.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index c76283dccf..0ce91c13d3 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) > #define CR_OUT1 (1 << 12) > #define CR_RTS (1 << 11) > #define CR_DTR (1 << 10) > +#define CR_RXE (1 << 9) > #define CR_TXE (1 << 8) > #define CR_LBE (1 << 7) > #define CR_UARTEN (1 << 0) > @@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset, > static int pl011_can_receive(void *opaque) > { > PL011State *s = (PL011State *)opaque; > - int r; > + int r = 0; > > - r = s->read_count < pl011_get_fifo_depth(s); > + if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) { > + r = s->read_count < pl011_get_fifo_depth(s); > + } > trace_pl011_can_receive(s->lcr, s->read_count, r); > return r; I have a vague recollection of a previous conversation where we discussed whether tightening up the UART-enable checks would break existing only-tested-on-QEMU baremetal simple example code. But I can't find it in my email archive -- do you remember a discussion about this or am I misremembering? thanks -- PMM
Hi Peter, On 29/7/24 17:51, Peter Maydell wrote: > On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Do not receive characters when UART or receiver are disabled. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> hw/char/pl011.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c >> index c76283dccf..0ce91c13d3 100644 >> --- a/hw/char/pl011.c >> +++ b/hw/char/pl011.c >> @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) >> #define CR_OUT1 (1 << 12) >> #define CR_RTS (1 << 11) >> #define CR_DTR (1 << 10) >> +#define CR_RXE (1 << 9) >> #define CR_TXE (1 << 8) >> #define CR_LBE (1 << 7) >> #define CR_UARTEN (1 << 0) >> @@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset, >> static int pl011_can_receive(void *opaque) >> { >> PL011State *s = (PL011State *)opaque; >> - int r; >> + int r = 0; >> >> - r = s->read_count < pl011_get_fifo_depth(s); >> + if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) { >> + r = s->read_count < pl011_get_fifo_depth(s); >> + } >> trace_pl011_can_receive(s->lcr, s->read_count, r); >> return r; > > I have a vague recollection of a previous conversation > where we discussed whether tightening up the UART-enable > checks would break existing only-tested-on-QEMU baremetal > simple example code. But I can't find it in my email > archive -- do you remember a discussion about this or > am I misremembering? The previous conversation is: https://lore.kernel.org/qemu-devel/CAFEAcA_7dkWB9qPkzYmW6_F1eaAet0PPk0PHywGS2EpAkFAsUQ@mail.gmail.com/ I understood 'my first assembly "Hello, World" program' has to keep working, which only uses the TX channel; and the boot-serial-qtest is fixed in a previous patch. Now I get you were thinking of a wider baremetal example code. I'll convert to GUEST_ERROR log level, similar to the TX path, since it is still relevant for developers who plan to run their guest code on real hardware. Regards, Phil.
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index c76283dccf..0ce91c13d3 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) #define CR_OUT1 (1 << 12) #define CR_RTS (1 << 11) #define CR_DTR (1 << 10) +#define CR_RXE (1 << 9) #define CR_TXE (1 << 8) #define CR_LBE (1 << 7) #define CR_UARTEN (1 << 0) @@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset, static int pl011_can_receive(void *opaque) { PL011State *s = (PL011State *)opaque; - int r; + int r = 0; - r = s->read_count < pl011_get_fifo_depth(s); + if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) { + r = s->read_count < pl011_get_fifo_depth(s); + } trace_pl011_can_receive(s->lcr, s->read_count, r); return r; }