Subject: Fwd: [PATCH 1/8] watchdog: davinci: change driver to use WDT core

From: [email-masked] (ivan.khoronzhuk)
Date: Mon, 18 Nov 2013 13:47:44 +0200

On 11/16/2013 10:29 PM, Guenter Roeck wrote:
> On 11/06/2013 03:31 AM, ivan.khoronzhuk wrote:
>> To reduce code duplicate and increase code readability use WDT core
>> code to handle WDT interface.
>>
>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>> Remove wdt_state as the WDT core track state with its own variable.
>>
>> The watchdog_init_timeout() can read timeout value from timeout-sec
>> property if the passed value is out of bounds. So set initial
>> heartbeat value more than max value in order to initialize heartbeat
>> in next way. If heartbeat is not set thought module parameter, try
>> to read it's value from WDT node timeout-sec property. If node has
>> no one, use default value.
>>
>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>> order to set timeout period.
>>
>> Signed-off-by: Ivan Khoronzhuk <[email-masked]>
>> ---
>>   drivers/watchdog/Kconfig       |    1 +
>>   drivers/watchdog/davinci_wdt.c |  150 
>> ++++++++++------------------------------
>>   2 files changed, 38 insertions(+), 113 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index d1d53f3..2c954b5 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -271,6 +271,7 @@ config IOP_WATCHDOG
>>   config DAVINCI_WATCHDOG
>>       tristate "DaVinci watchdog"
>>       depends on ARCH_DAVINCI
>> +    select WATCHDOG_CORE
>>       help
>>         Say Y here if to include support for the watchdog timer
>>         in the DaVinci DM644x/DM646x processors.
>> diff --git a/drivers/watchdog/davinci_wdt.c 
>> b/drivers/watchdog/davinci_wdt.c
>> index bead774..a6eef71 100644
>> --- a/drivers/watchdog/davinci_wdt.c
>> +++ b/drivers/watchdog/davinci_wdt.c
>> @@ -3,7 +3,7 @@
>>    *
>>    * Watchdog driver for DaVinci DM644x/DM646x processors
>>    *
>> - * Copyright (C) 2006 Texas Instruments.
>> + * Copyright (C) 2013 Texas Instruments.
> 
> 2006-2013
>

Ok

>>    *
>>    * 2007 (c) MontaVista Software, Inc. This file is licensed under
>>    * the terms of the GNU General Public License version 2. This program
>> @@ -15,18 +15,12 @@
>>   #include <linux/moduleparam.h>
>>   #include <linux/types.h>
>>   #include <linux/kernel.h>
>> -#include <linux/fs.h>
>> -#include <linux/miscdevice.h>
>>   #include <linux/watchdog.h>
>>   #include <linux/init.h>
>> -#include <linux/bitops.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/spinlock.h>
>> -#include <linux/uaccess.h>
>>   #include <linux/io.h>
>>   #include <linux/device.h>
>>   #include <linux/clk.h>
>> -#include <linux/slab.h>
>>   #include <linux/err.h>
>>
>>   #define MODULE_NAME "DAVINCI-WDT: "
>> @@ -61,31 +55,13 @@
>>   #define WDKEY_SEQ0        (0xa5c6 << 16)
>>   #define WDKEY_SEQ1        (0xda7e << 16)
>>
>> -static int heartbeat = DEFAULT_HEARTBEAT;
>> -
>> -static DEFINE_SPINLOCK(io_lock);
>> -static unsigned long wdt_status;
>> -#define WDT_IN_USE        0
>> -#define WDT_OK_TO_CLOSE   1
>> -#define WDT_REGION_INITED 2
>> -#define WDT_DEVICE_INITED 3
>> -
>> +static int heartbeat = MAX_HEARTBEAT + 1;
> 
> Initializing it with 0 (ie not at all) would be just as good. Also see 
> below.
>

Ok

>>   static void __iomem    *wdt_base;
>>   struct clk        *wdt_clk;
>> +static struct watchdog_device    wdt_wdd;
>>
>> -static void wdt_service(void)
>> -{
>> -    spin_lock(&io_lock);
>> -
>> -    /* put watchdog in service state */
>> -    iowrite32(WDKEY_SEQ0, wdt_base + WDTCR);
>> -    /* put watchdog in active state */
>> -    iowrite32(WDKEY_SEQ1, wdt_base + WDTCR);
>> -
>> -    spin_unlock(&io_lock);
>> -}
>> -
>> -static void wdt_enable(void)
>> +/* davinci_wdt_start - enable watchdog */
> 
> That comment doesn't really provide much value.
> 
>> +static int davinci_wdt_start(struct watchdog_device *wdd)
>>   {
>>       u32 tgcr;
>>       u32 timer_margin;
>> @@ -93,8 +69,6 @@ static void wdt_enable(void)
>>
>>       wdt_freq = clk_get_rate(wdt_clk);
>>
>> -    spin_lock(&io_lock);
>> -
>>       /* disable, internal clock source */
>>       iowrite32(0, wdt_base + TCR);
>>       /* reset timer, set mode to 64-bit watchdog, and unreset */
>> @@ -105,9 +79,9 @@ static void wdt_enable(void)
>>       iowrite32(0, wdt_base + TIM12);
>>       iowrite32(0, wdt_base + TIM34);
>>       /* set timeout period */
>> -    timer_margin = (((u64)heartbeat * wdt_freq) & 0xffffffff);
>> +    timer_margin = (((u64)wdd->timeout * wdt_freq) & 0xffffffff);
>>       iowrite32(timer_margin, wdt_base + PRD12);
>> -    timer_margin = (((u64)heartbeat * wdt_freq) >> 32);
>> +    timer_margin = (((u64)wdd->timeout * wdt_freq) >> 32);
>>       iowrite32(timer_margin, wdt_base + PRD34);
>>       /* enable run continuously */
>>       iowrite32(ENAMODE12_PERIODIC, wdt_base + TCR);
>> @@ -119,84 +93,28 @@ static void wdt_enable(void)
>>       iowrite32(WDKEY_SEQ0 | WDEN, wdt_base + WDTCR);
>>       /* put watchdog in active state */
>>       iowrite32(WDKEY_SEQ1 | WDEN, wdt_base + WDTCR);
>> -
>> -    spin_unlock(&io_lock);
>> -}
>> -
>> -static int davinci_wdt_open(struct inode *inode, struct file *file)
>> -{
>> -    if (test_and_set_bit(WDT_IN_USE, &wdt_status))
>> -        return -EBUSY;
>> -
>> -    wdt_enable();
>> -
>> -    return nonseekable_open(inode, file);
>> +    return 0;
>>   }
>>
>> -static ssize_t
>> -davinci_wdt_write(struct file *file, const char *data, size_t len,
>> -          loff_t *ppos)
>> +static int davinci_wdt_ping(struct watchdog_device *wdd)
>>   {
>> -    if (len)
>> -        wdt_service();
>> -
>> -    return len;
>> +    /* put watchdog in service state */
>> +    iowrite32(WDKEY_SEQ0, wdt_base + WDTCR);
>> +    /* put watchdog in active state */
>> +    iowrite32(WDKEY_SEQ1, wdt_base + WDTCR);
>> +    return 0;
>>   }
>>
>> -static const struct watchdog_info ident = {
>> +static const struct watchdog_info davinci_wdt_info = {
>>       .options = WDIOF_KEEPALIVEPING,
>>       .identity = "DaVinci Watchdog",
>>   };
>>
>> -static long davinci_wdt_ioctl(struct file *file,
>> -                    unsigned int cmd, unsigned long arg)
>> -{
>> -    int ret = -ENOTTY;
>> -
>> -    switch (cmd) {
>> -    case WDIOC_GETSUPPORT:
>> -        ret = copy_to_user((struct watchdog_info *)arg, &ident,
>> -                   sizeof(ident)) ? -EFAULT : 0;
>> -        break;
>> -
>> -    case WDIOC_GETSTATUS:
>> -    case WDIOC_GETBOOTSTATUS:
>> -        ret = put_user(0, (int *)arg);
>> -        break;
>> -
>> -    case WDIOC_KEEPALIVE:
>> -        wdt_service();
>> -        ret = 0;
>> -        break;
>> -
>> -    case WDIOC_GETTIMEOUT:
>> -        ret = put_user(heartbeat, (int *)arg);
>> -        break;
>> -    }
>> -    return ret;
>> -}
>> -
>> -static int davinci_wdt_release(struct inode *inode, struct file *file)
>> -{
>> -    wdt_service();
>> -    clear_bit(WDT_IN_USE, &wdt_status);
>> -
>> -    return 0;
>> -}
>> -
>> -static const struct file_operations davinci_wdt_fops = {
>> -    .owner = THIS_MODULE,
>> -    .llseek = no_llseek,
>> -    .write = davinci_wdt_write,
>> -    .unlocked_ioctl = davinci_wdt_ioctl,
>> -    .open = davinci_wdt_open,
>> -    .release = davinci_wdt_release,
>> -};
>> -
>> -static struct miscdevice davinci_wdt_miscdev = {
>> -    .minor = WATCHDOG_MINOR,
>> -    .name = "watchdog",
>> -    .fops = &davinci_wdt_fops,
>> +static const struct watchdog_ops davinci_wdt_ops = {
>> +    .owner        = THIS_MODULE,
>> +    .start        = davinci_wdt_start,
>> +    .stop        = davinci_wdt_ping,
>> +    .ping        = davinci_wdt_ping,
>>   };
>>
>>   static int davinci_wdt_probe(struct platform_device *pdev)
>> @@ -204,6 +122,7 @@ static int davinci_wdt_probe(struct 
>> platform_device *pdev)
>>       int ret = 0;
>>       struct device *dev = &pdev->dev;
>>       struct resource  *wdt_mem;
>> +    struct watchdog_device *wdd;
>>
>>       wdt_clk = devm_clk_get(dev, NULL);
>>       if (WARN_ON(IS_ERR(wdt_clk)))
>> @@ -211,29 +130,35 @@ static int davinci_wdt_probe(struct 
>> platform_device *pdev)
>>
>>       clk_prepare_enable(wdt_clk);
>>
>> -    if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
>> -        heartbeat = DEFAULT_HEARTBEAT;
>> +    wdd            = &wdt_wdd;
>> +    wdd->info        = &davinci_wdt_info;
>> +    wdd->ops        = &davinci_wdt_ops;
>> +    wdd->min_timeout     = 1;
>> +    wdd->max_timeout     = MAX_HEARTBEAT;
>> +    wdd->timeout        = DEFAULT_HEARTBEAT;
>> +
>> +    if (heartbeat)
>> +        watchdog_init_timeout(wdd, heartbeat, dev);
>> +
> The if statement here is unnecessary. Actually, you always want to call 
> watchdog_init_timeout()
> to ensure that it gets the configuration from fdt if available.
>

Ok

>> +    dev_info(dev, "heartbeat %d sec\n", wdd->timeout);
>>
>> -    dev_info(dev, "heartbeat %d sec\n", heartbeat);
>> +    watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
>>
>>       wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       wdt_base = devm_ioremap_resource(dev, wdt_mem);
>>       if (IS_ERR(wdt_base))
>>           return PTR_ERR(wdt_base);
>>
>> -    ret = misc_register(&davinci_wdt_miscdev);
>> -    if (ret < 0) {
>> -        dev_err(dev, "cannot register misc device\n");
>> -    } else {
>> -        set_bit(WDT_DEVICE_INITED, &wdt_status);
>> -    }
>> +    ret = watchdog_register_device(wdd);
>> +    if (ret < 0)
>> +        dev_err(dev, "cannot register watchdog device\n");
>>
>>       return ret;
>>   }
>>
>>   static int davinci_wdt_remove(struct platform_device *pdev)
>>   {
>> -    misc_deregister(&davinci_wdt_miscdev);
>> +    watchdog_unregister_device(&wdt_wdd);
>>       clk_disable_unprepare(wdt_clk);
>>
>>       return 0;
>> @@ -267,5 +192,4 @@ MODULE_PARM_DESC(heartbeat,
>>            __MODULE_STRING(DEFAULT_HEARTBEAT));
>>
>>   MODULE_LICENSE("GPL");
>> -MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
>>   MODULE_ALIAS("platform:watchdog");
>>
> You might want to rename the platform driver to something like 
> davinci-wdt and change the MODULE_ALIAS
> accordingly.
>

Yes, it could be changed but not in this patch.

> Guenter

-- 
Regards,
Ivan Khoronzhuk