[v11,6/7] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0

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 <digetx@gmail.com>
---
 hw/core/ptimer.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

On Thu, Jan 21, 2016 at 11:03 AM, Dmitry Osipenko <digetx@gmail.com> 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 <digetx@gmail.com>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  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
>

24.01.2016 07:28, Peter Crosthwaite пишет:

[snip]

>>   /* 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;

I noticed that this should be g_assert(freq != 0 && freq <= 1000000000), 
otherwise it possible to make period = 0 for the running timer when setting freq 
 > 1GHz, which doesn't make sense because we can't set period < 1ns.

Or maybe we should clamp period = max(1, period), freq = min(1000000000, freq) 
to "allow" higher timer freq's?

What do you think?