[PATCH 1/6] Driver for the watchdog timer on Freescale IMX2 (and later) processors.

Wolfram Sang w.sang at pengutronix.de
Tue May 25 05:18:40 EDT 2010


Hi Wim,

thanks for the review!

On Fri, May 21, 2010 at 11:52:11PM +0200, Wim Van Sebroeck wrote:
> Hi Wolfram,
> 
> > Ping. Wim, did you notice this one? Shall the driver go via your tree or via
> > the imx-tree along with the resource updates (with your ack)?
> 
> I was not to happy with how the close of /dev/watchdog was being done.
> So I modified your code so that
> * the close is better supported.
> * the code is ready for when we change to the generic watchdog api.

Cool, any concrete plans for this happening?

> * the magic close feature is supported.

OK, I left it out intentionally, as I expected it to come for free with the new
api at a later stage:)

> 
> I didn't compile test the code. Could you have a look at it? compile it and test it?

Will do. Some initial comments. (An incremental patch might have been easier to
check)

> Goal is to get this in during this merge window still...

The new driver-rule should apply here, no?

> 
> Kind regards,
> Wim.
> 

> /*
>  * Watchdog driver for IMX2 and later processors
>  *
>  *  Copyright (C) 2010 Wolfram Sang, Pengutronix e.K. <w.sang at pengutronix.de>
>  *
>  * some parts adapted by similar drivers from Darius Augulis and Vladimir
>  * Zapolskiy.
>  *
>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2 as published by
>  * the Free Software Foundation.
>  *
>  * NOTE: MX1 has a slightly different Watchdog than MX2 and later:
>  *
>  *			MX1:		MX2+:
>  *			----		-----
>  * Registers:		32-bit		16-bit
>  * Stopable timer:	Yes		No
>  * Need to enable clk:	No		Yes
>  * Halt on suspend:	Manual		Can be automatic
>  */
> 
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/platform_device.h>
> #include <linux/watchdog.h>
> #include <linux/clk.h>
> #include <linux/fs.h>
> #include <linux/io.h>
> #include <linux/uaccess.h>
> #include <linux/timer.h>
> #include <linux/jiffies.h>
> #include <mach/hardware.h>
> 
> #define DRIVER_NAME "imx2-wdt"
> 
> #define IMX2_WDT_WCR		0x00		/* Control Register */
> #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> #define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> !Watchdog Assertion */
> #define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> !Software Reset Signal */
> #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
> #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
> #define IMX2_WDT_WCR_WDBG	(1 << 1)	/* -> Watchdog Debug Enable */
> #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog Low Power */
> 
> #define IMX2_WDT_WSR		0x02		/* Service Register */
> #define IMX2_WDT_SEQ1		0x5555		/* -> service sequence 1 */
> #define IMX2_WDT_SEQ2		0xAAAA		/* -> service sequence 2 */
> 
> #define IMX2_WDT_WRSR		0x04		/* Reset Status Register */
> #define IMX2_WDT_WRSR_PWR	(1 << 4)	/* -> Power-On Reset */
> #define IMX2_WDT_WRSR_EXT	(1 << 3)	/* -> External Reset */
> #define IMX2_WDT_WRSR_TOUT	(1 << 1)	/* -> WDOG Time-Out Reset */
> #define IMX2_WDT_WRSR_SFTW	(1 << 0)	/* -> Software Reset */
> 
> #define IMX2_WDT_MAX_TIME	128
> #define IMX2_WDT_DEFAULT_TIME	60		/* in seconds */
> 
> #define WDOG_SEC_TO_COUNT(s)	((s * 2) << 8)

Any reason you dropped the '- 1' from my version here?

> 
> #define IMX2_WDT_STATUS_OPEN	0
> #define IMX2_WDT_STATUS_STARTED	1
> #define IMX2_WDT_EXPECT_CLOSE	2
> 
> /* Apply nand and or masks to data read from addr and write back
>  * we nand first so that we can erase the timeout before setting the new one */
> #define IMX2_WDT_CHG_BITS(addr, nand, or) \
> 	__raw_writew((__raw_readw(addr) & ~nand) | or, addr)

I don't like such a macro very much, but well...

> 
> 
> static struct {
> 	struct clk *clk;
> 	void __iomem *base;
> 	unsigned timeout;
> 	unsigned long status;
> 	struct timer_list timer;	/* Pings the watchdog when closed */
> } imx2_wdt;
> 
> static struct miscdevice imx2_wdt_miscdev;
> 
> static int nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, int, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> 
> 
> static unsigned timeout = IMX2_WDT_DEFAULT_TIME;
> module_param(timeout, uint, 0);
> MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> 				__MODULE_STRING(IMX2_WDT_DEFAULT_TIME) ")");
> 
> static const struct watchdog_info imx2_wdt_info = {
> 	.identity = "imx2+ watchdog",
> 	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> };
> 
> static inline void imx2_wdt_init(void)
> {
> 	unsigned int bits_off;
> 	unsigned int bits_on;
> 
> 	/* Strip the old watchdog Time-Out value */
> 	bits_off = IMX2_WDT_WCR_WT;
> 	/* Generate reset if WDOG times out */
> 	bits_off |= IMX2_WDT_WCR_WRE;
> 	/* Keep Watchdog Disabled */
> 	bits_off |= IMX2_WDT_WCR_WDE;
> 	/* Continue timer operation during DEBUG mode */
> 	bits_off |= IMX2_WDT_WCR_WDBG;
> 	/* Continue Watchdog Timer during Low Power modes */
> 	bits_off |= IMX2_WDT_WCR_WDZST;
> 	
> 	/* Set the watchdog's Time-Out value */
> 	bits_on = WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
> 	/* Don't set Watchdog Assertion */
> 	bits_on |= IMX2_WDT_WCR_WDA;
> 	/* Don't set Software Reset Signal */
> 	bits_on |= IMX2_WDT_WCR_SRS;

This is too excessive IMO. The bootloader might already have setup the watchdog
according to the board.

> 
> 	IMX2_WDT_CHG_BITS(imx2_wdt.base + IMX2_WDT_WCR, bits_off, bits_on);
> }
> 
> static inline void imx2_wdt_enable(void)
> {
> 	IMX2_WDT_CHG_BITS(imx2_wdt.base + IMX2_WDT_WCR, 0, IMX2_WDT_WCR_WDE);
> }

I assume this is for the easy migration to the generic watchdog API? Otherwise
I see little use for a seperate function.

> 
> static inline void imx2_wdt_ping(void)
> {
> 	/* When enabled, the Watchdog requires that a service sequence be
> 	 * written to the Watchdog Service Register (WSR) */

IMHO this is maybe over-commenting :)

> 	__raw_writew(IMX2_WDT_SEQ1, imx2_wdt.base + IMX2_WDT_WSR);
> 	__raw_writew(IMX2_WDT_SEQ2, imx2_wdt.base + IMX2_WDT_WSR);
> }
> 
> static void imx2_wdt_timer_ping(void)
> {
> 	/* ping it every imx2_wdt.timeout / 2 seconds to prevent reboot */
> 	imx2_wdt_ping();
> 	mod_timer(&imx2_wdt.timer, jiffies + imx2_wdt.timeout * HZ / 2);
> }
> 
> static void imx2_wdt_start(void)
> {
> 	if (!test_and_set_bit(IMX2_WDT_STATUS_STARTED, &imx2_wdt.status)) {
> 		/* at our first start we enable clock and do initialisations */
> 		clk_enable(imx2_wdt.clk);
> 
> 		imx2_wdt_init();
> 		imx2_wdt_enable();
> 	} else	/* delete the timer that pings the watchdog after close */
> 		del_timer_sync(&imx2_wdt.timer);
> 
> 	/* Watchdog is enabled - time to reload the timeout value */
> 	imx2_wdt_ping();
> }
> 
> static void imx2_wdt_stop(void)
> {
> 	/* we don't need a clk_disable, it cannot be disabled once started.
> 	 * We use a timer to ping the watchdog while /dev/watchdog is closed */
> 	imx2_wdt_timer_ping();
> }
> 
> static void imx2_wdt_set_timeout(int new_timeout)
> {
> 	/* set the new timeout value in the WSR */
> 	IMX2_WDT_CHG_BITS(imx2_wdt.base + IMX2_WDT_WCR, IMX2_WDT_WCR_WT,
> 					WDOG_SEC_TO_COUNT(new_timeout));
> }
> 
> static int imx2_wdt_open(struct inode *inode, struct file *file)
> {
> 	if (test_and_set_bit(IMX2_WDT_STATUS_OPEN, &imx2_wdt.status))
> 		return -EBUSY;
> 
> 	imx2_wdt_start();
> 	return nonseekable_open(inode, file);
> }
> 
> static int imx2_wdt_close(struct inode *inode, struct file *file)
> {
> 	if (test_bit(IMX2_WDT_EXPECT_CLOSE, &imx2_wdt.status) && !nowayout)
> 		imx2_wdt_stop();
> 	else {
> 		dev_crit(imx2_wdt_miscdev.parent,
> 			"Unexpected close: Expect reboot!\n");
> 		imx2_wdt_ping();
> 	}
> 
> 	clear_bit(IMX2_WDT_EXPECT_CLOSE, &imx2_wdt.status);
> 	clear_bit(IMX2_WDT_STATUS_OPEN, &imx2_wdt.status);
> 	return 0;
> }
> 
> static long imx2_wdt_ioctl(struct file *file, unsigned int cmd,
> 							unsigned long arg)
> {
> 	void __user *argp = (void __user *)arg;
> 	int __user *p = argp;
> 	int new_value;
> 
> 	switch (cmd) {
> 	case WDIOC_GETSUPPORT:
> 		return copy_to_user(argp, &imx2_wdt_info,
> 			sizeof(struct watchdog_info)) ? -EFAULT : 0;
> 
> 	case WDIOC_GETSTATUS:
> 	case WDIOC_GETBOOTSTATUS:
> 		return put_user(0, p);
> 
> 	case WDIOC_KEEPALIVE:
> 		imx2_wdt_ping();
> 		return 0;
> 
> 	case WDIOC_SETTIMEOUT:
> 		if (get_user(new_value, p))
> 			return -EFAULT;
> 		if ((new_value < 1) || (new_value > IMX2_WDT_MAX_TIME))
> 			return -EINVAL;
> 		imx2_wdt_set_timeout(new_value);
> 		imx2_wdt.timeout = new_value;
> 		imx2_wdt_ping();
> 
> 		/* Fallthrough to return current value */
> 	case WDIOC_GETTIMEOUT:
> 		return put_user(imx2_wdt.timeout, p);
> 
> 	default:
> 		return -ENOTTY;
> 	}
> }
> 
> static ssize_t imx2_wdt_write(struct file *file, const char __user *data,
> 						size_t len, loff_t *ppos)
> {
> 	size_t i;
> 	char c;
> 
> 	if (len == 0)	/* Can we see this even ? */
> 		return 0;
> 
> 	clear_bit(IMX2_WDT_EXPECT_CLOSE, &imx2_wdt.status);
> 	/* scan to see whether or not we got the magic character */
> 	for (i = 0; i != len; i++) {
> 		if (get_user(c, data + i))
> 			return -EFAULT;
> 		if (c == 'V')
> 			set_bit(IMX2_WDT_EXPECT_CLOSE, &imx2_wdt.status);
> 		}
> 	}
> 	imx2_wdt_ping();
> 	return len;
> }
> 
> static const struct file_operations imx2_wdt_fops = {
> 	.owner = THIS_MODULE,
> 	.llseek = no_llseek,
> 	.unlocked_ioctl = imx2_wdt_ioctl,
> 	.open = imx2_wdt_open,
> 	.release = imx2_wdt_close,
> 	.write = imx2_wdt_write,
> };
> 
> static struct miscdevice imx2_wdt_miscdev = {
> 	.minor = WATCHDOG_MINOR,
> 	.name = "watchdog",
> 	.fops = &imx2_wdt_fops,
> };
> 
> static int __init imx2_wdt_probe(struct platform_device *pdev)
> {
> 	int ret;
> 	int res_size;
> 	struct resource *res;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	if (!res) {
> 		dev_err(&pdev->dev, "can't get device resources\n");
> 		return -ENODEV;
> 	}
> 
> 	res_size = resource_size(res);
> 	if (!devm_request_mem_region(&pdev->dev, res->start, res_size,
> 		res->name)) {
> 		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
> 			res_size, res->start);
> 		return -ENOMEM;
> 	}
> 
> 	imx2_wdt.base = devm_ioremap_nocache(&pdev->dev, res->start, res_size);
> 	if (!imx2_wdt.base) {
> 		dev_err(&pdev->dev, "ioremap failed\n");
> 		return -ENOMEM;
> 	}
> 
> 	imx2_wdt.clk = clk_get_sys("imx-wdt.0", NULL);
> 	if (IS_ERR(imx2_wdt.clk)) {
> 		dev_err(&pdev->dev, "can't get Watchdog clock\n");
> 		return PTR_ERR(imx2_wdt.clk);
> 	}
> 
> 	imx2_wdt.timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
> 	if (imx2_wdt.timeout != timeout)
> 		dev_warn(&pdev->dev, "Initial timeout out of range! "
> 			"Clamped from %u to %u\n", timeout, imx2_wdt.timeout);
> 
> 	setup_timer(&imx2_wdt.timer, imx2_wdt_timer_ping, 0);
> 
> 	imx2_wdt_miscdev.parent = &pdev->dev;
> 	ret = misc_register(&imx2_wdt_miscdev);
> 	if (ret)
> 		goto fail;
> 
> 	dev_info(&pdev->dev,
> 		"IMX2+ Watchdog Timer enabled. timeout=%d sec (nowayout=%d)\n",
> 						imx2_wdt.timeout, nowayout);
> 	return 0;
> 
> fail:
> 	imx2_wdt_miscdev.parent = NULL;
> 	clk_put(imx2_wdt.clk);
> 	return ret;
> }
> 
> static int __exit imx2_wdt_remove(struct platform_device *pdev)
> {
> 	misc_deregister(&imx2_wdt_miscdev);
> 
> 	if (test_bit(IMX2_WDT_STATUS_STARTED, &imx2_wdt.status)) {
> 		del_timer_sync(&imx2_wdt.timer);
> 
> 		dev_crit(imx2_wdt_miscdev.parent,
> 			"Device removed: Expect reboot!\n");
> 	} else
> 		clk_put(imx2_wdt.clk);
> 
> 	imx2_wdt_miscdev.parent = NULL;
> 	return 0;
> }
> 
> static void imx2_wdt_shutdown(struct platform_device *pdev)
> {
> 	if (test_bit(IMX2_WDT_STATUS_STARTED, &imx2_wdt.status)) {
> 		/* we are running, we need to delete the timer but will give
> 		 * max timeout before reboot will take place */
> 		del_timer_sync(&imx2_wdt.timer);
> 		imx2_wdt_set_timeout(IMX2_WDT_MAX_TIME);
> 		imx2_wdt_ping();
> 
> 		dev_crit(imx2_wdt_miscdev.parent,
> 			"Device shutdown: Expect reboot!\n");
> 	}
> }
> 
> static struct platform_driver imx2_wdt_driver = {
> 	.probe		= imx2_wdt_probe,
> 	.remove		= __exit_p(imx2_wdt_remove),
> 	.shutdown	= imx2_wdt_shutdown,
> 	.driver		= {
> 		.name	= DRIVER_NAME,
> 		.owner	= THIS_MODULE,
> 	},
> };
> 
> static int __init imx2_wdt_init(void)
> {
> 	return platform_driver_probe(&imx2_wdt_driver, imx2_wdt_probe);
> }
> module_init(imx2_wdt_init);
> 
> static void __exit imx2_wdt_exit(void)
> {
> 	platform_driver_unregister(&imx2_wdt_driver);
> }
> module_exit(imx2_wdt_exit);
> 
> MODULE_AUTHOR("Wolfram Sang");
> MODULE_DESCRIPTION("Watchdog driver for IMX2 and later");
> MODULE_LICENSE("GPL v2");
> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> MODULE_ALIAS("platform:" DRIVER_NAME);

The rest looks good, thanks for that. Some functions looks a bit too trivial
(and are called just once) for my taste, but I guess this is for easier
migration later? Oh, and a few whitespace issues, easily fixed.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/736f6d78/attachment-0001.sig>


More information about the linux-arm-kernel mailing list