[RFC PATCH 00/10] hw/avr: Introduce the Arduino board

[RFC PATCH 00/10] hw/avr: Introduce the Arduino board

[Philippe Mathieu-Daudé]

Hi Michael,

I complained I'd rather have QEMU model real hardware, with
documentation (schematics).
Since your series is almost ready to get merged, I prefered to
spend some time now to write down what I wanted. This is mostly
a rewrite of your board, but matching the Arduino boards.

Some bug slipped in (uart interrupt not raised) but I'm too tired
to find it, and since I won't have time to look at it the next
days, I prefer to send this now.

The first part of the series are quick review notes, which you
should squash in your previous patches.

I still have in my TODO before merge:
- Fix the USART IRQ bug
- Split "Add limited support for USART and 16 bit timer peripherals"
  in 3 patches: USART/Timer16/INTC

And TODO after merge is:
- Extract Timer8 common parts from Timer16
- Add GPIOs
- Connect LED to GPIO on Arduino

Thank you for having insisted with this during so long!

Regards,

Phil.

Based-on: <20191127175257.23480-1-mrolnik@gmail.com>
https://www.mail-archive.com/qemu-devel@nongnu.org/msg661553.html

Philippe Mathieu-Daudé (10):
  hw/avr: Kludge to fix build failure
  target/avr: Remove unused include
  target/avr: Add missing definitions
  target/avr: Fix IRQ count
  hw/char/avr: Reduce USART I/O size
  hw/avr: Add ATmega microcontrollers
  hw/avr: Add few Arduino boards
  tests/acceptance: Keep multilines comment consistent with other tests
  tests/acceptance: Use the ATmega2560 board
  hw/avr: Remove the 'sample' board

 hw/avr/atmega.h                  |  58 +++++
 include/hw/char/avr_usart.h      |   2 +
 target/avr/cpu.h                 |   2 +
 hw/avr/arduino.c                 | 173 ++++++++++++++
 hw/avr/atmega.c                  | 379 +++++++++++++++++++++++++++++++
 hw/avr/sample.c                  | 282 -----------------------
 hw/char/avr_usart.c              |   2 +-
 target/avr/cpu.c                 |   2 +-
 target/avr/helper.c              |   1 -
 hw/avr/Makefile.objs             |   3 +-
 tests/acceptance/machine_avr6.py |  10 +-
 11 files changed, 623 insertions(+), 291 deletions(-)
 create mode 100644 hw/avr/atmega.h
 create mode 100644 hw/avr/arduino.c
 create mode 100644 hw/avr/atmega.c
 delete mode 100644 hw/avr/sample.c

-- 
2.21.0

[NOTFORMERGE PATCH 01/10] hw/avr: Kludge to fix build failure

[Philippe Mathieu-Daudé]

Incomplete rename between Michael v36/v37

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/avr_usart.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/char/avr_usart.h b/include/hw/char/avr_usart.h
index 8e9ee88bbd..87f3f54cf8 100644
--- a/include/hw/char/avr_usart.h
+++ b/include/hw/char/avr_usart.h
@@ -94,4 +94,6 @@ typedef struct {
     qemu_irq dre_irq;
 } AVRUsartState;
 
+typedef AVRUsartState AvrUsartState;
+
 #endif /* HW_AVR_USART_H */
-- 
2.21.0

[PATCH 02/10] target/avr: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index f1939bd5a7..75946209b8 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -21,7 +21,6 @@
 #include "qemu/osdep.h"
 
 #include "cpu.h"
-#include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "exec/exec-all.h"
-- 
2.21.0

[PATCH 03/10] target/avr: Add missing definitions

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index a3e615a1eb..c1448a865f 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -26,6 +26,8 @@
 
 #define TCG_GUEST_DEFAULT_MO 0
 
+#define AVR_CPU_TYPE_SUFFIX "-" TYPE_AVR_CPU
+#define AVR_CPU_TYPE_NAME(name) (name AVR_CPU_TYPE_SUFFIX)
 #define CPU_RESOLVING_TYPE TYPE_AVR_CPU
 
 /*
-- 
2.21.0

[NOTFORMERGE PATCH 04/10] target/avr: Fix IRQ count

[Philippe Mathieu-Daudé]

Fix a SEGV when using IRQ#57 by expanding to 64 IRQs.
64 is a magic number.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 52ec21dd16..8198f9d49c 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -129,7 +129,7 @@ static void avr_cpu_initfn(Object *obj)
 
 #ifndef CONFIG_USER_ONLY
     /* Set the number of interrupts supported by the CPU. */
-    qdev_init_gpio_in(DEVICE(cpu), avr_cpu_set_int, 57);
+    qdev_init_gpio_in(DEVICE(cpu), avr_cpu_set_int, 64);
 #endif
 }
 
-- 
2.21.0

[RFC PATCH 05/10] hw/char/avr: Reduce USART I/O size

[Philippe Mathieu-Daudé]

Per the datasheet the USART uses 6 consecutive 8-bit registers.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/avr_usart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index d039689c56..ba17bdd470 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -284,7 +284,7 @@ static void avr_usart_init(Object *obj)
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rxc_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->dre_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->txc_irq);
-    memory_region_init_io(&s->mmio, obj, &avr_usart_ops, s, TYPE_AVR_USART, 8);
+    memory_region_init_io(&s->mmio, obj, &avr_usart_ops, s, TYPE_AVR_USART, 6);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
     qdev_init_gpio_in(DEVICE(s), avr_usart_pr, 1);
     s->enabled = true;
-- 
2.21.0

[RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

Add famous ATmega MCUs:

- middle range: ATmega168 and ATmega328
- high range: ATmega1280 and ATmega2560

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/atmega.h      |  58 +++++++
 hw/avr/atmega.c      | 379 +++++++++++++++++++++++++++++++++++++++++++
 hw/avr/Makefile.objs |   1 +
 3 files changed, 438 insertions(+)
 create mode 100644 hw/avr/atmega.h
 create mode 100644 hw/avr/atmega.c

diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
new file mode 100644
index 0000000000..d22d90a962
--- /dev/null
+++ b/hw/avr/atmega.h
@@ -0,0 +1,58 @@
+/*
+ * QEMU ATmega MCU
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_AVR_ATMEGA_H
+#define HW_AVR_ATMEGA_H
+
+#include "hw/char/avr_usart.h"
+#include "hw/char/avr_usart.h"
+#include "hw/timer/avr_timer16.h"
+#include "hw/misc/avr_mask.h"
+#include "target/avr/cpu.h"
+
+#define TYPE_ATMEGA     "ATmega"
+#define TYPE_ATMEGA168  "ATmega168"
+#define TYPE_ATMEGA328  "ATmega328"
+#define TYPE_ATMEGA1280 "ATmega1280"
+#define TYPE_ATMEGA2560 "ATmega2560"
+#define ATMEGA(obj)     OBJECT_CHECK(AtmegaState, (obj), TYPE_ATMEGA)
+
+#define USART_MAX 4
+#define TIMER_MAX 6
+
+typedef struct AtmegaState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    AVRCPU cpu;
+    MemoryRegion flash;
+    MemoryRegion eeprom;
+    MemoryRegion sram;
+    DeviceState *io;
+    AVRMaskState pwr[2];
+    AVRUsartState usart[USART_MAX];
+    AVRTimer16State timer[TIMER_MAX];
+    uint64_t xtal_freq_hz;
+} AtmegaState;
+
+typedef struct AtmegaInfo AtmegaInfo;
+
+typedef struct AtmegaClass {
+    SysBusDeviceClass parent_class;
+    const AtmegaInfo *info;
+} AtmegaClass;
+
+#define ATMEGA_CLASS(klass) \
+    OBJECT_CLASS_CHECK(AtmegaClass, (klass), TYPE_ATMEGA)
+#define ATMEGA_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(AtmegaClass, (obj), TYPE_ATMEGA)
+
+#endif /* HW_AVR_ATMEGA_H */
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
new file mode 100644
index 0000000000..1f1b1246ef
--- /dev/null
+++ b/hw/avr/atmega.c
@@ -0,0 +1,379 @@
+/*
+ * QEMU ATmega MCU
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
+#include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for sram */
+#include "hw/misc/unimp.h"
+#include "atmega.h"
+
+enum AtmegaIrq {
+    USART0_RXC_IRQ, USART0_DRE_IRQ, USART0_TXC_IRQ,
+    USART1_RXC_IRQ, USART1_DRE_IRQ, USART1_TXC_IRQ,
+    USART2_RXC_IRQ, USART2_DRE_IRQ, USART2_TXC_IRQ,
+    USART3_RXC_IRQ, USART3_DRE_IRQ, USART3_TXC_IRQ,
+    TIMER0_CAPT_IRQ, TIMER0_COMPA_IRQ, TIMER0_COMPB_IRQ,
+        TIMER0_COMPC_IRQ, TIMER0_OVF_IRQ,
+    TIMER1_CAPT_IRQ, TIMER1_COMPA_IRQ, TIMER1_COMPB_IRQ,
+        TIMER1_COMPC_IRQ, TIMER1_OVF_IRQ,
+    TIMER2_CAPT_IRQ, TIMER2_COMPA_IRQ, TIMER2_COMPB_IRQ,
+        TIMER2_COMPC_IRQ, TIMER2_OVF_IRQ,
+    TIMER3_CAPT_IRQ, TIMER3_COMPA_IRQ, TIMER3_COMPB_IRQ,
+        TIMER3_COMPC_IRQ, TIMER3_OVF_IRQ,
+    TIMER4_CAPT_IRQ, TIMER4_COMPA_IRQ, TIMER4_COMPB_IRQ,
+        TIMER4_COMPC_IRQ, TIMER4_OVF_IRQ,
+    TIMER5_CAPT_IRQ, TIMER5_COMPA_IRQ, TIMER5_COMPB_IRQ,
+        TIMER5_COMPC_IRQ, TIMER5_OVF_IRQ,
+};
+#define IRQ_MAX             64
+
+#define USART_RXC_IRQ(n)    (3 * n + USART0_RXC_IRQ)
+#define USART_DRE_IRQ(n)    (3 * n + USART0_DRE_IRQ)
+#define USART_TXC_IRQ(n)    (3 * n + USART0_TXC_IRQ)
+#define TIMER_CAPT_IRQ(n)   (5 * n + TIMER0_CAPT_IRQ)
+#define TIMER_COMPA_IRQ(n)  (5 * n + TIMER0_COMPA_IRQ)
+#define TIMER_COMPB_IRQ(n)  (5 * n + TIMER0_COMPB_IRQ)
+#define TIMER_COMPC_IRQ(n)  (5 * n + TIMER0_COMPC_IRQ)
+#define TIMER_OVF_IRQ(n)    (5 * n + TIMER0_OVF_IRQ)
+
+static const uint8_t irq168_328[IRQ_MAX] = {
+    [TIMER2_COMPA_IRQ]      = 8,
+    [TIMER2_COMPB_IRQ]      = 9,
+    [TIMER2_OVF_IRQ]        = 10,
+    [TIMER1_CAPT_IRQ]       = 11,
+    [TIMER1_COMPA_IRQ]      = 12,
+    [TIMER1_COMPB_IRQ]      = 13,
+    [TIMER1_OVF_IRQ]        = 14,
+    [TIMER0_COMPA_IRQ]      = 15,
+    [TIMER0_COMPB_IRQ]      = 16,
+    [TIMER0_OVF_IRQ]        = 17,
+    [USART0_RXC_IRQ]        = 19,
+    [USART0_DRE_IRQ]        = 20,
+    [USART0_TXC_IRQ]        = 21,
+}, irq1280_2560[IRQ_MAX] = {
+    [TIMER2_COMPA_IRQ]      = 14,
+    [TIMER2_COMPB_IRQ]      = 15,
+    [TIMER2_OVF_IRQ]        = 16,
+    [TIMER1_CAPT_IRQ]       = 17,
+    [TIMER1_COMPA_IRQ]      = 18,
+    [TIMER1_COMPB_IRQ]      = 19,
+    [TIMER1_COMPC_IRQ]      = 20,
+    [TIMER1_OVF_IRQ]        = 21,
+    [TIMER0_COMPA_IRQ]      = 22,
+    [TIMER0_COMPB_IRQ]      = 23,
+    [TIMER0_OVF_IRQ]        = 24,
+    [USART0_RXC_IRQ]        = 26,
+    [USART0_DRE_IRQ]        = 27,
+    [USART0_TXC_IRQ]        = 28,
+    [TIMER3_CAPT_IRQ]       = 32,
+    [TIMER3_COMPA_IRQ]      = 33,
+    [TIMER3_COMPB_IRQ]      = 34,
+    [TIMER3_COMPC_IRQ]      = 35,
+    [TIMER3_OVF_IRQ]        = 36,
+    [USART1_RXC_IRQ]        = 37,
+    [USART1_DRE_IRQ]        = 38,
+    [USART1_TXC_IRQ]        = 39,
+    [USART2_RXC_IRQ]        = 52,
+    [USART2_DRE_IRQ]        = 53,
+    [USART2_TXC_IRQ]        = 54,
+    [USART3_RXC_IRQ]        = 55,
+    [USART3_DRE_IRQ]        = 56,
+    [USART3_TXC_IRQ]        = 57,
+};
+
+enum AtmegaPeripheralAddress {
+    USART0, USART1, USART2, USART3,
+    TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
+    DEV_MAX
+};
+
+#define USART_ADDR(n)       (n + USART0)
+#define TIMER_ADDR(n)       (n + TIMER0)
+
+typedef struct {
+    uint16_t addr;
+    uint16_t prr_addr;
+    uint8_t prr_bit;
+    /* timer specific */
+    uint16_t intmask_addr;
+    uint16_t intflag_addr;
+    bool is_timer16;
+} peripheral_cfg;
+
+static const peripheral_cfg dev168_328[DEV_MAX] = {
+    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
+    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
+    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
+    [USART0]        = {  0xc0, 0x64, 1 },
+}, dev1280_2560[DEV_MAX] = {
+    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
+    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
+    [TIMER3]        = {  0x90, 0x65, 3, 0x71, 0x38, true },
+    [TIMER4]        = {  0xa0, 0x65, 4, 0x72, 0x39, true },
+    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
+    [USART0]        = {  0xc0, 0x64, 1 },
+    [USART1]        = {  0xc8, 0x65, 0 },
+    [USART2]        = {  0xd0, 0x65, 1 },
+    [TIMER5]        = { 0x120, 0x65, 5, 0x73, 0x3a, true },
+    [USART3]        = { 0x130, 0x65, 2 },
+};
+
+struct AtmegaInfo {
+    const char *uc_name;
+    const char *cpu_type;
+    size_t flash_size;
+    size_t eeprom_size;
+    size_t sram_size;
+    size_t io_size;
+    size_t uart_count;
+    size_t timer_count;
+    size_t gpio_count;
+    size_t adc_count;
+    const uint8_t *irq;
+    const peripheral_cfg *dev;
+};
+
+static const AtmegaInfo atmega_mcu[] = {
+    {
+        .uc_name = TYPE_ATMEGA168,
+        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
+        .flash_size = 16 * KiB,
+        .eeprom_size = 512,
+        .sram_size = 1 * KiB,
+        .io_size = 256,
+        .uart_count = 1,
+        .gpio_count = 23,
+        .adc_count = 6,
+        .irq = irq168_328,
+        .dev = dev168_328,
+    },
+    {
+        .uc_name = TYPE_ATMEGA328,
+        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
+        .flash_size = 32 * KiB,
+        .eeprom_size = 1 * KiB,
+        .sram_size = 2 * KiB,
+        .io_size = 256,
+        .uart_count = 1,
+        .timer_count = 3,
+        .gpio_count = 23,
+        .adc_count = 6,
+        .irq = irq168_328,
+        .dev = dev168_328,
+    },
+    {
+        .uc_name = TYPE_ATMEGA1280,
+        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
+        .flash_size = 128 * KiB,
+        .eeprom_size = 4 * KiB,
+        .sram_size = 8 * KiB,
+        .io_size = 512,
+        .uart_count = 4,
+        .timer_count = 6,
+        .gpio_count = 86,
+        .adc_count = 16,
+        .irq = irq1280_2560,
+        .dev = dev1280_2560,
+    },
+    {
+        .uc_name = TYPE_ATMEGA2560,
+        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
+        .flash_size = 256 * KiB,
+        .eeprom_size = 4 * KiB,
+        .sram_size = 8 * KiB,
+        .io_size = 512,
+        .uart_count = 4,
+        .timer_count = 6,
+        .gpio_count = 54,
+        .adc_count = 16,
+        .irq = irq1280_2560,
+        .dev = dev1280_2560,
+    },
+};
+
+static void connect_nonnull_irq(SysBusDevice *sbd, DeviceState *dev,
+                                int n, int irq)
+{
+    if (irq) {
+        sysbus_connect_irq(sbd, n, qdev_get_gpio_in(dev, irq));
+    }
+}
+
+static void connect_pr_irq(AtmegaState *s, const AtmegaInfo *info,
+                           DeviceState *dev, int index)
+{
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pwr[info->dev[index].prr_addr & 1]),
+                       info->dev[index].prr_bit,
+                       qdev_get_gpio_in(dev, 0));
+}
+
+static void atmega_realize(DeviceState *dev, Error **errp)
+{
+    AtmegaState *s = ATMEGA(dev);
+    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
+    const AtmegaInfo *info = bc->info;
+    DeviceState *cpudev;
+    SysBusDevice *sbd;
+    Error *err = NULL;
+    char *devname;
+    size_t i;
+
+    if (!s->xtal_freq_hz) {
+        error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
+        return;
+    }
+
+    /* CPU */
+    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
+                            info->cpu_type, &err, NULL);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &error_abort);
+    cpudev = DEVICE(&s->cpu);
+
+    /* SRAM */
+    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
+                                         "sram", info->sram_size);
+    memory_region_add_subregion(get_system_memory(),
+                                OFFSET_DATA + 0x200, &s->sram);
+
+    /* Flash */
+    memory_region_init_rom(&s->flash, OBJECT(dev),
+                           "flash", info->flash_size, &error_fatal);
+    memory_region_add_subregion(get_system_memory(), OFFSET_CODE, &s->flash);
+
+    /* I/O */
+    s->io = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
+    qdev_prop_set_string(s->io, "name", "I/O");
+    qdev_prop_set_uint64(s->io, "size", info->io_size);
+    qdev_init_nofail(s->io);
+    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA, -1234);
+
+    /* Power */
+    for (i = 0; i < ARRAY_SIZE(s->pwr); i++) {
+        devname = g_strdup_printf("pwr%zu", i);
+        object_initialize_child(OBJECT(dev), devname,
+                                &s->pwr[i], sizeof(s->pwr[i]),
+                                TYPE_AVR_MASK, &error_abort, NULL);
+        object_property_set_bool(OBJECT(&s->pwr[i]), true, "realized",
+                                 &error_abort);
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0, OFFSET_DATA + 0x64 + i);
+        g_free(devname);
+    }
+
+    /* USART */
+    for (i = 0; i < info->uart_count; i++) {
+        devname = g_strdup_printf("usart%zu", i);
+        object_initialize_child(OBJECT(dev), devname,
+                                &s->usart[i], sizeof(s->usart[i]),
+                                TYPE_AVR_USART, &error_abort, NULL);
+        qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
+        object_property_set_bool(OBJECT(&s->usart[i]), true, "realized",
+                                 &error_abort);
+        sbd = SYS_BUS_DEVICE(&s->usart[i]);
+        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[USART_ADDR(i)].addr);
+        connect_nonnull_irq(sbd, cpudev, 0, info->irq[USART_RXC_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 1, info->irq[USART_DRE_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 2, info->irq[USART_TXC_IRQ(i)]);
+        connect_pr_irq(s, info, DEVICE(&s->usart[i]), 0);
+        g_free(devname);
+    }
+
+    /* Timer */
+    for (i = 0; i < info->timer_count; i++) {
+        int idx = TIMER_ADDR(i);
+        if (!info->dev[idx].is_timer16) {
+            create_unimplemented_device("avr-timer8",
+                                        OFFSET_DATA + info->dev[idx].addr, 7);
+            create_unimplemented_device("avr-timer8-intmask",
+                                        OFFSET_DATA
+                                        + info->dev[idx].intmask_addr, 1);
+            create_unimplemented_device("avr-timer8-intflag",
+                                        OFFSET_DATA
+                                        + info->dev[idx].intflag_addr, 1);
+            continue;
+        }
+        devname = g_strdup_printf("timer%zu", i);
+        object_initialize_child(OBJECT(dev), devname,
+                                &s->timer[i], sizeof(s->timer[i]),
+                                TYPE_AVR_TIMER16, &error_abort, NULL);
+        object_property_set_uint(OBJECT(&s->timer[i]), s->xtal_freq_hz,
+                                 "cpu-frequency-hz", &error_abort);
+        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized",
+                                 &error_abort);
+        sbd = SYS_BUS_DEVICE(&s->timer[i]);
+        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[idx].addr);
+        sysbus_mmio_map(sbd, 1, OFFSET_DATA + info->dev[idx].intmask_addr);
+        sysbus_mmio_map(sbd, 2, OFFSET_DATA + info->dev[idx].intflag_addr);
+        connect_nonnull_irq(sbd, cpudev, 0, info->irq[TIMER_CAPT_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 1, info->irq[TIMER_COMPA_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 2, info->irq[TIMER_COMPB_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 3, info->irq[TIMER_COMPC_IRQ(i)]);
+        connect_nonnull_irq(sbd, cpudev, 4, info->irq[TIMER_OVF_IRQ(i)]);
+        connect_pr_irq(s, info, DEVICE(&s->timer[i]), 0);
+        g_free(devname);
+    }
+}
+
+static Property atmega_props[] = {
+    DEFINE_PROP_UINT64("xtal-frequency-hz", AtmegaState,
+                       xtal_freq_hz, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void atmega_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    AtmegaClass *bc = ATMEGA_CLASS(oc);
+
+    bc->info = data;
+    dc->realize = atmega_realize;
+    dc->props = atmega_props;
+    /* Reason: Mapped at fixed location on the system bus */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo atmega_type_info = {
+    .name = TYPE_ATMEGA,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AtmegaState),
+    .class_size = sizeof(AtmegaClass),
+    .abstract = true,
+};
+
+static void atmega_register_types(void)
+{
+    size_t i;
+
+    type_register_static(&atmega_type_info);
+    for (i = 0; i < ARRAY_SIZE(atmega_mcu); i++) {
+        assert(atmega_mcu[i].io_size <= 0x200);
+        assert(atmega_mcu[i].uart_count <= USART_MAX);
+        assert(atmega_mcu[i].timer_count <= TIMER_MAX);
+        TypeInfo ti = {
+            .name = atmega_mcu[i].uc_name,
+            .parent = TYPE_ATMEGA,
+            .class_init = atmega_class_init,
+            .class_data = (void *) &atmega_mcu[i],
+        };
+        type_register(&ti);
+    }
+}
+
+type_init(atmega_register_types)
diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
index 626b7064b3..4b6b911820 100644
--- a/hw/avr/Makefile.objs
+++ b/hw/avr/Makefile.objs
@@ -1 +1,2 @@
 obj-y += sample.o
+obj-y += atmega.o
-- 
2.21.0

[RFC PATCH 07/10] hw/avr: Add few Arduino boards

[Philippe Mathieu-Daudé]

Add famous Arduino boards:

- Arduino Duemilanove
- Arduino Mega

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/arduino.c     | 173 +++++++++++++++++++++++++++++++++++++++++++
 hw/avr/Makefile.objs |   1 +
 2 files changed, 174 insertions(+)
 create mode 100644 hw/avr/arduino.c

diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
new file mode 100644
index 0000000000..191193d614
--- /dev/null
+++ b/hw/avr/arduino.c
@@ -0,0 +1,173 @@
+/*
+ * QEMU Arduino boards
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+/* TODO: Implement the use of EXTRAM */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "hw/boards.h"
+#include "hw/loader.h"
+#include "elf.h"
+#include "atmega.h"
+
+typedef struct ArduinoBoardConfig {
+    const char *name;
+    const char *desc;
+    const char *alias;
+    const char *mcu_type;
+    uint64_t xtal_hz;
+    size_t extram_size;
+    bool is_default;
+} ArduinoBoardConfig;
+
+static const ArduinoBoardConfig arduino_boards[] = {
+    {
+        /* https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove */
+        .name       = MACHINE_TYPE_NAME("arduino-duemilanove"),
+        .desc       = "Arduino Duemilanove (ATmega168)",
+        .alias      = "2009",
+        .mcu_type    = TYPE_ATMEGA168,
+        .xtal_hz    = 16 * 1000 * 1000,
+    }, {
+        /* https://store.arduino.cc/arduino-uno-rev3 */
+        .name       = MACHINE_TYPE_NAME("arduino-uno"),
+        .desc       = "Arduino Duemilanove (ATmega328P)",
+        .alias      = "UNO",
+        .mcu_type    = TYPE_ATMEGA328,
+        .xtal_hz    = 16 * 1000 * 1000,
+    }, {
+        /* https://www.arduino.cc/en/Main/ArduinoBoardMega */
+        .name       = MACHINE_TYPE_NAME("arduino-mega"),
+        .desc       = "Arduino Mega (ATmega1280)",
+        .alias      = "MEGA",
+        .mcu_type    = TYPE_ATMEGA1280,
+        .xtal_hz    = 16 * 1000 * 1000,
+    }, {
+        /* https://store.arduino.cc/arduino-mega-2560-rev3 */
+        .name       = MACHINE_TYPE_NAME("arduino-mega-2560-v3"),
+        .desc       = "Arduino Mega 2560 (ATmega2560)",
+        .alias      = "MEGA2560",
+        .mcu_type    = TYPE_ATMEGA2560,
+        .xtal_hz    = 16 * 1000 * 1000, /* CSTCE16M0V53-R0 */
+        .is_default = true,
+    },
+};
+
+typedef struct ArduinoMachineState {
+    /*< private >*/
+    MachineState parent_obj;
+    /*< public >*/
+    AtmegaState mcu;
+    MemoryRegion extram;
+} ArduinoMachineState;
+
+typedef struct ArduinoMachineClass {
+    /*< private >*/
+    MachineClass parent_class;
+    /*< public >*/
+    const ArduinoBoardConfig *config;
+} ArduinoMachineClass;
+
+#define TYPE_ARDUINO_MACHINE \
+        MACHINE_TYPE_NAME("arduino")
+#define ARDUINO_MACHINE(obj) \
+        OBJECT_CHECK(ArduinoMachineState, (obj), TYPE_ARDUINO_MACHINE)
+#define ARDUINO_MACHINE_CLASS(klass) \
+        OBJECT_CLASS_CHECK(ArduinoMachineClass, (klass), TYPE_ARDUINO_MACHINE)
+#define ARDUINO_MACHINE_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(ArduinoMachineClass, (obj), TYPE_ARDUINO_MACHINE)
+
+static void load_firmware(const char *firmware, uint64_t flash_size)
+{
+    const char *filename;
+    int bytes_loaded;
+
+    /* Load firmware (contents of flash) trying to auto-detect format */
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
+    if (filename == NULL) {
+        error_report("Unable to find %s", firmware);
+        exit(1);
+    }
+
+    bytes_loaded = load_elf(filename, NULL, NULL, NULL, NULL, NULL, NULL,
+                            0, EM_NONE, 0, 0);
+    if (bytes_loaded < 0) {
+        bytes_loaded = load_image_targphys(filename, OFFSET_CODE, flash_size);
+    }
+    if (bytes_loaded < 0) {
+        error_report("Unable to load firmware image %s as ELF or raw binary",
+                     firmware);
+        exit(1);
+    }
+}
+
+static void arduino_machine_init(MachineState *machine)
+{
+    ArduinoMachineClass *amc = ARDUINO_MACHINE_GET_CLASS(machine);
+    ArduinoMachineState *ams = ARDUINO_MACHINE(machine);
+
+    sysbus_init_child_obj(OBJECT(machine), "mcu", &ams->mcu, sizeof(ams->mcu),
+                          amc->config->mcu_type);
+    object_property_set_uint(OBJECT(&ams->mcu), amc->config->xtal_hz,
+                             "xtal-frequency-hz", &error_abort);
+    object_property_set_bool(OBJECT(&ams->mcu), true, "realized",
+                             &error_abort);
+
+    if (machine->firmware) {
+        load_firmware(machine->firmware, memory_region_size(&ams->mcu.flash));
+    }
+}
+
+static void arduino_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
+    const ArduinoBoardConfig *cfg = data;
+
+    mc->desc = cfg->desc;
+    mc->alias = cfg->alias;
+    mc->init = arduino_machine_init;
+    mc->default_cpus = 1;
+    mc->min_cpus = mc->default_cpus;
+    mc->max_cpus = mc->default_cpus;
+    mc->no_floppy = 1;
+    mc->no_cdrom = 1;
+    mc->no_parallel = 1;
+    mc->is_default = cfg->is_default;
+    mc->default_ram_size = cfg->extram_size;
+    amc->config = cfg;
+}
+
+static const TypeInfo arduino_machine_type = {
+    .name = TYPE_ARDUINO_MACHINE,
+    .parent = TYPE_MACHINE,
+    .instance_size = sizeof(ArduinoMachineState),
+    .class_size = sizeof(ArduinoMachineClass),
+    .abstract = true,
+};
+
+static void arduino_machine_types(void)
+{
+    size_t i;
+
+    type_register_static(&arduino_machine_type);
+    for (i = 0; i < ARRAY_SIZE(arduino_boards); ++i) {
+        TypeInfo ti = {
+            .name       = arduino_boards[i].name,
+            .parent     = TYPE_ARDUINO_MACHINE,
+            .class_init = arduino_machine_class_init,
+            .class_data = (void *)&arduino_boards[i],
+        };
+        type_register(&ti);
+    }
+}
+
+type_init(arduino_machine_types)
diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
index 4b6b911820..39ee3c32b2 100644
--- a/hw/avr/Makefile.objs
+++ b/hw/avr/Makefile.objs
@@ -1,2 +1,3 @@
 obj-y += sample.o
 obj-y += atmega.o
+obj-y += arduino.o
-- 
2.21.0

[PATCH 08/10] tests/acceptance: Keep multilines comment consistent with other tests

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/machine_avr6.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/acceptance/machine_avr6.py b/tests/acceptance/machine_avr6.py
index ba1f47dd70..2ef4a9ac2c 100644
--- a/tests/acceptance/machine_avr6.py
+++ b/tests/acceptance/machine_avr6.py
@@ -37,9 +37,9 @@ class AVR6Machine(Test):
         https://github.com/seharris/qemu-avr-tests/raw/master/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf
         constantly prints out 'ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWX'
         """
-        rom_url = 'https://github.com/seharris/qemu-avr-tests'
-        rom_sha1= '36c3e67b8755dcf37e06af6730ef5d477b8ed16d'
-        rom_url += '/raw/' + rom_sha1 + '/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf'
+        rom_url = ('https://github.com/seharris/qemu-avr-tests'
+                   '/raw/36c3e67b8755dcf/free-rtos/Demo'
+                   '/AVR_ATMega2560_GCC/demo.elf')
         rom_hash = '7eb521f511ca8f2622e0a3c5e8dd686efbb911d4'
         rom_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
 
-- 
2.21.0

[RFC PATCH 09/10] tests/acceptance: Use the ATmega2560 board

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/machine_avr6.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/machine_avr6.py b/tests/acceptance/machine_avr6.py
index 2ef4a9ac2c..abe8d45b65 100644
--- a/tests/acceptance/machine_avr6.py
+++ b/tests/acceptance/machine_avr6.py
@@ -31,7 +31,7 @@ class AVR6Machine(Test):
     def test_freertos(self):
         """
         :avocado: tags=arch:avr
-        :avocado: tags=machine:sample
+        :avocado: tags=machine:arduino-mega-2560-v3
         """
         """
         https://github.com/seharris/qemu-avr-tests/raw/master/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf
@@ -43,7 +43,7 @@ class AVR6Machine(Test):
         rom_hash = '7eb521f511ca8f2622e0a3c5e8dd686efbb911d4'
         rom_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
 
-        self.vm.set_machine('sample')
+        self.vm.set_machine('arduino-mega-2560-v3')
         self.vm.add_args('-bios', rom_path)
         self.vm.add_args('-nographic')
         self.vm.launch()
-- 
2.21.0

[NOTFORMERGE PATCH 10/10] hw/avr: Remove the 'sample' board

[Philippe Mathieu-Daudé]

We introduce real world boards with proper schematics,
let's remove this.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Patch meant to squash on 'target/avr: Add example board configuration'
---
 hw/avr/sample.c      | 282 -------------------------------------------
 hw/avr/Makefile.objs |   1 -
 2 files changed, 283 deletions(-)
 delete mode 100644 hw/avr/sample.c

diff --git a/hw/avr/sample.c b/hw/avr/sample.c
deleted file mode 100644
index 2295ec1b79..0000000000
--- a/hw/avr/sample.c
+++ /dev/null
@@ -1,282 +0,0 @@
-/*
- * QEMU AVR CPU
- *
- * Copyright (c) 2019 Michael Rolnik
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see
- * <http://www.gnu.org/licenses/lgpl-2.1.html>
- */
-
-/*
- *  NOTE:
- *      This is not a real AVR board, this is an example!
- *      The CPU is an approximation of an ATmega2560, but is missing various
- *      built-in peripherals.
- *
- *      This example board loads provided binary file into flash memory and
- *      executes it from 0x00000000 address in the code memory space.
- *
- *      Currently used for AVR CPU validation
- *
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "qemu-common.h"
-#include "cpu.h"
-#include "hw/hw.h"
-#include "sysemu/sysemu.h"
-#include "sysemu/qtest.h"
-#include "ui/console.h"
-#include "hw/boards.h"
-#include "hw/loader.h"
-#include "qemu/error-report.h"
-#include "exec/address-spaces.h"
-#include "include/hw/sysbus.h"
-#include "include/hw/char/avr_usart.h"
-#include "include/hw/timer/avr_timer16.h"
-#include "include/hw/misc/avr_mask.h"
-#include "elf.h"
-#include "hw/misc/unimp.h"
-
-#define SIZE_FLASH 0x00040000
-#define SIZE_SRAM 0x00002000
-/*
- * Size of additional "external" memory, as if the AVR were configured to use
- * an external RAM chip.
- * Note that the configuration registers that normally enable this feature are
- * unimplemented.
- */
-#define SIZE_EXMEM 0x00000000
-
-/* Offsets of peripherals in emulated memory space (i.e. not host addresses)  */
-#define PRR0_BASE 0x64
-#define PRR1_BASE 0x65
-#define USART_BASE 0xc0
-#define TIMER1_BASE 0x80
-#define TIMER1_IMSK_BASE 0x6f
-#define TIMER1_IFR_BASE 0x36
-
-/* Interrupt numbers used by peripherals */
-#define USART_RXC_IRQ 24
-#define USART_DRE_IRQ 25
-#define USART_TXC_IRQ 26
-
-#define TIMER1_CAPT_IRQ 15
-#define TIMER1_COMPA_IRQ 16
-#define TIMER1_COMPB_IRQ 17
-#define TIMER1_COMPC_IRQ 18
-#define TIMER1_OVF_IRQ 19
-
-/*  Power reduction     */
-#define PRR1_BIT_PRTIM5     0x05    /*  Timer/Counter5  */
-#define PRR1_BIT_PRTIM4     0x04    /*  Timer/Counter4  */
-#define PRR1_BIT_PRTIM3     0x03    /*  Timer/Counter3  */
-#define PRR1_BIT_PRUSART3   0x02    /*  USART3  */
-#define PRR1_BIT_PRUSART2   0x01    /*  USART2  */
-#define PRR1_BIT_PRUSART1   0x00    /*  USART1  */
-
-#define PRR0_BIT_PRTWI      0x06    /*  TWI */
-#define PRR0_BIT_PRTIM2     0x05    /*  Timer/Counter2  */
-#define PRR0_BIT_PRTIM0     0x04    /*  Timer/Counter0  */
-#define PRR0_BIT_PRTIM1     0x03    /*  Timer/Counter1  */
-#define PRR0_BIT_PRSPI      0x02    /*  Serial Peripheral Interface */
-#define PRR0_BIT_PRUSART0   0x01    /*  USART0  */
-#define PRR0_BIT_PRADC      0x00    /*  ADC */
-
-typedef struct {
-    MachineClass parent;
-} SampleMachineClass;
-
-typedef struct {
-    MachineState parent;
-    MemoryRegion *ram;
-    MemoryRegion *flash;
-    AVRUsartState *usart0;
-    AVRTimer16State *timer1;
-    AVRMaskState *prr[2];
-} SampleMachineState;
-
-#define TYPE_SAMPLE_MACHINE MACHINE_TYPE_NAME("sample")
-
-#define SAMPLE_MACHINE(obj) \
-    OBJECT_CHECK(SampleMachineState, obj, TYPE_SAMPLE_MACHINE)
-#define SAMPLE_MACHINE_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(SampleMachineClass, obj, TYPE_SAMPLE_MACHINE)
-#define SAMPLE_MACHINE_CLASS(klass) \
-    OBJECT_CLASS_CHECK(SampleMachineClass, klass, TYPE_SAMPLE_MACHINE)
-
-static void sample_init(MachineState *machine)
-{
-    SampleMachineState *sms = SAMPLE_MACHINE(machine);
-    MemoryRegion *system_memory = get_system_memory();
-    AVRCPU *cpu;
-    const char *firmware = NULL;
-    const char *filename;
-    int bytes_loaded;
-    SysBusDevice *busdev;
-    DeviceState *cpudev;
-
-    system_memory = get_system_memory();
-    sms->ram = g_new(MemoryRegion, 1);
-    sms->flash = g_new(MemoryRegion, 1);
-
-    cpu = AVR_CPU(cpu_create(machine->cpu_type));
-    cpudev = DEVICE(cpu);
-
-
-    memory_region_init_rom(sms->flash, NULL, "avr.flash", SIZE_FLASH,
-            &error_fatal);
-    memory_region_add_subregion(system_memory, OFFSET_CODE, sms->flash);
-
-    /* following are atmel2560 device */
-    create_unimplemented_device("usart 3", OFFSET_DATA + 0x0130, 0x0007);
-    create_unimplemented_device("timer-counter-16bit 5",
-            OFFSET_DATA + 0x0120, 0x000e);
-    create_unimplemented_device("gpio L", OFFSET_DATA + 0x0109, 0x0003);
-    create_unimplemented_device("gpio K", OFFSET_DATA + 0x0106, 0x0003);
-    create_unimplemented_device("gpio J", OFFSET_DATA + 0x0103, 0x0003);
-    create_unimplemented_device("gpio H", OFFSET_DATA + 0x0100, 0x0003);
-    create_unimplemented_device("usart 2", OFFSET_DATA + 0x00d0, 0x0007);
-    create_unimplemented_device("usart 1", OFFSET_DATA + 0x00c8, 0x0007);
-    create_unimplemented_device("usart 0", OFFSET_DATA + 0x00c0, 0x0007);
-    create_unimplemented_device("twi", OFFSET_DATA + 0x00b8, 0x0006);
-    create_unimplemented_device("timer-counter-async-8bit 2",
-            OFFSET_DATA + 0x00b0, 0x0007);
-    create_unimplemented_device("timer-counter-16bit 4",
-            OFFSET_DATA + 0x00a0, 0x000e);
-    create_unimplemented_device("timer-counter-16bit 3",
-            OFFSET_DATA + 0x0090, 0x000e);
-    create_unimplemented_device("timer-counter-16bit 1",
-            OFFSET_DATA + 0x0080, 0x000e);
-    create_unimplemented_device("ac / adc",
-            OFFSET_DATA + 0x0078, 0x0008);
-    create_unimplemented_device("ext-mem-iface",
-            OFFSET_DATA + 0x0074, 0x0002);
-    create_unimplemented_device("int-controller",
-            OFFSET_DATA + 0x0068, 0x000c);
-    create_unimplemented_device("sys",
-            OFFSET_DATA + 0x0060, 0x0007);
-    create_unimplemented_device("spi",
-            OFFSET_DATA + 0x004c, 0x0003);
-    create_unimplemented_device("ext-mem-iface",
-            OFFSET_DATA + 0x004a, 0x0002);
-    create_unimplemented_device("timer-counter-pwm-8bit 0",
-            OFFSET_DATA + 0x0043, 0x0006);
-    create_unimplemented_device("ext-mem-iface",
-            OFFSET_DATA + 0x003e, 0x0005);
-    create_unimplemented_device("int-controller",
-            OFFSET_DATA + 0x0035, 0x0009);
-    create_unimplemented_device("gpio G", OFFSET_DATA + 0x0032, 0x0003);
-    create_unimplemented_device("gpio F", OFFSET_DATA + 0x002f, 0x0003);
-    create_unimplemented_device("gpio E", OFFSET_DATA + 0x002c, 0x0003);
-    create_unimplemented_device("gpio D", OFFSET_DATA + 0x0029, 0x0003);
-    create_unimplemented_device("gpio C", OFFSET_DATA + 0x0026, 0x0003);
-    create_unimplemented_device("gpio B", OFFSET_DATA + 0x0023, 0x0003);
-    create_unimplemented_device("gpio A", OFFSET_DATA + 0x0020, 0x0003);
-
-    memory_region_allocate_system_memory(
-        sms->ram, NULL, "avr.ram", SIZE_SRAM + SIZE_EXMEM);
-    memory_region_add_subregion(system_memory, OFFSET_DATA + 0x200, sms->ram);
-
-    /* Power Reduction built-in peripheral */
-    sms->prr[0] = AVR_MASK(sysbus_create_simple(TYPE_AVR_MASK,
-                    OFFSET_DATA + PRR0_BASE, NULL));
-    sms->prr[1] = AVR_MASK(sysbus_create_simple(TYPE_AVR_MASK,
-                    OFFSET_DATA + PRR1_BASE, NULL));
-
-    /* USART 0 built-in peripheral */
-    sms->usart0 = AVR_USART(object_new(TYPE_AVR_USART));
-    busdev = SYS_BUS_DEVICE(sms->usart0);
-    qdev_prop_set_chr(DEVICE(sms->usart0), "chardev", serial_hd(0));
-    object_property_set_bool(OBJECT(sms->usart0), true, "realized",
-            &error_fatal);
-    sysbus_mmio_map(busdev, 0, OFFSET_DATA + USART_BASE);
-    /*
-     * These IRQ numbers don't match the datasheet because we're counting from
-     * zero and not including reset.
-     */
-    sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(cpudev, USART_RXC_IRQ));
-    sysbus_connect_irq(busdev, 1, qdev_get_gpio_in(cpudev, USART_DRE_IRQ));
-    sysbus_connect_irq(busdev, 2, qdev_get_gpio_in(cpudev, USART_TXC_IRQ));
-    sysbus_connect_irq(SYS_BUS_DEVICE(sms->prr[1]), PRR1_BIT_PRUSART1,
-            qdev_get_gpio_in(DEVICE(sms->usart0), 0));
-
-    /* Timer 1 built-in periphal */
-    sms->timer1 = AVR_TIMER16(object_new(TYPE_AVR_TIMER16));
-    object_property_set_bool(OBJECT(sms->timer1), true, "realized",
-            &error_fatal);
-    busdev = SYS_BUS_DEVICE(sms->timer1);
-    sysbus_mmio_map(busdev, 0, OFFSET_DATA + TIMER1_BASE);
-    sysbus_mmio_map(busdev, 1, OFFSET_DATA + TIMER1_IMSK_BASE);
-    sysbus_mmio_map(busdev, 2, OFFSET_DATA + TIMER1_IFR_BASE);
-    sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(cpudev, TIMER1_CAPT_IRQ));
-    sysbus_connect_irq(busdev, 1, qdev_get_gpio_in(cpudev, TIMER1_COMPA_IRQ));
-    sysbus_connect_irq(busdev, 2, qdev_get_gpio_in(cpudev, TIMER1_COMPB_IRQ));
-    sysbus_connect_irq(busdev, 3, qdev_get_gpio_in(cpudev, TIMER1_COMPC_IRQ));
-    sysbus_connect_irq(busdev, 4, qdev_get_gpio_in(cpudev, TIMER1_OVF_IRQ));
-    sysbus_connect_irq(SYS_BUS_DEVICE(sms->prr[0]), PRR0_BIT_PRTIM1,
-            qdev_get_gpio_in(DEVICE(sms->timer1), 0));
-
-    /* Load firmware (contents of flash) trying to auto-detect format */
-    firmware = machine->firmware;
-    if (firmware != NULL) {
-        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
-        if (filename == NULL) {
-            error_report("Unable to find %s", firmware);
-            exit(1);
-        }
-
-        bytes_loaded = load_elf(
-            filename, NULL, NULL, NULL, NULL, NULL, NULL, 0, EM_NONE, 0, 0);
-        if (bytes_loaded < 0) {
-            bytes_loaded = load_image_targphys(
-                filename, OFFSET_CODE, SIZE_FLASH);
-        }
-        if (bytes_loaded < 0) {
-            error_report(
-                "Unable to load firmware image %s as ELF or raw binary",
-                firmware);
-            exit(1);
-        }
-    }
-}
-
-static void sample_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->desc = "AVR sample/example board (ATmega2560)";
-    mc->init = sample_init;
-    mc->default_cpus = 1;
-    mc->min_cpus = mc->default_cpus;
-    mc->max_cpus = mc->default_cpus;
-    mc->default_cpu_type = "avr6-avr-cpu"; /* ATmega2560. */
-    mc->is_default = 1;
-}
-
-static const TypeInfo sample_info = {
-    .name = TYPE_SAMPLE_MACHINE,
-    .parent = TYPE_MACHINE,
-    .instance_size = sizeof(SampleMachineState),
-    .class_size = sizeof(SampleMachineClass),
-    .class_init = sample_class_init,
-};
-
-static void sample_machine_init(void)
-{
-    type_register_static(&sample_info);
-}
-
-type_init(sample_machine_init);
diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
index 39ee3c32b2..f2de4a066d 100644
--- a/hw/avr/Makefile.objs
+++ b/hw/avr/Makefile.objs
@@ -1,3 +1,2 @@
-obj-y += sample.o
 obj-y += atmega.o
 obj-y += arduino.o
-- 
2.21.0

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

On Thu, Nov 28, 2019 at 2:50 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add famous ATmega MCUs:
>
> - middle range: ATmega168 and ATmega328
> - high range: ATmega1280 and ATmega2560
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/avr/atmega.h      |  58 +++++++
>  hw/avr/atmega.c      | 379 +++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/Makefile.objs |   1 +
>  3 files changed, 438 insertions(+)
>  create mode 100644 hw/avr/atmega.h
>  create mode 100644 hw/avr/atmega.c
>
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> new file mode 100644
> index 0000000000..d22d90a962
> --- /dev/null
> +++ b/hw/avr/atmega.h
> @@ -0,0 +1,58 @@
> +/*
> + * QEMU ATmega MCU
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_AVR_ATMEGA_H
> +#define HW_AVR_ATMEGA_H
> +
> +#include "hw/char/avr_usart.h"
> +#include "hw/char/avr_usart.h"

Oops duplicated header.

> +#include "hw/timer/avr_timer16.h"
> +#include "hw/misc/avr_mask.h"
> +#include "target/avr/cpu.h"
[...]

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 20275 bytes --]

On Thursday, November 28, 2019, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Add famous ATmega MCUs:
>
> - middle range: ATmega168 and ATmega328
> - high range: ATmega1280 and ATmega2560
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---


Philippe, hi.

Thank you for the impetus you give us all.

However, is this the right direction?

Let's analyse some bits and pieces.

Starting from the commit message, the word "famous" is used, but I really
don't see enumerated CPUs/MCUs are any special in Atmel lineup. Other than
we often used the doc describing them (cited several times in our
discussions) as our reference, but that doesn't make them "famous".
Ofcourse, there other docs for other Atmel CPUs/MCUs, of at lest equivalent
significance. For example, "tiny" ones are at least as famous as "mega"
ones.

Then, you introduce the term MCU, without proper discussion with others on
terminology. In parlance of QEMU, ATmega168 at al. are CPUs (we all know
and assume that that are some peripherals in it). I am not against using
the term MCU, but let's first sync up on that.

The added terminology trouble is that MCUs, as you defined them, have in
array atmega_mcu[] a field called "cpu_type" - why is that field not called
"mcu_type"? *Very* confusing for any future reader. And then, similar
terminology conundrum continues with macro AVR_CPU_TYPE_NAME().

All of the above is far less important than this question: What are we
achieving with proposed CPU/MCU definitions? I certainly see the value of
fitting the complex variety of AVR CPUs/MCUs into QEMU object model. No
question about that. However, is this the right moment to do it? There are
still some unresolved architectural problems with peripheral definitions,
as I noted in yhe comment to Michael's last cover letter. This patch does
not solve them. It just assumes everything is ready with peripherals, let's
build CPUs/MCUs. The machines based on proposed CPUs/MCUs are not more real
that machine based on Michael's "sample" machine. At least Michal says
"this is not a real machine". If we used proposed CPUs/MCUs from this
patch, the resulting machine is as "paper" machine as yhe "sample" machine.
We would just live in a la-la lend of thinking: "wow, we model real
hardware now".

I would rather accept into QEMU a series admitting we are still far from
modelling real machine or CPU/MCU, than a series that gives an illusion
that we are modelling real machine or CPU/MCU.

As far as utility of this patch for Michael's series, it looks to me they
add more headake than help (not saying that the help is not present) to
Michael. He would have anotter abstraction layer to think of, at the moment
he desperately needs (in my opinion) to focus on providing the as solid as
possible, and as complete as possinle foundations. This patch looks like
building castles in the air. Again, I am not claiming that the patch is not
helpful at all.

In summary, I think that this patch is premature.

Sincerely,
Aleksansar



>  hw/avr/atmega.h      |  58 +++++++
>  hw/avr/atmega.c      | 379 +++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/Makefile.objs |   1 +
>  3 files changed, 438 insertions(+)
>  create mode 100644 hw/avr/atmega.h
>  create mode 100644 hw/avr/atmega.c
>
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> new file mode 100644
> index 0000000000..d22d90a962
> --- /dev/null
> +++ b/hw/avr/atmega.h
> @@ -0,0 +1,58 @@
> +/*
> + * QEMU ATmega MCU
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_AVR_ATMEGA_H
> +#define HW_AVR_ATMEGA_H
> +
> +#include "hw/char/avr_usart.h"
> +#include "hw/char/avr_usart.h"
> +#include "hw/timer/avr_timer16.h"
> +#include "hw/misc/avr_mask.h"
> +#include "target/avr/cpu.h"
> +
> +#define TYPE_ATMEGA     "ATmega"
> +#define TYPE_ATMEGA168  "ATmega168"
> +#define TYPE_ATMEGA328  "ATmega328"
> +#define TYPE_ATMEGA1280 "ATmega1280"
> +#define TYPE_ATMEGA2560 "ATmega2560"
> +#define ATMEGA(obj)     OBJECT_CHECK(AtmegaState, (obj), TYPE_ATMEGA)
> +
> +#define USART_MAX 4
> +#define TIMER_MAX 6
> +
> +typedef struct AtmegaState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    AVRCPU cpu;
> +    MemoryRegion flash;
> +    MemoryRegion eeprom;
> +    MemoryRegion sram;
> +    DeviceState *io;
> +    AVRMaskState pwr[2];
> +    AVRUsartState usart[USART_MAX];
> +    AVRTimer16State timer[TIMER_MAX];
> +    uint64_t xtal_freq_hz;
> +} AtmegaState;
> +
> +typedef struct AtmegaInfo AtmegaInfo;
> +
> +typedef struct AtmegaClass {
> +    SysBusDeviceClass parent_class;
> +    const AtmegaInfo *info;
> +} AtmegaClass;
> +
> +#define ATMEGA_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(AtmegaClass, (klass), TYPE_ATMEGA)
> +#define ATMEGA_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(AtmegaClass, (obj), TYPE_ATMEGA)
> +
> +#endif /* HW_AVR_ATMEGA_H */
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> new file mode 100644
> index 0000000000..1f1b1246ef
> --- /dev/null
> +++ b/hw/avr/atmega.c
> @@ -0,0 +1,379 @@
> +/*
> + * QEMU ATmega MCU
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "qapi/error.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/sysbus.h"
> +#include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for
> sram */
> +#include "hw/misc/unimp.h"
> +#include "atmega.h"
> +
> +enum AtmegaIrq {
> +    USART0_RXC_IRQ, USART0_DRE_IRQ, USART0_TXC_IRQ,
> +    USART1_RXC_IRQ, USART1_DRE_IRQ, USART1_TXC_IRQ,
> +    USART2_RXC_IRQ, USART2_DRE_IRQ, USART2_TXC_IRQ,
> +    USART3_RXC_IRQ, USART3_DRE_IRQ, USART3_TXC_IRQ,
> +    TIMER0_CAPT_IRQ, TIMER0_COMPA_IRQ, TIMER0_COMPB_IRQ,
> +        TIMER0_COMPC_IRQ, TIMER0_OVF_IRQ,
> +    TIMER1_CAPT_IRQ, TIMER1_COMPA_IRQ, TIMER1_COMPB_IRQ,
> +        TIMER1_COMPC_IRQ, TIMER1_OVF_IRQ,
> +    TIMER2_CAPT_IRQ, TIMER2_COMPA_IRQ, TIMER2_COMPB_IRQ,
> +        TIMER2_COMPC_IRQ, TIMER2_OVF_IRQ,
> +    TIMER3_CAPT_IRQ, TIMER3_COMPA_IRQ, TIMER3_COMPB_IRQ,
> +        TIMER3_COMPC_IRQ, TIMER3_OVF_IRQ,
> +    TIMER4_CAPT_IRQ, TIMER4_COMPA_IRQ, TIMER4_COMPB_IRQ,
> +        TIMER4_COMPC_IRQ, TIMER4_OVF_IRQ,
> +    TIMER5_CAPT_IRQ, TIMER5_COMPA_IRQ, TIMER5_COMPB_IRQ,
> +        TIMER5_COMPC_IRQ, TIMER5_OVF_IRQ,
> +};
> +#define IRQ_MAX             64
> +
> +#define USART_RXC_IRQ(n)    (3 * n + USART0_RXC_IRQ)
> +#define USART_DRE_IRQ(n)    (3 * n + USART0_DRE_IRQ)
> +#define USART_TXC_IRQ(n)    (3 * n + USART0_TXC_IRQ)
> +#define TIMER_CAPT_IRQ(n)   (5 * n + TIMER0_CAPT_IRQ)
> +#define TIMER_COMPA_IRQ(n)  (5 * n + TIMER0_COMPA_IRQ)
> +#define TIMER_COMPB_IRQ(n)  (5 * n + TIMER0_COMPB_IRQ)
> +#define TIMER_COMPC_IRQ(n)  (5 * n + TIMER0_COMPC_IRQ)
> +#define TIMER_OVF_IRQ(n)    (5 * n + TIMER0_OVF_IRQ)
> +
> +static const uint8_t irq168_328[IRQ_MAX] = {
> +    [TIMER2_COMPA_IRQ]      = 8,
> +    [TIMER2_COMPB_IRQ]      = 9,
> +    [TIMER2_OVF_IRQ]        = 10,
> +    [TIMER1_CAPT_IRQ]       = 11,
> +    [TIMER1_COMPA_IRQ]      = 12,
> +    [TIMER1_COMPB_IRQ]      = 13,
> +    [TIMER1_OVF_IRQ]        = 14,
> +    [TIMER0_COMPA_IRQ]      = 15,
> +    [TIMER0_COMPB_IRQ]      = 16,
> +    [TIMER0_OVF_IRQ]        = 17,
> +    [USART0_RXC_IRQ]        = 19,
> +    [USART0_DRE_IRQ]        = 20,
> +    [USART0_TXC_IRQ]        = 21,
> +}, irq1280_2560[IRQ_MAX] = {
> +    [TIMER2_COMPA_IRQ]      = 14,
> +    [TIMER2_COMPB_IRQ]      = 15,
> +    [TIMER2_OVF_IRQ]        = 16,
> +    [TIMER1_CAPT_IRQ]       = 17,
> +    [TIMER1_COMPA_IRQ]      = 18,
> +    [TIMER1_COMPB_IRQ]      = 19,
> +    [TIMER1_COMPC_IRQ]      = 20,
> +    [TIMER1_OVF_IRQ]        = 21,
> +    [TIMER0_COMPA_IRQ]      = 22,
> +    [TIMER0_COMPB_IRQ]      = 23,
> +    [TIMER0_OVF_IRQ]        = 24,
> +    [USART0_RXC_IRQ]        = 26,
> +    [USART0_DRE_IRQ]        = 27,
> +    [USART0_TXC_IRQ]        = 28,
> +    [TIMER3_CAPT_IRQ]       = 32,
> +    [TIMER3_COMPA_IRQ]      = 33,
> +    [TIMER3_COMPB_IRQ]      = 34,
> +    [TIMER3_COMPC_IRQ]      = 35,
> +    [TIMER3_OVF_IRQ]        = 36,
> +    [USART1_RXC_IRQ]        = 37,
> +    [USART1_DRE_IRQ]        = 38,
> +    [USART1_TXC_IRQ]        = 39,
> +    [USART2_RXC_IRQ]        = 52,
> +    [USART2_DRE_IRQ]        = 53,
> +    [USART2_TXC_IRQ]        = 54,
> +    [USART3_RXC_IRQ]        = 55,
> +    [USART3_DRE_IRQ]        = 56,
> +    [USART3_TXC_IRQ]        = 57,
> +};
> +
> +enum AtmegaPeripheralAddress {
> +    USART0, USART1, USART2, USART3,
> +    TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
> +    DEV_MAX
> +};
> +
> +#define USART_ADDR(n)       (n + USART0)
> +#define TIMER_ADDR(n)       (n + TIMER0)
> +
> +typedef struct {
> +    uint16_t addr;
> +    uint16_t prr_addr;
> +    uint8_t prr_bit;
> +    /* timer specific */
> +    uint16_t intmask_addr;
> +    uint16_t intflag_addr;
> +    bool is_timer16;
> +} peripheral_cfg;
> +
> +static const peripheral_cfg dev168_328[DEV_MAX] = {
> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
> +    [USART0]        = {  0xc0, 0x64, 1 },
> +}, dev1280_2560[DEV_MAX] = {
> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
> +    [TIMER3]        = {  0x90, 0x65, 3, 0x71, 0x38, true },
> +    [TIMER4]        = {  0xa0, 0x65, 4, 0x72, 0x39, true },
> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
> +    [USART0]        = {  0xc0, 0x64, 1 },
> +    [USART1]        = {  0xc8, 0x65, 0 },
> +    [USART2]        = {  0xd0, 0x65, 1 },
> +    [TIMER5]        = { 0x120, 0x65, 5, 0x73, 0x3a, true },
> +    [USART3]        = { 0x130, 0x65, 2 },
> +};
> +
> +struct AtmegaInfo {
> +    const char *uc_name;
> +    const char *cpu_type;
> +    size_t flash_size;
> +    size_t eeprom_size;
> +    size_t sram_size;
> +    size_t io_size;
> +    size_t uart_count;
> +    size_t timer_count;
> +    size_t gpio_count;
> +    size_t adc_count;
> +    const uint8_t *irq;
> +    const peripheral_cfg *dev;
> +};
> +
> +static const AtmegaInfo atmega_mcu[] = {
> +    {
> +        .uc_name = TYPE_ATMEGA168,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
> +        .flash_size = 16 * KiB,
> +        .eeprom_size = 512,
> +        .sram_size = 1 * KiB,
> +        .io_size = 256,
> +        .uart_count = 1,
> +        .gpio_count = 23,
> +        .adc_count = 6,
> +        .irq = irq168_328,
> +        .dev = dev168_328,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA328,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
> +        .flash_size = 32 * KiB,
> +        .eeprom_size = 1 * KiB,
> +        .sram_size = 2 * KiB,
> +        .io_size = 256,
> +        .uart_count = 1,
> +        .timer_count = 3,
> +        .gpio_count = 23,
> +        .adc_count = 6,
> +        .irq = irq168_328,
> +        .dev = dev168_328,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA1280,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
> +        .flash_size = 128 * KiB,
> +        .eeprom_size = 4 * KiB,
> +        .sram_size = 8 * KiB,
> +        .io_size = 512,
> +        .uart_count = 4,
> +        .timer_count = 6,
> +        .gpio_count = 86,
> +        .adc_count = 16,
> +        .irq = irq1280_2560,
> +        .dev = dev1280_2560,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA2560,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
> +        .flash_size = 256 * KiB,
> +        .eeprom_size = 4 * KiB,
> +        .sram_size = 8 * KiB,
> +        .io_size = 512,
> +        .uart_count = 4,
> +        .timer_count = 6,
> +        .gpio_count = 54,
> +        .adc_count = 16,
> +        .irq = irq1280_2560,
> +        .dev = dev1280_2560,
> +    },
> +};
> +
> +static void connect_nonnull_irq(SysBusDevice *sbd, DeviceState *dev,
> +                                int n, int irq)
> +{
> +    if (irq) {
> +        sysbus_connect_irq(sbd, n, qdev_get_gpio_in(dev, irq));
> +    }
> +}
> +
> +static void connect_pr_irq(AtmegaState *s, const AtmegaInfo *info,
> +                           DeviceState *dev, int index)
> +{
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pwr[info->dev[index].prr_addr
> & 1]),
> +                       info->dev[index].prr_bit,
> +                       qdev_get_gpio_in(dev, 0));
> +}
> +
> +static void atmega_realize(DeviceState *dev, Error **errp)
> +{
> +    AtmegaState *s = ATMEGA(dev);
> +    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
> +    const AtmegaInfo *info = bc->info;
> +    DeviceState *cpudev;
> +    SysBusDevice *sbd;
> +    Error *err = NULL;
> +    char *devname;
> +    size_t i;
> +
> +    if (!s->xtal_freq_hz) {
> +        error_setg(errp, "\"xtal-frequency-hz\" property must be
> provided.");
> +        return;
> +    }
> +
> +    /* CPU */
> +    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
> +                            info->cpu_type, &err, NULL);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized",
> &error_abort);
> +    cpudev = DEVICE(&s->cpu);
> +
> +    /* SRAM */
> +    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
> +                                         "sram", info->sram_size);
> +    memory_region_add_subregion(get_system_memory(),
> +                                OFFSET_DATA + 0x200, &s->sram);
> +
> +    /* Flash */
> +    memory_region_init_rom(&s->flash, OBJECT(dev),
> +                           "flash", info->flash_size, &error_fatal);
> +    memory_region_add_subregion(get_system_memory(), OFFSET_CODE,
> &s->flash);
> +
> +    /* I/O */
> +    s->io = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
> +    qdev_prop_set_string(s->io, "name", "I/O");
> +    qdev_prop_set_uint64(s->io, "size", info->io_size);
> +    qdev_init_nofail(s->io);
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA,
> -1234);
> +
> +    /* Power */
> +    for (i = 0; i < ARRAY_SIZE(s->pwr); i++) {
> +        devname = g_strdup_printf("pwr%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->pwr[i], sizeof(s->pwr[i]),
> +                                TYPE_AVR_MASK, &error_abort, NULL);
> +        object_property_set_bool(OBJECT(&s->pwr[i]), true, "realized",
> +                                 &error_abort);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0, OFFSET_DATA +
> 0x64 + i);
> +        g_free(devname);
> +    }
> +
> +    /* USART */
> +    for (i = 0; i < info->uart_count; i++) {
> +        devname = g_strdup_printf("usart%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->usart[i], sizeof(s->usart[i]),
> +                                TYPE_AVR_USART, &error_abort, NULL);
> +        qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
> +        object_property_set_bool(OBJECT(&s->usart[i]), true, "realized",
> +                                 &error_abort);
> +        sbd = SYS_BUS_DEVICE(&s->usart[i]);
> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA +
> info->dev[USART_ADDR(i)].addr);
> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[USART_RXC_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[USART_DRE_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[USART_TXC_IRQ(i)]);
> +        connect_pr_irq(s, info, DEVICE(&s->usart[i]), 0);
> +        g_free(devname);
> +    }
> +
> +    /* Timer */
> +    for (i = 0; i < info->timer_count; i++) {
> +        int idx = TIMER_ADDR(i);
> +        if (!info->dev[idx].is_timer16) {
> +            create_unimplemented_device("avr-timer8",
> +                                        OFFSET_DATA +
> info->dev[idx].addr, 7);
> +            create_unimplemented_device("avr-timer8-intmask",
> +                                        OFFSET_DATA
> +                                        + info->dev[idx].intmask_addr, 1);
> +            create_unimplemented_device("avr-timer8-intflag",
> +                                        OFFSET_DATA
> +                                        + info->dev[idx].intflag_addr, 1);
> +            continue;
> +        }
> +        devname = g_strdup_printf("timer%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->timer[i], sizeof(s->timer[i]),
> +                                TYPE_AVR_TIMER16, &error_abort, NULL);
> +        object_property_set_uint(OBJECT(&s->timer[i]), s->xtal_freq_hz,
> +                                 "cpu-frequency-hz", &error_abort);
> +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized",
> +                                 &error_abort);
> +        sbd = SYS_BUS_DEVICE(&s->timer[i]);
> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[idx].addr);
> +        sysbus_mmio_map(sbd, 1, OFFSET_DATA +
> info->dev[idx].intmask_addr);
> +        sysbus_mmio_map(sbd, 2, OFFSET_DATA +
> info->dev[idx].intflag_addr);
> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[TIMER_CAPT_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[TIMER_COMPA_IRQ(i)])
> ;
> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[TIMER_COMPB_IRQ(i)])
> ;
> +        connect_nonnull_irq(sbd, cpudev, 3, info->irq[TIMER_COMPC_IRQ(i)])
> ;
> +        connect_nonnull_irq(sbd, cpudev, 4, info->irq[TIMER_OVF_IRQ(i)]);
> +        connect_pr_irq(s, info, DEVICE(&s->timer[i]), 0);
> +        g_free(devname);
> +    }
> +}
> +
> +static Property atmega_props[] = {
> +    DEFINE_PROP_UINT64("xtal-frequency-hz", AtmegaState,
> +                       xtal_freq_hz, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void atmega_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    AtmegaClass *bc = ATMEGA_CLASS(oc);
> +
> +    bc->info = data;
> +    dc->realize = atmega_realize;
> +    dc->props = atmega_props;
> +    /* Reason: Mapped at fixed location on the system bus */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo atmega_type_info = {
> +    .name = TYPE_ATMEGA,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AtmegaState),
> +    .class_size = sizeof(AtmegaClass),
> +    .abstract = true,
> +};
> +
> +static void atmega_register_types(void)
> +{
> +    size_t i;
> +
> +    type_register_static(&atmega_type_info);
> +    for (i = 0; i < ARRAY_SIZE(atmega_mcu); i++) {
> +        assert(atmega_mcu[i].io_size <= 0x200);
> +        assert(atmega_mcu[i].uart_count <= USART_MAX);
> +        assert(atmega_mcu[i].timer_count <= TIMER_MAX);
> +        TypeInfo ti = {
> +            .name = atmega_mcu[i].uc_name,
> +            .parent = TYPE_ATMEGA,
> +            .class_init = atmega_class_init,
> +            .class_data = (void *) &atmega_mcu[i],
> +        };
> +        type_register(&ti);
> +    }
> +}
> +
> +type_init(atmega_register_types)
> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> index 626b7064b3..4b6b911820 100644
> --- a/hw/avr/Makefile.objs
> +++ b/hw/avr/Makefile.objs
> @@ -1 +1,2 @@
>  obj-y += sample.o
> +obj-y += atmega.o
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 24346 bytes --]

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[dovgaluk]

Aleksandar Markovic писал 2019-11-28 12:28:
> On Thursday, November 28, 2019, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
> 
>> Add famous ATmega MCUs:
>> 
>> - middle range: ATmega168 and ATmega328
>> - high range: ATmega1280 and ATmega2560
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
> Philippe, hi.
> 
> Thank you for the impetus you give us all.
> 
> However, is this the right direction?
> 
> Let's analyse some bits and pieces.
> 
> Starting from the commit message, the word "famous" is used, but I
> really don't see enumerated CPUs/MCUs are any special in Atmel lineup.
> Other than we often used the doc describing them (cited several times
> in our discussions) as our reference, but that doesn't make them
> "famous". Ofcourse, there other docs for other Atmel CPUs/MCUs, of at
> lest equivalent significance. For example, "tiny" ones are at least as
> famous as "mega" ones.
> 
> Then, you introduce the term MCU, without proper discussion with
> others on terminology. In parlance of QEMU, ATmega168 at al. are CPUs
> (we all know and assume that that are some peripherals in it). I am
> not against using the term MCU, but let's first sync up on that.
> 
> The added terminology trouble is that MCUs, as you defined them, have
> in array atmega_mcu[] a field called "cpu_type" - why is that field
> not called "mcu_type"? *Very* confusing for any future reader. And
> then, similar terminology conundrum continues with macro
> AVR_CPU_TYPE_NAME().

MCU is a system-on-chip which includes CPU core and peripheral devices.
Separating this is better that including everything into the machine.

E.g., different MCUs may have different IO addresses for USART.

> All of the above is far less important than this question: What are we
> achieving with proposed CPU/MCU definitions? I certainly see the value
> of fitting the complex variety of AVR CPUs/MCUs into QEMU object
> model. No question about that. However, is this the right moment to do
> it? There are still some unresolved architectural problems with
> peripheral definitions, as I noted in yhe comment to Michael's last
> cover letter. This patch does not solve them. It just assumes
> everything is ready with peripherals, let's build CPUs/MCUs. The
> machines based on proposed CPUs/MCUs are not more real that machine
> based on Michael's "sample" machine. At least Michal says "this is not
> a real machine". If we used proposed CPUs/MCUs from this patch, the
> resulting machine is as "paper" machine as yhe "sample" machine. We
> would just live in a la-la lend of thinking: "wow, we model real
> hardware now".
> 
> I would rather accept into QEMU a series admitting we are still far
> from modelling real machine or CPU/MCU, than a series that gives an
> illusion that we are modelling real machine or CPU/MCU.
> 
> As far as utility of this patch for Michael's series, it looks to me
> they add more headake than help (not saying that the help is not
> present) to Michael. He would have anotter abstraction layer to think
> of, at the moment he desperately needs (in my opinion) to focus on
> providing the as solid as possible, and as complete as possinle
> foundations. This patch looks like building castles in the air. Again,
> I am not claiming that the patch is not helpful at all.
> 
> In summary, I think that this patch is premature.
> 


Pavel Dovgalyuk

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 4082 bytes --]

On Thursday, November 28, 2019, dovgaluk <dovgaluk@ispras.ru> wrote:

> Aleksandar Markovic писал 2019-11-28 12:28:
>
>> On Thursday, November 28, 2019, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>
>> Add famous ATmega MCUs:
>>>
>>> - middle range: ATmega168 and ATmega328
>>> - high range: ATmega1280 and ATmega2560
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>
>>
>> Philippe, hi.
>>
>> Thank you for the impetus you give us all.
>>
>> However, is this the right direction?
>>
>> Let's analyse some bits and pieces.
>>
>> Starting from the commit message, the word "famous" is used, but I
>> really don't see enumerated CPUs/MCUs are any special in Atmel lineup.
>> Other than we often used the doc describing them (cited several times
>> in our discussions) as our reference, but that doesn't make them
>> "famous". Ofcourse, there other docs for other Atmel CPUs/MCUs, of at
>> lest equivalent significance. For example, "tiny" ones are at least as
>> famous as "mega" ones.
>>
>> Then, you introduce the term MCU, without proper discussion with
>> others on terminology. In parlance of QEMU, ATmega168 at al. are CPUs
>> (we all know and assume that that are some peripherals in it). I am
>> not against using the term MCU, but let's first sync up on that.
>>
>> The added terminology trouble is that MCUs, as you defined them, have
>> in array atmega_mcu[] a field called "cpu_type" - why is that field
>> not called "mcu_type"? *Very* confusing for any future reader. And
>> then, similar terminology conundrum continues with macro
>> AVR_CPU_TYPE_NAME().
>>
>
> MCU is a system-on-chip which includes CPU core and peripheral devices.
> Separating this is better that including everything into the machine.
>
> E.g., different MCUs may have different IO addresses for USART.
>
>
Pavel,

Do you know how is this resolved for other platforms?

How other platfirms organize and use terms "soc", "mcu", "cpu",  "core",
"cpu core"? And what is the relation between each of them and QEMU command
line options "-cpu" and "-machine"? Is thar organization the same accross
all platforms?

(I am asking you as you most likely have much wider experience in the
topic, sincr mine i limited to mips emulation)

Yours, Aleksandar





> All of the above is far less important than this question: What are we
>> achieving with proposed CPU/MCU definitions? I certainly see the value
>> of fitting the complex variety of AVR CPUs/MCUs into QEMU object
>> model. No question about that. However, is this the right moment to do
>> it? There are still some unresolved architectural problems with
>> peripheral definitions, as I noted in yhe comment to Michael's last
>> cover letter. This patch does not solve them. It just assumes
>> everything is ready with peripherals, let's build CPUs/MCUs. The
>> machines based on proposed CPUs/MCUs are not more real that machine
>> based on Michael's "sample" machine. At least Michal says "this is not
>> a real machine". If we used proposed CPUs/MCUs from this patch, the
>> resulting machine is as "paper" machine as yhe "sample" machine. We
>> would just live in a la-la lend of thinking: "wow, we model real
>> hardware now".
>>
>> I would rather accept into QEMU a series admitting we are still far
>> from modelling real machine or CPU/MCU, than a series that gives an
>> illusion that we are modelling real machine or CPU/MCU.
>>
>> As far as utility of this patch for Michael's series, it looks to me
>> they add more headake than help (not saying that the help is not
>> present) to Michael. He would have anotter abstraction layer to think
>> of, at the moment he desperately needs (in my opinion) to focus on
>> providing the as solid as possible, and as complete as possinle
>> foundations. This patch looks like building castles in the air. Again,
>> I am not claiming that the patch is not helpful at all.
>>
>> In summary, I think that this patch is premature.
>>
>>
>
> Pavel Dovgalyuk
>

[-- Attachment #2: Type: text/html, Size: 5293 bytes --]

Re: [RFC PATCH 00/10] hw/avr: Introduce the Arduino board

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

Hi Philippe.

This is really good news.

Should I do anything or this will be merged after my stuff goes through?

On Thu, Nov 28, 2019 at 3:50 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Michael,
>
> I complained I'd rather have QEMU model real hardware, with
> documentation (schematics).
> Since your series is almost ready to get merged, I prefered to
> spend some time now to write down what I wanted. This is mostly
> a rewrite of your board, but matching the Arduino boards.
>
> Some bug slipped in (uart interrupt not raised) but I'm too tired
> to find it, and since I won't have time to look at it the next
> days, I prefer to send this now.
>
> The first part of the series are quick review notes, which you
> should squash in your previous patches.
>
> I still have in my TODO before merge:
> - Fix the USART IRQ bug
> - Split "Add limited support for USART and 16 bit timer peripherals"
>   in 3 patches: USART/Timer16/INTC
>
> And TODO after merge is:
> - Extract Timer8 common parts from Timer16
> - Add GPIOs
> - Connect LED to GPIO on Arduino
>
> Thank you for having insisted with this during so long!
>
> Regards,
>
> Phil.
>
> Based-on: <20191127175257.23480-1-mrolnik@gmail.com>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg661553.html
>
> Philippe Mathieu-Daudé (10):
>   hw/avr: Kludge to fix build failure
>   target/avr: Remove unused include
>   target/avr: Add missing definitions
>   target/avr: Fix IRQ count
>   hw/char/avr: Reduce USART I/O size
>   hw/avr: Add ATmega microcontrollers
>   hw/avr: Add few Arduino boards
>   tests/acceptance: Keep multilines comment consistent with other tests
>   tests/acceptance: Use the ATmega2560 board
>   hw/avr: Remove the 'sample' board
>
>  hw/avr/atmega.h                  |  58 +++++
>  include/hw/char/avr_usart.h      |   2 +
>  target/avr/cpu.h                 |   2 +
>  hw/avr/arduino.c                 | 173 ++++++++++++++
>  hw/avr/atmega.c                  | 379 +++++++++++++++++++++++++++++++
>  hw/avr/sample.c                  | 282 -----------------------
>  hw/char/avr_usart.c              |   2 +-
>  target/avr/cpu.c                 |   2 +-
>  target/avr/helper.c              |   1 -
>  hw/avr/Makefile.objs             |   3 +-
>  tests/acceptance/machine_avr6.py |  10 +-
>  11 files changed, 623 insertions(+), 291 deletions(-)
>  create mode 100644 hw/avr/atmega.h
>  create mode 100644 hw/avr/arduino.c
>  create mode 100644 hw/avr/atmega.c
>  delete mode 100644 hw/avr/sample.c
>
> --
> 2.21.0
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 3523 bytes --]

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[dovgaluk]

Aleksandar Markovic писал 2019-11-28 13:20:
> On Thursday, November 28, 2019, dovgaluk <dovgaluk@ispras.ru> wrote:
> 
>> Aleksandar Markovic писал 2019-11-28 12:28:
>> On Thursday, November 28, 2019, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>> 
>> Add famous ATmega MCUs:
>> 
>> - middle range: ATmega168 and ATmega328
>> - high range: ATmega1280 and ATmega2560
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> 
>> Philippe, hi.
>> 
>> Thank you for the impetus you give us all.
>> 
>> However, is this the right direction?
>> 
>> Let's analyse some bits and pieces.
>> 
>> Starting from the commit message, the word "famous" is used, but I
>> really don't see enumerated CPUs/MCUs are any special in Atmel
>> lineup.
>> Other than we often used the doc describing them (cited several
>> times
>> in our discussions) as our reference, but that doesn't make them
>> "famous". Ofcourse, there other docs for other Atmel CPUs/MCUs, of
>> at
>> lest equivalent significance. For example, "tiny" ones are at least
>> as
>> famous as "mega" ones.
>> 
>> Then, you introduce the term MCU, without proper discussion with
>> others on terminology. In parlance of QEMU, ATmega168 at al. are
>> CPUs
>> (we all know and assume that that are some peripherals in it). I am
>> not against using the term MCU, but let's first sync up on that.
>> 
>> The added terminology trouble is that MCUs, as you defined them,
>> have
>> in array atmega_mcu[] a field called "cpu_type" - why is that field
>> not called "mcu_type"? *Very* confusing for any future reader. And
>> then, similar terminology conundrum continues with macro
>> AVR_CPU_TYPE_NAME().
> 
> MCU is a system-on-chip which includes CPU core and peripheral
> devices.
> Separating this is better that including everything into the machine.
> 
> E.g., different MCUs may have different IO addresses for USART.
> 
> Pavel,
> 
> Do you know how is this resolved for other platforms?
> 
> How other platfirms organize and use terms "soc", "mcu", "cpu",
> "core", "cpu core"? And what is the relation between each of them and
> QEMU command line options "-cpu" and "-machine"? Is thar organization
> the same accross all platforms?

Here is an ARM example:
  SoCs: hw/arm/aspeed_soc.c include/hw/arm/aspeed_soc.h
  Boards: hw/arm/aspeed.c

> (I am asking you as you most likely have much wider experience in the
> topic, sincr mine i limited to mips emulation)

As far as I know, there are MIPS SoCs too.
Doesn't QEMU emulate any of them?

>>> All of the above is far less important than this question: What
>>> are we
>>> achieving with proposed CPU/MCU definitions? I certainly see the
>>> value
>>> of fitting the complex variety of AVR CPUs/MCUs into QEMU object
>>> model. No question about that. However, is this the right moment
>>> to do
>>> it? There are still some unresolved architectural problems with
>>> peripheral definitions, as I noted in yhe comment to Michael's
>>> last
>>> cover letter. This patch does not solve them. It just assumes
>>> everything is ready with peripherals, let's build CPUs/MCUs. The
>>> machines based on proposed CPUs/MCUs are not more real that
>>> machine
>>> based on Michael's "sample" machine. At least Michal says "this is
>>> not
>>> a real machine". If we used proposed CPUs/MCUs from this patch,
>>> the
>>> resulting machine is as "paper" machine as yhe "sample" machine.
>>> We
>>> would just live in a la-la lend of thinking: "wow, we model real
>>> hardware now".
>>> 
>>> I would rather accept into QEMU a series admitting we are still
>>> far
>>> from modelling real machine or CPU/MCU, than a series that gives
>>> an
>>> illusion that we are modelling real machine or CPU/MCU.
>>> 
>>> As far as utility of this patch for Michael's series, it looks to
>>> me
>>> they add more headake than help (not saying that the help is not
>>> present) to Michael. He would have anotter abstraction layer to
>>> think
>>> of, at the moment he desperately needs (in my opinion) to focus on
>>> providing the as solid as possible, and as complete as possinle
>>> foundations. This patch looks like building castles in the air.
>>> Again,
>>> I am not claiming that the patch is not helpful at all.
>>> 
>>> In summary, I think that this patch is premature.



Pavel Dovgalyuk

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

On 11/28/19 11:20 AM, Aleksandar Markovic wrote:
> On Thursday, November 28, 2019, dovgaluk <dovgaluk@ispras.ru 
> <mailto:dovgaluk@ispras.ru>> wrote:
> 
>     Aleksandar Markovic писал 2019-11-28 12:28:
> 
>         On Thursday, November 28, 2019, Philippe Mathieu-Daudé
>         <f4bug@amsat.org <mailto:f4bug@amsat.org>> wrote:
> 
>             Add famous ATmega MCUs:
> 
>             - middle range: ATmega168 and ATmega328
>             - high range: ATmega1280 and ATmega2560
> 
>             Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>             <mailto:f4bug@amsat.org>>
>             ---
> 
> 
>         Philippe, hi.
> 
>         Thank you for the impetus you give us all.
> 
>         However, is this the right direction?
> 
>         Let's analyse some bits and pieces.
> 
>         Starting from the commit message, the word "famous" is used, but I
>         really don't see enumerated CPUs/MCUs are any special in Atmel
>         lineup.
>         Other than we often used the doc describing them (cited several
>         times
>         in our discussions) as our reference, but that doesn't make them
>         "famous". Ofcourse, there other docs for other Atmel CPUs/MCUs,
>         of at
>         lest equivalent significance. For example, "tiny" ones are at
>         least as
>         famous as "mega" ones.
> 
>         Then, you introduce the term MCU, without proper discussion with
>         others on terminology. In parlance of QEMU, ATmega168 at al. are
>         CPUs
>         (we all know and assume that that are some peripherals in it). I am
>         not against using the term MCU, but let's first sync up on that.
> 
>         The added terminology trouble is that MCUs, as you defined them,
>         have
>         in array atmega_mcu[] a field called "cpu_type" - why is that field
>         not called "mcu_type"? *Very* confusing for any future reader. And
>         then, similar terminology conundrum continues with macro
>         AVR_CPU_TYPE_NAME().
> 
> 
>     MCU is a system-on-chip which includes CPU core and peripheral devices.
>     Separating this is better that including everything into the machine.
> 
>     E.g., different MCUs may have different IO addresses for USART.
> 
> 
> Pavel,
> 
> Do you know how is this resolved for other platforms?
> 
> How other platfirms organize and use terms "soc", "mcu", "cpu",  "core", 
> "cpu core"? And what is the relation between each of them and QEMU 
> command line options "-cpu" and "-machine"? Is thar organization the 
> same accross all platforms?

The market/industry evolves quicker than the QEMU codebase, so some 
technical namings that were fine 14 years ago might be not sufficient 
nowadays.

QEMU started with dies/chipsets being really only a 
CentralProcessingUnit, cache managed off-die, external RAM, external 
peripherals.

For example the R4000 is a CPU with internal cache but QEMU doesn't 
model caches.

Now compare that with the I6500. Is it a CPU? Yes and no...
It has 4-6 cores (each having its own ScratchPad RAM), each core having 
various threads (now 'thread' == QEMU MIPS CPU), there are peripherals 
dedicated to inter-core communication; then there is also some I/O 
coherence units to use external peripherals. Add to that you can 
clusterize the cores :)
All that in the same die. The industry calls this 'System-on-Chip', 
abbreviated SoC.
Actually the 'SoC' abbreviation is used as it by MIPS marketing:
https://www.mips.com/products/warrior/i-class-i6500-multiprocessor-core/

   "As such, in conjunction with popular third party SoC coherent
   interconnect/fabric alternatives, the I6500 family can be used
   in “many core” implementations of up to 64 clusters, or >1500
   processing elements (up to four threads/core, up to six
   cores/cluster, up to 64 clusters)."


MCU and SoC are similar in that they contains CPU and peripherals in the 
same die.


Now the QEMU codebase has various 'subsystems' of interests. Group of 
developers are custom to work in the same subsystems. When I'm writing 
code/comments targeting a specific group, I try to use the technical 
vocabulary used by the group, often based on the subsystem technical 
documentation available (specs, datasheets). This is why here I used the 
MCU terminology.

> (I am asking you as you most likely have much wider experience in the 
> topic, sincr mine i limited to mips emulation)
> 
> Yours, Aleksandar
[...]

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 4927 bytes --]

On Thursday, November 28, 2019, dovgaluk <dovgaluk@ispras.ru> wrote:

> Aleksandar Markovic писал 2019-11-28 13:20:
>
>> On Thursday, November 28, 2019, dovgaluk <dovgaluk@ispras.ru> wrote:
>>
>> Aleksandar Markovic писал 2019-11-28 12:28:
>>> On Thursday, November 28, 2019, Philippe Mathieu-Daudé
>>> <f4bug@amsat.org> wrote:
>>>
>>> Add famous ATmega MCUs:
>>>
>>> - middle range: ATmega168 and ATmega328
>>> - high range: ATmega1280 and ATmega2560
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>
>>> Philippe, hi.
>>>
>>> Thank you for the impetus you give us all.
>>>
>>> However, is this the right direction?
>>>
>>> Let's analyse some bits and pieces.
>>>
>>> Starting from the commit message, the word "famous" is used, but I
>>> really don't see enumerated CPUs/MCUs are any special in Atmel
>>> lineup.
>>> Other than we often used the doc describing them (cited several
>>> times
>>> in our discussions) as our reference, but that doesn't make them
>>> "famous". Ofcourse, there other docs for other Atmel CPUs/MCUs, of
>>> at
>>> lest equivalent significance. For example, "tiny" ones are at least
>>> as
>>> famous as "mega" ones.
>>>
>>> Then, you introduce the term MCU, without proper discussion with
>>> others on terminology. In parlance of QEMU, ATmega168 at al. are
>>> CPUs
>>> (we all know and assume that that are some peripherals in it). I am
>>> not against using the term MCU, but let's first sync up on that.
>>>
>>> The added terminology trouble is that MCUs, as you defined them,
>>> have
>>> in array atmega_mcu[] a field called "cpu_type" - why is that field
>>> not called "mcu_type"? *Very* confusing for any future reader. And
>>> then, similar terminology conundrum continues with macro
>>> AVR_CPU_TYPE_NAME().
>>>
>>
>> MCU is a system-on-chip which includes CPU core and peripheral
>> devices.
>> Separating this is better that including everything into the machine.
>>
>> E.g., different MCUs may have different IO addresses for USART.
>>
>> Pavel,
>>
>> Do you know how is this resolved for other platforms?
>>
>> How other platfirms organize and use terms "soc", "mcu", "cpu",
>> "core", "cpu core"? And what is the relation between each of them and
>> QEMU command line options "-cpu" and "-machine"? Is thar organization
>> the same accross all platforms?
>>
>
> Here is an ARM example:
>  SoCs: hw/arm/aspeed_soc.c include/hw/arm/aspeed_soc.h
>  Boards: hw/arm/aspeed.c
>
> (I am asking you as you most likely have much wider experience in the
>> topic, sincr mine i limited to mips emulation)
>>
>
> As far as I know, there are MIPS SoCs too.
> Doesn't QEMU emulate any of them?


It does, but, admitedly, we could do a much better job in that area, and we
are certainly not good as a reference platform in that area. Some features
of MIPS SoCs are extremely difficult to implement in QEMU though (for
example, so called hardware miltithreading).

Thanks,
Aleksandar


>
> All of the above is far less important than this question: What
>>>> are we
>>>> achieving with proposed CPU/MCU definitions? I certainly see the
>>>> value
>>>> of fitting the complex variety of AVR CPUs/MCUs into QEMU object
>>>> model. No question about that. However, is this the right moment
>>>> to do
>>>> it? There are still some unresolved architectural problems with
>>>> peripheral definitions, as I noted in yhe comment to Michael's
>>>> last
>>>> cover letter. This patch does not solve them. It just assumes
>>>> everything is ready with peripherals, let's build CPUs/MCUs. The
>>>> machines based on proposed CPUs/MCUs are not more real that
>>>> machine
>>>> based on Michael's "sample" machine. At least Michal says "this is
>>>> not
>>>> a real machine". If we used proposed CPUs/MCUs from this patch,
>>>> the
>>>> resulting machine is as "paper" machine as yhe "sample" machine.
>>>> We
>>>> would just live in a la-la lend of thinking: "wow, we model real
>>>> hardware now".
>>>>
>>>> I would rather accept into QEMU a series admitting we are still
>>>> far
>>>> from modelling real machine or CPU/MCU, than a series that gives
>>>> an
>>>> illusion that we are modelling real machine or CPU/MCU.
>>>>
>>>> As far as utility of this patch for Michael's series, it looks to
>>>> me
>>>> they add more headake than help (not saying that the help is not
>>>> present) to Michael. He would have anotter abstraction layer to
>>>> think
>>>> of, at the moment he desperately needs (in my opinion) to focus on
>>>> providing the as solid as possible, and as complete as possinle
>>>> foundations. This patch looks like building castles in the air.
>>>> Again,
>>>> I am not claiming that the patch is not helpful at all.
>>>>
>>>> In summary, I think that this patch is premature.
>>>>
>>>
>
>
> Pavel Dovgalyuk
>
>

[-- Attachment #2: Type: text/html, Size: 6384 bytes --]

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

Hi Aleksandar, Richard.

On 11/28/19 10:28 AM, Aleksandar Markovic wrote:
> On Thursday, November 28, 2019, Philippe Mathieu-Daudé <f4bug@amsat.org 
> <mailto:f4bug@amsat.org>> wrote:
> 
>     Add famous ATmega MCUs:
> 
>     - middle range: ATmega168 and ATmega328
>     - high range: ATmega1280 and ATmega2560
> 
>     Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>     <mailto:f4bug@amsat.org>>
>     ---
> 
> 
> Philippe, hi.
> 
> Thank you for the impetus you give us all.
> 
> However, is this the right direction?

I am not the maintainer who will merge the AVR port, so I'm not an 
authoritative figure. I simply wanted to help Michael and Joaquin, so 
spent 6h of my personal time here.

> Let's analyse some bits and pieces.
> 
> Starting from the commit message, the word "famous" is used, but I 
> really don't see enumerated CPUs/MCUs are any special in Atmel lineup. 
> Other than we often used the doc describing them (cited several times in 
> our discussions) as our reference, but that doesn't make them "famous". 
> Ofcourse, there other docs for other Atmel CPUs/MCUs, of at lest 
> equivalent significance. For example, "tiny" ones are at least as famous 
> as "mega" ones.

As noted in the cover, this is sent as RFC and I was quite tired when 
posting so the documentation is rather scarce, but the patches were 
targeted at Michael/Joaquin as example how they could properly add AVR 
machines.

> Then, you introduce the term MCU, without proper discussion with others 
> on terminology. In parlance of QEMU, ATmega168 at al. are CPUs (we all 
> know and assume that that are some peripherals in it). I am not against 
> using the term MCU, but let's first sync up on that.
> 
> The added terminology trouble is that MCUs, as you defined them, have in 
> array atmega_mcu[] a field called "cpu_type" - why is that field not 
> called "mcu_type"? *Very* confusing for any future reader. And then, 
> similar terminology conundrum continues with macro AVR_CPU_TYPE_NAME().
> 
> All of the above is far less important than this question: What are we 
> achieving with proposed CPU/MCU definitions? I certainly see the value 
> of fitting the complex variety of AVR CPUs/MCUs into QEMU object model. 
> No question about that. However, is this the right moment to do it? 
> There are still some unresolved architectural problems with peripheral 
> definitions, as I noted in yhe comment to Michael's last cover letter. 
> This patch does not solve them. It just assumes everything is ready with 
> peripherals, let's build CPUs/MCUs. The machines based on proposed 
> CPUs/MCUs are not more real that machine based on Michael's "sample" 
> machine. At least Michal says "this is not a real machine". If we used 
> proposed CPUs/MCUs from this patch, the resulting machine is as "paper" 
> machine as yhe "sample" machine. We would just live in a la-la lend of 
> thinking: "wow, we model real hardware now".
> 
> I would rather accept into QEMU a series admitting we are still far from 
> modelling real machine or CPU/MCU, than a series that gives an illusion 
> that we are modelling real machine or CPU/MCU.

These patches try to do the same as the 'sample' machine modeled by 
Michael, but reordered in a different way.
The only documentation was "The CPU is an approximation of an 
ATmega2560" so I assumed he could be using the Arduino MEGA2560 board 
which matches. I used the same peripherals that Michael used, simply 
presented a more QEMU-up-to-date way.

> As far as utility of this patch for Michael's series, it looks to me 
> they add more headake than help (not saying that the help is not 
> present) to Michael. He would have anotter abstraction layer to think 
> of, at the moment he desperately needs (in my opinion) to focus on 
> providing the as solid as possible, and as complete as possinle 
> foundations. This patch looks like building castles in the air. Again, I 
> am not claiming that the patch is not helpful at all.
> 
> In summary, I think that this patch is premature.

If we merge the 'sample' board, the deprecation process will take at 
least 8 months, hoping no-one start to hack it.

So to save time, we can merge the architectural part (target/avr) of 
Michael work, without the hardware part (hw/avr).

Michael/Sarah can still test their 'instruction-tests' [*] suite using 
the 'none' machine.

See this example which creates a testing machine with a AVR3 core and 
1MB of RAM mapped at 0x0000:

$ avr-softmmu/qemu-system-avr -M none -cpu avr3-avr-cpu -m 1 -S -monitor 
stdio
QEMU 4.1.93 monitor - type 'help' for more information
(qemu) info mtree
address-space: cpu-memory-0
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-00000000000fffff (prio 0, ram): ram

(qemu) info qom-tree
/machine (none-machine)
   /unattached (container)
     /ram[0] (qemu:memory-region)
     /sysbus (System)
     /system[0] (qemu:memory-region)
     /io[0] (qemu:memory-region)
     /device[0] (avr3-avr-cpu)
     [...]

They won't be able to run the FreeRTOS test suite [*] until we find a 
consensus with the AVR hardware/boards.

This seems the clever outcome, so the AVR architectural part get merged 
as soon as 5.0 opens, and we have time to improve the hardware part.
And Michael won't have to repost his series until v42 :)

Richard, you seem to be the de-facto maintainer, is that correct?
If you agree with my suggestion, I can prepare a v38 based on Michael's 
last series, sanitized and without the HW part, so it you can queue it 
for avr-next.

[*] https://github.com/seharris/qemu-avr-tests

Re: [RFC PATCH 07/10] hw/avr: Add few Arduino boards

[Igor Mammedov]

On Thu, 28 Nov 2019 02:50:27 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Add famous Arduino boards:
> 
> - Arduino Duemilanove
> - Arduino Mega
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/avr/arduino.c     | 173 +++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/Makefile.objs |   1 +
>  2 files changed, 174 insertions(+)
>  create mode 100644 hw/avr/arduino.c
> 
> diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
> new file mode 100644
> index 0000000000..191193d614
> --- /dev/null
> +++ b/hw/avr/arduino.c
> @@ -0,0 +1,173 @@
> +/*
> + * QEMU Arduino boards
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +/* TODO: Implement the use of EXTRAM */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/loader.h"
> +#include "elf.h"
> +#include "atmega.h"
> +
> +typedef struct ArduinoBoardConfig {
> +    const char *name;
> +    const char *desc;
> +    const char *alias;
> +    const char *mcu_type;
> +    uint64_t xtal_hz;
> +    size_t extram_size;
> +    bool is_default;
> +} ArduinoBoardConfig;
> +
> +static const ArduinoBoardConfig arduino_boards[] = {
> +    {
> +        /* https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove */
> +        .name       = MACHINE_TYPE_NAME("arduino-duemilanove"),
> +        .desc       = "Arduino Duemilanove (ATmega168)",
> +        .alias      = "2009",
> +        .mcu_type    = TYPE_ATMEGA168,
> +        .xtal_hz    = 16 * 1000 * 1000,
> +    }, {
> +        /* https://store.arduino.cc/arduino-uno-rev3 */
> +        .name       = MACHINE_TYPE_NAME("arduino-uno"),
> +        .desc       = "Arduino Duemilanove (ATmega328P)",
> +        .alias      = "UNO",
> +        .mcu_type    = TYPE_ATMEGA328,
> +        .xtal_hz    = 16 * 1000 * 1000,
> +    }, {
> +        /* https://www.arduino.cc/en/Main/ArduinoBoardMega */
> +        .name       = MACHINE_TYPE_NAME("arduino-mega"),
> +        .desc       = "Arduino Mega (ATmega1280)",
> +        .alias      = "MEGA",
> +        .mcu_type    = TYPE_ATMEGA1280,
> +        .xtal_hz    = 16 * 1000 * 1000,
> +    }, {
> +        /* https://store.arduino.cc/arduino-mega-2560-rev3 */
> +        .name       = MACHINE_TYPE_NAME("arduino-mega-2560-v3"),
> +        .desc       = "Arduino Mega 2560 (ATmega2560)",
> +        .alias      = "MEGA2560",
> +        .mcu_type    = TYPE_ATMEGA2560,
> +        .xtal_hz    = 16 * 1000 * 1000, /* CSTCE16M0V53-R0 */
> +        .is_default = true,
> +    },
> +};

It's common pre QOM pattern, but it would be better use machine class
for defining machine properties. See
 baa4732bc10b3fd7 aspeed: Remove AspeedBoardConfig array and use AspeedMachineClass
that recently did conversion.


> +
> +typedef struct ArduinoMachineState {
> +    /*< private >*/
> +    MachineState parent_obj;
> +    /*< public >*/
> +    AtmegaState mcu;
> +    MemoryRegion extram;
> +} ArduinoMachineState;
> +
> +typedef struct ArduinoMachineClass {
> +    /*< private >*/
> +    MachineClass parent_class;
> +    /*< public >*/
> +    const ArduinoBoardConfig *config;
> +} ArduinoMachineClass;
> +
> +#define TYPE_ARDUINO_MACHINE \
> +        MACHINE_TYPE_NAME("arduino")
> +#define ARDUINO_MACHINE(obj) \
> +        OBJECT_CHECK(ArduinoMachineState, (obj), TYPE_ARDUINO_MACHINE)
> +#define ARDUINO_MACHINE_CLASS(klass) \
> +        OBJECT_CLASS_CHECK(ArduinoMachineClass, (klass), TYPE_ARDUINO_MACHINE)
> +#define ARDUINO_MACHINE_GET_CLASS(obj) \
> +        OBJECT_GET_CLASS(ArduinoMachineClass, (obj), TYPE_ARDUINO_MACHINE)
> +
> +static void load_firmware(const char *firmware, uint64_t flash_size)
> +{
> +    const char *filename;
> +    int bytes_loaded;
> +
> +    /* Load firmware (contents of flash) trying to auto-detect format */
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
> +    if (filename == NULL) {
> +        error_report("Unable to find %s", firmware);
> +        exit(1);
> +    }
> +
> +    bytes_loaded = load_elf(filename, NULL, NULL, NULL, NULL, NULL, NULL,
> +                            0, EM_NONE, 0, 0);
> +    if (bytes_loaded < 0) {
> +        bytes_loaded = load_image_targphys(filename, OFFSET_CODE, flash_size);
> +    }
> +    if (bytes_loaded < 0) {
> +        error_report("Unable to load firmware image %s as ELF or raw binary",
> +                     firmware);
> +        exit(1);
> +    }
> +}
> +
> +static void arduino_machine_init(MachineState *machine)
> +{
> +    ArduinoMachineClass *amc = ARDUINO_MACHINE_GET_CLASS(machine);
> +    ArduinoMachineState *ams = ARDUINO_MACHINE(machine);
> +
> +    sysbus_init_child_obj(OBJECT(machine), "mcu", &ams->mcu, sizeof(ams->mcu),
> +                          amc->config->mcu_type);
> +    object_property_set_uint(OBJECT(&ams->mcu), amc->config->xtal_hz,
> +                             "xtal-frequency-hz", &error_abort);
> +    object_property_set_bool(OBJECT(&ams->mcu), true, "realized",
> +                             &error_abort);
> +
> +    if (machine->firmware) {
> +        load_firmware(machine->firmware, memory_region_size(&ams->mcu.flash));
> +    }
> +}
> +
> +static void arduino_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +    const ArduinoBoardConfig *cfg = data;
> +
> +    mc->desc = cfg->desc;
> +    mc->alias = cfg->alias;
> +    mc->init = arduino_machine_init;
> +    mc->default_cpus = 1;
> +    mc->min_cpus = mc->default_cpus;
> +    mc->max_cpus = mc->default_cpus;
> +    mc->no_floppy = 1;
> +    mc->no_cdrom = 1;
> +    mc->no_parallel = 1;
> +    mc->is_default = cfg->is_default;
> +    mc->default_ram_size = cfg->extram_size;
> +    amc->config = cfg;
> +}
> +
> +static const TypeInfo arduino_machine_type = {
> +    .name = TYPE_ARDUINO_MACHINE,
> +    .parent = TYPE_MACHINE,
> +    .instance_size = sizeof(ArduinoMachineState),
> +    .class_size = sizeof(ArduinoMachineClass),
> +    .abstract = true,
> +};
> +
> +static void arduino_machine_types(void)
> +{
> +    size_t i;
> +
> +    type_register_static(&arduino_machine_type);
> +    for (i = 0; i < ARRAY_SIZE(arduino_boards); ++i) {
> +        TypeInfo ti = {
> +            .name       = arduino_boards[i].name,
> +            .parent     = TYPE_ARDUINO_MACHINE,
> +            .class_init = arduino_machine_class_init,
> +            .class_data = (void *)&arduino_boards[i],
> +        };
> +        type_register(&ti);
> +    }
> +}
> +
> +type_init(arduino_machine_types)
> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> index 4b6b911820..39ee3c32b2 100644
> --- a/hw/avr/Makefile.objs
> +++ b/hw/avr/Makefile.objs
> @@ -1,2 +1,3 @@
>  obj-y += sample.o
>  obj-y += atmega.o
> +obj-y += arduino.o

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Igor Mammedov]

On Thu, 28 Nov 2019 02:50:26 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Add famous ATmega MCUs:
> 
> - middle range: ATmega168 and ATmega328
> - high range: ATmega1280 and ATmega2560
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/avr/atmega.h      |  58 +++++++
>  hw/avr/atmega.c      | 379 +++++++++++++++++++++++++++++++++++++++++++
>  hw/avr/Makefile.objs |   1 +
>  3 files changed, 438 insertions(+)
>  create mode 100644 hw/avr/atmega.h
>  create mode 100644 hw/avr/atmega.c
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> new file mode 100644
> index 0000000000..d22d90a962
> --- /dev/null
> +++ b/hw/avr/atmega.h
> @@ -0,0 +1,58 @@
> +/*
> + * QEMU ATmega MCU
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_AVR_ATMEGA_H
> +#define HW_AVR_ATMEGA_H
> +
> +#include "hw/char/avr_usart.h"
> +#include "hw/char/avr_usart.h"
> +#include "hw/timer/avr_timer16.h"
> +#include "hw/misc/avr_mask.h"
> +#include "target/avr/cpu.h"
> +
> +#define TYPE_ATMEGA     "ATmega"
> +#define TYPE_ATMEGA168  "ATmega168"
> +#define TYPE_ATMEGA328  "ATmega328"
> +#define TYPE_ATMEGA1280 "ATmega1280"
> +#define TYPE_ATMEGA2560 "ATmega2560"
> +#define ATMEGA(obj)     OBJECT_CHECK(AtmegaState, (obj), TYPE_ATMEGA)
> +
> +#define USART_MAX 4
> +#define TIMER_MAX 6
> +
> +typedef struct AtmegaState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    AVRCPU cpu;
> +    MemoryRegion flash;
> +    MemoryRegion eeprom;
> +    MemoryRegion sram;
> +    DeviceState *io;
> +    AVRMaskState pwr[2];
> +    AVRUsartState usart[USART_MAX];
> +    AVRTimer16State timer[TIMER_MAX];
> +    uint64_t xtal_freq_hz;
> +} AtmegaState;
> +
> +typedef struct AtmegaInfo AtmegaInfo;
> +
> +typedef struct AtmegaClass {
> +    SysBusDeviceClass parent_class;
> +    const AtmegaInfo *info;
> +} AtmegaClass;
> +
> +#define ATMEGA_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(AtmegaClass, (klass), TYPE_ATMEGA)
> +#define ATMEGA_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(AtmegaClass, (obj), TYPE_ATMEGA)
> +
> +#endif /* HW_AVR_ATMEGA_H */
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> new file mode 100644
> index 0000000000..1f1b1246ef
> --- /dev/null
> +++ b/hw/avr/atmega.c
> @@ -0,0 +1,379 @@
> +/*
> + * QEMU ATmega MCU
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "qapi/error.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/sysbus.h"
> +#include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for sram */
> +#include "hw/misc/unimp.h"
> +#include "atmega.h"
> +
> +enum AtmegaIrq {
> +    USART0_RXC_IRQ, USART0_DRE_IRQ, USART0_TXC_IRQ,
> +    USART1_RXC_IRQ, USART1_DRE_IRQ, USART1_TXC_IRQ,
> +    USART2_RXC_IRQ, USART2_DRE_IRQ, USART2_TXC_IRQ,
> +    USART3_RXC_IRQ, USART3_DRE_IRQ, USART3_TXC_IRQ,
> +    TIMER0_CAPT_IRQ, TIMER0_COMPA_IRQ, TIMER0_COMPB_IRQ,
> +        TIMER0_COMPC_IRQ, TIMER0_OVF_IRQ,
> +    TIMER1_CAPT_IRQ, TIMER1_COMPA_IRQ, TIMER1_COMPB_IRQ,
> +        TIMER1_COMPC_IRQ, TIMER1_OVF_IRQ,
> +    TIMER2_CAPT_IRQ, TIMER2_COMPA_IRQ, TIMER2_COMPB_IRQ,
> +        TIMER2_COMPC_IRQ, TIMER2_OVF_IRQ,
> +    TIMER3_CAPT_IRQ, TIMER3_COMPA_IRQ, TIMER3_COMPB_IRQ,
> +        TIMER3_COMPC_IRQ, TIMER3_OVF_IRQ,
> +    TIMER4_CAPT_IRQ, TIMER4_COMPA_IRQ, TIMER4_COMPB_IRQ,
> +        TIMER4_COMPC_IRQ, TIMER4_OVF_IRQ,
> +    TIMER5_CAPT_IRQ, TIMER5_COMPA_IRQ, TIMER5_COMPB_IRQ,
> +        TIMER5_COMPC_IRQ, TIMER5_OVF_IRQ,
> +};
> +#define IRQ_MAX             64
> +
> +#define USART_RXC_IRQ(n)    (3 * n + USART0_RXC_IRQ)
> +#define USART_DRE_IRQ(n)    (3 * n + USART0_DRE_IRQ)
> +#define USART_TXC_IRQ(n)    (3 * n + USART0_TXC_IRQ)
> +#define TIMER_CAPT_IRQ(n)   (5 * n + TIMER0_CAPT_IRQ)
> +#define TIMER_COMPA_IRQ(n)  (5 * n + TIMER0_COMPA_IRQ)
> +#define TIMER_COMPB_IRQ(n)  (5 * n + TIMER0_COMPB_IRQ)
> +#define TIMER_COMPC_IRQ(n)  (5 * n + TIMER0_COMPC_IRQ)
> +#define TIMER_OVF_IRQ(n)    (5 * n + TIMER0_OVF_IRQ)
> +
> +static const uint8_t irq168_328[IRQ_MAX] = {
> +    [TIMER2_COMPA_IRQ]      = 8,
> +    [TIMER2_COMPB_IRQ]      = 9,
> +    [TIMER2_OVF_IRQ]        = 10,
> +    [TIMER1_CAPT_IRQ]       = 11,
> +    [TIMER1_COMPA_IRQ]      = 12,
> +    [TIMER1_COMPB_IRQ]      = 13,
> +    [TIMER1_OVF_IRQ]        = 14,
> +    [TIMER0_COMPA_IRQ]      = 15,
> +    [TIMER0_COMPB_IRQ]      = 16,
> +    [TIMER0_OVF_IRQ]        = 17,
> +    [USART0_RXC_IRQ]        = 19,
> +    [USART0_DRE_IRQ]        = 20,
> +    [USART0_TXC_IRQ]        = 21,
> +}, irq1280_2560[IRQ_MAX] = {
> +    [TIMER2_COMPA_IRQ]      = 14,
> +    [TIMER2_COMPB_IRQ]      = 15,
> +    [TIMER2_OVF_IRQ]        = 16,
> +    [TIMER1_CAPT_IRQ]       = 17,
> +    [TIMER1_COMPA_IRQ]      = 18,
> +    [TIMER1_COMPB_IRQ]      = 19,
> +    [TIMER1_COMPC_IRQ]      = 20,
> +    [TIMER1_OVF_IRQ]        = 21,
> +    [TIMER0_COMPA_IRQ]      = 22,
> +    [TIMER0_COMPB_IRQ]      = 23,
> +    [TIMER0_OVF_IRQ]        = 24,
> +    [USART0_RXC_IRQ]        = 26,
> +    [USART0_DRE_IRQ]        = 27,
> +    [USART0_TXC_IRQ]        = 28,
> +    [TIMER3_CAPT_IRQ]       = 32,
> +    [TIMER3_COMPA_IRQ]      = 33,
> +    [TIMER3_COMPB_IRQ]      = 34,
> +    [TIMER3_COMPC_IRQ]      = 35,
> +    [TIMER3_OVF_IRQ]        = 36,
> +    [USART1_RXC_IRQ]        = 37,
> +    [USART1_DRE_IRQ]        = 38,
> +    [USART1_TXC_IRQ]        = 39,
> +    [USART2_RXC_IRQ]        = 52,
> +    [USART2_DRE_IRQ]        = 53,
> +    [USART2_TXC_IRQ]        = 54,
> +    [USART3_RXC_IRQ]        = 55,
> +    [USART3_DRE_IRQ]        = 56,
> +    [USART3_TXC_IRQ]        = 57,
> +};
> +
> +enum AtmegaPeripheralAddress {
> +    USART0, USART1, USART2, USART3,
> +    TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
> +    DEV_MAX
> +};
> +
> +#define USART_ADDR(n)       (n + USART0)
> +#define TIMER_ADDR(n)       (n + TIMER0)
> +
> +typedef struct {
> +    uint16_t addr;
> +    uint16_t prr_addr;
> +    uint8_t prr_bit;
> +    /* timer specific */
> +    uint16_t intmask_addr;
> +    uint16_t intflag_addr;
> +    bool is_timer16;
> +} peripheral_cfg;
> +
> +static const peripheral_cfg dev168_328[DEV_MAX] = {
> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
> +    [USART0]        = {  0xc0, 0x64, 1 },
> +}, dev1280_2560[DEV_MAX] = {
> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
> +    [TIMER3]        = {  0x90, 0x65, 3, 0x71, 0x38, true },
> +    [TIMER4]        = {  0xa0, 0x65, 4, 0x72, 0x39, true },
> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
> +    [USART0]        = {  0xc0, 0x64, 1 },
> +    [USART1]        = {  0xc8, 0x65, 0 },
> +    [USART2]        = {  0xd0, 0x65, 1 },
> +    [TIMER5]        = { 0x120, 0x65, 5, 0x73, 0x3a, true },
> +    [USART3]        = { 0x130, 0x65, 2 },
> +};
> +
> +struct AtmegaInfo {
> +    const char *uc_name;
> +    const char *cpu_type;
> +    size_t flash_size;
> +    size_t eeprom_size;
> +    size_t sram_size;
> +    size_t io_size;
> +    size_t uart_count;
> +    size_t timer_count;
> +    size_t gpio_count;
> +    size_t adc_count;
> +    const uint8_t *irq;
> +    const peripheral_cfg *dev;
> +};
> +
> +static const AtmegaInfo atmega_mcu[] = {
> +    {
> +        .uc_name = TYPE_ATMEGA168,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
> +        .flash_size = 16 * KiB,
> +        .eeprom_size = 512,
> +        .sram_size = 1 * KiB,
> +        .io_size = 256,
> +        .uart_count = 1,
> +        .gpio_count = 23,
> +        .adc_count = 6,
> +        .irq = irq168_328,
> +        .dev = dev168_328,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA328,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
> +        .flash_size = 32 * KiB,
> +        .eeprom_size = 1 * KiB,
> +        .sram_size = 2 * KiB,
> +        .io_size = 256,
> +        .uart_count = 1,
> +        .timer_count = 3,
> +        .gpio_count = 23,
> +        .adc_count = 6,
> +        .irq = irq168_328,
> +        .dev = dev168_328,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA1280,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
> +        .flash_size = 128 * KiB,
> +        .eeprom_size = 4 * KiB,
> +        .sram_size = 8 * KiB,
> +        .io_size = 512,
> +        .uart_count = 4,
> +        .timer_count = 6,
> +        .gpio_count = 86,
> +        .adc_count = 16,
> +        .irq = irq1280_2560,
> +        .dev = dev1280_2560,
> +    },
> +    {
> +        .uc_name = TYPE_ATMEGA2560,
> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
> +        .flash_size = 256 * KiB,
> +        .eeprom_size = 4 * KiB,
> +        .sram_size = 8 * KiB,
> +        .io_size = 512,
> +        .uart_count = 4,
> +        .timer_count = 6,
> +        .gpio_count = 54,
> +        .adc_count = 16,
> +        .irq = irq1280_2560,
> +        .dev = dev1280_2560,
> +    },
> +};
> +
> +static void connect_nonnull_irq(SysBusDevice *sbd, DeviceState *dev,
> +                                int n, int irq)
> +{
> +    if (irq) {
> +        sysbus_connect_irq(sbd, n, qdev_get_gpio_in(dev, irq));
> +    }
> +}
> +
> +static void connect_pr_irq(AtmegaState *s, const AtmegaInfo *info,
> +                           DeviceState *dev, int index)
> +{
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pwr[info->dev[index].prr_addr & 1]),
> +                       info->dev[index].prr_bit,
> +                       qdev_get_gpio_in(dev, 0));
> +}
> +
> +static void atmega_realize(DeviceState *dev, Error **errp)
> +{
> +    AtmegaState *s = ATMEGA(dev);
> +    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
> +    const AtmegaInfo *info = bc->info;
> +    DeviceState *cpudev;
> +    SysBusDevice *sbd;
> +    Error *err = NULL;
> +    char *devname;
> +    size_t i;
> +
> +    if (!s->xtal_freq_hz) {
> +        error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
> +        return;
> +    }
> +
> +    /* CPU */
> +    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
> +                            info->cpu_type, &err, NULL);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &error_abort);
> +    cpudev = DEVICE(&s->cpu);
> +
> +    /* SRAM */
> +    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
> +                                         "sram", info->sram_size);
with main RAM conversion to hostmem backend, this API will go away
and RAM memory region will be allocated by generic machine code
and shall be treated as backend. Users would be able to access it
via MachineState::ram memory region.

Meanwhile I'd suggest to move this line to arduino_machine_init()
and pass it to MCU as a link property.

Also use MachineState::ram_size and add check that it matches mc->default_ram_size.
Ex: https://github.com/imammedo/qemu/commit/241c65d506ccba1e0239a2bc0632d7dc9c3517c1

> +    memory_region_add_subregion(get_system_memory(),
> +                                OFFSET_DATA + 0x200, &s->sram);
> +
> +    /* Flash */
> +    memory_region_init_rom(&s->flash, OBJECT(dev),
> +                           "flash", info->flash_size, &error_fatal);
> +    memory_region_add_subregion(get_system_memory(), OFFSET_CODE, &s->flash);
> +
> +    /* I/O */
> +    s->io = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
> +    qdev_prop_set_string(s->io, "name", "I/O");
> +    qdev_prop_set_uint64(s->io, "size", info->io_size);
> +    qdev_init_nofail(s->io);
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA, -1234);
> +
> +    /* Power */
> +    for (i = 0; i < ARRAY_SIZE(s->pwr); i++) {
> +        devname = g_strdup_printf("pwr%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->pwr[i], sizeof(s->pwr[i]),
> +                                TYPE_AVR_MASK, &error_abort, NULL);
> +        object_property_set_bool(OBJECT(&s->pwr[i]), true, "realized",
> +                                 &error_abort);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0, OFFSET_DATA + 0x64 + i);
> +        g_free(devname);
> +    }
> +
> +    /* USART */
> +    for (i = 0; i < info->uart_count; i++) {
> +        devname = g_strdup_printf("usart%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->usart[i], sizeof(s->usart[i]),
> +                                TYPE_AVR_USART, &error_abort, NULL);
> +        qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
> +        object_property_set_bool(OBJECT(&s->usart[i]), true, "realized",
> +                                 &error_abort);
> +        sbd = SYS_BUS_DEVICE(&s->usart[i]);
> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[USART_ADDR(i)].addr);
> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[USART_RXC_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[USART_DRE_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[USART_TXC_IRQ(i)]);
> +        connect_pr_irq(s, info, DEVICE(&s->usart[i]), 0);
> +        g_free(devname);
> +    }
> +
> +    /* Timer */
> +    for (i = 0; i < info->timer_count; i++) {
> +        int idx = TIMER_ADDR(i);
> +        if (!info->dev[idx].is_timer16) {
> +            create_unimplemented_device("avr-timer8",
> +                                        OFFSET_DATA + info->dev[idx].addr, 7);
> +            create_unimplemented_device("avr-timer8-intmask",
> +                                        OFFSET_DATA
> +                                        + info->dev[idx].intmask_addr, 1);
> +            create_unimplemented_device("avr-timer8-intflag",
> +                                        OFFSET_DATA
> +                                        + info->dev[idx].intflag_addr, 1);
> +            continue;
> +        }
> +        devname = g_strdup_printf("timer%zu", i);
> +        object_initialize_child(OBJECT(dev), devname,
> +                                &s->timer[i], sizeof(s->timer[i]),
> +                                TYPE_AVR_TIMER16, &error_abort, NULL);
> +        object_property_set_uint(OBJECT(&s->timer[i]), s->xtal_freq_hz,
> +                                 "cpu-frequency-hz", &error_abort);
> +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized",
> +                                 &error_abort);
> +        sbd = SYS_BUS_DEVICE(&s->timer[i]);
> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[idx].addr);
> +        sysbus_mmio_map(sbd, 1, OFFSET_DATA + info->dev[idx].intmask_addr);
> +        sysbus_mmio_map(sbd, 2, OFFSET_DATA + info->dev[idx].intflag_addr);
> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[TIMER_CAPT_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[TIMER_COMPA_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[TIMER_COMPB_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 3, info->irq[TIMER_COMPC_IRQ(i)]);
> +        connect_nonnull_irq(sbd, cpudev, 4, info->irq[TIMER_OVF_IRQ(i)]);
> +        connect_pr_irq(s, info, DEVICE(&s->timer[i]), 0);
> +        g_free(devname);
> +    }
> +}
> +
> +static Property atmega_props[] = {
> +    DEFINE_PROP_UINT64("xtal-frequency-hz", AtmegaState,
> +                       xtal_freq_hz, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void atmega_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    AtmegaClass *bc = ATMEGA_CLASS(oc);
> +
> +    bc->info = data;
> +    dc->realize = atmega_realize;
> +    dc->props = atmega_props;
> +    /* Reason: Mapped at fixed location on the system bus */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo atmega_type_info = {
> +    .name = TYPE_ATMEGA,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AtmegaState),
> +    .class_size = sizeof(AtmegaClass),
> +    .abstract = true,
> +};
> +
> +static void atmega_register_types(void)
> +{
> +    size_t i;
> +
> +    type_register_static(&atmega_type_info);
> +    for (i = 0; i < ARRAY_SIZE(atmega_mcu); i++) {
> +        assert(atmega_mcu[i].io_size <= 0x200);
> +        assert(atmega_mcu[i].uart_count <= USART_MAX);
> +        assert(atmega_mcu[i].timer_count <= TIMER_MAX);
> +        TypeInfo ti = {
> +            .name = atmega_mcu[i].uc_name,
> +            .parent = TYPE_ATMEGA,
> +            .class_init = atmega_class_init,
> +            .class_data = (void *) &atmega_mcu[i],
> +        };
> +        type_register(&ti);
> +    }
> +}
> +
> +type_init(atmega_register_types)
> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> index 626b7064b3..4b6b911820 100644
> --- a/hw/avr/Makefile.objs
> +++ b/hw/avr/Makefile.objs
> @@ -1 +1,2 @@
>  obj-y += sample.o
> +obj-y += atmega.o

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

Hi Igor,

On 12/20/19 11:09 AM, Igor Mammedov wrote:
> On Thu, 28 Nov 2019 02:50:26 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> Add famous ATmega MCUs:
>>
>> - middle range: ATmega168 and ATmega328
>> - high range: ATmega1280 and ATmega2560
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/avr/atmega.h      |  58 +++++++
>>   hw/avr/atmega.c      | 379 +++++++++++++++++++++++++++++++++++++++++++
>>   hw/avr/Makefile.objs |   1 +
>>   3 files changed, 438 insertions(+)
>>   create mode 100644 hw/avr/atmega.h
>>   create mode 100644 hw/avr/atmega.c
>>
>> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
>> new file mode 100644
>> index 0000000000..d22d90a962
>> --- /dev/null
>> +++ b/hw/avr/atmega.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * QEMU ATmega MCU
>> + *
>> + * Copyright (c) 2019 Philippe Mathieu-Daudé
>> + *
>> + * This work is licensed under the terms of the GNU GPLv2 or later.
>> + * See the COPYING file in the top-level directory.
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_AVR_ATMEGA_H
>> +#define HW_AVR_ATMEGA_H
>> +
>> +#include "hw/char/avr_usart.h"
>> +#include "hw/char/avr_usart.h"
>> +#include "hw/timer/avr_timer16.h"
>> +#include "hw/misc/avr_mask.h"
>> +#include "target/avr/cpu.h"
>> +
>> +#define TYPE_ATMEGA     "ATmega"
>> +#define TYPE_ATMEGA168  "ATmega168"
>> +#define TYPE_ATMEGA328  "ATmega328"
>> +#define TYPE_ATMEGA1280 "ATmega1280"
>> +#define TYPE_ATMEGA2560 "ATmega2560"
>> +#define ATMEGA(obj)     OBJECT_CHECK(AtmegaState, (obj), TYPE_ATMEGA)
>> +
>> +#define USART_MAX 4
>> +#define TIMER_MAX 6
>> +
>> +typedef struct AtmegaState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +    /*< public >*/
>> +
>> +    AVRCPU cpu;
>> +    MemoryRegion flash;
>> +    MemoryRegion eeprom;
>> +    MemoryRegion sram;
>> +    DeviceState *io;
>> +    AVRMaskState pwr[2];
>> +    AVRUsartState usart[USART_MAX];
>> +    AVRTimer16State timer[TIMER_MAX];
>> +    uint64_t xtal_freq_hz;
>> +} AtmegaState;
>> +
>> +typedef struct AtmegaInfo AtmegaInfo;
>> +
>> +typedef struct AtmegaClass {
>> +    SysBusDeviceClass parent_class;
>> +    const AtmegaInfo *info;
>> +} AtmegaClass;
>> +
>> +#define ATMEGA_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(AtmegaClass, (klass), TYPE_ATMEGA)
>> +#define ATMEGA_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(AtmegaClass, (obj), TYPE_ATMEGA)
>> +
>> +#endif /* HW_AVR_ATMEGA_H */
>> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
>> new file mode 100644
>> index 0000000000..1f1b1246ef
>> --- /dev/null
>> +++ b/hw/avr/atmega.c
>> @@ -0,0 +1,379 @@
>> +/*
>> + * QEMU ATmega MCU
>> + *
>> + * Copyright (c) 2019 Philippe Mathieu-Daudé
>> + *
>> + * This work is licensed under the terms of the GNU GPLv2 or later.
>> + * See the COPYING file in the top-level directory.
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/module.h"
>> +#include "qemu/units.h"
>> +#include "qapi/error.h"
>> +#include "exec/memory.h"
>> +#include "exec/address-spaces.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for sram */
>> +#include "hw/misc/unimp.h"
>> +#include "atmega.h"
>> +
>> +enum AtmegaIrq {
>> +    USART0_RXC_IRQ, USART0_DRE_IRQ, USART0_TXC_IRQ,
>> +    USART1_RXC_IRQ, USART1_DRE_IRQ, USART1_TXC_IRQ,
>> +    USART2_RXC_IRQ, USART2_DRE_IRQ, USART2_TXC_IRQ,
>> +    USART3_RXC_IRQ, USART3_DRE_IRQ, USART3_TXC_IRQ,
>> +    TIMER0_CAPT_IRQ, TIMER0_COMPA_IRQ, TIMER0_COMPB_IRQ,
>> +        TIMER0_COMPC_IRQ, TIMER0_OVF_IRQ,
>> +    TIMER1_CAPT_IRQ, TIMER1_COMPA_IRQ, TIMER1_COMPB_IRQ,
>> +        TIMER1_COMPC_IRQ, TIMER1_OVF_IRQ,
>> +    TIMER2_CAPT_IRQ, TIMER2_COMPA_IRQ, TIMER2_COMPB_IRQ,
>> +        TIMER2_COMPC_IRQ, TIMER2_OVF_IRQ,
>> +    TIMER3_CAPT_IRQ, TIMER3_COMPA_IRQ, TIMER3_COMPB_IRQ,
>> +        TIMER3_COMPC_IRQ, TIMER3_OVF_IRQ,
>> +    TIMER4_CAPT_IRQ, TIMER4_COMPA_IRQ, TIMER4_COMPB_IRQ,
>> +        TIMER4_COMPC_IRQ, TIMER4_OVF_IRQ,
>> +    TIMER5_CAPT_IRQ, TIMER5_COMPA_IRQ, TIMER5_COMPB_IRQ,
>> +        TIMER5_COMPC_IRQ, TIMER5_OVF_IRQ,
>> +};
>> +#define IRQ_MAX             64
>> +
>> +#define USART_RXC_IRQ(n)    (3 * n + USART0_RXC_IRQ)
>> +#define USART_DRE_IRQ(n)    (3 * n + USART0_DRE_IRQ)
>> +#define USART_TXC_IRQ(n)    (3 * n + USART0_TXC_IRQ)
>> +#define TIMER_CAPT_IRQ(n)   (5 * n + TIMER0_CAPT_IRQ)
>> +#define TIMER_COMPA_IRQ(n)  (5 * n + TIMER0_COMPA_IRQ)
>> +#define TIMER_COMPB_IRQ(n)  (5 * n + TIMER0_COMPB_IRQ)
>> +#define TIMER_COMPC_IRQ(n)  (5 * n + TIMER0_COMPC_IRQ)
>> +#define TIMER_OVF_IRQ(n)    (5 * n + TIMER0_OVF_IRQ)
>> +
>> +static const uint8_t irq168_328[IRQ_MAX] = {
>> +    [TIMER2_COMPA_IRQ]      = 8,
>> +    [TIMER2_COMPB_IRQ]      = 9,
>> +    [TIMER2_OVF_IRQ]        = 10,
>> +    [TIMER1_CAPT_IRQ]       = 11,
>> +    [TIMER1_COMPA_IRQ]      = 12,
>> +    [TIMER1_COMPB_IRQ]      = 13,
>> +    [TIMER1_OVF_IRQ]        = 14,
>> +    [TIMER0_COMPA_IRQ]      = 15,
>> +    [TIMER0_COMPB_IRQ]      = 16,
>> +    [TIMER0_OVF_IRQ]        = 17,
>> +    [USART0_RXC_IRQ]        = 19,
>> +    [USART0_DRE_IRQ]        = 20,
>> +    [USART0_TXC_IRQ]        = 21,
>> +}, irq1280_2560[IRQ_MAX] = {
>> +    [TIMER2_COMPA_IRQ]      = 14,
>> +    [TIMER2_COMPB_IRQ]      = 15,
>> +    [TIMER2_OVF_IRQ]        = 16,
>> +    [TIMER1_CAPT_IRQ]       = 17,
>> +    [TIMER1_COMPA_IRQ]      = 18,
>> +    [TIMER1_COMPB_IRQ]      = 19,
>> +    [TIMER1_COMPC_IRQ]      = 20,
>> +    [TIMER1_OVF_IRQ]        = 21,
>> +    [TIMER0_COMPA_IRQ]      = 22,
>> +    [TIMER0_COMPB_IRQ]      = 23,
>> +    [TIMER0_OVF_IRQ]        = 24,
>> +    [USART0_RXC_IRQ]        = 26,
>> +    [USART0_DRE_IRQ]        = 27,
>> +    [USART0_TXC_IRQ]        = 28,
>> +    [TIMER3_CAPT_IRQ]       = 32,
>> +    [TIMER3_COMPA_IRQ]      = 33,
>> +    [TIMER3_COMPB_IRQ]      = 34,
>> +    [TIMER3_COMPC_IRQ]      = 35,
>> +    [TIMER3_OVF_IRQ]        = 36,
>> +    [USART1_RXC_IRQ]        = 37,
>> +    [USART1_DRE_IRQ]        = 38,
>> +    [USART1_TXC_IRQ]        = 39,
>> +    [USART2_RXC_IRQ]        = 52,
>> +    [USART2_DRE_IRQ]        = 53,
>> +    [USART2_TXC_IRQ]        = 54,
>> +    [USART3_RXC_IRQ]        = 55,
>> +    [USART3_DRE_IRQ]        = 56,
>> +    [USART3_TXC_IRQ]        = 57,
>> +};
>> +
>> +enum AtmegaPeripheralAddress {
>> +    USART0, USART1, USART2, USART3,
>> +    TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
>> +    DEV_MAX
>> +};
>> +
>> +#define USART_ADDR(n)       (n + USART0)
>> +#define TIMER_ADDR(n)       (n + TIMER0)
>> +
>> +typedef struct {
>> +    uint16_t addr;
>> +    uint16_t prr_addr;
>> +    uint8_t prr_bit;
>> +    /* timer specific */
>> +    uint16_t intmask_addr;
>> +    uint16_t intflag_addr;
>> +    bool is_timer16;
>> +} peripheral_cfg;
>> +
>> +static const peripheral_cfg dev168_328[DEV_MAX] = {
>> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
>> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
>> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
>> +    [USART0]        = {  0xc0, 0x64, 1 },
>> +}, dev1280_2560[DEV_MAX] = {
>> +    [TIMER0]        = {  0x24, 0x64, 5, 0x6e, 0x35, false },
>> +    [TIMER1]        = {  0x80, 0x64, 3, 0x6f, 0x36, true },
>> +    [TIMER3]        = {  0x90, 0x65, 3, 0x71, 0x38, true },
>> +    [TIMER4]        = {  0xa0, 0x65, 4, 0x72, 0x39, true },
>> +    [TIMER2]        = {  0xb0, 0x64, 6, 0x70, 0x37, false },
>> +    [USART0]        = {  0xc0, 0x64, 1 },
>> +    [USART1]        = {  0xc8, 0x65, 0 },
>> +    [USART2]        = {  0xd0, 0x65, 1 },
>> +    [TIMER5]        = { 0x120, 0x65, 5, 0x73, 0x3a, true },
>> +    [USART3]        = { 0x130, 0x65, 2 },
>> +};
>> +
>> +struct AtmegaInfo {
>> +    const char *uc_name;
>> +    const char *cpu_type;
>> +    size_t flash_size;
>> +    size_t eeprom_size;
>> +    size_t sram_size;
>> +    size_t io_size;
>> +    size_t uart_count;
>> +    size_t timer_count;
>> +    size_t gpio_count;
>> +    size_t adc_count;
>> +    const uint8_t *irq;
>> +    const peripheral_cfg *dev;
>> +};
>> +
>> +static const AtmegaInfo atmega_mcu[] = {
>> +    {
>> +        .uc_name = TYPE_ATMEGA168,
>> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
>> +        .flash_size = 16 * KiB,
>> +        .eeprom_size = 512,
>> +        .sram_size = 1 * KiB,
>> +        .io_size = 256,
>> +        .uart_count = 1,
>> +        .gpio_count = 23,
>> +        .adc_count = 6,
>> +        .irq = irq168_328,
>> +        .dev = dev168_328,
>> +    },
>> +    {
>> +        .uc_name = TYPE_ATMEGA328,
>> +        .cpu_type = AVR_CPU_TYPE_NAME("avr5"),
>> +        .flash_size = 32 * KiB,
>> +        .eeprom_size = 1 * KiB,
>> +        .sram_size = 2 * KiB,
>> +        .io_size = 256,
>> +        .uart_count = 1,
>> +        .timer_count = 3,
>> +        .gpio_count = 23,
>> +        .adc_count = 6,
>> +        .irq = irq168_328,
>> +        .dev = dev168_328,
>> +    },
>> +    {
>> +        .uc_name = TYPE_ATMEGA1280,
>> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
>> +        .flash_size = 128 * KiB,
>> +        .eeprom_size = 4 * KiB,
>> +        .sram_size = 8 * KiB,
>> +        .io_size = 512,
>> +        .uart_count = 4,
>> +        .timer_count = 6,
>> +        .gpio_count = 86,
>> +        .adc_count = 16,
>> +        .irq = irq1280_2560,
>> +        .dev = dev1280_2560,
>> +    },
>> +    {
>> +        .uc_name = TYPE_ATMEGA2560,
>> +        .cpu_type = AVR_CPU_TYPE_NAME("avr6"),
>> +        .flash_size = 256 * KiB,
>> +        .eeprom_size = 4 * KiB,
>> +        .sram_size = 8 * KiB,
>> +        .io_size = 512,
>> +        .uart_count = 4,
>> +        .timer_count = 6,
>> +        .gpio_count = 54,
>> +        .adc_count = 16,
>> +        .irq = irq1280_2560,
>> +        .dev = dev1280_2560,
>> +    },
>> +};
>> +
>> +static void connect_nonnull_irq(SysBusDevice *sbd, DeviceState *dev,
>> +                                int n, int irq)
>> +{
>> +    if (irq) {
>> +        sysbus_connect_irq(sbd, n, qdev_get_gpio_in(dev, irq));
>> +    }
>> +}
>> +
>> +static void connect_pr_irq(AtmegaState *s, const AtmegaInfo *info,
>> +                           DeviceState *dev, int index)
>> +{
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pwr[info->dev[index].prr_addr & 1]),
>> +                       info->dev[index].prr_bit,
>> +                       qdev_get_gpio_in(dev, 0));
>> +}
>> +
>> +static void atmega_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AtmegaState *s = ATMEGA(dev);
>> +    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
>> +    const AtmegaInfo *info = bc->info;
>> +    DeviceState *cpudev;
>> +    SysBusDevice *sbd;
>> +    Error *err = NULL;
>> +    char *devname;
>> +    size_t i;
>> +
>> +    if (!s->xtal_freq_hz) {
>> +        error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
>> +        return;
>> +    }
>> +
>> +    /* CPU */
>> +    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
>> +                            info->cpu_type, &err, NULL);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &error_abort);
>> +    cpudev = DEVICE(&s->cpu);
>> +
>> +    /* SRAM */
>> +    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
>> +                                         "sram", info->sram_size);
> with main RAM conversion to hostmem backend, this API will go away
> and RAM memory region will be allocated by generic machine code
> and shall be treated as backend. Users would be able to access it
> via MachineState::ram memory region.
> 
> Meanwhile I'd suggest to move this line to arduino_machine_init()
> and pass it to MCU as a link property.

Thanks for reviewing this patch.

I think this MCU and few ARM SoC are good case for your RAM conversion.

These chipsets provide onboard RAM (SRAM). This amount of SRAM is enough 
to run u-boot, FreeRTOS, ... Some ARM boards add DRAM, usually larger 
than the SRAM amount.

The previous recomendation was to use 
memory_region_allocate_system_memory() only once, but on the biggest 
chunk of memory, for performance reasons.

In the previous cases, the RAM is not added by the board/machine, but is 
present in the MCU/SoC. This looks incorrect to me to allocate the RAM 
in the board/machine and pass it to the MCU/SoC.

You are saying the machine/board code will have to ask its children how 
many ram they need, then allocate, then pass it to each?

> Also use MachineState::ram_size and add check that it matches mc->default_ram_size.
> Ex: https://github.com/imammedo/qemu/commit/241c65d506ccba1e0239a2bc0632d7dc9c3517c1
> 
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                OFFSET_DATA + 0x200, &s->sram);
>> +
>> +    /* Flash */
>> +    memory_region_init_rom(&s->flash, OBJECT(dev),
>> +                           "flash", info->flash_size, &error_fatal);
>> +    memory_region_add_subregion(get_system_memory(), OFFSET_CODE, &s->flash);
>> +
>> +    /* I/O */
>> +    s->io = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
>> +    qdev_prop_set_string(s->io, "name", "I/O");
>> +    qdev_prop_set_uint64(s->io, "size", info->io_size);
>> +    qdev_init_nofail(s->io);
>> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA, -1234);
>> +
>> +    /* Power */
>> +    for (i = 0; i < ARRAY_SIZE(s->pwr); i++) {
>> +        devname = g_strdup_printf("pwr%zu", i);
>> +        object_initialize_child(OBJECT(dev), devname,
>> +                                &s->pwr[i], sizeof(s->pwr[i]),
>> +                                TYPE_AVR_MASK, &error_abort, NULL);
>> +        object_property_set_bool(OBJECT(&s->pwr[i]), true, "realized",
>> +                                 &error_abort);
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0, OFFSET_DATA + 0x64 + i);
>> +        g_free(devname);
>> +    }
>> +
>> +    /* USART */
>> +    for (i = 0; i < info->uart_count; i++) {
>> +        devname = g_strdup_printf("usart%zu", i);
>> +        object_initialize_child(OBJECT(dev), devname,
>> +                                &s->usart[i], sizeof(s->usart[i]),
>> +                                TYPE_AVR_USART, &error_abort, NULL);
>> +        qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
>> +        object_property_set_bool(OBJECT(&s->usart[i]), true, "realized",
>> +                                 &error_abort);
>> +        sbd = SYS_BUS_DEVICE(&s->usart[i]);
>> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[USART_ADDR(i)].addr);
>> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[USART_RXC_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[USART_DRE_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[USART_TXC_IRQ(i)]);
>> +        connect_pr_irq(s, info, DEVICE(&s->usart[i]), 0);
>> +        g_free(devname);
>> +    }
>> +
>> +    /* Timer */
>> +    for (i = 0; i < info->timer_count; i++) {
>> +        int idx = TIMER_ADDR(i);
>> +        if (!info->dev[idx].is_timer16) {
>> +            create_unimplemented_device("avr-timer8",
>> +                                        OFFSET_DATA + info->dev[idx].addr, 7);
>> +            create_unimplemented_device("avr-timer8-intmask",
>> +                                        OFFSET_DATA
>> +                                        + info->dev[idx].intmask_addr, 1);
>> +            create_unimplemented_device("avr-timer8-intflag",
>> +                                        OFFSET_DATA
>> +                                        + info->dev[idx].intflag_addr, 1);
>> +            continue;
>> +        }
>> +        devname = g_strdup_printf("timer%zu", i);
>> +        object_initialize_child(OBJECT(dev), devname,
>> +                                &s->timer[i], sizeof(s->timer[i]),
>> +                                TYPE_AVR_TIMER16, &error_abort, NULL);
>> +        object_property_set_uint(OBJECT(&s->timer[i]), s->xtal_freq_hz,
>> +                                 "cpu-frequency-hz", &error_abort);
>> +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized",
>> +                                 &error_abort);
>> +        sbd = SYS_BUS_DEVICE(&s->timer[i]);
>> +        sysbus_mmio_map(sbd, 0, OFFSET_DATA + info->dev[idx].addr);
>> +        sysbus_mmio_map(sbd, 1, OFFSET_DATA + info->dev[idx].intmask_addr);
>> +        sysbus_mmio_map(sbd, 2, OFFSET_DATA + info->dev[idx].intflag_addr);
>> +        connect_nonnull_irq(sbd, cpudev, 0, info->irq[TIMER_CAPT_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 1, info->irq[TIMER_COMPA_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 2, info->irq[TIMER_COMPB_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 3, info->irq[TIMER_COMPC_IRQ(i)]);
>> +        connect_nonnull_irq(sbd, cpudev, 4, info->irq[TIMER_OVF_IRQ(i)]);
>> +        connect_pr_irq(s, info, DEVICE(&s->timer[i]), 0);
>> +        g_free(devname);
>> +    }
>> +}
>> +
>> +static Property atmega_props[] = {
>> +    DEFINE_PROP_UINT64("xtal-frequency-hz", AtmegaState,
>> +                       xtal_freq_hz, 0),
>> +    DEFINE_PROP_END_OF_LIST()
>> +};
>> +
>> +static void atmega_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    AtmegaClass *bc = ATMEGA_CLASS(oc);
>> +
>> +    bc->info = data;
>> +    dc->realize = atmega_realize;
>> +    dc->props = atmega_props;
>> +    /* Reason: Mapped at fixed location on the system bus */
>> +    dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo atmega_type_info = {
>> +    .name = TYPE_ATMEGA,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(AtmegaState),
>> +    .class_size = sizeof(AtmegaClass),
>> +    .abstract = true,
>> +};
>> +
>> +static void atmega_register_types(void)
>> +{
>> +    size_t i;
>> +
>> +    type_register_static(&atmega_type_info);
>> +    for (i = 0; i < ARRAY_SIZE(atmega_mcu); i++) {
>> +        assert(atmega_mcu[i].io_size <= 0x200);
>> +        assert(atmega_mcu[i].uart_count <= USART_MAX);
>> +        assert(atmega_mcu[i].timer_count <= TIMER_MAX);
>> +        TypeInfo ti = {
>> +            .name = atmega_mcu[i].uc_name,
>> +            .parent = TYPE_ATMEGA,
>> +            .class_init = atmega_class_init,
>> +            .class_data = (void *) &atmega_mcu[i],
>> +        };
>> +        type_register(&ti);
>> +    }
>> +}
>> +
>> +type_init(atmega_register_types)
>> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
>> index 626b7064b3..4b6b911820 100644
>> --- a/hw/avr/Makefile.objs
>> +++ b/hw/avr/Makefile.objs
>> @@ -1 +1,2 @@
>>   obj-y += sample.o
>> +obj-y += atmega.o
> 
> 

Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Igor Mammedov]

On Fri, 20 Dec 2019 13:58:29 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor,
> 
> On 12/20/19 11:09 AM, Igor Mammedov wrote:
> > On Thu, 28 Nov 2019 02:50:26 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> Add famous ATmega MCUs:
> >>
> >> - middle range: ATmega168 and ATmega328
> >> - high range: ATmega1280 and ATmega2560
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
[...]
> >> +static void atmega_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    AtmegaState *s = ATMEGA(dev);
> >> +    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
> >> +    const AtmegaInfo *info = bc->info;
> >> +    DeviceState *cpudev;
> >> +    SysBusDevice *sbd;
> >> +    Error *err = NULL;
> >> +    char *devname;
> >> +    size_t i;
> >> +
> >> +    if (!s->xtal_freq_hz) {
> >> +        error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
> >> +        return;
> >> +    }
> >> +
> >> +    /* CPU */
> >> +    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
> >> +                            info->cpu_type, &err, NULL);
> >> +    if (err) {
> >> +        error_propagate(errp, err);
> >> +        return;
> >> +    }
> >> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &error_abort);
> >> +    cpudev = DEVICE(&s->cpu);
> >> +
> >> +    /* SRAM */
> >> +    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
> >> +                                         "sram", info->sram_size);  
> > with main RAM conversion to hostmem backend, this API will go away
> > and RAM memory region will be allocated by generic machine code
> > and shall be treated as backend. Users would be able to access it
> > via MachineState::ram memory region.
> > 
> > Meanwhile I'd suggest to move this line to arduino_machine_init()
> > and pass it to MCU as a link property.  
> 
> Thanks for reviewing this patch.
> 
> I think this MCU and few ARM SoC are good case for your RAM conversion.
> 
> These chipsets provide onboard RAM (SRAM). This amount of SRAM is enough 
> to run u-boot, FreeRTOS, ... Some ARM boards add DRAM, usually larger 
> than the SRAM amount.
> 
> The previous recomendation was to use 
> memory_region_allocate_system_memory() only once, but on the biggest 
> chunk of memory, for performance reasons.
that makes sense only if flexibility is necessary (mem-path, binding to numa nodes, prealloc, ...)
Do we really  care about it in non virt usecases.
 
> In the previous cases, the RAM is not added by the board/machine, but is 
> present in the MCU/SoC. This looks incorrect to me to allocate the RAM 
> in the board/machine and pass it to the MCU/SoC.
If it's not user specified value but a fixed memory embedded in SoC,
I'd for simplicity use memory_region_init_ram() directly
(which some boards do for built-in sram).
 
> You are saying the machine/board code will have to ask its children how 
> many ram they need, then allocate, then pass it to each?
machine code will use -m value (and that defaults to default_ram_size,
which board can override) or user provided memdev.

On board side, one could check if user provided backend is suitable and
refuse to start asking to correct CLI error.

So we are still talking about single backend RAM blob, which boards
could use as is or partition with aliases (x86) or map to some other
front-end devices. (point is not to mix device model with backend in this case)


> > Also use MachineState::ram_size and add check that it matches mc->default_ram_size.
> > Ex: https://github.com/imammedo/qemu/commit/241c65d506ccba1e0239a2bc0632d7dc9c3517c1
> >   
> >> +    memory_region_add_subregion(get_system_memory(),
> >> +                                OFFSET_DATA + 0x200, &s->sram);
> >> +
> >> +    /* Flash */
> >> +    memory_region_init_rom(&s->flash, OBJECT(dev),
> >> +                           "flash", info->flash_size, &error_fatal);
> >> +    memory_region_add_subregion(get_system_memory(), OFFSET_CODE, &s->flash);
> >> +
[...]