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

[Guenter Roeck]

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.
> 
> 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 ?

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;
>>>
>>
> 
>