2024-03-26 15:00:58

by Peter Yin

[permalink] [raw]
Subject: [PATCH v3 0/4] drivers: watchdog: ast2500 and ast2600 support bootstatus

Add WDIOF_EXTERN1 and WDIOF_CARDRESET bootstatus in ast2500/ast2600

Regarding the AST2600 specification, the WDTn Timeout Status Register
(WDT10) has bit 1 reserved. Bit 1 of the status register indicates
on ast2500 if the boot was from the second boot source.
It does not indicate that the most recent reset was triggered by
the watchdog. The code should just be changed to set WDIOF_CARDRESET
if bit 0 of the status register is set.

Include SCU register to veriy WDIOF_EXTERN1 in ast2600 SCU74 or
ast2500 SCU3C when bit1 is set.

v2 -> v3
- Fixed WDIOF_CARDRESET status bit check and added support
for WDIOF_EXTERN1 on ast2500 and ast2600.

v1 -> v2
- Add comment and support WDIOF_CARDRESET in ast2600

v1
- Patch 0001 - Add WDIOF_EXTERN1 bootstatus
---

Peter Yin (4):
ARM: dts: aspeed: Add the AST2500 WDT with SCU register
ARM: dts: aspeed: Add the AST2600 WDT with SCU register
dt-bindings: watchdog: aspeed-wdt: Add aspeed,scu
drivers: watchdog: ast2500 and ast2600 support bootstatus

.../bindings/watchdog/aspeed-wdt.txt | 4 ++
arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 3 ++
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 4 ++
drivers/watchdog/aspeed_wdt.c | 53 +++++++++++++------
4 files changed, 48 insertions(+), 16 deletions(-)

--
2.25.1



2024-03-26 15:01:39

by Peter Yin

[permalink] [raw]
Subject: [PATCH v3 2/4] ARM: dts: aspeed: Add the AST2600 WDT with SCU register

The AST2600 Watchdog Timer (WDT) references
the System Control Unit (SCU) register for its operation.

Signed-off-by: Peter Yin <[email protected]>
---
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 5f640b7d6b6d..2f7788f2f153 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -558,23 +558,27 @@ uart5: serial@1e784000 {
wdt1: watchdog@1e785000 {
compatible = "aspeed,ast2600-wdt";
reg = <0x1e785000 0x40>;
+ aspeed,scu = <&syscon>;
};

wdt2: watchdog@1e785040 {
compatible = "aspeed,ast2600-wdt";
reg = <0x1e785040 0x40>;
+ aspeed,scu = <&syscon>;
status = "disabled";
};

wdt3: watchdog@1e785080 {
compatible = "aspeed,ast2600-wdt";
reg = <0x1e785080 0x40>;
+ aspeed,scu = <&syscon>;
status = "disabled";
};

wdt4: watchdog@1e7850c0 {
compatible = "aspeed,ast2600-wdt";
reg = <0x1e7850C0 0x40>;
+ aspeed,scu = <&syscon>;
status = "disabled";
};

--
2.25.1


2024-03-26 15:02:18

by Peter Yin

[permalink] [raw]
Subject: [PATCH v3 4/4] drivers: watchdog: ast2500 and ast2600 support bootstatus

Add WDIOF_EXTERN1 and WDIOF_CARDRESET bootstatus in ast2600

Regarding the AST2600 specification, the WDTn Timeout Status Register
(WDT10) has bit 1 reserved. Bit 1 of the status register indicates
on ast2500 if the boot was from the second boot source.
It does not indicate that the most recent reset was triggered by
the watchdog. The code should just be changed to set WDIOF_CARDRESET
if bit 0 of the status register is set.

Include SCU register to veriy WDIOF_EXTERN1 in ast2600 SCU74 or
ast2500 SCU3C when bit1 is set.

Signed-off-by: Peter Yin <[email protected]>
---
drivers/watchdog/aspeed_wdt.c | 53 ++++++++++++++++++++++++-----------
1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index b4773a6aaf8c..52afc5240b1c 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;
@@ -65,23 +67,32 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
#define WDT_RELOAD_VALUE 0x04
#define WDT_RESTART 0x08
#define WDT_CTRL 0x0C
-#define WDT_CTRL_BOOT_SECONDARY BIT(7)
-#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
-#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
-#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
-#define WDT_CTRL_1MHZ_CLK BIT(4)
-#define WDT_CTRL_WDT_EXT BIT(3)
-#define WDT_CTRL_WDT_INTR BIT(2)
-#define WDT_CTRL_RESET_SYSTEM BIT(1)
-#define WDT_CTRL_ENABLE BIT(0)
+#define WDT_CTRL_BOOT_SECONDARY BIT(7)
+#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
+#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
+#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
+#define WDT_CTRL_1MHZ_CLK BIT(4)
+#define WDT_CTRL_WDT_EXT BIT(3)
+#define WDT_CTRL_WDT_INTR BIT(2)
+#define WDT_CTRL_RESET_SYSTEM BIT(1)
+#define WDT_CTRL_ENABLE BIT(0)
#define WDT_TIMEOUT_STATUS 0x10
-#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
-#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
+#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
+#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
+#define WDT_TIMEOUT_STATUS_EVENT BIT(0)
#define WDT_CLEAR_TIMEOUT_STATUS 0x14
-#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
+#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
#define WDT_RESET_MASK1 0x1c
#define WDT_RESET_MASK2 0x20

+/*
+ * Ast2600 SCU74 bit1 is External reset flag
+ * Ast2500 SCU3C bit1 is External reset flag
+ */
+#define EXTERN_RESET_FLAG BIT(1)
+#define AST2500_SYSTEM_RESET_EVENT (0x3C)
+#define AST2600_SYSTEM_RESET_EVENT (0x74)
+
/*
* WDT_RESET_WIDTH controls the characteristics of the external pulse (if
* enabled), specifically:
@@ -458,15 +469,25 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
writel(duration - 1, wdt->base + WDT_RESET_WIDTH);
}

+ struct regmap *scu_base = syscon_regmap_lookup_by_phandle(dev->of_node,
+ "aspeed,scu");
status = readl(wdt->base + WDT_TIMEOUT_STATUS);
- if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) {
+ if (status & WDT_TIMEOUT_STATUS_EVENT)
wdt->wdd.bootstatus = WDIOF_CARDRESET;

- 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,ast2600-wdt")) {
+ regmap_read(scu_base, AST2600_SYSTEM_RESET_EVENT, &status);
+ } else {
+ regmap_read(scu_base, AST2500_SYSTEM_RESET_EVENT, &status);
+ wdt->wdd.groups = bswitch_groups;
}

+ /*
+ * Reset cause by Extern Reset
+ */
+ if (status & EXTERN_RESET_FLAG)
+ wdt->wdd.bootstatus |= WDIOF_EXTERN1;
+
dev_set_drvdata(dev, wdt);

return devm_watchdog_register_device(dev, &wdt->wdd);
--
2.25.1


2024-03-26 15:05:29

by Peter Yin

[permalink] [raw]
Subject: [PATCH v3 3/4] dt-bindings: watchdog: aspeed-wdt: Add aspeed,scu

To use the SCU register to obtain reset flags for supporting
bootstatus.

Signed-off-by: Peter Yin <[email protected]>
---
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
index 3208adb3e52e..80a1f58b5a2e 100644
--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -8,6 +8,8 @@ Required properties:

- reg: physical base address of the controller and length of memory mapped
region
+ - aspeed,scu: a reference to the System Control Unit node of the Aspeed
+ SOC.

Optional properties:

@@ -62,6 +64,7 @@ Examples:
reg = <0x1e785000 0x1c>;
aspeed,reset-type = "system";
aspeed,external-signal;
+ aspeed,scu = <&syscon>;
};

#include <dt-bindings/watchdog/aspeed-wdt.h>
@@ -70,4 +73,5 @@ Examples:
reg = <0x1e785040 0x40>;
aspeed,reset-mask = <AST2600_WDT_RESET1_DEFAULT
(AST2600_WDT_RESET2_DEFAULT & ~AST2600_WDT_RESET2_LPC)>;
+ aspeed,scu = <&syscon>;
};
--
2.25.1


2024-03-26 15:06:33

by Peter Yin

[permalink] [raw]
Subject: [PATCH v3 1/4] ARM: dts: aspeed: Add the AST2500 WDT with SCU register

The AST2500 WDT references the System Control Unit
register for its operation.

Signed-off-by: Peter Yin <[email protected]>
---
arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
index 04f98d1dbb97..5fd12c057c31 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
@@ -410,12 +410,14 @@ wdt1: watchdog@1e785000 {
compatible = "aspeed,ast2500-wdt";
reg = <0x1e785000 0x20>;
clocks = <&syscon ASPEED_CLK_APB>;
+ aspeed,scu = <&syscon>;
};

wdt2: watchdog@1e785020 {
compatible = "aspeed,ast2500-wdt";
reg = <0x1e785020 0x20>;
clocks = <&syscon ASPEED_CLK_APB>;
+ aspeed,scu = <&syscon>;
};

wdt3: watchdog@1e785040 {
@@ -423,6 +425,7 @@ wdt3: watchdog@1e785040 {
reg = <0x1e785040 0x20>;
clocks = <&syscon ASPEED_CLK_APB>;
status = "disabled";
+ aspeed,scu = <&syscon>;
};

pwm_tacho: pwm-tacho-controller@1e786000 {
--
2.25.1


2024-03-26 15:42:15

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] drivers: watchdog: ast2500 and ast2600 support bootstatus

On 3/26/24 08:00, Peter Yin wrote:
> Add WDIOF_EXTERN1 and WDIOF_CARDRESET bootstatus in ast2600
>
> Regarding the AST2600 specification, the WDTn Timeout Status Register
> (WDT10) has bit 1 reserved. Bit 1 of the status register indicates
> on ast2500 if the boot was from the second boot source.
> It does not indicate that the most recent reset was triggered by
> the watchdog. The code should just be changed to set WDIOF_CARDRESET
> if bit 0 of the status register is set.
>
> Include SCU register to veriy WDIOF_EXTERN1 in ast2600 SCU74 or
> ast2500 SCU3C when bit1 is set.
>
> Signed-off-by: Peter Yin <[email protected]>
> ---
> drivers/watchdog/aspeed_wdt.c | 53 ++++++++++++++++++++++++-----------
> 1 file changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index b4773a6aaf8c..52afc5240b1c 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;
> @@ -65,23 +67,32 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> #define WDT_RELOAD_VALUE 0x04
> #define WDT_RESTART 0x08
> #define WDT_CTRL 0x0C
> -#define WDT_CTRL_BOOT_SECONDARY BIT(7)
> -#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
> -#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
> -#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
> -#define WDT_CTRL_1MHZ_CLK BIT(4)
> -#define WDT_CTRL_WDT_EXT BIT(3)
> -#define WDT_CTRL_WDT_INTR BIT(2)
> -#define WDT_CTRL_RESET_SYSTEM BIT(1)
> -#define WDT_CTRL_ENABLE BIT(0)
> +#define WDT_CTRL_BOOT_SECONDARY BIT(7)
> +#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
> +#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
> +#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
> +#define WDT_CTRL_1MHZ_CLK BIT(4)
> +#define WDT_CTRL_WDT_EXT BIT(3)
> +#define WDT_CTRL_WDT_INTR BIT(2)
> +#define WDT_CTRL_RESET_SYSTEM BIT(1)
> +#define WDT_CTRL_ENABLE BIT(0)
> #define WDT_TIMEOUT_STATUS 0x10
> -#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
> -#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
> +#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
> +#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
> +#define WDT_TIMEOUT_STATUS_EVENT BIT(0)
> #define WDT_CLEAR_TIMEOUT_STATUS 0x14
> -#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
> +#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
> #define WDT_RESET_MASK1 0x1c
> #define WDT_RESET_MASK2 0x20
>
> +/*
> + * Ast2600 SCU74 bit1 is External reset flag
> + * Ast2500 SCU3C bit1 is External reset flag
> + */
> +#define EXTERN_RESET_FLAG BIT(1)
> +#define AST2500_SYSTEM_RESET_EVENT (0x3C)
> +#define AST2600_SYSTEM_RESET_EVENT (0x74)

() around constants does not add any value.

> +
> /*
> * WDT_RESET_WIDTH controls the characteristics of the external pulse (if
> * enabled), specifically:
> @@ -458,15 +469,25 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> writel(duration - 1, wdt->base + WDT_RESET_WIDTH);
> }
>
> + struct regmap *scu_base = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "aspeed,scu");
> status = readl(wdt->base + WDT_TIMEOUT_STATUS);
> - if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) {
> + if (status & WDT_TIMEOUT_STATUS_EVENT)
> wdt->wdd.bootstatus = WDIOF_CARDRESET;
>
> - 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,ast2600-wdt")) {
> + regmap_read(scu_base, AST2600_SYSTEM_RESET_EVENT, &status);

scu_base as returned from syscon_regmap_lookup_by_phandle() can be an ERR_PTR.
If it is, this will crash. On top of ttat, regmap_read() can also return an
error. If it does, status will be unmodified, and WDIOF_EXTERN1 will be set randomly.
This will most definitely happen if REGMAP support is disabled (there is nothing
in Kconfig requiring that REGMAP must be enabled for this driver).

Guenter