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@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.

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