The current implementation uses nano-second precision, while
the watchdog can not be more precise than a micro-second.
Simplify by using a micro-second based timer.
Rename the timer 'timer_us' to have the unit explicit.
Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/watchdog/wdt_aspeed.h | 2 +-
hw/watchdog/wdt_aspeed.c | 24 +++++++++++++-----------
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index 819c22993a..e76a493788 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -25,7 +25,7 @@
typedef struct AspeedWDTState {
/*< private >*/
SysBusDevice parent_obj;
- QEMUTimer *timer;
+ QEMUTimer *timer_us;
/*< public >*/
MemoryRegion iomem;
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 6352ba1b0e..3fcb20f72b 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -98,23 +98,24 @@ static void aspeed_wdt_reload(AspeedWDTState *s)
uint64_t reload;
if (!(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK)) {
- reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
+ reload = muldiv64(s->regs[WDT_RELOAD_VALUE],
+ NANOSECONDS_PER_SECOND / SCALE_US,
s->pclk_freq);
} else {
- reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+ reload = s->regs[WDT_RELOAD_VALUE];
}
if (aspeed_wdt_is_enabled(s)) {
- timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+ timer_mod(s->timer_us, qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + reload);
}
}
static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
{
- uint64_t reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+ uint64_t reload = s->regs[WDT_RELOAD_VALUE];
if (aspeed_wdt_is_enabled(s)) {
- timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+ timer_mod(s->timer_us, qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + reload);
}
}
@@ -149,7 +150,7 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
awc->wdt_reload(s);
} else if (!enable && aspeed_wdt_is_enabled(s)) {
s->regs[WDT_CTRL] = data;
- timer_del(s->timer);
+ timer_del(s->timer_us);
}
break;
case WDT_RESET_WIDTH:
@@ -189,7 +190,7 @@ static const VMStateDescription vmstate_aspeed_wdt = {
.version_id = 0,
.minimum_version_id = 0,
.fields = (VMStateField[]) {
- VMSTATE_TIMER_PTR(timer, AspeedWDTState),
+ VMSTATE_TIMER_PTR(timer_us, AspeedWDTState),
VMSTATE_UINT32_ARRAY(regs, AspeedWDTState, ASPEED_WDT_REGS_MAX),
VMSTATE_END_OF_LIST()
}
@@ -214,7 +215,7 @@ static void aspeed_wdt_reset(DeviceState *dev)
s->regs[WDT_CTRL] = 0;
s->regs[WDT_RESET_WIDTH] = 0xFF;
- timer_del(s->timer);
+ timer_del(s->timer_us);
}
static void aspeed_wdt_timer_expired(void *dev)
@@ -224,7 +225,7 @@ static void aspeed_wdt_timer_expired(void *dev)
/* Do not reset on SDRAM controller reset */
if (s->scu->regs[reset_ctrl_reg] & SCU_RESET_SDRAM) {
- timer_del(s->timer);
+ timer_del(s->timer_us);
s->regs[WDT_CTRL] = 0;
return;
}
@@ -232,7 +233,7 @@ static void aspeed_wdt_timer_expired(void *dev)
qemu_log_mask(CPU_LOG_RESET, "Watchdog timer %" HWADDR_PRIx " expired.\n",
s->iomem.addr);
watchdog_perform_action();
- timer_del(s->timer);
+ timer_del(s->timer_us);
}
#define PCLK_HZ 24000000
@@ -244,7 +245,8 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
assert(s->scu);
- s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, aspeed_wdt_timer_expired, dev);
+ s->timer_us = timer_new_us(QEMU_CLOCK_VIRTUAL,
+ aspeed_wdt_timer_expired, dev);
/* FIXME: This setting should be derived from the SCU hw strapping
* register SCU70
--
2.21.3
On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > The current implementation uses nano-second precision, while > the watchdog can not be more precise than a micro-second. What's the basis for this assertion? It's true for the AST2500 and AST2600, but the AST2400 can run the watchdog from either a 1MHz clock source or the APB clock (which must be at least 16.5MHz on palmetto). The reset state on the AST2400 configures the watchdog for the APB clock rate. The Linux driver will eventually configure the watchdog for 1MHz mode regardless so perhaps the AST2400 reset state is a bit of a corner case, but I feel the assertion should be watered down a bit? Andrew
Hi Andrew, On 6/17/20 3:18 AM, Andrew Jeffery wrote: > On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: >> The current implementation uses nano-second precision, while >> the watchdog can not be more precise than a micro-second. > > What's the basis for this assertion? It's true for the AST2500 and AST2600, but > the AST2400 can run the watchdog from either a 1MHz clock source or the APB > clock (which must be at least 16.5MHz on palmetto). The reset state on the > AST2400 configures the watchdog for the APB clock rate. > > The Linux driver will eventually configure the watchdog for 1MHz mode > regardless so perhaps the AST2400 reset state is a bit of a corner case, but > I feel the assertion should be watered down a bit? What about this description? "The current implementation uses nano-second precision, but is not more precise than micro-second precision. Simplify by using a micro-second based timer. Rename the timer 'timer_us' to have the unit explicit." > > Andrew >
On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote: > Hi Andrew, > > On 6/17/20 3:18 AM, Andrew Jeffery wrote: > > On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > >> The current implementation uses nano-second precision, while > >> the watchdog can not be more precise than a micro-second. > > > > What's the basis for this assertion? It's true for the AST2500 and AST2600, but > > the AST2400 can run the watchdog from either a 1MHz clock source or the APB > > clock (which must be at least 16.5MHz on palmetto). The reset state on the > > AST2400 configures the watchdog for the APB clock rate. > > > > The Linux driver will eventually configure the watchdog for 1MHz mode > > regardless so perhaps the AST2400 reset state is a bit of a corner case, but > > I feel the assertion should be watered down a bit? > > What about this description? > > "The current implementation uses nano-second precision, but > is not more precise than micro-second precision. > Simplify by using a micro-second based timer. > Rename the timer 'timer_us' to have the unit explicit." So is this a limitation of QEMUTimer? I was establishing that the hardware can operate at greater than 1 micro-second precision. Andrew
On 6/22/20 2:21 AM, Andrew Jeffery wrote: > On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote: >> Hi Andrew, >> >> On 6/17/20 3:18 AM, Andrew Jeffery wrote: >>> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: >>>> The current implementation uses nano-second precision, while >>>> the watchdog can not be more precise than a micro-second. >>> >>> What's the basis for this assertion? It's true for the AST2500 and AST2600, but >>> the AST2400 can run the watchdog from either a 1MHz clock source or the APB >>> clock (which must be at least 16.5MHz on palmetto). The reset state on the >>> AST2400 configures the watchdog for the APB clock rate. >>> >>> The Linux driver will eventually configure the watchdog for 1MHz mode >>> regardless so perhaps the AST2400 reset state is a bit of a corner case, but >>> I feel the assertion should be watered down a bit? >> >> What about this description? >> >> "The current implementation uses nano-second precision, but >> is not more precise than micro-second precision. >> Simplify by using a micro-second based timer. >> Rename the timer 'timer_us' to have the unit explicit." > > So is this a limitation of QEMUTimer? I was establishing that the hardware can > operate at greater than 1 micro-second precision. No, I misread your comment about the AST2400 timer which can run at more than 1Mhz. The QEMUTimer doesn't have a such limitation; this patch aimed to simplify the code for reviewers, but you proved it incorrect, so let's disregard it. Thanks for your careful review! > > Andrew >
On Mon, 22 Jun 2020, at 18:13, Philippe Mathieu-Daudé wrote: > On 6/22/20 2:21 AM, Andrew Jeffery wrote: > > On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote: > >> Hi Andrew, > >> > >> On 6/17/20 3:18 AM, Andrew Jeffery wrote: > >>> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > >>>> The current implementation uses nano-second precision, while > >>>> the watchdog can not be more precise than a micro-second. > >>> > >>> What's the basis for this assertion? It's true for the AST2500 and AST2600, but > >>> the AST2400 can run the watchdog from either a 1MHz clock source or the APB > >>> clock (which must be at least 16.5MHz on palmetto). The reset state on the > >>> AST2400 configures the watchdog for the APB clock rate. > >>> > >>> The Linux driver will eventually configure the watchdog for 1MHz mode > >>> regardless so perhaps the AST2400 reset state is a bit of a corner case, but > >>> I feel the assertion should be watered down a bit? > >> > >> What about this description? > >> > >> "The current implementation uses nano-second precision, but > >> is not more precise than micro-second precision. > >> Simplify by using a micro-second based timer. > >> Rename the timer 'timer_us' to have the unit explicit." > > > > So is this a limitation of QEMUTimer? I was establishing that the hardware can > > operate at greater than 1 micro-second precision. > > No, I misread your comment about the AST2400 timer which can run > at more than 1Mhz. > > The QEMUTimer doesn't have a such limitation; this patch > aimed to simplify the code for reviewers, but you proved > it incorrect, so let's disregard it. > > Thanks for your careful review! Ah, great, I was wondering where my misunderstanding was. Thanks for clearing that up. Andrew
On 6/16/20 12:51 AM, Philippe Mathieu-Daudé wrote: > - reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND, > + reload = muldiv64(s->regs[WDT_RELOAD_VALUE], > + NANOSECONDS_PER_SECOND / SCALE_US, > s->pclk_freq); Similarly, I would prefer MICROSECONDS_PER_SECOND. Though again, I don't see what we gain from changing units. r~
© 2016 - 2026 Red Hat, Inc.