On Wed, Dec 02, 2020 at 02:55:32PM +0000, Ayyathurai, Vijayakannan wrote:
> 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);
>
Partially, only it makes for awkward code. After all, it is never really
necessary to set the timeout register after updating the pretimeout.
The "ping" parameter makes less and less sense with this in mind.
It might be better to split the set_timeout_reg function into
set_timeout_reg and set_pretimeout_reg and call those functions as needed
(and handle the if() above in the set_pretimeout_reg function).
Thanks,
Guenter
> wdog->pretimeout = t;
> keembay_wdt_set_timeout_reg(wdog, false);
>
> return 0;
> }
>
> > Thanks,
> > Guenter
> >
>
> Thanks,
> Vijay
