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

[王擎]

>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