lkml.org 
[lkml]   [2020]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv2 3/5] watchdog: rti-wdt: add support for window size configuration
From
Date
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

\
 
 \ /
  Last update: 2020-07-13 14:54    [W:0.073 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site