[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: |
Andrew Jeffery |
|
Subject: |
Re: [Qemu-arm] [PATCH 1/2] hw/adc: Add basic Aspeed ADC model |
|
Date: |
Mon, 22 May 2017 15:14:01 +1000 |
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.
Thanks for clearing that up Ryan.
Phil: I'll rework the model so the reads are 16-bits.
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.
signature.asc
Description: This is a digitally signed message part
[Qemu-arm] [PATCH 2/2] hw/arm: Integrate ADC model into Aspeed SoC, Andrew Jeffery, 2017/05/19
Re: [Qemu-arm] [Qemu-devel] [PATCH 0/2] hw/adc: Implement a basic Aspeed ADC model, no-reply, 2017/05/19