Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

0 views
Skip to first unread message

Doug Anderson

unread,
Dec 2, 2013, 1:20:02 PM12/2/13
to
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 <dian...@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
#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) {
+ 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;
+}
+
/* s3c2410_get_wdt_driver_data */
static inline struct s3c2410_wdt_variant *
get_wdt_drv_data(struct platform_device *pdev)
@@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;

wdt->drv_data = get_wdt_drv_data(pdev);
- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
+ if (wdt->drv_data->quirks & QUIRKS_NEED_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
"samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg)) {
@@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)

watchdog_set_nowayout(&wdt->wdt_device, nowayout);

+ wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
+
ret = watchdog_register_device(&wdt->wdt_device);
if (ret) {
dev_err(dev, "cannot register watchdog (%d)\n", ret);
--
1.8.4.1

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

Guenter Roeck

unread,
Dec 2, 2013, 3:30:06 PM12/2/13
to
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?
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.

Thanks,
Guenter

Olof Johansson

unread,
Dec 2, 2013, 3:50:06 PM12/2/13
to
Ah, I just commented on these things on our internal review site too
on this patch. I don't even think a quirk is needed -- just use the
presence of a non-0 rst_stat_reg to tell if you need to use regmap.


-Olof

Doug Anderson

unread,
Dec 2, 2013, 4:20:05 PM12/2/13
to
Guenter and Olof,

On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <li...@roeck-us.net> wrote:
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...
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;
>> +}
>> +

Guenter Roeck

unread,
Dec 2, 2013, 4:40:03 PM12/2/13
to
Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
as well.

Guenter

Leela Krishna Amudala

unread,
Dec 5, 2013, 3:00:01 AM12/5/13
to
Hi Guenter Roeck,
As Tomasz Figa suggested I introduced quirks,

Tomasz,
Any comments from you here...??


Best wishes,
Leela Krishna


> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in

Guenter Roeck

unread,
Dec 5, 2013, 11:10:02 AM12/5/13
to
'quirk' implies a workaround for non-standard or broken hardware,
not a flag indicating device specific functionality.

Guenter
--

Tomasz Figa

unread,
Dec 5, 2013, 11:20:02 AM12/5/13
to
I wouldn't limit meaning of "quirk" to only this. The word has been widely
used around the kernel as a name for differences between hardware
variants.

As for the original issue itself, IMHO Doug's original solution is the
most practical, as 0 can be a valid register offset and RST_STAT register
could be used for other purposes as well in future, depending on other
quirk flag.

Best regards,
Tomasz

Tomasz Figa

unread,
Dec 5, 2013, 11:30:02 AM12/5/13
to
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 <dian...@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.
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.

Best regards,
Tomasz

Guenter Roeck

unread,
Dec 5, 2013, 11:50:01 AM12/5/13
to
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;
...
}

Guenter

Tomasz Figa

unread,
Dec 5, 2013, 11:50:01 AM12/5/13
to
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

Doug Anderson

unread,
Dec 5, 2013, 1:20:02 PM12/5/13
to
Tomasz
OK, I'm keeping my concept but adjusting the names. Version 2 coming
up shortly.

-Doug

Doug Anderson

unread,
Dec 5, 2013, 1:20:03 PM12/5/13
to
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.

Note that exynos4 SoCs also provide the reset status, but providing
that is left as an exercise for future changes and is not plumbed up
in this patch series. Also note the exynos4 SoCs don't appear to need
any PMU config, which is why this patch separates the concepts of
having PMU Registers vs. needing PMU Config.

Signed-off-by: Doug Anderson <dian...@chromium.org>
---
Changes in v2:
- Explained QUIRK organization in patch descritpion.
- Reduced indentation as per Olof and Tomasz suggestion.
- Now atop proposed v12 of Leela Krishna's patches.
- NEEDS => HAS, EXYNOS5 prefix

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 6a00299..45a9503 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,15 @@
#define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
#define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

+#define EXYNOS5_RST_STAT_REG_OFFSET 0x0404
#define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408
#define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c
#define QUIRK_HAS_PMU_CONFIG (1 << 0)
+#define QUIRK_HAS_RST_STAT (1 << 1)
+
+/* These quirks require that we have a PMU register map */
+#define QUIRKS_HAVE_PMUREG (QUIRK_HAS_PMU_CONFIG | \
+ QUIRK_HAS_RST_STAT)

static bool nowayout = WATCHDOG_NOWAYOUT;
static int tmr_margin;
@@ -98,6 +104,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 +114,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 +142,20 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250 = {
.disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 20,
- .quirks = QUIRK_HAS_PMU_CONFIG
+ .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+ .rst_stat_bit = 20,
+ .quirks = QUIRK_HAS_PMU_CONFIG |
+ QUIRK_HAS_RST_STAT,
};

static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
.disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 0,
- .quirks = QUIRK_HAS_PMU_CONFIG
+ .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+ .rst_stat_bit = 9,
+ .quirks = QUIRK_HAS_PMU_CONFIG |
+ QUIRK_HAS_RST_STAT,
};

static const struct of_device_id s3c2410_wdt_match[] = {
@@ -427,6 +444,23 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
}
#endif

+static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
+{
+ unsigned int rst_stat;
+ int ret;
+
+ if (!(wdt->drv_data->quirks & QUIRK_HAS_RST_STAT))
+ return 0;
+
+ 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))
+ return WDIOF_CARDRESET;
+
+ return 0;
+}
+
/* s3c2410_get_wdt_driver_data */
static inline struct s3c2410_wdt_variant *
get_wdt_drv_data(struct platform_device *pdev)
@@ -464,7 +498,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;

wdt->drv_data = get_wdt_drv_data(pdev);
- if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) {
+ if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
"samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg)) {
@@ -535,6 +569,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)

watchdog_set_nowayout(&wdt->wdt_device, nowayout);

+ wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
+
ret = watchdog_register_device(&wdt->wdt_device);
if (ret) {
dev_err(dev, "cannot register watchdog (%d)\n", ret);
--
1.8.5.1

Doug Anderson

unread,
Dec 5, 2013, 1:20:03 PM12/5/13
to
Guenter,
Done in my proposed fixup for Leela Krishna's patch.

-Doug

Doug Anderson

unread,
Dec 5, 2013, 1:20:03 PM12/5/13
to
Tomasz,
Done in v2 (plus fixup of Leela Krishna's).
Done in v2. This is the same suggestion Olof made.

-Doug

Doug Anderson

unread,
Dec 5, 2013, 1:20:03 PM12/5/13
to
This patch is not intended to land upon itself but is proposed fixup
to Leela Krishna's patch (v11) that can be found at
<https://patchwork.kernel.org/patch/3251841/>, potentially making a v12.

Posted so that I can build upon his patch with the fixups requested by
Guenter, Olof, and Tomasz.

Signed-off-by: Doug Anderson <dian...@chromium.org>
---
Changes in v2:
- Added EXYNOS5 prefix to REG_OFFSET defines (Tomasz)
- NEEDS_PMU_CONFIG => HAS_PMU_CONFIG (Olof, Guenter)
- Move QUIRK_HAS_PMU_CONFIG to (Guenter)

drivers/watchdog/s3c2410_wdt.c | 59 +++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 47f4dcf..6a00299 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,9 @@
#define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
#define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

-#define WDT_DISABLE_REG_OFFSET 0x0408
-#define WDT_MASK_RESET_REG_OFFSET 0x040c
-#define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
+#define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408
+#define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c
+#define QUIRK_HAS_PMU_CONFIG (1 << 0)

static bool nowayout = WATCHDOG_NOWAYOUT;
static int tmr_margin;
@@ -128,17 +128,17 @@ static const struct s3c2410_wdt_variant drv_data_s3c2410 = {

#ifdef CONFIG_OF
static const struct s3c2410_wdt_variant drv_data_exynos5250 = {
- .disable_reg = WDT_DISABLE_REG_OFFSET,
- .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 20,
- .quirks = QUIRK_NEEDS_PMU_CONFIG
+ .quirks = QUIRK_HAS_PMU_CONFIG
};

static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
- .disable_reg = WDT_DISABLE_REG_OFFSET,
- .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 0,
- .quirks = QUIRK_NEEDS_PMU_CONFIG
+ .quirks = QUIRK_HAS_PMU_CONFIG
};

static const struct of_device_id s3c2410_wdt_match[] = {
@@ -183,6 +183,10 @@ static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
u32 mask_val = 1 << wdt->drv_data->mask_bit;
u32 val = 0;

+ /* No need to do anything if no PMU CONFIG needed */
+ if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG))
+ return 0;
+
if (mask)
val = mask_val;

@@ -460,7 +464,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;

wdt->drv_data = get_wdt_drv_data(pdev);
- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
+ if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
"samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg)) {
@@ -537,11 +541,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
goto err_cpufreq;
}

- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
- ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
- if (ret < 0)
- goto err_unregister;
- }
+ ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+ if (ret < 0)
+ goto err_unregister;

if (tmr_atboot && started == 0) {
dev_info(dev, "starting watchdog timer\n");
@@ -586,11 +588,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
int ret;
struct s3c2410_wdt *wdt = platform_get_drvdata(dev);

- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
- ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
- if (ret < 0)
- return ret;
- }
+ ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+ if (ret < 0)
+ return ret;

watchdog_unregister_device(&wdt->wdt_device);

@@ -606,8 +606,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
{
struct s3c2410_wdt *wdt = platform_get_drvdata(dev);

- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)
- s3c2410wdt_mask_and_disable_reset(wdt, true);
+ s3c2410wdt_mask_and_disable_reset(wdt, true);

s3c2410wdt_stop(&wdt->wdt_device);
}
@@ -623,11 +622,9 @@ static int s3c2410wdt_suspend(struct device *dev)
wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);

- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
- ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
- if (ret < 0)
- return ret;
- }
+ ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+ if (ret < 0)
+ return ret;

/* Note that WTCNT doesn't need to be saved. */
s3c2410wdt_stop(&wdt->wdt_device);
@@ -645,11 +642,9 @@ static int s3c2410wdt_resume(struct device *dev)
writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);

- if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
- ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
- if (ret < 0)
- return ret;
- }
+ ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+ if (ret < 0)
+ return ret;

dev_info(dev, "watchdog %sabled\n",
(wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis");
--
1.8.5.1

Doug Anderson

unread,
Dec 5, 2013, 1:20:02 PM12/5/13
to
Guenter and Olof,
I have changed all instances of NEED / NEEDS and HAS / HAVE. Please
take a look. Since some of those changes applied to Leela Krishna's
patch I'm sending up a "fixup" patch which hopefully he can
incorporate into a v12.

-Doug

Guenter Roeck

unread,
Dec 6, 2013, 3:00:01 PM12/6/13
to
On Thu, Dec 05, 2013 at 10:15:29AM -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.
>
> Note that exynos4 SoCs also provide the reset status, but providing
> that is left as an exercise for future changes and is not plumbed up
> in this patch series. Also note the exynos4 SoCs don't appear to need
> any PMU config, which is why this patch separates the concepts of
> having PMU Registers vs. needing PMU Config.
>
> Signed-off-by: Doug Anderson <dian...@chromium.org>
> ---
> Changes in v2:
> - Explained QUIRK organization in patch descritpion.
> - Reduced indentation as per Olof and Tomasz suggestion.
> - Now atop proposed v12 of Leela Krishna's patches.

Hi Doug,

The patch doesn't apply on top of v12, unfortunately.
Any reason not to use QUIRKS_HAVE_PMUREG ?

> };
>
> static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
> .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
> .mask_bit = 0,
> - .quirks = QUIRK_HAS_PMU_CONFIG
> + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> + .rst_stat_bit = 9,
> + .quirks = QUIRK_HAS_PMU_CONFIG |
> + QUIRK_HAS_RST_STAT,

Same here. Even if you don't use QUIRKS_HAVE_PMUREG, the continuation line is not needed.

Thanks,
Guenter

Doug Anderson

unread,
Dec 6, 2013, 4:10:02 PM12/6/13
to
Guenter,

On Fri, Dec 6, 2013 at 11:54 AM, Guenter Roeck <li...@roeck-us.net> wrote:
> On Thu, Dec 05, 2013 at 10:15:29AM -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.
>>
>> Note that exynos4 SoCs also provide the reset status, but providing
>> that is left as an exercise for future changes and is not plumbed up
>> in this patch series. Also note the exynos4 SoCs don't appear to need
>> any PMU config, which is why this patch separates the concepts of
>> having PMU Registers vs. needing PMU Config.
>>
>> Signed-off-by: Doug Anderson <dian...@chromium.org>
>> ---
>> Changes in v2:
>> - Explained QUIRK organization in patch descritpion.
>> - Reduced indentation as per Olof and Tomasz suggestion.
>> - Now atop proposed v12 of Leela Krishna's patches.
>
> Hi Doug,
>
> The patch doesn't apply on top of v12, unfortunately.

OK, I'll re-send shortly.
My intent was that the QUIRKS_HAVE_PMUREG is a list of quirks that
require a PMU register and is used for testing, not for setting. When
someone adds another quirk that requires the PMU Registers I don't
want that to automatically apply to old hardware.


>> };
>>
>> static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>> .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
>> .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
>> .mask_bit = 0,
>> - .quirks = QUIRK_HAS_PMU_CONFIG
>> + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
>> + .rst_stat_bit = 9,
>> + .quirks = QUIRK_HAS_PMU_CONFIG |
>> + QUIRK_HAS_RST_STAT,
>
> Same here. Even if you don't use QUIRKS_HAVE_PMUREG, the continuation line is not needed.

I will spin.


Thanks!

-Doug

Doug Anderson

unread,
Dec 6, 2013, 4:20:02 PM12/6/13
to
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.

Note that exynos4 SoCs also provide the reset status, but providing
that is left as an exercise for future changes and is not plumbed up
in this patch series. Also note the exynos4 SoCs don't appear to need
any PMU config, which is why this patch separates the concepts of
having PMU Registers vs. needing PMU Config.

Signed-off-by: Doug Anderson <dian...@chromium.org>
---
Changes in v3:
- Atop the real v12 of Leela Krishna's patches.
- Combine QURIKs to one line.

Changes in v2:
- Explained QUIRK organization in patch descritpion.
- Reduced indentation as per Olof and Tomasz suggestion.
- Now atop proposed v12 of Leela Krishna's patches.
- NEEDS => HAS, EXYNOS5 prefix

drivers/watchdog/s3c2410_wdt.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index e1b1a75..ff1b5cc 100644
@@ -131,14 +142,18 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250 = {
.disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 20,
- .quirks = QUIRK_HAS_PMU_CONFIG
+ .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+ .rst_stat_bit = 20,
+ .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT,
};

static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
.disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 0,
- .quirks = QUIRK_HAS_PMU_CONFIG
+ .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+ .rst_stat_bit = 9,
+ .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT,
};

static const struct of_device_id s3c2410_wdt_match[] = {
@@ -427,6 +442,23 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
}
#endif

+static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
+{
+ unsigned int rst_stat;
+ int ret;
+
+ if (!(wdt->drv_data->quirks & QUIRK_HAS_RST_STAT))
+ return 0;
+
+ 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))
+ return WDIOF_CARDRESET;
+
+ return 0;
+}
+
/* s3c2410_get_wdt_driver_data */
static inline struct s3c2410_wdt_variant *
get_wdt_drv_data(struct platform_device *pdev)
@@ -464,7 +496,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;

wdt->drv_data = get_wdt_drv_data(pdev);
- if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) {
+ if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
"samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg)) {
@@ -535,6 +567,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)

Guenter Roeck

unread,
Dec 6, 2013, 4:30:02 PM12/6/13
to
On Fri, Dec 06, 2013 at 01:08:07PM -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.
>
> Note that exynos4 SoCs also provide the reset status, but providing
> that is left as an exercise for future changes and is not plumbed up
> in this patch series. Also note the exynos4 SoCs don't appear to need
> any PMU config, which is why this patch separates the concepts of
> having PMU Registers vs. needing PMU Config.
>
> Signed-off-by: Doug Anderson <dian...@chromium.org>

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

Wim Van Sebroeck

unread,
Jan 13, 2014, 1:30:02 PM1/13/14
to
Hi Doug,

> 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.
>
> Note that exynos4 SoCs also provide the reset status, but providing
> that is left as an exercise for future changes and is not plumbed up
> in this patch series. Also note the exynos4 SoCs don't appear to need
> any PMU config, which is why this patch separates the concepts of
> having PMU Registers vs. needing PMU Config.
>
> Signed-off-by: Doug Anderson <dian...@chromium.org>

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.
0 new messages