Re: [PATCH v3 2/2] watchdog: Add Watchdog Timer driver for RZ/V2H(P)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

...
+     /*
+      * 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.

...

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.

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

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


+     /*
+      * 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().

Thanks,
Guenter





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux