| Message ID | 1257882180.14374.3@antares (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Grant Likely |
| Headers | show |
On Tue, Nov 10, 2009 at 12:43 PM, Albrecht Dreß <albrecht.dress@arcor.de> wrote: > Use the MPC5200 GPT api for the WDT which drastically simplifies this file. > > Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de> > --- > > drivers/watchdog/mpc5200_wdt.c | 246 +++++++++++----------------------------- > 1 files changed, 65 insertions(+), 181 deletions(-) Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver. g.
On Wed, Nov 11, 2009 at 1:32 AM, Albrecht Dreß <albrecht.dress@arcor.de> wrote: > Hi Grant: > > O.k., thanks for your comments. If Wim doesn't have any objections to it, I will provide a merged patch. One consequence I forgot to mention is that we loose the ability to build the wdt support as module, but I don't think it's a real problem. I'm not too worried about this. It is also potentially possible to rework the entire GPT driver as a module. But there are non-trivial architecture fiddly bits needed to make it safe to load IRQ controllers in a module. > I think we still should keep the kernel config option enable/disable the wdt support, which would mask out the wdt code if disabled. Is that ok for you? Yup. g.
Hi All, >> Can the WDT functionality just be merged entirely into >> arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for >> this file entirely? I think I'd rather have all the GPT "built in" >> behaviour handled by a single driver. > > I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public. > > However, the reasons I hesitated to do so are: > - I don't want to remove a file someone else wrote (even it doesn't work); > - WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov; > - a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing. > > You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes! My opinion: it is harder to maintain the watchdog code if it is being moved away from drivers/watchdog. I need to check the code before I comment any further on this, but my first thought is: why don't you do it with platform resources like other devices are doing? This way you can keep the platform code under arch and the watchdog itself under drivers/watchdog/. You can look at the following drivers as an example: adx_wdt.c ar7_wdt.c at32ap700x_wdt.c coh901327_wdt.c davinci_wdt.c mpcore_wdt.c mv64x60_wdt.c nuc900_wdt.c omap_wdt.c pnx4008_wdt.c rc32434_wdt.c s3c2410_wdt.c txx9wdt.c . Kind regards, Wim.
On Thu, Nov 12, 2009 at 2:36 PM, Wim Van Sebroeck <wim@iguana.be> wrote: > Hi All, > >>> Can the WDT functionality just be merged entirely into >>> arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for >>> this file entirely? I think I'd rather have all the GPT "built in" >>> behaviour handled by a single driver. >> >> I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public. >> >> However, the reasons I hesitated to do so are: >> - I don't want to remove a file someone else wrote (even it doesn't work); >> - WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov; >> - a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing. >> >> You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes! > > My opinion: it is harder to maintain the watchdog code if it is being moved away from drivers/watchdog. > I need to check the code before I comment any further on this, but my first thought is: why don't you do it with platform resources like other devices are doing? This way you can keep the platform code under arch and the watchdog itself under drivers/watchdog/. You can look at the following drivers as an example: adx_wdt.c ar7_wdt.c at32ap700x_wdt.c coh901327_wdt.c davinci_wdt.c mpcore_wdt.c mv64x60_wdt.c nuc900_wdt.c omap_wdt.c pnx4008_wdt.c rc32434_wdt.c s3c2410_wdt.c txx9wdt.c . In actual fact, it is a single device with multiple functions, some of which can be used at the same time. The driver for the device determines what the device instance supports and registers the appropriate interfaces. There is a GPIO controller, a PWM controller, a general purpose timer, and the watchdog. Because of the multifunction nature of the device, there are subtle interactions between the functions that the driver needs to maintain. I don't want to export functions from the driver which will only be used by a watchdog instance. I also don't want the added code and complexity of a secondary probe path. It is simpler and less code to roll all the behaviour up into the one driver single driver that gets probed once. From the maintenance perspective, having the main driver in arch/powerpc and the watchdog bit in drivers/watchdog doesn't really help much anyway because anything that changes the internal driver API (between the core and watchdog bits) will require cross-maintainer changes. ie. do changes go through my tree because they touch arch/powerpc, or do they go through yours because they touch drivers/watchdog? I'd much rather all the internal details be contained within a single driver. Besides, there is already precedence for very arch-specific drivers living under arch/*/. find ./arch -name *gpio* Cheers, g.
Hi Grant, Sorry for the delay, I need to deliver a project in a weeks time... > In actual fact, it is a single device with multiple functions, some of > which can be used at the same time. The driver for the device > determines what the device instance supports and registers the > appropriate interfaces. There is a GPIO controller, a PWM controller, > a general purpose timer, and the watchdog. Because of the > multifunction nature of the device, there are subtle interactions > between the functions that the driver needs to maintain. I don't want > to export functions from the driver which will only be used by a > watchdog instance. I also don't want the added code and complexity of > a secondary probe path. It is simpler and less code to roll all the > behaviour up into the one driver single driver that gets probed once. > > >From the maintenance perspective, having the main driver in > arch/powerpc and the watchdog bit in drivers/watchdog doesn't really > help much anyway because anything that changes the internal driver API > (between the core and watchdog bits) will require cross-maintainer > changes. ie. do changes go through my tree because they touch > arch/powerpc, or do they go through yours because they touch > drivers/watchdog? I'd much rather all the internal details be > contained within a single driver. Your argument about maintenance is the same one as I have: If all watchdog driver pieces are under drivers/watchdog then it's easier for me to maintain them. (Definitely if we are doing clean-up work and API changes). > Besides, there is already precedence for very arch-specific drivers > living under arch/*/. find ./arch -name *gpio* But in this case: I know where to find them and I will keep a mental note about this one. And yes indeed some very arch specific drivers can reside under arch/*/* . So please go ahead with pulling this one in a single driver. Kind regards, Wim.
On Tue, Dec 8, 2009 at 1:48 PM, Wim Van Sebroeck <wim@iguana.be> wrote: > Hi Grant, > > Sorry for the delay, I need to deliver a project in a weeks time... understood. >> In actual fact, it is a single device with multiple functions, some of >> which can be used at the same time. The driver for the device >> determines what the device instance supports and registers the >> appropriate interfaces. There is a GPIO controller, a PWM controller, >> a general purpose timer, and the watchdog. Because of the >> multifunction nature of the device, there are subtle interactions >> between the functions that the driver needs to maintain. I don't want >> to export functions from the driver which will only be used by a >> watchdog instance. I also don't want the added code and complexity of >> a secondary probe path. It is simpler and less code to roll all the >> behaviour up into the one driver single driver that gets probed once. >> >> >From the maintenance perspective, having the main driver in >> arch/powerpc and the watchdog bit in drivers/watchdog doesn't really >> help much anyway because anything that changes the internal driver API >> (between the core and watchdog bits) will require cross-maintainer >> changes. ie. do changes go through my tree because they touch >> arch/powerpc, or do they go through yours because they touch >> drivers/watchdog? I'd much rather all the internal details be >> contained within a single driver. > > Your argument about maintenance is the same one as I have: If all watchdog > driver pieces are under drivers/watchdog then it's easier for me to maintain > them. (Definitely if we are doing clean-up work and API changes). :-) >> Besides, there is already precedence for very arch-specific drivers >> living under arch/*/. find ./arch -name *gpio* > > But in this case: I know where to find them and I will keep a mental note > about this one. And yes indeed some very arch specific drivers can reside > under arch/*/* . > > So please go ahead with pulling this one in a single driver. Great, Thanks Wim. g.
diff --git a/drivers/watchdog/mpc5200_wdt.c b/drivers/watchdog/mpc5200_wdt.c index fa9c47c..5bb553c 100644 --- a/drivers/watchdog/mpc5200_wdt.c +++ b/drivers/watchdog/mpc5200_wdt.c @@ -2,25 +2,16 @@ #include <linux/module.h> #include <linux/miscdevice.h> #include <linux/watchdog.h> -#include <linux/io.h> -#include <linux/spinlock.h> #include <linux/of_platform.h> #include <linux/uaccess.h> #include <asm/mpc52xx.h> -#define GPT_MODE_WDT (1 << 15) -#define GPT_MODE_CE (1 << 12) -#define GPT_MODE_MS_TIMER (0x4) - +#define WDT_IDENTITY "mpc5200 watchdog on GPT0" struct mpc5200_wdt { - unsigned count; /* timer ticks before watchdog kicks in */ - long ipb_freq; - struct miscdevice miscdev; - struct resource mem; - struct mpc52xx_gpt __iomem *regs; - spinlock_t io_lock; + int timeout; + struct mpc52xx_gpt_priv *timer; }; /* is_active stores wether or not the /dev/watchdog device is opened */ @@ -32,80 +23,33 @@ static unsigned long is_active; static struct mpc5200_wdt *wdt_global; -/* helper to calculate timeout in timer counts */ -static void mpc5200_wdt_set_timeout(struct mpc5200_wdt *wdt, int timeout) -{ - /* use biggest prescaler of 64k */ - wdt->count = (wdt->ipb_freq + 0xffff) / 0x10000 * timeout; - - if (wdt->count > 0xffff) - wdt->count = 0xffff; -} -/* return timeout in seconds (calculated from timer count) */ -static int mpc5200_wdt_get_timeout(struct mpc5200_wdt *wdt) -{ - return wdt->count * 0x10000 / wdt->ipb_freq; -} - - -/* watchdog operations */ -static int mpc5200_wdt_start(struct mpc5200_wdt *wdt) -{ - spin_lock(&wdt->io_lock); - /* disable */ - out_be32(&wdt->regs->mode, 0); - /* set timeout, with maximum prescaler */ - out_be32(&wdt->regs->count, 0x0 | wdt->count); - /* enable watchdog */ - out_be32(&wdt->regs->mode, GPT_MODE_CE | GPT_MODE_WDT | - GPT_MODE_MS_TIMER); - spin_unlock(&wdt->io_lock); - - return 0; -} -static int mpc5200_wdt_ping(struct mpc5200_wdt *wdt) -{ - spin_lock(&wdt->io_lock); - /* writing A5 to OCPW resets the watchdog */ - out_be32(&wdt->regs->mode, 0xA5000000 | - (0xffffff & in_be32(&wdt->regs->mode))); - spin_unlock(&wdt->io_lock); - return 0; -} -static int mpc5200_wdt_stop(struct mpc5200_wdt *wdt) -{ - spin_lock(&wdt->io_lock); - /* disable */ - out_be32(&wdt->regs->mode, 0); - spin_unlock(&wdt->io_lock); - return 0; -} - - /* file operations */ static ssize_t mpc5200_wdt_write(struct file *file, const char __user *data, - size_t len, loff_t *ppos) + size_t len, loff_t *ppos) { struct mpc5200_wdt *wdt = file->private_data; - mpc5200_wdt_ping(wdt); + mpc52xx_gpt_wdt_ping(wdt->timer); return 0; } + static struct watchdog_info mpc5200_wdt_info = { .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, - .identity = "mpc5200 watchdog on GPT0", + .identity = WDT_IDENTITY, }; + static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) + unsigned long arg) { struct mpc5200_wdt *wdt = file->private_data; int __user *data = (int __user *)arg; int timeout; + u64 real_timeout; int ret = 0; switch (cmd) { case WDIOC_GETSUPPORT: ret = copy_to_user(data, &mpc5200_wdt_info, - sizeof(mpc5200_wdt_info)); + sizeof(mpc5200_wdt_info)); if (ret) ret = -EFAULT; break; @@ -116,19 +60,23 @@ static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd, break; case WDIOC_KEEPALIVE: - mpc5200_wdt_ping(wdt); + mpc52xx_gpt_wdt_ping(wdt->timer); break; case WDIOC_SETTIMEOUT: ret = get_user(timeout, data); if (ret) break; - mpc5200_wdt_set_timeout(wdt, timeout); - mpc5200_wdt_start(wdt); + ret = mpc52xx_gpt_wdt_start(wdt->timer, timeout); + if (ret) + break; + wdt->timeout = timeout; /* fall through and return the timeout */ case WDIOC_GETTIMEOUT: - timeout = mpc5200_wdt_get_timeout(wdt); + real_timeout = mpc52xx_gpt_timer_period(wdt->timer); + do_div(real_timeout, 1000000000); + timeout = (int) real_timeout; ret = put_user(timeout, data); break; @@ -140,154 +88,90 @@ static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd, static int mpc5200_wdt_open(struct inode *inode, struct file *file) { + int ret; + /* /dev/watchdog can only be opened once */ if (test_and_set_bit(0, &is_active)) return -EBUSY; /* Set and activate the watchdog */ - mpc5200_wdt_set_timeout(wdt_global, 30); - mpc5200_wdt_start(wdt_global); + ret = mpc52xx_gpt_wdt_start(wdt_global->timer, 30); + if (ret) { + clear_bit(0, &is_active); + return ret; + } + wdt_global->timeout = 30; + file->private_data = wdt_global; return nonseekable_open(inode, file); } + static int mpc5200_wdt_release(struct inode *inode, struct file *file) { -#if WATCHDOG_NOWAYOUT == 0 +#if !defined(CONFIG_WATCHDOG_NOWAYOUT) struct mpc5200_wdt *wdt = file->private_data; - mpc5200_wdt_stop(wdt); - wdt->count = 0; /* == disabled */ + mpc52xx_gpt_wdt_stop(wdt->timer); + wdt->timeout = 0; /* == disabled */ #endif clear_bit(0, &is_active); return 0; } static const struct file_operations mpc5200_wdt_fops = { - .owner = THIS_MODULE, - .write = mpc5200_wdt_write, + .owner = THIS_MODULE, + .llseek = no_llseek, + .write = mpc5200_wdt_write, .unlocked_ioctl = mpc5200_wdt_ioctl, - .open = mpc5200_wdt_open, - .release = mpc5200_wdt_release, + .open = mpc5200_wdt_open, + .release = mpc5200_wdt_release, +}; + +static struct miscdevice mpc5200_wdt_miscdev = { + .minor = WATCHDOG_MINOR, + .name = "watchdog", + .fops = &mpc5200_wdt_fops, }; /* module operations */ -static int mpc5200_wdt_probe(struct of_device *op, - const struct of_device_id *match) +static int __init mpc5200_wdt_init(void) { - struct mpc5200_wdt *wdt; int err; - const void *has_wdt; - int size; + struct mpc52xx_gpt_priv *timer; - has_wdt = of_get_property(op->node, "has-wdt", NULL); - if (!has_wdt) - has_wdt = of_get_property(op->node, "fsl,has-wdt", NULL); - if (!has_wdt) + /* grab the watchdog-capable gpt */ + timer = mpc52xx_gpt_wdt_probe(); + if (!timer) { + pr_err(WDT_IDENTITY ": probing failed\n"); return -ENODEV; + } - wdt = kzalloc(sizeof(*wdt), GFP_KERNEL); - if (!wdt) + wdt_global = kzalloc(sizeof(struct mpc5200_wdt), GFP_KERNEL); + if (!wdt_global) { + mpc52xx_gpt_wdt_release(timer); return -ENOMEM; - - wdt->ipb_freq = mpc5xxx_get_bus_frequency(op->node); - - err = of_address_to_resource(op->node, 0, &wdt->mem); - if (err) - goto out_free; - size = wdt->mem.end - wdt->mem.start + 1; - if (!request_mem_region(wdt->mem.start, size, "mpc5200_wdt")) { - err = -ENODEV; - goto out_free; - } - wdt->regs = ioremap(wdt->mem.start, size); - if (!wdt->regs) { - err = -ENODEV; - goto out_release; } - - dev_set_drvdata(&op->dev, wdt); - spin_lock_init(&wdt->io_lock); - - wdt->miscdev = (struct miscdevice) { - .minor = WATCHDOG_MINOR, - .name = "watchdog", - .fops = &mpc5200_wdt_fops, - .parent = &op->dev, - }; - wdt_global = wdt; - err = misc_register(&wdt->miscdev); + wdt_global->timer = timer; + err = misc_register(&mpc5200_wdt_miscdev); if (!err) - return 0; - - iounmap(wdt->regs); -out_release: - release_mem_region(wdt->mem.start, size); -out_free: - kfree(wdt); + pr_info(WDT_IDENTITY ": registered\n"); + else { + mpc52xx_gpt_wdt_release(timer); + kfree(wdt_global); + } return err; } -static int mpc5200_wdt_remove(struct of_device *op) -{ - struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev); - - mpc5200_wdt_stop(wdt); - misc_deregister(&wdt->miscdev); - iounmap(wdt->regs); - release_mem_region(wdt->mem.start, wdt->mem.end - wdt->mem.start + 1); - kfree(wdt); - - return 0; -} -static int mpc5200_wdt_suspend(struct of_device *op, pm_message_t state) -{ - struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev); - mpc5200_wdt_stop(wdt); - return 0; -} -static int mpc5200_wdt_resume(struct of_device *op) -{ - struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev); - if (wdt->count) - mpc5200_wdt_start(wdt); - return 0; -} -static int mpc5200_wdt_shutdown(struct of_device *op) -{ - struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev); - mpc5200_wdt_stop(wdt); - return 0; -} - -static struct of_device_id mpc5200_wdt_match[] = { - { .compatible = "mpc5200-gpt", }, - { .compatible = "fsl,mpc5200-gpt", }, - {}, -}; -static struct of_platform_driver mpc5200_wdt_driver = { - .owner = THIS_MODULE, - .name = "mpc5200-gpt-wdt", - .match_table = mpc5200_wdt_match, - .probe = mpc5200_wdt_probe, - .remove = mpc5200_wdt_remove, - .suspend = mpc5200_wdt_suspend, - .resume = mpc5200_wdt_resume, - .shutdown = mpc5200_wdt_shutdown, -}; - - -static int __init mpc5200_wdt_init(void) -{ - return of_register_platform_driver(&mpc5200_wdt_driver); -} - static void __exit mpc5200_wdt_exit(void) { - of_unregister_platform_driver(&mpc5200_wdt_driver); + mpc52xx_gpt_wdt_release(wdt_global->timer); + misc_deregister(&mpc5200_wdt_miscdev); + kfree(wdt_global); } module_init(mpc5200_wdt_init); module_exit(mpc5200_wdt_exit); -MODULE_AUTHOR("Domen Puncer <domen.puncer@telargo.com>"); +MODULE_AUTHOR("Domen Puncer <domen.puncer@telargo.com>, " + "Albrecht Dreß <albrecht.dress@arcor.de>"); MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
Use the MPC5200 GPT api for the WDT which drastically simplifies this file. Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de> --- drivers/watchdog/mpc5200_wdt.c | 246 +++++++++++----------------------------- 1 files changed, 65 insertions(+), 181 deletions(-)