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

[Joel Stanley]

On Thu, Nov 3, 2016 at 3:57 PM, Andrew Jeffery <andrew at aj.id.au> 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>
> ---
>
> 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.

The watchdog driver as upstreamed always runs from a 1MHz clock.

It looks like the model needs to be updated with this knowledge. Then
we don't need to worry about hardcoding. We print a warning if the bit
in the register that says "use 1MHz" is cleared.

Cheers,

Joel

>
>  hw/watchdog/wdt_aspeed.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 2ec01769b745..c81608d6ac74 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -61,11 +61,14 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
>
>  }
>
> +#define PCLK_HZ 24000000
> +
>  static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>                                  unsigned size)
>  {
>      AspeedWDTState *s = ASPEED_WDT(opaque);
>      bool en = data & BIT(0);
> +    bool pclk = !(data & BIT(4));
>
>      switch (offset) {
>      case WDT_STATUS:
> @@ -78,21 +81,36 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>          break;
>      case WDT_RESTART:
>          if ((data & 0xFFFF) == 0x4755) {
> +            uint32_t reload;
> +
>              s->reg_status = s->reg_reload_value;
>
> +            if (pclk) {
> +                reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
> +                        PCLK_HZ) ;
> +            } else {
> +                reload = s->reg_reload_value * 1000;
> +            }
> +
>              if (s->enabled) {
>                  timer_mod(s->timer,
> -                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -                          s->reg_reload_value * NANOSECONDS_PER_SECOND);
> +                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
>              }
>          }
>          break;
>      case WDT_CTRL:
>          if (en && !s->enabled) {
> +            uint32_t reload;
> +
> +            if (pclk) {
> +                reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
> +                        PCLK_HZ) ;
> +            } else {
> +                reload = s->reg_reload_value * 1000;
> +            }
> +
>              s->enabled = true;
> -            timer_mod(s->timer,
> -                      qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -                      s->reg_reload_value * NANOSECONDS_PER_SECOND);
> +            timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
>          }
>          else if (!en && s->enabled) {
>              s->enabled = false;
> --
> 2.7.4
>