Re: [PATCH 5/8] hw/timer/altera_timer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the altera_timer code away from bottom-half based ptimers to
> the new transaction-based ptimer API.  This just requires adding
> begin/commit calls around the various places that modify the ptimer
> state, and using the new ptimer_init() function to create the timer.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>   hw/timer/altera_timer.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
> index ee32e0ec1ff..79fc381252d 100644
> --- a/hw/timer/altera_timer.c
> +++ b/hw/timer/altera_timer.c
> @@ -19,7 +19,6 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   
> @@ -53,7 +52,6 @@ typedef struct AlteraTimer {
>       MemoryRegion  mmio;
>       qemu_irq      irq;
>       uint32_t      freq_hz;
> -    QEMUBH       *bh;
>       ptimer_state *ptimer;
>       uint32_t      regs[R_MAX];
>   } AlteraTimer;
> @@ -105,6 +103,7 @@ static void timer_write(void *opaque, hwaddr addr,
>           break;
>   
>       case R_CONTROL:
> +        ptimer_transaction_begin(t->ptimer);
>           t->regs[R_CONTROL] = value & (CONTROL_ITO | CONTROL_CONT);
>           if ((value & CONTROL_START) &&
>               !(t->regs[R_STATUS] & STATUS_RUN)) {
> @@ -115,10 +114,12 @@ static void timer_write(void *opaque, hwaddr addr,
>               ptimer_stop(t->ptimer);
>               t->regs[R_STATUS] &= ~STATUS_RUN;
>           }
> +        ptimer_transaction_commit(t->ptimer);
>           break;
>   
>       case R_PERIODL:
>       case R_PERIODH:
> +        ptimer_transaction_begin(t->ptimer);
>           t->regs[addr] = value & 0xFFFF;
>           if (t->regs[R_STATUS] & STATUS_RUN) {
>               ptimer_stop(t->ptimer);
> @@ -126,6 +127,7 @@ static void timer_write(void *opaque, hwaddr addr,
>           }
>           tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
>           ptimer_set_limit(t->ptimer, tvalue + 1, 1);
> +        ptimer_transaction_commit(t->ptimer);
>           break;
>   
>       case R_SNAPL:
> @@ -183,9 +185,10 @@ static void altera_timer_realize(DeviceState *dev, Error 
> **errp)
>           return;
>       }
>   
> -    t->bh = qemu_bh_new(timer_hit, t);
> -    t->ptimer = ptimer_init_with_bh(t->bh, PTIMER_POLICY_DEFAULT);
> +    t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(t->ptimer);
>       ptimer_set_freq(t->ptimer, t->freq_hz);
> +    ptimer_transaction_commit(t->ptimer);

This looks odd because timers are not running at this point (REALIZE),
but if we don't protect it, ptimer_set_freq() will trigger the assertion.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>   
>       memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
>                             TYPE_ALTERA_TIMER, R_MAX * sizeof(uint32_t));
> @@ -204,8 +207,10 @@ static void altera_timer_reset(DeviceState *dev)
>   {
>       AlteraTimer *t = ALTERA_TIMER(dev);
>   
> +    ptimer_transaction_begin(t->ptimer);
>       ptimer_stop(t->ptimer);
>       ptimer_set_limit(t->ptimer, 0xffffffff, 1);
> +    ptimer_transaction_commit(t->ptimer);
>       memset(t->regs, 0, sizeof(t->regs));
>   }
>