* [PATCH 2/2] ep93xx: added platform side support for TS-72xx WDT driver
2009-12-03 19:35 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Mika Westerberg
@ 2009-12-03 19:35 ` Mika Westerberg
2009-12-03 20:07 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Ryan Mallon
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2009-12-03 19:35 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
---
arch/arm/mach-ep93xx/include/mach/ts72xx.h | 2 ++
arch/arm/mach-ep93xx/ts72xx.c | 21 +++++++++++++++++++++
2 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-ep93xx/include/mach/ts72xx.h b/arch/arm/mach-ep93xx/include/mach/ts72xx.h
index 3bd934e..93107d8 100644
--- a/arch/arm/mach-ep93xx/include/mach/ts72xx.h
+++ b/arch/arm/mach-ep93xx/include/mach/ts72xx.h
@@ -65,6 +65,8 @@
#define TS72XX_RTC_DATA_PHYS_BASE 0x11700000
#define TS72XX_RTC_DATA_SIZE 0x00001000
+#define TS72XX_WDT_CONTROL_PHYS_BASE 0x23800000
+#define TS72XX_WDT_FEED_PHYS_BASE 0x23c00000
#ifndef __ASSEMBLY__
diff --git a/arch/arm/mach-ep93xx/ts72xx.c b/arch/arm/mach-ep93xx/ts72xx.c
index 259f782..fac1ec7 100644
--- a/arch/arm/mach-ep93xx/ts72xx.c
+++ b/arch/arm/mach-ep93xx/ts72xx.c
@@ -166,6 +166,26 @@ static struct platform_device ts72xx_rtc_device = {
.num_resources = 0,
};
+static struct resource ts72xx_wdt_resources[] = {
+ {
+ .start = TS72XX_WDT_CONTROL_PHYS_BASE,
+ .end = TS72XX_WDT_CONTROL_PHYS_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .start = TS72XX_WDT_FEED_PHYS_BASE,
+ .end = TS72XX_WDT_FEED_PHYS_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+};
+
+static struct platform_device ts72xx_wdt_device = {
+ .name = "ts72xx-wdt",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(ts72xx_wdt_resources),
+ .resource = ts72xx_wdt_resources,
+};
+
static struct ep93xx_eth_data ts72xx_eth_data = {
.phy_id = 1,
};
@@ -175,6 +195,7 @@ static void __init ts72xx_init_machine(void)
ep93xx_init_devices();
ts72xx_register_flash();
platform_device_register(&ts72xx_rtc_device);
+ platform_device_register(&ts72xx_wdt_device);
ep93xx_register_eth(&ts72xx_eth_data, 1);
}
--
1.5.6.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-03 19:35 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Mika Westerberg
2009-12-03 19:35 ` [PATCH 2/2] ep93xx: added platform side support for TS-72xx WDT driver Mika Westerberg
@ 2009-12-03 20:07 ` Ryan Mallon
2009-12-04 6:10 ` Mika Westerberg
2009-12-04 10:28 ` Alexander Clouter
2009-12-04 17:13 ` H Hartley Sweeten
3 siblings, 1 reply; 11+ messages in thread
From: Ryan Mallon @ 2009-12-03 20:07 UTC (permalink / raw)
To: linux-arm-kernel
Mika Westerberg wrote:
> Technologic Systems TS-72xx SBCs have external glue logic
> CPLD which includes watchdog timer. This driver implements
> kernel support for that.
Hi Mika, looks good. Some comments below.
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> ---
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ts72xx_wdt.c | 458 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+), 0 deletions(-)
> create mode 100644 drivers/watchdog/ts72xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3711b88..5204612 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -289,6 +289,17 @@ config ADX_WATCHDOG
> Say Y here if you want support for the watchdog timer on Avionic
> Design Xanthos boards.
>
> +config TS72XX_WATCHDOG
> + tristate "TS-72XX SBC Watchdog"
> + depends on MACH_TS72XX
> + help
> + Technologic Systems TS-7200, TS-7250 and TS-7260 boards have
> + watchdog timer implemented in a external CPLD chip. Say Y here
> + if you want to support for the watchdog timer on TS-72XX boards.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ts72xx_wdt.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 699199b..8e8a9b4 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
> obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
> obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
> +obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/ts72xx_wdt.c b/drivers/watchdog/ts72xx_wdt.c
> new file mode 100644
> index 0000000..88b75b9
> --- /dev/null
> +++ b/drivers/watchdog/ts72xx_wdt.c
> @@ -0,0 +1,458 @@
> +/*
> + * Watchdog driver for Technologic Systems TS-72xx based SBCs
> + * (TS-7200, TS-7250 and TS-7260). These boards have external
> + * glue logic CPLD chip, which includes programmable watchdog
> + * timer.
> + *
> + * Copyright (c) 2009 Mika Westerberg <mika.westerberg@iki.fi>
> + *
> + * This driver is based on ep93xx_wdt and wm831x_wdt drivers.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
Nitpick: Space between top comment and first line of includes.
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +
> +#define TS72XX_WDT_FEED_VAL 0x05
> +#define TS72XX_WDT_DEFAULT_TIMEOUT 8
> +
> +static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
> + "(1 <= timeout <= 8, default="
> + __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
> + ")");
> +
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
> +
> +/**
> + * struct ts72xx_wdt - watchdog control structure
> + * @lock: lock that protects this structure
> + * @flags: flags controlling watchdog device state
> + * @control_reg: watchdog control register
> + * @feed_reg: watchdog feed register
> + * @pdev: back pointer to platform dev
> + */
> +struct ts72xx_wdt {
> + struct mutex lock;
> + int timeout;
> +
> +#define TS72XX_WDT_BUSY_FLAG 1
> +#define TS72XX_WDT_EXPECT_CLOSE_FLAG 2
> + int flags;
> +
> + void __iomem *control_reg;
> + void __iomem *feed_reg;
> +
> + struct platform_device *pdev;
> +};
> +
> +struct platform_device *ts72xx_wdt_pdev;
> +
> +/*
> + * TS-72xx Watchdog supports following timeouts (value written
> + * to control register):
> + * value description
> + * -------------------------
> + * 0x00 watchdog disabled
> + * 0x01 250ms
> + * 0x02 500ms
> + * 0x03 1s
> + * 0x04 reserved
> + * 0x05 2s
> + * 0x06 4s
> + * 0x07 8s
> + *
> + * Timeouts below 1s are not very usable so we don't
> + * allow them at all.
> + */
> +static const struct {
> + int timeout;
> + u8 ctrval;
> +} ts72xx_wdt_map[] = {
> + { 1, 3 },
> + { 2, 5 },
> + { 4, 6 },
> + { 8, 7 },
> +};
> +
> +/**
> + * normalize_timeout() - normalizes given timeout
> + * @new_timeout: timeout in seconds to be normalized
> + *
> + * This function normalizes given timeout value (in seconds)
> + * to value that is valid to TS-72xx watchdog timer. Valid
> + * values are 1, 2, 4 and 8 seconds. Timeout is rounded up
> + * to nearest valid timeout so for example value 3 yields
> + * 4 etc.
> + */
> +static int normalize_timeout(int new_timeout)
> +{
> + int i;
> +
> + /* first limit it to 1 - 8 seconds */
> + if (new_timeout < 1)
> + new_timeout = 1;
> + else if (new_timeout > 8)
> + new_timeout = 8;
You can do:
new_timeout = clamp_val(new_timeout, 1, 8);
for the same effect here.
> +
> + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> + if (ts72xx_wdt_map[i].timeout >= new_timeout)
> + return ts72xx_wdt_map[i].timeout;
> + }
> +
> + BUG();
> + /*NOTREACHED*/
This is reached if !CONFIG_BUG, you should return -EINVAL, and handle it
gracefully in the caller.
> +}
> +
> +/**
> + * timeout_to_ctrval() - converts given timeout to control register value
> + * @new_timeout: timeout in seconds to be converted (1, 2, 4 or 8)
> + *
> + * This function converts timeout in seconds to valid control register
> + * value. Note that @new_timeout must be normalized first with call to
> + * normalize_timeout().
> + */
If you must call normalize timeout first, the can the two functions be
combined so that you only need to do the loop once?
> +static u8 timeout_to_ctrval(int new_timeout)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> + if (ts72xx_wdt_map[i].timeout == new_timeout)
> + return ts72xx_wdt_map[i].ctrval;
> + }
> +
> + BUG();
> + /*NOTREACHED*/
Same as above, should return -EINVAL
> +}
> +
> +/**
> + * ts72xx_wdt_kick() - kick the watchdog
> + * @wdt: watchdog to be kicked
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_kick(struct ts72xx_wdt *wdt)
Should probably be marked inline.
> +{
> + __raw_writeb(TS72XX_WDT_FEED_VAL, wdt->feed_reg);
> +}
> +
> +/**
> + * ts72xx_wdt_start() - starts the watchdog timer
> + * @wdt: watchdog to be started
> + *
> + * This function programs timeout to watchdog timer
> + * and starts it.
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_start(struct ts72xx_wdt *wdt)
> +{
> + /*
> + * To program the wdt, it first must be "fed" and
> + * only after that (within 30 usecs) the configuration
> + * can be changed.
> + */
> + ts72xx_wdt_kick(wdt);
> + __raw_writeb(timeout_to_ctrval(wdt->timeout), wdt->control_reg);
> +}
> +
> +/**
> + * ts72xx_wdt_stop() - stops the watchdog timer
> + * @wdt: watchdog to be stopped
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_stop(struct ts72xx_wdt *wdt)
> +{
> + ts72xx_wdt_kick(wdt);
> + __raw_writeb(0, wdt->control_reg);
> +}
> +
> +static int ts72xx_wdt_open(struct inode *inode, struct file *file)
> +{
> + struct ts72xx_wdt *wdt = platform_get_drvdata(ts72xx_wdt_pdev);
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + if ((wdt->flags & TS72XX_WDT_BUSY_FLAG) != 0) {
> + mutex_unlock(&wdt->lock);
> + return -EBUSY;
> + }
> +
> + wdt->flags = TS72XX_WDT_BUSY_FLAG;
> + wdt->timeout = normalize_timeout(timeout);
> + file->private_data = wdt;
> +
> + ts72xx_wdt_start(wdt);
> +
> + mutex_unlock(&wdt->lock);
> + return nonseekable_open(inode, file);
> +}
> +
> +static int ts72xx_wdt_release(struct inode *inode, struct file *file)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + if ((wdt->flags & TS72XX_WDT_EXPECT_CLOSE_FLAG) != 0) {
> + ts72xx_wdt_stop(wdt);
> + } else {
> + dev_warn(&wdt->pdev->dev,
> + "TS-72XX WDT device closed unexpectly. "
> + "Watchdog timer will not stop!\n");
> + /*
> + * Kick it one more time, to give userland some time
> + * to recover (for example, respawning the kicker
> + * daemon).
> + */
> + ts72xx_wdt_kick(wdt);
> + }
> +
> + wdt->flags = 0;
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static ssize_t ts72xx_wdt_write(struct file *file,
> + const char __user *data,
> + size_t len,
> + loff_t *ppos)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> +
> + if (!len)
> + return 0;
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + ts72xx_wdt_kick(wdt);
> +
> + /*
> + * Support for magic character closing. User process
> + * writes 'V' into the device, just before it is closed.
> + * This means that we know that the wdt timer can be
> + * stopped after user closes the device.
> + */
> + if (!nowayout) {
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + char c;
> +
> + /* In case it was set long ago */
> + wdt->flags &= ~TS72XX_WDT_EXPECT_CLOSE_FLAG;
> +
> + if (get_user(c, data + i)) {
> + mutex_unlock(&wdt->lock);
> + return -EFAULT;
> + }
> + if (c == 'V') {
> + wdt->flags |= TS72XX_WDT_EXPECT_CLOSE_FLAG;
> + break;
> + }
> + }
> + }
> +
> + mutex_unlock(&wdt->lock);
> + return len;
> +}
> +
> +static const struct watchdog_info winfo = {
> + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> + .firmware_version = 1,
> + .identity = "TS-72XX WDT",
> +};
Nitpick: Tab align struct assignments.
> +
> +static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> + void __user *argp = (void __user *)arg;
> + int __user *p = (int __user *)argp;
> + int error = 0;
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + switch (cmd) {
> + case WDIOC_GETSUPPORT:
> + error = copy_to_user(argp, &winfo, sizeof(winfo));
> + break;
> +
> + case WDIOC_KEEPALIVE:
> + ts72xx_wdt_kick(wdt);
> + break;
> +
> + case WDIOC_SETOPTIONS: {
> + int options;
> +
> + if (get_user(options, p)) {
> + error = -EFAULT;
> + break;
> + }
> +
> + error = -EINVAL;
> +
> + if ((options & WDIOS_DISABLECARD) != 0) {
> + ts72xx_wdt_stop(wdt);
> + error = 0;
> + }
> + if ((options & WDIOS_ENABLECARD) != 0) {
> + ts72xx_wdt_start(wdt);
> + error = 0;
> + }
> +
> + break;
> + }
> +
> + case WDIOC_SETTIMEOUT: {
> + int new_timeout;
> +
> + if (get_user(new_timeout, p)) {
> + error = -EFAULT;
> + } else {
> + ts72xx_wdt_stop(wdt);
> + wdt->timeout = normalize_timeout(new_timeout);
> + ts72xx_wdt_start(wdt);
> + }
> + /*FALLTHROUGH*/
> + }
> +
> + case WDIOC_GETTIMEOUT:
> + if (put_user(wdt->timeout, p))
> + error = -EFAULT;
> + break;
> +
> + default:
> + error = -ENOTTY;
> + break;
> + }
> +
> + mutex_unlock(&wdt->lock);
> + return error;
> +}
> +
> +static const struct file_operations ts72xx_wdt_fops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .open = ts72xx_wdt_open,
> + .release = ts72xx_wdt_release,
> + .write = ts72xx_wdt_write,
> + .unlocked_ioctl = ts72xx_wdt_ioctl,
> +};
> +
> +static struct miscdevice ts72xx_wdt_miscdev = {
> + .minor = WATCHDOG_MINOR,
> + .name = "watchdog",
> + .fops = &ts72xx_wdt_fops,
> +};
> +
> +static __devinit int ts72xx_wdt_probe(struct platform_device *pdev)
> +{
> + struct ts72xx_wdt *wdt;
> + struct resource *res;
> + int error = -EIO;
> +
> + wdt = kzalloc(sizeof(*wdt), GFP_KERNEL);
sizeof(struct ts72xx_wdt)
is the preferred style I think. If the other watchdog drivers do it this
way just leave it though.
> + if (!wdt)
> + return -ENOMEM;
> +
> + mutex_init(&wdt->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + goto fail;
> +
> + wdt->control_reg = ioremap(res->start, resource_size(res));
> + if (!wdt->control_reg)
> + goto fail;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + goto fail;
> +
> + wdt->feed_reg = ioremap(res->start, resource_size(res));
> + if (!wdt->feed_reg)
> + goto fail;
> +
> + error = misc_register(&ts72xx_wdt_miscdev);
> + if (error)
> + goto fail;
> +
> + platform_set_drvdata(pdev, wdt);
> + ts72xx_wdt_pdev = pdev;
> + wdt->pdev = pdev;
> +
> + dev_info(&pdev->dev, "TS-72xx Watchdog driver\n");
> +
> + return 0;
> +
> +fail:
> + platform_set_drvdata(pdev, NULL);
I don't think you need this in the error path since platform_set_drvdata
is called after the last goto fail.
> + if (wdt->control_reg)
> + iounmap(wdt->control_reg);
> + if (wdt->feed_reg)
> + iounmap(wdt->feed_reg);
> +
> + kfree(wdt);
> + return error;
> +}
> +
> +static __devexit int ts72xx_wdt_remove(struct platform_device *pdev)
> +{
> + struct ts72xx_wdt *wdt =
> + (struct ts72xx_wdt *)platform_get_drvdata(pdev);
> +
> + if (wdt->control_reg)
> + iounmap(wdt->control_reg);
> + if (wdt->feed_reg)
> + iounmap(wdt->feed_reg);
> +
> + kfree(wdt);
> +
> + platform_set_drvdata(pdev, NULL);
> + return misc_deregister(&ts72xx_wdt_miscdev);
> +}
> +
> +static struct platform_driver ts72xx_wdt_driver = {
> + .probe = ts72xx_wdt_probe,
> + .remove = __devexit_p(ts72xx_wdt_remove),
> + .driver = {
> + .name = "ts72xx-wdt",
> + },
> +};
> +
> +static __init int ts72xx_wdt_init(void)
> +{
> + return platform_driver_register(&ts72xx_wdt_driver);
> +}
> +module_init(ts72xx_wdt_init);
> +
> +static __exit void ts72xx_wdt_exit(void)
> +{
> + platform_driver_unregister(&ts72xx_wdt_driver);
> +}
> +module_exit(ts72xx_wdt_exit);
> +
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@iki.fi>");
> +MODULE_DESCRIPTION("TS-72xx SBC Watchdog");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ts72xx-wdt");
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-03 20:07 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Ryan Mallon
@ 2009-12-04 6:10 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2009-12-04 6:10 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ryan,
On Fri, Dec 04, 2009 at 09:07:10AM +1300, Ryan Mallon wrote:
> Mika Westerberg wrote:
> > Technologic Systems TS-72xx SBCs have external glue logic
> > CPLD which includes watchdog timer. This driver implements
> > kernel support for that.
>
> Hi Mika, looks good. Some comments below.
Thank you very much for your comments. My answers to the comments are
below.
I'll wait a bit if someone else still has comments and then prepare
version 2 of the patches.
Thanks,
MW
>
> > Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> > ---
> > drivers/watchdog/Kconfig | 11 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/ts72xx_wdt.c | 458 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 470 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/watchdog/ts72xx_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 3711b88..5204612 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -289,6 +289,17 @@ config ADX_WATCHDOG
> > Say Y here if you want support for the watchdog timer on Avionic
> > Design Xanthos boards.
> >
> > +config TS72XX_WATCHDOG
> > + tristate "TS-72XX SBC Watchdog"
> > + depends on MACH_TS72XX
> > + help
> > + Technologic Systems TS-7200, TS-7250 and TS-7260 boards have
> > + watchdog timer implemented in a external CPLD chip. Say Y here
> > + if you want to support for the watchdog timer on TS-72XX boards.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ts72xx_wdt.
> > +
> > # AVR32 Architecture
> >
> > config AT32AP700X_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 699199b..8e8a9b4 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -46,6 +46,7 @@ obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
> > obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
> > obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> > obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
> > +obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> >
> > # AVR32 Architecture
> > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> > diff --git a/drivers/watchdog/ts72xx_wdt.c b/drivers/watchdog/ts72xx_wdt.c
> > new file mode 100644
> > index 0000000..88b75b9
> > --- /dev/null
> > +++ b/drivers/watchdog/ts72xx_wdt.c
> > @@ -0,0 +1,458 @@
> > +/*
> > + * Watchdog driver for Technologic Systems TS-72xx based SBCs
> > + * (TS-7200, TS-7250 and TS-7260). These boards have external
> > + * glue logic CPLD chip, which includes programmable watchdog
> > + * timer.
> > + *
> > + * Copyright (c) 2009 Mika Westerberg <mika.westerberg@iki.fi>
> > + *
> > + * This driver is based on ep93xx_wdt and wm831x_wdt drivers.
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
>
> Nitpick: Space between top comment and first line of includes.
Will fix to v2.
>
> > +#include <linux/fs.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/uaccess.h>
> > +
> > +#define TS72XX_WDT_FEED_VAL 0x05
> > +#define TS72XX_WDT_DEFAULT_TIMEOUT 8
> > +
> > +static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
> > + "(1 <= timeout <= 8, default="
> > + __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
> > + ")");
> > +
> > +static int nowayout = WATCHDOG_NOWAYOUT;
> > +module_param(nowayout, int, 0);
> > +MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
> > +
> > +/**
> > + * struct ts72xx_wdt - watchdog control structure
> > + * @lock: lock that protects this structure
> > + * @flags: flags controlling watchdog device state
> > + * @control_reg: watchdog control register
> > + * @feed_reg: watchdog feed register
> > + * @pdev: back pointer to platform dev
> > + */
> > +struct ts72xx_wdt {
> > + struct mutex lock;
> > + int timeout;
> > +
> > +#define TS72XX_WDT_BUSY_FLAG 1
> > +#define TS72XX_WDT_EXPECT_CLOSE_FLAG 2
> > + int flags;
> > +
> > + void __iomem *control_reg;
> > + void __iomem *feed_reg;
> > +
> > + struct platform_device *pdev;
> > +};
> > +
> > +struct platform_device *ts72xx_wdt_pdev;
> > +
> > +/*
> > + * TS-72xx Watchdog supports following timeouts (value written
> > + * to control register):
> > + * value description
> > + * -------------------------
> > + * 0x00 watchdog disabled
> > + * 0x01 250ms
> > + * 0x02 500ms
> > + * 0x03 1s
> > + * 0x04 reserved
> > + * 0x05 2s
> > + * 0x06 4s
> > + * 0x07 8s
> > + *
> > + * Timeouts below 1s are not very usable so we don't
> > + * allow them at all.
> > + */
> > +static const struct {
> > + int timeout;
> > + u8 ctrval;
> > +} ts72xx_wdt_map[] = {
> > + { 1, 3 },
> > + { 2, 5 },
> > + { 4, 6 },
> > + { 8, 7 },
> > +};
> > +
> > +/**
> > + * normalize_timeout() - normalizes given timeout
> > + * @new_timeout: timeout in seconds to be normalized
> > + *
> > + * This function normalizes given timeout value (in seconds)
> > + * to value that is valid to TS-72xx watchdog timer. Valid
> > + * values are 1, 2, 4 and 8 seconds. Timeout is rounded up
> > + * to nearest valid timeout so for example value 3 yields
> > + * 4 etc.
> > + */
> > +static int normalize_timeout(int new_timeout)
> > +{
> > + int i;
> > +
> > + /* first limit it to 1 - 8 seconds */
> > + if (new_timeout < 1)
> > + new_timeout = 1;
> > + else if (new_timeout > 8)
> > + new_timeout = 8;
>
> You can do:
>
> new_timeout = clamp_val(new_timeout, 1, 8);
>
> for the same effect here.
Wow, I didn't even know that such functions (macros) exists
in kernel :) I will fix this in v2.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> > + if (ts72xx_wdt_map[i].timeout >= new_timeout)
> > + return ts72xx_wdt_map[i].timeout;
> > + }
> > +
> > + BUG();
> > + /*NOTREACHED*/
>
> This is reached if !CONFIG_BUG, you should return -EINVAL, and handle it
> gracefully in the caller.
OK. Will be fixed in v2.
>
> > +}
> > +
> > +/**
> > + * timeout_to_ctrval() - converts given timeout to control register value
> > + * @new_timeout: timeout in seconds to be converted (1, 2, 4 or 8)
> > + *
> > + * This function converts timeout in seconds to valid control register
> > + * value. Note that @new_timeout must be normalized first with call to
> > + * normalize_timeout().
> > + */
>
> If you must call normalize timeout first, the can the two functions be
> combined so that you only need to do the loop once?
Yeah, that's probably better way. I'll try to combine them
in v2.
>
> > +static u8 timeout_to_ctrval(int new_timeout)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> > + if (ts72xx_wdt_map[i].timeout == new_timeout)
> > + return ts72xx_wdt_map[i].ctrval;
> > + }
> > +
> > + BUG();
> > + /*NOTREACHED*/
>
> Same as above, should return -EINVAL
Will fix in v2.
>
> > +}
> > +
> > +/**
> > + * ts72xx_wdt_kick() - kick the watchdog
> > + * @wdt: watchdog to be kicked
> > + *
> > + * Called with @wdt->lock held.
> > + */
> > +static void ts72xx_wdt_kick(struct ts72xx_wdt *wdt)
>
> Should probably be marked inline.
Ok. Will do.
>
> > +{
> > + __raw_writeb(TS72XX_WDT_FEED_VAL, wdt->feed_reg);
> > +}
> > +
> > +/**
> > + * ts72xx_wdt_start() - starts the watchdog timer
> > + * @wdt: watchdog to be started
> > + *
> > + * This function programs timeout to watchdog timer
> > + * and starts it.
> > + *
> > + * Called with @wdt->lock held.
> > + */
> > +static void ts72xx_wdt_start(struct ts72xx_wdt *wdt)
> > +{
> > + /*
> > + * To program the wdt, it first must be "fed" and
> > + * only after that (within 30 usecs) the configuration
> > + * can be changed.
> > + */
> > + ts72xx_wdt_kick(wdt);
> > + __raw_writeb(timeout_to_ctrval(wdt->timeout), wdt->control_reg);
> > +}
> > +
> > +/**
> > + * ts72xx_wdt_stop() - stops the watchdog timer
> > + * @wdt: watchdog to be stopped
> > + *
> > + * Called with @wdt->lock held.
> > + */
> > +static void ts72xx_wdt_stop(struct ts72xx_wdt *wdt)
> > +{
> > + ts72xx_wdt_kick(wdt);
> > + __raw_writeb(0, wdt->control_reg);
> > +}
> > +
> > +static int ts72xx_wdt_open(struct inode *inode, struct file *file)
> > +{
> > + struct ts72xx_wdt *wdt = platform_get_drvdata(ts72xx_wdt_pdev);
> > +
> > + if (mutex_lock_interruptible(&wdt->lock))
> > + return -ERESTARTSYS;
> > +
> > + if ((wdt->flags & TS72XX_WDT_BUSY_FLAG) != 0) {
> > + mutex_unlock(&wdt->lock);
> > + return -EBUSY;
> > + }
> > +
> > + wdt->flags = TS72XX_WDT_BUSY_FLAG;
> > + wdt->timeout = normalize_timeout(timeout);
> > + file->private_data = wdt;
> > +
> > + ts72xx_wdt_start(wdt);
> > +
> > + mutex_unlock(&wdt->lock);
> > + return nonseekable_open(inode, file);
> > +}
> > +
> > +static int ts72xx_wdt_release(struct inode *inode, struct file *file)
> > +{
> > + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> > +
> > + if (mutex_lock_interruptible(&wdt->lock))
> > + return -ERESTARTSYS;
> > +
> > + if ((wdt->flags & TS72XX_WDT_EXPECT_CLOSE_FLAG) != 0) {
> > + ts72xx_wdt_stop(wdt);
> > + } else {
> > + dev_warn(&wdt->pdev->dev,
> > + "TS-72XX WDT device closed unexpectly. "
> > + "Watchdog timer will not stop!\n");
> > + /*
> > + * Kick it one more time, to give userland some time
> > + * to recover (for example, respawning the kicker
> > + * daemon).
> > + */
> > + ts72xx_wdt_kick(wdt);
> > + }
> > +
> > + wdt->flags = 0;
> > +
> > + mutex_unlock(&wdt->lock);
> > + return 0;
> > +}
> > +
> > +static ssize_t ts72xx_wdt_write(struct file *file,
> > + const char __user *data,
> > + size_t len,
> > + loff_t *ppos)
> > +{
> > + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> > +
> > + if (!len)
> > + return 0;
> > +
> > + if (mutex_lock_interruptible(&wdt->lock))
> > + return -ERESTARTSYS;
> > +
> > + ts72xx_wdt_kick(wdt);
> > +
> > + /*
> > + * Support for magic character closing. User process
> > + * writes 'V' into the device, just before it is closed.
> > + * This means that we know that the wdt timer can be
> > + * stopped after user closes the device.
> > + */
> > + if (!nowayout) {
> > + int i;
> > +
> > + for (i = 0; i < len; i++) {
> > + char c;
> > +
> > + /* In case it was set long ago */
> > + wdt->flags &= ~TS72XX_WDT_EXPECT_CLOSE_FLAG;
> > +
> > + if (get_user(c, data + i)) {
> > + mutex_unlock(&wdt->lock);
> > + return -EFAULT;
> > + }
> > + if (c == 'V') {
> > + wdt->flags |= TS72XX_WDT_EXPECT_CLOSE_FLAG;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + mutex_unlock(&wdt->lock);
> > + return len;
> > +}
> > +
> > +static const struct watchdog_info winfo = {
> > + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> > + .firmware_version = 1,
> > + .identity = "TS-72XX WDT",
> > +};
>
> Nitpick: Tab align struct assignments.
Ok. Will fix in v2.
>
> > +
> > +static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
> > + void __user *argp = (void __user *)arg;
> > + int __user *p = (int __user *)argp;
> > + int error = 0;
> > +
> > + if (mutex_lock_interruptible(&wdt->lock))
> > + return -ERESTARTSYS;
> > +
> > + switch (cmd) {
> > + case WDIOC_GETSUPPORT:
> > + error = copy_to_user(argp, &winfo, sizeof(winfo));
> > + break;
> > +
> > + case WDIOC_KEEPALIVE:
> > + ts72xx_wdt_kick(wdt);
> > + break;
> > +
> > + case WDIOC_SETOPTIONS: {
> > + int options;
> > +
> > + if (get_user(options, p)) {
> > + error = -EFAULT;
> > + break;
> > + }
> > +
> > + error = -EINVAL;
> > +
> > + if ((options & WDIOS_DISABLECARD) != 0) {
> > + ts72xx_wdt_stop(wdt);
> > + error = 0;
> > + }
> > + if ((options & WDIOS_ENABLECARD) != 0) {
> > + ts72xx_wdt_start(wdt);
> > + error = 0;
> > + }
> > +
> > + break;
> > + }
> > +
> > + case WDIOC_SETTIMEOUT: {
> > + int new_timeout;
> > +
> > + if (get_user(new_timeout, p)) {
> > + error = -EFAULT;
> > + } else {
> > + ts72xx_wdt_stop(wdt);
> > + wdt->timeout = normalize_timeout(new_timeout);
> > + ts72xx_wdt_start(wdt);
> > + }
> > + /*FALLTHROUGH*/
> > + }
> > +
> > + case WDIOC_GETTIMEOUT:
> > + if (put_user(wdt->timeout, p))
> > + error = -EFAULT;
> > + break;
> > +
> > + default:
> > + error = -ENOTTY;
> > + break;
> > + }
> > +
> > + mutex_unlock(&wdt->lock);
> > + return error;
> > +}
> > +
> > +static const struct file_operations ts72xx_wdt_fops = {
> > + .owner = THIS_MODULE,
> > + .llseek = no_llseek,
> > + .open = ts72xx_wdt_open,
> > + .release = ts72xx_wdt_release,
> > + .write = ts72xx_wdt_write,
> > + .unlocked_ioctl = ts72xx_wdt_ioctl,
> > +};
> > +
> > +static struct miscdevice ts72xx_wdt_miscdev = {
> > + .minor = WATCHDOG_MINOR,
> > + .name = "watchdog",
> > + .fops = &ts72xx_wdt_fops,
> > +};
> > +
> > +static __devinit int ts72xx_wdt_probe(struct platform_device *pdev)
> > +{
> > + struct ts72xx_wdt *wdt;
> > + struct resource *res;
> > + int error = -EIO;
> > +
> > + wdt = kzalloc(sizeof(*wdt), GFP_KERNEL);
>
> sizeof(struct ts72xx_wdt)
>
> is the preferred style I think. If the other watchdog drivers do it this
> way just leave it though.
It seems that both styles are used in watchdog drivers. I'll change
this to the preferred style in v2.
>
> > + if (!wdt)
> > + return -ENOMEM;
> > +
> > + mutex_init(&wdt->lock);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + goto fail;
> > +
> > + wdt->control_reg = ioremap(res->start, resource_size(res));
> > + if (!wdt->control_reg)
> > + goto fail;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res)
> > + goto fail;
> > +
> > + wdt->feed_reg = ioremap(res->start, resource_size(res));
> > + if (!wdt->feed_reg)
> > + goto fail;
> > +
> > + error = misc_register(&ts72xx_wdt_miscdev);
> > + if (error)
> > + goto fail;
> > +
> > + platform_set_drvdata(pdev, wdt);
> > + ts72xx_wdt_pdev = pdev;
> > + wdt->pdev = pdev;
> > +
> > + dev_info(&pdev->dev, "TS-72xx Watchdog driver\n");
> > +
> > + return 0;
> > +
> > +fail:
> > + platform_set_drvdata(pdev, NULL);
>
> I don't think you need this in the error path since platform_set_drvdata
> is called after the last goto fail.
Ah, good catch. Will fix in v2.
>
> > + if (wdt->control_reg)
> > + iounmap(wdt->control_reg);
> > + if (wdt->feed_reg)
> > + iounmap(wdt->feed_reg);
> > +
> > + kfree(wdt);
> > + return error;
> > +}
> > +
> > +static __devexit int ts72xx_wdt_remove(struct platform_device *pdev)
> > +{
> > + struct ts72xx_wdt *wdt =
> > + (struct ts72xx_wdt *)platform_get_drvdata(pdev);
> > +
> > + if (wdt->control_reg)
> > + iounmap(wdt->control_reg);
> > + if (wdt->feed_reg)
> > + iounmap(wdt->feed_reg);
> > +
> > + kfree(wdt);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > + return misc_deregister(&ts72xx_wdt_miscdev);
> > +}
> > +
> > +static struct platform_driver ts72xx_wdt_driver = {
> > + .probe = ts72xx_wdt_probe,
> > + .remove = __devexit_p(ts72xx_wdt_remove),
> > + .driver = {
> > + .name = "ts72xx-wdt",
> > + },
> > +};
> > +
> > +static __init int ts72xx_wdt_init(void)
> > +{
> > + return platform_driver_register(&ts72xx_wdt_driver);
> > +}
> > +module_init(ts72xx_wdt_init);
> > +
> > +static __exit void ts72xx_wdt_exit(void)
> > +{
> > + platform_driver_unregister(&ts72xx_wdt_driver);
> > +}
> > +module_exit(ts72xx_wdt_exit);
> > +
> > +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@iki.fi>");
> > +MODULE_DESCRIPTION("TS-72xx SBC Watchdog");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:ts72xx-wdt");
>
>
> --
> Bluewater Systems Ltd - ARM Technology Solution Centre
>
> Ryan Mallon 5 Amuri Park, 404 Barbadoes St
> ryan at bluewatersys.com PO Box 13 889, Christchurch 8013
> http://www.bluewatersys.com New Zealand
> Phone: +64 3 3779127 Freecall: Australia 1800 148 751
> Fax: +64 3 3779135 USA 1800 261 2934
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-03 19:35 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Mika Westerberg
2009-12-03 19:35 ` [PATCH 2/2] ep93xx: added platform side support for TS-72xx WDT driver Mika Westerberg
2009-12-03 20:07 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Ryan Mallon
@ 2009-12-04 10:28 ` Alexander Clouter
2009-12-04 11:33 ` Mika Westerberg
2009-12-04 17:13 ` H Hartley Sweeten
3 siblings, 1 reply; 11+ messages in thread
From: Alexander Clouter @ 2009-12-04 10:28 UTC (permalink / raw)
To: linux-arm-kernel
Mika Westerberg <mika.westerberg@iki.fi> wrote:
>
> Technologic Systems TS-72xx SBCs have external glue logic
> CPLD which includes watchdog timer. This driver implements
> kernel support for that.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> ---
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ts72xx_wdt.c | 458 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+), 0 deletions(-)
> create mode 100644 drivers/watchdog/ts72xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3711b88..5204612 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -289,6 +289,17 @@ config ADX_WATCHDOG
> Say Y here if you want support for the watchdog timer on Avionic
> Design Xanthos boards.
>
> +config TS72XX_WATCHDOG
> + tristate "TS-72XX SBC Watchdog"
> + depends on MACH_TS72XX
> + help
> + Technologic Systems TS-7200, TS-7250 and TS-7260 boards have
> + watchdog timer implemented in a external CPLD chip. Say Y here
> + if you want to support for the watchdog timer on TS-72XX boards.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ts72xx_wdt.
> +
> # AVR32 Architecture
>
....and also on the TS-78XX boards. Although my TS-7800 board is being
neglected at the moment it would be great if you could replace
everywhere you put ts72xx with ts7xxx and such. Means when I add the
platform hooks for the TS-78xx I do not have to submit a patch that is a
rather intrusive rename/relabelling one.
Cheers
--
Alexander Clouter
.sigmonster says: Monday is an awful way to spend one seventh of your life.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-04 10:28 ` Alexander Clouter
@ 2009-12-04 11:33 ` Mika Westerberg
2009-12-04 11:45 ` Alexander Clouter
0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2009-12-04 11:33 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 04, 2009 at 10:28:21AM +0000, Alexander Clouter wrote:
> ....and also on the TS-78XX boards. Although my TS-7800 board is being
> neglected at the moment it would be great if you could replace
> everywhere you put ts72xx with ts7xxx and such. Means when I add the
> platform hooks for the TS-78xx I do not have to submit a patch that is a
> rather intrusive rename/relabelling one.
Hi,
Is the TS-7800 watchdog timer similar than the ones in TS-72xx SBCs? I couldn't
find any documentation about that in TS web site. If this is the case then
I would be more than happy to do this stuff.
Thanks,
MW
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-04 11:33 ` Mika Westerberg
@ 2009-12-04 11:45 ` Alexander Clouter
2009-12-04 11:49 ` Alexander Clouter
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Clouter @ 2009-12-04 11:45 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
* Mika Westerberg <mika.westerberg@iki.fi> [2009-12-04 13:33:58+0200]:
>
> On Fri, Dec 04, 2009 at 10:28:21AM +0000, Alexander Clouter wrote:
>
> > ....and also on the TS-78XX boards. Although my TS-7800 board is being
> > neglected at the moment it would be great if you could replace
> > everywhere you put ts72xx with ts7xxx and such. Means when I add the
> > platform hooks for the TS-78xx I do not have to submit a patch that is a
> > rather intrusive rename/relabelling one.
>
> Is the TS-7800 watchdog timer similar than the ones in TS-72xx SBCs? I couldn't
> find any documentation about that in TS web site. If this is the case then
> I would be more than happy to do this stuff.
>
TS made the FPGA/AVR code/hardware more or less identical to retain
backwards compatibility. Obviously the platform specific resource maps
might be a little different but that is all. The NAND, for example, is
also identical on both, the TS-7800 however adds a hardware ECC and
option to allow DMA transfers.
The details are only really documented in the source code of a tool TS
provide:
http://svn.lee.org/swarm/trunk/spu/src/dillo7800/root/ts7800ctl.c
Cheers
--
Alexander Clouter
.sigmonster says: Now I understand the meaning of "THE MOD SQUAD"!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-04 11:45 ` Alexander Clouter
@ 2009-12-04 11:49 ` Alexander Clouter
2009-12-04 12:00 ` Mika Westerberg
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Clouter @ 2009-12-04 11:49 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
* Alexander Clouter <alex@digriz.org.uk> [2009-12-04 11:45:07+0000]:
>
> * Mika Westerberg <mika.westerberg@iki.fi> [2009-12-04 13:33:58+0200]:
> >
> > On Fri, Dec 04, 2009 at 10:28:21AM +0000, Alexander Clouter wrote:
> >
> > > ....and also on the TS-78XX boards. Although my TS-7800 board is being
> > > neglected at the moment it would be great if you could replace
> > > everywhere you put ts72xx with ts7xxx and such. Means when I add the
> > > platform hooks for the TS-78xx I do not have to submit a patch that is a
> > > rather intrusive rename/relabelling one.
> >
> > Is the TS-7800 watchdog timer similar than the ones in TS-72xx SBCs? I couldn't
> > find any documentation about that in TS web site. If this is the case then
> > I would be more than happy to do this stuff.
> >
> TS made the FPGA/AVR code/hardware more or less identical to retain
> backwards compatibility. Obviously the platform specific resource maps
> might be a little different but that is all. The NAND, for example, is
> also identical on both, the TS-7800 however adds a hardware ECC and
> option to allow DMA transfers.
>
> The details are only really documented in the source code of a tool TS
> provide:
>
> http://svn.lee.org/swarm/trunk/spu/src/dillo7800/root/ts7800ctl.c
>
Hmmmm, on closer reinspection, the AVR hides behind an I2C bus so
probably worth forgetting the renaming suggestion.
Cheers
--
Alexander Clouter
.sigmonster says: There Is No Cabal.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-04 11:49 ` Alexander Clouter
@ 2009-12-04 12:00 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2009-12-04 12:00 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 04, 2009 at 11:49:50AM +0000, Alexander Clouter wrote:
> Hi,
>
> * Alexander Clouter <alex@digriz.org.uk> [2009-12-04 11:45:07+0000]:
> >
> > * Mika Westerberg <mika.westerberg@iki.fi> [2009-12-04 13:33:58+0200]:
> > >
> > > On Fri, Dec 04, 2009 at 10:28:21AM +0000, Alexander Clouter wrote:
> > >
> > > > ....and also on the TS-78XX boards. Although my TS-7800 board is being
> > > > neglected at the moment it would be great if you could replace
> > > > everywhere you put ts72xx with ts7xxx and such. Means when I add the
> > > > platform hooks for the TS-78xx I do not have to submit a patch that is a
> > > > rather intrusive rename/relabelling one.
> > >
> > > Is the TS-7800 watchdog timer similar than the ones in TS-72xx SBCs? I couldn't
> > > find any documentation about that in TS web site. If this is the case then
> > > I would be more than happy to do this stuff.
> > >
> > TS made the FPGA/AVR code/hardware more or less identical to retain
> > backwards compatibility. Obviously the platform specific resource maps
> > might be a little different but that is all. The NAND, for example, is
> > also identical on both, the TS-7800 however adds a hardware ECC and
> > option to allow DMA transfers.
> >
> > The details are only really documented in the source code of a tool TS
> > provide:
> >
> > http://svn.lee.org/swarm/trunk/spu/src/dillo7800/root/ts7800ctl.c
> >
> Hmmmm, on closer reinspection, the AVR hides behind an I2C bus so
> probably worth forgetting the renaming suggestion.
OK. Thanks for the information. I'll keep it as ts72xx for now.
MW
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs
2009-12-03 19:35 ` [PATCH 1/2] ep93xx: implemented watchdog timer driver for TS-72xx SBCs Mika Westerberg
` (2 preceding siblings ...)
2009-12-04 10:28 ` Alexander Clouter
@ 2009-12-04 17:13 ` H Hartley Sweeten
3 siblings, 0 replies; 11+ messages in thread
From: H Hartley Sweeten @ 2009-12-04 17:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, December 03, 2009 12:35 PM, Mika Westerberg wrote:
> Technologic Systems TS-72xx SBCs have external glue logic
> CPLD which includes watchdog timer. This driver implements
> kernel support for that.
Hello Mika,
A couple comments in addition to the ones Ryan already mentioned.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> ---
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ts72xx_wdt.c | 458 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+), 0 deletions(-)
> create mode 100644 drivers/watchdog/ts72xx_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3711b88..5204612 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -289,6 +289,17 @@ config ADX_WATCHDOG
> Say Y here if you want support for the watchdog timer on Avionic
> Design Xanthos boards.
>
> +config TS72XX_WATCHDOG
> + tristate "TS-72XX SBC Watchdog"
> + depends on MACH_TS72XX
> + help
> + Technologic Systems TS-7200, TS-7250 and TS-7260 boards have
> + watchdog timer implemented in a external CPLD chip. Say Y here
> + if you want to support for the watchdog timer on TS-72XX boards.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ts72xx_wdt.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 699199b..8e8a9b4 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
> obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
> obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
> +obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/ts72xx_wdt.c b/drivers/watchdog/ts72xx_wdt.c
> new file mode 100644
> index 0000000..88b75b9
> --- /dev/null
> +++ b/drivers/watchdog/ts72xx_wdt.c
> @@ -0,0 +1,458 @@
> +/*
> + * Watchdog driver for Technologic Systems TS-72xx based SBCs
> + * (TS-7200, TS-7250 and TS-7260). These boards have external
> + * glue logic CPLD chip, which includes programmable watchdog
> + * timer.
> + *
> + * Copyright (c) 2009 Mika Westerberg <mika.westerberg@iki.fi>
> + *
> + * This driver is based on ep93xx_wdt and wm831x_wdt drivers.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +
> +#define TS72XX_WDT_FEED_VAL 0x05
> +#define TS72XX_WDT_DEFAULT_TIMEOUT 8
> +
> +static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
> + "(1 <= timeout <= 8, default="
> + __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
> + ")");
> +
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
> +
> +/**
> + * struct ts72xx_wdt - watchdog control structure
> + * @lock: lock that protects this structure
> + * @flags: flags controlling watchdog device state
> + * @control_reg: watchdog control register
> + * @feed_reg: watchdog feed register
> + * @pdev: back pointer to platform dev
> + */
> +struct ts72xx_wdt {
> + struct mutex lock;
> + int timeout;
> +
> +#define TS72XX_WDT_BUSY_FLAG 1
> +#define TS72XX_WDT_EXPECT_CLOSE_FLAG 2
> + int flags;
> +
> + void __iomem *control_reg;
> + void __iomem *feed_reg;
> +
> + struct platform_device *pdev;
> +};
> +
> +struct platform_device *ts72xx_wdt_pdev;
> +
> +/*
> + * TS-72xx Watchdog supports following timeouts (value written
> + * to control register):
> + * value description
> + * -------------------------
> + * 0x00 watchdog disabled
> + * 0x01 250ms
> + * 0x02 500ms
> + * 0x03 1s
> + * 0x04 reserved
> + * 0x05 2s
> + * 0x06 4s
> + * 0x07 8s
> + *
> + * Timeouts below 1s are not very usable so we don't
> + * allow them at all.
> + */
> +static const struct {
> + int timeout;
> + u8 ctrval;
> +} ts72xx_wdt_map[] = {
> + { 1, 3 },
> + { 2, 5 },
> + { 4, 6 },
> + { 8, 7 },
> +};
> +
> +/**
> + * normalize_timeout() - normalizes given timeout
> + * @new_timeout: timeout in seconds to be normalized
> + *
> + * This function normalizes given timeout value (in seconds)
> + * to value that is valid to TS-72xx watchdog timer. Valid
> + * values are 1, 2, 4 and 8 seconds. Timeout is rounded up
> + * to nearest valid timeout so for example value 3 yields
> + * 4 etc.
> + */
> +static int normalize_timeout(int new_timeout)
> +{
> + int i;
> +
> + /* first limit it to 1 - 8 seconds */
> + if (new_timeout < 1)
> + new_timeout = 1;
> + else if (new_timeout > 8)
> + new_timeout = 8;
> +
> + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> + if (ts72xx_wdt_map[i].timeout >= new_timeout)
> + return ts72xx_wdt_map[i].timeout;
> + }
> +
> + BUG();
> + /*NOTREACHED*/
> +}
> +
> +/**
> + * timeout_to_ctrval() - converts given timeout to control register value
> + * @new_timeout: timeout in seconds to be converted (1, 2, 4 or 8)
> + *
> + * This function converts timeout in seconds to valid control register
> + * value. Note that @new_timeout must be normalized first with call to
> + * normalize_timeout().
> + */
> +static u8 timeout_to_ctrval(int new_timeout)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
> + if (ts72xx_wdt_map[i].timeout == new_timeout)
> + return ts72xx_wdt_map[i].ctrval;
> + }
> +
> + BUG();
> + /*NOTREACHED*/
> +}
> +
> +/**
> + * ts72xx_wdt_kick() - kick the watchdog
> + * @wdt: watchdog to be kicked
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_kick(struct ts72xx_wdt *wdt)
> +{
> + __raw_writeb(TS72XX_WDT_FEED_VAL, wdt->feed_reg);
> +}
> +
> +/**
> + * ts72xx_wdt_start() - starts the watchdog timer
> + * @wdt: watchdog to be started
> + *
> + * This function programs timeout to watchdog timer
> + * and starts it.
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_start(struct ts72xx_wdt *wdt)
> +{
> + /*
> + * To program the wdt, it first must be "fed" and
> + * only after that (within 30 usecs) the configuration
> + * can be changed.
> + */
> + ts72xx_wdt_kick(wdt);
> + __raw_writeb(timeout_to_ctrval(wdt->timeout), wdt->control_reg);
> +}
> +
> +/**
> + * ts72xx_wdt_stop() - stops the watchdog timer
> + * @wdt: watchdog to be stopped
> + *
> + * Called with @wdt->lock held.
> + */
> +static void ts72xx_wdt_stop(struct ts72xx_wdt *wdt)
> +{
> + ts72xx_wdt_kick(wdt);
> + __raw_writeb(0, wdt->control_reg);
> +}
> +
> +static int ts72xx_wdt_open(struct inode *inode, struct file *file)
> +{
> + struct ts72xx_wdt *wdt = platform_get_drvdata(ts72xx_wdt_pdev);
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + if ((wdt->flags & TS72XX_WDT_BUSY_FLAG) != 0) {
> + mutex_unlock(&wdt->lock);
> + return -EBUSY;
> + }
> +
> + wdt->flags = TS72XX_WDT_BUSY_FLAG;
> + wdt->timeout = normalize_timeout(timeout);
> + file->private_data = wdt;
> +
> + ts72xx_wdt_start(wdt);
> +
> + mutex_unlock(&wdt->lock);
> + return nonseekable_open(inode, file);
> +}
> +
> +static int ts72xx_wdt_release(struct inode *inode, struct file *file)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
The cast is not needed, file->private_data is a void *.
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + if ((wdt->flags & TS72XX_WDT_EXPECT_CLOSE_FLAG) != 0) {
> + ts72xx_wdt_stop(wdt);
> + } else {
> + dev_warn(&wdt->pdev->dev,
> + "TS-72XX WDT device closed unexpectly. "
> + "Watchdog timer will not stop!\n");
> + /*
> + * Kick it one more time, to give userland some time
> + * to recover (for example, respawning the kicker
> + * daemon).
> + */
> + ts72xx_wdt_kick(wdt);
> + }
> +
> + wdt->flags = 0;
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static ssize_t ts72xx_wdt_write(struct file *file,
> + const char __user *data,
> + size_t len,
> + loff_t *ppos)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
Same here.
> +
> + if (!len)
> + return 0;
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + ts72xx_wdt_kick(wdt);
> +
> + /*
> + * Support for magic character closing. User process
> + * writes 'V' into the device, just before it is closed.
> + * This means that we know that the wdt timer can be
> + * stopped after user closes the device.
> + */
> + if (!nowayout) {
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + char c;
> +
> + /* In case it was set long ago */
> + wdt->flags &= ~TS72XX_WDT_EXPECT_CLOSE_FLAG;
> +
> + if (get_user(c, data + i)) {
> + mutex_unlock(&wdt->lock);
> + return -EFAULT;
> + }
> + if (c == 'V') {
> + wdt->flags |= TS72XX_WDT_EXPECT_CLOSE_FLAG;
> + break;
> + }
> + }
> + }
> +
> + mutex_unlock(&wdt->lock);
> + return len;
> +}
> +
> +static const struct watchdog_info winfo = {
> + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> + .firmware_version = 1,
> + .identity = "TS-72XX WDT",
> +};
> +
> +static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct ts72xx_wdt *wdt = (struct ts72xx_wdt *)file->private_data;
And here.
> + void __user *argp = (void __user *)arg;
> + int __user *p = (int __user *)argp;
> + int error = 0;
> +
> + if (mutex_lock_interruptible(&wdt->lock))
> + return -ERESTARTSYS;
> +
> + switch (cmd) {
> + case WDIOC_GETSUPPORT:
> + error = copy_to_user(argp, &winfo, sizeof(winfo));
> + break;
> +
> + case WDIOC_KEEPALIVE:
> + ts72xx_wdt_kick(wdt);
> + break;
> +
> + case WDIOC_SETOPTIONS: {
> + int options;
> +
> + if (get_user(options, p)) {
> + error = -EFAULT;
> + break;
> + }
> +
> + error = -EINVAL;
> +
> + if ((options & WDIOS_DISABLECARD) != 0) {
> + ts72xx_wdt_stop(wdt);
> + error = 0;
> + }
> + if ((options & WDIOS_ENABLECARD) != 0) {
> + ts72xx_wdt_start(wdt);
> + error = 0;
> + }
> +
> + break;
> + }
> +
> + case WDIOC_SETTIMEOUT: {
> + int new_timeout;
> +
> + if (get_user(new_timeout, p)) {
> + error = -EFAULT;
> + } else {
> + ts72xx_wdt_stop(wdt);
> + wdt->timeout = normalize_timeout(new_timeout);
> + ts72xx_wdt_start(wdt);
> + }
> + /*FALLTHROUGH*/
> + }
> +
> + case WDIOC_GETTIMEOUT:
> + if (put_user(wdt->timeout, p))
> + error = -EFAULT;
> + break;
> +
> + default:
> + error = -ENOTTY;
> + break;
> + }
> +
> + mutex_unlock(&wdt->lock);
> + return error;
> +}
> +
> +static const struct file_operations ts72xx_wdt_fops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .open = ts72xx_wdt_open,
> + .release = ts72xx_wdt_release,
> + .write = ts72xx_wdt_write,
> + .unlocked_ioctl = ts72xx_wdt_ioctl,
> +};
> +
> +static struct miscdevice ts72xx_wdt_miscdev = {
> + .minor = WATCHDOG_MINOR,
> + .name = "watchdog",
> + .fops = &ts72xx_wdt_fops,
> +};
> +
> +static __devinit int ts72xx_wdt_probe(struct platform_device *pdev)
> +{
> + struct ts72xx_wdt *wdt;
> + struct resource *res;
> + int error = -EIO;
> +
> + wdt = kzalloc(sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + mutex_init(&wdt->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + goto fail;
> +
> + wdt->control_reg = ioremap(res->start, resource_size(res));
> + if (!wdt->control_reg)
> + goto fail;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + goto fail;
> +
> + wdt->feed_reg = ioremap(res->start, resource_size(res));
> + if (!wdt->feed_reg)
> + goto fail;
> +
Please add a request_mem_region() before doing the ioremap()'s.
> + error = misc_register(&ts72xx_wdt_miscdev);
> + if (error)
> + goto fail;
> +
> + platform_set_drvdata(pdev, wdt);
> + ts72xx_wdt_pdev = pdev;
> + wdt->pdev = pdev;
> +
> + dev_info(&pdev->dev, "TS-72xx Watchdog driver\n");
> +
> + return 0;
> +
> +fail:
> + platform_set_drvdata(pdev, NULL);
> +
> + if (wdt->control_reg)
> + iounmap(wdt->control_reg);
> + if (wdt->feed_reg)
> + iounmap(wdt->feed_reg);
Also, add a release_mem_region() after each iounmap().
> +
> + kfree(wdt);
> + return error;
> +}
> +
> +static __devexit int ts72xx_wdt_remove(struct platform_device *pdev)
> +{
> + struct ts72xx_wdt *wdt =
> + (struct ts72xx_wdt *)platform_get_drvdata(pdev);
Unnecessary cast.
> +
> + if (wdt->control_reg)
> + iounmap(wdt->control_reg);
> + if (wdt->feed_reg)
> + iounmap(wdt->feed_reg);
release_mem_region() here also.
> +
> + kfree(wdt);
> +
> + platform_set_drvdata(pdev, NULL);
> + return misc_deregister(&ts72xx_wdt_miscdev);
The misc_deregister() needs to happen before the kfree() to prevent
any race conditions.
> +}
> +
> +static struct platform_driver ts72xx_wdt_driver = {
> + .probe = ts72xx_wdt_probe,
> + .remove = __devexit_p(ts72xx_wdt_remove),
> + .driver = {
> + .name = "ts72xx-wdt",
> + },
> +};
Nitpick: Tab align please.
> +
> +static __init int ts72xx_wdt_init(void)
> +{
> + return platform_driver_register(&ts72xx_wdt_driver);
> +}
> +module_init(ts72xx_wdt_init);
> +
> +static __exit void ts72xx_wdt_exit(void)
> +{
> + platform_driver_unregister(&ts72xx_wdt_driver);
> +}
> +module_exit(ts72xx_wdt_exit);
> +
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@iki.fi>");
> +MODULE_DESCRIPTION("TS-72xx SBC Watchdog");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ts72xx-wdt");
^ permalink raw reply [flat|nested] 11+ messages in thread