[v5,07/11] hw/char: Initial commit of Ibex UART

Message ID	73cce2d0edd0d41ba15df403a2096bfa70bf0565.1590704015.git.alistair.francis@wdc.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> IronPort-SDR: iZnpbj2oUjsH3/vzE6bzHPuFUfmorNu+VjUQgOnlgO4w8/5B86tohsElpqwy4vYkpeThEZ9Bto ip3y0toTUbsu1+3Gemux4rp4z3CP5+ptg8bSapOJxsRPfOorA4V4mkX1Fv8t028Fcw5WLFWvbo rja6qcSPrL7E3oQDjc2dcdhj3Om1ecrp89YjQpX6pnC1XvYe7Cy1ccCpky7X7r5FAq/oSc0Ayp hqKw9T79ba18SxxdUWtCODhZ+TS+EdFUmwSV7IyPW1xWOSgh3jEUt5rxS89smx058gItKAlvaD jJs= IronPort-SDR: O0KvUEwL/MPsjiOOlsSnKJnXoj270tMcoxyuRMSwzFTr+345GvCQqIOzaqi3hjH1sNCZmzxJ2j K+1OhtQRRrKz4KJcPFnHg8xFibC5l1tV4= IronPort-SDR: vRW4s0tR7IA8NYTmubyZPApoYZbTOGnatZiyGT1MlIyvEa56wqCGolNNFvEBqbsxlx1AfvLKPM lk9OrD1oM+1A== WDCIronportException: Internal From: Alistair Francis <alistair.francis@wdc.com> To: qemu-devel@nongnu.org, qemu-riscv@nongnu.org Subject: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART Date: Thu, 28 May 2020 15:14:25 -0700 Message-Id: <73cce2d0edd0d41ba15df403a2096bfa70bf0565.1590704015.git.alistair.francis@wdc.com> In-Reply-To: <cover.1590704015.git.alistair.francis@wdc.com> References: <cover.1590704015.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=216.71.153.141; envelope-from=prvs=4104b2603=alistair.francis@wdc.com; helo=esa3.hgst.iphmx.com Precedence: list Cc: alistair.francis@wdc.com, bmeng.cn@gmail.com, palmer@dabbelt.com, alistair23@gmail.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>
Series	RISC-V Add the OpenTitan Machine \| expand [v5,00/11] RISC-V Add the OpenTitan Machine [v5,01/11] riscv/boot: Add a missing header include [v5,02/11] target/riscv: Don't overwrite the reset vector [v5,03/11] target/riscv: Disable the MMU correctly [v5,04/11] target/riscv: Don't set PMP feature in the cpu init [v5,05/11] target/riscv: Add the lowRISC Ibex CPU [v5,06/11] riscv: Initial commit of OpenTitan machine [v5,07/11] hw/char: Initial commit of Ibex UART [v5,08/11] hw/intc: Initial commit of lowRISC Ibex PLIC [v5,09/11] riscv/opentitan: Connect the PLIC device [v5,10/11] riscv/opentitan: Connect the UART device [v5,11/11] target/riscv: Use a smaller guess size for no-MMU PMP

Alistair Francis May 28, 2020, 10:14 p.m. UTC

This is the initial commit of the Ibex UART device. Serial TX is
working, while RX has been implemeneted but untested.

This is based on the documentation from:
https://docs.opentitan.org/hw/ip/uart/doc/

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/hw/char/ibex_uart.h | 110 ++++++++
 hw/char/ibex_uart.c         | 492 ++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |   2 +
 hw/char/Makefile.objs       |   1 +
 hw/riscv/Kconfig            |   4 +
 5 files changed, 609 insertions(+)
 create mode 100644 include/hw/char/ibex_uart.h
 create mode 100644 hw/char/ibex_uart.c

Alistair Francis June 1, 2020, 9:23 p.m. UTC | #1

On Thu, May 28, 2020 at 3:23 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> This is the initial commit of the Ibex UART device. Serial TX is
> working, while RX has been implemeneted but untested.
>
> This is based on the documentation from:
> https://docs.opentitan.org/hw/ip/uart/doc/
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Ping! This is the last patch not reviewed.

Alistair

> ---
>  include/hw/char/ibex_uart.h | 110 ++++++++
>  hw/char/ibex_uart.c         | 492 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |   2 +
>  hw/char/Makefile.objs       |   1 +
>  hw/riscv/Kconfig            |   4 +
>  5 files changed, 609 insertions(+)
>  create mode 100644 include/hw/char/ibex_uart.h
>  create mode 100644 hw/char/ibex_uart.c
>
> diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> new file mode 100644
> index 0000000000..2bec772615
> --- /dev/null
> +++ b/include/hw/char/ibex_uart.h
> @@ -0,0 +1,110 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_IBEX_UART_H
> +#define HW_IBEX_UART_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +#include "qemu/timer.h"
> +
> +#define IBEX_UART_INTR_STATE   0x00
> +    #define INTR_STATE_TX_WATERMARK (1 << 0)
> +    #define INTR_STATE_RX_WATERMARK (1 << 1)
> +    #define INTR_STATE_TX_EMPTY     (1 << 2)
> +    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
> +#define IBEX_UART_INTR_ENABLE  0x04
> +#define IBEX_UART_INTR_TEST    0x08
> +
> +#define IBEX_UART_CTRL         0x0c
> +    #define UART_CTRL_TX_ENABLE     (1 << 0)
> +    #define UART_CTRL_RX_ENABLE     (1 << 1)
> +    #define UART_CTRL_NF            (1 << 2)
> +    #define UART_CTRL_SLPBK         (1 << 4)
> +    #define UART_CTRL_LLPBK         (1 << 5)
> +    #define UART_CTRL_PARITY_EN     (1 << 6)
> +    #define UART_CTRL_PARITY_ODD    (1 << 7)
> +    #define UART_CTRL_RXBLVL        (3 << 8)
> +    #define UART_CTRL_NCO           (0xFFFF << 16)
> +
> +#define IBEX_UART_STATUS       0x10
> +    #define UART_STATUS_TXFULL  (1 << 0)
> +    #define UART_STATUS_RXFULL  (1 << 1)
> +    #define UART_STATUS_TXEMPTY (1 << 2)
> +    #define UART_STATUS_RXIDLE  (1 << 4)
> +    #define UART_STATUS_RXEMPTY (1 << 5)
> +
> +#define IBEX_UART_RDATA        0x14
> +#define IBEX_UART_WDATA        0x18
> +
> +#define IBEX_UART_FIFO_CTRL    0x1c
> +    #define FIFO_CTRL_RXRST          (1 << 0)
> +    #define FIFO_CTRL_TXRST          (1 << 1)
> +    #define FIFO_CTRL_RXILVL         (7 << 2)
> +    #define FIFO_CTRL_RXILVL_SHIFT   (2)
> +    #define FIFO_CTRL_TXILVL         (3 << 5)
> +    #define FIFO_CTRL_TXILVL_SHIFT   (5)
> +
> +#define IBEX_UART_FIFO_STATUS  0x20
> +#define IBEX_UART_OVRD         0x24
> +#define IBEX_UART_VAL          0x28
> +#define IBEX_UART_TIMEOUT_CTRL 0x2c
> +
> +#define IBEX_UART_TX_FIFO_SIZE 16
> +
> +#define TYPE_IBEX_UART "ibex-uart"
> +#define IBEX_UART(obj) \
> +    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
> +
> +typedef struct {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion mmio;
> +
> +    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> +    uint32_t tx_level;
> +
> +    QEMUTimer *fifo_trigger_handle;
> +    uint64_t char_tx_time;
> +
> +    uint32_t uart_intr_state;
> +    uint32_t uart_intr_enable;
> +    uint32_t uart_ctrl;
> +    uint32_t uart_status;
> +    uint32_t uart_rdata;
> +    uint32_t uart_fifo_ctrl;
> +    uint32_t uart_fifo_status;
> +    uint32_t uart_ovrd;
> +    uint32_t uart_val;
> +    uint32_t uart_timeout_ctrl;
> +
> +    CharBackend chr;
> +    qemu_irq tx_watermark;
> +    qemu_irq rx_watermark;
> +    qemu_irq tx_empty;
> +    qemu_irq rx_overflow;
> +} IbexUartState;
> +#endif /* HW_IBEX_UART_H */
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> new file mode 100644
> index 0000000000..c416325d73
> --- /dev/null
> +++ b/hw/char/ibex_uart.c
> @@ -0,0 +1,492 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * For details check the documentation here:
> + *    https://docs.opentitan.org/hw/ip/uart/doc/
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/ibex_uart.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static void ibex_uart_update_irqs(IbexUartState *s)
> +{
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
> +        qemu_set_irq(s->tx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->tx_watermark, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
> +        qemu_set_irq(s->rx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->rx_watermark, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> +        qemu_set_irq(s->tx_empty, 1);
> +    } else {
> +        qemu_set_irq(s->tx_empty, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
> +        qemu_set_irq(s->rx_overflow, 1);
> +    } else {
> +        qemu_set_irq(s->rx_overflow, 0);
> +    }
> +}
> +
> +static int ibex_uart_can_receive(void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +
> +    if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint8_t rx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_RXILVL)
> +                            >> FIFO_CTRL_RXILVL_SHIFT;
> +
> +    s->uart_rdata = *buf;
> +
> +    s->uart_status &= ~UART_STATUS_RXIDLE;
> +    s->uart_status &= ~UART_STATUS_RXEMPTY;
> +
> +    if (size > rx_fifo_level) {
> +        s->uart_intr_state |= INTR_STATE_RX_WATERMARK;
> +    }
> +
> +    ibex_uart_update_irqs(s);
> +}
> +
> +static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
> +                               void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> +                            >> FIFO_CTRL_TXILVL_SHIFT;
> +    int ret;
> +
> +    /* instant drain the fifo when there's no back-end */
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        s->tx_level = 0;
> +        return FALSE;
> +    }
> +
> +    if (!s->tx_level) {
> +        s->uart_status &= UART_STATUS_TXFULL;
> +        s->uart_status |= UART_STATUS_TXEMPTY;
> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> +        ibex_uart_update_irqs(s);
> +        return FALSE;
> +    }
> +
> +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
> +
> +    if (ret >= 0) {
> +        s->tx_level -= ret;
> +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
> +    }
> +
> +    if (s->tx_level) {
> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                        ibex_uart_xmit, s);
> +        if (!r) {
> +            s->tx_level = 0;
> +            return FALSE;
> +        }
> +    }
> +
> +    /* Clear the TX Full bit */
> +    if (s->tx_level != IBEX_UART_TX_FIFO_SIZE) {
> +        s->uart_status &= ~UART_STATUS_TXFULL;
> +    }
> +
> +    /* Disable the TX_WATERMARK IRQ */
> +    if (s->tx_level < tx_fifo_level) {
> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> +    }
> +
> +    /* Set TX empty */
> +    if (s->tx_level == 0) {
> +        s->uart_status |= UART_STATUS_TXEMPTY;
> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> +    }
> +
> +    ibex_uart_update_irqs(s);
> +    return FALSE;
> +}
> +
> +static void uart_write_tx_fifo(IbexUartState *s, const uint8_t *buf,
> +                               int size)
> +{
> +    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> +                            >> FIFO_CTRL_TXILVL_SHIFT;
> +
> +    if (size > IBEX_UART_TX_FIFO_SIZE - s->tx_level) {
> +        size = IBEX_UART_TX_FIFO_SIZE - s->tx_level;
> +        qemu_log_mask(LOG_GUEST_ERROR, "ibex_uart: TX FIFO overflow");
> +    }
> +
> +    memcpy(s->tx_fifo + s->tx_level, buf, size);
> +    s->tx_level += size;
> +
> +    if (s->tx_level > 0) {
> +        s->uart_status &= ~UART_STATUS_TXEMPTY;
> +    }
> +
> +    if (s->tx_level >= tx_fifo_level) {
> +        s->uart_intr_state |= INTR_STATE_TX_WATERMARK;
> +        ibex_uart_update_irqs(s);
> +    }
> +
> +    if (s->tx_level == IBEX_UART_TX_FIFO_SIZE) {
> +        s->uart_status |= UART_STATUS_TXFULL;
> +    }
> +
> +    timer_mod(s->fifo_trigger_handle, current_time +
> +              (s->char_tx_time * 4));
> +}
> +
> +static void ibex_uart_reset(DeviceState *dev)
> +{
> +    IbexUartState *s = IBEX_UART(dev);
> +
> +    s->uart_intr_state = 0x00000000;
> +    s->uart_intr_state = 0x00000000;
> +    s->uart_intr_enable = 0x00000000;
> +    s->uart_ctrl = 0x00000000;
> +    s->uart_status = 0x0000003c;
> +    s->uart_rdata = 0x00000000;
> +    s->uart_fifo_ctrl = 0x00000000;
> +    s->uart_fifo_status = 0x00000000;
> +    s->uart_ovrd = 0x00000000;
> +    s->uart_val = 0x00000000;
> +    s->uart_timeout_ctrl = 0x00000000;
> +
> +    s->tx_level = 0;
> +
> +    s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
> +
> +    ibex_uart_update_irqs(s);
> +}
> +
> +static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
> +                                       unsigned int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint64_t retvalue = 0;
> +
> +    switch (addr) {
> +    case IBEX_UART_INTR_STATE:
> +        retvalue = s->uart_intr_state;
> +        break;
> +    case IBEX_UART_INTR_ENABLE:
> +        retvalue = s->uart_intr_enable;
> +        break;
> +    case IBEX_UART_INTR_TEST:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: wdata is write only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_CTRL:
> +        retvalue = s->uart_ctrl;
> +        break;
> +    case IBEX_UART_STATUS:
> +        retvalue = s->uart_status;
> +        break;
> +
> +    case IBEX_UART_RDATA:
> +        retvalue = s->uart_rdata;
> +        if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> +            qemu_chr_fe_accept_input(&s->chr);
> +
> +            s->uart_status |= UART_STATUS_RXIDLE;
> +            s->uart_status |= UART_STATUS_RXEMPTY;
> +        }
> +        break;
> +    case IBEX_UART_WDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: wdata is write only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_FIFO_CTRL:
> +        retvalue = s->uart_fifo_ctrl;
> +        break;
> +    case IBEX_UART_FIFO_STATUS:
> +        retvalue = s->uart_fifo_status;
> +
> +        retvalue |= s->tx_level & 0x1F;
> +
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: RX fifos are not supported\n", __func__);
> +        break;
> +
> +    case IBEX_UART_OVRD:
> +        retvalue = s->uart_ovrd;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ovrd is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_VAL:
> +        retvalue = s->uart_val;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: val is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_TIMEOUT_CTRL:
> +        retvalue = s->uart_timeout_ctrl;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: timeout_ctrl is not supported\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +        return 0;
> +    }
> +
> +    return retvalue;
> +}
> +
> +static void ibex_uart_write(void *opaque, hwaddr addr,
> +                                  uint64_t val64, unsigned int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint32_t value = val64;
> +
> +    switch (addr) {
> +    case IBEX_UART_INTR_STATE:
> +        /* Write 1 clear */
> +        s->uart_intr_state &= ~value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_ENABLE:
> +        s->uart_intr_enable = value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_TEST:
> +        s->uart_intr_state |= value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +
> +    case IBEX_UART_CTRL:
> +        s->uart_ctrl = value;
> +
> +        if (value & UART_CTRL_NF) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_NF is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_SLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_LLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_EN) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_EN is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_ODD) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_RXBLVL) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_NCO) {
> +            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
> +            baud *= 1000;
> +            baud /= 2 ^ 20;
> +
> +            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> +        }
> +        break;
> +    case IBEX_UART_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_RDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: rdata is read only\n", __func__);
> +        break;
> +    case IBEX_UART_WDATA:
> +        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
> +        break;
> +
> +    case IBEX_UART_FIFO_CTRL:
> +        s->uart_fifo_ctrl = value;
> +
> +        if (value & FIFO_CTRL_RXRST) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: RX fifos are not supported\n", __func__);
> +        }
> +        if (value & FIFO_CTRL_TXRST) {
> +            s->tx_level = 0;
> +        }
> +        break;
> +    case IBEX_UART_FIFO_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fifo_status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_OVRD:
> +        s->uart_ovrd = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ovrd is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_VAL:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: val is read only\n", __func__);
> +        break;
> +    case IBEX_UART_TIMEOUT_CTRL:
> +        s->uart_timeout_ctrl = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: timeout_ctrl is not supported\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +}
> +
> +static void fifo_trigger_update(void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +
> +    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
> +        ibex_uart_xmit(NULL, G_IO_OUT, s);
> +    }
> +}
> +
> +static const MemoryRegionOps ibex_uart_ops = {
> +    .read = ibex_uart_read,
> +    .write = ibex_uart_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +};
> +
> +static int cadence_uart_post_load(void *opaque, int version_id)
> +{
> +    IbexUartState *s = opaque;
> +
> +    ibex_uart_update_irqs(s);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ibex_uart = {
> +    .name = TYPE_IBEX_UART,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = cadence_uart_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(tx_fifo, IbexUartState,
> +                            IBEX_UART_TX_FIFO_SIZE),
> +        VMSTATE_UINT32(tx_level, IbexUartState),
> +        VMSTATE_UINT64(char_tx_time, IbexUartState),
> +        VMSTATE_TIMER_PTR(fifo_trigger_handle, IbexUartState),
> +        VMSTATE_UINT32(uart_intr_state, IbexUartState),
> +        VMSTATE_UINT32(uart_intr_enable, IbexUartState),
> +        VMSTATE_UINT32(uart_ctrl, IbexUartState),
> +        VMSTATE_UINT32(uart_status, IbexUartState),
> +        VMSTATE_UINT32(uart_rdata, IbexUartState),
> +        VMSTATE_UINT32(uart_fifo_ctrl, IbexUartState),
> +        VMSTATE_UINT32(uart_fifo_status, IbexUartState),
> +        VMSTATE_UINT32(uart_ovrd, IbexUartState),
> +        VMSTATE_UINT32(uart_val, IbexUartState),
> +        VMSTATE_UINT32(uart_timeout_ctrl, IbexUartState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property ibex_uart_properties[] = {
> +    DEFINE_PROP_CHR("chardev", IbexUartState, chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ibex_uart_init(Object *obj)
> +{
> +    IbexUartState *s = IBEX_UART(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_watermark);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_watermark);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_empty);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_overflow);
> +
> +    memory_region_init_io(&s->mmio, obj, &ibex_uart_ops, s,
> +                          TYPE_IBEX_UART, 0x400);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +
> +static void ibex_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    IbexUartState *s = IBEX_UART(dev);
> +
> +    s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                          fifo_trigger_update, s);
> +
> +    qemu_chr_fe_set_handlers(&s->chr, ibex_uart_can_receive,
> +                             ibex_uart_receive, NULL, NULL,
> +                             s, NULL, true);
> +}
> +
> +static void ibex_uart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = ibex_uart_reset;
> +    dc->realize = ibex_uart_realize;
> +    dc->vmsd = &vmstate_ibex_uart;
> +    device_class_set_props(dc, ibex_uart_properties);
> +}
> +
> +static const TypeInfo ibex_uart_info = {
> +    .name          = TYPE_IBEX_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IbexUartState),
> +    .instance_init = ibex_uart_init,
> +    .class_init    = ibex_uart_class_init,
> +};
> +
> +static void ibex_uart_register_types(void)
> +{
> +    type_register_static(&ibex_uart_info);
> +}
> +
> +type_init(ibex_uart_register_types)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e7d9cb0a5..a1ce186a7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1245,7 +1245,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
>  L: qemu-riscv@nongnu.org
>  S: Supported
>  F: hw/riscv/opentitan.c
> +F: hw/char/ibex_uart.c
>  F: include/hw/riscv/opentitan.h
> +F: include/hw/char/ibex_uart.h
>
>  SH4 Machines
>  ------------
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 9e9a6c1aff..633996be5b 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
>  common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
>  common-obj-$(CONFIG_XEN) += xen_console.o
>  common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> +common-obj-$(CONFIG_IBEX) += ibex_uart.o
>
>  common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
>  common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index 94d19571f7..28947ef3e0 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -4,6 +4,9 @@ config HTIF
>  config HART
>      bool
>
> +config IBEX
> +    bool
> +
>  config SIFIVE
>      bool
>      select MSI_NONBROKEN
> @@ -29,6 +32,7 @@ config SPIKE
>
>  config OPENTITAN
>      bool
> +    select IBEX
>      select HART
>      select UNIMP
>
> --
> 2.26.2
>

LIU Zhiwei June 2, 2020, 11:22 a.m. UTC | #2

On 2020/5/29 6:14, Alistair Francis wrote:
> This is the initial commit of the Ibex UART device. Serial TX is
> working, while RX has been implemeneted but untested.
>
> This is based on the documentation from:
> https://docs.opentitan.org/hw/ip/uart/doc/
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   include/hw/char/ibex_uart.h | 110 ++++++++
>   hw/char/ibex_uart.c         | 492 ++++++++++++++++++++++++++++++++++++
>   MAINTAINERS                 |   2 +
>   hw/char/Makefile.objs       |   1 +
>   hw/riscv/Kconfig            |   4 +
>   5 files changed, 609 insertions(+)
>   create mode 100644 include/hw/char/ibex_uart.h
>   create mode 100644 hw/char/ibex_uart.c
>
> diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> new file mode 100644
> index 0000000000..2bec772615
> --- /dev/null
> +++ b/include/hw/char/ibex_uart.h
> @@ -0,0 +1,110 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_IBEX_UART_H
> +#define HW_IBEX_UART_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +#include "qemu/timer.h"
> +
> +#define IBEX_UART_INTR_STATE   0x00
> +    #define INTR_STATE_TX_WATERMARK (1 << 0)
> +    #define INTR_STATE_RX_WATERMARK (1 << 1)
> +    #define INTR_STATE_TX_EMPTY     (1 << 2)
> +    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
> +#define IBEX_UART_INTR_ENABLE  0x04
> +#define IBEX_UART_INTR_TEST    0x08
> +
> +#define IBEX_UART_CTRL         0x0c
> +    #define UART_CTRL_TX_ENABLE     (1 << 0)
> +    #define UART_CTRL_RX_ENABLE     (1 << 1)
> +    #define UART_CTRL_NF            (1 << 2)
> +    #define UART_CTRL_SLPBK         (1 << 4)
> +    #define UART_CTRL_LLPBK         (1 << 5)
> +    #define UART_CTRL_PARITY_EN     (1 << 6)
> +    #define UART_CTRL_PARITY_ODD    (1 << 7)
> +    #define UART_CTRL_RXBLVL        (3 << 8)
> +    #define UART_CTRL_NCO           (0xFFFF << 16)
> +
> +#define IBEX_UART_STATUS       0x10
> +    #define UART_STATUS_TXFULL  (1 << 0)
> +    #define UART_STATUS_RXFULL  (1 << 1)
> +    #define UART_STATUS_TXEMPTY (1 << 2)
> +    #define UART_STATUS_RXIDLE  (1 << 4)
> +    #define UART_STATUS_RXEMPTY (1 << 5)
> +
> +#define IBEX_UART_RDATA        0x14
> +#define IBEX_UART_WDATA        0x18
> +
> +#define IBEX_UART_FIFO_CTRL    0x1c
> +    #define FIFO_CTRL_RXRST          (1 << 0)
> +    #define FIFO_CTRL_TXRST          (1 << 1)
> +    #define FIFO_CTRL_RXILVL         (7 << 2)
> +    #define FIFO_CTRL_RXILVL_SHIFT   (2)
> +    #define FIFO_CTRL_TXILVL         (3 << 5)
> +    #define FIFO_CTRL_TXILVL_SHIFT   (5)
> +
> +#define IBEX_UART_FIFO_STATUS  0x20
> +#define IBEX_UART_OVRD         0x24
> +#define IBEX_UART_VAL          0x28
> +#define IBEX_UART_TIMEOUT_CTRL 0x2c
> +
> +#define IBEX_UART_TX_FIFO_SIZE 16
> +
> +#define TYPE_IBEX_UART "ibex-uart"
> +#define IBEX_UART(obj) \
> +    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
> +
> +typedef struct {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion mmio;
> +
> +    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> +    uint32_t tx_level;
> +
> +    QEMUTimer *fifo_trigger_handle;
> +    uint64_t char_tx_time;
> +
> +    uint32_t uart_intr_state;
> +    uint32_t uart_intr_enable;
> +    uint32_t uart_ctrl;
> +    uint32_t uart_status;
> +    uint32_t uart_rdata;
> +    uint32_t uart_fifo_ctrl;
> +    uint32_t uart_fifo_status;
> +    uint32_t uart_ovrd;
> +    uint32_t uart_val;
> +    uint32_t uart_timeout_ctrl;
> +
> +    CharBackend chr;
> +    qemu_irq tx_watermark;
> +    qemu_irq rx_watermark;
> +    qemu_irq tx_empty;
> +    qemu_irq rx_overflow;
> +} IbexUartState;
> +#endif /* HW_IBEX_UART_H */
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> new file mode 100644
> index 0000000000..c416325d73
> --- /dev/null
> +++ b/hw/char/ibex_uart.c
> @@ -0,0 +1,492 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * For details check the documentation here:
> + *    https://docs.opentitan.org/hw/ip/uart/doc/
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/ibex_uart.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static void ibex_uart_update_irqs(IbexUartState *s)
> +{
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
> +        qemu_set_irq(s->tx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->tx_watermark, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
> +        qemu_set_irq(s->rx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->rx_watermark, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> +        qemu_set_irq(s->tx_empty, 1);
> +    } else {
> +        qemu_set_irq(s->tx_empty, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
> +        qemu_set_irq(s->rx_overflow, 1);
> +    } else {
> +        qemu_set_irq(s->rx_overflow, 0);
> +    }
> +}
> +
> +static int ibex_uart_can_receive(void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +
> +    if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint8_t rx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_RXILVL)
> +                            >> FIFO_CTRL_RXILVL_SHIFT;
> +
> +    s->uart_rdata = *buf;
> +
> +    s->uart_status &= ~UART_STATUS_RXIDLE;
> +    s->uart_status &= ~UART_STATUS_RXEMPTY;
> +
> +    if (size > rx_fifo_level) {
> +        s->uart_intr_state |= INTR_STATE_RX_WATERMARK;
> +    }
> +
> +    ibex_uart_update_irqs(s);
> +}
> +
> +static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
> +                               void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> +                            >> FIFO_CTRL_TXILVL_SHIFT;
> +    int ret;
> +
> +    /* instant drain the fifo when there's no back-end */
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        s->tx_level = 0;
> +        return FALSE;
> +    }
> +
> +    if (!s->tx_level) {
> +        s->uart_status &= UART_STATUS_TXFULL;
Maybe  s->uart_status &= ~UART_STATUS_TXFULL;
> +        s->uart_status |= UART_STATUS_TXEMPTY;
> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> +        ibex_uart_update_irqs(s);
> +        return FALSE;
> +    }
> +
> +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
> +
> +    if (ret >= 0) {
> +        s->tx_level -= ret;
> +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
> +    }
> +
> +    if (s->tx_level) {
> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                        ibex_uart_xmit, s);
> +        if (!r) {
> +            s->tx_level = 0;
> +            return FALSE;
> +        }
> +    }
> +
> +    /* Clear the TX Full bit */
> +    if (s->tx_level != IBEX_UART_TX_FIFO_SIZE) {
> +        s->uart_status &= ~UART_STATUS_TXFULL;
> +    }
> +
> +    /* Disable the TX_WATERMARK IRQ */
> +    if (s->tx_level < tx_fifo_level) {
> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> +    }
> +
> +    /* Set TX empty */
> +    if (s->tx_level == 0) {
> +        s->uart_status |= UART_STATUS_TXEMPTY;
> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> +    }
> +
> +    ibex_uart_update_irqs(s);
> +    return FALSE;
> +}
> +
> +static void uart_write_tx_fifo(IbexUartState *s, const uint8_t *buf,
> +                               int size)
> +{
> +    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> +                            >> FIFO_CTRL_TXILVL_SHIFT;
> +
> +    if (size > IBEX_UART_TX_FIFO_SIZE - s->tx_level) {
> +        size = IBEX_UART_TX_FIFO_SIZE - s->tx_level;
> +        qemu_log_mask(LOG_GUEST_ERROR, "ibex_uart: TX FIFO overflow");
> +    }
> +
> +    memcpy(s->tx_fifo + s->tx_level, buf, size);
> +    s->tx_level += size;
> +
> +    if (s->tx_level > 0) {
> +        s->uart_status &= ~UART_STATUS_TXEMPTY;
> +    }
> +
> +    if (s->tx_level >= tx_fifo_level) {
> +        s->uart_intr_state |= INTR_STATE_TX_WATERMARK;
> +        ibex_uart_update_irqs(s);
> +    }
> +
> +    if (s->tx_level == IBEX_UART_TX_FIFO_SIZE) {
> +        s->uart_status |= UART_STATUS_TXFULL;
> +    }
> +
> +    timer_mod(s->fifo_trigger_handle, current_time +
> +              (s->char_tx_time * 4));
> +}
> +
> +static void ibex_uart_reset(DeviceState *dev)
> +{
> +    IbexUartState *s = IBEX_UART(dev);
> +
> +    s->uart_intr_state = 0x00000000;
> +    s->uart_intr_state = 0x00000000;
> +    s->uart_intr_enable = 0x00000000;
> +    s->uart_ctrl = 0x00000000;
> +    s->uart_status = 0x0000003c;
> +    s->uart_rdata = 0x00000000;
> +    s->uart_fifo_ctrl = 0x00000000;
> +    s->uart_fifo_status = 0x00000000;
> +    s->uart_ovrd = 0x00000000;
> +    s->uart_val = 0x00000000;
> +    s->uart_timeout_ctrl = 0x00000000;
> +
> +    s->tx_level = 0;
> +
> +    s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
> +
> +    ibex_uart_update_irqs(s);
> +}
> +
> +static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
> +                                       unsigned int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint64_t retvalue = 0;
> +
> +    switch (addr) {
> +    case IBEX_UART_INTR_STATE:
> +        retvalue = s->uart_intr_state;
> +        break;
> +    case IBEX_UART_INTR_ENABLE:
> +        retvalue = s->uart_intr_enable;
> +        break;
> +    case IBEX_UART_INTR_TEST:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: wdata is write only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_CTRL:
> +        retvalue = s->uart_ctrl;
> +        break;
> +    case IBEX_UART_STATUS:
> +        retvalue = s->uart_status;
> +        break;
> +
> +    case IBEX_UART_RDATA:
> +        retvalue = s->uart_rdata;
> +        if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> +            qemu_chr_fe_accept_input(&s->chr);
> +
> +            s->uart_status |= UART_STATUS_RXIDLE;
> +            s->uart_status |= UART_STATUS_RXEMPTY;
> +        }
> +        break;
> +    case IBEX_UART_WDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: wdata is write only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_FIFO_CTRL:
> +        retvalue = s->uart_fifo_ctrl;
> +        break;
> +    case IBEX_UART_FIFO_STATUS:
> +        retvalue = s->uart_fifo_status;
> +
> +        retvalue |= s->tx_level & 0x1F;
> +
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: RX fifos are not supported\n", __func__);
> +        break;
> +
> +    case IBEX_UART_OVRD:
> +        retvalue = s->uart_ovrd;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ovrd is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_VAL:
> +        retvalue = s->uart_val;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: val is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_TIMEOUT_CTRL:
> +        retvalue = s->uart_timeout_ctrl;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: timeout_ctrl is not supported\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +        return 0;
> +    }
> +
> +    return retvalue;
> +}
> +
> +static void ibex_uart_write(void *opaque, hwaddr addr,
> +                                  uint64_t val64, unsigned int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint32_t value = val64;
> +
> +    switch (addr) {
> +    case IBEX_UART_INTR_STATE:
> +        /* Write 1 clear */
> +        s->uart_intr_state &= ~value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_ENABLE:
> +        s->uart_intr_enable = value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_TEST:
> +        s->uart_intr_state |= value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +
> +    case IBEX_UART_CTRL:
> +        s->uart_ctrl = value;
> +
> +        if (value & UART_CTRL_NF) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_NF is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_SLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_LLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_EN) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_EN is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_ODD) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_RXBLVL) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_NCO) {
> +            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
> +            baud *= 1000;
> +            baud /= 2 ^ 20;
> +
> +            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> +        }
> +        break;
> +    case IBEX_UART_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_RDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: rdata is read only\n", __func__);
> +        break;
> +    case IBEX_UART_WDATA:
> +        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
> +        break;
> +
> +    case IBEX_UART_FIFO_CTRL:
> +        s->uart_fifo_ctrl = value;
> +
> +        if (value & FIFO_CTRL_RXRST) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: RX fifos are not supported\n", __func__);
> +        }
> +        if (value & FIFO_CTRL_TXRST) {
> +            s->tx_level = 0;
> +        }
> +        break;
> +    case IBEX_UART_FIFO_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fifo_status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_OVRD:
> +        s->uart_ovrd = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ovrd is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_VAL:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: val is read only\n", __func__);
> +        break;
> +    case IBEX_UART_TIMEOUT_CTRL:
> +        s->uart_timeout_ctrl = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: timeout_ctrl is not supported\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +}
> +
> +static void fifo_trigger_update(void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +
> +    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
> +        ibex_uart_xmit(NULL, G_IO_OUT, s);
> +    }
> +}
> +
> +static const MemoryRegionOps ibex_uart_ops = {
> +    .read = ibex_uart_read,
> +    .write = ibex_uart_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +};
> +
> +static int cadence_uart_post_load(void *opaque, int version_id)
Not a good name.
> +{
> +    IbexUartState *s = opaque;
> +
> +    ibex_uart_update_irqs(s);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ibex_uart = {
> +    .name = TYPE_IBEX_UART,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = cadence_uart_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(tx_fifo, IbexUartState,
> +                            IBEX_UART_TX_FIFO_SIZE),
> +        VMSTATE_UINT32(tx_level, IbexUartState),
> +        VMSTATE_UINT64(char_tx_time, IbexUartState),
> +        VMSTATE_TIMER_PTR(fifo_trigger_handle, IbexUartState),
> +        VMSTATE_UINT32(uart_intr_state, IbexUartState),
> +        VMSTATE_UINT32(uart_intr_enable, IbexUartState),
> +        VMSTATE_UINT32(uart_ctrl, IbexUartState),
> +        VMSTATE_UINT32(uart_status, IbexUartState),
> +        VMSTATE_UINT32(uart_rdata, IbexUartState),
> +        VMSTATE_UINT32(uart_fifo_ctrl, IbexUartState),
> +        VMSTATE_UINT32(uart_fifo_status, IbexUartState),
> +        VMSTATE_UINT32(uart_ovrd, IbexUartState),
> +        VMSTATE_UINT32(uart_val, IbexUartState),
> +        VMSTATE_UINT32(uart_timeout_ctrl, IbexUartState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property ibex_uart_properties[] = {
> +    DEFINE_PROP_CHR("chardev", IbexUartState, chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ibex_uart_init(Object *obj)
> +{
> +    IbexUartState *s = IBEX_UART(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_watermark);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_watermark);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_empty);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_overflow);
> +
> +    memory_region_init_io(&s->mmio, obj, &ibex_uart_ops, s,
> +                          TYPE_IBEX_UART, 0x400);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +
> +static void ibex_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    IbexUartState *s = IBEX_UART(dev);
> +
> +    s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                          fifo_trigger_update, s);
> +
> +    qemu_chr_fe_set_handlers(&s->chr, ibex_uart_can_receive,
> +                             ibex_uart_receive, NULL, NULL,
> +                             s, NULL, true);
> +}
> +
> +static void ibex_uart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = ibex_uart_reset;
> +    dc->realize = ibex_uart_realize;
> +    dc->vmsd = &vmstate_ibex_uart;
> +    device_class_set_props(dc, ibex_uart_properties);
> +}
> +
> +static const TypeInfo ibex_uart_info = {
> +    .name          = TYPE_IBEX_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IbexUartState),
> +    .instance_init = ibex_uart_init,
> +    .class_init    = ibex_uart_class_init,
> +};
> +
> +static void ibex_uart_register_types(void)
> +{
> +    type_register_static(&ibex_uart_info);
> +}
> +
> +type_init(ibex_uart_register_types)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e7d9cb0a5..a1ce186a7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1245,7 +1245,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
>   L: qemu-riscv@nongnu.org
>   S: Supported
>   F: hw/riscv/opentitan.c
> +F: hw/char/ibex_uart.c
>   F: include/hw/riscv/opentitan.h
> +F: include/hw/char/ibex_uart.h
>   
>   SH4 Machines
>   ------------
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 9e9a6c1aff..633996be5b 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
>   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
>   common-obj-$(CONFIG_XEN) += xen_console.o
>   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> +common-obj-$(CONFIG_IBEX) += ibex_uart.o
>   
>   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
>   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index 94d19571f7..28947ef3e0 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -4,6 +4,9 @@ config HTIF
>   config HART
>       bool
>   
> +config IBEX
> +    bool
> +
>   config SIFIVE
>       bool
>       select MSI_NONBROKEN
> @@ -29,6 +32,7 @@ config SPIKE
>   
>   config OPENTITAN
>       bool
> +    select IBEX
>       select HART
>       select UNIMP
>

LIU Zhiwei June 2, 2020, 12:28 p.m. UTC | #3

Hi Alistair,

There are still some questions I don't understand.

1. Is the baud rate  or fifo a necessary feature to simulate?
As you can see, qemu_chr_fe_write will send the byte as soon as possible.
When you want to transmit a byte through WDATA,  you can call 
qemu_chr_fe_write directly.

2.  The baud rate calculation method is not strictly right.
I think when a byte write to FIFO,  char_tx_time * 8 is the correct time 
to send the byte instead of
char_tx_time * 4.

3.  Why add a watch here?
> +        s->uart_status |= UART_STATUS_TXEMPTY;
> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> +        ibex_uart_update_irqs(s);
> +        return FALSE;
> +    }
> +
> +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
> +
> +    if (ret >= 0) {
> +        s->tx_level -= ret;
> +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
> +    }
> +
> +    if (s->tx_level) {
> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                        ibex_uart_xmit, s);
> +        if (!r) {
> +            s->tx_level = 0;
> +            return FALSE;
> +        }
> +    }
> + 

Zhiwei

Alistair Francis June 2, 2020, 5:46 p.m. UTC | #4

On Tue, Jun 2, 2020 at 4:22 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
>
>
> On 2020/5/29 6:14, Alistair Francis wrote:
> > This is the initial commit of the Ibex UART device. Serial TX is
> > working, while RX has been implemeneted but untested.
> >
> > This is based on the documentation from:
> > https://docs.opentitan.org/hw/ip/uart/doc/
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   include/hw/char/ibex_uart.h | 110 ++++++++
> >   hw/char/ibex_uart.c         | 492 ++++++++++++++++++++++++++++++++++++
> >   MAINTAINERS                 |   2 +
> >   hw/char/Makefile.objs       |   1 +
> >   hw/riscv/Kconfig            |   4 +
> >   5 files changed, 609 insertions(+)
> >   create mode 100644 include/hw/char/ibex_uart.h
> >   create mode 100644 hw/char/ibex_uart.c
> >
> > diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> > new file mode 100644
> > index 0000000000..2bec772615
> > --- /dev/null
> > +++ b/include/hw/char/ibex_uart.h
> > @@ -0,0 +1,110 @@
> > +/*
> > + * QEMU lowRISC Ibex UART device
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#ifndef HW_IBEX_UART_H
> > +#define HW_IBEX_UART_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "chardev/char-fe.h"
> > +#include "qemu/timer.h"
> > +
> > +#define IBEX_UART_INTR_STATE   0x00
> > +    #define INTR_STATE_TX_WATERMARK (1 << 0)
> > +    #define INTR_STATE_RX_WATERMARK (1 << 1)
> > +    #define INTR_STATE_TX_EMPTY     (1 << 2)
> > +    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
> > +#define IBEX_UART_INTR_ENABLE  0x04
> > +#define IBEX_UART_INTR_TEST    0x08
> > +
> > +#define IBEX_UART_CTRL         0x0c
> > +    #define UART_CTRL_TX_ENABLE     (1 << 0)
> > +    #define UART_CTRL_RX_ENABLE     (1 << 1)
> > +    #define UART_CTRL_NF            (1 << 2)
> > +    #define UART_CTRL_SLPBK         (1 << 4)
> > +    #define UART_CTRL_LLPBK         (1 << 5)
> > +    #define UART_CTRL_PARITY_EN     (1 << 6)
> > +    #define UART_CTRL_PARITY_ODD    (1 << 7)
> > +    #define UART_CTRL_RXBLVL        (3 << 8)
> > +    #define UART_CTRL_NCO           (0xFFFF << 16)
> > +
> > +#define IBEX_UART_STATUS       0x10
> > +    #define UART_STATUS_TXFULL  (1 << 0)
> > +    #define UART_STATUS_RXFULL  (1 << 1)
> > +    #define UART_STATUS_TXEMPTY (1 << 2)
> > +    #define UART_STATUS_RXIDLE  (1 << 4)
> > +    #define UART_STATUS_RXEMPTY (1 << 5)
> > +
> > +#define IBEX_UART_RDATA        0x14
> > +#define IBEX_UART_WDATA        0x18
> > +
> > +#define IBEX_UART_FIFO_CTRL    0x1c
> > +    #define FIFO_CTRL_RXRST          (1 << 0)
> > +    #define FIFO_CTRL_TXRST          (1 << 1)
> > +    #define FIFO_CTRL_RXILVL         (7 << 2)
> > +    #define FIFO_CTRL_RXILVL_SHIFT   (2)
> > +    #define FIFO_CTRL_TXILVL         (3 << 5)
> > +    #define FIFO_CTRL_TXILVL_SHIFT   (5)
> > +
> > +#define IBEX_UART_FIFO_STATUS  0x20
> > +#define IBEX_UART_OVRD         0x24
> > +#define IBEX_UART_VAL          0x28
> > +#define IBEX_UART_TIMEOUT_CTRL 0x2c
> > +
> > +#define IBEX_UART_TX_FIFO_SIZE 16
> > +
> > +#define TYPE_IBEX_UART "ibex-uart"
> > +#define IBEX_UART(obj) \
> > +    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
> > +
> > +typedef struct {
> > +    /* <private> */
> > +    SysBusDevice parent_obj;
> > +
> > +    /* <public> */
> > +    MemoryRegion mmio;
> > +
> > +    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> > +    uint32_t tx_level;
> > +
> > +    QEMUTimer *fifo_trigger_handle;
> > +    uint64_t char_tx_time;
> > +
> > +    uint32_t uart_intr_state;
> > +    uint32_t uart_intr_enable;
> > +    uint32_t uart_ctrl;
> > +    uint32_t uart_status;
> > +    uint32_t uart_rdata;
> > +    uint32_t uart_fifo_ctrl;
> > +    uint32_t uart_fifo_status;
> > +    uint32_t uart_ovrd;
> > +    uint32_t uart_val;
> > +    uint32_t uart_timeout_ctrl;
> > +
> > +    CharBackend chr;
> > +    qemu_irq tx_watermark;
> > +    qemu_irq rx_watermark;
> > +    qemu_irq tx_empty;
> > +    qemu_irq rx_overflow;
> > +} IbexUartState;
> > +#endif /* HW_IBEX_UART_H */
> > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> > new file mode 100644
> > index 0000000000..c416325d73
> > --- /dev/null
> > +++ b/hw/char/ibex_uart.c
> > @@ -0,0 +1,492 @@
> > +/*
> > + * QEMU lowRISC Ibex UART device
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * For details check the documentation here:
> > + *    https://docs.opentitan.org/hw/ip/uart/doc/
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/char/ibex_uart.h"
> > +#include "hw/irq.h"
> > +#include "hw/qdev-properties.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +
> > +static void ibex_uart_update_irqs(IbexUartState *s)
> > +{
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
> > +        qemu_set_irq(s->tx_watermark, 1);
> > +    } else {
> > +        qemu_set_irq(s->tx_watermark, 0);
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
> > +        qemu_set_irq(s->rx_watermark, 1);
> > +    } else {
> > +        qemu_set_irq(s->rx_watermark, 0);
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> > +        qemu_set_irq(s->tx_empty, 1);
> > +    } else {
> > +        qemu_set_irq(s->tx_empty, 0);
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
> > +        qemu_set_irq(s->rx_overflow, 1);
> > +    } else {
> > +        qemu_set_irq(s->rx_overflow, 0);
> > +    }
> > +}
> > +
> > +static int ibex_uart_can_receive(void *opaque)
> > +{
> > +    IbexUartState *s = opaque;
> > +
> > +    if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
> > +{
> > +    IbexUartState *s = opaque;
> > +    uint8_t rx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_RXILVL)
> > +                            >> FIFO_CTRL_RXILVL_SHIFT;
> > +
> > +    s->uart_rdata = *buf;
> > +
> > +    s->uart_status &= ~UART_STATUS_RXIDLE;
> > +    s->uart_status &= ~UART_STATUS_RXEMPTY;
> > +
> > +    if (size > rx_fifo_level) {
> > +        s->uart_intr_state |= INTR_STATE_RX_WATERMARK;
> > +    }
> > +
> > +    ibex_uart_update_irqs(s);
> > +}
> > +
> > +static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
> > +                               void *opaque)
> > +{
> > +    IbexUartState *s = opaque;
> > +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> > +                            >> FIFO_CTRL_TXILVL_SHIFT;
> > +    int ret;
> > +
> > +    /* instant drain the fifo when there's no back-end */
> > +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> > +        s->tx_level = 0;
> > +        return FALSE;
> > +    }
> > +
> > +    if (!s->tx_level) {
> > +        s->uart_status &= UART_STATUS_TXFULL;
> Maybe  s->uart_status &= ~UART_STATUS_TXFULL;

Good spot, fixed.

> > +        s->uart_status |= UART_STATUS_TXEMPTY;
> > +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> > +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> > +        ibex_uart_update_irqs(s);
> > +        return FALSE;
> > +    }
> > +
> > +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
> > +
> > +    if (ret >= 0) {
> > +        s->tx_level -= ret;
> > +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
> > +    }
> > +
> > +    if (s->tx_level) {
> > +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> > +                                        ibex_uart_xmit, s);
> > +        if (!r) {
> > +            s->tx_level = 0;
> > +            return FALSE;
> > +        }
> > +    }
> > +
> > +    /* Clear the TX Full bit */
> > +    if (s->tx_level != IBEX_UART_TX_FIFO_SIZE) {
> > +        s->uart_status &= ~UART_STATUS_TXFULL;
> > +    }
> > +
> > +    /* Disable the TX_WATERMARK IRQ */
> > +    if (s->tx_level < tx_fifo_level) {
> > +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> > +    }
> > +
> > +    /* Set TX empty */
> > +    if (s->tx_level == 0) {
> > +        s->uart_status |= UART_STATUS_TXEMPTY;
> > +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> > +    }
> > +
> > +    ibex_uart_update_irqs(s);
> > +    return FALSE;
> > +}
> > +
> > +static void uart_write_tx_fifo(IbexUartState *s, const uint8_t *buf,
> > +                               int size)
> > +{
> > +    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
> > +                            >> FIFO_CTRL_TXILVL_SHIFT;
> > +
> > +    if (size > IBEX_UART_TX_FIFO_SIZE - s->tx_level) {
> > +        size = IBEX_UART_TX_FIFO_SIZE - s->tx_level;
> > +        qemu_log_mask(LOG_GUEST_ERROR, "ibex_uart: TX FIFO overflow");
> > +    }
> > +
> > +    memcpy(s->tx_fifo + s->tx_level, buf, size);
> > +    s->tx_level += size;
> > +
> > +    if (s->tx_level > 0) {
> > +        s->uart_status &= ~UART_STATUS_TXEMPTY;
> > +    }
> > +
> > +    if (s->tx_level >= tx_fifo_level) {
> > +        s->uart_intr_state |= INTR_STATE_TX_WATERMARK;
> > +        ibex_uart_update_irqs(s);
> > +    }
> > +
> > +    if (s->tx_level == IBEX_UART_TX_FIFO_SIZE) {
> > +        s->uart_status |= UART_STATUS_TXFULL;
> > +    }
> > +
> > +    timer_mod(s->fifo_trigger_handle, current_time +
> > +              (s->char_tx_time * 4));
> > +}
> > +
> > +static void ibex_uart_reset(DeviceState *dev)
> > +{
> > +    IbexUartState *s = IBEX_UART(dev);
> > +
> > +    s->uart_intr_state = 0x00000000;
> > +    s->uart_intr_state = 0x00000000;
> > +    s->uart_intr_enable = 0x00000000;
> > +    s->uart_ctrl = 0x00000000;
> > +    s->uart_status = 0x0000003c;
> > +    s->uart_rdata = 0x00000000;
> > +    s->uart_fifo_ctrl = 0x00000000;
> > +    s->uart_fifo_status = 0x00000000;
> > +    s->uart_ovrd = 0x00000000;
> > +    s->uart_val = 0x00000000;
> > +    s->uart_timeout_ctrl = 0x00000000;
> > +
> > +    s->tx_level = 0;
> > +
> > +    s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
> > +
> > +    ibex_uart_update_irqs(s);
> > +}
> > +
> > +static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
> > +                                       unsigned int size)
> > +{
> > +    IbexUartState *s = opaque;
> > +    uint64_t retvalue = 0;
> > +
> > +    switch (addr) {
> > +    case IBEX_UART_INTR_STATE:
> > +        retvalue = s->uart_intr_state;
> > +        break;
> > +    case IBEX_UART_INTR_ENABLE:
> > +        retvalue = s->uart_intr_enable;
> > +        break;
> > +    case IBEX_UART_INTR_TEST:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: wdata is write only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_CTRL:
> > +        retvalue = s->uart_ctrl;
> > +        break;
> > +    case IBEX_UART_STATUS:
> > +        retvalue = s->uart_status;
> > +        break;
> > +
> > +    case IBEX_UART_RDATA:
> > +        retvalue = s->uart_rdata;
> > +        if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
> > +            qemu_chr_fe_accept_input(&s->chr);
> > +
> > +            s->uart_status |= UART_STATUS_RXIDLE;
> > +            s->uart_status |= UART_STATUS_RXEMPTY;
> > +        }
> > +        break;
> > +    case IBEX_UART_WDATA:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: wdata is write only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_FIFO_CTRL:
> > +        retvalue = s->uart_fifo_ctrl;
> > +        break;
> > +    case IBEX_UART_FIFO_STATUS:
> > +        retvalue = s->uart_fifo_status;
> > +
> > +        retvalue |= s->tx_level & 0x1F;
> > +
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: RX fifos are not supported\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_OVRD:
> > +        retvalue = s->uart_ovrd;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: ovrd is not supported\n", __func__);
> > +        break;
> > +    case IBEX_UART_VAL:
> > +        retvalue = s->uart_val;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: val is not supported\n", __func__);
> > +        break;
> > +    case IBEX_UART_TIMEOUT_CTRL:
> > +        retvalue = s->uart_timeout_ctrl;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: timeout_ctrl is not supported\n", __func__);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> > +        return 0;
> > +    }
> > +
> > +    return retvalue;
> > +}
> > +
> > +static void ibex_uart_write(void *opaque, hwaddr addr,
> > +                                  uint64_t val64, unsigned int size)
> > +{
> > +    IbexUartState *s = opaque;
> > +    uint32_t value = val64;
> > +
> > +    switch (addr) {
> > +    case IBEX_UART_INTR_STATE:
> > +        /* Write 1 clear */
> > +        s->uart_intr_state &= ~value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +    case IBEX_UART_INTR_ENABLE:
> > +        s->uart_intr_enable = value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +    case IBEX_UART_INTR_TEST:
> > +        s->uart_intr_state |= value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +
> > +    case IBEX_UART_CTRL:
> > +        s->uart_ctrl = value;
> > +
> > +        if (value & UART_CTRL_NF) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_NF is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_SLPBK) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_LLPBK) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_PARITY_EN) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_PARITY_EN is not supported\n",
> > +                          __func__);
> > +        }
> > +        if (value & UART_CTRL_PARITY_ODD) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
> > +                          __func__);
> > +        }
> > +        if (value & UART_CTRL_RXBLVL) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_NCO) {
> > +            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
> > +            baud *= 1000;
> > +            baud /= 2 ^ 20;
> > +
> > +            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> > +        }
> > +        break;
> > +    case IBEX_UART_STATUS:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: status is read only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_RDATA:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: rdata is read only\n", __func__);
> > +        break;
> > +    case IBEX_UART_WDATA:
> > +        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
> > +        break;
> > +
> > +    case IBEX_UART_FIFO_CTRL:
> > +        s->uart_fifo_ctrl = value;
> > +
> > +        if (value & FIFO_CTRL_RXRST) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: RX fifos are not supported\n", __func__);
> > +        }
> > +        if (value & FIFO_CTRL_TXRST) {
> > +            s->tx_level = 0;
> > +        }
> > +        break;
> > +    case IBEX_UART_FIFO_STATUS:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: fifo_status is read only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_OVRD:
> > +        s->uart_ovrd = value;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: ovrd is not supported\n", __func__);
> > +        break;
> > +    case IBEX_UART_VAL:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: val is read only\n", __func__);
> > +        break;
> > +    case IBEX_UART_TIMEOUT_CTRL:
> > +        s->uart_timeout_ctrl = value;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: timeout_ctrl is not supported\n", __func__);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> > +    }
> > +}
> > +
> > +static void fifo_trigger_update(void *opaque)
> > +{
> > +    IbexUartState *s = opaque;
> > +
> > +    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
> > +        ibex_uart_xmit(NULL, G_IO_OUT, s);
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps ibex_uart_ops = {
> > +    .read = ibex_uart_read,
> > +    .write = ibex_uart_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .impl.min_access_size = 4,
> > +    .impl.max_access_size = 4,
> > +};
> > +
> > +static int cadence_uart_post_load(void *opaque, int version_id)
> Not a good name.

Also fixed.

Alistair

> > +{
> > +    IbexUartState *s = opaque;
> > +
> > +    ibex_uart_update_irqs(s);
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_ibex_uart = {
> > +    .name = TYPE_IBEX_UART,
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .post_load = cadence_uart_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8_ARRAY(tx_fifo, IbexUartState,
> > +                            IBEX_UART_TX_FIFO_SIZE),
> > +        VMSTATE_UINT32(tx_level, IbexUartState),
> > +        VMSTATE_UINT64(char_tx_time, IbexUartState),
> > +        VMSTATE_TIMER_PTR(fifo_trigger_handle, IbexUartState),
> > +        VMSTATE_UINT32(uart_intr_state, IbexUartState),
> > +        VMSTATE_UINT32(uart_intr_enable, IbexUartState),
> > +        VMSTATE_UINT32(uart_ctrl, IbexUartState),
> > +        VMSTATE_UINT32(uart_status, IbexUartState),
> > +        VMSTATE_UINT32(uart_rdata, IbexUartState),
> > +        VMSTATE_UINT32(uart_fifo_ctrl, IbexUartState),
> > +        VMSTATE_UINT32(uart_fifo_status, IbexUartState),
> > +        VMSTATE_UINT32(uart_ovrd, IbexUartState),
> > +        VMSTATE_UINT32(uart_val, IbexUartState),
> > +        VMSTATE_UINT32(uart_timeout_ctrl, IbexUartState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static Property ibex_uart_properties[] = {
> > +    DEFINE_PROP_CHR("chardev", IbexUartState, chr),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void ibex_uart_init(Object *obj)
> > +{
> > +    IbexUartState *s = IBEX_UART(obj);
> > +
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_watermark);
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_watermark);
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_empty);
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_overflow);
> > +
> > +    memory_region_init_io(&s->mmio, obj, &ibex_uart_ops, s,
> > +                          TYPE_IBEX_UART, 0x400);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> > +}
> > +
> > +static void ibex_uart_realize(DeviceState *dev, Error **errp)
> > +{
> > +    IbexUartState *s = IBEX_UART(dev);
> > +
> > +    s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +                                          fifo_trigger_update, s);
> > +
> > +    qemu_chr_fe_set_handlers(&s->chr, ibex_uart_can_receive,
> > +                             ibex_uart_receive, NULL, NULL,
> > +                             s, NULL, true);
> > +}
> > +
> > +static void ibex_uart_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->reset = ibex_uart_reset;
> > +    dc->realize = ibex_uart_realize;
> > +    dc->vmsd = &vmstate_ibex_uart;
> > +    device_class_set_props(dc, ibex_uart_properties);
> > +}
> > +
> > +static const TypeInfo ibex_uart_info = {
> > +    .name          = TYPE_IBEX_UART,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(IbexUartState),
> > +    .instance_init = ibex_uart_init,
> > +    .class_init    = ibex_uart_class_init,
> > +};
> > +
> > +static void ibex_uart_register_types(void)
> > +{
> > +    type_register_static(&ibex_uart_info);
> > +}
> > +
> > +type_init(ibex_uart_register_types)
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3e7d9cb0a5..a1ce186a7e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1245,7 +1245,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
> >   L: qemu-riscv@nongnu.org
> >   S: Supported
> >   F: hw/riscv/opentitan.c
> > +F: hw/char/ibex_uart.c
> >   F: include/hw/riscv/opentitan.h
> > +F: include/hw/char/ibex_uart.h
> >
> >   SH4 Machines
> >   ------------
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index 9e9a6c1aff..633996be5b 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
> >   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
> >   common-obj-$(CONFIG_XEN) += xen_console.o
> >   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> > +common-obj-$(CONFIG_IBEX) += ibex_uart.o
> >
> >   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
> >   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index 94d19571f7..28947ef3e0 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -4,6 +4,9 @@ config HTIF
> >   config HART
> >       bool
> >
> > +config IBEX
> > +    bool
> > +
> >   config SIFIVE
> >       bool
> >       select MSI_NONBROKEN
> > @@ -29,6 +32,7 @@ config SPIKE
> >
> >   config OPENTITAN
> >       bool
> > +    select IBEX
> >       select HART
> >       select UNIMP
> >
>
>

Alistair Francis June 2, 2020, 5:54 p.m. UTC | #5

On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
> Hi Alistair,
>
> There are still some questions I don't understand.
>
> 1. Is the baud rate  or fifo a necessary feature to simulate?
> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
> When you want to transmit a byte through WDATA,  you can call
> qemu_chr_fe_write directly.

So qemu_chr_fe_write() will send the data straight away. This doesn't
match what teh hardware does though. So by modelling a FIFO and a
delay in sending we can better match the hardware.

>
> 2.  The baud rate calculation method is not strictly right.
> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
> to send the byte instead of
> char_tx_time * 4.

Do you mind explaining why 8 is correct instead of 4?

>
> 3.  Why add a watch here?

This is based on the Cadence UART implementation in QEMU (which does
the same thing). This will trigger a callback when we can write more
data or when the backend has hung up.

Alistair

> > +        s->uart_status |= UART_STATUS_TXEMPTY;
> > +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
> > +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
> > +        ibex_uart_update_irqs(s);
> > +        return FALSE;
> > +    }
> > +
> > +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
> > +
> > +    if (ret >= 0) {
> > +        s->tx_level -= ret;
> > +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
> > +    }
> > +
> > +    if (s->tx_level) {
> > +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> > +                                        ibex_uart_xmit, s);
> > +        if (!r) {
> > +            s->tx_level = 0;
> > +            return FALSE;
> > +        }
> > +    }
> > +
>
> Zhiwei
>
>

LIU Zhiwei June 3, 2020, 10:33 a.m. UTC | #6

On 2020/6/3 1:54, Alistair Francis wrote:
> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
>> Hi Alistair,
>>
>> There are still some questions I don't understand.
>>
>> 1. Is the baud rate  or fifo a necessary feature to simulate?
>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
>> When you want to transmit a byte through WDATA,  you can call
>> qemu_chr_fe_write directly.
> So qemu_chr_fe_write() will send the data straight away. This doesn't
> match what teh hardware does though. So by modelling a FIFO and a
> delay in sending we can better match the hardware.
I see many UARTs have similar features. Does the software really care about
these features? Usually I just want to print something to the terminal 
through UART.
Most simulation in QEMU is for running software, not exactly the details 
of hardware.
For example, we will not simulate the 16x oversamples in this UART.

There is no error here. Personally I  think it is necessary to simulate 
the FIFO and baud rate,
maybe for supporting some backends.

Can someone give a reasonable answer for this question?
>> 2.  The baud rate calculation method is not strictly right.
>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
>> to send the byte instead of
>> char_tx_time * 4.
> Do you mind explaining why 8 is correct instead of 4?
Usually write a byte to WDATA will trigger a uart_write_tx_fifo. 
Translate a bit will take
char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
>> 3.  Why add a watch here?
> This is based on the Cadence UART implementation in QEMU (which does
> the same thing). This will trigger a callback when we can write more
> data or when the backend has hung up.
Many other serials do the same thing, like virtio-console and serial. So 
it may be a common
interface here. I will try to understand it(Not yet).

Zhiwei
> Alistair
>
>>> +        s->uart_status |= UART_STATUS_TXEMPTY;
>>> +        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
>>> +        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
>>> +        ibex_uart_update_irqs(s);
>>> +        return FALSE;
>>> +    }
>>> +
>>> +    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
>>> +
>>> +    if (ret >= 0) {
>>> +        s->tx_level -= ret;
>>> +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
>>> +    }
>>> +
>>> +    if (s->tx_level) {
>>> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
>>> +                                        ibex_uart_xmit, s);
>>> +        if (!r) {
>>> +            s->tx_level = 0;
>>> +            return FALSE;
>>> +        }
>>> +    }
>>> +
>> Zhiwei
>>
>>

Alistair Francis June 3, 2020, 3:56 p.m. UTC | #7

On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
> On 2020/6/3 1:54, Alistair Francis wrote:
> > On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
> >> Hi Alistair,
> >>
> >> There are still some questions I don't understand.
> >>
> >> 1. Is the baud rate  or fifo a necessary feature to simulate?
> >> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
> >> When you want to transmit a byte through WDATA,  you can call
> >> qemu_chr_fe_write directly.
> > So qemu_chr_fe_write() will send the data straight away. This doesn't
> > match what teh hardware does though. So by modelling a FIFO and a
> > delay in sending we can better match the hardware.
> I see many UARTs have similar features. Does the software really care about
> these features? Usually I just want to print something to the terminal
> through UART.

In this case Tock (which is the OS used for OpenTitan) does car about
these features as it relies on interrupts generated by the HW to
complete the serial send task. It also just makes the QEMU model more
accurate.

> Most simulation in QEMU is for running software, not exactly the details
> of hardware.
> For example, we will not simulate the 16x oversamples in this UART.

Agreed. Lots of UARTs don't bother modelling the delay from the
hardware as generally it doesn't matter. In this case it does make a
difference for the software and it makes the QEMU model more accurate,
which is always a good thing.

>
> There is no error here. Personally I  think it is necessary to simulate
> the FIFO and baud rate,
> maybe for supporting some backends.

So baud rate doesn't need to be modelled as we aren't actually sending
UART data, just pretending and then printing it.

>
> Can someone give a reasonable answer for this question?

Which question?

> >> 2.  The baud rate calculation method is not strictly right.
> >> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
> >> to send the byte instead of
> >> char_tx_time * 4.
> > Do you mind explaining why 8 is correct instead of 4?
> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
> Translate a bit will take
> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.

I see your point. I just used the 4 as that is what the Cadence one
does. I don't think it matters too much as it's just the delay for a
timer (that isn't used as an accurate timer).

> >> 3.  Why add a watch here?
> > This is based on the Cadence UART implementation in QEMU (which does
> > the same thing). This will trigger a callback when we can write more
> > data or when the backend has hung up.
> Many other serials do the same thing, like virtio-console and serial. So
> it may be a common
> interface here. I will try to understand it(Not yet).

Yep, it's just a more complete model of that the HW does.

Alistair

LIU Zhiwei June 4, 2020, 1:59 a.m. UTC | #8

On 2020/6/3 23:56, Alistair Francis wrote:
> On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>> On 2020/6/3 1:54, Alistair Francis wrote:
>>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
>>>> Hi Alistair,
>>>>
>>>> There are still some questions I don't understand.
>>>>
>>>> 1. Is the baud rate  or fifo a necessary feature to simulate?
>>>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
>>>> When you want to transmit a byte through WDATA,  you can call
>>>> qemu_chr_fe_write directly.
>>> So qemu_chr_fe_write() will send the data straight away. This doesn't
>>> match what teh hardware does though. So by modelling a FIFO and a
>>> delay in sending we can better match the hardware.
>> I see many UARTs have similar features. Does the software really care about
>> these features? Usually I just want to print something to the terminal
>> through UART.
> In this case Tock (which is the OS used for OpenTitan) does car about
> these features as it relies on interrupts generated by the HW to
> complete the serial send task. It also just makes the QEMU model more
> accurate.

Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO. 
At least,
it can verify the ISP.
>> Most simulation in QEMU is for running software, not exactly the details
>> of hardware.
>> For example, we will not simulate the 16x oversamples in this UART.
> Agreed. Lots of UARTs don't bother modelling the delay from the
> hardware as generally it doesn't matter. In this case it does make a
> difference for the software and it makes the QEMU model more accurate,
> which is always a good thing.
>
>> There is no error here. Personally I  think it is necessary to simulate
>> the FIFO and baud rate,
>> maybe for supporting some backends.
> So baud rate doesn't need to be modelled as we aren't actually sending
> UART data, just pretending and then printing it.
>
>> Can someone give a reasonable answer for this question?
> Which question?
I see  the UART can work with many  different backends,  such as pty , 
file, socket and so on.
I wonder if this a backend, which has some requirements on the baud 
rate.  You can ignore it,
as it doesn't matter.
>
>>>> 2.  The baud rate calculation method is not strictly right.
>>>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
>>>> to send the byte instead of
>>>> char_tx_time * 4.
>>> Do you mind explaining why 8 is correct instead of 4?
>> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
>> Translate a bit will take
>> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
> I see your point. I just used the 4 as that is what the Cadence one
> does. I don't think it matters too much as it's just the delay for a
> timer (that isn't used as an accurate timer).
Got it. Just a way to send the bytes at sometime later.
>>>> 3.  Why add a watch here?
>>> This is based on the Cadence UART implementation in QEMU (which does
>>> the same thing). This will trigger a callback when we can write more
>>> data or when the backend has hung up.
>> Many other serials do the same thing, like virtio-console and serial. So
>> it may be a common
>> interface here. I will try to understand it(Not yet).
> Yep, it's just a more complete model of that the HW does.
I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback 
function.
The breakpoint did't match.

I just wonder if there is a case really need the callback function.

Zhiwei
>
> Alistair

Alistair Francis June 4, 2020, 4:35 a.m. UTC | #9

On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
>
>
> On 2020/6/3 23:56, Alistair Francis wrote:
> > On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >> On 2020/6/3 1:54, Alistair Francis wrote:
> >>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
> >>>> Hi Alistair,
> >>>>
> >>>> There are still some questions I don't understand.
> >>>>
> >>>> 1. Is the baud rate  or fifo a necessary feature to simulate?
> >>>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
> >>>> When you want to transmit a byte through WDATA,  you can call
> >>>> qemu_chr_fe_write directly.
> >>> So qemu_chr_fe_write() will send the data straight away. This doesn't
> >>> match what teh hardware does though. So by modelling a FIFO and a
> >>> delay in sending we can better match the hardware.
> >> I see many UARTs have similar features. Does the software really care about
> >> these features? Usually I just want to print something to the terminal
> >> through UART.
> > In this case Tock (which is the OS used for OpenTitan) does car about
> > these features as it relies on interrupts generated by the HW to
> > complete the serial send task. It also just makes the QEMU model more
> > accurate.
>
> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
> At least,
> it can verify the ISP.

Exactly :)

> >> Most simulation in QEMU is for running software, not exactly the details
> >> of hardware.
> >> For example, we will not simulate the 16x oversamples in this UART.
> > Agreed. Lots of UARTs don't bother modelling the delay from the
> > hardware as generally it doesn't matter. In this case it does make a
> > difference for the software and it makes the QEMU model more accurate,
> > which is always a good thing.
> >
> >> There is no error here. Personally I  think it is necessary to simulate
> >> the FIFO and baud rate,
> >> maybe for supporting some backends.
> > So baud rate doesn't need to be modelled as we aren't actually sending
> > UART data, just pretending and then printing it.
> >
> >> Can someone give a reasonable answer for this question?
> > Which question?
> I see  the UART can work with many  different backends,  such as pty ,
> file, socket and so on.
> I wonder if this a backend, which has some requirements on the baud

The backend should be independent so it doesn't matter what baud rate
we choose here.

> rate.  You can ignore it,
> as it doesn't matter.
> >
> >>>> 2.  The baud rate calculation method is not strictly right.
> >>>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
> >>>> to send the byte instead of
> >>>> char_tx_time * 4.
> >>> Do you mind explaining why 8 is correct instead of 4?
> >> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
> >> Translate a bit will take
> >> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
> > I see your point. I just used the 4 as that is what the Cadence one
> > does. I don't think it matters too much as it's just the delay for a
> > timer (that isn't used as an accurate timer).
> Got it. Just a way to send the bytes at sometime later.
> >>>> 3.  Why add a watch here?
> >>> This is based on the Cadence UART implementation in QEMU (which does
> >>> the same thing). This will trigger a callback when we can write more
> >>> data or when the backend has hung up.
> >> Many other serials do the same thing, like virtio-console and serial. So
> >> it may be a common
> >> interface here. I will try to understand it(Not yet).
> > Yep, it's just a more complete model of that the HW does.
> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
> function.
> The breakpoint did't match.
>
> I just wonder if there is a case really need the callback function.

AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
shouldn't be triggered.

Alistair

>
> Zhiwei
> >
> > Alistair
>

LIU Zhiwei June 4, 2020, 5:05 a.m. UTC | #10

On 2020/6/4 12:35, Alistair Francis wrote:
> On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>
>>
>> On 2020/6/3 23:56, Alistair Francis wrote:
>>> On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>>> On 2020/6/3 1:54, Alistair Francis wrote:
>>>>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
>>>>>> Hi Alistair,
>>>>>>
>>>>>> There are still some questions I don't understand.
>>>>>>
>>>>>> 1. Is the baud rate  or fifo a necessary feature to simulate?
>>>>>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
>>>>>> When you want to transmit a byte through WDATA,  you can call
>>>>>> qemu_chr_fe_write directly.
>>>>> So qemu_chr_fe_write() will send the data straight away. This doesn't
>>>>> match what teh hardware does though. So by modelling a FIFO and a
>>>>> delay in sending we can better match the hardware.
>>>> I see many UARTs have similar features. Does the software really care about
>>>> these features? Usually I just want to print something to the terminal
>>>> through UART.
>>> In this case Tock (which is the OS used for OpenTitan) does car about
>>> these features as it relies on interrupts generated by the HW to
>>> complete the serial send task. It also just makes the QEMU model more
>>> accurate.
>> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
>> At least,
>> it can verify the ISP.
> Exactly :)
>
>>>> Most simulation in QEMU is for running software, not exactly the details
>>>> of hardware.
>>>> For example, we will not simulate the 16x oversamples in this UART.
>>> Agreed. Lots of UARTs don't bother modelling the delay from the
>>> hardware as generally it doesn't matter. In this case it does make a
>>> difference for the software and it makes the QEMU model more accurate,
>>> which is always a good thing.
>>>
>>>> There is no error here. Personally I  think it is necessary to simulate
>>>> the FIFO and baud rate,
>>>> maybe for supporting some backends.
>>> So baud rate doesn't need to be modelled as we aren't actually sending
>>> UART data, just pretending and then printing it.
>>>
>>>> Can someone give a reasonable answer for this question?
>>> Which question?
>> I see  the UART can work with many  different backends,  such as pty ,
>> file, socket and so on.
>> I wonder if this a backend, which has some requirements on the baud
> The backend should be independent so it doesn't matter what baud rate
> we choose here.
>
>> rate.  You can ignore it,
>> as it doesn't matter.
>>>>>> 2.  The baud rate calculation method is not strictly right.
>>>>>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
>>>>>> to send the byte instead of
>>>>>> char_tx_time * 4.
>>>>> Do you mind explaining why 8 is correct instead of 4?
>>>> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
>>>> Translate a bit will take
>>>> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
>>> I see your point. I just used the 4 as that is what the Cadence one
>>> does. I don't think it matters too much as it's just the delay for a
>>> timer (that isn't used as an accurate timer).
>> Got it. Just a way to send the bytes at sometime later.
>>>>>> 3.  Why add a watch here?
>>>>> This is based on the Cadence UART implementation in QEMU (which does
>>>>> the same thing). This will trigger a callback when we can write more
>>>>> data or when the backend has hung up.
>>>> Many other serials do the same thing, like virtio-console and serial. So
>>>> it may be a common
>>>> interface here. I will try to understand it(Not yet).
>>> Yep, it's just a more complete model of that the HW does.
>> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
>> function.
>> The breakpoint did't match.
>>
>> I just wonder if there is a case really need the callback function.
> AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
> shouldn't be triggered.
I tried with the UART in the virt board.  It also registers the watch 
callback on
G_IO_HUP and G_IO_OUT.

However I will through the question alone in another mail.

After the two points addressed,

Reviewed-by: LIU Zhiwei<zhiwei_liu@c-sky.com>

Zhiwei
>
> Alistair
>
>> Zhiwei
>>> Alistair

LIU Zhiwei June 4, 2020, 5:40 a.m. UTC | #11

On 2020/6/4 12:35, Alistair Francis wrote:
> On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>
>>
>> On 2020/6/3 23:56, Alistair Francis wrote:
>>> On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>>> On 2020/6/3 1:54, Alistair Francis wrote:
>>>>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
>>>>>> Hi Alistair,
>>>>>>
>>>>>> There are still some questions I don't understand.
>>>>>>
>>>>>> 1. Is the baud rate  or fifo a necessary feature to simulate?
>>>>>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
>>>>>> When you want to transmit a byte through WDATA,  you can call
>>>>>> qemu_chr_fe_write directly.
>>>>> So qemu_chr_fe_write() will send the data straight away. This doesn't
>>>>> match what teh hardware does though. So by modelling a FIFO and a
>>>>> delay in sending we can better match the hardware.
>>>> I see many UARTs have similar features. Does the software really care about
>>>> these features? Usually I just want to print something to the terminal
>>>> through UART.
>>> In this case Tock (which is the OS used for OpenTitan) does car about
>>> these features as it relies on interrupts generated by the HW to
>>> complete the serial send task. It also just makes the QEMU model more
>>> accurate.
>> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
>> At least,
>> it can verify the ISP.
> Exactly :)
>
>>>> Most simulation in QEMU is for running software, not exactly the details
>>>> of hardware.
>>>> For example, we will not simulate the 16x oversamples in this UART.
>>> Agreed. Lots of UARTs don't bother modelling the delay from the
>>> hardware as generally it doesn't matter. In this case it does make a
>>> difference for the software and it makes the QEMU model more accurate,
>>> which is always a good thing.
>>>
>>>> There is no error here. Personally I  think it is necessary to simulate
>>>> the FIFO and baud rate,
>>>> maybe for supporting some backends.
>>> So baud rate doesn't need to be modelled as we aren't actually sending
>>> UART data, just pretending and then printing it.
>>>
>>>> Can someone give a reasonable answer for this question?
>>> Which question?
>> I see  the UART can work with many  different backends,  such as pty ,
>> file, socket and so on.
>> I wonder if this a backend, which has some requirements on the baud
> The backend should be independent so it doesn't matter what baud rate
> we choose here.
>
>> rate.  You can ignore it,
>> as it doesn't matter.
>>>>>> 2.  The baud rate calculation method is not strictly right.
>>>>>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
>>>>>> to send the byte instead of
>>>>>> char_tx_time * 4.
>>>>> Do you mind explaining why 8 is correct instead of 4?
>>>> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
>>>> Translate a bit will take
>>>> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
>>> I see your point. I just used the 4 as that is what the Cadence one
>>> does. I don't think it matters too much as it's just the delay for a
>>> timer (that isn't used as an accurate timer).
>> Got it. Just a way to send the bytes at sometime later.
>>>>>> 3.  Why add a watch here?
>>>>> This is based on the Cadence UART implementation in QEMU (which does
>>>>> the same thing). This will trigger a callback when we can write more
>>>>> data or when the backend has hung up.
>>>> Many other serials do the same thing, like virtio-console and serial. So
>>>> it may be a common
>>>> interface here. I will try to understand it(Not yet).
>>> Yep, it's just a more complete model of that the HW does.
>> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
>> function.
>> The breakpoint did't match.
>>
>> I just wonder if there is a case really need the callback function.
> AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
> shouldn't be triggered.
I tried with the UART in the virt board.  It also registers the watch 
callback on
G_IO_HUP and G_IO_OUT.

However I will throw the question alone in another thread.

After the two comments pointed at another email addressed,

Reviewed-by: LIU Zhiwei<zhiwei_liu@c-sky.com>

Zhiwei
>
> Alistair
>
>> Zhiwei
>>> Alistair

Alistair Francis June 4, 2020, 5:46 a.m. UTC | #12

On Wed, Jun 3, 2020 at 10:06 PM LIU Zhiwei <liuzwnan@163.com> wrote:
>
>
>
> On 2020/6/4 12:35, Alistair Francis wrote:
> > On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >>
> >>
> >> On 2020/6/3 23:56, Alistair Francis wrote:
> >>> On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >>>> On 2020/6/3 1:54, Alistair Francis wrote:
> >>>>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei<zhiwei_liu@c-sky.com>  wrote:
> >>>>>> Hi Alistair,
> >>>>>>
> >>>>>> There are still some questions I don't understand.
> >>>>>>
> >>>>>> 1. Is the baud rate  or fifo a necessary feature to simulate?
> >>>>>> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
> >>>>>> When you want to transmit a byte through WDATA,  you can call
> >>>>>> qemu_chr_fe_write directly.
> >>>>> So qemu_chr_fe_write() will send the data straight away. This doesn't
> >>>>> match what teh hardware does though. So by modelling a FIFO and a
> >>>>> delay in sending we can better match the hardware.
> >>>> I see many UARTs have similar features. Does the software really care about
> >>>> these features? Usually I just want to print something to the terminal
> >>>> through UART.
> >>> In this case Tock (which is the OS used for OpenTitan) does car about
> >>> these features as it relies on interrupts generated by the HW to
> >>> complete the serial send task. It also just makes the QEMU model more
> >>> accurate.
> >> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
> >> At least,
> >> it can verify the ISP.
> > Exactly :)
> >
> >>>> Most simulation in QEMU is for running software, not exactly the details
> >>>> of hardware.
> >>>> For example, we will not simulate the 16x oversamples in this UART.
> >>> Agreed. Lots of UARTs don't bother modelling the delay from the
> >>> hardware as generally it doesn't matter. In this case it does make a
> >>> difference for the software and it makes the QEMU model more accurate,
> >>> which is always a good thing.
> >>>
> >>>> There is no error here. Personally I  think it is necessary to simulate
> >>>> the FIFO and baud rate,
> >>>> maybe for supporting some backends.
> >>> So baud rate doesn't need to be modelled as we aren't actually sending
> >>> UART data, just pretending and then printing it.
> >>>
> >>>> Can someone give a reasonable answer for this question?
> >>> Which question?
> >> I see  the UART can work with many  different backends,  such as pty ,
> >> file, socket and so on.
> >> I wonder if this a backend, which has some requirements on the baud
> > The backend should be independent so it doesn't matter what baud rate
> > we choose here.
> >
> >> rate.  You can ignore it,
> >> as it doesn't matter.
> >>>>>> 2.  The baud rate calculation method is not strictly right.
> >>>>>> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
> >>>>>> to send the byte instead of
> >>>>>> char_tx_time * 4.
> >>>>> Do you mind explaining why 8 is correct instead of 4?
> >>>> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
> >>>> Translate a bit will take
> >>>> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
> >>> I see your point. I just used the 4 as that is what the Cadence one
> >>> does. I don't think it matters too much as it's just the delay for a
> >>> timer (that isn't used as an accurate timer).
> >> Got it. Just a way to send the bytes at sometime later.
> >>>>>> 3.  Why add a watch here?
> >>>>> This is based on the Cadence UART implementation in QEMU (which does
> >>>>> the same thing). This will trigger a callback when we can write more
> >>>>> data or when the backend has hung up.
> >>>> Many other serials do the same thing, like virtio-console and serial. So
> >>>> it may be a common
> >>>> interface here. I will try to understand it(Not yet).
> >>> Yep, it's just a more complete model of that the HW does.
> >> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
> >> function.
> >> The breakpoint did't match.
> >>
> >> I just wonder if there is a case really need the callback function.
> > AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
> > shouldn't be triggered.
> I tried with the UART in the virt board.  It also registers the watch
> callback on
> G_IO_HUP and G_IO_OUT.
>
> However I will through the question alone in another mail.

Ah, sorry I misunderstood what you meant.

I haven't looked at it, it's possible it's not enabled by Linux.

>
> After the two points addressed,
>
> Reviewed-by: LIU Zhiwei<zhiwei_liu@c-sky.com>

Thanks!

Alistair

>
> Zhiwei
> >
> > Alistair
> >
> >> Zhiwei
> >>> Alistair
>
>
>

[v5,07/11] hw/char: Initial commit of Ibex UART

Commit Message

Comments

Patch