Message-ID: <20200530065148.qswtkedsn6uibzlo@mobilestation>
Date:   Sat, 30 May 2020 09:51:48 +0300
From:   Serge Semin <Sergey.Semin@...kalelectronics.ru>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>, <linux-mips@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-watchdog@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 6/7] watchdog: dw_wdt: Add pre-timeouts support

On Fri, May 29, 2020 at 04:02:19PM -0700, Guenter Roeck wrote:
> On Tue, May 26, 2020 at 06:41:22PM +0300, Serge Semin wrote:
> > 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.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Rob Herring <robh+dt@...nel.org>
> > Cc: linux-mips@...r.kernel.org
> > Cc: devicetree@...r.kernel.org
> 

> Nitpick below, but I don't really know what to do about it, so
> 
> Reviewed-by: Guenter Roeck <linux@...ck-us.net>

Right. Semantically platform_get_irq_optional() should never return zero even
though the comment above that function definition states otherwise. I'll fix the
conditional statement to check for > 0 you've commented below and resend with
your tag attached. Thanks.

-Sergey

> 
> > ---
> > 
> > Changelog v2:
> > - Rearrange SoBs.
> > - Make the Pre-timeout IRQ being optionally supported.
> > ---
> >  drivers/watchdog/dw_wdt.c | 138 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 130 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index efbc36872670..3cd7c485cd70 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -22,6 +22,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>
> > @@ -36,6 +37,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)
> >  
> > @@ -59,6 +62,11 @@ 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_timeout {
> >  	u32 top_val;
> >  	unsigned int sec;
> > @@ -70,6 +78,7 @@ struct dw_wdt {
> >  	struct clk		*clk;
> >  	struct clk		*pclk;
> >  	unsigned long		rate;
> > +	enum dw_wdt_rmod	rmod;
> >  	struct dw_wdt_timeout	timeouts[DW_WDT_NUM_TOPS];
> >  	struct watchdog_device	wdd;
> >  	struct reset_control	*rst;
> > @@ -86,6 +95,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_val)
> >  {
> > @@ -145,7 +168,11 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >  			break;
> >  	}
> >  
> > -	return dw_wdt->timeouts[idx].sec;
> > +	/*
> > +	 * In IRQ mode due to the two stages counter, the actual timeout is
> > +	 * twice greater than the TOP setting.
> > +	 */
> > +	return dw_wdt->timeouts[idx].sec * dw_wdt->rmod;
> >  }
> >  
> >  static int dw_wdt_ping(struct watchdog_device *wdd)
> > @@ -164,7 +191,20 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >  	unsigned int timeout;
> >  	u32 top_val;
> >  
> > -	timeout = dw_wdt_find_best_top(dw_wdt, top_s, &top_val);
> > +	/*
> > +	 * 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. So basically the effective
> > +	 * watchdog-caused reset happens after two watchdog TOPs elapsed.
> > +	 */
> > +	timeout = dw_wdt_find_best_top(dw_wdt, DIV_ROUND_UP(top_s, dw_wdt->rmod),
> > +				       &top_val);
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> > +		wdd->pretimeout = timeout;
> > +	else
> > +		wdd->pretimeout = 0;
> >  
> >  	/*
> >  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> > @@ -175,25 +215,47 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >  	writel(top_val | top_val << 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
> >  	 * wdd->max_hw_heartbeat_ms
> >  	 */
> >  	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> > -		wdd->timeout = timeout;
> > +		wdd->timeout = timeout * dw_wdt->rmod;
> >  	else
> >  		wdd->timeout = top_s;
> >  
> >  	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;
> > +}
> > +
> >  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);
> > @@ -231,6 +293,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);
> > @@ -246,9 +309,19 @@ 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 sec;
> > +	u32 val;
> > +
> > +	val = readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET);
> > +	sec = val / dw_wdt->rate;
> >  
> > -	return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
> > -		dw_wdt->rate;
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ) {
> > +		val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> > +		if (!val)
> > +			sec += wdd->pretimeout;
> > +	}
> > +
> > +	return sec;
> >  }
> >  
> >  static const struct watchdog_info dw_wdt_ident = {
> > @@ -257,16 +330,41 @@ static const struct watchdog_info dw_wdt_ident = {
> >  	.identity	= "Synopsys DesignWare Watchdog",
> >  };
> >  
> > +static const struct watchdog_info dw_wdt_pt_ident = {
> > +	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> > +			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
> > +	.identity	= "Synopsys DesignWare Watchdog",
> > +};
> > +
> >  static const struct watchdog_ops dw_wdt_ops = {
> >  	.owner		= THIS_MODULE,
> >  	.start		= dw_wdt_start,
> >  	.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 the
> > +	 * 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)
> >  {
> > @@ -447,6 +545,31 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_pclk;
> >  	}
> >  
> > +	/* Enable normal reset without pre-timeout by default. */
> > +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> > +
> > +	/*
> > +	 * Pre-timeout IRQ is optional, since some hardware may lack support
> > +	 * of it. Note 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 = platform_get_irq_optional(pdev, 0);
> > +	if (ret >= 0) {
> 
> I keep seeing notes that an interrupt value of 0 is invalid.
> 
> > +		ret = devm_request_irq(dev, ret, dw_wdt_irq,
> > +				       IRQF_SHARED | IRQF_TRIGGER_RISING,
> > +				       pdev->name, dw_wdt);
> > +		if (ret)
> > +			goto out_disable_pclk;
> > +
> > +		dw_wdt->wdd.info = &dw_wdt_pt_ident;
> > +	} else {
> > +		if (ret == -EPROBE_DEFER)
> > +			goto out_disable_pclk;
> > +
> > +		dw_wdt->wdd.info = &dw_wdt_ident;
> > +	}
> > +
> >  	reset_control_deassert(dw_wdt->rst);
> >  
> >  	ret = dw_wdt_init_timeouts(dw_wdt, dev);
> > @@ -454,7 +577,6 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_clk;
> >  
> >  	wdd = &dw_wdt->wdd;
> > -	wdd->info = &dw_wdt_ident;
> >  	wdd->ops = &dw_wdt_ops;
> >  	wdd->min_timeout = dw_wdt_get_min_timeout(dw_wdt);
> >  	wdd->max_hw_heartbeat_ms = dw_wdt_get_max_timeout_ms(dw_wdt);

Powered by blists - more mailing lists