[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+V-a8sn0R3Vd7JFzitSKm24KzKUNdMf9SkFHn567X6y6nA-eg@mail.gmail.com>
Date: Tue, 6 Aug 2024 16:48:02 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
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)
Hi Guenter,
On Tue, Aug 6, 2024 at 3:03 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> 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.
>
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
*/
> ...
>
> > 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.
*/
> > 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().
>
OK
Cheers,
Prabhakar
Powered by blists - more mailing lists