Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-07-27 Thread Havard Skinnemoen 
On Tue, Jul 27, 2021 at 7:19 AM Peter Maydell  wrote:
>
> On Thu, 4 Feb 2021 at 22:38, Hao Wu  wrote:
> >
> > I don't see this error. It could be some error in the clock that the timer 
> > module does not get a correct clock input.
> > How do you reproduce this?
> >
> > On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé  
> > wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell 
> >> wrote:
> >> >
> >> > From: Hao Wu 
> >> >
> >> > This patch makes NPCM7XX Timer to use a the timer clock generated by the
> >> > CLK module instead of the magic number TIMER_REF_HZ.
> >> >
> >> > Reviewed-by: Havard Skinnemoen 
> >> > Reviewed-by: Tyrone Ting 
> >> > Signed-off-by: Hao Wu 
> >> > Message-id: 20210108190945.949196-3-wuhao...@google.com
> >> > Reviewed-by: Peter Maydell 
> >> > Signed-off-by: Peter Maydell 
> >> > ---
> >> >  include/hw/misc/npcm7xx_clk.h|  6 -
> >> >  include/hw/timer/npcm7xx_timer.h |  1 +
> >> >  hw/arm/npcm7xx.c |  5 
> >> >  hw/timer/npcm7xx_timer.c | 39 +++-
> >> >  4 files changed, 24 insertions(+), 27 deletions(-)
> >>
> >> Is that a spurious error (building with Clang)?
> >>
> >> Running test qtest-arm/npcm7xx_timer-test
> >> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> >> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
> >> == 0x0004)
> >> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> >> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> >> FAIL 155 qtest-arm/npcm7xx_timer-test
> >> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
> >> make: *** [Makefile.mtest:1033: run-test-127] Error 1
>
> This intermittent is still with us:
>
> /arm/npcm7xx_timer/tim[2]/timer[0]/periodic_interrupt: **
> ERROR:../../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)):
> (0x == 0x0001)
> **
> ERROR:../../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> FAIL

It looks like the interrupt fires consistently with the status bit. It
also only happens once during that loop of four timer periods (the
test calls g_test_set_nonfatal_assertions() so we would have seen more
failures reported if this wasn't the case).

I'm wondering if the clock_step_next() call is occasionally hitting a
different timer, causing it to return before it has advanced to the
next period of the timer module? If so, it might help to use
clock_step(tim_calculate_step(...)) like some of the other tests do.
This would also verify that the timer is actually firing at the right
interval.

Havard

Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-07-27 Thread Peter Maydell 
On Thu, 4 Feb 2021 at 22:38, Hao Wu  wrote:
>
> I don't see this error. It could be some error in the clock that the timer 
> module does not get a correct clock input.
> How do you reproduce this?
>
> On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé  wrote:
>>
>> Hi,
>>
>> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell 
>> wrote:
>> >
>> > From: Hao Wu 
>> >
>> > This patch makes NPCM7XX Timer to use a the timer clock generated by the
>> > CLK module instead of the magic number TIMER_REF_HZ.
>> >
>> > Reviewed-by: Havard Skinnemoen 
>> > Reviewed-by: Tyrone Ting 
>> > Signed-off-by: Hao Wu 
>> > Message-id: 20210108190945.949196-3-wuhao...@google.com
>> > Reviewed-by: Peter Maydell 
>> > Signed-off-by: Peter Maydell 
>> > ---
>> >  include/hw/misc/npcm7xx_clk.h|  6 -
>> >  include/hw/timer/npcm7xx_timer.h |  1 +
>> >  hw/arm/npcm7xx.c |  5 
>> >  hw/timer/npcm7xx_timer.c | 39 +++-
>> >  4 files changed, 24 insertions(+), 27 deletions(-)
>>
>> Is that a spurious error (building with Clang)?
>>
>> Running test qtest-arm/npcm7xx_timer-test
>> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
>> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
>> == 0x0004)
>> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
>> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
>> FAIL 155 qtest-arm/npcm7xx_timer-test
>> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
>> make: *** [Makefile.mtest:1033: run-test-127] Error 1

This intermittent is still with us:

/arm/npcm7xx_timer/tim[2]/timer[0]/periodic_interrupt: **
ERROR:../../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
assertion failed (tim_read(td, TISR) == tim_timer_bit(td)):
(0x == 0x0001)
**
ERROR:../../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
FAIL

I was able to reproduce it by running the test in a tight loop:

while QTEST_QEMU_BINARY=./build/x86/qemu-system-arm
./build/x86/tests/qtest/npcm7xx_timer-test ; do true ;done

and eventually it hit the assert.

-- PMM

Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-06-22 Thread Philippe Mathieu-Daudé 
On 2/10/21 12:54 PM, Philippe Mathieu-Daudé wrote:
> On 2/4/21 11:37 PM, Hao Wu wrote:
>> I don't see this error. It could be some error in the clock that the
>> timer module does not get a correct clock input.
>> How do you reproduce this?
> 
> I got it only once, can not reproduce, but it happened on our CI too:
> https://gitlab.com/qemu-project/qemu/-/jobs/1006073367#L4430

Again earlier today:
https://gitlab.com/qemu-project/qemu/-/jobs/1364690563#L3996

ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
== 0x0010)
**
ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
FAIL 179 qtest-arm/npcm7xx_timer-test
/arm/npcm7xx_timer/tim[2]/timer[4]/periodic_interrupt

> 
>>
>> On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé > > wrote:
>>
>> Hi,
>>
>> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell
>> mailto:peter.mayd...@linaro.org>>
>> wrote:
>> >
>> > From: Hao Wu mailto:wuhao...@google.com>>
>> >
>> > This patch makes NPCM7XX Timer to use a the timer clock generated
>> by the
>> > CLK module instead of the magic number TIMER_REF_HZ.
>> >
>> > Reviewed-by: Havard Skinnemoen > >
>> > Reviewed-by: Tyrone Ting > >
>> > Signed-off-by: Hao Wu > >
>> > Message-id: 20210108190945.949196-3-wuhao...@google.com
>> 
>> > Reviewed-by: Peter Maydell > >
>> > Signed-off-by: Peter Maydell > >
>> > ---
>> >  include/hw/misc/npcm7xx_clk.h    |  6 -
>> >  include/hw/timer/npcm7xx_timer.h |  1 +
>> >  hw/arm/npcm7xx.c                 |  5 
>> >  hw/timer/npcm7xx_timer.c         | 39
>> +++-
>> >  4 files changed, 24 insertions(+), 27 deletions(-)
>>
>> Is that a spurious error (building with Clang)?
>>
>> Running test qtest-arm/npcm7xx_timer-test
>> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
>> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
>> == 0x0004)
>> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
>> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
>> FAIL 155 qtest-arm/npcm7xx_timer-test
>> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
>> make: *** [Makefile.mtest:1033: run-test-127] Error 1
>>
>

Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-02-10 Thread Philippe Mathieu-Daudé 
On 2/4/21 11:37 PM, Hao Wu wrote:
> I don't see this error. It could be some error in the clock that the
> timer module does not get a correct clock input.
> How do you reproduce this?

I got it only once, can not reproduce, but it happened on our CI too:
https://gitlab.com/qemu-project/qemu/-/jobs/1006073367#L4430

> 
> On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé  > wrote:
> 
> Hi,
> 
> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell
> mailto:peter.mayd...@linaro.org>>
> wrote:
> >
> > From: Hao Wu mailto:wuhao...@google.com>>
> >
> > This patch makes NPCM7XX Timer to use a the timer clock generated
> by the
> > CLK module instead of the magic number TIMER_REF_HZ.
> >
> > Reviewed-by: Havard Skinnemoen  >
> > Reviewed-by: Tyrone Ting  >
> > Signed-off-by: Hao Wu  >
> > Message-id: 20210108190945.949196-3-wuhao...@google.com
> 
> > Reviewed-by: Peter Maydell  >
> > Signed-off-by: Peter Maydell  >
> > ---
> >  include/hw/misc/npcm7xx_clk.h    |  6 -
> >  include/hw/timer/npcm7xx_timer.h |  1 +
> >  hw/arm/npcm7xx.c                 |  5 
> >  hw/timer/npcm7xx_timer.c         | 39
> +++-
> >  4 files changed, 24 insertions(+), 27 deletions(-)
> 
> Is that a spurious error (building with Clang)?
> 
> Running test qtest-arm/npcm7xx_timer-test
> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
> == 0x0004)
> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> FAIL 155 qtest-arm/npcm7xx_timer-test
> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
> make: *** [Makefile.mtest:1033: run-test-127] Error 1
>

Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-02-04 Thread Hao Wu 
I don't see this error. It could be some error in the clock that the timer
module does not get a correct clock input.
How do you reproduce this?

On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé 
wrote:

> Hi,
>
> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell 
> wrote:
> >
> > From: Hao Wu 
> >
> > This patch makes NPCM7XX Timer to use a the timer clock generated by the
> > CLK module instead of the magic number TIMER_REF_HZ.
> >
> > Reviewed-by: Havard Skinnemoen 
> > Reviewed-by: Tyrone Ting 
> > Signed-off-by: Hao Wu 
> > Message-id: 20210108190945.949196-3-wuhao...@google.com
> > Reviewed-by: Peter Maydell 
> > Signed-off-by: Peter Maydell 
> > ---
> >  include/hw/misc/npcm7xx_clk.h|  6 -
> >  include/hw/timer/npcm7xx_timer.h |  1 +
> >  hw/arm/npcm7xx.c |  5 
> >  hw/timer/npcm7xx_timer.c | 39 +++-
> >  4 files changed, 24 insertions(+), 27 deletions(-)
>
> Is that a spurious error (building with Clang)?
>
> Running test qtest-arm/npcm7xx_timer-test
> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
> == 0x0004)
> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> FAIL 155 qtest-arm/npcm7xx_timer-test
> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
> make: *** [Makefile.mtest:1033: run-test-127] Error 1
>

Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-02-04 Thread Philippe Mathieu-Daudé 
Hi,

On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell 
wrote:
>
> From: Hao Wu 
>
> This patch makes NPCM7XX Timer to use a the timer clock generated by the
> CLK module instead of the magic number TIMER_REF_HZ.
>
> Reviewed-by: Havard Skinnemoen 
> Reviewed-by: Tyrone Ting 
> Signed-off-by: Hao Wu 
> Message-id: 20210108190945.949196-3-wuhao...@google.com
> Reviewed-by: Peter Maydell 
> Signed-off-by: Peter Maydell 
> ---
>  include/hw/misc/npcm7xx_clk.h|  6 -
>  include/hw/timer/npcm7xx_timer.h |  1 +
>  hw/arm/npcm7xx.c |  5 
>  hw/timer/npcm7xx_timer.c | 39 +++-
>  4 files changed, 24 insertions(+), 27 deletions(-)

Is that a spurious error (building with Clang)?

Running test qtest-arm/npcm7xx_timer-test
ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x
== 0x0004)
ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
FAIL 155 qtest-arm/npcm7xx_timer-test
/arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
make: *** [Makefile.mtest:1033: run-test-127] Error 1

[PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

2021-01-12 Thread Peter Maydell 
From: Hao Wu 

This patch makes NPCM7XX Timer to use a the timer clock generated by the
CLK module instead of the magic number TIMER_REF_HZ.

Reviewed-by: Havard Skinnemoen 
Reviewed-by: Tyrone Ting 
Signed-off-by: Hao Wu 
Message-id: 20210108190945.949196-3-wuhao...@google.com
Reviewed-by: Peter Maydell 
Signed-off-by: Peter Maydell 
---
 include/hw/misc/npcm7xx_clk.h|  6 -
 include/hw/timer/npcm7xx_timer.h |  1 +
 hw/arm/npcm7xx.c |  5 
 hw/timer/npcm7xx_timer.c | 39 +++-
 4 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
index f641f95f3e6..d5c8d16ca42 100644
--- a/include/hw/misc/npcm7xx_clk.h
+++ b/include/hw/misc/npcm7xx_clk.h
@@ -20,12 +20,6 @@
 #include "hw/clock.h"
 #include "hw/sysbus.h"
 
-/*
- * The reference clock frequency for the timer modules, and the SECCNT and
- * CNTR25M registers in this module, is always 25 MHz.
- */
-#define NPCM7XX_TIMER_REF_HZ(2500)
-
 /*
  * Number of registers in our device state structure. Don't change this without
  * incrementing the version_id in the vmstate.
diff --git a/include/hw/timer/npcm7xx_timer.h b/include/hw/timer/npcm7xx_timer.h
index 6993fd723a1..d45c051b56a 100644
--- a/include/hw/timer/npcm7xx_timer.h
+++ b/include/hw/timer/npcm7xx_timer.h
@@ -101,6 +101,7 @@ struct NPCM7xxTimerCtrlState {
 
 uint32_ttisr;
 
+Clock   *clock;
 NPCM7xxTimer timer[NPCM7XX_TIMERS_PER_CTRL];
 NPCM7xxWatchdogTimer watchdog_timer;
 };
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 47e2b6fc400..fabfb1697ba 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -22,6 +22,7 @@
 #include "hw/char/serial.h"
 #include "hw/loader.h"
 #include "hw/misc/unimp.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/units.h"
@@ -420,6 +421,10 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
 int first_irq;
 int j;
 
+/* Connect the timer clock. */
+qdev_connect_clock_in(DEVICE(>tim[i]), "clock", qdev_get_clock_out(
+DEVICE(>clk), "timer-clock"));
+
 sysbus_realize(sbd, _abort);
 sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]);
 
diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c
index d24445bd6e4..36e2c07db26 100644
--- a/hw/timer/npcm7xx_timer.c
+++ b/hw/timer/npcm7xx_timer.c
@@ -17,8 +17,8 @@
 #include "qemu/osdep.h"
 
 #include "hw/irq.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
-#include "hw/misc/npcm7xx_clk.h"
 #include "hw/timer/npcm7xx_timer.h"
 #include "migration/vmstate.h"
 #include "qemu/bitops.h"
@@ -128,23 +128,18 @@ static uint32_t npcm7xx_tcsr_prescaler(uint32_t tcsr)
 /* Convert a timer cycle count to a time interval in nanoseconds. */
 static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t, uint32_t count)
 {
-int64_t ns = count;
+int64_t ticks = count;
 
-ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ;
-ns *= npcm7xx_tcsr_prescaler(t->tcsr);
+ticks *= npcm7xx_tcsr_prescaler(t->tcsr);
 
-return ns;
+return clock_ticks_to_ns(t->ctrl->clock, ticks);
 }
 
 /* Convert a time interval in nanoseconds to a timer cycle count. */
 static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t, int64_t ns)
 {
-int64_t count;
-
-count = ns / (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ);
-count /= npcm7xx_tcsr_prescaler(t->tcsr);
-
-return count;
+return ns / clock_ticks_to_ns(t->ctrl->clock,
+  npcm7xx_tcsr_prescaler(t->tcsr));
 }
 
 static uint32_t npcm7xx_watchdog_timer_prescaler(const NPCM7xxWatchdogTimer *t)
@@ -166,8 +161,8 @@ static uint32_t npcm7xx_watchdog_timer_prescaler(const 
NPCM7xxWatchdogTimer *t)
 static void npcm7xx_watchdog_timer_reset_cycles(NPCM7xxWatchdogTimer *t,
 int64_t cycles)
 {
-uint32_t prescaler = npcm7xx_watchdog_timer_prescaler(t);
-int64_t ns = (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ) * cycles;
+int64_t ticks = cycles * npcm7xx_watchdog_timer_prescaler(t);
+int64_t ns = clock_ticks_to_ns(t->ctrl->clock, ticks);
 
 /*
  * The reset function always clears the current timer. The caller of the
@@ -176,7 +171,6 @@ static void 
npcm7xx_watchdog_timer_reset_cycles(NPCM7xxWatchdogTimer *t,
  */
 npcm7xx_timer_clear(>base_timer);
 
-ns *= prescaler;
 t->base_timer.remaining_ns = ns;
 }
 
@@ -606,10 +600,11 @@ static void npcm7xx_timer_hold_reset(Object *obj)
 qemu_irq_lower(s->watchdog_timer.irq);
 }
 
-static void npcm7xx_timer_realize(DeviceState *dev, Error **errp)
+static void npcm7xx_timer_init(Object *obj)
 {
-NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(dev);
-SysBusDevice *sbd = >parent;
+NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(obj);
+DeviceState *dev = DEVICE(obj);
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 int i;