From: Li Hua Qian <[email protected]>
The watchdog hardware of TI AM65X platform does not support
WDIOF_CARDRESET feature, add a reserved memory to save the watchdog
reset cause, to know if the board reboot is due to a watchdog reset.
Li Hua Qian (3):
dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET
arm64: dts: ti: Add reserved memory for watchdog
watchdog:rit_wdt: Add support for WDIOF_CARDRESET
.../bindings/watchdog/ti,rti-wdt.yaml | 13 ++++-
.../boot/dts/ti/k3-am65-iot2050-common.dtsi | 11 +++++
drivers/watchdog/rti_wdt.c | 48 +++++++++++++++++++
3 files changed, 71 insertions(+), 1 deletion(-)
--
2.34.1
From: Li Hua Qian <[email protected]>
This patch adds the WDIOF_CARDRESET support for the platform watchdog
whose hardware does not support this feature, to know if the board
reboot is due to a watchdog reset.
This is done via reserved memory(RAM), which indicates if specific
info saved, triggering the watchdog reset in last boot.
Signed-off-by: Li Hua Qian <[email protected]>
---
drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index ce8f18e93aa9..77fd6b54137c 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -18,6 +18,7 @@
#include <linux/pm_runtime.h>
#include <linux/types.h>
#include <linux/watchdog.h>
+#include <linux/of.h>
#define DEFAULT_HEARTBEAT 60
@@ -52,6 +53,11 @@
#define DWDST BIT(1)
+#define PON_REASON_SOF_NUM 0xBBBBCCCC
+#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
+#define PON_REASON_EOF_NUM 0xCCCCBBBB
+#define PON_REASON_ITEM_BITS 0xFFFFFFFF
+
static int heartbeat = DEFAULT_HEARTBEAT;
/*
@@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
struct rti_wdt_device *wdt;
struct clk *clk;
u32 last_ping = 0;
+ u32 reserved_mem_size;
+ unsigned long *vaddr;
+ unsigned long paddr;
+ u32 data[3];
+ u32 reg[8];
wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
@@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
}
}
+ ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
+ 0, ARRAY_SIZE(reg));
+ if (ret < 0) {
+ dev_err(dev, "cannot read the reg info.\n");
+ goto err_iomap;
+ }
+
+ /*
+ * If reserved memory is defined for watchdog reset cause.
+ * Readout the Power-on(PON) reason and pass to bootstatus.
+ */
+ if (ret == 8) {
+ paddr = reg[5];
+ reserved_mem_size = reg[7];
+ vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
+ if (vaddr == NULL) {
+ dev_err(dev, "Failed to map memory-region.\n");
+ goto err_iomap;
+ }
+
+ data[0] = *vaddr & PON_REASON_ITEM_BITS;
+ data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
+ data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
+
+ dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
+ data[0], data[1], data[2]);
+
+ if ((data[0] == PON_REASON_SOF_NUM)
+ && (data[1] == PON_REASON_MAGIC_NUM)
+ && (data[1] == PON_REASON_MAGIC_NUM)) {
+ dev_info(dev, "Watchdog reset cause detected.\n");
+ wdd->bootstatus |= WDIOF_CARDRESET;
+ }
+ memset(vaddr, 0, reserved_mem_size);
+ memunmap(vaddr);
+ }
+
watchdog_init_timeout(wdd, heartbeat, dev);
ret = watchdog_register_device(wdd);
--
2.34.1
From: Li Hua Qian <[email protected]>
TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
watchdog cause. Add a reserved memory to know the last reboot was caused
by the watchdog card. In the reserved memory, some specific info will be
saved to indicate whether the watchdog reset was triggered in last
boot.
Signed-off-by: Li Hua Qian <[email protected]>
---
.../devicetree/bindings/watchdog/ti,rti-wdt.yaml | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
index fc553211e42d..f227db08dc70 100644
--- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -26,7 +26,18 @@ properties:
- ti,j7-rti-wdt
reg:
- maxItems: 1
+ maxItems: 2
+ description:
+ - Contains the address and the size of MCU RTI register.
+ - Contains the address and the size of reserved memory, which
+ has the pre-stored watchdog reset cause as power-on reason. The
+ second item is optional.
+ In the reserved memory, the following values are needed at the
+ first 12 bytes to tell that last boot was caused by watchdog
+ reset.
+ - PON_REASON_SOF_NUM: 0xBBBBCCCC
+ - PON_REASON_MAGIC_NUM: 0xDDDDDDDD
+ - PON_REASON_EOF_NUM: 0xCCCCBBBB
clocks:
maxItems: 1
--
2.34.1
On Tue, 11 Jul 2023 17:17:11 +0800, [email protected] wrote:
> From: Li Hua Qian <[email protected]>
>
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
>
> Signed-off-by: Li Hua Qian <[email protected]>
> ---
> .../devicetree/bindings/watchdog/ti,rti-wdt.yaml | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: [error] syntax error: mapping values are not allowed here (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Tue, 2023-07-11 at 04:13 -0600, Rob Herring wrote:
>
> On Tue, 11 Jul 2023 17:17:11 +0800, [email protected] wrote:
> > From: Li Hua Qian <[email protected]>
> >
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> >
> > Signed-off-by: Li Hua Qian <[email protected]>
> > ---
> > .../devicetree/bindings/watchdog/ti,rti-wdt.yaml | 13
> > ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> [error] syntax error: mapping values are not allowed here (syntax)
>
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file
> 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26:
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts]
> Error 1
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:
> ignoring, error parsing file
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500:
> dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
> doc reference errors (make refcheckdocs):
>
> See
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
>
Thanks for the test and your advice, I will make sure this is no
problem in the next version.
On 7/11/23 02:17, [email protected] wrote:
> From: Li Hua Qian <[email protected]>
>
> This patch adds the WDIOF_CARDRESET support for the platform watchdog
> whose hardware does not support this feature, to know if the board
> reboot is due to a watchdog reset.
>
> This is done via reserved memory(RAM), which indicates if specific
> info saved, triggering the watchdog reset in last boot.
>
> Signed-off-by: Li Hua Qian <[email protected]>
> ---
> drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index ce8f18e93aa9..77fd6b54137c 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -18,6 +18,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> +#include <linux/of.h>
>
> #define DEFAULT_HEARTBEAT 60
>
> @@ -52,6 +53,11 @@
>
> #define DWDST BIT(1)
>
> +#define PON_REASON_SOF_NUM 0xBBBBCCCC
> +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
> +#define PON_REASON_EOF_NUM 0xCCCCBBBB
> +#define PON_REASON_ITEM_BITS 0xFFFFFFFF
> +
> static int heartbeat = DEFAULT_HEARTBEAT;
>
> /*
> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
> struct rti_wdt_device *wdt;
> struct clk *clk;
> u32 last_ping = 0;
> + u32 reserved_mem_size;
> + unsigned long *vaddr;
> + unsigned long paddr;
> + u32 data[3];
> + u32 reg[8];
>
> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> if (!wdt)
> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
> }
> }
>
> + ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
> + 0, ARRAY_SIZE(reg));
> + if (ret < 0) {
> + dev_err(dev, "cannot read the reg info.\n");
> + goto err_iomap;
> + }
This aborts if the property does not exist, which is unacceptable.
Any such addition must be optional.
> +
> + /*
> + * If reserved memory is defined for watchdog reset cause.
> + * Readout the Power-on(PON) reason and pass to bootstatus.
> + */
> + if (ret == 8) {
> + paddr = reg[5];
> + reserved_mem_size = reg[7];
It seems odd that reserved_mem_size is not checked, and that it is even provided
given that it needs to be (at least) 24 bytes, and any other value does not really
make sense.
> + vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
> + if (vaddr == NULL) {
> + dev_err(dev, "Failed to map memory-region.\n");
> + goto err_iomap;
This returns 8, which would be an odd error return.
> + }
> +
> + data[0] = *vaddr & PON_REASON_ITEM_BITS;
> + data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> + data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> +
The & seems pointless / wasteful. Why ignore the upper 32 bits of each location ?
Either make it u32 or make it u64 and use the entire 64 bit. Besides,
vaddr[0..2] would make the code much easier to read.
> + dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
> + data[0], data[1], data[2]);
> +
> + if ((data[0] == PON_REASON_SOF_NUM)
> + && (data[1] == PON_REASON_MAGIC_NUM)
> + && (data[1] == PON_REASON_MAGIC_NUM)) {
Unnecessary inner (), and I don't see the point of checking data[1] twice.
> + dev_info(dev, "Watchdog reset cause detected.\n");
Unnecessary noise.
> + wdd->bootstatus |= WDIOF_CARDRESET;
> + }
> + memset(vaddr, 0, reserved_mem_size);
> + memunmap(vaddr);
> + }
And some random data in the property is acceptable ? That is odd, especially
after mandating the property itself.
> +
> watchdog_init_timeout(wdd, heartbeat, dev);
>
> ret = watchdog_register_device(wdd);
On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> On 7/11/23 02:17, [email protected] wrote:
> > From: Li Hua Qian <[email protected]>
> >
> > This patch adds the WDIOF_CARDRESET support for the platform
> > watchdog
> > whose hardware does not support this feature, to know if the board
> > reboot is due to a watchdog reset.
> >
> > This is done via reserved memory(RAM), which indicates if specific
> > info saved, triggering the watchdog reset in last boot.
> >
> > Signed-off-by: Li Hua Qian <[email protected]>
> > ---
> > drivers/watchdog/rti_wdt.c | 48
> > ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/watchdog/rti_wdt.c
> > b/drivers/watchdog/rti_wdt.c
> > index ce8f18e93aa9..77fd6b54137c 100644
> > --- a/drivers/watchdog/rti_wdt.c
> > +++ b/drivers/watchdog/rti_wdt.c
> > @@ -18,6 +18,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > +#include <linux/of.h>
> >
> > #define DEFAULT_HEARTBEAT 60
> >
> > @@ -52,6 +53,11 @@
> >
> > #define DWDST BIT(1)
> >
> > +#define PON_REASON_SOF_NUM 0xBBBBCCCC
> > +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
> > +#define PON_REASON_EOF_NUM 0xCCCCBBBB
> > +#define PON_REASON_ITEM_BITS 0xFFFFFFFF
> > +
> > static int heartbeat = DEFAULT_HEARTBEAT;
> >
> > /*
> > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> > struct rti_wdt_device *wdt;
> > struct clk *clk;
> > u32 last_ping = 0;
> > + u32 reserved_mem_size;
> > + unsigned long *vaddr;
> > + unsigned long paddr;
> > + u32 data[3];
> > + u32 reg[8];
> >
> > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> > if (!wdt)
> > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> > }
> > }
> >
> > + ret = of_property_read_variable_u32_array(pdev-
> > >dev.of_node, "reg", reg,
> > + 0, ARRAY_SIZE(reg));
> > + if (ret < 0) {
> > + dev_err(dev, "cannot read the reg info.\n");
> > + goto err_iomap;
> > + }
>
> This aborts if the property does not exist, which is unacceptable.
> Any such addition must be optional.
Agree, refactor it.
>
> > +
> > + /*
> > + * If reserved memory is defined for watchdog reset cause.
> > + * Readout the Power-on(PON) reason and pass to bootstatus.
> > + */
> > + if (ret == 8) {
> > + paddr = reg[5];
> > + reserved_mem_size = reg[7];
>
> It seems odd that reserved_mem_size is not checked,
ACK
> and that it is even provided
> given that it needs to be (at least) 24 bytes, and any other value
> does not really
> make sense.
>
I was thinkg to add the reliability, but it seems to be unnecessary and
pointless. Were you suggesting that 8 bytes are enough?
> > + vaddr = memremap(paddr, reserved_mem_size,
> > MEMREMAP_WB);
> > + if (vaddr == NULL) {
> > + dev_err(dev, "Failed to map memory-
> > region.\n");
> > + goto err_iomap;
>
> This returns 8, which would be an odd error return.
>
ACK,refactor it.
> > + }
> > +
> > + data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > + data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > + data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > +
>
> The & seems pointless / wasteful. Why ignore the upper 32 bits of
> each location ?
> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
> vaddr[0..2] would make the code much easier to read.
>
ACK, refactor it.
> > + dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
> > = %lX\n",
> > + data[0], data[1], data[2]);
> > +
> > + if ((data[0] == PON_REASON_SOF_NUM)
> > + && (data[1] == PON_REASON_MAGIC_NUM)
> > + && (data[1] == PON_REASON_MAGIC_NUM)) {
>
> Unnecessary inner (), and I don't see the point of checking data[1]
> twice.
Yeah, a typo happened.
>
> > + dev_info(dev, "Watchdog reset cause
> > detected.\n");
>
> Unnecessary noise.
ACK,rename dev_info to dev_dbg.
>
> > + wdd->bootstatus |= WDIOF_CARDRESET;
> > + }
> > + memset(vaddr, 0, reserved_mem_size);
> > + memunmap(vaddr);
> > + }
>
> And some random data in the property is acceptable ? That is odd,
> especially
> after mandating the property itself.
>
Yeah, do you have any suggestions about how to store the watchdog
reset cause?
Best regards,
Li Hua Qian
> > +
> > watchdog_init_timeout(wdd, heartbeat, dev);
> >
> > ret = watchdog_register_device(wdd);
>
On 7/11/23 21:03, Li, Hua Qian wrote:
> On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
>> On 7/11/23 02:17, [email protected] wrote:
>>> From: Li Hua Qian <[email protected]>
>>>
>>> This patch adds the WDIOF_CARDRESET support for the platform
>>> watchdog
>>> whose hardware does not support this feature, to know if the board
>>> reboot is due to a watchdog reset.
>>>
>>> This is done via reserved memory(RAM), which indicates if specific
>>> info saved, triggering the watchdog reset in last boot.
>>>
>>> Signed-off-by: Li Hua Qian <[email protected]>
>>> ---
>>> drivers/watchdog/rti_wdt.c | 48
>>> ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c
>>> b/drivers/watchdog/rti_wdt.c
>>> index ce8f18e93aa9..77fd6b54137c 100644
>>> --- a/drivers/watchdog/rti_wdt.c
>>> +++ b/drivers/watchdog/rti_wdt.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/pm_runtime.h>
>>> #include <linux/types.h>
>>> #include <linux/watchdog.h>
>>> +#include <linux/of.h>
>>>
>>> #define DEFAULT_HEARTBEAT 60
>>>
>>> @@ -52,6 +53,11 @@
>>>
>>> #define DWDST BIT(1)
>>>
>>> +#define PON_REASON_SOF_NUM 0xBBBBCCCC
>>> +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
>>> +#define PON_REASON_EOF_NUM 0xCCCCBBBB
>>> +#define PON_REASON_ITEM_BITS 0xFFFFFFFF
>>> +
>>> static int heartbeat = DEFAULT_HEARTBEAT;
>>>
>>> /*
>>> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>> struct rti_wdt_device *wdt;
>>> struct clk *clk;
>>> u32 last_ping = 0;
>>> + u32 reserved_mem_size;
>>> + unsigned long *vaddr;
>>> + unsigned long paddr;
>>> + u32 data[3];
>>> + u32 reg[8];
>>>
>>> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>> if (!wdt)
>>> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>> }
>>> }
>>>
>>> + ret = of_property_read_variable_u32_array(pdev-
>>>> dev.of_node, "reg", reg,
>>> + 0, ARRAY_SIZE(reg));
>>> + if (ret < 0) {
>>> + dev_err(dev, "cannot read the reg info.\n");
>>> + goto err_iomap;
>>> + }
>>
>> This aborts if the property does not exist, which is unacceptable.
>> Any such addition must be optional.
> Agree, refactor it.
>>
>>> +
>>> + /*
>>> + * If reserved memory is defined for watchdog reset cause.
>>> + * Readout the Power-on(PON) reason and pass to bootstatus.
>>> + */
>>> + if (ret == 8) {
>>> + paddr = reg[5];
>>> + reserved_mem_size = reg[7];
>>
>> It seems odd that reserved_mem_size is not checked,
> ACK
>> and that it is even provided
>> given that it needs to be (at least) 24 bytes, and any other value
>> does not really
>> make sense.
>>
> I was thinkg to add the reliability, but it seems to be unnecessary and
> pointless. Were you suggesting that 8 bytes are enough?
>
No.
>>> MEMREMAP_WB);
>>> + if (vaddr == NULL) {
>>> + dev_err(dev, "Failed to map memory-
>>> region.\n");
>>> + goto err_iomap;
>>
>> This returns 8, which would be an odd error return.
>>
> ACK,refactor it.
>>> + }
>>> +
>>> + data[0] = *vaddr & PON_REASON_ITEM_BITS;
>>> + data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
>>> + data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
>>> +
>>
>> The & seems pointless / wasteful. Why ignore the upper 32 bits of
>> each location ?
>> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
>> vaddr[0..2] would make the code much easier to read.
>>
> ACK, refactor it.
>>> + dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
>>> = %lX\n",
>>> + data[0], data[1], data[2]);
>>> +
>>> + if ((data[0] == PON_REASON_SOF_NUM)
>>> + && (data[1] == PON_REASON_MAGIC_NUM)
>>> + && (data[1] == PON_REASON_MAGIC_NUM)) {
>>
>> Unnecessary inner (), and I don't see the point of checking data[1]
>> twice.
> Yeah, a typo happened.
>>
>>> + dev_info(dev, "Watchdog reset cause
>>> detected.\n");
>>
>> Unnecessary noise.
> ACK,rename dev_info to dev_dbg.
>>
>>> + wdd->bootstatus |= WDIOF_CARDRESET;
>>> + }
>>> + memset(vaddr, 0, reserved_mem_size);
>>> + memunmap(vaddr);
>>> + }
>>
>> And some random data in the property is acceptable ? That is odd,
>> especially
>> after mandating the property itself.
>>
> Yeah, do you have any suggestions about how to store the watchdog
> reset cause?
>
No, and that is not the point I was trying to make. Your code actively
aborts probe if the "reg" property does not exist at all, but then it
silently ignores if it contains a random number of elements (other
than 8). For example, something like
reg = <0x1234 0x5678>
is silently ignored. If anything, _that_ should return -EINVAL
because it is obviously wrong.
On a higher level, the entire code puzzles me. Obviously there
must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
into some memory area. That means the BIOS/ROMMON must be able
to detect that situation. Why not use the same code to detect this
in the driver without the complexity of passing it from BIOS/ROMMON
to driver in some random memory area ?
> Best regards,
> Li Hua Qian
>>> +
>>> watchdog_init_timeout(wdd, heartbeat, dev);
>>>
>>> ret = watchdog_register_device(wdd);
>>
>
On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
> On 7/11/23 21:03, Li, Hua Qian wrote:
> > On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> > > On 7/11/23 02:17, [email protected] wrote:
> > > > From: Li Hua Qian <[email protected]>
> > > >
> > > > This patch adds the WDIOF_CARDRESET support for the platform
> > > > watchdog
> > > > whose hardware does not support this feature, to know if the
> > > > board
> > > > reboot is due to a watchdog reset.
> > > >
> > > > This is done via reserved memory(RAM), which indicates if
> > > > specific
> > > > info saved, triggering the watchdog reset in last boot.
> > > >
> > > > Signed-off-by: Li Hua Qian <[email protected]>
> > > > ---
> > > > drivers/watchdog/rti_wdt.c | 48
> > > > ++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 48 insertions(+)
> > > >
> > > > diff --git a/drivers/watchdog/rti_wdt.c
> > > > b/drivers/watchdog/rti_wdt.c
> > > > index ce8f18e93aa9..77fd6b54137c 100644
> > > > --- a/drivers/watchdog/rti_wdt.c
> > > > +++ b/drivers/watchdog/rti_wdt.c
> > > > @@ -18,6 +18,7 @@
> > > > #include <linux/pm_runtime.h>
> > > > #include <linux/types.h>
> > > > #include <linux/watchdog.h>
> > > > +#include <linux/of.h>
> > > >
> > > > #define DEFAULT_HEARTBEAT 60
> > > >
> > > > @@ -52,6 +53,11 @@
> > > >
> > > > #define DWDST BIT(1)
> > > >
> > > > +#define PON_REASON_SOF_NUM 0xBBBBCCCC
> > > > +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
> > > > +#define PON_REASON_EOF_NUM 0xCCCCBBBB
> > > > +#define PON_REASON_ITEM_BITS 0xFFFFFFFF
> > > > +
> > > > static int heartbeat = DEFAULT_HEARTBEAT;
> > > >
> > > > /*
> > > > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > > struct rti_wdt_device *wdt;
> > > > struct clk *clk;
> > > > u32 last_ping = 0;
> > > > + u32 reserved_mem_size;
> > > > + unsigned long *vaddr;
> > > > + unsigned long paddr;
> > > > + u32 data[3];
> > > > + u32 reg[8];
> > > >
> > > > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> > > > if (!wdt)
> > > > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > > }
> > > > }
> > > >
> > > > + ret = of_property_read_variable_u32_array(pdev-
> > > > > dev.of_node, "reg", reg,
> > > > + 0, ARRAY_SIZE(reg));
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "cannot read the reg info.\n");
> > > > + goto err_iomap;
> > > > + }
> > >
> > > This aborts if the property does not exist, which is
> > > unacceptable.
> > > Any such addition must be optional.
> > Agree, refactor it.
> > >
> > > > +
> > > > + /*
> > > > + * If reserved memory is defined for watchdog reset
> > > > cause.
> > > > + * Readout the Power-on(PON) reason and pass to
> > > > bootstatus.
> > > > + */
> > > > + if (ret == 8) {
> > > > + paddr = reg[5];
> > > > + reserved_mem_size = reg[7];
> > >
> > > It seems odd that reserved_mem_size is not checked,
> > ACK
> > > and that it is even provided
> > > given that it needs to be (at least) 24 bytes, and any other
> > > value
> > > does not really
> > > make sense.
> > >
> > I was thinkg to add the reliability, but it seems to be unnecessary
> > and
> > pointless. Were you suggesting that 8 bytes are enough?
> >
>
> No.
I guess I misunderstood you. Here you was suggesting that
reserved_mem_size should be checked and make sure the value to be at
least 24 bytes. Am I understanding you correctly?
> > > > MEMREMAP_WB);
> > > > + if (vaddr == NULL) {
> > > > + dev_err(dev, "Failed to map memory-
> > > > region.\n");
> > > > + goto err_iomap;
> > >
> > > This returns 8, which would be an odd error return.
> > >
> > ACK,refactor it.
> > > > + }
> > > > +
> > > > + data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > > > + data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > > > + data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > > > +
> > >
> > > The & seems pointless / wasteful. Why ignore the upper 32 bits of
> > > each location ?
> > > Either make it u32 or make it u64 and use the entire 64 bit.
> > > Besides,
> > > vaddr[0..2] would make the code much easier to read.
> > >
> > ACK, refactor it.
> > > > + dev_dbg(dev, "Watchdog: sof = %lX, data = %lX,
> > > > eof
> > > > = %lX\n",
> > > > + data[0], data[1], data[2]);
> > > > +
> > > > + if ((data[0] == PON_REASON_SOF_NUM)
> > > > + && (data[1] == PON_REASON_MAGIC_NUM)
> > > > + && (data[1] == PON_REASON_MAGIC_NUM)) {
> > >
> > > Unnecessary inner (), and I don't see the point of checking
> > > data[1]
> > > twice.
> > Yeah, a typo happened.
> > >
> > > > + dev_info(dev, "Watchdog reset cause
> > > > detected.\n");
> > >
> > > Unnecessary noise.
> > ACK,rename dev_info to dev_dbg.
> > >
> > > > + wdd->bootstatus |= WDIOF_CARDRESET;
> > > > + }
> > > > + memset(vaddr, 0, reserved_mem_size);
> > > > + memunmap(vaddr);
> > > > + }
> > >
> > > And some random data in the property is acceptable ? That is odd,
> > > especially
> > > after mandating the property itself.
> > >
> > Yeah, do you have any suggestions about how to store the watchdog
> > reset cause?
> >
>
> No, and that is not the point I was trying to make. Your code
> actively
> aborts probe if the "reg" property does not exist at all, but then it
> silently ignores if it contains a random number of elements (other
> than 8). For example, something like
> reg = <0x1234 0x5678>
> is silently ignored. If anything, _that_ should return -EINVAL
> because it is obviously wrong.
Now I get it, many thanks for pointing out.
>
> On a higher level, the entire code puzzles me. Obviously there
> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
> into some memory area. That means the BIOS/ROMMON must be able
> to detect that situation. Why not use the same code to detect this
> in the driver without the complexity of passing it from BIOS/ROMMON
> to driver in some random memory area ?
>
In TI AM65x cases, the hardware is not capable of issuing a reset on
its own, and is not possible to record or detect the watchdog reset
situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
to detect the watchdog interrupt and reset the Soc. Here I am trying to
write the reason in this firmware when watchdog interrupt happens, and
read it out in kernel.
> > Best regards,
> > Li Hua Qian
> > > > +
> > > > watchdog_init_timeout(wdd, heartbeat, dev);
> > > >
> > > > ret = watchdog_register_device(wdd);
> > >
> >
>
On 12.07.23 08:05, Li, Hua Qian (DI FA CTR IPC CN PRC4) wrote:
> On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
>> On a higher level, the entire code puzzles me. Obviously there
>> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
>> into some memory area. That means the BIOS/ROMMON must be able
>> to detect that situation. Why not use the same code to detect this
>> in the driver without the complexity of passing it from BIOS/ROMMON
>> to driver in some random memory area ?
>>
> In TI AM65x cases, the hardware is not capable of issuing a reset on
> its own, and is not possible to record or detect the watchdog reset
> situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
> to detect the watchdog interrupt and reset the Soc. Here I am trying to
> write the reason in this firmware when watchdog interrupt happens, and
> read it out in kernel.
See https://github.com/siemens/k3-rti-wdt/issues/1 for where this
started and where the firmware part comes into play.
I still wonder why we couldn't have had a nicer hardware watchdog
instead but this can't be changed anymore.
Jan
--
Siemens AG, Technology
Competence Center Embedded Linux