Hi Guenter,

On Tue, Aug 6, 2024 at 3:03 PM Guenter Roeck [off-list ref] wrote:

On 8/6/24 06:47, Lad, Prabhakar wrote:

quoted

Hi Guenter,

...

quoted

quoted

quoted

+     /*
+      * WDTCR
+      * - CKS[7:4] - Clock Division Ratio Select - 0101b: oscclk/256
+      * - RPSS[13:12] - Window Start Position Select - 11b: 100%
+      * - RPES[9:8] - Window End Position Select - 11b: 0%
+      * - TOPS[1:0] - Timeout Period Select - 11b: 16384 cycles (3FFFh)
+      */
+     rzv2h_wdt_setup(wdev, WDTCR_CKS_CLK_256 | WDTCR_RPSS_100 |
+                     WDTCR_RPES_0 | WDTCR_TOPS_16384);
+
+     rzv2h_wdt_ping(wdev);
+

The need to ping the watchdog immediately after enabling it is unusual.
Please explain.

The down counting operation starts only after the ping operation, so
after starting the wdt a ping is issued here.

Please add that as comment to the code.

Sure, I will add the below comment:

    /*
     * Down counting starts after writing the sequence 00h -> FFh to the
     * WDTRR register. Hence, call the ping operation after loading the counter
     */

...

quoted

Ive now updated restart with below, so that we dont touch clocks if
they are already ON,

     if (!watchdog_active(wdev)) {
         ret = clk_enable(priv->pclk);
         if (ret)
             return ret;

         ret = clk_enable(priv->oscclk);
         if (ret) {
             clk_disable(priv->pclk);
             return ret;
         }
     }

     if (!watchdog_active(wdev))
         ret = reset_control_deassert(priv->rstc);
     else
         ret = reset_control_reset(priv->rstc);

Please rearrange to only require a single "if (!watchdog_active())".
Also, please add a comment explaining the need for calling
reset_control_reset() if the watchdog is active.

Sure I will rearrange the code and add the below comment on why reset
operation is required when wdt is active,

        /*
         * Writing to the WDT Control Register (WDTCR) or WDT Reset
         * Control Register (WDTRCR) is possible once between the
         * release from the reset state and the first refresh operation.
         * so issue a reset if watchdog is active.
         * Therefore, issue a reset if the watchdog is active.
         */

quoted

     if (ret) {
         clk_disable(priv->oscclk);
         clk_disable(priv->pclk);
         return ret;
     }

     /* delay to handle clock halt after de-assert operation */
     udelay(3);

quoted

quoted

+     /*
+      * WDTCR
+      * - CKS[7:4] - Clock Division Ratio Select - 0000b: oscclk/1
+      * - RPSS[13:12] - Window Start Position Select - 00b: 25%
+      * - RPES[9:8] - Window End Position Select - 00b: 75%
+      * - TOPS[1:0] - Timeout Period Select - 00b: 1024 cycles (03FFh)
+      */
+     rzv2h_wdt_setup(wdev, WDTCR_CKS_CLK_1 | WDTCR_RPSS_25 |
+                     WDTCR_RPES_75 | WDTCR_TOPS_1024);
+     rzv2h_wdt_ping(wdev);
+

Why is the ping here necessary ?

The down counting starts after the refresh operation, hence the WDT is pinged.

Should be covered with the explanation in rzv2h_wdt_start().

OK

Cheers,
Prabhakar