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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/03/2025 16:15, Daniel Lezcano wrote:
> +
> +struct s32g_wdt_device {
> +	int rate;
> +	void __iomem *base;
> +	struct watchdog_device wdog;
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout_param = S32G_WDT_DEFAULT_TIMEOUT;
> +module_param(timeout_param, uint, 0);
> +MODULE_PARM_DESC(timeout_param, "Watchdog timeout in seconds (default="
> +		 __MODULE_STRING(S32G_WDT_DEFAULT_TIMEOUT) ")");

Timeout is provided by DT.

> +
> +static bool early_enable = false;
> +module_param(early_enable, bool, 0);
> +MODULE_PARM_DESC(early_enable,
> +		 "Watchdog is started on module insertion (default=false)");
> +
> +static const struct watchdog_info s32g_wdt_info = {
> +	.identity = "s32g watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +	WDIOC_GETTIMEOUT | WDIOC_GETTIMELEFT,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define S32G_WDT_DEBUG_FS_REGS(__reg)		\
> +{						\
> +	.name = __stringify(__reg),		\
> +	.offset = __reg(0),			\
> +}
> +
> +static const struct debugfs_reg32 wdt_regs[] = {
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CR),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_TO),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CO),
> +};
> +
> +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);
> +
> +	dentry = debugfs_create_dir(dev_name(dev), dentry);
> +
> +	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->base = wdev->base;
> +	regset->regs = wdt_regs;
> +	regset->nregs = ARRAY_SIZE(wdt_regs);
> +
> +	debugfs_create_regset32("registers", 0400, dentry, regset);
> +}
> +#else
> +static inline void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> +{
> +}
> +#endif
> +
> +static struct s32g_wdt_device *wdd_to_s32g_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct s32g_wdt_device, wdog);
> +}
> +
> +static unsigned int wdog_sec_to_count(struct s32g_wdt_device *wdev, unsigned int timeout)
> +{
> +	return wdev->rate * timeout;
> +}
> +
> +static int s32g_wdt_ping(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +
> +	__raw_writel(S32G_WDT_SEQ1, S32G_SWT_SR(wdev->base));
> +	__raw_writel(S32G_WDT_SEQ2, S32G_SWT_SR(wdev->base));

I am confused why you do not use standard writel or don't have any
barriers here. I think this is very error prone and in general
discouraged practice (was for example raised by Arnd multiple times on
the lists).

> +
> +	return 0;
> +}
> +
> +static int s32g_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +	unsigned long val;
> +
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	val |= S32G_SWT_CR_WEN;
> +
> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
> +
> +	return 0;
> +}
> +

...

> +
> +static int s32g_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct s32g_wdt_device *wdev;
> +	struct watchdog_device *wdog;
> +	int ret;
> +
> +	wdev = devm_kzalloc(dev, sizeof(struct s32g_wdt_device), GFP_KERNEL);

sizeof(*)

> +	if (!wdev)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdev->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(wdev->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdev->base), "Can not get resource\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "Can't get Watchdog clock\n");
> +
> +	wdev->rate = clk_get_rate(clk);
> +	if (!wdev->rate) {
> +		dev_err(dev, "Input clock rate is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	wdog = &wdev->wdog;
> +	wdog->info = &s32g_wdt_info;
> +	wdog->ops = &s32g_wdt_ops;
> +
> +	/*
> +	 * The code converts the timeout into a counter a value, if
> +	 * the value is less than 0x100, then it is clamped by the SWT
> +	 * module, so it is safe to specify a zero value as the
> +	 * minimum timeout.
> +	 */
> +	wdog->min_timeout = 0;
> +
> +	/*
> +	 * The counter register is a 32 bits long, so the maximum
> +	 * counter value is UINT_MAX and the timeout in second is the
> +	 * value divided by the rate.
> +	 *
> +	 * For instance, a rate of 51MHz lead to 84 seconds maximum
> +	 * timeout.
> +	 */
> +	wdog->max_timeout = UINT_MAX / wdev->rate;
> +
> +	/*
> +	 * The module param and the DT 'timeout-sec' property will
> +	 * override the default value if they are specified.
> +	 */
> +	ret = watchdog_init_timeout(wdog, timeout_param, dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * As soon as the watchdog is started, there is no way to stop
> +	 * it if the 'nowayout' option is set at boot time
> +	 */
> +	watchdog_set_nowayout(wdog, nowayout);
> +
> +	/*
> +	 * The devm_ version of the watchdog_register_device()
> +	 * function will call watchdog_unregister_device() when the
> +	 * device is removed.
> +	 */
> +	watchdog_stop_on_unregister(wdog);
> +
> +	s32g_wdt_init(wdev);
> +
> +	/*
> +	 * The debugfs will create a directory with the configured
> +	 * watchdogs on the platform and a register file to give some
> +	 * register content.
> +	 */
> +	s32g_wdt_debugfs_init(dev, wdev);
> +
> +	ret = devm_watchdog_register_device(dev, wdog);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register watchdog device\n");
> +
> +	dev_info(dev, "S32G Watchdog Timer Registered. "
> +		 "timeout=%ds, nowayout=%d, early_enable=%d\n",
> +		 wdog->timeout, nowayout, early_enable);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id s32g_wdt_dt_ids[] = {
> +	{ .compatible = "nxp,s32g-wdt" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver s32g_wdt_driver = {
> +	.probe = s32g_wdt_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,

Drop, that's some ancient downstream code.

> +		.of_match_table = s32g_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(s32g_wdt_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("Watchdog driver for S32G SoC");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);

Drop, not needed. Fix your table module device Id instead... or start
from other recent driver as a skeleton to avoid repeating the same
issues we already fixed.

Best regards,
Krzysztof