[PATCH 0/7] watchdog: s3c2410: Add Exynos850 support

[PATCH 0/7] watchdog: s3c2410: Add Exynos850 support

[Sam Protsenko]

Exynos850 WDT IP-core differs a bit from existing platforms:
  - Another set of PMU registers
  - Separate WDT instance for each CPU cluster, having different PMU
    registers/bits
  - Source clock is different from PCLK

Implement all missing features above and enable Exynos850 WDT support.

Sam Protsenko (7):
  dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7
  dt-bindings: watchdog: Document Exynos850 watchdog bindings
  watchdog: s3c2410: Make reset disable optional
  watchdog: s3c2410: Add support for WDT counter enable
  watchdog: s3c2410: Introduce separate source clock
  watchdog: s3c2410: Add Exynos850 support
  watchdog: s3c2410: Let kernel kick watchdog

 .../bindings/watchdog/samsung-wdt.yaml        |  17 +-
 drivers/watchdog/s3c2410_wdt.c                | 215 ++++++++++++++----
 2 files changed, 190 insertions(+), 42 deletions(-)

-- 
2.30.2

[PATCH 1/7] dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7

[Sam Protsenko]

Exynos7 watchdog driver is clearly indicating that its dts node must
define syscon phandle property. That was probably forgotten, so add it.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
index 76cb9586ee00..93cd77a6e92c 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
@@ -39,8 +39,8 @@ properties:
   samsung,syscon-phandle:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-      Phandle to the PMU system controller node (in case of Exynos5250
-      and Exynos5420).
+      Phandle to the PMU system controller node (in case of Exynos5250,
+      Exynos5420 and Exynos7).
 
 required:
   - compatible
@@ -58,6 +58,7 @@ allOf:
             enum:
               - samsung,exynos5250-wdt
               - samsung,exynos5420-wdt
+              - samsung,exynos7-wdt
     then:
       required:
         - samsung,syscon-phandle
-- 
2.30.2

[PATCH 2/7] dt-bindings: watchdog: Document Exynos850 watchdog bindings

[Sam Protsenko]

Exynos850 SoC has two CPU clusters:
  - cluster 0: contains CPUs #0, #1, #2, #3
  - cluster 1: contains CPUs #4, #5, #6, #7

Each cluster has its own dedicater watchdog timer. Those WDT instances
are controlled using different bits in PMU registers, so there should be
two different compatible strings (for each cluster), to tell the driver
which bits to use for each WDT instance.

Also on Exynos850 the peripheral clock and the source clock are two
different clocks. Provide a way to specify two clocks in watchdog device
tree node.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../devicetree/bindings/watchdog/samsung-wdt.yaml  | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
index 93cd77a6e92c..19c7f7767559 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
@@ -22,16 +22,24 @@ properties:
       - samsung,exynos5250-wdt                # for Exynos5250
       - samsung,exynos5420-wdt                # for Exynos5420
       - samsung,exynos7-wdt                   # for Exynos7
+      - samsung,exynos850-cl0-wdt             # for Exynos850 (CPU cluster 0)
+      - samsung,exynos850-cl1-wdt             # for Exynos850 (CPU cluster 1)
 
   reg:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Peripheral clock used for register interface; if it's the
+                     only clock, it's also a source clock
+      - description: Source clock (optional)
 
   clock-names:
+    minItems: 1
     items:
       - const: watchdog
+      - const: watchdog_src
 
   interrupts:
     maxItems: 1
@@ -40,7 +48,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
       Phandle to the PMU system controller node (in case of Exynos5250,
-      Exynos5420 and Exynos7).
+      Exynos5420, Exynos7 and Exynos850).
 
 required:
   - compatible
@@ -59,6 +67,8 @@ allOf:
               - samsung,exynos5250-wdt
               - samsung,exynos5420-wdt
               - samsung,exynos7-wdt
+              - samsung,exynos850-cl0-wdt
+              - samsung,exynos850-cl1-wdt
     then:
       required:
         - samsung,syscon-phandle
-- 
2.30.2

[PATCH 3/7] watchdog: s3c2410: Make reset disable optional

[Sam Protsenko]

Not all SoCs have AUTOMATIC_WDT_RESET_DISABLE register, examples are
Exynos850 and Exynos9. On such chips reset disable register shouldn't be
accessed. Provide a way to avoid handling that register. This is done by
introducing separate callbacks to driver data structure: one for reset
disable register, and one for mask reset register. Now those callbacks
can be checked and called only when those were set in driver data.

This commit doesn't bring any functional change to existing devices, but
merely provides an infrastructure for upcoming chips support.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 81 ++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 23 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 2395f353e52d..7c163a257d3c 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -83,6 +83,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
 
+struct s3c2410_wdt;
+
 /**
  * struct s3c2410_wdt_variant - Per-variant config data
  *
@@ -96,6 +98,11 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
  * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
  * reset.
  * @quirks: A bitfield of quirks.
+ * @disable_auto_reset: If set, this function will be called to disable
+ * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
+ * disable_reg field.
+ * @mask_reset: If set, this function will be called to mask WDT reset request;
+ * uses mask_reset_reg and mask_bit fields.
  */
 
 struct s3c2410_wdt_variant {
@@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
 	int rst_stat_reg;
 	int rst_stat_bit;
 	u32 quirks;
+	int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
+	int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
 };
 
 struct s3c2410_wdt {
@@ -121,6 +130,9 @@ struct s3c2410_wdt {
 	struct regmap *pmureg;
 };
 
+static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
+static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
+
 static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
 	.quirks = 0
 };
@@ -138,6 +150,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
 	.rst_stat_bit = 20,
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
 		  | QUIRK_HAS_WTCLRINT_REG,
+	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
+	.mask_reset = s3c2410wdt_mask_wdt_reset,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
@@ -148,6 +162,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
 	.rst_stat_bit = 9,
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
 		  | QUIRK_HAS_WTCLRINT_REG,
+	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
+	.mask_reset = s3c2410wdt_mask_wdt_reset,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos7 = {
@@ -158,6 +174,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 	.rst_stat_bit = 23,	/* A57 WDTRESET */
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
 		  | QUIRK_HAS_WTCLRINT_REG,
+	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
+	.mask_reset = s3c2410wdt_mask_wdt_reset,
 };
 
 static const struct of_device_id s3c2410_wdt_match[] = {
@@ -200,35 +218,53 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
 	return container_of(nb, struct s3c2410_wdt, freq_transition);
 }
 
-static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
+static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 {
+	const u32 mask_val = 1 << wdt->drv_data->mask_bit;
+	const u32 val = mask ? mask_val : 0;
 	int ret;
-	u32 mask_val = 1 << wdt->drv_data->mask_bit;
-	u32 val = 0;
 
-	/* No need to do anything if no PMU CONFIG needed */
-	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG))
-		return 0;
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
+				 mask_val, val);
+	if (ret < 0)
+		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
-	if (mask)
-		val = mask_val;
+	return ret;
+}
 
-	ret = regmap_update_bits(wdt->pmureg,
-			wdt->drv_data->disable_reg,
-			mask_val, val);
-	if (ret < 0)
-		goto error;
+static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
+{
+	const u32 mask_val = 1 << wdt->drv_data->mask_bit;
+	const u32 val = mask ? mask_val : 0;
+	int ret;
 
-	ret = regmap_update_bits(wdt->pmureg,
-			wdt->drv_data->mask_reset_reg,
-			mask_val, val);
- error:
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
+				 mask_val, val);
 	if (ret < 0)
 		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
 	return ret;
 }
 
+static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
+{
+	int ret;
+
+	if (wdt->drv_data->disable_auto_reset) {
+		ret = wdt->drv_data->disable_auto_reset(wdt, !en);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (wdt->drv_data->mask_reset) {
+		ret = wdt->drv_data->mask_reset(wdt, !en);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
@@ -609,7 +645,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cpufreq;
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -655,7 +691,7 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 	int ret;
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+	ret = s3c2410wdt_enable(wdt, false);
 	if (ret < 0)
 		return ret;
 
@@ -672,8 +708,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
-	s3c2410wdt_mask_and_disable_reset(wdt, true);
-
+	s3c2410wdt_enable(wdt, false);
 	s3c2410wdt_stop(&wdt->wdt_device);
 }
 
@@ -688,7 +723,7 @@ static int s3c2410wdt_suspend(struct device *dev)
 	wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
 	wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+	ret = s3c2410wdt_enable(wdt, false);
 	if (ret < 0)
 		return ret;
 
@@ -708,7 +743,7 @@ static int s3c2410wdt_resume(struct device *dev)
 	writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
 	writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
 		return ret;
 
-- 
2.30.2

[PATCH 4/7] watchdog: s3c2410: Add support for WDT counter enable

[Sam Protsenko]

On new Exynos chips (like Exynos850) WDT counter must be enabled to make
WDT functional. It's done via CLUSTERx_NONCPU_OUT register, in
CNT_EN_WDT bit. Add infrastructure needed to enable that counter.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7c163a257d3c..a5ef7171a90e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -97,12 +97,16 @@ struct s3c2410_wdt;
  * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
  * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
  * reset.
+ * @cnt_en_reg: Offset in pmureg for the register that enables WDT counter.
+ * @cnt_en_bit: Bit number for "watchdog counter enable" in cnt_en register.
  * @quirks: A bitfield of quirks.
  * @disable_auto_reset: If set, this function will be called to disable
  * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
  * disable_reg field.
  * @mask_reset: If set, this function will be called to mask WDT reset request;
  * uses mask_reset_reg and mask_bit fields.
+ * @enable_counter: If set, this function will be called to enable WDT counter;
+ * uses cnt_en_reg and cnt_en_bit fields.
  */
 
 struct s3c2410_wdt_variant {
@@ -111,9 +115,12 @@ struct s3c2410_wdt_variant {
 	int mask_bit;
 	int rst_stat_reg;
 	int rst_stat_bit;
+	int cnt_en_reg;
+	int cnt_en_bit;
 	u32 quirks;
 	int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
 	int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
+	int (*enable_counter)(struct s3c2410_wdt *wdt, bool mask);
 };
 
 struct s3c2410_wdt {
@@ -132,6 +139,7 @@ struct s3c2410_wdt {
 
 static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
 static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
+static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en);
 
 static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
 	.quirks = 0
@@ -246,6 +254,20 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 	return ret;
 }
 
+static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
+{
+	const u32 mask_val = 1 << wdt->drv_data->cnt_en_bit;
+	const u32 val = en ? mask_val : 0;
+	int ret;
+
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
+				 mask_val, val);
+	if (ret < 0)
+		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
+
+	return ret;
+}
+
 static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
 {
 	int ret;
@@ -262,6 +284,12 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
 			return ret;
 	}
 
+	if (wdt->drv_data->enable_counter) {
+		ret = wdt->drv_data->enable_counter(wdt, en);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.30.2

[PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Sam Protsenko]

Some Exynos chips (like Exynos850) have dedicated source clock. That
clock is provided from device tree as "watchdog_src" clock. In such
case, "watchdog" clock is just a peripheral clock used for register
interface. If "watchdog_src" is present, use its rate instead of
"watchdog" for all timer related calculations.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index a5ef7171a90e..bfc5872ca497 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
 struct s3c2410_wdt {
 	struct device		*dev;
 	struct clk		*clock;
+	struct clk		*clock_src;
+	unsigned long		freq_src;
 	void __iomem		*reg_base;
 	unsigned int		count;
 	spinlock_t		lock;
@@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
 
 /* functions */
 
-static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
+static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
 {
-	unsigned long freq = clk_get_rate(clock);
-
 	return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
 				       / S3C2410_WTCON_MAXDIV);
 }
@@ -364,7 +364,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
 				    unsigned int timeout)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
-	unsigned long freq = clk_get_rate(wdt->clock);
+	unsigned long freq = wdt->freq_src;
 	unsigned int count;
 	unsigned int divisor = 1;
 	unsigned long wtcon;
@@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* "watchdog_src" clock is optional; if it's not present -- just skip */
+	wdt->clock_src = devm_clk_get(dev, "watchdog_src");
+	if (!IS_ERR(wdt->clock_src)) {
+		ret = clk_prepare_enable(wdt->clock_src);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable source clock\n");
+			ret = PTR_ERR(wdt->clock_src);
+			goto err_clk;
+		}
+		wdt->freq_src = clk_get_rate(wdt->clock_src);
+	} else {
+		wdt->freq_src = clk_get_rate(wdt->clock);
+	}
+
 	wdt->wdt_device.min_timeout = 1;
-	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
+	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->freq_src);
 
 	ret = s3c2410wdt_cpufreq_register(wdt);
 	if (ret < 0) {
 		dev_err(dev, "failed to register cpufreq\n");
-		goto err_clk;
+		goto err_clk_src;
 	}
 
 	watchdog_set_drvdata(&wdt->wdt_device, wdt);
@@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  err_cpufreq:
 	s3c2410wdt_cpufreq_deregister(wdt);
 
+ err_clk_src:
+	if (!IS_ERR(wdt->clock_src))
+		clk_disable_unprepare(wdt->clock_src);
+
  err_clk:
 	clk_disable_unprepare(wdt->clock);
 
@@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 
 	s3c2410wdt_cpufreq_deregister(wdt);
 
+	if (!IS_ERR(wdt->clock_src))
+		clk_disable_unprepare(wdt->clock_src);
+
 	clk_disable_unprepare(wdt->clock);
 
 	return 0;
-- 
2.30.2

[PATCH 6/7] watchdog: s3c2410: Add Exynos850 support

[Sam Protsenko]

Exynos850 is a bit different from SoCs already supported in WDT driver:
  - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
    always 0; .disable_auto_reset callback is not set for that reason
  - MASK_WDT_RESET_REQUEST register is replaced with
    CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
    reset interrupt it's now enabled with the same value; .mask_reset
    callback is reused for that functionality though
  - To make WDT functional, WDT counter needs to be enabled in
    CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
    callback

Also Exynos850 has two CPU clusters, each has its own dedicated WDT
instance. It takes two different driver data structures (and thus two
different compatibles), as for each cluster there are different
registers and different bits used.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 49 ++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index bfc5872ca497..ca082b1226e3 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -56,6 +56,10 @@
 #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
 #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
 #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c
+#define EXYNOS850_CLUSTER0_NONCPU_OUT		0x1220
+#define EXYNOS850_CLUSTER0_NONCPU_INT_EN	0x1244
+#define EXYNOS850_CLUSTER1_NONCPU_OUT		0x1620
+#define EXYNOS850_CLUSTER1_NONCPU_INT_EN	0x1644
 #define QUIRK_HAS_PMU_CONFIG			(1 << 0)
 #define QUIRK_HAS_RST_STAT			(1 << 1)
 #define QUIRK_HAS_WTCLRINT_REG			(1 << 2)
@@ -141,6 +145,7 @@ struct s3c2410_wdt {
 
 static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
 static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
+static int s3c2410wdt_enable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
 static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en);
 
 static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
@@ -188,6 +193,32 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 	.mask_reset = s3c2410wdt_mask_wdt_reset,
 };
 
+static const struct s3c2410_wdt_variant drv_data_exynos850_cl0 = {
+	.mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN,
+	.mask_bit = 2,
+	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+	.rst_stat_bit = 24,	/* CLUSTER0 WDTRESET */
+	.cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT,
+	.cnt_en_bit = 7,
+	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
+		  | QUIRK_HAS_WTCLRINT_REG,
+	.mask_reset = s3c2410wdt_enable_wdt_reset,
+	.enable_counter = s3c2410wdt_enable_counter,
+};
+
+static const struct s3c2410_wdt_variant drv_data_exynos850_cl1 = {
+	.mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN,
+	.mask_bit = 2,
+	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+	.rst_stat_bit = 23,	/* CLUSTER1 WDTRESET */
+	.cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT,
+	.cnt_en_bit = 7,
+	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
+		  | QUIRK_HAS_WTCLRINT_REG,
+	.mask_reset = s3c2410wdt_enable_wdt_reset,
+	.enable_counter = s3c2410wdt_enable_counter,
+};
+
 static const struct of_device_id s3c2410_wdt_match[] = {
 	{ .compatible = "samsung,s3c2410-wdt",
 	  .data = &drv_data_s3c2410 },
@@ -199,6 +230,10 @@ static const struct of_device_id s3c2410_wdt_match[] = {
 	  .data = &drv_data_exynos5420 },
 	{ .compatible = "samsung,exynos7-wdt",
 	  .data = &drv_data_exynos7 },
+	{ .compatible = "samsung,exynos850-cl0-wdt",
+	  .data = &drv_data_exynos850_cl0 },
+	{ .compatible = "samsung,exynos850-cl1-wdt",
+	  .data = &drv_data_exynos850_cl1 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
@@ -254,6 +289,20 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 	return ret;
 }
 
+static int s3c2410wdt_enable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
+{
+	const u32 mask_val = 1 << wdt->drv_data->mask_bit;
+	const u32 val = mask ? 0 : mask_val; /* reset interrupt enable value */
+	int ret;
+
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
+				 mask_val, val);
+	if (ret < 0)
+		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
+
+	return ret;
+}
+
 static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
 {
 	const u32 mask_val = 1 << wdt->drv_data->cnt_en_bit;
-- 
2.30.2

[PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

When "tmr_atboot" module param is set, the watchdog is started in
driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
watchdog core driver know it's running. This way wathcdog core can kick
the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
enabled), until user space takes control.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index ca082b1226e3..9af014ff1468 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
 	wdt->wdt_device.parent = dev;
 
+	/*
+	 * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
+	 * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
+	 *
+	 * If we're not enabling the watchdog, then ensure it is disabled if it
+	 * has been left running from the bootloader or other source.
+	 */
+	if (tmr_atboot && started == 0) {
+		dev_info(dev, "starting watchdog timer\n");
+		s3c2410wdt_start(&wdt->wdt_device);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
+	} else if (!tmr_atboot) {
+		s3c2410wdt_stop(&wdt->wdt_device);
+	}
+
 	ret = watchdog_register_device(&wdt->wdt_device);
 	if (ret)
 		goto err_cpufreq;
@@ -740,17 +755,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_unregister;
 
-	if (tmr_atboot && started == 0) {
-		dev_info(dev, "starting watchdog timer\n");
-		s3c2410wdt_start(&wdt->wdt_device);
-	} else if (!tmr_atboot) {
-		/* if we're not enabling the watchdog, then ensure it is
-		 * disabled if it has been left running from the bootloader
-		 * or other source */
-
-		s3c2410wdt_stop(&wdt->wdt_device);
-	}
-
 	platform_set_drvdata(pdev, wdt);
 
 	/* print out a statement of readiness */
-- 
2.30.2

Re: [PATCH 4/7] watchdog: s3c2410: Add support for WDT counter enable

[Guenter Roeck]

On 10/28/21 11:35 AM, Sam Protsenko wrote:
> On new Exynos chips (like Exynos850) WDT counter must be enabled to make
> WDT functional. It's done via CLUSTERx_NONCPU_OUT register, in
> CNT_EN_WDT bit. Add infrastructure needed to enable that counter.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/watchdog/s3c2410_wdt.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 7c163a257d3c..a5ef7171a90e 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -97,12 +97,16 @@ struct s3c2410_wdt;
>    * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
>    * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
>    * reset.
> + * @cnt_en_reg: Offset in pmureg for the register that enables WDT counter.
> + * @cnt_en_bit: Bit number for "watchdog counter enable" in cnt_en register.
>    * @quirks: A bitfield of quirks.
>    * @disable_auto_reset: If set, this function will be called to disable
>    * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
>    * disable_reg field.
>    * @mask_reset: If set, this function will be called to mask WDT reset request;
>    * uses mask_reset_reg and mask_bit fields.
> + * @enable_counter: If set, this function will be called to enable WDT counter;
> + * uses cnt_en_reg and cnt_en_bit fields.
>    */
>   
>   struct s3c2410_wdt_variant {
> @@ -111,9 +115,12 @@ struct s3c2410_wdt_variant {
>   	int mask_bit;
>   	int rst_stat_reg;
>   	int rst_stat_bit;
> +	int cnt_en_reg;
> +	int cnt_en_bit;
>   	u32 quirks;
>   	int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
>   	int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
> +	int (*enable_counter)(struct s3c2410_wdt *wdt, bool mask);

Unless there are different enable functions in the future,
the function is unnecessary. This can be handled as feature bit.

>   };
>   
>   struct s3c2410_wdt {
> @@ -132,6 +139,7 @@ struct s3c2410_wdt {
>   
>   static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
>   static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> +static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en);
>   
>   static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
>   	.quirks = 0
> @@ -246,6 +254,20 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>   	return ret;
>   }
>   
> +static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> +{
> +	const u32 mask_val = 1 << wdt->drv_data->cnt_en_bit;

BIT()

> +	const u32 val = en ? mask_val : 0;
> +	int ret;
> +
> +	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> +				 mask_val, val);
> +	if (ret < 0)
> +		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> +
> +	return ret;
> +}
> +
>   static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
>   {
>   	int ret;
> @@ -262,6 +284,12 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
>   			return ret;
>   	}
>   
> +	if (wdt->drv_data->enable_counter) {
> +		ret = wdt->drv_data->enable_counter(wdt, en);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
>   
> 

Re: [PATCH 3/7] watchdog: s3c2410: Make reset disable optional

[Guenter Roeck]

On 10/28/21 11:35 AM, Sam Protsenko wrote:
> Not all SoCs have AUTOMATIC_WDT_RESET_DISABLE register, examples are
> Exynos850 and Exynos9. On such chips reset disable register shouldn't be
> accessed. Provide a way to avoid handling that register. This is done by
> introducing separate callbacks to driver data structure: one for reset
> disable register, and one for mask reset register. Now those callbacks
> can be checked and called only when those were set in driver data.
> 
> This commit doesn't bring any functional change to existing devices, but
> merely provides an infrastructure for upcoming chips support.
> 

That doesn't explain why the callbacks are needed instead of additional
feature flags.

Guenter

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/watchdog/s3c2410_wdt.c | 81 ++++++++++++++++++++++++----------
>   1 file changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 2395f353e52d..7c163a257d3c 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -83,6 +83,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
>   
> +struct s3c2410_wdt;
> +
>   /**
>    * struct s3c2410_wdt_variant - Per-variant config data
>    *
> @@ -96,6 +98,11 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
>    * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
>    * reset.
>    * @quirks: A bitfield of quirks.
> + * @disable_auto_reset: If set, this function will be called to disable
> + * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
> + * disable_reg field.
> + * @mask_reset: If set, this function will be called to mask WDT reset request;
> + * uses mask_reset_reg and mask_bit fields.
>    */
>   
>   struct s3c2410_wdt_variant {
> @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>   	int rst_stat_reg;
>   	int rst_stat_bit;
>   	u32 quirks;
> +	int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
> +	int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
>   };
>   
>   struct s3c2410_wdt {
> @@ -121,6 +130,9 @@ struct s3c2410_wdt {
>   	struct regmap *pmureg;
>   };
>   
> +static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> +static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> +
>   static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
>   	.quirks = 0
>   };
> @@ -138,6 +150,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
>   	.rst_stat_bit = 20,
>   	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
>   		  | QUIRK_HAS_WTCLRINT_REG,
> +	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> +	.mask_reset = s3c2410wdt_mask_wdt_reset,
>   };
>   
>   static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> @@ -148,6 +162,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>   	.rst_stat_bit = 9,
>   	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
>   		  | QUIRK_HAS_WTCLRINT_REG,
> +	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> +	.mask_reset = s3c2410wdt_mask_wdt_reset,
>   };
>   
>   static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> @@ -158,6 +174,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
>   	.rst_stat_bit = 23,	/* A57 WDTRESET */
>   	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
>   		  | QUIRK_HAS_WTCLRINT_REG,
> +	.disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> +	.mask_reset = s3c2410wdt_mask_wdt_reset,
>   };
>   
>   static const struct of_device_id s3c2410_wdt_match[] = {
> @@ -200,35 +218,53 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
>   	return container_of(nb, struct s3c2410_wdt, freq_transition);
>   }
>   
> -static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
> +static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>   {
> +	const u32 mask_val = 1 << wdt->drv_data->mask_bit;
> +	const u32 val = mask ? mask_val : 0;
>   	int ret;
> -	u32 mask_val = 1 << wdt->drv_data->mask_bit;
> -	u32 val = 0;
>   
> -	/* No need to do anything if no PMU CONFIG needed */
> -	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG))
> -		return 0;
> +	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> +				 mask_val, val);
> +	if (ret < 0)
> +		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>   
> -	if (mask)
> -		val = mask_val;
> +	return ret;
> +}
>   
> -	ret = regmap_update_bits(wdt->pmureg,
> -			wdt->drv_data->disable_reg,
> -			mask_val, val);
> -	if (ret < 0)
> -		goto error;
> +static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> +{
> +	const u32 mask_val = 1 << wdt->drv_data->mask_bit;
> +	const u32 val = mask ? mask_val : 0;
> +	int ret;
>   
> -	ret = regmap_update_bits(wdt->pmureg,
> -			wdt->drv_data->mask_reset_reg,
> -			mask_val, val);
> - error:
> +	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> +				 mask_val, val);
>   	if (ret < 0)
>   		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>   
>   	return ret;
>   }
>   
> +static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> +{
> +	int ret;
> +
> +	if (wdt->drv_data->disable_auto_reset) {
> +		ret = wdt->drv_data->disable_auto_reset(wdt, !en);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (wdt->drv_data->mask_reset) {
> +		ret = wdt->drv_data->mask_reset(wdt, !en);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
>   {
>   	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> @@ -609,7 +645,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err_cpufreq;
>   
> -	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
> +	ret = s3c2410wdt_enable(wdt, true);
>   	if (ret < 0)
>   		goto err_unregister;
>   
> @@ -655,7 +691,7 @@ static int s3c2410wdt_remove(struct platform_device *dev)
>   	int ret;
>   	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
>   
> -	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
> +	ret = s3c2410wdt_enable(wdt, false);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -672,8 +708,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
>   {
>   	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
>   
> -	s3c2410wdt_mask_and_disable_reset(wdt, true);
> -
> +	s3c2410wdt_enable(wdt, false);
>   	s3c2410wdt_stop(&wdt->wdt_device);
>   }
>   
> @@ -688,7 +723,7 @@ static int s3c2410wdt_suspend(struct device *dev)
>   	wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
>   	wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
>   
> -	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
> +	ret = s3c2410wdt_enable(wdt, false);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -708,7 +743,7 @@ static int s3c2410wdt_resume(struct device *dev)
>   	writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
>   	writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);
>   
> -	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
> +	ret = s3c2410wdt_enable(wdt, true);
>   	if (ret < 0)
>   		return ret;
>   
> 

Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Guenter Roeck]

On 10/28/21 11:35 AM, Sam Protsenko wrote:
> Some Exynos chips (like Exynos850) have dedicated source clock. That
> clock is provided from device tree as "watchdog_src" clock. In such
> case, "watchdog" clock is just a peripheral clock used for register
> interface. If "watchdog_src" is present, use its rate instead of
> "watchdog" for all timer related calculations.
> 

If the "watchdog_src" clock is present, is "watchdog" clock still needed ?
Please state that explicitly, since it is kind of unusual.

Guenter

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index a5ef7171a90e..bfc5872ca497 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
>   struct s3c2410_wdt {
>   	struct device		*dev;
>   	struct clk		*clock;
> +	struct clk		*clock_src;
> +	unsigned long		freq_src;
>   	void __iomem		*reg_base;
>   	unsigned int		count;
>   	spinlock_t		lock;
> @@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
>   
>   /* functions */
>   
> -static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
> +static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
>   {
> -	unsigned long freq = clk_get_rate(clock);
> -
>   	return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
>   				       / S3C2410_WTCON_MAXDIV);
>   }
> @@ -364,7 +364,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
>   				    unsigned int timeout)
>   {
>   	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> -	unsigned long freq = clk_get_rate(wdt->clock);
> +	unsigned long freq = wdt->freq_src;
>   	unsigned int count;
>   	unsigned int divisor = 1;
>   	unsigned long wtcon;
> @@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	/* "watchdog_src" clock is optional; if it's not present -- just skip */
> +	wdt->clock_src = devm_clk_get(dev, "watchdog_src");
> +	if (!IS_ERR(wdt->clock_src)) {
> +		ret = clk_prepare_enable(wdt->clock_src);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable source clock\n");
> +			ret = PTR_ERR(wdt->clock_src);
> +			goto err_clk;
> +		}
> +		wdt->freq_src = clk_get_rate(wdt->clock_src);
> +	} else {
> +		wdt->freq_src = clk_get_rate(wdt->clock);
> +	}
> +
>   	wdt->wdt_device.min_timeout = 1;
> -	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
> +	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->freq_src);
>   
>   	ret = s3c2410wdt_cpufreq_register(wdt);
>   	if (ret < 0) {
>   		dev_err(dev, "failed to register cpufreq\n");
> -		goto err_clk;
> +		goto err_clk_src;
>   	}
>   
>   	watchdog_set_drvdata(&wdt->wdt_device, wdt);
> @@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>    err_cpufreq:
>   	s3c2410wdt_cpufreq_deregister(wdt);
>   
> + err_clk_src:
> +	if (!IS_ERR(wdt->clock_src))
> +		clk_disable_unprepare(wdt->clock_src);
> +
>    err_clk:
>   	clk_disable_unprepare(wdt->clock);
>   
> @@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
>   
>   	s3c2410wdt_cpufreq_deregister(wdt);
>   
> +	if (!IS_ERR(wdt->clock_src))
> +		clk_disable_unprepare(wdt->clock_src);
> +
>   	clk_disable_unprepare(wdt->clock);
>   
>   	return 0;
> 

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Guenter Roeck]

On 10/28/21 11:35 AM, Sam Protsenko wrote:
> When "tmr_atboot" module param is set, the watchdog is started in
> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> watchdog core driver know it's running. This way wathcdog core can kick
> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> enabled), until user space takes control.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index ca082b1226e3..9af014ff1468 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
>   	wdt->wdt_device.parent = dev;
>   
> +	/*
> +	 * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> +	 * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> +	 *
> +	 * If we're not enabling the watchdog, then ensure it is disabled if it
> +	 * has been left running from the bootloader or other source.
> +	 */
> +	if (tmr_atboot && started == 0) {
> +		dev_info(dev, "starting watchdog timer\n");
> +		s3c2410wdt_start(&wdt->wdt_device);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> +	} else if (!tmr_atboot) {
> +		s3c2410wdt_stop(&wdt->wdt_device);
> +	}
> +

This doesn't cover the case where the watchdog is already enabled by the BIOS.
In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
if the userspace handler is not loaded fast enough. The code should consistently
set WDOG_HW_RUNNING if the watchdog is running.

Guenter

>   	ret = watchdog_register_device(&wdt->wdt_device);
>   	if (ret)
>   		goto err_cpufreq;
> @@ -740,17 +755,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		goto err_unregister;
>   
> -	if (tmr_atboot && started == 0) {
> -		dev_info(dev, "starting watchdog timer\n");
> -		s3c2410wdt_start(&wdt->wdt_device);
> -	} else if (!tmr_atboot) {
> -		/* if we're not enabling the watchdog, then ensure it is
> -		 * disabled if it has been left running from the bootloader
> -		 * or other source */
> -
> -		s3c2410wdt_stop(&wdt->wdt_device);
> -	}
> -
>   	platform_set_drvdata(pdev, wdt);
>   
>   	/* print out a statement of readiness */
> 

Re: [PATCH 1/7] dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7

[Krzysztof Kozlowski]

On 28/10/2021 20:35, Sam Protsenko wrote:
> Exynos7 watchdog driver is clearly indicating that its dts node must
> define syscon phandle property. That was probably forgotten, so add it.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Fixes: 2b9366b66967 ("watchdog: s3c2410_wdt: Add support for Watchdog
device on Exynos7")

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: watchdog: Document Exynos850 watchdog bindings

[Krzysztof Kozlowski]

On 28/10/2021 20:35, Sam Protsenko wrote:
> Exynos850 SoC has two CPU clusters:
>   - cluster 0: contains CPUs #0, #1, #2, #3
>   - cluster 1: contains CPUs #4, #5, #6, #7
> 
> Each cluster has its own dedicater watchdog timer. Those WDT instances
> are controlled using different bits in PMU registers, so there should be
> two different compatible strings (for each cluster), to tell the driver
> which bits to use for each WDT instance.
> 
> Also on Exynos850 the peripheral clock and the source clock are two
> different clocks. Provide a way to specify two clocks in watchdog device
> tree node.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../devicetree/bindings/watchdog/samsung-wdt.yaml  | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> index 93cd77a6e92c..19c7f7767559 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> @@ -22,16 +22,24 @@ properties:
>        - samsung,exynos5250-wdt                # for Exynos5250
>        - samsung,exynos5420-wdt                # for Exynos5420
>        - samsung,exynos7-wdt                   # for Exynos7
> +      - samsung,exynos850-cl0-wdt             # for Exynos850 (CPU cluster 0)
> +      - samsung,exynos850-cl1-wdt             # for Exynos850 (CPU cluster 1)

I would prefer to have one compatible and additional u32 property
pointing to cluster index. The driver would use this property to adjust
the PMU register offsets or bits.

Why? Because if next time you have three clusters, you will need to make
three compatibles for something which differs by only two register
offsets. Both watchdog instances (or three in some unspecified future)
are here the same, they just control different blocks, therefore should
accept some parameter instead of making them different compatibles.

>  
>    reg:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Peripheral clock used for register interface; if it's the
> +                     only clock, it's also a source clock
> +      - description: Source clock (optional)
>  
>    clock-names:
> +    minItems: 1
>      items:
>        - const: watchdog
> +      - const: watchdog_src

Don't you require src clock on Exynos850?



Best regards,
Krzysztof

Re: [PATCH 3/7] watchdog: s3c2410: Make reset disable optional

[Krzysztof Kozlowski]

On 29/10/2021 02:16, Guenter Roeck wrote:
> On 10/28/21 11:35 AM, Sam Protsenko wrote:
>> Not all SoCs have AUTOMATIC_WDT_RESET_DISABLE register, examples are
>> Exynos850 and Exynos9. On such chips reset disable register shouldn't be
>> accessed. Provide a way to avoid handling that register. This is done by
>> introducing separate callbacks to driver data structure: one for reset
>> disable register, and one for mask reset register. Now those callbacks
>> can be checked and called only when those were set in driver data.
>>
>> This commit doesn't bring any functional change to existing devices, but
>> merely provides an infrastructure for upcoming chips support.
>>
> 
> That doesn't explain why the callbacks are needed instead of additional
> feature flags.
> 

Or why not skipping the disable operations if disable_reg is not provided?


Best regards,
Krzysztof

Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Krzysztof Kozlowski]

On 28/10/2021 20:35, Sam Protsenko wrote:
> Some Exynos chips (like Exynos850) have dedicated source clock. That
> clock is provided from device tree as "watchdog_src" clock. In such
> case, "watchdog" clock is just a peripheral clock used for register
> interface. If "watchdog_src" is present, use its rate instead of
> "watchdog" for all timer related calculations.

Please explain what is this source clock and remove the reference to
devicetree. Instead describe rather real HW. It's confusing now to have
one clock called watchdog and one watchdog source.

The source clock is the actual clock driving watchdog and it's counter,
right? Then let's document it and rename the variables to match reality
- one is pclk (or apb?) and second is counter or source?

> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index a5ef7171a90e..bfc5872ca497 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
>  struct s3c2410_wdt {
>  	struct device		*dev;
>  	struct clk		*clock;
> +	struct clk		*clock_src;
> +	unsigned long		freq_src;
>  	void __iomem		*reg_base;
>  	unsigned int		count;
>  	spinlock_t		lock;
> @@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
>  
>  /* functions */
>  
> -static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
> +static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
>  {
> -	unsigned long freq = clk_get_rate(clock);
> -
>  	return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
>  				       / S3C2410_WTCON_MAXDIV);
>  }
> @@ -364,7 +364,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
>  				    unsigned int timeout)
>  {
>  	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> -	unsigned long freq = clk_get_rate(wdt->clock);
> +	unsigned long freq = wdt->freq_src;

This does not look good. You are using fixed frequency (from probe).

>  	unsigned int count;
>  	unsigned int divisor = 1;
>  	unsigned long wtcon;
> @@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* "watchdog_src" clock is optional; if it's not present -- just skip */
> +	wdt->clock_src = devm_clk_get(dev, "watchdog_src");
> +	if (!IS_ERR(wdt->clock_src)) {
> +		ret = clk_prepare_enable(wdt->clock_src);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable source clock\n");
> +			ret = PTR_ERR(wdt->clock_src);
> +			goto err_clk;
> +		}
> +		wdt->freq_src = clk_get_rate(wdt->clock_src);
> +	} else {
> +		wdt->freq_src = clk_get_rate(wdt->clock);
> +	}
> +
>  	wdt->wdt_device.min_timeout = 1;
> -	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
> +	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->freq_src);
>  
>  	ret = s3c2410wdt_cpufreq_register(wdt);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to register cpufreq\n");
> -		goto err_clk;
> +		goto err_clk_src;
>  	}
>  
>  	watchdog_set_drvdata(&wdt->wdt_device, wdt);
> @@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   err_cpufreq:
>  	s3c2410wdt_cpufreq_deregister(wdt);
>  
> + err_clk_src:
> +	if (!IS_ERR(wdt->clock_src))
> +		clk_disable_unprepare(wdt->clock_src);

No. Errors in getting source clock should not be ignored, so you should
never store here ERR. You could store NULL. If() is anyway not needed in
both cases.

You can simplify all this and take pclk twice if src clock is missing.
Or assign src=pclk...

> +
>   err_clk:
>  	clk_disable_unprepare(wdt->clock);
>  
> @@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
>  
>  	s3c2410wdt_cpufreq_deregister(wdt);
>  
> +	if (!IS_ERR(wdt->clock_src))
> +		clk_disable_unprepare(wdt->clock_src);
> +
>  	clk_disable_unprepare(wdt->clock);
>  
>  	return 0;
> 


Best regards,
Krzysztof

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Krzysztof Kozlowski]

On 28/10/2021 20:35, Sam Protsenko wrote:
> When "tmr_atboot" module param is set, the watchdog is started in
> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> watchdog core driver know it's running. This way wathcdog core can kick

s/wathcdog/watchdog/

> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> enabled), until user space takes control.

This does not explain why you move the code around. I guess you miss
here information that we should start the watchdog before registering
it? If so please explain why.

> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index ca082b1226e3..9af014ff1468 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
>  	wdt->wdt_device.parent = dev;
>  
> +	/*
> +	 * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> +	 * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> +	 *
> +	 * If we're not enabling the watchdog, then ensure it is disabled if it
> +	 * has been left running from the bootloader or other source.
> +	 */
> +	if (tmr_atboot && started == 0) {
> +		dev_info(dev, "starting watchdog timer\n");
> +		s3c2410wdt_start(&wdt->wdt_device);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> +	} else if (!tmr_atboot) {
> +		s3c2410wdt_stop(&wdt->wdt_device);
> +	}
> +
>  	ret = watchdog_register_device(&wdt->wdt_device);
>  	if (ret)
>  		goto err_cpufreq;
> @@ -740,17 +755,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_unregister;
>  
> -	if (tmr_atboot && started == 0) {
> -		dev_info(dev, "starting watchdog timer\n");
> -		s3c2410wdt_start(&wdt->wdt_device);
> -	} else if (!tmr_atboot) {
> -		/* if we're not enabling the watchdog, then ensure it is
> -		 * disabled if it has been left running from the bootloader
> -		 * or other source */
> -
> -		s3c2410wdt_stop(&wdt->wdt_device);
> -	}
> -
>  	platform_set_drvdata(pdev, wdt);
>  
>  	/* print out a statement of readiness */
> 


Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: watchdog: Document Exynos850 watchdog bindings

[Sam Protsenko]

On Fri, 29 Oct 2021 at 11:02, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 28/10/2021 20:35, Sam Protsenko wrote:
> > Exynos850 SoC has two CPU clusters:
> >   - cluster 0: contains CPUs #0, #1, #2, #3
> >   - cluster 1: contains CPUs #4, #5, #6, #7
> >
> > Each cluster has its own dedicater watchdog timer. Those WDT instances
> > are controlled using different bits in PMU registers, so there should be
> > two different compatible strings (for each cluster), to tell the driver
> > which bits to use for each WDT instance.
> >
> > Also on Exynos850 the peripheral clock and the source clock are two
> > different clocks. Provide a way to specify two clocks in watchdog device
> > tree node.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  .../devicetree/bindings/watchdog/samsung-wdt.yaml  | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> > index 93cd77a6e92c..19c7f7767559 100644
> > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> > @@ -22,16 +22,24 @@ properties:
> >        - samsung,exynos5250-wdt                # for Exynos5250
> >        - samsung,exynos5420-wdt                # for Exynos5420
> >        - samsung,exynos7-wdt                   # for Exynos7
> > +      - samsung,exynos850-cl0-wdt             # for Exynos850 (CPU cluster 0)
> > +      - samsung,exynos850-cl1-wdt             # for Exynos850 (CPU cluster 1)
>
> I would prefer to have one compatible and additional u32 property
> pointing to cluster index. The driver would use this property to adjust
> the PMU register offsets or bits.
>
> Why? Because if next time you have three clusters, you will need to make
> three compatibles for something which differs by only two register
> offsets. Both watchdog instances (or three in some unspecified future)
> are here the same, they just control different blocks, therefore should
> accept some parameter instead of making them different compatibles.
>

Agreed. I considered both cases, both looked ugly to me. But having
one compatible is probably better in the end, although it'll require
some additional code in the driver. Anyway, will be done in v2, will
send it soon.

> >
> >    reg:
> >      maxItems: 1
> >
> >    clocks:
> > -    maxItems: 1
> > +    minItems: 1
> > +    items:
> > +      - description: Peripheral clock used for register interface; if it's the
> > +                     only clock, it's also a source clock
> > +      - description: Source clock (optional)
> >
> >    clock-names:
> > +    minItems: 1
> >      items:
> >        - const: watchdog
> > +      - const: watchdog_src
>
> Don't you require src clock on Exynos850?
>

Will be addressed in v2 properly, thanks.

>
>
> Best regards,
> Krzysztof

Re: [PATCH 3/7] watchdog: s3c2410: Make reset disable optional

[Sam Protsenko]

On Fri, 29 Oct 2021 at 03:16, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> > Not all SoCs have AUTOMATIC_WDT_RESET_DISABLE register, examples are
> > Exynos850 and Exynos9. On such chips reset disable register shouldn't be
> > accessed. Provide a way to avoid handling that register. This is done by
> > introducing separate callbacks to driver data structure: one for reset
> > disable register, and one for mask reset register. Now those callbacks
> > can be checked and called only when those were set in driver data.
> >
> > This commit doesn't bring any functional change to existing devices, but
> > merely provides an infrastructure for upcoming chips support.
> >
>
> That doesn't explain why the callbacks are needed instead of additional
> feature flags.
>

Ok, I'll rework this patch using quirks, don't have a strong
preference on this one. That's actually how I did that on my first
attempt. Guess I just like "callbacks way" more, matter of taste :)
But we already have quirks for that, so yeah, probably makes sense to
use those for consistency.

> Guenter
>
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   drivers/watchdog/s3c2410_wdt.c | 81 ++++++++++++++++++++++++----------
> >   1 file changed, 58 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 2395f353e52d..7c163a257d3c 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -83,6 +83,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> >                       __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >   MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
> >
> > +struct s3c2410_wdt;
> > +
> >   /**
> >    * struct s3c2410_wdt_variant - Per-variant config data
> >    *
> > @@ -96,6 +98,11 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
> >    * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> >    * reset.
> >    * @quirks: A bitfield of quirks.
> > + * @disable_auto_reset: If set, this function will be called to disable
> > + * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
> > + * disable_reg field.
> > + * @mask_reset: If set, this function will be called to mask WDT reset request;
> > + * uses mask_reset_reg and mask_bit fields.
> >    */
> >
> >   struct s3c2410_wdt_variant {
> > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
> >       int rst_stat_reg;
> >       int rst_stat_bit;
> >       u32 quirks;
> > +     int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
> > +     int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
> >   };
> >
> >   struct s3c2410_wdt {
> > @@ -121,6 +130,9 @@ struct s3c2410_wdt {
> >       struct regmap *pmureg;
> >   };
> >
> > +static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> > +static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> > +
> >   static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> >       .quirks = 0
> >   };
> > @@ -138,6 +150,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
> >       .rst_stat_bit = 20,
> >       .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> >                 | QUIRK_HAS_WTCLRINT_REG,
> > +     .disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> > +     .mask_reset = s3c2410wdt_mask_wdt_reset,
> >   };
> >
> >   static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> > @@ -148,6 +162,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> >       .rst_stat_bit = 9,
> >       .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> >                 | QUIRK_HAS_WTCLRINT_REG,
> > +     .disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> > +     .mask_reset = s3c2410wdt_mask_wdt_reset,
> >   };
> >
> >   static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> > @@ -158,6 +174,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> >       .rst_stat_bit = 23,     /* A57 WDTRESET */
> >       .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> >                 | QUIRK_HAS_WTCLRINT_REG,
> > +     .disable_auto_reset = s3c2410wdt_disable_wdt_reset,
> > +     .mask_reset = s3c2410wdt_mask_wdt_reset,
> >   };
> >
> >   static const struct of_device_id s3c2410_wdt_match[] = {
> > @@ -200,35 +218,53 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
> >       return container_of(nb, struct s3c2410_wdt, freq_transition);
> >   }
> >
> > -static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
> > +static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> >   {
> > +     const u32 mask_val = 1 << wdt->drv_data->mask_bit;
> > +     const u32 val = mask ? mask_val : 0;
> >       int ret;
> > -     u32 mask_val = 1 << wdt->drv_data->mask_bit;
> > -     u32 val = 0;
> >
> > -     /* No need to do anything if no PMU CONFIG needed */
> > -     if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG))
> > -             return 0;
> > +     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> > +                              mask_val, val);
> > +     if (ret < 0)
> > +             dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > -     if (mask)
> > -             val = mask_val;
> > +     return ret;
> > +}
> >
> > -     ret = regmap_update_bits(wdt->pmureg,
> > -                     wdt->drv_data->disable_reg,
> > -                     mask_val, val);
> > -     if (ret < 0)
> > -             goto error;
> > +static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> > +{
> > +     const u32 mask_val = 1 << wdt->drv_data->mask_bit;
> > +     const u32 val = mask ? mask_val : 0;
> > +     int ret;
> >
> > -     ret = regmap_update_bits(wdt->pmureg,
> > -                     wdt->drv_data->mask_reset_reg,
> > -                     mask_val, val);
> > - error:
> > +     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> > +                              mask_val, val);
> >       if (ret < 0)
> >               dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> >       return ret;
> >   }
> >
> > +static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> > +{
> > +     int ret;
> > +
> > +     if (wdt->drv_data->disable_auto_reset) {
> > +             ret = wdt->drv_data->disable_auto_reset(wdt, !en);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     if (wdt->drv_data->mask_reset) {
> > +             ret = wdt->drv_data->mask_reset(wdt, !en);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
> >   {
> >       struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> > @@ -609,7 +645,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto err_cpufreq;
> >
> > -     ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
> > +     ret = s3c2410wdt_enable(wdt, true);
> >       if (ret < 0)
> >               goto err_unregister;
> >
> > @@ -655,7 +691,7 @@ static int s3c2410wdt_remove(struct platform_device *dev)
> >       int ret;
> >       struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
> >
> > -     ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
> > +     ret = s3c2410wdt_enable(wdt, false);
> >       if (ret < 0)
> >               return ret;
> >
> > @@ -672,8 +708,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
> >   {
> >       struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
> >
> > -     s3c2410wdt_mask_and_disable_reset(wdt, true);
> > -
> > +     s3c2410wdt_enable(wdt, false);
> >       s3c2410wdt_stop(&wdt->wdt_device);
> >   }
> >
> > @@ -688,7 +723,7 @@ static int s3c2410wdt_suspend(struct device *dev)
> >       wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
> >       wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
> >
> > -     ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
> > +     ret = s3c2410wdt_enable(wdt, false);
> >       if (ret < 0)
> >               return ret;
> >
> > @@ -708,7 +743,7 @@ static int s3c2410wdt_resume(struct device *dev)
> >       writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
> >       writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);
> >
> > -     ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
> > +     ret = s3c2410wdt_enable(wdt, true);
> >       if (ret < 0)
> >               return ret;
> >
> >
>

Re: [PATCH 3/7] watchdog: s3c2410: Make reset disable optional

[Sam Protsenko]

On Fri, 29 Oct 2021 at 11:04, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 29/10/2021 02:16, Guenter Roeck wrote:
> > On 10/28/21 11:35 AM, Sam Protsenko wrote:
> >> Not all SoCs have AUTOMATIC_WDT_RESET_DISABLE register, examples are
> >> Exynos850 and Exynos9. On such chips reset disable register shouldn't be
> >> accessed. Provide a way to avoid handling that register. This is done by
> >> introducing separate callbacks to driver data structure: one for reset
> >> disable register, and one for mask reset register. Now those callbacks
> >> can be checked and called only when those were set in driver data.
> >>
> >> This commit doesn't bring any functional change to existing devices, but
> >> merely provides an infrastructure for upcoming chips support.
> >>
> >
> > That doesn't explain why the callbacks are needed instead of additional
> > feature flags.
> >
>
> Or why not skipping the disable operations if disable_reg is not provided?
>

Yeah, that was my first thought too :) Then I figured disable_reg is
offset, and 0x0 is a valid offset too. Anyway, I'll rework this patch
using quirks, as discussed above. Will send v2 soon.

>
> Best regards,
> Krzysztof

Re: [PATCH 4/7] watchdog: s3c2410: Add support for WDT counter enable

[Sam Protsenko]

On Fri, 29 Oct 2021 at 03:16, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> > On new Exynos chips (like Exynos850) WDT counter must be enabled to make
> > WDT functional. It's done via CLUSTERx_NONCPU_OUT register, in
> > CNT_EN_WDT bit. Add infrastructure needed to enable that counter.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   drivers/watchdog/s3c2410_wdt.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 7c163a257d3c..a5ef7171a90e 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -97,12 +97,16 @@ struct s3c2410_wdt;
> >    * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
> >    * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> >    * reset.
> > + * @cnt_en_reg: Offset in pmureg for the register that enables WDT counter.
> > + * @cnt_en_bit: Bit number for "watchdog counter enable" in cnt_en register.
> >    * @quirks: A bitfield of quirks.
> >    * @disable_auto_reset: If set, this function will be called to disable
> >    * automatic setting the WDT as a reset reason in RST_STAT on CPU reset; uses
> >    * disable_reg field.
> >    * @mask_reset: If set, this function will be called to mask WDT reset request;
> >    * uses mask_reset_reg and mask_bit fields.
> > + * @enable_counter: If set, this function will be called to enable WDT counter;
> > + * uses cnt_en_reg and cnt_en_bit fields.
> >    */
> >
> >   struct s3c2410_wdt_variant {
> > @@ -111,9 +115,12 @@ struct s3c2410_wdt_variant {
> >       int mask_bit;
> >       int rst_stat_reg;
> >       int rst_stat_bit;
> > +     int cnt_en_reg;
> > +     int cnt_en_bit;
> >       u32 quirks;
> >       int (*disable_auto_reset)(struct s3c2410_wdt *wdt, bool mask);
> >       int (*mask_reset)(struct s3c2410_wdt *wdt, bool mask);
> > +     int (*enable_counter)(struct s3c2410_wdt *wdt, bool mask);
>
> Unless there are different enable functions in the future,
> the function is unnecessary. This can be handled as feature bit.
>

Thanks for review. I've reworked all patches to use quirk bits instead
of callbacks. Will send v2 soon.

> >   };
> >
> >   struct s3c2410_wdt {
> > @@ -132,6 +139,7 @@ struct s3c2410_wdt {
> >
> >   static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> >   static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask);
> > +static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en);
> >
> >   static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> >       .quirks = 0
> > @@ -246,6 +254,20 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> >       return ret;
> >   }
> >
> > +static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> > +{
> > +     const u32 mask_val = 1 << wdt->drv_data->cnt_en_bit;
>
> BIT()
>
> > +     const u32 val = en ? mask_val : 0;
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> > +                              mask_val, val);
> > +     if (ret < 0)
> > +             dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> >   static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> >   {
> >       int ret;
> > @@ -262,6 +284,12 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> >                       return ret;
> >       }
> >
> > +     if (wdt->drv_data->enable_counter) {
> > +             ret = wdt->drv_data->enable_counter(wdt, en);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> >       return 0;
> >   }
> >
> >
>

Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Sam Protsenko]

On Fri, 29 Oct 2021 at 03:21, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> > Some Exynos chips (like Exynos850) have dedicated source clock. That
> > clock is provided from device tree as "watchdog_src" clock. In such
> > case, "watchdog" clock is just a peripheral clock used for register
> > interface. If "watchdog_src" is present, use its rate instead of
> > "watchdog" for all timer related calculations.
> >
>
> If the "watchdog_src" clock is present, is "watchdog" clock still needed ?
> Please state that explicitly, since it is kind of unusual.
>

Done, I've reworded the commit message. Will send v2 soon, thanks.

> Guenter
>
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
> >   1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index a5ef7171a90e..bfc5872ca497 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
> >   struct s3c2410_wdt {
> >       struct device           *dev;
> >       struct clk              *clock;
> > +     struct clk              *clock_src;
> > +     unsigned long           freq_src;
> >       void __iomem            *reg_base;
> >       unsigned int            count;
> >       spinlock_t              lock;
> > @@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
> >
> >   /* functions */
> >
> > -static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
> > +static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
> >   {
> > -     unsigned long freq = clk_get_rate(clock);
> > -
> >       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> >                                      / S3C2410_WTCON_MAXDIV);
> >   }
> > @@ -364,7 +364,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> >                                   unsigned int timeout)
> >   {
> >       struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> > -     unsigned long freq = clk_get_rate(wdt->clock);
> > +     unsigned long freq = wdt->freq_src;
> >       unsigned int count;
> >       unsigned int divisor = 1;
> >       unsigned long wtcon;
> > @@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     /* "watchdog_src" clock is optional; if it's not present -- just skip */
> > +     wdt->clock_src = devm_clk_get(dev, "watchdog_src");
> > +     if (!IS_ERR(wdt->clock_src)) {
> > +             ret = clk_prepare_enable(wdt->clock_src);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable source clock\n");
> > +                     ret = PTR_ERR(wdt->clock_src);
> > +                     goto err_clk;
> > +             }
> > +             wdt->freq_src = clk_get_rate(wdt->clock_src);
> > +     } else {
> > +             wdt->freq_src = clk_get_rate(wdt->clock);
> > +     }
> > +
> >       wdt->wdt_device.min_timeout = 1;
> > -     wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
> > +     wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->freq_src);
> >
> >       ret = s3c2410wdt_cpufreq_register(wdt);
> >       if (ret < 0) {
> >               dev_err(dev, "failed to register cpufreq\n");
> > -             goto err_clk;
> > +             goto err_clk_src;
> >       }
> >
> >       watchdog_set_drvdata(&wdt->wdt_device, wdt);
> > @@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >    err_cpufreq:
> >       s3c2410wdt_cpufreq_deregister(wdt);
> >
> > + err_clk_src:
> > +     if (!IS_ERR(wdt->clock_src))
> > +             clk_disable_unprepare(wdt->clock_src);
> > +
> >    err_clk:
> >       clk_disable_unprepare(wdt->clock);
> >
> > @@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
> >
> >       s3c2410wdt_cpufreq_deregister(wdt);
> >
> > +     if (!IS_ERR(wdt->clock_src))
> > +             clk_disable_unprepare(wdt->clock_src);
> > +
> >       clk_disable_unprepare(wdt->clock);
> >
> >       return 0;
> >
>

Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Sam Protsenko]

On Fri, 29 Oct 2021 at 11:18, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 28/10/2021 20:35, Sam Protsenko wrote:
> > Some Exynos chips (like Exynos850) have dedicated source clock. That
> > clock is provided from device tree as "watchdog_src" clock. In such
> > case, "watchdog" clock is just a peripheral clock used for register
> > interface. If "watchdog_src" is present, use its rate instead of
> > "watchdog" for all timer related calculations.
>
> Please explain what is this source clock and remove the reference to
> devicetree. Instead describe rather real HW. It's confusing now to have
> one clock called watchdog and one watchdog source.
>
> The source clock is the actual clock driving watchdog and it's counter,
> right? Then let's document it and rename the variables to match reality
> - one is pclk (or apb?) and second is counter or source?
>

Done, will be present in v2.

> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index a5ef7171a90e..bfc5872ca497 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
> >  struct s3c2410_wdt {
> >       struct device           *dev;
> >       struct clk              *clock;
> > +     struct clk              *clock_src;
> > +     unsigned long           freq_src;
> >       void __iomem            *reg_base;
> >       unsigned int            count;
> >       spinlock_t              lock;
> > @@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
> >
> >  /* functions */
> >
> > -static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
> > +static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
> >  {
> > -     unsigned long freq = clk_get_rate(clock);
> > -
> >       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> >                                      / S3C2410_WTCON_MAXDIV);
> >  }
> > @@ -364,7 +364,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> >                                   unsigned int timeout)
> >  {
> >       struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> > -     unsigned long freq = clk_get_rate(wdt->clock);
> > +     unsigned long freq = wdt->freq_src;
>
> This does not look good. You are using fixed frequency (from probe).
>

Ok, will avoid caching this value in v2.

> >       unsigned int count;
> >       unsigned int divisor = 1;
> >       unsigned long wtcon;
> > @@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     /* "watchdog_src" clock is optional; if it's not present -- just skip */
> > +     wdt->clock_src = devm_clk_get(dev, "watchdog_src");
> > +     if (!IS_ERR(wdt->clock_src)) {
> > +             ret = clk_prepare_enable(wdt->clock_src);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable source clock\n");
> > +                     ret = PTR_ERR(wdt->clock_src);
> > +                     goto err_clk;
> > +             }
> > +             wdt->freq_src = clk_get_rate(wdt->clock_src);
> > +     } else {
> > +             wdt->freq_src = clk_get_rate(wdt->clock);
> > +     }
> > +
> >       wdt->wdt_device.min_timeout = 1;
> > -     wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
> > +     wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->freq_src);
> >
> >       ret = s3c2410wdt_cpufreq_register(wdt);
> >       if (ret < 0) {
> >               dev_err(dev, "failed to register cpufreq\n");
> > -             goto err_clk;
> > +             goto err_clk_src;
> >       }
> >
> >       watchdog_set_drvdata(&wdt->wdt_device, wdt);
> > @@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >   err_cpufreq:
> >       s3c2410wdt_cpufreq_deregister(wdt);
> >
> > + err_clk_src:
> > +     if (!IS_ERR(wdt->clock_src))
> > +             clk_disable_unprepare(wdt->clock_src);
>
> No. Errors in getting source clock should not be ignored, so you should
> never store here ERR. You could store NULL. If() is anyway not needed in
> both cases.
>
> You can simplify all this and take pclk twice if src clock is missing.
> Or assign src=pclk...
>

Hmm, I don't want to take the same clock twice. It'll increase its
refcount twice, which might be confusing in some cases. I guess I'll
rework it to be like this in v2:
  - add "has_src_clk" bool field to struct wdt
  - if "watchdog_src" is provided: set has_src_clk "true"
  - if "watchdog_src" is not provided: set has_src_clk "false"
(default BSS val) and assign src=pclk
  - only enable/disable src clock when has_src_clk is "true"

That simplifies clock using, fixes stored pointer value, and avoids
taking the clock twice, all at the same time. Hope that way is fine
with you.

> > +
> >   err_clk:
> >       clk_disable_unprepare(wdt->clock);
> >
> > @@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
> >
> >       s3c2410wdt_cpufreq_deregister(wdt);
> >
> > +     if (!IS_ERR(wdt->clock_src))
> > +             clk_disable_unprepare(wdt->clock_src);
> > +
> >       clk_disable_unprepare(wdt->clock);
> >
> >       return 0;
> >
>
>
> Best regards,
> Krzysztof

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

On Fri, 29 Oct 2021 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> > When "tmr_atboot" module param is set, the watchdog is started in
> > driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> > watchdog core driver know it's running. This way wathcdog core can kick
> > the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> > enabled), until user space takes control.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
> >   1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index ca082b1226e3..9af014ff1468 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> >       wdt->wdt_device.parent = dev;
> >
> > +     /*
> > +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> > +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> > +      *
> > +      * If we're not enabling the watchdog, then ensure it is disabled if it
> > +      * has been left running from the bootloader or other source.
> > +      */
> > +     if (tmr_atboot && started == 0) {
> > +             dev_info(dev, "starting watchdog timer\n");
> > +             s3c2410wdt_start(&wdt->wdt_device);
> > +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> > +     } else if (!tmr_atboot) {
> > +             s3c2410wdt_stop(&wdt->wdt_device);
> > +     }
> > +
>
> This doesn't cover the case where the watchdog is already enabled by the BIOS.
> In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
> if the userspace handler is not loaded fast enough. The code should consistently
> set WDOG_HW_RUNNING if the watchdog is running.
>

As I understand, in the case when bootloader started the watchdog, the
driver just stops it. You can see it in the code you replied to.

    } else if (!tmr_atboot) {
            s3c2410wdt_stop(&wdt->wdt_device);

In other words, having "tmr_atboot" module param makes it irrelevant
whether bootloader enabled WDT or no.

> Guenter
>
> >       ret = watchdog_register_device(&wdt->wdt_device);
> >       if (ret)
> >               goto err_cpufreq;
> > @@ -740,17 +755,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       if (ret < 0)
> >               goto err_unregister;
> >
> > -     if (tmr_atboot && started == 0) {
> > -             dev_info(dev, "starting watchdog timer\n");
> > -             s3c2410wdt_start(&wdt->wdt_device);
> > -     } else if (!tmr_atboot) {
> > -             /* if we're not enabling the watchdog, then ensure it is
> > -              * disabled if it has been left running from the bootloader
> > -              * or other source */
> > -
> > -             s3c2410wdt_stop(&wdt->wdt_device);
> > -     }
> > -
> >       platform_set_drvdata(pdev, wdt);
> >
> >       /* print out a statement of readiness */
> >
>

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

On Fri, 29 Oct 2021 at 11:20, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 28/10/2021 20:35, Sam Protsenko wrote:
> > When "tmr_atboot" module param is set, the watchdog is started in
> > driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> > watchdog core driver know it's running. This way wathcdog core can kick
>
> s/wathcdog/watchdog/
>
> > the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> > enabled), until user space takes control.
>
> This does not explain why you move the code around. I guess you miss
> here information that we should start the watchdog before registering
> it? If so please explain why.
>

Will do in v2, thanks.

> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index ca082b1226e3..9af014ff1468 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> >       wdt->wdt_device.parent = dev;
> >
> > +     /*
> > +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> > +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> > +      *
> > +      * If we're not enabling the watchdog, then ensure it is disabled if it
> > +      * has been left running from the bootloader or other source.
> > +      */
> > +     if (tmr_atboot && started == 0) {
> > +             dev_info(dev, "starting watchdog timer\n");
> > +             s3c2410wdt_start(&wdt->wdt_device);
> > +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> > +     } else if (!tmr_atboot) {
> > +             s3c2410wdt_stop(&wdt->wdt_device);
> > +     }
> > +
> >       ret = watchdog_register_device(&wdt->wdt_device);
> >       if (ret)
> >               goto err_cpufreq;
> > @@ -740,17 +755,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       if (ret < 0)
> >               goto err_unregister;
> >
> > -     if (tmr_atboot && started == 0) {
> > -             dev_info(dev, "starting watchdog timer\n");
> > -             s3c2410wdt_start(&wdt->wdt_device);
> > -     } else if (!tmr_atboot) {
> > -             /* if we're not enabling the watchdog, then ensure it is
> > -              * disabled if it has been left running from the bootloader
> > -              * or other source */
> > -
> > -             s3c2410wdt_stop(&wdt->wdt_device);
> > -     }
> > -
> >       platform_set_drvdata(pdev, wdt);
> >
> >       /* print out a statement of readiness */
> >
>
>
> Best regards,
> Krzysztof

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Guenter Roeck]

On 10/30/21 7:29 AM, Sam Protsenko wrote:
> On Fri, 29 Oct 2021 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 10/28/21 11:35 AM, Sam Protsenko wrote:
>>> When "tmr_atboot" module param is set, the watchdog is started in
>>> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
>>> watchdog core driver know it's running. This way wathcdog core can kick
>>> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
>>> enabled), until user space takes control.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>    drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
>>>    1 file changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>> index ca082b1226e3..9af014ff1468 100644
>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>        wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
>>>        wdt->wdt_device.parent = dev;
>>>
>>> +     /*
>>> +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
>>> +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
>>> +      *
>>> +      * If we're not enabling the watchdog, then ensure it is disabled if it
>>> +      * has been left running from the bootloader or other source.
>>> +      */
>>> +     if (tmr_atboot && started == 0) {
>>> +             dev_info(dev, "starting watchdog timer\n");
>>> +             s3c2410wdt_start(&wdt->wdt_device);
>>> +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>>> +     } else if (!tmr_atboot) {
>>> +             s3c2410wdt_stop(&wdt->wdt_device);
>>> +     }
>>> +
>>
>> This doesn't cover the case where the watchdog is already enabled by the BIOS.
>> In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
>> if the userspace handler is not loaded fast enough. The code should consistently
>> set WDOG_HW_RUNNING if the watchdog is running.
>>
> 
> As I understand, in the case when bootloader started the watchdog, the
> driver just stops it. You can see it in the code you replied to.
> 
>      } else if (!tmr_atboot) {
>              s3c2410wdt_stop(&wdt->wdt_device);
> 
> In other words, having "tmr_atboot" module param makes it irrelevant
> whether bootloader enabled WDT or no.
> 

Sure, but I am concerned about "if (tmr_atboot && started)", which doesn't
set WDOG_HW_RUNNING with your current code, and I was looking for something
like

	if (tmr_atboot) {
		if (!started) {
			dev_info(dev, "starting watchdog timer\n");
			s3c2410wdt_start(&wdt->wdt_device);
		}
		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
	} else {
		s3c2410wdt_stop(&wdt->wdt_device);
	}

Guenter

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

On Sat, 30 Oct 2021 at 18:14, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/30/21 7:29 AM, Sam Protsenko wrote:
> > On Fri, 29 Oct 2021 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> >>> When "tmr_atboot" module param is set, the watchdog is started in
> >>> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> >>> watchdog core driver know it's running. This way wathcdog core can kick
> >>> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> >>> enabled), until user space takes control.
> >>>
> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>> ---
> >>>    drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
> >>>    1 file changed, 15 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> >>> index ca082b1226e3..9af014ff1468 100644
> >>> --- a/drivers/watchdog/s3c2410_wdt.c
> >>> +++ b/drivers/watchdog/s3c2410_wdt.c
> >>> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>        wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> >>>        wdt->wdt_device.parent = dev;
> >>>
> >>> +     /*
> >>> +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> >>> +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> >>> +      *
> >>> +      * If we're not enabling the watchdog, then ensure it is disabled if it
> >>> +      * has been left running from the bootloader or other source.
> >>> +      */
> >>> +     if (tmr_atboot && started == 0) {
> >>> +             dev_info(dev, "starting watchdog timer\n");
> >>> +             s3c2410wdt_start(&wdt->wdt_device);
> >>> +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> >>> +     } else if (!tmr_atboot) {
> >>> +             s3c2410wdt_stop(&wdt->wdt_device);
> >>> +     }
> >>> +
> >>
> >> This doesn't cover the case where the watchdog is already enabled by the BIOS.
> >> In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
> >> if the userspace handler is not loaded fast enough. The code should consistently
> >> set WDOG_HW_RUNNING if the watchdog is running.
> >>
> >
> > As I understand, in the case when bootloader started the watchdog, the
> > driver just stops it. You can see it in the code you replied to.
> >
> >      } else if (!tmr_atboot) {
> >              s3c2410wdt_stop(&wdt->wdt_device);
> >
> > In other words, having "tmr_atboot" module param makes it irrelevant
> > whether bootloader enabled WDT or no.
> >
>
> Sure, but I am concerned about "if (tmr_atboot && started)", which doesn't
> set WDOG_HW_RUNNING with your current code, and I was looking for something
> like
>
>         if (tmr_atboot) {
>                 if (!started) {
>                         dev_info(dev, "starting watchdog timer\n");
>                         s3c2410wdt_start(&wdt->wdt_device);
>                 }
>                 set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>         } else {
>                 s3c2410wdt_stop(&wdt->wdt_device);
>         }
>

Wow, I really overlooked that case. Nice catch! Not having '} else {'
section is vicious...

Though if started != 0, it means s3c2410wdt_set_heartbeat() failed to
set wdd->timeout, and without that the watchdog core won't be able to
calculate correctly ping interval in watchdog_next_keepalive(), and
WDOG_HW_RUNNING bit won't do much good, right? So I'll probably just
call s3c2410wdt_stop() in that case, to be on the safe side.

Also this 'started' variable name is misleading, I'll convert it to
"bool timeout_ok" while at it.

> Guenter

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Guenter Roeck]

On 10/30/21 9:59 AM, Sam Protsenko wrote:
> On Sat, 30 Oct 2021 at 18:14, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 10/30/21 7:29 AM, Sam Protsenko wrote:
>>> On Fri, 29 Oct 2021 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 10/28/21 11:35 AM, Sam Protsenko wrote:
>>>>> When "tmr_atboot" module param is set, the watchdog is started in
>>>>> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
>>>>> watchdog core driver know it's running. This way wathcdog core can kick
>>>>> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
>>>>> enabled), until user space takes control.
>>>>>
>>>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>>>> ---
>>>>>     drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
>>>>>     1 file changed, 15 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>>>> index ca082b1226e3..9af014ff1468 100644
>>>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>>>> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>>         wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
>>>>>         wdt->wdt_device.parent = dev;
>>>>>
>>>>> +     /*
>>>>> +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
>>>>> +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
>>>>> +      *
>>>>> +      * If we're not enabling the watchdog, then ensure it is disabled if it
>>>>> +      * has been left running from the bootloader or other source.
>>>>> +      */
>>>>> +     if (tmr_atboot && started == 0) {
>>>>> +             dev_info(dev, "starting watchdog timer\n");
>>>>> +             s3c2410wdt_start(&wdt->wdt_device);
>>>>> +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>>>>> +     } else if (!tmr_atboot) {
>>>>> +             s3c2410wdt_stop(&wdt->wdt_device);
>>>>> +     }
>>>>> +
>>>>
>>>> This doesn't cover the case where the watchdog is already enabled by the BIOS.
>>>> In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
>>>> if the userspace handler is not loaded fast enough. The code should consistently
>>>> set WDOG_HW_RUNNING if the watchdog is running.
>>>>
>>>
>>> As I understand, in the case when bootloader started the watchdog, the
>>> driver just stops it. You can see it in the code you replied to.
>>>
>>>       } else if (!tmr_atboot) {
>>>               s3c2410wdt_stop(&wdt->wdt_device);
>>>
>>> In other words, having "tmr_atboot" module param makes it irrelevant
>>> whether bootloader enabled WDT or no.
>>>
>>
>> Sure, but I am concerned about "if (tmr_atboot && started)", which doesn't
>> set WDOG_HW_RUNNING with your current code, and I was looking for something
>> like
>>
>>          if (tmr_atboot) {
>>                  if (!started) {
>>                          dev_info(dev, "starting watchdog timer\n");
>>                          s3c2410wdt_start(&wdt->wdt_device);
>>                  }
>>                  set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
>>          } else {
>>                  s3c2410wdt_stop(&wdt->wdt_device);
>>          }
>>
> 
> Wow, I really overlooked that case. Nice catch! Not having '} else {'
> section is vicious...
> 
> Though if started != 0, it means s3c2410wdt_set_heartbeat() failed to
> set wdd->timeout, and without that the watchdog core won't be able to
> calculate correctly ping interval in watchdog_next_keepalive(), and
> WDOG_HW_RUNNING bit won't do much good, right? So I'll probably just
> call s3c2410wdt_stop() in that case, to be on the safe side.
> 
> Also this 'started' variable name is misleading, I'll convert it to
> "bool timeout_ok" while at it.
> 

This driver is a mess :-(. "started" true means that the driver doesn't
work as currently written because there is no known valid timeout. In
reality, s3c2410wdt_set_heartbeat() should in that case select a valid
timeout and set it. On top of that, a timeout value out of range should
never be passed to it in the first place. The check for "if (timeout < 1)"
is, in that context, pointless. The range check should happen in
s3c2410wdt_max_timeout(). If that range check is invalid, ie if
s3c2410wdt_set_heartbeat() fails even though the timeout is in the range
of 1 ..s3c2410wdt_max_timeout(), s3c2410wdt_max_timeout() is buggy.

The simplest fix (kludge/hack) might be to fail driver installation if
s3c2410wdt_set_heartbeat() fails.

Guenter

Re: [PATCH 7/7] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

On Sat, 30 Oct 2021 at 20:47, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/30/21 9:59 AM, Sam Protsenko wrote:
> > On Sat, 30 Oct 2021 at 18:14, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 10/30/21 7:29 AM, Sam Protsenko wrote:
> >>> On Fri, 29 Oct 2021 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On 10/28/21 11:35 AM, Sam Protsenko wrote:
> >>>>> When "tmr_atboot" module param is set, the watchdog is started in
> >>>>> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> >>>>> watchdog core driver know it's running. This way wathcdog core can kick
> >>>>> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> >>>>> enabled), until user space takes control.
> >>>>>
> >>>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>>>> ---
> >>>>>     drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
> >>>>>     1 file changed, 15 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> >>>>> index ca082b1226e3..9af014ff1468 100644
> >>>>> --- a/drivers/watchdog/s3c2410_wdt.c
> >>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
> >>>>> @@ -732,6 +732,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>         wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> >>>>>         wdt->wdt_device.parent = dev;
> >>>>>
> >>>>> +     /*
> >>>>> +      * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> >>>>> +      * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> >>>>> +      *
> >>>>> +      * If we're not enabling the watchdog, then ensure it is disabled if it
> >>>>> +      * has been left running from the bootloader or other source.
> >>>>> +      */
> >>>>> +     if (tmr_atboot && started == 0) {
> >>>>> +             dev_info(dev, "starting watchdog timer\n");
> >>>>> +             s3c2410wdt_start(&wdt->wdt_device);
> >>>>> +             set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> >>>>> +     } else if (!tmr_atboot) {
> >>>>> +             s3c2410wdt_stop(&wdt->wdt_device);
> >>>>> +     }
> >>>>> +
> >>>>
> >>>> This doesn't cover the case where the watchdog is already enabled by the BIOS.
> >>>> In that case, WDOG_HW_RUNNING won't be set, and the watchdog will time out
> >>>> if the userspace handler is not loaded fast enough. The code should consistently
> >>>> set WDOG_HW_RUNNING if the watchdog is running.
> >>>>
> >>>
> >>> As I understand, in the case when bootloader started the watchdog, the
> >>> driver just stops it. You can see it in the code you replied to.
> >>>
> >>>       } else if (!tmr_atboot) {
> >>>               s3c2410wdt_stop(&wdt->wdt_device);
> >>>
> >>> In other words, having "tmr_atboot" module param makes it irrelevant
> >>> whether bootloader enabled WDT or no.
> >>>
> >>
> >> Sure, but I am concerned about "if (tmr_atboot && started)", which doesn't
> >> set WDOG_HW_RUNNING with your current code, and I was looking for something
> >> like
> >>
> >>          if (tmr_atboot) {
> >>                  if (!started) {
> >>                          dev_info(dev, "starting watchdog timer\n");
> >>                          s3c2410wdt_start(&wdt->wdt_device);
> >>                  }
> >>                  set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> >>          } else {
> >>                  s3c2410wdt_stop(&wdt->wdt_device);
> >>          }
> >>
> >
> > Wow, I really overlooked that case. Nice catch! Not having '} else {'
> > section is vicious...
> >
> > Though if started != 0, it means s3c2410wdt_set_heartbeat() failed to
> > set wdd->timeout, and without that the watchdog core won't be able to
> > calculate correctly ping interval in watchdog_next_keepalive(), and
> > WDOG_HW_RUNNING bit won't do much good, right? So I'll probably just
> > call s3c2410wdt_stop() in that case, to be on the safe side.
> >
> > Also this 'started' variable name is misleading, I'll convert it to
> > "bool timeout_ok" while at it.
> >
>
> This driver is a mess :-(. "started" true means that the driver doesn't
> work as currently written because there is no known valid timeout. In
> reality, s3c2410wdt_set_heartbeat() should in that case select a valid
> timeout and set it. On top of that, a timeout value out of range should
> never be passed to it in the first place. The check for "if (timeout < 1)"
> is, in that context, pointless. The range check should happen in
> s3c2410wdt_max_timeout(). If that range check is invalid, ie if
> s3c2410wdt_set_heartbeat() fails even though the timeout is in the range
> of 1 ..s3c2410wdt_max_timeout(), s3c2410wdt_max_timeout() is buggy.
>
> The simplest fix (kludge/hack) might be to fail driver installation if
> s3c2410wdt_set_heartbeat() fails.
>

Thanks for suggesting that. I'll send that change along in v2.

> Guenter