| Message ID | 20190625141048.99479-1-tomas.melin@vaisala.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: cadence: Support all available prescaler values | expand |
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 >
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
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 {
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(-)