Re: [PATCH v2 4/4] hw/misc: Add a PWM module for NPCM7XX

[Philippe Mathieu-Daudé]

On 12/11/20 11:22 PM, Hao Wu via wrote:
> The PWM module is part of NPCM7XX module. Each NPCM7XX module has two
> identical PWM modules. Each module contains 4 PWM entries. Each PWM has
> two outputs: frequency and duty_cycle. Both are computed using inputs
> from software side.
> 
> This module does not model detail pulse signals since it is expensive.
> It also does not model interrupts and watchdogs that are dependant on
> the detail models. The interfaces for these are left in the module so
> that anyone in need for these functionalities can implement on their
> own.
> 
> 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/arm/npcm7xx.c               |  26 +-
>  hw/misc/meson.build            |   1 +
>  hw/misc/npcm7xx_pwm.c          | 535 +++++++++++++++++++++++++++++++++
>  include/hw/arm/npcm7xx.h       |   2 +
>  include/hw/misc/npcm7xx_pwm.h  | 106 +++++++
>  tests/qtest/meson.build        |   1 +
>  tests/qtest/npcm7xx_pwm-test.c | 490 ++++++++++++++++++++++++++++++
>  8 files changed, 1160 insertions(+), 3 deletions(-)

As this patch is quite big, maybe add the test after the model?

...
> +++ b/hw/misc/npcm7xx_pwm.c
> @@ -0,0 +1,535 @@
> +/*
> + * Nuvoton NPCM7xx PWM 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 "qemu/osdep.h"
> +
> +#include "hw/irq.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/misc/npcm7xx_pwm.h"
> +#include "migration/vmstate.h"
> +#include "qemu/bitops.h"
> +#include "qemu/error-report.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "trace.h"
> +
> +/* 32-bit register indices. */
> +enum NPCM7xxPWMRegisters {
> +    NPCM7XX_PWM_PPR,
> +    NPCM7XX_PWM_CSR,
> +    NPCM7XX_PWM_PCR,
> +    NPCM7XX_PWM_CNR0,
> +    NPCM7XX_PWM_CMR0,
> +    NPCM7XX_PWM_PDR0,
> +    NPCM7XX_PWM_CNR1,
> +    NPCM7XX_PWM_CMR1,
> +    NPCM7XX_PWM_PDR1,
> +    NPCM7XX_PWM_CNR2,
> +    NPCM7XX_PWM_CMR2,
> +    NPCM7XX_PWM_PDR2,
> +    NPCM7XX_PWM_CNR3,
> +    NPCM7XX_PWM_CMR3,
> +    NPCM7XX_PWM_PDR3,
> +    NPCM7XX_PWM_PIER,
> +    NPCM7XX_PWM_PIIR,
> +    NPCM7XX_PWM_PWDR0,
> +    NPCM7XX_PWM_PWDR1,
> +    NPCM7XX_PWM_PWDR2,
> +    NPCM7XX_PWM_PWDR3,
> +    NPCM7XX_PWM_REGS_END,
> +};
> +
> +/* Register field definitions. */
> +#define NPCM7XX_PPR(rv, index)      extract32((rv), npcm7xx_ppr_base[index], 
> 8)
> +#define NPCM7XX_CSR(rv, index)      extract32((rv), npcm7xx_csr_base[index], 
> 3)
> +#define NPCM7XX_CH(rv, index)       extract32((rv), npcm7xx_ch_base[index], 
> 4)
> +#define NPCM7XX_CH_EN               BIT(0)
> +#define NPCM7XX_CH_INV              BIT(2)
> +#define NPCM7XX_CH_MOD              BIT(3)
> +
> +/* Offset of each PWM channel's prescaler in the PPR register. */
> +static const int npcm7xx_ppr_base[] = { 0, 0, 8, 8 };
> +/* Offset of each PWM channel's clock selector in the CSR register. */
> +static const int npcm7xx_csr_base[] = { 0, 4, 8, 12 };
> +/* Offset of each PWM channel's control variable in the PCR register. */
> +static const int npcm7xx_ch_base[] = { 0, 8, 12, 16 };
> +
> +static uint32_t npcm7xx_pwm_calculate_freq(NPCM7xxPWM *p)
> +{
> +    uint32_t ppr;
> +    uint32_t csr;
> +    uint32_t freq;
> +
> +    if (!p->running) {
> +        return 0;
> +    }
> +
> +    csr = NPCM7XX_CSR(p->module->csr, p->index);
> +    ppr = NPCM7XX_PPR(p->module->ppr, p->index);
> +    freq = clock_get_hz(p->module->clock);
> +    freq /= ppr + 1;
> +    /* csr can only be 0~4 */
> +    if (csr > 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid csr value %u\n",
> +                      __func__, csr);
> +        csr = 4;
> +    }
> +    /* freq won't be changed if csr == 4. */
> +    if (csr < 4) {
> +        freq >>= csr + 1;
> +    }
> +
> +    return freq / (p->cnr + 1);

Can you add a trace event here? Passing the unit as parameter
would allow to display it in the traced event.

Alternatively you can introduce:

...pwm_update_freq(pwm, index)
{
    freq = npcm7xx_pwm_calculate_freq(&s->pwm[i]);
    if (freq != s->pwm[i].freq) {
      trace_..pwm_freq_updated(i, s->pwm[i].freq, freq);
      s->pwm[i].freq = freq;
    }
}

> +}
> +
> +static uint32_t npcm7xx_pwm_calculate_duty(NPCM7xxPWM *p)
> +{
> +    uint64_t duty;
> +
> +    if (p->running) {
> +        if (p->cnr == 0) {
> +            duty = 0;
> +        } else if (p->cmr >= p->cnr) {
> +            duty = NPCM7XX_PWM_MAX_DUTY;
> +        } else {
> +            duty = NPCM7XX_PWM_MAX_DUTY * (p->cmr + 1) / (p->cnr + 1);
> +        }
> +    } else {
> +        duty = 0;
> +    }
> +
> +    if (p->inverted) {
> +        duty = NPCM7XX_PWM_MAX_DUTY - duty;
> +    }
> +

Ditto, trace event and pwm_update_duty().

> +    return duty;
> +}
> +
> +static void npcm7xx_pwm_calculate_output(NPCM7xxPWM *p)
> +{
> +    p->freq = npcm7xx_pwm_calculate_freq(p);
> +    p->duty = npcm7xx_pwm_calculate_duty(p);
> +}
...

Rest looks good (not looking at the test yet).

Similar comment than previous ADC patch, at some point I'd like
to introduce a common PWM interface to produce DSP sources.

Regards,

Phil.