qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v1 3/7] STM32F2xx: Add the ADC device


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v1 3/7] STM32F2xx: Add the ADC device
Date: Sat, 25 Apr 2015 11:43:20 -0700

On Sat, Apr 25, 2015 at 1:18 AM, Alistair Francis <address@hidden> wrote:
> Add the STM32F2xx ADC device. This device randomly
> generates values on each read.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
>  default-configs/arm-softmmu.mak |   1 +
>  hw/misc/Makefile.objs           |   1 +
>  hw/misc/stm32f2xx_adc.c         | 332 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/stm32f2xx_adc.h |  96 ++++++++++++
>  4 files changed, 430 insertions(+)
>  create mode 100644 hw/misc/stm32f2xx_adc.c
>  create mode 100644 include/hw/misc/stm32f2xx_adc.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a767e4b..2b16590 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -84,6 +84,7 @@ CONFIG_ZYNQ=y
>  CONFIG_STM32F2XX_TIMER=y
>  CONFIG_STM32F2XX_USART=y
>  CONFIG_STM32F2XX_SYSCFG=y
> +CONFIG_STM32F2XX_ADC=y
>  CONFIG_STM32F205_SOC=y
>
>  CONFIG_VERSATILE_PCI=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 4aa76ff..b65ec3d 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -37,6 +37,7 @@ obj-$(CONFIG_OMAP) += omap_tap.o
>  obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>  obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
> +obj-$(CONFIG_STM32F2XX_ADC) += stm32f2xx_adc.o
>
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>  obj-$(CONFIG_EDU) += edu.o
> diff --git a/hw/misc/stm32f2xx_adc.c b/hw/misc/stm32f2xx_adc.c
> new file mode 100644
> index 0000000..659f14f
> --- /dev/null
> +++ b/hw/misc/stm32f2xx_adc.c
> @@ -0,0 +1,332 @@
> +/*
> + * STM32F2XX ADC
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/hw.h"
> +#include "hw/misc/stm32f2xx_adc.h"
> +

Misc is starting to pile up in random features whereas ADC seems to be
a nice self contained category. Can we create hw/adc? Are there any
other ADCs in misc?

> +#ifndef STM_ADC_ERR_DEBUG
> +#define STM_ADC_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(lvl, fmt, args...) do { \
> +    if (STM_ADC_ERR_DEBUG >= lvl) { \
> +        qemu_log("%s: " fmt, __func__, ## args); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +
> +static void stm32f2xx_adc_reset(DeviceState *dev)
> +{
> +    STM32F2XXAdcState *s = STM32F2XX_ADC(dev);
> +
> +    s->adc_sr = 0x00000000;
> +    s->adc_cr1 = 0x00000000;
> +    s->adc_cr2 = 0x00000000;
> +    s->adc_smpr1 = 0x00000000;
> +    s->adc_smpr2 = 0x00000000;
> +    s->adc_jofr1 = 0x00000000;
> +    s->adc_jofr2 = 0x00000000;
> +    s->adc_jofr3 = 0x00000000;
> +    s->adc_jofr4 = 0x00000000;
> +    s->adc_htr = 0x00000FFF;
> +    s->adc_ltr = 0x00000000;
> +    s->adc_sqr1 = 0x00000000;
> +    s->adc_sqr2 = 0x00000000;
> +    s->adc_sqr3 = 0x00000000;
> +    s->adc_jsqr = 0x00000000;
> +    s->adc_jdr1 = 0x00000000;
> +    s->adc_jdr2 = 0x00000000;
> +    s->adc_jdr3 = 0x00000000;
> +    s->adc_jdr4 = 0x00000000;
> +    s->adc_dr = 0x00000000;

I'm guessing the documentation prefixes all the register symbolic
names with "ADC". In these cases where the doc adds such prefixes I
usually drop them from variables names as the doc is usually
documenting them as part of a bigger register block. In this case the
fact that it is adc is implicit so can the prefix be dropped and save
some text?

> +}
> +
> +static uint32_t stm32f2xx_adc_generate_value(STM32F2XXAdcState *s)
> +{
> +    /* Attempts to fake some ADC values */
> +    #ifdef RAND_AVALIABLE

#ifdef should be non-indented.

Although I have been thinking about this, and perhaps it is better to
make the value (or its randomization factor) runtime controllable. Can
you propertyify something and then you can set the ADC value from the
monitor using a property setter? Even if it cannot be set from the
monitor it might help to at least be start-time configurable using
-global.

> +    s->adc_dr = s->adc_dr + rand();
> +    #else
> +    s->adc_dr = s->adc_dr + 7;
> +    #endif
> +
> +    if (((s->adc_cr1 & ADC_CR1_RES) >> 24) == 0) {

extract to get field value.

switch (s->adc_cr1 & ADC_CR1_RES) >> 24

But could be handled with:

bit_width = 12 - 2 * extract32(a->adc_cr1, ADC_CR1_RES_SHIFT,
ADC_CR1_RES_LENGTH);
s->adc_dr &= (1 << bit_width) - 1;

> +        /* 12-bit */
> +        s->adc_dr = s->adc_dr & 0xFFF;

&=

> +    } else if (((s->adc_cr1 & ADC_CR1_RES) >> 24) == 1) {
> +        /* 10-bit */
> +        s->adc_dr = s->adc_dr & 0x3FF;
> +    } else if (((s->adc_cr1 & ADC_CR1_RES) >> 24) == 2) {
> +        /* 8-bit */
> +        s->adc_dr = s->adc_dr & 0xFF;
> +    } else {
> +        /* 6-bit */
> +        s->adc_dr = s->adc_dr & 0x3F;
> +    }
> +
> +    if (s->adc_cr2 & ADC_CR2_ALIGN) {
> +        return (s->adc_dr << 1) & 0xFFF0;
> +    } else {
> +        return s->adc_dr;
> +    }
> +}
> +
> +static uint64_t stm32f2xx_adc_read(void *opaque, hwaddr addr,
> +                                     unsigned int size)
> +{
> +    STM32F2XXAdcState *s = opaque;
> +
> +    DB_PRINT("%s: Address: 0x%"HWADDR_PRIx"\n", __func__, addr);
> +
> +    if (addr >= 0x100) {

Macro for register offset (even if unimplemented)

> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ADC Common Register Unsupported\n", __func__);
> +    }
> +
> +    switch (addr) {
> +    case ADC_SR:
> +        return s->adc_sr;
> +    case ADC_CR1:
> +        return s->adc_cr1;
> +    case ADC_CR2:
> +        return s->adc_cr2 & 0xFFFFFFF;
> +    case ADC_SMPR1:
> +        return s->adc_smpr1;
> +    case ADC_SMPR2:
> +        return s->adc_smpr2;
> +    case ADC_JOFR1:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);

"included"

> +        return s->adc_jofr1;
> +    case ADC_JOFR2:

Can you use an array for the JOFRs as they are consecutive registers
with the same semantics. Then the 4X repitition can be removed.

> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jofr2;
> +    case ADC_JOFR3:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jofr3;
> +    case ADC_JOFR4:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jofr4;
> +    case ADC_HTR:
> +        return s->adc_htr;
> +    case ADC_LTR:
> +        return s->adc_ltr;
> +    case ADC_SQR1:

Same for SQRs

> +        return s->adc_sqr1;
> +    case ADC_SQR2:
> +        return s->adc_sqr2;
> +    case ADC_SQR3:
> +        return s->adc_sqr3;
> +    case ADC_JSQR:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);

"Included".

> +        return s->adc_jsqr;
> +    case ADC_JDR1:

Array here too.

> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jdr1 - s->adc_jofr1;
> +    case ADC_JDR2:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jdr2 - s->adc_jofr2;
> +    case ADC_JDR3:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jdr3 - s->adc_jofr3;
> +    case ADC_JDR4:
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        return s->adc_jdr4 - s->adc_jofr4;
> +    case ADC_DR:
> +        if ((s->adc_cr2 & ADC_CR2_ADON) && (s->adc_cr2 & ADC_CR2_SWSTART)) {
> +            s->adc_cr2 ^= ADC_CR2_SWSTART;
> +            return stm32f2xx_adc_generate_value(s);
> +        } else {
> +            return 0x00000000;
> +        }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +
> +    return 0;
> +}
> +
> +static void stm32f2xx_adc_write(void *opaque, hwaddr addr,
> +                       uint64_t val64, unsigned int size)
> +{
> +    STM32F2XXAdcState *s = opaque;
> +    uint32_t value = (uint32_t) val64;
> +
> +    DB_PRINT("%s: Address: 0x%"HWADDR_PRIx", Value: 0x%x\n",
> +             __func__, addr, value);
> +
> +    if (addr >= 0x100) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ADC Common Register Unsupported\n", __func__);
> +    }
> +
> +    switch (addr) {
> +    case ADC_SR:
> +        s->adc_sr &= (value & 0x3F);
> +        break;
> +    case ADC_CR1:
> +        s->adc_cr1 = value;
> +        break;
> +    case ADC_CR2:
> +        s->adc_cr2 = value;
> +        break;
> +    case ADC_SMPR1:
> +        s->adc_smpr1 = value;
> +        break;
> +    case ADC_SMPR2:
> +        s->adc_smpr2 = value;
> +        break;
> +    case ADC_JOFR1:
> +        s->adc_jofr1 = (value & 0xFFF);
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JOFR2:
> +        s->adc_jofr2 = (value & 0xFFF);
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JOFR3:
> +        s->adc_jofr3 = (value & 0xFFF);
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JOFR4:
> +        s->adc_jofr4 = (value & 0xFFF);
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_HTR:
> +        s->adc_htr = value;
> +        break;
> +    case ADC_LTR:
> +        s->adc_ltr = value;
> +        break;
> +    case ADC_SQR1:
> +        s->adc_sqr1 = value;
> +        break;
> +    case ADC_SQR2:
> +        s->adc_sqr2 = value;
> +        break;
> +    case ADC_SQR3:
> +        s->adc_sqr3 = value;
> +        break;
> +    case ADC_JSQR:
> +        s->adc_jsqr = value;
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JDR1:
> +        s->adc_jdr1 = value;
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JDR2:
> +        s->adc_jdr2 = value;
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JDR3:
> +        s->adc_jdr3 = value;
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    case ADC_JDR4:
> +        s->adc_jdr4 = value;
> +        qemu_log_mask(LOG_UNIMP, "%s: " \
> +                      "Injection ADC is not implemented, the registers are " 
> \
> +                      "includded for compatability\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +}
> +
> +static const MemoryRegionOps stm32f2xx_adc_ops = {
> +    .read = stm32f2xx_adc_read,
> +    .write = stm32f2xx_adc_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void stm32f2xx_adc_init(Object *obj)
> +{
> +    STM32F2XXAdcState *s = STM32F2XX_ADC(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> +
> +    memory_region_init_io(&s->mmio, obj, &stm32f2xx_adc_ops, s,
> +                          TYPE_STM32F2XX_ADC, 0xFF);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +
> +static void stm32f2xx_adc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = stm32f2xx_adc_reset;
> +}
> +
> +static const TypeInfo stm32f2xx_adc_info = {
> +    .name          = TYPE_STM32F2XX_ADC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(STM32F2XXAdcState),
> +    .instance_init = stm32f2xx_adc_init,
> +    .class_init    = stm32f2xx_adc_class_init,
> +};
> +
> +static void stm32f2xx_adc_register_types(void)
> +{
> +    type_register_static(&stm32f2xx_adc_info);
> +}
> +
> +type_init(stm32f2xx_adc_register_types)
> diff --git a/include/hw/misc/stm32f2xx_adc.h b/include/hw/misc/stm32f2xx_adc.h
> new file mode 100644
> index 0000000..e0ec1d7
> --- /dev/null
> +++ b/include/hw/misc/stm32f2xx_adc.h
> @@ -0,0 +1,96 @@
> +/*
> + * STM32F2XX ADC
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_STM32F2XX_ADC_H
> +#define HW_STM32F2XX_ADC_H
> +
> +#define ADC_SR    0x00
> +#define ADC_CR1   0x04
> +#define ADC_CR2   0x08
> +#define ADC_SMPR1 0x0C
> +#define ADC_SMPR2 0x10
> +#define ADC_JOFR1 0x14
> +#define ADC_JOFR2 0x18
> +#define ADC_JOFR3 0x1C
> +#define ADC_JOFR4 0x20
> +#define ADC_HTR   0x24
> +#define ADC_LTR   0x28
> +#define ADC_SQR1  0x2C
> +#define ADC_SQR2  0x30
> +#define ADC_SQR3  0x34
> +#define ADC_JSQR  0x38
> +#define ADC_JDR1  0x3C
> +#define ADC_JDR2  0x40
> +#define ADC_JDR3  0x44
> +#define ADC_JDR4  0x48
> +#define ADC_DR    0x4C
> +
> +#define ADC_CR2_ADON    0x01
> +#define ADC_CR2_CONT    0x02
> +#define ADC_CR2_ALIGN   0x800
> +#define ADC_CR2_SWSTART 0x40000000
> +
> +#define ADC_CR1_RES 0x3000000
> +

I'm acutally starting to see a big problem with having the reg defs in
the common header, as it is not namespace safe. If you have two
periphs defining a reg with the same name in the same soc you get a
collision. The only safe way to do it would be to preface every symbol
with the full type name. This would be super verbose.

On the other side if we don't headerify them they are not available to
test code.

Out of scope so don't need to do anything for now but maybe we need to
start a top lvl thread.

> +#define TYPE_STM32F2XX_ADC "stm32f2xx-adc"
> +#define STM32F2XX_ADC(obj) \
> +    OBJECT_CHECK(STM32F2XXAdcState, (obj), TYPE_STM32F2XX_ADC)
> +
> +#ifdef RAND_MAX
> +/* The rand() function is avaliable */
> +#define RAND_AVALIABLE

RAND_AVAILABLE

> +#undef RAND_MAX
> +#define RAND_MAX 0xFF
> +#endif /* RAND_MAX */
> +
> +typedef struct {

/* <private> */

> +    SysBusDevice parent_obj;
> +

/* <public> */

> +    MemoryRegion mmio;
> +
> +    uint32_t adc_sr;
> +    uint32_t adc_cr1;
> +    uint32_t adc_cr2;
> +    uint32_t adc_smpr1;
> +    uint32_t adc_smpr2;
> +    uint32_t adc_jofr1;
> +    uint32_t adc_jofr2;
> +    uint32_t adc_jofr3;
> +    uint32_t adc_jofr4;
> +    uint32_t adc_htr;
> +    uint32_t adc_ltr;
> +    uint32_t adc_sqr1;
> +    uint32_t adc_sqr2;
> +    uint32_t adc_sqr3;
> +    uint32_t adc_jsqr;
> +    uint32_t adc_jdr1;
> +    uint32_t adc_jdr2;
> +    uint32_t adc_jdr3;
> +    uint32_t adc_jdr4;
> +    uint32_t adc_dr;
> +
> +    qemu_irq irq;
> +} STM32F2XXAdcState;

STM32F2XXADCState

Regards,
Peter

> +
> +#endif /* HW_STM32F2XX_ADC_H */
> --
> 2.1.4
>
>



reply via email to

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