[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART |
|
Date: |
Fri, 15 May 2020 12:46:48 -0700 |
On Fri, May 15, 2020 at 12:28 AM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> On 5/7/20 9:13 PM, 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 <address@hidden>
> > ---
> > MAINTAINERS | 2 +
> > hw/char/Makefile.objs | 1 +
> > hw/char/ibex_uart.c | 490 ++++++++++++++++++++++++++++++++++++
> > hw/riscv/Kconfig | 4 +
> > include/hw/char/ibex_uart.h | 110 ++++++++
>
> If possible setup scripts/git.orderfile to ease review (avoid scrolling).
I have set this up.
>
> > 5 files changed, 607 insertions(+)
> > create mode 100644 hw/char/ibex_uart.c
> > create mode 100644 include/hw/char/ibex_uart.h
> >
> [...]
> > +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,
>
> As all registers are 32-bit, you want .impl min/max = 4 here.
Added.
>
> Otherwise patch looks good.
Thanks!
Alistair
>
> > + .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> [...]
> > 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 */
> >
>
- Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector, (continued)
- [PATCH v2 6/9] hw/intc: Initial commit of lowRISC Ibex PLIC, Alistair Francis, 2020/05/07
- [PATCH v2 7/9] riscv/opentitan: Connect the PLIC device, Alistair Francis, 2020/05/07
- [PATCH v2 8/9] riscv/opentitan: Connect the UART device, Alistair Francis, 2020/05/07
- [PATCH v2 9/9] target/riscv: Use a smaller guess size for no-MMU PMP, Alistair Francis, 2020/05/07
- Re: [PATCH v2 0/9] RISC-V Add the OpenTitan Machine, Alistair Francis, 2020/05/13