[Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Juro Bystricky posted 1 patch 163 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1486666321-21536-1-git-send-email-juro.bystricky@intel.com
Test checkpatch passed
Test docker passed
Test s390x passed
default-configs/nios2-softmmu.mak |   1 +
hw/char/Makefile.objs             |   1 +
hw/char/altera_juart.c            | 288 ++++++++++++++++++++++++++++++++++++++
include/hw/char/altera_juart.h    |  46 ++++++
4 files changed, 336 insertions(+)
create mode 100644 hw/char/altera_juart.c
create mode 100644 include/hw/char/altera_juart.h

[Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Juro Bystricky 163 weeks ago
JTAG UART core eliminates the need for a separate RS-232 serial
connection to a host PC for character I/O.

Hardware emulation based on:
https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
(Please see "Register Map" on page 65)

Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
---
 default-configs/nios2-softmmu.mak |   1 +
 hw/char/Makefile.objs             |   1 +
 hw/char/altera_juart.c            | 288 ++++++++++++++++++++++++++++++++++++++
 include/hw/char/altera_juart.h    |  46 ++++++
 4 files changed, 336 insertions(+)
 create mode 100644 hw/char/altera_juart.c
 create mode 100644 include/hw/char/altera_juart.h

diff --git a/default-configs/nios2-softmmu.mak b/default-configs/nios2-softmmu.mak
index 74dc70c..6159846 100644
--- a/default-configs/nios2-softmmu.mak
+++ b/default-configs/nios2-softmmu.mak
@@ -4,3 +4,4 @@ CONFIG_NIOS2=y
 CONFIG_SERIAL=y
 CONFIG_PTIMER=y
 CONFIG_ALTERA_TIMER=y
+CONFIG_ALTERA_JUART=y
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 6ea76fe..f0d0e25 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -27,5 +27,6 @@ common-obj-$(CONFIG_LM32) += lm32_juart.o
 common-obj-$(CONFIG_LM32) += lm32_uart.o
 common-obj-$(CONFIG_MILKYMIST) += milkymist-uart.o
 common-obj-$(CONFIG_SCLPCONSOLE) += sclpconsole.o sclpconsole-lm.o
+common-obj-$(CONFIG_ALTERA_JUART) += altera_juart.o
 
 obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o
diff --git a/hw/char/altera_juart.c b/hw/char/altera_juart.c
new file mode 100644
index 0000000..0325ddc
--- /dev/null
+++ b/hw/char/altera_juart.c
@@ -0,0 +1,288 @@
+/*
+ * QEMU model of the Altera JTAG UART.
+ *
+ * Copyright (c) 2016-2017 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * The Altera JTAG UART hardware registers are described in:
+ * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
+ * (In particular "Register Map" on page 65)
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/altera_juart.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
+
+/* Data register */
+#define OFFSET_R_DATA   0
+#define DATA_RVALID     BIT(15)
+#define DATA_RAVAIL     0xFFFF0000
+
+/* Control register */
+#define OFFSET_R_CONTROL 4
+#define CONTROL_RE      BIT(0)
+#define CONTROL_WE      BIT(1)
+#define CONTROL_RI      BIT(8)
+#define CONTROL_WI      BIT(9)
+#define CONTROL_AC      BIT(10)
+#define CONTROL_WSPACE  0xFFFF0000
+
+#define CONTROL_WMASK (CONTROL_RE | CONTROL_WE | CONTROL_AC)
+
+#define TYPE_ALTERA_JUART "altera-juart"
+#define ALTERA_JUART(obj) \
+    OBJECT_CHECK(AlteraJUARTState, (obj), TYPE_ALTERA_JUART)
+
+/*
+ * The JTAG UART core generates an interrupt when either of the individual
+ * interrupt conditions is pending and enabled.
+ */
+static void altera_juart_update_irq(AlteraJUARTState *s)
+{
+    unsigned int irq;
+
+    irq = ((s->jcontrol & CONTROL_WE) && (s->jcontrol & CONTROL_WI)) ||
+          ((s->jcontrol & CONTROL_RE) && (s->jcontrol & CONTROL_RI));
+
+    qemu_set_irq(s->irq, irq);
+}
+
+static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    AlteraJUARTState *s = opaque;
+    uint32_t r;
+
+    switch (addr) {
+    case OFFSET_R_DATA:
+        r = s->rx_fifo[(s->rx_fifo_pos - s->rx_fifo_len) & (s->rx_fifo_size - 1)];
+        if (s->rx_fifo_len) {
+            s->rx_fifo_len--;
+            qemu_chr_fe_accept_input(&s->chr);
+            s->jdata = r | DATA_RVALID | (s->rx_fifo_len << 16);
+            s->jcontrol |= CONTROL_RI;
+        } else {
+            s->jdata = 0;
+            s->jcontrol &= ~CONTROL_RI;
+        }
+
+        altera_juart_update_irq(s);
+        return s->jdata;
+
+    case OFFSET_R_CONTROL:
+        return s->jcontrol;
+    }
+
+    return 0;
+}
+
+static void altera_juart_write(void *opaque, hwaddr addr,
+                       uint64_t value, unsigned int size)
+{
+    AlteraJUARTState *s = opaque;
+    unsigned char c;
+
+    switch (addr) {
+    case OFFSET_R_DATA:
+        c = value & 0xFF;
+        /*
+         * We do not decrement the write fifo,
+         * we "tranmsmit" instanteniously, CONTROL_WI always asserted
+         */
+        s->jcontrol |= CONTROL_WI;
+        s->jdata = c;
+        qemu_chr_fe_write(&s->chr, &c, 1);
+        altera_juart_update_irq(s);
+        break;
+
+    case OFFSET_R_CONTROL:
+        /* Only RE and WE are writable */
+        value &= CONTROL_WMASK;
+        s->jcontrol &= ~CONTROL_WMASK;
+        s->jcontrol |= value;
+
+        /* Writing 1 to AC clears it to 0 */
+        if (value & CONTROL_AC) {
+            s->jcontrol &= ~CONTROL_AC;
+        }
+        altera_juart_update_irq(s);
+        break;
+    }
+}
+
+static void altera_juart_receive(void *opaque, const uint8_t *buf, int size)
+{
+    int i;
+    AlteraJUARTState *s = opaque;
+
+    for (i = 0; i < size; i++) {
+        s->rx_fifo[s->rx_fifo_pos] = buf[i];
+        s->rx_fifo_pos++;
+        s->rx_fifo_pos &= (s->rx_fifo_size - 1);
+        s->rx_fifo_len++;
+    }
+    s->jcontrol |= CONTROL_RI;
+    altera_juart_update_irq(s);
+}
+
+static int altera_juart_can_receive(void *opaque)
+{
+    AlteraJUARTState *s = opaque;
+    return s->rx_fifo_size - s->rx_fifo_len;
+}
+
+static void altera_juart_reset(DeviceState *d)
+{
+    AlteraJUARTState *s = ALTERA_JUART(d);
+
+    s->jdata = 0;
+
+    /* The number of spaces available in the write FIFO */
+    s->jcontrol = s->rx_fifo_size << 16;
+    s->rx_fifo_pos = 0;
+    s->rx_fifo_len = 0;
+}
+
+static const MemoryRegionOps juart_ops = {
+    .read = altera_juart_read,
+    .write = altera_juart_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4
+    }
+};
+
+static void altera_juart_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    AlteraJUARTState *s = ALTERA_JUART(obj);
+
+    memory_region_init_io(&s->mmio,  OBJECT(s), &juart_ops, s,
+                          TYPE_ALTERA_JUART, 2 * 4);
+    sysbus_init_mmio(sbd, &s->mmio);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq, uint32_t fifo_size)
+{
+    DeviceState *dev;
+    SysBusDevice *bus;
+    Chardev *chr;
+    const char chr_name[] = "juart";
+    char label[ARRAY_SIZE(chr_name) + 1];
+    bool fifo_size_ok;
+    uint32_t size;
+
+    dev = qdev_create(NULL, TYPE_ALTERA_JUART);
+
+    if (channel >= MAX_SERIAL_PORTS) {
+        error_report("Only %d serial ports are supported by QEMU",
+                     MAX_SERIAL_PORTS);
+        exit(1);
+    }
+
+    chr = serial_hds[channel];
+    if (!chr) {
+        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
+        chr = qemu_chr_new(label, "null");
+        if (!chr) {
+            error_report("Can't assign serial port to altera juart%d", channel);
+            exit(1);
+        }
+    }
+    qdev_prop_set_chr(dev, "chardev", chr);
+
+    /*
+     * FIFO size can be set from 8 to 32,768 bytes.
+     * Only powers of two are allowed.
+     */
+    fifo_size_ok = false;
+    for (size = 8; size <= 32768; size *= 2) {
+        if (size == fifo_size) {
+            fifo_size_ok = true;
+            break;
+        }
+    }
+
+    if (!fifo_size_ok) {
+        error_report("Invalid FIFO size: %u for juart%d", fifo_size, channel);
+        exit(1);
+    }
+
+    qdev_prop_set_uint32(dev, "fifo-size", fifo_size);
+    bus = SYS_BUS_DEVICE(dev);
+    qdev_init_nofail(dev);
+
+    if (addr != (hwaddr)-1) {
+        sysbus_mmio_map(bus, 0, addr);
+    }
+
+    sysbus_connect_irq(bus, 0, irq);
+}
+
+static const VMStateDescription vmstate_altera_juart = {
+    .name = "altera-juart" ,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(jdata, AlteraJUARTState),
+        VMSTATE_UINT32(jcontrol, AlteraJUARTState),
+        VMSTATE_VBUFFER_UINT32(rx_fifo, AlteraJUARTState, 1, NULL, 0, rx_fifo_size),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void altera_juart_realize(DeviceState *dev, Error **errp)
+{
+    AlteraJUARTState *s = ALTERA_JUART(dev);
+    qemu_chr_fe_set_handlers(&s->chr, altera_juart_can_receive,
+                             altera_juart_receive, NULL, s, NULL, true);
+    s->rx_fifo = g_malloc(s->rx_fifo_size);
+}
+
+static void altera_juart_unrealize(DeviceState *dev, Error **errp)
+{
+    AlteraJUARTState *s = ALTERA_JUART(dev);
+    g_free(s->rx_fifo);
+}
+
+static Property altera_juart_props[] = {
+    DEFINE_PROP_CHR("chardev", AlteraJUARTState, chr),
+    DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void altera_juart_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = altera_juart_realize;
+    dc->unrealize = altera_juart_unrealize;
+    dc->props = altera_juart_props;
+    dc->vmsd = &vmstate_altera_juart;
+    dc->reset = altera_juart_reset;
+    dc->desc = "Altera JTAG UART";
+}
+
+static const TypeInfo altera_juart_info = {
+    .name          = TYPE_ALTERA_JUART,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AlteraJUARTState),
+    .instance_init = altera_juart_init,
+    .class_init    = altera_juart_class_init,
+};
+
+static void altera_juart_register(void)
+{
+    type_register_static(&altera_juart_info);
+}
+
+type_init(altera_juart_register)
diff --git a/include/hw/char/altera_juart.h b/include/hw/char/altera_juart.h
new file mode 100644
index 0000000..8b0a4a6
--- /dev/null
+++ b/include/hw/char/altera_juart.h
@@ -0,0 +1,46 @@
+/*
+ * Altera JTAG UART emulation
+ *
+ * Copyright (c) 2016-2017 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * 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 ALTERA_JUART_H
+#define ALTERA_JUART_H
+
+#include "hw/sysbus.h"
+#include "sysemu/char.h"
+
+/*
+ * The read and write FIFO depths can be set from 8 to 32,768 bytes.
+ * Only powers of two are allowed. A depth of 64 is generally optimal for
+ * performance, and larger values are rarely necessary.
+ */
+
+#define DEFAULT_FIFO_SIZE 64
+
+typedef struct AlteraJUARTState {
+    SysBusDevice busdev;
+    MemoryRegion mmio;
+    CharBackend chr;
+    qemu_irq irq;
+
+    unsigned int rx_fifo_size;
+    unsigned int rx_fifo_pos;
+    unsigned int rx_fifo_len;
+    uint32_t jdata;
+    uint32_t jcontrol;
+    uint8_t *rx_fifo;
+} AlteraJUARTState;
+
+void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq,
+                                                        uint32_t fifo_size);
+
+#endif /* ALTERA_JUART_H */
-- 
2.7.4


Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Marek Vasut 163 weeks ago
On 02/09/2017 07:52 PM, Juro Bystricky wrote:
> JTAG UART core eliminates the need for a separate RS-232 serial
> connection to a host PC for character I/O.

And how does this describe the content of this patch ? This patch adds a
model of JTAG UART , it doesn't eliminate anything ...

> Hardware emulation based on:
> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> (Please see "Register Map" on page 65)
> 
> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> ---

Changelog is missing ...

>  default-configs/nios2-softmmu.mak |   1 +
>  hw/char/Makefile.objs             |   1 +
>  hw/char/altera_juart.c            | 288 ++++++++++++++++++++++++++++++++++++++
>  include/hw/char/altera_juart.h    |  46 ++++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 hw/char/altera_juart.c
>  create mode 100644 include/hw/char/altera_juart.h
> 
> diff --git a/default-configs/nios2-softmmu.mak b/default-configs/nios2-softmmu.mak
> index 74dc70c..6159846 100644
> --- a/default-configs/nios2-softmmu.mak
> +++ b/default-configs/nios2-softmmu.mak
> @@ -4,3 +4,4 @@ CONFIG_NIOS2=y
>  CONFIG_SERIAL=y
>  CONFIG_PTIMER=y
>  CONFIG_ALTERA_TIMER=y
> +CONFIG_ALTERA_JUART=y
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 6ea76fe..f0d0e25 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -27,5 +27,6 @@ common-obj-$(CONFIG_LM32) += lm32_juart.o
>  common-obj-$(CONFIG_LM32) += lm32_uart.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-uart.o
>  common-obj-$(CONFIG_SCLPCONSOLE) += sclpconsole.o sclpconsole-lm.o
> +common-obj-$(CONFIG_ALTERA_JUART) += altera_juart.o
>  
>  obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o
> diff --git a/hw/char/altera_juart.c b/hw/char/altera_juart.c
> new file mode 100644
> index 0000000..0325ddc
> --- /dev/null
> +++ b/hw/char/altera_juart.c
> @@ -0,0 +1,288 @@
> +/*
> + * QEMU model of the Altera JTAG UART.
> + *
> + * Copyright (c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * The Altera JTAG UART hardware registers are described in:
> + * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> + * (In particular "Register Map" on page 65)
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/altera_juart.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +/* Data register */
> +#define OFFSET_R_DATA   0
> +#define DATA_RVALID     BIT(15)
> +#define DATA_RAVAIL     0xFFFF0000
> +
> +/* Control register */
> +#define OFFSET_R_CONTROL 4
> +#define CONTROL_RE      BIT(0)
> +#define CONTROL_WE      BIT(1)
> +#define CONTROL_RI      BIT(8)
> +#define CONTROL_WI      BIT(9)
> +#define CONTROL_AC      BIT(10)
> +#define CONTROL_WSPACE  0xFFFF0000
> +
> +#define CONTROL_WMASK (CONTROL_RE | CONTROL_WE | CONTROL_AC)
> +
> +#define TYPE_ALTERA_JUART "altera-juart"
> +#define ALTERA_JUART(obj) \
> +    OBJECT_CHECK(AlteraJUARTState, (obj), TYPE_ALTERA_JUART)
> +
> +/*
> + * The JTAG UART core generates an interrupt when either of the individual
> + * interrupt conditions is pending and enabled.
> + */
> +static void altera_juart_update_irq(AlteraJUARTState *s)
> +{
> +    unsigned int irq;
> +
> +    irq = ((s->jcontrol & CONTROL_WE) && (s->jcontrol & CONTROL_WI)) ||
> +          ((s->jcontrol & CONTROL_RE) && (s->jcontrol & CONTROL_RI));
> +
> +    qemu_set_irq(s->irq, irq);
> +}
> +
> +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    uint32_t r;
> +
> +    switch (addr) {
> +    case OFFSET_R_DATA:
> +        r = s->rx_fifo[(s->rx_fifo_pos - s->rx_fifo_len) & (s->rx_fifo_size - 1)];
> +        if (s->rx_fifo_len) {
> +            s->rx_fifo_len--;
> +            qemu_chr_fe_accept_input(&s->chr);
> +            s->jdata = r | DATA_RVALID | (s->rx_fifo_len << 16);
> +            s->jcontrol |= CONTROL_RI;
> +        } else {
> +            s->jdata = 0;
> +            s->jcontrol &= ~CONTROL_RI;
> +        }
> +
> +        altera_juart_update_irq(s);
> +        return s->jdata;
> +
> +    case OFFSET_R_CONTROL:
> +        return s->jcontrol;
> +    }
> +
> +    return 0;
> +}
> +
> +static void altera_juart_write(void *opaque, hwaddr addr,
> +                       uint64_t value, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    unsigned char c;
> +
> +    switch (addr) {
> +    case OFFSET_R_DATA:
> +        c = value & 0xFF;
> +        /*
> +         * We do not decrement the write fifo,
> +         * we "tranmsmit" instanteniously, CONTROL_WI always asserted
> +         */
> +        s->jcontrol |= CONTROL_WI;
> +        s->jdata = c;
> +        qemu_chr_fe_write(&s->chr, &c, 1);
> +        altera_juart_update_irq(s);
> +        break;
> +
> +    case OFFSET_R_CONTROL:
> +        /* Only RE and WE are writable */
> +        value &= CONTROL_WMASK;
> +        s->jcontrol &= ~CONTROL_WMASK;
> +        s->jcontrol |= value;
> +
> +        /* Writing 1 to AC clears it to 0 */
> +        if (value & CONTROL_AC) {
> +            s->jcontrol &= ~CONTROL_AC;
> +        }
> +        altera_juart_update_irq(s);
> +        break;
> +    }
> +}
> +
> +static void altera_juart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    int i;
> +    AlteraJUARTState *s = opaque;
> +
> +    for (i = 0; i < size; i++) {
> +        s->rx_fifo[s->rx_fifo_pos] = buf[i];
> +        s->rx_fifo_pos++;
> +        s->rx_fifo_pos &= (s->rx_fifo_size - 1);
> +        s->rx_fifo_len++;
> +    }
> +    s->jcontrol |= CONTROL_RI;
> +    altera_juart_update_irq(s);
> +}
> +
> +static int altera_juart_can_receive(void *opaque)
> +{
> +    AlteraJUARTState *s = opaque;
> +    return s->rx_fifo_size - s->rx_fifo_len;
> +}
> +
> +static void altera_juart_reset(DeviceState *d)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(d);
> +
> +    s->jdata = 0;
> +
> +    /* The number of spaces available in the write FIFO */
> +    s->jcontrol = s->rx_fifo_size << 16;
> +    s->rx_fifo_pos = 0;
> +    s->rx_fifo_len = 0;
> +}
> +
> +static const MemoryRegionOps juart_ops = {
> +    .read = altera_juart_read,
> +    .write = altera_juart_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void altera_juart_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    AlteraJUARTState *s = ALTERA_JUART(obj);
> +
> +    memory_region_init_io(&s->mmio,  OBJECT(s), &juart_ops, s,
> +                          TYPE_ALTERA_JUART, 2 * 4);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq, uint32_t fifo_size)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *bus;
> +    Chardev *chr;
> +    const char chr_name[] = "juart";
> +    char label[ARRAY_SIZE(chr_name) + 1];
> +    bool fifo_size_ok;
> +    uint32_t size;
> +
> +    dev = qdev_create(NULL, TYPE_ALTERA_JUART);
> +
> +    if (channel >= MAX_SERIAL_PORTS) {
> +        error_report("Only %d serial ports are supported by QEMU",
> +                     MAX_SERIAL_PORTS);
> +        exit(1);
> +    }
> +
> +    chr = serial_hds[channel];
> +    if (!chr) {
> +        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
> +        chr = qemu_chr_new(label, "null");
> +        if (!chr) {
> +            error_report("Can't assign serial port to altera juart%d", channel);
> +            exit(1);
> +        }
> +    }
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +
> +    /*
> +     * FIFO size can be set from 8 to 32,768 bytes.
> +     * Only powers of two are allowed.
> +     */
> +    fifo_size_ok = false;
> +    for (size = 8; size <= 32768; size *= 2) {
> +        if (size == fifo_size) {
> +            fifo_size_ok = true;
> +            break;
> +        }
> +    }

Isn't that like

if (size < 8 || size > 32768 || (size & ~(1 << ffs(size))))
  error()

> +    if (!fifo_size_ok) {
> +        error_report("Invalid FIFO size: %u for juart%d", fifo_size, channel);
> +        exit(1);
> +    }
> +
> +    qdev_prop_set_uint32(dev, "fifo-size", fifo_size);
> +    bus = SYS_BUS_DEVICE(dev);
> +    qdev_init_nofail(dev);
> +
> +    if (addr != (hwaddr)-1) {
> +        sysbus_mmio_map(bus, 0, addr);
> +    }
> +
> +    sysbus_connect_irq(bus, 0, irq);
> +}
> +
> +static const VMStateDescription vmstate_altera_juart = {
> +    .name = "altera-juart" ,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(jdata, AlteraJUARTState),
> +        VMSTATE_UINT32(jcontrol, AlteraJUARTState),
> +        VMSTATE_VBUFFER_UINT32(rx_fifo, AlteraJUARTState, 1, NULL, 0, rx_fifo_size),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void altera_juart_realize(DeviceState *dev, Error **errp)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(dev);
> +    qemu_chr_fe_set_handlers(&s->chr, altera_juart_can_receive,
> +                             altera_juart_receive, NULL, s, NULL, true);
> +    s->rx_fifo = g_malloc(s->rx_fifo_size);
> +}
> +
> +static void altera_juart_unrealize(DeviceState *dev, Error **errp)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(dev);
> +    g_free(s->rx_fifo);
> +}
> +
> +static Property altera_juart_props[] = {
> +    DEFINE_PROP_CHR("chardev", AlteraJUARTState, chr),
> +    DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void altera_juart_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = altera_juart_realize;
> +    dc->unrealize = altera_juart_unrealize;
> +    dc->props = altera_juart_props;
> +    dc->vmsd = &vmstate_altera_juart;
> +    dc->reset = altera_juart_reset;
> +    dc->desc = "Altera JTAG UART";
> +}
> +
> +static const TypeInfo altera_juart_info = {
> +    .name          = TYPE_ALTERA_JUART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AlteraJUARTState),
> +    .instance_init = altera_juart_init,
> +    .class_init    = altera_juart_class_init,
> +};
> +
> +static void altera_juart_register(void)
> +{
> +    type_register_static(&altera_juart_info);
> +}
> +
> +type_init(altera_juart_register)
> diff --git a/include/hw/char/altera_juart.h b/include/hw/char/altera_juart.h
> new file mode 100644
> index 0000000..8b0a4a6
> --- /dev/null
> +++ b/include/hw/char/altera_juart.h
> @@ -0,0 +1,46 @@
> +/*
> + * Altera JTAG UART emulation
> + *
> + * Copyright (c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * 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 ALTERA_JUART_H
> +#define ALTERA_JUART_H
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/char.h"
> +
> +/*
> + * The read and write FIFO depths can be set from 8 to 32,768 bytes.
> + * Only powers of two are allowed. A depth of 64 is generally optimal for
> + * performance, and larger values are rarely necessary.
> + */
> +
> +#define DEFAULT_FIFO_SIZE 64

This is still not QOM property , why ?

> +typedef struct AlteraJUARTState {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +    CharBackend chr;
> +    qemu_irq irq;
> +
> +    unsigned int rx_fifo_size;
> +    unsigned int rx_fifo_pos;
> +    unsigned int rx_fifo_len;
> +    uint32_t jdata;
> +    uint32_t jcontrol;
> +    uint8_t *rx_fifo;
> +} AlteraJUARTState;
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq,
> +                                                        uint32_t fifo_size);
> +
> +#endif /* ALTERA_JUART_H */
> 


-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Bystricky, Juro 163 weeks ago

> On 02/09/2017 07:52 PM, Juro Bystricky wrote:
> > JTAG UART core eliminates the need for a separate RS-232 serial
> > connection to a host PC for character I/O.
> 
> And how does this describe the content of this patch ? This patch adds a
> model of JTAG UART , it doesn't eliminate anything ...
> 

The commit message explicitly says:
"Add Altera JTAG UART emulation."

But I will amend the message with more details.

> > Hardware emulation based on:
> > https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> > (Please see "Register Map" on page 65)
> >
> > Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> > ---
> 
> Changelog is missing ...

I modelled this patch based on other nios2 patches, none of them contained 
Changelog. So please elaborate.


> > +    /*
> > +     * FIFO size can be set from 8 to 32,768 bytes.
> > +     * Only powers of two are allowed.
> > +     */
> > +    fifo_size_ok = false;
> > +    for (size = 8; size <= 32768; size *= 2) {
> > +        if (size == fifo_size) {
> > +            fifo_size_ok = true;
> > +            break;
> > +        }
> > +    }
> 
> Isn't that like
> 
> if (size < 8 || size > 32768 || (size & ~(1 << ffs(size))))
>   error()
> 

possibly, however ffs is rejected by checkpatch as non-portable
libc call. 


> > +#define DEFAULT_FIFO_SIZE 64
> 
> This is still not QOM property , why ?
> 

Not sure what you mean, the code has:

DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),


Thanks

Juro


Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Marek Vasut 163 weeks ago
On 02/09/2017 09:53 PM, Bystricky, Juro wrote:
> 
> 
>> On 02/09/2017 07:52 PM, Juro Bystricky wrote:
>>> JTAG UART core eliminates the need for a separate RS-232 serial
>>> connection to a host PC for character I/O.
>>
>> And how does this describe the content of this patch ? This patch adds a
>> model of JTAG UART , it doesn't eliminate anything ...
>>
> 
> The commit message explicitly says:
> "Add Altera JTAG UART emulation."
> 
> But I will amend the message with more details.

The commit message is totally misleading though.

>>> Hardware emulation based on:
>>> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
>>> (Please see "Register Map" on page 65)
>>>
>>> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
>>> ---
>>
>> Changelog is missing ...
> 
> I modelled this patch based on other nios2 patches, none of them contained 
> Changelog. So please elaborate.

It's common courtesy to avoid wasting reviewers time by adding a
changelog between patch versions.

>>> +    /*
>>> +     * FIFO size can be set from 8 to 32,768 bytes.
>>> +     * Only powers of two are allowed.
>>> +     */
>>> +    fifo_size_ok = false;
>>> +    for (size = 8; size <= 32768; size *= 2) {
>>> +        if (size == fifo_size) {
>>> +            fifo_size_ok = true;
>>> +            break;
>>> +        }
>>> +    }
>>
>> Isn't that like
>>
>> if (size < 8 || size > 32768 || (size & ~(1 << ffs(size))))
>>   error()
>>
> 
> possibly, however ffs is rejected by checkpatch as non-portable
> libc call. 

Doesn't it suggest to use ctz32() ?
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03661.html

>>> +#define DEFAULT_FIFO_SIZE 64
>>
>> This is still not QOM property , why ?
>>
> 
> Not sure what you mean, the code has:
> 
> DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),

Ah ok, I missed that (and that should've been part of the changelog for
example ... )

-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Frederic Konrad 163 weeks ago
On 02/09/2017 09:53 PM, Bystricky, Juro wrote:
> 
> 
>> On 02/09/2017 07:52 PM, Juro Bystricky wrote:
>>> JTAG UART core eliminates the need for a separate RS-232 serial
>>> connection to a host PC for character I/O.
>>
>> And how does this describe the content of this patch ? This patch adds a
>> model of JTAG UART , it doesn't eliminate anything ...
>>
> 
> The commit message explicitly says:
> "Add Altera JTAG UART emulation."
> 
> But I will amend the message with more details.
> 
>>> Hardware emulation based on:
>>> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
>>> (Please see "Register Map" on page 65)
>>>
>>> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
>>> ---
>>
>> Changelog is missing ...
> 
> I modelled this patch based on other nios2 patches, none of them contained 
> Changelog. So please elaborate.

Things after "---" disappear when commited in the git.

Thanks,
Fred

> 
> 
>>> +    /*
>>> +     * FIFO size can be set from 8 to 32,768 bytes.
>>> +     * Only powers of two are allowed.
>>> +     */
>>> +    fifo_size_ok = false;
>>> +    for (size = 8; size <= 32768; size *= 2) {
>>> +        if (size == fifo_size) {
>>> +            fifo_size_ok = true;
>>> +            break;
>>> +        }
>>> +    }
>>
>> Isn't that like
>>
>> if (size < 8 || size > 32768 || (size & ~(1 << ffs(size))))
>>   error()
>>
> 
> possibly, however ffs is rejected by checkpatch as non-portable
> libc call. 
> 
> 
>>> +#define DEFAULT_FIFO_SIZE 64
>>
>> This is still not QOM property , why ?
>>
> 
> Not sure what you mean, the code has:
> 
> DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),
> 
> 
> Thanks
> 
> Juro
> 
> 


Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Bystricky, Juro 163 weeks ago
Hello Fred, thanks for the clarification.

> >> Changelog is missing ...
> >
> > I modelled this patch based on other nios2 patches, none of them
> contained
> > Changelog. So please elaborate.
> 
> Things after "---" disappear when commited in the git.
> 
> Thanks,
> Fred
> 

Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Frederic Konrad 163 weeks ago
Hi,

On 02/09/2017 07:52 PM, Juro Bystricky wrote:
> JTAG UART core eliminates the need for a separate RS-232 serial
> connection to a host PC for character I/O.
> 
> Hardware emulation based on:
> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> (Please see "Register Map" on page 65)
> 
> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> ---
>  default-configs/nios2-softmmu.mak |   1 +
>  hw/char/Makefile.objs             |   1 +
>  hw/char/altera_juart.c            | 288 ++++++++++++++++++++++++++++++++++++++
>  include/hw/char/altera_juart.h    |  46 ++++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 hw/char/altera_juart.c
>  create mode 100644 include/hw/char/altera_juart.h
> 
> diff --git a/default-configs/nios2-softmmu.mak b/default-configs/nios2-softmmu.mak
> index 74dc70c..6159846 100644
> --- a/default-configs/nios2-softmmu.mak
> +++ b/default-configs/nios2-softmmu.mak
> @@ -4,3 +4,4 @@ CONFIG_NIOS2=y
>  CONFIG_SERIAL=y
>  CONFIG_PTIMER=y
>  CONFIG_ALTERA_TIMER=y
> +CONFIG_ALTERA_JUART=y
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 6ea76fe..f0d0e25 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -27,5 +27,6 @@ common-obj-$(CONFIG_LM32) += lm32_juart.o
>  common-obj-$(CONFIG_LM32) += lm32_uart.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-uart.o
>  common-obj-$(CONFIG_SCLPCONSOLE) += sclpconsole.o sclpconsole-lm.o
> +common-obj-$(CONFIG_ALTERA_JUART) += altera_juart.o
>  
>  obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o
> diff --git a/hw/char/altera_juart.c b/hw/char/altera_juart.c
> new file mode 100644
> index 0000000..0325ddc
> --- /dev/null
> +++ b/hw/char/altera_juart.c
> @@ -0,0 +1,288 @@
> +/*
> + * QEMU model of the Altera JTAG UART.
> + *
> + * Copyright (c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * The Altera JTAG UART hardware registers are described in:
> + * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> + * (In particular "Register Map" on page 65)
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/altera_juart.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +/* Data register */
> +#define OFFSET_R_DATA   0
> +#define DATA_RVALID     BIT(15)
> +#define DATA_RAVAIL     0xFFFF0000
> +
> +/* Control register */
> +#define OFFSET_R_CONTROL 4
> +#define CONTROL_RE      BIT(0)
> +#define CONTROL_WE      BIT(1)
> +#define CONTROL_RI      BIT(8)
> +#define CONTROL_WI      BIT(9)
> +#define CONTROL_AC      BIT(10)
> +#define CONTROL_WSPACE  0xFFFF0000
> +
> +#define CONTROL_WMASK (CONTROL_RE | CONTROL_WE | CONTROL_AC)
> +
> +#define TYPE_ALTERA_JUART "altera-juart"
> +#define ALTERA_JUART(obj) \
> +    OBJECT_CHECK(AlteraJUARTState, (obj), TYPE_ALTERA_JUART)
> +
> +/*
> + * The JTAG UART core generates an interrupt when either of the individual
> + * interrupt conditions is pending and enabled.
> + */
> +static void altera_juart_update_irq(AlteraJUARTState *s)
> +{
> +    unsigned int irq;
> +
> +    irq = ((s->jcontrol & CONTROL_WE) && (s->jcontrol & CONTROL_WI)) ||
> +          ((s->jcontrol & CONTROL_RE) && (s->jcontrol & CONTROL_RI));
> +
> +    qemu_set_irq(s->irq, irq);
> +}
> +
> +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    uint32_t r;
> +
> +    switch (addr) {
> +    case OFFSET_R_DATA:
> +        r = s->rx_fifo[(s->rx_fifo_pos - s->rx_fifo_len) & (s->rx_fifo_size - 1)];
> +        if (s->rx_fifo_len) {
> +            s->rx_fifo_len--;
> +            qemu_chr_fe_accept_input(&s->chr);
> +            s->jdata = r | DATA_RVALID | (s->rx_fifo_len << 16);
> +            s->jcontrol |= CONTROL_RI;
> +        } else {
> +            s->jdata = 0;
> +            s->jcontrol &= ~CONTROL_RI;
> +        }
> +
> +        altera_juart_update_irq(s);
> +        return s->jdata;
> +
> +    case OFFSET_R_CONTROL:
> +        return s->jcontrol;
> +    }
> +
> +    return 0;
> +}
> +
> +static void altera_juart_write(void *opaque, hwaddr addr,
> +                       uint64_t value, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    unsigned char c;
> +
> +    switch (addr) {
> +    case OFFSET_R_DATA:
> +        c = value & 0xFF;
> +        /*
> +         * We do not decrement the write fifo,
> +         * we "tranmsmit" instanteniously, CONTROL_WI always asserted
> +         */
> +        s->jcontrol |= CONTROL_WI;
> +        s->jdata = c;
> +        qemu_chr_fe_write(&s->chr, &c, 1);
> +        altera_juart_update_irq(s);
> +        break;
> +
> +    case OFFSET_R_CONTROL:
> +        /* Only RE and WE are writable */
> +        value &= CONTROL_WMASK;
> +        s->jcontrol &= ~CONTROL_WMASK;
> +        s->jcontrol |= value;
> +
> +        /* Writing 1 to AC clears it to 0 */
> +        if (value & CONTROL_AC) {
> +            s->jcontrol &= ~CONTROL_AC;
> +        }
> +        altera_juart_update_irq(s);
> +        break;
> +    }
> +}
> +
> +static void altera_juart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    int i;
> +    AlteraJUARTState *s = opaque;
> +
> +    for (i = 0; i < size; i++) {
> +        s->rx_fifo[s->rx_fifo_pos] = buf[i];
> +        s->rx_fifo_pos++;
> +        s->rx_fifo_pos &= (s->rx_fifo_size - 1);
> +        s->rx_fifo_len++;
> +    }
> +    s->jcontrol |= CONTROL_RI;
> +    altera_juart_update_irq(s);
> +}
> +
> +static int altera_juart_can_receive(void *opaque)
> +{
> +    AlteraJUARTState *s = opaque;
> +    return s->rx_fifo_size - s->rx_fifo_len;
> +}
> +
> +static void altera_juart_reset(DeviceState *d)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(d);
> +
> +    s->jdata = 0;
> +
> +    /* The number of spaces available in the write FIFO */
> +    s->jcontrol = s->rx_fifo_size << 16;
> +    s->rx_fifo_pos = 0;
> +    s->rx_fifo_len = 0;
> +}
> +
> +static const MemoryRegionOps juart_ops = {
> +    .read = altera_juart_read,
> +    .write = altera_juart_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void altera_juart_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    AlteraJUARTState *s = ALTERA_JUART(obj);
> +
> +    memory_region_init_io(&s->mmio,  OBJECT(s), &juart_ops, s,
> +                          TYPE_ALTERA_JUART, 2 * 4);

What's this 2 * 4?
I'd suggest you use a #define for that.

> +    sysbus_init_mmio(sbd, &s->mmio);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq, uint32_t fifo_size)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *bus;
> +    Chardev *chr;
> +    const char chr_name[] = "juart";
> +    char label[ARRAY_SIZE(chr_name) + 1];
> +    bool fifo_size_ok;
> +    uint32_t size;
> +
> +    dev = qdev_create(NULL, TYPE_ALTERA_JUART);
> +
> +    if (channel >= MAX_SERIAL_PORTS) {
> +        error_report("Only %d serial ports are supported by QEMU",
> +                     MAX_SERIAL_PORTS);
> +        exit(1);
> +    }
> +
> +    chr = serial_hds[channel];
> +    if (!chr) {
> +        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
> +        chr = qemu_chr_new(label, "null");
> +        if (!chr) {
> +            error_report("Can't assign serial port to altera juart%d", channel);

Can't you reuse label?

> +            exit(1);
> +        }
> +    }
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +
> +    /*
> +     * FIFO size can be set from 8 to 32,768 bytes.
> +     * Only powers of two are allowed.
> +     */
> +    fifo_size_ok = false;
> +    for (size = 8; size <= 32768; size *= 2) {
> +        if (size == fifo_size) {
> +            fifo_size_ok = true;
> +            break;
> +        }
> +    }
> +
> +    if (!fifo_size_ok) {
> +        error_report("Invalid FIFO size: %u for juart%d", fifo_size, channel);
> +        exit(1);
> +    }
> +
> +    qdev_prop_set_uint32(dev, "fifo-size", fifo_size);
> +    bus = SYS_BUS_DEVICE(dev);
> +    qdev_init_nofail(dev);
> +
> +    if (addr != (hwaddr)-1) {
> +        sysbus_mmio_map(bus, 0, addr);
> +    }
> +
> +    sysbus_connect_irq(bus, 0, irq);
> +}
> +
> +static const VMStateDescription vmstate_altera_juart = {
> +    .name = "altera-juart" ,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(jdata, AlteraJUARTState),
> +        VMSTATE_UINT32(jcontrol, AlteraJUARTState),
> +        VMSTATE_VBUFFER_UINT32(rx_fifo, AlteraJUARTState, 1, NULL, 0, rx_fifo_size),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void altera_juart_realize(DeviceState *dev, Error **errp)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(dev);
> +    qemu_chr_fe_set_handlers(&s->chr, altera_juart_can_receive,
> +                             altera_juart_receive, NULL, s, NULL, true);
> +    s->rx_fifo = g_malloc(s->rx_fifo_size);
> +}
> +
> +static void altera_juart_unrealize(DeviceState *dev, Error **errp)
> +{
> +    AlteraJUARTState *s = ALTERA_JUART(dev);
> +    g_free(s->rx_fifo);
> +}
> +
> +static Property altera_juart_props[] = {
> +    DEFINE_PROP_CHR("chardev", AlteraJUARTState, chr),
> +    DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, DEFAULT_FIFO_SIZE),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void altera_juart_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = altera_juart_realize;
> +    dc->unrealize = altera_juart_unrealize;
> +    dc->props = altera_juart_props;
> +    dc->vmsd = &vmstate_altera_juart;
> +    dc->reset = altera_juart_reset;
> +    dc->desc = "Altera JTAG UART";
> +}
> +
> +static const TypeInfo altera_juart_info = {
> +    .name          = TYPE_ALTERA_JUART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AlteraJUARTState),
> +    .instance_init = altera_juart_init,
> +    .class_init    = altera_juart_class_init,
> +};
> +
> +static void altera_juart_register(void)
> +{
> +    type_register_static(&altera_juart_info);
> +}
> +
> +type_init(altera_juart_register)
> diff --git a/include/hw/char/altera_juart.h b/include/hw/char/altera_juart.h
> new file mode 100644
> index 0000000..8b0a4a6
> --- /dev/null
> +++ b/include/hw/char/altera_juart.h
> @@ -0,0 +1,46 @@
> +/*
> + * Altera JTAG UART emulation
> + *
> + * Copyright (c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * 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 ALTERA_JUART_H
> +#define ALTERA_JUART_H
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/char.h"
> +
> +/*
> + * The read and write FIFO depths can be set from 8 to 32,768 bytes.
> + * Only powers of two are allowed. A depth of 64 is generally optimal for
> + * performance, and larger values are rarely necessary.
> + */
> +
> +#define DEFAULT_FIFO_SIZE 64

The name here might conflict with other thing.
I'd change that by ALTERA_JUART_DEFAULT_FIFO_SZ or something.


> +
> +typedef struct AlteraJUARTState {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +    CharBackend chr;
> +    qemu_irq irq;
> +
> +    unsigned int rx_fifo_size;
> +    unsigned int rx_fifo_pos;
> +    unsigned int rx_fifo_len;
> +    uint32_t jdata;
> +    uint32_t jcontrol;
> +    uint8_t *rx_fifo;
> +} AlteraJUARTState;
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq,
> +                                                        uint32_t fifo_size);
> +
> +#endif /* ALTERA_JUART_H */
> 

The rest seems ok to me.

Thanks,
Fred

Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation

Posted by Bystricky, Juro 163 weeks ago
Hello Konrad, 
I agree with all your comments.

[...]

> > +static void altera_juart_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    AlteraJUARTState *s = ALTERA_JUART(obj);
> > +
> > +    memory_region_init_io(&s->mmio,  OBJECT(s), &juart_ops, s,
> > +                          TYPE_ALTERA_JUART, 2 * 4);
> 
> What's this 2 * 4?
> I'd suggest you use a #define for that.

yes, not very clear it is the memory size, will redo

[...]

> > +    chr = serial_hds[channel];
> > +    if (!chr) {
> > +        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
> > +        chr = qemu_chr_new(label, "null");
> > +        if (!chr) {
> > +            error_report("Can't assign serial port to altera juart%d",
> channel);
> 
> Can't you reuse label?
> 

Will rework the message to use "label"

[...]


> > +#define DEFAULT_FIFO_SIZE 64
> 
> The name here might conflict with other thing.
> I'd change that by ALTERA_JUART_DEFAULT_FIFO_SZ or something.
> 
> 

I agree, will rename

[...]

> 
> The rest seems ok to me.
> 


Thanks for reviewing the code.
Juro