Re: [PATCH] Added MIPS RM9K watchdog driver

Re: [PATCH] Added MIPS RM9K watchdog driver

[Wim Van Sebroeck]

Hi All,

Can you all review the following watchdog driver? I want to include
it in my linux-2.6-watchdog-mm tree, but would appreciate it if you
could have all a look at it again.

Thomas: I moved start and stop code into seperate functions. I also
deleted the #include <rm9k_wdt.h> because the file doesn't exist.
Note that the shutdown function of the platform_device_driver is
similar to a reboot notifier.

Thanks in advance,
Wim.

--------------------------------------------------------------------------------
/*
 *  Watchdog implementation for GPI h/w found on PMC-Sierra RM9xxx
 *  chips.
 *
 *  Copyright (C) 2004 by Basler Vision Technologies AG
 *  Author: Thomas Koeller <thomas.koeller@baslerweb.com>
 *
 *  This program is free software; you can redistribute it and/or modify
 *  it under the terms of the GNU General Public License as published by
 *  the Free Software Foundation; either version 2 of the License, or
 *  (at your option) any later version.
 *
 *  This program is distributed in the hope that 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.
 *
 *  You should have received a copy of the GNU General Public License
 *  along with this program; if not, write to the Free Software
 *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */

#include <linux/platform_device.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/interrupt.h>
#include <linux/fs.h>
#include <linux/reboot.h>
#include <linux/notifier.h>
#include <linux/miscdevice.h>
#include <linux/watchdog.h>
#include <asm/io.h>
#include <asm/atomic.h>
#include <asm/processor.h>
#include <asm/uaccess.h>
#include <asm/system.h>
#include <asm/rm9k-ocd.h>


#define CLOCK                  125000000
#define MAX_TIMEOUT_SECONDS    32
#define CPCCR                  0x0080
#define CPGIG1SR               0x0044
#define CPGIG1ER               0x0054



/* Function prototypes */
static int __init wdt_gpi_probe(struct device *);
static int __exit wdt_gpi_remove(struct device *);
static void wdt_gpi_start(void);
static void wdt_gpi_stop(void);
static void wdt_gpi_set_timeout(unsigned int);
static int wdt_gpi_open(struct inode *, struct file *);
static int wdt_gpi_release(struct inode *, struct file *);
static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, loff_t *);
static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);
static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);




static const char wdt_gpi_name[] = "wdt_gpi";
static atomic_t opencnt;
static int expect_close;
static int locked = 0;



/* These are set from device resources */
static void __iomem * wd_regs;
static unsigned int wd_irq, wd_ctr;



/* Module arguments */
static int timeout = MAX_TIMEOUT_SECONDS;
module_param(timeout, int, 0444);
static unsigned long resetaddr = 0xbffdc200;
module_param(resetaddr, ulong, 0444);
static unsigned long flagaddr = 0xbffdc104;
module_param(flagaddr, ulong, 0444);
static int powercycle = 0;
module_param(powercycle, bool, 0444);

static int nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0444);



static struct file_operations fops = {
	.owner		= THIS_MODULE,
	.open		= wdt_gpi_open,
	.release	= wdt_gpi_release,
	.write		= wdt_gpi_write,
	.unlocked_ioctl	= wdt_gpi_ioctl,
};

static struct miscdevice miscdev = {
	.minor		= WATCHDOG_MINOR,
	.name		= wdt_gpi_name,
	.fops		= &fops,
};

static struct device_driver wdt_gpi_driver = {
	.name		= (char *) wdt_gpi_name,
	.bus		= &platform_bus_type,
	.owner		= THIS_MODULE,
	.probe		= wdt_gpi_probe,
	.remove		= __exit_p(wdt_gpi_remove),
	.shutdown	= NULL,
	.suspend	= NULL,
	.resume		= NULL,
};

static struct notifier_block wdt_gpi_shutdown = {
	.notifier_call	= wdt_gpi_notify,
};



static const struct resource *
wdt_gpi_get_resource(struct platform_device *pdv, const char *name,
		      unsigned int type)
{
	char buf[80];
	if (snprintf(buf, sizeof buf, "%s_0", name) >= sizeof buf)
		return NULL;
	return platform_get_resource_byname(pdv, type, buf);
}



/* No hotplugging on the platform bus - use __init */
static int __init wdt_gpi_probe(struct device *dev)
{
	int res;
	struct platform_device * const pdv = to_platform_device(dev);
	const struct resource
		* const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
						  IORESOURCE_MEM),
		* const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
						  IORESOURCE_IRQ),
		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,
						  0);

	if (unlikely(!rr || !ri || !rc))
		return -ENXIO;

	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);
	if (unlikely(!wd_regs))
		return -ENOMEM;
	wd_irq = ri->start;
	wd_ctr = rc->start;
	res = misc_register(&miscdev);
	if (res)
		iounmap(wd_regs);
	else
		register_reboot_notifier(&wdt_gpi_shutdown);
	return res;
}



static int __exit wdt_gpi_remove(struct device *dev)
{
	int res;

	unregister_reboot_notifier(&wdt_gpi_shutdown);
	res = misc_deregister(&miscdev);
	iounmap(wd_regs);
	wd_regs = NULL;
	return res;
}


static void wdt_gpi_start(void)
{
	u32 reg;

	lock_titan_regs();
	reg = titan_readl(CPGIG1ER);
	titan_writel(reg | (0x100 << wd_ctr), CPGIG1ER);
	iob();
	unlock_titan_regs();
}

static void wdt_gpi_stop(void)
{
	u32 reg;

	lock_titan_regs();
	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
	titan_writel(reg, CPCCR);
	reg = titan_readl(CPGIG1ER);
	titan_writel(reg & ~(0x100 << wd_ctr), CPGIG1ER);
	iob();
	unlock_titan_regs();
}

static void wdt_gpi_set_timeout(unsigned int to)
{
	u32 reg;
	const u32 wdval = (to * CLOCK) & ~0x0000000f;

	lock_titan_regs();
	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
	titan_writel(reg, CPCCR);
	wmb();
	__raw_writel(wdval, wd_regs + 0x0000);
	wmb();
	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
	wmb();
	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
	iob();
	unlock_titan_regs();
}



static int wdt_gpi_open(struct inode *inode, struct file *file)
{
	int res;

	if (unlikely(0 > atomic_dec_if_positive(&opencnt)))
		return -EBUSY;

	expect_close = 0;
	if (locked) {
		module_put(THIS_MODULE);
		free_irq(wd_irq, &miscdev);
		locked = 0;
	}

	res = request_irq(wd_irq, wdt_gpi_irqhdl, SA_SHIRQ | SA_INTERRUPT,
			  wdt_gpi_name, &miscdev);
	if (unlikely(res))
		return res;

	wdt_gpi_set_timeout(timeout);
	wdt_gpi_start();

	printk(KERN_INFO "%s: watchdog started, timeout = %u seconds\n",
		wdt_gpi_name, timeout);
	return nonseekable_open(inode, file);
}



static int wdt_gpi_release(struct inode *i, struct file *f)
{
	if (nowayout) {
		printk(KERN_NOTICE "%s: no way out - watchdog left running\n",
			wdt_gpi_name);
		__module_get(THIS_MODULE);
		locked = 1;
	} else {
		if (expect_close) {
			wdt_gpi_stop();
			free_irq(wd_irq, &miscdev);
			printk(KERN_INFO "%s: watchdog stopped\n", wdt_gpi_name);
		} else {
			printk(KERN_NOTICE "%s: unexpected close() -"
				" watchdog left running\n",
				wdt_gpi_name);
			wdt_gpi_set_timeout(timeout);
			__module_get(THIS_MODULE);
			locked = 1;
		}
	}

	atomic_inc(&opencnt);
	return 0;
}



static ssize_t
wdt_gpi_write(struct file *f, const char __user *d, size_t s, loff_t *o)
{
	char val;

	wdt_gpi_set_timeout(timeout);
	expect_close = (s > 0) && !get_user(val, d) && (val == 'V');
	return s ? 1 : 0;
}



static long
wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
	long res = -ENOTTY;
	const long size = _IOC_SIZE(cmd);
	int stat;
	static struct watchdog_info wdinfo = {
		.identity               = "RM9xxx/GPI watchdog",
		.firmware_version       = 0,
		.options                = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
	};

	if (unlikely(_IOC_TYPE(cmd) != WATCHDOG_IOCTL_BASE))
		return -ENOTTY;

	if ((_IOC_DIR(cmd) & _IOC_READ)
	    && !access_ok(VERIFY_WRITE, arg, size))
		return -EFAULT;

	if ((_IOC_DIR(cmd) & _IOC_WRITE)
	    && !access_ok(VERIFY_READ, arg, size))
		return -EFAULT;

	expect_close = 0;

	switch (cmd) {
	case WDIOC_GETSUPPORT:
		wdinfo.options = nowayout ?
			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING :
			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE;
		res = __copy_to_user((void __user *)arg, &wdinfo, size) ?
			-EFAULT : size;
		break;

	case WDIOC_GETSTATUS:
		break;

	case WDIOC_GETBOOTSTATUS:
		stat = (*(volatile char *) flagaddr & 0x01)
			? WDIOF_CARDRESET : 0;
		res = __copy_to_user((void __user *)arg, &stat, size) ?
			-EFAULT : size;
		break;

	case WDIOC_SETOPTIONS:
		break;

	case WDIOC_KEEPALIVE:
		wdt_gpi_set_timeout(timeout);
		res = size;
		break;

	case WDIOC_SETTIMEOUT:
		{
			int val;
			if (unlikely(__copy_from_user(&val, (const void __user *) arg,
					size))) {
				res = -EFAULT;
				break;
			}

			if (val > 32)
				val = 32;
			timeout = val;
			wdt_gpi_set_timeout(val);
			res = size;
			printk("%s: timeout set to %u seconds\n",
				wdt_gpi_name, timeout);
		}
		break;

	case WDIOC_GETTIMEOUT:
		res = __copy_to_user((void __user *) arg, &timeout, size) ?
			-EFAULT : size;
		break;
	}

	return res;
}




static irqreturn_t wdt_gpi_irqhdl(int irq, void *ctxt, struct pt_regs *regs)
{
	if (!unlikely(__raw_readl(wd_regs + 0x0008) & 0x1))
		return IRQ_NONE;
	__raw_writel(0x1, wd_regs + 0x0008);


	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
		wdt_gpi_name);

	*(volatile char *) flagaddr |= 0x01;
	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
	iob();
	while (1)
		cpu_relax();
}



static int
wdt_gpi_notify(struct notifier_block *this, unsigned long code, void *unused)
{
	if(code == SYS_DOWN || code == SYS_HALT) {
		wdt_gpi_stop();
	}
	return NOTIFY_DONE;
}



static int __init wdt_gpi_init_module(void)
{
	atomic_set(&opencnt, 1);
	if (timeout > MAX_TIMEOUT_SECONDS)
		timeout = MAX_TIMEOUT_SECONDS;
	return driver_register(&wdt_gpi_driver);
}



static void __exit wdt_gpi_cleanup_module(void)
{
	driver_unregister(&wdt_gpi_driver);
}

module_init(wdt_gpi_init_module);
module_exit(wdt_gpi_cleanup_module);



MODULE_AUTHOR("Thomas Koeller <thomas.koeller@baslerweb.com>");
MODULE_DESCRIPTION("Basler eXcite watchdog driver for gpi devices");
MODULE_VERSION("0.1");
MODULE_LICENSE("GPL");
MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
MODULE_PARM_DESC(resetaddr, "Address to write to to force a reset");
MODULE_PARM_DESC(flagaddr, "Address to write to boot flags to");
MODULE_PARM_DESC(nowayout, "Watchdog cannot be disabled once started");
MODULE_PARM_DESC(powercycle, "Cycle power if watchdog expires");

Re: [PATCH] Added MIPS RM9K watchdog driver

[Ralf Baechle]

On Wed, Nov 01, 2006 at 07:46:33PM +0100, Wim Van Sebroeck wrote:

> Thomas: I moved start and stop code into seperate functions. I also
> deleted the #include <rm9k_wdt.h> because the file doesn't exist.

You just didn't see it, include/asm-mips/mach-excite/rm9k_wdt.h.

  Ralf

Re: [PATCH] Added MIPS RM9K watchdog driver

[Wim Van Sebroeck]

Hi Ralf,

> > Thomas: I moved start and stop code into seperate functions. I also
> > deleted the #include <rm9k_wdt.h> because the file doesn't exist.
> 
> You just didn't see it, include/asm-mips/mach-excite/rm9k_wdt.h.

Thanks, didn't see it indeed :-). I'll include that again.
Any other remarks still?

Greetings,
Wim.

Re: [PATCH] Added MIPS RM9K watchdog driver

[Sam Ravnborg]

Hi Wim.

Not much of my area so only some nitpicking stuff.

> /*
>  *  Watchdog implementation for GPI h/w found on PMC-Sierra RM9xxx
>  *  chips.
>  *
>  *  Copyright (C) 2004 by Basler Vision Technologies AG
>  *  Author: Thomas Koeller <thomas.koeller@baslerweb.com>
>  *
>  *  This program is free software; you can redistribute it and/or modify
We have COPYING in top level directory. No need to include GPL here.

> #define CPGIG1ER               0x0054
> 
> 
> 
> /* Function prototypes */

Too many empty lines. Get it down to 2. Seen on many places.

> static int __init wdt_gpi_probe(struct device *);
> static int __exit wdt_gpi_remove(struct device *);

> static void wdt_gpi_start(void);
> static void wdt_gpi_stop(void);
> static void wdt_gpi_set_timeout(unsigned int);
3 prototpyes not needed

> static int wdt_gpi_open(struct inode *, struct file *);
> static int wdt_gpi_release(struct inode *, struct file *);
> static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, loff_t *);
> static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
> static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);
80 char coloum
Prototype not needed.

> /* These are set from device resources */
> static void __iomem * wd_regs;
Good to see code annotated.
I assume sparse did not give any warnings on this code?

> /* Module arguments */
> static int timeout = MAX_TIMEOUT_SECONDS;
> module_param(timeout, int, 0444);
> static unsigned long resetaddr = 0xbffdc200;
> module_param(resetaddr, ulong, 0444);
> static unsigned long flagaddr = 0xbffdc104;
> module_param(flagaddr, ulong, 0444);
> static int powercycle = 0;
> module_param(powercycle, bool, 0444);
> 
> static int nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0444);

Locate parameter descriptions clode to parameter definition - not in
 bottom of file.

> static struct device_driver wdt_gpi_driver = {
> 	.name		= (char *) wdt_gpi_name,
If this cast is really needed then struct device_driver ought to be updated?

> static int __init wdt_gpi_probe(struct device *dev)
> {
> 	int res;
> 	struct platform_device * const pdv = to_platform_device(dev);
> 	const struct resource
> 		* const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
> 						  IORESOURCE_MEM),
> 		* const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
> 						  IORESOURCE_IRQ),
> 		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,

Separate variable definition and assignment => nicer code.

> static long
> wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> {

arg is a __user pointer - why hide this fact?

> 		res = __copy_to_user((void __user *)arg, &wdinfo, size) ?
> 			-EFAULT : size;
then you get rid of this cast.
And code this as if () else
Using ?: is just ugly here.

> 	case WDIOC_GETBOOTSTATUS:
> 		stat = (*(volatile char *) flagaddr & 0x01)
> 			? WDIOF_CARDRESET : 0;

Use of volatile almost always indicate a bug.
Please explain why volatile is needed and consider the other options.

> 			printk("%s: timeout set to %u seconds\n",
> 				wdt_gpi_name, timeout);

I just noticed this pringk miss a <KERNEL_DEBUG> or similar specifier.
I guess this is all of them.


> 	if (!unlikely(__raw_readl(wd_regs + 0x0008) & 0x1))
> 		return IRQ_NONE;
> 	__raw_writel(0x1, wd_regs + 0x0008);
Magics suchs as '0x0008' deserve a comment.

> 	*(volatile char *) flagaddr |= 0x01;
> 	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;

volatile again...

> MODULE_AUTHOR("Thomas Koeller <thomas.koeller@baslerweb.com>");
> MODULE_DESCRIPTION("Basler eXcite watchdog driver for gpi devices");
> MODULE_VERSION("0.1");
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
These five tags belongs in the start of the file.

> MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> MODULE_PARM_DESC(resetaddr, "Address to write to to force a reset");
> MODULE_PARM_DESC(flagaddr, "Address to write to boot flags to");
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be disabled once started");
> MODULE_PARM_DESC(powercycle, "Cycle power if watchdog expires");
Same here - but already noted.

	Sam

Re: [PATCH] Added MIPS RM9K watchdog driver

[Randy Dunlap]

On Wed, 1 Nov 2006 19:46:33 +0100 Wim Van Sebroeck wrote:

> /* Function prototypes */
> static int __init wdt_gpi_probe(struct device *);
> static int __exit wdt_gpi_remove(struct device *);
> static void wdt_gpi_start(void);
> static void wdt_gpi_stop(void);
> static void wdt_gpi_set_timeout(unsigned int);
> static int wdt_gpi_open(struct inode *, struct file *);
> static int wdt_gpi_release(struct inode *, struct file *);
> static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, loff_t *);
> static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
> static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);

Some lines too long (stay <= 80 columns).

> static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
> static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);
> 
> 
> 
> 
> static const char wdt_gpi_name[] = "wdt_gpi";
> static atomic_t opencnt;
> static int expect_close;
> static int locked = 0;

Don't need to init to 0.

> /* Module arguments */
> static int timeout = MAX_TIMEOUT_SECONDS;
> module_param(timeout, int, 0444);
> static unsigned long resetaddr = 0xbffdc200;
> module_param(resetaddr, ulong, 0444);
> static unsigned long flagaddr = 0xbffdc104;
> module_param(flagaddr, ulong, 0444);
> static int powercycle = 0;

no need to init to 0.

> module_param(powercycle, bool, 0444);
> 
> static int nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0444);
> 
> 
> 
> /* No hotplugging on the platform bus - use __init */
> static int __init wdt_gpi_probe(struct device *dev)
> {
> 	int res;
> 	struct platform_device * const pdv = to_platform_device(dev);
> 	const struct resource
> 		* const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
> 						  IORESOURCE_MEM),
> 		* const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
> 						  IORESOURCE_IRQ),
> 		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,
> 						  0);
> 
> 	if (unlikely(!rr || !ri || !rc))
> 		return -ENXIO;
> 
> 	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);
> 	if (unlikely(!wd_regs))
> 		return -ENOMEM;

There's no way to return the resources on failure?

> 	wd_irq = ri->start;
> 	wd_ctr = rc->start;
> 	res = misc_register(&miscdev);
> 	if (res)
> 		iounmap(wd_regs);
> 	else
> 		register_reboot_notifier(&wdt_gpi_shutdown);
> 	return res;
> }
> 
> 
> 
> static int wdt_gpi_open(struct inode *inode, struct file *file)
> {
> 	int res;
> 
> 	if (unlikely(0 > atomic_dec_if_positive(&opencnt)))

Style: Instead of
		if (constant op function_or_variable)
we prefer
		if (function_or_variable op constant)


> 		return -EBUSY;
> 
> 	expect_close = 0;
> 	if (locked) {
> 		module_put(THIS_MODULE);
> 		free_irq(wd_irq, &miscdev);
> 		locked = 0;
> 	}
> 
> 	res = request_irq(wd_irq, wdt_gpi_irqhdl, SA_SHIRQ | SA_INTERRUPT,
> 			  wdt_gpi_name, &miscdev);
> 	if (unlikely(res))
> 		return res;
> 
> 	wdt_gpi_set_timeout(timeout);
> 	wdt_gpi_start();
> 
> 	printk(KERN_INFO "%s: watchdog started, timeout = %u seconds\n",
> 		wdt_gpi_name, timeout);
> 	return nonseekable_open(inode, file);
> }
> 
> 
> 
> 
> static long
> wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> {
> 	long res = -ENOTTY;
> 	const long size = _IOC_SIZE(cmd);
> 	int stat;
> 	static struct watchdog_info wdinfo = {
> 		.identity               = "RM9xxx/GPI watchdog",
> 		.firmware_version       = 0,
> 		.options                = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
> 	};
> 
> 	if (unlikely(_IOC_TYPE(cmd) != WATCHDOG_IOCTL_BASE))
> 		return -ENOTTY;
> 
> 	if ((_IOC_DIR(cmd) & _IOC_READ)
> 	    && !access_ok(VERIFY_WRITE, arg, size))
> 		return -EFAULT;
> 
> 	if ((_IOC_DIR(cmd) & _IOC_WRITE)
> 	    && !access_ok(VERIFY_READ, arg, size))
> 		return -EFAULT;
> 
> 	expect_close = 0;
> 
> 	switch (cmd) {
> 	case WDIOC_GETSUPPORT:
> 		wdinfo.options = nowayout ?
> 			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING :
> 			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE;
> 		res = __copy_to_user((void __user *)arg, &wdinfo, size) ?
> 			-EFAULT : size;
> 		break;
> 
> 	case WDIOC_GETSTATUS:
> 		break;
> 
> 	case WDIOC_GETBOOTSTATUS:
> 		stat = (*(volatile char *) flagaddr & 0x01)
> 			? WDIOF_CARDRESET : 0;
> 		res = __copy_to_user((void __user *)arg, &stat, size) ?
> 			-EFAULT : size;
> 		break;
> 
> 	case WDIOC_SETOPTIONS:
> 		break;
> 
> 	case WDIOC_KEEPALIVE:
> 		wdt_gpi_set_timeout(timeout);
> 		res = size;
> 		break;
> 
> 	case WDIOC_SETTIMEOUT:
> 		{
> 			int val;
> 			if (unlikely(__copy_from_user(&val, (const void __user *) arg,
> 					size))) {
> 				res = -EFAULT;
> 				break;
> 			}
> 
> 			if (val > 32)
> 				val = 32;

There's a #defined constant for that "32".
Please use it.

> 			timeout = val;
> 			wdt_gpi_set_timeout(val);
> 			res = size;
> 			printk("%s: timeout set to %u seconds\n",
> 				wdt_gpi_name, timeout);
> 		}
> 		break;
> 
> 	case WDIOC_GETTIMEOUT:
> 		res = __copy_to_user((void __user *) arg, &timeout, size) ?
> 			-EFAULT : size;
> 		break;
> 	}
> 
> 	return res;
> }
> 
> 
> 
> 
> static irqreturn_t wdt_gpi_irqhdl(int irq, void *ctxt, struct pt_regs *regs)
> {
> 	if (!unlikely(__raw_readl(wd_regs + 0x0008) & 0x1))
> 		return IRQ_NONE;
> 	__raw_writel(0x1, wd_regs + 0x0008);
> 
> 
> 	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
> 		wdt_gpi_name);

I would expect a KERN_ALERT or KERN_EMERG or KERN_CRIT there...

> 
> 	*(volatile char *) flagaddr |= 0x01;
> 	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
> 	iob();
> 	while (1)
> 		cpu_relax();
> }
> 
> 
> 
> static int
> wdt_gpi_notify(struct notifier_block *this, unsigned long code, void *unused)
> {
> 	if(code == SYS_DOWN || code == SYS_HALT) {

Space between if and (.

> 		wdt_gpi_stop();
> 	}

No braces around one-statement blocks.

> 	return NOTIFY_DONE;
> }

---
~Randy

Re: [PATCH] Added MIPS RM9K watchdog driver

[Ralf Baechle]

On Wed, Nov 01, 2006 at 10:11:25PM -0800, Randy Dunlap wrote:

> > 	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);
> > 	if (unlikely(!wd_regs))
> > 		return -ENOMEM;
> 
> There's no way to return the resources on failure?

MIPS drivers (and this one is specific to a particular MIPS SOC) are
generally a bit sloopy about checking of return values of ioremap because
ioremap is only doing some address arithmetic but no allocations that
actually could fail.  So for 64-bit kernels or addresses below 0x20000000
on a 32-bit system ioremap cannot fail.  In the same cases ioremap happens
to be a no-op because where nothing was allocated nothing needs to be
freed.

> > 			if (unlikely(__copy_from_user(&val, (const void __user *) arg,

Note to self, __copy_from_user and gang are generally assume to not
return an error so it might be a good idea to move that unlikely() into
the macro definitions.

  Ralf

[PATCH] Added MIPS RM9K watchdog driver

[thomas]

This is a driver for the on-chip watchdog device found on some
MIPS RM9000 processors.

Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
---
 drivers/char/watchdog/Kconfig    |   10 +
 drivers/char/watchdog/Makefile   |    1 
 drivers/char/watchdog/rm9k_wdt.c |  435 
++++++++++++++++++++++++++++++++++++++
 3 files changed, 446 insertions(+), 0 deletions(-)

diff --git a/drivers/char/watchdog/Kconfig b/drivers/char/watchdog/Kconfig
index d53f664..3207299 100644
--- a/drivers/char/watchdog/Kconfig
+++ b/drivers/char/watchdog/Kconfig
@@ -477,6 +477,16 @@ config INDYDOG
 	  timer expired and no process has written to /dev/watchdog during
 	  that time.
 
+config WDT_RM9K_GPI
+	tristate "RM9000/GPI hardware watchdog"
+	depends on WATCHDOG && CPU_RM9000
+	help
+	  Watchdog implementation using the GPI hardware found on
+	  PMC-Sierra RM9xxx CPUs.
+	  
+	  To compile this driver as a module, choose M here: the
+	  module will be called rm9k_wdt.
+
 # S390 Architecture
 
 config ZVM_WATCHDOG
diff --git a/drivers/char/watchdog/Makefile b/drivers/char/watchdog/Makefile
index 6ab77b6..f2a6281 100644
--- a/drivers/char/watchdog/Makefile
+++ b/drivers/char/watchdog/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
 
 # MIPS Architecture
 obj-$(CONFIG_INDYDOG) += indydog.o
+obj-$(CONFIG_WDT_RM9K_GPI) += rm9k_wdt.o
 
 # S390 Architecture
 
diff --git a/drivers/char/watchdog/rm9k_wdt.c 
b/drivers/char/watchdog/rm9k_wdt.c
new file mode 100644
index 0000000..f6a9d17
--- /dev/null
+++ b/drivers/char/watchdog/rm9k_wdt.c
@@ -0,0 +1,435 @@
+/*
+ *  Watchdog implementation for GPI h/w found on PMC-Sierra RM9xxx
+ *  chips.
+ *
+ *  Copyright (C) 2006 by Basler Vision Technologies AG
+ *  Author: Thomas Koeller <thomas.koeller@baslerweb.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that 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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+ 
+#include <linux/config.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/interrupt.h>
+#include <linux/fs.h>
+#include <linux/reboot.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <asm/io.h>
+#include <asm/atomic.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+#include <asm/rm9k-ocd.h>
+
+#include <rm9k_wdt.h>
+
+
+#define CLOCK			125000000
+#define MAX_TIMEOUT_SECONDS	32
+#define CPCCR			0x0080
+#define CPGIG1SR		0x0044
+#define CPGIG1ER		0x0054
+
+
+
+/* Function prototypes */
+static int __init wdt_gpi_probe(struct device *);
+static int __exit wdt_gpi_remove(struct device *);
+static void wdt_gpi_set_timeout(unsigned int);
+static int wdt_gpi_open(struct inode *, struct file *);
+static int wdt_gpi_release(struct inode *, struct file *);
+static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, 
loff_t *);
+static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
+static const struct resource *wdt_gpi_get_resource(struct platform_device *, 
const char *, unsigned int);
+static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
+static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);
+
+
+
+
+static const char wdt_gpi_name[] = "wdt_gpi";
+static atomic_t opencnt;
+static int expect_close;
+static int locked = 0;
+
+
+
+/* These are set from device resources */
+static void __iomem * wd_regs;
+static unsigned int wd_irq, wd_ctr;
+
+
+
+/* Module arguments */
+static int timeout = MAX_TIMEOUT_SECONDS;
+module_param(timeout, int, 444);
+static unsigned long resetaddr = 0xbffdc200;
+module_param(resetaddr, ulong, 444);
+static unsigned long flagaddr = 0xbffdc104;
+module_param(flagaddr, ulong, 444);
+static int powercycle = 0;
+module_param(powercycle, bool, 444);
+
+static int nowayout =
+#if defined(CONFIG_WATCHDOG_NOWAYOUT)
+	1;
+#else
+	0;
+#endif
+module_param(nowayout, bool, 444);
+
+
+
+static struct file_operations fops =
+{
+	.owner		= THIS_MODULE,
+	.open		= wdt_gpi_open,
+	.release	= wdt_gpi_release,
+	.write		= wdt_gpi_write,
+	.unlocked_ioctl	= wdt_gpi_ioctl
+};
+
+static struct miscdevice miscdev =
+{
+	.minor		= WATCHDOG_MINOR,
+	.name		= wdt_gpi_name,
+	.fops		= &fops
+};
+
+static struct device_driver wdt_gpi_driver =
+{
+	.name		= (char *) wdt_gpi_name,
+	.bus		= &platform_bus_type,
+	.owner		= THIS_MODULE,
+	.probe		= wdt_gpi_probe,
+	.remove		= __exit_p(wdt_gpi_remove),
+	.shutdown	= NULL,
+	.suspend	= NULL,
+	.resume		= NULL
+};
+
+static struct notifier_block wdt_gpi_shutdown =
+{
+	wdt_gpi_notify
+};
+
+
+
+static const struct resource *
+wdt_gpi_get_resource(struct platform_device *pdv, const char *name,
+		     unsigned int type)
+{
+	char buf[80];
+	if (snprintf(buf, sizeof buf, "%s_0", name) >= sizeof buf)
+		return NULL;
+	return platform_get_resource_byname(pdv, type, buf);
+}
+
+
+
+/* No hotplugging on the platform bus - use __init */
+static int __init wdt_gpi_probe(struct device *dev)
+{
+	int res;
+	struct platform_device * const pdv = to_platform_device(dev);
+	const struct resource
+		* const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
+						  IORESOURCE_MEM),
+		* const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
+						  IORESOURCE_IRQ),
+		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,
+						  0);
+
+	if (unlikely(!rr || !ri || !rc))
+		return -ENXIO;
+
+	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);
+	wd_irq = ri->start;
+	wd_ctr = rc->start;
+	res = misc_register(&miscdev);
+	if (res)
+		iounmap(wd_regs);
+	register_reboot_notifier(&wdt_gpi_shutdown);
+	return res;
+}
+
+
+
+static int __exit wdt_gpi_remove(struct device *dev)
+{
+	int res;
+
+	unregister_reboot_notifier(&wdt_gpi_shutdown);
+	res = misc_deregister(&miscdev);
+	iounmap(wd_regs);
+	wd_regs = NULL;
+	return res;
+}
+
+
+static void wdt_gpi_set_timeout(unsigned int to)
+{
+	u32 reg;
+	const u32 wdval = (to * CLOCK) & ~0x0000000f;
+
+	lock_titan_regs();
+	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
+	titan_writel(reg, CPCCR);
+	wmb();
+	__raw_writel(wdval, wd_regs + 0x0000);
+	wmb();
+	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
+	wmb();
+	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
+	iob();
+	unlock_titan_regs();
+}
+
+
+
+static int wdt_gpi_open(struct inode *i, struct file *f)
+{
+	int res;
+	u32 reg;
+
+	if (unlikely(0 > atomic_dec_if_positive(&opencnt)))
+		return -EBUSY;
+
+	expect_close = 0;
+	if (locked) {
+		module_put(THIS_MODULE);
+		free_irq(wd_irq, &miscdev);
+		locked = 0;
+	}
+
+	res = request_irq(wd_irq, wdt_gpi_irqhdl, SA_SHIRQ | SA_INTERRUPT,
+			  wdt_gpi_name, &miscdev);
+	if (unlikely(res))
+		return res;
+
+	wdt_gpi_set_timeout(timeout);
+
+	lock_titan_regs();
+	reg = titan_readl(CPGIG1ER);
+	titan_writel(reg | (0x100 << wd_ctr), CPGIG1ER);
+	iob();
+	unlock_titan_regs();
+
+	printk(KERN_INFO "%s: watchdog started, timeout = %u seconds\n",
+	       wdt_gpi_name, timeout);
+	return 0;
+}
+
+
+
+static int wdt_gpi_release(struct inode *i, struct file *f)
+{
+	if (nowayout) {
+		printk(KERN_NOTICE "%s: no way out - watchdog left running\n",
+		       wdt_gpi_name);
+		__module_get(THIS_MODULE);
+		locked = 1;
+	} else {
+		if (expect_close) {
+			u32 reg;
+
+			lock_titan_regs();
+			reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
+			titan_writel(reg, CPCCR);
+			reg = titan_readl(CPGIG1ER);
+			titan_writel(reg & ~(0x100 << wd_ctr), CPGIG1ER);
+			iob();
+			unlock_titan_regs();
+			free_irq(wd_irq, &miscdev);
+			printk(KERN_INFO "%s: watchdog stopped\n", wdt_gpi_name);
+		} else {
+			printk(KERN_NOTICE "%s: unexpected close() -"
+			       " watchdog left running\n",
+			       wdt_gpi_name);
+			wdt_gpi_set_timeout(timeout);
+			__module_get(THIS_MODULE);
+			locked = 1;
+		}
+	}
+
+	atomic_inc(&opencnt);
+	return 0;
+}
+
+
+
+static ssize_t
+wdt_gpi_write(struct file *f, const char __user *d, size_t s, loff_t *o)
+{
+	char val;
+
+	wdt_gpi_set_timeout(timeout);
+	expect_close = (s > 0) && !get_user(val, d) && (val == 'V');
+	return s ? 1 : 0;
+}
+
+
+
+static long
+wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
+{
+	long res = -ENOTTY;
+	const long size = _IOC_SIZE(cmd);
+	int stat;
+	static struct watchdog_info wdinfo = {
+		.identity		= "RM9xxx/GPI watchdog",
+		.firmware_version	= 0,
+		.options		= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
+	};
+
+	if (unlikely(_IOC_TYPE(cmd) != WATCHDOG_IOCTL_BASE))
+		return -ENOTTY;
+
+	if ((_IOC_DIR(cmd) & _IOC_READ)
+	    && !access_ok(VERIFY_WRITE, arg, size))
+	 	return -EFAULT;
+
+	if ((_IOC_DIR(cmd) & _IOC_WRITE)
+	    && !access_ok(VERIFY_READ, arg, size))
+	 	return -EFAULT;
+
+	expect_close = 0;
+
+	switch (cmd) {
+		case WDIOC_GETSUPPORT:
+			wdinfo.options = nowayout ?
+				WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING :
+				WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE;
+			copy_to_user((void __user *)arg, &wdinfo, size);
+			res = size;
+			break;
+
+		case WDIOC_GETSTATUS:
+			break;
+		
+		case WDIOC_GETBOOTSTATUS:
+			stat = (*(volatile char *) flagaddr & 0x01)
+				? WDIOF_CARDRESET : 0;
+			copy_to_user((void __user *)arg, &stat, size);
+			res = size;
+			break;
+
+		case WDIOC_SETOPTIONS:
+			break;
+
+		case WDIOC_KEEPALIVE:
+			wdt_gpi_set_timeout(timeout);
+			res = size;
+			break;
+
+		case WDIOC_SETTIMEOUT:
+			{
+				int val;
+				__copy_from_user(&val, (const void __user *) arg,
+						 size);
+				if (val > 32)
+					val = 32;
+					
+				timeout = val;
+				wdt_gpi_set_timeout(val);
+				res = size;
+				printk("%s: timeout set to %u seconds\n",
+				       wdt_gpi_name, timeout);
+			}
+			break;
+
+		case WDIOC_GETTIMEOUT:
+			__copy_to_user((void __user *) arg, &timeout, size);
+			res = size;
+			break;
+	}
+	
+	return res;
+}
+
+
+
+
+static irqreturn_t wdt_gpi_irqhdl(int irq, void *ctxt, struct pt_regs *regs)
+{
+	if (!unlikely(__raw_readl(wd_regs + 0x0008) & 0x1))
+		return IRQ_NONE;
+	__raw_writel(0x1, wd_regs + 0x0008);
+
+
+	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
+	       wdt_gpi_name);
+
+	*(volatile char *) flagaddr |= 0x01;
+	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
+	iob();
+	while (1) continue;
+
+	return IRQ_HANDLED;
+}
+
+
+
+static int
+wdt_gpi_notify(struct notifier_block *this, unsigned long code, void *unused)
+{
+	if(code == SYS_DOWN || code == SYS_HALT) {
+		u32 reg;
+
+		lock_titan_regs();
+		reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
+		titan_writel(reg, CPCCR);
+		reg = titan_readl(CPGIG1ER);
+		titan_writel(reg & ~(0x100 << wd_ctr), CPGIG1ER);
+		iob();
+		unlock_titan_regs();
+	}
+	return NOTIFY_DONE;
+}
+
+
+
+static int __init wdt_gpi_init_module(void)
+{
+	atomic_set(&opencnt, 1);
+	if (timeout > MAX_TIMEOUT_SECONDS)
+		timeout = MAX_TIMEOUT_SECONDS;
+	return driver_register(&wdt_gpi_driver);
+}
+
+
+
+static void __exit wdt_gpi_cleanup_module(void)
+{
+	driver_unregister(&wdt_gpi_driver);
+}
+
+module_init(wdt_gpi_init_module);
+module_exit(wdt_gpi_cleanup_module);
+
+
+
+MODULE_AUTHOR("Thomas Koeller <thomas.koeller@baslerweb.com>");
+MODULE_DESCRIPTION("Basler eXcite watchdog driver for gpi devices");
+MODULE_VERSION("0.1");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
+MODULE_PARM_DESC(resetaddr, "Address to write to to force a reset");
+MODULE_PARM_DESC(flagaddr, "Address to write to boot flags to");
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be disabled once started");
+MODULE_PARM_DESC(powercycle, "Cycle power if watchdog expires");
-- 
1.4.0


-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] Added MIPS RM9K watchdog driver

[Alan Cox]

Ar Iau, 2006-08-10 am 23:19 +0200, ysgrifennodd
thomas@koeller.dyndns.org:
> +	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);

If this fails ?

> +	res = misc_register(&miscdev);
> +	if (res)
> +		iounmap(wd_regs);
> +	register_reboot_notifier(&wdt_gpi_shutdown);
> +	return res;

Failure path appears incomplete, surely you don't want to register a
reboot notifier then unload and error ?

> +			copy_to_user((void __user *)arg, &wdinfo, size);

This function returns an error and should be checked. (The tricks with
IOC bits and verify_area aren't enough to be sure it won't error and
actually probably aren't worth doing)

> +	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
> +	       wdt_gpi_name);
> +
> +	*(volatile char *) flagaddr |= 0x01;
> +	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
> +	iob();
> +	while (1) continue;

cpu_relax();
> +
> +	return IRQ_HANDLED;

Unreachable code.

Also if this is a software watchdog why is it better than using
softdog ?

Otherwise it looks pretty sound.

Re: [PATCH] Added MIPS RM9K watchdog driver

[Thomas Koeller]

On Friday 11 August 2006 22:07, Alan Cox wrote:
> > +	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
> > +	       wdt_gpi_name);
> > +
> > +	*(volatile char *) flagaddr |= 0x01;
> > +	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
> > +	iob();
> > +	while (1) continue;
>
> cpu_relax();

I tried to find out about the purpose of cpu_relax(). On MIPS, at least,
it maps to barrier(). I do not quite understand why I would need a
barrier() in this place. Would you, or someone else, care to
enlighten me?

I am sending a revised patch in a separate mail.

Thomas
-- 
Thomas Koeller
thomas@koeller.dyndns.org

Re: [PATCH] Added MIPS RM9K watchdog driver

[Ralf Baechle]

On Sat, Aug 12, 2006 at 06:06:02PM +0200, Thomas Koeller wrote:

> > > +	while (1) continue;
> >
> > cpu_relax();
> 
> I tried to find out about the purpose of cpu_relax(). On MIPS, at least,
> it maps to barrier(). I do not quite understand why I would need a
> barrier() in this place. Would you, or someone else, care to
> enlighten me?

Busy wait loops are meant to be filled with cpu_relax() in Linux.  On
processors like the Pentium 4 this expands into something that keeps
the CPU from consuming excessive amounts of energy for just twiddling
thumbs and probably also CPU dependant.  On MIPS cpu_relax() so far is
meaningless and therfore just defined as barrier().

  Ralf

Re: [PATCH] Added MIPS RM9K watchdog driver

[Thomas Koeller]

On Friday 11 August 2006 22:07, Alan Cox wrote:
> Also if this is a software watchdog why is it better than using
> softdog ?
>

This is _not_ a software watchdog. If the timer expires, an interrupt
is generated, and the timer is reset to count through another cycle.
If it expires again, it resets the CPU.

Thomas

-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] Added MIPS RM9K watchdog driver

[Alan Cox]

Ar Sad, 2006-08-12 am 19:45 +0200, ysgrifennodd Thomas Koeller:
> On Friday 11 August 2006 22:07, Alan Cox wrote:
> > Also if this is a software watchdog why is it better than using
> > softdog ?
> >
> 
> This is _not_ a software watchdog. If the timer expires, an interrupt
> is generated, and the timer is reset to count through another cycle.
> If it expires again, it resets the CPU.

Ok thanks, then it does make sense for it to be in kernel.

Re: [PATCH] Added MIPS RM9K watchdog driver

[Dave Jones]

On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
 > This is a driver for the on-chip watchdog device found on some
 > MIPS RM9000 processors.
 > 
 > Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>

Mostly same nit-picking comments as your other driver..

 > +++ b/drivers/char/watchdog/rm9k_wdt.c
 > ... 
 > + 
 > +#include <linux/config.h>

not needed.

 > +/* Function prototypes */
 > +static int __init wdt_gpi_probe(struct device *);
 > +static int __exit wdt_gpi_remove(struct device *);
 > +static void wdt_gpi_set_timeout(unsigned int);
 > +static int wdt_gpi_open(struct inode *, struct file *);
 > +static int wdt_gpi_release(struct inode *, struct file *);
 > +static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, 
 > loff_t *);
 > +static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
 > +static const struct resource *wdt_gpi_get_resource(struct platform_device *, 
 > const char *, unsigned int);
 > +static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
 > +static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);

Can probably (mostly?) go away with some creative reordering.

 > +static int locked = 0;

unneeded initialisation.

 > +static int nowayout =
 > +#if defined(CONFIG_WATCHDOG_NOWAYOUT)
 > +	1;
 > +#else
 > +	0;
 > +#endif

static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;

should work.

 > +static void wdt_gpi_set_timeout(unsigned int to)
 > +{
 > +	u32 reg;
 > +	const u32 wdval = (to * CLOCK) & ~0x0000000f;
 > +
 > +	lock_titan_regs();
 > +	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
 > +	titan_writel(reg, CPCCR);
 > +	wmb();
 > +	__raw_writel(wdval, wd_regs + 0x0000);
 > +	wmb();
 > +	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
 > +	wmb();
 > +	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
 > +	iob();
 > +	unlock_titan_regs();
 > +}

As in the previous driver, are these barriers strong enough?
Or do they need explicit reads of the written addresses to flush the write?
 
		Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] Added MIPS RM9K watchdog driver

[Thomas Koeller]

On Friday 11 August 2006 22:56, Dave Jones wrote:
> On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
>  > This is a driver for the on-chip watchdog device found on some
>  > MIPS RM9000 processors.
>  >
>  > Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
>
> Mostly same nit-picking comments as your other driver..

Which one?

>
>  > +++ b/drivers/char/watchdog/rm9k_wdt.c
>  > ...
>  > +
>  > +#include <linux/config.h>
>
> not needed.

It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.

>
>  > +/* Function prototypes */
>  > +static int __init wdt_gpi_probe(struct device *);
>  > +static int __exit wdt_gpi_remove(struct device *);
>  > +static void wdt_gpi_set_timeout(unsigned int);
>  > +static int wdt_gpi_open(struct inode *, struct file *);
>  > +static int wdt_gpi_release(struct inode *, struct file *);
>  > +static ssize_t wdt_gpi_write(struct file *, const char __user *,
>  > size_t, loff_t *);
>  > +static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
>  > +static const struct resource *wdt_gpi_get_resource(struct
>  > platform_device *, const char *, unsigned int);
>  > +static int wdt_gpi_notify(struct notifier_block *, unsigned long, void
>  > *); +static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);
>
> Can probably (mostly?) go away with some creative reordering.

Probably, but should it? I always considered it good style to have
prototypes for all functions.

>
>  > +static int locked = 0;
>
> unneeded initialisation.

Not strictly needed, that's true, but does not do any harm either
and expresses the intention clearly.

>
>  > +static int nowayout =
>  > +#if defined(CONFIG_WATCHDOG_NOWAYOUT)
>  > +	1;
>  > +#else
>  > +	0;
>  > +#endif
>
> static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;
>
> should work.

Does not work. If the option is not selected, CONFIG_WATCHDOG_NOWAYOUT
is undefined, not zero.

>
>  > +static void wdt_gpi_set_timeout(unsigned int to)
>  > +{
>  > +	u32 reg;
>  > +	const u32 wdval = (to * CLOCK) & ~0x0000000f;
>  > +
>  > +	lock_titan_regs();
>  > +	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
>  > +	titan_writel(reg, CPCCR);
>  > +	wmb();
>  > +	__raw_writel(wdval, wd_regs + 0x0000);
>  > +	wmb();
>  > +	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
>  > +	wmb();
>  > +	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
>  > +	iob();
>  > +	unlock_titan_regs();
>  > +}
>
> As in the previous driver, are these barriers strong enough?
> Or do they need explicit reads of the written addresses to flush the write?

I think they are. Remember, the entire device is integrated in the
processor. No external buses involved.

>
> 		Dave

Thanks for your comments!

Thomas

-- 
Thomas Koeller
thomas@koeller.dyndns.org

Re: [PATCH] Added MIPS RM9K watchdog driver

[Oleg Verych]

Thomas Koeller wrote:
> On Friday 11 August 2006 22:56, Dave Jones wrote:
> 
>>On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
>> > This is a driver for the on-chip watchdog device found on some
>> > MIPS RM9000 processors.
>> >
>> > Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
>>
>>Mostly same nit-picking comments as your other driver..
> 
> 
> Which one?
>
<http://permalink.gmane.org/gmane.linux.ports.mips.general/13500>

--
-o--=O`C
  #oo'L O
<___=E M

Re: [PATCH] Added MIPS RM9K watchdog driver

[Dave Jones]

On Sat, Aug 12, 2006 at 01:49:23AM +0200, Thomas Koeller wrote:
 > On Friday 11 August 2006 22:56, Dave Jones wrote:
 > > On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
 > >  > This is a driver for the on-chip watchdog device found on some
 > >  > MIPS RM9000 processors.
 > >  >
 > >  > Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
 > >
 > > Mostly same nit-picking comments as your other driver..
 > 
 > Which one?

The image capture driver.

 > >  > +#include <linux/config.h>
 > >
 > > not needed.
 > 
 > It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.

kbuild automatically includes it for you in the last few kernels.


 > > As in the previous driver, are these barriers strong enough?
 > > Or do they need explicit reads of the written addresses to flush the write?
 > 
 > I think they are. Remember, the entire device is integrated in the
 > processor. No external buses involved.

Ok.

		Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] Added MIPS RM9K watchdog driver

[Ralf Baechle]

On Fri, Aug 11, 2006 at 08:06:36PM -0400, Dave Jones wrote:

>  > I think they are. Remember, the entire device is integrated in the
>  > processor. No external buses involved.
> 
> Ok.

I think it's ok in this case, we're unlikely to see these peripherals
on other chips than PMC-Sierra's - but that's of course just a guess.

With a broader perspective the embedded world is increasingly converging
to using standardized components (so called "IP") and on-chip
interconnects such as OCP for new designs and portable drivers should
reflect this.

  Ralf

Re: [PATCH] Added MIPS RM9K watchdog driver

[Joseph Fannin]

On Sat, Aug 12, 2006 at 01:49:23AM +0200, Thomas Koeller wrote:
> On Friday 11 August 2006 22:56, Dave Jones wrote:
> > On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
> >  > +
> >  > +#include <linux/config.h>
> >
> > not needed.
>
> It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.

    It shouldn't be necessary, so it's probably a bug.  I could not
begin to tell you where.

    I've CC'd the Kbuild maintainer -- apologies if I'm way off base
here.

    Still, if this #include is to stay, you'd probably better comment
it, or it's likely someone will rip it out in a cleanup:

http://www.gossamer-threads.com/lists/linux/kernel/663918

>
> >
> >  > +static int locked = 0;
> >
> > unneeded initialisation.
>
> Not strictly needed, that's true, but does not do any harm either
> and expresses the intention clearly.

    My meager understanding is that it makes the kernel image bigger.

> >
> >  > +static int nowayout =
> >  > +#if defined(CONFIG_WATCHDOG_NOWAYOUT)
> >  > +	1;
> >  > +#else
> >  > +	0;
> >  > +#endif
> >
> > static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;
> >
> > should work.
>
> Does not work. If the option is not selected, CONFIG_WATCHDOG_NOWAYOUT
> is undefined, not zero.


    Possibly related?


--
Joseph Fannin
jhf@columbus.rr.com || jfannin@gmail.com

Re: [PATCH] Added MIPS RM9K watchdog driver

[Sam Ravnborg]

On Mon, Aug 14, 2006 at 10:14:45AM -0400, Joseph Fannin wrote:
> On Sat, Aug 12, 2006 at 01:49:23AM +0200, Thomas Koeller wrote:
> > On Friday 11 August 2006 22:56, Dave Jones wrote:
> > > On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
> > >  > +
> > >  > +#include <linux/config.h>
> > >
> > > not needed.
> >
> > It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.
Yes you do - try it.
make V=1 tells you that -include include/linux/autoconf.h pulls in the
CONFIG_ definitions.

	Sam

Re: [PATCH] Added MIPS RM9K watchdog driver

[Randy.Dunlap]

On Mon, 14 Aug 2006 17:30:33 +0200 Sam Ravnborg wrote:

> On Mon, Aug 14, 2006 at 10:14:45AM -0400, Joseph Fannin wrote:
> > On Sat, Aug 12, 2006 at 01:49:23AM +0200, Thomas Koeller wrote:
> > > On Friday 11 August 2006 22:56, Dave Jones wrote:
> > > > On Thu, Aug 10, 2006 at 11:19:13PM +0200,
> > > > thomas@koeller.dyndns.org wrote:
> > > >  > +
> > > >  > +#include <linux/config.h>
> > > >
> > > > not needed.
> > >
> > > It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.
> Yes you do - try it.
> make V=1 tells you that -include include/linux/autoconf.h pulls in
> the CONFIG_ definitions.

Sure, autoconf.h is included, but I think his point is that
CONFIG_WATCHDOG_NOWAYOUT may not be defined there at all,
as in my 2.6.18-rc4 autoconf.h file, since my .config file says:
# CONFIG_WATCHDOG_NOWAYOUT is not set

---
~Randy

Re: [PATCH] Added MIPS RM9K watchdog driver

[Dave Jones]

On Mon, Aug 14, 2006 at 09:21:24AM -0700, Randy.Dunlap wrote:
 > On Mon, 14 Aug 2006 17:30:33 +0200 Sam Ravnborg wrote:
 > 
 > > On Mon, Aug 14, 2006 at 10:14:45AM -0400, Joseph Fannin wrote:
 > > > On Sat, Aug 12, 2006 at 01:49:23AM +0200, Thomas Koeller wrote:
 > > > > On Friday 11 August 2006 22:56, Dave Jones wrote:
 > > > > > On Thu, Aug 10, 2006 at 11:19:13PM +0200,
 > > > > > thomas@koeller.dyndns.org wrote:
 > > > > >  > +
 > > > > >  > +#include <linux/config.h>
 > > > > >
 > > > > > not needed.
 > > > >
 > > > > It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.
 > > Yes you do - try it.
 > > make V=1 tells you that -include include/linux/autoconf.h pulls in
 > > the CONFIG_ definitions.
 > 
 > Sure, autoconf.h is included, but I think his point is that
 > CONFIG_WATCHDOG_NOWAYOUT may not be defined there at all,
 > as in my 2.6.18-rc4 autoconf.h file, since my .config file says:
 > # CONFIG_WATCHDOG_NOWAYOUT is not set

As Wim pointed out, linux/watchdog.h has this magick..

#ifdef CONFIG_WATCHDOG_NOWAYOUT
#define WATCHDOG_NOWAYOUT   1
#else
#define WATCHDOG_NOWAYOUT   0
#endif

Which takes some of the ugly out of the drivers :-)

		Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] Added MIPS RM9K watchdog driver

[Roland Dreier]

    Randy> Sure, autoconf.h is included, but I think his point is that
    Randy> CONFIG_WATCHDOG_NOWAYOUT may not be defined there at all,
    Randy> as in my 2.6.18-rc4 autoconf.h file, since my .config file
    Randy> says: # CONFIG_WATCHDOG_NOWAYOUT is not set

Huh?  How would including <linux/config.h> help with that?  And why
would you want CONFIG_WATCHDOG_NOWAYOUT to be defined if
WATCHDOG_NOWAYOUT is not set in your configuration?  That would
utterly break code that does something like

    #ifdef CONFIG_WATCHDOG_NOWAYOUT

 - R.

Re: [PATCH] Added MIPS RM9K watchdog driver

[Wim Van Sebroeck]

Hi All,

> >  > +#include <linux/config.h>
> >
> > not needed.
> 
> It is, otherwise I do not get CONFIG_WATCHDOG_NOWAYOUT.

We'll fix this in the watchdog.h include file.

> >  > +static int nowayout =
> >  > +#if defined(CONFIG_WATCHDOG_NOWAYOUT)
> >  > +	1;
> >  > +#else
> >  > +	0;
> >  > +#endif
> >
> > static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;
> >
> > should work.
> 
> Does not work. If the option is not selected, CONFIG_WATCHDOG_NOWAYOUT
> is undefined, not zero.

This should be:
static int nowayout = WATCHDOG_NOWAYOUT;

Can you resent me your latest diff?

Thanks,
Wim.

Re: [PATCH] Added MIPS RM9K watchdog driver

[Luca]

thomas@koeller.dyndns.org ha scritto:
> diff --git a/drivers/char/watchdog/rm9k_wdt.c 
> b/drivers/char/watchdog/rm9k_wdt.c
> new file mode 100644
> index 0000000..f6a9d17
> --- /dev/null
> +++ b/drivers/char/watchdog/rm9k_wdt.c
> @@ -0,0 +1,435 @@
[...]
> +/* Module arguments */
> +static int timeout = MAX_TIMEOUT_SECONDS;
> +module_param(timeout, int, 444);
> +static unsigned long resetaddr = 0xbffdc200;
> +module_param(resetaddr, ulong, 444);
> +static unsigned long flagaddr = 0xbffdc104;
> +module_param(flagaddr, ulong, 444);
> +static int powercycle = 0;
> +module_param(powercycle, bool, 444);

File permissions should be in octal ;)

Luca
-- 
Home: http://kronoz.cjb.net
"New processes are created by other processes, just like new
 humans. New humans are created by other humans, of course,
 not by processes." -- Unix System Administration Handbook