On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelhei...@git.sr.ht> wrote: > > From: Axel Heider <axel.hei...@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.hei...@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