[PATCH 0/3] parse watchdog node to read PMU registers addresses

[PATCH 0/3] parse watchdog node to read PMU registers addresses

[Leela Krishna Amudala]

This patch set does the following things
	- add the clock entries to watchdog node.
	- 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 is dependent on
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg20582.html

Note: Tested WDT reset on SMDK5420.

Leela Krishna Amudala (3):
  watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers
    adresses
  ARM: exynos5420: dt: add clock entries to watchdog node
  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                     |    2 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |    7 +++
 drivers/watchdog/s3c2410_wdt.c                     |   56 ++++++++++++++++++++
 4 files changed, 77 insertions(+), 2 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] 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@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 .../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 739dbd3..a6fb86f 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 */
@@ -116,6 +119,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 = to_s3c2410_wdt(wdd);
@@ -346,6 +376,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);
 
@@ -378,6 +410,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");
@@ -451,6 +502,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:
@@ -468,6 +520,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);
@@ -482,6 +535,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);
 }
 
@@ -495,6 +549,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);
 
@@ -510,6 +565,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.10.4

[PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Leela Krishna Amudala]

This patch adds clock entries to watchdog node for exynos5420
as per the common clock framework of exynos5420

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 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 8c54c4b..e1d2d20 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -145,4 +145,10 @@
 		clocks = <&clock 260>, <&clock 131>;
 		clock-names = "uart", "clk_uart_baud0";
 	};
+
+	watchdog {
+		clocks = <&clock 316>;
+		clock-names = "watchdog";
+		status = "okay";
+	};
 };
-- 
1.7.10.4

[PATCH 3/3] 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@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 arch/arm/boot/dts/exynos5.dtsi    |    2 +-
 arch/arm/boot/dts/exynos5420.dtsi |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index f65e124..4d63b74 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -104,7 +104,7 @@
 
 	watchdog {
 		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/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index e1d2d20..ea5b49f 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -150,5 +150,6 @@
 		clocks = <&clock 316>;
 		clock-names = "watchdog";
 		status = "okay";
+		reset-mask-bit = <0>;
 	};
 };
-- 
1.7.10.4

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

[Sachin Kamat]

Hi Leela,

On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> 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@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5.dtsi    |    2 +-
>  arch/arm/boot/dts/exynos5420.dtsi |    1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
> index f65e124..4d63b74 100644
> --- a/arch/arm/boot/dts/exynos5.dtsi
> +++ b/arch/arm/boot/dts/exynos5.dtsi
> @@ -104,7 +104,7 @@
>
>         watchdog {
>                 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/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index e1d2d20..ea5b49f 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -150,5 +150,6 @@
>                 clocks = <&clock 316>;
>                 clock-names = "watchdog";
>                 status = "okay";
> +               reset-mask-bit = <0>;

Why not add this hunk to the previous patch itself?

-- 
With warm regards,
Sachin

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Sachin Kamat]

Hi Leela,

On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> This patch adds clock entries to watchdog node for exynos5420
> as per the common clock framework of exynos5420
>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
>  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 8c54c4b..e1d2d20 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -145,4 +145,10 @@
>                 clocks = <&clock 260>, <&clock 131>;
>                 clock-names = "uart", "clk_uart_baud0";
>         };
> +
> +       watchdog {
> +               clocks = <&clock 316>;
> +               clock-names = "watchdog";
> +               status = "okay";

Generally you do "okay" in specific board dts files.

-- 
With warm regards,
Sachin

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Tomasz Figa]

Hi Sachin,

On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> Hi Leela,
> 
> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com> 
wrote:
> > This patch adds clock entries to watchdog node for exynos5420
> > as per the common clock framework of exynos5420
> > 
> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > ---
> > 
> >  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 8c54c4b..e1d2d20 100644
> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> > @@ -145,4 +145,10 @@
> > 
> >                 clocks = <&clock 260>, <&clock 131>;
> >                 clock-names = "uart", "clk_uart_baud0";
> >         
> >         };
> > 
> > +
> > +       watchdog {
> > +               clocks = <&clock 316>;
> > +               clock-names = "watchdog";
> > +               status = "okay";
> 
> Generally you do "okay" in specific board dts files.

Not necessarily. The status property should be set to okay whenever the 
device represented by such node can already work with given set of 
information (properties).

Given the fact that watchdog driver does not require any board specific 
information, it can be instantiated regardless of the board.

Best regards,
Tomasz

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Sachin Kamat]

Hi Tomasz,

On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Sachin,
>
> On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
>> Hi Leela,
>>
>> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com>
> wrote:
>> > This patch adds clock entries to watchdog node for exynos5420
>> > as per the common clock framework of exynos5420
>> >
>> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>> > Reviewed-by: Doug Anderson <dianders@chromium.org>
>> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> > ---
>> >
>> >  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 8c54c4b..e1d2d20 100644
>> > --- a/arch/arm/boot/dts/exynos5420.dtsi
>> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> > @@ -145,4 +145,10 @@
>> >
>> >                 clocks = <&clock 260>, <&clock 131>;
>> >                 clock-names = "uart", "clk_uart_baud0";
>> >
>> >         };
>> >
>> > +
>> > +       watchdog {
>> > +               clocks = <&clock 316>;
>> > +               clock-names = "watchdog";
>> > +               status = "okay";
>>
>> Generally you do "okay" in specific board dts files.
>
> Not necessarily. The status property should be set to okay whenever the
> device represented by such node can already work with given set of
> information (properties).
>
> Given the fact that watchdog driver does not require any board specific
> information, it can be instantiated regardless of the board.

Yes you are right. But I was thinking of keeping this (enabling) as an
option at the board level.
We do this for some of the other IPs too where even though we have all
the properties we keep them disabled.

-- 
With warm regards,
Sachin

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Leela Krishna Amudala]

Hi Sachin,

On Wed, Jul 24, 2013 at 3:31 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Tomasz,
>
> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Sachin,
>>
>> On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
>>> Hi Leela,
>>>
>>> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com>
>> wrote:
>>> > This patch adds clock entries to watchdog node for exynos5420
>>> > as per the common clock framework of exynos5420
>>> >
>>> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>>> > Reviewed-by: Doug Anderson <dianders@chromium.org>
>>> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>> > ---
>>> >
>>> >  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 8c54c4b..e1d2d20 100644
>>> > --- a/arch/arm/boot/dts/exynos5420.dtsi
>>> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
>>> > @@ -145,4 +145,10 @@
>>> >
>>> >                 clocks = <&clock 260>, <&clock 131>;
>>> >                 clock-names = "uart", "clk_uart_baud0";
>>> >
>>> >         };
>>> >
>>> > +
>>> > +       watchdog {
>>> > +               clocks = <&clock 316>;
>>> > +               clock-names = "watchdog";
>>> > +               status = "okay";
>>>
>>> Generally you do "okay" in specific board dts files.
>>
>> Not necessarily. The status property should be set to okay whenever the
>> device represented by such node can already work with given set of
>> information (properties).
>>
>> Given the fact that watchdog driver does not require any board specific
>> information, it can be instantiated regardless of the board.
>
> Yes you are right. But I was thinking of keeping this (enabling) as an
> option at the board level.
> We do this for some of the other IPs too where even though we have all
> the properties we keep them disabled.
>

Watchdog does not require any board specific information so I kept it
in 5420.dtsi file.

Best wishes,
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 3/3] ARM: dts: exynos: add PMU registers addresses and mask bit to watchdog node

[Leela Krishna Amudala]

Hi Sachin,

On Wed, Jul 24, 2013 at 3:16 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Leela,
>
> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
>> 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@chromium.org>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos5.dtsi    |    2 +-
>>  arch/arm/boot/dts/exynos5420.dtsi |    1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
>> index f65e124..4d63b74 100644
>> --- a/arch/arm/boot/dts/exynos5.dtsi
>> +++ b/arch/arm/boot/dts/exynos5.dtsi
>> @@ -104,7 +104,7 @@
>>
>>         watchdog {
>>                 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/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index e1d2d20..ea5b49f 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -150,5 +150,6 @@
>>                 clocks = <&clock 316>;
>>                 clock-names = "watchdog";
>>                 status = "okay";
>> +               reset-mask-bit = <0>;
>
> Why not add this hunk to the previous patch itself?
>

This particular property holds a bit number that has to be programmed
in the PMU registers (mentioned in exynos5.dtsi).
So I added this property along with those registers.

Best Wishes,
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 3/3] ARM: dts: exynos: add PMU registers addresses and mask bit to watchdog node

[Sachin Kamat]

Hi Leela,

On 24 July 2013 16:24, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
> Hi Sachin,
>
> On Wed, Jul 24, 2013 at 3:16 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> Hi Leela,
>>
>> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com> wrote:
>>> 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@chromium.org>
>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>> ---
>>>  arch/arm/boot/dts/exynos5.dtsi    |    2 +-
>>>  arch/arm/boot/dts/exynos5420.dtsi |    1 +
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
>>> index f65e124..4d63b74 100644
>>> --- a/arch/arm/boot/dts/exynos5.dtsi
>>> +++ b/arch/arm/boot/dts/exynos5.dtsi
>>> @@ -104,7 +104,7 @@
>>>
>>>         watchdog {
>>>                 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/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>>> index e1d2d20..ea5b49f 100644
>>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>>> @@ -150,5 +150,6 @@
>>>                 clocks = <&clock 316>;
>>>                 clock-names = "watchdog";
>>>                 status = "okay";
>>> +               reset-mask-bit = <0>;
>>
>> Why not add this hunk to the previous patch itself?
>>
>
> This particular property holds a bit number that has to be programmed
> in the PMU registers (mentioned in exynos5.dtsi).
> So I added this property along with those registers.

In this case you can swap patches 2 and 3 i.e., add the PMU registers
to exynos5.dtsi in patch 2 and then add the watchdog node along with
reset-mask-bit in patch3.

-- 
With warm regards,
Sachin

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Tomasz Figa]

On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
> Hi Tomasz,
> 
> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Sachin,
> > 
> > On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> >> Hi Leela,
> >> 
> >> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com>
> > 
> > wrote:
> >> > This patch adds clock entries to watchdog node for exynos5420
> >> > as per the common clock framework of exynos5420
> >> > 
> >> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> >> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> >> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> > ---
> >> > 
> >> >  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 8c54c4b..e1d2d20 100644
> >> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> >> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> >> > @@ -145,4 +145,10 @@
> >> > 
> >> >                 clocks = <&clock 260>, <&clock 131>;
> >> >                 clock-names = "uart", "clk_uart_baud0";
> >> >         
> >> >         };
> >> > 
> >> > +
> >> > +       watchdog {
> >> > +               clocks = <&clock 316>;
> >> > +               clock-names = "watchdog";
> >> > +               status = "okay";
> >> 
> >> Generally you do "okay" in specific board dts files.
> > 
> > Not necessarily. The status property should be set to okay whenever the
> > device represented by such node can already work with given set of
> > information (properties).
> > 
> > Given the fact that watchdog driver does not require any board specific
> > information, it can be instantiated regardless of the board.
> 
> Yes you are right. But I was thinking of keeping this (enabling) as an
> option at the board level.
> We do this for some of the other IPs too where even though we have all
> the properties we keep them disabled.

Yes and this is wrong. Device tree is only a way to list all the hardware 
present on particular platform. You don't define which components are used 
or not depending on use case, but rather all the hardware that can be used 
on given board should be enabled on DT level.

To illustrate the problem please consider that in the end, a dtb file will 
be fused into board ROM (or at most flash memory) and passed to the kernel 
by the bootloader. If you disable some hardware on DT level even if it can 
be physically used on the board, there will be no way to reenable it, if 
some user wanted to use it, because that would require editing the fused 
dtb.

Best regards,
Tomasz

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Sachin Kamat]

On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
>> Hi Tomasz,
>>
>> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
>> > Hi Sachin,
>> >
>> > On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
>> >> Hi Leela,
>> >>
>> >> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com>
>> >
>> > wrote:
>> >> > This patch adds clock entries to watchdog node for exynos5420
>> >> > as per the common clock framework of exynos5420
>> >> >
>> >> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>> >> > Reviewed-by: Doug Anderson <dianders@chromium.org>
>> >> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> >> > ---
>> >> >
>> >> >  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 8c54c4b..e1d2d20 100644
>> >> > --- a/arch/arm/boot/dts/exynos5420.dtsi
>> >> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> >> > @@ -145,4 +145,10 @@
>> >> >
>> >> >                 clocks = <&clock 260>, <&clock 131>;
>> >> >                 clock-names = "uart", "clk_uart_baud0";
>> >> >
>> >> >         };
>> >> >
>> >> > +
>> >> > +       watchdog {
>> >> > +               clocks = <&clock 316>;
>> >> > +               clock-names = "watchdog";
>> >> > +               status = "okay";
>> >>
>> >> Generally you do "okay" in specific board dts files.
>> >
>> > Not necessarily. The status property should be set to okay whenever the
>> > device represented by such node can already work with given set of
>> > information (properties).
>> >
>> > Given the fact that watchdog driver does not require any board specific
>> > information, it can be instantiated regardless of the board.
>>
>> Yes you are right. But I was thinking of keeping this (enabling) as an
>> option at the board level.
>> We do this for some of the other IPs too where even though we have all
>> the properties we keep them disabled.
>
> Yes and this is wrong. Device tree is only a way to list all the hardware
> present on particular platform. You don't define which components are used
> or not depending on use case, but rather all the hardware that can be used
> on given board should be enabled on DT level.

This is contrary to the fact that we disable everything by default in
the top level dt files and only enable them as required in the board
dts files.

>
> To illustrate the problem please consider that in the end, a dtb file will
> be fused into board ROM (or at most flash memory) and passed to the kernel
> by the bootloader. If you disable some hardware on DT level even if it can
> be physically used on the board, there will be no way to reenable it, if
> some user wanted to use it, because that would require editing the fused
> dtb.

I believe some h/w will be disabled in dt only if it should not be
used for whatever reason. If there is no reason then ofcourse they
would be enabled IMHO. Whatever be the case the choice of enabling or
disabling should be done at the leaf node (at board level). No?

-- 
With warm regards,
Sachin

RE: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Kukjin Kim]

Sachin Kamat wrote:
> 
> On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
> > On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
> >> Hi Tomasz,
> >>
> >> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> >> > Hi Sachin,
> >> >
> >> > On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> >> >> Hi Leela,
> >> >>
> >> >> On 24 July 2013 15:08, Leela Krishna Amudala <l.krishna@samsung.com>
> >> >
> >> > wrote:
> >> >> > This patch adds clock entries to watchdog node for exynos5420
> >> >> > as per the common clock framework of exynos5420
> >> >> >
> >> >> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> >> >> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> >> >> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> >> > ---
> >> >> >
> >> >> >  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 8c54c4b..e1d2d20 100644
> >> >> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> >> >> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> >> >> > @@ -145,4 +145,10 @@
> >> >> >
> >> >> >                 clocks = <&clock 260>, <&clock 131>;
> >> >> >                 clock-names = "uart", "clk_uart_baud0";
> >> >> >
> >> >> >         };
> >> >> >
> >> >> > +
> >> >> > +       watchdog {
> >> >> > +               clocks = <&clock 316>;
> >> >> > +               clock-names = "watchdog";
> >> >> > +               status = "okay";
> >> >>
> >> >> Generally you do "okay" in specific board dts files.
> >> >
> >> > Not necessarily. The status property should be set to okay whenever
> the
> >> > device represented by such node can already work with given set of
> >> > information (properties).
> >> >
> >> > Given the fact that watchdog driver does not require any board
> specific
> >> > information, it can be instantiated regardless of the board.
> >>
> >> Yes you are right. But I was thinking of keeping this (enabling) as an
> >> option at the board level.
> >> We do this for some of the other IPs too where even though we have all
> >> the properties we keep them disabled.
> >
> > Yes and this is wrong. Device tree is only a way to list all the
> hardware
> > present on particular platform. You don't define which components are
> used
> > or not depending on use case, but rather all the hardware that can be
> used
> > on given board should be enabled on DT level.
> 
> This is contrary to the fact that we disable everything by default in
> the top level dt files and only enable them as required in the board
> dts files.
> 
> >
> > To illustrate the problem please consider that in the end, a dtb file
> will
> > be fused into board ROM (or at most flash memory) and passed to the
> kernel
> > by the bootloader. If you disable some hardware on DT level even if it
> can
> > be physically used on the board, there will be no way to reenable it, if
> > some user wanted to use it, because that would require editing the fused
> > dtb.
> 
> I believe some h/w will be disabled in dt only if it should not be
> used for whatever reason. If there is no reason then ofcourse they
> would be enabled IMHO. Whatever be the case the choice of enabling or
> disabling should be done at the leaf node (at board level). No?
> 
In my opinion, some IPs can be disabled according to the board manager. And
if updated DTB is required due to kernel updating or whatever, DTB should be
updated accordingly.

One more, actually we don't have no general way to fuse DTB in firmware when
kernel is updated. So board manger who controls/decides what features should
be supported on their board should consider the board's features and
environments...

- Kukjin

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

[Kukjin Kim]

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@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
>  .../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 739dbd3..a6fb86f 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 */
> @@ -116,6 +119,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 = to_s3c2410_wdt(wdd);
> @@ -346,6 +376,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);
> 
> @@ -378,6 +410,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");
> @@ -451,6 +502,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:
> @@ -468,6 +520,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);
> @@ -482,6 +535,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);
>  }
> 
> @@ -495,6 +549,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);
> 
> @@ -510,6 +565,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.10.4

+ Wim Van Sebroeck who is a maintainer for WDT :-)

- Kukjin

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Tomasz Figa]

On Wednesday 24 of July 2013 20:56:15 Kukjin Kim wrote:
> Sachin Kamat wrote:
> > On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
> > > On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
> > >> Hi Tomasz,
> > >> 
> > >> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> > >> > Hi Sachin,
> > >> > 
> > >> > On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> > >> >> Hi Leela,
> > >> >> 
> > >> >> On 24 July 2013 15:08, Leela Krishna Amudala
> > >> >> <l.krishna@samsung.com>
> > >> > 
> > >> > wrote:
> > >> >> > This patch adds clock entries to watchdog node for exynos5420
> > >> >> > as per the common clock framework of exynos5420
> > >> >> > 
> > >> >> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> > >> >> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> > >> >> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > >> >> > ---
> > >> >> > 
> > >> >> >  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 8c54c4b..e1d2d20
> > >> >> > 100644
> > >> >> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> > >> >> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> > >> >> > @@ -145,4 +145,10 @@
> > >> >> > 
> > >> >> >                 clocks = <&clock 260>, <&clock 131>;
> > >> >> >                 clock-names = "uart", "clk_uart_baud0";
> > >> >> >         
> > >> >> >         };
> > >> >> > 
> > >> >> > +
> > >> >> > +       watchdog {
> > >> >> > +               clocks = <&clock 316>;
> > >> >> > +               clock-names = "watchdog";
> > >> >> > +               status = "okay";
> > >> >> 
> > >> >> Generally you do "okay" in specific board dts files.
> > >> > 
> > >> > Not necessarily. The status property should be set to okay
> > >> > whenever
> > 
> > the
> > 
> > >> > device represented by such node can already work with given set of
> > >> > information (properties).
> > >> > 
> > >> > Given the fact that watchdog driver does not require any board
> > 
> > specific
> > 
> > >> > information, it can be instantiated regardless of the board.
> > >> 
> > >> Yes you are right. But I was thinking of keeping this (enabling) as
> > >> an
> > >> option at the board level.
> > >> We do this for some of the other IPs too where even though we have
> > >> all
> > >> the properties we keep them disabled.
> > > 
> > > Yes and this is wrong. Device tree is only a way to list all the
> > 
> > hardware
> > 
> > > present on particular platform. You don't define which components are
> > 
> > used
> > 
> > > or not depending on use case, but rather all the hardware that can be
> > 
> > used
> > 
> > > on given board should be enabled on DT level.
> > 
> > This is contrary to the fact that we disable everything by default in
> > the top level dt files and only enable them as required in the board
> > dts files.
> > 
> > > To illustrate the problem please consider that in the end, a dtb file
> > 
> > will
> > 
> > > be fused into board ROM (or at most flash memory) and passed to the
> > 
> > kernel
> > 
> > > by the bootloader. If you disable some hardware on DT level even if
> > > it
> > 
> > can
> > 
> > > be physically used on the board, there will be no way to reenable it,
> > > if
> > > some user wanted to use it, because that would require editing the
> > > fused
> > > dtb.
> > 
> > I believe some h/w will be disabled in dt only if it should not be
> > used for whatever reason. If there is no reason then ofcourse they
> > would be enabled IMHO. Whatever be the case the choice of enabling or
> > disabling should be done at the leaf node (at board level). No?
> 
> In my opinion, some IPs can be disabled according to the board manager.
> And if updated DTB is required due to kernel updating or whatever, DTB
> should be updated accordingly.

No, not really. DTB should considered as immutable and modification of which 
should be allowed only in special cases. Currently such huge special case 
is that our DT bindings are still in development and so device tree is 
likely to change, but we are getting towards stabilizing them and so things 
should be done with correct assumptions.

> One more, actually we don't have no general way to fuse DTB in firmware
> when kernel is updated. So board manger who controls/decides what
> features should be supported on their board should consider the board's
> features and environments...

Again, device tree is not a way to specify use cases. It is a way to list 
hardware available on the platform and its parameters. DTB should be set up 
in a way enabling users to use any hardware available on their machines, 
without the need to change DTB.

Best regards,
Tomasz

P.S. CCing more Device Tree people for their opinion on this.

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Tomasz Figa]

On Wednesday 24 of July 2013 16:51:06 Sachin Kamat wrote:
> On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
> > On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
> >> Hi Tomasz,
> >> 
> >> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> >> > Hi Sachin,
> >> > 
> >> > On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> >> >> Hi Leela,
> >> >> 
> >> >> On 24 July 2013 15:08, Leela Krishna Amudala
> >> >> <l.krishna@samsung.com>
> >> > 
> >> > wrote:
> >> >> > This patch adds clock entries to watchdog node for exynos5420
> >> >> > as per the common clock framework of exynos5420
> >> >> > 
> >> >> > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> >> >> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> >> >> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> >> > ---
> >> >> > 
> >> >> >  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 8c54c4b..e1d2d20 100644
> >> >> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> >> >> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> >> >> > @@ -145,4 +145,10 @@
> >> >> > 
> >> >> >                 clocks = <&clock 260>, <&clock 131>;
> >> >> >                 clock-names = "uart", "clk_uart_baud0";
> >> >> >         
> >> >> >         };
> >> >> > 
> >> >> > +
> >> >> > +       watchdog {
> >> >> > +               clocks = <&clock 316>;
> >> >> > +               clock-names = "watchdog";
> >> >> > +               status = "okay";
> >> >> 
> >> >> Generally you do "okay" in specific board dts files.
> >> > 
> >> > Not necessarily. The status property should be set to okay whenever
> >> > the
> >> > device represented by such node can already work with given set of
> >> > information (properties).
> >> > 
> >> > Given the fact that watchdog driver does not require any board
> >> > specific
> >> > information, it can be instantiated regardless of the board.
> >> 
> >> Yes you are right. But I was thinking of keeping this (enabling) as an
> >> option at the board level.
> >> We do this for some of the other IPs too where even though we have all
> >> the properties we keep them disabled.
> > 
> > Yes and this is wrong. Device tree is only a way to list all the
> > hardware present on particular platform. You don't define which
> > components are used or not depending on use case, but rather all the
> > hardware that can be used on given board should be enabled on DT
> > level.
> 
> This is contrary to the fact that we disable everything by default in
> the top level dt files and only enable them as required in the board
> dts files.

No, we don't disable everything. We disable things that require board 
specific setup or can't work without other support from board side. If there 
is some hardware disabled in SoC level dtsi that can work without any 
support from board side, then it is a BUG() and must be fixed.

> > To illustrate the problem please consider that in the end, a dtb file
> > will be fused into board ROM (or at most flash memory) and passed to
> > the kernel by the bootloader. If you disable some hardware on DT level
> > even if it can be physically used on the board, there will be no way
> > to reenable it, if some user wanted to use it, because that would
> > require editing the fused dtb.
> 
> I believe some h/w will be disabled in dt only if it should not be
> used for whatever reason. If there is no reason then ofcourse they
> would be enabled IMHO.

Yes. This is what I meant. However the reason must be valid - e.g. 
"hardware does not allow such configuration", not like "some very important 
manager decided that this board should not use this".

> Whatever be the case the choice of enabling or
> disabling should be done at the leaf node (at board level). No?

It depends. For components that don't require any support from board side 
it can be globally enabled on SoC level. If a SoC component requires 
support from board side (like regulators, GPIOs, etc.) then it should be 
disabled on SoC level and enabled on board level only if all the 
dependencies are provided.

Best regards,
Tomasz

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Sylwester Nawrocki]

On 07/24/2013 04:09 PM, Tomasz Figa wrote:
> On Wednesday 24 of July 2013 20:56:15 Kukjin Kim wrote:
>> Sachin Kamat wrote:
>>> On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
>>>> On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
>>>>> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>>> On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
>>>>>>> On 24 July 2013 15:08, Leela Krishna Amudala
>>>>>>> <l.krishna@samsung.com>
>>>>>> wrote:
>>>>>>>> This patch adds clock entries to watchdog node for exynos5420
>>>>>>>> as per the common clock framework of exynos5420
>>>>>>>>
>>>>>>>> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>>>>>> Reviewed-by: Doug Anderson <dianders@chromium.org>
>>>>>>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  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 8c54c4b..e1d2d20
>>>>>>>> 100644
>>>>>>>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>>>>>>>> @@ -145,4 +145,10 @@
>>>>>>>>
>>>>>>>>                 clocks = <&clock 260>, <&clock 131>;
>>>>>>>>                 clock-names = "uart", "clk_uart_baud0";
>>>>>>>>         
>>>>>>>>         };
>>>>>>>>
>>>>>>>> +
>>>>>>>> +       watchdog {
>>>>>>>> +               clocks = <&clock 316>;
>>>>>>>> +               clock-names = "watchdog";
>>>>>>>> +               status = "okay";
>>>>>>>
>>>>>>> Generally you do "okay" in specific board dts files.
>>>>>>
>>>>>> Not necessarily. The status property should be set to okay
>>>>>> whenever the
>>>>>> device represented by such node can already work with given set of
>>>>>> information (properties).
>>>>>>
>>>>>> Given the fact that watchdog driver does not require any board
>>> specific
>>>>>> information, it can be instantiated regardless of the board.
>>>>>
>>>>> Yes you are right. But I was thinking of keeping this (enabling) as
>>>>> an
>>>>> option at the board level.
>>>>> We do this for some of the other IPs too where even though we have
>>>>> all
>>>>> the properties we keep them disabled.
>>>>
>>>> Yes and this is wrong. Device tree is only a way to list all the
>>> hardware
>>>> present on particular platform. You don't define which components are
>>> used
>>>> or not depending on use case, but rather all the hardware that can be
>>> used
>>>> on given board should be enabled on DT level.
>>>
>>> This is contrary to the fact that we disable everything by default in
>>> the top level dt files and only enable them as required in the board
>>> dts files.
>>>
>>>> To illustrate the problem please consider that in the end, a dtb file
>>> will
>>>> be fused into board ROM (or at most flash memory) and passed to the
>>> kernel
>>>> by the bootloader. If you disable some hardware on DT level even if
>>>> it can
>>>> be physically used on the board, there will be no way to reenable it,
>>>> if
>>>> some user wanted to use it, because that would require editing the
>>>> fused dtb.
>>>
>>> I believe some h/w will be disabled in dt only if it should not be
>>> used for whatever reason. If there is no reason then ofcourse they
>>> would be enabled IMHO. Whatever be the case the choice of enabling or
>>> disabling should be done at the leaf node (at board level). No?
>>
>> In my opinion, some IPs can be disabled according to the board manager.
>> And if updated DTB is required due to kernel updating or whatever, DTB
>> should be updated accordingly.
> 
> No, not really. DTB should considered as immutable and modification of which 
> should be allowed only in special cases. Currently such huge special case 
> is that our DT bindings are still in development and so device tree is 
> likely to change, but we are getting towards stabilizing them and so things 
> should be done with correct assumptions.
> 
>> One more, actually we don't have no general way to fuse DTB in firmware
>> when kernel is updated. So board manger who controls/decides what
>> features should be supported on their board should consider the board's
>> features and environments...

I agree, device tree needs to be really stable and well standardized
to even consider storing it in OTP memory. In most cases it's not a big
deal to update the firmware and same applies to DTB. And disabling IP
blocks that will never be used in a complex SoC, depending on how the
SoC is wired up on the board, should be IMO nothing unusual.

I think this discussion is more on _where_ we disable devices - in dtsi
or board dts files, rather than _if_ we disable some IP blocks in firmware.

> Again, device tree is not a way to specify use cases. It is a way to list 
> hardware available on the platform and its parameters. DTB should be set up 
> in a way enabling users to use any hardware available on their machines, 
> without the need to change DTB.

I don't think there is a reason not to allow to have some functionality
disabled by the firmware. Embedded systems are often highly specialized
and may use only a subset of functionality an SoC provides. And the
Application Processor SoCs are often loaded with so many features it is
sensible to have all disabled by default and enable only what's needed
in the board dts file. It's probably better to stick to one rule - either
all devices have status property set to "disabled" by default in common
dtsi files or all are enabled by default. Having a mixture of both is
a bit messy IMHO.

Besides, I believe it should be all considered individually for each
chipset. IMHO there is not much point in enforcing general rule that
everything what's possible but not necessarily sensible should be
enabled.

> Best regards,
> Tomasz
> 
> P.S. CCing more Device Tree people for their opinion on this.

Thanks,
Sylwester

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Tomasz Figa]

On Wednesday 24 of July 2013 16:52:43 Sylwester Nawrocki wrote:
> On 07/24/2013 04:09 PM, Tomasz Figa wrote:
> > On Wednesday 24 of July 2013 20:56:15 Kukjin Kim wrote:
> >> Sachin Kamat wrote:
> >>> On 24 July 2013 16:42, Tomasz Figa <t.figa@samsung.com> wrote:
> >>>> On Wednesday 24 of July 2013 15:31:43 Sachin Kamat wrote:
> >>>>> On 24 July 2013 15:24, Tomasz Figa <t.figa@samsung.com> wrote:
> >>>>>> On Wednesday 24 of July 2013 15:18:26 Sachin Kamat wrote:
> >>>>>>> On 24 July 2013 15:08, Leela Krishna Amudala
> >>>>>>> <l.krishna@samsung.com>
> >>>>>> 
> >>>>>> wrote:
> >>>>>>>> This patch adds clock entries to watchdog node for exynos5420
> >>>>>>>> as per the common clock framework of exynos5420
> >>>>>>>> 
> >>>>>>>> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> >>>>>>>> Reviewed-by: Doug Anderson <dianders@chromium.org>
> >>>>>>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>>  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 8c54c4b..e1d2d20
> >>>>>>>> 100644
> >>>>>>>> --- a/arch/arm/boot/dts/exynos5420.dtsi
> >>>>>>>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> >>>>>>>> @@ -145,4 +145,10 @@
> >>>>>>>> 
> >>>>>>>>                 clocks = <&clock 260>, <&clock 131>;
> >>>>>>>>                 clock-names = "uart", "clk_uart_baud0";
> >>>>>>>>         
> >>>>>>>>         };
> >>>>>>>> 
> >>>>>>>> +
> >>>>>>>> +       watchdog {
> >>>>>>>> +               clocks = <&clock 316>;
> >>>>>>>> +               clock-names = "watchdog";
> >>>>>>>> +               status = "okay";
> >>>>>>> 
> >>>>>>> Generally you do "okay" in specific board dts files.
> >>>>>> 
> >>>>>> Not necessarily. The status property should be set to okay
> >>>>>> whenever the
> >>>>>> device represented by such node can already work with given set of
> >>>>>> information (properties).
> >>>>>> 
> >>>>>> Given the fact that watchdog driver does not require any board
> >>> 
> >>> specific
> >>> 
> >>>>>> information, it can be instantiated regardless of the board.
> >>>>> 
> >>>>> Yes you are right. But I was thinking of keeping this (enabling) as
> >>>>> an
> >>>>> option at the board level.
> >>>>> We do this for some of the other IPs too where even though we have
> >>>>> all
> >>>>> the properties we keep them disabled.
> >>>> 
> >>>> Yes and this is wrong. Device tree is only a way to list all the
> >>> 
> >>> hardware
> >>> 
> >>>> present on particular platform. You don't define which components
> >>>> are
> >>> 
> >>> used
> >>> 
> >>>> or not depending on use case, but rather all the hardware that can
> >>>> be
> >>> 
> >>> used
> >>> 
> >>>> on given board should be enabled on DT level.
> >>> 
> >>> This is contrary to the fact that we disable everything by default in
> >>> the top level dt files and only enable them as required in the board
> >>> dts files.
> >>> 
> >>>> To illustrate the problem please consider that in the end, a dtb
> >>>> file
> >>> 
> >>> will
> >>> 
> >>>> be fused into board ROM (or at most flash memory) and passed to the
> >>> 
> >>> kernel
> >>> 
> >>>> by the bootloader. If you disable some hardware on DT level even if
> >>>> it can
> >>>> be physically used on the board, there will be no way to reenable
> >>>> it,
> >>>> if
> >>>> some user wanted to use it, because that would require editing the
> >>>> fused dtb.
> >>> 
> >>> I believe some h/w will be disabled in dt only if it should not be
> >>> used for whatever reason. If there is no reason then ofcourse they
> >>> would be enabled IMHO. Whatever be the case the choice of enabling or
> >>> disabling should be done at the leaf node (at board level). No?
> >> 
> >> In my opinion, some IPs can be disabled according to the board
> >> manager.
> >> And if updated DTB is required due to kernel updating or whatever, DTB
> >> should be updated accordingly.
> > 
> > No, not really. DTB should considered as immutable and modification of
> > which should be allowed only in special cases. Currently such huge
> > special case is that our DT bindings are still in development and so
> > device tree is likely to change, but we are getting towards
> > stabilizing them and so things should be done with correct
> > assumptions.
> > 
> >> One more, actually we don't have no general way to fuse DTB in
> >> firmware
> >> when kernel is updated. So board manger who controls/decides what
> >> features should be supported on their board should consider the
> >> board's
> >> features and environments...
> 
> I agree, device tree needs to be really stable and well standardized
> to even consider storing it in OTP memory. In most cases it's not a big
> deal to update the firmware and same applies to DTB. And disabling IP
> blocks that will never be used in a complex SoC, depending on how the
> SoC is wired up on the board, should be IMO nothing unusual.

Yes, the case of IPs that are not wired up on the board is obvious, but if 
such IP can be used even if it's not wired (consider our FIMC as an 
example, which can be used as a mem to mem device, even without any camera 
sensors connected) then DTB should allow users to use it. Users should not 
be restricted by DTB.

> I think this discussion is more on _where_ we disable devices - in dtsi
> or board dts files, rather than _if_ we disable some IP blocks in
> firmware.

Where we put the status = "okay" or "disabled" clause is a completely 
different question. It is just a convention. I suggested using following 
convention:
 - a device should have status = "disabled" in top level instance of its 
node, if it can't be used without extra data from lower levels, otherwise 
no status should be set for it, which defaults to "okay",
 - status should be overridden to "okay" in an instance of device node 
which provides sufficient amount of data to make the device operational, at 
least to some extent.

> > Again, device tree is not a way to specify use cases. It is a way to
> > list hardware available on the platform and its parameters. DTB should
> > be set up in a way enabling users to use any hardware available on
> > their machines, without the need to change DTB.
> 
> I don't think there is a reason not to allow to have some functionality
> disabled by the firmware. Embedded systems are often highly specialized
> and may use only a subset of functionality an SoC provides. And the
> Application Processor SoCs are often loaded with so many features it is
> sensible to have all disabled by default and enable only what's needed
> in the board dts file.

If firmware can set up the unused hardware to defined, disabled (powered 
down) state, then it's fine. But as we all know, state left after firmware is 
rarely ideal and we need drivers even for unused hardware to put it into 
some defined (powered down) state.

In addition, having the device enabled in device tree doesn't imply that 
such device must be used. The same for binding a driver to such device. 
Having the driver loaded should not have any negative effects if the 
hardware is not used. If it does, then something is wrong with such driver.

> It's probably better to stick to one rule - either
> all devices have status property set to "disabled" by default in common
> dtsi files or all are enabled by default. Having a mixture of both is a
> bit messy IMHO.

Not really. This is a well defined rule. According to ePAPR Version 1.1 
chapter 2.3.4:

The status property indicates the operational status of a device.

“okay”
Indicates the device is operational

“disabled”
Indicates that the device is not presently operational, but it might
become operational in the future (for example, something is not
plugged in, or switched off).

This is not about the device being used or not, but whether it is 
operational and can be used. Where we define value of this property is just 
our convention, but it should represent the real environment, not something 
a manager decided.

> Besides, I believe it should be all considered individually for each
> chipset. IMHO there is not much point in enforcing general rule that
> everything what's possible but not necessarily sensible should be
> enabled.

Device Tree is all about general rules. This is not a board file where you 
are free to define whatever kind of proprietary method to pass platform 
specific data to your drivers. If we don't enforce some general rules we 
will end up with a real mess of each platform (or even parts of the same 
platforms) doing completely different things, stretching device tree 
infrastructure just to fit their needs.

Best regards,
Tomasz

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

[Tomasz Figa]

Hi Leela,

On Wednesday 24 of July 2013 15:08:17 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@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
>  .../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.

I don't think PMU registers should be passed like this, even if USB PHY 
driver currently does it - it needs to be fixed. Every time you are mapping 
a single 4-byte register, you are creating new mapping for the whole page 
(4K) containing it. In addition, this makes the WDT driver map IO memory 
that does not belong to the IP block it handles, which is not nice.

I would suggest looking into regmap helpers to implement a driver for the 
PMU that can share PMU registers with interested drivers.

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

If WDT of Exynos 5420 needs special masking, it is no longer compatible 
with "samsung,s3c2410-wdt". You need to create separate compatible for it 
("samsung,exynos5420-wdt") and do this special masking only for devices 
using it.

> +watchdog {
> +	compatible = "samsung,s3c2410-wdt";
> +	reg = <0x101D0000 0x100>, <0x10040408 0x4>, <0x1004040c 0x4>;
> +	interrupts = <0 42 0>;
> +	status = "disabled";
> +	reset-mask-bit = <0>;
> +};

By the way, you need to Cc devicetree@vger.kernel.org mailing if you modify 
device tree bindings.

Best regards,
Tomasz

Re: [PATCH 2/3] ARM: exynos5420: dt: add clock entries to watchdog node

[Stephen Warren]

On 07/24/2013 07:14 AM, Tomasz Figa wrote:
> On Wednesday 24 of July 2013 16:51:06 Sachin Kamat wrote:
...
>> This is contrary to the fact that we disable everything by default in
>> the top level dt files and only enable them as required in the board
>> dts files.
> 
> No, we don't disable everything. We disable things that require board 
> specific setup or can't work without other support from board side. If there 
> is some hardware disabled in SoC level dtsi that can work without any 
> support from board side, then it is a BUG() and must be fixed.
> 
>>> To illustrate the problem please consider that in the end, a dtb file
>>> will be fused into board ROM (or at most flash memory) and passed to
>>> the kernel by the bootloader. If you disable some hardware on DT level
>>> even if it can be physically used on the board, there will be no way
>>> to reenable it, if some user wanted to use it, because that would
>>> require editing the fused dtb.
>>
>> I believe some h/w will be disabled in dt only if it should not be
>> used for whatever reason. If there is no reason then ofcourse they
>> would be enabled IMHO.
> 
> Yes. This is what I meant. However the reason must be valid - e.g. 
> "hardware does not allow such configuration", not like "some very important 
> manager decided that this board should not use this".
> 
>> Whatever be the case the choice of enabling or
>> disabling should be done at the leaf node (at board level). No?
> 
> It depends. For components that don't require any support from board side 
> it can be globally enabled on SoC level. If a SoC component requires 
> support from board side (like regulators, GPIOs, etc.) then it should be 
> disabled on SoC level and enabled on board level only if all the 
> dependencies are provided.

I tend to agree with Tomasz.

One note though: This is talking about the *.dts files, which may be
different from the DTB that gets passed to the kernel.

In other words, I don't think that the SoC or board .dtsi file (at least
public versions that are hosted outside company-internal/downstream
branches) have any business disabling HW based on policy or use-cases,
rather than actual HW issues such as requiring other HW to support it
that isn't present or doesn't work.

However, I don't think anyone can influence what e.g.a bootloader does
to the DTB before passing it to the kernel; it could add
status="disabled" to some nodes based on policy, and nobody in the Linux
kernel has any absolute right to influence it, although there's sure a
right to complain about it and point out why it's a bad idea.

Equally, if somebody is creating a "BSP" (ick!) for a specific product's
production flash image, rather than contributing to upstream SW support
for that HW board, we probably don't have too much say in what they do.

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

[Doug Anderson]

Tomasz,

On Thu, Jul 25, 2013 at 3:27 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Leela,
>
> On Wednesday 24 of July 2013 15:08:17 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@chromium.org>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> ---
>>  .../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.
>
> I don't think PMU registers should be passed like this, even if USB PHY
> driver currently does it - it needs to be fixed. Every time you are mapping
> a single 4-byte register, you are creating new mapping for the whole page
> (4K) containing it. In addition, this makes the WDT driver map IO memory
> that does not belong to the IP block it handles, which is not nice.
>
> I would suggest looking into regmap helpers to implement a driver for the
> PMU that can share PMU registers with interested drivers.

We've done this in some other drivers as well.  I remember someone
suggested that this was fine when I posted the change to the ADC
driver for something similar.

Do you really think it's worth adding a level of abstraction here?
The argument I remember in the past was that it was fine as long as
the register itself was used only by this driver.

If we want to do as you say, we'll have to figure out whether you want
to create a new generic interface for this or reuse an existing one.
You could kinda think of these bits as enabling power, so you could
use the regulator framework.  ...but that framework often gets abused.

Olof: I think I got your advice when I did the ADC work.  Do you have
any advice here?

Thanks!

-Doug

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

[Olof Johansson]

On Thu, Jul 25, 2013 at 5:32 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Thu, Jul 25, 2013 at 3:27 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Leela,
>>
>> On Wednesday 24 of July 2013 15:08:17 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@chromium.org>
>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>> ---
>>>  .../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.
>>
>> I don't think PMU registers should be passed like this, even if USB PHY
>> driver currently does it - it needs to be fixed. Every time you are mapping
>> a single 4-byte register, you are creating new mapping for the whole page
>> (4K) containing it. In addition, this makes the WDT driver map IO memory
>> that does not belong to the IP block it handles, which is not nice.
>>
>> I would suggest looking into regmap helpers to implement a driver for the
>> PMU that can share PMU registers with interested drivers.
>
> We've done this in some other drivers as well.  I remember someone
> suggested that this was fine when I posted the change to the ADC
> driver for something similar.
>
> Do you really think it's worth adding a level of abstraction here?
> The argument I remember in the past was that it was fine as long as
> the register itself was used only by this driver.
>
> If we want to do as you say, we'll have to figure out whether you want
> to create a new generic interface for this or reuse an existing one.
> You could kinda think of these bits as enabling power, so you could
> use the regulator framework.  ...but that framework often gets abused.
>
> Olof: I think I got your advice when I did the ADC work.  Do you have
> any advice here?

These tradeoffs are always tricky since it's hard to decide what
corners are OK to cut a bit off of, and when you have to be a stickler
for really following hard strict rules and stick to abstraction
models.

I think the more reasonable approach is the one you've taken here,
Doug. I think we risk over-abstracting this otherwise, to little
actual gain. The register is not shared so there's no conflict in that
area. The argument that it wastes a 4K page isn't a big deal either,
since we don't do this everywhere all the time.

I'd rather have the register described in the reg range like this,
than having the driver go out and find the PMU node and find a magic
offset into that, with or without an abstraction layer for the PMU
register access.


-Olof