[PATCH v3 0/2] Update ASPEED WDT bootstatus

[PATCH v3 0/2] Update ASPEED WDT bootstatus

[Chin-Ting Kuo]

This patch series inherits the patch submitted by Peter.
https://patchwork.kernel.org/project/linux-watchdog/patch/20240430143114.1323686-2-peteryin.openbmc@gmail.com/
Besides, the boot status modififed in the WDT driver
obeys the rules proposed in the OpenBMC.
https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-update.md#proposed-design
Moreover, WDT SW restart mechanism is supported by HW
since AST2600 platform and is also included in this
patch series.

Changes in v2:
  - Support SW restart on AST2600 by default without
    adding any dts property.

Changes in v3:
  - Get watchdog controller index by dividing register
    base offset by register size.

Chin-Ting Kuo (2):
  watchdog: aspeed: Update bootstatus handling
  watchdog: aspeed: Add support for SW restart

 drivers/watchdog/aspeed_wdt.c | 154 ++++++++++++++++++++++++++++++++--
 1 file changed, 146 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Update the bootstatus according to the latest design guide
from the OpenBMC shown as below.
https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-update.md#proposed-design

In short,
- WDIOF_EXTERN1   => system is reset by Software
- WDIOF_CARDRESET => system is reset by WDT
- Others          => other reset events, e.g., power on reset.

On AST2400 platform, only a bit, SCU3C[1], represents that the
system is reset by WDT1 or WDT2.

On AST2500 platform, SCU3C[4:2] are WDT reset flags.
  SCU3C[4]: system is reset by WDT3.
  SCU3C[3]: system is reset by WDT2.
  SCU3C[2]: system is reset by WDT1.

On AST2600 platform, SCU074[31:16] are WDT reset flags.
  SCU074[31:28]: system is reset by WDT4
    SCU074[31]: system is reset by WDT4 software reset.
  SCU074[27:24]: system is reset by WDT3
    SCU074[27]: system is reset by WDT3 software reset.
  SCU074[23:20]: system is reset by WDT2
    SCU074[23]: system is reset by WDT2 software reset.
  SCU074[19:16]: system is reset by WDT1
    SCU074[19]: system is reset by WDT1 software reset.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/watchdog/aspeed_wdt.c | 114 +++++++++++++++++++++++++++++++---
 1 file changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index b4773a6aaf8c..add76be3ee42 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -11,10 +11,12 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/kstrtox.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/watchdog.h>
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -22,15 +24,44 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+
+/* AST SCU Register for System Reset Event Log Register Set
+ * ast2600 is scu074 ast2400/2500 is scu03c
+ */
+#define AST2400_SCU_SYS_RESET_STATUS	0x3c
+#define   AST2400_SCU_SYS_RESET_WDT_MASK	0x1
+#define   AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT	1
+
+#define   AST2500_SCU_SYS_RESET_WDT_MASK	0x1
+#define   AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT	2
+
+#define AST2600_SCU_SYS_RESET_STATUS	0x74
+#define   AST2600_SCU_SYS_RESET_WDT_MASK	0xf
+#define   AST2600_SCU_SYS_RESET_WDT_SW_MASK	0x8
+#define   AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT	16
+
+#define WDT_REG_OFFSET_MASK		0x00000fff
+
+struct aspeed_wdt_scu {
+	const char *compatible;
+	u32 reset_status_reg;
+	u32 wdt_reset_mask;
+	u32 wdt_sw_reset_mask;
+	u32 wdt_reset_mask_shift;
+};
+
 struct aspeed_wdt_config {
 	u32 ext_pulse_width_mask;
 	u32 irq_shift;
 	u32 irq_mask;
+	u32 reg_size;
+	struct aspeed_wdt_scu scu;
 };
 
 struct aspeed_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*base;
+	int			idx;
 	u32			ctrl;
 	const struct aspeed_wdt_config *cfg;
 };
@@ -39,18 +70,42 @@ static const struct aspeed_wdt_config ast2400_config = {
 	.ext_pulse_width_mask = 0xff,
 	.irq_shift = 0,
 	.irq_mask = 0,
+	.reg_size = 0x20,
+	.scu = {
+		.compatible = "aspeed,ast2400-scu",
+		.reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
+		.wdt_reset_mask = AST2400_SCU_SYS_RESET_WDT_MASK,
+		.wdt_sw_reset_mask = 0,
+		.wdt_reset_mask_shift = AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT,
+	},
 };
 
 static const struct aspeed_wdt_config ast2500_config = {
 	.ext_pulse_width_mask = 0xfffff,
 	.irq_shift = 12,
 	.irq_mask = GENMASK(31, 12),
+	.reg_size = 0x20,
+	.scu = {
+		.compatible = "aspeed,ast2500-scu",
+		.reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
+		.wdt_reset_mask = AST2500_SCU_SYS_RESET_WDT_MASK,
+		.wdt_sw_reset_mask = 0,
+		.wdt_reset_mask_shift = AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT,
+	},
 };
 
 static const struct aspeed_wdt_config ast2600_config = {
 	.ext_pulse_width_mask = 0xfffff,
 	.irq_shift = 0,
 	.irq_mask = GENMASK(31, 10),
+	.reg_size = 0x40,
+	.scu = {
+		.compatible = "aspeed,ast2600-scu",
+		.reset_status_reg = AST2600_SCU_SYS_RESET_STATUS,
+		.wdt_reset_mask = AST2600_SCU_SYS_RESET_WDT_MASK,
+		.wdt_sw_reset_mask = AST2600_SCU_SYS_RESET_WDT_SW_MASK,
+		.wdt_reset_mask_shift = AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT,
+	},
 };
 
 static const struct of_device_id aspeed_wdt_of_table[] = {
@@ -213,6 +268,51 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
 	return 0;
 }
 
+static int aspeed_wdt_get_bootstatus(struct device *dev,
+				     struct aspeed_wdt *wdt)
+{
+	struct device_node *np = dev->of_node;
+	struct aspeed_wdt_scu scu = wdt->cfg->scu;
+	struct regmap *scu_base;
+	u32 reset_mask_width;
+	u32 reset_mask_shift;
+	u32 status;
+	int ret;
+
+	wdt->idx = ((u32)wdt->base & WDT_REG_OFFSET_MASK) /
+		   wdt->cfg->reg_size;
+
+	/* On ast2400, only a bit is used to represent WDT reset */
+	if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
+		wdt->idx = 0;
+
+	scu_base = syscon_regmap_lookup_by_compatible(scu.compatible);
+	if (IS_ERR(scu_base))
+		return PTR_ERR(scu_base);
+
+	ret = regmap_read(scu_base, scu.reset_status_reg, &status);
+	if (ret)
+		return ret;
+
+	reset_mask_width = hweight32(scu.wdt_reset_mask);
+	reset_mask_shift = scu.wdt_reset_mask_shift +
+			   reset_mask_width * wdt->idx;
+
+	if (status & (scu.wdt_sw_reset_mask << reset_mask_shift))
+		wdt->wdd.bootstatus = WDIOF_EXTERN1;
+	else if (status & (scu.wdt_reset_mask << reset_mask_shift))
+		wdt->wdd.bootstatus = WDIOF_CARDRESET;
+	else
+		wdt->wdd.bootstatus = 0;
+
+	ret = regmap_write(scu_base, scu.reset_status_reg,
+			   scu.wdt_reset_mask << reset_mask_shift);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /* access_cs0 shows if cs0 is accessible, hence the reverted bit */
 static ssize_t access_cs0_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
@@ -312,7 +412,6 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	struct device_node *np;
 	const char *reset_type;
 	u32 duration;
-	u32 status;
 	int ret;
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
@@ -458,14 +557,13 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		writel(duration - 1, wdt->base + WDT_RESET_WIDTH);
 	}
 
-	status = readl(wdt->base + WDT_TIMEOUT_STATUS);
-	if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) {
-		wdt->wdd.bootstatus = WDIOF_CARDRESET;
+	ret = aspeed_wdt_get_bootstatus(dev, wdt);
+	if (ret)
+		return ret;
 
-		if (of_device_is_compatible(np, "aspeed,ast2400-wdt") ||
-		    of_device_is_compatible(np, "aspeed,ast2500-wdt"))
-			wdt->wdd.groups = bswitch_groups;
-	}
+	if (of_device_is_compatible(np, "aspeed,ast2400-wdt") ||
+	    of_device_is_compatible(np, "aspeed,ast2500-wdt"))
+		wdt->wdd.groups = bswitch_groups;
 
 	dev_set_drvdata(dev, wdt);
 
-- 
2.34.1

Re: [PATCH v3 1/2] watchdog: aspeed: Update bootstatus handling

[Andrew Jeffery]

Hello,

On Wed, 2024-10-30 at 18:47 +0800, Chin-Ting Kuo wrote:
> Update the bootstatus according to the latest design guide
> from the OpenBMC shown as below.
> https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-update.md#proposed-design
> 
> In short,
> - WDIOF_EXTERN1   => system is reset by Software
> - WDIOF_CARDRESET => system is reset by WDT
> - Others          => other reset events, e.g., power on reset.

...

> 
> On AST2400 platform, only a bit, SCU3C[1], represents that the
> system is reset by WDT1 or WDT2.
> 
> On AST2500 platform, SCU3C[4:2] are WDT reset flags.
>   SCU3C[4]: system is reset by WDT3.
>   SCU3C[3]: system is reset by WDT2.
>   SCU3C[2]: system is reset by WDT1.
> 
> On AST2600 platform, SCU074[31:16] are WDT reset flags.
>   SCU074[31:28]: system is reset by WDT4
>     SCU074[31]: system is reset by WDT4 software reset.
>   SCU074[27:24]: system is reset by WDT3
>     SCU074[27]: system is reset by WDT3 software reset.
>   SCU074[23:20]: system is reset by WDT2
>     SCU074[23]: system is reset by WDT2 software reset.
>   SCU074[19:16]: system is reset by WDT1
>     SCU074[19]: system is reset by WDT1 software reset.

This information emerges in the code (though it's a bit of a maze back
through the derivations). I'd rather you discuss how the observable
behaviour of the driver changes (with respect to booting from the
alternate boot region) and what choices you've made when the hardware
doesn't tell us whether this is a (graceful) software reset.

> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 114 +++++++++++++++++++++++++++++++-
> --
>  1 file changed, 106 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c
> b/drivers/watchdog/aspeed_wdt.c
> index b4773a6aaf8c..add76be3ee42 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -11,10 +11,12 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/kstrtox.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/watchdog.h>
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -22,15 +24,44 @@ module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> (default="
>                                 __MODULE_STRING(WATCHDOG_NOWAYOUT)
> ")");
>  
> +
> +/* AST SCU Register for System Reset Event Log Register Set
> + * ast2600 is scu074 ast2400/2500 is scu03c
> + */
> +#define AST2400_SCU_SYS_RESET_STATUS   0x3c
> +#define   AST2400_SCU_SYS_RESET_WDT_MASK       0x1
> +#define   AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT 1
> +
> +#define   AST2500_SCU_SYS_RESET_WDT_MASK       0x1
> +#define   AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT 2
> +
> +#define AST2600_SCU_SYS_RESET_STATUS   0x74
> +#define   AST2600_SCU_SYS_RESET_WDT_MASK       0xf
> +#define   AST2600_SCU_SYS_RESET_WDT_SW_MASK    0x8
> +#define   AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT 16

I think for the most part these macros are only used to assign into the
config data provided through the match struct? I kept having to scroll
back-and-forth to track the values. I feel it might be better in this
instance to _not_ define macros for them and assign the literals
straight into the struct members down below.

> +
> +#define WDT_REG_OFFSET_MASK            0x00000fff
> +
> +struct aspeed_wdt_scu {
> +       const char *compatible;
> +       u32 reset_status_reg;
> +       u32 wdt_reset_mask;
> +       u32 wdt_sw_reset_mask;
> +       u32 wdt_reset_mask_shift;
> +};
> +
>  struct aspeed_wdt_config {
>         u32 ext_pulse_width_mask;
>         u32 irq_shift;
>         u32 irq_mask;
> +       u32 reg_size;

This is already specified in the devicetree, though admittedly aspeed-
g4.dtsi sets the size to 0x1c, which is a little unhelpful for your
calculations. I guess we can leave it for now.

> +       struct aspeed_wdt_scu scu;
>  };
>  
>  struct aspeed_wdt {
>         struct watchdog_device  wdd;
>         void __iomem            *base;
> +       int                     idx;
>         u32                     ctrl;
>         const struct aspeed_wdt_config *cfg;
>  };
> @@ -39,18 +70,42 @@ static const struct aspeed_wdt_config
> ast2400_config = {
>         .ext_pulse_width_mask = 0xff,
>         .irq_shift = 0,
>         .irq_mask = 0,
> +       .reg_size = 0x20,
> +       .scu = {
> +               .compatible = "aspeed,ast2400-scu",
> +               .reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
> +               .wdt_reset_mask = AST2400_SCU_SYS_RESET_WDT_MASK,
> +               .wdt_sw_reset_mask = 0,
> +               .wdt_reset_mask_shift =
> AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT,
> +       },
>  };
>  
>  static const struct aspeed_wdt_config ast2500_config = {
>         .ext_pulse_width_mask = 0xfffff,
>         .irq_shift = 12,
>         .irq_mask = GENMASK(31, 12),
> +       .reg_size = 0x20,
> +       .scu = {
> +               .compatible = "aspeed,ast2500-scu",
> +               .reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
> +               .wdt_reset_mask = AST2500_SCU_SYS_RESET_WDT_MASK,
> +               .wdt_sw_reset_mask = 0,
> +               .wdt_reset_mask_shift =
> AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT,
> +       },
>  };
>  
>  static const struct aspeed_wdt_config ast2600_config = {
>         .ext_pulse_width_mask = 0xfffff,
>         .irq_shift = 0,
>         .irq_mask = GENMASK(31, 10),
> +       .reg_size = 0x40,
> +       .scu = {
> +               .compatible = "aspeed,ast2600-scu",
> +               .reset_status_reg = AST2600_SCU_SYS_RESET_STATUS,
> +               .wdt_reset_mask = AST2600_SCU_SYS_RESET_WDT_MASK,
> +               .wdt_sw_reset_mask =
> AST2600_SCU_SYS_RESET_WDT_SW_MASK,
> +               .wdt_reset_mask_shift =
> AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT,
> +       },
>  };
>  
>  static const struct of_device_id aspeed_wdt_of_table[] = {
> @@ -213,6 +268,51 @@ static int aspeed_wdt_restart(struct
> watchdog_device *wdd,
>         return 0;
>  }
>  
> +static int aspeed_wdt_get_bootstatus(struct device *dev,
> +                                    struct aspeed_wdt *wdt)

We're not really providing the boot status as a direct result of the
function (i.e. in the return value or through an out-value pointer). I
I feel this might be better named `aspeed_wdt_update_bootstatus()`.

> +{
> +       struct device_node *np = dev->of_node;
> +       struct aspeed_wdt_scu scu = wdt->cfg->scu;

Is there a reason to take a copy? Can we make this a const pointer?

> +       struct regmap *scu_base;
> +       u32 reset_mask_width;
> +       u32 reset_mask_shift;
> +       u32 status;
> +       int ret;
> +
> +       wdt->idx = ((u32)wdt->base & WDT_REG_OFFSET_MASK) /

I think `(intptr_t)wdt->base` better conveys the intent here.

> +                  wdt->cfg->reg_size;
> +
> +       /* On ast2400, only a bit is used to represent WDT reset */
> +       if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
> +               wdt->idx = 0;
> +
> +       scu_base =
> syscon_regmap_lookup_by_compatible(scu.compatible);
> +       if (IS_ERR(scu_base))
> +               return PTR_ERR(scu_base);
> +
> +       ret = regmap_read(scu_base, scu.reset_status_reg, &status);
> +       if (ret)
> +               return ret;
> +
> +       reset_mask_width = hweight32(scu.wdt_reset_mask);
> +       reset_mask_shift = scu.wdt_reset_mask_shift +
> +                          reset_mask_width * wdt->idx;
> +
> +       if (status & (scu.wdt_sw_reset_mask << reset_mask_shift))
> +               wdt->wdd.bootstatus = WDIOF_EXTERN1;
> +       else if (status & (scu.wdt_reset_mask << reset_mask_shift))
> +               wdt->wdd.bootstatus = WDIOF_CARDRESET;
> +       else
> +               wdt->wdd.bootstatus = 0;

...

> +
> +       ret = regmap_write(scu_base, scu.reset_status_reg,
> +                          scu.wdt_reset_mask << reset_mask_shift);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return regmap_write(...) ?

Andrew

RE: [PATCH v3 1/2] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Thursday, October 31, 2024 7:54 AM
> Subject: Re: [PATCH v3 1/2] watchdog: aspeed: Update bootstatus handling
> 
> Hello,
> 
> On Wed, 2024-10-30 at 18:47 +0800, Chin-Ting Kuo wrote:
> > Update the bootstatus according to the latest design guide from the
> > OpenBMC shown as below.
> > https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-u
> > pdate.md#proposed-design
> >
> > In short,
> > - WDIOF_EXTERN1   => system is reset by Software
> > - WDIOF_CARDRESET => system is reset by WDT
> > - Others          => other reset events, e.g., power on reset.
> 
> ...
> 
> >
> > On AST2400 platform, only a bit, SCU3C[1], represents that the system
> > is reset by WDT1 or WDT2.
> >
> > On AST2500 platform, SCU3C[4:2] are WDT reset flags.
> >   SCU3C[4]: system is reset by WDT3.
> >   SCU3C[3]: system is reset by WDT2.
> >   SCU3C[2]: system is reset by WDT1.
> >
> > On AST2600 platform, SCU074[31:16] are WDT reset flags.
> >   SCU074[31:28]: system is reset by WDT4
> >     SCU074[31]: system is reset by WDT4 software reset.
> >   SCU074[27:24]: system is reset by WDT3
> >     SCU074[27]: system is reset by WDT3 software reset.
> >   SCU074[23:20]: system is reset by WDT2
> >     SCU074[23]: system is reset by WDT2 software reset.
> >   SCU074[19:16]: system is reset by WDT1
> >     SCU074[19]: system is reset by WDT1 software reset.
> 
> This information emerges in the code (though it's a bit of a maze back through
> the derivations). I'd rather you discuss how the observable behaviour of the
> driver changes (with respect to booting from the alternate boot region) and
> what choices you've made when the hardware doesn't tell us whether this is a
> (graceful) software reset.
> 

Okay, the commit message will be updated in the next patch version.
For booting from alternate region, it is also triggered by HW WDT SoC reset which is classified as card reset.
This information will also be updated in the next patch version.

> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 114
> +++++++++++++++++++++++++++++++-
> > --
> >  1 file changed, 106 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c
> > b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..add76be3ee42
> > 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -11,10 +11,12 @@
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> >  #include <linux/kstrtox.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> >  #include <linux/watchdog.h>
> >
> >  static bool nowayout = WATCHDOG_NOWAYOUT; @@ -22,15 +24,44 @@
> > module_param(nowayout, bool, 0);
> >  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
> started
> > (default="
> >                                 __MODULE_STRING(WATC
> HDOG_NOWAYOUT)
> > ")");
> >
> > +
> > +/* AST SCU Register for System Reset Event Log Register Set
> > + * ast2600 is scu074 ast2400/2500 is scu03c  */ #define
> > +AST2400_SCU_SYS_RESET_STATUS   0x3c #define
> > +AST2400_SCU_SYS_RESET_WDT_MASK       0x1 #define
> > +AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT 1
> > +
> > +#define   AST2500_SCU_SYS_RESET_WDT_MASK       0x1
> #define
> > +AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT 2
> > +
> > +#define AST2600_SCU_SYS_RESET_STATUS   0x74 #define
> > +AST2600_SCU_SYS_RESET_WDT_MASK       0xf #define
> > +AST2600_SCU_SYS_RESET_WDT_SW_MASK    0x8 #define
> > +AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT 16
> 
> I think for the most part these macros are only used to assign into the config
> data provided through the match struct? I kept having to scroll back-and-forth
> to track the values. I feel it might be better in this instance to _not_ define
> macros for them and assign the literals straight into the struct members down
> below.
> 

Okay, theses definitions will be removed in the next patch version.

> > +
> > +#define WDT_REG_OFFSET_MASK            0x00000fff
> > +
> > +struct aspeed_wdt_scu {
> > +       const char *compatible;
> > +       u32 reset_status_reg;
> > +       u32 wdt_reset_mask;
> > +       u32 wdt_sw_reset_mask;
> > +       u32 wdt_reset_mask_shift;
> > +};
> > +
> >  struct aspeed_wdt_config {
> >         u32 ext_pulse_width_mask;
> >         u32 irq_shift;
> >         u32 irq_mask;
> > +       u32 reg_size;
> 
> This is already specified in the devicetree, though admittedly aspeed- g4.dtsi
> sets the size to 0x1c, which is a little unhelpful for your calculations. I guess we
> can leave it for now.
> 

Thanks for the reminder. It is also be updated the next patch.

> > +       struct aspeed_wdt_scu scu;
> >  };
> >
> >  struct aspeed_wdt {
> >         struct watchdog_device  wdd;
> >         void __iomem            *base;
> > +       int                     idx;
> >         u32                     ctrl;
> >         const struct aspeed_wdt_config *cfg;
> >  };
> > @@ -39,18 +70,42 @@ static const struct aspeed_wdt_config
> > ast2400_config = {
> >         .ext_pulse_width_mask = 0xff,
> >         .irq_shift = 0,
> >         .irq_mask = 0,
> > +       .reg_size = 0x20,
> > +       .scu = {
> > +               .compatible = "aspeed,ast2400-scu",
> > +               .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS,
> > +               .wdt_reset_mask =
> AST2400_SCU_SYS_RESET_WDT_MASK,
> > +               .wdt_sw_reset_mask = 0,
> > +               .wdt_reset_mask_shift =
> > AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > +       },
> >  };
> >
> >  static const struct aspeed_wdt_config ast2500_config = {
> >         .ext_pulse_width_mask = 0xfffff,
> >         .irq_shift = 12,
> >         .irq_mask = GENMASK(31, 12),
> > +       .reg_size = 0x20,
> > +       .scu = {
> > +               .compatible = "aspeed,ast2500-scu",
> > +               .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS,
> > +               .wdt_reset_mask =
> AST2500_SCU_SYS_RESET_WDT_MASK,
> > +               .wdt_sw_reset_mask = 0,
> > +               .wdt_reset_mask_shift =
> > AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > +       },
> >  };
> >
> >  static const struct aspeed_wdt_config ast2600_config = {
> >         .ext_pulse_width_mask = 0xfffff,
> >         .irq_shift = 0,
> >         .irq_mask = GENMASK(31, 10),
> > +       .reg_size = 0x40,
> > +       .scu = {
> > +               .compatible = "aspeed,ast2600-scu",
> > +               .reset_status_reg =
> AST2600_SCU_SYS_RESET_STATUS,
> > +               .wdt_reset_mask =
> AST2600_SCU_SYS_RESET_WDT_MASK,
> > +               .wdt_sw_reset_mask =
> > AST2600_SCU_SYS_RESET_WDT_SW_MASK,
> > +               .wdt_reset_mask_shift =
> > AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > +       },
> >  };
> >
> >  static const struct of_device_id aspeed_wdt_of_table[] = { @@ -213,6
> > +268,51 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
> >         return 0;
> >  }
> >
> > +static int aspeed_wdt_get_bootstatus(struct device *dev,
> > +                                    struct aspeed_wdt
> *wdt)
> 
> We're not really providing the boot status as a direct result of the function (i.e.
> in the return value or through an out-value pointer). I I feel this might be
> better named `aspeed_wdt_update_bootstatus()`.
> 

Okay.

> > +{
> > +       struct device_node *np = dev->of_node;
> > +       struct aspeed_wdt_scu scu = wdt->cfg->scu;
> 
> Is there a reason to take a copy? Can we make this a const pointer?
> 
> > +       struct regmap *scu_base;
> > +       u32 reset_mask_width;
> > +       u32 reset_mask_shift;
> > +       u32 status;
> > +       int ret;
> > +
> > +       wdt->idx = ((u32)wdt->base & WDT_REG_OFFSET_MASK) /
> 
> I think `(intptr_t)wdt->base` better conveys the intent here.
> 

Okay.

> > +                  wdt->cfg->reg_size;
> > +
> > +       /* On ast2400, only a bit is used to represent WDT reset */
> > +       if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
> > +               wdt->idx = 0;
> > +
> > +       scu_base =
> > syscon_regmap_lookup_by_compatible(scu.compatible);
> > +       if (IS_ERR(scu_base))
> > +               return PTR_ERR(scu_base);
> > +
> > +       ret = regmap_read(scu_base, scu.reset_status_reg, &status);
> > +       if (ret)
> > +               return ret;
> > +
> > +       reset_mask_width = hweight32(scu.wdt_reset_mask);
> > +       reset_mask_shift = scu.wdt_reset_mask_shift +
> > +                          reset_mask_width * wdt->idx;
> > +
> > +       if (status & (scu.wdt_sw_reset_mask << reset_mask_shift))
> > +               wdt->wdd.bootstatus = WDIOF_EXTERN1;
> > +       else if (status & (scu.wdt_reset_mask << reset_mask_shift))
> > +               wdt->wdd.bootstatus = WDIOF_CARDRESET;
> > +       else
> > +               wdt->wdd.bootstatus = 0;
> 
> ...
> 
> > +
> > +       ret = regmap_write(scu_base, scu.reset_status_reg,
> > +                          scu.wdt_reset_mask <<
> reset_mask_shift);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> 
> return regmap_write(...) ?
> 

Okay.

> Andrew


Chin-Ting

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

[Chin-Ting Kuo]

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;
 	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) {
+			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)) {
-- 
2.34.1

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

[Andrew Jeffery]

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

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

[Chin-Ting Kuo]

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