Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer DriverCore.

[Wim Van Sebroeck]

Hi H Hartley,

> > [...]
> >> @@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
> >>  {
> >>  	int err;
> >>  
> >> -	err = misc_register(&ep93xx_wdt_miscdev);
> >> +	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
> >> +	ep93xx_wdd.timeout = timeout;
> >> +
> >> +	err = watchdog_register_device(&ep93xx_wdd);
> >> +	if (err)
> >> +		return err;
> >>  
> >> -	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
> >> +	setup_timer(&timer, ep93xx_timer_ping, 1);
> >
> > Shouldn't the bootstatus setting be:
> > ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
> > (or something similar with the WDIOF_OVERHEAT, ... flags).
> 
> The ep93xx watchdog status register doesn't line up nicely with the standard
> WDIOF_* flags.  The register has these bits defined:
> 
> PLSDN		6	Pulse Disable Not
> OVRID		5	Software Override of HWDIS
> SWDIS		4	Software Watchdog Disable
> HWDIS		3	Hardware Watchdog Disable
> URST		2	User Reset Detected
> 3KRST		1	Three-Key Reset Detected
> WD		0	Watchdog Reset Detected

Hmm, it would be nice to have this info also in the driver. Certainly at least a define for the WD bit.
Secondly: what it is doing now is allready incorrect: The WD bit is returned as bit 0 of the bootstatus value.
This is actually: WDIOF_OVERHEAT (Reset due to CPU overheat) instead of
WDIOF_CARDRESET (0x0020 Card previously reset the CPU)...

> The original bootstatus setting just checked for the WD bit.  I would like
> to pass the full status register so that userspace can figure out what caused
> the reset.  Is this an inappropriate abuse of WDIOC_GETBOOTSTATUS?

It's indeed an inappropriate abuse of WDIOC_GETBOOTSTATUS.
The WDIOC_GETBOOTSTATUS should return a result for all watchdog drivers.
The result is based on the WDIOF_* flags
This however does not mean that we can add flags. The 'User Reset detected' looks to me as something that can be usefull in embedded environments. We need to think about this and see what would be usefull in general.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/