clk: mvebu: Do not gate ge0/1 and runit clocks on Kirkwood

Commits f479db "ARM: Kirkwood: Ensure runit clock always ticks." and 128789 "ARM: Kirkwood: Fix clk problems modular ethernet driver" ensured that the ge and runit clocks always tick on Kirkwood. This prevents the device from locking up and from forgetting the MAC addresses which are usually set by the boot loader.

When moving the clock gating control to this driver for DT devices, these changes were disabled. Ensure that the respective clocks have the CLK_IGNORE_UNUSED flag set.

Signed-off-by Simon Baatz <[email protected]>

---

Hi,

kernel 3.8-rc5 will hang on kirkwood DT if the Ethernet driver is built as a module or when no driver claiming the runit clock is built in. (Usually, at least the serial driver is built in, but it won't request the clock if "clock-frequency" is given in DT.)

In the past, we fixed this by keeping the clocks ticking (which probably is not be the nicest solution for the ge clocks).

Simon,

Quoted 11 lines · Simon Baatz

Commits f479db "ARM: Kirkwood: Ensure runit clock always ticks." and 128789 "ARM: Kirkwood: Fix clk problems modular ethernet driver" ensured that the ge and runit clocks always tick on Kirkwood. This prevents the device from locking up and from forgetting the MAC addresses which are usually set by the boot loader.

When moving the clock gating control to this driver for DT devices, these changes were disabled. Ensure that the respective clocks have the CLK_IGNORE_UNUSED flag set.

Quoted 2 lines

In the past, we fixed this by keeping the clocks ticking (which probably is not be the nicest solution for the ge clocks).

I have to admit, I'm not really keen on this. Most of these boards only have one ethernet port, so at least one port would be energized unnecessarily.

Another facet of this problem is the keymile board. It has to enable the clocks for sata in order to boot. (ref: board-km_kirkwood.c).

Perhaps there is some way we could declare certain gate clocks to be non-gateable in the dts? runit comes to mind, sata for keymile, and the relevant ge[01] per board. After all, it is a characteristic of the board. ;-)

eg in kirkwood-km_kirkwood.dts:

gate_clk: clock-gating-control at 2011c {
	/*
	 * need both sata clks enabled in order to boot
	 * even though we have no sata
	 */
	ungateable = <14 15>;
};

and in any other board:

gate_clk: clock-gating-control at 2011c {
	/* don't lose eth0 mac address */
	ungateable = <0>;
};

where in kirkwood.dtsi we had:

gate_clk: clock-gating-control at 2011c {
	compatible = "marvell,kirkwood-gating-clock";
	reg = <0x2011c 0x4>;
	clocks = <&core_clk 0>;
	#clock-cells = <1>;
	ungateable = <7>; /* never gate runit */
};

or, s/ungateable/ignore_unused/g

Quoted 2 lines · Jason Cooper

Another facet of this problem is the keymile board. It has to enable the clocks for sata in order to boot. (ref: board-km_kirkwood.c).

I think we should do the following for 3.8:

- Get the clocks by device name in kirkwood_ge0x_init()
- Only set CLK_IGNORE_UNUSED for "runit" in clk-gating-ctrl.c. (I can do this by simply adding another case to the existing "ddr" exception, which makes the patch much less intrusive)

Quoted 6 lines

and in any other board:

gate_clk: clock-gating-control at 2011c {
	/* don't lose eth0 mac address */
	ungateable =<0>;
};

Quoted 7 lines

I think we should do the following for 3.8:

- Get the clocks by device name in kirkwood_ge0x_init()
- Only set CLK_IGNORE_UNUSED for "runit" in clk-gating-ctrl.c. (I can do this by simply adding another case to the existing "ddr" exception, which makes the patch much less intrusive)

I agree that we should have .flags for clk-gating-ctrl but that can live without any DT properties as we already have SoC specific structs there (and Keymile's kirkwood doesn't hang on clk gating control register accesses)

Quoted 7 lines

Quoted 7 lines

and in any other board:

gate_clk: clock-gating-control at 2011c {
	/* don't lose eth0 mac address */
	ungateable =<0>;
};

If there is any hang issue with gated ge clk on kirkwood, the solution is not to just disable the clock gate but make any code that accesses ge registers prepare the clock.

Quoted 8 lines · Sebastian Hesselbarth

Quoted 7 lines

I think we should do the following for 3.8:

- Get the clocks by device name in kirkwood_ge0x_init()
- Only set CLK_IGNORE_UNUSED for "runit" in clk-gating-ctrl.c. (I can do this by simply adding another case to the existing "ddr" exception, which makes the patch much less intrusive)

Quoted 4 lines

I agree that we should have .flags for clk-gating-ctrl but that can live without any DT properties as we already have SoC specific structs there (and Keymile's kirkwood doesn't hang on clk gating control register accesses)

Oh, I just posted the patch with the simple if, not the .flags. As said, the .flags create much noise. But I don't mind changing that back or posting the .flags change separately for 3.9.

I think for "runit" it makes sense to keep it that way (i.e. not in DT properties). But the ge clocks are really depending on the number of interfaces on the particular board. Having a way not to keep them running for all boards just not to forget the MAC addresses for those boards who use them would be nice.

Quoted 3 lines

If there is any hang issue with gated ge clk on kirkwood, the solution is not to just disable the clock gate but make any code that accesses ge registers prepare the clock.

Indeed, there are two issue here. The ge interface forgets its MAC address (set by the boot loader) when the clock is gated.

In the ge driver, accesses are in the wrong order or there is a missing delay between the enabling and the hardware access. Andrew digged into this at the time. But even if that would work, the interface would not be usable without a valid MAC address.

Except for loosing the MAC address, I disagree. From a "clock gating" point-of-view all kirkwoods (except that crippled one on keymile board) are the same. Even if there is no/one/two ethernet _jacks_ on a specific board you still want to disable all _unused_ ethernet modules inside the SoC.

We should find a way to retain the MAC address and in the meantime we can add a "always prepare ge clocks" workaround in kirkwood_clk_legacy_init().

Quoted 6 lines

Except for loosing the MAC address, I disagree. From a "clock gating" point-of-view all kirkwoods (except that crippled one on keymile board) are the same. Even if there is no/one/two ethernet _jacks_ on a specific board you still want to disable all _unused_ ethernet modules inside the SoC.

Quoted 3 lines

We should find a way to retain the MAC address and in the meantime we can add a "always prepare ge clocks" workaround in kirkwood_clk_legacy_init().

I've been thinking the same, store the MAC address before turning the clock off and restore it when enabling the clock. We have a need for special clocks on kirkwood anyway for turning off the sata and pcie PHYs. So adding special clocks for ethernet is not too big a deal.

I've been thinking the same, store the MAC address before turning the clock off and restore it when enabling the clock. We have a need for special clocks on kirkwood anyway for turning off the sata and pcie PHYs. So adding special clocks for ethernet is not too big a deal.

Dear Andrew Lunn,

Quoted 4 lines · Andrew Lunn

I've been thinking the same, store the MAC address before turning the clock off and restore it when enabling the clock. We have a need for special clocks on kirkwood anyway for turning off the sata and pcie PHYs. So adding special clocks for ethernet is not too big a deal.

At least on Armada 370/XP, U-Boot passes the MAC address through a special ATAG. Willy Tarreau has written some code to read this special ATAG and feed it into the Device Tree, so that we get the MAC address as set by U-Boot. I unfortunately haven't had the time to look at Willy's code and push it, but it does seem like an interesting solution.