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

["Li, Hua Qian" <HuaQian.Li@siemens.com>]

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