* [PATCH v2] hw/char: disable ibex uart receive if the buffer is full
@ 2021-03-09 15:21 Alexander Wagner
2021-03-09 16:15 ` Alistair Francis
2021-03-09 21:30 ` Alistair Francis
0 siblings, 2 replies; 3+ messages in thread
From: Alexander Wagner @ 2021-03-09 15:21 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, Alistair.Francis, Alexander Wagner, pbonzini
Not disabling the UART leads to QEMU overwriting the UART receive buffer with
the newest received byte. The rx_level variable is added to allow the use of
the existing OpenTitan driver libraries.
Signed-off-by: Alexander Wagner <alexander.wagner@ulal.de>
---
hw/char/ibex_uart.c | 23 ++++++++++++++++++-----
include/hw/char/ibex_uart.h | 4 ++++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 89f1182c9b..bc3e2482bc 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -66,7 +66,8 @@ static int ibex_uart_can_receive(void *opaque)
{
IbexUartState *s = opaque;
- if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
+ if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK)
+ && !(s->uart_status & R_STATUS_RXFULL_MASK)) {
return 1;
}
@@ -83,6 +84,11 @@ static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
s->uart_status &= ~R_STATUS_RXIDLE_MASK;
s->uart_status &= ~R_STATUS_RXEMPTY_MASK;
+ /* The RXFULL is set after receiving a single byte
+ * as the FIFO buffers are not yet implemented.
+ */
+ s->uart_status |= R_STATUS_RXFULL_MASK;
+ s->rx_level += 1;
if (size > rx_fifo_level) {
s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK;
@@ -199,6 +205,7 @@ static void ibex_uart_reset(DeviceState *dev)
s->uart_timeout_ctrl = 0x00000000;
s->tx_level = 0;
+ s->rx_level = 0;
s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
@@ -243,11 +250,15 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
case R_RDATA:
retvalue = s->uart_rdata;
- if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
+ if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) && (s->rx_level > 0)) {
qemu_chr_fe_accept_input(&s->chr);
- s->uart_status |= R_STATUS_RXIDLE_MASK;
- s->uart_status |= R_STATUS_RXEMPTY_MASK;
+ s->rx_level -= 1;
+ s->uart_status &= ~R_STATUS_RXFULL_MASK;
+ if (s->rx_level == 0) {
+ s->uart_status |= R_STATUS_RXIDLE_MASK;
+ s->uart_status |= R_STATUS_RXEMPTY_MASK;
+ }
}
break;
case R_WDATA:
@@ -261,7 +272,8 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
case R_FIFO_STATUS:
retvalue = s->uart_fifo_status;
- retvalue |= s->tx_level & 0x1F;
+ retvalue |= (s->rx_level & 0x1F) << R_FIFO_STATUS_RXLVL_SHIFT;
+ retvalue |= (s->tx_level & 0x1F) << R_FIFO_STATUS_TXLVL_SHIFT;
qemu_log_mask(LOG_UNIMP,
"%s: RX fifos are not supported\n", __func__);
@@ -364,6 +376,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
s->uart_fifo_ctrl = value;
if (value & R_FIFO_CTRL_RXRST_MASK) {
+ s->rx_level = 0;
qemu_log_mask(LOG_UNIMP,
"%s: RX fifos are not supported\n", __func__);
}
diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
index 03d19e3f6f..546f958eb8 100644
--- a/include/hw/char/ibex_uart.h
+++ b/include/hw/char/ibex_uart.h
@@ -62,6 +62,8 @@ REG32(FIFO_CTRL, 0x1c)
FIELD(FIFO_CTRL, RXILVL, 2, 3)
FIELD(FIFO_CTRL, TXILVL, 5, 2)
REG32(FIFO_STATUS, 0x20)
+ FIELD(FIFO_STATUS, TXLVL, 0, 5)
+ FIELD(FIFO_STATUS, RXLVL, 16, 5)
REG32(OVRD, 0x24)
REG32(VAL, 0x28)
REG32(TIMEOUT_CTRL, 0x2c)
@@ -82,6 +84,8 @@ struct IbexUartState {
uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
uint32_t tx_level;
+ uint32_t rx_level;
+
QEMUTimer *fifo_trigger_handle;
uint64_t char_tx_time;
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/char: disable ibex uart receive if the buffer is full
2021-03-09 15:21 [PATCH v2] hw/char: disable ibex uart receive if the buffer is full Alexander Wagner
@ 2021-03-09 16:15 ` Alistair Francis
2021-03-09 21:30 ` Alistair Francis
1 sibling, 0 replies; 3+ messages in thread
From: Alistair Francis @ 2021-03-09 16:15 UTC (permalink / raw)
To: Alexander Wagner
Cc: Marc-André Lureau, Alistair Francis,
qemu-devel@nongnu.org Developers, Paolo Bonzini
On Tue, Mar 9, 2021 at 10:56 AM Alexander Wagner
<alexander.wagner@ulal.de> wrote:
>
> Not disabling the UART leads to QEMU overwriting the UART receive buffer with
> the newest received byte. The rx_level variable is added to allow the use of
> the existing OpenTitan driver libraries.
>
> Signed-off-by: Alexander Wagner <alexander.wagner@ulal.de>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/char/ibex_uart.c | 23 ++++++++++++++++++-----
> include/hw/char/ibex_uart.h | 4 ++++
> 2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> index 89f1182c9b..bc3e2482bc 100644
> --- a/hw/char/ibex_uart.c
> +++ b/hw/char/ibex_uart.c
> @@ -66,7 +66,8 @@ static int ibex_uart_can_receive(void *opaque)
> {
> IbexUartState *s = opaque;
>
> - if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
> + if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK)
> + && !(s->uart_status & R_STATUS_RXFULL_MASK)) {
> return 1;
> }
>
> @@ -83,6 +84,11 @@ static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
>
> s->uart_status &= ~R_STATUS_RXIDLE_MASK;
> s->uart_status &= ~R_STATUS_RXEMPTY_MASK;
> + /* The RXFULL is set after receiving a single byte
> + * as the FIFO buffers are not yet implemented.
> + */
> + s->uart_status |= R_STATUS_RXFULL_MASK;
> + s->rx_level += 1;
>
> if (size > rx_fifo_level) {
> s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK;
> @@ -199,6 +205,7 @@ static void ibex_uart_reset(DeviceState *dev)
> s->uart_timeout_ctrl = 0x00000000;
>
> s->tx_level = 0;
> + s->rx_level = 0;
>
> s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
>
> @@ -243,11 +250,15 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
>
> case R_RDATA:
> retvalue = s->uart_rdata;
> - if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
> + if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) && (s->rx_level > 0)) {
> qemu_chr_fe_accept_input(&s->chr);
>
> - s->uart_status |= R_STATUS_RXIDLE_MASK;
> - s->uart_status |= R_STATUS_RXEMPTY_MASK;
> + s->rx_level -= 1;
> + s->uart_status &= ~R_STATUS_RXFULL_MASK;
> + if (s->rx_level == 0) {
> + s->uart_status |= R_STATUS_RXIDLE_MASK;
> + s->uart_status |= R_STATUS_RXEMPTY_MASK;
> + }
> }
> break;
> case R_WDATA:
> @@ -261,7 +272,8 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
> case R_FIFO_STATUS:
> retvalue = s->uart_fifo_status;
>
> - retvalue |= s->tx_level & 0x1F;
> + retvalue |= (s->rx_level & 0x1F) << R_FIFO_STATUS_RXLVL_SHIFT;
> + retvalue |= (s->tx_level & 0x1F) << R_FIFO_STATUS_TXLVL_SHIFT;
>
> qemu_log_mask(LOG_UNIMP,
> "%s: RX fifos are not supported\n", __func__);
> @@ -364,6 +376,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
> s->uart_fifo_ctrl = value;
>
> if (value & R_FIFO_CTRL_RXRST_MASK) {
> + s->rx_level = 0;
> qemu_log_mask(LOG_UNIMP,
> "%s: RX fifos are not supported\n", __func__);
> }
> diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> index 03d19e3f6f..546f958eb8 100644
> --- a/include/hw/char/ibex_uart.h
> +++ b/include/hw/char/ibex_uart.h
> @@ -62,6 +62,8 @@ REG32(FIFO_CTRL, 0x1c)
> FIELD(FIFO_CTRL, RXILVL, 2, 3)
> FIELD(FIFO_CTRL, TXILVL, 5, 2)
> REG32(FIFO_STATUS, 0x20)
> + FIELD(FIFO_STATUS, TXLVL, 0, 5)
> + FIELD(FIFO_STATUS, RXLVL, 16, 5)
> REG32(OVRD, 0x24)
> REG32(VAL, 0x28)
> REG32(TIMEOUT_CTRL, 0x2c)
> @@ -82,6 +84,8 @@ struct IbexUartState {
> uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> uint32_t tx_level;
>
> + uint32_t rx_level;
> +
> QEMUTimer *fifo_trigger_handle;
> uint64_t char_tx_time;
>
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/char: disable ibex uart receive if the buffer is full
2021-03-09 15:21 [PATCH v2] hw/char: disable ibex uart receive if the buffer is full Alexander Wagner
2021-03-09 16:15 ` Alistair Francis
@ 2021-03-09 21:30 ` Alistair Francis
1 sibling, 0 replies; 3+ messages in thread
From: Alistair Francis @ 2021-03-09 21:30 UTC (permalink / raw)
To: Alexander Wagner
Cc: Marc-André Lureau, Alistair Francis,
qemu-devel@nongnu.org Developers, Paolo Bonzini
On Tue, Mar 9, 2021 at 10:56 AM Alexander Wagner
<alexander.wagner@ulal.de> wrote:
>
> Not disabling the UART leads to QEMU overwriting the UART receive buffer with
> the newest received byte. The rx_level variable is added to allow the use of
> the existing OpenTitan driver libraries.
>
> Signed-off-by: Alexander Wagner <alexander.wagner@ulal.de>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/char/ibex_uart.c | 23 ++++++++++++++++++-----
> include/hw/char/ibex_uart.h | 4 ++++
> 2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> index 89f1182c9b..bc3e2482bc 100644
> --- a/hw/char/ibex_uart.c
> +++ b/hw/char/ibex_uart.c
> @@ -66,7 +66,8 @@ static int ibex_uart_can_receive(void *opaque)
> {
> IbexUartState *s = opaque;
>
> - if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
> + if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK)
> + && !(s->uart_status & R_STATUS_RXFULL_MASK)) {
> return 1;
> }
>
> @@ -83,6 +84,11 @@ static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
>
> s->uart_status &= ~R_STATUS_RXIDLE_MASK;
> s->uart_status &= ~R_STATUS_RXEMPTY_MASK;
> + /* The RXFULL is set after receiving a single byte
> + * as the FIFO buffers are not yet implemented.
> + */
> + s->uart_status |= R_STATUS_RXFULL_MASK;
> + s->rx_level += 1;
>
> if (size > rx_fifo_level) {
> s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK;
> @@ -199,6 +205,7 @@ static void ibex_uart_reset(DeviceState *dev)
> s->uart_timeout_ctrl = 0x00000000;
>
> s->tx_level = 0;
> + s->rx_level = 0;
>
> s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
>
> @@ -243,11 +250,15 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
>
> case R_RDATA:
> retvalue = s->uart_rdata;
> - if (s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) {
> + if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) && (s->rx_level > 0)) {
> qemu_chr_fe_accept_input(&s->chr);
>
> - s->uart_status |= R_STATUS_RXIDLE_MASK;
> - s->uart_status |= R_STATUS_RXEMPTY_MASK;
> + s->rx_level -= 1;
> + s->uart_status &= ~R_STATUS_RXFULL_MASK;
> + if (s->rx_level == 0) {
> + s->uart_status |= R_STATUS_RXIDLE_MASK;
> + s->uart_status |= R_STATUS_RXEMPTY_MASK;
> + }
> }
> break;
> case R_WDATA:
> @@ -261,7 +272,8 @@ static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
> case R_FIFO_STATUS:
> retvalue = s->uart_fifo_status;
>
> - retvalue |= s->tx_level & 0x1F;
> + retvalue |= (s->rx_level & 0x1F) << R_FIFO_STATUS_RXLVL_SHIFT;
> + retvalue |= (s->tx_level & 0x1F) << R_FIFO_STATUS_TXLVL_SHIFT;
>
> qemu_log_mask(LOG_UNIMP,
> "%s: RX fifos are not supported\n", __func__);
> @@ -364,6 +376,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
> s->uart_fifo_ctrl = value;
>
> if (value & R_FIFO_CTRL_RXRST_MASK) {
> + s->rx_level = 0;
> qemu_log_mask(LOG_UNIMP,
> "%s: RX fifos are not supported\n", __func__);
> }
> diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> index 03d19e3f6f..546f958eb8 100644
> --- a/include/hw/char/ibex_uart.h
> +++ b/include/hw/char/ibex_uart.h
> @@ -62,6 +62,8 @@ REG32(FIFO_CTRL, 0x1c)
> FIELD(FIFO_CTRL, RXILVL, 2, 3)
> FIELD(FIFO_CTRL, TXILVL, 5, 2)
> REG32(FIFO_STATUS, 0x20)
> + FIELD(FIFO_STATUS, TXLVL, 0, 5)
> + FIELD(FIFO_STATUS, RXLVL, 16, 5)
> REG32(OVRD, 0x24)
> REG32(VAL, 0x28)
> REG32(TIMEOUT_CTRL, 0x2c)
> @@ -82,6 +84,8 @@ struct IbexUartState {
> uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> uint32_t tx_level;
>
> + uint32_t rx_level;
> +
> QEMUTimer *fifo_trigger_handle;
> uint64_t char_tx_time;
>
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-09 21:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-09 15:21 [PATCH v2] hw/char: disable ibex uart receive if the buffer is full Alexander Wagner
2021-03-09 16:15 ` Alistair Francis
2021-03-09 21:30 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).