RE: [PATCH v3 1/2] watchdog: Add watchdog driver for Intel Keembay Soc

["Ayyathurai, Vijayakannan" <vijayakannan.ayyathurai@xxxxxxxxx>]

Hi Guenter,

> From: Guenter Roeck <linux@xxxxxxxxxxxx>
> Sent: Wednesday, 2 December, 2020 12:31 AM
> 
> On Tue, Dec 01, 2020 at 11:10:33PM +0800,
> vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
> >
> > Intel Keembay Soc requires watchdog timer support.
> > Add watchdog driver to enable this.
> >
> > +static void keembay_wdt_set_timeout_reg(struct watchdog_device *wdog,
> bool ping)
> > +{
> > +	struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > +	u32 th_val = 0;
> > +
> > +	if (!ping && wdog->pretimeout) {
> > +		th_val = wdog->timeout - wdog->pretimeout;
> > +		keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val
> * wdt->rate);
> 
> Sorry for annoying you now, but I may have found another potential problem.
> 
> What happens if the user sets a pretimeout, then removes it ?
> What should TIM_WATCHDOG_INT_THRES be set to in that case ?
> Right now TIM_WATCHDOG_INT_THRES won't be updated anymore

It is a good catch. Indeed, I don't have coverage like this.

> in that case, which seems wrong. This might get worse with
> the following sequence.
> 
> - set pretimeout
> - clear pretimeout
> - set timeout to some other value
> 

Can the below method resolve this issue?


static int keembay_wdt_set_pretimeout(struct watchdog_device *wdog, u32 t)
{
        struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);

        if(!t)
                keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, 0);

        wdog->pretimeout = t;
        keembay_wdt_set_timeout_reg(wdog, false);

        return 0;
}

> Thanks,
> Guenter
> 

Thanks,
Vijay