From mboxrd@z Thu Jan 1 00:00:00 1970 From: w.sang@pengutronix.de (Wolfram Sang) Date: Tue, 25 May 2010 11:18:40 +0200 Subject: [PATCH 1/6] Driver for the watchdog timer on Freescale IMX2 (and later) processors. In-Reply-To: <20100521215211.GV3972@infomag.iguana.be> References: <1272528202-24159-1-git-send-email-w.sang@pengutronix.de> <1272528202-24159-2-git-send-email-w.sang@pengutronix.de> <20100514024915.GE30794@pengutronix.de> <20100521215211.GV3972@infomag.iguana.be> Message-ID: <20100525091840.GA25521@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. > * > * 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 > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > > #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: