Re: [PATCH qemu.git v2 6/9] hw/timer/imx_epit: remove explicit fields cnt and freq

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> The CNT register is a read-only register. There is no need to
> store it's value, it can be calculated on demand.
> The calculated frequency is needed temporarily only.

This patch bumps the vmstate version ID for the device, which
is a migration compatibility break. For this device/SoC,
that's fine, but we generally prefer to note the break
explicitly in the commit message (eg see commit 759ae1b47e7
or commit 23f6e3b11be for examples).

> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>  hw/timer/imx_epit.c         | 76 +++++++++++++++----------------------
>  include/hw/timer/imx_epit.h |  2 -
>  2 files changed, 30 insertions(+), 48 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 6af460946f..b0ef727efb 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -61,27 +61,16 @@ static const IMXClk imx_epit_clocks[] =  {
>      CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
>  };
>
> -/*
> - * Must be called from within a ptimer_transaction_begin/commit block
> - * for both s->timer_cmp and s->timer_reload.
> - */
> -static void imx_epit_set_freq(IMXEPITState *s)
> +static uint32_t imx_epit_get_freq(IMXEPITState *s)
>  {
> -    uint32_t clksrc;
> -    uint32_t prescaler;
> -
> -    clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, CR_CLKSRC_BITS);
> -    prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, CR_PRESCALE_BITS);
> -
> -    s->freq = imx_ccm_get_clock_frequency(s->ccm,
> -                                imx_epit_clocks[clksrc]) / prescaler;
> -
> -    DPRINTF("Setting ptimer frequency to %u\n", s->freq);
> -
> -    if (s->freq) {
> -        ptimer_set_freq(s->timer_reload, s->freq);
> -        ptimer_set_freq(s->timer_cmp, s->freq);
> -    }
> +    uint32_t clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, CR_CLKSRC_BITS);
> +    uint32_t prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT,
> +                                       CR_PRESCALE_BITS);

These lines have been reformatted but that doesn't have anything
to do with the change to switch from s->freq to a local variable.
If you want to make formatting-type changes can you keep those
separate from other patches, please? It makes things a lot easier
to review.

> +    uint32_t f_in = imx_ccm_get_clock_frequency(s->ccm,
> +                                                imx_epit_clocks[clksrc]);
> +    uint32_t freq = f_in / prescaler;
> +    DPRINTF("ptimer frequency is %u\n", freq);
> +    return freq;
>  }
>
>  static void imx_epit_reset(DeviceState *dev)
> @@ -93,36 +82,26 @@ static void imx_epit_reset(DeviceState *dev)
>      s->sr = 0;
>      s->lr = EPIT_TIMER_MAX;
>      s->cmp = 0;
> -    s->cnt = 0;
> -
>      /* clear the interrupt */
>      qemu_irq_lower(s->irq);
>
>      ptimer_transaction_begin(s->timer_cmp);
>      ptimer_transaction_begin(s->timer_reload);
> -    /* stop both timers */
> +
> +    /*
> +     * The reset switches off the input clock, so even if the CR.EN is still
> +     * set, the timers are no longer running.
> +     */
> +    assert(0 == imx_epit_get_freq(s));

Don't use yoda conditionals, please. "imx_epit_get_freq(s) == 0" is the
QEMU standard way to write this.

>      ptimer_stop(s->timer_cmp);
>      ptimer_stop(s->timer_reload);
> -    /* compute new frequency */
> -    imx_epit_set_freq(s);
>      /* init both timers to EPIT_TIMER_MAX */
>      ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
>      ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
> -    if (s->freq && (s->cr & CR_EN)) {
> -        /* if the timer is still enabled, restart it */
> -        ptimer_run(s->timer_reload, 0);
> -    }
>      ptimer_transaction_commit(s->timer_cmp);
>      ptimer_transaction_commit(s->timer_reload);
>  }

Otherwise the patch looks good.

thanks
-- PMM