Re: [Qemu-devel] [PATCH v8 19/23] SiFive RISC-V UART Device

[Antony Pavlov]

On Tue, 10 Apr 2018 08:17:32 +0200
Thomas Huth <address@hidden> wrote:

> On 10.04.2018 05:21, Antony Pavlov wrote:
> > On Sat,  3 Mar 2018 02:51:47 +1300
> > Michael Clark <address@hidden> wrote:
> > 
> >> QEMU model of the UART on the SiFive E300 and U500 series SOCs.
> >> BBL supports the SiFive UART for early console access via the SBI
> >> (Supervisor Binary Interface) and the linux kernel SBI console.
> >>
> >> The SiFive UART implements the pre qom legacy interface consistent
> >> with the 16550a UART in 'hw/char/serial.c'.
> >>
> >> Acked-by: Richard Henderson <address@hidden>
> >> Signed-off-by: Stefan O'Rear <address@hidden>
> >> Signed-off-by: Palmer Dabbelt <address@hidden>
> >> Signed-off-by: Michael Clark <address@hidden>
> >> ---
> >>  hw/riscv/sifive_uart.c         | 176 
> >> +++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/riscv/sifive_uart.h |  71 +++++++++++++++++
> >>  2 files changed, 247 insertions(+)
> >>  create mode 100644 hw/riscv/sifive_uart.c
> >>  create mode 100644 include/hw/riscv/sifive_uart.h
> >>
> >> diff --git a/hw/riscv/sifive_uart.c b/hw/riscv/sifive_uart.c
> >> new file mode 100644
> >> index 0000000..b0c3798
> >> --- /dev/null
> >> +++ b/hw/riscv/sifive_uart.c
> >> @@ -0,0 +1,176 @@
> >> +/*
> >> + * QEMU model of the UART on the SiFive E300 and U500 series SOCs.
> >> + *
> >> + * Copyright (c) 2016 Stefan O'Rear
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >> for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License 
> >> along with
> >> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "hw/sysbus.h"
> >> +#include "chardev/char.h"
> >> +#include "chardev/char-fe.h"
> >> +#include "target/riscv/cpu.h"
> >> +#include "hw/riscv/sifive_uart.h"
> >>
> >> +/*
> >> + * Not yet implemented:
> >> + *
> >> + * Transmit FIFO using "qemu/fifo8.h"
> >> + * SIFIVE_UART_IE_TXWM interrupts
> >> + * SIFIVE_UART_IE_RXWM interrupts must honor fifo watermark
> >> + * Rx FIFO watermark interrupt trigger threshold
> >> + * Tx FIFO watermark interrupt trigger threshold.
> >> + */
> >> +
> >> +static void update_irq(SiFiveUARTState *s)
> >> +{
> >> +    int cond = 0;
> >> +    if ((s->ie & SIFIVE_UART_IE_RXWM) && s->rx_fifo_len) {
> >> +        cond = 1;
> >> +    }
> >> +    if (cond) {
> >> +        qemu_irq_raise(s->irq);
> >> +    } else {
> >> +        qemu_irq_lower(s->irq);
> >> +    }
> >> +}
> >> +
> >> +static uint64_t
> >> +uart_read(void *opaque, hwaddr addr, unsigned int size)
> >> +{
> >> +    SiFiveUARTState *s = opaque;
> >> +    unsigned char r;
> >> +    switch (addr) {
> >> +    case SIFIVE_UART_RXFIFO:
> >> +        if (s->rx_fifo_len) {
> >> +            r = s->rx_fifo[0];
> >> +            memmove(s->rx_fifo, s->rx_fifo + 1, s->rx_fifo_len - 1);
> >> +            s->rx_fifo_len--;
> >> +            qemu_chr_fe_accept_input(&s->chr);
> >> +            update_irq(s);
> >> +            return r;
> >> +        }
> >> +        return 0x80000000;
> >> +
> >> +    case SIFIVE_UART_TXFIFO:
> >> +        return 0; /* Should check tx fifo */
> >> +    case SIFIVE_UART_IE:
> >> +        return s->ie;
> >> +    case SIFIVE_UART_IP:
> >> +        return s->rx_fifo_len ? SIFIVE_UART_IP_RXWM : 0;
> >> +    case SIFIVE_UART_TXCTRL:
> >> +        return s->txctrl;
> >> +    case SIFIVE_UART_RXCTRL:
> >> +        return s->rxctrl;
> >> +    case SIFIVE_UART_DIV:
> >> +        return s->div;
> >> +    }
> >> +
> >> +    hw_error("%s: bad read: addr=0x%x\n",
> >> +        __func__, (int)addr);
> >> +    return 0;
> >> +}
> >> +
> >> +static void
> >> +uart_write(void *opaque, hwaddr addr,
> >> +           uint64_t val64, unsigned int size)
> >> +{
> >> +    SiFiveUARTState *s = opaque;
> >> +    uint32_t value = val64;
> >> +    unsigned char ch = value;
> >> +
> >> +    switch (addr) {
> >> +    case SIFIVE_UART_TXFIFO:
> >> +        qemu_chr_fe_write(&s->chr, &ch, 1);
> >> +        return;
> >> +    case SIFIVE_UART_IE:
> >> +        s->ie = val64;
> >> +        update_irq(s);
> >> +        return;
> >> +    case SIFIVE_UART_TXCTRL:
> >> +        s->txctrl = val64;
> >> +        return;
> >> +    case SIFIVE_UART_RXCTRL:
> >> +        s->rxctrl = val64;
> >> +        return;
> >> +    case SIFIVE_UART_DIV:
> >> +        s->div = val64;
> >> +        return;
> >> +    }
> >> +    hw_error("%s: bad write: addr=0x%x v=0x%x\n",
> >> +        __func__, (int)addr, (int)value);
> >> +}
> >> +
> >> +static const MemoryRegionOps uart_ops = {
> >> +    .read = uart_read,
> >> +    .write = uart_write,
> >> +    .endianness = DEVICE_NATIVE_ENDIAN,
> >> +    .valid = {
> >> +        .min_access_size = 4,
> >> +        .max_access_size = 4
> >> +    }
> >> +};
> >> +
> >> +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> >> +{
> >> +    SiFiveUARTState *s = opaque;
> >> +
> >> +    /* Got a byte.  */
> >> +    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
> >> +        printf("WARNING: UART dropped char.\n");
> >> +        return;
> >> +    }
> >> +    s->rx_fifo[s->rx_fifo_len++] = *buf;
> >> +
> >> +    update_irq(s);
> >> +}
> >> +
> >> +static int uart_can_rx(void *opaque)
> >> +{
> >> +    SiFiveUARTState *s = opaque;
> >> +
> >> +    return s->rx_fifo_len < sizeof(s->rx_fifo);
> >> +}
> >> +
> >> +static void uart_event(void *opaque, int event)
> >> +{
> >> +}
> >> +
> >> +static int uart_be_change(void *opaque)
> >> +{
> >> +    SiFiveUARTState *s = opaque;
> >> +
> >> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx, uart_event,
> >> +        uart_be_change, s, NULL, true);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +/*
> >> + * Create UART device.
> >> + */
> >> +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, hwaddr 
> >> base,
> >> +    Chardev *chr, qemu_irq irq)
> >> +{
> >> +    SiFiveUARTState *s = g_malloc0(sizeof(SiFiveUARTState));
> >> +    s->irq = irq;
> >> +    qemu_chr_fe_init(&s->chr, chr, &error_abort);
> >> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx, uart_event,
> >> +        uart_be_change, s, NULL, true);
> >> +    memory_region_init_io(&s->mmio, NULL, &uart_ops, s,
> >> +                          TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
> >> +    memory_region_add_subregion(address_space, base, &s->mmio);
> >> +    return s;
> >> +}
> >> diff --git a/include/hw/riscv/sifive_uart.h 
> >> b/include/hw/riscv/sifive_uart.h
> >> new file mode 100644
> >> index 0000000..504f18a
> >> --- /dev/null
> >> +++ b/include/hw/riscv/sifive_uart.h
> >> @@ -0,0 +1,71 @@
> >> +/*
> >> + * SiFive UART interface
> >> + *
> >> + * Copyright (c) 2016 Stefan O'Rear
> >> + * Copyright (c) 2017 SiFive, Inc.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >> for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License 
> >> along with
> >> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#ifndef HW_SIFIVE_UART_H
> >> +#define HW_SIFIVE_UART_H
> >> +
> >> +enum {
> >> +    SIFIVE_UART_TXFIFO        = 0,
> >> +    SIFIVE_UART_RXFIFO        = 4,
> >> +    SIFIVE_UART_TXCTRL        = 8,
> >> +    SIFIVE_UART_TXMARK        = 10,
> >> +    SIFIVE_UART_RXCTRL        = 12,
> >> +    SIFIVE_UART_RXMARK        = 14,
> >> +    SIFIVE_UART_IE            = 16,
> >> +    SIFIVE_UART_IP            = 20,
> >> +    SIFIVE_UART_DIV           = 24,
> >> +    SIFIVE_UART_MAX           = 32
> >> +};
> >> +
> >> +enum {
> >> +    SIFIVE_UART_IE_TXWM       = 1, /* Transmit watermark interrupt enable 
> >> */
> >> +    SIFIVE_UART_IE_RXWM       = 2  /* Receive watermark interrupt enable 
> >> */
> >> +};
> >> +
> >> +enum {
> >> +    SIFIVE_UART_IP_TXWM       = 1, /* Transmit watermark interrupt 
> >> pending */
> >> +    SIFIVE_UART_IP_RXWM       = 2  /* Receive watermark interrupt pending 
> >> */
> >> +};
> >> +
> >> +#define TYPE_SIFIVE_UART "riscv.sifive.uart"
> >> +
> >> +#define SIFIVE_UART(obj) \
> >> +    OBJECT_CHECK(SiFiveUARTState, (obj), TYPE_SIFIVE_UART)
> >> +
> >> +typedef struct SiFiveUARTState {
> >> +    /*< private >*/
> >> +    SysBusDevice parent_obj;
> > 
> > 
> > You use SysBusDevive in this header file but there is no 'include 
> > "hw/sysbus.h"' in the header file itself.
> > 
> > Please see my comment 
> > https://github.com/riscv/riscv-qemu/pull/130#issuecomment-379640538
> > 
> > 
> >> +    /*< public >*/
> >> +    qemu_irq irq;
> >> +    MemoryRegion mmio;
> >> +    CharBackend chr;
> > 
> > Just the same thing. CharBackend is defined in "chardev/char-fe.h" please 
> > include it.
> 
> Honestly, I rather prefer to *not* add more includes to header files
> than we already have. We already have got lots of "touch this header and
> you have to recompile almost the whole QEMU" conditions, so to avoid
> that this situation gets worse, we should rather avoid including headers
> from headers if it is not necessary. Thus if the current sources build
> fine, no need to change anything here. Just my 0.02 €.

Adding ONLY NECESSARY header files inclusions to header file __can't produce__
additional recompile efforts.
Moreover this can decrease number of include directives in c-files.

I __rebased__ my RISC-V board in my out-of tree qemu branch 
(https://github.com/miet-riscv-workgroup/riscv-qemu/tree/20180409.erizo). I 
faced with problem: I have to track dependencies of
header files from include/hw/riscv/ which I use.

So your "not-add-more-includes-to-header-files" approach has an disadvantage:
if a header file required header list changes, each c-file that includes that 
header file
must be edited to update the #include statement list.

RISC-V is a very promising architecture. It is possible that we will have many 
RISC-V-related
c-files in the future in qemu. It will be very painful to change these c-files 
on every RISC-V header
files requirements change.

To Eric Blake:
I have added you to CC because of your comment in the conversation 
https://patchwork.kernel.org/patch/10147099/

-- 
Best regards,
  Antony Pavlov