Guenter and Olof, On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Mon, Dec 02, 2013 at 10:14:41AM -0800, 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@xxxxxxxxxxxx> >> --- >> 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 >> #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) >> > I am not really happy about the NEED (both of them, really) here. > How about HAS instead ? Are you suggesting also changing QUIRK_NEEDS_PMU_CONFIG, then? That would be a request for Leela Krishna on his patch? ...see below for more... > >> 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, > > Why not use QUIRKS_NEED_PMUREG ? > > Also, is there ever a chance that the two bits don't come together ? > If not a single bit might be sufficient. Here's my logic: According to our 3.4 sources (exynos_get_bootstatus) there is a RST_STAT register on exynos4. See <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.4/arch/arm/mach-exynos/pmu.c>. According to Tomasz at <http://www.spinics.net/lists/linux-watchdog/msg02942.html> the extra PMU_CONFIG was needed on exynos5250/5420 and not needed on exynos4. My patch doesn't actually add RST_STAT support for exynos4 but it seems wise to pave the way for someone else to add it. ...so basically I was saying: * On exynos5250 and exynos5420 we _need_ to configure the PMU to get proper functioning * On various devices we _have_ a reset status that register that we can query. * If we need to use the PMU or we want to query the reset status we need to have PMU registers specified. Does any of that change your mind(s)? >> 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) { Internally Olof requested to reverse this "if" and return 0 early (avoid indentation). I'll fix that up for the next patch. >> + unsigned int rst_stat; >> + >> + ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, >> + &rst_stat); >> + if (ret) >> + dev_warn(wdt->dev, "Couldn't get RST_STAT register\n"); >> + else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit)) >> + bootstatus |= WDIOF_CARDRESET; >> + } >> + >> + return bootstatus; >> +} >> + -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |