Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Guenter Roeck]

On 3/28/25 12:42, Daniel Lezcano wrote:
> Hi Guenter,
>
> thanks for your review
>
> On 28/03/2025 19:10, Guenter Roeck wrote:
> > On 3/28/25 08:15, Daniel Lezcano wrote:
> > > The S32 platform has several Software Watchdog Timer available and
> >
> > Why "Software" ? This is a hardware watchdog, or am I missing something ?
>
> I have no idea why it is called 'Software' because it is indeed a hardware watchdog. It is how NXP called it in their technical reference manual.

I guess it is because it resets the software. Please drop the term;
it is misleading.

> > > tied with a CPU. The SWT0 is the only one which directly asserts the
> > > reset line, other SWT require an external setup to configure the reset
> > > behavior which is not part of this change.
> > >
> > > The maximum watchdog value depends on the clock feeding the SWT
> > > counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
> > > of 51MHz which lead to 83 seconds maximum timeout.
> > >
> > > The timeout can be specified via the device tree with the usual
> > > existing bindings 'timeout-sec' or via the module param timeout.
> > >
> > > The watchdog can be loaded with the 'nowayout' option, preventing the
> > > watchdog to be stopped.
> > >
> > > The watchdog can be started at boot time with the 'early-enable'
> > > option, thus letting the watchdog framework to service the watchdog
> > > counter.
> > >
> > > the watchdog support the magic character to stop when the userspace
> > > releases the device.
> > >
> > > Cc: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
> > > Cc: Thomas Fossati <thomas.fossati@xxxxxxxxxx>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > ---
>
> [ ... ]
>
> > > --- /dev/null
> > > +++ b/drivers/watchdog/s32g_wdt.c
> > > @@ -0,0 +1,362 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Watchdog driver for S32G SoC
> > > + *
> > > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > > + * Copyright 2017-2019, 2021-2025 NXP.
> >
> > Does this originate from out-of-tree code ?
> > If so, a reference would be helpful.
>
> Well, I kept the copyright but this implementation is mostly from scratch.
I am not a copyright expert, but if this isn't derived from some other driver
it should not include old copyrights.

> > > +#include <linux/debugfs.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/watchdog.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/io.h>
> > > +#include <linux/of.h>
> >
> > Alphabetic include file order, please.
> >
> > > +
> > > +#define DRIVER_NAME "s32g-wdt"
> > > +
> > > +#define S32G_SWT_CR(__base)    (__base + 0x00)        /* Control Register offset    */
> >
> > checkpatch:
> >      CHECK: Macro argument '__base' may be better as '(__base)' to avoid precedence issues
>
> I'm not sure to get this one.

#define S32G_SWT_CR(__base)    ((__base) + 0x00)

> > > +#define S32G_SWT_CR_SM        BIT(9) | BIT(10)    /* -> Service Mode        */
> >
> > checkpatch:
> >      ERROR: Macros with complex values should be enclosed in parentheses
> >
> > I am not going to comment on the other issues reported by checkpatch,
> > but I expect them to be fixed in the next version. I would strongly suggest
> > to run "checkpatch o--strict" on the patch and fix what it reports.
>
> Sure, I will do that, thanks
>
> [ ... ]
>
> > > +static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> > > +{
> > > +    struct debugfs_regset32 *regset;
> > > +    static struct dentry *dentry = NULL;
> > > +
> > > +    if (!dentry)
> > > +        dentry = debugfs_create_dir("watchdog", NULL);
> >
> > That is a terribly generic debugfs directory name. That is unacceptable.
> > Pick a name that is driver specific.
>
> Why is it terrible ? We end up with:
>
> watchdog/40100000.watchdog
>
> There are 7 watchdogs on the platform, the directory is there to group them all. It seems to me it is self-explanatory, no ?

It should be something like "s32g/40100000.watchdog". Someone could have some other watchdog
(say, a real software watchdog) in the system and decide to use the top level directory name
for whatever reason. The top level should be something driver specific, not a generic name.

> > > +
> > > +    dentry = debugfs_create_dir(dev_name(dev), dentry);
> > > +
> >
> > Where is this removed if the driver is unloaded ?
>
> Oh right, I missed it. Thanks for pointing this out.
>
> > Also, if the driver is built into the kernel, it seems to me that a second
> > instance will create a nested directory. That seems odd.
>
> No, because there is the test above if (!dentry) which is a static variable.

Yes, and then the second watchdog will create "watchdog/40100000.watchdog/40200000.watchdog"
or similar because dentry is overwritten with the reference to "40100000.watchdog"

Thanks,
Guenter