Hi Lad, Prabhakar,
> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> 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@xxxxxxxxxxxx> wrote:
> >
> > On 12/13/24 09:44, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> > > ---
> > > @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@10410000 {
> > > };
> > >
> > > cpg: clock-controller@10420000 {
> > > - 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@14400000 {
> > > 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@xxxxxxxxxx/
Cheers,
Biju
