[Qemu-devel] [PATCH v3 0/1] nios2: Add Altera JTAG UART emulation

[Qemu-devel] [PATCH v3 0/1] nios2: Add Altera JTAG UART emulation

[Juro Bystricky]

v3: including a link to HW registers specs


Juro Bystricky (1):
  nios2: Add Altera JTAG UART emulation

 default-configs/nios2-softmmu.mak |   1 +
 hw/char/Makefile.objs             |   1 +
 hw/char/altera_juart.c            | 268 ++++++++++++++++++++++++++++++++++++++
 include/hw/char/altera_juart.h    |  44 +++++++
 4 files changed, 314 insertions(+)
 create mode 100644 hw/char/altera_juart.c
 create mode 100644 include/hw/char/altera_juart.h

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Juro Bystricky]

Hardware emulation based on:
https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf

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            | 268 ++++++++++++++++++++++++++++++++++++++
 include/hw/char/altera_juart.h    |  44 +++++++
 4 files changed, 314 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..f143167
--- /dev/null
+++ b/hw/char/altera_juart.c
@@ -0,0 +1,268 @@
+/*
+ * 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
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/altera_juart.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
+
+/* Data register */
+#define R_DATA      0
+#define DATA_RVALID     BIT(15)
+#define DATA_RAVAIL     0xFFFF0000
+
+/* Control register */
+#define R_CONTROL   1
+#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;
+
+    addr >>= 2;
+
+    switch (addr) {
+    case R_DATA:
+        r = s->rx_fifo[(s->rx_fifo_pos - s->rx_fifo_len) & (FIFO_LENGTH - 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 R_CONTROL:
+        return s->jcontrol;
+    }
+
+    return 0;
+}
+
+static void altera_juart_write(void *opaque, hwaddr addr,
+                       uint64_t val64, unsigned int size)
+{
+    AlteraJUARTState *s = opaque;
+    uint32_t value = val64;
+    unsigned char c;
+
+    addr >>= 2;
+
+    switch (addr) {
+    case 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 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;
+
+    if (s->rx_fifo_len >= FIFO_LENGTH) {
+        fprintf(stderr, "WARNING: UART dropped char.\n");
+        return;
+    }
+
+    for (i = 0; i < size; i++) {
+        s->rx_fifo[s->rx_fifo_pos] = buf[i];
+        s->rx_fifo_pos++;
+        s->rx_fifo_pos &= (FIFO_LENGTH - 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 FIFO_LENGTH - 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 = FIFO_LENGTH << 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)
+{
+    DeviceState *dev;
+    SysBusDevice *bus;
+    Chardev *chr;
+    const char chr_name[] = "juart";
+    char label[ARRAY_SIZE(chr_name) + 1];
+
+    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);
+    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_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);
+}
+
+static Property altera_juart_props[] = {
+    DEFINE_PROP_CHR("chardev", AlteraJUARTState, chr),
+    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->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..8a9c892
--- /dev/null
+++ b/include/hw/char/altera_juart.h
@@ -0,0 +1,44 @@
+/*
+ * 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 FIFO_LENGTH 64
+
+typedef struct AlteraJUARTState {
+    SysBusDevice busdev;
+    MemoryRegion mmio;
+    CharBackend chr;
+    qemu_irq irq;
+
+    unsigned int rx_fifo_pos;
+    unsigned int rx_fifo_len;
+    uint32_t jdata;
+    uint32_t jcontrol;
+    uint8_t rx_fifo[FIFO_LENGTH];
+} AlteraJUARTState;
+
+void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq);
+
+#endif /* ALTERA_JUART_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Marek Vasut]

On 02/09/2017 12:06 AM, Juro Bystricky wrote:
> Hardware emulation based on:
> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf

Please make the commit message a bit more verbose, looking just at the
commit message, I have no clue what patch I'm looking at. The embedded
IP UG also has like 30 cores in it, so some detail on which core and in
which chapter/page would help.

> 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            | 268 ++++++++++++++++++++++++++++++++++++++
>  include/hw/char/altera_juart.h    |  44 +++++++
>  4 files changed, 314 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..f143167
> --- /dev/null
> +++ b/hw/char/altera_juart.c
> @@ -0,0 +1,268 @@
> +/*
> + * 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/>.

Isn't QEMU GPLv2 only and NOT GPLv2+ ?

> + * The Altera JTAG UART hardware registers are described in:
> + * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> + *

Drop the trailing newline

> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/altera_juart.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +/* Data register */
> +#define R_DATA      0
> +#define DATA_RVALID     BIT(15)
> +#define DATA_RAVAIL     0xFFFF0000
> +
> +/* Control register */
> +#define R_CONTROL   1
> +#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;
> +
> +    addr >>= 2;

Hmmmmm, how will unaligned read from one of these registers be handled
on real HW ? ie. read from address 0x3 ? What about writes ?

> +    switch (addr) {
> +    case R_DATA:
> +        r = s->rx_fifo[(s->rx_fifo_pos - s->rx_fifo_len) & (FIFO_LENGTH - 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 R_CONTROL:
> +        return s->jcontrol;
> +    }
> +
> +    return 0;
> +}
> +
> +static void altera_juart_write(void *opaque, hwaddr addr,
> +                       uint64_t val64, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    uint32_t value = val64;

Is this needed ?

> +    unsigned char c;
> +
> +    addr >>= 2;
> +
> +    switch (addr) {
> +    case 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 R_CONTROL:
> +        /* Only RE and WE are writable */
> +        value &= CONTROL_WMASK;
> +        s->jcontrol &= ~(CONTROL_WMASK);

The parenthesis are not needed.

> +        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;
> +
> +    if (s->rx_fifo_len >= FIFO_LENGTH) {
> +        fprintf(stderr, "WARNING: UART dropped char.\n");

Can this ever happen ? can_receive will IMO prevent this case.

> +        return;
> +    }
> +
> +    for (i = 0; i < size; i++) {
> +        s->rx_fifo[s->rx_fifo_pos] = buf[i];
> +        s->rx_fifo_pos++;
> +        s->rx_fifo_pos &= (FIFO_LENGTH - 1);
> +        s->rx_fifo_len++;
> +    }
> +    s->jcontrol |= CONTROL_RI;
> +    altera_juart_update_irq(s);
> +}

[...]

> diff --git a/include/hw/char/altera_juart.h b/include/hw/char/altera_juart.h
> new file mode 100644
> index 0000000..8a9c892
> --- /dev/null
> +++ b/include/hw/char/altera_juart.h
> @@ -0,0 +1,44 @@
> +/*
> + * 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 FIFO_LENGTH 64

Should probably be a QOM property, no ?

> +typedef struct AlteraJUARTState {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +    CharBackend chr;
> +    qemu_irq irq;
> +
> +    unsigned int rx_fifo_pos;
> +    unsigned int rx_fifo_len;
> +    uint32_t jdata;
> +    uint32_t jcontrol;
> +    uint8_t rx_fifo[FIFO_LENGTH];
> +} AlteraJUARTState;
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq);
> +
> +#endif /* ALTERA_JUART_H */
> 
btw for trivial patches like this, cover letter is not necessary .

-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Peter Maydell]

On 8 February 2017 at 23:22, Marek Vasut <marex@denx.de> wrote:
> On 02/09/2017 12:06 AM, Juro Bystricky wrote:
>> + * 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/>.
>
> Isn't QEMU GPLv2 only and NOT GPLv2+ ?

As a binary as a whole QEMU is GPLv2, because that is the most
restrictive and thus the controlling license. Individual
source files are under a variety of licenses, which is
fine as long as those licenses are GPL-2-compatible.
We have a preference against 2-only (see the LICENSE file),
and also you should at least use a license we're already
using. GPL2-or-later is fine and a common choice.

> btw for trivial patches like this, cover letter is not necessary .

The rough rule is "single patches don't need a cover letter;
series of more than one patch always do."

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Bystricky, Juro]

> Please make the commit message a bit more verbose, looking just at the
> commit message, I have no clue what patch I'm looking at. The embedded
> IP UG also has like 30 cores in it, so some detail on which core and in
> which chapter/page would help.

OK, will do

> > + * The Altera JTAG UART hardware registers are described in:
> > + * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> > + *
> 
> Drop the trailing newline
> 

OK, will do

> > +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned
> int size)
> > +{
> > +    AlteraJUARTState *s = opaque;
> > +    uint32_t r;
> > +
> > +    addr >>= 2;
> 
> Hmmmmm, how will unaligned read from one of these registers be handled
> on real HW ? ie. read from address 0x3 ? What about writes ?
> 

there is no reading/writing going on via "addr". 
This just maps the hw address into register number, where registers are at
4 bytes boundaries (so they are aligned as needed) but indexed as 1,2,3.... 
(Pretty common code in other drivers.)
But will redo the code anyway so there are no shifts.


> > +static void altera_juart_write(void *opaque, hwaddr addr,
> > +                       uint64_t val64, unsigned int size)
> > +{
> > +    AlteraJUARTState *s = opaque;
> > +    uint32_t value = val64;
> 
> Is this needed ?

Good question. Legacy code, probably not needed anymore. Same in
extraxfs_ser.c, Xilinx_uartlite.c, stm32f2xx_usart.c.
I suspect in the past some compiler generated warnings about type mismatch (32/64). Will remove.

> 
> > +    unsigned char c;
> > +
> > +    addr >>= 2;
> > +
> > +    switch (addr) {
> > +    case 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 R_CONTROL:
> > +        /* Only RE and WE are writable */
> > +        value &= CONTROL_WMASK;
> > +        s->jcontrol &= ~(CONTROL_WMASK);
> 
> The parenthesis are not needed.

OK, will remove them

> 
> > +static void altera_juart_receive(void *opaque, const uint8_t *buf, int
> size)
> > +{
> > +    int i;
> > +    AlteraJUARTState *s = opaque;
> > +
> > +    if (s->rx_fifo_len >= FIFO_LENGTH) {
> > +        fprintf(stderr, "WARNING: UART dropped char.\n");
> 
> Can this ever happen ? can_receive will IMO prevent this case.
> 

Fair question. I did not go through all QEMU code to see when/where
in the bowels of QEMU can_receive is actually called. If it is always called prior "receive", then this check may be redundant. Again, there are
other serial drivers that do this check as well (etraxfs, Xilinx,...)
But most don't, so I'll follow the majority and remove it.

> > diff --git a/include/hw/char/altera_juart.h
> b/include/hw/char/altera_juart.h
> > new file mode 100644
> > index 0000000..8a9c892
> > --- /dev/null
> > +++ b/include/hw/char/altera_juart.h
> > @@ -0,0 +1,44 @@
> > +/*
> > + * 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 FIFO_LENGTH 64
> 
> Should probably be a QOM property, no ?

Did not want to mess with dynamic FIFO buffer allocation.


> 
> > +typedef struct AlteraJUARTState {
> > +    SysBusDevice busdev;
> > +    MemoryRegion mmio;
> > +    CharBackend chr;
> > +    qemu_irq irq;
> > +
> > +    unsigned int rx_fifo_pos;
> > +    unsigned int rx_fifo_len;
> > +    uint32_t jdata;
> > +    uint32_t jcontrol;
> > +    uint8_t rx_fifo[FIFO_LENGTH];
> > +} AlteraJUARTState;
> > +
> > +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq);
> > +
> > +#endif /* ALTERA_JUART_H */
> >
> btw for trivial patches like this, cover letter is not necessary .

Thanks, good to know, makes life easier
Juro

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Marek Vasut]

On 02/09/2017 01:54 AM, Bystricky, Juro wrote:

[...]

>>> +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned
>> int size)
>>> +{
>>> +    AlteraJUARTState *s = opaque;
>>> +    uint32_t r;
>>> +
>>> +    addr >>= 2;
>>
>> Hmmmmm, how will unaligned read from one of these registers be handled
>> on real HW ? ie. read from address 0x3 ? What about writes ?
>>
> 
> there is no reading/writing going on via "addr". 
> This just maps the hw address into register number, where registers are at
> 4 bytes boundaries (so they are aligned as needed) but indexed as 1,2,3.... 
> (Pretty common code in other drivers.)
> But will redo the code anyway so there are no shifts.

This doesn't answer my question at all. How does real hardware behave if
you read from unaligned address in the register space , ie. offset 0x3 ?

[...]

>>> +static void altera_juart_receive(void *opaque, const uint8_t *buf, int
>> size)
>>> +{
>>> +    int i;
>>> +    AlteraJUARTState *s = opaque;
>>> +
>>> +    if (s->rx_fifo_len >= FIFO_LENGTH) {
>>> +        fprintf(stderr, "WARNING: UART dropped char.\n");
>>
>> Can this ever happen ? can_receive will IMO prevent this case.
>>
> 
> Fair question. I did not go through all QEMU code to see when/where
> in the bowels of QEMU can_receive is actually called. If it is always called prior "receive", then this check may be redundant. Again, there are
> other serial drivers that do this check as well (etraxfs, Xilinx,...)
> But most don't, so I'll follow the majority and remove it.

Maybe you can check the core code or someone else can jump in an answer
this.

>>> diff --git a/include/hw/char/altera_juart.h
>> b/include/hw/char/altera_juart.h
>>> new file mode 100644
>>> index 0000000..8a9c892
>>> --- /dev/null
>>> +++ b/include/hw/char/altera_juart.h
>>> @@ -0,0 +1,44 @@
>>> +/*
>>> + * 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 FIFO_LENGTH 64
>>
>> Should probably be a QOM property, no ?
> 
> Did not want to mess with dynamic FIFO buffer allocation.

You probably should, since this is configurable at the FPGA level (in
QSys), right ?

>>> +typedef struct AlteraJUARTState {
>>> +    SysBusDevice busdev;
>>> +    MemoryRegion mmio;
>>> +    CharBackend chr;
>>> +    qemu_irq irq;
>>> +
>>> +    unsigned int rx_fifo_pos;
>>> +    unsigned int rx_fifo_len;
>>> +    uint32_t jdata;
>>> +    uint32_t jcontrol;
>>> +    uint8_t rx_fifo[FIFO_LENGTH];
>>> +} AlteraJUARTState;
>>> +
>>> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq);
>>> +
>>> +#endif /* ALTERA_JUART_H */
>>>
>> btw for trivial patches like this, cover letter is not necessary .
> 
> Thanks, good to know, makes life easier
> Juro
> 


-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Bystricky, Juro]

> [...]
> 
> >>> +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned
> >> int size)
> >>> +{
> >>> +    AlteraJUARTState *s = opaque;
> >>> +    uint32_t r;
> >>> +
> >>> +    addr >>= 2;
> >>
> >> Hmmmmm, how will unaligned read from one of these registers be handled
> >> on real HW ? ie. read from address 0x3 ? What about writes ?
> >>
> >
> > there is no reading/writing going on via "addr".
> > This just maps the hw address into register number, where registers are
> at
> > 4 bytes boundaries (so they are aligned as needed) but indexed as
> 1,2,3....
> > (Pretty common code in other drivers.)
> > But will redo the code anyway so there are no shifts.
> 
> This doesn't answer my question at all. How does real hardware behave if
> you read from unaligned address in the register space , ie. offset 0x3 ?
> 

Not sure I understand the question here. Which "real hardware" are we talking about?
If "real hardware" contains MMU or MPU then an exception is generated on misalign access.

> [...]
> 
> >>> +static void altera_juart_receive(void *opaque, const uint8_t *buf, int
> >> size)
> >>> +{
> >>> +    int i;
> >>> +    AlteraJUARTState *s = opaque;
> >>> +
> >>> +    if (s->rx_fifo_len >= FIFO_LENGTH) {
> >>> +        fprintf(stderr, "WARNING: UART dropped char.\n");
> >>
> >> Can this ever happen ? can_receive will IMO prevent this case.
> >>
> >
> > Fair question. I did not go through all QEMU code to see when/where
> > in the bowels of QEMU can_receive is actually called. If it is always
> called prior "receive", then this check may be redundant. Again, there are
> > other serial drivers that do this check as well (etraxfs, Xilinx,...)
> > But most don't, so I'll follow the majority and remove it.
> 
> Maybe you can check the core code or someone else can jump in an answer
> this.

OK, will look deeper

[...] 

> >>> +#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 FIFO_LENGTH 64
> >>
> >> Should probably be a QOM property, no ?
> >
> > Did not want to mess with dynamic FIFO buffer allocation.
> 
> You probably should, since this is configurable at the FPGA level (in
> QSys), right ?
> 

OK, will implement fifo size as a property


Thanks
Juro

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Marek Vasut]

On 02/09/2017 04:22 PM, Bystricky, Juro wrote:
> 
> 
>> [...]
>>
>>>>> +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned
>>>> int size)
>>>>> +{
>>>>> +    AlteraJUARTState *s = opaque;
>>>>> +    uint32_t r;
>>>>> +
>>>>> +    addr >>= 2;
>>>>
>>>> Hmmmmm, how will unaligned read from one of these registers be handled
>>>> on real HW ? ie. read from address 0x3 ? What about writes ?
>>>>
>>>
>>> there is no reading/writing going on via "addr".
>>> This just maps the hw address into register number, where registers are
>> at
>>> 4 bytes boundaries (so they are aligned as needed) but indexed as
>> 1,2,3....
>>> (Pretty common code in other drivers.)
>>> But will redo the code anyway so there are no shifts.
>>
>> This doesn't answer my question at all. How does real hardware behave if
>> you read from unaligned address in the register space , ie. offset 0x3 ?
>>
> 
> Not sure I understand the question here. Which "real hardware" are we talking about?

By real hardware I mean real Nios2 system ...

> If "real hardware" contains MMU or MPU then an exception is generated on misalign access.

Is this handled here or not ?

>> [...]
>>
>>>>> +static void altera_juart_receive(void *opaque, const uint8_t *buf, int
>>>> size)
>>>>> +{
>>>>> +    int i;
>>>>> +    AlteraJUARTState *s = opaque;
>>>>> +
>>>>> +    if (s->rx_fifo_len >= FIFO_LENGTH) {
>>>>> +        fprintf(stderr, "WARNING: UART dropped char.\n");
>>>>
>>>> Can this ever happen ? can_receive will IMO prevent this case.
>>>>
>>>
>>> Fair question. I did not go through all QEMU code to see when/where
>>> in the bowels of QEMU can_receive is actually called. If it is always
>> called prior "receive", then this check may be redundant. Again, there are
>>> other serial drivers that do this check as well (etraxfs, Xilinx,...)
>>> But most don't, so I'll follow the majority and remove it.
>>
>> Maybe you can check the core code or someone else can jump in an answer
>> this.
> 
> OK, will look deeper
> 
> [...] 
> 
>>>>> +#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 FIFO_LENGTH 64
>>>>
>>>> Should probably be a QOM property, no ?
>>>
>>> Did not want to mess with dynamic FIFO buffer allocation.
>>
>> You probably should, since this is configurable at the FPGA level (in
>> QSys), right ?
>>
> 
> OK, will implement fifo size as a property
> 
> 
> Thanks
> Juro
> 


-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Bystricky, Juro]

> >> [...]
> >>
> >>>>> +static uint64_t altera_juart_read(void *opaque, hwaddr addr,
> unsigned
> >>>> int size)
> >>>>> +{
> >>>>> +    AlteraJUARTState *s = opaque;
> >>>>> +    uint32_t r;
> >>>>> +
> >>>>> +    addr >>= 2;
> >>>>
> >>>> Hmmmmm, how will unaligned read from one of these registers be handled
> >>>> on real HW ? ie. read from address 0x3 ? What about writes ?
> >>>>
> >>>
> >>> there is no reading/writing going on via "addr".
> >>> This just maps the hw address into register number, where registers are
> >> at
> >>> 4 bytes boundaries (so they are aligned as needed) but indexed as
> >> 1,2,3....
> >>> (Pretty common code in other drivers.)
> >>> But will redo the code anyway so there are no shifts.
> >>
> >> This doesn't answer my question at all. How does real hardware behave if
> >> you read from unaligned address in the register space , ie. offset 0x3 ?
> >>
> >
> > Not sure I understand the question here. Which "real hardware" are we
> talking about?
> 
> By real hardware I mean real Nios2 system ...
> 
> > If "real hardware" contains MMU or MPU then an exception is generated on
> misalign access.
> 
> Is this handled here or not ?
> 

Sorry, I am not sure I understand the question.
Exceptions are handled by interrupt controller.
This code for JTAG UART does not have any misaligned accesses.

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Marek Vasut]

On 02/09/2017 05:07 PM, Bystricky, Juro wrote:
>>>> [...]
>>>>
>>>>>>> +static uint64_t altera_juart_read(void *opaque, hwaddr addr,
>> unsigned
>>>>>> int size)
>>>>>>> +{
>>>>>>> +    AlteraJUARTState *s = opaque;
>>>>>>> +    uint32_t r;
>>>>>>> +
>>>>>>> +    addr >>= 2;
>>>>>>
>>>>>> Hmmmmm, how will unaligned read from one of these registers be handled
>>>>>> on real HW ? ie. read from address 0x3 ? What about writes ?
>>>>>>
>>>>>
>>>>> there is no reading/writing going on via "addr".
>>>>> This just maps the hw address into register number, where registers are
>>>> at
>>>>> 4 bytes boundaries (so they are aligned as needed) but indexed as
>>>> 1,2,3....
>>>>> (Pretty common code in other drivers.)
>>>>> But will redo the code anyway so there are no shifts.
>>>>
>>>> This doesn't answer my question at all. How does real hardware behave if
>>>> you read from unaligned address in the register space , ie. offset 0x3 ?
>>>>
>>>
>>> Not sure I understand the question here. Which "real hardware" are we
>> talking about?
>>
>> By real hardware I mean real Nios2 system ...
>>
>>> If "real hardware" contains MMU or MPU then an exception is generated on
>> misalign access.
>>
>> Is this handled here or not ?
>>
> 
> Sorry, I am not sure I understand the question.
> Exceptions are handled by interrupt controller.
> This code for JTAG UART does not have any misaligned accesses.
> 
> 
The code running on nios2 can issue unaligned access to the jtag uart
registers, yes ? If that happens, what happens on real HW and how is
this emulated ?

-- 
Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Bystricky, Juro]

> The code running on nios2 can issue unaligned access to the jtag uart
> registers, yes ? If that happens, what happens on real HW and how is
> this emulated ?

Misaligned accesses to any registers would raise an exception.
The exception is handled by interrupt controller, either "real"
or emulated. Such access should never reach code for jtag uart.

Also note we only allow access size 4 in the code:

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
    }
};

Thanks
Juro

Re: [Qemu-devel] [PATCH v3 1/1] nios2: Add Altera JTAG UART emulation

[Marek Vasut]

On 02/09/2017 05:34 PM, Bystricky, Juro wrote:
>> The code running on nios2 can issue unaligned access to the jtag uart
>> registers, yes ? If that happens, what happens on real HW and how is
>> this emulated ?
> 
> Misaligned accesses to any registers would raise an exception.
> The exception is handled by interrupt controller, either "real"
> or emulated. Such access should never reach code for jtag uart.
> 
> Also note we only allow access size 4 in the code:
> 
> 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
>     }
> };

This answers my question , thanks.

-- 
Best regards,
Marek Vasut