qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-arm] [PATCH 1/2] hw/adc: Add basic Aspeed ADC model
Date: Wed, 24 May 2017 02:15:32 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

Hi Andrew,

On 05/22/2017 02:14 AM, Andrew Jeffery wrote:
On Mon, 2017-05-22 at 03:15 +0000, Ryan Chen wrote:
In ASPEED SoC chip, all register access have following rule.
Most of controller write access is only support 32bit access.
Read is support 8bits/16bits/32bits.

This makes sens thinking about how a DMA controller can take advantage of the ADC.


Thanks for clearing that up Ryan.

Phil: I'll rework the model so the reads are 16-bits.

This shouldn't be necessary, QEMU is supposed to supports different access size for different implemented size, so you can declare your implementation as 32-bit and valid accesses from 8 to 32:

 static const MemoryRegionOps aspeed_adc_ops = {
     .read = aspeed_adc_read,
     .write = aspeed_adc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid.min_access_size = 1,
     .valid.max_access_size = 4,
     .valid.unaligned = false,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
 };

This way an I/O access from the CPU or a DMA could use 8/16-bit while you keep a 32-bit implementation. The adjustment is done by access_with_adjusted_size() from memory.c.

Afaik there is, however, no distinction between read/write different access size in current QEMU MemoryRegionOps model.


Thanks,

Andrew




Best Regards,
Ryan

信驊科技股份有限公司
ASPEED Technology Inc.
2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 30077, Taiwan
Tel: 886-3-578-9568  #857
Fax: 886-3-578-9586
************* Email Confidentiality Notice ********************
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other 
confidential information. If you have received it in error, please notify the 
sender by reply e-mail and immediately delete the e-mail and any attachments 
without copying or disclosing the contents. Thank you.

-----Original Message-----
From: Andrew Jeffery [mailto:address@hidden
Sent: Monday, May 22, 2017 8:24 AM
To: Philippe Mathieu-Daudé <address@hidden>; address@hidden
Cc: Peter Maydell <address@hidden>; Ryan Chen <address@hidden>; Alistair Francis 
<address@hidden>; address@hidden; Cédric Le Goater <address@hidden>; Joel Stanley 
<address@hidden>
Subject: Re: [Qemu-arm] [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

Hi Phil,

On Sat, 2017-05-20 at 00:21 -0300, Philippe Mathieu-Daudé wrote:
Hi Andrew,

On 05/19/2017 09:26 PM, Andrew Jeffery wrote:
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.

Signed-off-by: Andrew Jeffery <address@hidden>

---
 hw/adc/Makefile.objs        |   1 +
 hw/adc/aspeed_adc.c         | 246
++++++++++++++++++++++++++++++++++++++++++++
 include/hw/adc/aspeed_adc.h |  33 ++++++
 3 files changed, 280 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

diff --git a/hw/adc/Makefile.objs b/hw/adc/Makefile.objs index
3f6dfdedaec7..2bf9362ba3c4 100644
--- a/hw/adc/Makefile.objs
+++ b/hw/adc/Makefile.objs
@@ -1 +1,2 @@
 obj-$(CONFIG_STM32F2XX_ADC) += stm32f2xx_adc.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_adc.o
diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c new file mode
100644 index 000000000000..d08f1684f7bc
--- /dev/null
+++ b/hw/adc/aspeed_adc.c
@@ -0,0 +1,246 @@
+/*
+ * Aspeed ADC
+ *
+ * Andrew Jeffery <address@hidden>

+ *
+ * Copyright 2017 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/adc/aspeed_adc.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+
+#define ASPEED_ADC_ENGINE_CTRL                  0x00 #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_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)
+
+static inline uint32_t update_channels(uint32_t current) {
+    const uint32_t next = (current + 7) & 0x3ff;
+
+    return (next << 16) | next;
+}
+
+static bool breaks_threshold(AspeedADCState *s, int ch_off) {
+    const uint32_t a = ASPEED_ADC_L(s->channels[ch_off]);
+    const uint32_t a_lower = ASPEED_ADC_L(s->bounds[2 * ch_off]);
+    const uint32_t a_upper = ASPEED_ADC_H(s->bounds[2 * ch_off]);
+    const uint32_t b = ASPEED_ADC_H(s->channels[ch_off]);
+    const uint32_t b_lower = ASPEED_ADC_L(s->bounds[2 * ch_off +
+1]);
+    const uint32_t b_upper = ASPEED_ADC_H(s->bounds[2 * ch_off +
+1]);
+
+    return ((a < a_lower || a > a_upper)) ||
+           ((b < b_lower || b > b_upper)); }
+
+static uint32_t read_channel_sample(AspeedADCState *s, int ch_off)
+{
+    uint32_t ret;
+
+    /* Poor man's sampling */
+    ret = s->channels[ch_off];
+    s->channels[ch_off] = update_channels(s->channels[ch_off]);
+
+    if (breaks_threshold(s, ch_off)) {
+        qemu_irq_raise(s->irq);
+    }
+
+    return ret;
+}
+
+#define TO_INDEX(addr, base) (((addr) - (base)) >> 2)
+
+static uint64_t aspeed_adc_read(void *opaque, hwaddr addr,
+                                     unsigned int size) {
+    AspeedADCState *s = ASPEED_ADC(opaque);
+    uint64_t ret;
+
+    switch (addr) {
+    case 0x00:
+        ret = s->engine_ctrl;
+        break;
+    case 0x04:
+        ret = s->irq_ctrl;
+        break;
+    case 0x08:
+        ret = s->vga_detect_ctrl;
+        break;
+    case 0x0c:
+        ret = s->adc_clk_ctrl;
+        break;
+    case 0x10 ... 0x2e:
+        ret = read_channel_sample(s, TO_INDEX(addr, 0x10));
+        break;
+    case 0x30 ... 0x6e:
+        ret = s->bounds[TO_INDEX(addr, 0x30)];
+        break;
+    case 0x70 ... 0xae:
+        ret = s->hysteresis[TO_INDEX(addr, 0x70)];
+        break;
+    case 0xc0:
+        ret = s->irq_src;
+        break;
+    case 0xc4:
+        ret = s->comp_trim;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: addr: 0x%lx, size: %u\n",
+__func__, addr,
+                size);
+        ret = 0;
+        break;
+    }
+
+    return ret;
+}
+
+static void aspeed_adc_write(void *opaque, hwaddr addr,
+                       uint64_t val, unsigned int size) {
+    AspeedADCState *s = ASPEED_ADC(opaque);
+
+    switch (addr) {
+    case 0x00:
+        {
+            uint32_t init;
+
+            init = !!(val & ASPEED_ADC_ENGINE_EN);
+            init *= ASPEED_ADC_ENGINE_INIT;
+
+            val &= ~ASPEED_ADC_ENGINE_INIT;
+            val |= init;
+        }
+
+        val &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
+        s->engine_ctrl = val;
+
+        break;
+    case 0x04:
+        s->irq_ctrl = val;
+        break;
+    case 0x08:
+        s->vga_detect_ctrl = val;
+        break;
+    case 0x0c:
+        s->adc_clk_ctrl = val;
+        break;
+    case 0x10 ... 0x2e:
+        s->channels[TO_INDEX(addr, 0x10)] = val;
+        break;
+    case 0x30 ... 0x6e:
+        s->bounds[TO_INDEX(addr, 0x30)] = (val &
+ASPEED_ADC_LH_MASK);
+        break;
+    case 0x70 ... 0xae:
+        s->hysteresis[TO_INDEX(addr, 0x70)] =
+            (val & (BIT(31) | ASPEED_ADC_LH_MASK));

Can you add a #define for this BIT(31)?

Sorry, that was an oversight!


+        break;
+    case 0xc0:
+        s->irq_src = (val & 0xffff);
+        break;
+    case 0xc4:
+        s->comp_trim = (val & 0xf);
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: %lu\n", __func__, addr);
+        break;
+    }
+}
+
+static const MemoryRegionOps aspeed_adc_ops = {
+    .read = aspeed_adc_read,
+    .write = aspeed_adc_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,

You sure about that? How the registers are aligned it seems this
device can also be accessed 2-byte wide.

I'll address this below.


+    .valid.max_access_size = 4,
+    .valid.unaligned = false,
+};
+
+static void aspeed_adc_reset(DeviceState *dev) {
+    struct AspeedADCState *s = ASPEED_ADC(dev);
+
+    s->engine_ctrl = 0;
+    s->irq_ctrl = 0;
+    s->vga_detect_ctrl = 0x0000000f;
+    s->adc_clk_ctrl = 0x0000000f;
+    memset(s->channels, 0, sizeof(s->channels));
+    memset(s->bounds, 0, sizeof(s->bounds));
+    memset(s->hysteresis, 0, sizeof(s->hysteresis));
+    s->irq_src = 0;
+    s->comp_trim = 0;
+}
+
+static void aspeed_adc_realize(DeviceState *dev, Error **errp) {
+    AspeedADCState *s = ASPEED_ADC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_adc_ops, s,
+            TYPE_ASPEED_ADC, 0x1000);
+
+    sysbus_init_mmio(sbd, &s->mmio); }
+
+static const VMStateDescription vmstate_aspeed_adc = {
+    .name = TYPE_ASPEED_ADC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(engine_ctrl, AspeedADCState),
+        VMSTATE_UINT32(irq_ctrl, AspeedADCState),
+        VMSTATE_UINT32(vga_detect_ctrl, AspeedADCState),
+        VMSTATE_UINT32(adc_clk_ctrl, AspeedADCState),
+        VMSTATE_UINT32_ARRAY(channels, AspeedADCState,
+                ASPEED_ADC_NR_CHANNELS / 2),
+        VMSTATE_UINT32_ARRAY(bounds, AspeedADCState,
+ASPEED_ADC_NR_CHANNELS),
+        VMSTATE_UINT32_ARRAY(hysteresis, AspeedADCState,
+                ASPEED_ADC_NR_CHANNELS),
+        VMSTATE_UINT32(irq_src, AspeedADCState),
+        VMSTATE_UINT32(comp_trim, AspeedADCState),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+static void aspeed_adc_class_init(ObjectClass *klass, void *data) {
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_adc_realize;
+    dc->reset = aspeed_adc_reset;
+    dc->desc = "Aspeed Analog-to-Digital Converter",
+    dc->vmsd = &vmstate_aspeed_adc; }
+
+static const TypeInfo aspeed_adc_info = {
+    .name = TYPE_ASPEED_ADC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedADCState),
+    .class_init = aspeed_adc_class_init, };
+
+static void aspeed_adc_register_types(void) {
+    type_register_static(&aspeed_adc_info);
+}
+
+type_init(aspeed_adc_register_types);
diff --git a/include/hw/adc/aspeed_adc.h
b/include/hw/adc/aspeed_adc.h new file mode 100644 index
000000000000..ae2089ac62ca
--- /dev/null
+++ b/include/hw/adc/aspeed_adc.h
@@ -0,0 +1,33 @@
+#ifndef _ASPEED_ADC_H_
+#define _ASPEED_ADC_H_
+

You missed the license.

Whoops.


Can you also add a comment about which ADC are modeled (2400/2500 no
difference) as you explain in your cover letter?

Yes, will add this.


+#include <stdint.h>
+
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_ADC "aspeed.adc"
+#define ASPEED_ADC(obj) OBJECT_CHECK(AspeedADCState, (obj),
+TYPE_ASPEED_ADC)
+
+#define ASPEED_ADC_NR_CHANNELS 16
+
+typedef struct AspeedADCState {
+    /* <private> */
+    SysBusDevice parent;
+
+    MemoryRegion mmio;
+    qemu_irq irq;
+
+    uint32_t engine_ctrl;
+    uint32_t irq_ctrl;
+    uint32_t vga_detect_ctrl;
+    uint32_t adc_clk_ctrl;
+    uint32_t channels[ASPEED_ADC_NR_CHANNELS / 2];

I can't find the datasheet

Unfortunately the datasheet isn't publicly available. However, Ryan
(Cc'ed) can help answer specific hardware questions.

, but it seems this ADC has a 10-bit
resolution, so a channel sample can fit into a uint16_t.

Indeed. I think I'll add a bit of a blurb about the device to cover this off.

It sounds weird to read ASPEED_ADC_NR_CHANNELS=16 then use
ASPEED_ADC_NR_CHANNELS/2 channels.
I think using an uint16_t array you can get rid of the ASPEED_ADC_L/H
macros.

This is something I tried to clarify with Ryan on Friday. I am under the 
impression the hardware only supports 32-bit accesses. In the datasheet the 
registers are defined in 32-bit quantities, which drove this approach. The 
driver from the SDK's kernel also uses 32-bit accesses and shifts.

Ryan: So to clarify here, can we do 16-bit (aligned) MMIO accesses on the 
hardware? Or are we restricted to 32-bit MMIO access as I've specified above?

Cheers,

Andrew


+    uint32_t bounds[ASPEED_ADC_NR_CHANNELS];
+    uint32_t hysteresis[ASPEED_ADC_NR_CHANNELS];
+    uint32_t irq_src;
+    uint32_t comp_trim;
+} AspeedADCState;
+
+#endif /* _ASPEED_ADC_H_ */


Regards,

Phil.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]