| Message ID | 1386008082-28740-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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) > I am not really happy about the NEED (both of them, really) here. How about HAS instead ? > 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. Thanks, Guenter > }; > > 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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) >> > I am not really happy about the NEED (both of them, really) here. > How about HAS instead ? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter and Olof, On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) >> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) > >> > > I am not really happy about the NEED (both of them, really) here. > > How about HAS instead ? > > 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. > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers as well. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter Roeck, On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) > > >> > > > I am not really happy about the NEED (both of them, really) here. > > > How about HAS instead ? > > > > 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. > > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers > as well. > 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 > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote: > Hi Guenter Roeck, > > On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: > > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) > > > >> > > > > I am not really happy about the NEED (both of them, really) here. > > > > How about HAS instead ? > > > > > > 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. > > > > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers > > as well. > > > > As Tomasz Figa suggested I introduced quirks, > 'quirk' implies a workaround for non-standard or broken hardware, not a flag indicating device specific functionality. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote: > On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote: > > Hi Guenter Roeck, > > > > On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: > > > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) > > > > >> > > > > > I am not really happy about the NEED (both of them, really) here. > > > > > How about HAS instead ? > > > > > > > > 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. > > > > > > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers > > > as well. > > > > > > > As Tomasz Figa suggested I introduced quirks, > > > 'quirk' implies a workaround for non-standard or broken hardware, > not a flag indicating device specific functionality. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; ... } Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter and Olof, On Mon, Dec 2, 2013 at 1:36 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: >> On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) >> >> >> > I am not really happy about the NEED (both of them, really) here. >> > How about HAS instead ? >> >> 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. >> > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers > as well. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz On Thu, Dec 5, 2013 at 8:18 AM, Tomasz Figa <t.figa@samsung.com> wrote: > On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote: >> On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote: >> > Hi Guenter Roeck, >> > >> > On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> > > >> > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote: >> > > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck <linux@roeck-us.net> 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@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) >> > > > >> >> > > > > I am not really happy about the NEED (both of them, really) here. >> > > > > How about HAS instead ? >> > > > >> > > > 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. >> > > > >> > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers >> > > as well. >> > > >> > >> > As Tomasz Figa suggested I introduced quirks, >> > >> 'quirk' implies a workaround for non-standard or broken hardware, >> not a flag indicating device specific functionality. > > 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. OK, I'm keeping my concept but adjusting the names. Version 2 coming up shortly. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Thu, Dec 5, 2013 at 8:21 AM, Tomasz Figa <t.figa@samsung.com> 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. Done in v2 (plus fixup of Leela Krishna's). >> #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. Done in v2. This is the same suggestion Olof made. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter, On Thu, Dec 5, 2013 at 8:40 AM, Guenter Roeck <linux@roeck-us.net> 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; > ... > } Done in my proposed fixup for Leela Krishna's patch. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)