[PATCH v4 0/3] Update ASPEED WDT bootstatus

[PATCH v4 0/3] 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 updated 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 restart mechanism is supported by HW
since AST2600 platform and is also included in this
patch series.

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.

Chin-Ting Kuo (3):
  watchdog: aspeed: Update bootstatus handling
  watchdog: aspeed: Change aspeed_wdt_config struct name to
    aspeed_wdt_data
  watchdog: aspeed: Update restart implementations

 drivers/watchdog/aspeed_wdt.c | 170 ++++++++++++++++++++++++++++------
 1 file changed, 144 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

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-update.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@aspeedtech.com>
---
 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;
+	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;
+
+	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;
+
+	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;
+
+	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;
-- 
2.34.1

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> 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-update.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.

I'm quite surprised that the above is relevant for a kernel driver at
all.  Isn't "EXTERN1" a name of a real watchdog signal from your
hardware (my recollection is that there are 2 external watchdogs).  I
think the point of this referenced design document was that most users
of BMCs have "EXTERN1" used a for software reset conditions.
`CARDRESET` should be representing resets by the watchdog itself.

The purpose of this design proposal was not to require very specific
changes to individual watchdog drivers, but to align the userspace use
with the best practices already from other watchdog drivers.  I don't
think the kernel driver should be bending to match a particular
userspace implementation; you should be exposing the information
available to your hardware.

Having said that, it was known that there would need to be changes to
the driver because some of these conditions were not adequately exposed
at all.  I'm just still surprised that we're needing to reference that
document as part of these changes.

> 
> 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.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Andrew Jeffery]

On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote:
> On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > 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-update.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.
> 
> I'm quite surprised that the above is relevant for a kernel driver at
> all.  Isn't "EXTERN1" a name of a real watchdog signal from your
> hardware (my recollection is that there are 2 external watchdogs).

I think you may be referring to WDTRST1 (and WDTRST2) here.

WDIOF_EXTERN1 and WDIOF_EXTERN2 have an unrelated history:

https://github.com/torvalds/linux/blame/master/include/uapi/linux/watchdog.h

>   I
> think the point of this referenced design document was that most
> users
> of BMCs have "EXTERN1" used a for software reset conditions.
> `CARDRESET` should be representing resets by the watchdog itself.

I think this is what Chin-Ting is proposing for the Aspeed driver?

> 
> The purpose of this design proposal was not to require very specific
> changes to individual watchdog drivers, but to align the userspace
> use
> with the best practices already from other watchdog drivers.  I don't
> think the kernel driver should be bending to match a particular
> userspace implementation; you should be exposing the information
> available to your hardware.

I agree with this in principle, but I'm not sure what else is meant to
be done here.

> 
> Having said that, it was known that there would need to be changes to
> the driver because some of these conditions were not adequately
> exposed
> at all.  I'm just still surprised that we're needing to reference
> that
> document as part of these changes.

I think the main question is whether an internal, graceful (userspace-
requested) reset is a reasonable use of WDIOF_EXTERN[12]. My feeling
no. I wonder whether defining a new flag (WDIOF_REBOOT?
WDIOF_GRACEFUL?) in the UAPI would be acceptable?

Andrew

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Andrew,

Thanks for the check.

> -----Original Message-----
> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Monday, November 4, 2024 8:02 AM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote:
> > On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > > 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
> > > -update.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.
> >
> > I'm quite surprised that the above is relevant for a kernel driver at
> > all.  Isn't "EXTERN1" a name of a real watchdog signal from your
> > hardware (my recollection is that there are 2 external watchdogs).
> 
> I think you may be referring to WDTRST1 (and WDTRST2) here.
> 

WDTRST1, wdt_ext, is a pulse signal generated when WDT timeout
occurs. However, depending on the HW board design, wdt_ext doesn’t
always affect the system reset. Thus, EXTERN1 boot status can be
ignored in ASPEED WDT driver and just implement "CARDRESET" and
"others" types since EXTERN1 is not always affected/controlled by WDT
controller directly. Or, an additional property in dts can be added to
distinguish whether the current EXTRST# reset event is triggered by
wdt_ext signal.

> WDIOF_EXTERN1 and WDIOF_EXTERN2 have an unrelated history:
> 
> https://github.com/torvalds/linux/blame/master/include/uapi/linux/watchdog.
> h
> 
> >   I
> > think the point of this referenced design document was that most users
> > of BMCs have "EXTERN1" used a for software reset conditions.
> > `CARDRESET` should be representing resets by the watchdog itself.
> 
> I think this is what Chin-Ting is proposing for the Aspeed driver?
> 

Yes.

> >
> > The purpose of this design proposal was not to require very specific
> > changes to individual watchdog drivers, but to align the userspace use
> > with the best practices already from other watchdog drivers.  I don't
> > think the kernel driver should be bending to match a particular
> > userspace implementation; you should be exposing the information
> > available to your hardware.
> 
> I agree with this in principle, but I'm not sure what else is meant to be done
> here.
> 
> >
> > Having said that, it was known that there would need to be changes to
> > the driver because some of these conditions were not adequately
> > exposed at all.  I'm just still surprised that we're needing to
> > reference that document as part of these changes.
> 
> I think the main question is whether an internal, graceful (userspace-
> requested) reset is a reasonable use of WDIOF_EXTERN[12]. My feeling no. I
> wonder whether defining a new flag (WDIOF_REBOOT?
> WDIOF_GRACEFUL?) in the UAPI would be acceptable?
> 

Agree, but this is out of the scope of this patch series and can be discussed and
implemented in the other future patches. The purpose of this patch series is to
complete the boot status mechanism based on the current reset reason.

> Andrew

Chin-Ting

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Andrew Jeffery]

Hi Chin-Ting,

On Thu, 2024-11-07 at 05:35 +0000, Chin-Ting Kuo wrote:
> Hi Andrew,
> 
> Thanks for the check.
> 
> > -----Original Message-----
> > From: Andrew Jeffery <andrew@codeconstruct.com.au>
> > Sent: Monday, November 4, 2024 8:02 AM
> > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
> > handling
> > 
> > On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote:
> > > On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > > > 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
> > > > -update.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.
> > > 
> > > I'm quite surprised that the above is relevant for a kernel
> > > driver at
> > > all.  Isn't "EXTERN1" a name of a real watchdog signal from your
> > > hardware (my recollection is that there are 2 external
> > > watchdogs).
> > 
> > I think you may be referring to WDTRST1 (and WDTRST2) here.
> > 
> 
> WDTRST1, wdt_ext, is a pulse signal generated when WDT timeout
> occurs. However, depending on the HW board design, wdt_ext doesn’t
> always affect the system reset. Thus, EXTERN1 boot status can be
> ignored in ASPEED WDT driver and just implement "CARDRESET" and
> "others" types since EXTERN1 is not always affected/controlled by WDT
> controller directly. Or, an additional property in dts can be added
> to
> distinguish whether the current EXTRST# reset event is triggered by
> wdt_ext signal.

Yep, I understand how it works. I was responding to Patrick's query to
clear up some confusion around the watchdog signal names.

> > 
> > > 
> > > Having said that, it was known that there would need to be
> > > changes to
> > > the driver because some of these conditions were not adequately
> > > exposed at all.  I'm just still surprised that we're needing to
> > > reference that document as part of these changes.
> > 
> > I think the main question is whether an internal, graceful
> > (userspace-
> > requested) reset is a reasonable use of WDIOF_EXTERN[12]. My
> > feeling no. I
> > wonder whether defining a new flag (WDIOF_REBOOT?
> > WDIOF_GRACEFUL?) in the UAPI would be acceptable?
> > 
> 
> Agree, but this is out of the scope of this patch series and can be
> discussed and
> implemented in the other future patches.

I disagree, because then you're changing the userspace-visible
behaviour of the driver yet again. I don't prefer the proposed patch as
the way forward because I think it is abusing the meaning of
WDIOF_EXTERN1. I think the concept needs input from the watchdog
maintainers.

Andrew

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Friday, November 8, 2024 7:50 AM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> Hi Chin-Ting,
> 
> On Thu, 2024-11-07 at 05:35 +0000, Chin-Ting Kuo wrote:
> > Hi Andrew,
> >
> > Thanks for the check.
> >
> > > -----Original Message-----
> > > From: Andrew Jeffery <andrew@codeconstruct.com.au>
> > > Sent: Monday, November 4, 2024 8:02 AM
> > > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
> > > handling
> > >
> > > On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote:
> > > > On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > > > > 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-c
> > > > > ause
> > > > > -update.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.
> > > >
> > > > I'm quite surprised that the above is relevant for a kernel driver
> > > > at all.  Isn't "EXTERN1" a name of a real watchdog signal from
> > > > your hardware (my recollection is that there are 2 external
> > > > watchdogs).
> > >
> > > I think you may be referring to WDTRST1 (and WDTRST2) here.
> > >
> >
> > WDTRST1, wdt_ext, is a pulse signal generated when WDT timeout occurs.
> > However, depending on the HW board design, wdt_ext doesn’t always
> > affect the system reset. Thus, EXTERN1 boot status can be ignored in
> > ASPEED WDT driver and just implement "CARDRESET" and "others" types
> > since EXTERN1 is not always affected/controlled by WDT controller
> > directly. Or, an additional property in dts can be added to
> > distinguish whether the current EXTRST# reset event is triggered by
> > wdt_ext signal.
> 
> Yep, I understand how it works. I was responding to Patrick's query to clear up
> some confusion around the watchdog signal names.
> 

Okay.

> > >
> > > >
> > > > Having said that, it was known that there would need to be changes
> > > > to the driver because some of these conditions were not adequately
> > > > exposed at all.  I'm just still surprised that we're needing to
> > > > reference that document as part of these changes.
> > >
> > > I think the main question is whether an internal, graceful
> > > (userspace-
> > > requested) reset is a reasonable use of WDIOF_EXTERN[12]. My feeling
> > > no. I wonder whether defining a new flag (WDIOF_REBOOT?
> > > WDIOF_GRACEFUL?) in the UAPI would be acceptable?
> > >
> >
> > Agree, but this is out of the scope of this patch series and can be
> > discussed and
> > implemented in the other future patches.
> 
> I disagree, because then you're changing the userspace-visible
> behaviour of the driver yet again. I don't prefer the proposed patch as
> the way forward because I think it is abusing the meaning of
> WDIOF_EXTERN1. I think the concept needs input from the watchdog
> maintainers.
> 

Previously, I meant that only implement "CARDRESET " and "others"
reset type to complete the current driver for "CARDRESET ".
The implementation of SW restart mechanism can be postponed to the
next new patch series.

But now, I think it will be better to add a patch for creating a new
reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in watchdog.h
of uapi. Can I include this change, creating a new reset reason, in
this patch series? Or, should I create an extra new patch series for
this purpose?

> Andrew

Chin-Ting

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Guenter Roeck]

On 11/7/24 21:42, Chin-Ting Kuo wrote:

> But now, I think it will be better to add a patch for creating a new
> reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in watchdog.h
> of uapi. Can I include this change, creating a new reset reason, in
> this patch series? Or, should I create an extra new patch series for
> this purpose?
> 

This is a UAPI change. That is a major change. It needs to be discussed
separately, on its own, and can not be sneaked in like this.

Guenter

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Guenter,

Thanks for the reply.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, November 8, 2024 10:08 PM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> On 11/7/24 21:42, Chin-Ting Kuo wrote:
> 
> > But now, I think it will be better to add a patch for creating a new
> > reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in watchdog.h of
> > uapi. Can I include this change, creating a new reset reason, in this
> > patch series? Or, should I create an extra new patch series for this
> > purpose?
> >
> 
> This is a UAPI change. That is a major change. It needs to be discussed
> separately, on its own, and can not be sneaked in like this.
> 

Agree. However, how to trigger this discussion? Can I just send a new
patch separately with only this UAPI modification? This is the first time
I change such common source code.

> Guenter

Chin-Ting

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Guenter Roeck]

On 11/18/24 04:46, Chin-Ting Kuo wrote:
> Hi Guenter,
> 
> Thanks for the reply.
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Friday, November 8, 2024 10:08 PM
>> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
>>
>> On 11/7/24 21:42, Chin-Ting Kuo wrote:
>>
>>> But now, I think it will be better to add a patch for creating a new
>>> reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in watchdog.h of
>>> uapi. Can I include this change, creating a new reset reason, in this
>>> patch series? Or, should I create an extra new patch series for this
>>> purpose?
>>>
>>
>> This is a UAPI change. That is a major change. It needs to be discussed
>> separately, on its own, and can not be sneaked in like this.
>>
> 
> Agree. However, how to trigger this discussion? Can I just send a new
> patch separately with only this UAPI modification? This is the first time
> I change such common source code.
> 

Yes. That needs to include arguments explaining why this specific new flag
even adds value. I for my part don't immediately see that value.

Guenter

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Andrew Jeffery]

On Mon, 2024-11-18 at 12:50 -0800, Guenter Roeck wrote:
> On 11/18/24 04:46, Chin-Ting Kuo wrote:
> > Hi Guenter,
> > 
> > Thanks for the reply.
> > 
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> > > Roeck
> > > Sent: Friday, November 8, 2024 10:08 PM
> > > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
> > > handling
> > > 
> > > On 11/7/24 21:42, Chin-Ting Kuo wrote:
> > > 
> > > > But now, I think it will be better to add a patch for creating
> > > > a new
> > > > reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in
> > > > watchdog.h of
> > > > uapi. Can I include this change, creating a new reset reason,
> > > > in this
> > > > patch series? Or, should I create an extra new patch series for
> > > > this
> > > > purpose?
> > > > 
> > > 
> > > This is a UAPI change. That is a major change. It needs to be
> > > discussed
> > > separately, on its own, and can not be sneaked in like this.
> > > 
> > 
> > Agree. However, how to trigger this discussion? Can I just send a
> > new
> > patch separately with only this UAPI modification? This is the
> > first time
> > I change such common source code.
> > 
> 
> Yes. That needs to include arguments explaining why this specific new
> flag
> even adds value. I for my part don't immediately see that value.

So maybe I was derailed with my WDIOF_REBOOT suggestion by the proposal
to repurpose WDIOF_EXTERN1 to indicate a regular reboot. I still don't
think repurposing WDIOF_EXTERN1 is the right direction. But, perhaps
the thing to do for a regular reboot is to not set any reason flags at
all? It just depends on whether we're wanting to separate a cold boot
from a reboot (as they _may_ behave differently on Aspeed hardware), as
on a cold boot we wouldn't set any reason flags either.

Andrew

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Guenter Roeck]

On 11/18/24 15:00, Andrew Jeffery wrote:
> On Mon, 2024-11-18 at 12:50 -0800, Guenter Roeck wrote:
>> On 11/18/24 04:46, Chin-Ting Kuo wrote:
>>> Hi Guenter,
>>>
>>> Thanks for the reply.
>>>
>>>> -----Original Message-----
>>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
>>>> Roeck
>>>> Sent: Friday, November 8, 2024 10:08 PM
>>>> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
>>>> handling
>>>>
>>>> On 11/7/24 21:42, Chin-Ting Kuo wrote:
>>>>
>>>>> But now, I think it will be better to add a patch for creating
>>>>> a new
>>>>> reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in
>>>>> watchdog.h of
>>>>> uapi. Can I include this change, creating a new reset reason,
>>>>> in this
>>>>> patch series? Or, should I create an extra new patch series for
>>>>> this
>>>>> purpose?
>>>>>
>>>>
>>>> This is a UAPI change. That is a major change. It needs to be
>>>> discussed
>>>> separately, on its own, and can not be sneaked in like this.
>>>>
>>>
>>> Agree. However, how to trigger this discussion? Can I just send a
>>> new
>>> patch separately with only this UAPI modification? This is the
>>> first time
>>> I change such common source code.
>>>
>>
>> Yes. That needs to include arguments explaining why this specific new
>> flag
>> even adds value. I for my part don't immediately see that value.
> 
> So maybe I was derailed with my WDIOF_REBOOT suggestion by the proposal
> to repurpose WDIOF_EXTERN1 to indicate a regular reboot. I still don't
> think repurposing WDIOF_EXTERN1 is the right direction. But, perhaps
> the thing to do for a regular reboot is to not set any reason flags at
> all? It just depends on whether we're wanting to separate a cold boot
> from a reboot (as they _may_ behave differently on Aspeed hardware), as
> on a cold boot we wouldn't set any reason flags either.
> 


Thew point here is that this is just a warm reboot which happens to use
the watchdog as vehicle trigger a reset. A UAPI change along that line
would tell the user just that, and I don't see the value in it.

FWIW, the only really valuable flag is WDIOF_CARDRESET. All others are
at best misleading since the watchdog isn't typically involved in
making policy decisions such as WDIOF_OVERHEAT or WDIOF_FANFAULT and thus
can not report such reasons to userspace.

Unfortunately we can at best deprecate all those existing flags. At the same
time I really don't want to introduce new ones unless they provide real value.
While it might be valuable to know if a reboot was a cold or a warm reboot,
the watchdog subsystem is not involved in this either on almost all
platforms, meaning userspace can not really rely on it. Yes, Aspeed
hardware may behave differently, but typically all those differences
would need to be handled in the kernel when (re-)initializing the hardware,
not in userspace.

So, again, what exactly would userspace do with the information that this
was a watchdog triggered warm reboot ? Why would it need that information ?

Thanks,
Guenter

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Andrew Jeffery]

On Mon, 2024-11-18 at 17:27 -0800, Guenter Roeck wrote:
> So, again, what exactly would userspace do with the information that
> this
> was a watchdog triggered warm reboot ? Why would it need that
> information ?

I'll defer to the others on To/Cc to answer that.

My only position is I don't think changing behaviour of existing
drivers to exploit WDIOF_EXTERN1 as a graceful-reboot indicator is a
good idea either. Obviously I don't have much skin in the game with
watchdog maintenance, so my thoughts shouldn't have much influence
beyond the Aspeed-specifics, but I just didn't want to see some fun new
confusion or incompatibility arise as a result.

Andrew

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Wednesday, November 20, 2024 12:54 PM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> On Mon, 2024-11-18 at 17:27 -0800, Guenter Roeck wrote:
> > So, again, what exactly would userspace do with the information that
> > this was a watchdog triggered warm reboot ? Why would it need that
> > information ?
> 
> I'll defer to the others on To/Cc to answer that.
> 
> My only position is I don't think changing behaviour of existing drivers to
> exploit WDIOF_EXTERN1 as a graceful-reboot indicator is a good idea either.
> Obviously I don't have much skin in the game with watchdog maintenance, so
> my thoughts shouldn't have much influence beyond the Aspeed-specifics, but I
> just didn't want to see some fun new confusion or incompatibility arise as a
> result.
> 
Agree to your misgiving, in the next patches, only two categories, "Power on reset"
and "WDT reset" (Card reset), will be taken into consideration. The graceful-reboot
scenario will be postponed to the patches in the future.


Thanks,
Chin-Ting

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Patrick,

Thanks for the check and reply.
	
> -----Original Message-----
> From: Patrick Williams <patrick@stwcx.xyz>
> Sent: Saturday, November 2, 2024 2:21 AM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > 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.
> 
> I'm quite surprised that the above is relevant for a kernel driver at all.  Isn't
> "EXTERN1" a name of a real watchdog signal from your hardware (my
> recollection is that there are 2 external watchdogs).  I think the point of this
> referenced design document was that most users of BMCs have "EXTERN1"
> used a for software reset conditions.
> `CARDRESET` should be representing resets by the watchdog itself.
> 

ASPEED WDT controller is able to generate an external signal, wdt_ext, when the
WDT timeout occurs. However, after system is rebooted, WDT controller doesn't
know whether wdt_ext signal is generated previously. Moreover, whether BMC is
reset due to this wdt_ext signal depends on the HW board design. On some boards,
wdt_ext can affect EXTRST#, BMC external reset signal, indirectly. For this scenario,
EXTERN1 can be classified to wdt_ext. On the other boards, wdt_ext just represents
WDT timeout event for specific tasks. Thus, EXTERN1 boot status can be ignored in
ASPEED WDT driver and just implement "CARDRESET" and "others" since EXTERN1
is not always affected/controlled by WDT controller directly. Or, an additional
property in dts can be added to distinguish whether the EXTRST# reset event is
triggered by wdt_ext signal.

> The purpose of this design proposal was not to require very specific changes to
> individual watchdog drivers, but to align the userspace use with the best
> practices already from other watchdog drivers.  I don't think the kernel driver
> should be bending to match a particular userspace implementation; you should
> be exposing the information available to your hardware.
> 

Agree.

> Having said that, it was known that there would need to be changes to the
> driver because some of these conditions were not adequately exposed at all.
> I'm just still surprised that we're needing to reference that document as part of
> these changes.
> 

Yes, if only "CARDRESET" and "others" types are taken into consideration, some
patches are still needed to complete the boot status mechanism in ASPEED
WDT driver.

> >
> > 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.
> 
> --
> Patrick Williams


Chin-Ting

Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Guenter Roeck]

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-update.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@aspeedtech.com>
> ---
>   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.

> +	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;
		}
	}

> +	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 ?

> +
> +	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.

> +
> +	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;

RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Chin-Ting Kuo]

Hi Guenter,

Thanks for the review.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> 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@aspeedtech.com>
> > ---
> >   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

[PATCH v4 2/3] watchdog: aspeed: Change aspeed_wdt_config struct name to aspeed_wdt_data

[Chin-Ting Kuo]

aspeed_wdt_config struct is used to store some HW configuration
information. Changing its naming to a more generic one,
aspeed_wdt_data, in order to contain more platform specific
inforamtion or SW callback functions.

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

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 4ad6335ff25b..63b5ff9e2751 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -31,7 +31,7 @@ struct aspeed_wdt_scu {
 	u32 wdt_reset_mask_shift;
 };
 
-struct aspeed_wdt_config {
+struct aspeed_wdt_data {
 	u32 ext_pulse_width_mask;
 	u32 irq_shift;
 	u32 irq_mask;
@@ -42,10 +42,10 @@ struct aspeed_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*base;
 	u32			ctrl;
-	const struct aspeed_wdt_config *cfg;
+	const struct aspeed_wdt_data *data;
 };
 
-static const struct aspeed_wdt_config ast2400_config = {
+static const struct aspeed_wdt_data ast2400_data = {
 	.ext_pulse_width_mask = 0xff,
 	.irq_shift = 0,
 	.irq_mask = 0,
@@ -58,7 +58,7 @@ static const struct aspeed_wdt_config ast2400_config = {
 	},
 };
 
-static const struct aspeed_wdt_config ast2500_config = {
+static const struct aspeed_wdt_data ast2500_data = {
 	.ext_pulse_width_mask = 0xfffff,
 	.irq_shift = 12,
 	.irq_mask = GENMASK(31, 12),
@@ -71,7 +71,7 @@ static const struct aspeed_wdt_config ast2500_config = {
 	},
 };
 
-static const struct aspeed_wdt_config ast2600_config = {
+static const struct aspeed_wdt_data ast2600_data = {
 	.ext_pulse_width_mask = 0xfffff,
 	.irq_shift = 0,
 	.irq_mask = GENMASK(31, 10),
@@ -85,9 +85,9 @@ static const struct aspeed_wdt_config ast2600_config = {
 };
 
 static const struct of_device_id aspeed_wdt_of_table[] = {
-	{ .compatible = "aspeed,ast2400-wdt", .data = &ast2400_config },
-	{ .compatible = "aspeed,ast2500-wdt", .data = &ast2500_config },
-	{ .compatible = "aspeed,ast2600-wdt", .data = &ast2600_config },
+	{ .compatible = "aspeed,ast2400-wdt", .data = &ast2400_data },
+	{ .compatible = "aspeed,ast2500-wdt", .data = &ast2500_data },
+	{ .compatible = "aspeed,ast2600-wdt", .data = &ast2600_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
@@ -216,8 +216,8 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd,
 {
 	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
 	u32 actual = pretimeout * WDT_RATE_1MHZ;
-	u32 s = wdt->cfg->irq_shift;
-	u32 m = wdt->cfg->irq_mask;
+	u32 s = wdt->data->irq_shift;
+	u32 m = wdt->data->irq_mask;
 
 	wdd->pretimeout = pretimeout;
 	wdt->ctrl &= ~m;
@@ -248,7 +248,7 @@ 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 aspeed_wdt_scu scu = wdt->data->scu;
 	struct regmap *scu_base;
 	u32 reset_mask_width;
 	u32 reset_mask_shift;
@@ -401,7 +401,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	ofdid = of_match_node(aspeed_wdt_of_table, np);
 	if (!ofdid)
 		return -EINVAL;
-	wdt->cfg = ofdid->data;
+	wdt->data = ofdid->data;
 
 	wdt->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(wdt->base))
@@ -409,7 +409,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 
 	wdt->wdd.info = &aspeed_wdt_info;
 
-	if (wdt->cfg->irq_mask) {
+	if (wdt->data->irq_mask) {
 		int irq = platform_get_irq_optional(pdev, 0);
 
 		if (irq > 0) {
@@ -485,7 +485,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		size_t nrstmask = of_device_is_compatible(np, "aspeed,ast2600-wdt") ? 2 : 1;
 		u32 reg = readl(wdt->base + WDT_RESET_WIDTH);
 
-		reg &= wdt->cfg->ext_pulse_width_mask;
+		reg &= wdt->data->ext_pulse_width_mask;
 		if (of_property_read_bool(np, "aspeed,ext-active-high"))
 			reg |= WDT_ACTIVE_HIGH_MAGIC;
 		else
@@ -493,7 +493,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 
 		writel(reg, wdt->base + WDT_RESET_WIDTH);
 
-		reg &= wdt->cfg->ext_pulse_width_mask;
+		reg &= wdt->data->ext_pulse_width_mask;
 		if (of_property_read_bool(np, "aspeed,ext-push-pull"))
 			reg |= WDT_PUSH_PULL_MAGIC;
 		else
@@ -510,7 +510,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	}
 
 	if (!of_property_read_u32(np, "aspeed,ext-pulse-duration", &duration)) {
-		u32 max_duration = wdt->cfg->ext_pulse_width_mask + 1;
+		u32 max_duration = wdt->data->ext_pulse_width_mask + 1;
 
 		if (duration == 0 || duration > max_duration) {
 			dev_err(dev, "Invalid pulse duration: %uus\n",
-- 
2.34.1

Re: [PATCH v4 2/3] watchdog: aspeed: Change aspeed_wdt_config struct name to aspeed_wdt_data

[Guenter Roeck]

On 11/1/24 05:12, Chin-Ting Kuo wrote:
> aspeed_wdt_config struct is used to store some HW configuration
> information. Changing its naming to a more generic one,
> aspeed_wdt_data, in order to contain more platform specific
> inforamtion or SW callback functions.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>

I fail to see the point of this patch. It is just unnecessary churn.
Just like drivers should not be renamed because of an extended scope
or because someone doesn't like the old name, renaming variables should
be avoided as well. Such renames just make future bug fixes (which may
need to be backported) more difficult.

Guenter

[PATCH v4 3/3] watchdog: aspeed: Update restart implementations

[Chin-Ting Kuo]

Restart callback function is created to distinguish the restart mechanisms
between AST2400/AST2500 and AST2600.

On AST2400 and AST2500 platforms, system can only be reset by enabling
WDT and waiting for WDT timeout. Since AST2600, SW can trigger the reset
event consciously and directly by just cinfiguring some HW registers
without waiting for WDT timeout. This mechanism is named "SW restart" and
is implemented by HW. The WDT counter is not enabled when SW restart
is adopted.

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

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 63b5ff9e2751..28a16a0c442d 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -23,6 +23,14 @@ 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 {
+	struct watchdog_device	wdd;
+	void __iomem		*base;
+	u32			ctrl;
+	const struct aspeed_wdt_data *data;
+};
+
 struct aspeed_wdt_scu {
 	const char *compatible;
 	u32 reset_status_reg;
@@ -36,14 +44,11 @@ struct aspeed_wdt_data {
 	u32 irq_shift;
 	u32 irq_mask;
 	struct aspeed_wdt_scu scu;
+	int (*restart)(struct aspeed_wdt *wdt);
 };
 
-struct aspeed_wdt {
-	struct watchdog_device	wdd;
-	void __iomem		*base;
-	u32			ctrl;
-	const struct aspeed_wdt_data *data;
-};
+static int aspeed_ast2400_wdt_restart(struct aspeed_wdt *wdt);
+static int aspeed_ast2600_wdt_restart(struct aspeed_wdt *wdt);
 
 static const struct aspeed_wdt_data ast2400_data = {
 	.ext_pulse_width_mask = 0xff,
@@ -56,6 +61,7 @@ static const struct aspeed_wdt_data ast2400_data = {
 		.wdt_sw_reset_mask = 0,
 		.wdt_reset_mask_shift = 1,
 	},
+	.restart = aspeed_ast2400_wdt_restart,
 };
 
 static const struct aspeed_wdt_data ast2500_data = {
@@ -69,6 +75,7 @@ static const struct aspeed_wdt_data ast2500_data = {
 		.wdt_sw_reset_mask = 0,
 		.wdt_reset_mask_shift = 2,
 	},
+	.restart = aspeed_ast2400_wdt_restart,
 };
 
 static const struct aspeed_wdt_data ast2600_data = {
@@ -82,6 +89,7 @@ static const struct aspeed_wdt_data ast2600_data = {
 		.wdt_sw_reset_mask = 0x8,
 		.wdt_reset_mask_shift = 16,
 	},
+	.restart = aspeed_ast2600_wdt_restart,
 };
 
 static const struct of_device_id aspeed_wdt_of_table[] = {
@@ -112,6 +120,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
@@ -231,11 +244,8 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd,
 	return 0;
 }
 
-static int aspeed_wdt_restart(struct watchdog_device *wdd,
-			      unsigned long action, void *data)
+static int aspeed_ast2400_wdt_restart(struct aspeed_wdt *wdt)
 {
-	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
-
 	wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY;
 	aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
 
@@ -244,6 +254,35 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
 	return 0;
 }
 
+static int aspeed_ast2600_wdt_restart(struct aspeed_wdt *wdt)
+{
+	u32 reg;
+	u32 ctrl = WDT_CTRL_RESET_MODE_SOC |
+		   WDT_CTRL_RESET_SYSTEM;
+
+	reg = readl(wdt->base + WDT_RESET_MASK1);
+	writel(reg, wdt->base + WDT_SW_RESET_MASK1);
+	reg = readl(wdt->base + WDT_RESET_MASK2);
+	writel(reg, wdt->base + WDT_SW_RESET_MASK2);
+
+	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);
+
+	return 0;
+}
+
+static int aspeed_wdt_restart(struct watchdog_device *wdd,
+			      unsigned long action, void *data)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+
+	return wdt->data->restart(wdt);
+}
+
 static int aspeed_wdt_update_bootstatus(struct platform_device *pdev,
 					struct aspeed_wdt *wdt)
 {
-- 
2.34.1