| Message ID | 20200703120406.7092-4-t-kristo@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2,1/5] watchdog: use __watchdog_ping in startup | expand |
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. > > 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. > + 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. > /* 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. > 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"); >
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
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); + 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__); + 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; /* 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; + 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");
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. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++------ 1 file changed, 95 insertions(+), 17 deletions(-) -- 2.17.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki