Re: [PATCH v4 3/6] hw/adc: Add an ADC module for NPCM7XX

[Hao Wu via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 10808 bytes --]

Thanks for your review. We'll apply your suggestions.

On Thu, Jan 7, 2021 at 1:07 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhaotsh@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 <hskinnemoen@google.com>
> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > Signed-off-by: Hao Wu <wuhaotsh@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
>

[-- Attachment #2: Type: text/html, Size: 14419 bytes --]