Make Armada 375 use the reference clock when possible

This commit makes sure the timer clock is prepared and enabled before retrieving its rate.

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

---
 drivers/clocksource/time-armada-370-xp.c | 2 ++
 1 file changed, 2 insertions(+)

This commit makes sure the timer clock is prepared and enabled before retrieving its rate.

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

---
 drivers/clocksource/time-armada-370-xp.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/clocksource/time-armada-370-xp.c +2 -0

index 0451e62..d8555f9 100644
@@ -293,6 +293,7 @@ static void __init armada_xp_timer_init(struct device_node *np)
 
 	/* The 25Mhz fixed clock is mandatory, and must always be available */
 	BUG_ON(IS_ERR(clk));
+	clk_prepare_enable(clk);
 	timer_clk = clk_get_rate(clk);
 
 	armada_370_xp_timer_common_init(np);

@@ -305,6 +306,7 @@ static void __init armada_370_timer_init(struct device_node *np)
 	struct clk *clk = of_clk_get(np, 0);
 
 	BUG_ON(IS_ERR(clk));
+	clk_prepare_enable(clk);
 	timer_clk = clk_get_rate(clk) / TIMER_DIVIDER;
 	timer25Mhz = false;
 

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

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

---
 drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

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

---
 drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

drivers/watchdog/orion_wdt.c +39 -1

index 00d0741..6452fa2 100644
@@ -114,6 +114,44 @@ 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 +432,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 · 7 lines

2.1.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 4 lines · Ezequiel Garcia

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

Hi Ezequiel

There is now quite a lot of code in orion_wdt.c which is not relevant to Orion5x and Kirkwood. Would it be possible to put some of it inside a #ifdef MACH_MVEBU_V7?

Quoted 6 lines · Andrew Lunn

Quoted 4 lines · Ezequiel Garcia

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

Quoted 6 lines

Hi Ezequiel

There is now quite a lot of code in orion_wdt.c which is not relevant to Orion5x and Kirkwood. Would it be possible to put some of it inside a #ifdef MACH_MVEBU_V7?

Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. Just did a quick hack enclosing all the armada-xxx stuff around #if 0 and here's the result:

$ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o
add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68)
function                                     old     new   delta
orion_wdt_probe                              732     744     +12
orion_wdt_get_timeleft                        44      40      -4
orion_enabled                                 68      60      -8
orion_start                                  120      88     -32
orion_wdt_ping                                80      44     -36

To be honest, I don't think it's worth the ugliness.

Quoted 6 lines · Andrew Lunn

Quoted 4 lines · Ezequiel Garcia

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

Quoted 6 lines

Hi Ezequiel

There is now quite a lot of code in orion_wdt.c which is not relevant to Orion5x and Kirkwood. Would it be possible to put some of it inside a #ifdef MACH_MVEBU_V7?

Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. Just did a quick hack enclosing all the armada-xxx stuff around #if 0 and here's the result:

$ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o
add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68)
function                                     old     new   delta
orion_wdt_probe                              732     744     +12
orion_wdt_get_timeleft                        44      40      -4
orion_enabled                                 68      60      -8
orion_start                                  120      88     -32
orion_wdt_ping                                80      44     -36

Quoted 18 lines · Ezequiel Garcia

Quoted 6 lines · Andrew Lunn

Quoted 4 lines · Ezequiel Garcia

The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes 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.

Quoted 6 lines

Hi Ezequiel

There is now quite a lot of code in orion_wdt.c which is not relevant to Orion5x and Kirkwood. Would it be possible to put some of it inside a #ifdef MACH_MVEBU_V7?

Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. Just did a quick hack enclosing all the armada-xxx stuff around #if 0 and here's the result:

Quoted 8 lines

$ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o
add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68)
function                                     old     new   delta
orion_wdt_probe                              732     744     +12
orion_wdt_get_timeleft                        44      40      -4
orion_enabled                                 68      60      -8
orion_start                                  120      88     -32
orion_wdt_ping                                80      44     -36

Hi Ezequiel

I did a similar test:

size drivers/watchdog/orion_wdt.o-*
text data bss dec hex filename

   4428	    100       1    4529    11b1	drivers/watchdog/orion_wdt.o-full-fat
   2324	    100	      1    2425	    979	drivers/watchdog/orion_wdt.o-skimmed

So for v5 kirkwood/orion5x, there is about 90% overhead from the v7 code, in the text section. This is for -rc1 code, before adding any more v7 code in this patchset.

I don't think the #ifdef's look that scary.

drivers/clocksource/time-armada-370-xp.c +12 -0

+static void __init armada_375_timer_init(struct device_node *np)
+{
+	struct clk *clk;
+
+	clk = of_clk_get_by_name(np, "fixed");
+	if (!IS_ERR(clk)) {
+		clk_prepare_enable(clk);
+		timer_clk = clk_get_rate(clk);
+	} else {
+
+		/*
+		 * This fallback is required in order to retain proper

Dear Ezequiel Garcia,

Quoted 12 lines · Ezequiel Garcia

drivers/clocksource/time-armada-370-xp.c +12 -0

+static void __init armada_375_timer_init(struct device_node *np)
+{
+	struct clk *clk;
+
+	clk = of_clk_get_by_name(np, "fixed");
+	if (!IS_ERR(clk)) {
+		clk_prepare_enable(clk);
+		timer_clk = clk_get_rate(clk);
+	} else {
+
+		/*
+		 * This fallback is required in order to retain proper

drivers/clocksource/time-armada-370-xp.c static void __init armada_xp_timer_init(struct device_node *np) · line 325

Quoted 10 lines

drivers/clocksource/time-armada-370-xp.c +10 -0

+		 * devicetree backwards compatibility.
+		 */
+		clk = of_clk_get(np, 0);
+
+		/* Must have at least a clock */
+		BUG_ON(IS_ERR(clk));
+		clk_prepare_enable(clk);
+		timer_clk = clk_get_rate(clk) / TIMER_DIVIDER;
+		timer25Mhz = false;
+	}

Maybe we could re-use a bit more existing code, don't know if it makes things clearer though:

	clk = of_clk_get_by_name(np, "fixed");
	if (!IS_ERR(clk))
		armada_xp_timer_init(np);
	else
		armada_370_timer_init(np);

Dear Ezequiel Garcia,

arch/arm/boot/dts/armada-375.dtsi line 376

Quoted 3 lines · Ezequiel Garcia

arch/arm/boot/dts/armada-375.dtsi +2 -1

-				clocks = <&coreclk 0>;
+				clocks = <&refclk>;
+				clock-names = "fixed";

Why not do like we do on Armada XP, and actually represent the hardware, by showing that it takes two clocks as input:

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to use 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 Z0) 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, so boards where SSCG is enabled exhibit a very large timer drift.

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to use 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 Z0)

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, so boards where SSCG is enabled exhibit a very large timer drift.

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

You don't clearly state whether your patch series keep compatibility with the 375 Z1 or not. And in fact, it doesn't keep compatibility with Z1.

I'm fine with that, but then it means we should officially declare the Z1 support in mainline as dead, and get rid of the workarounds that applied only to 375 Z1.

How bad is it if you run these patches on a Z1? Is it worth adding a revision check and calling BUG()?

Dear Andrew Lunn,

Quoted 2 lines · Andrew Lunn

How bad is it if you run these patches on a Z1? Is it worth adding a revision check and calling BUG()?

To be tested, I don't have the answer off-hand.

Quoted 3 lines

You don't clearly state whether your patch series keep compatibility with the 375 Z1 or not. And in fact, it doesn't keep compatibility with Z1.

Hi Thomas

That answers my question. Thanks

Quoted 3 lines

I'm fine with that, but then it means we should officially declare the Z1 support in mainline as dead, and get rid of the workarounds that applied only to 375 Z1.

A big ACK.

How bad is it if you run these patches on a Z1? Is it worth adding a revision check and calling BUG()?

Dear Ezequiel Garcia,

Quoted 5 lines · Ezequiel Garcia

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to use 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 Z0)

Actually: s/Z0/Z1/.

Quoted 7 lines

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, so boards where SSCG is enabled exhibit a very large timer drift.

Quoted 2 lines

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

You don't clearly state whether your patch series keep compatibility with the 375 Z1 or not. And in fact, it doesn't keep compatibility with Z1. I'm fine with that, but then it means we should officially declare the Z1 support in mainline as dead, and get rid of the workarounds that applied only to 375 Z1.

Quoted 5 lines

You don't clearly state whether your patch series keep compatibility with the 375 Z1 or not. And in fact, it doesn't keep compatibility with Z1. I'm fine with that, but then it means we should officially declare the Z1 support in mainline as dead, and get rid of the workarounds that applied only to 375 Z1.

How many people have Z1 boards? And is someone actually using one for something? Do we have any other reason to support Z1?

FWIW, a v3.18-rc1 kernel built with mvebu_v7_defconfig, silently stalls in the middle of the boot on Z1. Had to remove lots of compile time options to make it boot.

Before I start digging into this, maybe we can discuss your suggestion to drop it. If nobody is booting this often enough, maybe nobody cares about this, and it makes sense to drop the support?

Dear Ezequiel Garcia,

Quoted 9 lines · Ezequiel Garcia

Quoted 5 lines

You don't clearly state whether your patch series keep compatibility with the 375 Z1 or not. And in fact, it doesn't keep compatibility with Z1. I'm fine with that, but then it means we should officially declare the Z1 support in mainline as dead, and get rid of the workarounds that applied only to 375 Z1.

How many people have Z1 boards? And is someone actually using one for something? Do we have any other reason to support Z1?

Quoted 7 lines

FWIW, a v3.18-rc1 kernel built with mvebu_v7_defconfig, silently stalls in the middle of the boot on Z1. Had to remove lots of compile time options to make it boot.

Before I start digging into this, maybe we can discuss your suggestion to drop it. If nobody is booting this often enough, maybe nobody cares about this, and it makes sense to drop the support?

Marvell has always said they are not interested in having Z1 supported in mainline, except as a first step to start getting the 375 support in mainline. I have been waiting for everyone of us to have access to 375 A0 platforms, which is now the case. So I believe we can probably get rid of the 375 Z1 support entirely.

It saddens me to know that a shiny development board in the office will no longer be useful for anything else but its own physical beauty, but I believe that's a normal retirement strategy for a very early development platform :-)

Quoted 11 lines

Marvell has always said they are not interested in having Z1 supported in mainline, except as a first step to start getting the 375 support in mainline. I have been waiting for everyone of us to have access to 375 A0 platforms, which is now the case. So I believe we can probably get rid of the 375 Z1 support entirely.

It saddens me to know that a shiny development board in the office will no longer be useful for anything else but its own physical beauty, but I believe that's a normal retirement strategy for a very early development platform :-)

I'm afraid I have news for you. Even now, the board is almost unusable, as the mvpp2 network driver only supports A0 (Z1 support was never even planned).

So you have a NAS development board without network... sounds pretty useless to me :)

This series adds support for the 25 MHz reference clock available on Armada 375 SoC to use 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 Z0) 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, so boards where SSCG is enabled exhibit a very large timer drift.

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-rc1. As usual, any feedback is well received!

Ezequiel Garcia (4):
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
ARM: dts: Enable the reference clock for timer and watchdog on Armada

    375 SoC

 .../bindings/timer/marvell,armada-370-xp-timer.txt |  9 +++--
 arch/arm/boot/dts/armada-375.dtsi                  | 12 +++++--
 drivers/clocksource/time-armada-370-xp.c           | 30 ++++++++++++++++
 drivers/watchdog/orion_wdt.c                       | 40 +++++++++++++++++++++-
 4 files changed, 86 insertions(+), 5 deletions(-)

Quoted 3 lines · Ezequiel Garcia

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

I agree with Thomas P. comments: could you see if it was possible to reuse the same function and dt bindings that the ones used for Armada XP ?
I am not aware of any difference between Armada XP and Armada 375 for this IP.

Dear Gregory CLEMENT,

Quoted 5 lines · Gregory CLEMENT

Quoted 3 lines · Ezequiel Garcia

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

Quoted 3 lines

I agree with Thomas P. comments: could you see if it was possible to reuse the same function and dt bindings that the ones used for Armada XP ?
I am not aware of any difference between Armada XP and Armada 375 for this IP.

Well, there's one difference: on Armada XP we don't need to support the "old" Device Tree, which didn't had the fixed 25 Mhz clock input described.

So either we decide that it was a mistake due to an early version of the 375 and we skip backward DT compatibility. Or we need in some way a different logic than Armada XP, because Armada XP doesn't have this backward compatibility requirement.