[RFC/RFT PATCH] watchdog: rdc321x_wdt: Convert to use watchdog subsystem

From: Guenter Roeck
Date: Fri Aug 07 2020 - 19:30:43 EST


Convert driver to use watchdog subsystem.

RFC/RFT: Completely untested. Also see FIXME comments in code.

Notes:
- The driver no longer uses platform data.
Using resources to pass PCI configuration register addresses
does not make much sense, and the PCI device can be obtained
with to_pci_dev(dev->parent).
- WDIOC_GETSTATUS no longer returns the raw watchdog register
value.
- WDIOS_DISABLECARD no longer returns -EIO but just stops
the watchdog.

Cc: Florian Fainelli <f.fainelli@xxxxxxxxx>
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
drivers/watchdog/rdc321x_wdt.c | 274 +++++++++------------------------
1 file changed, 69 insertions(+), 205 deletions(-)

diff --git a/drivers/watchdog/rdc321x_wdt.c b/drivers/watchdog/rdc321x_wdt.c
index 57187efeb86f..5a3eaca6b9f8 100644
--- a/drivers/watchdog/rdc321x_wdt.c
+++ b/drivers/watchdog/rdc321x_wdt.c
@@ -8,19 +8,10 @@
*/

#include <linux/module.h>
-#include <linux/moduleparam.h>
#include <linux/types.h>
#include <linux/errno.h>
-#include <linux/miscdevice.h>
-#include <linux/fs.h>
-#include <linux/ioport.h>
-#include <linux/timer.h>
-#include <linux/completion.h>
-#include <linux/jiffies.h>
#include <linux/platform_device.h>
#include <linux/watchdog.h>
-#include <linux/io.h>
-#include <linux/uaccess.h>
#include <linux/mfd/rdc321x.h>

#define RDC_WDT_MASK 0x80000000 /* Mask */
@@ -32,247 +23,120 @@
#define RDC_WDT_CNT 0x00000001 /* WDT count */

#define RDC_CLS_TMR 0x80003844 /* Clear timer */
+ // xxxx44 -> 2.34s or 2.68s timeout
+ // (or maybe 2.5s / 2.85s)
+ // xxx8xx -> route to irq[1]
+ // xx3xxx -> reserved
+ // (should that have been 3xxxxx ?)
+ // 8xxxxxxx -> not defined in datasheet

-#define RDC_WDT_INTERVAL (HZ/10+1)
+#define DEFAULT_TIMEOUT 30 /* seconds */

-static int ticks = 1000;
-
-/* some device data */
-
-static struct {
- struct completion stop;
- int running;
- struct timer_list timer;
- int queue;
- int default_ticks;
- unsigned long inuse;
+struct rdc321x_wdt_device {
+ struct watchdog_device wdd;
spinlock_t lock;
struct pci_dev *sb_pdev;
- int base_reg;
-} rdc321x_wdt_device;
-
-/* generic helper functions */
+};

-static void rdc321x_wdt_trigger(struct timer_list *unused)
+static int rdc321x_wdt_ping(struct watchdog_device *wdd)
{
+ struct rdc321x_wdt_device *wdt = watchdog_get_drvdata(wdd);
unsigned long flags;
u32 val;

- if (rdc321x_wdt_device.running)
- ticks--;
-
- /* keep watchdog alive */
- spin_lock_irqsave(&rdc321x_wdt_device.lock, flags);
- pci_read_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg, &val);
+ spin_lock_irqsave(&wdt->lock, flags);
+ /* FIXME it appears that writing RDC_WDT_EN restarts the timer */
+ pci_read_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, &val);
val |= RDC_WDT_EN;
- pci_write_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg, val);
- spin_unlock_irqrestore(&rdc321x_wdt_device.lock, flags);
-
- /* requeue?? */
- if (rdc321x_wdt_device.queue && ticks)
- mod_timer(&rdc321x_wdt_device.timer,
- jiffies + RDC_WDT_INTERVAL);
- else {
- /* ticks doesn't matter anyway */
- complete(&rdc321x_wdt_device.stop);
- }
-
-}
-
-static void rdc321x_wdt_reset(void)
-{
- ticks = rdc321x_wdt_device.default_ticks;
-}
-
-static void rdc321x_wdt_start(void)
-{
- unsigned long flags;
-
- if (!rdc321x_wdt_device.queue) {
- rdc321x_wdt_device.queue = 1;
-
- /* Clear the timer */
- spin_lock_irqsave(&rdc321x_wdt_device.lock, flags);
- pci_write_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg, RDC_CLS_TMR);
-
- /* Enable watchdog and set the timeout to 81.92 us */
- pci_write_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg,
- RDC_WDT_EN | RDC_WDT_CNT);
- spin_unlock_irqrestore(&rdc321x_wdt_device.lock, flags);
-
- mod_timer(&rdc321x_wdt_device.timer,
- jiffies + RDC_WDT_INTERVAL);
- }
+ pci_write_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, val);
+ spin_unlock_irqrestore(&wdt->lock, flags);

- /* if process dies, counter is not decremented */
- rdc321x_wdt_device.running++;
-}
-
-static int rdc321x_wdt_stop(void)
-{
- if (rdc321x_wdt_device.running)
- rdc321x_wdt_device.running = 0;
-
- ticks = rdc321x_wdt_device.default_ticks;
-
- return -EIO;
-}
-
-/* filesystem operations */
-static int rdc321x_wdt_open(struct inode *inode, struct file *file)
-{
- if (test_and_set_bit(0, &rdc321x_wdt_device.inuse))
- return -EBUSY;
-
- return stream_open(inode, file);
-}
-
-static int rdc321x_wdt_release(struct inode *inode, struct file *file)
-{
- clear_bit(0, &rdc321x_wdt_device.inuse);
return 0;
}

-static long rdc321x_wdt_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
+static int rdc321x_wdt_start(struct watchdog_device *wdd)
{
- void __user *argp = (void __user *)arg;
- u32 value;
- static const struct watchdog_info ident = {
- .options = WDIOF_CARDRESET,
- .identity = "RDC321x WDT",
- };
+ struct rdc321x_wdt_device *wdt = watchdog_get_drvdata(wdd);
unsigned long flags;

- switch (cmd) {
- case WDIOC_KEEPALIVE:
- rdc321x_wdt_reset();
- break;
- case WDIOC_GETSTATUS:
- /* Read the value from the DATA register */
- spin_lock_irqsave(&rdc321x_wdt_device.lock, flags);
- pci_read_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg, &value);
- spin_unlock_irqrestore(&rdc321x_wdt_device.lock, flags);
- if (copy_to_user(argp, &value, sizeof(u32)))
- return -EFAULT;
- break;
- case WDIOC_GETSUPPORT:
- if (copy_to_user(argp, &ident, sizeof(ident)))
- return -EFAULT;
- break;
- case WDIOC_SETOPTIONS:
- if (copy_from_user(&value, argp, sizeof(int)))
- return -EFAULT;
- switch (value) {
- case WDIOS_ENABLECARD:
- rdc321x_wdt_start();
- break;
- case WDIOS_DISABLECARD:
- return rdc321x_wdt_stop();
- default:
- return -EINVAL;
- }
- break;
- default:
- return -ENOTTY;
- }
+ spin_lock_irqsave(&wdt->lock, flags);
+ /* Clear the timer */
+ pci_write_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, RDC_CLS_TMR);
+ /* Enable watchdog and set the timeout to 81.92 us */
+ // FIXME The above comment doesn't really make sense.
+ // The kernel would be unable to handle a timeout of 81.92 us.
+ // Also, the code used to generate a heartbeat every ~100 ms.
+ pci_write_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, RDC_WDT_EN | RDC_WDT_CNT);
+ spin_unlock_irqrestore(&wdt->lock, flags);
+
return 0;
}

-static ssize_t rdc321x_wdt_write(struct file *file, const char __user *buf,
- size_t count, loff_t *ppos)
+static int rdc321x_wdt_stop(struct watchdog_device *wdd)
{
- if (!count)
- return -EIO;
+ struct rdc321x_wdt_device *wdt = watchdog_get_drvdata(wdd);

- rdc321x_wdt_reset();
+ pci_write_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, 0);

- return count;
+ return 0;
}

-static const struct file_operations rdc321x_wdt_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .unlocked_ioctl = rdc321x_wdt_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
- .open = rdc321x_wdt_open,
- .write = rdc321x_wdt_write,
- .release = rdc321x_wdt_release,
+static const struct watchdog_ops rdc321x_wdt_ops = {
+ .start = rdc321x_wdt_start,
+ .stop = rdc321x_wdt_stop,
+ .ping = rdc321x_wdt_ping,
+ // FIXME We can't set the timeout since we don't know
+ // how to write the counter register bits.
+ // .set_timeout = rdc321x_wdt_set_timeout,
};

-static struct miscdevice rdc321x_wdt_misc = {
- .minor = WATCHDOG_MINOR,
- .name = "watchdog",
- .fops = &rdc321x_wdt_fops,
+static struct watchdog_info rdc321x_wdt_info = {
+ .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_CARDRESET,
+ .identity = "RDC321x WDT",
};

static int rdc321x_wdt_probe(struct platform_device *pdev)
{
- int err;
- struct resource *r;
- struct rdc321x_wdt_pdata *pdata;
+ struct device *dev = &pdev->dev;
+ struct rdc321x_wdt_device *wdt;
+ struct watchdog_device *wdd;
+ u32 val;

- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data supplied\n");
+ if (!dev->parent) {
+ dev_err(dev, "no parent device\n");
return -ENODEV;
}

- r = platform_get_resource_byname(pdev, IORESOURCE_IO, "wdt-reg");
- if (!r) {
- dev_err(&pdev->dev, "failed to get wdt-reg resource\n");
- return -ENODEV;
- }
+ wdt = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;

- rdc321x_wdt_device.sb_pdev = pdata->sb_pdev;
- rdc321x_wdt_device.base_reg = r->start;
+ wdt->sb_pdev = to_pci_dev(dev->parent);

- err = misc_register(&rdc321x_wdt_misc);
- if (err < 0) {
- dev_err(&pdev->dev, "misc_register failed\n");
- return err;
- }
+ spin_lock_init(&wdt->lock);

- spin_lock_init(&rdc321x_wdt_device.lock);
+ wdd = &wdt->wdd;
+ wdd->parent = dev;
+ wdd->info = &rdc321x_wdt_info,
+ wdd->min_timeout = 1,
+ // FIXME
+ // wdd->max_hw_heartbeat_ms = 4690, /* Assumes external clock */
+ wdd->max_hw_heartbeat_ms = 2340, /* guess */
+ wdd->timeout = DEFAULT_TIMEOUT,
+ wdd->ops = &rdc321x_wdt_ops,
+ watchdog_set_drvdata(wdd, wdt);

- /* Reset the watchdog */
- pci_write_config_dword(rdc321x_wdt_device.sb_pdev,
- rdc321x_wdt_device.base_reg, RDC_WDT_RST);
+ pci_read_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, &val);
+ if (val & RDC_WDT_RST)
+ wdd->bootstatus = WDIOF_CARDRESET;

- init_completion(&rdc321x_wdt_device.stop);
- rdc321x_wdt_device.queue = 0;
+ pci_write_config_dword(wdt->sb_pdev, RDC321X_WDT_CTRL, RDC_WDT_RST);

- clear_bit(0, &rdc321x_wdt_device.inuse);
-
- timer_setup(&rdc321x_wdt_device.timer, rdc321x_wdt_trigger, 0);
-
- rdc321x_wdt_device.default_ticks = ticks;
-
- dev_info(&pdev->dev, "watchdog init success\n");
-
- return 0;
-}
-
-static int rdc321x_wdt_remove(struct platform_device *pdev)
-{
- if (rdc321x_wdt_device.queue) {
- rdc321x_wdt_device.queue = 0;
- wait_for_completion(&rdc321x_wdt_device.stop);
- }
-
- misc_deregister(&rdc321x_wdt_misc);
-
- return 0;
+ return devm_watchdog_register_device(dev, wdd);
}

static struct platform_driver rdc321x_wdt_driver = {
.probe = rdc321x_wdt_probe,
- .remove = rdc321x_wdt_remove,
.driver = {
.name = "rdc321x-wdt",
},
--
2.17.1