[Qemu-devel] arm_mptimer fixes

[Qemu-devel] arm_mptimer fixes

[Dmitry Osipenko]

Hello, this series fixes 3 arm_mptimer issues. All 3 patches were successfully
tested on ARM Cortex-A9 QEMU machine booting Linux kernel and behavior was
compared to real hw by running couple handcrafted mptimer tests.

arm_mptimer: Fix timer shutdown
arm_mptimer: Fix ONE-SHOT -> PERIODIC mode change
arm_mptimer: Respect IT bit state

[Qemu-devel] [PATCH 1/3 v3] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

Timer, running in periodic mode, can't be stopped or coming one-shot tick
won't be canceled because timer control code just doesn't handle timer
disabling. Fix it by deleting timer if enable bit isn't set.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

v2: Avoid calling timer_del() if the timer was already disabled as per
    Peter Maydell suggestion.

v3: Do not change "reload the timer" logic as it is separate bug.

 hw/timer/arm_mptimer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8b93b3c..e230063 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (((old & 1) == 0) && (value & 1)) {
+        if ((old & 1) == (value & 1)) {
+            break;
+        }
+        if (value & 1) {
             if (tb->count == 0 && (tb->control & 2)) {
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
+        } else {
+            /* Shutdown timer.  */
+            timer_del(tb->timer);
         }
         break;
     case 12: /* Interrupt status.  */
-- 
2.4.4

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

[Dmitry Osipenko]

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)) {
             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) {
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
-- 
2.4.4

Re: [Qemu-devel] [PATCH 2/3] arm_mptimer: Fix ONE-SHOT -> PERIODIC mode change

[Peter Crosthwaite]

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

Re: [Qemu-devel] [PATCH 2/3] arm_mptimer: Fix ONE-SHOT -> PERIODIC mode change

[Dmitry Osipenko]

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.

-- 
Dmitry

[Qemu-devel] [PATCH 3/3] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

Timer fires interrupt regardless of current IT(interrupt enable) bit state.
Fix it by making timer to respect IT state.

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

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 58e17c4..82c4462 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
 
 static inline void timerblock_update_irq(TimerBlock *tb)
 {
-    qemu_set_irq(tb->irq, tb->status);
+    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH 3/3] arm_mptimer: Respect IT bit state

[Peter Crosthwaite]

On Thu, Jul 2, 2015 at 3:52 PM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer fires interrupt regardless of current IT(interrupt enable) bit state.
> Fix it by making timer to respect IT state.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/timer/arm_mptimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 58e17c4..82c4462 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
>
>  static inline void timerblock_update_irq(TimerBlock *tb)
>  {
> -    qemu_set_irq(tb->irq, tb->status);
> +    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));

You also need to trigger timerblock_update_irq on change of state for
the control bit itself. "case 8: /* Control.  */" in the _write
handler needs to call this fn.

Regards,
Peter

>  }
>
>  /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
> --
> 2.4.4
>
>

Re: [Qemu-devel] [PATCH 3/3] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

Hello Peter,

04.07.2015 22:39, Peter Crosthwaite пишет:
>
> You also need to trigger timerblock_update_irq on change of state for
> the control bit itself. "case 8: /* Control.  */" in the _write
> handler needs to call this fn.
>

Right, as it will mask/unmask interrupt line. Good catch, thanks.

-- 
Dmitry

[Qemu-devel] [v2] arm_mptimer fixes

[Dmitry Osipenko]

Hello, this is V2 of arm_mptimer patch series. Comments and suggestion from V1
has been addressed and the series was re-tested, including new test for IT bit
masking/unmasking.

[PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change
[PATCH v2 2/2] arm_mptimer: Respect IT bit state

[Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Dmitry Osipenko]

Timer, running in periodic mode, can't be stopped or coming one-shot
tick won't be canceled because timer control code just doesn't handle
timer disabling. Fix it by deleting the timer if enable bit isn't set.

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 the timer isn't ticking right now.

To avoid code churning, these two fixes are squashed in one commit.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

Commits are squashed as per Peter Crosthwaite suggestion.

 hw/timer/arm_mptimer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8b93b3c..0e132b1 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -122,11 +122,18 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (((old & 1) == 0) && (value & 1)) {
-            if (tb->count == 0 && (tb->control & 2)) {
+        if (value & 1) {
+            if ((old & 1) && (tb->count != 0)) {
+                /* Do nothing if timer is ticking right now.  */
+                break;
+            }
+            if (tb->control & 2) {
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
+        } else if (old & 1) {
+            /* Shutdown the timer.  */
+            timer_del(tb->timer);
         }
         break;
     case 12: /* Interrupt status.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer, running in periodic mode, can't be stopped or coming one-shot
> tick won't be canceled because timer control code just doesn't handle
> timer disabling. Fix it by deleting the timer if enable bit isn't set.
>
> 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 the timer isn't ticking right now.
>
> To avoid code churning, these two fixes are squashed in one commit.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>
> Commits are squashed as per Peter Crosthwaite suggestion.
>
>  hw/timer/arm_mptimer.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 8b93b3c..0e132b1 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -122,11 +122,18 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if (((old & 1) == 0) && (value & 1)) {
> -            if (tb->count == 0 && (tb->control & 2)) {
> +        if (value & 1) {
> +            if ((old & 1) && (tb->count != 0)) {
> +                /* Do nothing if timer is ticking right now.  */
> +                break;
> +            }
> +            if (tb->control & 2) {

So when the timer was previously disabled (!(old & 1)) and the count
is non-zero this will cause a spurious auto-reload. I don't this
causes a bug today because the code as-is doesn't support arbitrary
count values, but it is a developer trap should the assumption that
tb->count equals either 0 or the reload value not hold true.

>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> +        } else if (old & 1) {
> +            /* Shutdown the timer.  */
> +            timer_del(tb->timer);

In general, this seems to now dup the code paths for control and
load/counter writes. Both now have a del and reload call for various
changes-of state. I had a go to see if I can consolidate. Turns out,
doing so should implement timer pause and resumption while fixing both
of your bugs, I'll send some patches.

Regards,
Peter

>          }
>          break;
>      case 12: /* Interrupt status.  */
> --
> 2.4.4
>
>

Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Dmitry Osipenko]

05.07.2015 22:07, Peter Crosthwaite пишет:
>> -        if (((old & 1) == 0) && (value & 1)) {
>> -            if (tb->count == 0 && (tb->control & 2)) {
>> +        if (value & 1) {
>> +            if ((old & 1) && (tb->count != 0)) {
>> +                /* Do nothing if timer is ticking right now.  */
>> +                break;
>> +            }
>> +            if (tb->control & 2) {
>
> So when the timer was previously disabled (!(old & 1)) and the count
> is non-zero this will cause a spurious auto-reload. I don't this
> causes a bug today because the code as-is doesn't support arbitrary
> count values, but it is a developer trap should the assumption that
> tb->count equals either 0 or the reload value not hold true.
>

tb->count can be either 0 or tb->load, so it shouldn't hurt to re-load it here.

>>                   tb->count = tb->load;
>>               }
>>               timerblock_reload(tb, 1);
>> +        } else if (old & 1) {
>> +            /* Shutdown the timer.  */
>> +            timer_del(tb->timer);
>
> In general, this seems to now dup the code paths for control and
> load/counter writes. Both now have a del and reload call for various
> changes-of state. I had a go to see if I can consolidate. Turns out,
> doing so should implement timer pause and resumption while fixing both
> of your bugs, I'll send some patches.

Yeah, still there is some room for optimizations.

Well, I think it would be more reasonable to implement pausing with a conversion 
to ptimer use, since it should result in a cleaner and simpler code.

-- 
Dmitry

Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer, running in periodic mode, can't be stopped or coming one-shot
> tick won't be canceled because timer control code just doesn't handle
> timer disabling. Fix it by deleting the timer if enable bit isn't set.
>
You don't need to itemize one-shot and periodic separately, disabling
the running timer just doesn't work universally.

> 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 the timer isn't ticking right now.
>

Needs some grammar work. Try:

The running timer can't be stopped because timer control code just
doesn't handle disabling the timer. Fix it by deleting the timer if
the enable bit is cleared.

The timer won't start periodic ticking if a ONE-SHOT -> PERIODIC mode
change happens after a one-shot tick was completed. Fix it by
re-starting ticking if the timer isn't ticking right now.

> To avoid code churning, these two fixes are squashed in one commit.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Otherwise:

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

Regards,
Peter

> ---
>
> Commits are squashed as per Peter Crosthwaite suggestion.
>
>  hw/timer/arm_mptimer.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 8b93b3c..0e132b1 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -122,11 +122,18 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if (((old & 1) == 0) && (value & 1)) {
> -            if (tb->count == 0 && (tb->control & 2)) {
> +        if (value & 1) {
> +            if ((old & 1) && (tb->count != 0)) {
> +                /* Do nothing if timer is ticking right now.  */
> +                break;
> +            }
> +            if (tb->control & 2) {
>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> +        } else if (old & 1) {
> +            /* Shutdown the timer.  */
> +            timer_del(tb->timer);
>          }
>          break;
>      case 12: /* Interrupt status.  */
> --
> 2.4.4
>
>

Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Dmitry Osipenko]

06.07.2015 00:19, Peter Crosthwaite пишет:
> On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Timer, running in periodic mode, can't be stopped or coming one-shot
>> tick won't be canceled because timer control code just doesn't handle
>> timer disabling. Fix it by deleting the timer if enable bit isn't set.
>>
> You don't need to itemize one-shot and periodic separately, disabling
> the running timer just doesn't work universally.
>

Fair enough.


>> 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 the timer isn't ticking right now.
>>
>
> Needs some grammar work. Try:
>
> The running timer can't be stopped because timer control code just
> doesn't handle disabling the timer. Fix it by deleting the timer if
> the enable bit is cleared.
>
> The timer won't start periodic ticking if a ONE-SHOT -> PERIODIC mode
> change happens after a one-shot tick was completed. Fix it by
> re-starting ticking if the timer isn't ticking right now.
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Regards,
> Peter
>

Sounds good, I'll pick it. Thanks.

-- 
Dmitry

[Qemu-devel] [PATCH v2 2/2] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

Timer should fire interrupt only if IT(interrupt enable) bit state of control
register is enabled and timer should update IRQ status on IT bit change as it
would mask/unmask the interrupt line.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

v2: Added missed IRQ status update on control register write as per
    Peter Crosthwaite comment.

 hw/timer/arm_mptimer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 0e132b1..22fa46e 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
 
 static inline void timerblock_update_irq(TimerBlock *tb)
 {
-    qemu_set_irq(tb->irq, tb->status);
+    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
@@ -122,6 +122,9 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
+        if ((old & 4) != (value & 4)) {
+            timerblock_update_irq(tb);
+        }
         if (value & 1) {
             if ((old & 1) && (tb->count != 0)) {
                 /* Do nothing if timer is ticking right now.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH v2 2/2] arm_mptimer: Respect IT bit state

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer should fire interrupt only if IT(interrupt enable) bit state of control
> register is enabled and timer should update IRQ status on IT bit change as it
> would mask/unmask the interrupt line.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

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

> ---
>
> v2: Added missed IRQ status update on control register write as per
>     Peter Crosthwaite comment.
>
>  hw/timer/arm_mptimer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 0e132b1..22fa46e 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
>
>  static inline void timerblock_update_irq(TimerBlock *tb)
>  {
> -    qemu_set_irq(tb->irq, tb->status);
> +    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));
>  }
>
>  /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
> @@ -122,6 +122,9 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> +        if ((old & 4) != (value & 4)) {
> +            timerblock_update_irq(tb);
> +        }
>          if (value & 1) {
>              if ((old & 1) && (tb->count != 0)) {
>                  /* Do nothing if timer is ticking right now.  */
> --
> 2.4.4
>
>

Re: [Qemu-devel] [v2] arm_mptimer fixes

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> Hello, this is V2 of arm_mptimer patch series. Comments and suggestion from V1
> has been addressed and the series was re-tested, including new test for IT bit
> masking/unmasking.
>
> [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change
> [PATCH v2 2/2] arm_mptimer: Respect IT bit state
>

This looks like an ad-hoc cover letter. Use git to generate patch series covers.

git format-patch HEAD~N --thread --cover-letter -vX

For N patches version X of the series.

Edit the 0000-cover-letter file with your cover mail contents and send
all the generated patches along with cover.

This series doesn't show up in the patch tracking system due to irregular cover.

Regards,
Peter

Re: [Qemu-devel] [v2] arm_mptimer fixes

[Dmitry Osipenko]

05.07.2015 22:52, Peter Crosthwaite пишет:
> On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Hello, this is V2 of arm_mptimer patch series. Comments and suggestion from V1
>> has been addressed and the series was re-tested, including new test for IT bit
>> masking/unmasking.
>>
>> [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change
>> [PATCH v2 2/2] arm_mptimer: Respect IT bit state
>>
>
> This looks like an ad-hoc cover letter. Use git to generate patch series covers.
>
> git format-patch HEAD~N --thread --cover-letter -vX
>
> For N patches version X of the series.
>
> Edit the 0000-cover-letter file with your cover mail contents and send
> all the generated patches along with cover.
>
> This series doesn't show up in the patch tracking system due to irregular cover.
>
> Regards,
> Peter
>

Thanks for advice, I used --compose --no-chain-reply-to. Please let me know if I 
should re-sent and sorry for the mess.

-- 
Dmitry

Re: [Qemu-devel] [v2] arm_mptimer fixes

[Dmitry Osipenko]

05.07.2015 22:52, Peter Crosthwaite пишет:
> This series doesn't show up in the patch tracking system due to irregular cover.

BTW, doesn't QEMU use patchwork for tracking? I see both patches there.

-- 
Dmitry

[Qemu-devel] [PATCH v3 0/2] arm_mptimer fixes

[Dmitry Osipenko]

Hello, this is V3 of arm_mptimer patch series. No code changes here, just
grammar fixes for "shutdown and mode change" patch and general re-send, as
V2 was screwed for patchtracker.

Dmitry Osipenko (2):
  arm_mptimer: Fix timer shutdown and mode change
  arm_mptimer: Respect IT bit state

 hw/timer/arm_mptimer.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

-- 
2.4.4

[Qemu-devel] [PATCH v2 1/2] arm_mptimer: Fix timer shutdown and mode change

[Dmitry Osipenko]

The running timer can't be stopped because timer control code just
doesn't handle disabling the timer. Fix it by deleting the timer if
the enable bit is cleared.

The timer won't start periodic ticking if a ONE-SHOT -> PERIODIC mode
change happens after a one-shot tick was completed. Fix it by
re-starting ticking if the timer isn't ticking right now.

To avoid code churning, these two fixes are squashed in one commit.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

v1: Commits are squashed as per Peter Crosthwaite suggestion.

v2: Grammar fixes of commit message as per Peter Crosthwaite suggestion.

 hw/timer/arm_mptimer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8b93b3c..0e132b1 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -122,11 +122,18 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (((old & 1) == 0) && (value & 1)) {
-            if (tb->count == 0 && (tb->control & 2)) {
+        if (value & 1) {
+            if ((old & 1) && (tb->count != 0)) {
+                /* Do nothing if timer is ticking right now.  */
+                break;
+            }
+            if (tb->control & 2) {
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
+        } else if (old & 1) {
+            /* Shutdown the timer.  */
+            timer_del(tb->timer);
         }
         break;
     case 12: /* Interrupt status.  */
-- 
2.4.4

[Qemu-devel] [PATCH v3 2/2] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

The timer should fire interrupt only if IT(interrupt enable) bit state of
control register is enabled and the timer should update IRQ status on IT
bit change as it would mask/unmask the interrupt line.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

v2: Added missed IRQ status update on control register write as per
    Peter Crosthwaite comment.

v3: No code change, just re-send.

 hw/timer/arm_mptimer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 0e132b1..22fa46e 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
 
 static inline void timerblock_update_irq(TimerBlock *tb)
 {
-    qemu_set_irq(tb->irq, tb->status);
+    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
@@ -122,6 +122,9 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
+        if ((old & 4) != (value & 4)) {
+            timerblock_update_irq(tb);
+        }
         if (value & 1) {
             if ((old & 1) && (tb->count != 0)) {
                 /* Do nothing if timer is ticking right now.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH v3 2/2] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

> v2: Added missed IRQ status update on control register write as per
>      Peter Crosthwaite comment.

Oh, no! Turned out, that is wrong. I wasn't testing that case properly on HW, V1 
is correct. Quote from ARM doc "If the timer interrupt is enabled, Interrupt ID 
29 is set as Pending in the Interrupt Distributor after the event flag is set."

-- 
Dmitry

[Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

The timer should fire the interrupt only if the IT (interrupt enable) bit
state of the control register is enabled.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

v2: Added missed IRQ status update on control register write as per
    Peter Crosthwaite comment.

v3: No code change, just re-send.

v4: Revert to v1, as it was correct, with a slightly change commit message.

 hw/timer/arm_mptimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 0e132b1..3e59c2a 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -38,7 +38,7 @@ static inline int get_current_cpu(ARMMPTimerState *s)
 
 static inline void timerblock_update_irq(TimerBlock *tb)
 {
-    qemu_set_irq(tb->irq, tb->status);
+    qemu_set_irq(tb->irq, tb->status && (tb->control & 4));
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Peter Maydell]

On 6 July 2015 at 02:27, Dmitry Osipenko <digetx@gmail.com> wrote:
> The timer should fire the interrupt only if the IT (interrupt enable) bit
> state of the control register is enabled.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

If you need to update a patch, please resend the whole series,
not just one patch from it. (This is one of the things we talk
about on http://wiki.qemu.org/Contribute/SubmitAPatch -- the
v2/3/4 etc version number applies to a series, not a single
patch). Otherwise it gets very confusing because it's hard to
see if you're really looking at the most recent version of the
patchset, and as Peter C says, some patch handling tools don't
cope with the failure to thread a series properly.

I'll pick these up manually since there are only two patches
here, but for future submissions it would be really helpful
if you could fix this. We generally recommend using git
format-patch/send-email to put together the series and its
cover letter with the right references headers.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

06.07.2015 12:23, Peter Maydell пишет:
> If you need to update a patch, please resend the whole series,
> not just one patch from it. (This is one of the things we talk
> about on http://wiki.qemu.org/Contribute/SubmitAPatch -- the
> v2/3/4 etc version number applies to a series, not a single

I made a quick glance at "SubmitAPatch" before posting. Now, reading more 
closely, I see that it all mentioned there and I feel sorry for that.

> patch). Otherwise it gets very confusing because it's hard to
> see if you're really looking at the most recent version of the
> patchset, and as Peter C says, some patch handling tools don't
> cope with the failure to thread a series properly.
>

Probably, I should go and try those tools myself to have better sense of what I 
should and shouldn't be doing.

> I'll pick these up manually since there are only two patches
> here, but for future submissions it would be really helpful
> if you could fix this. We generally recommend using git
> format-patch/send-email to put together the series and its
> cover letter with the right references headers.
>

Thanks a lot! I very appreciate any help and advices.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Peter Maydell]

On 6 July 2015 at 14:11, Dmitry Osipenko <digetx@gmail.com> wrote:
> I made a quick glance at "SubmitAPatch" before posting. Now, reading more
> closely, I see that it all mentioned there and I feel sorry for that.

No problem -- it's a pretty big page and it mentions a lot of
different things. (I keep meaning to try cleaning it up by
putting in different subsections for 'before sending your
initial patch', 'dealing with review', and so on.)

>> patch). Otherwise it gets very confusing because it's hard to
>> see if you're really looking at the most recent version of the
>> patchset, and as Peter C says, some patch handling tools don't
>> cope with the failure to thread a series properly.
>>
>
> Probably, I should go and try those tools myself to have better sense of
> what I should and shouldn't be doing.

The particular one I find useful is Anthony Liguori's "patches"
tool: https://github.com/aliguori/patches
But it's really intended to help maintainers and submaintainers:
as somebody who's only submitting patches to the list you
probably don't have any need to use it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

06.07.2015 16:16, Peter Maydell пишет:
>> Probably, I should go and try those tools myself to have better sense of
>> what I should and shouldn't be doing.
>
> The particular one I find useful is Anthony Liguori's "patches"
> tool: https://github.com/aliguori/patches

Thanks! I'll take a look at it.

> But it's really intended to help maintainers and submaintainers:
> as somebody who's only submitting patches to the list you
> probably don't have any need to use it.
>

Sure, nevertheless, it won't hurt to know what's going on behind the scene.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 6:38 AM, Dmitry Osipenko <digetx@gmail.com> wrote:
> 06.07.2015 16:16, Peter Maydell пишет:
>>>
>>> Probably, I should go and try those tools myself to have better sense of
>>> what I should and shouldn't be doing.
>>
>>
>> The particular one I find useful is Anthony Liguori's "patches"
>> tool: https://github.com/aliguori/patches
>
>
> Thanks! I'll take a look at it.
>
>> But it's really intended to help maintainers and submaintainers:
>> as somebody who's only submitting patches to the list you
>> probably don't have any need to use it.
>>

As someone who doesn't send PULLs, I still find patches very useful.
It sanity checks your email sends. A while back, I drafted a paragraph
on how to use patches to verify receipt of your series if we want to
add it to "submit a patch".

Regards,
Peter

>
> Sure, nevertheless, it won't hurt to know what's going on behind the scene.
>
> --
> Dmitry
>

Re: [Qemu-devel] [PATCH v4] arm_mptimer: Respect IT bit state

[Dmitry Osipenko]

06.07.2015 19:45, Peter Crosthwaite пишет:
> As someone who doesn't send PULLs, I still find patches very useful.
> It sanity checks your email sends. A while back, I drafted a paragraph
> on how to use patches to verify receipt of your series if we want to
> add it to "submit a patch".
>

Sounds useful. However, I think, it would make sense to do only if the whole 
preparing/sending process description is split-up in distinguished sections, 
like Peter proposed, otherwise it tends to bloat text and stay unnoticed.

-- 
Dmitry