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.
> > + ret = regmap_read(regmap, CPG_ERROR_RST2, &buf);
> > + if (ret)
> > + return -EINVAL;
>
> Pass error codes back to caller. This is most definitely not an
> "Invalid argument".
>
OK.
> "
> > +
> > + if (buf & CPG_ERROR_RST2_ERR_RST1) {
> > + ret = regmap_write(regmap, CPG_ERROR_RST2,
> > + CPG_ERROR_RST2_ERR_RST1_WEN | CPG_ERROR_RST2_ERR_RST1);
> > + if (ret)
> > + return -EINVAL;
>
> Same as above.
>
OK.
Cheers,
Prabhakar
