[PATCH v3 2/2] watchdog: aspeed: Add support for SW restart

[Chin-Ting Kuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew at codeconstruct.com.au>
> Sent: Thursday, October 31, 2024 8:04 AM
> Subject: Re: [PATCH v3 2/2] watchdog: aspeed: Add support for SW restart
> 
> On Wed, 2024-10-30 at 18:47 +0800, Chin-Ting Kuo wrote:
> > Since AST2600, except for HW WDT counter timeout, HW WDT reset can
> > also be triggered by just cinfiguring some HW registers by SW
> > directly. We named it "SW restart".
> > Although it is "SW" restart, its mechanism is implemented by HW.
> >
> > Originally, system can only know it is reset by WDT through a reset
> > flag. However, since AST2600, SW can trigger the reset event
> > consciously and directly without wait for WDT timeout. WDT counter is
> > not enabled when SW restart is adopted. After that, an independent
> > reset event flag will be set after systemis reset by SW.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo at aspeedtech.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 40
> > +++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c
> > b/drivers/watchdog/aspeed_wdt.c index add76be3ee42..1e9808d42023
> > 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -42,6 +42,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot
> be
> > stopped once started (default="
> >
> >  #define WDT_REG_OFFSET_MASK            0x00000fff
> >
> > +/* WDT behavior control flag */
> > +#define WDT_RESTART_SYSTEM_SW_SUPPORT  0x00000001
> > +
> >  struct aspeed_wdt_scu {
> >         const char *compatible;
> >         u32 reset_status_reg;
> > @@ -55,6 +58,7 @@ struct aspeed_wdt_config {
> >         u32 irq_shift;
> >         u32 irq_mask;
> >         u32 reg_size;
> > +       u32 flags;
> 
> Why add the flags member rather than change the restart callback for the
> 2600? The latter seems more direct to me.
> 

Restart callback function will be better and it will be added in the next patch.

> >         struct aspeed_wdt_scu scu;
> >  };
> >
> > @@ -71,6 +75,7 @@ static const struct aspeed_wdt_config ast2400_config
> > = {
> >         .irq_shift = 0,
> >         .irq_mask = 0,
> >         .reg_size = 0x20,
> > +       .flags = 0,
> >         .scu = {
> >                 .compatible = "aspeed,ast2400-scu",
> >                 .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS, @@
> > -85,6 +90,7 @@ static const struct aspeed_wdt_config ast2500_config =
> > {
> >         .irq_shift = 12,
> >         .irq_mask = GENMASK(31, 12),
> >         .reg_size = 0x20,
> > +       .flags = 0,
> >         .scu = {
> >                 .compatible = "aspeed,ast2500-scu",
> >                 .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS, @@
> > -99,6 +105,7 @@ static const struct aspeed_wdt_config ast2600_config =
> > {
> >         .irq_shift = 0,
> >         .irq_mask = GENMASK(31, 10),
> >         .reg_size = 0x40,
> > +       .flags = WDT_RESTART_SYSTEM_SW_SUPPORT,
> >         .scu = {
> >                 .compatible = "aspeed,ast2600-scu",
> >                 .reset_status_reg =
> AST2600_SCU_SYS_RESET_STATUS, @@
> > -136,6 +143,11 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> >  #define   WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION    B
> IT(0)
> >  #define WDT_RESET_MASK1                0x1c
> >  #define WDT_RESET_MASK2                0x20
> > +#define WDT_SW_RESET_CTRL      0x24
> > +#define   WDT_SW_RESET_COUNT_CLEAR     0xDEADDEAD
> #define
> > +WDT_SW_RESET_ENABLE  0xAEEDF123 #define
> WDT_SW_RESET_MASK1     0x28
> > +#define WDT_SW_RESET_MASK2     0x2c
> >
> >  /*
> >   * WDT_RESET_WIDTH controls the characteristics of the external pulse
> > (if @@ -255,10 +267,31 @@ static int aspeed_wdt_set_pretimeout(struct
> > watchdog_device *wdd,
> >         return 0;
> >  }
> >
> > +static void aspeed_wdt_sw_reset(struct watchdog_device *wdd) {
> > +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > +       u32 ctrl = WDT_CTRL_RESET_MODE_SOC |
> > +                  WDT_CTRL_RESET_SYSTEM;
> > +
> > +       writel(ctrl, wdt->base + WDT_CTRL);
> > +       writel(WDT_SW_RESET_COUNT_CLEAR,
> > +              wdt->base + WDT_SW_RESET_CTRL);
> > +       writel(WDT_SW_RESET_ENABLE, wdt->base +
> WDT_SW_RESET_CTRL);
> > +
> > +       /* system must be reset immediately */
> > +       mdelay(1000);
> > +}
> > +
> >  static int aspeed_wdt_restart(struct watchdog_device *wdd,
> >                               unsigned long action, void
> *data)
> >  {
> >         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > +       const struct aspeed_wdt_config *cfg = wdt->cfg;
> > +
> > +       if (cfg->flags & WDT_RESTART_SYSTEM_SW_SUPPORT) {
> > +               aspeed_wdt_sw_reset(wdd);
> > +               return 0;
> > +       }
> >
> >         wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY;
> >         aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000); @@
> -529,6
> > +562,13 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >                         if (nrstmask > 1)
> >                                 writel(reset_mask[1],
> wdt->base +
> > WDT_RESET_MASK2);
> >                 }
> > +
> > +               if (wdt->cfg->flags &
> WDT_RESTART_SYSTEM_SW_SUPPORT)
> 
> This condition could be a match against the compatible if you drop the flags
> member.
> 
> Or, assuming the software reset masks are not adjusted elsewhere, move the
> copies below into the ast2600-specific restart callback implementation?
> 

Okay, it will be moved into the ast2600-specific restart callback function.

> Andrew
> 
> > {
> > +                       reg = readl(wdt->base +
> WDT_RESET_MASK1);
> > +                       writel(reg, wdt->base +
> WDT_SW_RESET_MASK1);
> > +                       reg = readl(wdt->base +
> WDT_RESET_MASK2);
> > +                       writel(reg, wdt->base +
> WDT_SW_RESET_MASK2);
> > +               }
> >         }
> >
> >         if (!of_property_read_u32(np, "aspeed,ext-pulse-duration",
> > &duration)) {

Chin-Ting