Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-15 Thread Alistair Francis
On Fri, May 15, 2020 at 12:28 AM Philippe Mathieu-Daudé
 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 
> > ---
> >   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 *) , 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 & 

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-15 Thread Philippe Mathieu-Daudé

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 
---
  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).


  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 *) , 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.

Otherwise patch looks good.


+.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 00..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 

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-15 Thread Philippe Mathieu-Daudé

On 5/14/20 11:59 PM, Alistair Francis wrote:

On Thu, May 14, 2020 at 11:00 AM Philippe Mathieu-Daudé
 wrote:


Hi Alistair,

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 
---
   MAINTAINERS |   2 +
   hw/char/Makefile.objs   |   1 +
   hw/char/ibex_uart.c | 490 
   hw/riscv/Kconfig|   4 +
   include/hw/char/ibex_uart.h | 110 
   5 files changed, 607 insertions(+)
   create mode 100644 hw/char/ibex_uart.c
   create mode 100644 include/hw/char/ibex_uart.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c3d77f0861..d3d47564ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,7 +1236,9 @@ M: Alistair Francis 
   L: qemu-ri...@nongnu.org
   S: Supported
   F: hw/riscv/opentitan.c
+F: hw/char/ibex_uart.c
   F: include/hw/riscv/opentitan.h
+F: include/hw/char/ibex_uart.h


   SH4 Machines
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 9e9a6c1aff..633996be5b 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
   common-obj-$(CONFIG_XEN) += xen_console.o
   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
+common-obj-$(CONFIG_IBEX) += ibex_uart.o

   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
new file mode 100644
index 00..f6215ae23d
--- /dev/null
+++ b/hw/char/ibex_uart.c
@@ -0,0 +1,490 @@
+/*
+ * QEMU lowRISC Ibex UART device
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * For details check the documentation here:
+ *https://docs.opentitan.org/hw/ip/uart/doc/
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/ibex_uart.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+static void ibex_uart_update_irqs(IbexUartState *s)
+{
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
+qemu_set_irq(s->tx_watermark, 1);
+} else {
+qemu_set_irq(s->tx_watermark, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
+qemu_set_irq(s->rx_watermark, 1);
+} else {
+qemu_set_irq(s->rx_watermark, 0);


I wonder if having both bit separate can't lead to odd pulse behavior
(this function should have the same result if you invert the RX/TX
processing here). I'd be safer using a local 'raise_watermark' boolean
variable, then call qemu_set_irq() once.


I'm not sure what you mean. Are you worried that TX and RX will both
go high/low at the same time?


Disregard my comment, it was late and I misread that rx/tx are different 
outgoing IRQs (I read this as a single watermark IRQ).




Alistair




+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
+qemu_set_irq(s->tx_empty, 1);
+} else {
+qemu_set_irq(s->tx_empty, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
+qemu_set_irq(s->rx_overflow, 1);
+} else {
+qemu_set_irq(s->rx_overflow, 0);
+}
+}

[...]








Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-14 Thread Alistair Francis
On Thu, May 14, 2020 at 11:00 AM Philippe Mathieu-Daudé
 wrote:
>
> Hi Alistair,
>
> 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 
> > ---
> >   MAINTAINERS |   2 +
> >   hw/char/Makefile.objs   |   1 +
> >   hw/char/ibex_uart.c | 490 
> >   hw/riscv/Kconfig|   4 +
> >   include/hw/char/ibex_uart.h | 110 
> >   5 files changed, 607 insertions(+)
> >   create mode 100644 hw/char/ibex_uart.c
> >   create mode 100644 include/hw/char/ibex_uart.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c3d77f0861..d3d47564ce 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1236,7 +1236,9 @@ M: Alistair Francis 
> >   L: qemu-ri...@nongnu.org
> >   S: Supported
> >   F: hw/riscv/opentitan.c
> > +F: hw/char/ibex_uart.c
> >   F: include/hw/riscv/opentitan.h
> > +F: include/hw/char/ibex_uart.h
> >
> >
> >   SH4 Machines
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index 9e9a6c1aff..633996be5b 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
> >   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
> >   common-obj-$(CONFIG_XEN) += xen_console.o
> >   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> > +common-obj-$(CONFIG_IBEX) += ibex_uart.o
> >
> >   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
> >   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> > new file mode 100644
> > index 00..f6215ae23d
> > --- /dev/null
> > +++ b/hw/char/ibex_uart.c
> > @@ -0,0 +1,490 @@
> > +/*
> > + * QEMU lowRISC Ibex UART device
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * For details check the documentation here:
> > + *https://docs.opentitan.org/hw/ip/uart/doc/
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), 
> > to deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> > IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/char/ibex_uart.h"
> > +#include "hw/irq.h"
> > +#include "hw/qdev-properties.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +
> > +static void ibex_uart_update_irqs(IbexUartState *s)
> > +{
> > +if (s->uart_intr_state & s->uart_intr_enable & 
> > INTR_STATE_TX_WATERMARK) {
> > +qemu_set_irq(s->tx_watermark, 1);
> > +} else {
> > +qemu_set_irq(s->tx_watermark, 0);
> > +}
> > +
> > +if (s->uart_intr_state & s->uart_intr_enable & 
> > INTR_STATE_RX_WATERMARK) {
> > +qemu_set_irq(s->rx_watermark, 1);
> > +} else {
> > +qemu_set_irq(s->rx_watermark, 0);
>
> I wonder if having both bit separate can't lead to odd pulse behavior
> (this function should have the same result if you invert the RX/TX
> processing here). I'd be safer using a local 'raise_watermark' boolean
> variable, then call qemu_set_irq() once.

I'm not sure what you mean. Are you worried that TX and RX will both
go high/low at the same time?

Alistair

>
> > +}
> > +
> > +if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> > +qemu_set_irq(s->tx_empty, 1);
> > +} else {
> > +qemu_set_irq(s->tx_empty, 0);
> > +}
> > +
> > +if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) 
> > {
> > +qemu_set_irq(s->rx_overflow, 1);
> > +} else {
> > +qemu_set_irq(s->rx_overflow, 0);
> > +}
> > +}
> [...]
>



Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-14 Thread Philippe Mathieu-Daudé

Hi Alistair,

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 
---
  MAINTAINERS |   2 +
  hw/char/Makefile.objs   |   1 +
  hw/char/ibex_uart.c | 490 
  hw/riscv/Kconfig|   4 +
  include/hw/char/ibex_uart.h | 110 
  5 files changed, 607 insertions(+)
  create mode 100644 hw/char/ibex_uart.c
  create mode 100644 include/hw/char/ibex_uart.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c3d77f0861..d3d47564ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,7 +1236,9 @@ M: Alistair Francis 
  L: qemu-ri...@nongnu.org
  S: Supported
  F: hw/riscv/opentitan.c
+F: hw/char/ibex_uart.c
  F: include/hw/riscv/opentitan.h
+F: include/hw/char/ibex_uart.h
  
  
  SH4 Machines

diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 9e9a6c1aff..633996be5b 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
  common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
  common-obj-$(CONFIG_XEN) += xen_console.o
  common-obj-$(CONFIG_CADENCE) += cadence_uart.o
+common-obj-$(CONFIG_IBEX) += ibex_uart.o
  
  common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o

  common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
new file mode 100644
index 00..f6215ae23d
--- /dev/null
+++ b/hw/char/ibex_uart.c
@@ -0,0 +1,490 @@
+/*
+ * QEMU lowRISC Ibex UART device
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * For details check the documentation here:
+ *https://docs.opentitan.org/hw/ip/uart/doc/
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/ibex_uart.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+static void ibex_uart_update_irqs(IbexUartState *s)
+{
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
+qemu_set_irq(s->tx_watermark, 1);
+} else {
+qemu_set_irq(s->tx_watermark, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
+qemu_set_irq(s->rx_watermark, 1);
+} else {
+qemu_set_irq(s->rx_watermark, 0);


I wonder if having both bit separate can't lead to odd pulse behavior 
(this function should have the same result if you invert the RX/TX 
processing here). I'd be safer using a local 'raise_watermark' boolean 
variable, then call qemu_set_irq() once.



+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
+qemu_set_irq(s->tx_empty, 1);
+} else {
+qemu_set_irq(s->tx_empty, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
+qemu_set_irq(s->rx_overflow, 1);
+} else {
+qemu_set_irq(s->rx_overflow, 0);
+}
+}

[...]




[PATCH v2 5/9] hw/char: Initial commit of Ibex UART

2020-05-07 Thread Alistair Francis
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 
---
 MAINTAINERS |   2 +
 hw/char/Makefile.objs   |   1 +
 hw/char/ibex_uart.c | 490 
 hw/riscv/Kconfig|   4 +
 include/hw/char/ibex_uart.h | 110 
 5 files changed, 607 insertions(+)
 create mode 100644 hw/char/ibex_uart.c
 create mode 100644 include/hw/char/ibex_uart.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c3d77f0861..d3d47564ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,7 +1236,9 @@ M: Alistair Francis 
 L: qemu-ri...@nongnu.org
 S: Supported
 F: hw/riscv/opentitan.c
+F: hw/char/ibex_uart.c
 F: include/hw/riscv/opentitan.h
+F: include/hw/char/ibex_uart.h
 
 
 SH4 Machines
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 9e9a6c1aff..633996be5b 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
 common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
 common-obj-$(CONFIG_XEN) += xen_console.o
 common-obj-$(CONFIG_CADENCE) += cadence_uart.o
+common-obj-$(CONFIG_IBEX) += ibex_uart.o
 
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
 common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
new file mode 100644
index 00..f6215ae23d
--- /dev/null
+++ b/hw/char/ibex_uart.c
@@ -0,0 +1,490 @@
+/*
+ * QEMU lowRISC Ibex UART device
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * For details check the documentation here:
+ *https://docs.opentitan.org/hw/ip/uart/doc/
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/ibex_uart.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+static void ibex_uart_update_irqs(IbexUartState *s)
+{
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
+qemu_set_irq(s->tx_watermark, 1);
+} else {
+qemu_set_irq(s->tx_watermark, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
+qemu_set_irq(s->rx_watermark, 1);
+} else {
+qemu_set_irq(s->rx_watermark, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
+qemu_set_irq(s->tx_empty, 1);
+} else {
+qemu_set_irq(s->tx_empty, 0);
+}
+
+if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
+qemu_set_irq(s->rx_overflow, 1);
+} else {
+qemu_set_irq(s->rx_overflow, 0);
+}
+}
+
+static int ibex_uart_can_receive(void *opaque)
+{
+IbexUartState *s = opaque;
+
+if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
+return 1;
+}
+
+return 0;
+}
+
+static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
+{
+IbexUartState *s = opaque;
+uint8_t rx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_RXILVL)
+>> FIFO_CTRL_RXILVL_SHIFT;
+
+s->uart_rdata = *buf;
+
+s->uart_status &= ~UART_STATUS_RXIDLE;
+s->uart_status &= ~UART_STATUS_RXEMPTY;
+
+if (size > rx_fifo_level) {
+s->uart_intr_state |= INTR_STATE_RX_WATERMARK;
+}
+
+ibex_uart_update_irqs(s);
+}
+
+static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
+   void *opaque)
+{
+IbexUartState *s = opaque;
+uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
+>> FIFO_CTRL_TXILVL_SHIFT;
+int ret;
+
+/* instant drain the fifo when there's no back-end */
+if (!qemu_chr_fe_backend_connected(>chr)) {
+s->tx_level = 0;
+return FALSE;
+}
+
+if