Re:Re: [PATCH V6 1/2] watchdog: mtk: support pre-timeout when the bark irq is available

From: 王擎
Date: Thu Apr 22 2021 - 03:05:53 EST



>On 4/21/21 8:46 PM, 王擎 wrote:
>>
>>> On 4/21/21 7:45 PM, Wang Qing wrote:
>>>> Use the bark interrupt as the pretimeout notifier if available.
>>>>
>>>> When the watchdog timer expires in dual mode, an interrupt will be
>>>> triggered first, then the timing restarts. The reset signal will be
>>>> initiated when the timer expires again.
>>>>
>>>> The pretimeout notification shall occur at timeout-sec/2.
>>>>
>>>> V2:
>>>> - panic() by default if WATCHDOG_PRETIMEOUT_GOV is not enabled.
>>>>
>>>> V3:
>>>> - Modify the pretimeout behavior, manually reset after the pretimeout
>>>> - is processed and wait until timeout.
>>>>
>>>> V4:
>>>> - Remove pretimeout related processing.
>>>> - Add dual mode control separately.
>>>>
>>>> V5:
>>>> - Fix some formatting and printing problems.
>>>>
>>>> V6:
>>>> - Realize pretimeout processing through dualmode.
>>>>
>>>> Signed-off-by: Wang Qing <wangqing@xxxxxxxx>
>>>> ---
>>>> drivers/watchdog/mtk_wdt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
>>>> 1 file changed, 48 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>> index 97ca993..ebc648b
>>>> --- a/drivers/watchdog/mtk_wdt.c
>>>> +++ b/drivers/watchdog/mtk_wdt.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/reset-controller.h>
>>>> #include <linux/types.h>
>>>> #include <linux/watchdog.h>
>>>> +#include <linux/interrupt.h>
>>>>
>>>> #define WDT_MAX_TIMEOUT 31
>>>> #define WDT_MIN_TIMEOUT 1
>>>> @@ -184,15 +185,22 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
>>>> {
>>>> struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
>>>> void __iomem *wdt_base = mtk_wdt->wdt_base;
>>>> + unsigned int timeout_interval;
>>>> u32 reg;
>>>>
>>>> - wdt_dev->timeout = timeout;
>>>> + timeout_interval = wdt_dev->timeout = timeout;
>>>> + /*
>>>> + * In dual mode, irq will be triggered at timeout/2
>>>> + * the real timeout occurs at timeout
>>>> + */
>>>> + if (wdt_dev->pretimeout)
>>>> + timeout_interval = wdt_dev->pretimeout = timeout/2;
>>>
>>> Please run checkpatch --strict and fix what it reports.
>>> Also, there should be a set_pretimeout function to set the
>>> pretimeout. It is ok to update it here, but it should be set
>>> in its own function to make sure that the actual value
>>> is reported back to userspace.
>>>
>>> Thanks,
>>> Guenter
>>
>> The reason why the set_pretimeout interface is not provided is
>> because the pretimeout is fixed after the timeout is set, we need
>> to modify timeout after setting pretimeout, which is puzzling.
>>
>
>What you need to do is to set pretimeout = timeout / 2 if a pretimeout
>is set to a value != 0. Just like we adjust timeout to valid values
>when set, we adjust pretimeout as well. I don't see a problem with that.
>
>Guenter

Thanks, Guenter. But this will complicate the situation:
First, set_pretimeout will become an interface for dynamically enable and
disable the pre-timeout func, instead of adjusting the pretimeout time.

Secondly, when the irq is not registered, the user cannot be allowed to set
the pretimeout to non-zero. When irq is registered, it doesn't make any sense
to turn off pre-timeout func.

Because of the particularity of dual mode, I still insist on enabling the
pretimeout or not through devicetree. And looking forward to your suggestions.

Thanks,
Qing