[PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers

[PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers

[Leela Krishna Amudala]

This patchset does the following things
	- Adds pmusysreg device node to exynos5.dtsi file
	- Adds watchdog DT nodes to Exynos5250 and 5420
	- Uses syscon regmap interface to configure pmu registers
	  to mask/unmask enable/disable of watchdog.
	
This patch set is rebased on Kgene's for-next branch and tested on SMDK5420

changes since V8:
	- modified the patch description for the below patch
	  "watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register"
	- removed MODULE_ALIAS in watchdog driver
	- changed dev_warn to dev_err in one failure case handling (suggested by Guenter Roeck <linux@roeck-us.net>)
	- renamed variable name from pmu_config to drv_data
	- changed the compatible field description in documentation file
	  (suggested by Tomasz Figa <t.figa@samsung.com>)

changes since V7:
	- re-ordered the patches in the series
	- moved pmu_config_s3c2410 structure out of ifdef CONFIG_OF
	  and limited only this structure to platform match table
	- renamed structure name from s3c_wdt_driver_ids to s3c2410_wdt_ids
	- removed exynos variants from platform match table
	  (suggested by Tomasz Figa <t.figa@samsung.com>)

changes since V6:
	- added SoC-specific compatible value to syscon node and documented it
	- given more patch description for below patch
	  ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420
	- added platform_device_id array for watchdog devices
	- selected MFD_SYSCON in Kconfig entry for watchdog

Changes since V5:
	- removed status property in DT nodes
	- changed the return type for the function s3c2410wdt_mask_and_disable_reset()
	  and handled error cases
	- Handled to get driver data in non-DT cases
	- Addressed comments given by Tomasz Figa <t.figa@samsung.com>

Changes since V4:
	- changed the node name from pmusysreg to syscon and node label from pmu_sys_reg to pmu_syscon
	- changed the property name from samsung,pmusysreg to samsung,syscon-phandle
	- used regmap_update_bits instead of remap_read/regmap_write
	- Addressed other comments given by Tomasz Figa <t.figa@samsung.com>

Changes since V3:
	- changed the compatible strings for watchdog node
	- splitted up adding pmusysreg node and made it separate patch
	- Addressed comments given by Sachin Kamat <sachin.kamat@linaro.org>

Changes since V2:
	- used syscon regmap interface to configure pmu registers in WDT driver
	  (suggested by Tomasz Figa <t.figa@samsung.com>)

Changes since V1:
        - Added new compatible string for Exynos5 SoCs
        - Introduced quirk mechanism to program PMU registers
        - Addressed comments given by Tomasz Figa <t.figa@samsung.com>

Leela Krishna Amudala (3):
  ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files
  watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu
    register
  ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420

 .../devicetree/bindings/arm/samsung/pmu.txt        |   16 +++
 .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
 arch/arm/boot/dts/exynos5.dtsi                     |    7 -
 arch/arm/boot/dts/exynos5250.dtsi                  |   11 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |   14 ++
 drivers/watchdog/Kconfig                           |    1 +
 drivers/watchdog/s3c2410_wdt.c                     |  145 ++++++++++++++++++--
 7 files changed, 197 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/samsung/pmu.txt

-- 
1.7.10.4

[PATCH V9 1/3] ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files

[Leela Krishna Amudala]

This patch adds pmusysreg node to exynos5250 and exynos5420 dtsi files to
handle PMU register accesses in a centralized way using syscon driver

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
---
 Documentation/devicetree/bindings/arm/samsung/pmu.txt |   16 ++++++++++++++++
 arch/arm/boot/dts/exynos5250.dtsi                     |    5 +++++
 arch/arm/boot/dts/exynos5420.dtsi                     |    5 +++++
 3 files changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/samsung/pmu.txt

diff --git a/Documentation/devicetree/bindings/arm/samsung/pmu.txt b/Documentation/devicetree/bindings/arm/samsung/pmu.txt
new file mode 100644
index 0000000..307e727
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/samsung/pmu.txt
@@ -0,0 +1,16 @@
+SAMSUNG Exynos SoC series PMU Registers
+
+Properties:
+ - name : should be 'syscon';
+ - compatible : should contain two values. First value must be one from following list:
+		   - "samsung,exynos5250-pmu" - for Exynos5250 SoC,
+		   - "samsung,exynos5420-pmu" - for Exynos5420 SoC.
+		second value must be always "syscon".
+
+ - reg : offset and length of the register set.
+
+Example :
+pmu_syscon: syscon@10040000 {
+	compatible = "samsung,exynos5250-pmu", "syscon";
+	reg = <0x10040000 0x5000>;
+};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index c4a8662..6056a83 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -158,6 +158,11 @@
 		interrupts = <0 47 0>;
 	};
 
+	pmu_syscon: syscon@10040000 {
+		compatible = "samsung,exynos5250-pmu", "syscon";
+		reg = <0x10040000 0x5000>;
+	};
+
 	watchdog {
 		clocks = <&clock 336>;
 		clock-names = "watchdog";
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 6ffefd1..39ce15a 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -369,4 +369,9 @@
 		clock-names = "gscl";
 		samsung,power-domain = <&gsc_pd>;
 	};
+
+	pmu_syscon: syscon@10040000 {
+		compatible = "samsung,exynos5420-pmu", "syscon";
+		reg = <0x10040000 0x5000>;
+	};
 };
-- 
1.7.10.4

[PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Leela Krishna Amudala]

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");
+			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

[PATCH V9 3/3] ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420

[Leela Krishna Amudala]

In Exynos5 series SoCs, PMU has registers to enable/disable mask/unmask
watchdog timer which is not the case with s3c series SoCs so, there is a
need to have different compatible names for watchdog to handle these pmu
registers access.

Hence this patch removes watchdog node from Exynos5.dtsi common file and
make it separate by updating existing node in Exynos5250 and adding new node
to Exynos5420. This patch also makes the watchdog node enabled by default

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/boot/dts/exynos5.dtsi    |    7 -------
 arch/arm/boot/dts/exynos5250.dtsi |    6 +++++-
 arch/arm/boot/dts/exynos5420.dtsi |    9 +++++++++
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index e52b038..f1fea28 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -81,13 +81,6 @@
 		status = "disabled";
 	};
 
-	watchdog {
-		compatible = "samsung,s3c2410-wdt";
-		reg = <0x101D0000 0x100>;
-		interrupts = <0 42 0>;
-		status = "disabled";
-	};
-
 	fimd@14400000 {
 		compatible = "samsung,exynos5250-fimd";
 		interrupt-parent = <&combiner>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 6056a83..69f6c6a 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -163,9 +163,13 @@
 		reg = <0x10040000 0x5000>;
 	};
 
-	watchdog {
+	watchdog@101D0000 {
+		compatible = "samsung,exynos5250-wdt";
+		reg = <0x101D0000 0x100>;
+		interrupts = <0 42 0>;
 		clocks = <&clock 336>;
 		clock-names = "watchdog";
+		samsung,syscon-phandle = <&pmu_syscon>;
 	};
 
 	g2d@10850000 {
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 39ce15a..61764bb 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -374,4 +374,13 @@
 		compatible = "samsung,exynos5420-pmu", "syscon";
 		reg = <0x10040000 0x5000>;
 	};
+
+        watchdog@101D0000 {
+		compatible = "samsung,exynos5420-wdt";
+		reg = <0x101D0000 0x100>;
+		interrupts = <0 42 0>;
+		clocks = <&clock 316>;
+		clock-names = "watchdog";
+		samsung,syscon-phandle = <&pmu_syscon>;
+        };
 };
-- 
1.7.10.4

Re: [PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers

[Tomasz Figa]

Hi Leela,

On Monday 18 of November 2013 15:19:46 Leela Krishna Amudala wrote:
> This patchset does the following things
> 	- Adds pmusysreg device node to exynos5.dtsi file
> 	- Adds watchdog DT nodes to Exynos5250 and 5420
> 	- Uses syscon regmap interface to configure pmu registers
> 	  to mask/unmask enable/disable of watchdog.

For the whole series:

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

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
> 
> 

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Leela Krishna Amudala]

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

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

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
>
>

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Leela Krishna Amudala]

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

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

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

Re: [PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers

[Leela Krishna Amudala]

Hello Wim Van Sebroeck,

Can you please review this series and take necessary action.

Best Wishes,
Leela Krishna.

On Mon, Nov 18, 2013 at 4:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Leela,
>
> On Monday 18 of November 2013 15:19:46 Leela Krishna Amudala wrote:
>> This patchset does the following things
>>       - Adds pmusysreg device node to exynos5.dtsi file
>>       - Adds watchdog DT nodes to Exynos5250 and 5420
>>       - Uses syscon regmap interface to configure pmu registers
>>         to mask/unmask enable/disable of watchdog.
>
> For the whole series:
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
>
> 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

Re: [PATCH V9 1/3] ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files

[Doug Anderson]

Hi Leela Krishna,

On Mon, Nov 18, 2013 at 1:49 AM, Leela Krishna Amudala
<l.krishna@samsung.com> wrote:
> This patch adds pmusysreg node to exynos5250 and exynos5420 dtsi files to
> handle PMU register accesses in a centralized way using syscon driver
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> ---
>  Documentation/devicetree/bindings/arm/samsung/pmu.txt |   16 ++++++++++++++++
>  arch/arm/boot/dts/exynos5250.dtsi                     |    5 +++++
>  arch/arm/boot/dts/exynos5420.dtsi                     |    5 +++++
>  3 files changed, 26 insertions(+)

Looks good to me.  I've tested this applied to our local tree
<https://chromium-review.googlesource.com/#/c/177931/> and it's
working well for me.

Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Doug Anderson]

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

Re: [PATCH V9 3/3] ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420

[Doug Anderson]

Leela Krishna,

On Mon, Nov 18, 2013 at 1:49 AM, Leela Krishna Amudala
<l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> In Exynos5 series SoCs, PMU has registers to enable/disable mask/unmask
> watchdog timer which is not the case with s3c series SoCs so, there is a
> need to have different compatible names for watchdog to handle these pmu
> registers access.
>
> Hence this patch removes watchdog node from Exynos5.dtsi common file and
> make it separate by updating existing node in Exynos5250 and adding new node
> to Exynos5420. This patch also makes the watchdog node enabled by default
>
> Signed-off-by: Leela Krishna Amudala <l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Reviewed-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/boot/dts/exynos5.dtsi    |    7 -------
>  arch/arm/boot/dts/exynos5250.dtsi |    6 +++++-
>  arch/arm/boot/dts/exynos5420.dtsi |    9 +++++++++
>  3 files changed, 14 insertions(+), 8 deletions(-)

I've got this locally running in our tree
<https://chromium-review.googlesource.com/#/c/177933/> based on your
patch.

Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Tested-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Tomasz Figa]

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

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Leela Krishna Amudala]

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