[PATCH qemu] wdt: aspeed: Fix failed reboot due to timer miscalculation

[Cédric Le Goater]

On 11/03/2016 06:27 AM, Andrew Jeffery wrote:
> The implementation was incorrectly scaling the reload value by
> nanoseconds per second. In reality the span represented by the reload
> value is determined by the selected clock, either APB or 1MHz.
> 
> Test the selected clock and apply the appropriate scaling, then convert to
> nanoseconds for the timer infrastructure. Note that the APB rate is hard-coded
> and reflects the value used by the timer controller, and isn't necessarily an
> accurate representation of the clock configuration programmed in the SCU.
> 
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>

ah ! reboot works ! thanks for taking some time to fix this.

> ---
> 
> I'm hesitant to send this upstream as I don't like the idea of hardcoding the
> clock rates in the way that we have (i.e. the timer as well). I think we should
> rework the timer and the watchdog to derive their clocks from the SCU, but
> that's more work than I'm willing to put in to get reboots working.

I agree that we should start being more precise in our models and use SCU
when needed. See below for a first try 

Cheers,

C.



wdt: aspeed: use scu to get clock freq

Signed-off-by: Cédric Le Goater <clg at kaod.org>
---
 hw/arm/aspeed_soc.c              |    2 ++
 hw/misc/aspeed_scu.c             |   12 ++++++++++++
 hw/watchdog/wdt_aspeed.c         |   17 +++++++++++++++--
 include/hw/misc/aspeed_scu.h     |    1 +
 include/hw/watchdog/wdt_aspeed.h |    2 ++
 5 files changed, 32 insertions(+), 2 deletions(-)

Index: qemu-aspeed.git/hw/arm/aspeed_soc.c
===================================================================
--- qemu-aspeed.git.orig/hw/arm/aspeed_soc.c
+++ qemu-aspeed.git/hw/arm/aspeed_soc.c
@@ -144,6 +144,8 @@ static void aspeed_soc_init(Object *obj)
     object_initialize(&s->wdt, sizeof(s->wdt), TYPE_ASPEED_WDT);
     object_property_add_child(obj, "wdt", OBJECT(&s->wdt), NULL);
     qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
+    object_property_add_const_link(OBJECT(&s->wdt), "scu", OBJECT(&s->scu),
+                                   NULL);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
Index: qemu-aspeed.git/hw/misc/aspeed_scu.c
===================================================================
--- qemu-aspeed.git.orig/hw/misc/aspeed_scu.c
+++ qemu-aspeed.git/hw/misc/aspeed_scu.c
@@ -266,6 +266,18 @@ bool is_supported_silicon_rev(uint32_t s
     return false;
 }
 
+#define ASPEED_PLL_25MHZ    25000000
+#define ASPEED_PLL_24MHZ    24000000
+#define ASPEED_PLL_12MHZ    12000000
+
+uint32_t aspeed_scu_get_clk(AspeedSCUState *scu)
+{
+    if (scu->hw_strap1 & AST2400_CLK_25M_IN)
+        return ASPEED_PLL_25MHZ;
+    else
+        return ASPEED_PLL_24MHZ;
+}
+
 static void aspeed_scu_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
Index: qemu-aspeed.git/hw/watchdog/wdt_aspeed.c
===================================================================
--- qemu-aspeed.git.orig/hw/watchdog/wdt_aspeed.c
+++ qemu-aspeed.git/hw/watchdog/wdt_aspeed.c
@@ -7,10 +7,12 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "sysemu/watchdog.h"
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/watchdog/wdt_aspeed.h"
+#include "hw/misc/aspeed_scu.h"
 
 #define WDT_IO_REGION_SIZE 0x1000
 
@@ -87,7 +89,7 @@ static void aspeed_wdt_write(void *opaqu
 
             if (pclk) {
                 reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
-                        PCLK_HZ) ;
+                        s->pclk_freq) ;
             } else {
                 reload = s->reg_reload_value * 1000;
             }
@@ -104,7 +106,7 @@ static void aspeed_wdt_write(void *opaqu
 
             if (pclk) {
                 reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
-                        PCLK_HZ) ;
+                        s->pclk_freq) ;
             } else {
                 reload = s->reg_reload_value * 1000;
             }
@@ -183,10 +185,21 @@ static void aspeed_wdt_realize(DeviceSta
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedWDTState *s = ASPEED_WDT(dev);
+    Object *obj;
+    Error *err = NULL;
 
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, aspeed_wdt_timer_expired,
                             dev);
 
+    obj = object_property_get_link(OBJECT(dev), "scu", &err);
+    if (!obj) {
+        error_setg(errp, "%s: required link 'scu' not found: %s",
+                   __func__, error_get_pretty(err));
+        return;
+    }
+
+    s->pclk_freq = aspeed_scu_get_clk(ASPEED_SCU(obj));
+
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
                           TYPE_ASPEED_WDT, WDT_IO_REGION_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
Index: qemu-aspeed.git/include/hw/misc/aspeed_scu.h
===================================================================
--- qemu-aspeed.git.orig/include/hw/misc/aspeed_scu.h
+++ qemu-aspeed.git/include/hw/misc/aspeed_scu.h
@@ -36,6 +36,7 @@ typedef struct AspeedSCUState {
 #define AST2500_A1_SILICON_REV   0x04010303U
 
 extern bool is_supported_silicon_rev(uint32_t silicon_rev);
+extern uint32_t aspeed_scu_get_clk(AspeedSCUState *scu);
 
 /*
  * Extracted from Aspeed SDK v00.03.21. Fixes and extra definitions
Index: qemu-aspeed.git/include/hw/watchdog/wdt_aspeed.h
===================================================================
--- qemu-aspeed.git.orig/include/hw/watchdog/wdt_aspeed.h
+++ qemu-aspeed.git/include/hw/watchdog/wdt_aspeed.h
@@ -36,6 +36,8 @@ typedef struct AspeedWDTState {
     uint32_t reg_reload_value;
     uint32_t reg_restart;
     uint32_t reg_ctrl;
+
+    uint32_t pclk_freq;
 } AspeedWDTState;
 
 #endif  /* ASPEED_WDT_H */