Re: [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog

["Cédric Le Goater" <clg@kaod.org>]

On 21/06/2019 08:52, Joel Stanley wrote:
> The ast2500 uses the watchdog to reset the SDRAM controller. This
> operation is usually performed by u-boot's memory training procedure,
> and it is enabled by setting a bit in the SCU and then causing the
> watchdog to expire. Therefore, we need the watchdog to be able to
> access the SCU's register space.
> 
> This causes the watchdog to not perform a system reset when the bit is
> set. In the future it could perform a reset of the SDMC model.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

I was keeping this patch in my tree (hence the Sob) hoping that
someone could find the time to study the reset question. But this 
patch is useful as it is and I think we should merge it.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> v2: rebase on upstream, rework commit message
> ---
>  hw/arm/aspeed_soc.c              |  2 ++
>  hw/watchdog/wdt_aspeed.c         | 20 ++++++++++++++++++++
>  include/hw/watchdog/wdt_aspeed.h |  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index a2ea8c748449..ddd5dfacd7d6 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -155,6 +155,8 @@ static void aspeed_soc_init(Object *obj)
>                                sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
>          qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
>                                      sc->info->silicon_rev);
> +        object_property_add_const_link(OBJECT(&s->wdt[i]), "scu",
> +                                       OBJECT(&s->scu), &error_abort);
>      }
>  
>      sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 4a8409f0daf5..57fe24ae6b1f 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -44,6 +44,9 @@
>  
>  #define WDT_RESTART_MAGIC               0x4755
>  
> +#define SCU_RESET_CONTROL1              (0x04 / 4)
> +#define    SCU_RESET_SDRAM              BIT(0)
> +
>  static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
>  {
>      return s->regs[WDT_CTRL] & WDT_CTRL_ENABLE;
> @@ -222,6 +225,13 @@ static void aspeed_wdt_timer_expired(void *dev)
>  {
>      AspeedWDTState *s = ASPEED_WDT(dev);
>  
> +    /* Do not reset on SDRAM controller reset */
> +    if (s->scu->regs[SCU_RESET_CONTROL1] & SCU_RESET_SDRAM) {
> +        timer_del(s->timer);
> +        s->regs[WDT_CTRL] = 0;
> +        return;
> +    }
> +
>      qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
>      watchdog_perform_action();
>      timer_del(s->timer);
> @@ -233,6 +243,16 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      AspeedWDTState *s = ASPEED_WDT(dev);
> +    Error *err = NULL;
> +    Object *obj;
> +
> +    obj = object_property_get_link(OBJECT(dev), "scu", &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'scu' not found: ");
> +        return;
> +    }
> +    s->scu = ASPEED_SCU(obj);
>  
>      if (!is_supported_silicon_rev(s->silicon_rev)) {
>          error_setg(errp, "Unknown silicon revision: 0x%" PRIx32,
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index 88d8be4f78d6..daef0c0e230b 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -27,6 +27,7 @@ typedef struct AspeedWDTState {
>      MemoryRegion iomem;
>      uint32_t regs[ASPEED_WDT_REGS_MAX];
>  
> +    AspeedSCUState *scu;
>      uint32_t pclk_freq;
>      uint32_t silicon_rev;
>      uint32_t ext_pulse_width_mask;
>