Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello,

adding Wolfram to Cc who authored the watchdog bits in the rtc driver.

On Mon, Jul 09, 2018 at 09:19:24AM -0700, Guenter Roeck wrote:
> On Mon, Jul 09, 2018 at 10:30:41AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
> > > As pointed out in the added comment resetting the rtc also stops the
> > > included watchdog. This is bad if the bootloader started the watchdog to
> > > secure the boot process. So don't reset if the watchdog is running.
> > 
> > I didn't get any feedback for this patch yet. Assuming my patch is
> > considered ok, my expectation would be that the watchdog people ack it
> > and that then it goes in via the rtc tree.
> > 
> 
> Guess there was a good reason for not passing an accessor via regmap ?

I don't know the details, but maybe this concept wasn't picked because
just passing a single function for setting the timeout was considered
simpler. Also given that the registers for wdg and rtc are not separated
cleanly giving the wdg driver a regmap isn't "nice" IMHO.

Last time I worked on this driver I wondered if it would be cleaner to
merge both drivers into drivers/rtc as the relevant functionality
(stmp3xxx_wdt_set_timeout()) is in that driver anyhow and
drivers/watchdog/stmp3xxx_rtc_wdt.c mostly contains stuff to handle the
separation. Oh, and there is a reboot notifier that looks strange where
I wonder if it is really needed.

> Either case, stmp_reset_block() is also called from resume, meaning the
> watchdog, if running, will likely be disabled by a suspend/resume cycle.

Yeah, that's right. The watchdog driver has the corresponding callbacks
and restarts the timer on resume though.

> I have no idea why the rtc block is reset in the first place;
> one would assume that there was a reason for that.

The usual reason is to ensure a clean hardware state. I wouldn't expect
a deeper reason here. (And if there is one, and removing it really
breaks something, the right way forward is to reintroduce the reset with
an explaining comment and think about how to fix the watchdog issue I
fixed in another way.)

> The associated watchdog driver does not support telling the watchdog
> core that the watchdog is running. As such, a watchdog running at boot
> will only be handled if the watchdog daemon is started before the
> watchdog times out. I assume this is understood and acceptable.

Yeah, that's the use case we have.

> Given all that, and taking no responsibility for the actual impact
> of the changes, I can only confirm that the patch looks syntactically
> correct.
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |