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

[Chris Packham]

On Thu, 2019-08-29 at 05:46 -0700, Guenter Roeck wrote:
> On 8/29/19 1:50 AM, 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).
> > 
> > The Armada SoCs 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@xxxxxxxxxxxxxxxxxxx>

<snip>

> > @@ -277,7 +291,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 +301,10 @@ 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;
> > +	if (wdt_dev->info->options & WDIOF_PRETIMEOUT)
> > +		mask &= ~TIMER1_ENABLE_BIT;
> 
> Sorry, I am lost. Why &= and ~ ?

Blame late night coding.

I saw the lines above with 'reg &= ~dev->data->rstout_enable_bit' and
without the requisite level of caffine in my system my brain said "you
need to clear that bit". Which is exactly what the atomic_io_modify()
below would do if I actually gave it the right mask.

> Guenter
> 
> > +	atomic_io_modify(dev->reg + TIMER_CTRL, mask, 0);
> >   
> >   	return 0;
> >   }
> > @@ -349,7 +366,7 @@ 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 const struct watchdog_info orion_wdt_info = {
> > +static struct watchdog_info orion_wdt_info = {
> >   	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> >   	.identity = "Orion Watchdog",
> >   };
> > @@ -368,6 +385,16 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
> >   	return IRQ_HANDLED;
> >   }
> >   
> > +static irqreturn_t orion_wdt_pre_irq(int irq, void *devid)
> > +{
> > +	struct orion_watchdog *dev = devid;
> > +
> > +	atomic_io_modify(dev->reg + TIMER_A370_STATUS,
> > +			 TIMER1_STATUS_BIT, 0);
> > +	watchdog_notify_pretimeout(&dev->wdt);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >   /*
> >    * The original devicetree binding for this driver specified only
> >    * one memory resource, so in order to keep DT backwards compatibility
> > @@ -589,6 +616,18 @@ static int orion_wdt_probe(struct platform_device *pdev)
> >   		}
> >   	}
> >   

I'll add a comment above the first platform_get_irq() about
intentionally not handling EPROBE_DEFER. And a comment here about the
2nd interrupt being optional.

> > +	irq = platform_get_irq(pdev, 1);
> > +	if (irq > 0) {
> > +		orion_wdt_info.options |= WDIOF_PRETIMEOUT;
> > +		ret = devm_request_irq(&pdev->dev, irq, orion_wdt_pre_irq,
> > +				       0, pdev->name, dev);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "failed to request IRQ\n");
> > +			goto disable_clk;
> > +		}
> > +	}
> > +
> > +
> >   	watchdog_set_nowayout(&dev->wdt, nowayout);
> >   	ret = watchdog_register_device(&dev->wdt);
> >   	if (ret)
> > 
> 
>