Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

Tomasz Figa
2013-12-05T16:44:29Z

On Thursday 05 of December 2013 08:40:38 Guenter Roeck wrote:
> On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
> > Hi Doug,
> >
> > Please see my comments inline.
> >
> > On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
> > > A good watchdog driver is supposed to report when it was responsible
> > > for resetting the system. Implement this for the s3c2410, at least on
> > > exynos5250 and exynos5420 where we already have a pointer to the PMU
> > > registers to read the information.
> > >
> > > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > > ---
> > > This patch is based atop Leela Krishna's recent series that ends with
> > > (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > > AKA <https://patchwork.kernel.org/patch/3251861/>.
> > >
> > > drivers/watchdog/s3c2410_wdt.c | 42 +++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 39 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > > index 47f4dcf..2c87d37 100644
> > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > @@ -62,9 +62,13 @@
> > > #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
> > > #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> > >
> > > +#define RST_STAT_REG_OFFSET 0x0404
> >
> > IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
> > registers below should be as well, but I missed this in the patch adding
> > them.
> >
> > > #define WDT_DISABLE_REG_OFFSET 0x0408
> > > #define WDT_MASK_RESET_REG_OFFSET 0x040c
> > > #define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
> > > +#define QUIRK_HAS_RST_STAT (1 << 1)
> > > +#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
> > > + QUIRK_HAS_RST_STAT)
> > >
> > > static bool nowayout = WATCHDOG_NOWAYOUT;
> > > static int tmr_margin;
> > > @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)");
> > > * timer reset functionality.
> > > * @mask_bit: Bit number for the watchdog timer in the disable register and the
> > > * mask reset register.
> > > + * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
> > > + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> > > + * reset.
> > > * @quirks: A bitfield of quirks.
> > > */
> > >
> > > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
> > > int disable_reg;
> > > int mask_reset_reg;
> > > int mask_bit;
> > > + int rst_stat_reg;
> > > + int rst_stat_bit;
> > > u32 quirks;
> > > };
> > >
> > > @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250 = {
> > > .disable_reg = WDT_DISABLE_REG_OFFSET,
> > > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > > .mask_bit = 20,
> > > - .quirks = QUIRK_NEEDS_PMU_CONFIG
> > > + .rst_stat_reg = RST_STAT_REG_OFFSET,
> > > + .rst_stat_bit = 20,
> > > + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > + QUIRK_HAS_RST_STAT,
> > > };
> > >
> > > static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> > > .disable_reg = WDT_DISABLE_REG_OFFSET,
> > > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > > .mask_bit = 0,
> > > - .quirks = QUIRK_NEEDS_PMU_CONFIG
> > > + .rst_stat_reg = RST_STAT_REG_OFFSET,
> > > + .rst_stat_bit = 9,
> > > + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > + QUIRK_HAS_RST_STAT,
> > > };
> > >
> > > static const struct of_device_id s3c2410_wdt_match[] = {
> > > @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
> > > }
> > > #endif
> > >
> > > +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> > > +{
> > > + unsigned int bootstatus = 0;
> > > + int ret;
> > > +
> > > + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
> >
> > nit: I guess it's just a matter of taste, but to reduce code indentation
> > you could inverse the check and simply return 0 here.
> >
>
> nit: If so, it would make sense to to the same for the other 'quirk'
> for consistency.
>
> static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
> {
> ...
> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
> return 0;
> ...
> }

That's quite a bit different story, as currently the check is outside of
this function, but I agree, it would look better.

Best regards,
Tomasz