Re: [PATCH 1/2] hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API

[Philippe Mathieu-Daudé]

On 10/17/19 3:23 PM, Peter Maydell wrote:
> Switch the grlib_gptimer 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/grlib_gptimer.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
> index bb09268ea14..7a9371c0e30 100644
> --- a/hw/timer/grlib_gptimer.c
> +++ b/hw/timer/grlib_gptimer.c
> @@ -29,7 +29,6 @@
>   #include "hw/irq.h"
>   #include "hw/ptimer.h"
>   #include "hw/qdev-properties.h"
> -#include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   
>   #include "trace.h"
> @@ -63,7 +62,6 @@ typedef struct GPTimer     GPTimer;
>   typedef struct GPTimerUnit GPTimerUnit;
>   
>   struct GPTimer {
> -    QEMUBH *bh;
>       struct ptimer_state *ptimer;
>   
>       qemu_irq     irq;
> @@ -93,6 +91,17 @@ struct GPTimerUnit {
>       uint32_t config;
>   };
>   
> +static void grlib_gptimer_tx_begin(GPTimer *timer)
> +{
> +    ptimer_transaction_begin(timer->ptimer);
> +}
> +
> +static void grlib_gptimer_tx_commit(GPTimer *timer)
> +{
> +    ptimer_transaction_commit(timer->ptimer);
> +}
> +
> +/* Must be called within grlib_gptimer_tx_begin/commit block */
>   static void grlib_gptimer_enable(GPTimer *timer)
>   {
>       assert(timer != NULL);
> @@ -115,6 +124,7 @@ static void grlib_gptimer_enable(GPTimer *timer)
>       ptimer_run(timer->ptimer, 1);
>   }
>   
> +/* Must be called within grlib_gptimer_tx_begin/commit block */
>   static void grlib_gptimer_restart(GPTimer *timer)
>   {
>       assert(timer != NULL);
> @@ -141,7 +151,9 @@ static void grlib_gptimer_set_scaler(GPTimerUnit *unit, 
> uint32_t scaler)
>       trace_grlib_gptimer_set_scaler(scaler, value);
>   
>       for (i = 0; i < unit->nr_timers; i++) {
> +        ptimer_transaction_begin(unit->timers[i].ptimer);
>           ptimer_set_freq(unit->timers[i].ptimer, value);
> +        ptimer_transaction_commit(unit->timers[i].ptimer);
>       }
>   }
>   
> @@ -266,8 +278,10 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>           switch (timer_addr) {
>           case COUNTER_OFFSET:
>               trace_grlib_gptimer_writel(id, addr, value);
> +            grlib_gptimer_tx_begin(&unit->timers[id]);
>               unit->timers[id].counter = value;
>               grlib_gptimer_enable(&unit->timers[id]);
> +            grlib_gptimer_tx_commit(&unit->timers[id]);
>               return;
>   
>           case COUNTER_RELOAD_OFFSET:
> @@ -291,6 +305,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>               /* gptimer_restart calls gptimer_enable, so if "enable" and "load"
>                  bits are present, we just have to call restart. */
>   
> +            grlib_gptimer_tx_begin(&unit->timers[id]);
>               if (value & GPTIMER_LOAD) {
>                   grlib_gptimer_restart(&unit->timers[id]);
>               } else if (value & GPTIMER_ENABLE) {
> @@ -301,6 +316,7 @@ static void grlib_gptimer_write(void *opaque, hwaddr addr,
>               value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
>   
>               unit->timers[id].config = value;
> +            grlib_gptimer_tx_commit(&unit->timers[id]);
>               return;
>   
>           default:
> @@ -344,9 +360,11 @@ static void grlib_gptimer_reset(DeviceState *d)
>           timer->counter = 0;
>           timer->reload = 0;
>           timer->config = 0;
> +        ptimer_transaction_begin(timer->ptimer);
>           ptimer_stop(timer->ptimer);
>           ptimer_set_count(timer->ptimer, 0);
>           ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +        ptimer_transaction_commit(timer->ptimer);
>       }
>   }
>   
> @@ -365,14 +383,16 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
>           GPTimer *timer = &unit->timers[i];
>   
>           timer->unit   = unit;
> -        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
> -        timer->ptimer = ptimer_init_with_bh(timer->bh, PTIMER_POLICY_DEFAULT);
> +        timer->ptimer = ptimer_init(grlib_gptimer_hit, timer,
> +                                    PTIMER_POLICY_DEFAULT);
>           timer->id     = i;
>   
>           /* One IRQ line for each timer */
>           sysbus_init_irq(sbd, &timer->irq);
>   
> +        ptimer_transaction_begin(timer->ptimer);
>           ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +        ptimer_transaction_commit(timer->ptimer);
>       }
>   
>       memory_region_init_io(&unit->iomem, OBJECT(unit), &grlib_gptimer_ops,

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