[RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Mans Rullgard]

This adds support for the Sigma Designs SMP86xx family built-in
watchdog.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/watchdog/Kconfig      |   7 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/watchdog/tangox_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 79e1aa1..0ed5ee8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1337,6 +1337,13 @@ config RALINK_WDT
 	help
 	  Hardware driver for the Ralink SoC Watchdog Timer.
 
+config TANGOX_WDT
+	tristate "SMP86xx watchdog"
+	select WATCHDOG_CORE
+	depends on ARCH_TANGOX
+	help
+	  Watchdog driver for Sigma Designs SMP86xx.
+
 # PARISC Architecture
 
 # POWERPC Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..ed00796 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -148,6 +148,7 @@ octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
 obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
 obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
 obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
+obj-$(CONFIG_TANGOX_WDT) += tangox_wdt.o
 
 # PARISC Architecture
 
diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
new file mode 100644
index 0000000..305759f
--- /dev/null
+++ b/drivers/watchdog/tangox_wdt.c
@@ -0,0 +1,185 @@
+/*
+ *  Copyright (C) 2014, Mans Rullgard <mans@mansr.com>
+ *  SMP86xx Watchdog driver
+ *
+ *  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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/notifier.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#define DEFAULT_TIMEOUT	30
+#define MAX_TIMEOUT	120
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static unsigned int timeout = DEFAULT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(watchdog_timeout, "Watchdog timeout");
+
+#define WD_COUNTER	0
+#define WD_CONFIG	4
+
+struct tangox_wdt_device {
+	struct watchdog_device wdt;
+	void __iomem *base;
+	unsigned int timeout;
+	struct clk *clk;
+	struct notifier_block restart;
+};
+
+static int tangox_wdt_ping(struct watchdog_device *wdt)
+{
+	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
+
+	writel(dev->timeout, dev->base + WD_COUNTER);
+
+	return 0;
+}
+
+static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
+				  unsigned int new_timeout)
+{
+	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
+
+	wdt->timeout = new_timeout;
+	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
+
+	tangox_wdt_ping(wdt);
+
+	return 0;
+}
+
+static int tangox_wdt_start(struct watchdog_device *wdt)
+{
+	return tangox_wdt_set_timeout(wdt, wdt->timeout);
+}
+
+static int tangox_wdt_stop(struct watchdog_device *wdt)
+{
+	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
+
+	writel(0, dev->base + WD_COUNTER);
+
+	return 0;
+}
+
+static const struct watchdog_info tangox_wdt_info = {
+	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "tangox watchdog",
+};
+
+static const struct watchdog_ops tangox_wdt_ops = {
+	.start		= tangox_wdt_start,
+	.stop		= tangox_wdt_stop,
+	.ping		= tangox_wdt_ping,
+	.set_timeout	= tangox_wdt_set_timeout,
+};
+
+static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct tangox_wdt_device *dev =
+		container_of(nb, struct tangox_wdt_device, restart);
+
+	writel(1, dev->base + WD_COUNTER);
+
+	return NOTIFY_DONE;
+}
+
+static int tangox_wdt_probe(struct platform_device *pdev)
+{
+	struct tangox_wdt_device *dev;
+	struct resource *res;
+	int err;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->wdt.parent = &pdev->dev;
+	dev->wdt.info = &tangox_wdt_info;
+	dev->wdt.ops = &tangox_wdt_ops;
+	dev->wdt.timeout = timeout;
+	dev->wdt.min_timeout = 1;
+	dev->wdt.max_timeout = MAX_TIMEOUT;
+	watchdog_set_nowayout(&dev->wdt, nowayout);
+	watchdog_set_drvdata(&dev->wdt, dev);
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	dev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dev->base))
+		return PTR_ERR(dev->base);
+
+	writel(0, dev->base + WD_COUNTER);
+	writel(1, dev->base + WD_CONFIG);
+
+	err = watchdog_register_device(&dev->wdt);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, dev);
+
+	dev->restart.notifier_call = tangox_wdt_restart;
+	dev->restart.priority = 128;
+	err = register_restart_handler(&dev->restart);
+	if (err)
+		dev_err(&pdev->dev, "failed to register restart handler\n");
+
+	dev_info(dev->wdt.dev, "SMP86xx watchdog registered\n");
+
+	return 0;
+}
+
+static int tangox_wdt_remove(struct platform_device *pdev)
+{
+	struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
+
+	tangox_wdt_stop(&dev->wdt);
+	unregister_restart_handler(&dev->restart);
+	watchdog_unregister_device(&dev->wdt);
+
+	return 0;
+}
+
+static const struct of_device_id tangox_wdt_dt_ids[] = {
+	{ .compatible = "sigma,smp8642-wdt" },
+	{ }
+};
+
+static struct platform_driver tangox_wdt_driver = {
+	.probe	= tangox_wdt_probe,
+	.remove	= tangox_wdt_remove,
+	.driver	= {
+		.name		= "tangox-wdt",
+		.of_match_table	= tangox_wdt_dt_ids,
+	},
+};
+
+module_platform_driver(tangox_wdt_driver);
+
+MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
+MODULE_DESCRIPTION("SMP86xx Watchdog driver");
+MODULE_LICENSE("GPL");
-- 
2.6.3

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Guenter Roeck]

On 11/13/2015 05:14 AM, Mans Rullgard wrote:
> This adds support for the Sigma Designs SMP86xx family built-in
> watchdog.
>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>   drivers/watchdog/Kconfig      |   7 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++

Why tangox_wdt instead of smp86xx_wdt.c ?

tangox also implies that this would (should) work for SMP87xx as well,
about which no statement is made. So why not tango3_wdt ?

[ ok, I see all drivers are named tangox, so if the other maintainers
   are ok with that, so am I. ]

Is it known if the driver will work for any of the other chips of the
series (SMP86XX/SMP87XX) ?

I think it would be helpful to describe in more detail which chips
are supported, or at least which chips should work but are untested.

>   3 files changed, 193 insertions(+)
>   create mode 100644 drivers/watchdog/tangox_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 79e1aa1..0ed5ee8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1337,6 +1337,13 @@ config RALINK_WDT
>   	help
>   	  Hardware driver for the Ralink SoC Watchdog Timer.
>
> +config TANGOX_WDT
> +	tristate "SMP86xx watchdog"
> +	select WATCHDOG_CORE
> +	depends on ARCH_TANGOX
> +	help
> +	  Watchdog driver for Sigma Designs SMP86xx.

Not really; it is for SMP8642, and we don't know if other (later ?) chips
will be supported by the same driver. You should be explicit here. More chips
can be added later (that would be needed for the devicetree bindings anyway)
as they are tested.

> +
>   # PARISC Architecture
>
>   # POWERPC Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c616e3..ed00796 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -148,6 +148,7 @@ octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
>   obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
>   obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
>   obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
> +obj-$(CONFIG_TANGOX_WDT) += tangox_wdt.o
>
>   # PARISC Architecture
>
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> new file mode 100644
> index 0000000..305759f
> --- /dev/null
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -0,0 +1,185 @@
> +/*
> + *  Copyright (C) 2014, Mans Rullgard <mans@mansr.com>
> + *  SMP86xx Watchdog driver
> + *
> + *  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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/notifier.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>

Please list include files in alphabetic order.

> +
> +#define DEFAULT_TIMEOUT	30
> +#define MAX_TIMEOUT	120
> +

This looks like an arbitrary value. You should calculate the real
maximum by dividing the maximum register value by the clock rate.

> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +		 "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(watchdog_timeout, "Watchdog timeout");
> +
> +#define WD_COUNTER	0
> +#define WD_CONFIG	4
> +
> +struct tangox_wdt_device {
> +	struct watchdog_device wdt;
> +	void __iomem *base;
> +	unsigned int timeout;
> +	struct clk *clk;
> +	struct notifier_block restart;
> +};
> +
> +static int tangox_wdt_ping(struct watchdog_device *wdt)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	writel(dev->timeout, dev->base + WD_COUNTER);
> +
> +	return 0;
> +}
> +
> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
> +				  unsigned int new_timeout)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	wdt->timeout = new_timeout;
> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);

Why "1 +" ?

> +
> +	tangox_wdt_ping(wdt);
> +
The infrastructure does that already.

> +	return 0;
> +}
> +
> +static int tangox_wdt_start(struct watchdog_device *wdt)
> +{
> +	return tangox_wdt_set_timeout(wdt, wdt->timeout);

This is an odd way of setting dev->timeout.
You should set it in the probe function instead.
Unless I am missing something, the ping and start functions will then
be the same and you'll only need one of them.

> +}
> +
> +static int tangox_wdt_stop(struct watchdog_device *wdt)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	writel(0, dev->base + WD_COUNTER);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info tangox_wdt_info = {
> +	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "tangox watchdog",
> +};
> +
> +static const struct watchdog_ops tangox_wdt_ops = {
> +	.start		= tangox_wdt_start,
> +	.stop		= tangox_wdt_stop,
> +	.ping		= tangox_wdt_ping,
> +	.set_timeout	= tangox_wdt_set_timeout,
> +};
> +
> +static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
> +			      void *data)
> +{
> +	struct tangox_wdt_device *dev =
> +		container_of(nb, struct tangox_wdt_device, restart);
> +
> +	writel(1, dev->base + WD_COUNTER);
> +

A comment might be useful here, explaining what this does (reset after minimum timeout ?).
Also, the code should wait a bit to ensure that the reset 'catches'
before the function returns.

> +	return NOTIFY_DONE;
> +}
> +
> +static int tangox_wdt_probe(struct platform_device *pdev)
> +{
> +	struct tangox_wdt_device *dev;
> +	struct resource *res;
> +	int err;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->wdt.parent = &pdev->dev;
> +	dev->wdt.info = &tangox_wdt_info;
> +	dev->wdt.ops = &tangox_wdt_ops;
> +	dev->wdt.timeout = timeout;

It might make more sense to use watchdog_init_timeout(),
especially since the driver supports devicetree and the timeout
can be set in devicetree data.

> +	dev->wdt.min_timeout = 1;
> +	dev->wdt.max_timeout = MAX_TIMEOUT;
> +	watchdog_set_nowayout(&dev->wdt, nowayout);
> +	watchdog_set_drvdata(&dev->wdt, dev);
> +
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dev->clk))
> +		return PTR_ERR(dev->clk);

Normally one calls clk_prepare_enable() after clk_get()
(and clk_disable_unprepare when exiting).

Also, it might make sense to read the clock rate once and store it
in a variable.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	dev->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dev->base))
> +		return PTR_ERR(dev->base);
> +
> +	writel(0, dev->base + WD_COUNTER);

This always stops the watchdog even if already running.
Is this on purpose ?

> +	writel(1, dev->base + WD_CONFIG);

A define or a comment might help here to explain what this write does.

> +
> +	err = watchdog_register_device(&dev->wdt);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->restart.notifier_call = tangox_wdt_restart;
> +	dev->restart.priority = 128;
> +	err = register_restart_handler(&dev->restart);
> +	if (err)
> +		dev_err(&pdev->dev, "failed to register restart handler\n");

dev_warn

> +
> +	dev_info(dev->wdt.dev, "SMP86xx watchdog registered\n");
> +
> +	return 0;
> +}
> +
> +static int tangox_wdt_remove(struct platform_device *pdev)
> +{
> +	struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
> +
> +	tangox_wdt_stop(&dev->wdt);
> +	unregister_restart_handler(&dev->restart);
> +	watchdog_unregister_device(&dev->wdt);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tangox_wdt_dt_ids[] = {
> +	{ .compatible = "sigma,smp8642-wdt" },

So this is really for smp8642 only, not for any other chips in the series ?

> +	{ }

Do you have a devicetree bindings file ? You should have, and it should
describe all necessary and optional bindings including clocks.

> +};
> +
> +static struct platform_driver tangox_wdt_driver = {
> +	.probe	= tangox_wdt_probe,
> +	.remove	= tangox_wdt_remove,
> +	.driver	= {
> +		.name		= "tangox-wdt",
> +		.of_match_table	= tangox_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(tangox_wdt_driver);
> +
> +MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
> +MODULE_DESCRIPTION("SMP86xx Watchdog driver");
> +MODULE_LICENSE("GPL");
>
MODULE_ALIAS ?

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Måns Rullgård]

Guenter Roeck <linux@roeck-us.net> writes:

> On 11/13/2015 05:14 AM, Mans Rullgard wrote:
>> This adds support for the Sigma Designs SMP86xx family built-in
>> watchdog.
>>
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>   drivers/watchdog/Kconfig      |   7 ++
>>   drivers/watchdog/Makefile     |   1 +
>>   drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
>
> Why tangox_wdt instead of smp86xx_wdt.c ?
>
> tangox also implies that this would (should) work for SMP87xx as well,
> about which no statement is made. So why not tango3_wdt ?
>
> [ ok, I see all drivers are named tangox, so if the other maintainers
>   are ok with that, so am I. ]
>
> Is it known if the driver will work for any of the other chips of the
> series (SMP86XX/SMP87XX) ?

It does work on SMP87xx (tango4) as well.  I wrote the driver before I
had any such hardware, then forgot to update the help text and commit
message.

> I think it would be helpful to describe in more detail which chips
> are supported, or at least which chips should work but are untested.
>
>>   3 files changed, 193 insertions(+)
>>   create mode 100644 drivers/watchdog/tangox_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 79e1aa1..0ed5ee8 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1337,6 +1337,13 @@ config RALINK_WDT
>>   	help
>>   	  Hardware driver for the Ralink SoC Watchdog Timer.
>>
>> +config TANGOX_WDT
>> +	tristate "SMP86xx watchdog"
>> +	select WATCHDOG_CORE
>> +	depends on ARCH_TANGOX
>> +	help
>> +	  Watchdog driver for Sigma Designs SMP86xx.
>
> Not really; it is for SMP8642, and we don't know if other (later ?) chips
> will be supported by the same driver. You should be explicit here. More chips
> can be added later (that would be needed for the devicetree bindings anyway)
> as they are tested.

I have tested it on SMP8642 and SMP8759.  The documentation for SMP8654
agrees.

>> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>> +				  unsigned int new_timeout)
>> +{
>> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>> +
>> +	wdt->timeout = new_timeout;
>> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
>
> Why "1 +" ?

The counter counts down from the loaded value and asserts the reset pin
when it reaches 1.  Setting it to zero disables the watchdog.

>> +static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
>> +			      void *data)
>> +{
>> +	struct tangox_wdt_device *dev =
>> +		container_of(nb, struct tangox_wdt_device, restart);
>> +
>> +	writel(1, dev->base + WD_COUNTER);
>> +
>
> A comment might be useful here, explaining what this does (reset after minimum timeout ?).
> Also, the code should wait a bit to ensure that the reset 'catches'
> before the function returns.

Writing 1 to the counter asserts the reset immediately.

>> +static const struct of_device_id tangox_wdt_dt_ids[] = {
>> +	{ .compatible = "sigma,smp8642-wdt" },
>
> So this is really for smp8642 only, not for any other chips in the series ?

It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips.  Should I
list them all?  I don't even know where to find a comprehensive list of
device numbers.

-- 
Måns Rullgård
mans@mansr.com

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Guenter Roeck]

On 11/13/2015 08:53 AM, Måns Rullgård wrote:
> Guenter Roeck <linux@roeck-us.net> writes:
>
>> On 11/13/2015 05:14 AM, Mans Rullgard wrote:
>>> This adds support for the Sigma Designs SMP86xx family built-in
>>> watchdog.
>>>
>>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>>> ---
>>>    drivers/watchdog/Kconfig      |   7 ++
>>>    drivers/watchdog/Makefile     |   1 +
>>>    drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
>>
>> Why tangox_wdt instead of smp86xx_wdt.c ?
>>
>> tangox also implies that this would (should) work for SMP87xx as well,
>> about which no statement is made. So why not tango3_wdt ?
>>
>> [ ok, I see all drivers are named tangox, so if the other maintainers
>>    are ok with that, so am I. ]
>>
>> Is it known if the driver will work for any of the other chips of the
>> series (SMP86XX/SMP87XX) ?
>
> It does work on SMP87xx (tango4) as well.  I wrote the driver before I
> had any such hardware, then forgot to update the help text and commit
> message.
>
>> I think it would be helpful to describe in more detail which chips
>> are supported, or at least which chips should work but are untested.
>>
>>>    3 files changed, 193 insertions(+)
>>>    create mode 100644 drivers/watchdog/tangox_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 79e1aa1..0ed5ee8 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -1337,6 +1337,13 @@ config RALINK_WDT
>>>    	help
>>>    	  Hardware driver for the Ralink SoC Watchdog Timer.
>>>
>>> +config TANGOX_WDT
>>> +	tristate "SMP86xx watchdog"
>>> +	select WATCHDOG_CORE
>>> +	depends on ARCH_TANGOX
>>> +	help
>>> +	  Watchdog driver for Sigma Designs SMP86xx.
>>
>> Not really; it is for SMP8642, and we don't know if other (later ?) chips
>> will be supported by the same driver. You should be explicit here. More chips
>> can be added later (that would be needed for the devicetree bindings anyway)
>> as they are tested.
>
> I have tested it on SMP8642 and SMP8759.  The documentation for SMP8654
> agrees.
>

We should have that information somewhere - maybe in the driver header.
It is very useful to know which hardware this was tested with and which
hardware is supposed to work.

>>> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>>> +				  unsigned int new_timeout)
>>> +{
>>> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>>> +
>>> +	wdt->timeout = new_timeout;
>>> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
>>
>> Why "1 +" ?
>
> The counter counts down from the loaded value and asserts the reset pin
> when it reaches 1.  Setting it to zero disables the watchdog.
>

You might want to explain that somewhere. Maybe use a define, explain it there,
and use the define here and below.

>>> +static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
>>> +			      void *data)
>>> +{
>>> +	struct tangox_wdt_device *dev =
>>> +		container_of(nb, struct tangox_wdt_device, restart);
>>> +
>>> +	writel(1, dev->base + WD_COUNTER);
>>> +
>>
>> A comment might be useful here, explaining what this does (reset after minimum timeout ?).
>> Also, the code should wait a bit to ensure that the reset 'catches'
>> before the function returns.
>
> Writing 1 to the counter asserts the reset immediately.
>
>>> +static const struct of_device_id tangox_wdt_dt_ids[] = {
>>> +	{ .compatible = "sigma,smp8642-wdt" },
>>
>> So this is really for smp8642 only, not for any other chips in the series ?
>
> It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips.  Should I
> list them all?  I don't even know where to find a comprehensive list of
> device numbers.
>
I thought so, but I am not a devicetree expert, and I see some "xx" in existing
devicetree bindings. Something to ask when you submit the bindings to the
devicetree mailing list. Either case, I think it would be either something
like "sigma,smp86xx-wdt" or a list of all of them, but not "sigma,smp8642-wdt"
to be used for all chips.

As for which chips to list, the easy answer would be to only list the IDs
for chips known to work.

Thanks,
Guenter

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Måns Rullgård]

Guenter Roeck <linux@roeck-us.net> writes:

>>>> +config TANGOX_WDT
>>>> +	tristate "SMP86xx watchdog"
>>>> +	select WATCHDOG_CORE
>>>> +	depends on ARCH_TANGOX
>>>> +	help
>>>> +	  Watchdog driver for Sigma Designs SMP86xx.
>>>
>>> Not really; it is for SMP8642, and we don't know if other (later ?) chips
>>> will be supported by the same driver. You should be explicit here. More chips
>>> can be added later (that would be needed for the devicetree bindings anyway)
>>> as they are tested.
>>
>> I have tested it on SMP8642 and SMP8759.  The documentation for SMP8654
>> agrees.
>>
>
> We should have that information somewhere - maybe in the driver header.
> It is very useful to know which hardware this was tested with and which
> hardware is supposed to work.

OK, I'll add that info somewhere.

>>>> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>>>> +				  unsigned int new_timeout)
>>>> +{
>>>> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>>>> +
>>>> +	wdt->timeout = new_timeout;
>>>> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
>>>
>>> Why "1 +" ?
>>
>> The counter counts down from the loaded value and asserts the reset pin
>> when it reaches 1.  Setting it to zero disables the watchdog.
>
> You might want to explain that somewhere. Maybe use a define, explain
> it there, and use the define here and below.

Will do.

>>>> +static const struct of_device_id tangox_wdt_dt_ids[] = {
>>>> +	{ .compatible = "sigma,smp8642-wdt" },
>>>
>>> So this is really for smp8642 only, not for any other chips in the series ?
>>
>> It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips.  Should I
>> list them all?  I don't even know where to find a comprehensive list of
>> device numbers.
>>
> I thought so, but I am not a devicetree expert, and I see some "xx" in
> existing devicetree bindings. Something to ask when you submit the
> bindings to the devicetree mailing list. Either case, I think it would
> be either something like "sigma,smp86xx-wdt" or a list of all of them,
> but not "sigma,smp8642-wdt" to be used for all chips.

The general advice is to not use wildcards in DT bindings since the next
chip matching the pattern might not be compatible at all.  New chips
known to be compatible with an old one can specify both the exact chip
and the older one such that existing drivers will work automatically.
If at some point they are found not to be compatible after all (hardware
bugs, perhaps) a fixed driver will work with existing device trees.

Thanks for the review.

-- 
Måns Rullgård
mans@mansr.com

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Guenter Roeck]

On 11/13/2015 10:02 AM, Måns Rullgård wrote:
>>>>> +static const struct of_device_id tangox_wdt_dt_ids[] = {
>>>>> +	{ .compatible = "sigma,smp8642-wdt" },
>>>>
>>>> So this is really for smp8642 only, not for any other chips in the series ?
>>>
>>> It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips.  Should I
>>> list them all?  I don't even know where to find a comprehensive list of
>>> device numbers.
>>>
>> I thought so, but I am not a devicetree expert, and I see some "xx" in
>> existing devicetree bindings. Something to ask when you submit the
>> bindings to the devicetree mailing list. Either case, I think it would
>> be either something like "sigma,smp86xx-wdt" or a list of all of them,
>> but not "sigma,smp8642-wdt" to be used for all chips.
>
> The general advice is to not use wildcards in DT bindings since the next
> chip matching the pattern might not be compatible at all.  New chips
> known to be compatible with an old one can specify both the exact chip
> and the older one such that existing drivers will work automatically.
> If at some point they are found not to be compatible after all (hardware
> bugs, perhaps) a fixed driver will work with existing device trees.
>
So I think the best approach here would be to list the chips known to work.

Thanks,
Guenter

Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Guenter Roeck]

On 11/13/2015 05:14 AM, Mans Rullgard wrote:
> This adds support for the Sigma Designs SMP86xx family built-in
> watchdog.
>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>   drivers/watchdog/Kconfig      |   7 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 193 insertions(+)
>   create mode 100644 drivers/watchdog/tangox_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 79e1aa1..0ed5ee8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1337,6 +1337,13 @@ config RALINK_WDT
>   	help
>   	  Hardware driver for the Ralink SoC Watchdog Timer.
>
> +config TANGOX_WDT
> +	tristate "SMP86xx watchdog"
> +	select WATCHDOG_CORE
> +	depends on ARCH_TANGOX

Please make this "depends on ARCH_TANGOX | COMPILE_TEST" so the driver
can be compile-tested without ARCH_TANGOX.

One more comment below.

Thanks,
Guenter

> +	help
> +	  Watchdog driver for Sigma Designs SMP86xx.
> +
>   # PARISC Architecture
>
>   # POWERPC Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c616e3..ed00796 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -148,6 +148,7 @@ octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
>   obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
>   obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
>   obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
> +obj-$(CONFIG_TANGOX_WDT) += tangox_wdt.o
>
>   # PARISC Architecture
>
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> new file mode 100644
> index 0000000..305759f
> --- /dev/null
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -0,0 +1,185 @@
> +/*
> + *  Copyright (C) 2014, Mans Rullgard <mans@mansr.com>
> + *  SMP86xx Watchdog driver
> + *
> + *  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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/notifier.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +#define DEFAULT_TIMEOUT	30
> +#define MAX_TIMEOUT	120
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +		 "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(watchdog_timeout, "Watchdog timeout");
> +
> +#define WD_COUNTER	0
> +#define WD_CONFIG	4
> +
> +struct tangox_wdt_device {
> +	struct watchdog_device wdt;
> +	void __iomem *base;
> +	unsigned int timeout;
> +	struct clk *clk;
> +	struct notifier_block restart;
> +};
> +
> +static int tangox_wdt_ping(struct watchdog_device *wdt)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	writel(dev->timeout, dev->base + WD_COUNTER);
> +
> +	return 0;
> +}
> +
> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
> +				  unsigned int new_timeout)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	wdt->timeout = new_timeout;
> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
> +
> +	tangox_wdt_ping(wdt);
> +
> +	return 0;
> +}
> +
> +static int tangox_wdt_start(struct watchdog_device *wdt)
> +{
> +	return tangox_wdt_set_timeout(wdt, wdt->timeout);
> +}
> +
> +static int tangox_wdt_stop(struct watchdog_device *wdt)
> +{
> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> +
> +	writel(0, dev->base + WD_COUNTER);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info tangox_wdt_info = {
> +	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "tangox watchdog",
> +};
> +
> +static const struct watchdog_ops tangox_wdt_ops = {
> +	.start		= tangox_wdt_start,
> +	.stop		= tangox_wdt_stop,
> +	.ping		= tangox_wdt_ping,
> +	.set_timeout	= tangox_wdt_set_timeout,
> +};
> +
> +static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
> +			      void *data)
> +{
> +	struct tangox_wdt_device *dev =
> +		container_of(nb, struct tangox_wdt_device, restart);
> +
> +	writel(1, dev->base + WD_COUNTER);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int tangox_wdt_probe(struct platform_device *pdev)
> +{
> +	struct tangox_wdt_device *dev;
> +	struct resource *res;
> +	int err;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->wdt.parent = &pdev->dev;
> +	dev->wdt.info = &tangox_wdt_info;
> +	dev->wdt.ops = &tangox_wdt_ops;
> +	dev->wdt.timeout = timeout;
> +	dev->wdt.min_timeout = 1;
> +	dev->wdt.max_timeout = MAX_TIMEOUT;
> +	watchdog_set_nowayout(&dev->wdt, nowayout);
> +	watchdog_set_drvdata(&dev->wdt, dev);
> +
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dev->clk))
> +		return PTR_ERR(dev->clk);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
Unnecessary error check; devm_ioremap_resource() generates an error
if res is NULL.

> +	dev->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dev->base))
> +		return PTR_ERR(dev->base);
> +
> +	writel(0, dev->base + WD_COUNTER);
> +	writel(1, dev->base + WD_CONFIG);
> +
> +	err = watchdog_register_device(&dev->wdt);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->restart.notifier_call = tangox_wdt_restart;
> +	dev->restart.priority = 128;
> +	err = register_restart_handler(&dev->restart);
> +	if (err)
> +		dev_err(&pdev->dev, "failed to register restart handler\n");
> +
> +	dev_info(dev->wdt.dev, "SMP86xx watchdog registered\n");
> +
> +	return 0;
> +}
> +
> +static int tangox_wdt_remove(struct platform_device *pdev)
> +{
> +	struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
> +
> +	tangox_wdt_stop(&dev->wdt);
> +	unregister_restart_handler(&dev->restart);
> +	watchdog_unregister_device(&dev->wdt);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tangox_wdt_dt_ids[] = {
> +	{ .compatible = "sigma,smp8642-wdt" },
> +	{ }
> +};
> +
> +static struct platform_driver tangox_wdt_driver = {
> +	.probe	= tangox_wdt_probe,
> +	.remove	= tangox_wdt_remove,
> +	.driver	= {
> +		.name		= "tangox-wdt",
> +		.of_match_table	= tangox_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(tangox_wdt_driver);
> +
> +MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
> +MODULE_DESCRIPTION("SMP86xx Watchdog driver");
> +MODULE_LICENSE("GPL");
>