[PATCH] watchdog: cadence: Support all available prescaler values

[PATCH] watchdog: cadence: Support all available prescaler values

[Melin Tomas]

Cadence watchdog HW supports prescaler values of
8, 64, 512 and 4096. For low frequency input clocks, the smaller
prescaler values are preferrable to improve the granularity and
extend the timeout range towards the lower end.

Prescaler logic is extended to support timeout values [1s - 4095s],
with prescaler selected based on input clock frequency.
For clock frequencies higher than ~2 MHz, the largest prescaler
value is used.

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

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index c3924356d173..65191183ecd8 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -32,16 +32,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
@@ -348,7 +349,13 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	}
 
 	clock_f = clk_get_rate(wdt->clk);
-	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
+	if (clock_f <= CDNS_WDT_CLK_32KHZ) {
+		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_8) {
+		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_64) {
 		wdt->prescaler = CDNS_WDT_PRESCALE_512;
 		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
 	} else {
-- 
2.17.2

Re: [PATCH] watchdog: cadence: Support all available prescaler values

[Guenter Roeck]

On Tue, Jun 25, 2019 at 02:11:11PM +0000, Melin Tomas wrote:
> Cadence watchdog HW supports prescaler values of
> 8, 64, 512 and 4096. For low frequency input clocks, the smaller
> prescaler values are preferrable to improve the granularity and
> extend the timeout range towards the lower end.
> 
> Prescaler logic is extended to support timeout values [1s - 4095s],
> with prescaler selected based on input clock frequency.
> For clock frequencies higher than ~2 MHz, the largest prescaler
> value is used.
> 

I have two problems with this patch:

"improve the granularity and extend the timeout range towards the lower
end" suggests that the timeout is not always selected in multiples of
one second. Since the set-timeout function is supposed to return the
actually selected timeout, this points to a possible bug.

Also, "extended to support timeout values [1s - 4095s]" is at the very
least misleading since timeouts larger than 516 seconds are not selectable
even after this patch has been applied (see CDNS_WDT_MAX_TIMEOUT and its
use).

I am not opposed to making better use of the prescaler, but it needs
to be documented properly, and if the timeout granularity is larger
than 1 second, the actual timeout needs to be calculated and reflected
in wdd->timeout.

Thanks,
Guenter

> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/watchdog/cadence_wdt.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index c3924356d173..65191183ecd8 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -32,16 +32,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
> @@ -348,7 +349,13 @@ static int cdns_wdt_probe(struct platform_device *pdev)
>  	}
>  
>  	clock_f = clk_get_rate(wdt->clk);
> -	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
> +	if (clock_f <= CDNS_WDT_CLK_32KHZ) {
> +		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_8) {
> +		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_64) {
>  		wdt->prescaler = CDNS_WDT_PRESCALE_512;
>  		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
>  	} else {
> -- 
> 2.17.2
> 

Re: [PATCH] watchdog: cadence: Support all available prescaler values

[Melin Tomas]

On 6/26/19 12:23 AM, Guenter Roeck wrote:

> On Tue, Jun 25, 2019 at 02:11:11PM +0000, Melin Tomas wrote:
>> Cadence watchdog HW supports prescaler values of
>> 8, 64, 512 and 4096. For low frequency input clocks, the smaller
>> prescaler values are preferrable to improve the granularity and
>> extend the timeout range towards the lower end.
>>
>> Prescaler logic is extended to support timeout values [1s - 4095s],
>> with prescaler selected based on input clock frequency.
>> For clock frequencies higher than ~2 MHz, the largest prescaler
>> value is used.
>>
> I have two problems with this patch:
>
> "improve the granularity and extend the timeout range towards the lower
> end" suggests that the timeout is not always selected in multiples of
> one second. Since the set-timeout function is supposed to return the
> actually selected timeout, this points to a possible bug.
>
> Also, "extended to support timeout values [1s - 4095s]" is at the very
> least misleading since timeouts larger than 516 seconds are not selectable
> even after this patch has been applied (see CDNS_WDT_MAX_TIMEOUT and its
> use).
>
> I am not opposed to making better use of the prescaler, but it needs
> to be documented properly, and if the timeout granularity is larger
> than 1 second, the actual timeout needs to be calculated and reflected
> in wdd->timeout.

Thanks for valuable feedback. I'll check and rework this and get back with

an updated version.


thanks,

Tomas