Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured
From: Chris Packham
Date: Wed Feb 27 2019 - 15:59:13 EST
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@xxxxxxxxxxxxxxxxxxx>
>>
>> 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);
>>>
>>
>>
>
>