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

[PATCH V6 0/3] Add watchdog DT nodes and use syscon regmap interfac 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 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 Exynos5 dtsi file
  ARM: dts: add watchdog device nodes for Exynos5250 and Exynos5420
  watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu
    register

 .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
 arch/arm/boot/dts/exynos5.dtsi                     |   12 +-
 arch/arm/boot/dts/exynos5250.dtsi                  |    6 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |    9 ++
 drivers/watchdog/s3c2410_wdt.c                     |  139 ++++++++++++++++++--
 5 files changed, 170 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH V6 1/3] ARM: dts: Add pmu sysreg node to Exynos5 dtsi file

[Leela Krishna Amudala]

This patch adds pmusysreg node to Exynos5 dtsi file 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>
---
 arch/arm/boot/dts/exynos5.dtsi |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index e52b038..ad6e47c9 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -106,4 +106,9 @@
 		#size-cells = <0>;
 		status = "disabled";
 	};
+
+	pmu_syscon: syscon@10040000 {
+		compatible = "syscon";
+		reg = <0x10040000 0x5000>;
+	};
 };
-- 
1.7.10.4

Re: [PATCH V6 1/3] ARM: dts: Add pmu sysreg node to Exynos5 dtsi file

[Tomasz Figa]

Hi Leela,

On Thursday 07 of November 2013 17:21:16 Leela Krishna Amudala wrote:
> This patch adds pmusysreg node to Exynos5 dtsi file 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>
> ---
>  arch/arm/boot/dts/exynos5.dtsi |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
> index e52b038..ad6e47c9 100644
> --- a/arch/arm/boot/dts/exynos5.dtsi
> +++ b/arch/arm/boot/dts/exynos5.dtsi
> @@ -106,4 +106,9 @@
>  		#size-cells = <0>;
>  		status = "disabled";
>  	};
> +
> +	pmu_syscon: syscon@10040000 {
> +		compatible = "syscon";
> +		reg = <0x10040000 0x5000>;
> +	};
>  };
> 

Just to be safe, I would also add some SoC-specific compatible value to
this node. My proposal is "samsung,exynos5250-pmu" for the PMU of
Exynos5250 (unless all Exynos 5 SoCs really have identical set of PMU
registers, but based on your patch to WDT driver, I don't think so) and
put the node in dtsi of particular SoC.

We already have a PMU driver in arch/arm/mach-exynos and we should rather
allow it to bind to some device tree node and access the registers it
needs when we finally stop mapping PMU registers statically.

You should also document such binding somewhere. IMHO [1] would be a good
place for it.

[1] - Documentation/devicetree/bindings/arm/samsung/pmu.txt

Best regards,
Tomasz

[PATCH V6 2/3] ARM: dts: add watchdog device nodes for Exynos5250 and Exynos5420

[Leela Krishna Amudala]

Adds watchdog device nodes to the DT device list for Exynos5250 and Exynos5420

Signed-off-by: Leela Krishna Amudala <l.krishna@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 ad6e47c9..e6ab5d9 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 c4a8662..42b8682 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -158,9 +158,13 @@
 		interrupts = <0 47 0>;
 	};
 
-	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 6ffefd1..4f5fda3 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -369,4 +369,13 @@
 		clock-names = "gscl";
 		samsung,power-domain = <&gsc_pd>;
 	};
+
+        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 V6 2/3] ARM: dts: add watchdog device nodes for Exynos5250 and Exynos5420

[Tomasz Figa]

Hi Leela,

On Thursday 07 of November 2013 17:21:17 Leela Krishna Amudala wrote:
> Adds watchdog device nodes to the DT device list for Exynos5250 and Exynos5420

s/add/update/ since there was already a device tree node in exynos5.dtsi.

Also, patch description should mention all the changes done in the patch
and I see more changes than described by your description. This patch also
makes the watchdog node enabled by default.

Otherwise the patch looks good.

Best regards,
Tomasz

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

[Leela Krishna Amudala]

The syscon regmap interface is used 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/s3c2410_wdt.c                     |  139 ++++++++++++++++++--
 2 files changed, 151 insertions(+), 9 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/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..a151c20 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,40 @@ struct s3c2410_wdt {
 	unsigned long		wtdat_save;
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
+	struct s3c2410_wdt_variant *pmu_config;
+	struct regmap *pmureg;
+};
+
+#ifdef CONFIG_OF
+static const struct s3c2410_wdt_variant pmu_config_s3c2410 = {
+	.quirks = 0
+};
+
+static const struct s3c2410_wdt_variant pmu_config_5250  = {
+	.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 pmu_config_5420 = {
+	.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 = &pmu_config_s3c2410 },
+	{ .compatible = "samsung,exynos5250-wdt",
+	  .data = &pmu_config_5250 },
+	{ .compatible = "samsung,exynos5420-wdt",
+	  .data = &pmu_config_5420 },
+	{},
 };
+MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
+#endif
 
 /* watchdog control routines */
 
@@ -111,6 +157,30 @@ 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->pmu_config->mask_bit;
+	u32 val = 0;
+
+	if (mask)
+		val = mask_val;
+
+	ret = regmap_update_bits(wdt->pmureg,
+			wdt->pmu_config->disable_reg,
+			mask_val, val);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(wdt->pmureg,
+			wdt->pmu_config->mask_reset_reg,
+			mask_val, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
@@ -332,6 +402,21 @@ 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)
+{
+#ifdef CONFIG_OF
+	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;
+	}
+#endif
+	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 +439,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	spin_lock_init(&wdt->lock);
 	wdt->wdt_device = s3c2410_wdd;
 
+	wdt->pmu_config = get_wdt_drv_data(pdev);
+	if (wdt->pmu_config->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 +539,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		 (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
 		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
 
+	if (wdt->pmu_config->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");
+			goto err_cpufreq;
+		}
+	}
+
 	return 0;
 
  err_cpufreq:
@@ -459,8 +562,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->pmu_config->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 +583,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->pmu_config->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 +599,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->pmu_config->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 +620,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 +628,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->pmu_config->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,14 +644,6 @@ 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,
-- 
1.7.10.4

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

[Tomasz Figa]

Hi Leela,

On Thursday 07 of November 2013 17:21:18 Leela Krishna Amudala wrote:
> The syscon regmap interface is used 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/s3c2410_wdt.c                     |  139 ++++++++++++++++++--
>  2 files changed, 151 insertions(+), 9 deletions(-)
[snip]
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 23aad7c..a151c20 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
[snip]
> +/* s3c2410_get_wdt_driver_data */
> +static inline struct s3c2410_wdt_variant *
> +get_wdt_drv_data(struct platform_device *pdev)
> +{
> +#ifdef CONFIG_OF
> +	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;
> +	}
> +#endif
> +	return (struct s3c2410_wdt_variant *)
> +		platform_get_device_id(pdev)->driver_data;

This code calls platform_get_device_id(), but I don't see any array of
platform_device_id in this driver. This will always crash on a NULL
pointer dereference whenever booted on a platform using board files.

Also, as this driver is now using syscon API, its Kconfig entry should
select MFD_SYSCON.

Other than that, the patch looks fine.

Best regards,
Tomasz