[2/3] arm_mptimer: Fix ONE-SHOT -> PERIODIC mode change

Timer won't start periodic ticking if ONE-SHOT -> PERIODIC mode change happened
after one-shot tick was completed. Fix it by starting ticking only if timer was
disabled previously and isn't ticking right now.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/timer/arm_mptimer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

On Thu, Jul 2, 2015 at 3:52 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer won't start periodic ticking if ONE-SHOT -> PERIODIC mode change happened
> after one-shot tick was completed. Fix it by starting ticking only if timer was
> disabled previously and isn't ticking right now.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/timer/arm_mptimer.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index e230063..58e17c4 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -122,11 +122,16 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if ((old & 1) == (value & 1)) {
> +        /* Don't do anything if timer already disabled.  */
> +        if (((old & 1) == 0) && ((value & 1) == 0)) {

Your do-nothing code paths are now inconsistent between the 0 and 1
cases. I think this if can be consolidated with:

if (value & 1) {
    if ((old & 1) && (tb->count != 0)) {
        break;
    }
    if (tb->control & 2) {
        ...
    }
    tb_reload();
} else if (old & 1) {
    timer_del();
}
break;

I think it's ok to squash patches 1 and 2. and just make a note in the
commit message of the multiple issues. It's hard to split this without
churning.

>              break;
>          }
>          if (value & 1) {
> -            if (tb->count == 0 && (tb->control & 2)) {
> +            /* Don't do anything if timer already ticking.  */
> +            if (((old & 1) != 0) && (tb->count != 0)) {
> +                break;
> +            }
> +            if (tb->control & 2) {

You have dropped the tb->count == 0 which looks like another bugfix
again. It looks like you are making sure the load value is loaded
regardless of timer run-state. If this is correct it just needs a note
in commit message.

Regards,
Peter

>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> --
> 2.4.4
>
>

04.07.2015 22:36, Peter Crosthwaite пишет:
>
> Your do-nothing code paths are now inconsistent between the 0 and 1
> cases. I think this if can be consolidated with:
>
> if (value & 1) {
>      if ((old & 1) && (tb->count != 0)) {
>          break;
>      }
>      if (tb->control & 2) {
>          ...
>      }
>      tb_reload();
> } else if (old & 1) {
>      timer_del();
> }
> break;
>

Code, you are suggesting, is logically equal to my patch. Your variant looks 
cleaner, I'll switch to it. Thanks.


> You have dropped the tb->count == 0 which looks like another bugfix
> again. It looks like you are making sure the load value is loaded
> regardless of timer run-state. If this is correct it just needs a note
> in commit message.
>

Sorry, I don't see where tb->count == 0 got dropped. I think you missed that 
timerblock_reload() checks for tb->count == 0.

To clarify:

Timer is stopped in two cases:
1) timer was shutdown; (old & 1) == 0, tb->count may be unequal to 0 (if timer 
was shutdown while it was ticking).
2) one-shot was completed; (old & 1) == 1, tb->count = 0

On timer enabling we don't need to do anything if either one-shot or periodic 
count is currently in fly, because timerblock_tick() would correctly handle mode 
change, otherwise we re-load tb->count in case of periodic mode or starting with 
whatever(left after periodic shutdown or after loading 'count' via load 
register) tb->count in case of one-shot.


BTW, timer pausing is not implemented and timer can be in two states only: 
running and stopped. This leads to opportunity to convert arm_mptimer to use 
ptimer, I'd like to do it after fixing current issues.


 > I think it's ok to squash patches 1 and 2. and just make a note in the
 > commit message of the multiple issues. It's hard to split this without
 > churning.
 >

Well... yeah, it may worth squashing them. I'll do it.

Thanks for review.