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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <45d4760e-17bf-49f2-a139-d79a0202b630@roeck-us.net>
Date: Tue, 6 Aug 2024 07:03:17 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
 Geert Uytterhoeven <geert+renesas@...der.be>,
 Magnus Damm <magnus.damm@...il.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 Biju Das <biju.das.jz@...renesas.com>,
 Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 2/2] watchdog: Add Watchdog Timer driver for RZ/V2H(P)

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