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

Andrew Jeffery andrew at aj.id.au
Fri Nov 4 12:49:17 AEDT 2016


On Thu, 2016-11-03 at 09:02 +0100, Cédric Le Goater wrote:
> 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 */
> 
> 

I think we need a bit more magic in hw/arm/aspeed.c to create the SCU
link, but other than that it seems reasonable to me. A while back this
was sent to the list:

https://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg00867.html

But I don't think it ever got applied. Otherwise we could be
implementing the above in terms of struct qemu_clk.

Finally I think if we're going to do something like what you've got
above we should fix the timer as well. Should I add the timer work to
my list? Do we want to gate this reboot fix on preparing timer patches?

Cheers,

Andrew
-------------- 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/20161104/385d0f91/attachment.sig>


More information about the openbmc mailing list