Thanks for your review. We'll apply your suggestions. On Thu, Jan 7, 2021 at 1:07 PM Peter Maydell wrote: > On Thu, 17 Dec 2020 at 00:45, Hao Wu 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 > > Reviewed-by: Tyrone Ting > > Signed-off-by: Hao Wu > > --- > > 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 >