Messages in this thread | | | Subject | Re: [PATCHv2 3/5] watchdog: rti-wdt: add support for window size configuration | From | Tero Kristo <> | Date | Mon, 13 Jul 2020 15:51:15 +0300 |
| |
On 05/07/2020 17:49, Guenter Roeck wrote: > On 7/3/20 5:04 AM, Tero Kristo wrote: >> RTI watchdog can support different open window sizes. Add support for >> these and add a new module parameter to configure it. The default open >> window size for the driver still remains at 50%. >> >> Also, modify the margin calculation logic a bit for 32k source clock, >> instead of adding a margin to every window config, assume the 32k source >> clock is running slower.
I'll drop this patch mostly in next rev to get rid of the dynamic config for window size and always use 50%. I will just read the window size in case someone has started the watchdog beforehand.
>> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++------ >> 1 file changed, 95 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >> index d456dd72d99a..110bfc8d0bb3 100644 >> --- a/drivers/watchdog/rti_wdt.c >> +++ b/drivers/watchdog/rti_wdt.c >> @@ -19,7 +19,8 @@ >> #include <linux/types.h> >> #include <linux/watchdog.h> >> >> -#define DEFAULT_HEARTBEAT 60 >> +#define DEFAULT_HEARTBEAT 60 >> +#define DEFAULT_WINDOWSIZE 50 >> >> /* Max heartbeat is calculated at 32kHz source clock */ >> #define MAX_HEARTBEAT 1000 >> @@ -35,9 +36,13 @@ >> >> #define RTIWWDRX_NMI 0xa >> >> -#define RTIWWDSIZE_50P 0x50 >> +#define RTIWWDSIZE_50P 0x50 >> +#define RTIWWDSIZE_25P 0x500 >> +#define RTIWWDSIZE_12P5 0x5000 >> +#define RTIWWDSIZE_6P25 0x50000 >> +#define RTIWWDSIZE_3P125 0x500000 >> >> -#define WDENABLE_KEY 0xa98559da >> +#define WDENABLE_KEY 0xa98559da >> >> #define WDKEY_SEQ0 0xe51a >> #define WDKEY_SEQ1 0xa35c >> @@ -48,7 +53,8 @@ >> >> #define DWDST BIT(1) >> >> -static int heartbeat; >> +static int heartbeat = DEFAULT_HEARTBEAT; >> +static u32 wsize = DEFAULT_WINDOWSIZE; >> >> /* >> * struct to hold data for each WDT device >> @@ -62,34 +68,93 @@ struct rti_wdt_device { >> struct watchdog_device wdd; >> }; >> >> +static int rti_wdt_convert_wsize(void) >> +{ >> + if (wsize >= 50) { >> + wsize = RTIWWDSIZE_50P; >> + } else if (wsize >= 25) { >> + wsize = RTIWWDSIZE_25P; >> + } else if (wsize > 12) { >> + wsize = RTIWWDSIZE_12P5; >> + } else if (wsize > 6) { >> + wsize = RTIWWDSIZE_6P25; >> + } else if (wsize > 3) { >> + wsize = RTIWWDSIZE_3P125; >> + } else { >> + pr_err("%s: bad windowsize: %d\n", __func__, wsize); > > Please do not use pr_ functions. Pass the watchdog device as argument > and use dev_err(). > > Also, this function modifies the wsize parameter. When called > again, that parameter will have a totally different meaning, and > the second call to this function will always set the window size > to 50. > > On top of all that, window sizes larger than 50 are set to 50, > window sizes between 4 and 49 are adjusted, and window sizes <= 3 > are rejected. That is not exactly consistent. > > Does this module parameter really add value / make sense ? > What is the use case ? We should not add such complexity without > use case.
I'll ditch the support for this. Just thought that it would be neat feature to have this in place because I ended up implementing most of this for the attach feature anyways.
> >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd) >> +{ >> + /* >> + * RTI only supports a windowed mode, where the watchdog can only >> + * be petted during the open window; not too early or not too late. >> + * The HW configuration options only allow for the open window size >> + * to be 50% or less than that. >> + */ >> + switch (wsize) { >> + case RTIWWDSIZE_50P: >> + /* 50% open window => 50% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 500 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_25P: >> + /* 25% open window => 75% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 750 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_12P5: >> + /* 12.5% open window => 87.5% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 875 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_6P25: >> + /* 6.5% open window => 93.5% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 935 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_3P125: >> + /* 3.125% open window => 96.9% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 969 * heartbeat; >> + break; >> + >> + default: >> + pr_err("%s: Bad watchdog window size!\n", __func__); > > Same here. > >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static int rti_wdt_start(struct watchdog_device *wdd) >> { >> u32 timer_margin; >> struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd); >> + int ret; >> >> /* set timeout period */ >> - timer_margin = (u64)wdd->timeout * wdt->freq; >> + timer_margin = (u64)heartbeat * wdt->freq; >> timer_margin >>= WDT_PRELOAD_SHIFT; >> if (timer_margin > WDT_PRELOAD_MAX) >> timer_margin = WDT_PRELOAD_MAX; >> writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD); >> >> - /* >> - * RTI only supports a windowed mode, where the watchdog can only >> - * be petted during the open window; not too early or not too late. >> - * The HW configuration options only allow for the open window size >> - * to be 50% or less than that; we obviouly want to configure the open >> - * window as large as possible so we select the 50% option. To avoid >> - * any glitches, we accommodate 5% safety margin also, so we setup >> - * the min_hw_hearbeat at 55% of the timeout period. >> - */ >> - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; >> + ret = rti_wdt_convert_wsize(); >> + if (ret) >> + return ret; >> + >> + ret = rti_wdt_setup_hw_hb(wdd); >> + if (ret) >> + return ret; >> > > This is the wrong place to validate the window size. It should be done > only once, in the probe function. The start function should not fail > because of a bad window size. > > With such parameters, the wsize written into the chip should be kept > in struct rti_wdt_device if it needs to be set more than once. > The module parameter should not be changed, and it should not be used > to store the register value. min_hw_heartbeat_ms needs to be set in the > probe function, not in the start function. Sorry that I didn't notice > that before.
Yeah, this will be gone.
> >> /* Generate NMI when wdt expires */ >> writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); >> >> - /* Open window size 50%; this is the largest window size available */ >> - writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL); >> + writel_relaxed(wsize, wdt->base + RTIWWDSIZECTRL); >> >> readl_relaxed(wdt->base + RTIWWDSIZECTRL); >> >> @@ -169,6 +234,14 @@ static int rti_wdt_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> + /* >> + * If watchdog is running at 32k clock, it is not accurate. >> + * Adjust frequency down in this case so that we don't pet >> + * the watchdog too often. >> + */ >> + if (wdt->freq > 30000 && wdt->freq < 32768) >> + wdt->freq = 30000; >> + > > Combining that with a window size of 96.9% min heartbeat is asking > for trouble. It will be all but impossible to catch the window with > such constraints if the frequency is really that inaccurate.
Yeah, I don't think anything else than 50% window size makes any sense. Anyways, will drop the support for configuring this runtime.
> >> pm_runtime_enable(dev); >> ret = pm_runtime_get_sync(dev); >> if (ret) { >> @@ -251,5 +324,10 @@ MODULE_PARM_DESC(heartbeat, >> __MODULE_STRING(MAX_HEARTBEAT) ", default " >> __MODULE_STRING(DEFAULT_HEARTBEAT)); >> >> +module_param(wsize, uint, 0); >> +MODULE_PARM_DESC(wsize, >> + "Watchdog open window size in percentage from 3 to 50, " >> + "default " __MODULE_STRING(DEFAULT_WINDOW_SIZE)); >> + >> MODULE_LICENSE("GPL"); >> MODULE_ALIAS("platform:rti-wdt"); >> >
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
| |