Message-ID: <52F327E5.7070507@roeck-us.net>
Date:	Wed, 05 Feb 2014 22:12:53 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Andrew Chew <achew@...dia.com>, wim@...ana.be, rob@...dley.net,
	swarren@...dotorg.org, thierry.reding@...il.com,
	grant.likely@...aro.org, robh+dt@...nel.org, abrestic@...omium.org,
	dgreid@...omium.org, katierh@...omium.org
CC:	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
	linux-watchdog@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 1/1] watchdog: Add tegra watchdog

On 02/05/2014 07:38 PM, Andrew Chew wrote:
> Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and
> later).  This driver will configure one watchdog timer that will reset the
> system in the case of a watchdog timeout.
>
> This driver binds to the nvidia,tegra30-timer device node and gets its
> register base from there.
>
> Signed-off-by: Andrew Chew <achew@...dia.com>
> ---
> Changes from V2:
>
> Applied all of Stephen Warren's comments.
> Modified suspend callback by only stopping the watchdog timer if it was
> currently active.
> Added some logging during suspend/resume.
>
>   Documentation/watchdog/watchdog-parameters.txt |   5 +
>   drivers/watchdog/Kconfig                       |  11 +
>   drivers/watchdog/Makefile                      |   1 +
>   drivers/watchdog/tegra_wdt.c                   | 371 +++++++++++++++++++++++++
>   4 files changed, 388 insertions(+)
>   create mode 100644 drivers/watchdog/tegra_wdt.c
>
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index f9492fe..b39f355 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot
>   stmp3xxx_wdt:
>   heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19
>   -------------------------------------------------
> +tegra_wdt:
> +heartbeat: Watchdog heartbeats in seconds. (default = 120)
> +nowayout: Watchdog cannot be stopped once started
> +	(default=kernel config parameter)
> +-------------------------------------------------
>   ts72xx_wdt:
>   timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8)
>   nowayout: Disable watchdog shutdown on close
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 4c4c566..3aae2da 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG
>   	  Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When
>   	  the watchdog triggers the system will be reset.
>
> +config TEGRA_WATCHDOG
> +	tristate "Tegra watchdog"
> +	depends on ARCH_TEGRA || COMPILE_TEST

Hope you tested this on a couple of platforms ?

> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the watchdog timer
> +	  embedded in NVIDIA Tegra SoCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called tegra_wdt.
> +
>   # AVR32 Architecture
>
>   config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 985a66c..1b5f3d5 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
>   obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o
>   obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o
>   obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
> +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>
>   # AVR32 Architecture
>   obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
> new file mode 100644
> index 0000000..1314475
> --- /dev/null
> +++ b/drivers/watchdog/tegra_wdt.c
> @@ -0,0 +1,371 @@
> +/*
> + * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
> + *

2014 ?

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>

Not needed - also see below.

> +#include <linux/watchdog.h>
> +
> +/* minimum and maximum watchdog trigger timeout, in seconds */
> +#define MIN_WDT_TIMEOUT			1
> +#define MAX_WDT_TIMEOUT			255
> +
> +/*
> + * Base of the WDT registers, from the timer base address.  There are
> + * actually 5 watchdogs that can be configured (by pairing with an available
> + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4.
> + * This driver only configures the first watchdog (WDT ID 0).
> + */
> +#define WDT_BASE			0x100
> +#define WDT_ID				0
> +
> +/*
> + * Register base of the timer that's selected for pairing with the watchdog.
> + * This driver arbitrarily uses timer 5, which is currently unused by
> + * other drivers (in particular, the Tegra clocksource driver).  If this
> + * needs to change, take care that the new timer is not used by the
> + * clocksource driver.
> + */
> +#define WDT_TIMER_BASE			0x60
> +#define WDT_TIMER_ID			5
> +
> +/* WDT registers */
> +#define WDT_CFG				0x0
> +#define WDT_CFG_PERIOD_SHIFT		4
> +#define WDT_CFG_PERIOD_MASK		0xff
> +#define WDT_CFG_INT_EN			(1 << 12)
> +#define WDT_CFG_PMC2CAR_RST_EN		(1 << 15)
> +#define WDT_STS				0x4
> +#define WDT_STS_COUNT_SHIFT		4
> +#define WDT_STS_COUNT_MASK		0xff
> +#define WDT_STS_EXP_SHIFT		12
> +#define WDT_STS_EXP_MASK		0x3
> +#define WDT_CMD				0x8
> +#define WDT_CMD_START_COUNTER		(1 << 0)
> +#define WDT_CMD_DISABLE_COUNTER		(1 << 1)
> +#define WDT_UNLOCK			(0xc)
> +#define WDT_UNLOCK_PATTERN		(0xc45a << 0)
> +
> +/* Timer registers */
> +#define TIMER_PTV			0x0
> +#define TIMER_EN			(1 << 31)
> +#define TIMER_PERIODIC			(1 << 30)
> +
> +struct tegra_wdt {
> +	struct watchdog_device	wdd;
> +	struct kref		kref;
> +	void __iomem		*wdt_regs;
> +	void __iomem		*tmr_regs;
> +};
> +
> +#define WDT_HEARTBEAT 120
> +static int heartbeat = WDT_HEARTBEAT;
> +module_param(heartbeat, int, 0);
> +MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
> +	"(default = " __MODULE_STRING(WDT_HEARTBEAT) ")");

I'd suggest to split this such that the chackpatch warning goes away.

> +
> +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) ")");

Same here.

> +
> +static void tegra_wdt_release_resources(struct kref *r)
> +{
> +}
> +
> +static int tegra_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 val;
> +
> +	/* This thing has a fixed 1MHz clock.  Normally, we would set the

Multi-line comment coding style.

> +	 * period to 1 second by writing 1000000ul, but the watchdog system
> +	 * reset actually occurs on the 4th expiration of this counter,
> +	 * so we set the period to 1/4 of this amount.
> +	 */
> +	val = 1000000ul / 4;
> +	val |= (TIMER_EN | TIMER_PERIODIC);
> +	writel(val, wdt->tmr_regs + TIMER_PTV);
> +
> +	/*
> +	 * Set number of periods and start counter.
> +	 *
> +	 * Interrupt handler is not required for user space
> +	 * WDT accesses, since the caller is responsible to ping the
> +	 * WDT to reset the counter before expiration, through ioctls.
> +	 */
> +	val = WDT_TIMER_ID |
> +	      (wdd->timeout << WDT_CFG_PERIOD_SHIFT) |
> +	      WDT_CFG_PMC2CAR_RST_EN;
> +	writel(val, wdt->wdt_regs + WDT_CFG);
> +
> +	writel(WDT_CMD_START_COUNTER, wdt->wdt_regs + WDT_CMD);
> +
> +	return 0;
> +}
> +
> +static int tegra_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	writel(WDT_UNLOCK_PATTERN, wdt->wdt_regs + WDT_UNLOCK);
> +	writel(WDT_CMD_DISABLE_COUNTER, wdt->wdt_regs + WDT_CMD);
> +	writel(0, wdt->tmr_regs + TIMER_PTV);
> +
> +	return 0;
> +}
> +
> +static int tegra_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	writel(WDT_CMD_START_COUNTER, wdt->wdt_regs + WDT_CMD);
> +
> +	return 0;
> +}
> +
> +static int tegra_wdt_set_timeout(struct watchdog_device *wdd,
> +				 unsigned int timeout)
> +{
> +	int ret;
> +
> +	ret = tegra_wdt_stop(wdd);
> +	if (ret)
> +		return ret;

Unusual. Is it necessary to stop the wdt to change its timeout ?
If so, might be worth a comment.

> +
> +	wdd->timeout = clamp_t(int, timeout, MIN_WDT_TIMEOUT, MAX_WDT_TIMEOUT);
> +
This is checked by the infrastructure.

> +	if (watchdog_active(wdd))
> +		return tegra_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static unsigned int tegra_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 val;
> +	int count;
> +	int exp;
> +
> +	val = readl(wdt->wdt_regs + WDT_STS);
> +
> +	/* Current countdown (from timeout) */
> +	count = (val >> WDT_STS_COUNT_SHIFT) & WDT_STS_COUNT_MASK;
> +
> +	/* Number of expirations (we are waiting for the 4th expiration) */
> +	exp = (val >> WDT_STS_EXP_SHIFT) & WDT_STS_EXP_MASK;
> +
> +	/*
> +	 * The entire thing is divided by 4 because we are ticking down 4 times
> +	 * faster due to needing to wait for the 4th expiration.
> +	 */
> +	return (((3 - exp) * wdd->timeout) + count) / 4;
> +}
> +
> +static void tegra_wdt_ref(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	kref_get(&wdt->kref);
> +}
> +
> +static void tegra_wdt_unref(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	kref_put(&wdt->kref, tegra_wdt_release_resources);
> +}
> +
> +static const struct watchdog_info tegra_wdt_info = {
> +	.options	= WDIOF_SETTIMEOUT |
> +			  WDIOF_MAGICCLOSE |
> +			  WDIOF_KEEPALIVEPING,
> +	.firmware_version = 0,
> +	.identity	= "Tegra Watchdog",
> +};
> +
> +static struct watchdog_ops tegra_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = tegra_wdt_start,
> +	.stop = tegra_wdt_stop,
> +	.ping = tegra_wdt_ping,
> +	.set_timeout = tegra_wdt_set_timeout,
> +	.get_timeleft = tegra_wdt_get_timeleft,
> +	.ref = tegra_wdt_ref,
> +	.ref = tegra_wdt_unref,

I am quite at loss what the reference counting is for, but I guess
I don't have to understand it ;-).

Anyway, assigning the unref function to the ref variable definitely
doesn't make much sense.

> +};
> +
> +static int tegra_wdt_probe(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdd;
> +	struct tegra_wdt *wdt;
> +	struct resource *res;
> +	void __iomem *regs;
> +	int ret;
> +
> +	/* This is the timer base. */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "incorrect resources\n");
> +		return -ENOENT;

"No such file or directory" ? -ENODEV or -EINVAL seems more appropriate.

Besides, devm_ioremap_resource() creates that same error message and
returns -EINVAL.

> +	}
> +
> +	regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR_OR_NULL(regs)) {

Does not return NULL on error.

> +		dev_err(&pdev->dev, "unable to map registers\n");

devm_ioremap_resource() already creates an error message.
No need to repeat it.

> +		return -ENOMEM;

	return PTR_ERR(regs);

> +	}
> +
> +	/*
> +	 * Allocate our watchdog driver data, which has the
> +	 * struct watchdog_device nested within it.
> +	 */
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(&pdev->dev, "out of memory\n");

The memory allocation code already dumps an error message.
No need to repeat it.

> +		return -ENOMEM;
> +	}
> +
> +	/* Initialize struct tegra_wdt. */
> +	wdt->wdt_regs = regs + WDT_BASE;
> +	wdt->tmr_regs = regs + WDT_TIMER_BASE;
> +
> +	/* Initialize struct watchdog_device. */
> +	wdd = &wdt->wdd;
> +	wdd->timeout = heartbeat;
> +	wdd->info = &tegra_wdt_info;
> +	wdd->ops = &tegra_wdt_ops;
> +	wdd->min_timeout = MIN_WDT_TIMEOUT;
> +	wdd->max_timeout = MAX_WDT_TIMEOUT;
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +
> +	kref_init(&wdt->kref);
> +
> +	ret = tegra_wdt_stop(wdd);

Sure you want to stop the watchdog if it is running ?
If that is the case, chances are that it is running on purpose,
eg if the watchdog is enabled in rommon/bios.

Besides, this really defeats nowayout. Just unload the driver,
load it again, and unload it again, and the watchdog is stopped.

> +	if (ret) {

tegra_wdt_stop() always returns 0. I don't think it makes much sense
to have dummy error checking code each time you call it,


> +		dev_err(&pdev->dev,
> +			"failed to stop watchdog device\n");

much less an error message which will never be displayed
and just takes space in the code.

> +		return ret;
> +	}
> +
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	dev_info(&pdev->dev,
> +		 "initialized wdt %d, using timer %d\n", WDT_ID, WDT_TIMER_ID);

I don't think writing those constants into the log provides much
if any value.

> +	dev_info(&pdev->dev,
> +		 "enabled wdt %d (heartbeat = %d sec, nowayout = %d)\n",
> +		 WDT_ID, heartbeat, nowayout);
> +
Seems to me you stopped the watchdog above. Sure, it is initialized, but
I don't think you can claim that it is enabled.

> +	return 0;
> +}
> +
> +static int tegra_wdt_remove(struct platform_device *pdev)
> +{
> +	struct tegra_wdt *wdt = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = tegra_wdt_stop(&wdt->wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to stop watchdog %d\n", WDT_ID);
> +		return ret;
> +	}
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	kref_put(&wdt->kref, tegra_wdt_release_resources);
> +
> +	dev_info(&pdev->dev, "removed wdt %d\n", WDT_ID);
> +
Same as above - %d is always 0 and doesn't provide any value.
If anything it is confising as it suggests that there might be
a wdt 1 as well. Same applies everywhere else where you refer
to wdt 0.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_wdt_runtime_suspend(struct device *dev)
> +{
> +	struct tegra_wdt *wdt = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (watchdog_active(&wdt->wdd)) {
> +		dev_info(dev, "suspend, stopping watchdog timer\n");
> +

I don't think that is worth clogging the log for.

> +		ret = tegra_wdt_stop(&wdt->wdd);
> +		if (ret) {
> +			dev_err(dev, "failed to stop watchdog %d\n", WDT_ID);
> +			return ret;
> +		}

Same as above - this error checking and the message is really quite value-free.

> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_wdt_runtime_resume(struct device *dev)
> +{
> +	struct tegra_wdt *wdt = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (watchdog_active(&wdt->wdd)) {
> +		dev_info(dev, "resume, restarting watchdog timer\n");
> +
Same here.

> +		ret = tegra_wdt_start(&wdt->wdd);
> +		if (ret) {
> +			dev_err(dev, "failed to restart watchdog %d\n", WDT_ID);
> +			return ret;
> +		}

Same here. Never happens.

> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct of_device_id tegra_wdt_of_match[] = {
> +	{ .compatible = "nvidia,tegra30-timer", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_wdt_of_match);
> +
> +static const struct dev_pm_ops tegra_wdt_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_wdt_runtime_suspend,
> +				tegra_wdt_runtime_resume)
> +};
> +
> +static struct platform_driver tegra_wdt_driver = {
> +	.probe		= tegra_wdt_probe,
> +	.remove		= tegra_wdt_remove,
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "tegra-wdt",
> +		.pm	= &tegra_wdt_pm_ops,
> +		.of_match_table = tegra_wdt_of_match,
> +	},
> +};
> +module_platform_driver(tegra_wdt_driver);
> +
> +MODULE_AUTHOR("NVIDIA Corporation");
> +MODULE_DESCRIPTION("Tegra Watchdog Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
>
The MISCDEV alias is not needed. We just got rid of all those.
Please don't re-introduce it.

Thanks,
Guenter



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists