Make Armada 375 use the reference clock when possible

drivers/watchdog/orion_wdt.c +29 -0

+static int armada375_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (!IS_ERR(dev->clk)) {
+		ret = clk_prepare_enable(dev->clk);
+		if (ret) {
+			clk_put(dev->clk);
+			return ret;
+		}
+
+		atomic_io_modify(dev->reg + TIMER_CTRL,
+				WDT_AXP_FIXED_ENABLE_BIT,
+				WDT_AXP_FIXED_ENABLE_BIT);
+		dev->clk_rate = clk_get_rate(dev->clk);
+
+		return 0;
+	}
+
+	/* Mandatory fallback for proper devicetree backward compatibility */
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		clk_put(dev->clk);

drivers/watchdog/orion_wdt.c +2 -1

+
 static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 				   struct orion_watchdog *dev)
 {

@@ -394,7 +434,7 @@ static const struct orion_watchdog_data armada375_data = {
 	.rstout_mask_bit = BIT(10),
 	.wdt_enable_bit = BIT(8),
 	.wdt_counter_offset = 0x34,
-	.clock_init = armada370_wdt_clock_init,
+	.clock_init = armada375_wdt_clock_init,
 	.enabled = armada375_enabled,
 	.start = armada375_start,
 	.stop = armada375_stop,

Hi Ezequiel,

[...]

Quoted 29 lines

drivers/watchdog/orion_wdt.c +29 -0

+static int armada375_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (!IS_ERR(dev->clk)) {
+		ret = clk_prepare_enable(dev->clk);
+		if (ret) {
+			clk_put(dev->clk);
+			return ret;
+		}
+
+		atomic_io_modify(dev->reg + TIMER_CTRL,
+				WDT_AXP_FIXED_ENABLE_BIT,
+				WDT_AXP_FIXED_ENABLE_BIT);
+		dev->clk_rate = clk_get_rate(dev->clk);
+
+		return 0;
+	}
+
+	/* Mandatory fallback for proper devicetree backward compatibility */
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		clk_put(dev->clk);

drivers/watchdog/orion_wdt.c static int armada370_wdt_clock_init(struct platform_device *pdev, · line 155

Quoted 10 lines

drivers/watchdog/orion_wdt.c +10 -0

+		return ret;
+	}
+
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+
+	return 0;
+}

Shouldn't be possible to do the following:

static int armada375_wdt_clock_init(struct platform_device *pdev,
struct orion_watchdog *dev)

{
	if (armadaxp_wdt_clock_init(pdev, dev)) {
		/* Mandatory fallback for proper devicetree backward compatibility */
		return armadaxp_wdt_clock_init(pdev, dev));

	}
	return 0;
}

Actually reusing the armadaxp_wdt_clock_init() function was also suggested by Thomas on your first version but I didn't find your answer about it.

Quoted 46 lines · Gregory CLEMENT

Hi Ezequiel,

[...]

Quoted 29 lines

drivers/watchdog/orion_wdt.c +29 -0

+static int armada375_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (!IS_ERR(dev->clk)) {
+		ret = clk_prepare_enable(dev->clk);
+		if (ret) {
+			clk_put(dev->clk);
+			return ret;
+		}
+
+		atomic_io_modify(dev->reg + TIMER_CTRL,
+				WDT_AXP_FIXED_ENABLE_BIT,
+				WDT_AXP_FIXED_ENABLE_BIT);
+		dev->clk_rate = clk_get_rate(dev->clk);
+
+		return 0;
+	}
+
+	/* Mandatory fallback for proper devicetree backward compatibility */
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		clk_put(dev->clk);

drivers/watchdog/orion_wdt.c static int armada370_wdt_clock_init(struct platform_device *pdev, · line 155

Quoted 10 lines

drivers/watchdog/orion_wdt.c +10 -0

+		return ret;
+	}
+
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+
+	return 0;
+}

Shouldn't be possible to do the following:

Quoted 6 lines

static int armada375_wdt_clock_init(struct platform_device *pdev,
struct orion_watchdog *dev)

{
	if (armadaxp_wdt_clock_init(pdev, dev)) {
		/* Mandatory fallback for proper devicetree backward compatibility */
		return armadaxp_wdt_clock_init(pdev, dev));

I guess you meant armada370_wdt_clock_init for the fallback?

Quoted 8 lines

	}
	return 0;
}

Actually reusing the armadaxp_wdt_clock_init() function was also suggested by Thomas on your first version but I didn't find your answer about it.

I replied here to the same objection on the clocksource driver:
http://www.spinics.net/lists/linux-watchdog/msg05318.html

I found that it's a fragile practice, just to save a few lines of code.

Someone can go and change the 370/xp clock init, in some way that's incompatible with 375.

I guess I'm being paranoid, but it's a way to keep the code robust and we are only duplicating a few lines.

The 25 MHz reference clock has better stability so its use is prefered over the core clock. Change the Armada 375 clock initialization to use this reference clock. To ensure the driver is compatible with an old devicetree, also provide a fallback path which will silently return to the previous behavior.

While here, add the clock specification to the binding documentation.

Acked-by Jason Cooper <[email protected]>

Reviewed-by Thomas Petazzoni <[email protected]>

Tested-by Thomas Petazzoni <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
 drivers/watchdog/orion_wdt.c                       | 42 +++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/marvel.txt +13 -0

index 97223fd..858ed92 100644
@@ -17,6 +17,18 @@ For "marvell,armada-375-wdt" and "marvell,armada-380-wdt":
 - reg		: A third entry is mandatory and should contain the
                   shared mask/unmask RSTOUT address.
 
+Clocks required for compatibles = "marvell,orion-wdt",
+				  "marvell,armada-370-wdt":
+- clocks : Must contain a single entry describing the clock input
+
+Clocks required for compatibles = "marvell,armada-xp-wdt"
+				  "marvell,armada-375-wdt"
+				  "marvell,armada-380-wdt":
+- clocks : Must contain an entry for each entry in clock-names.
+- clock-names : Must include the following entries:
+  "nbclk" (L2/coherency fabric clock),
+  "fixed" (Reference 25 MHz fixed-clock).
+
 Optional properties:
 
 - interrupts	: Contains the IRQ for watchdog expiration

@@ -30,4 +42,5 @@ Example:
 		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";
+		clocks = <&gate_clk 7>;
 	};

drivers/watchdog/orion_wdt.c +41 -1

index 00d0741..8cb1ff3 100644
@@ -114,6 +114,46 @@ static int armada370_wdt_clock_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int armada375_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (!IS_ERR(dev->clk)) {
+		ret = clk_prepare_enable(dev->clk);
+		if (ret) {
+			clk_put(dev->clk);
+			return ret;
+		}
+
+		atomic_io_modify(dev->reg + TIMER_CTRL,
+				WDT_AXP_FIXED_ENABLE_BIT,
+				WDT_AXP_FIXED_ENABLE_BIT);
+		dev->clk_rate = clk_get_rate(dev->clk);
+
+		return 0;
+	}
+
+	/* Mandatory fallback for proper devicetree backward compatibility */
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		clk_put(dev->clk);
+		return ret;
+	}
+
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+
+	return 0;
+}
+
 static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 				   struct orion_watchdog *dev)
 {

@@ -394,7 +434,7 @@ static const struct orion_watchdog_data armada375_data = {
 	.rstout_mask_bit = BIT(10),
 	.wdt_enable_bit = BIT(8),
 	.wdt_counter_offset = 0x34,
-	.clock_init = armada370_wdt_clock_init,
+	.clock_init = armada375_wdt_clock_init,
 	.enabled = armada375_enabled,
 	.start = armada375_start,
 	.stop = armada375_stop,

Signature · 1 line

2.1.0

Hi Daniel, Wim:

Seems we are ready to merge these, unless some objection raises. Do you think you can take the patches through your trees?

Changes from v2:

* Fix messed up empty lines, as suggested by Thomas Petazzoni.

Changes from v1:

* Make the A375 timer and watchdog devicetree binding match the hardware and require the two available clocks: core and reference clock.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to be used on the timer and watchdog drivers. It is similar to the one present in Armada XP SoC.

Given we initially had access to only a very early SoC revision (A375 Z1) and due to a hardware issue, the timer and watchdog support was originally submitted to use the core clock.

Now that the A0 SoC revision is out, we can fix this and use the reference clock. The reason for this change is that the core clock is subject to the SSCG (Spread Spectrum Clock Generator), so boards where SSCG is enabled exhibit a very large timer drift.

Notice that this change will produce unpleseant results on A375 Z1, given the reference 25 MHz clock doesn't work properly on that SoC revision.

But since the Z1 revision is not supported by the recently submitted A375 network driver (mvpp2), and therefore nobody is expected to run mainline (if there are users at all of this early revision), the support will be dropped entirely in a near future.

To prevent any compatibility issues when booting with an older devicetree, this series provides proper fall backs in each case.

The series applies on v3.18-rc2.

Ezequiel Garcia (3):
clocksource: armada-370-xp: Add missing clock enable
watchdog: orion: Use the reference clock on Armada 375 SoC
clocksource: armada-370-xp: Use the reference clock on A375 SoC

  .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
  .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
  drivers/clocksource/time-armada-370-xp.c           | 30 ++++++++++++++++
  drivers/watchdog/orion_wdt.c                       | 42 +++++++++++++++++++++-
  4 files changed, 90 insertions(+), 4 deletions(-)

Quoted 5 lines

Hi Daniel, Wim:

Seems we are ready to merge these, unless some objection raises. Do you think
you can take the patches through your trees?

Hi Wim,

I can take through my tree the entire patchset if you want but I need your blessing for the watchdog part.

Thanks
-- Daniel

Quoted 41 lines

Changes from v2:

* Fix messed up empty lines, as suggested by Thomas Petazzoni.

Changes from v1:

* Make the A375 timer and watchdog devicetree binding match the hardware and require the two available clocks: core and reference clock.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to be used on the timer and watchdog drivers. It is similar to the one present in Armada XP SoC.

Given we initially had access to only a very early SoC revision (A375 Z1) and due to a hardware issue, the timer and watchdog support was originally submitted to use the core clock.

Now that the A0 SoC revision is out, we can fix this and use the reference clock. The reason for this change is that the core clock is subject to the SSCG (Spread Spectrum Clock Generator), so boards where SSCG is enabled exhibit a very large timer drift.

Notice that this change will produce unpleseant results on A375 Z1, given the
reference 25 MHz clock doesn't work properly on that SoC revision.

But since the Z1 revision is not supported by the recently submitted A375 network driver (mvpp2), and therefore nobody is expected to run mainline (if there are users at all of this early revision), the support will be dropped entirely in a near future.

To prevent any compatibility issues when booting with an older devicetree, this series provides proper fall backs in each case.

The series applies on v3.18-rc2.

Ezequiel Garcia (3):
clocksource: armada-370-xp: Add missing clock enable
watchdog: orion: Use the reference clock on Armada 375 SoC
clocksource: armada-370-xp: Use the reference clock on A375 SoC

Quoted 7 lines

 .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
 .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
 drivers/clocksource/time-armada-370-xp.c           | 30 ++++++++++++++++
 drivers/watchdog/orion_wdt.c                       | 42 
 +++++++++++++++++++++-
 4 files changed, 90 insertions(+), 4 deletions(-)

Signature · 5 lines

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Hi Daniel,

I'll review before the weekend and will let you know something. But everything through one tree is fine by me.

Kind regards,
Wim.

Quoted 72 lines · Ezequiel Garcia

Quoted 5 lines

Hi Daniel, Wim:

Seems we are ready to merge these, unless some objection raises. Do you think
you can take the patches through your trees?

Hi Wim,

I can take through my tree the entire patchset if you want but I need your blessing for the watchdog part.

Thanks
-- Daniel

Quoted 41 lines

Changes from v2:

* Fix messed up empty lines, as suggested by Thomas Petazzoni.

Changes from v1:

* Make the A375 timer and watchdog devicetree binding match the hardware and require the two available clocks: core and reference clock.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to be used on the timer and watchdog drivers. It is similar to the one present in Armada XP SoC.

Given we initially had access to only a very early SoC revision (A375 Z1) and due to a hardware issue, the timer and watchdog support was originally submitted to use the core clock.

Now that the A0 SoC revision is out, we can fix this and use the reference clock. The reason for this change is that the core clock is subject to the SSCG (Spread Spectrum Clock Generator), so boards where SSCG is enabled exhibit a very large timer drift.

Notice that this change will produce unpleseant results on A375 Z1, given the
reference 25 MHz clock doesn't work properly on that SoC revision.

But since the Z1 revision is not supported by the recently submitted A375 network driver (mvpp2), and therefore nobody is expected to run mainline (if there are users at all of this early revision), the support will be dropped entirely in a near future.

To prevent any compatibility issues when booting with an older devicetree, this series provides proper fall backs in each case.

The series applies on v3.18-rc2.

Ezequiel Garcia (3):
clocksource: armada-370-xp: Add missing clock enable
watchdog: orion: Use the reference clock on Armada 375 SoC
clocksource: armada-370-xp: Use the reference clock on A375 SoC

Quoted 7 lines

  .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
  .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
  drivers/clocksource/time-armada-370-xp.c           | 30 ++++++++++++++++
  drivers/watchdog/orion_wdt.c                       | 42
  +++++++++++++++++++++-
  4 files changed, 90 insertions(+), 4 deletions(-)

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Quoted 4 lines

Hi Daniel,

I'll review before the weekend and will let you know something. But everything through one tree is fine by me.

Hi Wim,

did you have time to review this patchset ? I would like to send my PR for tglx as soon as possible.

Thanks
-- Daniel

Quoted 75 lines

Kind regards,
Wim.

Quoted 71 lines · Ezequiel Garcia

Quoted 5 lines

Hi Daniel, Wim:

Seems we are ready to merge these, unless some objection raises. Do you think
you can take the patches through your trees?

Hi Wim,

I can take through my tree the entire patchset if you want but I need your blessing for the watchdog part.

Thanks
-- Daniel

Quoted 54 lines

Changes from v2:

* Fix messed up empty lines, as suggested by Thomas Petazzoni.

Changes from v1:

* Make the A375 timer and watchdog devicetree binding match the hardware
and require the two available clocks: core and reference clock.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to be used on the timer and watchdog drivers. It is similar to the one present in Armada XP SoC.

Given we initially had access to only a very early SoC revision (A375 Z1) and due to a hardware issue, the timer and watchdog support was originally
submitted to use the core clock.

Now that the A0 SoC revision is out, we can fix this and use the reference
clock. The reason for this change is that the core clock is subject to the
SSCG (Spread Spectrum Clock Generator), so boards where SSCG is enabled exhibit a very large timer drift.

Notice that this change will produce unpleseant results on A375 Z1, given the
reference 25 MHz clock doesn't work properly on that SoC revision.

But since the Z1 revision is not supported by the recently submitted A375 network driver (mvpp2), and therefore nobody is expected to run mainline (if there are users at all of this early revision), the support will be dropped entirely in a near future.

To prevent any compatibility issues when booting with an older devicetree,
this series provides proper fall backs in each case.

The series applies on v3.18-rc2.

Ezequiel Garcia (3):
clocksource: armada-370-xp: Add missing clock enable
watchdog: orion: Use the reference clock on Armada 375 SoC
clocksource: armada-370-xp: Use the reference clock on A375 SoC

 .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
 .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
 drivers/clocksource/time-armada-370-xp.c           | 30 
 ++++++++++++++++
 drivers/watchdog/orion_wdt.c                       | 42
 +++++++++++++++++++++-
 4 files changed, 90 insertions(+), 4 deletions(-)

Quoted 8 lines

Quoted 8 lines

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Signature · 5 lines

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Hi Daniel,

I'm OK with the patchset. You can add my Acked-by.

Kind regards,
Wim.

Quoted 106 lines · Wim Van Sebroeck

Quoted 4 lines

Hi Daniel,

I'll review before the weekend and will let you know something. But everything through one tree is fine by me.

Hi Wim,

did you have time to review this patchset ? I would like to send my PR for tglx as soon as possible.

Thanks
-- Daniel

Quoted 83 lines

Kind regards,
Wim.

Quoted 79 lines · Ezequiel Garcia

Quoted 5 lines

Hi Daniel, Wim:

Seems we are ready to merge these, unless some objection raises. Do you think
you can take the patches through your trees?

Hi Wim,

I can take through my tree the entire patchset if you want but I need your blessing for the watchdog part.

Thanks
-- Daniel

Quoted 46 lines

Changes from v2:

* Fix messed up empty lines, as suggested by Thomas Petazzoni.

Changes from v1:

* Make the A375 timer and watchdog devicetree binding match the hardware
and require the two available clocks: core and reference clock.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to be used on the timer and watchdog drivers. It is similar to the one present in Armada XP SoC.

Given we initially had access to only a very early SoC revision (A375 Z1) and due to a hardware issue, the timer and watchdog support was originally
submitted to use the core clock.

Now that the A0 SoC revision is out, we can fix this and use the reference
clock. The reason for this change is that the core clock is subject to the
SSCG (Spread Spectrum Clock Generator), so boards where SSCG is enabled exhibit a very large timer drift.

Notice that this change will produce unpleseant results on A375 Z1, given the
reference 25 MHz clock doesn't work properly on that SoC revision.

But since the Z1 revision is not supported by the recently submitted A375 network driver (mvpp2), and therefore nobody is expected to run mainline (if there are users at all of this early revision), the support will be dropped entirely in a near future.

To prevent any compatibility issues when booting with an older devicetree,
this series provides proper fall backs in each case.

The series applies on v3.18-rc2.

Ezequiel Garcia (3):
clocksource: armada-370-xp: Add missing clock enable
watchdog: orion: Use the reference clock on Armada 375 SoC
clocksource: armada-370-xp: Use the reference clock on A375 SoC

Quoted 8 lines

  .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
  .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
  drivers/clocksource/time-armada-370-xp.c           | 30
  ++++++++++++++++
  drivers/watchdog/orion_wdt.c                       | 42
  +++++++++++++++++++++-
  4 files changed, 90 insertions(+), 4 deletions(-)

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog