Re: [PATCH qemu.git v2 5/9] hw/timer/imx_epit: do not persist CR.SWR bit

[Peter Maydell]

On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>  hw/timer/imx_epit.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 5315d9633e..6af460946f 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -191,8 +191,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      case 0: /* CR */
>
>          oldcr = s->cr;
> -        s->cr = value & 0x03ffffff;
> -        if (s->cr & CR_SWR) {
> +        /* SWR bit is never persisted, it clears itself once reset is done */
> +        s->cr = (value & ~CR_SWR) & 0x03ffffff;
> +        if (value & CR_SWR) {
>              /* handle the reset */
>              imx_epit_reset(DEVICE(s));
>              /*

There's a comment just below here that says "can we just 'break'
in this case?". That's there because last time we had to touch
this device we didn't have enough specific knowledge of the hardware
or test cases so we made a refactor that left the code with the same
probably-incorrect behaviour it had before the refactor. But, since
you're working on this device anyway: can we simply add the "break"
after imx_epit_reset() ?

If we can just say "if CR_SWR is set then the device resets like
a hardware reset", then this all simplifies out a lot and this
patch isn't necessary at all. (imx_epit_reset() clears the CR_SWR bit.)
I'm fairly sure we ought to be able to do that, and the missing 'break'
was just a bug...

> @@ -205,7 +206,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, 
> uint64_t value,
>          ptimer_transaction_begin(s->timer_reload);
>
>          /* Update the frequency. Has been done already in case of a reset. */
> -        if (!(s->cr & CR_SWR)) {
> +        if (!(value & CR_SWR)) {
>              imx_epit_set_freq(s);
>          }

thanks
-- PMM