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.
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:and...@aj.id.au] Sent: Monday, May 22, 2017 8:24 AM To: Philippe Mathieu-Daudé <f4...@amsat.org>; qemu-...@nongnu.org Cc: Peter Maydell <peter.mayd...@linaro.org>; Ryan Chen <ryan_c...@aspeedtech.com>; Alistair Francis <alist...@alistair23.me>; qemu-devel@nongnu.org; Cédric Le Goater <c...@kaod.org>; Joel Stanley <j...@jms.id.au> 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 <and...@aj.id.au> > > --- > > 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 <and...@aj.id.au> > > + * > > + * 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.
