Re: [Qemu-arm] [PATCH v11 6/7] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0

[Peter Crosthwaite]

On Thu, Jan 21, 2016 at 11:03 AM, Dmitry Osipenko <address@hidden> wrote:
> Currently ptimer would print error message and clear enable flag for an
> arming timer that has delta = load = 0. That actually could be a valid case
> for some hardware, like instant IRQ trigger for oneshot timer or continuous
> in periodic mode. Support those cases by removing the error message and
> stopping the timer when delta = 0. Abort execution when period = 0 instead
> of printing the error message, otherwise there is a chance to miss that error.
>
> In addition, don't re-load oneshot timer when delta = 0 and remove duplicated
> code from ptimer_tick(), since ptimer_reload would invoke trigger and stop
> the timer.
>
> Signed-off-by: Dmitry Osipenko <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  hw/core/ptimer.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 142cc64..cec59e1 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -39,11 +39,14 @@ static void ptimer_reload(ptimer_state *s)
>
>      if (s->delta == 0) {
>          ptimer_trigger(s);
> +    }
> +
> +    if (s->delta == 0 && s->enabled == 1) {
>          s->delta = s->limit;
>      }
> -    if (s->delta == 0 || s->period == 0) {
> -        fprintf(stderr, "Timer with period zero, disabling\n");
> -        s->enabled = 0;
> +
> +    if (s->delta == 0) {
> +        ptimer_stop(s);
>          return;
>      }
>
> @@ -72,27 +75,22 @@ static void ptimer_reload(ptimer_state *s)
>  static void ptimer_tick(void *opaque)
>  {
>      ptimer_state *s = (ptimer_state *)opaque;
> -    ptimer_trigger(s);
>      s->delta = 0;
> -    if (s->enabled == 2) {
> -        s->enabled = 0;
> -    } else {
> -        ptimer_reload(s);
> -    }
> +    ptimer_reload(s);
>  }
>
>  uint64_t ptimer_get_count(ptimer_state *s)
>  {
>      uint64_t counter;
>
> -    if (s->enabled) {
> +    if (s->enabled && s->delta != 0) {
>          int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          int64_t next = s->next_event;
>          bool expired = (now - next >= 0);
>          bool oneshot = (s->enabled == 2);
>
>          /* Figure out the current counter value.  */
> -        if (s->period == 0 || (expired && (oneshot || use_icount))) {
> +        if (expired && (oneshot || use_icount)) {
>              /* Prevent timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
> @@ -164,10 +162,7 @@ void ptimer_run(ptimer_state *s, int oneshot)
>  {
>      bool was_disabled = !s->enabled;
>
> -    if (was_disabled && s->period == 0) {
> -        fprintf(stderr, "Timer with period zero, disabling\n");
> -        return;
> -    }
> +    g_assert(s->period != 0);
>      s->enabled = oneshot ? 2 : 1;
>      if (was_disabled) {
>          s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -190,6 +185,7 @@ void ptimer_stop(ptimer_state *s)
>  /* Set counter increment interval in nanoseconds.  */
>  void ptimer_set_period(ptimer_state *s, int64_t period)
>  {
> +    g_assert(period != 0);
>      s->delta = ptimer_get_count(s);
>      s->period = period;
>      s->period_frac = 0;
> @@ -202,6 +198,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
>  /* Set counter frequency in Hz.  */
>  void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>  {
> +    g_assert(freq != 0);
>      s->delta = ptimer_get_count(s);
>      s->period = 1000000000ll / freq;
>      s->period_frac = (1000000000ll << 32) / freq;
> --
> 2.7.0
>