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

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@aspeedtech.com>
---
 drivers/watchdog/aspeed_wdt.c | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

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@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.

>         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    BIT(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?

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)) {

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@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@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