[PATCH 0/4] [PATCH 0/4] Update ASPEED WDT bootstatus

[PATCH 0/4] [PATCH 0/4] 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 reset is supported since AST2600 platform
and is also included in this patch series.

Chin-Ting Kuo (4):
  dt-bindings: watchdog: aspeed: Add property for WDT SW reset
  ARM: dts: aspeed: Add WDT controller into alias field
  watchdog: aspeed: Update bootstatus handling
  watchdog: aspeed: Add support for SW restart

 .../bindings/watchdog/aspeed,ast2400-wdt.yaml |  11 ++
 arch/arm/boot/dts/aspeed/aspeed-g4.dtsi       |   2 +
 arch/arm/boot/dts/aspeed/aspeed-g5.dtsi       |   3 +
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi       |   4 +
 drivers/watchdog/aspeed_wdt.c                 | 149 ++++++++++++++++--
 5 files changed, 160 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Chin-Ting Kuo]

Add "aspeed,restart-sw" property to distinguish normal WDT
reset from system restart triggered by SW consciously.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
index be78a9865584..6cc3604c295a 100644
--- a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
@@ -95,6 +95,17 @@ properties:
       array with the first word defined using the AST2600_WDT_RESET1_* macros,
       and the second word defined using the AST2600_WDT_RESET2_* macros.
 
+  aspeed,restart-sw:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: >
+      Normally, ASPEED WDT reset may occur when system hangs or reboot
+      triggered by SW consciously. However, system doesn't know whether the
+      restart is triggered by SW consciously since the reset event flag is
+      the same as normal WDT timeout reset. With this property, SW can
+      restart the system immediately and directly without wait for WDT
+      timeout occurs. The reset event flag is also different from the normal
+      WDT reset. This property is only supported since AST2600 platform.
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH 2/4] ARM: dts: aspeed: Add WDT controller into alias field

[Chin-Ting Kuo]

Add WDT controller into alias field. After that, WDT index,
used to distinguish different WDT controllers in the driver,
can be gotten by using of_alias_get_id dts API.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g4.dtsi | 2 ++
 arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 3 +++
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 4 ++++
 3 files changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g4.dtsi
index 78c967812492..d8b4136d0ca0 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g4.dtsi
@@ -29,6 +29,8 @@ aliases {
 		serial3 = &uart4;
 		serial4 = &uart5;
 		serial5 = &vuart;
+		watchdog0 = &wdt1;
+		watchdog1 = &wdt2;
 	};
 
 	cpus {
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
index 57a699a7c149..4dd220bca617 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
@@ -30,6 +30,9 @@ aliases {
 		serial3 = &uart4;
 		serial4 = &uart5;
 		serial5 = &vuart;
+		watchdog0 = &wdt1;
+		watchdog1 = &wdt2;
+		watchdog2 = &wdt3;
 	};
 
 	cpus {
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 8ed715bd53aa..c0a47c795fff 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -40,6 +40,10 @@ aliases {
 		mdio1 = &mdio1;
 		mdio2 = &mdio2;
 		mdio3 = &mdio3;
+		watchdog0 = &wdt1;
+		watchdog1 = &wdt2;
+		watchdog2 = &wdt3;
+		watchdog3 = &wdt4;
 	};
 
 
-- 
2.34.1

[PATCH 3/4] 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 | 109 +++++++++++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index b4773a6aaf8c..68eaada8a564 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,41 @@ 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
+
+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;
+	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 +67,39 @@ static const struct aspeed_wdt_config ast2400_config = {
 	.ext_pulse_width_mask = 0xff,
 	.irq_shift = 0,
 	.irq_mask = 0,
+	.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),
+	.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),
+	.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 +262,52 @@ 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 = of_alias_get_id(np, "watchdog");
+	if (wdt->idx < 0)
+		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;
+
+	/* On AST2400, only a bit used to represent WDT reset */
+	if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
+		wdt->idx = 0;
+
+	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 = WDIOF_UNKNOWN;
+
+	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 +407,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 +552,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

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

[Chin-Ting Kuo]

WDT reset can be triggered when system hangs or a deliberate
SW restart scenario. Originally, system can only know it is
reset by WDT through a reset flag. However, since AST2600,
a SW reset mechanism is created, SW can trigger the reset
event consciously and directly without wait for WDT timeout.
This function can be achieved by adding "aspeed,restart-sw"
property in dts. After that, an independent reset event flag
will be set after system reset by SW.

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

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 68eaada8a564..eefca972dfa4 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -61,6 +61,7 @@ struct aspeed_wdt {
 	int			idx;
 	u32			ctrl;
 	const struct aspeed_wdt_config *cfg;
+	u32			flags;
 };
 
 static const struct aspeed_wdt_config ast2400_config = {
@@ -130,6 +131,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
@@ -170,6 +176,9 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
 #define WDT_DEFAULT_TIMEOUT	30
 #define WDT_RATE_1MHZ		1000000
 
+/* WDT behavior control flag */
+#define WDT_RESTART_SYSTEM_SW	0x00000001
+
 static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
 {
 	return container_of(wdd, struct aspeed_wdt, wdd);
@@ -249,11 +258,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);
 
+	if (wdt->flags & WDT_RESTART_SYSTEM_SW) {
+		aspeed_wdt_sw_reset(wdd);
+		return 0;
+	}
+
 	wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY;
 	aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
 
@@ -521,8 +550,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		ret = of_property_read_u32_array(np, "aspeed,reset-mask", reset_mask, nrstmask);
 		if (!ret) {
 			writel(reset_mask[0], wdt->base + WDT_RESET_MASK1);
-			if (nrstmask > 1)
+			writel(reset_mask[0], wdt->base + WDT_SW_RESET_MASK1);
+			if (nrstmask > 1) {
 				writel(reset_mask[1], wdt->base + WDT_RESET_MASK2);
+				writel(reset_mask[1], wdt->base + WDT_SW_RESET_MASK2);
+			}
 		}
 	}
 
@@ -552,6 +584,12 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		writel(duration - 1, wdt->base + WDT_RESET_WIDTH);
 	}
 
+	wdt->flags = 0;
+	if (!of_device_is_compatible(np, "aspeed,ast2400-wdt") &&
+	    !of_device_is_compatible(np, "aspeed,ast2500-wdt") &&
+	    of_property_read_bool(np, "aspeed,restart-sw"))
+		wdt->flags |= WDT_RESTART_SYSTEM_SW;
+
 	ret = aspeed_wdt_get_bootstatus(dev, wdt);
 	if (ret)
 		return ret;
-- 
2.34.1

Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Krzysztof Kozlowski]

On 07/10/2024 08:34, Chin-Ting Kuo wrote:
> Add "aspeed,restart-sw" property to distinguish normal WDT
> reset from system restart triggered by SW consciously.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> index be78a9865584..6cc3604c295a 100644
> --- a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> @@ -95,6 +95,17 @@ properties:
>        array with the first word defined using the AST2600_WDT_RESET1_* macros,
>        and the second word defined using the AST2600_WDT_RESET2_* macros.
>  
> +  aspeed,restart-sw:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: >

Why >?

> +      Normally, ASPEED WDT reset may occur when system hangs or reboot
> +      triggered by SW consciously. However, system doesn't know whether the
> +      restart is triggered by SW consciously since the reset event flag is
> +      the same as normal WDT timeout reset. With this property, SW can

So DTS has this property and watchdog bites (timeout) but you will
ignore it and claim that it was software choice?

This does not make much sense to me, at least based on this explanation

> +      restart the system immediately and directly without wait for WDT
> +      timeout occurs. The reset event flag is also different from the normal
> +      WDT reset. This property is only supported since AST2600 platform.

Supported as drivers? How is this related? Or you mean hardware? Then
property should be restricted there.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Rob Herring]

On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote:
> Add "aspeed,restart-sw" property to distinguish normal WDT
> reset from system restart triggered by SW consciously.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> index be78a9865584..6cc3604c295a 100644
> --- a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> @@ -95,6 +95,17 @@ properties:
>        array with the first word defined using the AST2600_WDT_RESET1_* macros,
>        and the second word defined using the AST2600_WDT_RESET2_* macros.
>  
> +  aspeed,restart-sw:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: >
> +      Normally, ASPEED WDT reset may occur when system hangs or reboot
> +      triggered by SW consciously. However, system doesn't know whether the
> +      restart is triggered by SW consciously since the reset event flag is
> +      the same as normal WDT timeout reset. With this property, SW can
> +      restart the system immediately and directly without wait for WDT
> +      timeout occurs. The reset event flag is also different from the normal
> +      WDT reset. This property is only supported since AST2600 platform.

Why can't this be implicit based on the ast2600 compatible string?

Rob

Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Guenter Roeck]

On 10/7/24 10:59, Rob Herring wrote:
> On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote:
>> Add "aspeed,restart-sw" property to distinguish normal WDT
>> reset from system restart triggered by SW consciously.
>>
>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>> ---
>>   .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>> index be78a9865584..6cc3604c295a 100644
>> --- a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>> @@ -95,6 +95,17 @@ properties:
>>         array with the first word defined using the AST2600_WDT_RESET1_* macros,
>>         and the second word defined using the AST2600_WDT_RESET2_* macros.
>>   
>> +  aspeed,restart-sw:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: >
>> +      Normally, ASPEED WDT reset may occur when system hangs or reboot
>> +      triggered by SW consciously. However, system doesn't know whether the
>> +      restart is triggered by SW consciously since the reset event flag is
>> +      the same as normal WDT timeout reset. With this property, SW can
>> +      restart the system immediately and directly without wait for WDT
>> +      timeout occurs. The reset event flag is also different from the normal
>> +      WDT reset. This property is only supported since AST2600 platform.
> 
> Why can't this be implicit based on the ast2600 compatible string?
> 

Same question here.

Guenter

RE: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Chin-Ting Kuo]

Hi Krzysztof,

Thanks for the review.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, October 7, 2024 2:58 PM
> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT
> SW reset
> 
> On 07/10/2024 08:34, Chin-Ting Kuo wrote:
> > Add "aspeed,restart-sw" property to distinguish normal WDT reset from
> > system restart triggered by SW consciously.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11
> +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > index be78a9865584..6cc3604c295a 100644
> > ---
> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya
> > +++ ml
> > @@ -95,6 +95,17 @@ properties:
> >        array with the first word defined using the AST2600_WDT_RESET1_*
> macros,
> >        and the second word defined using the AST2600_WDT_RESET2_*
> macros.
> >
> > +  aspeed,restart-sw:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: >
> 
> Why >?
> 

">" will be removed in the next patch series and the description content will be
concatenated after the colon, ":".

> > +      Normally, ASPEED WDT reset may occur when system hangs or
> reboot
> > +      triggered by SW consciously. However, system doesn't know whether
> the
> > +      restart is triggered by SW consciously since the reset event flag is
> > +      the same as normal WDT timeout reset. With this property, SW
> > + can
> 
> So DTS has this property and watchdog bites (timeout) but you will ignore it
> and claim that it was software choice?
> 

No. Normally, when WDT is enabled, a counter is also be enabled. When the counter
is equal to an expected value, timeout event occurs. AST2600 hardware supports a SW
mode, when a magic number is filled into a specific register, WDT reset is triggered
immediately without controlling the counter and the counter is not counted.
Thus, WDT timeout doesn't occur.

> This does not make much sense to me, at least based on this explanation
> 
> > +      restart the system immediately and directly without wait for WDT
> > +      timeout occurs. The reset event flag is also different from the
> normal
> > +      WDT reset. This property is only supported since AST2600 platform.
> 
> Supported as drivers? How is this related? Or you mean hardware? Then
> property should be restricted there.
> 

It is a hardware supported function on AST2600. For platform compatibility, without
this property, all behaviors are the same as the previous generation platform, AST2500.

This property may be removed in the next patch series with referring to Rob suggestion
in the other reply. After checking with the major users, it is feasible to remove this
property and using SW reset by default when the restart operation is triggered by SW
deliberately on AST2600 platform.

> Best regards,
> Krzysztof

Best Wishes,
Chin-Ting

RE: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Chin-Ting Kuo]

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, October 8, 2024 2:00 AM
> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT
> SW reset
> 
> On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote:
> > Add "aspeed,restart-sw" property to distinguish normal WDT reset from
> > system restart triggered by SW consciously.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11
> +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > index be78a9865584..6cc3604c295a 100644
> > ---
> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya
> > +++ ml
> > @@ -95,6 +95,17 @@ properties:
> >        array with the first word defined using the AST2600_WDT_RESET1_*
> macros,
> >        and the second word defined using the AST2600_WDT_RESET2_*
> macros.
> >
> > +  aspeed,restart-sw:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: >
> > +      Normally, ASPEED WDT reset may occur when system hangs or
> reboot
> > +      triggered by SW consciously. However, system doesn't know whether
> the
> > +      restart is triggered by SW consciously since the reset event flag is
> > +      the same as normal WDT timeout reset. With this property, SW can
> > +      restart the system immediately and directly without wait for WDT
> > +      timeout occurs. The reset event flag is also different from the
> normal
> > +      WDT reset. This property is only supported since AST2600 platform.
> 
> Why can't this be implicit based on the ast2600 compatible string?
> 

Yes, this property will be implicit based on the ast2600 compatible string
in the next patch series.

> Rob

Chin-Ting

RE: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Chin-Ting Kuo]

Hi Guenter,

Thanks for the review.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, October 8, 2024 3:55 AM
> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT
> SW reset
> 
> On 10/7/24 10:59, Rob Herring wrote:
> > On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote:
> >> Add "aspeed,restart-sw" property to distinguish normal WDT reset from
> >> system restart triggered by SW consciously.
> >>
> >> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> >> ---
> >>   .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11
> +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >> b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >> index be78a9865584..6cc3604c295a 100644
> >> ---
> >> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.y
> >> +++ aml
> >> @@ -95,6 +95,17 @@ properties:
> >>         array with the first word defined using the
> AST2600_WDT_RESET1_* macros,
> >>         and the second word defined using the AST2600_WDT_RESET2_*
> macros.
> >>
> >> +  aspeed,restart-sw:
> >> +    $ref: /schemas/types.yaml#/definitions/flag
> >> +    description: >
> >> +      Normally, ASPEED WDT reset may occur when system hangs or
> reboot
> >> +      triggered by SW consciously. However, system doesn't know
> whether the
> >> +      restart is triggered by SW consciously since the reset event flag is
> >> +      the same as normal WDT timeout reset. With this property, SW can
> >> +      restart the system immediately and directly without wait for WDT
> >> +      timeout occurs. The reset event flag is also different from the
> normal
> >> +      WDT reset. This property is only supported since AST2600 platform.
> >
> > Why can't this be implicit based on the ast2600 compatible string?
> >
> 
> Same question here.
> 
Yes, this property will be implicit based on the ast2600 compatible string
in the next patch series.

> Guenter

Chin-Ting

Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Krzysztof Kozlowski]

On 14/10/2024 04:07, Chin-Ting Kuo wrote:
> Hi Krzysztof,
> 
> Thanks for the review.
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Monday, October 7, 2024 2:58 PM
>> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT
>> SW reset
>>
>> On 07/10/2024 08:34, Chin-Ting Kuo wrote:
>>> Add "aspeed,restart-sw" property to distinguish normal WDT reset from
>>> system restart triggered by SW consciously.
>>>
>>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>>> ---
>>>  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11
>> +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>>> b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>>> index be78a9865584..6cc3604c295a 100644
>>> ---
>>> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya
>>> +++ ml
>>> @@ -95,6 +95,17 @@ properties:
>>>        array with the first word defined using the AST2600_WDT_RESET1_*
>> macros,
>>>        and the second word defined using the AST2600_WDT_RESET2_*
>> macros.
>>>
>>> +  aspeed,restart-sw:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: >
>>
>> Why >?
>>
> 
> ">" will be removed in the next patch series and the description content will be
> concatenated after the colon, ":".
> 
>>> +      Normally, ASPEED WDT reset may occur when system hangs or
>> reboot
>>> +      triggered by SW consciously. However, system doesn't know whether
>> the
>>> +      restart is triggered by SW consciously since the reset event flag is
>>> +      the same as normal WDT timeout reset. With this property, SW
>>> + can
>>
>> So DTS has this property and watchdog bites (timeout) but you will ignore it
>> and claim that it was software choice?
>>
> 
> No. Normally, when WDT is enabled, a counter is also be enabled. When the counter
> is equal to an expected value, timeout event occurs. AST2600 hardware supports a SW
> mode, when a magic number is filled into a specific register, WDT reset is triggered
> immediately without controlling the counter and the counter is not counted.
> Thus, WDT timeout doesn't occur.

How is this a no?

> 
>> This does not make much sense to me, at least based on this explanation
>>
>>> +      restart the system immediately and directly without wait for WDT
>>> +      timeout occurs. The reset event flag is also different from the
>> normal
>>> +      WDT reset. This property is only supported since AST2600 platform.
>>
>> Supported as drivers? How is this related? Or you mean hardware? Then
>> property should be restricted there.
>>
> 
> It is a hardware supported function on AST2600. For platform compatibility, without
> this property, all behaviors are the same as the previous generation platform, AST2500.
> 
> This property may be removed in the next patch series with referring to Rob suggestion

s/may/will/

> in the other reply. After checking with the major users, it is feasible to remove this
> property and using SW reset by default when the restart operation is triggered by SW
> deliberately on AST2600 platform.
> 


Best regards,
Krzysztof

RE: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT SW reset

[Chin-Ting Kuo]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, October 14, 2024 2:53 PM
> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT
> SW reset
> 
> On 14/10/2024 04:07, Chin-Ting Kuo wrote:
> > Hi Krzysztof,
> >
> > Thanks for the review.
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Monday, October 7, 2024 2:58 PM
> >> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property
> >> for WDT SW reset
> >>
> >> On 07/10/2024 08:34, Chin-Ting Kuo wrote:
> >>> Add "aspeed,restart-sw" property to distinguish normal WDT reset
> >>> from system restart triggered by SW consciously.
> >>>
> >>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> >>> ---
> >>>  .../bindings/watchdog/aspeed,ast2400-wdt.yaml         | 11
> >> +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git
> >>>
> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >>>
> b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >>> index be78a9865584..6cc3604c295a 100644
> >>> ---
> >>>
> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> >>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.
> >>> +++ ya
> >>> +++ ml
> >>> @@ -95,6 +95,17 @@ properties:
> >>>        array with the first word defined using the
> >>> AST2600_WDT_RESET1_*
> >> macros,
> >>>        and the second word defined using the AST2600_WDT_RESET2_*
> >> macros.
> >>>
> >>> +  aspeed,restart-sw:
> >>> +    $ref: /schemas/types.yaml#/definitions/flag
> >>> +    description: >
> >>
> >> Why >?
> >>
> >
> > ">" will be removed in the next patch series and the description
> > content will be concatenated after the colon, ":".
> >
> >>> +      Normally, ASPEED WDT reset may occur when system hangs or
> >> reboot
> >>> +      triggered by SW consciously. However, system doesn't know
> >>> + whether
> >> the
> >>> +      restart is triggered by SW consciously since the reset event flag is
> >>> +      the same as normal WDT timeout reset. With this property, SW
> >>> + can
> >>
> >> So DTS has this property and watchdog bites (timeout) but you will
> >> ignore it and claim that it was software choice?
> >>
> >
> > No. Normally, when WDT is enabled, a counter is also be enabled. When
> > the counter is equal to an expected value, timeout event occurs.
> > AST2600 hardware supports a SW mode, when a magic number is filled
> > into a specific register, WDT reset is triggered immediately without
> controlling the counter and the counter is not counted.
> > Thus, WDT timeout doesn't occur.
> 
> How is this a no?
> 

It is used to emphasize that the driver doesn’t ignore the timeout event
because the counter is not counted when SW mode is used and thus, no
timeout occurs.

> >
> >> This does not make much sense to me, at least based on this
> >> explanation
> >>
> >>> +      restart the system immediately and directly without wait for WDT
> >>> +      timeout occurs. The reset event flag is also different from
> >>> + the
> >> normal
> >>> +      WDT reset. This property is only supported since AST2600
> platform.
> >>
> >> Supported as drivers? How is this related? Or you mean hardware? Then
> >> property should be restricted there.
> >>
> >
> > It is a hardware supported function on AST2600. For platform
> > compatibility, without this property, all behaviors are the same as the
> previous generation platform, AST2500.
> >
> > This property may be removed in the next patch series with referring
> > to Rob suggestion
> 
> s/may/will/
> 

This property will be removed in the next patch series.

> > in the other reply. After checking with the major users, it is
> > feasible to remove this property and using SW reset by default when
> > the restart operation is triggered by SW deliberately on AST2600 platform.
> >
> 
> 

Best Wishes,
Chin-Ting