| Message ID | 1519998711-73430-20-git-send-email-mjc@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/02/2018 02:51 PM, Michael Clark 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 <richard.henderson@linaro.org> > Signed-off-by: Stefan O'Rear <sorear2@gmail.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > Signed-off-by: Michael Clark <mjc@sifive.com> > --- > 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); > + } or qemu_set_irq(s->irq, cond); > +} > + > +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; Can you add a #define for this bit? > + > + 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); qemu_log(GUEST_ERROR)? > + 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); Ditto. > +} > + > +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"); replace printf() by warn_report()? > + 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 */ > +}; These 2 enums don't need to be exported and can stay in sifive_uart.c. > + > +#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; > + > + /*< public >*/ > + qemu_irq irq; > + MemoryRegion mmio; > + CharBackend chr; > + uint8_t rx_fifo[8]; > + unsigned int rx_fifo_len; > + uint32_t ie; > + uint32_t ip; > + uint32_t txctrl; > + uint32_t rxctrl; > + uint32_t div; > +} SiFiveUARTState; > + > +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, hwaddr base, > + Chardev *chr, qemu_irq irq); > + > +#endif > removing printf(): Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 03/02/2018 02:51 PM, Michael Clark 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 <richard.henderson@linaro.org> > > Signed-off-by: Stefan O'Rear <sorear2@gmail.com> > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > Signed-off-by: Michael Clark <mjc@sifive.com> > > --- > > 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); > > + } > > or qemu_set_irq(s->irq, cond); > > > +} > > + > > +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; > > Can you add a #define for this bit? > > > + > > + 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); > > qemu_log(GUEST_ERROR)? > > > + 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); > > Ditto. > > > +} > > + > > +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"); > > replace printf() by warn_report()? > > > + 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 */ > > +}; > > These 2 enums don't need to be exported and can stay in sifive_uart.c. > > > + > > +#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; > > + > > + /*< public >*/ > > + qemu_irq irq; > > + MemoryRegion mmio; > > + CharBackend chr; > > + uint8_t rx_fifo[8]; > > + unsigned int rx_fifo_len; > > + uint32_t ie; > > + uint32_t ip; > > + uint32_t txctrl; > > + uint32_t rxctrl; > > + uint32_t div; > > +} SiFiveUARTState; > > + > > +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, > hwaddr base, > > + Chardev *chr, qemu_irq irq); > > + > > +#endif > > > > removing printf(): > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > I will go through and make these changes and include them in the post-merge cleanup patch series...
On 10/03/18 03:02, Michael Clark wrote: > On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> > wrote: > >> On 03/02/2018 02:51 PM, Michael Clark 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'. FWIW I can highly recommend switching over to the QOM/qdev APIs where possible since it makes rewiring machines much easier, and passing around direct types such as qemu_irq via custom functions means people are less likely to touch them. >>> Acked-by: Richard Henderson <richard.henderson@linaro.org> >>> Signed-off-by: Stefan O'Rear <sorear2@gmail.com> >>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >>> Signed-off-by: Michael Clark <mjc@sifive.com> >>> --- >>> 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); >>> + } >> >> or qemu_set_irq(s->irq, cond); >> >>> +} >>> + >>> +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; >> >> Can you add a #define for this bit? >> >>> + >>> + 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); >> >> qemu_log(GUEST_ERROR)? >> >>> + 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); >> >> Ditto. >> >>> +} >>> + >>> +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"); >> >> replace printf() by warn_report()? >> >>> + 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 */ >>> +}; >> >> These 2 enums don't need to be exported and can stay in sifive_uart.c. >> >>> + >>> +#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; >>> + >>> + /*< public >*/ >>> + qemu_irq irq; >>> + MemoryRegion mmio; >>> + CharBackend chr; >>> + uint8_t rx_fifo[8]; >>> + unsigned int rx_fifo_len; >>> + uint32_t ie; >>> + uint32_t ip; >>> + uint32_t txctrl; >>> + uint32_t rxctrl; >>> + uint32_t div; >>> +} SiFiveUARTState; >>> + >>> +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, >> hwaddr base, >>> + Chardev *chr, qemu_irq irq); >>> + >>> +#endif >>> >> >> removing printf(): >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> > > I will go through and make these changes and include them in the post-merge > cleanup patch series... Another general note: for each of the main QEMU platforms supported there is a home page on the official wiki, so do make sure that you set yourself a template for RiscV and add some information to help people get started. As an example here's the SPARC page which I currently maintain: https://wiki.qemu.org/Documentation/Platforms/SPARC. I tend to also keep a rough changelog there too so people can see which features have been added in which QEMU version. HTH, Mark.
Hi Mark, On 03/10/2018 10:40 AM, Mark Cave-Ayland wrote: > On 10/03/18 03:02, Michael Clark wrote: > >> On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> >> wrote: >> [...] > Another general note: for each of the main QEMU platforms supported > there is a home page on the official wiki, so do make sure that you set > yourself a template for RiscV and add some information to help people > get started. > Thanks for the pointer. I went ahead and created a basic page for RISC-V with instructions on how to build QEMU and boot Fedora. Cheers, Bastian
On Sun, Mar 11, 2018 at 4:43 AM, Bastian Koppelmann < kbastian@mail.uni-paderborn.de> wrote: > Hi Mark, > > On 03/10/2018 10:40 AM, Mark Cave-Ayland wrote: > > On 10/03/18 03:02, Michael Clark wrote: > > > >> On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé < > f4bug@amsat.org> > >> wrote: > >> > [...] > > Another general note: for each of the main QEMU platforms supported > > there is a home page on the official wiki, so do make sure that you set > > yourself a template for RiscV and add some information to help people > > get started. > > > Thanks for the pointer. I went ahead and created a basic page for RISC-V > with instructions on how to build QEMU and boot Fedora. > Thanks Bastian! I'll add a link back to the official QEMU RISC-V Architecture page from the riscv-qemu repo wiki...
On Fri, Mar 16, 2018 at 11:30 AM, Michael Clark <mjc@sifive.com> wrote: > > > On Sun, Mar 11, 2018 at 4:43 AM, Bastian Koppelmann < > kbastian@mail.uni-paderborn.de> wrote: > >> Hi Mark, >> >> On 03/10/2018 10:40 AM, Mark Cave-Ayland wrote: >> > On 10/03/18 03:02, Michael Clark wrote: >> > >> >> On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé < >> f4bug@amsat.org> >> >> wrote: >> >> >> [...] >> > Another general note: for each of the main QEMU platforms supported >> > there is a home page on the official wiki, so do make sure that you set >> > yourself a template for RiscV and add some information to help people >> > get started. >> > >> Thanks for the pointer. I went ahead and created a basic page for RISC-V >> with instructions on how to build QEMU and boot Fedora. >> > > Thanks Bastian! I'll add a link back to the official QEMU RISC-V > Architecture page from the riscv-qemu repo wiki... > I noticed there is a spelling mistake in the description: "implented" instead of "implemented" Michael.
On 03/16/2018 07:36 PM, Michael Clark wrote: > On Fri, Mar 16, 2018 at 11:30 AM, Michael Clark <mjc@sifive.com> wrote: > >> >> >> On Sun, Mar 11, 2018 at 4:43 AM, Bastian Koppelmann < >> kbastian@mail.uni-paderborn.de> wrote: >> >>> Hi Mark, >>> >>> On 03/10/2018 10:40 AM, Mark Cave-Ayland wrote: >>>> On 10/03/18 03:02, Michael Clark wrote: >>>> >>>>> On Sat, Mar 10, 2018 at 1:39 AM, Philippe Mathieu-Daudé < >>> f4bug@amsat.org> >>>>> wrote: >>>>> >>> [...] >>>> Another general note: for each of the main QEMU platforms supported >>>> there is a home page on the official wiki, so do make sure that you set >>>> yourself a template for RiscV and add some information to help people >>>> get started. >>>> >>> Thanks for the pointer. I went ahead and created a basic page for RISC-V >>> with instructions on how to build QEMU and boot Fedora. >>> >> >> Thanks Bastian! I'll add a link back to the official QEMU RISC-V >> Architecture page from the riscv-qemu repo wiki... >> > > I noticed there is a spelling mistake in the description: "implented" > instead of "implemented" Woops, fixed :). Cheers, Bastian
On Sat, 3 Mar 2018 02:51:47 +1300 Michael Clark <mjc@sifive.com> 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 <richard.henderson@linaro.org> > Signed-off-by: Stefan O'Rear <sorear2@gmail.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > Signed-off-by: Michael Clark <mjc@sifive.com> > --- > 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. > + uint8_t rx_fifo[8]; > + unsigned int rx_fifo_len; > + uint32_t ie; > + uint32_t ip; > + uint32_t txctrl; > + uint32_t rxctrl; > + uint32_t div; > +} SiFiveUARTState; > + > +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, hwaddr base, > + Chardev *chr, qemu_irq irq); > + > +#endif > -- > 2.7.0 > >
On 10.04.2018 05:21, Antony Pavlov wrote: > On Sat, 3 Mar 2018 02:51:47 +1300 > Michael Clark <mjc@sifive.com> 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 <richard.henderson@linaro.org> >> Signed-off-by: Stefan O'Rear <sorear2@gmail.com> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> Signed-off-by: Michael Clark <mjc@sifive.com> >> --- >> 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 €. Thomas
On Tue, 10 Apr 2018 08:17:32 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 10.04.2018 05:21, Antony Pavlov wrote: > > On Sat, 3 Mar 2018 02:51:47 +1300 > > Michael Clark <mjc@sifive.com> 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 <richard.henderson@linaro.org> > >> Signed-off-by: Stefan O'Rear <sorear2@gmail.com> > >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > >> Signed-off-by: Michael Clark <mjc@sifive.com> > >> --- > >> 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/
On Tue, Apr 10, 2018 at 8:04 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote: > On Tue, 10 Apr 2018 08:17:32 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 10.04.2018 05:21, Antony Pavlov wrote: > > > On Sat, 3 Mar 2018 02:51:47 +1300 > > > Michael Clark <mjc@sifive.com> 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 <richard.henderson@linaro.org> > > >> Signed-off-by: Stefan O'Rear <sorear2@gmail.com> > > >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > >> Signed-off-by: Michael Clark <mjc@sifive.com> > > >> --- > > >> 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/ Currently no headers in include/hw/riscv/*.h include any other headers and i'd like to keep it that way if possible. It's not painful if no headers include any other headers. On the contrary it keeps things simple. There are no implicit dependencies. Core QEMU header dependencies should not be brought in by riscv specific headers. That would lead to source modules that do not include their QEMU dependencies making the situation for managing source modules more opaque. We have followed the best practice that source files should explicitly include their header dependencies.
On 04/10/2018 03:04 AM, Antony Pavlov wrote: >>>> +++ b/include/hw/riscv/sifive_uart.h >>>> + >>>> +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. That is, this header is not standalone; a .c file can't use the header unless it first includes hw/sysbus.h prior to sifive_uart.h. >>> >>> 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. If you were to use a CharBackend*, you could get by with just the typedef. Since all .c files include osdeps.h, which in turn includes typedefs.h, you wouldn't have to include anything if you only refer to the type via a pointer. But here, you are including a full object, so the compiler has to know the size of the type, which means this header DOES depend on "chardev/char-fe.h" being included first (either in this .h to keep it standalone, or in all .c files prior to the point where they include sifive_uart.h). >> >> 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. My personal preference: if your header only refers to a type via a pointer where the header is still standalone with just the appropriate typedefs, then DON'T include another .h from your header. But if your header has a hard dependency on something not already included by osdeps.h, where failing to include that other header first creates a compile error, then including the .h in your header is appropriate, as it is less work for all .c clients that use your header. The art of reducing compile-time dependencies is figuring out which structs must be included inline (requiring .h in headers), and where you can use pointers, or opaque types that live in .c, or whatever other solutions, so that the headers become lighter-weight. But it is NOT designed to break standalone use of a header (other than the one exception that headers DON'T include osdeps.h because that had to already be included first by all .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. Indeed, that's why I argue that include statements in .h files that are necessary for standalone compilation of that header is a good idea, and not something to burden every .c file with.
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; + + /*< public >*/ + qemu_irq irq; + MemoryRegion mmio; + CharBackend chr; + uint8_t rx_fifo[8]; + unsigned int rx_fifo_len; + uint32_t ie; + uint32_t ip; + uint32_t txctrl; + uint32_t rxctrl; + uint32_t div; +} SiFiveUARTState; + +SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, hwaddr base, + Chardev *chr, qemu_irq irq); + +#endif