[PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> On 7/11/23 02:17, huaqian.li at siemens.com wrote:
> > From: Li Hua Qian <huaqian.li at 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 at 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);
>