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

[Joel Stanley]

On Fri, 21 Jun 2019 at 09:06, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> On 6/21/19 10:25 AM, Cédric Le Goater wrote:
> > 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 <address@hidden>
> >
> > 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 <address@hidden>
> >
> > Thanks,
> >
> > C.
> >
> >> Signed-off-by: Cédric Le Goater <address@hidden>

> >> --- 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) {
>
> This would be cleaner as an static inlined function in
> "hw/misc/aspeed_scu.h" IMO, maybe 'bool scu_sdram_is_reset()'.

I will take this suggestion on board in the future when I model the
watchdog reset behavior in more detail.

>
> Anyway the patch looks sane:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Thanks.

Joel