Re: [PATCH qemu.git v2 3/9] hw/timer/imx_epit: simplify interrupt logic

[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 | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 8ec770f674..2e9dae0bc8 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -61,18 +61,6 @@ static const IMXClk imx_epit_clocks[] =  {
>      CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
>  };
>
> -/*
> - * Update interrupt status
> - */
> -static void imx_epit_update_int(IMXEPITState *s)
> -{
> -    if (s->sr && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) {
> -        qemu_irq_raise(s->irq);
> -    } else {
> -        qemu_irq_lower(s->irq);
> -    }
> -}
> -
>  /*
>   * Must be called from within a ptimer_transaction_begin/commit block
>   * for both s->timer_cmp and s->timer_reload.
> @@ -253,10 +241,10 @@ static void imx_epit_write(void *opaque, hwaddr offset, 
> uint64_t value,
>          break;
>
>      case 1: /* SR - ACK*/
> -        /* writing 1 to OCIF clears the OCIF bit */
> +        /* writing 1 to OCIF clears the OCIF bit and the interrupt */
>          if (value & 0x01) {
>              s->sr = 0;
> -            imx_epit_update_int(s);
> +            qemu_irq_lower(s->irq);
>          }
>          break;
>
> @@ -305,10 +293,17 @@ static void imx_epit_cmp(void *opaque)
>  {
>      IMXEPITState *s = IMX_EPIT(opaque);
>
> +    /* Set the interrupt status flag to signaled. */
>      DPRINTF("sr was %d\n", s->sr);
> -
>      s->sr = 1;
> -    imx_epit_update_int(s);
> +
> +    /*
> +     * An actual interrupt is generated only if the peripheral is enabled
> +     * and the interrupt generation is enabled.
> +     */
> +    if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
> +        qemu_irq_raise(s->irq);
> +    }
>  }

Having an "update interrupt" function is the more common
convention in QEMU device models -- it means you have one
function you can call from any point where you've updated
any of the state that affects whether an interrupt is generated or not.

For instance there's currently I think a bug where when the
guest writes to the CR register and changes the value of
the CR_OCIEN bit we aren't updating the state of the IRQ line.
If you keep the imx_epit_update_int() function then fixing
that bug is fairly straightforward: you just need to call it
in the appropriate place. Without the function then the
logic of "what is the IRQ line state given the current
device register state" ends up dispersed across the device
model code and potentially duplicated.

thanks
-- PMM