On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhao...@google.com> wrote:
>
> The ADC is part of NPCM7XX Module. Its behavior is controled by the
> ADC_CON register. It converts one of the eight analog inputs into a
> digital input and stores it in the ADC_DATA register when enabled.
>
> Users can alter input value by using qom-set QMP command.
>
> Reviewed-by: Havard Skinnemoen <hskinnem...@google.com>
> Reviewed-by: Tyrone Ting <kft...@nuvoton.com>
> Signed-off-by: Hao Wu <wuhao...@google.com>
> ---
> docs/system/arm/nuvoton.rst | 2 +-
> hw/adc/meson.build | 1 +
> hw/adc/npcm7xx_adc.c | 321 ++++++++++++++++++++++++++
> hw/adc/trace-events | 5 +
> hw/arm/npcm7xx.c | 24 +-
> include/hw/adc/npcm7xx_adc.h | 72 ++++++
> include/hw/arm/npcm7xx.h | 2 +
> meson.build | 1 +
> tests/qtest/meson.build | 3 +-
> tests/qtest/npcm7xx_adc-test.c | 400 +++++++++++++++++++++++++++++++++
> 10 files changed, 828 insertions(+), 3 deletions(-)
> create mode 100644 hw/adc/npcm7xx_adc.c
> create mode 100644 hw/adc/trace-events
> create mode 100644 include/hw/adc/npcm7xx_adc.h
> create mode 100644 tests/qtest/npcm7xx_adc-test.c
>
> diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
> index b00d405d52..35829f8d0b 100644
> --- a/docs/system/arm/nuvoton.rst
> +++ b/docs/system/arm/nuvoton.rst
> @@ -41,6 +41,7 @@ Supported devices
> * Random Number Generator (RNG)
> * USB host (USBH)
> * GPIO controller
> + * Analog to Digital Converter (ADC)
>
> Missing devices
> ---------------
> @@ -58,7 +59,6 @@ Missing devices
> * USB device (USBD)
> * SMBus controller (SMBF)
> * Peripheral SPI controller (PSPI)
> - * Analog to Digital Converter (ADC)
> * SD/MMC host
> * PECI interface
> * Pulse Width Modulation (PWM)
> diff --git a/hw/adc/meson.build b/hw/adc/meson.build
> index 0d62ae96ae..6ddee23813 100644
> --- a/hw/adc/meson.build
> +++ b/hw/adc/meson.build
> @@ -1 +1,2 @@
> softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true:
> files('stm32f2xx_adc.c'))
> +softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
> diff --git a/hw/adc/npcm7xx_adc.c b/hw/adc/npcm7xx_adc.c
> new file mode 100644
> index 0000000000..f213b6a6df
> --- /dev/null
> +++ b/hw/adc/npcm7xx_adc.c
> @@ -0,0 +1,321 @@
> +/*
> + * Nuvoton NPCM7xx ADC Module
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/adc/npcm7xx_adc.h"
First #include in every .c file must always be "qemu/osdep.h"
(and .h files never include osdep.h).
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/timer.h"
> +#include "qemu/units.h"
> +#include "trace.h"
> +
> +/* 32-bit register indices. */
> +enum NPCM7xxADCRegisters {
> + NPCM7XX_ADC_CON,
> + NPCM7XX_ADC_DATA,
> + NPCM7XX_ADC_REGS_END,
> +};
> +
> +/* Register field definitions. */
> +#define NPCM7XX_ADC_CON_MUX(rv) extract32(rv, 24, 4)
> +#define NPCM7XX_ADC_CON_INT_EN BIT(21)
> +#define NPCM7XX_ADC_CON_REFSEL BIT(19)
> +#define NPCM7XX_ADC_CON_INT BIT(18)
> +#define NPCM7XX_ADC_CON_EN BIT(17)
> +#define NPCM7XX_ADC_CON_RST BIT(16)
> +#define NPCM7XX_ADC_CON_CONV BIT(14)
> +#define NPCM7XX_ADC_CON_DIV(rv) extract32(rv, 1, 8)
> +
> +#define NPCM7XX_ADC_MAX_RESULT 1023
> +#define NPCM7XX_ADC_DEFAULT_IREF 2000000
> +#define NPCM7XX_ADC_CONV_CYCLES 20
> +#define NPCM7XX_ADC_RESET_CYCLES 10
> +#define NPCM7XX_ADC_R0_INPUT 500000
> +#define NPCM7XX_ADC_R1_INPUT 1500000
> +
> +static void npcm7xx_adc_reset(NPCM7xxADCState *s)
> +{
> + timer_del(&s->conv_timer);
> + timer_del(&s->reset_timer);
> + s->con = 0x000c0001;
> + s->data = 0x00000000;
> +}
> +
> +static uint32_t npcm7xx_adc_convert(uint32_t input, uint32_t ref)
> +{
> + uint32_t result;
> +
> + result = input * (NPCM7XX_ADC_MAX_RESULT + 1) / ref;
> + if (result > NPCM7XX_ADC_MAX_RESULT) {
> + result = NPCM7XX_ADC_MAX_RESULT;
> + }
> +
> + return result;
> +}
> +
> +static uint32_t npcm7xx_adc_prescaler(NPCM7xxADCState *s)
> +{
> + return 2 * (NPCM7XX_ADC_CON_DIV(s->con) + 1);
> +}
> +
> +static void npcm7xx_adc_start_timer(Clock *clk, QEMUTimer *timer,
> + uint32_t cycles, uint32_t prescaler)
> +{
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + int64_t freq = clock_get_hz(clk);
> + int64_t ns;
> +
> + ns = (NANOSECONDS_PER_SECOND * cycles * prescaler / freq);
Don't calculate time-in-nanoseconds via clock_get_hz(),
please. Use (the new) clock_ticks_to_ns().
> + ns += now;
> + timer_mod(timer, ns);
> +}
> +
> +static void npcm7xx_adc_start_reset(NPCM7xxADCState *s)
> +{
> + uint32_t prescaler = npcm7xx_adc_prescaler(s);
> +
> + npcm7xx_adc_start_timer(s->clock, &s->reset_timer,
> NPCM7XX_ADC_RESET_CYCLES,
> + prescaler);
> +}
> +
> +static void npcm7xx_adc_start_convert(NPCM7xxADCState *s)
> +{
> + uint32_t prescaler = npcm7xx_adc_prescaler(s);
> +
> + npcm7xx_adc_start_timer(s->clock, &s->conv_timer,
> NPCM7XX_ADC_CONV_CYCLES,
> + prescaler);
> +}
> +
> +static void npcm7xx_adc_reset_done(void *opaque)
> +{
> + NPCM7xxADCState *s = opaque;
> +
> + npcm7xx_adc_reset(s);
> +}
> +
> +static void npcm7xx_adc_convert_done(void *opaque)
> +{
> + NPCM7xxADCState *s = opaque;
> + uint32_t input = NPCM7XX_ADC_CON_MUX(s->con);
> + uint32_t ref = (s->con & NPCM7XX_ADC_CON_REFSEL)
> + ? s->iref : s->vref;
> +
> + g_assert(input < NPCM7XX_ADC_NUM_INPUTS);
It looks to me given that the CON_MUX field is 4 bits and
NUM_INPUTS is only 8 that the guest can trigger this assert
if it writes a bogus value to the register. You should do
something other than asserting in this situation (eg you
can log a guest error and do nothing, or if you happen to
know what the h/w does in this case that's the best thing).
> + s->data = npcm7xx_adc_convert(s->adci[input], ref);
> + if (s->con & NPCM7XX_ADC_CON_INT_EN) {
> + s->con |= NPCM7XX_ADC_CON_INT;
> + qemu_irq_raise(s->irq);
> + }
> + s->con &= ~NPCM7XX_ADC_CON_CONV;
> +}
> +
> +static void npcm7xx_adc_calibrate(NPCM7xxADCState *adc)
> +{
> + adc->calibration_r_values[0] = npcm7xx_adc_convert(NPCM7XX_ADC_R0_INPUT,
> + adc->iref);
> + adc->calibration_r_values[1] = npcm7xx_adc_convert(NPCM7XX_ADC_R1_INPUT,
> + adc->iref);
> +}
> +
> +static void npcm7xx_adc_write_con(NPCM7xxADCState *s, uint32_t new_con)
> +{
> + uint32_t old_con = s->con;
> +
> + /* Write ADC_INT to 1 to clear it */
> + if (new_con & NPCM7XX_ADC_CON_INT) {
> + new_con &= ~NPCM7XX_ADC_CON_INT;
> + } else if (old_con & NPCM7XX_ADC_CON_INT) {
> + new_con |= NPCM7XX_ADC_CON_INT;
> + }
> +
> + s->con = new_con;
> +
> + if (s->con & NPCM7XX_ADC_CON_RST) {
> + if (!(old_con & NPCM7XX_ADC_CON_RST)) {
> + npcm7xx_adc_start_reset(s);
> + }
> + } else {
> + timer_del(&s->reset_timer);
> + }
Emulating "this device really takes X length of time to
complete a guest-requested reset" is usually a higher
degree of fidelity than we bother to model. I assume
that some guest software can't cope with the device
resetting faster than advertised ?
> + if ((s->con & NPCM7XX_ADC_CON_EN)) {
> + if (s->con & NPCM7XX_ADC_CON_CONV) {
> + if (!(old_con & NPCM7XX_ADC_CON_CONV)) {
> + npcm7xx_adc_start_convert(s);
> + }
> + } else {
> + timer_del(&s->conv_timer);
> + }
> + }
> +}
> +
> +static uint64_t npcm7xx_adc_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + uint64_t value = 0;
> + NPCM7xxADCState *s = opaque;
> + hwaddr reg = offset / sizeof(uint32_t);
If you defined your register offsets with the REG32() macro
in include/hw/registerfields.h then it would define
A_FOO constants for you which are at the byte offsets of
the 32-bit registers, and you could avoid converting
the offset by dividing by 4 here. This isn't an obligatory
change, but I think it ends up neater code.
> diff --git a/include/hw/adc/npcm7xx_adc.h b/include/hw/adc/npcm7xx_adc.h
> new file mode 100644
> index 0000000000..7f9acbeaa1
> --- /dev/null
> +++ b/include/hw/adc/npcm7xx_adc.h
> @@ -0,0 +1,72 @@
> +/*
> + * Nuvoton NPCM7xx ADC Module
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +#ifndef NPCM7XX_ADC_H
> +#define NPCM7XX_ADC_H
> +
> +#include "qemu/osdep.h"
.h files never include osdep.h (because the .c files always do).
thanks
-- PMM
