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

[Chris Packham <Chris.Packham@alliedtelesis.co.nz>]

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