[PATCH 0/4] Add watchdog DT nodes and parse it to read PMU registers addresses

[PATCH 0/4] Add watchdog DT nodes and parse it to read PMU registers addresses

[Leela Krishna Amudala]

This patchset does the following things
	- Fixes the watchdog DT node name to follow convention
	- Adds watchdog DT node to Exynos5420 SoC
	- parse the watchdog device node to read pmu wdt sys registers
	  addresses and do mask/unmask enable/disable of WDT in probe
	  and s2r scenarios.

This patch set is rebased on Kgene's for-next branch and tested on SMDK5420

Leela Krishna Amudala (4):
  ARM: dts: Fix the watchdog DT node name for Exynos5
  ARM: dts: add watchdog device tree node for exynos5420
  watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers
    adresses
  ARM: dts: exynos: add PMU registers addresses and mask bit to
    watchdog node

 .../devicetree/bindings/watchdog/samsung-wdt.txt   |   14 ++++-
 arch/arm/boot/dts/exynos5.dtsi                     |    4 +-
 arch/arm/boot/dts/exynos5250.dtsi                  |    4 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |    7 +++
 drivers/watchdog/s3c2410_wdt.c                     |   56 ++++++++++++++++++++
 5 files changed, 81 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] ARM: dts: Fix the watchdog DT node name for Exynos5

[Leela Krishna Amudala]

Fixes the watchdog DT node name for Exynos5 as per the DT node naming
convention also update "status" property for Exynos5250 SoC.

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 arch/arm/boot/dts/exynos5.dtsi    |    2 +-
 arch/arm/boot/dts/exynos5250.dtsi |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index 074739d..a8bf0c2 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -102,7 +102,7 @@
 		status = "disabled";
 	};
 
-	watchdog {
+	watchdog@101D0000 {
 		compatible = "samsung,s3c2410-wdt";
 		reg = <0x101D0000 0x100>;
 		interrupts = <0 42 0>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 7d7cc77..6bb42cf 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -158,9 +158,10 @@
 		interrupts = <0 47 0>;
 	};
 
-	watchdog {
+	watchdog@101D0000 {
 		clocks = <&clock 336>;
 		clock-names = "watchdog";
+		status = "okay";
 	};
 
 	g2d@10850000 {
-- 
1.7.9.5

[PATCH 2/4] ARM: dts: add watchdog device tree node for exynos5420

[Leela Krishna Amudala]

Add watchdog device tree node for exynos5420

Signed-off-by: Leela Krishna Amudala <l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5420.dtsi |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index d537cd7..9fadc23 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -186,6 +186,12 @@
 		status = "okay";
 	};
 
+	watchdog@101D0000 {
+		clocks = <&clock 316>;
+		clock-names = "watchdog";
+		status = "okay";
+	};
+
 	serial@12C00000 {
 		clocks = <&clock 257>, <&clock 128>;
 		clock-names = "uart", "clk_uart_baud0";
-- 
1.7.9.5

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

[PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Leela Krishna Amudala]

This patch parses the watchdog node to read pmu wdt sys registers addresses
and do mask/unmask enable/disable of WDT in probe and s2r scenarios.

Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Leela Krishna Amudala <l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/watchdog/samsung-wdt.txt   |   14 ++++-
 drivers/watchdog/s3c2410_wdt.c                     |   56 ++++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..4c798e3 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -7,8 +7,20 @@ occurred.
 Required properties:
 - compatible : should be "samsung,s3c2410-wdt"
 - reg : base physical address of the controller and length of memory mapped
-	region.
+	region and the optional (addresses and length of memory mapped regions
+	of) PMU registers for masking/unmasking WDT.
 - interrupts : interrupt number to the cpu.
 
 Optional properties:
 - timeout-sec : contains the watchdog timeout in seconds.
+- reset-mask-bit: bit number in the PMU registers to program mask/unmask WDT.
+
+Example:
+
+watchdog {
+	compatible = "samsung,s3c2410-wdt";
+	reg = <0x101D0000 0x100>, <0x10040408 0x4>, <0x1004040c 0x4>;
+	interrupts = <0 42 0>;
+	status = "disabled";
+	reset-mask-bit = <0>;
+};
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..a68e4dd 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -94,6 +94,9 @@ struct s3c2410_wdt {
 	unsigned long		wtdat_save;
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
+	void __iomem		*pmu_disable_reg;
+	void __iomem		*pmu_mask_reset_reg;
+	int			pmu_mask_bit;
 };
 
 /* watchdog control routines */
@@ -111,6 +114,33 @@ 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 value;
+
+	if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
+					 || (wdt->pmu_mask_bit < 0))
+		return;
+
+	if (mask) {
+		value = readl(wdt->pmu_disable_reg);
+		value |= (1 << wdt->pmu_mask_bit);
+		writel(value, wdt->pmu_disable_reg);
+
+		value = readl(wdt->pmu_mask_reset_reg);
+		value |= (1 << wdt->pmu_mask_bit);
+		writel(value, wdt->pmu_mask_reset_reg);
+	} else {
+		value = readl(wdt->pmu_disable_reg);
+		value &= ~(1 << wdt->pmu_mask_bit);
+		writel(value, wdt->pmu_disable_reg);
+
+		value = readl(wdt->pmu_mask_reset_reg);
+		value &= ~(1 << wdt->pmu_mask_bit);
+		writel(value, wdt->pmu_mask_reset_reg);
+	}
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
@@ -341,6 +371,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	unsigned int wtcon;
 	int started = 0;
 	int ret;
+	struct resource *res;
+	unsigned int mask_bit;
 
 	DBG("%s: probe=%p\n", __func__, pdev);
 
@@ -369,6 +401,25 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	wdt->pmu_disable_reg = devm_ioremap_resource(&pdev->dev, res);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	wdt->pmu_mask_reset_reg = devm_ioremap_resource(&pdev->dev, res);
+
+	if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
+		if (pdev->dev.of_node) {
+			if (of_property_read_u32(pdev->dev.of_node,
+							"reset-mask-bit",
+							&mask_bit)) {
+				dev_warn(dev, "reset-mask-bit not specified\n");
+				wdt->pmu_mask_bit = -EINVAL;
+			} else {
+				wdt->pmu_mask_bit = mask_bit;
+			}
+		}
+	}
+
 	DBG("probe: mapped reg_base=%p\n", wdt->reg_base);
 
 	wdt->clock = devm_clk_get(dev, "watchdog");
@@ -444,6 +495,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		 (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
 		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
 
+	s3c2410wdt_mask_and_disable_reset(0, wdt);
 	return 0;
 
  err_cpufreq:
@@ -461,6 +513,7 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
+	s3c2410wdt_mask_and_disable_reset(1, wdt);
 	watchdog_unregister_device(&wdt->wdt_device);
 
 	s3c2410wdt_cpufreq_deregister(wdt);
@@ -475,6 +528,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
+	s3c2410wdt_mask_and_disable_reset(1, wdt);
 	s3c2410wdt_stop(&wdt->wdt_device);
 }
 
@@ -488,6 +542,7 @@ 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);
 
+	s3c2410wdt_mask_and_disable_reset(1, wdt);
 	/* Note that WTCNT doesn't need to be saved. */
 	s3c2410wdt_stop(&wdt->wdt_device);
 
@@ -503,6 +558,7 @@ 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);
 
+	s3c2410wdt_mask_and_disable_reset(0, wdt);
 	dev_info(dev, "watchdog %sabled\n",
 		(wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis");
 
-- 
1.7.9.5

--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

Hi Leela,

Please see my comments below.

On Tuesday 17 of September 2013 16:13:42 Leela Krishna Amudala wrote:
> This patch parses the watchdog node to read pmu wdt sys registers
> addresses and do mask/unmask enable/disable of WDT in probe and s2r
> scenarios.
> 
> Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/watchdog/samsung-wdt.txt   |   14 ++++-
>  drivers/watchdog/s3c2410_wdt.c                     |   56
> ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index
> 2aa486c..4c798e3 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> @@ -7,8 +7,20 @@ occurred.
>  Required properties:
>  - compatible : should be "samsung,s3c2410-wdt"

Since the WDT block of Exynos 5420 needs some extra configuration in PMU 
registers, it is no longer compatible with samsung.s3c2410-wdt. Please 
introduce separate compatible ("samsung,exynos5420-wdt") and make the 
driver handle the additional configuration only if running on a device with 
this compatible value.

I'd suggest introducing quirk system to the driver and adding a 
NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420-
wdt" compatible.

>  - reg : base physical address of the controller and length of memory
> mapped -	region.
> +	region and the optional (addresses and length of memory mapped regions
> +	of) PMU registers for masking/unmasking WDT.
>  - interrupts : interrupt number to the cpu.
> 
>  Optional properties:
>  - timeout-sec : contains the watchdog timeout in seconds.
> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask
> WDT. +

I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It 
should be handled depending on compatible value.

>
[...]
> +static void s3c2410wdt_mask_and_disable_reset(int mask, struct
> s3c2410_wdt *wdt) +{
> +	unsigned int value;
> +
> +	if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
> +					 || (wdt->pmu_mask_bit < 0))
> +		return;

This function could be called only if respective quirk is active and the 
check above could be dropped.

> +
> +	if (mask) {
> +		value = readl(wdt->pmu_disable_reg);
> +		value |= (1 << wdt->pmu_mask_bit);
> +		writel(value, wdt->pmu_disable_reg);
> +
> +		value = readl(wdt->pmu_mask_reset_reg);
> +		value |= (1 << wdt->pmu_mask_bit);
> +		writel(value, wdt->pmu_mask_reset_reg);
> +	} else {
> +		value = readl(wdt->pmu_disable_reg);
> +		value &= ~(1 << wdt->pmu_mask_bit);
> +		writel(value, wdt->pmu_disable_reg);
> +
> +		value = readl(wdt->pmu_mask_reset_reg);
> +		value &= ~(1 << wdt->pmu_mask_bit);
> +		writel(value, wdt->pmu_mask_reset_reg);
> +	}

This can be greatly simplified by moving readl and writel outside the 
conditional block, i.e.

	u32 disable, mask_reset;

	disable = readl(wdt->pmu_disable_reg);
	mask_reset = readl(wdt->pmu_mask_reset_reg);

	if (mask) {
		disable |= (1 << wdt->pmu_mask_bit);
		mask_reset |= (1 << wdt->pmu_mask_bit);
	} else {
		disable &= ~(1 << wdt->pmu_mask_bit);
		mask_reset &= ~(1 << wdt->pmu_mask_bit);
	}

	writel(disable, wdt->pmu_disable_reg);
	writel(mask_reset, wdt->pmu_mask_reset_reg);

> +}
> +
>  static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
>  {
>  	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> @@ -341,6 +371,8 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) unsigned int wtcon;
>  	int started = 0;
>  	int ret;
> +	struct resource *res;
> +	unsigned int mask_bit;
> 
>  	DBG("%s: probe=%p\n", __func__, pdev);
> 
> @@ -369,6 +401,25 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) goto err;
>  	}

The code added below could be handled conditionally and missing PMU 
registers could simply trigger an error.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	wdt->pmu_disable_reg = devm_ioremap_resource(&pdev->dev, res);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	wdt->pmu_mask_reset_reg = devm_ioremap_resource(&pdev->dev, res);
> +
> +	if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg))
> { +		if (pdev->dev.of_node) {
> +			if (of_property_read_u32(pdev->dev.of_node,
> +							"reset-mask-bit",
> +							&mask_bit)) {
> +				dev_warn(dev, "reset-mask-bit not specified\n");
> +				wdt->pmu_mask_bit = -EINVAL;
> +			} else {
> +				wdt->pmu_mask_bit = mask_bit;
> +			}
> +		}
> +	}
> +
>  	DBG("probe: mapped reg_base=%p\n", wdt->reg_base);
> 
>  	wdt->clock = devm_clk_get(dev, "watchdog");
> @@ -444,6 +495,7 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
>  		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
> 
> +	s3c2410wdt_mask_and_disable_reset(0, wdt);

The call above (and further call to this function bellow) could happen 
conditionally.

Best regards,
Tomasz

--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Doug Anderson]

Tomasz,

On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>> @@ -7,8 +7,20 @@ occurred.
>>  Required properties:
>>  - compatible : should be "samsung,s3c2410-wdt"
>
> Since the WDT block of Exynos 5420 needs some extra configuration in PMU
> registers, it is no longer compatible with samsung.s3c2410-wdt. Please
> introduce separate compatible ("samsung,exynos5420-wdt") and make the
> driver handle the additional configuration only if running on a device with
> this compatible value.
>
> I'd suggest introducing quirk system to the driver and adding a
> NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420-
> wdt" compatible.
>
>>  - reg : base physical address of the controller and length of memory
>> mapped -      region.
>> +     region and the optional (addresses and length of memory mapped regions
>> +     of) PMU registers for masking/unmasking WDT.
>>  - interrupts : interrupt number to the cpu.
>>
>>  Optional properties:
>>  - timeout-sec : contains the watchdog timeout in seconds.
>> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask
>> WDT. +
>
> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
> should be handled depending on compatible value.

I think at least 5250 needs something similar.  I believe we got away
with it in the past since other (non-WDT) code was tweaking with this
bit, but that was a little bit gross.  Leela Krishna can correct me if
I'm wrong.

-Doug

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Leela Krishna Amudala]

Tomasz,

On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Tomasz,
>
> On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>> @@ -7,8 +7,20 @@ occurred.
>>>  Required properties:
>>>  - compatible : should be "samsung,s3c2410-wdt"
>>
>> Since the WDT block of Exynos 5420 needs some extra configuration in PMU
>> registers, it is no longer compatible with samsung.s3c2410-wdt. Please
>> introduce separate compatible ("samsung,exynos5420-wdt") and make the
>> driver handle the additional configuration only if running on a device with
>> this compatible value.
>>
>> I'd suggest introducing quirk system to the driver and adding a
>> NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420-
>> wdt" compatible.
>>
>>>  - reg : base physical address of the controller and length of memory
>>> mapped -      region.
>>> +     region and the optional (addresses and length of memory mapped regions
>>> +     of) PMU registers for masking/unmasking WDT.
>>>  - interrupts : interrupt number to the cpu.
>>>
>>>  Optional properties:
>>>  - timeout-sec : contains the watchdog timeout in seconds.
>>> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask
>>> WDT. +
>>
>> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
>> should be handled depending on compatible value.
>
> I think at least 5250 needs something similar.  I believe we got away
> with it in the past since other (non-WDT) code was tweaking with this
> bit, but that was a little bit gross.  Leela Krishna can correct me if
> I'm wrong.
>

Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
other than Exynos5xxx.
Hence I kept it as optional parameter.

I took care of this code such that it won't break on older SoCs.

If you notice the code in probe function, I used the check condition

if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
}

i.e., if any of the PMU register address is not mentioned in the DT
node it simply skips reading reset-mask-bit
and continues execution (which may happen in older SoC DT node).

..also, in s3c2410wdt_mask_and_disable_reset() function, below
condition check is happening

if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
                                || (wdt->pmu_mask_bit < 0))
        return;

i.e., if any of the registers is not specified in DT node simply
return without programming
PMU registers(which is true in case of older SoCs).

If you think this doesn't sounds good way of handling older SoCs.
I'll add new compatible string for Exynos5xxx and do like what you said. :)

Best Wishes,
Leela Krishna Amudala

> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote:
> Tomasz,
> 
> On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders@chromium.org> wrote:
> > Tomasz,
> >
> > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> >>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> >>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> >>> @@ -7,8 +7,20 @@ occurred.
> >>>  Required properties:
> >>>  - compatible : should be "samsung,s3c2410-wdt"
> >>
> >> Since the WDT block of Exynos 5420 needs some extra configuration in PMU
> >> registers, it is no longer compatible with samsung.s3c2410-wdt. Please
> >> introduce separate compatible ("samsung,exynos5420-wdt") and make the
> >> driver handle the additional configuration only if running on a device with
> >> this compatible value.
> >>
> >> I'd suggest introducing quirk system to the driver and adding a
> >> NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420-
> >> wdt" compatible.
> >>
> >>>  - reg : base physical address of the controller and length of memory
> >>> mapped -      region.
> >>> +     region and the optional (addresses and length of memory mapped regions
> >>> +     of) PMU registers for masking/unmasking WDT.
> >>>  - interrupts : interrupt number to the cpu.
> >>>
> >>>  Optional properties:
> >>>  - timeout-sec : contains the watchdog timeout in seconds.
> >>> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask
> >>> WDT. +
> >>
> >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
> >> should be handled depending on compatible value.
> >
> > I think at least 5250 needs something similar.  I believe we got away
> > with it in the past since other (non-WDT) code was tweaking with this
> > bit, but that was a little bit gross.  Leela Krishna can correct me if
> > I'm wrong.
> >
> 
> Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
> other than Exynos5xxx.
> Hence I kept it as optional parameter.
> 
> I took care of this code such that it won't break on older SoCs.
> 
> If you notice the code in probe function, I used the check condition
> 
> if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
> }
> 
> i.e., if any of the PMU register address is not mentioned in the DT
> node it simply skips reading reset-mask-bit
> and continues execution (which may happen in older SoC DT node).
> 
> ..also, in s3c2410wdt_mask_and_disable_reset() function, below
> condition check is happening
> 
> if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
>                                 || (wdt->pmu_mask_bit < 0))
>         return;
> 
> i.e., if any of the registers is not specified in DT node simply
> return without programming
> PMU registers(which is true in case of older SoCs).
> 
> If you think this doesn't sounds good way of handling older SoCs.
> I'll add new compatible string for Exynos5xxx and do like what you said. :)

Yes, please re-do the code per Tomasz's suggestions.

This would also allow you to check return values of devm_ioremap_resource()
calls in the probe method and require them to succeed in order to register
watchdog device (which is unfortunately not what happens currently).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

On Monday 23 of September 2013 19:03:10 Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote:
> > Tomasz,
> > 
> > On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders@chromium.org> wrote:
> > > Tomasz,
> > >
> > > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
> > >> should be handled depending on compatible value.
> > >
> > > I think at least 5250 needs something similar.  I believe we got away
> > > with it in the past since other (non-WDT) code was tweaking with this
> > > bit, but that was a little bit gross.  Leela Krishna can correct me if
> > > I'm wrong.
> > >
> > 
> > Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
> > other than Exynos5xxx.
> > Hence I kept it as optional parameter.
> > 
> > I took care of this code such that it won't break on older SoCs.
> > 
> > If you notice the code in probe function, I used the check condition
> > 
> > if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
> > }
> > 
> > i.e., if any of the PMU register address is not mentioned in the DT
> > node it simply skips reading reset-mask-bit
> > and continues execution (which may happen in older SoC DT node).
> > 
> > ..also, in s3c2410wdt_mask_and_disable_reset() function, below
> > condition check is happening
> > 
> > if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
> >                                 || (wdt->pmu_mask_bit < 0))
> >         return;
> > 
> > i.e., if any of the registers is not specified in DT node simply
> > return without programming
> > PMU registers(which is true in case of older SoCs).
> > 
> > If you think this doesn't sounds good way of handling older SoCs.
> > I'll add new compatible string for Exynos5xxx and do like what you said. :)
> 
> Yes, please re-do the code per Tomasz's suggestions.
> 
> This would also allow you to check return values of devm_ioremap_resource()
> calls in the probe method and require them to succeed in order to register
> watchdog device (which is unfortunately not what happens currently).

Now as I think of it, the driver doesn't seem to reconfigure those PMU
registers in any watchdog API callback, only in probe, remove, suspend
and resume.

Since we already have PMU "driver" in mach-exynos, which already has
suspend/resume syscore ops, what about placing such configuration there
instead?

Best regards,
Tomasz

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

On Monday 23 of September 2013 19:11:11 Tomasz Figa wrote:
> On Monday 23 of September 2013 19:03:10 Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote:
> > > Tomasz,
> > > 
> > > On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders@chromium.org> wrote:
> > > > Tomasz,
> > > >
> > > > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > > >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
> > > >> should be handled depending on compatible value.
> > > >
> > > > I think at least 5250 needs something similar.  I believe we got away
> > > > with it in the past since other (non-WDT) code was tweaking with this
> > > > bit, but that was a little bit gross.  Leela Krishna can correct me if
> > > > I'm wrong.
> > > >
> > > 
> > > Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
> > > other than Exynos5xxx.
> > > Hence I kept it as optional parameter.
> > > 
> > > I took care of this code such that it won't break on older SoCs.
> > > 
> > > If you notice the code in probe function, I used the check condition
> > > 
> > > if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
> > > }
> > > 
> > > i.e., if any of the PMU register address is not mentioned in the DT
> > > node it simply skips reading reset-mask-bit
> > > and continues execution (which may happen in older SoC DT node).
> > > 
> > > ..also, in s3c2410wdt_mask_and_disable_reset() function, below
> > > condition check is happening
> > > 
> > > if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
> > >                                 || (wdt->pmu_mask_bit < 0))
> > >         return;
> > > 
> > > i.e., if any of the registers is not specified in DT node simply
> > > return without programming
> > > PMU registers(which is true in case of older SoCs).
> > > 
> > > If you think this doesn't sounds good way of handling older SoCs.
> > > I'll add new compatible string for Exynos5xxx and do like what you said. :)
> > 
> > Yes, please re-do the code per Tomasz's suggestions.
> > 
> > This would also allow you to check return values of devm_ioremap_resource()
> > calls in the probe method and require them to succeed in order to register
> > watchdog device (which is unfortunately not what happens currently).
> 
> Now as I think of it, the driver doesn't seem to reconfigure those PMU
> registers in any watchdog API callback, only in probe, remove, suspend
> and resume.
> 
> Since we already have PMU "driver" in mach-exynos, which already has
> suspend/resume syscore ops, what about placing such configuration there
> instead?

Any opinions on this?

Best regards,
Tomasz

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Leela Krishna Amudala]

Tomasz,

On Fri, Sep 27, 2013 at 3:42 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Monday 23 of September 2013 19:11:11 Tomasz Figa wrote:
>> On Monday 23 of September 2013 19:03:10 Bartlomiej Zolnierkiewicz wrote:
>> >
>> > Hi,
>> >
>> > On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote:
>> > > Tomasz,
>> > >
>> > > On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders@chromium.org> wrote:
>> > > > Tomasz,
>> > > >
>> > > > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > > >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
>> > > >> should be handled depending on compatible value.
>> > > >
>> > > > I think at least 5250 needs something similar.  I believe we got away
>> > > > with it in the past since other (non-WDT) code was tweaking with this
>> > > > bit, but that was a little bit gross.  Leela Krishna can correct me if
>> > > > I'm wrong.
>> > > >
>> > >
>> > > Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
>> > > other than Exynos5xxx.
>> > > Hence I kept it as optional parameter.
>> > >
>> > > I took care of this code such that it won't break on older SoCs.
>> > >
>> > > If you notice the code in probe function, I used the check condition
>> > >
>> > > if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
>> > > }
>> > >
>> > > i.e., if any of the PMU register address is not mentioned in the DT
>> > > node it simply skips reading reset-mask-bit
>> > > and continues execution (which may happen in older SoC DT node).
>> > >
>> > > ..also, in s3c2410wdt_mask_and_disable_reset() function, below
>> > > condition check is happening
>> > >
>> > > if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
>> > >                                 || (wdt->pmu_mask_bit < 0))
>> > >         return;
>> > >
>> > > i.e., if any of the registers is not specified in DT node simply
>> > > return without programming
>> > > PMU registers(which is true in case of older SoCs).
>> > >
>> > > If you think this doesn't sounds good way of handling older SoCs.
>> > > I'll add new compatible string for Exynos5xxx and do like what you said. :)
>> >
>> > Yes, please re-do the code per Tomasz's suggestions.
>> >
>> > This would also allow you to check return values of devm_ioremap_resource()
>> > calls in the probe method and require them to succeed in order to register
>> > watchdog device (which is unfortunately not what happens currently).
>>
>> Now as I think of it, the driver doesn't seem to reconfigure those PMU
>> registers in any watchdog API callback, only in probe, remove, suspend
>> and resume.
>>
>> Since we already have PMU "driver" in mach-exynos, which already has
>> suspend/resume syscore ops, what about placing such configuration there
>> instead?
>
> Any opinions on this?
>

In PMU we have control registers for other IPs (like USB, MIPI etc.,)
also and I'm not sure where those registers are getting configured.
If we want to configure WDT register in PMU driver then we have to
consider configuring control regs for other IPs also and the DT node
has to have all these control bit numbers. So instead of that I feel
configuring it in WDT driver looks fine.

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

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

On Friday 27 of September 2013 16:48:34 Leela Krishna Amudala wrote:
> Tomasz,
> 
> On Fri, Sep 27, 2013 at 3:42 PM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> > On Monday 23 of September 2013 19:11:11 Tomasz Figa wrote:
> >> On Monday 23 of September 2013 19:03:10 Bartlomiej Zolnierkiewicz wrote:
> >> >
> >> > Hi,
> >> >
> >> > On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote:
> >> > > Tomasz,
> >> > >
> >> > > On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >> > > > Tomasz,
> >> > > >
> >> > > > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >> > > >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
> >> > > >> should be handled depending on compatible value.
> >> > > >
> >> > > > I think at least 5250 needs something similar.  I believe we got away
> >> > > > with it in the past since other (non-WDT) code was tweaking with this
> >> > > > bit, but that was a little bit gross.  Leela Krishna can correct me if
> >> > > > I'm wrong.
> >> > > >
> >> > >
> >> > > Yes, 5250 also needs this reset-mask-bit, but not required by SoCs
> >> > > other than Exynos5xxx.
> >> > > Hence I kept it as optional parameter.
> >> > >
> >> > > I took care of this code such that it won't break on older SoCs.
> >> > >
> >> > > If you notice the code in probe function, I used the check condition
> >> > >
> >> > > if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) {
> >> > > }
> >> > >
> >> > > i.e., if any of the PMU register address is not mentioned in the DT
> >> > > node it simply skips reading reset-mask-bit
> >> > > and continues execution (which may happen in older SoC DT node).
> >> > >
> >> > > ..also, in s3c2410wdt_mask_and_disable_reset() function, below
> >> > > condition check is happening
> >> > >
> >> > > if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
> >> > >                                 || (wdt->pmu_mask_bit < 0))
> >> > >         return;
> >> > >
> >> > > i.e., if any of the registers is not specified in DT node simply
> >> > > return without programming
> >> > > PMU registers(which is true in case of older SoCs).
> >> > >
> >> > > If you think this doesn't sounds good way of handling older SoCs.
> >> > > I'll add new compatible string for Exynos5xxx and do like what you said. :)
> >> >
> >> > Yes, please re-do the code per Tomasz's suggestions.
> >> >
> >> > This would also allow you to check return values of devm_ioremap_resource()
> >> > calls in the probe method and require them to succeed in order to register
> >> > watchdog device (which is unfortunately not what happens currently).
> >>
> >> Now as I think of it, the driver doesn't seem to reconfigure those PMU
> >> registers in any watchdog API callback, only in probe, remove, suspend
> >> and resume.
> >>
> >> Since we already have PMU "driver" in mach-exynos, which already has
> >> suspend/resume syscore ops, what about placing such configuration there
> >> instead?
> >
> > Any opinions on this?
> >
> 
> In PMU we have control registers for other IPs (like USB, MIPI etc.,)
> also and I'm not sure where those registers are getting configured.
> If we want to configure WDT register in PMU driver then we have to
> consider configuring control regs for other IPs also and the DT node
> has to have all these control bit numbers. So instead of that I feel
> configuring it in WDT driver looks fine.

I believe this depends on possible use cases, not the IP block itself.
If this is just a low level initial (and SoC specific) initialization
that doesn't require any co-operation with high level kernel/userspace
APIs, then I think there is no need to entangle the driver for watchdog
IP with SoC specific details outside the IP itself.

[Adding more people on Cc for further discussion.]

Best regards,
Tomasz

--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Doug Anderson]

Tomasz

On Fri, Sep 27, 2013 at 4:25 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> >> Since we already have PMU "driver" in mach-exynos, which already has
>> >> suspend/resume syscore ops, what about placing such configuration there
>> >> instead?
>> >
>> > Any opinions on this?
>> >
>>
>> In PMU we have control registers for other IPs (like USB, MIPI etc.,)
>> also and I'm not sure where those registers are getting configured.
>> If we want to configure WDT register in PMU driver then we have to
>> consider configuring control regs for other IPs also and the DT node
>> has to have all these control bit numbers. So instead of that I feel
>> configuring it in WDT driver looks fine.
>
> I believe this depends on possible use cases, not the IP block itself.
> If this is just a low level initial (and SoC specific) initialization
> that doesn't require any co-operation with high level kernel/userspace
> APIs, then I think there is no need to entangle the driver for watchdog
> IP with SoC specific details outside the IP itself.
>
> [Adding more people on Cc for further discussion.]

So isn't the register in the PMU there to save power in the case that
the watchdog timer isn't being used?  How is the PMU "driver" to know
whether the watchdog is being used?  Better IMHO that the watchdog
driver knows to enable and disable itself as needed, right?

-Doug

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

On Friday 27 of September 2013 08:20:25 Doug Anderson wrote:
> Tomasz
> 
> On Fri, Sep 27, 2013 at 4:25 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >> >> Since we already have PMU "driver" in mach-exynos, which already
> >> >> has
> >> >> suspend/resume syscore ops, what about placing such configuration
> >> >> there
> >> >> instead?
> >> > 
> >> > Any opinions on this?
> >> 
> >> In PMU we have control registers for other IPs (like USB, MIPI etc.,)
> >> also and I'm not sure where those registers are getting configured.
> >> If we want to configure WDT register in PMU driver then we have to
> >> consider configuring control regs for other IPs also and the DT node
> >> has to have all these control bit numbers. So instead of that I feel
> >> configuring it in WDT driver looks fine.
> > 
> > I believe this depends on possible use cases, not the IP block itself.
> > If this is just a low level initial (and SoC specific) initialization
> > that doesn't require any co-operation with high level kernel/userspace
> > APIs, then I think there is no need to entangle the driver for
> > watchdog
> > IP with SoC specific details outside the IP itself.
> > 
> > [Adding more people on Cc for further discussion.]
> 
> So isn't the register in the PMU there to save power in the case that
> the watchdog timer isn't being used?  How is the PMU "driver" to know
> whether the watchdog is being used?  Better IMHO that the watchdog
> driver knows to enable and disable itself as needed, right?

How much power can you save on one low frequency counter? Anyway, those 
bits look more like reset signal masks to me, unrelated to any power 
saving and even if, this driver switches them on at probe regardless of 
whether the watchdog is actually used or not.

Best regards,
Tomasz

--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Doug Anderson]

Tomasz,

On Fri, Sep 27, 2013 at 11:14 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> So isn't the register in the PMU there to save power in the case that
>> the watchdog timer isn't being used?  How is the PMU "driver" to know
>> whether the watchdog is being used?  Better IMHO that the watchdog
>> driver knows to enable and disable itself as needed, right?
>
> How much power can you save on one low frequency counter? Anyway, those
> bits look more like reset signal masks to me, unrelated to any power
> saving and even if, this driver switches them on at probe regardless of
> whether the watchdog is actually used or not.

I don't know for sure how much power it saves if any.  The description
I have of those fields is also definitely a little on the confusing
side.  The fact that they are in the PMU leads me to believe that they
save power, but perhaps that's not the case here.

It still does seem nice to keep watchdog related code in the watchdog
driver, though...  I guess I could also imagine ordering problems if
we tweaked this bit in the PMU driver, though I don't have actual
evidence of this.  Maybe cases where enabling at the wrong time could
cause spurious watchdog resets depending on how the BIOS left the
state of things.  It would be unfortunate if we found we needed to
reach into the watchdog register bank from the PMU driver to "pet" the
watchdog before enabling it.

-Doug

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

On Friday 27 of September 2013 11:48:53 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, Sep 27, 2013 at 11:14 AM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
wrote:
> >> So isn't the register in the PMU there to save power in the case that
> >> the watchdog timer isn't being used?  How is the PMU "driver" to know
> >> whether the watchdog is being used?  Better IMHO that the watchdog
> >> driver knows to enable and disable itself as needed, right?
> > 
> > How much power can you save on one low frequency counter? Anyway,
> > those
> > bits look more like reset signal masks to me, unrelated to any power
> > saving and even if, this driver switches them on at probe regardless
> > of
> > whether the watchdog is actually used or not.
> 
> I don't know for sure how much power it saves if any.  The description
> I have of those fields is also definitely a little on the confusing
> side.  The fact that they are in the PMU leads me to believe that they
> save power, but perhaps that's not the case here.
> 
> It still does seem nice to keep watchdog related code in the watchdog
> driver, though...  

For me, this thing looks more related to PMU than watchdog, since the code 
writes to PMU registers and does this only on system power state 
transitions, i.e. once per boot, suspend, resume and shutdown.

> I guess I could also imagine ordering problems if
> we tweaked this bit in the PMU driver, though I don't have actual
> evidence of this.  Maybe cases where enabling at the wrong time could
> cause spurious watchdog resets depending on how the BIOS left the
> state of things.

Even if you put this code in watchdog driver you don't have any warranties 
that the firmware leaves both the watchdog and PMU bits sets correctly. 
I'm not sure what would be the point of firmware existence if you can't 
trust it to set up at least the basic working environment for you and I 
consider watchdog to be part of such.

> It would be unfortunate if we found we needed to
> reach into the watchdog register bank from the PMU driver to "pet" the
> watchdog before enabling it.

If you find any such need in future, then with the solution I proposed 
there will be no problem in changing things to be done as you and Leela 
propose. The other way around would not be that simple, because the 
introduced binding would still have to be supported and the code touching 
PMU would have to be kept in watchdog code...

However, if you really insist on handling this in watchdog driver, please 
consider using something smarter to access the PMU. I believe that device 
node of watchdog should not represent parts of PMU, which should be 
represented as a separate node instead.

Could you use the syscon driver instead to handle PMU register accesses in 
a centralized way, put the PMU register offset and respective bit masks 
inside the watchdog driver (preferrably in a struct bound to respective OF 
match entry) and access the registers using the regmap API?

Best regards,
Tomasz

--
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 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Doug Anderson]

Tomasz,

On Sat, Sep 28, 2013 at 6:49 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On Fri, Sep 27, 2013 at 11:14 AM, Tomasz Figa <tomasz.figa@gmail.com>
> wrote:
>> >> So isn't the register in the PMU there to save power in the case that
>> >> the watchdog timer isn't being used?  How is the PMU "driver" to know
>> >> whether the watchdog is being used?  Better IMHO that the watchdog
>> >> driver knows to enable and disable itself as needed, right?
>> >
>> > How much power can you save on one low frequency counter? Anyway,
>> > those
>> > bits look more like reset signal masks to me, unrelated to any power
>> > saving and even if, this driver switches them on at probe regardless
>> > of
>> > whether the watchdog is actually used or not.
>>
>> I don't know for sure how much power it saves if any.  The description
>> I have of those fields is also definitely a little on the confusing
>> side.  The fact that they are in the PMU leads me to believe that they
>> save power, but perhaps that's not the case here.
>>
>> It still does seem nice to keep watchdog related code in the watchdog
>> driver, though...
>
> For me, this thing looks more related to PMU than watchdog, since the code
> writes to PMU registers and does this only on system power state
> transitions, i.e. once per boot, suspend, resume and shutdown.

I wonder if there is any reason that it shouldn't be put in the
s3c2410wdt_start() and s3c2410wdt_stop() functions?


>> I guess I could also imagine ordering problems if
>> we tweaked this bit in the PMU driver, though I don't have actual
>> evidence of this.  Maybe cases where enabling at the wrong time could
>> cause spurious watchdog resets depending on how the BIOS left the
>> state of things.
>
> Even if you put this code in watchdog driver you don't have any warranties
> that the firmware leaves both the watchdog and PMU bits sets correctly.
> I'm not sure what would be the point of firmware existence if you can't
> trust it to set up at least the basic working environment for you and I
> consider watchdog to be part of such.

I trust it to leave the system in some state that is safe.  ...but I
don't trust the firmware of all exynos-based products to have the same
idea of what "safe" is.  I could imagine some of them disabling the
watchdog before booting linux by tweaking the settings in the PMU (and
leaving the reset of the watchdog configured and running) and some of
them disabling the watchdog by leaving the PMU settings alone and
turning off the enable bit.  I could also imagine some of them
actually leaving the watchdog fully configured and running to try to
catch the case where the kernel crashes at bootup.  I don't know of
any official rules here, which means that the kernel needs to be
flexible enough.


>> It would be unfortunate if we found we needed to
>> reach into the watchdog register bank from the PMU driver to "pet" the
>> watchdog before enabling it.
>
> If you find any such need in future, then with the solution I proposed
> there will be no problem in changing things to be done as you and Leela
> propose. The other way around would not be that simple, because the
> introduced binding would still have to be supported and the code touching
> PMU would have to be kept in watchdog code...
>
> However, if you really insist on handling this in watchdog driver,

I certainly don't insist.  I have not contributed nearly enough to the
community to insist on anything.  At this point I'm merely stating my
opinion.  Feel free to take it into consideration or ignore it.  I
will not be offended if you choose to ignore my opinion here.  It is
really up to Leela Krishna and the maintainer of the subsystem if I
understand correctly.


> please
> consider using something smarter to access the PMU. I believe that device
> node of watchdog should not represent parts of PMU, which should be
> represented as a separate node instead.

We've already had this discussion before.  Assuming I'm understanding
him correctly, Olof weighed in and suggested that he preferred to do
the simpler thing in this case (AKA: what Leela Krishna is doing):

http://www.spinics.net/lists/devicetree/msg00812.html

-Doug

Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

Doug,

On Monday 30 of September 2013 09:54:33 Doug Anderson wrote:
> Tomasz,
> 
> On Sat, Sep 28, 2013 at 6:49 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >> On Fri, Sep 27, 2013 at 11:14 AM, Tomasz Figa <tomasz.figa@gmail.com>
> > wrote:
> >> >> So isn't the register in the PMU there to save power in the case that
> >> >> the watchdog timer isn't being used?  How is the PMU "driver" to know
> >> >> whether the watchdog is being used?  Better IMHO that the watchdog
> >> >> driver knows to enable and disable itself as needed, right?
> >> >
> >> > How much power can you save on one low frequency counter? Anyway,
> >> > those
> >> > bits look more like reset signal masks to me, unrelated to any power
> >> > saving and even if, this driver switches them on at probe regardless
> >> > of
> >> > whether the watchdog is actually used or not.
> >>
> >> I don't know for sure how much power it saves if any.  The description
> >> I have of those fields is also definitely a little on the confusing
> >> side.  The fact that they are in the PMU leads me to believe that they
> >> save power, but perhaps that's not the case here.
> >>
> >> It still does seem nice to keep watchdog related code in the watchdog
> >> driver, though...
> >
> > For me, this thing looks more related to PMU than watchdog, since the code
> > writes to PMU registers and does this only on system power state
> > transitions, i.e. once per boot, suspend, resume and shutdown.
> 
> I wonder if there is any reason that it shouldn't be put in the
> s3c2410wdt_start() and s3c2410wdt_stop() functions?

Well, this could actually make some sense, assuming that there would be
any use case for it and any advantages of doing so. According to user's
manual those masks are only masking the reset signal, but there are no
use scenarios described.

> >> I guess I could also imagine ordering problems if
> >> we tweaked this bit in the PMU driver, though I don't have actual
> >> evidence of this.  Maybe cases where enabling at the wrong time could
> >> cause spurious watchdog resets depending on how the BIOS left the
> >> state of things.
> >
> > Even if you put this code in watchdog driver you don't have any warranties
> > that the firmware leaves both the watchdog and PMU bits sets correctly.
> > I'm not sure what would be the point of firmware existence if you can't
> > trust it to set up at least the basic working environment for you and I
> > consider watchdog to be part of such.
> 
> I trust it to leave the system in some state that is safe.  ...but I
> don't trust the firmware of all exynos-based products to have the same
> idea of what "safe" is.  I could imagine some of them disabling the
> watchdog before booting linux by tweaking the settings in the PMU (and
> leaving the reset of the watchdog configured and running) and some of
> them disabling the watchdog by leaving the PMU settings alone and
> turning off the enable bit.  I could also imagine some of them
> actually leaving the watchdog fully configured and running to try to
> catch the case where the kernel crashes at bootup.  I don't know of
> any official rules here, which means that the kernel needs to be
> flexible enough.

Nothing stops the kernel from simply disabling the watchdog by watchdog
(not PMU registers), but still, what about kernels compiled without the
watchdog driver?

> >> It would be unfortunate if we found we needed to
> >> reach into the watchdog register bank from the PMU driver to "pet" the
> >> watchdog before enabling it.
> >
> > If you find any such need in future, then with the solution I proposed
> > there will be no problem in changing things to be done as you and Leela
> > propose. The other way around would not be that simple, because the
> > introduced binding would still have to be supported and the code touching
> > PMU would have to be kept in watchdog code...
> >
> > However, if you really insist on handling this in watchdog driver,
> 
> I certainly don't insist.  I have not contributed nearly enough to the
> community to insist on anything.  At this point I'm merely stating my
> opinion.  Feel free to take it into consideration or ignore it.  I
> will not be offended if you choose to ignore my opinion here.  It is
> really up to Leela Krishna and the maintainer of the subsystem if I
> understand correctly.

Well, everyone has their right to insist on something. Since you're
writing to this mailing list, you're part of the community and you're free
to suggest the superiority of your preferred solution :).

> > please
> > consider using something smarter to access the PMU. I believe that device
> > node of watchdog should not represent parts of PMU, which should be
> > represented as a separate node instead.
> 
> We've already had this discussion before.  Assuming I'm understanding
> him correctly, Olof weighed in and suggested that he preferred to do
> the simpler thing in this case (AKA: what Leela Krishna is doing):
> 
> http://www.spinics.net/lists/devicetree/msg00812.html

Well, the patches themselves in their last version looks fine to me,
however this solution has several issues:
 - a full page of virtual address space is being mapped for just 4 bytes,
   for every such register,
 - the MASK_WDT_RESET_REQUEST register (0x1004040c) contains multiple
   fields, not just SYS_WDT_RESET, so to be future proof, access to it
   must be somehow synchronized,
 - register area of multiple IPs are being specified in device tree node
   of one IP,
 - every driver that accesses PMU will have to contain such extra reg
   parsing and mapping, instead of just requesting a named regmap.

In addition, on the contrary to keeping this in PMU:
 - the driver must be taught some SoC specific details, even if the IPs
   are identical, without any real need to do so.

I don't insist on keeping this in PMU driver, but at least having a common
regmap for all PMU registers that could be accessed by all drivers that
need to do something in PMU registers would be much more elegant and
future-proof.

Best regards,
Tomasz

[PATCH 4/4] ARM: dts: exynos: add PMU registers addresses and mask bit to watchdog node

[Leela Krishna Amudala]

This patch adds support for specifying the pmu registers and masking bit
for watchdog to enable/disable and mask/unmask the watchdog reset request.

Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Leela Krishna Amudala <l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5.dtsi    |    2 +-
 arch/arm/boot/dts/exynos5250.dtsi |    1 +
 arch/arm/boot/dts/exynos5420.dtsi |    1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index a8bf0c2..e1cc6ef 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -104,7 +104,7 @@
 
 	watchdog@101D0000 {
 		compatible = "samsung,s3c2410-wdt";
-		reg = <0x101D0000 0x100>;
+		reg = <0x101D0000 0x100>, <0x10040408 0x4>, <0x1004040c 0x4>;
 		interrupts = <0 42 0>;
 		status = "disabled";
 	};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 6bb42cf..8bf28a7a 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -161,6 +161,7 @@
 	watchdog@101D0000 {
 		clocks = <&clock 336>;
 		clock-names = "watchdog";
+		reset-mask-bit = <20>;
 		status = "okay";
 	};
 
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 9fadc23..29235ea 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -189,6 +189,7 @@
 	watchdog@101D0000 {
 		clocks = <&clock 316>;
 		clock-names = "watchdog";
+		reset-mask-bit = <0>;
 		status = "okay";
 	};
 
-- 
1.7.9.5

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