| Message ID | 20250112081204.263216-1-chin-ting_kuo@aspeedtech.com |
|---|---|
| Headers | show |
| Series | Update ASPEED WDT bootstatus | expand |
On 1/12/25 00:12, Chin-Ting Kuo wrote: > The boot status in the watchdog device struct is updated during > controller probe stage. Application layer can get the boot status > through the command, cat /sys/class/watchdog/watchdogX/bootstatus. > The bootstatus can be, > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > Others => other reset events, e.g., power on reset. > > On ASPEED platform, the boot status is recorded in the SCU registers. > - AST2400: Only a bit represents for any WDT reset. > - AST2500/AST2600: The reset triggered by different WDT controllers > can be distinguished by different SCU bits. > > Besides, on AST2400 and AST2500, since alternating boot event is > triggered by WDT SoC reset, it is classified as WDIOF_CARDRESET. > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
On Sun, 2025-01-12 at 16:12 +0800, Chin-Ting Kuo wrote: > The boot status in the watchdog device struct is updated during > controller probe stage. Application layer can get the boot status > through the command, cat /sys/class/watchdog/watchdogX/bootstatus. > The bootstatus can be, > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > Others => other reset events, e.g., power on reset. > > On ASPEED platform, the boot status is recorded in the SCU registers. > - AST2400: Only a bit represents for any WDT reset. > - AST2500/AST2600: The reset triggered by different WDT controllers > can be distinguished by different SCU bits. > > Besides, on AST2400 and AST2500, since alternating boot event is > triggered by WDT SoC reset, it is classified as WDIOF_CARDRESET. > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > --- > drivers/watchdog/aspeed_wdt.c | 81 > ++++++++++++++++++++++++++++++++++- > 1 file changed, 79 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/aspeed_wdt.c > b/drivers/watchdog/aspeed_wdt.c > index b4773a6aaf8c..369635b38ca0 100644 > --- a/drivers/watchdog/aspeed_wdt.c > +++ b/drivers/watchdog/aspeed_wdt.c > @@ -11,21 +11,30 @@ > #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; > module_param(nowayout, bool, 0); > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started > (default=" > __MODULE_STRING(WATCHDOG_NOWAYOUT) > ")"); > +struct aspeed_wdt_scu { > + const char *compatible; > + u32 reset_status_reg; > + u32 wdt_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 { > @@ -39,18 +48,36 @@ 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 = 0x3c, > + .wdt_reset_mask = 0x1, > + .wdt_reset_mask_shift = 1, > + }, > }; > > 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 = 0x3c, > + .wdt_reset_mask = 0x1, > + .wdt_reset_mask_shift = 2, > + }, > }; > > 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 = 0x74, > + .wdt_reset_mask = 0xf, > + .wdt_reset_mask_shift = 16, > + }, > }; > > static const struct of_device_id aspeed_wdt_of_table[] = { > @@ -213,6 +240,56 @@ static int aspeed_wdt_restart(struct > watchdog_device *wdd, > return 0; > } > > +static void aspeed_wdt_update_bootstatus(struct platform_device > *pdev, > + struct aspeed_wdt *wdt) > +{ > + const struct resource *res; > + struct aspeed_wdt_scu scu = wdt->cfg->scu; > + struct regmap *scu_base; > + u32 reset_mask_width; > + u32 reset_mask_shift; > + u32 idx = 0; > + u32 status; > + int ret; > + > + if (!of_device_is_compatible(pdev->dev.of_node, > "aspeed,ast2400-wdt")) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + idx = ((intptr_t)wdt->base & 0x00000fff) / > resource_size(res); > + } > + > + scu_base = > syscon_regmap_lookup_by_compatible(scu.compatible); > + if (IS_ERR(scu_base)) { > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > + return; > + } > + > + ret = regmap_read(scu_base, scu.reset_status_reg, &status); > + if (ret) { > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > + return; > + } > + > + reset_mask_width = hweight32(scu.wdt_reset_mask); > + reset_mask_shift = scu.wdt_reset_mask_shift + > + reset_mask_width * idx; > + > + if (status & (scu.wdt_reset_mask << reset_mask_shift)) > + wdt->wdd.bootstatus = WDIOF_CARDRESET; How precise is the wording in your commit message? The AST2600, for instance, has 4 reset types: 1. "SoC" 2. "Full" 3. "ARM" 4. "Software" In the commit message, you said: > WDIOF_CARDRESET => the system is reset by WDT SoC reset. However the logic here (with the way you've initialised the config struct for the AST2600) will signal WDIOF_CARDRESET even if what occurred was e.g. a (graceful?) software reset. The only thing WDIOF_CARDRESET differentiates is a power-on reset from any other kind of watchdog-driven reset. Is that what's intended? I'm just wary of confusion for what appears to be a generic use of "SoC" in the commit message vs the specific "SoC" mode of the watchdog controller (should some documentation be written/updated?) Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Jeffery <andrew@codeconstruct.com.au> > Sent: Monday, January 13, 2025 11:29 AM > Subject: Re: [PATCH v6 1/1] watchdog: aspeed: Update bootstatus handling > > On Sun, 2025-01-12 at 16:12 +0800, Chin-Ting Kuo wrote: > > The boot status in the watchdog device struct is updated during > > controller probe stage. Application layer can get the boot status > > through the command, cat /sys/class/watchdog/watchdogX/bootstatus. > > The bootstatus can be, > > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > > Others => other reset events, e.g., power on reset. > > > > On ASPEED platform, the boot status is recorded in the SCU registers. > > - AST2400: Only a bit represents for any WDT reset. > > - AST2500/AST2600: The reset triggered by different WDT controllers > > can be distinguished by different SCU bits. > > > > Besides, on AST2400 and AST2500, since alternating boot event is > > triggered by WDT SoC reset, it is classified as WDIOF_CARDRESET. > > > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > > --- > > drivers/watchdog/aspeed_wdt.c | 81 > > ++++++++++++++++++++++++++++++++++- > > 1 file changed, 79 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/watchdog/aspeed_wdt.c > > b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..369635b38ca0 > > 100644 > > --- a/drivers/watchdog/aspeed_wdt.c > > +++ b/drivers/watchdog/aspeed_wdt.c > > @@ -11,21 +11,30 @@ > > #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; > > module_param(nowayout, bool, 0); > > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once > started > > (default=" > > __MODULE_STRING(WATC > HDOG_NOWAYOUT) > > ")"); > > +struct aspeed_wdt_scu { > > + const char *compatible; > > + u32 reset_status_reg; > > + u32 wdt_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 { > > @@ -39,18 +48,36 @@ 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 = 0x3c, > > + .wdt_reset_mask = 0x1, > > + .wdt_reset_mask_shift = 1, > > + }, > > }; > > > > 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 = 0x3c, > > + .wdt_reset_mask = 0x1, > > + .wdt_reset_mask_shift = 2, > > + }, > > }; > > > > 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 = 0x74, > > + .wdt_reset_mask = 0xf, > > + .wdt_reset_mask_shift = 16, > > + }, > > }; > > > > static const struct of_device_id aspeed_wdt_of_table[] = { @@ -213,6 > > +240,56 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd, > > return 0; > > } > > > > +static void aspeed_wdt_update_bootstatus(struct platform_device > > *pdev, > > + struct > aspeed_wdt *wdt) { > > + const struct resource *res; > > + struct aspeed_wdt_scu scu = wdt->cfg->scu; > > + struct regmap *scu_base; > > + u32 reset_mask_width; > > + u32 reset_mask_shift; > > + u32 idx = 0; > > + u32 status; > > + int ret; > > + > > + if (!of_device_is_compatible(pdev->dev.of_node, > > "aspeed,ast2400-wdt")) { > > + res = platform_get_resource(pdev, > IORESOURCE_MEM, 0); > > + idx = ((intptr_t)wdt->base & 0x00000fff) / > > resource_size(res); > > + } > > + > > + scu_base = > > syscon_regmap_lookup_by_compatible(scu.compatible); > > + if (IS_ERR(scu_base)) { > > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > > + return; > > + } > > + > > + ret = regmap_read(scu_base, scu.reset_status_reg, &status); > > + if (ret) { > > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > > + return; > > + } > > + > > + reset_mask_width = hweight32(scu.wdt_reset_mask); > > + reset_mask_shift = scu.wdt_reset_mask_shift + > > + reset_mask_width * idx; > > + > > + if (status & (scu.wdt_reset_mask << reset_mask_shift)) > > + wdt->wdd.bootstatus = WDIOF_CARDRESET; > > How precise is the wording in your commit message? The AST2600, for > instance, has 4 reset types: > > 1. "SoC" > 2. "Full" > 3. "ARM" > 4. "Software" > > In the commit message, you said: > > > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > The commit message should be changed for more precisely. "WDIOF_CARDRESET => the system is reset due to WDT timeout occurs." This change can be updated with a new patch version. > However the logic here (with the way you've initialised the config struct for the > AST2600) will signal WDIOF_CARDRESET even if what occurred was e.g. a > (graceful?) software reset. > As the discussion in the previous patch series, there is no consensus for graceful reset flag in WDT framework. Thus, this patch only distinguishes WDT reset from other reset reasons. > The only thing WDIOF_CARDRESET differentiates is a power-on reset from any > other kind of watchdog-driven reset. > Yes. > Is that what's intended? I'm just wary of confusion for what appears to be a > generic use of "SoC" in the commit message vs the specific "SoC" > mode of the watchdog controller (should some documentation be > written/updated?) > The commit message can be updated. Chin-Ting
On Mon, 2025-01-13 at 03:41 +0000, Chin-Ting Kuo wrote: > Hi Andrew, > > > -----Original Message----- > > From: Andrew Jeffery <andrew@codeconstruct.com.au> > > Sent: Monday, January 13, 2025 11:29 AM > > Subject: Re: [PATCH v6 1/1] watchdog: aspeed: Update bootstatus > > handling > > > > On Sun, 2025-01-12 at 16:12 +0800, Chin-Ting Kuo wrote: > > > The boot status in the watchdog device struct is updated during > > > controller probe stage. Application layer can get the boot status > > > through the command, cat > > > /sys/class/watchdog/watchdogX/bootstatus. > > > The bootstatus can be, > > > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > > > Others => other reset events, e.g., power on reset. > > > > > > On ASPEED platform, the boot status is recorded in the SCU > > > registers. > > > - AST2400: Only a bit represents for any WDT reset. > > > - AST2500/AST2600: The reset triggered by different WDT > > > controllers > > > can be distinguished by different SCU bits. > > > > > > Besides, on AST2400 and AST2500, since alternating boot event is > > > triggered by WDT SoC reset, it is classified as WDIOF_CARDRESET. > > > > > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > > > --- > > > drivers/watchdog/aspeed_wdt.c | 81 > > > ++++++++++++++++++++++++++++++++++- > > > 1 file changed, 79 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/watchdog/aspeed_wdt.c > > > b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..369635b38ca0 > > > 100644 > > > --- a/drivers/watchdog/aspeed_wdt.c > > > +++ b/drivers/watchdog/aspeed_wdt.c > > > @@ -11,21 +11,30 @@ > > > #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; > > > module_param(nowayout, bool, 0); > > > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once > > started > > > (default=" > > > __MODULE_STRING(WATC > > HDOG_NOWAYOUT) > > > ")"); > > > +struct aspeed_wdt_scu { > > > + const char *compatible; > > > + u32 reset_status_reg; > > > + u32 wdt_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 { > > > @@ -39,18 +48,36 @@ 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 = 0x3c, > > > + .wdt_reset_mask = 0x1, > > > + .wdt_reset_mask_shift = 1, > > > + }, > > > }; > > > > > > 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 = 0x3c, > > > + .wdt_reset_mask = 0x1, > > > + .wdt_reset_mask_shift = 2, > > > + }, > > > }; > > > > > > 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 = 0x74, > > > + .wdt_reset_mask = 0xf, > > > + .wdt_reset_mask_shift = 16, > > > + }, > > > }; > > > > > > static const struct of_device_id aspeed_wdt_of_table[] = { @@ - > > > 213,6 > > > +240,56 @@ static int aspeed_wdt_restart(struct watchdog_device > > > *wdd, > > > return 0; > > > } > > > > > > +static void aspeed_wdt_update_bootstatus(struct platform_device > > > *pdev, > > > + struct > > aspeed_wdt *wdt) { > > > + const struct resource *res; > > > + struct aspeed_wdt_scu scu = wdt->cfg->scu; > > > + struct regmap *scu_base; > > > + u32 reset_mask_width; > > > + u32 reset_mask_shift; > > > + u32 idx = 0; > > > + u32 status; > > > + int ret; > > > + > > > + if (!of_device_is_compatible(pdev->dev.of_node, > > > "aspeed,ast2400-wdt")) { > > > + res = platform_get_resource(pdev, > > IORESOURCE_MEM, 0); > > > + idx = ((intptr_t)wdt->base & 0x00000fff) / > > > resource_size(res); > > > + } > > > + > > > + scu_base = > > > syscon_regmap_lookup_by_compatible(scu.compatible); > > > + if (IS_ERR(scu_base)) { > > > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > > > + return; > > > + } > > > + > > > + ret = regmap_read(scu_base, scu.reset_status_reg, > > > &status); > > > + if (ret) { > > > + wdt->wdd.bootstatus = WDIOS_UNKNOWN; > > > + return; > > > + } > > > + > > > + reset_mask_width = hweight32(scu.wdt_reset_mask); > > > + reset_mask_shift = scu.wdt_reset_mask_shift + > > > + reset_mask_width * idx; > > > + > > > + if (status & (scu.wdt_reset_mask << reset_mask_shift)) > > > + wdt->wdd.bootstatus = WDIOF_CARDRESET; > > > > How precise is the wording in your commit message? The AST2600, for > > instance, has 4 reset types: > > > > 1. "SoC" > > 2. "Full" > > 3. "ARM" > > 4. "Software" > > > > In the commit message, you said: > > > > > WDIOF_CARDRESET => the system is reset by WDT SoC reset. > > > > The commit message should be changed for more precisely. > "WDIOF_CARDRESET => the system is reset due to WDT timeout occurs." > This change can be updated with a new patch version. > > > However the logic here (with the way you've initialised the config > > struct for the > > AST2600) will signal WDIOF_CARDRESET even if what occurred was e.g. > > a > > (graceful?) software reset. > > > > As the discussion in the previous patch series, there is no consensus > for graceful > reset flag in WDT framework. Thus, this patch only distinguishes WDT > reset from > other reset reasons. > > > The only thing WDIOF_CARDRESET differentiates is a power-on reset > > from any > > other kind of watchdog-driven reset. > > > > Yes. > > > Is that what's intended? I'm just wary of confusion for what > > appears to be a > > generic use of "SoC" in the commit message vs the specific "SoC" > > mode of the watchdog controller (should some documentation be > > written/updated?) > > > > The commit message can be updated. Okay, thanks. Andrew
This patch series inherits the patch submitted by Peter. https://patchwork.kernel.org/project/linux-watchdog/patch/20240430143114.1323686-2-peteryin.openbmc@gmail.com/ Bootstatus mechanism is reconstructed to the latest architecture and for all existing ASPEED BMC platforms. 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. Changes in v4: - Update the commit message for updating bootstatus handling patch. - Rename aspeed_wdt_config struct to aspeed_wdt_data. - Create restart callback function. Changes in v5: - Remove SW reset mechanism since there is no consensus about bootstatus for SW reset currently. - Correct the method for clearing reset event flag on AST2400 and AST2500 legacy platforms. Changes in v6: - Use resource_size() function to get WDT controller registers size. Chin-Ting Kuo (1): watchdog: aspeed: Update bootstatus handling drivers/watchdog/aspeed_wdt.c | 81 ++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)