Thanks for your review. We'll apply your suggestions.
On Thu, Jan 7, 2021 at 1:07 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> 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).
>
We'll apply this globally in the patchset.
>
> > +#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().
>
Agree. We'll apply this (in PWM as well.)
>
>
> > + 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).'
>
The hardware behavior in this case is undefined. We'll log a guest error in
this case.
>
> > + 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 ?
>
Thanks for the suggestion. From the Linux driver it is unlikely to cause
any problem if we reset immediately. So we can remove the reset_timer
feature.
>
> > + 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.
>
Thanks. We'll apply.
>
>
>
> > 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
>
