[PATCH v4 0/3] watchdog: add wdt and reset for renesas r7s72100

[PATCH v4 0/3] watchdog: add wdt and reset for renesas r7s72100

[Chris Brandt]

Some Renesas SoCs do not have a reset register and the only way to do a SW
controlled reset is to use the watchdog timer. So while this series started
out by only adding a reset feature, now it's a full watchdog timer driver that
includes a reset handler.

The longest WDT overflow you can get with a RZ/A1 (R7S72100) with its 8-bit
wide counter is 125ms. Not very long.

However, by setting max_hw_heartbeat_ms, the watchdog core will now handle
pinging the WDT automatically and allow the user to set times as big as they
want. Therefore, the default timeout for this driver is 30 seconds.

Of course if system interrupts are disabled too long and wdt can't be pinged
by the watchdog core, then the system will reset before the user specified
timeout. But hey, it's better nothing.

This driver was tested on an RZ/A1 RSK board using watchdog-simple.c from
samples/watchdog/.

v4:
* r7s72100.dtsi: changed from timer@ to watchdog@

v3:
* changed from a reset driver to a watchdog timer driver
* use udelay(20) instead of msleep for reset handler
* added Reviewed-by for r7s72100.dtsi and renesas-wdt.txt
* added Acked-by for renesas-wdt.txt

v2:
* added to renesas-wdt.txt instead of creating a new file
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added "renesas,rza-wdt" as a fallback
* added interupt property (even though it is not used)
* added clocks property
* changed hard coded register values to defines
* added msleep to while(1) loop
* removed unnecessary #include files
* added Reviewed-by: Geert Uytterhoeven for renesas-reset.c

Chris Brandt (3):
  watchdog: add rza_wdt driver
  watchdog: renesas-wdt: add support for rza
  ARM: dts: r7s72100: Add watchdog timer

 .../devicetree/bindings/watchdog/renesas-wdt.txt   |   4 +-
 arch/arm/boot/dts/r7s72100.dtsi                    |   7 +
 drivers/watchdog/Kconfig                           |   8 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/rza_wdt.c                         | 208 +++++++++++++++++++++
 5 files changed, 227 insertions(+), 1 deletion(-)
 create mode 100644 drivers/watchdog/rza_wdt.c

-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/3] watchdog: add wdt and reset for renesas r7s72100

[Chris Brandt]

Some Renesas SoCs do not have a reset register and the only way to do a SW
controlled reset is to use the watchdog timer. So while this series started
out by only adding a reset feature, now it's a full watchdog timer driver that
includes a reset handler.

The longest WDT overflow you can get with a RZ/A1 (R7S72100) with its 8-bit
wide counter is 125ms. Not very long.

However, by setting max_hw_heartbeat_ms, the watchdog core will now handle
pinging the WDT automatically and allow the user to set times as big as they
want. Therefore, the default timeout for this driver is 30 seconds.

Of course if system interrupts are disabled too long and wdt can't be pinged
by the watchdog core, then the system will reset before the user specified
timeout. But hey, it's better nothing.

This driver was tested on an RZ/A1 RSK board using watchdog-simple.c from
samples/watchdog/.

v4:
* r7s72100.dtsi: changed from timer@ to watchdog@

v3:
* changed from a reset driver to a watchdog timer driver
* use udelay(20) instead of msleep for reset handler
* added Reviewed-by for r7s72100.dtsi and renesas-wdt.txt
* added Acked-by for renesas-wdt.txt

v2:
* added to renesas-wdt.txt instead of creating a new file
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added "renesas,rza-wdt" as a fallback
* added interupt property (even though it is not used)
* added clocks property
* changed hard coded register values to defines
* added msleep to while(1) loop
* removed unnecessary #include files
* added Reviewed-by: Geert Uytterhoeven for renesas-reset.c

Chris Brandt (3):
  watchdog: add rza_wdt driver
  watchdog: renesas-wdt: add support for rza
  ARM: dts: r7s72100: Add watchdog timer

 .../devicetree/bindings/watchdog/renesas-wdt.txt   |   4 +-
 arch/arm/boot/dts/r7s72100.dtsi                    |   7 +
 drivers/watchdog/Kconfig                           |   8 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/rza_wdt.c                         | 208 +++++++++++++++++++++
 5 files changed, 227 insertions(+), 1 deletion(-)
 create mode 100644 drivers/watchdog/rza_wdt.c

-- 
2.10.1

[PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
 	  This driver adds watchdog support for the integrated watchdogs in the
 	  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
 
+config RENESAS_RZAWDT
+	tristate "Renesas RZ/A WDT Watchdog"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This driver adds watchdog support for the integrated watchdogs in the
+	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
 	tristate "Aspeed 2400 watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 
 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 0000000..17442c5
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,208 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+
+#define DEFAULT_TIMEOUT 30
+
+/* Watchdog Timer Registers */
+#define WTCSR 0
+#define WTCSR_MAGIC 0xA500
+#define WTSCR_WT (1<<6)
+#define WTSCR_TME (1<<5)
+#define WTSCR_CKS(i) i
+
+#define WTCNT 2
+#define WTCNT_MAGIC 0x5A00
+
+#define WRCSR 4
+#define WRCSR_MAGIC 0x5A00
+#define WRCSR_RSTE (1<<6)
+#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
+
+struct rza_wdt {
+	struct watchdog_device wdev;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	/* Stop timer */
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	/* Must dummy read WRCSR:WOVF at least once before clearing */
+	readb(priv->base + WRCSR);
+	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+	/*
+	 * Start timer with slowest clock source and reset option enabled.
+	 */
+	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+	       priv->base + WTCSR);
+
+	return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+	return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+			    void *data)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	/* Stop timer */
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	/* Must dummy read WRCSR:WOVF at least once before clearing */
+	readb(priv->base + WRCSR);
+	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+	/*
+	 * Start timer with fastest clock source and only 1 clock left before
+	 * overflow with reset option enabled.
+	 */
+	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+	writew(WTCNT_MAGIC | 255, priv->base + WTCNT);
+	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR);
+
+	/*
+	 * Actually make sure the above sequence hits hardware before sleeping.
+	 */
+	wmb();
+
+	/* Wait for WDT overflow (reset) */
+	udelay(20);
+
+	return 0;
+}
+
+static const struct watchdog_info rza_wdt_ident = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity = "Renesas RZ/A WDT Watchdog",
+};
+
+static const struct watchdog_ops rza_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = rza_wdt_start,
+	.stop = rza_wdt_stop,
+	.ping = rza_wdt_ping,
+	.restart = rza_wdt_restart,
+};
+
+static int rza_wdt_probe(struct platform_device *pdev)
+{
+	struct rza_wdt *priv;
+	struct resource *res;
+	unsigned long rate;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -ENOENT;
+
+	/* Assume slowest clock rate possible (CKS=7) */
+	rate /= 16384;
+
+	priv->wdev.info = &rza_wdt_ident,
+	priv->wdev.ops = &rza_wdt_ops,
+	priv->wdev.parent = &pdev->dev;
+
+	/*
+	 * Since the max possible timeout of our 8-bit count register is less
+	 * than a second, we must use max_hw_heartbeat_ms.
+	 */
+	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
+	dev_info(&pdev->dev, "max hw timeout of %dms\n",
+		 priv->wdev.max_hw_heartbeat_ms);
+
+	priv->wdev.min_timeout = 1;
+	priv->wdev.timeout = DEFAULT_TIMEOUT;
+
+	platform_set_drvdata(pdev, priv);
+	watchdog_set_drvdata(&priv->wdev, priv);
+
+	ret = watchdog_register_device(&priv->wdev);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int rza_wdt_remove(struct platform_device *pdev)
+{
+	struct rza_wdt *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdev);
+	iounmap(priv->base);
+	return 0;
+}
+
+static const struct of_device_id rza_wdt_of_match[] = {
+	{ .compatible = "renesas,rza-wdt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rza_wdt_of_match);
+
+static struct platform_driver rza_wdt_driver = {
+	.probe = rza_wdt_probe,
+	.remove = rza_wdt_remove,
+	.driver = {
+		.name = "rza_wdt",
+		.of_match_table = rza_wdt_of_match,
+	},
+};
+
+module_platform_driver(rza_wdt_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/A WDT Driver");
+MODULE_AUTHOR("Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
 	  This driver adds watchdog support for the integrated watchdogs in the
 	  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
 
+config RENESAS_RZAWDT
+	tristate "Renesas RZ/A WDT Watchdog"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This driver adds watchdog support for the integrated watchdogs in the
+	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
 	tristate "Aspeed 2400 watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 
 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 0000000..17442c5
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,208 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+
+#define DEFAULT_TIMEOUT 30
+
+/* Watchdog Timer Registers */
+#define WTCSR 0
+#define WTCSR_MAGIC 0xA500
+#define WTSCR_WT (1<<6)
+#define WTSCR_TME (1<<5)
+#define WTSCR_CKS(i) i
+
+#define WTCNT 2
+#define WTCNT_MAGIC 0x5A00
+
+#define WRCSR 4
+#define WRCSR_MAGIC 0x5A00
+#define WRCSR_RSTE (1<<6)
+#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
+
+struct rza_wdt {
+	struct watchdog_device wdev;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	/* Stop timer */
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	/* Must dummy read WRCSR:WOVF at least once before clearing */
+	readb(priv->base + WRCSR);
+	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+	/*
+	 * Start timer with slowest clock source and reset option enabled.
+	 */
+	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+	       priv->base + WTCSR);
+
+	return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+	return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+			    void *data)
+{
+	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+	/* Stop timer */
+	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+	/* Must dummy read WRCSR:WOVF at least once before clearing */
+	readb(priv->base + WRCSR);
+	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+	/*
+	 * Start timer with fastest clock source and only 1 clock left before
+	 * overflow with reset option enabled.
+	 */
+	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+	writew(WTCNT_MAGIC | 255, priv->base + WTCNT);
+	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR);
+
+	/*
+	 * Actually make sure the above sequence hits hardware before sleeping.
+	 */
+	wmb();
+
+	/* Wait for WDT overflow (reset) */
+	udelay(20);
+
+	return 0;
+}
+
+static const struct watchdog_info rza_wdt_ident = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity = "Renesas RZ/A WDT Watchdog",
+};
+
+static const struct watchdog_ops rza_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = rza_wdt_start,
+	.stop = rza_wdt_stop,
+	.ping = rza_wdt_ping,
+	.restart = rza_wdt_restart,
+};
+
+static int rza_wdt_probe(struct platform_device *pdev)
+{
+	struct rza_wdt *priv;
+	struct resource *res;
+	unsigned long rate;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -ENOENT;
+
+	/* Assume slowest clock rate possible (CKS=7) */
+	rate /= 16384;
+
+	priv->wdev.info = &rza_wdt_ident,
+	priv->wdev.ops = &rza_wdt_ops,
+	priv->wdev.parent = &pdev->dev;
+
+	/*
+	 * Since the max possible timeout of our 8-bit count register is less
+	 * than a second, we must use max_hw_heartbeat_ms.
+	 */
+	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
+	dev_info(&pdev->dev, "max hw timeout of %dms\n",
+		 priv->wdev.max_hw_heartbeat_ms);
+
+	priv->wdev.min_timeout = 1;
+	priv->wdev.timeout = DEFAULT_TIMEOUT;
+
+	platform_set_drvdata(pdev, priv);
+	watchdog_set_drvdata(&priv->wdev, priv);
+
+	ret = watchdog_register_device(&priv->wdev);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int rza_wdt_remove(struct platform_device *pdev)
+{
+	struct rza_wdt *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdev);
+	iounmap(priv->base);
+	return 0;
+}
+
+static const struct of_device_id rza_wdt_of_match[] = {
+	{ .compatible = "renesas,rza-wdt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rza_wdt_of_match);
+
+static struct platform_driver rza_wdt_driver = {
+	.probe = rza_wdt_probe,
+	.remove = rza_wdt_remove,
+	.driver = {
+		.name = "rza_wdt",
+		.of_match_table = rza_wdt_of_match,
+	},
+};
+
+module_platform_driver(rza_wdt_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/A WDT Driver");
+MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.10.1

[PATCH v4 2/3] watchdog: renesas-wdt: add support for rza

[Chris Brandt]

Describe the WDT hardware in the RZ/A series.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rob Herring <robh@kernel.org>
---
v3:
* Add Acked-by, Reviewed-by.
v2:
* added to renesas-wdt.txt instead of creating a new file
* changed commit title
* added "renesas,rza-wdt" as a fallback
* added interrupts property
---
 Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index da24e31..9e306af 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -2,10 +2,11 @@ Renesas Watchdog Timer (WDT) Controller
 
 Required properties:
 - compatible : Should be "renesas,<soctype>-wdt", and
-	       "renesas,rcar-gen3-wdt" as fallback.
+	       "renesas,rcar-gen3-wdt" or "renesas,rza-wdt" as fallback.
 	       Examples with soctypes are:
 	         - "renesas,r8a7795-wdt" (R-Car H3)
 	         - "renesas,r8a7796-wdt" (R-Car M3-W)
+	         - "renesas,r7s72100-wdt" (RZ/A1)
 
   When compatible with the generic version, nodes must list the SoC-specific
   version corresponding to the platform first, followed by the generic
@@ -17,6 +18,7 @@ Required properties:
 Optional properties:
 - timeout-sec : Contains the watchdog timeout in seconds
 - power-domains : the power domain the WDT belongs to
+- interrupts: Some WDTs have an interrupt when used in interval timer mode
 
 Examples:
 
-- 
2.10.1

[PATCH v4 3/3] ARM: dts: r7s72100: Add watchdog timer

[Chris Brandt]

Add watchdog timer support for RZ/A1.
For the RZ/A1, the only way to do a reset is to overflow the WDT, so this
is useful even if you don't need the watchdog functionality.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
* changed from timer@ to watchdog@
v3:
* added Reviewed-by
v2:
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added interupt property (even though it is not used)
* added clocks property
---
 arch/arm/boot/dts/r7s72100.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ed62e19..22f96b7 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -382,6 +382,13 @@
 		cache-level = <2>;
 	};
 
+	wdt: watchdog@fcfe0000 {
+		compatible = "renesas,r7s72100-wdt", "renesas,rza-wdt";
+		reg = <0xfcfe0000 0x6>;
+		interrupts = <GIC_SPI 106 IRQ_TYPE_EDGE_RISING>;
+		clocks = <&p0_clk>;
+	};
+
 	i2c0: i2c@fcfee000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.10.1

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On 03/02/2017 05:57 AM, Chris Brandt wrote:
> Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
> handler is also included since a WDT overflow is the only method for
> restarting an RZ/A SoC.
>
> Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/watchdog/Kconfig   |   8 ++
>  drivers/watchdog/Makefile  |   1 +
>  drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/watchdog/rza_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b5..123c516 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -701,6 +701,14 @@ config RENESAS_WDT
>  	  This driver adds watchdog support for the integrated watchdogs in the
>  	  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
>
> +config RENESAS_RZAWDT
> +	tristate "Renesas RZ/A WDT Watchdog"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  This driver adds watchdog support for the integrated watchdogs in the
> +	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
> +
>  config ASPEED_WATCHDOG
>  	tristate "Aspeed 2400 watchdog support"
>  	depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c3d35e..84b897c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>  obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>  obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
>  obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> +obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
>  obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>
>  # AVR32 Architecture
> diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> new file mode 100644
> index 0000000..17442c5
> --- /dev/null
> +++ b/drivers/watchdog/rza_wdt.c
> @@ -0,0 +1,208 @@
> +/*
> + * Renesas RZ/A Series WDT Driver
> + *
> + * Copyright (C) 2017 Renesas Electronics America, Inc.
> + * Copyright (C) 2017 Chris Brandt
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + *

The above two lines are unnecessary.

> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/delay.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +
> +#define DEFAULT_TIMEOUT 30
> +
> +/* Watchdog Timer Registers */
> +#define WTCSR 0
> +#define WTCSR_MAGIC 0xA500
> +#define WTSCR_WT (1<<6)
> +#define WTSCR_TME (1<<5)

BIT()

> +#define WTSCR_CKS(i) i

(i)

> +
> +#define WTCNT 2
> +#define WTCNT_MAGIC 0x5A00
> +
> +#define WRCSR 4
> +#define WRCSR_MAGIC 0x5A00
> +#define WRCSR_RSTE (1<<6)

BIT()

> +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */

Please use
#define SOMETHING<tab>value
throughout and make sure value is aligned.

> +
> +struct rza_wdt {
> +	struct watchdog_device wdev;
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static int rza_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Stop timer */
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	/* Must dummy read WRCSR:WOVF at least once before clearing */
> +	readb(priv->base + WRCSR);
> +	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
> +
> +	/*
> +	 * Start timer with slowest clock source and reset option enabled.
> +	 */
> +	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> +	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> +	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
> +	       priv->base + WTCSR);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
> +			    void *data)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Stop timer */
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	/* Must dummy read WRCSR:WOVF at least once before clearing */
> +	readb(priv->base + WRCSR);
> +	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
> +
> +	/*
> +	 * Start timer with fastest clock source and only 1 clock left before
> +	 * overflow with reset option enabled.
> +	 */
> +	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> +	writew(WTCNT_MAGIC | 255, priv->base + WTCNT);
> +	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR);
> +
> +	/*
> +	 * Actually make sure the above sequence hits hardware before sleeping.
> +	 */
> +	wmb();
> +
> +	/* Wait for WDT overflow (reset) */
> +	udelay(20);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info rza_wdt_ident = {
> +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +	.identity = "Renesas RZ/A WDT Watchdog",
> +};
> +
> +static const struct watchdog_ops rza_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = rza_wdt_start,
> +	.stop = rza_wdt_stop,
> +	.ping = rza_wdt_ping,
> +	.restart = rza_wdt_restart,
> +};
> +
> +static int rza_wdt_probe(struct platform_device *pdev)
> +{
> +	struct rza_wdt *priv;
> +	struct resource *res;
> +	unsigned long rate;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	rate = clk_get_rate(priv->clk);
> +	if (!rate)
> +		return -ENOENT;
> +
> +	/* Assume slowest clock rate possible (CKS=7) */
> +	rate /= 16384;
> +

The rate check should probably be here to avoid situations where rate < 16384.

> +	priv->wdev.info = &rza_wdt_ident,
> +	priv->wdev.ops = &rza_wdt_ops,
> +	priv->wdev.parent = &pdev->dev;
> +
> +	/*
> +	 * Since the max possible timeout of our 8-bit count register is less
> +	 * than a second, we must use max_hw_heartbeat_ms.
> +	 */
> +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;

space before and after /

> +	dev_info(&pdev->dev, "max hw timeout of %dms\n",
> +		 priv->wdev.max_hw_heartbeat_ms);

dev_dbg ?

> +
> +	priv->wdev.min_timeout = 1;
> +	priv->wdev.timeout = DEFAULT_TIMEOUT;
> +

Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get the timeout from dt ?

> +	platform_set_drvdata(pdev, priv);
> +	watchdog_set_drvdata(&priv->wdev, priv);
> +
> +	ret = watchdog_register_device(&priv->wdev);

devm_watchdog_register_device()

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_remove(struct platform_device *pdev)
> +{
> +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&priv->wdev);
> +	iounmap(priv->base);

iounmap is unnecessary (and wrong).

> +	return 0;
> +}
> +
> +static const struct of_device_id rza_wdt_of_match[] = {
> +	{ .compatible = "renesas,rza-wdt", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rza_wdt_of_match);
> +
> +static struct platform_driver rza_wdt_driver = {
> +	.probe = rza_wdt_probe,
> +	.remove = rza_wdt_remove,
> +	.driver = {
> +		.name = "rza_wdt",
> +		.of_match_table = rza_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(rza_wdt_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/A WDT Driver");
> +MODULE_AUTHOR("Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
>

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On 03/02/2017 05:57 AM, Chris Brandt wrote:
> Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
> handler is also included since a WDT overflow is the only method for
> restarting an RZ/A SoC.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/watchdog/Kconfig   |   8 ++
>  drivers/watchdog/Makefile  |   1 +
>  drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/watchdog/rza_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b5..123c516 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -701,6 +701,14 @@ config RENESAS_WDT
>  	  This driver adds watchdog support for the integrated watchdogs in the
>  	  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
>
> +config RENESAS_RZAWDT
> +	tristate "Renesas RZ/A WDT Watchdog"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  This driver adds watchdog support for the integrated watchdogs in the
> +	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
> +
>  config ASPEED_WATCHDOG
>  	tristate "Aspeed 2400 watchdog support"
>  	depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c3d35e..84b897c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>  obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>  obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
>  obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> +obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
>  obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>
>  # AVR32 Architecture
> diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
> new file mode 100644
> index 0000000..17442c5
> --- /dev/null
> +++ b/drivers/watchdog/rza_wdt.c
> @@ -0,0 +1,208 @@
> +/*
> + * Renesas RZ/A Series WDT Driver
> + *
> + * Copyright (C) 2017 Renesas Electronics America, Inc.
> + * Copyright (C) 2017 Chris Brandt
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + *

The above two lines are unnecessary.

> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/delay.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +
> +#define DEFAULT_TIMEOUT 30
> +
> +/* Watchdog Timer Registers */
> +#define WTCSR 0
> +#define WTCSR_MAGIC 0xA500
> +#define WTSCR_WT (1<<6)
> +#define WTSCR_TME (1<<5)

BIT()

> +#define WTSCR_CKS(i) i

(i)

> +
> +#define WTCNT 2
> +#define WTCNT_MAGIC 0x5A00
> +
> +#define WRCSR 4
> +#define WRCSR_MAGIC 0x5A00
> +#define WRCSR_RSTE (1<<6)

BIT()

> +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */

Please use
#define SOMETHING<tab>value
throughout and make sure value is aligned.

> +
> +struct rza_wdt {
> +	struct watchdog_device wdev;
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static int rza_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Stop timer */
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	/* Must dummy read WRCSR:WOVF at least once before clearing */
> +	readb(priv->base + WRCSR);
> +	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
> +
> +	/*
> +	 * Start timer with slowest clock source and reset option enabled.
> +	 */
> +	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> +	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> +	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
> +	       priv->base + WTCSR);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
> +			    void *data)
> +{
> +	struct rza_wdt *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Stop timer */
> +	writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
> +
> +	/* Must dummy read WRCSR:WOVF at least once before clearing */
> +	readb(priv->base + WRCSR);
> +	writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
> +
> +	/*
> +	 * Start timer with fastest clock source and only 1 clock left before
> +	 * overflow with reset option enabled.
> +	 */
> +	writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
> +	writew(WTCNT_MAGIC | 255, priv->base + WTCNT);
> +	writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR);
> +
> +	/*
> +	 * Actually make sure the above sequence hits hardware before sleeping.
> +	 */
> +	wmb();
> +
> +	/* Wait for WDT overflow (reset) */
> +	udelay(20);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info rza_wdt_ident = {
> +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +	.identity = "Renesas RZ/A WDT Watchdog",
> +};
> +
> +static const struct watchdog_ops rza_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = rza_wdt_start,
> +	.stop = rza_wdt_stop,
> +	.ping = rza_wdt_ping,
> +	.restart = rza_wdt_restart,
> +};
> +
> +static int rza_wdt_probe(struct platform_device *pdev)
> +{
> +	struct rza_wdt *priv;
> +	struct resource *res;
> +	unsigned long rate;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	rate = clk_get_rate(priv->clk);
> +	if (!rate)
> +		return -ENOENT;
> +
> +	/* Assume slowest clock rate possible (CKS=7) */
> +	rate /= 16384;
> +

The rate check should probably be here to avoid situations where rate < 16384.

> +	priv->wdev.info = &rza_wdt_ident,
> +	priv->wdev.ops = &rza_wdt_ops,
> +	priv->wdev.parent = &pdev->dev;
> +
> +	/*
> +	 * Since the max possible timeout of our 8-bit count register is less
> +	 * than a second, we must use max_hw_heartbeat_ms.
> +	 */
> +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;

space before and after /

> +	dev_info(&pdev->dev, "max hw timeout of %dms\n",
> +		 priv->wdev.max_hw_heartbeat_ms);

dev_dbg ?

> +
> +	priv->wdev.min_timeout = 1;
> +	priv->wdev.timeout = DEFAULT_TIMEOUT;
> +

Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get the timeout from dt ?

> +	platform_set_drvdata(pdev, priv);
> +	watchdog_set_drvdata(&priv->wdev, priv);
> +
> +	ret = watchdog_register_device(&priv->wdev);

devm_watchdog_register_device()

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int rza_wdt_remove(struct platform_device *pdev)
> +{
> +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&priv->wdev);
> +	iounmap(priv->base);

iounmap is unnecessary (and wrong).

> +	return 0;
> +}
> +
> +static const struct of_device_id rza_wdt_of_match[] = {
> +	{ .compatible = "renesas,rza-wdt", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rza_wdt_of_match);
> +
> +static struct platform_driver rza_wdt_driver = {
> +	.probe = rza_wdt_probe,
> +	.remove = rza_wdt_remove,
> +	.driver = {
> +		.name = "rza_wdt",
> +		.of_match_table = rza_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(rza_wdt_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/A WDT Driver");
> +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
> +MODULE_LICENSE("GPL v2");
>

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

Hello Guenter,

Thank you for your review!


On Thursday, March 02, 2017, Guenter Roeck wrote:
> > +/*
> > + * Renesas RZ/A Series WDT Driver
> > + *
> > + * Copyright (C) 2017 Renesas Electronics America, Inc.
> > + * Copyright (C) 2017 Chris Brandt
> > + *
> > + * This file is subject to the terms and conditions of the GNU
> > +General Public
> > + * License.  See the file "COPYING" in the main directory of this
> > +archive
> > + * for more details.
> > + *
> > + *
> 
> The above two lines are unnecessary.

OK.

#I'll assume you mean take out just the last sentence (2 lines), not both
sentences (all 3 lines).

 
> > +/* Watchdog Timer Registers */
> > +#define WTCSR 0
> > +#define WTCSR_MAGIC 0xA500
> > +#define WTSCR_WT (1<<6)
> > +#define WTSCR_TME (1<<5)
> 
> BIT()

OK.

> > +#define WTSCR_CKS(i) i
> 
> (i)

OK.


> > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
> 
> Please use
> #define SOMETHING<tab>value
> throughout and make sure value is aligned.

OK.

> > +	rate = clk_get_rate(priv->clk);
> > +	if (!rate)
> > +		return -ENOENT;
> > +
> > +	/* Assume slowest clock rate possible (CKS=7) */
> > +	rate /= 16384;
> > +
> 
> The rate check should probably be here to avoid situations where rate <
> 16384.

Do I need that if it's technically not possible to have a 'rate' less than 25MHz?

These watchdogs HW are always feed directly from the peripheral clock and there
is no such thing as a 16kHz peripheral block an any Renesas SoC.


> > +	priv->wdev.info = &rza_wdt_ident,
> > +	priv->wdev.ops = &rza_wdt_ops,
> > +	priv->wdev.parent = &pdev->dev;
> > +
> > +	/*
> > +	 * Since the max possible timeout of our 8-bit count register is
> less
> > +	 * than a second, we must use max_hw_heartbeat_ms.
> > +	 */
> > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> 
> space before and after /

OK.
#Funny because checkpatch.pl said it didn't like a space on one side but
 not the other, so I choose no spaces and it was happy. I'm way below 80
 characters for that line so it doesn't matter to me.


> > +	dev_info(&pdev->dev, "max hw timeout of %dms\n",
> > +		 priv->wdev.max_hw_heartbeat_ms);
> 
> dev_dbg ?

OK.
#I guess we don't need to see that info on every boot.

 
> > +
> > +	priv->wdev.min_timeout = 1;
> > +	priv->wdev.timeout = DEFAULT_TIMEOUT;
> > +
> 
> Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get
> the timeout from dt ?

OK. Good idea.

> > +	platform_set_drvdata(pdev, priv);
> > +	watchdog_set_drvdata(&priv->wdev, priv);
> > +
> > +	ret = watchdog_register_device(&priv->wdev);
> 
> devm_watchdog_register_device()

OK.

 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rza_wdt_remove(struct platform_device *pdev) {
> > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > +
> > +	watchdog_unregister_device(&priv->wdev);
> > +	iounmap(priv->base);
> 
> iounmap is unnecessary (and wrong).

Anything mapped with devm_ioremap_resource() automatically gets unmapped
when the drive gets unloaded?
I didn't know that.



I'll wait till the end of the day to see if anyone finds anything else, and
then I'll send out a v5.


Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

Hello Guenter,

Thank you for your review!


On Thursday, March 02, 2017, Guenter Roeck wrote:
> > +/*
> > + * Renesas RZ/A Series WDT Driver
> > + *
> > + * Copyright (C) 2017 Renesas Electronics America, Inc.
> > + * Copyright (C) 2017 Chris Brandt
> > + *
> > + * This file is subject to the terms and conditions of the GNU
> > +General Public
> > + * License.  See the file "COPYING" in the main directory of this
> > +archive
> > + * for more details.
> > + *
> > + *
> 
> The above two lines are unnecessary.

OK.

#I'll assume you mean take out just the last sentence (2 lines), not both
sentences (all 3 lines).

 
> > +/* Watchdog Timer Registers */
> > +#define WTCSR 0
> > +#define WTCSR_MAGIC 0xA500
> > +#define WTSCR_WT (1<<6)
> > +#define WTSCR_TME (1<<5)
> 
> BIT()

OK.

> > +#define WTSCR_CKS(i) i
> 
> (i)

OK.


> > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
> 
> Please use
> #define SOMETHING<tab>value
> throughout and make sure value is aligned.

OK.

> > +	rate = clk_get_rate(priv->clk);
> > +	if (!rate)
> > +		return -ENOENT;
> > +
> > +	/* Assume slowest clock rate possible (CKS=7) */
> > +	rate /= 16384;
> > +
> 
> The rate check should probably be here to avoid situations where rate <
> 16384.

Do I need that if it's technically not possible to have a 'rate' less than 25MHz?

These watchdogs HW are always feed directly from the peripheral clock and there
is no such thing as a 16kHz peripheral block an any Renesas SoC.


> > +	priv->wdev.info = &rza_wdt_ident,
> > +	priv->wdev.ops = &rza_wdt_ops,
> > +	priv->wdev.parent = &pdev->dev;
> > +
> > +	/*
> > +	 * Since the max possible timeout of our 8-bit count register is
> less
> > +	 * than a second, we must use max_hw_heartbeat_ms.
> > +	 */
> > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> 
> space before and after /

OK.
#Funny because checkpatch.pl said it didn't like a space on one side but
 not the other, so I choose no spaces and it was happy. I'm way below 80
 characters for that line so it doesn't matter to me.


> > +	dev_info(&pdev->dev, "max hw timeout of %dms\n",
> > +		 priv->wdev.max_hw_heartbeat_ms);
> 
> dev_dbg ?

OK.
#I guess we don't need to see that info on every boot.

 
> > +
> > +	priv->wdev.min_timeout = 1;
> > +	priv->wdev.timeout = DEFAULT_TIMEOUT;
> > +
> 
> Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get
> the timeout from dt ?

OK. Good idea.

> > +	platform_set_drvdata(pdev, priv);
> > +	watchdog_set_drvdata(&priv->wdev, priv);
> > +
> > +	ret = watchdog_register_device(&priv->wdev);
> 
> devm_watchdog_register_device()

OK.

 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rza_wdt_remove(struct platform_device *pdev) {
> > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > +
> > +	watchdog_unregister_device(&priv->wdev);
> > +	iounmap(priv->base);
> 
> iounmap is unnecessary (and wrong).

Anything mapped with devm_ioremap_resource() automatically gets unmapped
when the drive gets unloaded?
I didn't know that.



I'll wait till the end of the day to see if anyone finds anything else, and
then I'll send out a v5.


Chris

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 03:38:07PM +0000, Chris Brandt wrote:
> Hello Guenter,
> 
> Thank you for your review!
> 
> 
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > +/*
> > > + * Renesas RZ/A Series WDT Driver
> > > + *
> > > + * Copyright (C) 2017 Renesas Electronics America, Inc.
> > > + * Copyright (C) 2017 Chris Brandt
> > > + *
> > > + * This file is subject to the terms and conditions of the GNU
> > > +General Public
> > > + * License.  See the file "COPYING" in the main directory of this
> > > +archive
> > > + * for more details.
> > > + *
> > > + *
> > 
> > The above two lines are unnecessary.
> 
> OK.
> 
> #I'll assume you mean take out just the last sentence (2 lines), not both
> sentences (all 3 lines).
> 
The two empty lines.

>  
> > > +/* Watchdog Timer Registers */
> > > +#define WTCSR 0
> > > +#define WTCSR_MAGIC 0xA500
> > > +#define WTSCR_WT (1<<6)
> > > +#define WTSCR_TME (1<<5)
> > 
> > BIT()
> 
> OK.
> 
> > > +#define WTSCR_CKS(i) i
> > 
> > (i)
> 
> OK.
> 
> 
> > > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
> > 
> > Please use
> > #define SOMETHING<tab>value
> > throughout and make sure value is aligned.
> 
> OK.
> 
> > > +	rate = clk_get_rate(priv->clk);
> > > +	if (!rate)
> > > +		return -ENOENT;
> > > +
> > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > +	rate /= 16384;
> > > +
> > 
> > The rate check should probably be here to avoid situations where rate <
> > 16384.
> 
> Do I need that if it's technically not possible to have a 'rate' less than 25MHz?
> 
> These watchdogs HW are always feed directly from the peripheral clock and there
> is no such thing as a 16kHz peripheral block an any Renesas SoC.
> 
Following that line of argument, can clk_get_rate() ever return 0 ?

> 
> > > +	priv->wdev.info = &rza_wdt_ident,
> > > +	priv->wdev.ops = &rza_wdt_ops,
> > > +	priv->wdev.parent = &pdev->dev;
> > > +
> > > +	/*
> > > +	 * Since the max possible timeout of our 8-bit count register is
> > less
> > > +	 * than a second, we must use max_hw_heartbeat_ms.
> > > +	 */
> > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > 
> > space before and after /
> 
> OK.
> #Funny because checkpatch.pl said it didn't like a space on one side but
>  not the other, so I choose no spaces and it was happy. I'm way below 80
>  characters for that line so it doesn't matter to me.
> 

That would be a bug in checkpatch. coding style, chapter 3.1, still applies.
Or at least I hope so.

> 
> > > +	dev_info(&pdev->dev, "max hw timeout of %dms\n",
> > > +		 priv->wdev.max_hw_heartbeat_ms);
> > 
> > dev_dbg ?
> 
> OK.
> #I guess we don't need to see that info on every boot.
> 
>  
> > > +
> > > +	priv->wdev.min_timeout = 1;
> > > +	priv->wdev.timeout = DEFAULT_TIMEOUT;
> > > +
> > 
> > Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get
> > the timeout from dt ?
> 
> OK. Good idea.
> 
> > > +	platform_set_drvdata(pdev, priv);
> > > +	watchdog_set_drvdata(&priv->wdev, priv);
> > > +
> > > +	ret = watchdog_register_device(&priv->wdev);
> > 
> > devm_watchdog_register_device()
> 
> OK.
> 
>  
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;

Also just
	return ret;

> > > +}
> > > +
> > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > +
> > > +	watchdog_unregister_device(&priv->wdev);
> > > +	iounmap(priv->base);
> > 
> > iounmap is unnecessary (and wrong).
> 
> Anything mapped with devm_ioremap_resource() automatically gets unmapped
> when the drive gets unloaded?

That is the point of devm_ functions. It also means that you won't need
a remove function if you also use devm_watchdog_register_device().

Guenter

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

On Thursday, March 02, 2017, Guenter Roeck worte:
> > > The above two lines are unnecessary.
> >
> > OK.
> >
> > #I'll assume you mean take out just the last sentence (2 lines), not
> > both sentences (all 3 lines).
> >
> The two empty lines.

Ooops! That makes more sense.


> > > > +	rate = clk_get_rate(priv->clk);
> > > > +	if (!rate)
> > > > +		return -ENOENT;
> > > > +
> > > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > > +	rate /= 16384;
> > > > +
> > >
> > > The rate check should probably be here to avoid situations where
> > > rate < 16384.
> >
> > Do I need that if it's technically not possible to have a 'rate' less
> than 25MHz?
> >
> > These watchdogs HW are always feed directly from the peripheral clock
> > and there is no such thing as a 16kHz peripheral block an any Renesas
> SoC.
> >
> Following that line of argument, can clk_get_rate() ever return 0 ?

In the DT binding, it says that a clock source is required to be present.

If the user leaves out the "clocks =", then devm_clk_get will fail.

If the user puts in some crazy value for "clocks = ", then maybe you could get
0 (assuming there is a valid clock node they made by themselves somewhere that
runs at 0Hz).
But in that extreme case, I think they deserve to have it crash and burn because
who knows what they are doing.


> > > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > >
> > > space before and after /
> >
> > OK.
> > #Funny because checkpatch.pl said it didn't like a space on one side
> > but  not the other, so I choose no spaces and it was happy. I'm way
> > below 80  characters for that line so it doesn't matter to me.
> >
> 
> That would be a bug in checkpatch. coding style, chapter 3.1, still
> applies.
> Or at least I hope so.

OK. Thank you for the clarification.


> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> 
> Also just
> 	return ret;

OK.


> > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > +
> > > > +	watchdog_unregister_device(&priv->wdev);
> > > > +	iounmap(priv->base);
> > >
> > > iounmap is unnecessary (and wrong).
> >
> > Anything mapped with devm_ioremap_resource() automatically gets
> > unmapped when the drive gets unloaded?
> 
> That is the point of devm_ functions. It also means that you won't need a
> remove function if you also use devm_watchdog_register_device().

OK.
I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
maybe that is a new method.


Thank you,
Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

On Thursday, March 02, 2017, Guenter Roeck worte:
> > > The above two lines are unnecessary.
> >
> > OK.
> >
> > #I'll assume you mean take out just the last sentence (2 lines), not
> > both sentences (all 3 lines).
> >
> The two empty lines.

Ooops! That makes more sense.


> > > > +	rate = clk_get_rate(priv->clk);
> > > > +	if (!rate)
> > > > +		return -ENOENT;
> > > > +
> > > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > > +	rate /= 16384;
> > > > +
> > >
> > > The rate check should probably be here to avoid situations where
> > > rate < 16384.
> >
> > Do I need that if it's technically not possible to have a 'rate' less
> than 25MHz?
> >
> > These watchdogs HW are always feed directly from the peripheral clock
> > and there is no such thing as a 16kHz peripheral block an any Renesas
> SoC.
> >
> Following that line of argument, can clk_get_rate() ever return 0 ?

In the DT binding, it says that a clock source is required to be present.

If the user leaves out the "clocks =", then devm_clk_get will fail.

If the user puts in some crazy value for "clocks = ", then maybe you could get
0 (assuming there is a valid clock node they made by themselves somewhere that
runs at 0Hz).
But in that extreme case, I think they deserve to have it crash and burn because
who knows what they are doing.


> > > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > >
> > > space before and after /
> >
> > OK.
> > #Funny because checkpatch.pl said it didn't like a space on one side
> > but  not the other, so I choose no spaces and it was happy. I'm way
> > below 80  characters for that line so it doesn't matter to me.
> >
> 
> That would be a bug in checkpatch. coding style, chapter 3.1, still
> applies.
> Or at least I hope so.

OK. Thank you for the clarification.


> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> 
> Also just
> 	return ret;

OK.


> > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > +
> > > > +	watchdog_unregister_device(&priv->wdev);
> > > > +	iounmap(priv->base);
> > >
> > > iounmap is unnecessary (and wrong).
> >
> > Anything mapped with devm_ioremap_resource() automatically gets
> > unmapped when the drive gets unloaded?
> 
> That is the point of devm_ functions. It also means that you won't need a
> remove function if you also use devm_watchdog_register_device().

OK.
I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
maybe that is a new method.


Thank you,
Chris

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 05:31:18PM +0000, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck worte:
> > > > The above two lines are unnecessary.
> > >
> > > OK.
> > >
> > > #I'll assume you mean take out just the last sentence (2 lines), not
> > > both sentences (all 3 lines).
> > >
> > The two empty lines.
> 
> Ooops! That makes more sense.
> 
> 
> > > > > +	rate = clk_get_rate(priv->clk);
> > > > > +	if (!rate)
> > > > > +		return -ENOENT;
> > > > > +
> > > > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > > > +	rate /= 16384;
> > > > > +
> > > >
> > > > The rate check should probably be here to avoid situations where
> > > > rate < 16384.
> > >
> > > Do I need that if it's technically not possible to have a 'rate' less
> > than 25MHz?
> > >
> > > These watchdogs HW are always feed directly from the peripheral clock
> > > and there is no such thing as a 16kHz peripheral block an any Renesas
> > SoC.
> > >
> > Following that line of argument, can clk_get_rate() ever return 0 ?
> 
> In the DT binding, it says that a clock source is required to be present.
> 
> If the user leaves out the "clocks =", then devm_clk_get will fail.
> 
> If the user puts in some crazy value for "clocks = ", then maybe you could get
> 0 (assuming there is a valid clock node they made by themselves somewhere that
> runs at 0Hz).
> But in that extreme case, I think they deserve to have it crash and burn because
> who knows what they are doing.
> 

But then there could also be a clock source with a rate of less than 16 kHz, as
wrong as it may be ?

Anyway, I disagree about the crash and burn. It isn't as if this would be really
fatal except for the watchdog driver. Bad data in devicetree should not result
in a system crash.

> 
> > > > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > > >
> > > > space before and after /
> > >
> > > OK.
> > > #Funny because checkpatch.pl said it didn't like a space on one side
> > > but  not the other, so I choose no spaces and it was happy. I'm way
> > > below 80  characters for that line so it doesn't matter to me.
> > >
> > 
> > That would be a bug in checkpatch. coding style, chapter 3.1, still
> > applies.
> > Or at least I hope so.
> 
> OK. Thank you for the clarification.
> 
> 
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	return 0;
> > 
> > Also just
> > 	return ret;
> 
> OK.
> 
> 
> > > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	watchdog_unregister_device(&priv->wdev);
> > > > > +	iounmap(priv->base);
> > > >
> > > > iounmap is unnecessary (and wrong).
> > >
> > > Anything mapped with devm_ioremap_resource() automatically gets
> > > unmapped when the drive gets unloaded?
> > 
> > That is the point of devm_ functions. It also means that you won't need a
> > remove function if you also use devm_watchdog_register_device().
> 
> OK.
> I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
> maybe that is a new method.
> 
Yes, it is quite new. Still, you are a bit behind. I count 19 users
in the mainline kernel.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 05:31:18PM +0000, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck worte:
> > > > The above two lines are unnecessary.
> > >
> > > OK.
> > >
> > > #I'll assume you mean take out just the last sentence (2 lines), not
> > > both sentences (all 3 lines).
> > >
> > The two empty lines.
> 
> Ooops! That makes more sense.
> 
> 
> > > > > +	rate = clk_get_rate(priv->clk);
> > > > > +	if (!rate)
> > > > > +		return -ENOENT;
> > > > > +
> > > > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > > > +	rate /= 16384;
> > > > > +
> > > >
> > > > The rate check should probably be here to avoid situations where
> > > > rate < 16384.
> > >
> > > Do I need that if it's technically not possible to have a 'rate' less
> > than 25MHz?
> > >
> > > These watchdogs HW are always feed directly from the peripheral clock
> > > and there is no such thing as a 16kHz peripheral block an any Renesas
> > SoC.
> > >
> > Following that line of argument, can clk_get_rate() ever return 0 ?
> 
> In the DT binding, it says that a clock source is required to be present.
> 
> If the user leaves out the "clocks =", then devm_clk_get will fail.
> 
> If the user puts in some crazy value for "clocks = ", then maybe you could get
> 0 (assuming there is a valid clock node they made by themselves somewhere that
> runs at 0Hz).
> But in that extreme case, I think they deserve to have it crash and burn because
> who knows what they are doing.
> 

But then there could also be a clock source with a rate of less than 16 kHz, as
wrong as it may be ?

Anyway, I disagree about the crash and burn. It isn't as if this would be really
fatal except for the watchdog driver. Bad data in devicetree should not result
in a system crash.

> 
> > > > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > > >
> > > > space before and after /
> > >
> > > OK.
> > > #Funny because checkpatch.pl said it didn't like a space on one side
> > > but  not the other, so I choose no spaces and it was happy. I'm way
> > > below 80  characters for that line so it doesn't matter to me.
> > >
> > 
> > That would be a bug in checkpatch. coding style, chapter 3.1, still
> > applies.
> > Or at least I hope so.
> 
> OK. Thank you for the clarification.
> 
> 
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	return 0;
> > 
> > Also just
> > 	return ret;
> 
> OK.
> 
> 
> > > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	watchdog_unregister_device(&priv->wdev);
> > > > > +	iounmap(priv->base);
> > > >
> > > > iounmap is unnecessary (and wrong).
> > >
> > > Anything mapped with devm_ioremap_resource() automatically gets
> > > unmapped when the drive gets unloaded?
> > 
> > That is the point of devm_ functions. It also means that you won't need a
> > remove function if you also use devm_watchdog_register_device().
> 
> OK.
> I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
> maybe that is a new method.
> 
Yes, it is quite new. Still, you are a bit behind. I count 19 users
in the mainline kernel.

Guenter

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > The rate check should probably be here to avoid situations where
> > > > > rate < 16384.
> > > >
> > > > Do I need that if it's technically not possible to have a 'rate'
> > > > less
> > > than 25MHz?
> > > >
> > > > These watchdogs HW are always feed directly from the peripheral
> > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > any Renesas
> > > SoC.
> > > >
> > > Following that line of argument, can clk_get_rate() ever return 0 ?
> >
> > In the DT binding, it says that a clock source is required to be present.
> >
> > If the user leaves out the "clocks =", then devm_clk_get will fail.
> >
> > If the user puts in some crazy value for "clocks = ", then maybe you
> > could get
> > 0 (assuming there is a valid clock node they made by themselves
> > somewhere that runs at 0Hz).
> > But in that extreme case, I think they deserve to have it crash and
> > burn because who knows what they are doing.
> >
> 
> But then there could also be a clock source with a rate of less than 16
> kHz, as wrong as it may be ?
> 
> Anyway, I disagree about the crash and burn. It isn't as if this would be
> really fatal except for the watchdog driver. Bad data in devicetree should
> not result in a system crash.

OK. I will put the check in. Something like:

	rate = clk_get_rate(priv->clk); 
	if (rate < 16384) {
		dev_err(&pdev->dev, "invalid clock specified\n");
		return -ENOENT;
	}


> > > That is the point of devm_ functions. It also means that you won't
> > > need a remove function if you also use devm_watchdog_register_device().
> >
> > OK.
> > I see that only 1 driver is using devm_watchdog_register_device
> > (wdat_wdt.c), so maybe that is a new method.
> >
> Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> mainline kernel.

OK, I see now.


Thank you,
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Chris Brandt]

On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > The rate check should probably be here to avoid situations where
> > > > > rate < 16384.
> > > >
> > > > Do I need that if it's technically not possible to have a 'rate'
> > > > less
> > > than 25MHz?
> > > >
> > > > These watchdogs HW are always feed directly from the peripheral
> > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > any Renesas
> > > SoC.
> > > >
> > > Following that line of argument, can clk_get_rate() ever return 0 ?
> >
> > In the DT binding, it says that a clock source is required to be present.
> >
> > If the user leaves out the "clocks =", then devm_clk_get will fail.
> >
> > If the user puts in some crazy value for "clocks = ", then maybe you
> > could get
> > 0 (assuming there is a valid clock node they made by themselves
> > somewhere that runs at 0Hz).
> > But in that extreme case, I think they deserve to have it crash and
> > burn because who knows what they are doing.
> >
> 
> But then there could also be a clock source with a rate of less than 16
> kHz, as wrong as it may be ?
> 
> Anyway, I disagree about the crash and burn. It isn't as if this would be
> really fatal except for the watchdog driver. Bad data in devicetree should
> not result in a system crash.

OK. I will put the check in. Something like:

	rate = clk_get_rate(priv->clk); 
	if (rate < 16384) {
		dev_err(&pdev->dev, "invalid clock specified\n");
		return -ENOENT;
	}


> > > That is the point of devm_ functions. It also means that you won't
> > > need a remove function if you also use devm_watchdog_register_device().
> >
> > OK.
> > I see that only 1 driver is using devm_watchdog_register_device
> > (wdat_wdt.c), so maybe that is a new method.
> >
> Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> mainline kernel.

OK, I see now.


Thank you,
Chris

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 06:22:17PM +0000, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > > The rate check should probably be here to avoid situations where
> > > > > > rate < 16384.
> > > > >
> > > > > Do I need that if it's technically not possible to have a 'rate'
> > > > > less
> > > > than 25MHz?
> > > > >
> > > > > These watchdogs HW are always feed directly from the peripheral
> > > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > > any Renesas
> > > > SoC.
> > > > >
> > > > Following that line of argument, can clk_get_rate() ever return 0 ?
> > >
> > > In the DT binding, it says that a clock source is required to be present.
> > >
> > > If the user leaves out the "clocks =", then devm_clk_get will fail.
> > >
> > > If the user puts in some crazy value for "clocks = ", then maybe you
> > > could get
> > > 0 (assuming there is a valid clock node they made by themselves
> > > somewhere that runs at 0Hz).
> > > But in that extreme case, I think they deserve to have it crash and
> > > burn because who knows what they are doing.
> > >
> > 
> > But then there could also be a clock source with a rate of less than 16
> > kHz, as wrong as it may be ?
> > 
> > Anyway, I disagree about the crash and burn. It isn't as if this would be
> > really fatal except for the watchdog driver. Bad data in devicetree should
> > not result in a system crash.
> 
> OK. I will put the check in. Something like:
> 
> 	rate = clk_get_rate(priv->clk); 
> 	if (rate < 16384) {
> 		dev_err(&pdev->dev, "invalid clock specified\n");
> 		return -ENOENT;
> 	}
> 
> 
Sounds good. Maybe display the wrong rate as well ?
Not a strong opinion though.

Thanks,
Guenter

> > > > That is the point of devm_ functions. It also means that you won't
> > > > need a remove function if you also use devm_watchdog_register_device().
> > >
> > > OK.
> > > I see that only 1 driver is using devm_watchdog_register_device
> > > (wdat_wdt.c), so maybe that is a new method.
> > >
> > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> > mainline kernel.
> 
> OK, I see now.
> 
> 
> Thank you,
> Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck]

On Thu, Mar 02, 2017 at 06:22:17PM +0000, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > > The rate check should probably be here to avoid situations where
> > > > > > rate < 16384.
> > > > >
> > > > > Do I need that if it's technically not possible to have a 'rate'
> > > > > less
> > > > than 25MHz?
> > > > >
> > > > > These watchdogs HW are always feed directly from the peripheral
> > > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > > any Renesas
> > > > SoC.
> > > > >
> > > > Following that line of argument, can clk_get_rate() ever return 0 ?
> > >
> > > In the DT binding, it says that a clock source is required to be present.
> > >
> > > If the user leaves out the "clocks =", then devm_clk_get will fail.
> > >
> > > If the user puts in some crazy value for "clocks = ", then maybe you
> > > could get
> > > 0 (assuming there is a valid clock node they made by themselves
> > > somewhere that runs at 0Hz).
> > > But in that extreme case, I think they deserve to have it crash and
> > > burn because who knows what they are doing.
> > >
> > 
> > But then there could also be a clock source with a rate of less than 16
> > kHz, as wrong as it may be ?
> > 
> > Anyway, I disagree about the crash and burn. It isn't as if this would be
> > really fatal except for the watchdog driver. Bad data in devicetree should
> > not result in a system crash.
> 
> OK. I will put the check in. Something like:
> 
> 	rate = clk_get_rate(priv->clk); 
> 	if (rate < 16384) {
> 		dev_err(&pdev->dev, "invalid clock specified\n");
> 		return -ENOENT;
> 	}
> 
> 
Sounds good. Maybe display the wrong rate as well ?
Not a strong opinion though.

Thanks,
Guenter

> > > > That is the point of devm_ functions. It also means that you won't
> > > > need a remove function if you also use devm_watchdog_register_device().
> > >
> > > OK.
> > > I see that only 1 driver is using devm_watchdog_register_device
> > > (wdat_wdt.c), so maybe that is a new method.
> > >
> > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> > mainline kernel.
> 
> OK, I see now.
> 
> 
> Thank you,
> Chris

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Geert Uytterhoeven]

Hi Chris,

On Thu, Mar 2, 2017 at 4:38 PM, Chris Brandt <Chris.Brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
>> > +   /*
>> > +    * Since the max possible timeout of our 8-bit count register is
>> less
>> > +    * than a second, we must use max_hw_heartbeat_ms.
>> > +    */
>> > +   priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
>>
>> space before and after /
>
> OK.
> #Funny because checkpatch.pl said it didn't like a space on one side but
>  not the other, so I choose no spaces and it was happy. I'm way below 80
>  characters for that line so it doesn't matter to me.

Just drop the parentheses? Standard C operator precedence is fine here.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Geert Uytterhoeven]

Hi Chris,

On Thu, Mar 2, 2017 at 4:38 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
>> > +   /*
>> > +    * Since the max possible timeout of our 8-bit count register is
>> less
>> > +    * than a second, we must use max_hw_heartbeat_ms.
>> > +    */
>> > +   priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
>>
>> space before and after /
>
> OK.
> #Funny because checkpatch.pl said it didn't like a space on one side but
>  not the other, so I choose no spaces and it was happy. I'm way below 80
>  characters for that line so it doesn't matter to me.

Just drop the parentheses? Standard C operator precedence is fine here.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds