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

[Andrew Jeffery]

On Thu, 2016-11-03 at 16:09 +1030, Joel Stanley wrote:
> 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.

So I assume you mean the Linux driver here; certainly it's configured
that way.

> 
> It looks like the model needs to be updated with this knowledge. Then
> we don't need to worry about hardcoding. 

Aside from that this suggestion is just hardcoding something
different...

> We print a warning if the bit
> in the register that says "use 1MHz" is cleared.

... as I assume you mean we carry on clocking as if in 1MHz mode (given
the alternative is "undefined")? Whilst the fact that we're arguing
between two hacks might make the discussion moot, I don't see why qemu
needs to know anything about what Linux might be doing. As such I
prefer not encoding that information in the model.

Cheers,

Andrew

> 
> 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
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161103/f65e4253/attachment.sig>