Re: [PATCH v6 4/8] watchdog: rzv2h: Propagate WDTDCR access errors

[Lad, Prabhakar]

Hi Guenter,

Thank you for the review.

On Wed, Sep 23, 2026 at 10:17 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Wed, Sep 23, 2026 at 11:40:50AM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > The WDTDCR helpers access the register directly using readl()/writel() and
> > therefore cannot report failures to their callers. WDTDCR is located in a
> > shared syscon region and will be accessed through regmap in a subsequent
> > change, where register accesses can fail.
> >
> > Make rzt2h_wdt_wdtdcr_count_start() and rzt2h_wdt_wdtdcr_count_stop()
> > return an error so their callers can propagate failures.
> >
> > Handle these errors in the watchdog start and stop paths and unwind the
> > resources acquired before the WDTDCR access, restoring the reset state.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v5->v6:
> > - No change
> >
> > v4->v5:
> > - Dropped checking rzt2h_wdt_wdtdcr_count_start() return value in
> >   restart path.
> >
> > v2->v3:
> > - New patch, split from v2 patch #1 to make thing easier to review.
> > ---
> >  drivers/watchdog/rzv2h_wdt.c | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> > index 9774fe8e441e..a65e5d6c48ad 100644
> > --- a/drivers/watchdog/rzv2h_wdt.c
> > +++ b/drivers/watchdog/rzv2h_wdt.c
> > @@ -87,18 +87,22 @@ static int rzv2h_wdt_ping(struct watchdog_device *wdev)
> >       return 0;
> >  }
> >
> > -static void rzt2h_wdt_wdtdcr_count_stop(struct rzv2h_wdt_priv *priv)
> > +static int rzt2h_wdt_wdtdcr_count_stop(struct rzv2h_wdt_priv *priv)
> >  {
> >       u32 reg = readl(priv->wdtdcr + WDTDCR);
> >
> >       writel(reg | WDTDCR_WDTSTOPCTRL, priv->wdtdcr + WDTDCR);
> > +
> > +     return 0;
> >  }
> >
> > -static void rzt2h_wdt_wdtdcr_count_start(struct rzv2h_wdt_priv *priv)
> > +static int rzt2h_wdt_wdtdcr_count_start(struct rzv2h_wdt_priv *priv)
> >  {
> >       u32 reg = readl(priv->wdtdcr + WDTDCR);
> >
> >       writel(reg & ~WDTDCR_WDTSTOPCTRL, priv->wdtdcr + WDTDCR);
> > +
> > +     return 0;
> >  }
> >
> >  static void rzv2h_wdt_setup(struct watchdog_device *wdev, u16 wdtcr)
> > @@ -148,8 +152,14 @@ static int rzv2h_wdt_start(struct watchdog_device *wdev)
> >       rzv2h_wdt_setup(wdev, of_data->cks_max | WDTCR_RPSS_100 |
> >                       WDTCR_RPES_0 | of_data->tops);
> >
> > -     if (priv->of_data->wdtdcr)
> > -             rzt2h_wdt_wdtdcr_count_start(priv);
> > +     if (priv->of_data->wdtdcr) {
> > +             ret = rzt2h_wdt_wdtdcr_count_start(priv);
> > +             if (ret) {
> > +                     reset_control_assert(priv->rstc);
>
> It seems to me this does way more than handling the error:
> it puts the module into reset and calls pm_runtime_put()
> on the parent. This is a substantial functional change
> that is not explained.
>
The intention was only to unwind what rzv2h_wdt_start() had already
done before the WDTDCR access (the runtime PM get and the reset
deassert), so that a failure would leave the module in the same state
as before the call. I agree this should have been explained, and that
it adds more churn than the change is worth.

> > +                     pm_runtime_put(wdev->parent);
> > +                     return ret;
> > +             }
> > +     }
> >
> >       /*
> >        * Down counting starts after writing the sequence 00h -> FFh to the
> > @@ -169,8 +179,13 @@ static int rzv2h_wdt_stop(struct watchdog_device *wdev)
> >       if (ret)
> >               return ret;
> >
> > -     if (priv->of_data->wdtdcr)
> > -             rzt2h_wdt_wdtdcr_count_stop(priv);
> > +     if (priv->of_data->wdtdcr) {
> > +             ret = rzt2h_wdt_wdtdcr_count_stop(priv);
> > +             if (ret) {
> > +                     reset_control_deassert(priv->rstc);
>
> Same here, and the logic behind taking the module out of reset
> in the stop function and putting it into reset in the start function
> isn't obvious to me.
>
Same reasoning as above.

> Frankly, I think it would be much better and risk fewer problems
> to just ignore the theoretic errors from the regmap functions.
>
Agreed. For the MMIO-backed regmap these accesses cannot fail in
practice, so in the next version I'll keep
rzt2h_wdt_wdtdcr_count_start() and rzt2h_wdt_wdtdcr_count_stop()
returning void, drop the error unwinding from the start/stop paths,
and ignore the return values of the regmap calls.

Cheers,
Prabhakar

> > +                     return ret;
> > +             }
> > +     }
> >
> >       pm_runtime_put_sync(wdev->parent);
> >
> > @@ -219,8 +234,10 @@ static int rzv2h_wdt_restart(struct watchdog_device *wdev,
> >       rzv2h_wdt_setup(wdev, priv->of_data->cks_min | WDTCR_RPSS_25 |
> >                       WDTCR_RPES_75 | WDTCR_TOPS_1024);
> >
> > -     if (priv->of_data->wdtdcr)
> > +     if (priv->of_data->wdtdcr) {
> > +             /* best effort, ignore ret */
> >               rzt2h_wdt_wdtdcr_count_start(priv);
> > +     }
> >
> >       rzv2h_wdt_ping(wdev);
> >
> > @@ -251,11 +268,11 @@ static int rzt2h_wdt_wdtdcr_init(struct platform_device *pdev,
> >       if (ret)
> >               return ret;
> >
> > -     rzt2h_wdt_wdtdcr_count_stop(priv);
> > +     ret = rzt2h_wdt_wdtdcr_count_stop(priv);
> >
> >       pm_runtime_put_sync(&pdev->dev);
> >
> > -     return 0;
> > +     return ret;
> >  }
> >
> >  static int rzv2h_wdt_probe(struct platform_device *pdev)
> > --
> > 2.55.0
> >