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 <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/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.
