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

[PATCH V4 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 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   |   19 +++-
 arch/arm/boot/dts/exynos5.dtsi                     |   12 +--
 arch/arm/boot/dts/exynos5250.dtsi                  |    7 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |   10 ++
 drivers/watchdog/s3c2410_wdt.c                     |  112 ++++++++++++++++++--
 5 files changed, 143 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH V4 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>
---
 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..918e732 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_sys_reg: pmusysreg@1004000 {
+		compatible = "syscon";
+		reg = <0x10040000 0x5000>;
+	};
 };
-- 
1.7.10.4

[PATCH V4 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 |    7 ++++++-
 arch/arm/boot/dts/exynos5420.dtsi |   10 ++++++++++
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index 918e732..fec8aa7 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..e46b5d9 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -158,9 +158,14 @@
 		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,pmusysreg = <&pmu_sys_reg>;
+		status = "okay";
 	};
 
 	g2d@10850000 {
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 6ffefd1..c95385b 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -369,4 +369,14 @@
 		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,pmusysreg = <&pmu_sys_reg>;
+		status = "okay";
+        };
 };
-- 
1.7.10.4

[PATCH V4 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   |   19 +++-
 drivers/watchdog/s3c2410_wdt.c                     |  112 ++++++++++++++++++--
 2 files changed, 122 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..5a9889b 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -5,10 +5,27 @@ 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,pmusysreg : reference node to pmu sysreg (required only in case of Exynos5250 and 5420)
 
 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,sysreg = <&pmu_sys_reg>;
+	status = "okay";
+};
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..d8aefef 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,12 @@ 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 pmu_config {
+	int	disable_reg;
+	int	mask_reset_reg;
+	int	mask_bit;
+};
+
 struct s3c2410_wdt {
 	struct device		*dev;
 	struct clk		*clock;
@@ -94,7 +106,34 @@ struct s3c2410_wdt {
 	unsigned long		wtdat_save;
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
+	struct pmu_config	*wdt_pmu_config;
+	struct regmap		*pmureg;
+	unsigned int		quirks;
+};
+
+#ifdef CONFIG_OF
+static struct pmu_config pmu_config_5250  = {
+	.disable_reg = WDT_DISABLE_REG_OFFSET,
+	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+	.mask_bit = 20
+};
+
+static struct pmu_config pmu_config_5420 = {
+	.disable_reg = WDT_DISABLE_REG_OFFSET,
+	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+	.mask_bit = 0
+};
+
+static const struct of_device_id s3c2410_wdt_match[] = {
+	{ .compatible = "samsung,s3c2410-wdt" },
+	{ .compatible = "samsung,exynos5250-wdt",
+	  .data = (struct pmu_config *) &pmu_config_5250 },
+	{ .compatible = "samsung,exynos5420-wdt",
+	  .data = (struct pmu_config *) &pmu_config_5420 },
+	{},
 };
+MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
+#endif
 
 /* watchdog control routines */
 
@@ -111,6 +150,38 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
 	return container_of(nb, struct s3c2410_wdt, freq_transition);
 }
 
+static void s3c2410wdt_mask_and_disable_reset(int mask, struct s3c2410_wdt *wdt)
+{
+	unsigned int disable, mask_reset;
+	int ret;
+
+	ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->disable_reg,
+								&disable);
+	if (ret < 0) {
+		dev_err(wdt->dev, "failed to read disable reg(%d)\n", ret);
+		return;
+	}
+
+	ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
+								&mask_reset);
+	if (ret < 0) {
+		dev_err(wdt->dev, "failed to read mask reset reg(%d)\n", ret);
+		return;
+	}
+
+	if (mask) {
+		disable |= (1 << wdt->wdt_pmu_config->mask_bit);
+		mask_reset |= (1 << wdt->wdt_pmu_config->mask_bit);
+	} else {
+		disable &= ~(1 << wdt->wdt_pmu_config->mask_bit);
+		mask_reset &= ~(1 << wdt->wdt_pmu_config->mask_bit);
+	}
+
+	regmap_write(wdt->pmureg, wdt->wdt_pmu_config->disable_reg, disable);
+	regmap_write(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
+								mask_reset);
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
@@ -332,6 +403,14 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
 }
 #endif
 
+/* s3c2410_get_wdt_driver_data */
+static inline unsigned int get_wdt_driver_data(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node);
+	return (unsigned int)match->data;
+}
+
 static int s3c2410wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev;
@@ -354,6 +433,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	spin_lock_init(&wdt->lock);
 	wdt->wdt_device = s3c2410_wdd;
 
+	wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
+						"samsung,pmusysreg");
+	if (IS_ERR(wdt->pmureg)) {
+		dev_err(dev, "syscon regmap lookup failed.\n");
+		return PTR_ERR(wdt->pmureg);
+	}
+
+	wdt->wdt_pmu_config = (struct pmu_config *) get_wdt_driver_data(pdev);
+	if (wdt->wdt_pmu_config)
+		wdt->quirks = QUIRK_NEEDS_PMU_CONFIG;
+
 	wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (wdt_irq == NULL) {
 		dev_err(dev, "no irq resource specified\n");
@@ -444,6 +534,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		 (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
 		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
 
+	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
+		s3c2410wdt_mask_and_disable_reset(0, wdt);
 	return 0;
 
  err_cpufreq:
@@ -461,6 +553,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
+	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
+		s3c2410wdt_mask_and_disable_reset(1, wdt);
+
 	watchdog_unregister_device(&wdt->wdt_device);
 
 	s3c2410wdt_cpufreq_deregister(wdt);
@@ -475,6 +570,9 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
+	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
+		s3c2410wdt_mask_and_disable_reset(1, wdt);
+
 	s3c2410wdt_stop(&wdt->wdt_device);
 }
 
@@ -488,6 +586,9 @@ static int s3c2410wdt_suspend(struct device *dev)
 	wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
 	wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
 
+	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
+		s3c2410wdt_mask_and_disable_reset(1, wdt);
+
 	/* Note that WTCNT doesn't need to be saved. */
 	s3c2410wdt_stop(&wdt->wdt_device);
 
@@ -503,6 +604,9 @@ static int s3c2410wdt_resume(struct device *dev)
 	writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
 	writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);
 
+	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
+		s3c2410wdt_mask_and_disable_reset(0, wdt);
+
 	dev_info(dev, "watchdog %sabled\n",
 		(wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis");
 
@@ -513,14 +617,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 V4 1/3] ARM: dts: Add pmu sysreg node to Exynos5 dtsi file

[Sachin Kamat]

Hi Leela,

On 30 October 2013 15:21, Leela Krishna Amudala <l.krishna@samsung.com> 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>
> ---
>  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..918e732 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_sys_reg: pmusysreg@1004000 {
> +               compatible = "syscon";
> +               reg = <0x10040000 0x5000>;
> +       };
>  };

Had a look at this in a bit detail and found the following.
The register base address for this block on 5250 and 5420 as per the
TRM is 0x10050000.

Also, the binding document specifies the naming convention. According
to it this node
should like:

sys_reg: sysreg@10050000 {
           compatible = "samsung,exynos5-sysreg", "syscon";
           reg = <0x10050000 0x500>;
};

-- 
With warm regards,
Sachin

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

[Leela Krishna Amudala]

Hi,

On Wed, Oct 30, 2013 at 3:22 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Leela,
>
> On 30 October 2013 15:21, Leela Krishna Amudala <l.krishna@samsung.com> 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>
>> ---
>>  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..918e732 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_sys_reg: pmusysreg@1004000 {
>> +               compatible = "syscon";
>> +               reg = <0x10040000 0x5000>;
>> +       };
>>  };
>
> Had a look at this in a bit detail and found the following.
> The register base address for this block on 5250 and 5420 as per the
> TRM is 0x10050000.
>
> Also, the binding document specifies the naming convention. According
> to it this node
> should like:
>
> sys_reg: sysreg@10050000 {
>            compatible = "samsung,exynos5-sysreg", "syscon";
>            reg = <0x10050000 0x500>;
> };
>

I know, but here my intention is not to regmap system register (0x10050000),
but instead PMU register (0x10040000), Hence created this node.

~Leela Krishna

> --
> With warm regards,
> Sachin
> --
> 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 V4 1/3] ARM: dts: Add pmu sysreg node to Exynos5 dtsi file

[Sachin Kamat]

On 30 October 2013 15:39, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> Hi,
>
> On Wed, Oct 30, 2013 at 3:22 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> Hi Leela,
>>
>> On 30 October 2013 15:21, Leela Krishna Amudala <l.krishna@samsung.com> 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>
>>> ---
>>>  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..918e732 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_sys_reg: pmusysreg@1004000 {
>>> +               compatible = "syscon";
>>> +               reg = <0x10040000 0x5000>;
>>> +       };
>>>  };
>>
>> Had a look at this in a bit detail and found the following.
>> The register base address for this block on 5250 and 5420 as per the
>> TRM is 0x10050000.
>>
>> Also, the binding document specifies the naming convention. According
>> to it this node
>> should like:
>>
>> sys_reg: sysreg@10050000 {
>>            compatible = "samsung,exynos5-sysreg", "syscon";
>>            reg = <0x10050000 0x500>;
>> };
>>
>
> I know, but here my intention is not to regmap system register (0x10050000),
> but instead PMU register (0x10040000), Hence created this node.

This clashes with the existing binding for this type of node. Probably
you will need to
define it differently?

-- 
With warm regards,
Sachin

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

[Tomasz Figa]

On Wednesday 30 of October 2013 15:43:19 Sachin Kamat wrote:
> On 30 October 2013 15:39, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2013 at 3:22 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> >> Hi Leela,
> >>
> >> On 30 October 2013 15:21, Leela Krishna Amudala <l.krishna@samsung.com> 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>
> >>> ---
> >>>  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..918e732 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_sys_reg: pmusysreg@1004000 {
> >>> +               compatible = "syscon";
> >>> +               reg = <0x10040000 0x5000>;
> >>> +       };
> >>>  };
> >>
> >> Had a look at this in a bit detail and found the following.
> >> The register base address for this block on 5250 and 5420 as per the
> >> TRM is 0x10050000.
> >>
> >> Also, the binding document specifies the naming convention. According
> >> to it this node
> >> should like:
> >>
> >> sys_reg: sysreg@10050000 {
> >>            compatible = "samsung,exynos5-sysreg", "syscon";
> >>            reg = <0x10050000 0x500>;
> >> };
> >>
> >
> > I know, but here my intention is not to regmap system register (0x10050000),
> > but instead PMU register (0x10040000), Hence created this node.
> 
> This clashes with the existing binding for this type of node. Probably
> you will need to
> define it differently?

PMU and System Registers are two completely separate entities. However
a generic syscon binding can be used to represent both, because they are
just collections of registers shared by multiple IPs.

Best regards,
Tomasz

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

[Tomasz Figa]

Hi Leela,

On Wednesday 30 of October 2013 15:21:13 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   |   19 +++-
>  drivers/watchdog/s3c2410_wdt.c                     |  112 ++++++++++++++++++--
>  2 files changed, 122 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> index 2aa486c..5a9889b 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> @@ -5,10 +5,27 @@ 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,pmusysreg : reference node to pmu sysreg (required only in case of Exynos5250 and 5420)

I don't really like the name of this property. Maybe
samsung,syscon-phandle would be better?

I'm not sure if this property should be tied to PMU, as on future SoCs
those registers might be located in another syscon-style IP. The
description should note that in case of Exynos 5250 and 5420 this should
point to the PMU node, though.

>  
>  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,sysreg = <&pmu_sys_reg>;
> +	status = "okay";
> +};
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 23aad7c..d8aefef 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,12 @@ 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 pmu_config {

For the sake of future extensibility I'd rename this struct to
s3c2410_wdt_variant and add an u32 quirks field, which would has
the QUIRK_NEEDS_PMU_CONFIG set for 5250 and 5420 variants.

> +	int	disable_reg;
> +	int	mask_reset_reg;
> +	int	mask_bit;
> +};
> +
>  struct s3c2410_wdt {
>  	struct device		*dev;
>  	struct clk		*clock;
> @@ -94,7 +106,34 @@ struct s3c2410_wdt {
>  	unsigned long		wtdat_save;
>  	struct watchdog_device	wdt_device;
>  	struct notifier_block	freq_transition;
> +	struct pmu_config	*wdt_pmu_config;
> +	struct regmap		*pmureg;
> +	unsigned int		quirks;

Quirks should be a static per-variant property.

> +};
> +
> +#ifdef CONFIG_OF
> +static struct pmu_config pmu_config_5250  = {
> +	.disable_reg = WDT_DISABLE_REG_OFFSET,
> +	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> +	.mask_bit = 20
> +};
> +
> +static struct pmu_config pmu_config_5420 = {
> +	.disable_reg = WDT_DISABLE_REG_OFFSET,
> +	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> +	.mask_bit = 0
> +};
> +
> +static const struct of_device_id s3c2410_wdt_match[] = {
> +	{ .compatible = "samsung,s3c2410-wdt" },

If you follow what I suggested above, the basic variant would also have
a variant struct associated with it, which would be more consistent.

> +	{ .compatible = "samsung,exynos5250-wdt",
> +	  .data = (struct pmu_config *) &pmu_config_5250 },
> +	{ .compatible = "samsung,exynos5420-wdt",
> +	  .data = (struct pmu_config *) &pmu_config_5420 },
> +	{},
>  };
> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> +#endif
>  
>  /* watchdog control routines */
>  
> @@ -111,6 +150,38 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
>  	return container_of(nb, struct s3c2410_wdt, freq_transition);
>  }
>  
> +static void s3c2410wdt_mask_and_disable_reset(int mask, struct s3c2410_wdt *wdt)
> +{
> +	unsigned int disable, mask_reset;
> +	int ret;
> +
> +	ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->disable_reg,
> +								&disable);
> +	if (ret < 0) {
> +		dev_err(wdt->dev, "failed to read disable reg(%d)\n", ret);
> +		return;
> +	}
> +
> +	ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
> +								&mask_reset);
> +	if (ret < 0) {
> +		dev_err(wdt->dev, "failed to read mask reset reg(%d)\n", ret);
> +		return;
> +	}
> +
> +	if (mask) {
> +		disable |= (1 << wdt->wdt_pmu_config->mask_bit);
> +		mask_reset |= (1 << wdt->wdt_pmu_config->mask_bit);
> +	} else {
> +		disable &= ~(1 << wdt->wdt_pmu_config->mask_bit);
> +		mask_reset &= ~(1 << wdt->wdt_pmu_config->mask_bit);
> +	}
> +
> +	regmap_write(wdt->pmureg, wdt->wdt_pmu_config->disable_reg, disable);
> +	regmap_write(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
> +								mask_reset);

I believe you can (and on some SoCs even have to, due to other bits in the
registers being used for other purposes) use regmap_update_bits() for
read-modify-write sequences above.

> +}
> +
>  static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
>  {
>  	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> @@ -332,6 +403,14 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
>  }
>  #endif
>  
> +/* s3c2410_get_wdt_driver_data */
> +static inline unsigned int get_wdt_driver_data(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node);
> +	return (unsigned int)match->data;

Why do you need this cast? I believe this function should simply return
(const struct pmu_config *) or (const struct s3c2410_wdt_variant *) when
my comments above get addressed.

> +}
> +
>  static int s3c2410wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev;
> @@ -354,6 +433,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  	spin_lock_init(&wdt->lock);
>  	wdt->wdt_device = s3c2410_wdd;
>  
> +	wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						"samsung,pmusysreg");
> +	if (IS_ERR(wdt->pmureg)) {
> +		dev_err(dev, "syscon regmap lookup failed.\n");
> +		return PTR_ERR(wdt->pmureg);
> +	}

This should be performed only if given variant has the
QUIRK_NEEDS_PMU_CONFIG flag set in its variant data. Otherwise you don't
have any distinction between cases when PMU is not needed (and so
unspecified) and PMU is needed, but unspecified.

> +
> +	wdt->wdt_pmu_config = (struct pmu_config *) get_wdt_driver_data(pdev);
> +	if (wdt->wdt_pmu_config)
> +		wdt->quirks = QUIRK_NEEDS_PMU_CONFIG;

See my comment on quirks above.

> +
>  	wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  	if (wdt_irq == NULL) {
>  		dev_err(dev, "no irq resource specified\n");
> @@ -444,6 +534,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  		 (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
>  		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
>  
> +	if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
> +		s3c2410wdt_mask_and_disable_reset(0, wdt);

nit: Blank line here would be nice.

Best regards,
Tomasz

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

[Tomasz Figa]

Hi Leela,

On Wednesday 30 of October 2013 15:21:11 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>
> ---
>  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..918e732 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_sys_reg: pmusysreg@1004000 {

Please keep the node name generic, syscon should be fine. Also there is
a typo in unit-address suffix. The label could also have a better name,
like pmu_syscon.

Best regards,
Tomasz

> +		compatible = "syscon";
> +		reg = <0x10040000 0x5000>;
> +	};
>  };
> 

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

[Guenter Roeck]

On Wed, Oct 30, 2013 at 12:22:09PM +0100, Tomasz Figa wrote:
> On Wednesday 30 of October 2013 15:43:19 Sachin Kamat wrote:
> > On 30 October 2013 15:39, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> > > Hi,
> > >
> > > On Wed, Oct 30, 2013 at 3:22 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> > >> Hi Leela,
> > >>
> > >> On 30 October 2013 15:21, Leela Krishna Amudala <l.krishna@samsung.com> 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>
> > >>> ---
> > >>>  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..918e732 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_sys_reg: pmusysreg@1004000 {
> > >>> +               compatible = "syscon";
> > >>> +               reg = <0x10040000 0x5000>;
> > >>> +       };
> > >>>  };
> > >>
> > >> Had a look at this in a bit detail and found the following.
> > >> The register base address for this block on 5250 and 5420 as per the
> > >> TRM is 0x10050000.
> > >>
> > >> Also, the binding document specifies the naming convention. According
> > >> to it this node
> > >> should like:
> > >>
> > >> sys_reg: sysreg@10050000 {
> > >>            compatible = "samsung,exynos5-sysreg", "syscon";
> > >>            reg = <0x10050000 0x500>;
> > >> };
> > >>
> > >
> > > I know, but here my intention is not to regmap system register (0x10050000),
> > > but instead PMU register (0x10040000), Hence created this node.
> > 
> > This clashes with the existing binding for this type of node. Probably
> > you will need to
> > define it differently?
> 
> PMU and System Registers are two completely separate entities. However
> a generic syscon binding can be used to represent both, because they are
> just collections of registers shared by multiple IPs.
> 

I would suggest for the participants in this discussion to send Reviewed-by:
or Acked-by: feedback once you are happy with the patches. I am sure this
would help Wim tremendously when deciding if the series is ready for
integration.

Thanks,
Guenter

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

[Leela Krishna Amudala]

Hi Tomasz,

On Wed, Oct 30, 2013 at 8:12 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Leela,
>
> On Wednesday 30 of October 2013 15:21:11 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>
>> ---
>>  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..918e732 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_sys_reg: pmusysreg@1004000 {
>
> Please keep the node name generic, syscon should be fine. Also there is
> a typo in unit-address suffix. The label could also have a better name,
> like pmu_syscon.
>

Okay, will change it.
/Leela Krishna

> Best regards,
> Tomasz
>
>> +             compatible = "syscon";
>> +             reg = <0x10040000 0x5000>;
>> +     };
>>  };
>>
>
> --
> 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 V4 3/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Leela Krishna Amudala]

Hi Tomasz,

On Wed, Oct 30, 2013 at 8:09 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Leela,
>
> On Wednesday 30 of October 2013 15:21:13 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   |   19 +++-
>>  drivers/watchdog/s3c2410_wdt.c                     |  112 ++++++++++++++++++--
>>  2 files changed, 122 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>> index 2aa486c..5a9889b 100644
>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>> @@ -5,10 +5,27 @@ 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,pmusysreg : reference node to pmu sysreg (required only in case of Exynos5250 and 5420)
>
> I don't really like the name of this property. Maybe
> samsung,syscon-phandle would be better?
>
Okay, changed.

> I'm not sure if this property should be tied to PMU, as on future SoCs
> those registers might be located in another syscon-style IP. The
> description should note that in case of Exynos 5250 and 5420 this should
> point to the PMU node, though.
>

Added

>>
>>  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,sysreg = <&pmu_sys_reg>;
>> +     status = "okay";
>> +};
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index 23aad7c..d8aefef 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,12 @@ 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 pmu_config {
>
> For the sake of future extensibility I'd rename this struct to
> s3c2410_wdt_variant and add an u32 quirks field, which would has
> the QUIRK_NEEDS_PMU_CONFIG set for 5250 and 5420 variants.
>

Modified

>> +     int     disable_reg;
>> +     int     mask_reset_reg;
>> +     int     mask_bit;
>> +};
>> +
>>  struct s3c2410_wdt {
>>       struct device           *dev;
>>       struct clk              *clock;
>> @@ -94,7 +106,34 @@ struct s3c2410_wdt {
>>       unsigned long           wtdat_save;
>>       struct watchdog_device  wdt_device;
>>       struct notifier_block   freq_transition;
>> +     struct pmu_config       *wdt_pmu_config;
>> +     struct regmap           *pmureg;
>> +     unsigned int            quirks;
>
> Quirks should be a static per-variant property.
>

Taken care

>> +};
>> +
>> +#ifdef CONFIG_OF
>> +static struct pmu_config pmu_config_5250  = {
>> +     .disable_reg = WDT_DISABLE_REG_OFFSET,
>> +     .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>> +     .mask_bit = 20
>> +};
>> +
>> +static struct pmu_config pmu_config_5420 = {
>> +     .disable_reg = WDT_DISABLE_REG_OFFSET,
>> +     .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>> +     .mask_bit = 0
>> +};
>> +
>> +static const struct of_device_id s3c2410_wdt_match[] = {
>> +     { .compatible = "samsung,s3c2410-wdt" },
>
> If you follow what I suggested above, the basic variant would also have
> a variant struct associated with it, which would be more consistent.
>
>> +     { .compatible = "samsung,exynos5250-wdt",
>> +       .data = (struct pmu_config *) &pmu_config_5250 },
>> +     { .compatible = "samsung,exynos5420-wdt",
>> +       .data = (struct pmu_config *) &pmu_config_5420 },
>> +     {},
>>  };
>> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
>> +#endif
>>
>>  /* watchdog control routines */
>>
>> @@ -111,6 +150,38 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
>>       return container_of(nb, struct s3c2410_wdt, freq_transition);
>>  }
>>
>> +static void s3c2410wdt_mask_and_disable_reset(int mask, struct s3c2410_wdt *wdt)
>> +{
>> +     unsigned int disable, mask_reset;
>> +     int ret;
>> +
>> +     ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->disable_reg,
>> +                                                             &disable);
>> +     if (ret < 0) {
>> +             dev_err(wdt->dev, "failed to read disable reg(%d)\n", ret);
>> +             return;
>> +     }
>> +
>> +     ret = regmap_read(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
>> +                                                             &mask_reset);
>> +     if (ret < 0) {
>> +             dev_err(wdt->dev, "failed to read mask reset reg(%d)\n", ret);
>> +             return;
>> +     }
>> +
>> +     if (mask) {
>> +             disable |= (1 << wdt->wdt_pmu_config->mask_bit);
>> +             mask_reset |= (1 << wdt->wdt_pmu_config->mask_bit);
>> +     } else {
>> +             disable &= ~(1 << wdt->wdt_pmu_config->mask_bit);
>> +             mask_reset &= ~(1 << wdt->wdt_pmu_config->mask_bit);
>> +     }
>> +
>> +     regmap_write(wdt->pmureg, wdt->wdt_pmu_config->disable_reg, disable);
>> +     regmap_write(wdt->pmureg, wdt->wdt_pmu_config->mask_reset_reg,
>> +                                                             mask_reset);
>
> I believe you can (and on some SoCs even have to, due to other bits in the
> registers being used for other purposes) use regmap_update_bits() for
> read-modify-write sequences above.
>

Okay, Modified

>> +}
>> +
>>  static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
>>  {
>>       struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
>> @@ -332,6 +403,14 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
>>  }
>>  #endif
>>
>> +/* s3c2410_get_wdt_driver_data */
>> +static inline unsigned int get_wdt_driver_data(struct platform_device *pdev)
>> +{
>> +     const struct of_device_id *match;
>> +     match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node);
>> +     return (unsigned int)match->data;
>
> Why do you need this cast? I believe this function should simply return
> (const struct pmu_config *) or (const struct s3c2410_wdt_variant *) when
> my comments above get addressed.
>

My mistake, Changed

>> +}
>> +
>>  static int s3c2410wdt_probe(struct platform_device *pdev)
>>  {
>>       struct device *dev;
>> @@ -354,6 +433,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>       spin_lock_init(&wdt->lock);
>>       wdt->wdt_device = s3c2410_wdd;
>>
>> +     wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                                             "samsung,pmusysreg");
>> +     if (IS_ERR(wdt->pmureg)) {
>> +             dev_err(dev, "syscon regmap lookup failed.\n");
>> +             return PTR_ERR(wdt->pmureg);
>> +     }
>
> This should be performed only if given variant has the
> QUIRK_NEEDS_PMU_CONFIG flag set in its variant data. Otherwise you don't
> have any distinction between cases when PMU is not needed (and so
> unspecified) and PMU is needed, but unspecified.
>

Okay, Taken care

>> +
>> +     wdt->wdt_pmu_config = (struct pmu_config *) get_wdt_driver_data(pdev);
>> +     if (wdt->wdt_pmu_config)
>> +             wdt->quirks = QUIRK_NEEDS_PMU_CONFIG;
>
> See my comment on quirks above.
>
>> +
>>       wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>       if (wdt_irq == NULL) {
>>               dev_err(dev, "no irq resource specified\n");
>> @@ -444,6 +534,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>                (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
>>                (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
>>
>> +     if (wdt->quirks & QUIRK_NEEDS_PMU_CONFIG)
>> +             s3c2410wdt_mask_and_disable_reset(0, wdt);
>
> nit: Blank line here would be nice.
>

Done.

Best Wishes,
Leela Krishna

> 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