linux-kernel - RE: [RFC PATCH] watchdog: rzv2h_wdt: Add support to retrieve the bootstatus information

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:
 <TY3PR01MB11346774419BA8D51043C762986392@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Sat, 14 Dec 2024 11:32:17 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>, Guenter Roeck
	<linux@...ck-us.net>
CC: Wim Van Sebroeck <wim@...ux-watchdog.org>, Philipp Zabel
	<p.zabel@...gutronix.de>, Geert Uytterhoeven <geert+renesas@...der.be>, Rob
 Herring <robh@...nel.org>, "linux-watchdog@...r.kernel.org"
	<linux-watchdog@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>, Fabrizio Castro
	<fabrizio.castro.jz@...esas.com>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>
Subject: RE: [RFC PATCH] watchdog: rzv2h_wdt: Add support to retrieve the
 bootstatus information

Hi Lad, Prabhakar,

> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@...il.com>
> Sent: 13 December 2024 19:08
> Subject: Re: [RFC PATCH] watchdog: rzv2h_wdt: Add support to retrieve the bootstatus information
> 
> Hi Guenter,
> 
> Thank you for the review.
> 
> On Fri, Dec 13, 2024 at 6:03 PM Guenter Roeck <linux@...ck-us.net> wrote:
> >
> > On 12/13/24 09:44, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > >
> > > On the RZ/V2H(P) SoC we can determine if the current boot is due to
> > > `Power-on-Reset` or due to the `Watchdog`. The information used to
> > > determine this is present on the CPG block.
> > >
> > > The CPG_ERROR_RSTm(m = 2 -8 ) registers are set in response to an
> > > error interrupt causing an reset. CPG_ERROR_RST2[ERROR_RST1] is set
> > > if there was an underflow/overflow on WDT1 causing an error interrupt.
> > >
> > > To fetch this information from CPG block `syscon` is used and
> > > bootstatus field in the watchdog device is updated based on the
> > > CPG_ERROR_RST2[ERROR_RST1] bit. Upon consumig
> > > CPG_ERROR_RST2[ERROR_RST1] bit we are also clearing it.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@...renesas.com>
> > > ---
> > > @Geert, I intend to drop WDT0/2/3 nodes (and related clocks) as HW
> > > manual says WDT1 is for CA55 (I'll first confirm this internally)
> > >
> > > Hi Geert/Rob,
> > >
> > > I havent included a binding changes as part of the RFC as I wanted
> > > to get some initial feedback on the implementation. Currently CPG
> > > block doesnt have the "syscon" this patch has been tested with below
> > > diff to SoC DTSI
> > >
> > > Cheers,
> > > Prabhakar
> > >
> > > Changes to SoC DTSI:
> > > @@ -243,7 +243,7 @@ pinctrl: pinctrl@...10000 {
> > >                  };
> > >
> > >                  cpg: clock-controller@...20000 {
> > > -                       compatible = "renesas,r9a09g057-cpg";
> > > +                       compatible = "renesas,r9a09g057-cpg",
> > > + "syscon";
> > >                          reg = <0 0x10420000 0 0x10000>;
> > >                          clocks = <&audio_extal_clk>, <&rtxin_clk>, <&qextal_clk>;
> > >                          clock-names = "audio_extal", "rtxin",
> > > "qextal"; @@ -455,6 +456,7 @@ wdt1: watchdog@...00000 {
> > >                          clock-names = "pclk", "oscclk";
> > >                          resets = <&cpg 0x76>;
> > >                          power-domains = <&cpg>;
> > > +                       syscon = <&cpg>;
> > >                          status = "disabled";
> > >                  };
> > >
> > > ---
> > >   drivers/watchdog/rzv2h_wdt.c | 27 ++++++++++++++++++++++++++-
> > >   1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/rzv2h_wdt.c
> > > b/drivers/watchdog/rzv2h_wdt.c index 8defd0241213..8e0901bb7d2b
> > > 100644
> > > --- a/drivers/watchdog/rzv2h_wdt.c
> > > +++ b/drivers/watchdog/rzv2h_wdt.c
> > > @@ -4,14 +4,17 @@
> > >    *
> > >    * Copyright (C) 2024 Renesas Electronics Corporation.
> > >    */
> > > +#include <linux/bitfield.h>
> > >   #include <linux/clk.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/io.h>
> > >   #include <linux/kernel.h>
> > > +#include <linux/mfd/syscon.h>
> > >   #include <linux/module.h>
> > >   #include <linux/of.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/pm_runtime.h>
> > > +#include <linux/regmap.h>
> > >   #include <linux/reset.h>
> > >   #include <linux/units.h>
> > >   #include <linux/watchdog.h>
> > > @@ -40,6 +43,10 @@
> > >
> > >   #define WDT_DEFAULT_TIMEOUT 60U
> > >
> > > +#define CPG_ERROR_RST2                       0xb40
> > > +#define CPG_ERROR_RST2_ERR_RST1              BIT(1)
> > > +#define CPG_ERROR_RST2_ERR_RST1_WEN  (BIT(1) << 16)
> >
> > I could understand BIT(17) or BIT(1 + 16) or
> >
> > #define CPG_ERROR_RST2_ERR_RST1_BIT     1
> > #define CPG_ERROR_RST2_ERR_RST1         BIT(CPG_ERROR_RST2_ERR_RST1_BIT)
> > #define CPG_ERROR_RST2_ERR_RST1_WEN     BIT(CPG_ERROR_RST2_ERR_RST1_BIT + 16)
> >
> > but "BIT(1) << 16" really does not add value.
> >
> OK, I will switch to the above mentioned macros.
> 
> > > +
> > >   static bool nowayout = WATCHDOG_NOWAYOUT;
> > >   module_param(nowayout, bool, 0);
> > >   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> > > @@ -135,7 +142,7 @@ static int rzv2h_wdt_stop(struct watchdog_device *wdev)
> > >   }
> > >
> > >   static const struct watchdog_info rzv2h_wdt_ident = {
> > > -     .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> > > +     .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> > > + WDIOF_SETTIMEOUT | WDIOF_CARDRESET,
> > >       .identity = "Renesas RZ/V2H WDT Watchdog",
> > >   };
> > >
> > > @@ -207,12 +214,29 @@ static int rzv2h_wdt_probe(struct platform_device *pdev)
> > >   {
> > >       struct device *dev = &pdev->dev;
> > >       struct rzv2h_wdt_priv *priv;
> > > +     struct regmap *regmap;
> > > +     unsigned int buf;
> >
> > That is a bad variable name since it suggests a buffer, not some
> > register content.
> >
> Agreed I will rename it to reg.
> 
> > >       int ret;
> > >
> > >       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >       if (!priv)
> > >               return -ENOMEM;
> > >
> > > +     regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > > +     if (IS_ERR(regmap))
> > > +             return PTR_ERR(regmap);
> > > +
> >
> > That is a change in behavior. Up to now the syscon phandle did not
> > have to exist for the driver to work. Is it guaranteed to not result
> > in regressions on systems where it doesn't ? Also, is this documented ? I don't seem to be able to
> find it.
> >
> Agreed. I will add a fallback mechanism to handle cases where the syscon property is not present in
> the WDT node. This will ensure no regressions occur, and the bootstatus will simply be set to 0 in
> such scenarios. As mentioned in the patch comments, I have not yet submitted the DT binding changes
> because I wanted feedback on the syscon approach. The new RZ SoCs have registers scattered across
> various locations, and I was exploring if there might be a better way to handle this.

See, syscon compatible not needed with [1]

[1]
https://lore.kernel.org/all/20241211-syscon-fixes-v1-3-b5ac8c219e96@kernel.org/

Cheers,
Biju