Hi Guenter, Thanks for the review. > -----Original Message----- > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > Sent: Saturday, November 2, 2024 6:17 AM > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling > > On 11/1/24 05:11, 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 boot status mapping rule follows the latest design guide from the > > OpenBMC shown as below. > > > https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-up > date.md#proposed-design > > - WDIOF_EXTERN1 => system is reset by Software > > - WDIOF_CARDRESET => 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: The reset triggered by different WDT controllers can be > > distinguished by different SCU bits. But, WDIOF_EXTERN1 or > > WDIOF_CARDRESET still cannot be identified due to > > HW limitation. > > - AST2600: Different from AST2500, additional HW bits are added for > > distinguishing WDIOF_EXTERN1 and WDIOF_CARDRESET. > > > > Besides, 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@xxxxxxxxxxxxxx> > > --- > > drivers/watchdog/aspeed_wdt.c | 83 > ++++++++++++++++++++++++++++++++++- > > 1 file changed, 81 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/watchdog/aspeed_wdt.c > > b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..4ad6335ff25b > > 100644 > > --- a/drivers/watchdog/aspeed_wdt.c > > +++ b/drivers/watchdog/aspeed_wdt.c > > @@ -11,21 +11,31 @@ > > #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_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 { > > @@ -39,18 +49,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 = 0x3c, > > + .wdt_reset_mask = 0x1, > > + .wdt_sw_reset_mask = 0, > > + .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_sw_reset_mask = 0, > > + .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_sw_reset_mask = 0x8, > > + .wdt_reset_mask_shift = 16, > > + }, > > }; > > > > static const struct of_device_id aspeed_wdt_of_table[] = { @@ -213,6 > > +244,52 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd, > > return 0; > > } > > > > +static int aspeed_wdt_update_bootstatus(struct platform_device *pdev, > > + struct aspeed_wdt *wdt) > > +{ > > + struct resource *res; > > + struct aspeed_wdt_scu scu = wdt->cfg->scu; > > + struct regmap *scu_base; > > + u32 reset_mask_width; > > + u32 reset_mask_shift; > > + u32 reg_size = 0; > > Please no unnecesary initializations. > Okay, it will be updated in the next patch version. > > + u32 idx = 0; > > + u32 status; > > + int ret; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + reg_size = res->end - res->start; > > + > > + if (reg_size != 0) > > + idx = ((intptr_t)wdt->base & 0x00000fff) / reg_size; > > + > > + /* On ast2400, only a bit is used to represent WDT reset */ > > + if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2400-wdt")) > > + idx = 0; > > + > > There is some redundancy in the above code, and platform_get_resource() can > return NULL. If idx==0 for aspeed,ast2400-wdt anyway, the code can be > rewritten as > > if (!of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2400-wdt")) > { > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (res) { > reg_size = res->end - res->start; > if (reg_size) > idx = ((intptr_t)wdt->base & 0x00000fff) / reg_size; > } > } > Okay, it looks more concise. Thanks for the reminder. > > + 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; > > The above only affects bootstatus. Why fail to load the driver just because > bootstatus can not be read ? > It will be updated in the next patch. > > + > > + reset_mask_width = hweight32(scu.wdt_reset_mask); > > + reset_mask_shift = scu.wdt_reset_mask_shift + > > + reset_mask_width * 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; > > That is already 0. > Okay. > > + > > + return regmap_write(scu_base, scu.reset_status_reg, > > + scu.wdt_reset_mask << reset_mask_shift); } > > + > > /* 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) @@ > -458,10 > > +535,12 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > > writel(duration - 1, wdt->base + WDT_RESET_WIDTH); > > } > > > > + ret = aspeed_wdt_update_bootstatus(pdev, wdt); > > + if (ret) > > + return ret; > > + > > status = readl(wdt->base + WDT_TIMEOUT_STATUS); > > if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) { > > - 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; Chin-Ting
