Re: [PATCH v2 4/4] watchdog/aspeed: Fix AST2600 frequency behaviour
Joel Stanley writes:
> The AST2600 control register sneakily changed the meaning of bit 4
> without anyone noticing. It no longer controls the 1MHz vs APB clock
> select, and instead always runs at 1MHz.
>
> The AST2500 was always 1MHz too, but it retained bit 4, making it read
> only. We can model both using the same fixed 1MHz calculation.
>
> Fixes: 6b2b2a703cad ("hw: wdt_aspeed: Add AST2600 support")
> Reviewed-by: Cédric Le Goater
> Signed-off-by: Joel Stanley
Reviewed-by: Alex Bennée
> ---
> v2: Fix Fixes line in commit message
> ---
> hw/watchdog/wdt_aspeed.c | 21 +
> include/hw/watchdog/wdt_aspeed.h | 1 +
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 8787c5ad0f97..9a8a2200fd8e 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -93,11 +93,11 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr
> offset, unsigned size)
>
> }
>
> -static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
> +static void aspeed_wdt_reload(AspeedWDTState *s)
> {
> uint64_t reload;
>
> -if (pclk) {
> +if (!(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK)) {
> reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
>s->pclk_freq);
> } else {
> @@ -109,6 +109,16 @@ static void aspeed_wdt_reload(AspeedWDTState *s, bool
> pclk)
> }
> }
>
> +static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
> +{
> +uint64_t reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
> +
> +if (aspeed_wdt_is_enabled(s)) {
> +timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
> +}
> +}
> +
> +
> static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
> unsigned size)
> {
> @@ -130,13 +140,13 @@ static void aspeed_wdt_write(void *opaque, hwaddr
> offset, uint64_t data,
> case WDT_RESTART:
> if ((data & 0x) == WDT_RESTART_MAGIC) {
> s->regs[WDT_STATUS] = s->regs[WDT_RELOAD_VALUE];
> -aspeed_wdt_reload(s, !(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK));
> +awc->wdt_reload(s);
> }
> break;
> case WDT_CTRL:
> if (enable && !aspeed_wdt_is_enabled(s)) {
> s->regs[WDT_CTRL] = data;
> -aspeed_wdt_reload(s, !(data & WDT_CTRL_1MHZ_CLK));
> +awc->wdt_reload(s);
> } else if (!enable && aspeed_wdt_is_enabled(s)) {
> s->regs[WDT_CTRL] = data;
> timer_del(s->timer);
> @@ -283,6 +293,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass
> *klass, void *data)
> awc->offset = 0x20;
> awc->ext_pulse_width_mask = 0xff;
> awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
> +awc->wdt_reload = aspeed_wdt_reload;
> }
>
> static const TypeInfo aspeed_2400_wdt_info = {
> @@ -317,6 +328,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass
> *klass, void *data)
> awc->ext_pulse_width_mask = 0xf;
> awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
> awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> +awc->wdt_reload = aspeed_wdt_reload_1mhz;
> }
>
> static const TypeInfo aspeed_2500_wdt_info = {
> @@ -336,6 +348,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass
> *klass, void *data)
> awc->ext_pulse_width_mask = 0xf; /* TODO */
> awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
> awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> +awc->wdt_reload = aspeed_wdt_reload_1mhz;
> }
>
> static const TypeInfo aspeed_2600_wdt_info = {
> diff --git a/include/hw/watchdog/wdt_aspeed.h
> b/include/hw/watchdog/wdt_aspeed.h
> index dfedd7662dd1..819c22993a6e 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -47,6 +47,7 @@ typedef struct AspeedWDTClass {
> uint32_t ext_pulse_width_mask;
> uint32_t reset_ctrl_reg;
> void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
> +void (*wdt_reload)(AspeedWDTState *s);
> } AspeedWDTClass;
>
> #endif /* WDT_ASPEED_H */
--
Alex Bennée
[PATCH v2 4/4] watchdog/aspeed: Fix AST2600 frequency behaviour
The AST2600 control register sneakily changed the meaning of bit 4
without anyone noticing. It no longer controls the 1MHz vs APB clock
select, and instead always runs at 1MHz.
The AST2500 was always 1MHz too, but it retained bit 4, making it read
only. We can model both using the same fixed 1MHz calculation.
Fixes: 6b2b2a703cad ("hw: wdt_aspeed: Add AST2600 support")
Reviewed-by: Cédric Le Goater
Signed-off-by: Joel Stanley
---
v2: Fix Fixes line in commit message
---
hw/watchdog/wdt_aspeed.c | 21 +
include/hw/watchdog/wdt_aspeed.h | 1 +
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 8787c5ad0f97..9a8a2200fd8e 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -93,11 +93,11 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr
offset, unsigned size)
}
-static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
+static void aspeed_wdt_reload(AspeedWDTState *s)
{
uint64_t reload;
-if (pclk) {
+if (!(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK)) {
reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
s->pclk_freq);
} else {
@@ -109,6 +109,16 @@ static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
}
}
+static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
+{
+uint64_t reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+
+if (aspeed_wdt_is_enabled(s)) {
+timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+}
+}
+
+
static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
unsigned size)
{
@@ -130,13 +140,13 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset,
uint64_t data,
case WDT_RESTART:
if ((data & 0x) == WDT_RESTART_MAGIC) {
s->regs[WDT_STATUS] = s->regs[WDT_RELOAD_VALUE];
-aspeed_wdt_reload(s, !(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK));
+awc->wdt_reload(s);
}
break;
case WDT_CTRL:
if (enable && !aspeed_wdt_is_enabled(s)) {
s->regs[WDT_CTRL] = data;
-aspeed_wdt_reload(s, !(data & WDT_CTRL_1MHZ_CLK));
+awc->wdt_reload(s);
} else if (!enable && aspeed_wdt_is_enabled(s)) {
s->regs[WDT_CTRL] = data;
timer_del(s->timer);
@@ -283,6 +293,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass,
void *data)
awc->offset = 0x20;
awc->ext_pulse_width_mask = 0xff;
awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
+awc->wdt_reload = aspeed_wdt_reload;
}
static const TypeInfo aspeed_2400_wdt_info = {
@@ -317,6 +328,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass,
void *data)
awc->ext_pulse_width_mask = 0xf;
awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
+awc->wdt_reload = aspeed_wdt_reload_1mhz;
}
static const TypeInfo aspeed_2500_wdt_info = {
@@ -336,6 +348,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass,
void *data)
awc->ext_pulse_width_mask = 0xf; /* TODO */
awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
+awc->wdt_reload = aspeed_wdt_reload_1mhz;
}
static const TypeInfo aspeed_2600_wdt_info = {
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index dfedd7662dd1..819c22993a6e 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -47,6 +47,7 @@ typedef struct AspeedWDTClass {
uint32_t ext_pulse_width_mask;
uint32_t reset_ctrl_reg;
void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
+void (*wdt_reload)(AspeedWDTState *s);
} AspeedWDTClass;
#endif /* WDT_ASPEED_H */
--
2.24.0
