hw/char/ibex_uart.c | 20 +++++++++++++++----- include/hw/char/ibex_uart.h | 4 ++++ 2 files changed, 19 insertions(+), 5 deletions(-)
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 | 20 +++++++++++++++-----
include/hw/char/ibex_uart.h | 4 ++++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 89f1182c9b..dac09d53d6 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,8 @@ 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;
+ 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 +202,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 +247,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 +269,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 +373,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
ping
https://patchew.org/QEMU/20210215231528.2718086-1-alexander.wagner@ulal.de/
On 16.02.21 00:15, Alexander Wagner 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>
> ---
> hw/char/ibex_uart.c | 20 +++++++++++++++-----
> include/hw/char/ibex_uart.h | 4 ++++
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> index 89f1182c9b..dac09d53d6 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,8 @@ 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;
> + 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 +202,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 +247,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 +269,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 +373,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;
>
On Mon, Feb 15, 2021 at 9:06 PM 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>
Thanks for the patch!
I'm glad to see others using OT on QEMU.
> ---
> hw/char/ibex_uart.c | 20 +++++++++++++++-----
> include/hw/char/ibex_uart.h | 4 ++++
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> index 89f1182c9b..dac09d53d6 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,8 @@ 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;
> + s->uart_status |= R_STATUS_RXFULL_MASK;
Doesn't this mean we set RXFULL on every receive? Shouldn't this check
the rx_level first?
Alistair
> + s->rx_level += 1;
>
> if (size > rx_fifo_level) {
> s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK;
> @@ -199,6 +202,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 +247,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 +269,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 +373,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
>
>
On 08.03.21 14:47, Alistair Francis wrote:
>> hw/char/ibex_uart.c | 20 +++++++++++++++-----
>> include/hw/char/ibex_uart.h | 4 ++++
>> 2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
>> index 89f1182c9b..dac09d53d6 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,8 @@ 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;
>> + s->uart_status |= R_STATUS_RXFULL_MASK;
> Doesn't this mean we set RXFULL on every receive? Shouldn't this check
> the rx_level first?
>
> Alistair
Thank you for having a look! :)
Yes, this is correct. The RXFULL is currently set on every receive. The
RXFULL is used to indicate to QEMU that the device cannot receive any
further bytes.
As the FIFO buffers are currently not yet implemented I thought it would
make sense to behave like the OT UART could only receive one byte at a time.
Alex
On Tue, Mar 9, 2021 at 2:27 AM Alexander Wagner
<alexander.wagner@ulal.de> wrote:
>
>
> On 08.03.21 14:47, Alistair Francis wrote:
> >> hw/char/ibex_uart.c | 20 +++++++++++++++-----
> >> include/hw/char/ibex_uart.h | 4 ++++
> >> 2 files changed, 19 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> >> index 89f1182c9b..dac09d53d6 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,8 @@ 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;
> >> + s->uart_status |= R_STATUS_RXFULL_MASK;
> > Doesn't this mean we set RXFULL on every receive? Shouldn't this check
> > the rx_level first?
> >
> > Alistair
>
> Thank you for having a look! :)
>
> Yes, this is correct. The RXFULL is currently set on every receive. The
> RXFULL is used to indicate to QEMU that the device cannot receive any
> further bytes.
>
> As the FIFO buffers are currently not yet implemented I thought it would
> make sense to behave like the OT UART could only receive one byte at a time.
Ah, good point.
Can you add a comment where it is set describing that then?
Alistair
>
> Alex
>
On 09.03.21 15:29, Alistair Francis wrote:
> On Tue, Mar 9, 2021 at 2:27 AM Alexander Wagner
> <alexander.wagner@ulal.de> wrote:
>>
>> On 08.03.21 14:47, Alistair Francis wrote:
>>>> hw/char/ibex_uart.c | 20 +++++++++++++++-----
>>>> include/hw/char/ibex_uart.h | 4 ++++
>>>> 2 files changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
>>>> index 89f1182c9b..dac09d53d6 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,8 @@ 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;
>>>> + s->uart_status |= R_STATUS_RXFULL_MASK;
>>> Doesn't this mean we set RXFULL on every receive? Shouldn't this check
>>> the rx_level first?
>>>
>>> Alistair
>> Thank you for having a look! :)
>>
>> Yes, this is correct. The RXFULL is currently set on every receive. The
>> RXFULL is used to indicate to QEMU that the device cannot receive any
>> further bytes.
>>
>> As the FIFO buffers are currently not yet implemented I thought it would
>> make sense to behave like the OT UART could only receive one byte at a time.
> Ah, good point.
>
> Can you add a comment where it is set describing that then?
>
> Alistair
>
Sure, I just added a comment and emailed this as patch v2.
Alex
© 2016 - 2024 Red Hat, Inc.