| Message ID | 1384768189-2839-3-git-send-email-l.krishna@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote: > Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface > to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU > to mask/unmask enable/disable of watchdog in probe and s2r scenarios. > > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> > --- > .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- > drivers/watchdog/Kconfig | 1 + > drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- > 3 files changed, 157 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > index 2aa486c..5dea363 100644 > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset event has not > occurred. > > Required properties: > -- compatible : should be "samsung,s3c2410-wdt" > +- compatible : should be one among the following > + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs > + (b) "samsung,exynos5250-wdt" for Exynos5250 > + (c) "samsung,exynos5420-wdt" for Exynos5420 > + > - reg : base physical address of the controller and length of memory mapped > region. > - interrupts : interrupt number to the cpu. > +- samsung,syscon-phandle : reference to syscon node (This property required only > + in case of compatible being "samsung,exynos5250-wdt" or "samsung,exynos5420-wdt". > + In case of Exynos5250 and 5420 this property points to syscon node holding the PMU > + base address) > > Optional properties: > - timeout-sec : contains the watchdog timeout in seconds. > + > +Example: > + > +watchdog@101D0000 { > + compatible = "samsung,exynos5250-wdt"; > + reg = <0x101D0000 0x100>; > + interrupts = <0 42 0>; > + clocks = <&clock 336>; > + clock-names = "watchdog"; > + samsung,syscon-phandle = <&pmu_sys_reg>; > +}; > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index d1d53f3..0d272ae 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG > tristate "S3C2410 Watchdog" > depends on HAVE_S3C2410_WATCHDOG > select WATCHDOG_CORE > + select MFD_SYSCON if ARCH_EXYNOS5 > help > Watchdog timer block in the Samsung SoCs. This will reboot > the system when the timer expires with the watchdog enabled. > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 23aad7c..42e0fd3 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -41,6 +41,8 @@ > #include <linux/slab.h> > #include <linux/err.h> > #include <linux/of.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > > #define S3C2410_WTCON 0x00 > #define S3C2410_WTDAT 0x04 > @@ -61,6 +63,10 @@ > #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) > + > static bool nowayout = WATCHDOG_NOWAYOUT; > static int tmr_margin; > static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; > @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, " > "0 to reboot (default 0)"); > MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)"); > > +struct s3c2410_wdt_variant { > + int disable_reg; > + int mask_reset_reg; > + int mask_bit; > + u32 quirks; > +}; > + > struct s3c2410_wdt { > struct device *dev; > struct clk *clock; > @@ -94,7 +107,49 @@ struct s3c2410_wdt { > unsigned long wtdat_save; > struct watchdog_device wdt_device; > struct notifier_block freq_transition; > + struct s3c2410_wdt_variant *drv_data; > + struct regmap *pmureg; > +}; > + > +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { > + .quirks = 0 > +}; > + > +#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, > + .mask_bit = 20, > + .quirks = QUIRK_NEEDS_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, > + .mask_bit = 0, > + .quirks = QUIRK_NEEDS_PMU_CONFIG > +}; > + > +static const struct of_device_id s3c2410_wdt_match[] = { > + { .compatible = "samsung,s3c2410-wdt", > + .data = &drv_data_s3c2410 }, > + { .compatible = "samsung,exynos5250-wdt", > + .data = &drv_data_exynos5250 }, > + { .compatible = "samsung,exynos5420-wdt", > + .data = &drv_data_exynos5420 }, > + {}, > }; > +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); > +#endif > + > +static const struct platform_device_id s3c2410_wdt_ids[] = { > + { > + .name = "s3c2410-wdt", > + .driver_data = (unsigned long)&drv_data_s3c2410, > + }, > + {} > +}; > +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); > > /* watchdog control routines */ > > @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) > return container_of(nb, struct s3c2410_wdt, freq_transition); > } > > +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) > +{ > + int ret; > + u32 mask_val = 1 << wdt->drv_data->mask_bit; > + u32 val = 0; > + > + if (mask) > + val = mask_val; > + > + ret = regmap_update_bits(wdt->pmureg, > + wdt->drv_data->disable_reg, > + mask_val, val); > + if (ret < 0) > + return ret; > + > + return regmap_update_bits(wdt->pmureg, > + wdt->drv_data->mask_reset_reg, > + mask_val, val); > +} > + > static int s3c2410wdt_keepalive(struct watchdog_device *wdd) > { > struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); > @@ -332,6 +407,20 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) > } > #endif > > +/* s3c2410_get_wdt_driver_data */ > +static inline struct s3c2410_wdt_variant * > +get_wdt_drv_data(struct platform_device *pdev) > +{ > + if (pdev->dev.of_node) { > + const struct of_device_id *match; > + match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); > + return (struct s3c2410_wdt_variant *)match->data; > + } else { > + return (struct s3c2410_wdt_variant *) > + platform_get_device_id(pdev)->driver_data; > + } > +} > + > static int s3c2410wdt_probe(struct platform_device *pdev) > { > struct device *dev; > @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > spin_lock_init(&wdt->lock); > wdt->wdt_device = s3c2410_wdd; > > + wdt->drv_data = get_wdt_drv_data(pdev); > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > + "samsung,syscon-phandle"); > + if (IS_ERR(wdt->pmureg)) { > + dev_err(dev, "syscon regmap lookup failed.\n"); > + return PTR_ERR(wdt->pmureg); > + } > + } > + > wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (wdt_irq == NULL) { > dev_err(dev, "no irq resource specified\n"); > @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); > > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); > + if (ret < 0) { > + dev_err(wdt->dev, "failed to update pmu register"); Hmm ... still not happy ;-). This message is the same for each call to s3c2410wdt_mask_and_disable_reset(). Why not create it there ? Sure, you'd have to decide if you want to use dev_warn() or dev_err(), but that would still be better than repeating the same message (and code) five times. The same is true for the if() statement. It might be worthwhile calling s3c2410wdt_mask_and_disable_reset() unconditionally and adding something like if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)) return 0; to it. Thanks, Guenter > + goto err_cpufreq; > + } > + } > + > return 0; > > err_cpufreq: > @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > 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) > + dev_warn(wdt->dev, "failed to update pmu register"); > + } > + > watchdog_unregister_device(&wdt->wdt_device); > > s3c2410wdt_cpufreq_deregister(wdt); > @@ -473,8 +587,15 @@ static int s3c2410wdt_remove(struct platform_device *dev) > > static void s3c2410wdt_shutdown(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) > + dev_warn(wdt->dev, "failed to update pmu register"); > + } > + > s3c2410wdt_stop(&wdt->wdt_device); > } > > @@ -482,12 +603,19 @@ static void s3c2410wdt_shutdown(struct platform_device *dev) > > static int s3c2410wdt_suspend(struct device *dev) > { > + int ret; > struct s3c2410_wdt *wdt = dev_get_drvdata(dev); > > /* Save watchdog state, and turn it off. */ > 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) > + dev_warn(wdt->dev, "failed to update pmu register"); > + } > + > /* Note that WTCNT doesn't need to be saved. */ > s3c2410wdt_stop(&wdt->wdt_device); > > @@ -496,6 +624,7 @@ static int s3c2410wdt_suspend(struct device *dev) > > static int s3c2410wdt_resume(struct device *dev) > { > + int ret; > struct s3c2410_wdt *wdt = dev_get_drvdata(dev); > > /* Restore watchdog state. */ > @@ -503,6 +632,12 @@ 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) > + dev_warn(wdt->dev, "failed to update pmu register"); > + } > + > dev_info(dev, "watchdog %sabled\n", > (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); > > @@ -513,18 +648,11 @@ static int s3c2410wdt_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, > s3c2410wdt_resume); > > -#ifdef CONFIG_OF > -static const struct of_device_id s3c2410_wdt_match[] = { > - { .compatible = "samsung,s3c2410-wdt" }, > - {}, > -}; > -MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); > -#endif > - > static struct platform_driver s3c2410wdt_driver = { > .probe = s3c2410wdt_probe, > .remove = s3c2410wdt_remove, > .shutdown = s3c2410wdt_shutdown, > + .id_table = s3c2410_wdt_ids, > .driver = { > .owner = THIS_MODULE, > .name = "s3c2410-wdt", > @@ -540,4 +668,3 @@ MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>, " > MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver"); > MODULE_LICENSE("GPL"); > MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); > -MODULE_ALIAS("platform:s3c2410-wdt"); > -- > 1.7.10.4 > > -- 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, Thanks for reviewing the patch. On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote: >> Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface >> to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU >> to mask/unmask enable/disable of watchdog in probe and s2r scenarios. >> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >> --- >> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- >> drivers/watchdog/Kconfig | 1 + >> drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- >> 3 files changed, 157 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> index 2aa486c..5dea363 100644 >> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset event has not >> occurred. >> >> Required properties: >> -- compatible : should be "samsung,s3c2410-wdt" >> +- compatible : should be one among the following >> + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs >> + (b) "samsung,exynos5250-wdt" for Exynos5250 >> + (c) "samsung,exynos5420-wdt" for Exynos5420 >> + >> - reg : base physical address of the controller and length of memory mapped >> region. >> - interrupts : interrupt number to the cpu. >> +- samsung,syscon-phandle : reference to syscon node (This property required only >> + in case of compatible being "samsung,exynos5250-wdt" or "samsung,exynos5420-wdt". >> + In case of Exynos5250 and 5420 this property points to syscon node holding the PMU >> + base address) >> >> Optional properties: >> - timeout-sec : contains the watchdog timeout in seconds. >> + >> +Example: >> + >> +watchdog@101D0000 { >> + compatible = "samsung,exynos5250-wdt"; >> + reg = <0x101D0000 0x100>; >> + interrupts = <0 42 0>; >> + clocks = <&clock 336>; >> + clock-names = "watchdog"; >> + samsung,syscon-phandle = <&pmu_sys_reg>; >> +}; >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index d1d53f3..0d272ae 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG >> tristate "S3C2410 Watchdog" >> depends on HAVE_S3C2410_WATCHDOG >> select WATCHDOG_CORE >> + select MFD_SYSCON if ARCH_EXYNOS5 >> help >> Watchdog timer block in the Samsung SoCs. This will reboot >> the system when the timer expires with the watchdog enabled. >> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >> index 23aad7c..42e0fd3 100644 >> --- a/drivers/watchdog/s3c2410_wdt.c >> +++ b/drivers/watchdog/s3c2410_wdt.c >> @@ -41,6 +41,8 @@ >> #include <linux/slab.h> >> #include <linux/err.h> >> #include <linux/of.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/regmap.h> >> >> #define S3C2410_WTCON 0x00 >> #define S3C2410_WTDAT 0x04 >> @@ -61,6 +63,10 @@ >> #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) >> + >> static bool nowayout = WATCHDOG_NOWAYOUT; >> static int tmr_margin; >> static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; >> @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, " >> "0 to reboot (default 0)"); >> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)"); >> >> +struct s3c2410_wdt_variant { >> + int disable_reg; >> + int mask_reset_reg; >> + int mask_bit; >> + u32 quirks; >> +}; >> + >> struct s3c2410_wdt { >> struct device *dev; >> struct clk *clock; >> @@ -94,7 +107,49 @@ struct s3c2410_wdt { >> unsigned long wtdat_save; >> struct watchdog_device wdt_device; >> struct notifier_block freq_transition; >> + struct s3c2410_wdt_variant *drv_data; >> + struct regmap *pmureg; >> +}; >> + >> +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { >> + .quirks = 0 >> +}; >> + >> +#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, >> + .mask_bit = 20, >> + .quirks = QUIRK_NEEDS_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, >> + .mask_bit = 0, >> + .quirks = QUIRK_NEEDS_PMU_CONFIG >> +}; >> + >> +static const struct of_device_id s3c2410_wdt_match[] = { >> + { .compatible = "samsung,s3c2410-wdt", >> + .data = &drv_data_s3c2410 }, >> + { .compatible = "samsung,exynos5250-wdt", >> + .data = &drv_data_exynos5250 }, >> + { .compatible = "samsung,exynos5420-wdt", >> + .data = &drv_data_exynos5420 }, >> + {}, >> }; >> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >> +#endif >> + >> +static const struct platform_device_id s3c2410_wdt_ids[] = { >> + { >> + .name = "s3c2410-wdt", >> + .driver_data = (unsigned long)&drv_data_s3c2410, >> + }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); >> >> /* watchdog control routines */ >> >> @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) >> return container_of(nb, struct s3c2410_wdt, freq_transition); >> } >> >> +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) >> +{ >> + int ret; >> + u32 mask_val = 1 << wdt->drv_data->mask_bit; >> + u32 val = 0; >> + >> + if (mask) >> + val = mask_val; >> + >> + ret = regmap_update_bits(wdt->pmureg, >> + wdt->drv_data->disable_reg, >> + mask_val, val); >> + if (ret < 0) >> + return ret; >> + >> + return regmap_update_bits(wdt->pmureg, >> + wdt->drv_data->mask_reset_reg, >> + mask_val, val); >> +} >> + >> static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >> { >> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); >> @@ -332,6 +407,20 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >> } >> #endif >> >> +/* s3c2410_get_wdt_driver_data */ >> +static inline struct s3c2410_wdt_variant * >> +get_wdt_drv_data(struct platform_device *pdev) >> +{ >> + if (pdev->dev.of_node) { >> + const struct of_device_id *match; >> + match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); >> + return (struct s3c2410_wdt_variant *)match->data; >> + } else { >> + return (struct s3c2410_wdt_variant *) >> + platform_get_device_id(pdev)->driver_data; >> + } >> +} >> + >> static int s3c2410wdt_probe(struct platform_device *pdev) >> { >> struct device *dev; >> @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> spin_lock_init(&wdt->lock); >> wdt->wdt_device = s3c2410_wdd; >> >> + wdt->drv_data = get_wdt_drv_data(pdev); >> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >> + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, >> + "samsung,syscon-phandle"); >> + if (IS_ERR(wdt->pmureg)) { >> + dev_err(dev, "syscon regmap lookup failed.\n"); >> + return PTR_ERR(wdt->pmureg); >> + } >> + } >> + >> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> if (wdt_irq == NULL) { >> dev_err(dev, "no irq resource specified\n"); >> @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", >> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); >> >> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >> + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); >> + if (ret < 0) { >> + dev_err(wdt->dev, "failed to update pmu register"); > > Hmm ... still not happy ;-). This message is the same for each call > to s3c2410wdt_mask_and_disable_reset(). Why not create it there ? > Sure, you'd have to decide if you want to use dev_warn() or dev_err(), > but that would still be better than repeating the same message (and code) > five times. > > The same is true for the if() statement. It might be worthwhile calling > s3c2410wdt_mask_and_disable_reset() unconditionally and adding something > like > > if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)) > return 0; > > to it. > As Tomasz Figa suggested I handled the error conditions here Please go through this link for your reference https://patchwork.kernel.org/patch/3118771/ Best Wishes, Leela Krishna. > Thanks, > Guenter > >> + goto err_cpufreq; >> + } >> + } >> + >> return 0; >> >> err_cpufreq: >> @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> >> 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) >> + dev_warn(wdt->dev, "failed to update pmu register"); >> + } >> + >> watchdog_unregister_device(&wdt->wdt_device); >> >> s3c2410wdt_cpufreq_deregister(wdt); >> @@ -473,8 +587,15 @@ static int s3c2410wdt_remove(struct platform_device *dev) >> >> static void s3c2410wdt_shutdown(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) >> + dev_warn(wdt->dev, "failed to update pmu register"); >> + } >> + >> s3c2410wdt_stop(&wdt->wdt_device); >> } >> >> @@ -482,12 +603,19 @@ static void s3c2410wdt_shutdown(struct platform_device *dev) >> >> static int s3c2410wdt_suspend(struct device *dev) >> { >> + int ret; >> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >> >> /* Save watchdog state, and turn it off. */ >> 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) >> + dev_warn(wdt->dev, "failed to update pmu register"); >> + } >> + >> /* Note that WTCNT doesn't need to be saved. */ >> s3c2410wdt_stop(&wdt->wdt_device); >> >> @@ -496,6 +624,7 @@ static int s3c2410wdt_suspend(struct device *dev) >> >> static int s3c2410wdt_resume(struct device *dev) >> { >> + int ret; >> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >> >> /* Restore watchdog state. */ >> @@ -503,6 +632,12 @@ 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) >> + dev_warn(wdt->dev, "failed to update pmu register"); >> + } >> + >> dev_info(dev, "watchdog %sabled\n", >> (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); >> >> @@ -513,18 +648,11 @@ static int s3c2410wdt_resume(struct device *dev) >> static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, >> s3c2410wdt_resume); >> >> -#ifdef CONFIG_OF >> -static const struct of_device_id s3c2410_wdt_match[] = { >> - { .compatible = "samsung,s3c2410-wdt" }, >> - {}, >> -}; >> -MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >> -#endif >> - >> static struct platform_driver s3c2410wdt_driver = { >> .probe = s3c2410wdt_probe, >> .remove = s3c2410wdt_remove, >> .shutdown = s3c2410wdt_shutdown, >> + .id_table = s3c2410_wdt_ids, >> .driver = { >> .owner = THIS_MODULE, >> .name = "s3c2410-wdt", >> @@ -540,4 +668,3 @@ MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>, " >> MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver"); >> MODULE_LICENSE("GPL"); >> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); >> -MODULE_ALIAS("platform:s3c2410-wdt"); >> -- >> 1.7.10.4 >> >> > -- > 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 11/18/2013 08:36 PM, Leela Krishna Amudala wrote: > Hi Guenter Roeck, > > Thanks for reviewing the patch. > > > On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote: >>> Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface >>> to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU >>> to mask/unmask enable/disable of watchdog in probe and s2r scenarios. >>> >>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >>> --- >>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- >>> drivers/watchdog/Kconfig | 1 + >>> drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- >>> 3 files changed, 157 insertions(+), 10 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> index 2aa486c..5dea363 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset event has not >>> occurred. >>> >>> Required properties: >>> -- compatible : should be "samsung,s3c2410-wdt" >>> +- compatible : should be one among the following >>> + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs >>> + (b) "samsung,exynos5250-wdt" for Exynos5250 >>> + (c) "samsung,exynos5420-wdt" for Exynos5420 >>> + >>> - reg : base physical address of the controller and length of memory mapped >>> region. >>> - interrupts : interrupt number to the cpu. >>> +- samsung,syscon-phandle : reference to syscon node (This property required only >>> + in case of compatible being "samsung,exynos5250-wdt" or "samsung,exynos5420-wdt". >>> + In case of Exynos5250 and 5420 this property points to syscon node holding the PMU >>> + base address) >>> >>> Optional properties: >>> - timeout-sec : contains the watchdog timeout in seconds. >>> + >>> +Example: >>> + >>> +watchdog@101D0000 { >>> + compatible = "samsung,exynos5250-wdt"; >>> + reg = <0x101D0000 0x100>; >>> + interrupts = <0 42 0>; >>> + clocks = <&clock 336>; >>> + clock-names = "watchdog"; >>> + samsung,syscon-phandle = <&pmu_sys_reg>; >>> +}; >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index d1d53f3..0d272ae 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG >>> tristate "S3C2410 Watchdog" >>> depends on HAVE_S3C2410_WATCHDOG >>> select WATCHDOG_CORE >>> + select MFD_SYSCON if ARCH_EXYNOS5 >>> help >>> Watchdog timer block in the Samsung SoCs. This will reboot >>> the system when the timer expires with the watchdog enabled. >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >>> index 23aad7c..42e0fd3 100644 >>> --- a/drivers/watchdog/s3c2410_wdt.c >>> +++ b/drivers/watchdog/s3c2410_wdt.c >>> @@ -41,6 +41,8 @@ >>> #include <linux/slab.h> >>> #include <linux/err.h> >>> #include <linux/of.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/regmap.h> >>> >>> #define S3C2410_WTCON 0x00 >>> #define S3C2410_WTDAT 0x04 >>> @@ -61,6 +63,10 @@ >>> #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) >>> + >>> static bool nowayout = WATCHDOG_NOWAYOUT; >>> static int tmr_margin; >>> static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; >>> @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, " >>> "0 to reboot (default 0)"); >>> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)"); >>> >>> +struct s3c2410_wdt_variant { >>> + int disable_reg; >>> + int mask_reset_reg; >>> + int mask_bit; >>> + u32 quirks; >>> +}; >>> + >>> struct s3c2410_wdt { >>> struct device *dev; >>> struct clk *clock; >>> @@ -94,7 +107,49 @@ struct s3c2410_wdt { >>> unsigned long wtdat_save; >>> struct watchdog_device wdt_device; >>> struct notifier_block freq_transition; >>> + struct s3c2410_wdt_variant *drv_data; >>> + struct regmap *pmureg; >>> +}; >>> + >>> +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { >>> + .quirks = 0 >>> +}; >>> + >>> +#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, >>> + .mask_bit = 20, >>> + .quirks = QUIRK_NEEDS_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, >>> + .mask_bit = 0, >>> + .quirks = QUIRK_NEEDS_PMU_CONFIG >>> +}; >>> + >>> +static const struct of_device_id s3c2410_wdt_match[] = { >>> + { .compatible = "samsung,s3c2410-wdt", >>> + .data = &drv_data_s3c2410 }, >>> + { .compatible = "samsung,exynos5250-wdt", >>> + .data = &drv_data_exynos5250 }, >>> + { .compatible = "samsung,exynos5420-wdt", >>> + .data = &drv_data_exynos5420 }, >>> + {}, >>> }; >>> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >>> +#endif >>> + >>> +static const struct platform_device_id s3c2410_wdt_ids[] = { >>> + { >>> + .name = "s3c2410-wdt", >>> + .driver_data = (unsigned long)&drv_data_s3c2410, >>> + }, >>> + {} >>> +}; >>> +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); >>> >>> /* watchdog control routines */ >>> >>> @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) >>> return container_of(nb, struct s3c2410_wdt, freq_transition); >>> } >>> >>> +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) >>> +{ >>> + int ret; >>> + u32 mask_val = 1 << wdt->drv_data->mask_bit; >>> + u32 val = 0; >>> + >>> + if (mask) >>> + val = mask_val; >>> + >>> + ret = regmap_update_bits(wdt->pmureg, >>> + wdt->drv_data->disable_reg, >>> + mask_val, val); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return regmap_update_bits(wdt->pmureg, >>> + wdt->drv_data->mask_reset_reg, >>> + mask_val, val); >>> +} >>> + >>> static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >>> { >>> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); >>> @@ -332,6 +407,20 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >>> } >>> #endif >>> >>> +/* s3c2410_get_wdt_driver_data */ >>> +static inline struct s3c2410_wdt_variant * >>> +get_wdt_drv_data(struct platform_device *pdev) >>> +{ >>> + if (pdev->dev.of_node) { >>> + const struct of_device_id *match; >>> + match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); >>> + return (struct s3c2410_wdt_variant *)match->data; >>> + } else { >>> + return (struct s3c2410_wdt_variant *) >>> + platform_get_device_id(pdev)->driver_data; >>> + } >>> +} >>> + >>> static int s3c2410wdt_probe(struct platform_device *pdev) >>> { >>> struct device *dev; >>> @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >>> spin_lock_init(&wdt->lock); >>> wdt->wdt_device = s3c2410_wdd; >>> >>> + wdt->drv_data = get_wdt_drv_data(pdev); >>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>> + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, >>> + "samsung,syscon-phandle"); >>> + if (IS_ERR(wdt->pmureg)) { >>> + dev_err(dev, "syscon regmap lookup failed.\n"); >>> + return PTR_ERR(wdt->pmureg); >>> + } >>> + } >>> + >>> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>> if (wdt_irq == NULL) { >>> dev_err(dev, "no irq resource specified\n"); >>> @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >>> (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", >>> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); >>> >>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>> + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); >>> + if (ret < 0) { >>> + dev_err(wdt->dev, "failed to update pmu register"); >> >> Hmm ... still not happy ;-). This message is the same for each call >> to s3c2410wdt_mask_and_disable_reset(). Why not create it there ? >> Sure, you'd have to decide if you want to use dev_warn() or dev_err(), >> but that would still be better than repeating the same message (and code) >> five times. >> >> The same is true for the if() statement. It might be worthwhile calling >> s3c2410wdt_mask_and_disable_reset() unconditionally and adding something >> like >> >> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)) >> return 0; >> >> to it. >> > > As Tomasz Figa suggested I handled the error conditions here > Please go through this link for your reference > https://patchwork.kernel.org/patch/3118771/ > His proposed function had the error message in the function, so I am not entirely following your logic. He said you should _handle_ the error condition in the calling code. Dumping an error message and doing nothing isn't really "handling" the error condition. Guenter > Best Wishes, > Leela Krishna. > >> Thanks, >> Guenter >> >>> + goto err_cpufreq; >>> + } >>> + } >>> + >>> return 0; >>> >>> err_cpufreq: >>> @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >>> >>> 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) >>> + dev_warn(wdt->dev, "failed to update pmu register"); >>> + } >>> + >>> watchdog_unregister_device(&wdt->wdt_device); >>> >>> s3c2410wdt_cpufreq_deregister(wdt); >>> @@ -473,8 +587,15 @@ static int s3c2410wdt_remove(struct platform_device *dev) >>> >>> static void s3c2410wdt_shutdown(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) >>> + dev_warn(wdt->dev, "failed to update pmu register"); >>> + } >>> + >>> s3c2410wdt_stop(&wdt->wdt_device); >>> } >>> >>> @@ -482,12 +603,19 @@ static void s3c2410wdt_shutdown(struct platform_device *dev) >>> >>> static int s3c2410wdt_suspend(struct device *dev) >>> { >>> + int ret; >>> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >>> >>> /* Save watchdog state, and turn it off. */ >>> 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) >>> + dev_warn(wdt->dev, "failed to update pmu register"); >>> + } >>> + >>> /* Note that WTCNT doesn't need to be saved. */ >>> s3c2410wdt_stop(&wdt->wdt_device); >>> >>> @@ -496,6 +624,7 @@ static int s3c2410wdt_suspend(struct device *dev) >>> >>> static int s3c2410wdt_resume(struct device *dev) >>> { >>> + int ret; >>> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >>> >>> /* Restore watchdog state. */ >>> @@ -503,6 +632,12 @@ 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) >>> + dev_warn(wdt->dev, "failed to update pmu register"); >>> + } >>> + >>> dev_info(dev, "watchdog %sabled\n", >>> (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); >>> >>> @@ -513,18 +648,11 @@ static int s3c2410wdt_resume(struct device *dev) >>> static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, >>> s3c2410wdt_resume); >>> >>> -#ifdef CONFIG_OF >>> -static const struct of_device_id s3c2410_wdt_match[] = { >>> - { .compatible = "samsung,s3c2410-wdt" }, >>> - {}, >>> -}; >>> -MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >>> -#endif >>> - >>> static struct platform_driver s3c2410wdt_driver = { >>> .probe = s3c2410wdt_probe, >>> .remove = s3c2410wdt_remove, >>> .shutdown = s3c2410wdt_shutdown, >>> + .id_table = s3c2410_wdt_ids, >>> .driver = { >>> .owner = THIS_MODULE, >>> .name = "s3c2410-wdt", >>> @@ -540,4 +668,3 @@ MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>, " >>> MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver"); >>> MODULE_LICENSE("GPL"); >>> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); >>> -MODULE_ALIAS("platform:s3c2410-wdt"); >>> -- >>> 1.7.10.4 >>> >>> >> -- >> 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-watchdog" 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
Hi Guenter Roeck, On Tue, Nov 19, 2013 at 10:30 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On 11/18/2013 08:36 PM, Leela Krishna Amudala wrote: >> >> Hi Guenter Roeck, >> >> Thanks for reviewing the patch. >> >> >> On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck <linux@roeck-us.net> >> wrote: >>> >>> On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote: >>>> >>>> Add device tree support for exynos5250 and 5420 SoCs and use syscon >>>> regmap interface >>>> to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST >>>> registers of PMU >>>> to mask/unmask enable/disable of watchdog in probe and s2r scenarios. >>>> >>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >>>> --- >>>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- >>>> drivers/watchdog/Kconfig | 1 + >>>> drivers/watchdog/s3c2410_wdt.c | 145 >>>> ++++++++++++++++++-- >>>> 3 files changed, 157 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>> index 2aa486c..5dea363 100644 >>>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>> @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT >>>> reset event has not >>>> occurred. >>>> >>>> Required properties: >>>> -- compatible : should be "samsung,s3c2410-wdt" >>>> +- compatible : should be one among the following >>>> + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs >>>> + (b) "samsung,exynos5250-wdt" for Exynos5250 >>>> + (c) "samsung,exynos5420-wdt" for Exynos5420 >>>> + >>>> - reg : base physical address of the controller and length of memory >>>> mapped >>>> region. >>>> - interrupts : interrupt number to the cpu. >>>> +- samsung,syscon-phandle : reference to syscon node (This property >>>> required only >>>> + in case of compatible being "samsung,exynos5250-wdt" or >>>> "samsung,exynos5420-wdt". >>>> + In case of Exynos5250 and 5420 this property points to syscon node >>>> holding the PMU >>>> + base address) >>>> >>>> Optional properties: >>>> - timeout-sec : contains the watchdog timeout in seconds. >>>> + >>>> +Example: >>>> + >>>> +watchdog@101D0000 { >>>> + compatible = "samsung,exynos5250-wdt"; >>>> + reg = <0x101D0000 0x100>; >>>> + interrupts = <0 42 0>; >>>> + clocks = <&clock 336>; >>>> + clock-names = "watchdog"; >>>> + samsung,syscon-phandle = <&pmu_sys_reg>; >>>> +}; >>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>> index d1d53f3..0d272ae 100644 >>>> --- a/drivers/watchdog/Kconfig >>>> +++ b/drivers/watchdog/Kconfig >>>> @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG >>>> tristate "S3C2410 Watchdog" >>>> depends on HAVE_S3C2410_WATCHDOG >>>> select WATCHDOG_CORE >>>> + select MFD_SYSCON if ARCH_EXYNOS5 >>>> help >>>> Watchdog timer block in the Samsung SoCs. This will reboot >>>> the system when the timer expires with the watchdog enabled. >>>> diff --git a/drivers/watchdog/s3c2410_wdt.c >>>> b/drivers/watchdog/s3c2410_wdt.c >>>> index 23aad7c..42e0fd3 100644 >>>> --- a/drivers/watchdog/s3c2410_wdt.c >>>> +++ b/drivers/watchdog/s3c2410_wdt.c >>>> @@ -41,6 +41,8 @@ >>>> #include <linux/slab.h> >>>> #include <linux/err.h> >>>> #include <linux/of.h> >>>> +#include <linux/mfd/syscon.h> >>>> +#include <linux/regmap.h> >>>> >>>> #define S3C2410_WTCON 0x00 >>>> #define S3C2410_WTDAT 0x04 >>>> @@ -61,6 +63,10 @@ >>>> #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) >>>> + >>>> static bool nowayout = WATCHDOG_NOWAYOUT; >>>> static int tmr_margin; >>>> static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; >>>> @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set >>>> to 1 to ignore reboots, " >>>> "0 to reboot (default 0)"); >>>> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default >>>> 0)"); >>>> >>>> +struct s3c2410_wdt_variant { >>>> + int disable_reg; >>>> + int mask_reset_reg; >>>> + int mask_bit; >>>> + u32 quirks; >>>> +}; >>>> + >>>> struct s3c2410_wdt { >>>> struct device *dev; >>>> struct clk *clock; >>>> @@ -94,7 +107,49 @@ struct s3c2410_wdt { >>>> unsigned long wtdat_save; >>>> struct watchdog_device wdt_device; >>>> struct notifier_block freq_transition; >>>> + struct s3c2410_wdt_variant *drv_data; >>>> + struct regmap *pmureg; >>>> +}; >>>> + >>>> +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { >>>> + .quirks = 0 >>>> +}; >>>> + >>>> +#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, >>>> + .mask_bit = 20, >>>> + .quirks = QUIRK_NEEDS_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, >>>> + .mask_bit = 0, >>>> + .quirks = QUIRK_NEEDS_PMU_CONFIG >>>> +}; >>>> + >>>> +static const struct of_device_id s3c2410_wdt_match[] = { >>>> + { .compatible = "samsung,s3c2410-wdt", >>>> + .data = &drv_data_s3c2410 }, >>>> + { .compatible = "samsung,exynos5250-wdt", >>>> + .data = &drv_data_exynos5250 }, >>>> + { .compatible = "samsung,exynos5420-wdt", >>>> + .data = &drv_data_exynos5420 }, >>>> + {}, >>>> }; >>>> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >>>> +#endif >>>> + >>>> +static const struct platform_device_id s3c2410_wdt_ids[] = { >>>> + { >>>> + .name = "s3c2410-wdt", >>>> + .driver_data = (unsigned long)&drv_data_s3c2410, >>>> + }, >>>> + {} >>>> +}; >>>> +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); >>>> >>>> /* watchdog control routines */ >>>> >>>> @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt >>>> *freq_to_wdt(struct notifier_block *nb) >>>> return container_of(nb, struct s3c2410_wdt, freq_transition); >>>> } >>>> >>>> +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, >>>> bool mask) >>>> +{ >>>> + int ret; >>>> + u32 mask_val = 1 << wdt->drv_data->mask_bit; >>>> + u32 val = 0; >>>> + >>>> + if (mask) >>>> + val = mask_val; >>>> + >>>> + ret = regmap_update_bits(wdt->pmureg, >>>> + wdt->drv_data->disable_reg, >>>> + mask_val, val); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + return regmap_update_bits(wdt->pmureg, >>>> + wdt->drv_data->mask_reset_reg, >>>> + mask_val, val); >>>> +} >>>> + >>>> static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >>>> { >>>> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); >>>> @@ -332,6 +407,20 @@ static inline void >>>> s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >>>> } >>>> #endif >>>> >>>> +/* s3c2410_get_wdt_driver_data */ >>>> +static inline struct s3c2410_wdt_variant * >>>> +get_wdt_drv_data(struct platform_device *pdev) >>>> +{ >>>> + if (pdev->dev.of_node) { >>>> + const struct of_device_id *match; >>>> + match = of_match_node(s3c2410_wdt_match, >>>> pdev->dev.of_node); >>>> + return (struct s3c2410_wdt_variant *)match->data; >>>> + } else { >>>> + return (struct s3c2410_wdt_variant *) >>>> + platform_get_device_id(pdev)->driver_data; >>>> + } >>>> +} >>>> + >>>> static int s3c2410wdt_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev; >>>> @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device >>>> *pdev) >>>> spin_lock_init(&wdt->lock); >>>> wdt->wdt_device = s3c2410_wdd; >>>> >>>> + wdt->drv_data = get_wdt_drv_data(pdev); >>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>>> + wdt->pmureg = >>>> syscon_regmap_lookup_by_phandle(dev->of_node, >>>> + >>>> "samsung,syscon-phandle"); >>>> + if (IS_ERR(wdt->pmureg)) { >>>> + dev_err(dev, "syscon regmap lookup failed.\n"); >>>> + return PTR_ERR(wdt->pmureg); >>>> + } >>>> + } >>>> + >>>> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>>> if (wdt_irq == NULL) { >>>> dev_err(dev, "no irq resource specified\n"); >>>> @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device >>>> *pdev) >>>> (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", >>>> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); >>>> >>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>>> + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); >>>> + if (ret < 0) { >>>> + dev_err(wdt->dev, "failed to update pmu >>>> register"); >>> >>> >>> Hmm ... still not happy ;-). This message is the same for each call >>> to s3c2410wdt_mask_and_disable_reset(). Why not create it there ? >>> Sure, you'd have to decide if you want to use dev_warn() or dev_err(), >>> but that would still be better than repeating the same message (and code) >>> five times. >>> >>> The same is true for the if() statement. It might be worthwhile calling >>> s3c2410wdt_mask_and_disable_reset() unconditionally and adding something >>> like >>> >>> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)) >>> return 0; >>> >>> to it. >>> >> >> As Tomasz Figa suggested I handled the error conditions here >> Please go through this link for your reference >> https://patchwork.kernel.org/patch/3118771/ >> > > His proposed function had the error message in the function, > so I am not entirely following your logic. > > He said you should _handle_ the error condition in the calling code. > Dumping an error message and doing nothing isn't really "handling" > the error condition. > I know printing an error message is not really "handling" the error condition. But apart from probe function I don't have anything to handle in remove, shutdown, suspend and resume functions. In remove, shutdown, suspend funtions I must do stop/deregister the device even if regmap_update_bits fails so I simply do dev_warn and continuing So I removed the error message prints in the s3c2410wdt_mask_and_disable_reset function and added it in the above mentioned functions as part of error handling. Best Wishes, Leela Krishna. > Guenter > >> Best Wishes, >> Leela Krishna. >> >>> Thanks, >>> Guenter >>> >>>> + goto err_cpufreq; >>>> + } >>>> + } >>>> + >>>> return 0; >>>> >>>> err_cpufreq: >>>> @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device >>>> *pdev) >>>> >>>> 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) >>>> + dev_warn(wdt->dev, "failed to update pmu >>>> register"); >>>> + } >>>> + >>>> watchdog_unregister_device(&wdt->wdt_device); >>>> >>>> s3c2410wdt_cpufreq_deregister(wdt); >>>> @@ -473,8 +587,15 @@ static int s3c2410wdt_remove(struct platform_device >>>> *dev) >>>> >>>> static void s3c2410wdt_shutdown(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) >>>> + dev_warn(wdt->dev, "failed to update pmu >>>> register"); >>>> + } >>>> + >>>> s3c2410wdt_stop(&wdt->wdt_device); >>>> } >>>> >>>> @@ -482,12 +603,19 @@ static void s3c2410wdt_shutdown(struct >>>> platform_device *dev) >>>> >>>> static int s3c2410wdt_suspend(struct device *dev) >>>> { >>>> + int ret; >>>> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >>>> >>>> /* Save watchdog state, and turn it off. */ >>>> 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) >>>> + dev_warn(wdt->dev, "failed to update pmu >>>> register"); >>>> + } >>>> + >>>> /* Note that WTCNT doesn't need to be saved. */ >>>> s3c2410wdt_stop(&wdt->wdt_device); >>>> >>>> @@ -496,6 +624,7 @@ static int s3c2410wdt_suspend(struct device *dev) >>>> >>>> static int s3c2410wdt_resume(struct device *dev) >>>> { >>>> + int ret; >>>> struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >>>> >>>> /* Restore watchdog state. */ >>>> @@ -503,6 +632,12 @@ 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) >>>> + dev_warn(wdt->dev, "failed to update pmu >>>> register"); >>>> + } >>>> + >>>> dev_info(dev, "watchdog %sabled\n", >>>> (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); >>>> >>>> @@ -513,18 +648,11 @@ static int s3c2410wdt_resume(struct device *dev) >>>> static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, >>>> s3c2410wdt_resume); >>>> >>>> -#ifdef CONFIG_OF >>>> -static const struct of_device_id s3c2410_wdt_match[] = { >>>> - { .compatible = "samsung,s3c2410-wdt" }, >>>> - {}, >>>> -}; >>>> -MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >>>> -#endif >>>> - >>>> static struct platform_driver s3c2410wdt_driver = { >>>> .probe = s3c2410wdt_probe, >>>> .remove = s3c2410wdt_remove, >>>> .shutdown = s3c2410wdt_shutdown, >>>> + .id_table = s3c2410_wdt_ids, >>>> .driver = { >>>> .owner = THIS_MODULE, >>>> .name = "s3c2410-wdt", >>>> @@ -540,4 +668,3 @@ MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>, " >>>> MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver"); >>>> MODULE_LICENSE("GPL"); >>>> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); >>>> -MODULE_ALIAS("platform:s3c2410-wdt"); >>>> -- >>>> 1.7.10.4 >>>> >>>> >>> -- >>> 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-watchdog" >> 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 -- 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 11/18/2013 09:26 PM, Leela Krishna Amudala wrote: > Hi Guenter Roeck, > > On Tue, Nov 19, 2013 at 10:30 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 11/18/2013 08:36 PM, Leela Krishna Amudala wrote: >>> >>> Hi Guenter Roeck, >>> >>> Thanks for reviewing the patch. >>> >>> >>> On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck <linux@roeck-us.net> >>> wrote: >>>> >>>> On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote: >>>>> >>>>> Add device tree support for exynos5250 and 5420 SoCs and use syscon >>>>> regmap interface >>>>> to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST >>>>> registers of PMU >>>>> to mask/unmask enable/disable of watchdog in probe and s2r scenarios. >>>>> >>>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >>>>> --- >>>>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- >>>>> drivers/watchdog/Kconfig | 1 + >>>>> drivers/watchdog/s3c2410_wdt.c | 145 >>>>> ++++++++++++++++++-- >>>>> 3 files changed, 157 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>>> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>>> index 2aa486c..5dea363 100644 >>>>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>>>> @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT >>>>> reset event has not >>>>> occurred. >>>>> >>>>> Required properties: >>>>> -- compatible : should be "samsung,s3c2410-wdt" >>>>> +- compatible : should be one among the following >>>>> + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs >>>>> + (b) "samsung,exynos5250-wdt" for Exynos5250 >>>>> + (c) "samsung,exynos5420-wdt" for Exynos5420 >>>>> + >>>>> - reg : base physical address of the controller and length of memory >>>>> mapped >>>>> region. >>>>> - interrupts : interrupt number to the cpu. >>>>> +- samsung,syscon-phandle : reference to syscon node (This property >>>>> required only >>>>> + in case of compatible being "samsung,exynos5250-wdt" or >>>>> "samsung,exynos5420-wdt". >>>>> + In case of Exynos5250 and 5420 this property points to syscon node >>>>> holding the PMU >>>>> + base address) >>>>> >>>>> Optional properties: >>>>> - timeout-sec : contains the watchdog timeout in seconds. >>>>> + >>>>> +Example: >>>>> + >>>>> +watchdog@101D0000 { >>>>> + compatible = "samsung,exynos5250-wdt"; >>>>> + reg = <0x101D0000 0x100>; >>>>> + interrupts = <0 42 0>; >>>>> + clocks = <&clock 336>; >>>>> + clock-names = "watchdog"; >>>>> + samsung,syscon-phandle = <&pmu_sys_reg>; >>>>> +}; >>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>>> index d1d53f3..0d272ae 100644 >>>>> --- a/drivers/watchdog/Kconfig >>>>> +++ b/drivers/watchdog/Kconfig >>>>> @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG >>>>> tristate "S3C2410 Watchdog" >>>>> depends on HAVE_S3C2410_WATCHDOG >>>>> select WATCHDOG_CORE >>>>> + select MFD_SYSCON if ARCH_EXYNOS5 >>>>> help >>>>> Watchdog timer block in the Samsung SoCs. This will reboot >>>>> the system when the timer expires with the watchdog enabled. >>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c >>>>> b/drivers/watchdog/s3c2410_wdt.c >>>>> index 23aad7c..42e0fd3 100644 >>>>> --- a/drivers/watchdog/s3c2410_wdt.c >>>>> +++ b/drivers/watchdog/s3c2410_wdt.c >>>>> @@ -41,6 +41,8 @@ >>>>> #include <linux/slab.h> >>>>> #include <linux/err.h> >>>>> #include <linux/of.h> >>>>> +#include <linux/mfd/syscon.h> >>>>> +#include <linux/regmap.h> >>>>> >>>>> #define S3C2410_WTCON 0x00 >>>>> #define S3C2410_WTDAT 0x04 >>>>> @@ -61,6 +63,10 @@ >>>>> #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) >>>>> + >>>>> static bool nowayout = WATCHDOG_NOWAYOUT; >>>>> static int tmr_margin; >>>>> static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; >>>>> @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set >>>>> to 1 to ignore reboots, " >>>>> "0 to reboot (default 0)"); >>>>> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default >>>>> 0)"); >>>>> >>>>> +struct s3c2410_wdt_variant { >>>>> + int disable_reg; >>>>> + int mask_reset_reg; >>>>> + int mask_bit; >>>>> + u32 quirks; >>>>> +}; >>>>> + >>>>> struct s3c2410_wdt { >>>>> struct device *dev; >>>>> struct clk *clock; >>>>> @@ -94,7 +107,49 @@ struct s3c2410_wdt { >>>>> unsigned long wtdat_save; >>>>> struct watchdog_device wdt_device; >>>>> struct notifier_block freq_transition; >>>>> + struct s3c2410_wdt_variant *drv_data; >>>>> + struct regmap *pmureg; >>>>> +}; >>>>> + >>>>> +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { >>>>> + .quirks = 0 >>>>> +}; >>>>> + >>>>> +#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, >>>>> + .mask_bit = 20, >>>>> + .quirks = QUIRK_NEEDS_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, >>>>> + .mask_bit = 0, >>>>> + .quirks = QUIRK_NEEDS_PMU_CONFIG >>>>> +}; >>>>> + >>>>> +static const struct of_device_id s3c2410_wdt_match[] = { >>>>> + { .compatible = "samsung,s3c2410-wdt", >>>>> + .data = &drv_data_s3c2410 }, >>>>> + { .compatible = "samsung,exynos5250-wdt", >>>>> + .data = &drv_data_exynos5250 }, >>>>> + { .compatible = "samsung,exynos5420-wdt", >>>>> + .data = &drv_data_exynos5420 }, >>>>> + {}, >>>>> }; >>>>> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); >>>>> +#endif >>>>> + >>>>> +static const struct platform_device_id s3c2410_wdt_ids[] = { >>>>> + { >>>>> + .name = "s3c2410-wdt", >>>>> + .driver_data = (unsigned long)&drv_data_s3c2410, >>>>> + }, >>>>> + {} >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); >>>>> >>>>> /* watchdog control routines */ >>>>> >>>>> @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt >>>>> *freq_to_wdt(struct notifier_block *nb) >>>>> return container_of(nb, struct s3c2410_wdt, freq_transition); >>>>> } >>>>> >>>>> +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, >>>>> bool mask) >>>>> +{ >>>>> + int ret; >>>>> + u32 mask_val = 1 << wdt->drv_data->mask_bit; >>>>> + u32 val = 0; >>>>> + >>>>> + if (mask) >>>>> + val = mask_val; >>>>> + >>>>> + ret = regmap_update_bits(wdt->pmureg, >>>>> + wdt->drv_data->disable_reg, >>>>> + mask_val, val); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + return regmap_update_bits(wdt->pmureg, >>>>> + wdt->drv_data->mask_reset_reg, >>>>> + mask_val, val); >>>>> +} >>>>> + >>>>> static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >>>>> { >>>>> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); >>>>> @@ -332,6 +407,20 @@ static inline void >>>>> s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >>>>> } >>>>> #endif >>>>> >>>>> +/* s3c2410_get_wdt_driver_data */ >>>>> +static inline struct s3c2410_wdt_variant * >>>>> +get_wdt_drv_data(struct platform_device *pdev) >>>>> +{ >>>>> + if (pdev->dev.of_node) { >>>>> + const struct of_device_id *match; >>>>> + match = of_match_node(s3c2410_wdt_match, >>>>> pdev->dev.of_node); >>>>> + return (struct s3c2410_wdt_variant *)match->data; >>>>> + } else { >>>>> + return (struct s3c2410_wdt_variant *) >>>>> + platform_get_device_id(pdev)->driver_data; >>>>> + } >>>>> +} >>>>> + >>>>> static int s3c2410wdt_probe(struct platform_device *pdev) >>>>> { >>>>> struct device *dev; >>>>> @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device >>>>> *pdev) >>>>> spin_lock_init(&wdt->lock); >>>>> wdt->wdt_device = s3c2410_wdd; >>>>> >>>>> + wdt->drv_data = get_wdt_drv_data(pdev); >>>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>>>> + wdt->pmureg = >>>>> syscon_regmap_lookup_by_phandle(dev->of_node, >>>>> + >>>>> "samsung,syscon-phandle"); >>>>> + if (IS_ERR(wdt->pmureg)) { >>>>> + dev_err(dev, "syscon regmap lookup failed.\n"); >>>>> + return PTR_ERR(wdt->pmureg); >>>>> + } >>>>> + } >>>>> + >>>>> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>>>> if (wdt_irq == NULL) { >>>>> dev_err(dev, "no irq resource specified\n"); >>>>> @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device >>>>> *pdev) >>>>> (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", >>>>> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); >>>>> >>>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { >>>>> + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); >>>>> + if (ret < 0) { >>>>> + dev_err(wdt->dev, "failed to update pmu >>>>> register"); >>>> >>>> >>>> Hmm ... still not happy ;-). This message is the same for each call >>>> to s3c2410wdt_mask_and_disable_reset(). Why not create it there ? >>>> Sure, you'd have to decide if you want to use dev_warn() or dev_err(), >>>> but that would still be better than repeating the same message (and code) >>>> five times. >>>> >>>> The same is true for the if() statement. It might be worthwhile calling >>>> s3c2410wdt_mask_and_disable_reset() unconditionally and adding something >>>> like >>>> >>>> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG)) >>>> return 0; >>>> >>>> to it. >>>> >>> >>> As Tomasz Figa suggested I handled the error conditions here >>> Please go through this link for your reference >>> https://patchwork.kernel.org/patch/3118771/ >>> >> >> His proposed function had the error message in the function, >> so I am not entirely following your logic. >> >> He said you should _handle_ the error condition in the calling code. >> Dumping an error message and doing nothing isn't really "handling" >> the error condition. >> > > I know printing an error message is not really "handling" the error condition. > But apart from probe function I don't have anything to handle in remove, > shutdown, suspend and resume functions. > In remove, shutdown, suspend funtions I must do stop/deregister > the device even if regmap_update_bits fails so I simply do dev_warn > and continuing > > So I removed the error message prints in the s3c2410wdt_mask_and_disable_reset > function and added it in the above mentioned functions as part of > error handling. > That doesn't make sense to me. I'll leave it up to Wim to decide how to handle this patch. 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 Leela Krishna, On Mon, Nov 18, 2013 at 1:49 AM, Leela Krishna Amudala <l.krishna@samsung.com> wrote: > Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface > to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU > to mask/unmask enable/disable of watchdog in probe and s2r scenarios. > > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> > --- > .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- > drivers/watchdog/Kconfig | 1 + > drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- > 3 files changed, 157 insertions(+), 10 deletions(-) ... > +struct s3c2410_wdt_variant { > + int disable_reg; > + int mask_reset_reg; > + int mask_bit; > + u32 quirks; > +}; Ideally you could add descriptions. I added them in a patch based on yours <https://chromium-review.googlesource.com/#/c/177935/1/drivers/watchdog/s3c2410_wdt.c>, but since yours hasn't landed perhaps you can do it directly? /** * struct s3c2410_wdt_variant - Per-variant config data * * @disable_reg: Offset in pmureg for the register that disables the watchdog * timer reset functionality. * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog * timer reset functionality. * @mask_bit: Bit number for the watchdog timer in the disable register and the * mask reset register. * @quirks: A bitfield of quirks. */ > + > struct s3c2410_wdt { > struct device *dev; > struct clk *clock; > @@ -94,7 +107,49 @@ struct s3c2410_wdt { > unsigned long wtdat_save; > struct watchdog_device wdt_device; > struct notifier_block freq_transition; > + struct s3c2410_wdt_variant *drv_data; > + struct regmap *pmureg; Total nit, but everything else in this structure is tab aligned and your new elements are not. > static int s3c2410wdt_probe(struct platform_device *pdev) > { > struct device *dev; > @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > spin_lock_init(&wdt->lock); > wdt->wdt_device = s3c2410_wdd; > > + wdt->drv_data = get_wdt_drv_data(pdev); > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > + "samsung,syscon-phandle"); > + if (IS_ERR(wdt->pmureg)) { > + dev_err(dev, "syscon regmap lookup failed.\n"); > + return PTR_ERR(wdt->pmureg); nit: this function appears to never call "return" directly. You'll match other error handling better if you do: ret = PTR_ERR(wdt->pmureg); goto err; (if someone else has already said they don't like that, just ignore me). > + } > + } > + > wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (wdt_irq == NULL) { > dev_err(dev, "no irq resource specified\n"); > @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); > > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); > + if (ret < 0) { > + dev_err(wdt->dev, "failed to update pmu register"); > + goto err_cpufreq; > + } > + } There are a few minor problems here: 1. You're putting a potential failure condition _after_ printing that the watchdog is enabled. You should put your code above the "dev_info". 2. Error handling doesn't seem quite right. If you fail here you'll be returning an error but you might have started the watchdog already. Perhaps you should be moving the "mask_and_disable" up a little and then you an undo it if needed? 3. I think it would be fine to put the "dev_err" directly in the s3c2410wdt_mask_and_disable_reset() as per Guenter, but still return the error code. You'll still use the error code here, right? > + > return 0; > > err_cpufreq: > @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > 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); This function does return an error. Why not pass the error on through? The system wouldn't be in such a stellar state, but if regmap is failing it's probably pretty bad anyway... Nothing here is terrible and I wouldn't be upset if you landed these as-is, but since it seems that Guenter is requesting a spin. Perhaps you could address my comments, too? -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
On Monday 25 of November 2013 14:44:01 Doug Anderson wrote: > > + > > struct s3c2410_wdt { > > struct device *dev; > > struct clk *clock; > > @@ -94,7 +107,49 @@ struct s3c2410_wdt { > > unsigned long wtdat_save; > > struct watchdog_device wdt_device; > > struct notifier_block freq_transition; > > + struct s3c2410_wdt_variant *drv_data; > > + struct regmap *pmureg; > > Total nit, but everything else in this structure is tab aligned and > your new elements are not. That would mean adding extra tabs in lines above to align them with the longest drv_data field. AFAIK we're observing a trend of moving away from such field alignment, so IMHO it would be better to keep this as is in this version and just send a separate patch removing the alignment of remaining fields. > > > static int s3c2410wdt_probe(struct platform_device *pdev) > > { > > struct device *dev; > > @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > spin_lock_init(&wdt->lock); > > wdt->wdt_device = s3c2410_wdd; > > > > + wdt->drv_data = get_wdt_drv_data(pdev); > > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > > + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > > + "samsung,syscon-phandle"); > > + if (IS_ERR(wdt->pmureg)) { > > + dev_err(dev, "syscon regmap lookup failed.\n"); > > + return PTR_ERR(wdt->pmureg); > > nit: this function appears to never call "return" directly. You'll > match other error handling better if you do: > > ret = PTR_ERR(wdt->pmureg); > goto err; Jumping away just to a single return statement isn't really a good idea. If there is nothing to do, returning right away seems less confusing IMHO (and saves you one line of code per each such error case as a side effect). A separate patch could be possibly made to clean-up remaining error cases and remove the err label. As for all the remaining points, I agree with Doug. 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, Thanks for reviewing the patch series. On Tue, Nov 26, 2013 at 4:14 AM, Doug Anderson <dianders@chromium.org> wrote: > > Hi Leela Krishna, > > On Mon, Nov 18, 2013 at 1:49 AM, Leela Krishna Amudala > <l.krishna@samsung.com> wrote: > > Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface > > to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU > > to mask/unmask enable/disable of watchdog in probe and s2r scenarios. > > > > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> > > --- > > .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- > > drivers/watchdog/Kconfig | 1 + > > drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- > > 3 files changed, 157 insertions(+), 10 deletions(-) > > ... > > > +struct s3c2410_wdt_variant { > > + int disable_reg; > > + int mask_reset_reg; > > + int mask_bit; > > + u32 quirks; > > +}; > > Ideally you could add descriptions. I added them in a patch based on > yours <https://chromium-review.googlesource.com/#/c/177935/1/drivers/watchdog/s3c2410_wdt.c>, > but since yours hasn't landed perhaps you can do it directly? > > /** > * struct s3c2410_wdt_variant - Per-variant config data > * > * @disable_reg: Offset in pmureg for the register that disables the watchdog > * timer reset functionality. > * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog > * timer reset functionality. > * @mask_bit: Bit number for the watchdog timer in the disable register and the > * mask reset register. > * @quirks: A bitfield of quirks. > */ > Okay, Added the above descriptions > > + > > struct s3c2410_wdt { > > struct device *dev; > > struct clk *clock; > > @@ -94,7 +107,49 @@ struct s3c2410_wdt { > > unsigned long wtdat_save; > > struct watchdog_device wdt_device; > > struct notifier_block freq_transition; > > + struct s3c2410_wdt_variant *drv_data; > > + struct regmap *pmureg; > > Total nit, but everything else in this structure is tab aligned and > your new elements are not. > Me and Tomasz had a discussion about it and he replied for this comment > > static int s3c2410wdt_probe(struct platform_device *pdev) > > { > > struct device *dev; > > @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > spin_lock_init(&wdt->lock); > > wdt->wdt_device = s3c2410_wdd; > > > > + wdt->drv_data = get_wdt_drv_data(pdev); > > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > > + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > > + "samsung,syscon-phandle"); > > + if (IS_ERR(wdt->pmureg)) { > > + dev_err(dev, "syscon regmap lookup failed.\n"); > > + return PTR_ERR(wdt->pmureg); > > nit: this function appears to never call "return" directly. You'll > match other error handling better if you do: > > ret = PTR_ERR(wdt->pmureg); > goto err; > > (if someone else has already said they don't like that, just ignore me). > Will consider Tomasz suggestion for this > > + } > > + } > > + > > wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > if (wdt_irq == NULL) { > > dev_err(dev, "no irq resource specified\n"); > > @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", > > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); > > > > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { > > + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); > > + if (ret < 0) { > > + dev_err(wdt->dev, "failed to update pmu register"); > > + goto err_cpufreq; > > + } > > + } > > There are a few minor problems here: > > 1. You're putting a potential failure condition _after_ printing that > the watchdog is enabled. You should put your code above the > "dev_info". > Yes moved it like below static int s3c2410wdt_probe(struct platform_device *pdev) { [snip] if (tmr_atboot && started == 0) { dev_info(dev, "starting watchdog timer\n"); s3c2410wdt_start(&wdt->wdt_device); } [snip] > 2. Error handling doesn't seem quite right. If you fail here you'll > be returning an error but you might have started the watchdog already. > Perhaps you should be moving the "mask_and_disable" up a little and > then you an undo it if needed? > Above mentioned comment will take care of this > 3. I think it would be fine to put the "dev_err" directly in the > s3c2410wdt_mask_and_disable_reset() as per Guenter, but still return > the error code. You'll still use the error code here, right? > Yes, used dev_err and moved it into s3c2410wdt_mask_and_disable_reset() and removed the dev_err message in the if (ret < 0) check > > + > > return 0; > > > > err_cpufreq: > > @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > > > 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); > > This function does return an error. Why not pass the error on > through? The system wouldn't be in such a stellar state, but if > regmap is failing it's probably pretty bad anyway... > > Yes, returning the error here and doing the same in other functions also like suspend and resume Will post the next version tomorrow Best Wishes, Leela Krishna. > > Nothing here is terrible and I wouldn't be upset if you landed these > as-is, but since it seems that Guenter is requesting a spin. Perhaps > you could address my comments, too? > > > -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 -- 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/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index 2aa486c..5dea363 100644 --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset event has not occurred. Required properties: -- compatible : should be "samsung,s3c2410-wdt" +- compatible : should be one among the following + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs + (b) "samsung,exynos5250-wdt" for Exynos5250 + (c) "samsung,exynos5420-wdt" for Exynos5420 + - reg : base physical address of the controller and length of memory mapped region. - interrupts : interrupt number to the cpu. +- samsung,syscon-phandle : reference to syscon node (This property required only + in case of compatible being "samsung,exynos5250-wdt" or "samsung,exynos5420-wdt". + In case of Exynos5250 and 5420 this property points to syscon node holding the PMU + base address) Optional properties: - timeout-sec : contains the watchdog timeout in seconds. + +Example: + +watchdog@101D0000 { + compatible = "samsung,exynos5250-wdt"; + reg = <0x101D0000 0x100>; + interrupts = <0 42 0>; + clocks = <&clock 336>; + clock-names = "watchdog"; + samsung,syscon-phandle = <&pmu_sys_reg>; +}; diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index d1d53f3..0d272ae 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG tristate "S3C2410 Watchdog" depends on HAVE_S3C2410_WATCHDOG select WATCHDOG_CORE + select MFD_SYSCON if ARCH_EXYNOS5 help Watchdog timer block in the Samsung SoCs. This will reboot the system when the timer expires with the watchdog enabled. diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index 23aad7c..42e0fd3 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -41,6 +41,8 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> #define S3C2410_WTCON 0x00 #define S3C2410_WTDAT 0x04 @@ -61,6 +63,10 @@ #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) + static bool nowayout = WATCHDOG_NOWAYOUT; static int tmr_margin; static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT; @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, " "0 to reboot (default 0)"); MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)"); +struct s3c2410_wdt_variant { + int disable_reg; + int mask_reset_reg; + int mask_bit; + u32 quirks; +}; + struct s3c2410_wdt { struct device *dev; struct clk *clock; @@ -94,7 +107,49 @@ struct s3c2410_wdt { unsigned long wtdat_save; struct watchdog_device wdt_device; struct notifier_block freq_transition; + struct s3c2410_wdt_variant *drv_data; + struct regmap *pmureg; +}; + +static const struct s3c2410_wdt_variant drv_data_s3c2410 = { + .quirks = 0 +}; + +#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, + .mask_bit = 20, + .quirks = QUIRK_NEEDS_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, + .mask_bit = 0, + .quirks = QUIRK_NEEDS_PMU_CONFIG +}; + +static const struct of_device_id s3c2410_wdt_match[] = { + { .compatible = "samsung,s3c2410-wdt", + .data = &drv_data_s3c2410 }, + { .compatible = "samsung,exynos5250-wdt", + .data = &drv_data_exynos5250 }, + { .compatible = "samsung,exynos5420-wdt", + .data = &drv_data_exynos5420 }, + {}, }; +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); +#endif + +static const struct platform_device_id s3c2410_wdt_ids[] = { + { + .name = "s3c2410-wdt", + .driver_data = (unsigned long)&drv_data_s3c2410, + }, + {} +}; +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); /* watchdog control routines */ @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) return container_of(nb, struct s3c2410_wdt, freq_transition); } +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) +{ + int ret; + u32 mask_val = 1 << wdt->drv_data->mask_bit; + u32 val = 0; + + if (mask) + val = mask_val; + + ret = regmap_update_bits(wdt->pmureg, + wdt->drv_data->disable_reg, + mask_val, val); + if (ret < 0) + return ret; + + return regmap_update_bits(wdt->pmureg, + wdt->drv_data->mask_reset_reg, + mask_val, val); +} + static int s3c2410wdt_keepalive(struct watchdog_device *wdd) { struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); @@ -332,6 +407,20 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) } #endif +/* s3c2410_get_wdt_driver_data */ +static inline struct s3c2410_wdt_variant * +get_wdt_drv_data(struct platform_device *pdev) +{ + if (pdev->dev.of_node) { + const struct of_device_id *match; + match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); + return (struct s3c2410_wdt_variant *)match->data; + } else { + return (struct s3c2410_wdt_variant *) + platform_get_device_id(pdev)->driver_data; + } +} + static int s3c2410wdt_probe(struct platform_device *pdev) { struct device *dev; @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) spin_lock_init(&wdt->lock); wdt->wdt_device = s3c2410_wdd; + wdt->drv_data = get_wdt_drv_data(pdev); + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, + "samsung,syscon-phandle"); + if (IS_ERR(wdt->pmureg)) { + dev_err(dev, "syscon regmap lookup failed.\n"); + return PTR_ERR(wdt->pmureg); + } + } + wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (wdt_irq == NULL) { dev_err(dev, "no irq resource specified\n"); @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) { + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); + if (ret < 0) { + dev_err(wdt->dev, "failed to update pmu register"); + goto err_cpufreq; + } + } + return 0; err_cpufreq: @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev) 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) + dev_warn(wdt->dev, "failed to update pmu register"); + } + watchdog_unregister_device(&wdt->wdt_device); s3c2410wdt_cpufreq_deregister(wdt); @@ -473,8 +587,15 @@ static int s3c2410wdt_remove(struct platform_device *dev) static void s3c2410wdt_shutdown(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) + dev_warn(wdt->dev, "failed to update pmu register"); + } + s3c2410wdt_stop(&wdt->wdt_device); } @@ -482,12 +603,19 @@ static void s3c2410wdt_shutdown(struct platform_device *dev) static int s3c2410wdt_suspend(struct device *dev) { + int ret; struct s3c2410_wdt *wdt = dev_get_drvdata(dev); /* Save watchdog state, and turn it off. */ 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) + dev_warn(wdt->dev, "failed to update pmu register"); + } + /* Note that WTCNT doesn't need to be saved. */ s3c2410wdt_stop(&wdt->wdt_device); @@ -496,6 +624,7 @@ static int s3c2410wdt_suspend(struct device *dev) static int s3c2410wdt_resume(struct device *dev) { + int ret; struct s3c2410_wdt *wdt = dev_get_drvdata(dev); /* Restore watchdog state. */ @@ -503,6 +632,12 @@ 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) + dev_warn(wdt->dev, "failed to update pmu register"); + } + dev_info(dev, "watchdog %sabled\n", (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); @@ -513,18 +648,11 @@ static int s3c2410wdt_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, s3c2410wdt_resume); -#ifdef CONFIG_OF -static const struct of_device_id s3c2410_wdt_match[] = { - { .compatible = "samsung,s3c2410-wdt" }, - {}, -}; -MODULE_DEVICE_TABLE(of, s3c2410_wdt_match); -#endif - static struct platform_driver s3c2410wdt_driver = { .probe = s3c2410wdt_probe, .remove = s3c2410wdt_remove, .shutdown = s3c2410wdt_shutdown, + .id_table = s3c2410_wdt_ids, .driver = { .owner = THIS_MODULE, .name = "s3c2410-wdt", @@ -540,4 +668,3 @@ MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>, " MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver"); MODULE_LICENSE("GPL"); MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); -MODULE_ALIAS("platform:s3c2410-wdt");
Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU to mask/unmask enable/disable of watchdog in probe and s2r scenarios. Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> --- .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++- drivers/watchdog/Kconfig | 1 + drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++-- 3 files changed, 157 insertions(+), 10 deletions(-)