RE: [PATCH v1 2/2] aspeed/wdt: Support software reset mode for AST2600

[Jamin Lin]

Hi Cedric, 

> From: Cédric Le Goater <clg@kaod.org>
> Sent: Friday, January 24, 2025 2:45 AM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 2/2] aspeed/wdt: Support software reset mode for
> AST2600
> 
> On 1/23/25 09:19, Jamin Lin wrote:
> > On the AST2400 and AST2500 platforms, the system can only be reset by
> > enabling the WDT (Watchdog Timer) and waiting for the WDT timeout.
> > However, starting from the AST2600 platform, the reset event can be
> > triggered directly and intentionally by software, without relying on the WDT
> timeout.
> >
> > This mechanism, referred to as "software restart", is implemented in
> hardware.
> > When using the software restart mechanism, the WDT counter is not
> enabled.
> >
> > To trigger a reset generation in software mode, write 0xAEEDF123 to
> > register
> > 0x24 and software mode reset only support SOC reset mode.
> >
> > A new function, "aspeed_wdt_is_soc_reset_mode", is introduced to
> > determine whether the SoC reset mode is active.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/watchdog/wdt_aspeed.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> > 22e94e7b9c..94fb643395 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -51,11 +51,20 @@
> >   #define WDT_TIMEOUT_CLEAR               (0x14 / 4)
> >
> >   #define WDT_RESTART_MAGIC               0x4755
> > +#define WDT_SW_RESET_ENABLE             0xAEEDF123
> >
> >   #define AST2600_SCU_RESET_CONTROL1      (0x40 / 4)
> >   #define SCU_RESET_CONTROL1              (0x04 / 4)
> >   #define    SCU_RESET_SDRAM              BIT(0)
> >
> > +static bool aspeed_wdt_is_soc_reset_mode(const AspeedWDTState *s) {
> > +    uint32_t mode;
> > +
> > +    mode = extract32(s->regs[WDT_CTRL], 5, 2);
> 
> Could we test the extracted field against WDT_CTRL_RESET_MODE_SOC ?
> 
Will update it.

Thanks for review.
Jamin

> With that,
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> 
> 
> > +    return (mode == 0);
> > +}
> > +
> >   static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
> >   {
> >       return s->regs[WDT_CTRL] & WDT_CTRL_ENABLE; @@ -199,13
> +208,18
> > @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t
> data,
> >       case WDT_TIMEOUT_STATUS:
> >       case WDT_TIMEOUT_CLEAR:
> >       case WDT_RESET_MASK2:
> > -    case WDT_SW_RESET_CTRL:
> >       case WDT_SW_RESET_MASK1:
> >       case WDT_SW_RESET_MASK2:
> >           qemu_log_mask(LOG_UNIMP,
> >                         "%s: uninmplemented write at offset 0x%"
> HWADDR_PRIx "\n",
> >                         __func__, offset);
> >           break;
> > +    case WDT_SW_RESET_CTRL:
> > +        if (aspeed_wdt_is_soc_reset_mode(s) &&
> > +            (data == WDT_SW_RESET_ENABLE)) {
> > +            watchdog_perform_action();
> > +        }
> > +        break;
> >       default:
> >           qemu_log_mask(LOG_GUEST_ERROR,
> >                         "%s: Out-of-bounds write at offset 0x%"
> > HWADDR_PRIx "\n",