Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5] watchdog: orion_wdt: use timer1 as a pretimeout
@ 2019-08-29  8:50 Chris Packham
  2019-08-29 12:46 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Chris Packham @ 2019-08-29  8:50 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-kernel, Chris Packham

The orion watchdog can either reset the CPU or generate an interrupt.
The interrupt would be useful for debugging as it provides panic()
output about the watchdog expiry, however if the interrupt is used the
watchdog can't reset the CPU in the event of being stuck in a loop with
interrupts disabled or if the CPU is prevented from accessing memory
(e.g. an unterminated DMA).

The Armada SoCs have spare timers that aren't currently used by the
Linux kernel. We can use timer1 to provide a pre-timeout ahead of the
watchdog timer and provide the possibility of gathering debug before the
reset triggers.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes in v5:
- Group bit values with register addresses
- Address review comments from Gunter
Changes in v4:
- rebase against linux/master
Changes in v2:
- apply changes to armada-38x only

 drivers/watchdog/orion_wdt.c | 65 ++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index cdb0d174c5e2..5a23cb448ed5 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -35,7 +35,15 @@
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
-#define TIMER_A370_STATUS	0x04
+#define TIMER1_FIXED_ENABLE_BIT	BIT(12)
+#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
+#define TIMER1_ENABLE_BIT	BIT(2)
+
+#define TIMER_A370_STATUS	0x0004
+#define WDT_A370_EXPIRED	BIT(31)
+#define TIMER1_STATUS_BIT	BIT(8)
+
+#define TIMER1_VAL_OFF		0x001c
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 
@@ -43,9 +51,6 @@
 #define WDT_A370_RATIO_SHIFT	5
 #define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
 
-#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
-#define WDT_A370_EXPIRED	BIT(31)
-
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
@@ -158,6 +163,7 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 				   struct orion_watchdog *dev)
 {
 	int ret;
+	u32 val;
 
 	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
 	if (IS_ERR(dev->clk))
@@ -168,10 +174,9 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 		return ret;
 	}
 
-	/* Enable the fixed watchdog clock input */
-	atomic_io_modify(dev->reg + TIMER_CTRL,
-			 WDT_AXP_FIXED_ENABLE_BIT,
-			 WDT_AXP_FIXED_ENABLE_BIT);
+	/* Fix the wdt and timer1 clock freqency to 25MHz */
+	val = WDT_AXP_FIXED_ENABLE_BIT | TIMER1_FIXED_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, val, val);
 
 	dev->clk_rate = clk_get_rate(dev->clk);
 	return 0;
@@ -183,6 +188,10 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	/* Reload watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
 	       dev->reg + dev->data->wdt_counter_offset);
+	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
+		writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
+		       dev->reg + TIMER1_VAL_OFF);
+
 	return 0;
 }
 
@@ -194,13 +203,18 @@ static int armada375_start(struct watchdog_device *wdt_dev)
 	/* Set watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
 	       dev->reg + dev->data->wdt_counter_offset);
+	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
+		writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
+		       dev->reg + TIMER1_VAL_OFF);
 
 	/* Clear the watchdog expiration bit */
 	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
-						dev->data->wdt_enable_bit);
+	reg = dev->data->wdt_enable_bit;
+	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
+		reg |= TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, reg, reg);
 
 	/* Enable reset on watchdog */
 	reg = readl(dev->rstout);
@@ -277,7 +291,7 @@ static int orion_stop(struct watchdog_device *wdt_dev)
 static int armada375_stop(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	u32 reg;
+	u32 reg, mask;
 
 	/* Disable reset on watchdog */
 	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit,
@@ -287,7 +301,10 @@ static int armada375_stop(struct watchdog_device *wdt_dev)
 	writel(reg, dev->rstout);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
+	mask = dev->data->wdt_enable_bit;
+	if (wdt_dev->info->options & WDIOF_PRETIMEOUT)
+		mask &= ~TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
 
 	return 0;
 }
@@ -349,7 +366,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 	return readl(dev->reg + dev->data->wdt_counter_offset) / dev->clk_rate;
 }
 
-static const struct watchdog_info orion_wdt_info = {
+static struct watchdog_info orion_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "Orion Watchdog",
 };
@@ -368,6 +385,16 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t orion_wdt_pre_irq(int irq, void *devid)
+{
+	struct orion_watchdog *dev = devid;
+
+	atomic_io_modify(dev->reg + TIMER_A370_STATUS,
+			 TIMER1_STATUS_BIT, 0);
+	watchdog_notify_pretimeout(&dev->wdt);
+	return IRQ_HANDLED;
+}
+
 /*
  * The original devicetree binding for this driver specified only
  * one memory resource, so in order to keep DT backwards compatibility
@@ -589,6 +616,18 @@ static int orion_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
+	irq = platform_get_irq(pdev, 1);
+	if (irq > 0) {
+		orion_wdt_info.options |= WDIOF_PRETIMEOUT;
+		ret = devm_request_irq(&pdev->dev, irq, orion_wdt_pre_irq,
+				       0, pdev->name, dev);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ\n");
+			goto disable_clk;
+		}
+	}
+
+
 	watchdog_set_nowayout(&dev->wdt, nowayout);
 	ret = watchdog_register_device(&dev->wdt);
 	if (ret)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v5] watchdog: orion_wdt: use timer1 as a pretimeout
  2019-08-29  8:50 [PATCH v5] watchdog: orion_wdt: use timer1 as a pretimeout Chris Packham
@ 2019-08-29 12:46 ` Guenter Roeck
  2019-08-29 21:02   ` Chris Packham
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-08-29 12:46 UTC (permalink / raw)
  To: Chris Packham, wim; +Cc: linux-watchdog, linux-kernel

On 8/29/19 1:50 AM, Chris Packham wrote:
> The orion watchdog can either reset the CPU or generate an interrupt.
> The interrupt would be useful for debugging as it provides panic()
> output about the watchdog expiry, however if the interrupt is used the
> watchdog can't reset the CPU in the event of being stuck in a loop with
> interrupts disabled or if the CPU is prevented from accessing memory
> (e.g. an unterminated DMA).
> 
> The Armada SoCs have spare timers that aren't currently used by the
> Linux kernel. We can use timer1 to provide a pre-timeout ahead of the
> watchdog timer and provide the possibility of gathering debug before the
> reset triggers.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Changes in v5:
> - Group bit values with register addresses
> - Address review comments from Gunter
> Changes in v4:
> - rebase against linux/master
> Changes in v2:
> - apply changes to armada-38x only
> 
>   drivers/watchdog/orion_wdt.c | 65 ++++++++++++++++++++++++++++--------
>   1 file changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index cdb0d174c5e2..5a23cb448ed5 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -35,7 +35,15 @@
>    * Watchdog timer block registers.
>    */
>   #define TIMER_CTRL		0x0000
> -#define TIMER_A370_STATUS	0x04
> +#define TIMER1_FIXED_ENABLE_BIT	BIT(12)
> +#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
> +#define TIMER1_ENABLE_BIT	BIT(2)
> +
> +#define TIMER_A370_STATUS	0x0004
> +#define WDT_A370_EXPIRED	BIT(31)
> +#define TIMER1_STATUS_BIT	BIT(8)
> +
> +#define TIMER1_VAL_OFF		0x001c
>   
>   #define WDT_MAX_CYCLE_COUNT	0xffffffff
>   
> @@ -43,9 +51,6 @@
>   #define WDT_A370_RATIO_SHIFT	5
>   #define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
>   
> -#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
> -#define WDT_A370_EXPIRED	BIT(31)
> -
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   static int heartbeat = -1;		/* module parameter (seconds) */
>   
> @@ -158,6 +163,7 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
>   				   struct orion_watchdog *dev)
>   {
>   	int ret;
> +	u32 val;
>   
>   	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
>   	if (IS_ERR(dev->clk))
> @@ -168,10 +174,9 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
>   		return ret;
>   	}
>   
> -	/* Enable the fixed watchdog clock input */
> -	atomic_io_modify(dev->reg + TIMER_CTRL,
> -			 WDT_AXP_FIXED_ENABLE_BIT,
> -			 WDT_AXP_FIXED_ENABLE_BIT);
> +	/* Fix the wdt and timer1 clock freqency to 25MHz */
> +	val = WDT_AXP_FIXED_ENABLE_BIT | TIMER1_FIXED_ENABLE_BIT;
> +	atomic_io_modify(dev->reg + TIMER_CTRL, val, val);
>   
>   	dev->clk_rate = clk_get_rate(dev->clk);
>   	return 0;
> @@ -183,6 +188,10 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
>   	/* Reload watchdog duration */
>   	writel(dev->clk_rate * wdt_dev->timeout,
>   	       dev->reg + dev->data->wdt_counter_offset);
> +	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
> +		writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
> +		       dev->reg + TIMER1_VAL_OFF);
> +
>   	return 0;
>   }
>   
> @@ -194,13 +203,18 @@ static int armada375_start(struct watchdog_device *wdt_dev)
>   	/* Set watchdog duration */
>   	writel(dev->clk_rate * wdt_dev->timeout,
>   	       dev->reg + dev->data->wdt_counter_offset);
> +	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
> +		writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
> +		       dev->reg + TIMER1_VAL_OFF);
>   
>   	/* Clear the watchdog expiration bit */
>   	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
>   
>   	/* Enable watchdog timer */
> -	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
> -						dev->data->wdt_enable_bit);
> +	reg = dev->data->wdt_enable_bit;
> +	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
> +		reg |= TIMER1_ENABLE_BIT;
> +	atomic_io_modify(dev->reg + TIMER_CTRL, reg, reg);
>   
>   	/* Enable reset on watchdog */
>   	reg = readl(dev->rstout);
> @@ -277,7 +291,7 @@ static int orion_stop(struct watchdog_device *wdt_dev)
>   static int armada375_stop(struct watchdog_device *wdt_dev)
>   {
>   	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> -	u32 reg;
> +	u32 reg, mask;
>   
>   	/* Disable reset on watchdog */
>   	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit,
> @@ -287,7 +301,10 @@ static int armada375_stop(struct watchdog_device *wdt_dev)
>   	writel(reg, dev->rstout);
>   
>   	/* Disable watchdog timer */
> -	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
> +	mask = dev->data->wdt_enable_bit;
> +	if (wdt_dev->info->options & WDIOF_PRETIMEOUT)
> +		mask &= ~TIMER1_ENABLE_BIT;

Sorry, I am lost. Why &= and ~ ?

Guenter

> +	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
>   
>   	return 0;
>   }
> @@ -349,7 +366,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
>   	return readl(dev->reg + dev->data->wdt_counter_offset) / dev->clk_rate;
>   }
>   
> -static const struct watchdog_info orion_wdt_info = {
> +static struct watchdog_info orion_wdt_info = {
>   	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>   	.identity = "Orion Watchdog",
>   };
> @@ -368,6 +385,16 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
>   	return IRQ_HANDLED;
>   }
>   
> +static irqreturn_t orion_wdt_pre_irq(int irq, void *devid)
> +{
> +	struct orion_watchdog *dev = devid;
> +
> +	atomic_io_modify(dev->reg + TIMER_A370_STATUS,
> +			 TIMER1_STATUS_BIT, 0);
> +	watchdog_notify_pretimeout(&dev->wdt);
> +	return IRQ_HANDLED;
> +}
> +
>   /*
>    * The original devicetree binding for this driver specified only
>    * one memory resource, so in order to keep DT backwards compatibility
> @@ -589,6 +616,18 @@ static int orion_wdt_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq > 0) {
> +		orion_wdt_info.options |= WDIOF_PRETIMEOUT;
> +		ret = devm_request_irq(&pdev->dev, irq, orion_wdt_pre_irq,
> +				       0, pdev->name, dev);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to request IRQ\n");
> +			goto disable_clk;
> +		}
> +	}
> +
> +
>   	watchdog_set_nowayout(&dev->wdt, nowayout);
>   	ret = watchdog_register_device(&dev->wdt);
>   	if (ret)
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v5] watchdog: orion_wdt: use timer1 as a pretimeout
  2019-08-29 12:46 ` Guenter Roeck
@ 2019-08-29 21:02   ` Chris Packham
  0 siblings, 0 replies; 3+ messages in thread
From: Chris Packham @ 2019-08-29 21:02 UTC (permalink / raw)
  To: linux, wim; +Cc: linux-kernel, linux-watchdog

On Thu, 2019-08-29 at 05:46 -0700, Guenter Roeck wrote:
> On 8/29/19 1:50 AM, Chris Packham wrote:
> > The orion watchdog can either reset the CPU or generate an interrupt.
> > The interrupt would be useful for debugging as it provides panic()
> > output about the watchdog expiry, however if the interrupt is used the
> > watchdog can't reset the CPU in the event of being stuck in a loop with
> > interrupts disabled or if the CPU is prevented from accessing memory
> > (e.g. an unterminated DMA).
> > 
> > The Armada SoCs have spare timers that aren't currently used by the
> > Linux kernel. We can use timer1 to provide a pre-timeout ahead of the
> > watchdog timer and provide the possibility of gathering debug before the
> > reset triggers.
> > 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

<snip>

> > @@ -277,7 +291,7 @@ static int orion_stop(struct watchdog_device *wdt_dev)
> >   static int armada375_stop(struct watchdog_device *wdt_dev)
> >   {
> >   	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> > -	u32 reg;
> > +	u32 reg, mask;
> >   
> >   	/* Disable reset on watchdog */
> >   	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit,
> > @@ -287,7 +301,10 @@ static int armada375_stop(struct watchdog_device *wdt_dev)
> >   	writel(reg, dev->rstout);
> >   
> >   	/* Disable watchdog timer */
> > -	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
> > +	mask = dev->data->wdt_enable_bit;
> > +	if (wdt_dev->info->options & WDIOF_PRETIMEOUT)
> > +		mask &= ~TIMER1_ENABLE_BIT;
> 
> Sorry, I am lost. Why &= and ~ ?

Blame late night coding.

I saw the lines above with 'reg &= ~dev->data->rstout_enable_bit' and
without the requisite level of caffine in my system my brain said "you
need to clear that bit". Which is exactly what the atomic_io_modify()
below would do if I actually gave it the right mask.

> Guenter
> 
> > +	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
> >   
> >   	return 0;
> >   }
> > @@ -349,7 +366,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
> >   	return readl(dev->reg + dev->data->wdt_counter_offset) / dev->clk_rate;
> >   }
> >   
> > -static const struct watchdog_info orion_wdt_info = {
> > +static struct watchdog_info orion_wdt_info = {
> >   	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> >   	.identity = "Orion Watchdog",
> >   };
> > @@ -368,6 +385,16 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
> >   	return IRQ_HANDLED;
> >   }
> >   
> > +static irqreturn_t orion_wdt_pre_irq(int irq, void *devid)
> > +{
> > +	struct orion_watchdog *dev = devid;
> > +
> > +	atomic_io_modify(dev->reg + TIMER_A370_STATUS,
> > +			 TIMER1_STATUS_BIT, 0);
> > +	watchdog_notify_pretimeout(&dev->wdt);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >   /*
> >    * The original devicetree binding for this driver specified only
> >    * one memory resource, so in order to keep DT backwards compatibility
> > @@ -589,6 +616,18 @@ static int orion_wdt_probe(struct platform_device *pdev)
> >   		}
> >   	}
> >   

I'll add a comment above the first platform_get_irq() about
intentionally not handling EPROBE_DEFER. And a comment here about the
2nd interrupt being optional.

> > +	irq = platform_get_irq(pdev, 1);
> > +	if (irq > 0) {
> > +		orion_wdt_info.options |= WDIOF_PRETIMEOUT;
> > +		ret = devm_request_irq(&pdev->dev, irq, orion_wdt_pre_irq,
> > +				       0, pdev->name, dev);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "failed to request IRQ\n");
> > +			goto disable_clk;
> > +		}
> > +	}
> > +
> > +
> >   	watchdog_set_nowayout(&dev->wdt, nowayout);
> >   	ret = watchdog_register_device(&dev->wdt);
> >   	if (ret)
> > 
> 
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-29  8:50 [PATCH v5] watchdog: orion_wdt: use timer1 as a pretimeout Chris Packham
2019-08-29 12:46 ` Guenter Roeck
2019-08-29 21:02   ` Chris Packham

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox