[PATCH 0/2] hw/adc: Add basic Aspeed ADC model

[PATCH 0/2] hw/adc: Add basic Aspeed ADC model

[pdel]

From: Peter Delevoryas <pdel@fb.com>

Following up from

https://lore.kernel.org/qemu-devel/20210930004235.1656003-1-pdel@fb.com/

This is a resubmission of Andrew Jeffery's ADC model, but with the
registers converted from typed-member-fields to a regs[] array.
Otherwise, it should be pretty much equivalent.

References to the original patches from Andrew:

https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4
https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f

I did A/B "--trace aspeed_adc_engine_*" testing and the output from the
boot sequence was equivalent, and I tested that the channel simulation
code produces the same sequence of values.

Here's the initialization sequence:

aspeed_adc_engine_write engine[0] 0x0 0xf  ; Engine init
aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set
aspeed_adc_engine_write engine[0] 0x0 0x2f ; Auto compensating sensing mode
aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set (redoing engine init, lol), bit 5 reset

Here's some of the channel simulation:

(qemu) xp 0x1e6e9010
000000001e6e9010: 0x00070005
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x000e000a
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x0015000f
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x001c0014
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x00230019
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x002a001e
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x00310023
(qemu) xp 0x1e6e9010
000000001e6e9010: 0x00380028

This was tested with a Fuji OpenBMC image:

https://github.com/peterdelevoryas/openbmc/releases/download/fuji.mtd.0/fuji.mtd

My refactoring was kinda awkward, it took me a few tries to finally get
something that made sense to me. In particular, I don't know if you guys
realized this previously, but in the AST2600, there's 2 engines, but
still only 16 channels: each engine only has half the registers, as far
as I understand.

That's why I added the "nr-channels" property to the engine. I also
tried to add guest-error logs when a driver tries to read or write to
the upper-eight channel registers in the 2600 engines. For example:

(qemu) xp 0x1e6e9000
000000001e6e9000: 0xffff010f
(qemu) xp 0x1e6e9020
aspeed_adc_engine_read: engine[0]: data register 4 invalid, only 0...3 valid
000000001e6e9020: 0x00000000
(qemu) xp 0x1e6e9030
000000001e6e9030: 0x00000000
(qemu) xp 0x1e6e9040
000000001e6e9040: 0x00000000
(qemu) xp 0x1e6e9050
aspeed_adc_engine_read: engine[0]: bounds register 8 invalid, only 0...7 valid
000000001e6e9050: 0x00000000
(qemu) xp 0x1e6e9060
aspeed_adc_engine_read: engine[0]: bounds register 12 invalid, only 0...7 valid
000000001e6e9060: 0x00000000
(qemu) xp 0x1e6e9070
000000001e6e9070: 0x00000000
(qemu) xp 0x1e6e9080
000000001e6e9080: 0x00000000
(qemu) xp 0x1e6e9090
aspeed_adc_engine_read: engine[0]: hysteresis register 8 invalid, only 0...7 valid
000000001e6e9090: 0x00000000
(qemu) xp 0x1e6e90a0
aspeed_adc_engine_read: engine[0]: hysteresis register 12 invalid, only 0...7 valid
000000001e6e90a0: 0x00000000

It might turn out that we should just stick with a closer version of
Andrew's original patch, and that would be totally fine with me, I just
want to get the ADC support merged one way or another.

Also, I'm interested in resubmitting Andrew's work on supporting
unaligned accesses for models that only implement aligned access, to
make supporting 8-bit and 16-bit reads transparent to this model, but
I'll submit that separately.

Reference: https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/

Thanks,
Peter

Andrew Jeffery (2):
  hw/adc: Add basic Aspeed ADC model
  hw/arm: Integrate ADC model into Aspeed SoC

 hw/adc/aspeed_adc.c         | 416 ++++++++++++++++++++++++++++++++++++
 hw/adc/meson.build          |   1 +
 hw/adc/trace-events         |   3 +
 hw/arm/aspeed_ast2600.c     |  11 +
 hw/arm/aspeed_soc.c         |  11 +
 include/hw/adc/aspeed_adc.h |  55 +++++
 include/hw/arm/aspeed_soc.h |   2 +
 7 files changed, 499 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

-- 
2.30.2

[PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[pdel]

From: Andrew Jeffery <andrew@aj.id.au>

This model implements enough behaviour to do basic functionality tests
such as device initialisation and read out of dummy sample values. The
sample value generation strategy is similar to the STM ADC already in
the tree.

This patch was originally created by Andrew Jeffery, but I (Peter)
refactored it to use a regs[] array and added some extra guest-error
traces for the AST2600 multi-engine configuration.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
[clg : support for multiple engines (AST2600) ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/adc/aspeed_adc.c         | 417 ++++++++++++++++++++++++++++++++++++
 hw/adc/meson.build          |   1 +
 hw/adc/trace-events         |   3 +
 include/hw/adc/aspeed_adc.h |  55 +++++
 4 files changed, 476 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
new file mode 100644
index 0000000000..8a132ef375
--- /dev/null
+++ b/hw/adc/aspeed_adc.c
@@ -0,0 +1,417 @@
+/*
+ * Aspeed ADC
+ *
+ * Copyright 2017-2021 IBM Corp.
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "hw/adc/aspeed_adc.h"
+#include "trace.h"
+
+#define  ASPEED_ADC_ENGINE_CH_EN_MASK           0xffff0000
+#define   ASPEED_ADC_ENGINE_CH_EN(x)            ((BIT(x)) << 16)
+#define  ASPEED_ADC_ENGINE_INIT                 BIT(8)
+#define  ASPEED_ADC_ENGINE_AUTO_COMP            BIT(5)
+#define  ASPEED_ADC_ENGINE_COMP                 BIT(4)
+#define  ASPEED_ADC_ENGINE_MODE_MASK            0x0000000e
+#define   ASPEED_ADC_ENGINE_MODE_OFF            (0b000 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_STANDBY        (0b001 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_NORMAL         (0b111 << 1)
+#define  ASPEED_ADC_ENGINE_EN                   BIT(0)
+#define ASPEED_ADC_HYST_EN                      BIT(31)
+
+#define ASPEED_ADC_L_MASK       ((1 << 10) - 1)
+#define ASPEED_ADC_L(x)         ((x) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_H(x)         (((x) >> 16) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_LH_MASK      (ASPEED_ADC_L_MASK << 16 | ASPEED_ADC_L_MASK)
+#define LOWER_CHANNEL_MASK      ((1 << 10) - 1)
+#define LOWER_CHANNEL_DATA(x)   ((x) & LOWER_CHANNEL_MASK)
+#define UPPER_CHANNEL_DATA(x)   (((x) >> 16) & LOWER_CHANNEL_MASK)
+
+#define TO_REG(addr) (addr >> 2)
+
+#define ENGINE_CONTROL              TO_REG(0x00)
+#define INTERRUPT_CONTROL           TO_REG(0x04)
+#define VGA_DETECT_CONTROL          TO_REG(0x08)
+#define CLOCK_CONTROL               TO_REG(0x0C)
+#define DATA_CHANNEL_1_AND_0        TO_REG(0x10)
+#define DATA_CHANNEL_7_AND_6        TO_REG(0x1C)
+#define DATA_CHANNEL_9_AND_8        TO_REG(0x20)
+#define DATA_CHANNEL_15_AND_14      TO_REG(0x2C)
+#define BOUNDS_CHANNEL_0            TO_REG(0x30)
+#define BOUNDS_CHANNEL_7            TO_REG(0x4C)
+#define BOUNDS_CHANNEL_8            TO_REG(0x50)
+#define BOUNDS_CHANNEL_15           TO_REG(0x6C)
+#define HYSTERESIS_CHANNEL_0        TO_REG(0x70)
+#define HYSTERESIS_CHANNEL_7        TO_REG(0x8C)
+#define HYSTERESIS_CHANNEL_8        TO_REG(0x90)
+#define HYSTERESIS_CHANNEL_15       TO_REG(0xAC)
+#define INTERRUPT_SOURCE            TO_REG(0xC0)
+#define COMPENSATING_AND_TRIMMING   TO_REG(0xC4)
+
+static inline uint32_t update_channels(uint32_t current)
+{
+    return ((((current >> 16) & ASPEED_ADC_L_MASK) + 7) << 16) |
+        ((current + 5) & ASPEED_ADC_L_MASK);
+}
+
+static bool breaks_threshold(AspeedADCEngineState *s, int reg)
+{
+    assert(reg >= DATA_CHANNEL_1_AND_0 &&
+           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
+
+    int a_bounds_reg = BOUNDS_CHANNEL_0 + (reg - DATA_CHANNEL_1_AND_0) * 2;
+    int b_bounds_reg = a_bounds_reg + 1;
+    uint32_t a_and_b = s->regs[reg];
+    uint32_t a_bounds = s->regs[a_bounds_reg];
+    uint32_t b_bounds = s->regs[b_bounds_reg];
+    uint32_t a = ASPEED_ADC_L(a_and_b);
+    uint32_t b = ASPEED_ADC_H(a_and_b);
+    uint32_t a_lower = ASPEED_ADC_L(a_bounds);
+    uint32_t a_upper = ASPEED_ADC_H(a_bounds);
+    uint32_t b_lower = ASPEED_ADC_L(b_bounds);
+    uint32_t b_upper = ASPEED_ADC_H(b_bounds);
+
+    return (a < a_lower || a > a_upper) ||
+           (b < b_lower || b > b_upper);
+}
+
+static uint32_t read_channel_sample(AspeedADCEngineState *s, int reg)
+{
+    assert(reg >= DATA_CHANNEL_1_AND_0 &&
+           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
+
+    /* Poor man's sampling */
+    uint32_t value = s->regs[reg];
+    s->regs[reg] = update_channels(s->regs[reg]);
+
+    if (breaks_threshold(s, reg)) {
+        s->regs[INTERRUPT_CONTROL] |= BIT(reg - DATA_CHANNEL_1_AND_0);
+        qemu_irq_raise(s->irq);
+    }
+
+    return value;
+}
+
+static uint64_t aspeed_adc_engine_read(void *opaque, hwaddr addr,
+                                       unsigned int size)
+{
+    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
+    int reg = TO_REG(addr);
+    uint32_t value = 0;
+
+    switch (reg) {
+    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "bounds register %u invalid, only 0...7 valid\n",
+                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
+            break;
+        }
+        /* fallthrough */
+    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "hysteresis register %u invalid, only 0...7 valid\n",
+                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
+            break;
+        }
+        /* fallthrough */
+    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
+    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
+    case ENGINE_CONTROL:
+    case INTERRUPT_CONTROL:
+    case VGA_DETECT_CONTROL:
+    case CLOCK_CONTROL:
+    case INTERRUPT_SOURCE:
+    case COMPENSATING_AND_TRIMMING:
+        value = s->regs[reg];
+        break;
+    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "data register %u invalid, only 0...3 valid\n",
+                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
+            break;
+        }
+        /* fallthrough */
+    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
+        value = read_channel_sample(s, reg);
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: 0x%" HWADDR_PRIx "\n",
+                      __func__, s->engine_id, addr);
+        break;
+    }
+
+    trace_aspeed_adc_engine_read(s->engine_id, addr, value);
+    return value;
+}
+
+static void aspeed_adc_engine_write(void *opaque, hwaddr addr, uint64_t value,
+                                    unsigned int size)
+{
+    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
+    int reg = TO_REG(addr);
+    uint32_t init = 0;
+
+    trace_aspeed_adc_engine_write(s->engine_id, addr, value);
+
+    switch (reg) {
+    case ENGINE_CONTROL:
+        init = !!(value & ASPEED_ADC_ENGINE_EN);
+        init *= ASPEED_ADC_ENGINE_INIT;
+
+        value &= ~ASPEED_ADC_ENGINE_INIT;
+        value |= init;
+
+        value &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
+        break;
+    case INTERRUPT_CONTROL:
+    case VGA_DETECT_CONTROL:
+    case CLOCK_CONTROL:
+        break;
+    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "data register %u invalid, only 0...3 valid\n",
+                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
+            return;
+        }
+        /* fallthrough */
+    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "bounds register %u invalid, only 0...7 valid\n",
+                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
+            return;
+        }
+        /* fallthrough */
+    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
+    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
+        value &= ASPEED_ADC_LH_MASK;
+        break;
+    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
+        if (s->nr_channels <= 8) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
+                          "hysteresis register %u invalid, only 0...7 valid\n",
+                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
+            return;
+        }
+        /* fallthrough */
+    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
+        value &= (ASPEED_ADC_HYST_EN | ASPEED_ADC_LH_MASK);
+        break;
+    case INTERRUPT_SOURCE:
+        value &= 0xffff;
+        break;
+    case COMPENSATING_AND_TRIMMING:
+        value &= 0xf;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: "
+                      "0x%" HWADDR_PRIx " 0x%" PRIx64 "\n",
+                      __func__, s->engine_id, addr, value);
+        break;
+    }
+
+    s->regs[reg] = value;
+}
+
+static const MemoryRegionOps aspeed_adc_engine_ops = {
+    .read = aspeed_adc_engine_read,
+    .write = aspeed_adc_engine_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+        .unaligned = false,
+    },
+};
+
+static const uint32_t aspeed_adc_resets[ASPEED_ADC_NR_REGS] = {
+    [ENGINE_CONTROL]     = 0x00000000,
+    [INTERRUPT_CONTROL]  = 0x00000000,
+    [VGA_DETECT_CONTROL] = 0x0000000f,
+    [CLOCK_CONTROL]      = 0x0000000f,
+};
+
+static void aspeed_adc_engine_reset(DeviceState *dev)
+{
+    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
+
+    memcpy(s->regs, aspeed_adc_resets, sizeof(aspeed_adc_resets));
+}
+
+static void aspeed_adc_engine_realize(DeviceState *dev, Error **errp)
+{
+    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    g_autofree char *name = g_strdup_printf(TYPE_ASPEED_ADC_ENGINE ".%d",
+                                            s->engine_id);
+
+    assert(s->engine_id < 2);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_adc_engine_ops, s, name,
+                          0x100);
+
+    sysbus_init_mmio(sbd, &s->mmio);
+}
+
+static const VMStateDescription vmstate_aspeed_adc_engine = {
+    .name = TYPE_ASPEED_ADC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedADCEngineState, ASPEED_ADC_NR_REGS),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+static Property aspeed_adc_engine_properties[] = {
+    DEFINE_PROP_UINT32("engine-id", AspeedADCEngineState, engine_id, 0),
+    DEFINE_PROP_UINT32("nr-channels", AspeedADCEngineState, nr_channels, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_adc_engine_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_adc_engine_realize;
+    dc->reset = aspeed_adc_engine_reset;
+    device_class_set_props(dc, aspeed_adc_engine_properties);
+    dc->desc = "Aspeed Analog-to-Digital Engine";
+    dc->vmsd = &vmstate_aspeed_adc_engine;
+}
+
+static const TypeInfo aspeed_adc_engine_info = {
+    .name = TYPE_ASPEED_ADC_ENGINE,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedADCEngineState),
+    .class_init = aspeed_adc_engine_class_init,
+};
+
+static void aspeed_adc_instance_init(Object *obj)
+{
+    AspeedADCState *s = ASPEED_ADC(obj);
+    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
+    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
+
+    for (int i = 0; i < aac->nr_engines; i++) {
+        AspeedADCEngineState *engine = &s->engines[i];
+        object_initialize_child(obj, "engine[*]", engine,
+                                TYPE_ASPEED_ADC_ENGINE);
+        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);
+        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
+    }
+}
+
+static void aspeed_adc_set_irq(void *opaque, int n, int level)
+{
+    AspeedADCState *s = opaque;
+    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
+    uint32_t pending = 0;
+
+    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
+    for (int i = 0; i < aac->nr_engines; i++) {
+        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
+        pending |= irq_status << (i * 8);
+    }
+
+    qemu_set_irq(s->irq, !!pending);
+}
+
+static void aspeed_adc_realize(DeviceState *dev, Error **errp)
+{
+    AspeedADCState *s = ASPEED_ADC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
+
+    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
+                                        s, NULL, aac->nr_engines);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
+
+    sysbus_init_mmio(sbd, &s->mmio);
+
+    for (int i = 0; i < aac->nr_engines; i++) {
+        Object *eng = OBJECT(&s->engines[i]);
+
+        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
+            return;
+        }
+        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
+                           qdev_get_gpio_in(DEVICE(sbd), i));
+        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);
+    }
+}
+
+static void aspeed_adc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
+
+    dc->realize = aspeed_adc_realize;
+    dc->desc = "Aspeed Analog-to-Digital Converter";
+    aac->nr_engines = 1;
+}
+
+static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
+
+    dc->desc = "ASPEED 2600 ADC Controller";
+    aac->nr_engines = 2;
+}
+
+static const TypeInfo aspeed_adc_info = {
+    .name = TYPE_ASPEED_ADC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = aspeed_adc_instance_init,
+    .instance_size = sizeof(AspeedADCState),
+    .class_init = aspeed_adc_class_init,
+    .class_size = sizeof(AspeedADCClass),
+    .abstract   = true,
+};
+
+static const TypeInfo aspeed_2400_adc_info = {
+    .name = TYPE_ASPEED_2400_ADC,
+    .parent = TYPE_ASPEED_ADC,
+};
+
+static const TypeInfo aspeed_2500_adc_info = {
+    .name = TYPE_ASPEED_2500_ADC,
+    .parent = TYPE_ASPEED_ADC,
+};
+
+static const TypeInfo aspeed_2600_adc_info = {
+    .name = TYPE_ASPEED_2600_ADC,
+    .parent = TYPE_ASPEED_ADC,
+    .class_init = aspeed_2600_adc_class_init,
+};
+
+static void aspeed_adc_register_types(void)
+{
+    type_register_static(&aspeed_adc_engine_info);
+    type_register_static(&aspeed_adc_info);
+    type_register_static(&aspeed_2400_adc_info);
+    type_register_static(&aspeed_2500_adc_info);
+    type_register_static(&aspeed_2600_adc_info);
+}
+
+type_init(aspeed_adc_register_types);
diff --git a/hw/adc/meson.build b/hw/adc/meson.build
index ac4f093fea..b29ac7ccdf 100644
--- a/hw/adc/meson.build
+++ b/hw/adc/meson.build
@@ -1,4 +1,5 @@
 softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
+softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
 softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
 softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
 softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
diff --git a/hw/adc/trace-events b/hw/adc/trace-events
index 456f21c8f4..5a4c444d77 100644
--- a/hw/adc/trace-events
+++ b/hw/adc/trace-events
@@ -3,3 +3,6 @@
 # npcm7xx_adc.c
 npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
 npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
+
+aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
+aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
new file mode 100644
index 0000000000..2f166e8be1
--- /dev/null
+++ b/include/hw/adc/aspeed_adc.h
@@ -0,0 +1,55 @@
+/*
+ * Aspeed ADC
+ *
+ * Copyright 2017-2021 IBM Corp.
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_ADC_ASPEED_ADC_H
+#define HW_ADC_ASPEED_ADC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_ADC "aspeed.adc"
+#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
+#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
+#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
+OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
+
+#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
+
+#define ASPEED_ADC_NR_CHANNELS 16
+#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
+
+struct AspeedADCEngineState {
+    /* <private> */
+    SysBusDevice parent;
+
+    MemoryRegion mmio;
+    qemu_irq irq;
+    uint32_t engine_id;
+    uint32_t nr_channels;
+    uint32_t regs[ASPEED_ADC_NR_REGS];
+};
+
+struct AspeedADCState {
+    /* <private> */
+    SysBusDevice parent;
+
+    MemoryRegion mmio;
+    qemu_irq irq;
+
+    AspeedADCEngineState engines[2];
+};
+
+struct AspeedADCClass {
+    SysBusDeviceClass parent_class;
+
+    uint32_t nr_engines;
+};
+
+#endif /* HW_ADC_ASPEED_ADC_H */
-- 
2.30.2

Re: [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Cédric Le Goater]

Hello Peter

( Please fix the From: pdel@fbc.om :)

On 10/2/21 23:44, pdel@fbc.om wrote:
> From: Andrew Jeffery <andrew@aj.id.au>
> 
> This model implements enough behaviour to do basic functionality tests
> such as device initialisation and read out of dummy sample values. The
> sample value generation strategy is similar to the STM ADC already in
> the tree.
> 
> This patch was originally created by Andrew Jeffery, but I (Peter)
> refactored it to use a regs[] array and added some extra guest-error
> traces for the AST2600 multi-engine configuration.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> [clg : support for multiple engines (AST2600) ]
> Signed-off-by: Cédric Le Goater <clg@kaod.

Could you please add a summary of all the changes you did just before
the s-o-b.

> Signed-off-by: Peter Delevoryas <pdel@fb.com>

Looks good to me. One comment below,


> ---
>   hw/adc/aspeed_adc.c         | 417 ++++++++++++++++++++++++++++++++++++
>   hw/adc/meson.build          |   1 +
>   hw/adc/trace-events         |   3 +
>   include/hw/adc/aspeed_adc.h |  55 +++++
>   4 files changed, 476 insertions(+)
>   create mode 100644 hw/adc/aspeed_adc.c
>   create mode 100644 include/hw/adc/aspeed_adc.h
> 
> diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
> new file mode 100644
> index 0000000000..8a132ef375
> --- /dev/null
> +++ b/hw/adc/aspeed_adc.c
> @@ -0,0 +1,417 @@
> +/*
> + * Aspeed ADC
> + *
> + * Copyright 2017-2021 IBM Corp.
> + *
> + * Andrew Jeffery <andrew@aj.id.au>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "hw/adc/aspeed_adc.h"
> +#include "trace.h"
> +
> +#define  ASPEED_ADC_ENGINE_CH_EN_MASK           0xffff0000
> +#define   ASPEED_ADC_ENGINE_CH_EN(x)            ((BIT(x)) << 16)
> +#define  ASPEED_ADC_ENGINE_INIT                 BIT(8)
> +#define  ASPEED_ADC_ENGINE_AUTO_COMP            BIT(5)
> +#define  ASPEED_ADC_ENGINE_COMP                 BIT(4)
> +#define  ASPEED_ADC_ENGINE_MODE_MASK            0x0000000e
> +#define   ASPEED_ADC_ENGINE_MODE_OFF            (0b000 << 1)
> +#define   ASPEED_ADC_ENGINE_MODE_STANDBY        (0b001 << 1)
> +#define   ASPEED_ADC_ENGINE_MODE_NORMAL         (0b111 << 1)
> +#define  ASPEED_ADC_ENGINE_EN                   BIT(0)
> +#define ASPEED_ADC_HYST_EN                      BIT(31)
> +
> +#define ASPEED_ADC_L_MASK       ((1 << 10) - 1)
> +#define ASPEED_ADC_L(x)         ((x) & ASPEED_ADC_L_MASK)
> +#define ASPEED_ADC_H(x)         (((x) >> 16) & ASPEED_ADC_L_MASK)
> +#define ASPEED_ADC_LH_MASK      (ASPEED_ADC_L_MASK << 16 | ASPEED_ADC_L_MASK)
> +#define LOWER_CHANNEL_MASK      ((1 << 10) - 1)
> +#define LOWER_CHANNEL_DATA(x)   ((x) & LOWER_CHANNEL_MASK)
> +#define UPPER_CHANNEL_DATA(x)   (((x) >> 16) & LOWER_CHANNEL_MASK)
> +
> +#define TO_REG(addr) (addr >> 2)
> +
> +#define ENGINE_CONTROL              TO_REG(0x00)
> +#define INTERRUPT_CONTROL           TO_REG(0x04)
> +#define VGA_DETECT_CONTROL          TO_REG(0x08)
> +#define CLOCK_CONTROL               TO_REG(0x0C)
> +#define DATA_CHANNEL_1_AND_0        TO_REG(0x10)
> +#define DATA_CHANNEL_7_AND_6        TO_REG(0x1C)
> +#define DATA_CHANNEL_9_AND_8        TO_REG(0x20)
> +#define DATA_CHANNEL_15_AND_14      TO_REG(0x2C)
> +#define BOUNDS_CHANNEL_0            TO_REG(0x30)
> +#define BOUNDS_CHANNEL_7            TO_REG(0x4C)
> +#define BOUNDS_CHANNEL_8            TO_REG(0x50)
> +#define BOUNDS_CHANNEL_15           TO_REG(0x6C)
> +#define HYSTERESIS_CHANNEL_0        TO_REG(0x70)
> +#define HYSTERESIS_CHANNEL_7        TO_REG(0x8C)
> +#define HYSTERESIS_CHANNEL_8        TO_REG(0x90)
> +#define HYSTERESIS_CHANNEL_15       TO_REG(0xAC)
> +#define INTERRUPT_SOURCE            TO_REG(0xC0)
> +#define COMPENSATING_AND_TRIMMING   TO_REG(0xC4)
> +
> +static inline uint32_t update_channels(uint32_t current)
> +{
> +    return ((((current >> 16) & ASPEED_ADC_L_MASK) + 7) << 16) |
> +        ((current + 5) & ASPEED_ADC_L_MASK);
> +}
> +
> +static bool breaks_threshold(AspeedADCEngineState *s, int reg)
> +{
> +    assert(reg >= DATA_CHANNEL_1_AND_0 &&
> +           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
> +
> +    int a_bounds_reg = BOUNDS_CHANNEL_0 + (reg - DATA_CHANNEL_1_AND_0) * 2;
> +    int b_bounds_reg = a_bounds_reg + 1;
> +    uint32_t a_and_b = s->regs[reg];
> +    uint32_t a_bounds = s->regs[a_bounds_reg];
> +    uint32_t b_bounds = s->regs[b_bounds_reg];
> +    uint32_t a = ASPEED_ADC_L(a_and_b);
> +    uint32_t b = ASPEED_ADC_H(a_and_b);
> +    uint32_t a_lower = ASPEED_ADC_L(a_bounds);
> +    uint32_t a_upper = ASPEED_ADC_H(a_bounds);
> +    uint32_t b_lower = ASPEED_ADC_L(b_bounds);
> +    uint32_t b_upper = ASPEED_ADC_H(b_bounds);
> +
> +    return (a < a_lower || a > a_upper) ||
> +           (b < b_lower || b > b_upper);
> +}
> +
> +static uint32_t read_channel_sample(AspeedADCEngineState *s, int reg)
> +{
> +    assert(reg >= DATA_CHANNEL_1_AND_0 &&
> +           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
> +
> +    /* Poor man's sampling */
> +    uint32_t value = s->regs[reg];
> +    s->regs[reg] = update_channels(s->regs[reg]);
> +
> +    if (breaks_threshold(s, reg)) {
> +        s->regs[INTERRUPT_CONTROL] |= BIT(reg - DATA_CHANNEL_1_AND_0);
> +        qemu_irq_raise(s->irq);
> +    }
> +
> +    return value;
> +}
> +
> +static uint64_t aspeed_adc_engine_read(void *opaque, hwaddr addr,
> +                                       unsigned int size)
> +{
> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
> +    int reg = TO_REG(addr);
> +    uint32_t value = 0;
> +
> +    switch (reg) {
> +    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "bounds register %u invalid, only 0...7 valid\n",
> +                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
> +            break;
> +        }
> +        /* fallthrough */
> +    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "hysteresis register %u invalid, only 0...7 valid\n",
> +                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
> +            break;
> +        }
> +        /* fallthrough */
> +    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
> +    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
> +    case ENGINE_CONTROL:
> +    case INTERRUPT_CONTROL:
> +    case VGA_DETECT_CONTROL:
> +    case CLOCK_CONTROL:
> +    case INTERRUPT_SOURCE:
> +    case COMPENSATING_AND_TRIMMING:
> +        value = s->regs[reg];
> +        break;
> +    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "data register %u invalid, only 0...3 valid\n",
> +                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
> +            break;
> +        }
> +        /* fallthrough */
> +    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
> +        value = read_channel_sample(s, reg);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: 0x%" HWADDR_PRIx "\n",
> +                      __func__, s->engine_id, addr);
> +        break;
> +    }
> +
> +    trace_aspeed_adc_engine_read(s->engine_id, addr, value);
> +    return value;
> +}
> +
> +static void aspeed_adc_engine_write(void *opaque, hwaddr addr, uint64_t value,
> +                                    unsigned int size)
> +{
> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
> +    int reg = TO_REG(addr);
> +    uint32_t init = 0;
> +
> +    trace_aspeed_adc_engine_write(s->engine_id, addr, value);
> +
> +    switch (reg) {
> +    case ENGINE_CONTROL:
> +        init = !!(value & ASPEED_ADC_ENGINE_EN);
> +        init *= ASPEED_ADC_ENGINE_INIT;
> +
> +        value &= ~ASPEED_ADC_ENGINE_INIT;
> +        value |= init;
> +
> +        value &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
> +        break;
> +    case INTERRUPT_CONTROL:
> +    case VGA_DETECT_CONTROL:
> +    case CLOCK_CONTROL:
> +        break;
> +    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "data register %u invalid, only 0...3 valid\n",
> +                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
> +            return;
> +        }
> +        /* fallthrough */
> +    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "bounds register %u invalid, only 0...7 valid\n",
> +                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
> +            return;
> +        }
> +        /* fallthrough */
> +    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
> +    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
> +        value &= ASPEED_ADC_LH_MASK;
> +        break;
> +    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
> +        if (s->nr_channels <= 8) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
> +                          "hysteresis register %u invalid, only 0...7 valid\n",
> +                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
> +            return;
> +        }
> +        /* fallthrough */
> +    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
> +        value &= (ASPEED_ADC_HYST_EN | ASPEED_ADC_LH_MASK);
> +        break;
> +    case INTERRUPT_SOURCE:
> +        value &= 0xffff;
> +        break;
> +    case COMPENSATING_AND_TRIMMING:
> +        value &= 0xf;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: "
> +                      "0x%" HWADDR_PRIx " 0x%" PRIx64 "\n",
> +                      __func__, s->engine_id, addr, value);
> +        break;
> +    }
> +
> +    s->regs[reg] = value;
> +}
> +
> +static const MemoryRegionOps aspeed_adc_engine_ops = {
> +    .read = aspeed_adc_engine_read,
> +    .write = aspeed_adc_engine_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +        .unaligned = false,
> +    },
> +};
> +
> +static const uint32_t aspeed_adc_resets[ASPEED_ADC_NR_REGS] = {
> +    [ENGINE_CONTROL]     = 0x00000000,
> +    [INTERRUPT_CONTROL]  = 0x00000000,
> +    [VGA_DETECT_CONTROL] = 0x0000000f,
> +    [CLOCK_CONTROL]      = 0x0000000f,
> +};
> +
> +static void aspeed_adc_engine_reset(DeviceState *dev)
> +{
> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
> +
> +    memcpy(s->regs, aspeed_adc_resets, sizeof(aspeed_adc_resets));
> +}
> +
> +static void aspeed_adc_engine_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    g_autofree char *name = g_strdup_printf(TYPE_ASPEED_ADC_ENGINE ".%d",
> +                                            s->engine_id);
> +
> +    assert(s->engine_id < 2);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_adc_engine_ops, s, name,
> +                          0x100);
> +
> +    sysbus_init_mmio(sbd, &s->mmio);
> +}
> +
> +static const VMStateDescription vmstate_aspeed_adc_engine = {
> +    .name = TYPE_ASPEED_ADC,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, AspeedADCEngineState, ASPEED_ADC_NR_REGS),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static Property aspeed_adc_engine_properties[] = {
> +    DEFINE_PROP_UINT32("engine-id", AspeedADCEngineState, engine_id, 0),
> +    DEFINE_PROP_UINT32("nr-channels", AspeedADCEngineState, nr_channels, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void aspeed_adc_engine_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = aspeed_adc_engine_realize;
> +    dc->reset = aspeed_adc_engine_reset;
> +    device_class_set_props(dc, aspeed_adc_engine_properties);
> +    dc->desc = "Aspeed Analog-to-Digital Engine";
> +    dc->vmsd = &vmstate_aspeed_adc_engine;
> +}
> +
> +static const TypeInfo aspeed_adc_engine_info = {
> +    .name = TYPE_ASPEED_ADC_ENGINE,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AspeedADCEngineState),
> +    .class_init = aspeed_adc_engine_class_init,
> +};
> +
> +static void aspeed_adc_instance_init(Object *obj)
> +{
> +    AspeedADCState *s = ASPEED_ADC(obj);
> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
> +    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
> +
> +    for (int i = 0; i < aac->nr_engines; i++) {
> +        AspeedADCEngineState *engine = &s->engines[i];
> +        object_initialize_child(obj, "engine[*]", engine,
> +                                TYPE_ASPEED_ADC_ENGINE);
> +        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);

Why have you moved the property initialization in the instance_init routine ?

Thanks,

C.

> +        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
> +    }
> +}
> +
> +static void aspeed_adc_set_irq(void *opaque, int n, int level)
> +{
> +    AspeedADCState *s = opaque;
> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
> +    uint32_t pending = 0;
> +
> +    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
> +    for (int i = 0; i < aac->nr_engines; i++) {
> +        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
> +        pending |= irq_status << (i * 8);
> +    }
> +
> +    qemu_set_irq(s->irq, !!pending);
> +}
> +
> +static void aspeed_adc_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedADCState *s = ASPEED_ADC(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
> +
> +    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
> +                                        s, NULL, aac->nr_engines);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
> +
> +    sysbus_init_mmio(sbd, &s->mmio);
> +
> +    for (int i = 0; i < aac->nr_engines; i++) {
> +        Object *eng = OBJECT(&s->engines[i]);
> +
> +        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
> +            return;
> +        }
> +        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
> +                           qdev_get_gpio_in(DEVICE(sbd), i));
> +        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);
> +    }
> +}
> +
> +static void aspeed_adc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
> +
> +    dc->realize = aspeed_adc_realize;
> +    dc->desc = "Aspeed Analog-to-Digital Converter";
> +    aac->nr_engines = 1;
> +}
> +
> +static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
> +
> +    dc->desc = "ASPEED 2600 ADC Controller";
> +    aac->nr_engines = 2;
> +}
> +
> +static const TypeInfo aspeed_adc_info = {
> +    .name = TYPE_ASPEED_ADC,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_init = aspeed_adc_instance_init,
> +    .instance_size = sizeof(AspeedADCState),
> +    .class_init = aspeed_adc_class_init,
> +    .class_size = sizeof(AspeedADCClass),
> +    .abstract   = true,
> +};
> +
> +static const TypeInfo aspeed_2400_adc_info = {
> +    .name = TYPE_ASPEED_2400_ADC,
> +    .parent = TYPE_ASPEED_ADC,
> +};
> +
> +static const TypeInfo aspeed_2500_adc_info = {
> +    .name = TYPE_ASPEED_2500_ADC,
> +    .parent = TYPE_ASPEED_ADC,
> +};
> +
> +static const TypeInfo aspeed_2600_adc_info = {
> +    .name = TYPE_ASPEED_2600_ADC,
> +    .parent = TYPE_ASPEED_ADC,
> +    .class_init = aspeed_2600_adc_class_init,
> +};
> +
> +static void aspeed_adc_register_types(void)
> +{
> +    type_register_static(&aspeed_adc_engine_info);
> +    type_register_static(&aspeed_adc_info);
> +    type_register_static(&aspeed_2400_adc_info);
> +    type_register_static(&aspeed_2500_adc_info);
> +    type_register_static(&aspeed_2600_adc_info);
> +}
> +
> +type_init(aspeed_adc_register_types);
> diff --git a/hw/adc/meson.build b/hw/adc/meson.build
> index ac4f093fea..b29ac7ccdf 100644
> --- a/hw/adc/meson.build
> +++ b/hw/adc/meson.build
> @@ -1,4 +1,5 @@
>   softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
>   softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
>   softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
>   softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
> diff --git a/hw/adc/trace-events b/hw/adc/trace-events
> index 456f21c8f4..5a4c444d77 100644
> --- a/hw/adc/trace-events
> +++ b/hw/adc/trace-events
> @@ -3,3 +3,6 @@
>   # npcm7xx_adc.c
>   npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>   npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
> +
> +aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
> +aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
> diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
> new file mode 100644
> index 0000000000..2f166e8be1
> --- /dev/null
> +++ b/include/hw/adc/aspeed_adc.h
> @@ -0,0 +1,55 @@
> +/*
> + * Aspeed ADC
> + *
> + * Copyright 2017-2021 IBM Corp.
> + *
> + * Andrew Jeffery <andrew@aj.id.au>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_ADC_ASPEED_ADC_H
> +#define HW_ADC_ASPEED_ADC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_ADC "aspeed.adc"
> +#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
> +#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
> +#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
> +OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
> +
> +#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
> +
> +#define ASPEED_ADC_NR_CHANNELS 16
> +#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
> +
> +struct AspeedADCEngineState {
> +    /* <private> */
> +    SysBusDevice parent;
> +
> +    MemoryRegion mmio;
> +    qemu_irq irq;
> +    uint32_t engine_id;
> +    uint32_t nr_channels;
> +    uint32_t regs[ASPEED_ADC_NR_REGS];
> +};
> +
> +struct AspeedADCState {
> +    /* <private> */
> +    SysBusDevice parent;
> +
> +    MemoryRegion mmio;
> +    qemu_irq irq;
> +
> +    AspeedADCEngineState engines[2];
> +};
> +
> +struct AspeedADCClass {
> +    SysBusDeviceClass parent_class;
> +
> +    uint32_t nr_engines;
> +};
> +
> +#endif /* HW_ADC_ASPEED_ADC_H */
> 

Re: [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Peter Delevoryas]

> On Oct 3, 2021, at 6:53 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello Peter
> 
> ( Please fix the From: pdel@fbc.om :)

Oh dang it, yes I’ll fix that.

> 
> On 10/2/21 23:44, pdel@fbc.om wrote:
>> From: Andrew Jeffery <andrew@aj.id.au>
>> This model implements enough behaviour to do basic functionality tests
>> such as device initialisation and read out of dummy sample values. The
>> sample value generation strategy is similar to the STM ADC already in
>> the tree.
>> This patch was originally created by Andrew Jeffery, but I (Peter)
>> refactored it to use a regs[] array and added some extra guest-error
>> traces for the AST2600 multi-engine configuration.
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> [clg : support for multiple engines (AST2600) ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.
> 
> Could you please add a summary of all the changes you did just before
> the s-o-b.

Ohh yes, yeah I’ll add:

[pdel : refactored engine register struct fields to regs[] array field]
[pdel : added guest-error checking for upper-8 channel regs in AST2600]

> 
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> 
> Looks good to me. One comment below,
> 
> 
>> ---
>>  hw/adc/aspeed_adc.c         | 417 ++++++++++++++++++++++++++++++++++++
>>  hw/adc/meson.build          |   1 +
>>  hw/adc/trace-events         |   3 +
>>  include/hw/adc/aspeed_adc.h |  55 +++++
>>  4 files changed, 476 insertions(+)
>>  create mode 100644 hw/adc/aspeed_adc.c
>>  create mode 100644 include/hw/adc/aspeed_adc.h
>> diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
>> new file mode 100644
>> index 0000000000..8a132ef375
>> --- /dev/null
>> +++ b/hw/adc/aspeed_adc.c
>> @@ -0,0 +1,417 @@
>> +/*
>> + * Aspeed ADC
>> + *
>> + * Copyright 2017-2021 IBM Corp.
>> + *
>> + * Andrew Jeffery <andrew@aj.id.au>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/log.h"
>> +#include "hw/irq.h"
>> +#include "hw/qdev-properties.h"
>> +#include "migration/vmstate.h"
>> +#include "hw/adc/aspeed_adc.h"
>> +#include "trace.h"
>> +
>> +#define  ASPEED_ADC_ENGINE_CH_EN_MASK           0xffff0000
>> +#define   ASPEED_ADC_ENGINE_CH_EN(x)            ((BIT(x)) << 16)
>> +#define  ASPEED_ADC_ENGINE_INIT                 BIT(8)
>> +#define  ASPEED_ADC_ENGINE_AUTO_COMP            BIT(5)
>> +#define  ASPEED_ADC_ENGINE_COMP                 BIT(4)
>> +#define  ASPEED_ADC_ENGINE_MODE_MASK            0x0000000e
>> +#define   ASPEED_ADC_ENGINE_MODE_OFF            (0b000 << 1)
>> +#define   ASPEED_ADC_ENGINE_MODE_STANDBY        (0b001 << 1)
>> +#define   ASPEED_ADC_ENGINE_MODE_NORMAL         (0b111 << 1)
>> +#define  ASPEED_ADC_ENGINE_EN                   BIT(0)
>> +#define ASPEED_ADC_HYST_EN                      BIT(31)
>> +
>> +#define ASPEED_ADC_L_MASK       ((1 << 10) - 1)
>> +#define ASPEED_ADC_L(x)         ((x) & ASPEED_ADC_L_MASK)
>> +#define ASPEED_ADC_H(x)         (((x) >> 16) & ASPEED_ADC_L_MASK)
>> +#define ASPEED_ADC_LH_MASK      (ASPEED_ADC_L_MASK << 16 | ASPEED_ADC_L_MASK)
>> +#define LOWER_CHANNEL_MASK      ((1 << 10) - 1)
>> +#define LOWER_CHANNEL_DATA(x)   ((x) & LOWER_CHANNEL_MASK)
>> +#define UPPER_CHANNEL_DATA(x)   (((x) >> 16) & LOWER_CHANNEL_MASK)
>> +
>> +#define TO_REG(addr) (addr >> 2)
>> +
>> +#define ENGINE_CONTROL              TO_REG(0x00)
>> +#define INTERRUPT_CONTROL           TO_REG(0x04)
>> +#define VGA_DETECT_CONTROL          TO_REG(0x08)
>> +#define CLOCK_CONTROL               TO_REG(0x0C)
>> +#define DATA_CHANNEL_1_AND_0        TO_REG(0x10)
>> +#define DATA_CHANNEL_7_AND_6        TO_REG(0x1C)
>> +#define DATA_CHANNEL_9_AND_8        TO_REG(0x20)
>> +#define DATA_CHANNEL_15_AND_14      TO_REG(0x2C)
>> +#define BOUNDS_CHANNEL_0            TO_REG(0x30)
>> +#define BOUNDS_CHANNEL_7            TO_REG(0x4C)
>> +#define BOUNDS_CHANNEL_8            TO_REG(0x50)
>> +#define BOUNDS_CHANNEL_15           TO_REG(0x6C)
>> +#define HYSTERESIS_CHANNEL_0        TO_REG(0x70)
>> +#define HYSTERESIS_CHANNEL_7        TO_REG(0x8C)
>> +#define HYSTERESIS_CHANNEL_8        TO_REG(0x90)
>> +#define HYSTERESIS_CHANNEL_15       TO_REG(0xAC)
>> +#define INTERRUPT_SOURCE            TO_REG(0xC0)
>> +#define COMPENSATING_AND_TRIMMING   TO_REG(0xC4)
>> +
>> +static inline uint32_t update_channels(uint32_t current)
>> +{
>> +    return ((((current >> 16) & ASPEED_ADC_L_MASK) + 7) << 16) |
>> +        ((current + 5) & ASPEED_ADC_L_MASK);
>> +}
>> +
>> +static bool breaks_threshold(AspeedADCEngineState *s, int reg)
>> +{
>> +    assert(reg >= DATA_CHANNEL_1_AND_0 &&
>> +           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
>> +
>> +    int a_bounds_reg = BOUNDS_CHANNEL_0 + (reg - DATA_CHANNEL_1_AND_0) * 2;
>> +    int b_bounds_reg = a_bounds_reg + 1;
>> +    uint32_t a_and_b = s->regs[reg];
>> +    uint32_t a_bounds = s->regs[a_bounds_reg];
>> +    uint32_t b_bounds = s->regs[b_bounds_reg];
>> +    uint32_t a = ASPEED_ADC_L(a_and_b);
>> +    uint32_t b = ASPEED_ADC_H(a_and_b);
>> +    uint32_t a_lower = ASPEED_ADC_L(a_bounds);
>> +    uint32_t a_upper = ASPEED_ADC_H(a_bounds);
>> +    uint32_t b_lower = ASPEED_ADC_L(b_bounds);
>> +    uint32_t b_upper = ASPEED_ADC_H(b_bounds);
>> +
>> +    return (a < a_lower || a > a_upper) ||
>> +           (b < b_lower || b > b_upper);
>> +}
>> +
>> +static uint32_t read_channel_sample(AspeedADCEngineState *s, int reg)
>> +{
>> +    assert(reg >= DATA_CHANNEL_1_AND_0 &&
>> +           reg < DATA_CHANNEL_1_AND_0 + s->nr_channels / 2);
>> +
>> +    /* Poor man's sampling */
>> +    uint32_t value = s->regs[reg];
>> +    s->regs[reg] = update_channels(s->regs[reg]);
>> +
>> +    if (breaks_threshold(s, reg)) {
>> +        s->regs[INTERRUPT_CONTROL] |= BIT(reg - DATA_CHANNEL_1_AND_0);
>> +        qemu_irq_raise(s->irq);
>> +    }
>> +
>> +    return value;
>> +}
>> +
>> +static uint64_t aspeed_adc_engine_read(void *opaque, hwaddr addr,
>> +                                       unsigned int size)
>> +{
>> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
>> +    int reg = TO_REG(addr);
>> +    uint32_t value = 0;
>> +
>> +    switch (reg) {
>> +    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "bounds register %u invalid, only 0...7 valid\n",
>> +                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
>> +            break;
>> +        }
>> +        /* fallthrough */
>> +    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "hysteresis register %u invalid, only 0...7 valid\n",
>> +                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
>> +            break;
>> +        }
>> +        /* fallthrough */
>> +    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
>> +    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
>> +    case ENGINE_CONTROL:
>> +    case INTERRUPT_CONTROL:
>> +    case VGA_DETECT_CONTROL:
>> +    case CLOCK_CONTROL:
>> +    case INTERRUPT_SOURCE:
>> +    case COMPENSATING_AND_TRIMMING:
>> +        value = s->regs[reg];
>> +        break;
>> +    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "data register %u invalid, only 0...3 valid\n",
>> +                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
>> +            break;
>> +        }
>> +        /* fallthrough */
>> +    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
>> +        value = read_channel_sample(s, reg);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: 0x%" HWADDR_PRIx "\n",
>> +                      __func__, s->engine_id, addr);
>> +        break;
>> +    }
>> +
>> +    trace_aspeed_adc_engine_read(s->engine_id, addr, value);
>> +    return value;
>> +}
>> +
>> +static void aspeed_adc_engine_write(void *opaque, hwaddr addr, uint64_t value,
>> +                                    unsigned int size)
>> +{
>> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(opaque);
>> +    int reg = TO_REG(addr);
>> +    uint32_t init = 0;
>> +
>> +    trace_aspeed_adc_engine_write(s->engine_id, addr, value);
>> +
>> +    switch (reg) {
>> +    case ENGINE_CONTROL:
>> +        init = !!(value & ASPEED_ADC_ENGINE_EN);
>> +        init *= ASPEED_ADC_ENGINE_INIT;
>> +
>> +        value &= ~ASPEED_ADC_ENGINE_INIT;
>> +        value |= init;
>> +
>> +        value &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
>> +        break;
>> +    case INTERRUPT_CONTROL:
>> +    case VGA_DETECT_CONTROL:
>> +    case CLOCK_CONTROL:
>> +        break;
>> +    case DATA_CHANNEL_9_AND_8 ... DATA_CHANNEL_15_AND_14:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "data register %u invalid, only 0...3 valid\n",
>> +                          __func__, s->engine_id, reg - DATA_CHANNEL_1_AND_0);
>> +            return;
>> +        }
>> +        /* fallthrough */
>> +    case BOUNDS_CHANNEL_8 ... BOUNDS_CHANNEL_15:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "bounds register %u invalid, only 0...7 valid\n",
>> +                          __func__, s->engine_id, reg - BOUNDS_CHANNEL_0);
>> +            return;
>> +        }
>> +        /* fallthrough */
>> +    case DATA_CHANNEL_1_AND_0 ... DATA_CHANNEL_7_AND_6:
>> +    case BOUNDS_CHANNEL_0 ... BOUNDS_CHANNEL_7:
>> +        value &= ASPEED_ADC_LH_MASK;
>> +        break;
>> +    case HYSTERESIS_CHANNEL_8 ... HYSTERESIS_CHANNEL_15:
>> +        if (s->nr_channels <= 8) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: engine[%u]: "
>> +                          "hysteresis register %u invalid, only 0...7 valid\n",
>> +                          __func__, s->engine_id, reg - HYSTERESIS_CHANNEL_0);
>> +            return;
>> +        }
>> +        /* fallthrough */
>> +    case HYSTERESIS_CHANNEL_0 ... HYSTERESIS_CHANNEL_7:
>> +        value &= (ASPEED_ADC_HYST_EN | ASPEED_ADC_LH_MASK);
>> +        break;
>> +    case INTERRUPT_SOURCE:
>> +        value &= 0xffff;
>> +        break;
>> +    case COMPENSATING_AND_TRIMMING:
>> +        value &= 0xf;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: engine[%u]: "
>> +                      "0x%" HWADDR_PRIx " 0x%" PRIx64 "\n",
>> +                      __func__, s->engine_id, addr, value);
>> +        break;
>> +    }
>> +
>> +    s->regs[reg] = value;
>> +}
>> +
>> +static const MemoryRegionOps aspeed_adc_engine_ops = {
>> +    .read = aspeed_adc_engine_read,
>> +    .write = aspeed_adc_engine_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +        .unaligned = false,
>> +    },
>> +};
>> +
>> +static const uint32_t aspeed_adc_resets[ASPEED_ADC_NR_REGS] = {
>> +    [ENGINE_CONTROL]     = 0x00000000,
>> +    [INTERRUPT_CONTROL]  = 0x00000000,
>> +    [VGA_DETECT_CONTROL] = 0x0000000f,
>> +    [CLOCK_CONTROL]      = 0x0000000f,
>> +};
>> +
>> +static void aspeed_adc_engine_reset(DeviceState *dev)
>> +{
>> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
>> +
>> +    memcpy(s->regs, aspeed_adc_resets, sizeof(aspeed_adc_resets));
>> +}
>> +
>> +static void aspeed_adc_engine_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AspeedADCEngineState *s = ASPEED_ADC_ENGINE(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    g_autofree char *name = g_strdup_printf(TYPE_ASPEED_ADC_ENGINE ".%d",
>> +                                            s->engine_id);
>> +
>> +    assert(s->engine_id < 2);
>> +
>> +    sysbus_init_irq(sbd, &s->irq);
>> +
>> +    memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_adc_engine_ops, s, name,
>> +                          0x100);
>> +
>> +    sysbus_init_mmio(sbd, &s->mmio);
>> +}
>> +
>> +static const VMStateDescription vmstate_aspeed_adc_engine = {
>> +    .name = TYPE_ASPEED_ADC,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(regs, AspeedADCEngineState, ASPEED_ADC_NR_REGS),
>> +        VMSTATE_END_OF_LIST(),
>> +    }
>> +};
>> +
>> +static Property aspeed_adc_engine_properties[] = {
>> +    DEFINE_PROP_UINT32("engine-id", AspeedADCEngineState, engine_id, 0),
>> +    DEFINE_PROP_UINT32("nr-channels", AspeedADCEngineState, nr_channels, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void aspeed_adc_engine_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = aspeed_adc_engine_realize;
>> +    dc->reset = aspeed_adc_engine_reset;
>> +    device_class_set_props(dc, aspeed_adc_engine_properties);
>> +    dc->desc = "Aspeed Analog-to-Digital Engine";
>> +    dc->vmsd = &vmstate_aspeed_adc_engine;
>> +}
>> +
>> +static const TypeInfo aspeed_adc_engine_info = {
>> +    .name = TYPE_ASPEED_ADC_ENGINE,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(AspeedADCEngineState),
>> +    .class_init = aspeed_adc_engine_class_init,
>> +};
>> +
>> +static void aspeed_adc_instance_init(Object *obj)
>> +{
>> +    AspeedADCState *s = ASPEED_ADC(obj);
>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
>> +    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
>> +
>> +    for (int i = 0; i < aac->nr_engines; i++) {
>> +        AspeedADCEngineState *engine = &s->engines[i];
>> +        object_initialize_child(obj, "engine[*]", engine,
>> +                                TYPE_ASPEED_ADC_ENGINE);
>> +        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);
> 
> Why have you moved the property initialization in the instance_init routine ?

I think for some reason I thought this was necessary, or maybe I thought it
was more appropriate? I think I was looking at aspeed_soc.c and saw
most of the child initialization in the init() functions, not realize(), but
I’ll just put both properties back in realize(), I don’t think there was any
functional reason.

> 
> Thanks,
> 
> C.
> 
>> +        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
>> +    }
>> +}
>> +
>> +static void aspeed_adc_set_irq(void *opaque, int n, int level)
>> +{
>> +    AspeedADCState *s = opaque;
>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
>> +    uint32_t pending = 0;
>> +
>> +    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
>> +    for (int i = 0; i < aac->nr_engines; i++) {
>> +        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
>> +        pending |= irq_status << (i * 8);
>> +    }
>> +
>> +    qemu_set_irq(s->irq, !!pending);
>> +}
>> +
>> +static void aspeed_adc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AspeedADCState *s = ASPEED_ADC(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
>> +
>> +    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
>> +                                        s, NULL, aac->nr_engines);
>> +
>> +    sysbus_init_irq(sbd, &s->irq);
>> +
>> +    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
>> +
>> +    sysbus_init_mmio(sbd, &s->mmio);
>> +
>> +    for (int i = 0; i < aac->nr_engines; i++) {
>> +        Object *eng = OBJECT(&s->engines[i]);
>> +
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
>> +            return;
>> +        }
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
>> +                           qdev_get_gpio_in(DEVICE(sbd), i));
>> +        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);
>> +    }
>> +}
>> +
>> +static void aspeed_adc_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>> +
>> +    dc->realize = aspeed_adc_realize;
>> +    dc->desc = "Aspeed Analog-to-Digital Converter";
>> +    aac->nr_engines = 1;
>> +}
>> +
>> +static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>> +
>> +    dc->desc = "ASPEED 2600 ADC Controller";
>> +    aac->nr_engines = 2;
>> +}
>> +
>> +static const TypeInfo aspeed_adc_info = {
>> +    .name = TYPE_ASPEED_ADC,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_init = aspeed_adc_instance_init,
>> +    .instance_size = sizeof(AspeedADCState),
>> +    .class_init = aspeed_adc_class_init,
>> +    .class_size = sizeof(AspeedADCClass),
>> +    .abstract   = true,
>> +};
>> +
>> +static const TypeInfo aspeed_2400_adc_info = {
>> +    .name = TYPE_ASPEED_2400_ADC,
>> +    .parent = TYPE_ASPEED_ADC,
>> +};
>> +
>> +static const TypeInfo aspeed_2500_adc_info = {
>> +    .name = TYPE_ASPEED_2500_ADC,
>> +    .parent = TYPE_ASPEED_ADC,
>> +};
>> +
>> +static const TypeInfo aspeed_2600_adc_info = {
>> +    .name = TYPE_ASPEED_2600_ADC,
>> +    .parent = TYPE_ASPEED_ADC,
>> +    .class_init = aspeed_2600_adc_class_init,
>> +};
>> +
>> +static void aspeed_adc_register_types(void)
>> +{
>> +    type_register_static(&aspeed_adc_engine_info);
>> +    type_register_static(&aspeed_adc_info);
>> +    type_register_static(&aspeed_2400_adc_info);
>> +    type_register_static(&aspeed_2500_adc_info);
>> +    type_register_static(&aspeed_2600_adc_info);
>> +}
>> +
>> +type_init(aspeed_adc_register_types);
>> diff --git a/hw/adc/meson.build b/hw/adc/meson.build
>> index ac4f093fea..b29ac7ccdf 100644
>> --- a/hw/adc/meson.build
>> +++ b/hw/adc/meson.build
>> @@ -1,4 +1,5 @@
>>  softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
>>  softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
>>  softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
>>  softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
>> diff --git a/hw/adc/trace-events b/hw/adc/trace-events
>> index 456f21c8f4..5a4c444d77 100644
>> --- a/hw/adc/trace-events
>> +++ b/hw/adc/trace-events
>> @@ -3,3 +3,6 @@
>>  # npcm7xx_adc.c
>>  npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>>  npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>> +
>> +aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>> +aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>> diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
>> new file mode 100644
>> index 0000000000..2f166e8be1
>> --- /dev/null
>> +++ b/include/hw/adc/aspeed_adc.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Aspeed ADC
>> + *
>> + * Copyright 2017-2021 IBM Corp.
>> + *
>> + * Andrew Jeffery <andrew@aj.id.au>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_ADC_ASPEED_ADC_H
>> +#define HW_ADC_ASPEED_ADC_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_ASPEED_ADC "aspeed.adc"
>> +#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
>> +#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
>> +#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
>> +OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
>> +
>> +#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
>> +
>> +#define ASPEED_ADC_NR_CHANNELS 16
>> +#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
>> +
>> +struct AspeedADCEngineState {
>> +    /* <private> */
>> +    SysBusDevice parent;
>> +
>> +    MemoryRegion mmio;
>> +    qemu_irq irq;
>> +    uint32_t engine_id;
>> +    uint32_t nr_channels;
>> +    uint32_t regs[ASPEED_ADC_NR_REGS];
>> +};
>> +
>> +struct AspeedADCState {
>> +    /* <private> */
>> +    SysBusDevice parent;
>> +
>> +    MemoryRegion mmio;
>> +    qemu_irq irq;
>> +
>> +    AspeedADCEngineState engines[2];
>> +};
>> +
>> +struct AspeedADCClass {
>> +    SysBusDeviceClass parent_class;
>> +
>> +    uint32_t nr_engines;
>> +};
>> +
>> +#endif /* HW_ADC_ASPEED_ADC_H */
> 

Re: [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Cédric Le Goater]

>>> +static void aspeed_adc_instance_init(Object *obj)
>>> +{
>>> +    AspeedADCState *s = ASPEED_ADC(obj);
>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
>>> +    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
>>> +
>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>> +        AspeedADCEngineState *engine = &s->engines[i];
>>> +        object_initialize_child(obj, "engine[*]", engine,
>>> +                                TYPE_ASPEED_ADC_ENGINE);
>>> +        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);
>>
>> Why have you moved the property initialization in the instance_init routine ?
> 
> I think for some reason I thought this was necessary, or maybe I thought it
> was more appropriate? I think I was looking at aspeed_soc.c and saw
> most of the child initialization in the init() functions, not realize(), but

The only one is "silicon-rev" which is a constant for the SoC. That's why
we set it from the instance_init routine and I think we need it to initialize
other models also.

> I’ll just put both properties back in realize(), I don’t think there was any
> functional reason.

No. It makes sense. "engine-id" is an internal id which has no reason to
change after init. "nr-channels" is constant and could be derived from
AspeedADCClass. Idealy, we would compute "nr-channels" in AspeedADCEngineState
but this would require another property on the owning AspeedADCState object.

Let's it keep it. Unless someone has a better idea.

One extra remark below.

>>> +        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
>>> +    }
>>> +}
>>> +
>>> +static void aspeed_adc_set_irq(void *opaque, int n, int level)
>>> +{
>>> +    AspeedADCState *s = opaque;
>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
>>> +    uint32_t pending = 0;
>>> +
>>> +    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>> +        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
>>> +        pending |= irq_status << (i * 8);
>>> +    }
>>> +
>>> +    qemu_set_irq(s->irq, !!pending);
>>> +}
>>> +
>>> +static void aspeed_adc_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    AspeedADCState *s = ASPEED_ADC(dev);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
>>> +
>>> +    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
>>> +                                        s, NULL, aac->nr_engines);
>>> +
>>> +    sysbus_init_irq(sbd, &s->irq);
>>> +
>>> +    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
>>> +
>>> +    sysbus_init_mmio(sbd, &s->mmio);
>>> +
>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>> +        Object *eng = OBJECT(&s->engines[i]);
>>> +
>>> +        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
>>> +            return;
>>> +        }
>>> +        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
>>> +                           qdev_get_gpio_in(DEVICE(sbd), i));
>>> +        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);

Since we use 0x100 twice (my fault). May be add a define for it ?

Thanks,

C.

>>> +    }
>>> +}
>>> +
>>> +static void aspeed_adc_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>>> +
>>> +    dc->realize = aspeed_adc_realize;
>>> +    dc->desc = "Aspeed Analog-to-Digital Converter";
>>> +    aac->nr_engines = 1;
>>> +}
>>> +
>>> +static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>>> +
>>> +    dc->desc = "ASPEED 2600 ADC Controller";
>>> +    aac->nr_engines = 2;
>>> +}
>>> +
>>> +static const TypeInfo aspeed_adc_info = {
>>> +    .name = TYPE_ASPEED_ADC,
>>> +    .parent = TYPE_SYS_BUS_DEVICE,
>>> +    .instance_init = aspeed_adc_instance_init,
>>> +    .instance_size = sizeof(AspeedADCState),
>>> +    .class_init = aspeed_adc_class_init,
>>> +    .class_size = sizeof(AspeedADCClass),
>>> +    .abstract   = true,
>>> +};
>>> +
>>> +static const TypeInfo aspeed_2400_adc_info = {
>>> +    .name = TYPE_ASPEED_2400_ADC,
>>> +    .parent = TYPE_ASPEED_ADC,
>>> +};
>>> +
>>> +static const TypeInfo aspeed_2500_adc_info = {
>>> +    .name = TYPE_ASPEED_2500_ADC,
>>> +    .parent = TYPE_ASPEED_ADC,
>>> +};
>>> +
>>> +static const TypeInfo aspeed_2600_adc_info = {
>>> +    .name = TYPE_ASPEED_2600_ADC,
>>> +    .parent = TYPE_ASPEED_ADC,
>>> +    .class_init = aspeed_2600_adc_class_init,
>>> +};
>>> +
>>> +static void aspeed_adc_register_types(void)
>>> +{
>>> +    type_register_static(&aspeed_adc_engine_info);
>>> +    type_register_static(&aspeed_adc_info);
>>> +    type_register_static(&aspeed_2400_adc_info);
>>> +    type_register_static(&aspeed_2500_adc_info);
>>> +    type_register_static(&aspeed_2600_adc_info);
>>> +}
>>> +
>>> +type_init(aspeed_adc_register_types);
>>> diff --git a/hw/adc/meson.build b/hw/adc/meson.build
>>> index ac4f093fea..b29ac7ccdf 100644
>>> --- a/hw/adc/meson.build
>>> +++ b/hw/adc/meson.build
>>> @@ -1,4 +1,5 @@
>>>   softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
>>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
>>>   softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
>>>   softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
>>>   softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
>>> diff --git a/hw/adc/trace-events b/hw/adc/trace-events
>>> index 456f21c8f4..5a4c444d77 100644
>>> --- a/hw/adc/trace-events
>>> +++ b/hw/adc/trace-events
>>> @@ -3,3 +3,6 @@
>>>   # npcm7xx_adc.c
>>>   npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>>>   npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>>> +
>>> +aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>>> +aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>>> diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
>>> new file mode 100644
>>> index 0000000000..2f166e8be1
>>> --- /dev/null
>>> +++ b/include/hw/adc/aspeed_adc.h
>>> @@ -0,0 +1,55 @@
>>> +/*
>>> + * Aspeed ADC
>>> + *
>>> + * Copyright 2017-2021 IBM Corp.
>>> + *
>>> + * Andrew Jeffery <andrew@aj.id.au>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef HW_ADC_ASPEED_ADC_H
>>> +#define HW_ADC_ASPEED_ADC_H
>>> +
>>> +#include "hw/sysbus.h"
>>> +
>>> +#define TYPE_ASPEED_ADC "aspeed.adc"
>>> +#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
>>> +#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
>>> +#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
>>> +OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
>>> +
>>> +#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
>>> +
>>> +#define ASPEED_ADC_NR_CHANNELS 16
>>> +#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
>>> +
>>> +struct AspeedADCEngineState {
>>> +    /* <private> */
>>> +    SysBusDevice parent;
>>> +
>>> +    MemoryRegion mmio;
>>> +    qemu_irq irq;
>>> +    uint32_t engine_id;
>>> +    uint32_t nr_channels;
>>> +    uint32_t regs[ASPEED_ADC_NR_REGS];
>>> +};
>>> +
>>> +struct AspeedADCState {
>>> +    /* <private> */
>>> +    SysBusDevice parent;
>>> +
>>> +    MemoryRegion mmio;
>>> +    qemu_irq irq;
>>> +
>>> +    AspeedADCEngineState engines[2];
>>> +};
>>> +
>>> +struct AspeedADCClass {
>>> +    SysBusDeviceClass parent_class;
>>> +
>>> +    uint32_t nr_engines;
>>> +};
>>> +
>>> +#endif /* HW_ADC_ASPEED_ADC_H */
>>
> 

Re: [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Peter Delevoryas]

> On Oct 3, 2021, at 10:34 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
>>>> 
>>>> +static void aspeed_adc_instance_init(Object *obj)
>>>> +{
>>>> +    AspeedADCState *s = ASPEED_ADC(obj);
>>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(obj);
>>>> +    uint32_t nr_channels = ASPEED_ADC_NR_CHANNELS / aac->nr_engines;
>>>> +
>>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>>> +        AspeedADCEngineState *engine = &s->engines[i];
>>>> +        object_initialize_child(obj, "engine[*]", engine,
>>>> +                                TYPE_ASPEED_ADC_ENGINE);
>>>> +        qdev_prop_set_uint32(DEVICE(engine), "engine-id", i);
>>> 
>>> Why have you moved the property initialization in the instance_init routine ?
>> I think for some reason I thought this was necessary, or maybe I thought it
>> was more appropriate? I think I was looking at aspeed_soc.c and saw
>> most of the child initialization in the init() functions, not realize(), but
> 
> The only one is "silicon-rev" which is a constant for the SoC. That's why
> we set it from the instance_init routine and I think we need it to initialize
> other models also.
> 
>> I’ll just put both properties back in realize(), I don’t think there was any
>> functional reason.
> 
> No. It makes sense. "engine-id" is an internal id which has no reason to
> change after init. "nr-channels" is constant and could be derived from
> AspeedADCClass. Idealy, we would compute "nr-channels" in AspeedADCEngineState
> but this would require another property on the owning AspeedADCState object.

Yeah initially I was going to try to compute “nr-channels” in AspeedADCEngineState
but I came to the same conclusion, it would require adding properties in other
places.

> 
> Let's it keep it. Unless someone has a better idea.

Oh ok then, sounds good!

> 
> One extra remark below.
> 
>>>> +        qdev_prop_set_uint32(DEVICE(engine), "nr-channels", nr_channels);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void aspeed_adc_set_irq(void *opaque, int n, int level)
>>>> +{
>>>> +    AspeedADCState *s = opaque;
>>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(s);
>>>> +    uint32_t pending = 0;
>>>> +
>>>> +    /* TODO: update Global IRQ status register on AST2600 (Need specs) */
>>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>>> +        uint32_t irq_status = s->engines[i].regs[INTERRUPT_CONTROL] & 0xFF;
>>>> +        pending |= irq_status << (i * 8);
>>>> +    }
>>>> +
>>>> +    qemu_set_irq(s->irq, !!pending);
>>>> +}
>>>> +
>>>> +static void aspeed_adc_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    AspeedADCState *s = ASPEED_ADC(dev);
>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> +    AspeedADCClass *aac = ASPEED_ADC_GET_CLASS(dev);
>>>> +
>>>> +    qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_adc_set_irq,
>>>> +                                        s, NULL, aac->nr_engines);
>>>> +
>>>> +    sysbus_init_irq(sbd, &s->irq);
>>>> +
>>>> +    memory_region_init(&s->mmio, OBJECT(s), TYPE_ASPEED_ADC, 0x1000);
>>>> +
>>>> +    sysbus_init_mmio(sbd, &s->mmio);
>>>> +
>>>> +    for (int i = 0; i < aac->nr_engines; i++) {
>>>> +        Object *eng = OBJECT(&s->engines[i]);
>>>> +
>>>> +        if (!sysbus_realize(SYS_BUS_DEVICE(eng), errp)) {
>>>> +            return;
>>>> +        }
>>>> +        sysbus_connect_irq(SYS_BUS_DEVICE(eng), 0,
>>>> +                           qdev_get_gpio_in(DEVICE(sbd), i));
>>>> +        memory_region_add_subregion(&s->mmio, i * 0x100, &s->engines[i].mmio);
> 
> Since we use 0x100 twice (my fault). May be add a define for it ?

Oh yeah sure: I can also add a define for the parent region’s size, 0x1000,
even though it’s not used in two places, since somebody who’s interested
in one is probably interested in knowing the parent region size too.

#define ASPEED_ADC_MEMORY_REGION_SIZE        0x1000
#define ASPEED_ADC_ENGINE_MEMORY_REGION_SIZE 0x100

> 
> Thanks,
> 
> C.
> 
>>>> +    }
>>>> +}
>>>> +
>>>> +static void aspeed_adc_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>>>> +
>>>> +    dc->realize = aspeed_adc_realize;
>>>> +    dc->desc = "Aspeed Analog-to-Digital Converter";
>>>> +    aac->nr_engines = 1;
>>>> +}
>>>> +
>>>> +static void aspeed_2600_adc_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
>>>> +
>>>> +    dc->desc = "ASPEED 2600 ADC Controller";
>>>> +    aac->nr_engines = 2;
>>>> +}
>>>> +
>>>> +static const TypeInfo aspeed_adc_info = {
>>>> +    .name = TYPE_ASPEED_ADC,
>>>> +    .parent = TYPE_SYS_BUS_DEVICE,
>>>> +    .instance_init = aspeed_adc_instance_init,
>>>> +    .instance_size = sizeof(AspeedADCState),
>>>> +    .class_init = aspeed_adc_class_init,
>>>> +    .class_size = sizeof(AspeedADCClass),
>>>> +    .abstract   = true,
>>>> +};
>>>> +
>>>> +static const TypeInfo aspeed_2400_adc_info = {
>>>> +    .name = TYPE_ASPEED_2400_ADC,
>>>> +    .parent = TYPE_ASPEED_ADC,
>>>> +};
>>>> +
>>>> +static const TypeInfo aspeed_2500_adc_info = {
>>>> +    .name = TYPE_ASPEED_2500_ADC,
>>>> +    .parent = TYPE_ASPEED_ADC,
>>>> +};
>>>> +
>>>> +static const TypeInfo aspeed_2600_adc_info = {
>>>> +    .name = TYPE_ASPEED_2600_ADC,
>>>> +    .parent = TYPE_ASPEED_ADC,
>>>> +    .class_init = aspeed_2600_adc_class_init,
>>>> +};
>>>> +
>>>> +static void aspeed_adc_register_types(void)
>>>> +{
>>>> +    type_register_static(&aspeed_adc_engine_info);
>>>> +    type_register_static(&aspeed_adc_info);
>>>> +    type_register_static(&aspeed_2400_adc_info);
>>>> +    type_register_static(&aspeed_2500_adc_info);
>>>> +    type_register_static(&aspeed_2600_adc_info);
>>>> +}
>>>> +
>>>> +type_init(aspeed_adc_register_types);
>>>> diff --git a/hw/adc/meson.build b/hw/adc/meson.build
>>>> index ac4f093fea..b29ac7ccdf 100644
>>>> --- a/hw/adc/meson.build
>>>> +++ b/hw/adc/meson.build
>>>> @@ -1,4 +1,5 @@
>>>>  softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
>>>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_adc.c'))
>>>>  softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
>>>>  softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq-xadc.c'))
>>>>  softmmu_ss.add(when: 'CONFIG_MAX111X', if_true: files('max111x.c'))
>>>> diff --git a/hw/adc/trace-events b/hw/adc/trace-events
>>>> index 456f21c8f4..5a4c444d77 100644
>>>> --- a/hw/adc/trace-events
>>>> +++ b/hw/adc/trace-events
>>>> @@ -3,3 +3,6 @@
>>>>  # npcm7xx_adc.c
>>>>  npcm7xx_adc_read(const char *id, uint64_t offset, uint32_t value) " %s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>>>>  npcm7xx_adc_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0x%04" PRIx64 " value 0x%04" PRIx32
>>>> +
>>>> +aspeed_adc_engine_read(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>>>> +aspeed_adc_engine_write(uint32_t engine_id, uint64_t addr, uint64_t value) "engine[%u] 0x%" PRIx64 " 0x%" PRIx64
>>>> diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
>>>> new file mode 100644
>>>> index 0000000000..2f166e8be1
>>>> --- /dev/null
>>>> +++ b/include/hw/adc/aspeed_adc.h
>>>> @@ -0,0 +1,55 @@
>>>> +/*
>>>> + * Aspeed ADC
>>>> + *
>>>> + * Copyright 2017-2021 IBM Corp.
>>>> + *
>>>> + * Andrew Jeffery <andrew@aj.id.au>
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#ifndef HW_ADC_ASPEED_ADC_H
>>>> +#define HW_ADC_ASPEED_ADC_H
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +
>>>> +#define TYPE_ASPEED_ADC "aspeed.adc"
>>>> +#define TYPE_ASPEED_2400_ADC TYPE_ASPEED_ADC "-ast2400"
>>>> +#define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
>>>> +#define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
>>>> +OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
>>>> +
>>>> +#define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedADCEngineState, ASPEED_ADC_ENGINE)
>>>> +
>>>> +#define ASPEED_ADC_NR_CHANNELS 16
>>>> +#define ASPEED_ADC_NR_REGS     (0xD0 >> 2)
>>>> +
>>>> +struct AspeedADCEngineState {
>>>> +    /* <private> */
>>>> +    SysBusDevice parent;
>>>> +
>>>> +    MemoryRegion mmio;
>>>> +    qemu_irq irq;
>>>> +    uint32_t engine_id;
>>>> +    uint32_t nr_channels;
>>>> +    uint32_t regs[ASPEED_ADC_NR_REGS];
>>>> +};
>>>> +
>>>> +struct AspeedADCState {
>>>> +    /* <private> */
>>>> +    SysBusDevice parent;
>>>> +
>>>> +    MemoryRegion mmio;
>>>> +    qemu_irq irq;
>>>> +
>>>> +    AspeedADCEngineState engines[2];
>>>> +};
>>>> +
>>>> +struct AspeedADCClass {
>>>> +    SysBusDeviceClass parent_class;
>>>> +
>>>> +    uint32_t nr_engines;
>>>> +};
>>>> +
>>>> +#endif /* HW_ADC_ASPEED_ADC_H */

[PATCH 2/2] hw/arm: Integrate ADC model into Aspeed SoC

[pdel]

From: Andrew Jeffery <andrew@aj.id.au>

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/arm/aspeed_ast2600.c     | 11 +++++++++++
 hw/arm/aspeed_soc.c         | 11 +++++++++++
 include/hw/arm/aspeed_soc.h |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 9d70e8e060..d553ee2388 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -148,6 +148,9 @@ static void aspeed_soc_ast2600_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
     object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 
+    snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
+    object_initialize_child(obj, "adc", &s->adc, typename);
+
     snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
     object_initialize_child(obj, "i2c", &s->i2c, typename);
 
@@ -322,6 +325,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
+    /* ADC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->adc), 0, sc->memmap[ASPEED_DEV_ADC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
+
     /* UART - attach an 8250 to the IO space as our UART */
     serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
                    aspeed_soc_get_irq(s, s->uart_default), 38400,
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index ed84502e23..aaec2bba38 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -162,6 +162,9 @@ static void aspeed_soc_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
     object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 
+    snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
+    object_initialize_child(obj, "adc", &s->adc, typename);
+
     snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
     object_initialize_child(obj, "i2c", &s->i2c, typename);
 
@@ -287,6 +290,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
+    /* ADC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->adc), 0, sc->memmap[ASPEED_DEV_ADC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
+
     /* UART - attach an 8250 to the IO space as our UART */
     serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
                    aspeed_soc_get_irq(s, s->uart_default), 38400,
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 87d76c9259..8139358549 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -15,6 +15,7 @@
 #include "hw/cpu/a15mpcore.h"
 #include "hw/intc/aspeed_vic.h"
 #include "hw/misc/aspeed_scu.h"
+#include "hw/adc/aspeed_adc.h"
 #include "hw/misc/aspeed_sdmc.h"
 #include "hw/misc/aspeed_xdma.h"
 #include "hw/timer/aspeed_timer.h"
@@ -53,6 +54,7 @@ struct AspeedSoCState {
     AspeedSCUState scu;
     AspeedHACEState hace;
     AspeedXDMAState xdma;
+    AspeedADCState adc;
     AspeedSMCState fmc;
     AspeedSMCState spi[ASPEED_SPIS_NUM];
     EHCISysBusState ehci[ASPEED_EHCIS_NUM];
-- 
2.30.2

Re: [PATCH 0/2] hw/adc: Add basic Aspeed ADC model

[Cédric Le Goater]

On 10/2/21 23:44, pdel@fbc.om wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> Following up from
> 
> https://lore.kernel.org/qemu-devel/20210930004235.1656003-1-pdel@fb.com/
> 
> This is a resubmission of Andrew Jeffery's ADC model, but with the
> registers converted from typed-member-fields to a regs[] array.
> Otherwise, it should be pretty much equivalent.

Thanks for taking over.

> 
> References to the original patches from Andrew:
> 
> https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4
> https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f
> 
> I did A/B "--trace aspeed_adc_engine_*" testing and the output from the
> boot sequence was equivalent, and I tested that the channel simulation
> code produces the same sequence of values.
> 
> Here's the initialization sequence:
> 
> aspeed_adc_engine_write engine[0] 0x0 0xf  ; Engine init
> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set
> aspeed_adc_engine_write engine[0] 0x0 0x2f ; Auto compensating sensing mode
> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set (redoing engine init, lol), bit 5 reset
> 
> Here's some of the channel simulation:
> 
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00070005
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x000e000a
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x0015000f
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x001c0014
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00230019
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x002a001e
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00310023
> (qemu) xp 0x1e6e9010
> 000000001e6e9010: 0x00380028
> 
> This was tested with a Fuji OpenBMC image:
> 
> https://github.com/peterdelevoryas/openbmc/releases/download/fuji.mtd.0/fuji.mtd
> 
> My refactoring was kinda awkward, it took me a few tries to finally get
> something that made sense to me. In particular, I don't know if you guys
> realized this previously, but in the AST2600, there's 2 engines, but
> still only 16 channels: each engine only has half the registers, as far
> as I understand.

yes.

  
> That's why I added the "nr-channels" property to the engine. I also
> tried to add guest-error logs when a driver tries to read or write to
> the upper-eight channel registers in the 2600 engines. For example:
> 
> (qemu) xp 0x1e6e9000
> 000000001e6e9000: 0xffff010f
> (qemu) xp 0x1e6e9020
> aspeed_adc_engine_read: engine[0]: data register 4 invalid, only 0...3 valid
> 000000001e6e9020: 0x00000000
> (qemu) xp 0x1e6e9030
> 000000001e6e9030: 0x00000000
> (qemu) xp 0x1e6e9040
> 000000001e6e9040: 0x00000000
> (qemu) xp 0x1e6e9050
> aspeed_adc_engine_read: engine[0]: bounds register 8 invalid, only 0...7 valid
> 000000001e6e9050: 0x00000000
> (qemu) xp 0x1e6e9060
> aspeed_adc_engine_read: engine[0]: bounds register 12 invalid, only 0...7 valid
> 000000001e6e9060: 0x00000000
> (qemu) xp 0x1e6e9070
> 000000001e6e9070: 0x00000000
> (qemu) xp 0x1e6e9080
> 000000001e6e9080: 0x00000000
> (qemu) xp 0x1e6e9090
> aspeed_adc_engine_read: engine[0]: hysteresis register 8 invalid, only 0...7 valid
> 000000001e6e9090: 0x00000000
> (qemu) xp 0x1e6e90a0
> aspeed_adc_engine_read: engine[0]: hysteresis register 12 invalid, only 0...7 valid
> 000000001e6e90a0: 0x00000000

This looks nice.

> 
> It might turn out that we should just stick with a closer version of
> Andrew's original patch, and that would be totally fine with me, I just
> want to get the ADC support merged one way or another.


Do you have a test case we could include in qtest ? This is not a
requirement but it's always good to have.

Thanks,

C.

> Also, I'm interested in resubmitting Andrew's work on supporting
> unaligned accesses for models that only implement aligned access, to
> make supporting 8-bit and 16-bit reads transparent to this model, but
> I'll submit that separately.
>
> 
> Reference: https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/
> 
> Thanks,
> Peter
> 
> Andrew Jeffery (2):
>    hw/adc: Add basic Aspeed ADC model
>    hw/arm: Integrate ADC model into Aspeed SoC
> 
>   hw/adc/aspeed_adc.c         | 416 ++++++++++++++++++++++++++++++++++++
>   hw/adc/meson.build          |   1 +
>   hw/adc/trace-events         |   3 +
>   hw/arm/aspeed_ast2600.c     |  11 +
>   hw/arm/aspeed_soc.c         |  11 +
>   include/hw/adc/aspeed_adc.h |  55 +++++
>   include/hw/arm/aspeed_soc.h |   2 +
>   7 files changed, 499 insertions(+)
>   create mode 100644 hw/adc/aspeed_adc.c
>   create mode 100644 include/hw/adc/aspeed_adc.h
> 

Re: [PATCH 0/2] hw/adc: Add basic Aspeed ADC model

[Peter Delevoryas]

> On Oct 3, 2021, at 6:56 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 10/2/21 23:44, pdel@fbc.om wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>> Following up from
>> https://lore.kernel.org/qemu-devel/20210930004235.1656003-1-pdel@fb.com/
>> This is a resubmission of Andrew Jeffery's ADC model, but with the
>> registers converted from typed-member-fields to a regs[] array.
>> Otherwise, it should be pretty much equivalent.
> 
> Thanks for taking over.
> 
>> References to the original patches from Andrew:
>> https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4
>> https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f
>> I did A/B "--trace aspeed_adc_engine_*" testing and the output from the
>> boot sequence was equivalent, and I tested that the channel simulation
>> code produces the same sequence of values.
>> Here's the initialization sequence:
>> aspeed_adc_engine_write engine[0] 0x0 0xf  ; Engine init
>> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set
>> aspeed_adc_engine_write engine[0] 0x0 0x2f ; Auto compensating sensing mode
>> aspeed_adc_engine_read engine[0] 0x0 0x10f ; bit 8 set (redoing engine init, lol), bit 5 reset
>> Here's some of the channel simulation:
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x00070005
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x000e000a
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x0015000f
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x001c0014
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x00230019
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x002a001e
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x00310023
>> (qemu) xp 0x1e6e9010
>> 000000001e6e9010: 0x00380028
>> This was tested with a Fuji OpenBMC image:
>> https://github.com/peterdelevoryas/openbmc/releases/download/fuji.mtd.0/fuji.mtd
>> My refactoring was kinda awkward, it took me a few tries to finally get
>> something that made sense to me. In particular, I don't know if you guys
>> realized this previously, but in the AST2600, there's 2 engines, but
>> still only 16 channels: each engine only has half the registers, as far
>> as I understand.
> 
> yes.
> 
> 
>> That's why I added the "nr-channels" property to the engine. I also
>> tried to add guest-error logs when a driver tries to read or write to
>> the upper-eight channel registers in the 2600 engines. For example:
>> (qemu) xp 0x1e6e9000
>> 000000001e6e9000: 0xffff010f
>> (qemu) xp 0x1e6e9020
>> aspeed_adc_engine_read: engine[0]: data register 4 invalid, only 0...3 valid
>> 000000001e6e9020: 0x00000000
>> (qemu) xp 0x1e6e9030
>> 000000001e6e9030: 0x00000000
>> (qemu) xp 0x1e6e9040
>> 000000001e6e9040: 0x00000000
>> (qemu) xp 0x1e6e9050
>> aspeed_adc_engine_read: engine[0]: bounds register 8 invalid, only 0...7 valid
>> 000000001e6e9050: 0x00000000
>> (qemu) xp 0x1e6e9060
>> aspeed_adc_engine_read: engine[0]: bounds register 12 invalid, only 0...7 valid
>> 000000001e6e9060: 0x00000000
>> (qemu) xp 0x1e6e9070
>> 000000001e6e9070: 0x00000000
>> (qemu) xp 0x1e6e9080
>> 000000001e6e9080: 0x00000000
>> (qemu) xp 0x1e6e9090
>> aspeed_adc_engine_read: engine[0]: hysteresis register 8 invalid, only 0...7 valid
>> 000000001e6e9090: 0x00000000
>> (qemu) xp 0x1e6e90a0
>> aspeed_adc_engine_read: engine[0]: hysteresis register 12 invalid, only 0...7 valid
>> 000000001e6e90a0: 0x00000000
> 
> This looks nice.
> 
>> It might turn out that we should just stick with a closer version of
>> Andrew's original patch, and that would be totally fine with me, I just
>> want to get the ADC support merged one way or another.
> 
> 
> Do you have a test case we could include in qtest ? This is not a
> requirement but it's always good to have.

Yeah absolutely! I have to go look more at qtest a little, but I’m sure I
can add some kind of test for this.

> 
> Thanks,
> 
> C.
> 
>> Also, I'm interested in resubmitting Andrew's work on supporting
>> unaligned accesses for models that only implement aligned access, to
>> make supporting 8-bit and 16-bit reads transparent to this model, but
>> I'll submit that separately.
>> 
>> Reference: https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/
>> Thanks,
>> Peter
>> Andrew Jeffery (2):
>>   hw/adc: Add basic Aspeed ADC model
>>   hw/arm: Integrate ADC model into Aspeed SoC
>>  hw/adc/aspeed_adc.c         | 416 ++++++++++++++++++++++++++++++++++++
>>  hw/adc/meson.build          |   1 +
>>  hw/adc/trace-events         |   3 +
>>  hw/arm/aspeed_ast2600.c     |  11 +
>>  hw/arm/aspeed_soc.c         |  11 +
>>  include/hw/adc/aspeed_adc.h |  55 +++++
>>  include/hw/arm/aspeed_soc.h |   2 +
>>  7 files changed, 499 insertions(+)
>>  create mode 100644 hw/adc/aspeed_adc.c
>>  create mode 100644 include/hw/adc/aspeed_adc.h