[PATCH 0/3] getting more output from orion_wdt

[PATCH 0/3] getting more output from orion_wdt

[Chris Packham]

On the Armada-38x platforms (and probably 37x/XP) the current behaviour of on
exipry of the watchdog is a silent reboot of the CPU. The Armada-38x does have
an interrupt which if we don't enable the Global WD bit in the RSTOUTn register
gives us a chance to generate some panic messages that may help track down the
cause of the watchdog timeout.

I've been a bit bold and enabled this in the generic armada-38x.dtsi file. I'd
be happy to leave that part out and have the interrupt enabled on a
board-by-board basis if there are objections.

Chris Packham (3):
  watchdog: orion: fix typo
  watchdog: orion: don't enable rstout if an interrupt is configured
  ARM: mvebu: dts: connect interrupt for WD on armada-38x

 arch/arm/boot/dts/armada-38x.dtsi |  1 +
 drivers/watchdog/orion_wdt.c      | 27 ++++++++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.14.2

[PATCH 1/3] watchdog: orion: fix typo

[Chris Packham]

Correct typo in comment "insterted" -> "inserted".

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/watchdog/orion_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 83af7d6cc37c..ea676d233e1e 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -576,7 +576,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	/*
 	 * Let's make sure the watchdog is fully stopped, unless it's
 	 * explicitly enabled. This may be the case if the module was
-	 * removed and re-insterted, or if the bootloader explicitly
+	 * removed and re-inserted, or if the bootloader explicitly
 	 * set a running watchdog before booting the kernel.
 	 */
 	if (!orion_wdt_enabled(&dev->wdt))
-- 
2.14.2

[PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

[Chris Packham]

The orion_wdt_irq invokes panic() so we are going to reset the CPU
regardless.  By not setting this bit we get a chance to gather debug
from the panic output before the system is reset.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/watchdog/orion_wdt.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index ea676d233e1e..ce88f339ef7f 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -71,6 +71,7 @@ struct orion_watchdog {
 	unsigned long clk_rate;
 	struct clk *clk;
 	const struct orion_watchdog_data *data;
+	int irq;
 };
 
 static int orion_wdt_clock_init(struct platform_device *pdev,
@@ -203,9 +204,11 @@ static int armada375_start(struct watchdog_device *wdt_dev)
 						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	reg = readl(dev->rstout);
-	reg |= dev->data->rstout_enable_bit;
-	writel(reg, dev->rstout);
+	if (!dev->irq) {
+		reg = readl(dev->rstout);
+		reg |= dev->data->rstout_enable_bit;
+		writel(reg, dev->rstout);
+	}
 
 	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
 	return 0;
@@ -228,9 +231,12 @@ static int armada370_start(struct watchdog_device *wdt_dev)
 						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	reg = readl(dev->rstout);
-	reg |= dev->data->rstout_enable_bit;
-	writel(reg, dev->rstout);
+	if (!dev->irq) {
+		reg = readl(dev->rstout);
+		reg |= dev->data->rstout_enable_bit;
+		writel(reg, dev->rstout);
+	}
+
 	return 0;
 }
 
@@ -247,8 +253,9 @@ static int orion_start(struct watchdog_device *wdt_dev)
 						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
-				      dev->data->rstout_enable_bit);
+	if (!dev->irq)
+		atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+					      dev->data->rstout_enable_bit);
 
 	return 0;
 }
@@ -595,6 +602,8 @@ static int orion_wdt_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "failed to request IRQ\n");
 			goto disable_clk;
 		}
+
+		dev->irq = irq;
 	}
 
 	watchdog_set_nowayout(&dev->wdt, nowayout);
-- 
2.14.2

[PATCH 3/3] ARM: mvebu: dts: connect interrupt for WD on armada-38x

[Chris Packham]

The watchdog timer interrupt is available internally on the Armada-38x
SoCs. Connect it so that we can have the orion_wdt_irq generate debug
information when a watchdog timeout occurs.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm/boot/dts/armada-38x.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index 7ff0811e61db..9ac76c54c9e5 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -408,6 +408,7 @@
 				reg = <0x20300 0x34>, <0x20704 0x4>, <0x18260 0x4>;
 				clocks = <&coreclk 2>, <&refclk>;
 				clock-names = "nbclk", "fixed";
+				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			cpurst: cpurst@20800 {
-- 
2.14.2

Re: [PATCH 1/3] watchdog: orion: fix typo

[Guenter Roeck]

On 10/10/2017 07:29 PM, Chris Packham wrote:
> Correct typo in comment "insterted" -> "inserted".
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/orion_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 83af7d6cc37c..ea676d233e1e 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -576,7 +576,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
>   	/*
>   	 * Let's make sure the watchdog is fully stopped, unless it's
>   	 * explicitly enabled. This may be the case if the module was
> -	 * removed and re-insterted, or if the bootloader explicitly
> +	 * removed and re-inserted, or if the bootloader explicitly
>   	 * set a running watchdog before booting the kernel.
>   	 */
>   	if (!orion_wdt_enabled(&dev->wdt))
> 

Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

[Guenter Roeck]

On 10/10/2017 07:29 PM, Chris Packham wrote:
> The orion_wdt_irq invokes panic() so we are going to reset the CPU
> regardless.  By not setting this bit we get a chance to gather debug
> from the panic output before the system is reset.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Unless I am missing something, this assumes that the interrupt is
handled, ie that the system is not stuck with interrupts disabled.
This makes the watchdog less reliable. This added verbosity comes
at a significant cost. I'd like to get input from others if this
is acceptable.

That would be different if there was a means to configure a pretimeout,
ie a means to tell the system to generate an irq first, followed by a
hard reset if the interrupt is not served. that does not seem to be
the case here, though.

Guenter

> ---
>   drivers/watchdog/orion_wdt.c | 25 +++++++++++++++++--------
>   1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index ea676d233e1e..ce88f339ef7f 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -71,6 +71,7 @@ struct orion_watchdog {
>   	unsigned long clk_rate;
>   	struct clk *clk;
>   	const struct orion_watchdog_data *data;
> +	int irq;
>   };
>   
>   static int orion_wdt_clock_init(struct platform_device *pdev,
> @@ -203,9 +204,11 @@ static int armada375_start(struct watchdog_device *wdt_dev)
>   						dev->data->wdt_enable_bit);
>   
>   	/* Enable reset on watchdog */
> -	reg = readl(dev->rstout);
> -	reg |= dev->data->rstout_enable_bit;
> -	writel(reg, dev->rstout);
> +	if (!dev->irq) {
> +		reg = readl(dev->rstout);
> +		reg |= dev->data->rstout_enable_bit;
> +		writel(reg, dev->rstout);
> +	}
>   
>   	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
>   	return 0;
> @@ -228,9 +231,12 @@ static int armada370_start(struct watchdog_device *wdt_dev)
>   						dev->data->wdt_enable_bit);
>   
>   	/* Enable reset on watchdog */
> -	reg = readl(dev->rstout);
> -	reg |= dev->data->rstout_enable_bit;
> -	writel(reg, dev->rstout);
> +	if (!dev->irq) {
> +		reg = readl(dev->rstout);
> +		reg |= dev->data->rstout_enable_bit;
> +		writel(reg, dev->rstout);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -247,8 +253,9 @@ static int orion_start(struct watchdog_device *wdt_dev)
>   						dev->data->wdt_enable_bit);
>   
>   	/* Enable reset on watchdog */
> -	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
> -				      dev->data->rstout_enable_bit);
> +	if (!dev->irq)
> +		atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
> +					      dev->data->rstout_enable_bit);
>   
>   	return 0;
>   }
> @@ -595,6 +602,8 @@ static int orion_wdt_probe(struct platform_device *pdev)
>   			dev_err(&pdev->dev, "failed to request IRQ\n");
>   			goto disable_clk;
>   		}
> +
> +		dev->irq = irq;
>   	}
>   
>   	watchdog_set_nowayout(&dev->wdt, nowayout);
> 

Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

[Chris Packham]

On 11/10/17 16:42, Guenter Roeck wrote:
> On 10/10/2017 07:29 PM, Chris Packham wrote:
>> The orion_wdt_irq invokes panic() so we are going to reset the CPU
>> regardless.  By not setting this bit we get a chance to gather debug
>> from the panic output before the system is reset.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> 
> Unless I am missing something, this assumes that the interrupt is
> handled, ie that the system is not stuck with interrupts disabled.
> This makes the watchdog less reliable. This added verbosity comes
> at a significant cost. I'd like to get input from others if this
> is acceptable.
> 
> That would be different if there was a means to configure a pretimeout,
> ie a means to tell the system to generate an irq first, followed by a
> hard reset if the interrupt is not served. that does not seem to be
> the case here, though.
> 

As far as I can tell there is no pretimeout capability in the orion 
/armada WDT. I have got a work-in-progress patch that enables the RSTOUT 
in the interrupt handler which some-what mitigates your concern but 
still requires the interrupt to be handled at least once.

Another option would be to use one of the spare global timers to provide 
the interrupt-driven aspect.

Of course if the irq isn't specified then the existing behaviour is 
retained which would make the 3/3 patch of this series the debatable part.


> Guenter
> 
>> ---
>>    drivers/watchdog/orion_wdt.c | 25 +++++++++++++++++--------
>>    1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
>> index ea676d233e1e..ce88f339ef7f 100644
>> --- a/drivers/watchdog/orion_wdt.c
>> +++ b/drivers/watchdog/orion_wdt.c
>> @@ -71,6 +71,7 @@ struct orion_watchdog {
>>    	unsigned long clk_rate;
>>    	struct clk *clk;
>>    	const struct orion_watchdog_data *data;
>> +	int irq;
>>    };
>>    
>>    static int orion_wdt_clock_init(struct platform_device *pdev,
>> @@ -203,9 +204,11 @@ static int armada375_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	reg = readl(dev->rstout);
>> -	reg |= dev->data->rstout_enable_bit;
>> -	writel(reg, dev->rstout);
>> +	if (!dev->irq) {
>> +		reg = readl(dev->rstout);
>> +		reg |= dev->data->rstout_enable_bit;
>> +		writel(reg, dev->rstout);
>> +	}
>>    
>>    	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
>>    	return 0;
>> @@ -228,9 +231,12 @@ static int armada370_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	reg = readl(dev->rstout);
>> -	reg |= dev->data->rstout_enable_bit;
>> -	writel(reg, dev->rstout);
>> +	if (!dev->irq) {
>> +		reg = readl(dev->rstout);
>> +		reg |= dev->data->rstout_enable_bit;
>> +		writel(reg, dev->rstout);
>> +	}
>> +
>>    	return 0;
>>    }
>>    
>> @@ -247,8 +253,9 @@ static int orion_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>> -				      dev->data->rstout_enable_bit);
>> +	if (!dev->irq)
>> +		atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>> +					      dev->data->rstout_enable_bit);
>>    
>>    	return 0;
>>    }
>> @@ -595,6 +602,8 @@ static int orion_wdt_probe(struct platform_device *pdev)
>>    			dev_err(&pdev->dev, "failed to request IRQ\n");
>>    			goto disable_clk;
>>    		}
>> +
>> +		dev->irq = irq;
>>    	}
>>    
>>    	watchdog_set_nowayout(&dev->wdt, nowayout);
>>
> 
> 

Re: [PATCH 3/3] ARM: mvebu: dts: connect interrupt for WD on armada-38x

[Gregory CLEMENT]

Hi Chris,
 
 On mer., oct. 11 2017, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> The watchdog timer interrupt is available internally on the Armada-38x
> SoCs. Connect it so that we can have the orion_wdt_irq generate debug
> information when a watchdog timeout occurs.

Given the patch 2, when an interrupt is provided by the dt, then the
behavior of the watchdog changes. So we can't do it for all the
platform.

As you propose on your cover letter it is better to set it at board
level.

So instead of this patch you should update the binding documentation
with this new resources. Thanks to this each board developer will be
able to decide to use this feature or not.

Thanks,

Gregory

>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm/boot/dts/armada-38x.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
> index 7ff0811e61db..9ac76c54c9e5 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
> @@ -408,6 +408,7 @@
>  				reg = <0x20300 0x34>, <0x20704 0x4>, <0x18260 0x4>;
>  				clocks = <&coreclk 2>, <&refclk>;
>  				clock-names = "nbclk", "fixed";
> +				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  
>  			cpurst: cpurst@20800 {
> -- 
> 2.14.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/3] ARM: mvebu: dts: connect interrupt for WD on armada-38x

[Chris Packham]

On 13/10/17 03:17, Gregory CLEMENT wrote:
> Hi Chris,
>   
>   On mer., oct. 11 2017, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
>> The watchdog timer interrupt is available internally on the Armada-38x
>> SoCs. Connect it so that we can have the orion_wdt_irq generate debug
>> information when a watchdog timeout occurs.
> 
> Given the patch 2, when an interrupt is provided by the dt, then the
> behavior of the watchdog changes. So we can't do it for all the
> platform.
> 
> As you propose on your cover letter it is better to set it at board
> level.
> 
> So instead of this patch you should update the binding documentation
> with this new resources. Thanks to this each board developer will be
> able to decide to use this feature or not.

Agreed. I'll drop this patch in favor of a documentation update in v2.

> 
> Thanks,
> 
> Gregory
> 
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   arch/arm/boot/dts/armada-38x.dtsi | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
>> index 7ff0811e61db..9ac76c54c9e5 100644
>> --- a/arch/arm/boot/dts/armada-38x.dtsi
>> +++ b/arch/arm/boot/dts/armada-38x.dtsi
>> @@ -408,6 +408,7 @@
>>   				reg = <0x20300 0x34>, <0x20704 0x4>, <0x18260 0x4>;
>>   				clocks = <&coreclk 2>, <&refclk>;
>>   				clock-names = "nbclk", "fixed";
>> +				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>>   			};
>>   
>>   			cpurst: cpurst@20800 {
>> -- 
>> 2.14.2
>>
> 

Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

[Chris Packham]

Hijacking an old thread,

On 11/10/17 5:30 PM, Chris Packham wrote:
> On 11/10/17 16:42, Guenter Roeck wrote:
>> On 10/10/2017 07:29 PM, Chris Packham wrote:
>>> The orion_wdt_irq invokes panic() so we are going to reset the CPU
>>> regardless.  By not setting this bit we get a chance to gather debug
>>> from the panic output before the system is reset.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>
>> Unless I am missing something, this assumes that the interrupt is
>> handled, ie that the system is not stuck with interrupts disabled.
>> This makes the watchdog less reliable. This added verbosity comes
>> at a significant cost. I'd like to get input from others if this
>> is acceptable.
>>
>> That would be different if there was a means to configure a pretimeout,
>> ie a means to tell the system to generate an irq first, followed by a
>> hard reset if the interrupt is not served. that does not seem to be
>> the case here, though.
>>
> 
> As far as I can tell there is no pretimeout capability in the orion
> /armada WDT. I have got a work-in-progress patch that enables the RSTOUT
> in the interrupt handler which some-what mitigates your concern but
> still requires the interrupt to be handled at least once.
> 
> Another option would be to use one of the spare global timers to provide
> the interrupt-driven aspect.

So as Guenter rightly pointed out my original patch meant that we'd hang 
if we got stuck in a loop with interrupts disabled. Shortly after that 
that's exactly what happened on my test setup.

I've now been able to follow-up on the idea of using one of the spare 
global timers as a pretimeout indication and it's looking pretty 
promising. The patch is below (beware MUA wrapping). I'll submit it 
properly but I'm wondering if I should add the timer1 based watchdog as 
a separate watchdog device or like I have below roll it into the 
existing watchdog device.

--- 8< ---
diff --git a/arch/arm/boot/dts/armada-38x.dtsi 
b/arch/arm/boot/dts/armada-38x.dtsi
index 8530d3594ca2..abd45adb4728 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -421,6 +421,7 @@
                                 reg = <0x20300 0x34>, <0x20704 0x4>, 
<0x18260 0x4>;
                                 clocks = <&coreclk 2>, <&refclk>;
                                 clock-names = "nbclk", "fixed";
+                               interrupts-extended = <&gic GIC_SPI 9 
IRQ_TYPE_LEVEL_HIGH>;
                         };

                         cpurst: cpurst@20800 {
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index c6b8f4a43bde..89eae53b1b17 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -37,13 +37,16 @@
  #define TIMER_CTRL             0x0000
  #define TIMER_A370_STATUS      0x04

+#define TIMER1_RELOAD_OFF      0x0018
+#define TIMER1_VAL_OFF         0x001c
+
  #define WDT_MAX_CYCLE_COUNT    0xffffffff

  #define WDT_A370_RATIO_MASK(v) ((v) << 16)
  #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_AXP_FIXED_ENABLE_BIT BIT(10) | BIT(12) 
 
 

  #define WDT_A370_EXPIRED       BIT(31) 
 
 

 
 
 

  static bool nowayout = WATCHDOG_NOWAYOUT; 
 
 

@@ -183,6 +186,9 @@ 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); 
 
 

+       writel(dev->clk_rate * (wdt_dev->timeout/2), 
 
 

+              dev->reg + TIMER1_VAL_OFF); 
 
 

+ 
 
 

         return 0; 
 
 

  } 
 
 

 
 
 

@@ -194,6 +200,8 @@ 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); 
 
 

+       writel(dev->clk_rate * (wdt_dev->timeout/2), 
 
 

+              dev->reg + TIMER1_VAL_OFF); 
 
 

 
 
 

         /* Clear the watchdog expiration bit */ 
 
 

         atomic_io_modify(dev->reg + TIMER_A370_STATUS, 
WDT_A370_EXPIRED, 0); 
 

@@ -443,7 +451,7 @@ static const struct orion_watchdog_data 
armada375_data = { 
 

  static const struct orion_watchdog_data armada380_data = { 
 
 

         .rstout_enable_bit = BIT(8), 
 
 

         .rstout_mask_bit = BIT(10), 
 
 

-       .wdt_enable_bit = BIT(8), 
 
 

+       .wdt_enable_bit = BIT(8) | BIT(2), 
 
 

         .wdt_counter_offset = 0x34, 
 
 

         .clock_init = armadaxp_wdt_clock_init, 
 
 

         .enabled = armada375_enabled,
--- 8< ---

> 
> Of course if the irq isn't specified then the existing behaviour is
> retained which would make the 3/3 patch of this series the debatable part.
> 
> 
>> Guenter
>>
>>> ---
>>>     drivers/watchdog/orion_wdt.c | 25 +++++++++++++++++--------
>>>     1 file changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
>>> index ea676d233e1e..ce88f339ef7f 100644
>>> --- a/drivers/watchdog/orion_wdt.c
>>> +++ b/drivers/watchdog/orion_wdt.c
>>> @@ -71,6 +71,7 @@ struct orion_watchdog {
>>>     	unsigned long clk_rate;
>>>     	struct clk *clk;
>>>     	const struct orion_watchdog_data *data;
>>> +	int irq;
>>>     };
>>>     
>>>     static int orion_wdt_clock_init(struct platform_device *pdev,
>>> @@ -203,9 +204,11 @@ static int armada375_start(struct watchdog_device *wdt_dev)
>>>     						dev->data->wdt_enable_bit);
>>>     
>>>     	/* Enable reset on watchdog */
>>> -	reg = readl(dev->rstout);
>>> -	reg |= dev->data->rstout_enable_bit;
>>> -	writel(reg, dev->rstout);
>>> +	if (!dev->irq) {
>>> +		reg = readl(dev->rstout);
>>> +		reg |= dev->data->rstout_enable_bit;
>>> +		writel(reg, dev->rstout);
>>> +	}
>>>     
>>>     	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
>>>     	return 0;
>>> @@ -228,9 +231,12 @@ static int armada370_start(struct watchdog_device *wdt_dev)
>>>     						dev->data->wdt_enable_bit);
>>>     
>>>     	/* Enable reset on watchdog */
>>> -	reg = readl(dev->rstout);
>>> -	reg |= dev->data->rstout_enable_bit;
>>> -	writel(reg, dev->rstout);
>>> +	if (!dev->irq) {
>>> +		reg = readl(dev->rstout);
>>> +		reg |= dev->data->rstout_enable_bit;
>>> +		writel(reg, dev->rstout);
>>> +	}
>>> +
>>>     	return 0;
>>>     }
>>>     
>>> @@ -247,8 +253,9 @@ static int orion_start(struct watchdog_device *wdt_dev)
>>>     						dev->data->wdt_enable_bit);
>>>     
>>>     	/* Enable reset on watchdog */
>>> -	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>>> -				      dev->data->rstout_enable_bit);
>>> +	if (!dev->irq)
>>> +		atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>>> +					      dev->data->rstout_enable_bit);
>>>     
>>>     	return 0;
>>>     }
>>> @@ -595,6 +602,8 @@ static int orion_wdt_probe(struct platform_device *pdev)
>>>     			dev_err(&pdev->dev, "failed to request IRQ\n");
>>>     			goto disable_clk;
>>>     		}
>>> +
>>> +		dev->irq = irq;
>>>     	}
>>>     
>>>     	watchdog_set_nowayout(&dev->wdt, nowayout);
>>>
>>
>>
> 
> 

Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

[Guenter Roeck]

On Wed, Feb 27, 2019 at 08:59:07PM +0000, Chris Packham wrote:
> Hijacking an old thread,
> 
> On 11/10/17 5:30 PM, Chris Packham wrote:
> > On 11/10/17 16:42, Guenter Roeck wrote:
> >> On 10/10/2017 07:29 PM, Chris Packham wrote:
> >>> The orion_wdt_irq invokes panic() so we are going to reset the CPU
> >>> regardless.  By not setting this bit we get a chance to gather debug
> >>> from the panic output before the system is reset.
> >>>
> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>
> >> Unless I am missing something, this assumes that the interrupt is
> >> handled, ie that the system is not stuck with interrupts disabled.
> >> This makes the watchdog less reliable. This added verbosity comes
> >> at a significant cost. I'd like to get input from others if this
> >> is acceptable.
> >>
> >> That would be different if there was a means to configure a pretimeout,
> >> ie a means to tell the system to generate an irq first, followed by a
> >> hard reset if the interrupt is not served. that does not seem to be
> >> the case here, though.
> >>
> > 
> > As far as I can tell there is no pretimeout capability in the orion
> > /armada WDT. I have got a work-in-progress patch that enables the RSTOUT
> > in the interrupt handler which some-what mitigates your concern but
> > still requires the interrupt to be handled at least once.
> > 
> > Another option would be to use one of the spare global timers to provide
> > the interrupt-driven aspect.
> 
> So as Guenter rightly pointed out my original patch meant that we'd hang 
> if we got stuck in a loop with interrupts disabled. Shortly after that 
> that's exactly what happened on my test setup.
> 
> I've now been able to follow-up on the idea of using one of the spare 
> global timers as a pretimeout indication and it's looking pretty 
> promising. The patch is below (beware MUA wrapping). I'll submit it 
> properly but I'm wondering if I should add the timer1 based watchdog as 
> a separate watchdog device or like I have below roll it into the 
> existing watchdog device.
> 

Why not use a pretimeout ? Isn't this exactly what you are doing,
with a fixed pretimeout of timeout / 2 ?

Guenter

[PATCH 0/2] watchdog: orion_wdt: add pretimeout support

[Chris Packham]

This is a follow up from https://lore.kernel.org/lkml/20190227230707.GA28635@roeck-us.net/

It appears that the kernel has only ever used timer0 on the
orion/kirkwood/armada platforms so timer1 appears to be spare on all of
these. Armada-XP and Armada-385 also have timers 2 and 3 which could be
used in a similar way if it is deemed that timer1 should be kept for
some other purpose.

Chris Packham (2):
  watchdog: orion: remove orion_wdt_set_timeout
  watchdog: orion_wdt: use timer1 as a pretimeout

 arch/arm/boot/dts/armada-38x.dtsi |  1 +
 drivers/watchdog/orion_wdt.c      | 66 ++++++++++++++++++-------------
 2 files changed, 39 insertions(+), 28 deletions(-)

-- 
2.21.0

[PATCH 1/2] watchdog: orion_wdt: remove orion_wdt_set_timeout

[Chris Packham]

The watchdog core will do the same thing if no set_timeout
is supplied so we can safely remove orion_wdt_set_timeout.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/watchdog/orion_wdt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 9db3b09f7568..8b259c712c52 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -349,13 +349,6 @@ 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 int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,
-				 unsigned int timeout)
-{
-	wdt_dev->timeout = timeout;
-	return 0;
-}
-
 static const struct watchdog_info orion_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "Orion Watchdog",
@@ -366,7 +359,6 @@ static const struct watchdog_ops orion_wdt_ops = {
 	.start = orion_wdt_start,
 	.stop = orion_wdt_stop,
 	.ping = orion_wdt_ping,
-	.set_timeout = orion_wdt_set_timeout,
 	.get_timeleft = orion_wdt_get_timeleft,
 };
 
-- 
2.21.0

[PATCH 2/2] watchdog: orion_wdt: use timer1 as a pretimeout

[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).

All of the orion based CPU cores (at least back as far as Kirkwood) 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>
---
 arch/arm/boot/dts/armada-38x.dtsi |  1 +
 drivers/watchdog/orion_wdt.c      | 58 ++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index 929459c42760..fd0caa9714f2 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -376,6 +376,7 @@
 				reg = <0x20300 0x34>, <0x20704 0x4>, <0x18260 0x4>;
 				clocks = <&coreclk 2>, <&refclk>;
 				clock-names = "nbclk", "fixed";
+				interrupts-extended = <&gic  GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			cpurst: cpurst@20800 {
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 8b259c712c52..bf1dc75c2045 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -46,6 +46,10 @@
 #define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 #define WDT_A370_EXPIRED	BIT(31)
 
+#define TIMER1_VAL_OFF		0x001c
+#define TIMER1_ENABLE_BIT	BIT(2)
+#define TIMER1_FIXED_ENABLE_BIT	BIT(12)
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
@@ -118,6 +122,7 @@ static int armada375_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)) {
@@ -127,9 +132,8 @@ static int armada375_wdt_clock_init(struct platform_device *pdev,
 			return ret;
 		}
 
-		atomic_io_modify(dev->reg + TIMER_CTRL,
-				WDT_AXP_FIXED_ENABLE_BIT,
-				WDT_AXP_FIXED_ENABLE_BIT);
+		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;
@@ -158,6 +162,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))
@@ -169,38 +174,46 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 	}
 
 	/* Enable the fixed watchdog clock input */
-	atomic_io_modify(dev->reg + TIMER_CTRL,
-			 WDT_AXP_FIXED_ENABLE_BIT,
-			 WDT_AXP_FIXED_ENABLE_BIT);
+	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;
 }
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
 	/* Reload watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
 	       dev->reg + dev->data->wdt_counter_offset);
+	writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
+	       dev->reg + TIMER1_VAL_OFF);
+
 	return 0;
 }
 
 static int armada375_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	u32 reg;
+	u32 reg, val;
 
 	/* Set watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
 	       dev->reg + dev->data->wdt_counter_offset);
+	if (wdt_dev->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);
+	val = dev->data->wdt_enable_bit | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, val, val);
 
 	/* Enable reset on watchdog */
 	reg = readl(dev->rstout);
@@ -214,7 +227,7 @@ static int armada375_start(struct watchdog_device *wdt_dev)
 static int armada370_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	u32 reg;
+	u32 reg, val;
 
 	/* Set watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
@@ -224,8 +237,8 @@ static int armada370_start(struct watchdog_device *wdt_dev)
 	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);
+	val = dev->data->wdt_enable_bit | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, val, val);
 
 	/* Enable reset on watchdog */
 	reg = readl(dev->rstout);
@@ -237,14 +250,15 @@ static int armada370_start(struct watchdog_device *wdt_dev)
 static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+	u32 val;
 
 	/* Set watchdog duration */
 	writel(dev->clk_rate * wdt_dev->timeout,
 	       dev->reg + dev->data->wdt_counter_offset);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
-						dev->data->wdt_enable_bit);
+	val = dev->data->wdt_enable_bit | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, val, val);
 
 	/* Enable reset on watchdog */
 	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
@@ -264,12 +278,14 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 static int orion_stop(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+	u32 mask;
 
 	/* Disable reset on watchdog */
 	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
+	mask = dev->data->wdt_enable_bit | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
 
 	return 0;
 }
@@ -277,7 +293,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 +303,8 @@ 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 | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
 
 	return 0;
 }
@@ -295,7 +312,7 @@ static int armada375_stop(struct watchdog_device *wdt_dev)
 static int armada370_stop(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	u32 reg;
+	u32 reg, mask;
 
 	/* Disable reset on watchdog */
 	reg = readl(dev->rstout);
@@ -303,7 +320,8 @@ static int armada370_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 | TIMER1_ENABLE_BIT;
+	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
 
 	return 0;
 }
@@ -350,7 +368,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 }
 
 static const struct watchdog_info orion_wdt_info = {
-	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
 	.identity = "Orion Watchdog",
 };
 
-- 
2.21.0

Re: [PATCH 2/2] watchdog: orion_wdt: use timer1 as a pretimeout

[Andrew Lunn]

On Tue, Mar 05, 2019 at 11:51:52AM +1300, 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).
> 
> All of the orion based CPU cores (at least back as far as Kirkwood) 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.

Hi Chris

I had a quick look at other drivers implementing pre-timeout. They
seem to call watchdog_notify_pretimeout(). I don't see that here? What
happens when timer1 fires?

> @@ -169,38 +174,46 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
>  	}
>  
>  	/* Enable the fixed watchdog clock input */
> -	atomic_io_modify(dev->reg + TIMER_CTRL,
> -			 WDT_AXP_FIXED_ENABLE_BIT,
> -			 WDT_AXP_FIXED_ENABLE_BIT);
> +	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);
> +
> +

One blank line is sufficient,


>  	return 0;
>  }

   Andrew

Re: [PATCH 2/2] watchdog: orion_wdt: use timer1 as a pretimeout

[Chris Packham]

On 5/03/19 1:57 PM, Andrew Lunn wrote:
> On Tue, Mar 05, 2019 at 11:51:52AM +1300, 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).
>>
>> All of the orion based CPU cores (at least back as far as Kirkwood) have
>> spare timers that aren't currently used by the Linux kernel.

Actually this appears to be incorrect Kirkwood does configure timer1 as 
a clockevent timer. So I can't just grab timer1 for all platforms.

>> 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.
> 
> Hi Chris
> 
> I had a quick look at other drivers implementing pre-timeout. They
> seem to call watchdog_notify_pretimeout(). I don't see that here? What
> happens when timer1 fires?
> 

It invokes the regular orion_wdt_irq(). On Armada-385 prior to this 
change the irq was not specified because the reset always kicked in so 
there was no point.

For correctness I could make the devicetree binding specify 2 
interrupts. One for the regular watchdog interrupt (which would never 
usually get hit because the reset would kick in) and one for the 
pretimeout/timer1.

>> @@ -169,38 +174,46 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
>>   	}
>>   
>>   	/* Enable the fixed watchdog clock input */
>> -	atomic_io_modify(dev->reg + TIMER_CTRL,
>> -			 WDT_AXP_FIXED_ENABLE_BIT,
>> -			 WDT_AXP_FIXED_ENABLE_BIT);
>> +	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);
>> +
>> +
> 
> One blank line is sufficient,
> 
> 
>>   	return 0;
>>   }
> 
>     Andrew
> 

Re: [PATCH 2/2] watchdog: orion_wdt: use timer1 as a pretimeout

[Andrew Lunn]

> > Hi Chris
> > 
> > I had a quick look at other drivers implementing pre-timeout. They
> > seem to call watchdog_notify_pretimeout(). I don't see that here? What
> > happens when timer1 fires?
> > 
> 
> It invokes the regular orion_wdt_irq(). On Armada-385 prior to this 
> change the irq was not specified because the reset always kicked in so 
> there was no point.
> 
> For correctness I could make the devicetree binding specify 2 
> interrupts. One for the regular watchdog interrupt (which would never 
> usually get hit because the reset would kick in) and one for the 
> pretimeout/timer1.

Hi Chris

If the regular watchdog interrupt would never actually fire because
the SoC gets reset, maybe make the IRQ handler call
watchdog_notify_pretimeout()?

	Andrew

Re: [PATCH 1/2] watchdog: orion_wdt: remove orion_wdt_set_timeout

[Guenter Roeck]

On Tue, Mar 05, 2019 at 11:51:51AM +1300, Chris Packham wrote:
> The watchdog core will do the same thing if no set_timeout
> is supplied so we can safely remove orion_wdt_set_timeout.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/orion_wdt.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 9db3b09f7568..8b259c712c52 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -349,13 +349,6 @@ 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 int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,
> -				 unsigned int timeout)
> -{
> -	wdt_dev->timeout = timeout;
> -	return 0;
> -}
> -
>  static const struct watchdog_info orion_wdt_info = {
>  	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>  	.identity = "Orion Watchdog",
> @@ -366,7 +359,6 @@ static const struct watchdog_ops orion_wdt_ops = {
>  	.start = orion_wdt_start,
>  	.stop = orion_wdt_stop,
>  	.ping = orion_wdt_ping,
> -	.set_timeout = orion_wdt_set_timeout,
>  	.get_timeleft = orion_wdt_get_timeleft,
>  };
>  
> -- 
> 2.21.0
> 

Re: [PATCH 2/2] watchdog: orion_wdt: use timer1 as a pretimeout

[Guenter Roeck]

On Tue, Mar 05, 2019 at 01:26:08AM +0000, Chris Packham wrote:
> On 5/03/19 1:57 PM, Andrew Lunn wrote:
> > On Tue, Mar 05, 2019 at 11:51:52AM +1300, 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).
> >>
> >> All of the orion based CPU cores (at least back as far as Kirkwood) have
> >> spare timers that aren't currently used by the Linux kernel.
> 
> Actually this appears to be incorrect Kirkwood does configure timer1 as 
> a clockevent timer. So I can't just grab timer1 for all platforms.
> 
If you can't use it unconditionally, can you specify it (and use it)
as clock ?

> >> 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.
> > 
> > Hi Chris
> > 
> > I had a quick look at other drivers implementing pre-timeout. They
> > seem to call watchdog_notify_pretimeout(). I don't see that here? What
> > happens when timer1 fires?
> > 
> 
> It invokes the regular orion_wdt_irq(). On Armada-385 prior to this 
> change the irq was not specified because the reset always kicked in so 
> there was no point.
> 

I would suggest to update that function to actually call
watchdog_notify_pretimeout() if a pretimeout is configured configured.
After all, we do want to support the infrastructure, and that includes
support for the various pretimeout governors (if enabled).

> For correctness I could make the devicetree binding specify 2 
> interrupts. One for the regular watchdog interrupt (which would never 
> usually get hit because the reset would kick in) and one for the 
> pretimeout/timer1.
> 
Yes, if they are different interrupts and orion_wdt_irq() is only supposed
to handle the real timeout.

Thanks,
Guenter

> >> @@ -169,38 +174,46 @@ static int armadaxp_wdt_clock_init(struct platform_device *pdev,
> >>   	}
> >>   
> >>   	/* Enable the fixed watchdog clock input */
> >> -	atomic_io_modify(dev->reg + TIMER_CTRL,
> >> -			 WDT_AXP_FIXED_ENABLE_BIT,
> >> -			 WDT_AXP_FIXED_ENABLE_BIT);
> >> +	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);
> >> +
> >> +
> > 
> > One blank line is sufficient,
> > 
> > 
> >>   	return 0;
> >>   }
> > 
> >     Andrew
> > 
>