| Message ID | 20250218031709.103823-1-guoheyi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] driver/aspeed-wdt: fix pretimeout for counting down logic | expand |
On 2/17/25 19:16, Heyi Guo wrote: > Aspeed watchdog uses counting down logic, so the value set to register > should be the value of subtracting pretimeout from total timeout. > > Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") > > Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> > > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Joel Stanley <joel@jms.id.au> > Cc: Andrew Jeffery <andrew@codeconstruct.com.au> > Cc: Eddie James <eajames@linux.ibm.com> > --- > drivers/watchdog/aspeed_wdt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > index b4773a6aaf8c..520d8aba12a5 100644 > --- a/drivers/watchdog/aspeed_wdt.c > +++ b/drivers/watchdog/aspeed_wdt.c > @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd, > u32 actual = pretimeout * WDT_RATE_1MHZ; > u32 s = wdt->cfg->irq_shift; > u32 m = wdt->cfg->irq_mask; > + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); > + It is unusual to use a register value here and not the configured timeout value. I would have assumed that pretimeout is compared against wdt->timout, not against the register value, and that the multiplication with WDT_RATE_1MHZ is done after validation. This needs an explanation. > + if (actual >= reload) > + return -EINVAL; > + On top of that, you'll also need to explain why watchdog_pretimeout_invalid() and with it the validation in watchdog_set_pretimeout() does not work for this watchdog and why this extra validation is necessary. Guenter > + /* watchdog timer is counting down */ > + actual = reload - actual; > > wdd->pretimeout = pretimeout; > wdt->ctrl &= ~m;
On Tue, 18 Feb 2025 11:16:58 +0800, Heyi Guo wrote: > Aspeed watchdog uses counting down logic, so the value set to register > should be the value of subtracting pretimeout from total timeout. > > Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") > > Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> > > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Joel Stanley <joel@jms.id.au> > Cc: Andrew Jeffery <andrew@codeconstruct.com.au> > Cc: Eddie James <eajames@linux.ibm.com> > --- > drivers/watchdog/aspeed_wdt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y for arch/arm/boot/dts/aspeed/' for 20250218031709.103823-1-guoheyi@linux.alibaba.com: arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-s6q.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-s6q.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-s6q.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-quanta-s6q.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge-4u.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge-4u.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge-4u.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge-4u.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb-a1.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb-a1.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb-a1.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-fuji.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb-a1.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-fuji.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-fuji.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-fuji.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-sbp1.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-sbp1.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-sbp1.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-sbp1.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-yosemite4.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-starscream.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-starscream.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-starscream.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-starscream.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-1s4u.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-1s4u.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-1s4u.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-1s4u.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-catalina.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-catalina.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-catalina.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-catalina.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-blueridge.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-4u.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-4u.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-4u.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier-4u.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-asus-x4tf.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-asus-x4tf.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-asus-x4tf.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-asus-x4tf.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ufispace-ncplite.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ufispace-ncplite.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ufispace-ncplite.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ufispace-ncplite.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-minerva.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-minerva.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-minerva.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-minerva.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-qcom-dc-scm-v1.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-qcom-dc-scm-v1.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-qcom-dc-scm-v1.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-qcom-dc-scm-v1.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-greatlakes.dtb: watchdog@1e785000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-greatlakes.dtb: watchdog@1e785040: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-greatlakes.dtb: watchdog@1e785080: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml# arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-greatlakes.dtb: watchdog@1e7850c0: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml#
On Mon, 2025-02-17 at 21:33 -0800, Guenter Roeck wrote: > On 2/17/25 19:16, Heyi Guo wrote: > > Aspeed watchdog uses counting down logic, so the value set to register > > should be the value of subtracting pretimeout from total timeout. > > > > Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") > > > > Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> > > > > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Joel Stanley <joel@jms.id.au> > > Cc: Andrew Jeffery <andrew@codeconstruct.com.au> > > Cc: Eddie James <eajames@linux.ibm.com> > > --- > > drivers/watchdog/aspeed_wdt.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > > index b4773a6aaf8c..520d8aba12a5 100644 > > --- a/drivers/watchdog/aspeed_wdt.c > > +++ b/drivers/watchdog/aspeed_wdt.c > > @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd, > > u32 actual = pretimeout * WDT_RATE_1MHZ; > > u32 s = wdt->cfg->irq_shift; > > u32 m = wdt->cfg->irq_mask; > > + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); > > + > > It is unusual to use a register value here and not the configured timeout > value. I would have assumed that pretimeout is compared against wdt->timout, > not against the register value, and that the multiplication with WDT_RATE_1MHZ > is done after validation. This needs an explanation. +1 > > > + if (actual >= reload) > > + return -EINVAL; > > + > > On top of that, you'll also need to explain why watchdog_pretimeout_invalid() > and with it the validation in watchdog_set_pretimeout() does not work for this > watchdog and why this extra validation is necessary. +1 as well. Further, the logic looks broken regardless for the AST2400 where there's no pretimeout support. aspeed_wdt_set_pretimeout() should error out if wdt->cfg->irq_mask is 0. Andrew
On 2/18/25 17:25, Andrew Jeffery wrote: > On Mon, 2025-02-17 at 21:33 -0800, Guenter Roeck wrote: >> On 2/17/25 19:16, Heyi Guo wrote: >>> Aspeed watchdog uses counting down logic, so the value set to register >>> should be the value of subtracting pretimeout from total timeout. >>> >>> Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") >>> >>> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> >>> >>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: Joel Stanley <joel@jms.id.au> >>> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> >>> Cc: Eddie James <eajames@linux.ibm.com> >>> --- >>> drivers/watchdog/aspeed_wdt.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c >>> index b4773a6aaf8c..520d8aba12a5 100644 >>> --- a/drivers/watchdog/aspeed_wdt.c >>> +++ b/drivers/watchdog/aspeed_wdt.c >>> @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd, >>> u32 actual = pretimeout * WDT_RATE_1MHZ; >>> u32 s = wdt->cfg->irq_shift; >>> u32 m = wdt->cfg->irq_mask; >>> + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); >>> + >> >> It is unusual to use a register value here and not the configured timeout >> value. I would have assumed that pretimeout is compared against wdt->timout, >> not against the register value, and that the multiplication with WDT_RATE_1MHZ >> is done after validation. This needs an explanation. > > +1 > >> >>> + if (actual >= reload) >>> + return -EINVAL; >>> + >> >> On top of that, you'll also need to explain why watchdog_pretimeout_invalid() >> and with it the validation in watchdog_set_pretimeout() does not work for this >> watchdog and why this extra validation is necessary. > > +1 as well. > > Further, the logic looks broken regardless for the AST2400 where > there's no pretimeout support. aspeed_wdt_set_pretimeout() should error > out if wdt->cfg->irq_mask is 0. > It should not register as supporting pretimeout in the first place. Guenter
Hi Guenter, Thanks for your comments. On 2025/2/18 13:33, Guenter Roeck wrote: > On 2/17/25 19:16, Heyi Guo wrote: >> Aspeed watchdog uses counting down logic, so the value set to register >> should be the value of subtracting pretimeout from total timeout. >> >> Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") >> >> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> >> >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: Joel Stanley <joel@jms.id.au> >> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> >> Cc: Eddie James <eajames@linux.ibm.com> >> --- >> drivers/watchdog/aspeed_wdt.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/watchdog/aspeed_wdt.c >> b/drivers/watchdog/aspeed_wdt.c >> index b4773a6aaf8c..520d8aba12a5 100644 >> --- a/drivers/watchdog/aspeed_wdt.c >> +++ b/drivers/watchdog/aspeed_wdt.c >> @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct >> watchdog_device *wdd, >> u32 actual = pretimeout * WDT_RATE_1MHZ; >> u32 s = wdt->cfg->irq_shift; >> u32 m = wdt->cfg->irq_mask; >> + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); >> + > > It is unusual to use a register value here and not the configured timeout > value. I would have assumed that pretimeout is compared against > wdt->timout, > not against the register value, and that the multiplication with > WDT_RATE_1MHZ > is done after validation. This needs an explanation. It was supposed to be a straight-forward way to check if the pretimeout value is supported by the hardware. I can change to wdt->timeout if it is better. Further, in the case of wdt->timeout > max_hw_heartbeat_ms, shall we restrict the pretimeout to be larger than wdt->timeout - max_hw_heartbeat_ms / 2? For the watchdog_kworker works in max_hw_heartbeat_ms / 2 interval, pretimeout event may be triggered unexpected when watchdog is not pinged in (max_hw_heartbeat_ms - (timeout - pretimeout)). > >> + if (actual >= reload) >> + return -EINVAL; >> + > > On top of that, you'll also need to explain why > watchdog_pretimeout_invalid() > and with it the validation in watchdog_set_pretimeout() does not work > for this > watchdog and why this extra validation is necessary. watchdog_pretimeout_invalid() will return false if wdt->timeout == 0, but we can't determine the hardware pretimeout value if timeout == 0 here. Thanks, Heyi > > Guenter > >> + /* watchdog timer is counting down */ >> + actual = reload - actual; >> wdd->pretimeout = pretimeout; >> wdt->ctrl &= ~m;
Really sorry for this mail and previous noisy mails; my thunderbird client must be misconfigured to send out mail for command + s shortcut when I wanted to save draft... Heyi On 2025/2/19 11:40, Heyi Guo wrote: > Hi Guenter, > > Thanks for your comments. > > On 2025/2/18 13:33, Guenter Roeck wrote: > > On 2/17/25 19:16, Heyi Guo wrote: > >> Aspeed watchdog uses counting down logic, so the value set to register > >> should be the value of subtracting pretimeout from total timeout. > >> > >> Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") > >> > >> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> > >> > >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > >> Cc: Guenter Roeck <linux@roeck-us.net> > >> Cc: Joel Stanley <joel@jms.id.au> > >> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> > >> Cc: Eddie James <eajames@linux.ibm.com> > >> --- > >> drivers/watchdog/aspeed_wdt.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > >> index b4773a6aaf8c..520d8aba12a5 100644 > >> --- a/drivers/watchdog/aspeed_wdt.c > >> +++ b/drivers/watchdog/aspeed_wdt.c > >> @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct > >> watchdog_device *wdd, > >> u32 actual = pretimeout * WDT_RATE_1MHZ; > >> u32 s = wdt->cfg->irq_shift; > >> u32 m = wdt->cfg->irq_mask; > >> + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); > >> + > > > > It is unusual to use a register value here and not the configured timeout > > value. I would have assumed that pretimeout is compared against wdt->timout, > > not against the register value, and that the multiplication with WDT_RATE_1MHZ > > is done after validation. This needs an explanation. > It was supposed to be a straight-forward way to check if the pretimeout value is > supported by the hardware. I can change to wdt->timeout if it is better. > > Further, in the case of wdt->timeout > max_hw_heartbeat_ms, shall we restrict > the pretimeout to be larger than wdt->timeout - max_hw_heartbeat_ms / 2? For > the watchdog_kworker works in max_hw_heartbeat_ms / 2 interval, pretimeout > event may be triggered unexpected when watchdog is not pinged in > (max_hw_heartbeat_ms - (timeout - pretimeout)). > > > > >> + if (actual >= reload) > >> + return -EINVAL; > >> + > > > > On top of that, you'll also need to explain why watchdog_pretimeout_invalid() > > and with it the validation in watchdog_set_pretimeout() does not work for this > > watchdog and why this extra validation is necessary. > watchdog_pretimeout_invalid() will return false if wdt->timeout == 0, but we > can't determine the hardware pretimeout value if timeout == 0 here. > > > > Guenter > > > >> + /* watchdog timer is counting down */ > >> + actual = reload - actual; > >> wdd->pretimeout = pretimeout; > >> wdt->ctrl &= ~m;
On 2/18/25 19:41, Heyi Guo wrote: > Hi Guenter, > > Thanks for your comments. > > On 2025/2/18 13:33, Guenter Roeck wrote: >> On 2/17/25 19:16, Heyi Guo wrote: >>> Aspeed watchdog uses counting down logic, so the value set to register >>> should be the value of subtracting pretimeout from total timeout. >>> >>> Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") >>> >>> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> >>> >>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: Joel Stanley <joel@jms.id.au> >>> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> >>> Cc: Eddie James <eajames@linux.ibm.com> >>> --- >>> drivers/watchdog/aspeed_wdt.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c >>> index b4773a6aaf8c..520d8aba12a5 100644 >>> --- a/drivers/watchdog/aspeed_wdt.c >>> +++ b/drivers/watchdog/aspeed_wdt.c >>> @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd, >>> u32 actual = pretimeout * WDT_RATE_1MHZ; >>> u32 s = wdt->cfg->irq_shift; >>> u32 m = wdt->cfg->irq_mask; >>> + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); >>> + >> >> It is unusual to use a register value here and not the configured timeout >> value. I would have assumed that pretimeout is compared against wdt->timout, >> not against the register value, and that the multiplication with WDT_RATE_1MHZ >> is done after validation. This needs an explanation. > It was supposed to be a straight-forward way to check if the pretimeout value is supported by the hardware. I can change to wdt->timeout if it is better. > > Further, in the case of wdt->timeout > max_hw_heartbeat_ms, shall we restrict the pretimeout to be larger than wdt->timeout - max_hw_heartbeat_ms / 2? For the watchdog_kworker works in max_hw_heartbeat_ms / 2 interval, pretimeout event may be triggered unexpected when watchdog is not pinged in (max_hw_heartbeat_ms - (timeout - pretimeout)). > The kernel internal logic should handle that. If not, it needs to be modified/fixed. >> >>> + if (actual >= reload) >>> + return -EINVAL; >>> + >> >> On top of that, you'll also need to explain why watchdog_pretimeout_invalid() >> and with it the validation in watchdog_set_pretimeout() does not work for this >> watchdog and why this extra validation is necessary. > > watchdog_pretimeout_invalid() will return false if wdt->timeout == 0, but we can't determine the hardware pretimeout value if timeout == 0 here. > Sorry, I don't understand what you mean. If watchdog_pretimeout_invalid() returns false if timeout == 0, aspeed_wdt_set_pretimeout() won't be called. Why does that preclude depending on it ? On a side note, I do wonder why the driver accepts a timeout value of 0 seconds. Guenter
On 2025/2/19 14:07, Guenter Roeck wrote: > On 2/18/25 19:41, Heyi Guo wrote: >> Hi Guenter, >> >> Thanks for your comments. >> >> On 2025/2/18 13:33, Guenter Roeck wrote: >>> On 2/17/25 19:16, Heyi Guo wrote: >>>> Aspeed watchdog uses counting down logic, so the value set to register >>>> should be the value of subtracting pretimeout from total timeout. >>>> >>>> Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") >>>> >>>> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> >>>> >>>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>>> Cc: Guenter Roeck <linux@roeck-us.net> >>>> Cc: Joel Stanley <joel@jms.id.au> >>>> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> >>>> Cc: Eddie James <eajames@linux.ibm.com> >>>> --- >>>> drivers/watchdog/aspeed_wdt.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/watchdog/aspeed_wdt.c >>>> b/drivers/watchdog/aspeed_wdt.c >>>> index b4773a6aaf8c..520d8aba12a5 100644 >>>> --- a/drivers/watchdog/aspeed_wdt.c >>>> +++ b/drivers/watchdog/aspeed_wdt.c >>>> @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct >>>> watchdog_device *wdd, >>>> u32 actual = pretimeout * WDT_RATE_1MHZ; >>>> u32 s = wdt->cfg->irq_shift; >>>> u32 m = wdt->cfg->irq_mask; >>>> + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); >>>> + >>> >>> It is unusual to use a register value here and not the configured >>> timeout >>> value. I would have assumed that pretimeout is compared against >>> wdt->timout, >>> not against the register value, and that the multiplication with >>> WDT_RATE_1MHZ >>> is done after validation. This needs an explanation. >> It was supposed to be a straight-forward way to check if the >> pretimeout value is supported by the hardware. I can change to >> wdt->timeout if it is better. >> >> Further, in the case of wdt->timeout > max_hw_heartbeat_ms, shall we >> restrict the pretimeout to be larger than wdt->timeout - >> max_hw_heartbeat_ms / 2? For the watchdog_kworker works in >> max_hw_heartbeat_ms / 2 interval, pretimeout event may be triggered >> unexpected when watchdog is not pinged in (max_hw_heartbeat_ms - >> (timeout - pretimeout)). >> > > The kernel internal logic should handle that. If not, it needs to be > modified/fixed. Do you mean the watchdog core should also handle the case in which pretimeout < wdt->timeout - max_hw_heartbeat_ms / 2? > >>> >>>> + if (actual >= reload) >>>> + return -EINVAL; >>>> + >>> >>> On top of that, you'll also need to explain why >>> watchdog_pretimeout_invalid() >>> and with it the validation in watchdog_set_pretimeout() does not >>> work for this >>> watchdog and why this extra validation is necessary. >> >> watchdog_pretimeout_invalid() will return false if wdt->timeout == 0, >> but we can't determine the hardware pretimeout value if timeout == 0 >> here. >> > > Sorry, I don't understand what you mean. If watchdog_pretimeout_invalid() > returns false if timeout == 0, aspeed_wdt_set_pretimeout() won't be > called. > Why does that preclude depending on it ? if timeout == 0, watchdog_pretimeout_invalid() returns false, so the code will go on to call wdd->ops->set_pretimeout(). static int watchdog_set_pretimeout(struct watchdog_device *wdd, unsigned int timeout) { int err = 0; if (!watchdog_have_pretimeout(wdd)) return -EOPNOTSUPP; if (watchdog_pretimeout_invalid(wdd, timeout)) return -EINVAL; if (wdd->ops->set_pretimeout && (wdd->info->options & WDIOF_PRETIMEOUT)) err = wdd->ops->set_pretimeout(wdd, timeout); > > On a side note, I do wonder why the driver accepts a timeout value of > 0 seconds. From the code, it seems timeout == 0 / pretimeout == 0 will be considered as a turn off switch. Thanks, Heyi > > Guenter
diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..520d8aba12a5 100644 --- a/drivers/watchdog/aspeed_wdt.c +++ b/drivers/watchdog/aspeed_wdt.c @@ -187,6 +187,13 @@ static int aspeed_wdt_set_pretimeout(struct watchdog_device *wdd, u32 actual = pretimeout * WDT_RATE_1MHZ; u32 s = wdt->cfg->irq_shift; u32 m = wdt->cfg->irq_mask; + u32 reload = readl(wdt->base + WDT_RELOAD_VALUE); + + if (actual >= reload) + return -EINVAL; + + /* watchdog timer is counting down */ + actual = reload - actual; wdd->pretimeout = pretimeout; wdt->ctrl &= ~m;
Aspeed watchdog uses counting down logic, so the value set to register should be the value of subtracting pretimeout from total timeout. Fixes: 9ec0b7e06835 ("watchdog: aspeed: Enable pre-timeout interrupt") Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Joel Stanley <joel@jms.id.au> Cc: Andrew Jeffery <andrew@codeconstruct.com.au> Cc: Eddie James <eajames@linux.ibm.com> --- drivers/watchdog/aspeed_wdt.c | 7 +++++++ 1 file changed, 7 insertions(+)