Message-ID: <03f65f72-4727-44c7-90cb-6d251f360c85@linaro.org>
Date: Fri, 28 Mar 2025 20:42:52 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Guenter Roeck <linux@...ck-us.net>, wim@...ux-watchdog.org
Cc: linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
 S32@....com, Ghennadi Procopciuc <ghennadi.procopciuc@....com>,
 Thomas Fossati <thomas.fossati@...aro.org>
Subject: Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP
 S32 platform


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.

>> 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@....com>
>> Cc: Thomas Fossati <thomas.fossati@...aro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>> ---

[ ... ]

>> --- /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.

>> +#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_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 ?

>> +
>> +    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.

[ ... ]

>> +static int s32g_wdt_set_timeout(struct watchdog_device *wdog, 
>> unsigned int timeout)
>> +{
>> +    struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +
>> +    __raw_writel(wdog_sec_to_count(wdev, timeout), S32G_SWT_TO(wdev- 
>> >base));
>> +
>> +    /*
>> +     * Conforming to the documentation, the timeout counter is
>> +     * loaded when servicing is operated or when the counter is
>> +     * enabled. In case the watchdog is already started it must be
>> +     * stopped and started again to update the timeout
>> +     * register. Here we choose to service the watchdog for
>> +     * simpler code.
>> +     */
>> +    return s32g_wdt_ping(wdog);
> 
> Either check if the watchdog is running, or add a note explaining that a 
> ping
> on a stopped watchdog does not have adverse effect.

Ok, I think the comment is unclear. I'll provide a clarified version 
based on the documentation.

>> +}
>> +
>> +static unsigned int s32g_wdt_get_timeleft(struct watchdog_device *wdog)
>> +{
>> +    struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +    unsigned long val, counter;
>> +
>> +    /*
>> +     * The counter output can be read only if the SWT is
>> +     * disabled. Given the latency between the internal counter
>> +     * and the counter output update, there can be very small
>> +     * difference. However, we can accept this matter of fact
>> +     * given the resolution is a second based unit for the output.
>> +     */
>> +    val = __raw_readl(S32G_SWT_CR(wdev->base));
>> +
>> +    if (test_bit(S32G_SWT_CR_WEN, &val))
>> +        s32g_wdt_stop(wdog);
> 
> The watchdog core provides wdt_is_running() which would avoid the
> extra i/o access.

Ok, thanks for the suggestion

>> +
>> +    counter = __raw_readl(S32G_SWT_CO(wdev->base));
>> +
>> +    if (test_bit(S32G_SWT_CR_WEN, &val))
>> +        s32g_wdt_start(wdog);
>> +
>> +    return counter / wdev->rate;
>> +}
>> +
>> +static const struct watchdog_ops s32g_wdt_ops = {
>> +    .owner        = THIS_MODULE,
>> +    .start        = s32g_wdt_start,
>> +    .stop        = s32g_wdt_stop,
>> +    .ping        = s32g_wdt_ping,
>> +    .set_timeout    = s32g_wdt_set_timeout,
>> +    .get_timeleft    = s32g_wdt_get_timeleft,
>> +};
>> +
>> +static void s32g_wdt_init(struct s32g_wdt_device *wdev)
>> +{
>> +    unsigned long val;
>> +
>> +    /* Set the watchdog's Time-Out value */
>> +    val = wdog_sec_to_count(wdev, wdev->wdog.timeout);
>> +
>> +    __raw_writel(val, S32G_SWT_TO(wdev->base));
>> +
>> +    /*
>> +     * Get the control register content. We are at init time, the
>> +     * watchdog should not be started.
>> +     */
>> +    val = __raw_readl(S32G_SWT_CR(wdev->base));
>> +
>> +    /*
>> +     * We want to allow the watchdog timer to be stopped when
>> +     * device enters debug mode.
>> +     */
>> +    val |= S32G_SWT_CR_FRZ;
>> +
>> +    /*
>> +     * However, when the CPU is in WFI or suspend mode, the
>> +     * watchdog must continue. The documentation refers it as the
>> +     * stopped mode.
>> +     */
>> +    val &= ~S32G_SWT_CR_STP;
>> +
>> +    /*
>> +     * Use Fixed Service Sequence to ping the watchdog which is
>> +     * 0x00 configuration value for the service mode. It should be
>> +     * already set because it is the default value but we reset it
>> +     * in case.
>> +     */
>> +    val &= ~S32G_SWT_CR_SM;
>> +
>> +    __raw_writel(val, S32G_SWT_CR(wdev->base));
>> +
>> +    /*
>> +     * The watchdog must be started when the module is loaded,
>> +     * leading to getting ride of the userspace control. The
> 
> ride ? And why does it _have_ to be started when the module is loaded ?

The comment is misleading. I meant when the 'early_enable' option is 
set, then the watchdog must be started.

>> +     * watchdog framework will handle the pings. It is especially
>> +     * handy for kernel development.
>> +     */
>> +    if (early_enable) {
>> +        s32g_wdt_start(&wdev->wdog);
>> +        set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
>> +    }
>> +}
>> +

[ ... ]

Thanks

   -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists