linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values
@ 2019-07-09 20:09 Melin Tomas
  2019-07-09 20:09 ` [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe Melin Tomas
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:09 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-watchdog, Melin Tomas

Cadence watchdog HW supports prescaler values of 8, 64, 512 and 4096,
currently only 512 and 4096 are used by driver.

This series adds support to select prescaler values of 8 and 64 for
lower input clock frequencies.

Changes v3:
- Rebased on v5.2
- Move entire clock init block earlier in probe
- Add check for clock rate not being 0
- Add clock rate in error message

Changes v2:
- Add calculation of actual min/max timeout values
- Validate input clock frequencies to not allow
  frequencies that would result in timeout granularity
  of > 1 second.

Tomas Melin (4):
  watchdog: cadence_wdt: Move clock detection eariler in probe
  watchdog: cadence_wdt: Calculate actual timeout limits
  watchdog: cadence_wdt: Group struct member init statements
  watchdog: cadence_wdt: Support all available prescaler values

 drivers/watchdog/cadence_wdt.c | 65 ++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 27 deletions(-)

-- 
2.17.2


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe
  2019-07-09 20:09 [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
@ 2019-07-09 20:09 ` Melin Tomas
  2019-07-09 20:15   ` Guenter Roeck
  2019-07-09 20:09 ` [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits Melin Tomas
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:09 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-watchdog, Melin Tomas

Timeout calculation needs clock frequency, so init clock and calculate
prescaler value earlier in the probe.

Preparational step for calculating maximum and minimum timeout values
for driver.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/watchdog/cadence_wdt.c | 50 +++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index a22f2d431a35..ddbf602bdc40 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -300,6 +300,31 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	if (!wdt)
 		return -ENOMEM;
 
+	wdt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		dev_err(dev, "input clock not found\n");
+		return PTR_ERR(wdt->clk);
+	}
+
+	ret = clk_prepare_enable(wdt->clk);
+	if (ret) {
+		dev_err(dev, "unable to enable clock\n");
+		return ret;
+	}
+	ret = devm_add_action_or_reset(dev, cdns_clk_disable_unprepare,
+				       wdt->clk);
+	if (ret)
+		return ret;
+
+	clock_f = clk_get_rate(wdt->clk);
+	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
+		wdt->prescaler = CDNS_WDT_PRESCALE_512;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
+	} else {
+		wdt->prescaler = CDNS_WDT_PRESCALE_4096;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_4096;
+	}
+
 	cdns_wdt_device = &wdt->cdns_wdt_device;
 	cdns_wdt_device->info = &cdns_wdt_info;
 	cdns_wdt_device->ops = &cdns_wdt_ops;
@@ -333,31 +358,6 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(cdns_wdt_device);
 	watchdog_set_drvdata(cdns_wdt_device, wdt);
 
-	wdt->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(wdt->clk)) {
-		dev_err(dev, "input clock not found\n");
-		return PTR_ERR(wdt->clk);
-	}
-
-	ret = clk_prepare_enable(wdt->clk);
-	if (ret) {
-		dev_err(dev, "unable to enable clock\n");
-		return ret;
-	}
-	ret = devm_add_action_or_reset(dev, cdns_clk_disable_unprepare,
-				       wdt->clk);
-	if (ret)
-		return ret;
-
-	clock_f = clk_get_rate(wdt->clk);
-	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
-		wdt->prescaler = CDNS_WDT_PRESCALE_512;
-		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
-	} else {
-		wdt->prescaler = CDNS_WDT_PRESCALE_4096;
-		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_4096;
-	}
-
 	spin_lock_init(&wdt->io_lock);
 
 	watchdog_stop_on_reboot(cdns_wdt_device);
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits
  2019-07-09 20:09 [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
  2019-07-09 20:09 ` [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe Melin Tomas
@ 2019-07-09 20:09 ` Melin Tomas
  2019-07-09 20:14   ` Guenter Roeck
  2019-07-09 20:09 ` [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
  2019-07-09 20:09 ` [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements Melin Tomas
  3 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:09 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-watchdog, Melin Tomas

Maximum and minimum timeout values depend on the actual input clock
frequency and prescaler selection.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/watchdog/cadence_wdt.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index ddbf602bdc40..a0d7666e7d20 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -18,9 +18,6 @@
 #include <linux/watchdog.h>
 
 #define CDNS_WDT_DEFAULT_TIMEOUT	10
-/* Supports 1 - 516 sec */
-#define CDNS_WDT_MIN_TIMEOUT	1
-#define CDNS_WDT_MAX_TIMEOUT	516
 
 /* Restart key */
 #define CDNS_WDT_RESTART_KEY 0x00001999
@@ -28,6 +25,10 @@
 /* Counter register access key */
 #define CDNS_WDT_REGISTER_ACCESS_KEY 0x00920000
 
+/* Counter control register, counter restart values */
+#define CDNS_WDT_CCR_CRV_MIN 0xFFF
+#define CDNS_WDT_CCR_CRV_MAX 0xFFFFFF
+
 /* Counter value divisor */
 #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
 
@@ -317,7 +318,10 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	clock_f = clk_get_rate(wdt->clk);
-	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
+	if (clock_f == 0) {
+		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
+		return -EINVAL;
+	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
 		wdt->prescaler = CDNS_WDT_PRESCALE_512;
 		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
 	} else {
@@ -329,8 +333,10 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	cdns_wdt_device->info = &cdns_wdt_info;
 	cdns_wdt_device->ops = &cdns_wdt_ops;
 	cdns_wdt_device->timeout = CDNS_WDT_DEFAULT_TIMEOUT;
-	cdns_wdt_device->min_timeout = CDNS_WDT_MIN_TIMEOUT;
-	cdns_wdt_device->max_timeout = CDNS_WDT_MAX_TIMEOUT;
+	cdns_wdt_device->min_timeout =
+		CDNS_WDT_CCR_CRV_MIN * wdt->prescaler / clock_f;
+	cdns_wdt_device->max_timeout =
+		CDNS_WDT_CCR_CRV_MAX * wdt->prescaler / clock_f;
 
 	wdt->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(wdt->regs))
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements
  2019-07-09 20:09 [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
                   ` (2 preceding siblings ...)
  2019-07-09 20:09 ` [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
@ 2019-07-09 20:09 ` Melin Tomas
  2019-07-09 20:16   ` Guenter Roeck
  3 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:09 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-watchdog, Melin Tomas

Move init statement and remove stray comment.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/watchdog/cadence_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index a0d7666e7d20..0bdb275d904a 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -330,6 +330,7 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	}
 
 	cdns_wdt_device = &wdt->cdns_wdt_device;
+	cdns_wdt_device->parent = dev;
 	cdns_wdt_device->info = &cdns_wdt_info;
 	cdns_wdt_device->ops = &cdns_wdt_ops;
 	cdns_wdt_device->timeout = CDNS_WDT_DEFAULT_TIMEOUT;
@@ -356,9 +357,6 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* Initialize the members of cdns_wdt structure */
-	cdns_wdt_device->parent = dev;
-
 	watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
 	watchdog_stop_on_reboot(cdns_wdt_device);
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-09 20:09 [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
  2019-07-09 20:09 ` [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe Melin Tomas
  2019-07-09 20:09 ` [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits Melin Tomas
@ 2019-07-09 20:09 ` Melin Tomas
  2019-07-09 20:21   ` Guenter Roeck
  2019-07-09 20:09 ` [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements Melin Tomas
  3 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:09 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-watchdog, Melin Tomas

Cadence watchdog HW supports prescaler values of
8, 64, 512 and 4096.

Add support to select prescaler values of 8 and 64 for lower
input clock frequencies.

Prescaler value is selected to keep timeout resolution of 1 second.
For clock frequencies below 32kHz, 1 second resolution does
no longer hold, thereby returning an error.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index 0bdb275d904a..037faf557f9d 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -33,16 +33,17 @@
 #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
 
 /* Clock prescaler value and selection */
+#define CDNS_WDT_PRESCALE_8	8
 #define CDNS_WDT_PRESCALE_64	64
 #define CDNS_WDT_PRESCALE_512	512
 #define CDNS_WDT_PRESCALE_4096	4096
+#define CDNS_WDT_PRESCALE_SELECT_8	0
 #define CDNS_WDT_PRESCALE_SELECT_64	1
 #define CDNS_WDT_PRESCALE_SELECT_512	2
 #define CDNS_WDT_PRESCALE_SELECT_4096	3
 
-/* Input clock frequency */
-#define CDNS_WDT_CLK_10MHZ	10000000
-#define CDNS_WDT_CLK_75MHZ	75000000
+/* Base input clock frequency */
+#define CDNS_WDT_CLK_32KHZ 32768
 
 /* Counter maximum value */
 #define CDNS_WDT_COUNTER_MAX 0xFFF
@@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	clock_f = clk_get_rate(wdt->clk);
-	if (clock_f == 0) {
-		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
+	if (clock_f < CDNS_WDT_CLK_32KHZ) {
+		dev_err(dev,
+			"cannot find suitable clock prescaler, (f=%lu)\n",
+			clock_f);
 		return -EINVAL;
-	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
+	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
+		wdt->prescaler = CDNS_WDT_PRESCALE_8;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
+	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
+		wdt->prescaler = CDNS_WDT_PRESCALE_64;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
+	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
 		wdt->prescaler = CDNS_WDT_PRESCALE_512;
 		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
 	} else {
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits
  2019-07-09 20:09 ` [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits Melin Tomas
@ 2019-07-09 20:14   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-09 20:14 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Tue, Jul 09, 2019 at 08:09:05PM +0000, Melin Tomas wrote:
> Maximum and minimum timeout values depend on the actual input clock
> frequency and prescaler selection.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/watchdog/cadence_wdt.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index ddbf602bdc40..a0d7666e7d20 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -18,9 +18,6 @@
>  #include <linux/watchdog.h>
>  
>  #define CDNS_WDT_DEFAULT_TIMEOUT	10
> -/* Supports 1 - 516 sec */
> -#define CDNS_WDT_MIN_TIMEOUT	1
> -#define CDNS_WDT_MAX_TIMEOUT	516
>  
>  /* Restart key */
>  #define CDNS_WDT_RESTART_KEY 0x00001999
> @@ -28,6 +25,10 @@
>  /* Counter register access key */
>  #define CDNS_WDT_REGISTER_ACCESS_KEY 0x00920000
>  
> +/* Counter control register, counter restart values */
> +#define CDNS_WDT_CCR_CRV_MIN 0xFFF
> +#define CDNS_WDT_CCR_CRV_MAX 0xFFFFFF
                               ^ Please use tabs here

> +
>  /* Counter value divisor */
>  #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
>  
> @@ -317,7 +318,10 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	clock_f = clk_get_rate(wdt->clk);
> -	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> +	if (clock_f == 0) {
> +		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);

We know that clock_f is 0 here. No need to use %lu.

> +		return -EINVAL;

else after return is unnecessary (and static checkers will rightfully complain).

> +	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
>  		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>  		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>  	} else {
> @@ -329,8 +333,10 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  	cdns_wdt_device->info = &cdns_wdt_info;
>  	cdns_wdt_device->ops = &cdns_wdt_ops;
>  	cdns_wdt_device->timeout = CDNS_WDT_DEFAULT_TIMEOUT;
> -	cdns_wdt_device->min_timeout = CDNS_WDT_MIN_TIMEOUT;
> -	cdns_wdt_device->max_timeout = CDNS_WDT_MAX_TIMEOUT;
> +	cdns_wdt_device->min_timeout =
> +		CDNS_WDT_CCR_CRV_MIN * wdt->prescaler / clock_f;
> +	cdns_wdt_device->max_timeout =
> +		CDNS_WDT_CCR_CRV_MAX * wdt->prescaler / clock_f;
>  
>  	wdt->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(wdt->regs))
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe
  2019-07-09 20:09 ` [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe Melin Tomas
@ 2019-07-09 20:15   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-09 20:15 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Tue, Jul 09, 2019 at 08:09:03PM +0000, Melin Tomas wrote:
> Timeout calculation needs clock frequency, so init clock and calculate
> prescaler value earlier in the probe.
> 
> Preparational step for calculating maximum and minimum timeout values
> for driver.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>

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

> ---
>  drivers/watchdog/cadence_wdt.c | 50 +++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index a22f2d431a35..ddbf602bdc40 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -300,6 +300,31 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  	if (!wdt)
>  		return -ENOMEM;
>  
> +	wdt->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(wdt->clk)) {
> +		dev_err(dev, "input clock not found\n");
> +		return PTR_ERR(wdt->clk);
> +	}
> +
> +	ret = clk_prepare_enable(wdt->clk);
> +	if (ret) {
> +		dev_err(dev, "unable to enable clock\n");
> +		return ret;
> +	}
> +	ret = devm_add_action_or_reset(dev, cdns_clk_disable_unprepare,
> +				       wdt->clk);
> +	if (ret)
> +		return ret;
> +
> +	clock_f = clk_get_rate(wdt->clk);
> +	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> +		wdt->prescaler = CDNS_WDT_PRESCALE_512;
> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
> +	} else {
> +		wdt->prescaler = CDNS_WDT_PRESCALE_4096;
> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_4096;
> +	}
> +
>  	cdns_wdt_device = &wdt->cdns_wdt_device;
>  	cdns_wdt_device->info = &cdns_wdt_info;
>  	cdns_wdt_device->ops = &cdns_wdt_ops;
> @@ -333,31 +358,6 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(cdns_wdt_device);
>  	watchdog_set_drvdata(cdns_wdt_device, wdt);
>  
> -	wdt->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(wdt->clk)) {
> -		dev_err(dev, "input clock not found\n");
> -		return PTR_ERR(wdt->clk);
> -	}
> -
> -	ret = clk_prepare_enable(wdt->clk);
> -	if (ret) {
> -		dev_err(dev, "unable to enable clock\n");
> -		return ret;
> -	}
> -	ret = devm_add_action_or_reset(dev, cdns_clk_disable_unprepare,
> -				       wdt->clk);
> -	if (ret)
> -		return ret;
> -
> -	clock_f = clk_get_rate(wdt->clk);
> -	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> -		wdt->prescaler = CDNS_WDT_PRESCALE_512;
> -		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
> -	} else {
> -		wdt->prescaler = CDNS_WDT_PRESCALE_4096;
> -		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_4096;
> -	}
> -
>  	spin_lock_init(&wdt->io_lock);
>  
>  	watchdog_stop_on_reboot(cdns_wdt_device);
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements
  2019-07-09 20:09 ` [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements Melin Tomas
@ 2019-07-09 20:16   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-09 20:16 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Tue, Jul 09, 2019 at 08:09:06PM +0000, Melin Tomas wrote:
> Move init statement and remove stray comment.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>

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

> ---
>  drivers/watchdog/cadence_wdt.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index a0d7666e7d20..0bdb275d904a 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -330,6 +330,7 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  	}
>  
>  	cdns_wdt_device = &wdt->cdns_wdt_device;
> +	cdns_wdt_device->parent = dev;
>  	cdns_wdt_device->info = &cdns_wdt_info;
>  	cdns_wdt_device->ops = &cdns_wdt_ops;
>  	cdns_wdt_device->timeout = CDNS_WDT_DEFAULT_TIMEOUT;
> @@ -356,9 +357,6 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/* Initialize the members of cdns_wdt structure */
> -	cdns_wdt_device->parent = dev;
> -
>  	watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
>  	watchdog_set_nowayout(cdns_wdt_device, nowayout);
>  	watchdog_stop_on_reboot(cdns_wdt_device);
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-09 20:09 ` [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
@ 2019-07-09 20:21   ` Guenter Roeck
  2019-07-09 20:49     ` Melin Tomas
  0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-07-09 20:21 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Tue, Jul 09, 2019 at 08:09:06PM +0000, Melin Tomas wrote:
> Cadence watchdog HW supports prescaler values of
> 8, 64, 512 and 4096.
> 
> Add support to select prescaler values of 8 and 64 for lower
> input clock frequencies.
> 
> Prescaler value is selected to keep timeout resolution of 1 second.
> For clock frequencies below 32kHz, 1 second resolution does
> no longer hold, thereby returning an error.
> 

I think I am missing something. Why was this valid/supported up to now,
and if it was, why is it no longer possible to support it ?

I am also a bit confused about the logic. With a slower clock, I would
expect that the timeouts are getting larger, not smaller. Can you explain ?

> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index 0bdb275d904a..037faf557f9d 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -33,16 +33,17 @@
>  #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
>  
>  /* Clock prescaler value and selection */
> +#define CDNS_WDT_PRESCALE_8	8
>  #define CDNS_WDT_PRESCALE_64	64
>  #define CDNS_WDT_PRESCALE_512	512
>  #define CDNS_WDT_PRESCALE_4096	4096
> +#define CDNS_WDT_PRESCALE_SELECT_8	0
>  #define CDNS_WDT_PRESCALE_SELECT_64	1
>  #define CDNS_WDT_PRESCALE_SELECT_512	2
>  #define CDNS_WDT_PRESCALE_SELECT_4096	3
>  
> -/* Input clock frequency */
> -#define CDNS_WDT_CLK_10MHZ	10000000
> -#define CDNS_WDT_CLK_75MHZ	75000000
> +/* Base input clock frequency */
> +#define CDNS_WDT_CLK_32KHZ 32768
                             ^ Please use a tab here

>  
>  /* Counter maximum value */
>  #define CDNS_WDT_COUNTER_MAX 0xFFF
> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	clock_f = clk_get_rate(wdt->clk);
> -	if (clock_f == 0) {
> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
> +		dev_err(dev,
> +			"cannot find suitable clock prescaler, (f=%lu)\n",
> +			clock_f);
>  		return -EINVAL;
> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
>  		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>  		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>  	} else {
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-09 20:21   ` Guenter Roeck
@ 2019-07-09 20:49     ` Melin Tomas
  2019-07-09 21:07       ` Guenter Roeck
  0 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-09 20:49 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, linux-watchdog

On 7/9/19 11:21 PM, Guenter Roeck wrote:

> On Tue, Jul 09, 2019 at 08:09:06PM +0000, Melin Tomas wrote:
>> Cadence watchdog HW supports prescaler values of
>> 8, 64, 512 and 4096.
>>
>> Add support to select prescaler values of 8 and 64 for lower
>> input clock frequencies.
>>
>> Prescaler value is selected to keep timeout resolution of 1 second.
>> For clock frequencies below 32kHz, 1 second resolution does
>> no longer hold, thereby returning an error.
>>
> I think I am missing something. Why was this valid/supported up to now,
> and if it was, why is it no longer possible to support it ?

This driver hasn't really supported smaller input clock frequencies. The 
watchdog

can be driven from an internal clock with rather high frequency, which

I think is the default setting. So typically, one might not even use the 
smaller prescalers.

>
> I am also a bit confused about the logic. With a slower clock, I would
> expect that the timeouts are getting larger, not smaller. Can you explain ?

Yes, that is correct. So with a 32kHz clock using smallest available 
prescaler,

we get 1 second resolution (and 1 second as smallest timeout).

With an even slower clock than that, we would end up with granularity

and smallest value larger than 1 second.


Thanks,

Tomas

>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>   drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
>> index 0bdb275d904a..037faf557f9d 100644
>> --- a/drivers/watchdog/cadence_wdt.c
>> +++ b/drivers/watchdog/cadence_wdt.c
>> @@ -33,16 +33,17 @@
>>   #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
>>   
>>   /* Clock prescaler value and selection */
>> +#define CDNS_WDT_PRESCALE_8	8
>>   #define CDNS_WDT_PRESCALE_64	64
>>   #define CDNS_WDT_PRESCALE_512	512
>>   #define CDNS_WDT_PRESCALE_4096	4096
>> +#define CDNS_WDT_PRESCALE_SELECT_8	0
>>   #define CDNS_WDT_PRESCALE_SELECT_64	1
>>   #define CDNS_WDT_PRESCALE_SELECT_512	2
>>   #define CDNS_WDT_PRESCALE_SELECT_4096	3
>>   
>> -/* Input clock frequency */
>> -#define CDNS_WDT_CLK_10MHZ	10000000
>> -#define CDNS_WDT_CLK_75MHZ	75000000
>> +/* Base input clock frequency */
>> +#define CDNS_WDT_CLK_32KHZ 32768
>                               ^ Please use a tab here
>
>>   
>>   /* Counter maximum value */
>>   #define CDNS_WDT_COUNTER_MAX 0xFFF
>> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>>   		return ret;
>>   
>>   	clock_f = clk_get_rate(wdt->clk);
>> -	if (clock_f == 0) {
>> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
>> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
>> +		dev_err(dev,
>> +			"cannot find suitable clock prescaler, (f=%lu)\n",
>> +			clock_f);
>>   		return -EINVAL;
>> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
>> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
>> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
>>   		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>>   		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>>   	} else {
>> -- 
>> 2.17.2
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-09 20:49     ` Melin Tomas
@ 2019-07-09 21:07       ` Guenter Roeck
  2019-07-10 19:20         ` Melin Tomas
  0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-07-09 21:07 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Tue, Jul 09, 2019 at 08:49:20PM +0000, Melin Tomas wrote:
> On 7/9/19 11:21 PM, Guenter Roeck wrote:
> 
> > On Tue, Jul 09, 2019 at 08:09:06PM +0000, Melin Tomas wrote:
> >> Cadence watchdog HW supports prescaler values of
> >> 8, 64, 512 and 4096.
> >>
> >> Add support to select prescaler values of 8 and 64 for lower
> >> input clock frequencies.
> >>
> >> Prescaler value is selected to keep timeout resolution of 1 second.
> >> For clock frequencies below 32kHz, 1 second resolution does
> >> no longer hold, thereby returning an error.
> >>
> > I think I am missing something. Why was this valid/supported up to now,
> > and if it was, why is it no longer possible to support it ?
> 
> This driver hasn't really supported smaller input clock frequencies. The 
> watchdog
> 
> can be driven from an internal clock with rather high frequency, which
> 
> I think is the default setting. So typically, one might not even use the 
> smaller prescalers.
> 
> >
> > I am also a bit confused about the logic. With a slower clock, I would
> > expect that the timeouts are getting larger, not smaller. Can you explain ?
> 
> Yes, that is correct. So with a 32kHz clock using smallest available 
> prescaler,
> 
> we get 1 second resolution (and 1 second as smallest timeout).
> 
> With an even slower clock than that, we would end up with granularity
> 
> and smallest value larger than 1 second.
> 

Ah, we are talking about the _smallest_ timeout and about resolution.
But that is no reason to declare the clock invalid. Just set the minimum
to the actual minimum.  There is no reason to reject slow clocks entirely,
even if the granularity is in the multi-second range. The only caveat,
if granularity is more than one second, is that the set_timeout function
must select and report the actual timeout.

Thanks,
Guenter

> 
> Thanks,
> 
> Tomas
> 
> >
> >> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> >> ---
> >>   drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
> >>   1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> >> index 0bdb275d904a..037faf557f9d 100644
> >> --- a/drivers/watchdog/cadence_wdt.c
> >> +++ b/drivers/watchdog/cadence_wdt.c
> >> @@ -33,16 +33,17 @@
> >>   #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
> >>   
> >>   /* Clock prescaler value and selection */
> >> +#define CDNS_WDT_PRESCALE_8	8
> >>   #define CDNS_WDT_PRESCALE_64	64
> >>   #define CDNS_WDT_PRESCALE_512	512
> >>   #define CDNS_WDT_PRESCALE_4096	4096
> >> +#define CDNS_WDT_PRESCALE_SELECT_8	0
> >>   #define CDNS_WDT_PRESCALE_SELECT_64	1
> >>   #define CDNS_WDT_PRESCALE_SELECT_512	2
> >>   #define CDNS_WDT_PRESCALE_SELECT_4096	3
> >>   
> >> -/* Input clock frequency */
> >> -#define CDNS_WDT_CLK_10MHZ	10000000
> >> -#define CDNS_WDT_CLK_75MHZ	75000000
> >> +/* Base input clock frequency */
> >> +#define CDNS_WDT_CLK_32KHZ 32768
> >                               ^ Please use a tab here
> >
> >>   
> >>   /* Counter maximum value */
> >>   #define CDNS_WDT_COUNTER_MAX 0xFFF
> >> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
> >>   		return ret;
> >>   
> >>   	clock_f = clk_get_rate(wdt->clk);
> >> -	if (clock_f == 0) {
> >> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
> >> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
> >> +		dev_err(dev,
> >> +			"cannot find suitable clock prescaler, (f=%lu)\n",
> >> +			clock_f);
> >>   		return -EINVAL;
> >> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> >> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
> >> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
> >> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
> >> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
> >> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
> >> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
> >> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
> >>   		wdt->prescaler = CDNS_WDT_PRESCALE_512;
> >>   		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
> >>   	} else {
> >> -- 
> >> 2.17.2
> >>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-09 21:07       ` Guenter Roeck
@ 2019-07-10 19:20         ` Melin Tomas
  2019-07-10 20:39           ` Guenter Roeck
  0 siblings, 1 reply; 14+ messages in thread
From: Melin Tomas @ 2019-07-10 19:20 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, linux-watchdog


On 7/10/19 12:07 AM, Guenter Roeck wrote:
> Ah, we are talking about the _smallest_ timeout and about resolution.
> But that is no reason to declare the clock invalid. Just set the minimum
> to the actual minimum.  There is no reason to reject slow clocks entirely,
> even if the granularity is in the multi-second range. The only caveat,
> if granularity is more than one second, is that the set_timeout function
> must select and report the actual timeout.

I did consider supporting slower clocks but thought that the required

additional logic was perhaps not worth it. So instead just declared

those clock frequencies invalid.

However, if that logic is required, sure I can try to implement it.

It might take some weeks before I have time to look at it properly.


Thanks,

Tomas

>
> Thanks,
> Guenter
>
>> Thanks,
>>
>> Tomas
>>
>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>> ---
>>>>    drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
>>>>    1 file changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
>>>> index 0bdb275d904a..037faf557f9d 100644
>>>> --- a/drivers/watchdog/cadence_wdt.c
>>>> +++ b/drivers/watchdog/cadence_wdt.c
>>>> @@ -33,16 +33,17 @@
>>>>    #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
>>>>    
>>>>    /* Clock prescaler value and selection */
>>>> +#define CDNS_WDT_PRESCALE_8	8
>>>>    #define CDNS_WDT_PRESCALE_64	64
>>>>    #define CDNS_WDT_PRESCALE_512	512
>>>>    #define CDNS_WDT_PRESCALE_4096	4096
>>>> +#define CDNS_WDT_PRESCALE_SELECT_8	0
>>>>    #define CDNS_WDT_PRESCALE_SELECT_64	1
>>>>    #define CDNS_WDT_PRESCALE_SELECT_512	2
>>>>    #define CDNS_WDT_PRESCALE_SELECT_4096	3
>>>>    
>>>> -/* Input clock frequency */
>>>> -#define CDNS_WDT_CLK_10MHZ	10000000
>>>> -#define CDNS_WDT_CLK_75MHZ	75000000
>>>> +/* Base input clock frequency */
>>>> +#define CDNS_WDT_CLK_32KHZ 32768
>>>                                ^ Please use a tab here
>>>
>>>>    
>>>>    /* Counter maximum value */
>>>>    #define CDNS_WDT_COUNTER_MAX 0xFFF
>>>> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>>>>    		return ret;
>>>>    
>>>>    	clock_f = clk_get_rate(wdt->clk);
>>>> -	if (clock_f == 0) {
>>>> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
>>>> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
>>>> +		dev_err(dev,
>>>> +			"cannot find suitable clock prescaler, (f=%lu)\n",
>>>> +			clock_f);
>>>>    		return -EINVAL;
>>>> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
>>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
>>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
>>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
>>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
>>>>    		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>>>>    		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>>>>    	} else {
>>>> -- 
>>>> 2.17.2
>>>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-10 19:20         ` Melin Tomas
@ 2019-07-10 20:39           ` Guenter Roeck
  2019-07-11 20:10             ` Melin Tomas
  0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-07-10 20:39 UTC (permalink / raw)
  To: Melin Tomas; +Cc: wim, linux-watchdog

On Wed, Jul 10, 2019 at 07:20:59PM +0000, Melin Tomas wrote:
> 
> On 7/10/19 12:07 AM, Guenter Roeck wrote:
> > Ah, we are talking about the _smallest_ timeout and about resolution.
> > But that is no reason to declare the clock invalid. Just set the minimum
> > to the actual minimum.  There is no reason to reject slow clocks entirely,
> > even if the granularity is in the multi-second range. The only caveat,
> > if granularity is more than one second, is that the set_timeout function
> > must select and report the actual timeout.
> 
> I did consider supporting slower clocks but thought that the required
> 
> additional logic was perhaps not worth it. So instead just declared
> 
> those clock frequencies invalid.
> 

Hmm ... not sure I understand. What makes it so difficult ?

Guenter

> However, if that logic is required, sure I can try to implement it.
> 
> It might take some weeks before I have time to look at it properly.
> 
> 
> Thanks,
> 
> Tomas
> 
> >
> > Thanks,
> > Guenter
> >
> >> Thanks,
> >>
> >> Tomas
> >>
> >>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> >>>> ---
> >>>>    drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
> >>>>    1 file changed, 15 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> >>>> index 0bdb275d904a..037faf557f9d 100644
> >>>> --- a/drivers/watchdog/cadence_wdt.c
> >>>> +++ b/drivers/watchdog/cadence_wdt.c
> >>>> @@ -33,16 +33,17 @@
> >>>>    #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
> >>>>    
> >>>>    /* Clock prescaler value and selection */
> >>>> +#define CDNS_WDT_PRESCALE_8	8
> >>>>    #define CDNS_WDT_PRESCALE_64	64
> >>>>    #define CDNS_WDT_PRESCALE_512	512
> >>>>    #define CDNS_WDT_PRESCALE_4096	4096
> >>>> +#define CDNS_WDT_PRESCALE_SELECT_8	0
> >>>>    #define CDNS_WDT_PRESCALE_SELECT_64	1
> >>>>    #define CDNS_WDT_PRESCALE_SELECT_512	2
> >>>>    #define CDNS_WDT_PRESCALE_SELECT_4096	3
> >>>>    
> >>>> -/* Input clock frequency */
> >>>> -#define CDNS_WDT_CLK_10MHZ	10000000
> >>>> -#define CDNS_WDT_CLK_75MHZ	75000000
> >>>> +/* Base input clock frequency */
> >>>> +#define CDNS_WDT_CLK_32KHZ 32768
> >>>                                ^ Please use a tab here
> >>>
> >>>>    
> >>>>    /* Counter maximum value */
> >>>>    #define CDNS_WDT_COUNTER_MAX 0xFFF
> >>>> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
> >>>>    		return ret;
> >>>>    
> >>>>    	clock_f = clk_get_rate(wdt->clk);
> >>>> -	if (clock_f == 0) {
> >>>> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
> >>>> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
> >>>> +		dev_err(dev,
> >>>> +			"cannot find suitable clock prescaler, (f=%lu)\n",
> >>>> +			clock_f);
> >>>>    		return -EINVAL;
> >>>> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> >>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
> >>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
> >>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
> >>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
> >>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
> >>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
> >>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
> >>>>    		wdt->prescaler = CDNS_WDT_PRESCALE_512;
> >>>>    		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
> >>>>    	} else {
> >>>> -- 
> >>>> 2.17.2
> >>>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values
  2019-07-10 20:39           ` Guenter Roeck
@ 2019-07-11 20:10             ` Melin Tomas
  0 siblings, 0 replies; 14+ messages in thread
From: Melin Tomas @ 2019-07-11 20:10 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, linux-watchdog

On 7/10/19 11:39 PM, Guenter Roeck wrote:

> On Wed, Jul 10, 2019 at 07:20:59PM +0000, Melin Tomas wrote:
>> On 7/10/19 12:07 AM, Guenter Roeck wrote:
>>> Ah, we are talking about the _smallest_ timeout and about resolution.
>>> But that is no reason to declare the clock invalid. Just set the minimum
>>> to the actual minimum.  There is no reason to reject slow clocks entirely,
>>> even if the granularity is in the multi-second range. The only caveat,
>>> if granularity is more than one second, is that the set_timeout function
>>> must select and report the actual timeout.
>> I did consider supporting slower clocks but thought that the required
>>
>> additional logic was perhaps not worth it. So instead just declared
>>
>> those clock frequencies invalid.
>>
> Hmm ... not sure I understand. What makes it so difficult ?

Not necessarily difficult, just additional logic for

the set_timeout caveat.


Thanks,

Tomas



>
> Guenter
>
>> However, if that logic is required, sure I can try to implement it.
>>
>> It might take some weeks before I have time to look at it properly.
>>
>>
>> Thanks,
>>
>> Tomas
>>
>>> Thanks,
>>> Guenter
>>>
>>>> Thanks,
>>>>
>>>> Tomas
>>>>
>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>> ---
>>>>>>     drivers/watchdog/cadence_wdt.c | 21 +++++++++++++++------
>>>>>>     1 file changed, 15 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
>>>>>> index 0bdb275d904a..037faf557f9d 100644
>>>>>> --- a/drivers/watchdog/cadence_wdt.c
>>>>>> +++ b/drivers/watchdog/cadence_wdt.c
>>>>>> @@ -33,16 +33,17 @@
>>>>>>     #define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
>>>>>>     
>>>>>>     /* Clock prescaler value and selection */
>>>>>> +#define CDNS_WDT_PRESCALE_8	8
>>>>>>     #define CDNS_WDT_PRESCALE_64	64
>>>>>>     #define CDNS_WDT_PRESCALE_512	512
>>>>>>     #define CDNS_WDT_PRESCALE_4096	4096
>>>>>> +#define CDNS_WDT_PRESCALE_SELECT_8	0
>>>>>>     #define CDNS_WDT_PRESCALE_SELECT_64	1
>>>>>>     #define CDNS_WDT_PRESCALE_SELECT_512	2
>>>>>>     #define CDNS_WDT_PRESCALE_SELECT_4096	3
>>>>>>     
>>>>>> -/* Input clock frequency */
>>>>>> -#define CDNS_WDT_CLK_10MHZ	10000000
>>>>>> -#define CDNS_WDT_CLK_75MHZ	75000000
>>>>>> +/* Base input clock frequency */
>>>>>> +#define CDNS_WDT_CLK_32KHZ 32768
>>>>>                                 ^ Please use a tab here
>>>>>
>>>>>>     
>>>>>>     /* Counter maximum value */
>>>>>>     #define CDNS_WDT_COUNTER_MAX 0xFFF
>>>>>> @@ -318,10 +319,18 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>>>>>>     		return ret;
>>>>>>     
>>>>>>     	clock_f = clk_get_rate(wdt->clk);
>>>>>> -	if (clock_f == 0) {
>>>>>> -		dev_err(dev, "invalid clock frequency, (f=%lu)\n", clock_f);
>>>>>> +	if (clock_f < CDNS_WDT_CLK_32KHZ) {
>>>>>> +		dev_err(dev,
>>>>>> +			"cannot find suitable clock prescaler, (f=%lu)\n",
>>>>>> +			clock_f);
>>>>>>     		return -EINVAL;
>>>>>> -	} else if (clock_f <= CDNS_WDT_CLK_75MHZ) {
>>>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_8) {
>>>>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_8;
>>>>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_8;
>>>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_64) {
>>>>>> +		wdt->prescaler = CDNS_WDT_PRESCALE_64;
>>>>>> +		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_64;
>>>>>> +	} else if (clock_f <= CDNS_WDT_CLK_32KHZ * CDNS_WDT_PRESCALE_512) {
>>>>>>     		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>>>>>>     		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>>>>>>     	} else {
>>>>>> -- 
>>>>>> 2.17.2
>>>>>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-07-11 20:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-09 20:09 [PATCH v3 0/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
2019-07-09 20:09 ` [PATCH v3 1/4] watchdog: cadence_wdt: Move clock detection earlier in probe Melin Tomas
2019-07-09 20:15   ` Guenter Roeck
2019-07-09 20:09 ` [PATCH v3 2/4] watchdog: cadence_wdt: Calculate actual timeout limits Melin Tomas
2019-07-09 20:14   ` Guenter Roeck
2019-07-09 20:09 ` [PATCH v3 4/4] watchdog: cadence_wdt: Support all available prescaler values Melin Tomas
2019-07-09 20:21   ` Guenter Roeck
2019-07-09 20:49     ` Melin Tomas
2019-07-09 21:07       ` Guenter Roeck
2019-07-10 19:20         ` Melin Tomas
2019-07-10 20:39           ` Guenter Roeck
2019-07-11 20:10             ` Melin Tomas
2019-07-09 20:09 ` [PATCH v3 3/4] watchdog: cadence_wdt: Group struct member init statements Melin Tomas
2019-07-09 20:16   ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).