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

[王擎]

>On 4/22/21 8:48 PM, 王擎 wrote:
>> 
>>> On 4/22/21 7:53 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.
>>>>
>>>> V7:
>>>> - Add set_pretimeout().
>>>>
>>>> Signed-off-by: Wang Qing <wangqing@xxxxxxxx>
>>>> ---
>>>>  drivers/watchdog/mtk_wdt.c | 76 +++++++++++++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 71 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>> index 97ca993..ab3ac5d
>>>> --- 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,23 @@ 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 = timeout;
>>>>  	u32 reg;
>>>>  
>>>>  	wdt_dev->timeout = timeout;
>>>> -
>>>> +	/*
>>>> +	 * In dual mode, irq will be triggered at timeout / 2
>>>> +	 * the real timeout occurs at timeout
>>>> +	 */
>>>> +	if (wdt_dev->pretimeout) {
>>>> +		wdt_dev->pretimeout = timeout / 2;
>>>
>>> min_timeout is set to 1. I don't this works well if timeout == 1.
>>> You'll either need to set min_timeout to 2, or handle that case.
>> 
>> It is appropriate to change min_timeout  to 2.
>> 
>>>
>>>> +		timeout_interval = wdt_dev->pretimeout;
>>>
>>> timeout_interval is unnecessary. Just update timeout accordingly.
>>> It needs to take the situation of timeout == 1 into account, though.

I plan to remove timeout_interval and directly use (timeout-pretimeout)
update timeout.

>> 
>> timeout represents the reset time. When the user calls timeout_show, 
>> He hopes to get the configured timeout, not the value changed
>> by pre-timeout.
>> I modify it like this more in line with the original intention.
>> 
>>>
>>>> +	}
>>>>  	/*
>>>>  	 * One bit is the value of 512 ticks
>>>>  	 * The clock has 32 KHz
>>>>  	 */
>>>> -	reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
>>>> +	reg = WDT_LENGTH_TIMEOUT(timeout_interval << 6) | WDT_LENGTH_KEY;
>>>>  	iowrite32(reg, wdt_base + WDT_LENGTH);
>>>>  
>>>>  	mtk_wdt_ping(wdt_dev);
>>>> @@ -239,13 +248,46 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
>>>>  		return ret;
>>>>  
>>>>  	reg = ioread32(wdt_base + WDT_MODE);
>>>> -	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> +	if (wdt_dev->pretimeout)
>>>> +		reg |= (WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> +	else
>>>> +		reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>>  	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
>>>>  	iowrite32(reg, wdt_base + WDT_MODE);
>>>>  
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int mtk_wdt_set_pretimeout(struct watchdog_device *wdd,
>>>> +					unsigned int timeout)
>>>> +{
>>>> +	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdd);
>>>> +	void __iomem *wdt_base = mtk_wdt->wdt_base;
>>>> +	u32 reg = ioread32(wdt_base + WDT_MODE);
>>>> +
>>>> +	if (timeout && !wdd->pretimeout) {
>>>> +		wdd->pretimeout = wdd->timeout / 2;
>>>> +		reg |= (WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> +	} else if (!timeout && wdd->pretimeout) {
>>>> +		wdd->pretimeout = 0;
>>>> +		reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> +	} else
>>>> +		return 0;
>>>> +
>>>> +	iowrite32(reg, wdt_base + WDT_MODE);
>>>
>>> What is the point of setting the mode here ? It will
>>> be set again in mtk_wdt_set_timeout(). Seems to me all
>>> you need to do here is to set wdd->pretimeout,
>>> then call mtk_wdt_set_timeout().
>> 
>> mtk_wdt_set_timeout() only set timeout and ping().
>> Here also need to config to the dualmode or not.
>> 
>Ah, you are correct. Sorry, I confused this with the start function.
>That makes me wonder if it would be better to extract a separate
>function, mtk_wdt_set_mode(), for that purpose. Thoughts ?

I have done this, but I found there is no good abstract method, 
because wdt mode is used in combination, for example：
WDT_MODE_EN is included in start(), and here is not.
And the judgment of pretimeout here is also different.

Thanks,
Qing

>
>Thanks,
>Guenter
>
>> Thanks,
>> Qing
>>>
>>> Guenter
>>>
>>>> +
>>>> +	return mtk_wdt_set_timeout(wdd, wdd->timeout);
>>>> +}
>>>> +
>>>> +static irqreturn_t mtk_wdt_isr(int irq, void *arg)
>>>> +{
>>>> +	struct watchdog_device *wdd = arg;
>>>> +
>>>> +	watchdog_notify_pretimeout(wdd);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>>  static const struct watchdog_info mtk_wdt_info = {
>>>>  	.identity	= DRV_NAME,
>>>>  	.options	= WDIOF_SETTIMEOUT |
>>>> @@ -253,12 +295,21 @@ static const struct watchdog_info mtk_wdt_info = {
>>>>  			  WDIOF_MAGICCLOSE,
>>>>  };
>>>>  
>>>> +static const struct watchdog_info mtk_wdt_pt_info = {
>>>> +	.identity	= DRV_NAME,
>>>> +	.options	= WDIOF_SETTIMEOUT |
>>>> +			  WDIOF_PRETIMEOUT |
>>>> +			  WDIOF_KEEPALIVEPING |
>>>> +			  WDIOF_MAGICCLOSE,
>>>> +};
>>>> +
>>>>  static const struct watchdog_ops mtk_wdt_ops = {
>>>>  	.owner		= THIS_MODULE,
>>>>  	.start		= mtk_wdt_start,
>>>>  	.stop		= mtk_wdt_stop,
>>>>  	.ping		= mtk_wdt_ping,
>>>>  	.set_timeout	= mtk_wdt_set_timeout,
>>>> +	.set_pretimeout	= mtk_wdt_set_pretimeout,
>>>>  	.restart	= mtk_wdt_restart,
>>>>  };
>>>>  
>>>> @@ -267,7 +318,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>>>>  	struct device *dev = &pdev->dev;
>>>>  	struct mtk_wdt_dev *mtk_wdt;
>>>>  	const struct mtk_wdt_data *wdt_data;
>>>> -	int err;
>>>> +	int err, irq;
>>>>  
>>>>  	mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
>>>>  	if (!mtk_wdt)
>>>> @@ -279,7 +330,22 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>>>>  	if (IS_ERR(mtk_wdt->wdt_base))
>>>>  		return PTR_ERR(mtk_wdt->wdt_base);
>>>>  
>>>> -	mtk_wdt->wdt_dev.info = &mtk_wdt_info;
>>>> +	irq = platform_get_irq(pdev, 0);
>>>> +	if (irq > 0) {
>>>> +		err = devm_request_irq(&pdev->dev, irq, mtk_wdt_isr, 0, "wdt_bark",
>>>> +					&mtk_wdt->wdt_dev);
>>>> +		if (err)
>>>> +			return err;
>>>> +
>>>> +		mtk_wdt->wdt_dev.info = &mtk_wdt_pt_info;
>>>> +		mtk_wdt->wdt_dev.pretimeout = WDT_MAX_TIMEOUT / 2;
>>>> +	} else {
>>>> +		if (irq == -EPROBE_DEFER)
>>>> +			return -EPROBE_DEFER;
>>>> +
>>>> +		mtk_wdt->wdt_dev.info = &mtk_wdt_info;
>>>> +	}
>>>> +
>>>>  	mtk_wdt->wdt_dev.ops = &mtk_wdt_ops;
>>>>  	mtk_wdt->wdt_dev.timeout = WDT_MAX_TIMEOUT;
>>>>  	mtk_wdt->wdt_dev.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT * 1000;
>>>>
>>>
>> 
>> 
>