[PATCH v9 1/1] watchdog: aspeed: Revise handling of bootstatus

Tue Apr 30 18:44:55 PDT 2024

Hi Peter,

Overall this seems okay, however I have some follow-ups to my naming
nitpicks on v8. Broadly, my preferences are that:

 * It's clear from the macro name what SoC, controller and register
   each macro applies to
 * We have a consistent structure in the macro naming -
   <soc>_<controller>_<register>_<description> - i.e. the values for
   <soc> (AST, AST2400, AST2500, AST2600), <controller> (SCU), and
   <register> (SYS_RESET) are consistent across the macro names
 * I prefer consistent use of 'mask' instead of 'flag' for things that
   are used as masks, as to me flag implies a constraint of a single
   bit, where mask doesn't feel like it has such a constraint. However,
   it's fine if a mask consists of a single bit, it's still a mask.

On Tue, 2024-04-30 at 22:31 +0800, Peter Yin wrote:
> 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. However, this bit can be clear when
> watchdog register 0x0c bit1(Reset System after timeout) is enabled.
> Thereforce include SCU register to veriy WDIOF_EXTERN1 and WDIOF_CARDRESET
> in ast2600 SCU74 or ast2400/ast2500 SCU3C.
> 
> Fixes: 49d4d277ca54 ("aspeed: watchdog: Set bootstatus during probe")
> Signed-off-by: Peter Yin <peteryin.openbmc at gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 90 +++++++++++++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index b4773a6aaf8c..556493763793 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,10 +24,38 @@ 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 AST_SCU_SYS_RESET_POWERON_MASK BIT(0)
> +#define AST_SCU_SYS_RESET_EXTERN_FLAG BIT(1)

s/_FLAG/_MASK/ here too?

> +
> +#define AST2400_SYSTEM_RESET_STATUS 0x3C

You chose to use my suggestion of `..._SCU_SYS_RESET_...` for the
POWERON and EXTERN macros above, but here you've dropped `SCU` and also
used `SYSTEM_RESET` instead `SYS_RESET`. I'd prefer we pick a
consistent register name, so

```
#define AST2400_SCU_SYS_RESET_STATUS 0x3c
```

> +#define AST2400_WATCHDOG_RESET_MASK BIT(1)

Again, I'd prefer all these field macros at least have `SCU` in the
name, and preferably the register name too, so:

```
#define AST2400_SCU_SYS_RESET_WDT_MASK BIT(1)
```

> +#define AST2400_RESET_FLAG_CLEAR GENMASK(2, 0)

s/FLAG/FLAGS/ given it's defined over multiple bits? Also, to include
the register name in the macro name:

```
#define AST2400_SCU_SYS_RESET_FLAGS_CLEAR GENMASK(2, 0)
```

> +
> +#define AST2500_WATCHDOG_RESET_MASK GENMASK(4, 2)
> +#define AST2500_RESET_FLAG_CLEAR (AST2500_WATCHDOG_RESET_MASK | \
> + AST_SCU_SYS_RESET_POWERON_MASK | \
> + AST_SCU_SYS_RESET_EXTERN_FLAG)

The same comments above apply to the AST2500 macros.

> +
> +#define AST2600_SYSTEM_RESET_STATUS 0x74
> +#define AST2600_WATCHDOG_RESET_MASK GENMASK(31, 16)
> +#define AST2600_RESET_FLAG_CLEAR (AST2600_WATCHDOG_RESET_MASK | \
> + AST_SCU_SYS_RESET_POWERON_MASK | \
> + AST_SCU_SYS_RESET_EXTERN_FLAG)

... and the AST2600 macros.

> +
>  struct aspeed_wdt_config {
>  u32 ext_pulse_width_mask;
>  u32 irq_shift;
>  u32 irq_mask;
> + struct {
> + const char *compatible;
> + u32 reset_status_reg;
> + u32 watchdog_reset_mask;
> + u32 extern_reset_mask;
> + u32 reset_flag_clear;
> + } scu;
>  };
>  struct aspeed_wdt {
> @@ -39,18 +69,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_SYSTEM_RESET_STATUS,
> + .watchdog_reset_mask = AST2400_WATCHDOG_RESET_MASK,
> + .extern_reset_mask = 0,
> + .reset_flag_clear = AST2400_RESET_FLAG_CLEAR,
> + }
>  };
>  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_SYSTEM_RESET_STATUS,
> + .watchdog_reset_mask = AST2500_WATCHDOG_RESET_MASK,
> + .extern_reset_mask = AST_SCU_SYS_RESET_EXTERN_FLAG,
> + .reset_flag_clear = AST2500_RESET_FLAG_CLEAR,
> + }
>  };
>  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_SYSTEM_RESET_STATUS,
> + .watchdog_reset_mask = AST2600_WATCHDOG_RESET_MASK,
> + .extern_reset_mask = AST_SCU_SYS_RESET_EXTERN_FLAG,
> + .reset_flag_clear = AST2600_RESET_FLAG_CLEAR,
> + }
>  };
>  static const struct of_device_id aspeed_wdt_of_table[] = {
> @@ -310,6 +361,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>  const struct of_device_id *ofdid;
>  struct aspeed_wdt *wdt;
>  struct device_node *np;
> + struct regmap *scu;
>  const char *reset_type;
>  u32 duration;
>  u32 status;
> @@ -458,14 +510,36 @@ 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;
> -
> - if (of_device_is_compatible(np, "aspeed,ast2400-wdt") ||
> - of_device_is_compatible(np, "aspeed,ast2500-wdt"))
> - wdt->wdd.groups = bswitch_groups;
> - }
> + /*
> + * Power on reset is set when triggered by AC or SRSRST.

s/SRSRST/SRST/

> + * Thereforce, we clear flag to ensure

s/Thereforce/Therefore/

Also the line-wrapping for the comment seems a bit aggressive?

> + * next boot cause is a real watchdog case.
> + * We use the external reset flag to determine
> + * if it is an external reset or card reset.
> + * However, The ast2400 watchdog flag is cleared by an external reset,
> + * so it only supports WDIOF_CARDRESET.
> + */
> + scu = syscon_regmap_lookup_by_compatible(wdt->cfg->scu.compatible);
> + if (IS_ERR(scu))
> + return PTR_ERR(scu);
> +
> + ret = regmap_read(scu, wdt->cfg->scu.reset_status_reg, &status);
> + if (ret)
> + return ret;
> +
> + if (!(status & AST_SCU_SYS_RESET_POWERON_MASK) &&
> + status & wdt->cfg->scu.watchdog_reset_mask)
> + wdt->wdd.bootstatus = (status & wdt->cfg->scu.extern_reset_mask)
> + ? WDIOF_EXTERN1 : WDIOF_CARDRESET;
> +
> + status = wdt->cfg->scu.reset_flag_clear;

Seems unnecessary to assign the mask to clear the reset state into
status?

Andrew

> + ret = regmap_write(scu, wdt->cfg->scu.reset_status_reg, status);
> + 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;
>  dev_set_drvdata(dev, wdt);