Date:   Fri, 10 Apr 2020 22:04:00 +0300
From:   Sergey Semin <Sergey.Semin@...kalelectronics.ru>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        <linux-watchdog@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 6/7] watchdog: dw_wdt: Add pre-timeouts support

On Fri, Mar 06, 2020 at 07:14:50AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@...kalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > 
> > DW Watchdog can rise an interrupt in case if IRQ request mode
> > is enabled and timer reaches the zero value. In this case the IRQ
> > lane is left pending until either the next watchdog kick event
> > (watchdog restart) or until the WDT_EOI register is read or
> > the device/system reset. This interface can be used to implement
> > the pre-timeout functionality optionally provided by the Linux kernel
> > watchdog devices.
> > 
> > IRQ mode provides a two stages timeout interface. It means the IRQ is
> > raised when the counter reaches zero, while the system reset occurs
> > only after subsequent timeout if the timer restart is not performed.
> > Due to this peculiarity the pre-timeout value is actually set to the
> > achieved hardware timeout, while the real watchdog timeout is
> > considered to be twice as much of it. This applies a significant
> > limitation on the pre-timeout values, so current implementation
> > supports either zero value, which disables the pre-timeout events, or
> > non-zero values, which imply the pre-timeout to be at least half
> > of the current watchdog timeout.
> > 
> > Note that we ask the interrupt controller to detect the rising-edge
> > pre-timeout interrupts to prevent the high-level-IRQs flood, since
> > if the pre-timeout happens, the IRQ lane will be left pending until
> > it's cleared by the timer restart.
> > 
> > Seeing all currently supported platforms, which have the DW Watchdog
> > installed, provide the interrupt property in the corresponding watchdog
> > dts node, we can define the IRQ to be mandatory.
> 
> I don't see this as valid argument. It is only needed if one wants to have
> pretimeout support, and if one doesn't it is simply not necessary.
> "Everyone uses it" is not an argument - someone else might come tomorrow
> and not want to use it.

Ok. I'll make it optional.

-Sergey

> 
> Guenter
> 
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> > Cc: Paul Burton <paulburton@...nel.org>
> > Cc: Ralf Baechle <ralf@...ux-mips.org>
> > ---
> >  drivers/watchdog/dw_wdt.c | 125 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index eb909c63a1b5..3000120f7e39 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/of.h>
> >  #include <linux/pm.h>
> >  #include <linux/platform_device.h>
> > @@ -35,6 +36,8 @@
> >  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> >  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
> >  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> > +#define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
> > +#define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
> >  #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> >  #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> >  
> > @@ -58,11 +61,17 @@ module_param(nowayout, bool, 0);
> >  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >  		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >  
> > +enum dw_wdt_rmod {
> > +	DW_WDT_RMOD_RESET = 1,
> > +	DW_WDT_RMOD_IRQ = 2
> > +};
> > +
> >  struct dw_wdt {
> >  	void __iomem		*regs;
> >  	struct clk		*clk;
> >  	struct clk		*pclk;
> >  	unsigned long		rate;
> > +	enum dw_wdt_rmod	rmod;
> >  	unsigned int		max_top;
> >  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> >  	struct watchdog_device	wdd;
> > @@ -80,6 +89,20 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> >  		WDOG_CONTROL_REG_WDT_EN_MASK;
> >  }
> >  
> > +static void dw_wdt_update_mode(struct dw_wdt *dw_wdt, enum dw_wdt_rmod rmod)
> > +{
> > +	u32 val;
> > +
> > +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +	if (rmod == DW_WDT_RMOD_IRQ)
> > +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	else
> > +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +
> > +	dw_wdt->rmod = rmod;
> > +}
> > +
> >  static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> >  					 unsigned int timeout, u32 *top)
> >  {
> > @@ -141,7 +164,11 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >  {
> >  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
> >  
> > -	return dw_wdt->timeouts[top];
> > +	/*
> > +	 * In IRQ mode due to the two stages counter, the actual timeout is
> > +	 * twice greater than the TOP setting.
> > +	 */
> > +	return (dw_wdt->timeouts[top] * dw_wdt->rmod);
> >  }
> >  
> >  static int dw_wdt_ping(struct watchdog_device *wdd)
> > @@ -160,7 +187,21 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	unsigned int timeout;
> >  	u32 top;
> >  
> > -	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> > +	/*
> > +	 * We try to find a timeout achievable by the device or set the maximum
> > +	 * one. Note IRQ mode being enabled means having a non-zero pre-timeout
> > +	 * setup. In this case we try to find a TOP as close to the half of the
> > +	 * requested timeout as possible since DW Watchdog IRQ mode is designed
> > +	 * in two stages way - first timeout rises the pre-timeout interrupt,
> > +	 * second timeout performs the system reset.
> > +	 */
> > +	timeout = dw_wdt_find_best_top(dw_wdt,
> > +		req * (MSEC_PER_SEC / dw_wdt->rmod), &top);
> > +	timeout = (timeout * dw_wdt->rmod) / MSEC_PER_SEC;
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> > +		wdd->pretimeout = timeout / dw_wdt->rmod;
> > +	else
> > +		wdd->pretimeout = 0;
> >  
> >  	/*
> >  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> > @@ -171,6 +212,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  
> > +	/* Kick new TOP value into the watchdog counter if activated. */
> > +	if (watchdog_active(wdd))
> > +		dw_wdt_ping(wdd);
> > +
> >  	/*
> >  	 * In case users set bigger timeout value than HW can support,
> >  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> > @@ -179,7 +224,22 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> >  		wdd->timeout = req;
> >  	else
> > -		wdd->timeout = timeout / MSEC_PER_SEC;
> > +		wdd->timeout = timeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_wdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
> > +{
> > +	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > +
> > +	/*
> > +	 * We ignore actual value of the timeout passed from user-space
> > +	 * using it as a flag whether the pretimeout functionality is intended
> > +	 * to be activated.
> > +	 */
> > +	dw_wdt_update_mode(dw_wdt, req ? DW_WDT_RMOD_IRQ : DW_WDT_RMOD_RESET);
> > +	dw_wdt_set_timeout(wdd, wdd->timeout);
> >  
> >  	return 0;
> >  }
> > @@ -188,8 +248,11 @@ static void dw_wdt_arm_system_reset(struct dw_wdt *dw_wdt)
> >  {
> >  	u32 val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  
> > -	/* Disable interrupt mode; always perform system reset. */
> > -	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	/* Disable/enable interrupt mode depending on the RMOD flag. */
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> > +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	else
> > +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> >  	/* Enable watchdog. */
> >  	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
> >  	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > @@ -227,6 +290,7 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
> >  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> >  
> >  	writel(0, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> > +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> >  	if (dw_wdt_is_enabled(dw_wdt))
> >  		writel(WDOG_COUNTER_RESTART_KICK_VALUE,
> >  		       dw_wdt->regs + WDOG_COUNTER_RESTART_REG_OFFSET);
> > @@ -242,14 +306,24 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
> >  static unsigned int dw_wdt_get_timeleft(struct watchdog_device *wdd)
> >  {
> >  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > +	unsigned int time;
> > +	u32 val;
> > +
> > +	val = readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET);
> > +	time = val / dw_wdt->rate;
> > +
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ) {
> > +		val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> > +		if (!val)
> > +			time += wdd->pretimeout;
> > +	}
> >  
> > -	return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
> > -		dw_wdt->rate;
> > +	return time;
> >  }
> >  
> >  static const struct watchdog_info dw_wdt_ident = {
> >  	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> > -			  WDIOF_MAGICCLOSE,
> > +			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
> >  	.identity	= "Synopsys DesignWare Watchdog",
> >  };
> >  
> > @@ -259,10 +333,29 @@ static const struct watchdog_ops dw_wdt_ops = {
> >  	.stop		= dw_wdt_stop,
> >  	.ping		= dw_wdt_ping,
> >  	.set_timeout	= dw_wdt_set_timeout,
> > +	.set_pretimeout	= dw_wdt_set_pretimeout,
> >  	.get_timeleft	= dw_wdt_get_timeleft,
> >  	.restart	= dw_wdt_restart,
> >  };
> >  
> > +static irqreturn_t dw_wdt_irq(int irq, void *devid)
> > +{
> > +	struct dw_wdt *dw_wdt = devid;
> > +	u32 val;
> > +
> > +	/*
> > +	 * We don't clear the IRQ status. It's supposed to be done by following
> > +	 * ping operations.
> > +	 */
> > +	val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	watchdog_notify_pretimeout(&dw_wdt->wdd);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >  static int dw_wdt_suspend(struct device *dev)
> >  {
> > @@ -398,10 +491,26 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_pclk;
> >  	}
> >  
> > +	ret = platform_get_irq(pdev, 0);
> > +	if (ret < 0)
> > +		goto out_disable_pclk;
> > +
> > +	/*
> > +	 * We must request rising-edge IRQ, since the lane is left pending
> > +	 * either until the next watchdog kick event or up to the system reset.
> > +	 */
> > +	ret = devm_request_irq(dev, ret, dw_wdt_irq,
> > +			       IRQF_SHARED | IRQF_TRIGGER_RISING,
> > +			       pdev->name, dw_wdt);
> > +	if (ret)
> > +		goto out_disable_pclk;
> > +
> >  	reset_control_deassert(dw_wdt->rst);
> >  
> >  	dw_wdt_init_timeouts(dw_wdt, dev);
> >  
> > +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> > +
> >  	wdd = &dw_wdt->wdd;
> >  	wdd->info = &dw_wdt_ident;
> >  	wdd->ops = &dw_wdt_ops;
> > 
> 

Powered by blists - more mailing lists