|
From: | Evgeny Iakovlev |
Subject: | Re: [PATCH v3 5/5] hw/char/pl011: check if UART is enabled before RX or TX operation |
Date: | Mon, 23 Jan 2023 16:59:12 +0100 |
User-agent: | Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 |
On 1/23/2023 16:21, Philippe Mathieu-Daudé wrote:
On 23/1/23 15:43, Evgeny Iakovlev wrote: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 transmittingdata 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 :)pl011_can_receive() returns the number of bytes that pl011_receive() can accept, pl011_can_transmit() returns a boolean.
Hm, no, pl011_can_receive never actually returned the number of bytes. It's a boolean value as an int. Which is a bug, because qemu_chr_fe_set_handlers expects it to return the byte count, not a 0/1 value.
I'll fix it then.
I was thinking of: -- >8 -- diff --git a/hw/char/pl011.c b/hw/char/pl011.c index dd20b76609..ea5769a31c 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s) return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE; } +static inline bool pl011_can_receive(PL011State *s) +{ + return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE; +} + static void pl011_write(void *opaque, hwaddr offset, uint64_t value, unsigned size) {@@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,} } -static int pl011_can_receive(void *opaque) +static int pl011_receivable_bytes(void *opaque) { PL011State *s = (PL011State *)opaque; int r; - if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) { + if (!pl011_can_receive(s)) { r = 0; } else { r = s->read_count < pl011_get_fifo_depth(s);@@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error **errp){ PL011State *s = PL011(dev); - qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,+ qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes, pl011_receive,pl011_event, NULL, s, NULL, true); } --- with maybe a better name for pl011_receivable_bytes().
[Prev in Thread] | Current Thread | [Next in Thread] |