From: Li Hua Qian <huaqian.li@siemens.com>
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 <huaqian.li@siemens.com>
---
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.1On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
>
> 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 <huaqian.li@siemens.com>
> ---
> 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, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> >
> > 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 <huaqian.li@siemens.com>
> > ---
> > 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, huaqian.li@siemens.com wrote:
>>> From: Li Hua Qian <huaqian.li@siemens.com>
>>>
>>> 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 <huaqian.li@siemens.com>
>>> ---
>>> 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, huaqian.li@siemens.com wrote:
> > > > From: Li Hua Qian <huaqian.li@siemens.com>
> > > >
> > > > 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 <huaqian.li@siemens.com>
> > > > ---
> > > > 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
© 2016 - 2026 Red Hat, Inc.