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.pack...@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); >>> >> >> > >