Do not gate ge0/1 and runit clocks on Kirkwood

Commit 1611f87 (ARM: Kirkwood: switch to DT clock providers) broke the functions to initialize the ethernet interfaces (kirkwood_ge00_init() and kirkwood_ge01_init()). In the DT case, the functions could not enable the correct clocks.

Fix this by looking up the clocks through the device name.

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

---
  arch/arm/mach-kirkwood/common.c |   13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)

arch/arm/mach-kirkwood/common.c +1 -0

index bac21a5..2c97847 100644
...
  	tclk = clk_register_fixed_rate(NULL, "tclk", NULL,

@@ -288,12 +287,15 @@ void __init kirkwood_ehci_init(void)
   ****************************************************************************/
  void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data)
  {
+	struct clk *ge0;

Jason posted a patch set that makes mv643xx DT compatible. IMHO this patch is obsolete when we have DT support for mv643xx. kirkwood_ge00/01_init will not be called with DT support at all.

Quoted 3 lines · Sebastian Hesselbarth

Jason posted a patch set that makes mv643xx DT compatible. IMHO this patch is obsolete when we have DT support for mv643xx. kirkwood_ge00/01_init will not be called with DT support at all.

I know. This is to fix a regression in 3.8 (which currently does not boot with my "modularized" config).

Hi Sebastian,

Quoted 3 lines · Sebastian Hesselbarth

Jason posted a patch set that makes mv643xx DT compatible. IMHO this patch is obsolete when we have DT support for mv643xx. kirkwood_ge00/01_init will not be called with DT support at all.

I know. This is to fix a regression in 3.8 (which currently does not boot with my "modularized" config).

Quoted 9 lines · Simon Baatz

Hi Sebastian,

Quoted 3 lines · Sebastian Hesselbarth

Jason posted a patch set that makes mv643xx DT compatible. IMHO this patch is obsolete when we have DT support for mv643xx. kirkwood_ge00/01_init will not be called with DT support at all.

I know. This is to fix a regression in 3.8 (which currently does not boot with my "modularized" config).

Simon,

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

Quoted 4 lines

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

I believe Simon's issue is that the mv643xx_eth driver is not loaded at boot, it's clocks get gated, then when he loads the driver, there is no mac address. Is that correct Simon? I don't think unloading the driver after boot will trigger this regression.

Quoted 9 lines · Jason Cooper

Quoted 4 lines

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

I believe Simon's issue is that the mv643xx_eth driver is not loaded at boot, it's clocks get gated, then when he loads the driver, there is no mac address. Is that correct Simon? I don't think unloading the driver after boot will trigger this regression.

Loading and unloading the driver module hangs because of the missing clk_prepeare_enable in shared driver part. This should be fixed by the patch I sent you.

Dove and Kirkwood have the same gbe internally and I can boot into Dove without mv643xx_eth, load, unload, reload the module and it always finds its MAC address.

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Quoted 15 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Cooper

Quoted 4 lines

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

I believe Simon's issue is that the mv643xx_eth driver is not loaded at boot, it's clocks get gated, then when he loads the driver, there is no mac address. Is that correct Simon? I don't think unloading the driver after boot will trigger this regression.

Loading and unloading the driver module hangs because of the missing clk_prepeare_enable in shared driver part. This should be fixed by the patch I sent you.

Quoted 7 lines

Dove and Kirkwood have the same gbe internally and I can boot into Dove without mv643xx_eth, load, unload, reload the module and it always finds its MAC address.

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Ok, I just wanted to make sure we understood his problem correctly, and if possible, reproduce it.

Simon, can you give us some steps to reproduce this on our side so we can see exactly what's happening?

Hi Jason,

Quoted 27 lines · Jason Cooper

Quoted 15 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Cooper

Quoted 4 lines

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

I believe Simon's issue is that the mv643xx_eth driver is not loaded at boot, it's clocks get gated, then when he loads the driver, there is no mac address. Is that correct Simon? I don't think unloading the driver after boot will trigger this regression.

Loading and unloading the driver module hangs because of the missing clk_prepeare_enable in shared driver part. This should be fixed by the patch I sent you.

Quoted 7 lines

Dove and Kirkwood have the same gbe internally and I can boot into Dove without mv643xx_eth, load, unload, reload the module and it always finds its MAC address.

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Ok, I just wanted to make sure we understood his problem correctly, and if possible, reproduce it.

Quoted 2 lines

Simon, can you give us some steps to reproduce this on our side so we can see exactly what's happening?

Nothing special here. My config originally based on a Debian config for a Kirkwood kernel. Thus, amongst other drivers, mv643xx_eth is build as a module and is loaded from an initrd.
Because all relevant drivers are loaded as modules, I need my runit patch to boot at all.

Here are my findings with various patches: ("non-DT" means booting the IBNAS6210 with machine ID 1680 ???Marvell DB-88F6281-BP Development Board???, which works reasonably well)

3.8-rc5 + runit patch:

DT: Hangs at boot (when loading mv643xx_eth)
non-DT: Boots ok

3.8-rc5 + runit patch + ge00/ge01 patch:

DT: Boots ok
non-DT: Boots ok

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

3.8-rc5 + runit + using Sebastians patch + clks not ticking at module load:

DT: Boots, but no MAC
non-DT: Boots, but no MAC

hth,
Simon

Quoted 33 lines · Simon Baatz

Hi Jason,

Quoted 29 lines · Jason Cooper

Quoted 15 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Cooper

Quoted 4 lines

I _cannot_ confirm that gbe is loosing its MAC address on Dove. I will post a follow-up patch to Jason's cleanup patches that will also grab a clock for smi. With that patch insmod'ing/rmmod'ing mv643xx_eth does work just fine here on Dove.

I believe Simon's issue is that the mv643xx_eth driver is not loaded at boot, it's clocks get gated, then when he loads the driver, there is no mac address. Is that correct Simon? I don't think unloading the driver after boot will trigger this regression.

Loading and unloading the driver module hangs because of the missing clk_prepeare_enable in shared driver part. This should be fixed by the patch I sent you.

Quoted 7 lines

Dove and Kirkwood have the same gbe internally and I can boot into Dove without mv643xx_eth, load, unload, reload the module and it always finds its MAC address.

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Ok, I just wanted to make sure we understood his problem correctly, and if possible, reproduce it.

Simon, can you give us some steps to reproduce this on our side so we can see exactly what's happening?

Quoted 5 lines

Nothing special here. My config originally based on a Debian config for a Kirkwood kernel. Thus, amongst other drivers, mv643xx_eth is build as a module and is loaded from an initrd.
Because all relevant drivers are loaded as modules, I need my runit patch to boot at all.

Let's back up. If you config early printk and COMMON_CLK_DEBUG and a vanilla v3.8-rc5 with your current config, what happens?

Could you also please try kirkwood_defconfig and tell us if it boots?

Quoted 8 lines

Here are my findings with various patches: ("non-DT" means booting the IBNAS6210 with machine ID 1680 ???Marvell DB-88F6281-BP Development Board???, which works reasonably well)

3.8-rc5 + runit patch:

DT: Hangs at boot (when loading mv643xx_eth)
non-DT: Boots ok

Quoted 4 lines

3.8-rc5 + runit patch + ge00/ge01 patch:

DT: Boots ok
non-DT: Boots ok

Quoted 6 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

Quoted 6 lines

3.8-rc5 + runit + using Sebastians patch + clks not ticking at module load:

DT: Boots, but no MAC
non-DT: Boots, but no MAC

Quoted 7 lines · Jason Cooper

Quoted 5 lines · Simon Baatz

Nothing special here. My config originally based on a Debian config for a Kirkwood kernel. Thus, amongst other drivers, mv643xx_eth is build as a module and is loaded from an initrd.
Because all relevant drivers are loaded as modules, I need my runit patch to boot at all.

Quoted 2 lines

Let's back up. If you config early printk and COMMON_CLK_DEBUG and a vanilla v3.8-rc5 with your current config, what happens?

I thought it's clear what happens. "runit" is requested in the dts by serial, spi, watchdog, nand, i2c. Each of these are either not built or built as a module in my config, except serial.

of_serial.c does the following in of_platform_serial_setup():

	if (of_property_read_u32(np, "clock-frequency",&clk)) {

		/* Get clk rate through clk driver if present */
		info->clk = clk_get(&ofdev->dev, NULL);
		if (IS_ERR(info->clk)) {
			dev_warn(&ofdev->dev,
				"clk or clock-frequency not defined\n");
			return PTR_ERR(info->clk);
		}

		clk_prepare_enable(info->clk);
		clk = clk_get_rate(info->clk);
	}

and since "clock-frequency" is still given in the standard DTS for the IB-NAS 6210, it does not enable the clock.
Thus, no driver uses the clock, it gets disabled and the system locks up.
The system boots when removing "clock-frequency" from the DTS.

Which lead to two questions:
- Is it safe to remove "clock-frequency" now that we have the clocks in place?

- The behavior of of_serial.c looks strange to me, is this really the desired behavior?

For the "runit" clock, this means that any time you hit a configuration that does not keep the clock enabled, the system will lockup. (We have gone through this more than once now. Especially, this hits you hard when you try to support a new board and disable as much as possible in the config/dts for a start...).

Thus, we should keep it enabled even if it is unused, which is exactly the purpose of CLK_IGNORE_UNUSED if I understood this correctly.

Quoted 1 line

Could you also please try kirkwood_defconfig and tell us if it boots?

It is very easy to get my system to boot with a stock kernel. Just build one of the drivers I use directly into the kernel and it boots (up to the point where the gbe driver is loaded if built as a module, of course).

Quoted 8 lines

Quoted 8 lines

Here are my findings with various patches: ("non-DT" means booting the IBNAS6210 with machine ID 1680 ???Marvell DB-88F6281-BP Development Board???, which works reasonably well)

3.8-rc5 + runit patch:

DT: Hangs at boot (when loading mv643xx_eth)
non-DT: Boots ok

In the DT case ge0 and ge1 are NULL, and thus, kirkwood_ge0[01]_init() tries to enable NULL clocks, but not the gbe clocks. -> The clocks get disabled.

As described in Sebastian's patch (and also found by Andrew some time ago), this leads to a hanging system when loading the gbe driver.

Quoted 4 lines

Quoted 4 lines

3.8-rc5 + runit patch + ge00/ge01 patch:

DT: Boots ok
non-DT: Boots ok

Since the clocks are found now and kept enabled, DT behaves the same as non-DT.

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

This is the behavior originally found by Andrew. The clock patch eliminates the lockup, but the hardware forgets its MAC address.

Quoted 6 lines

Quoted 6 lines

3.8-rc5 + runit + using Sebastians patch + clks not ticking at module load:

DT: Boots, but no MAC
non-DT: Boots, but no MAC

I think non-DT and DT gated clocks behave differently, since the non-DT ones also enable/disable the PHY. I explicitly removed the clk_prepare_enable() from kirkwood_ge0[01]_init() in order to see if this makes a difference.

(Which reminds me, that we wanted to implement the PHY enabling in the driver.)

Apparently, it does not make a difference.

Quoted 11 lines · Simon Baatz

Quoted 9 lines · Jason Cooper

Quoted 5 lines · Simon Baatz

Nothing special here. My config originally based on a Debian config for a Kirkwood kernel. Thus, amongst other drivers, mv643xx_eth is build as a module and is loaded from an initrd.
Because all relevant drivers are loaded as modules, I need my runit patch to boot at all.

Let's back up. If you config early printk and COMMON_CLK_DEBUG and a vanilla v3.8-rc5 with your current config, what happens?

Quoted 3 lines

I thought it's clear what happens. "runit" is requested in the dts by serial, spi, watchdog, nand, i2c. Each of these are either not built or built as a module in my config, except serial.

Simon,

it is clear what happens but just to blindly disable clock gating will not help here. What you are suffering are several issues not only one.

Quoted 22 lines

of_serial.c does the following in of_platform_serial_setup():

	if (of_property_read_u32(np, "clock-frequency",&clk)) {

		/* Get clk rate through clk driver if present */
		info->clk = clk_get(&ofdev->dev, NULL);
		if (IS_ERR(info->clk)) {
			dev_warn(&ofdev->dev,
				"clk or clock-frequency not defined\n");
			return PTR_ERR(info->clk);
		}

		clk_prepare_enable(info->clk);
		clk = clk_get_rate(info->clk);
	}

and since "clock-frequency" is still given in the standard DTS for the IB-NAS 6210, it does not enable the clock.
Thus, no driver uses the clock, it gets disabled and the system locks up.
The system boots when removing "clock-frequency" from the DTS.

Quoted 3 lines

Which lead to two questions:
- Is it safe to remove "clock-frequency" now that we have the clocks in place?

Safe, as long as you replace it with the corresponding clocks=<&..> property.

Quoted 2 lines

- The behavior of of_serial.c looks strange to me, is this really the desired behavior?

I guess it is, rely on clock-frequency because a lot of boards still use it. There was no clocks property in of_serial when we started with kirkwood and dove. I think we can switch now to clocks property which will also remove all hard coded occurrences of tclk frequency.

Quoted 5 lines

For the "runit" clock, this means that any time you hit a configuration that does not keep the clock enabled, the system will lockup. (We have gone through this more than once now. Especially, this hits you hard when you try to support a new board and disable as much as possible in the config/dts for a start...).

Well, if there is no device/driver using runit it should be safe to disable it. If there is a driver using it, we need a clocks property for it. And as you already stated, serial is using it. And you want serial in every minimal kernel, don't you?

Quoted 3 lines

Thus, we should keep it enabled even if it is unused, which is exactly the purpose of CLK_IGNORE_UNUSED if I understood this correctly.

True, but only if it is that important that it should _never_ be gated. As long as it is 'only' used by peripheral devices, it should be safe to gate it.

Quoted 16 lines

Quoted 1 line

Could you also please try kirkwood_defconfig and tell us if it boots?

It is very easy to get my system to boot with a stock kernel. Just build one of the drivers I use directly into the kernel and it boots (up to the point where the gbe driver is loaded if built as a module, of course).

Quoted 8 lines

Quoted 8 lines

Here are my findings with various patches: ("non-DT" means booting the IBNAS6210 with machine ID 1680 ???Marvell DB-88F6281-BP Development Board???, which works reasonably well)

3.8-rc5 + runit patch:

DT: Hangs at boot (when loading mv643xx_eth)
non-DT: Boots ok

Quoted 6 lines

In the DT case ge0 and ge1 are NULL, and thus, kirkwood_ge0[01]_init() tries to enable NULL clocks, but not the gbe clocks. -> The clocks get disabled.

As described in Sebastian's patch (and also found by Andrew some time ago), this leads to a hanging system when loading the gbe driver.

If I got all your configs right, this DT case is with serial but no clocks property? All other runit "users" are _not_ built into the kernel? Maybe it is just a coincidence that it fails when loading eth but I guess it hangs because of runit getting gated while serial accessing it. When you replace serial's clock-frequency with clocks property to "runit" it shouldn't hang.

(Issue 1: gated runit clock hangs kernel due to serial access)

Quoted 22 lines

Quoted 4 lines

Quoted 4 lines

3.8-rc5 + runit patch + ge00/ge01 patch:

DT: Boots ok
non-DT: Boots ok

Since the clocks are found now and kept enabled, DT behaves the same as non-DT.

Quoted 6 lines

Quoted 6 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

This is the behavior originally found by Andrew. The clock patch eliminates the lockup, but the hardware forgets its MAC address.

Quoted 4 lines

Quoted 4 lines

3.8-rc5 + runit + using Sebastians patch + clks not ticking at module load:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

DT: Boots, but no MAC
non-DT: Boots, but no MAC

Ok, here my patch for smi clock enables gbe clock before accessing gbe registers.

(Issue 2: gated gbe clock hangs kernel due to smi access)

Quoted 4 lines

I think non-DT and DT gated clocks behave differently, since the non-DT ones also enable/disable the PHY. I explicitly removed the clk_prepare_enable() from kirkwood_ge0[01]_init() in order to see if this makes a difference.

True, DT gated clocks don't disable PHYs (and never will).

Quoted 4 lines

(Which reminds me, that we wanted to implement the PHY enabling in the driver.)

Apparently, it does not make a difference.

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

Quoted 1 line · Sebastian Hesselbarth

(Issue 1: gated runit clock hangs kernel due to serial access)

This can be fixed with a patch removing clock-frequency from all kirkwood boards (dove, orion, etc?) I'll gin this up and add it to the havoc ;-)

Quoted 1 line

(Issue 2: gated gbe clock hangs kernel due to smi access)

Here, I'm going to wait for Florian's mvmdio changes to settle out and then readdress this. My current understanding is that there will only be one DT node for this. iiuc, each board dts will have to say which gate clocks to consume to prevent the mvmdio node in the dtsi from consuming both unconditionally.

Quoted 3 lines

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

A patch to mv643xx_eth to pull this from DT should solve this.

Quoted 6 lines · Jason Cooper

Quoted 3 lines · Sebastian Hesselbarth

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

A patch to mv643xx_eth to pull this from DT should solve this.

Great.

Quoted 6 lines · Sebastian Hesselbarth

Quoted 3 lines

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

A patch to mv643xx_eth to pull this from DT should solve this.

Quoted 7 lines · Jason Cooper

Quoted 6 lines · Sebastian Hesselbarth

Quoted 3 lines

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

A patch to mv643xx_eth to pull this from DT should solve this.

Somewhere, Jason Gunthorpe shared his patch to do this. I'll poke around for it and try to get it merged in.

Quoted 6 lines · Jason Cooper

Quoted 6 lines · Jason Cooper

Quoted 5 lines · Sebastian Hesselbarth

Quoted 3 lines

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

Quoted 4 lines

Quoted 1 line

Quoted 1 line

A patch to mv643xx_eth to pull this from DT should solve this.

Somewhere, Jason Gunthorpe shared his patch to do this. I'll poke around for it and try to get it merged in.

Yes, you asked for the doc update and I haven't had a moment to get a tree setup for that..

Here are some words though:

- local-mac-address : Optional, the MAC address to assign to the device. If not specified then the MAC address in the HW registers is used, but the driver can not be made modular.

drivers/net/ethernet/marvell/mv643xx_eth.c +6 -0

index 7048d7c..2b2cfcb 100644
@@ -2891,6 +2891,8 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 	struct mv643xx_eth_private *mp;
 	struct net_device *dev;
 	struct resource *res;
+	const u8 *mac;
+	int len;
 	int err;
 
 	if (pdev->dev.of_node) {

@@ -2912,6 +2914,10 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 		else
 			pd->phy_addr = MV643XX_ETH_PHY_ADDR_DEFAULT;
 
+		mac = of_get_property(pdev->dev.of_node, "local-mac-address", &len);
+		if (mac && len == 6)
+		    memcpy(pd->mac_addr, mac, sizeof pd->mac_addr);
+
 		np = of_parse_phandle(pdev->dev.of_node, "mdio", 0);
 		if (np) {
 			pd->shared = of_find_device_by_node(np);

Signature · 1 line

1.7.5.4

Quoted 2 lines

Yes, you asked for the doc update and I haven't had a moment to get a tree setup for that..

Ahh, that's right. Sorry, wasn't trying to put pressure on you. Just trying to make a reminder to myself of what to look for when I get to around to it. (I have a separate IMAP folder for such things ;-) )

Quoted 5 lines

Here are some words though:

- local-mac-address : Optional, the MAC address to assign to the device. If not specified then the MAC address in the HW registers is used, but the driver can not be made modular.

I like it. If I haven't seen anything when I get to this, I'll add this to your patch. Please Ack if it's ok. I prefer not to change peoples patches without approval.

Quoted 11 lines · Simon Baatz

Quoted 9 lines · Jason Cooper

Quoted 5 lines · Simon Baatz

Nothing special here. My config originally based on a Debian config for a Kirkwood kernel. Thus, amongst other drivers, mv643xx_eth is build as a module and is loaded from an initrd.
Because all relevant drivers are loaded as modules, I need my runit patch to boot at all.

Let's back up. If you config early printk and COMMON_CLK_DEBUG and a vanilla v3.8-rc5 with your current config, what happens?

Quoted 3 lines

I thought it's clear what happens. "runit" is requested in the dts by serial, spi, watchdog, nand, i2c. Each of these are either not built or built as a module in my config, except serial.

Simon,

it is clear what happens but just to blindly disable clock gating will not help here. What you are suffering are several issues not only one.

Quoted 23 lines

of_serial.c does the following in of_platform_serial_setup():

	if (of_property_read_u32(np, "clock-frequency",&clk)) {

		/* Get clk rate through clk driver if present */
		info->clk = clk_get(&ofdev->dev, NULL);
		if (IS_ERR(info->clk)) {
			dev_warn(&ofdev->dev,
				"clk or clock-frequency not defined\n");
			return PTR_ERR(info->clk);
		}

		clk_prepare_enable(info->clk);
		clk = clk_get_rate(info->clk);
	}

and since "clock-frequency" is still given in the standard DTS for the IB-NAS 6210, it does not enable the clock.
Thus, no driver uses the clock, it gets disabled and the system locks up.
The system boots when removing "clock-frequency" from the DTS.

Quoted 3 lines

Which lead to two questions:
- Is it safe to remove "clock-frequency" now that we have the clocks in place?

Safe, as long as you replace it with the corresponding clocks=<&..> property.

Quoted 2 lines

- The behavior of of_serial.c looks strange to me, is this really the desired behavior?

I guess it is, rely on clock-frequency because a lot of boards still use it. There was no clocks property in of_serial when we started with kirkwood and dove. I think we can switch now to clocks property which will also remove all hard coded occurrences of tclk frequency.

Quoted 5 lines

For the "runit" clock, this means that any time you hit a configuration that does not keep the clock enabled, the system will lockup. (We have gone through this more than once now. Especially, this hits you hard when you try to support a new board and disable as much as possible in the config/dts for a start...).

Well, if there is no device/driver using runit it should be safe to disable it. If there is a driver using it, we need a clocks property for it. And as you already stated, serial is using it. And you want serial in every minimal kernel, don't you?

From f479db4:

Quoted 4 lines

Thus, we should keep it enabled even if it is unused, which is exactly the purpose of CLK_IGNORE_UNUSED if I understood this correctly.

True, but only if it is that important that it should _never_ be gated. As long as it is 'only' used by peripheral devices, it should be safe to gate it.

Quoted 16 lines

Quoted 1 line

Could you also please try kirkwood_defconfig and tell us if it boots?

It is very easy to get my system to boot with a stock kernel. Just build one of the drivers I use directly into the kernel and it boots (up to the point where the gbe driver is loaded if built as a module, of course).

Quoted 8 lines

Quoted 8 lines

Here are my findings with various patches: ("non-DT" means booting the IBNAS6210 with machine ID 1680 ???Marvell DB-88F6281-BP Development Board???, which works reasonably well)

3.8-rc5 + runit patch:

DT: Hangs at boot (when loading mv643xx_eth)
non-DT: Boots ok

Quoted 6 lines

In the DT case ge0 and ge1 are NULL, and thus, kirkwood_ge0[01]_init() tries to enable NULL clocks, but not the gbe clocks. -> The clocks get disabled.

As described in Sebastian's patch (and also found by Andrew some time ago), this leads to a hanging system when loading the gbe driver.

If I got all your configs right, this DT case is with serial but no clocks property? All other runit "users" are _not_ built into the kernel? Maybe it is just a coincidence that it fails when loading eth but I guess it hangs because of runit getting gated while serial accessing it. When you replace serial's clock-frequency with clocks property to "runit" it shouldn't hang.

(Issue 1: gated runit clock hangs kernel due to serial access)

Quoted 10 lines

Quoted 5 lines

Quoted 5 lines

3.8-rc5 + runit patch + ge00/ge01 patch:

DT: Boots ok
non-DT: Boots ok

Since the clocks are found now and kept enabled, DT behaves the same as non-DT.

Quoted 1 line

Quoted 1 line

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

This is the behavior originally found by Andrew. The clock patch eliminates the lockup, but the hardware forgets its MAC address.

Quoted 6 lines

Quoted 6 lines

3.8-rc5 + runit + using Sebastians patch + clks not ticking at module load:

DT: Boots, but no MAC
non-DT: Boots, but no MAC

Ok, here my patch for smi clock enables gbe clock before accessing gbe registers.

(Issue 2: gated gbe clock hangs kernel due to smi access)

Quoted 4 lines

I think non-DT and DT gated clocks behave differently, since the non-DT ones also enable/disable the PHY. I explicitly removed the clk_prepare_enable() from kirkwood_ge0[01]_init() in order to see if this makes a difference.

True, DT gated clocks don't disable PHYs (and never will).

Quoted 4 lines

(Which reminds me, that we wanted to implement the PHY enabling in the driver.)

Apparently, it does not make a difference.

Leaves Issue 3, gbe forgets about its MAC address when gated or powered down. That should be done with local-mac-address passed by DT enabled u-boot or any other (dirty) ATAG hack ;)

Quoted 4 lines · Sebastian Hesselbarth

Well, if there is no device/driver using runit it should be safe to disable it. If there is a driver using it, we need a clocks property for it. And as you already stated, serial is using it. And you want serial in every minimal kernel, don't you?

From f479db4:

Marvell engineers tell us:

It seems that many units use the RUNIT clock.
SPI, UART, NAND, TWSI, ...
So it's not possible to clock gate it.

Currently the SPI, NAND and TWSI driver will clk_prepaure_enable() this clk, but since we have no idea what ... is, and turning this clk off results in a hard lock, unconditionally enable runit.

Thus, if we know better than that, that's fine, but please don't tell me that this is "blindly" enabling clocks.

So, we have a statement from Marvell not to gate it, we know that it is needed for "...", can we please not gate it?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

This is the behavior originally found by Andrew. The clock patch eliminates the lockup, but the hardware forgets its MAC address.

Fine with all of that. But: I am talking about 3.8 all the time. We have three options for issues 2 and 3 from my point of view:

1. We do proper fixes in 3.8 for issues 2 and 3.

2. We fix this regression by not gating the clock in both the DT and the non-DT case, preferably by using the correct clocks in kirkwood_ge0[01]_init(). Additionally, Jason promises that all gets well with the DT aware driver in 3.9. ;-)

3. Do nothing. Simply accept that we broke modular Ethernet for DT because some code relies on two pointers being set. When we introduced a new code path for DT, we forgot about these pointers. Bad luck, the solution was not nice anyway and we will do proper fixes in 3.9.

As should be clear by now, I think we should go for option 2.

Quoted 14 lines · Simon Baatz

Quoted 4 lines · Sebastian Hesselbarth

Well, if there is no device/driver using runit it should be safe to disable it. If there is a driver using it, we need a clocks property for it. And as you already stated, serial is using it. And you want serial in every minimal kernel, don't you?

From f479db4:

Marvell engineers tell us:

It seems that many units use the RUNIT clock.
SPI, UART, NAND, TWSI, ...
So it's not possible to clock gate it.

Quoted 7 lines

Currently the SPI, NAND and TWSI driver will clk_prepaure_enable() this clk, but since we have no idea what ... is, and turning this clk off results in a hard lock, unconditionally enable runit.

Thus, if we know better than that, that's fine, but please don't tell me that this is "blindly" enabling clocks.

Hmm, should have seen that comment ealier. Based on your/Andrew's statement above using CLK_IGNORE_UNUSED on runit maybe is the best.

So I guess we take patch 2/2 and extend DT clock gating for .flags later?

Quoted 2 lines

So, we have a statement from Marvell not to gate it, we know that it is needed for "...", can we please not gate it?

Ack.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

3.8-rc5 + runit + Sebastians get smi clock patch (modified to use legacy clock names):

DT: Boots, but no MAC
non-DT: Boots ok

This is the behavior originally found by Andrew. The clock patch eliminates the lockup, but the hardware forgets its MAC address.

For the smi clock issue: DT is fixed by the smi clock patch, right? non-DT should be fixed in kirkwood_clk_init() with an orion_clkdev_add() alias for MV643XX_ETH_SHARED_NAME ".0" and ".1" respectively.

For the MAC address issue: non-DT doesn't need to be fixed, right? DT clock gates should be fixed in kirkwood_legacy_clk_init which will also save the clk_get_sys() call. We can easily remove it when mv643xx_eth catches the MAC address from either local-mac-address or ATAG.

Quoted 16 lines

Fine with all of that. But: I am talking about 3.8 all the time. We have three options for issues 2 and 3 from my point of view:

1. We do proper fixes in 3.8 for issues 2 and 3.

2. We fix this regression by not gating the clock in both the DT and the non-DT case, preferably by using the correct clocks in kirkwood_ge0[01]_init(). Additionally, Jason promises that all gets well with the DT aware driver in 3.9. ;-)

3. Do nothing. Simply accept that we broke modular Ethernet for DT because some code relies on two pointers being set. When we introduced a new code path for DT, we forgot about these pointers. Bad luck, the solution was not nice anyway and we will do proper fixes in 3.9.

Quoted 1 line

As should be clear by now, I think we should go for option 2.

Agreed, do you think all issues you are suffering will be solved with:

- [PATCH v2 2/2] clk: mvebu: Do not gate runit clock on Kirkwood (no lockup for minimal kernel configs)

- [PATCH] NET: mv643xx: get smi clock from device tree (no lockup for modular DT ethernet)

- Some patch that adds MV643XX_ETH_SHARED_NAME ".0" and ".1" clk aliases (no lockup for modular non-DT ethernet)

- Some patch that adds clk_prepare_enable to ge0/ge1 clocks to kirkwood_legacy_clk_init()
(retain MAC address for modular DT ethernet)

I'll prepare the latter patches and post them.

Sebastian

above using CLK_IGNORE_UNUSED on runit maybe is the best.

Quoted 10 lines

Fine with all of that. But: I am talking about 3.8 all the time. We have three options for issues 2 and 3 from my point of view:

1. We do proper fixes in 3.8 for issues 2 and 3.

2. We fix this regression by not gating the clock in both the DT and the non-DT case, preferably by using the correct clocks in kirkwood_ge0[01]_init(). Additionally, Jason promises that all gets well with the DT aware driver in 3.9. ;-)

Quoted 7 lines

3. Do nothing. Simply accept that we broke modular Ethernet for DT because some code relies on two pointers being set. When we introduced a new code path for DT, we forgot about these pointers. Bad luck, the solution was not nice anyway and we will do proper fixes in 3.9.

As should be clear by now, I think we should go for option 2.

Agreed, do you think all issues you are suffering will be solved with:

- [PATCH v2 2/2] clk: mvebu: Do not gate runit clock on Kirkwood (no lockup for minimal kernel configs)

- [PATCH] NET: mv643xx: get smi clock from device tree (no lockup for modular DT ethernet)

- Some patch that adds MV643XX_ETH_SHARED_NAME ".0" and ".1" clk aliases (no lockup for modular non-DT ethernet)

- Some patch that adds clk_prepare_enable to ge0/ge1 clocks to kirkwood_legacy_clk_init()
(retain MAC address for modular DT ethernet)

I'll prepare the latter patches and post them.

Quoted 3 lines · Sebastian Hesselbarth

- [PATCH v2 2/2] clk: mvebu: Do not gate runit clock on Kirkwood (no lockup for minimal kernel configs)

Quoted 5 lines

- [PATCH] NET: mv643xx: get smi clock from device tree (no lockup for modular DT ethernet)

- Some patch that adds MV643XX_ETH_SHARED_NAME ".0" and ".1" clk aliases (no lockup for modular non-DT ethernet)

I think your patch to get the smi clock is intended for device tree. Thus, the driver won't use these aliases, right?

Quoted 3 lines

- Some patch that adds clk_prepare_enable to ge0/ge1 clocks to kirkwood_legacy_clk_init()
(retain MAC address for modular DT ethernet)

I like mine better, since it only enables the clocks of the interfaces that are initialized in the init code. I tested it with non-DT as well. But either is fine with me.

Quoted 4 lines · Simon Baatz

Quoted 3 lines · Sebastian Hesselbarth

- [PATCH v2 2/2] clk: mvebu: Do not gate runit clock on Kirkwood (no lockup for minimal kernel configs)

Quoted 8 lines

Quoted 5 lines

- [PATCH] NET: mv643xx: get smi clock from device tree (no lockup for modular DT ethernet)

- Some patch that adds MV643XX_ETH_SHARED_NAME ".0" and ".1" clk aliases (no lockup for modular non-DT ethernet)

I think your patch to get the smi clock is intended for device tree. Thus, the driver won't use these aliases, right?

Actually, both patches above will not fix modular ethernet for 3.8-rc as shared driver is probed before core driver and not requesting any clk at all. The "NET: mv643xx: get smi clock from device tree" patch is based on Jason's attempt to separate shared driver.

If we need to fix modular ethernet now, we also need to add a clk_get to shared ethernet.

But yes, DT doesn't need any clock aliases.

Quoted 4 lines

Quoted 3 lines

- Some patch that adds clk_prepare_enable to ge0/ge1 clocks to kirkwood_legacy_clk_init()
(retain MAC address for modular DT ethernet)

Quoted 3 lines

I like mine better, since it only enables the clocks of the interfaces that are initialized in the init code. I tested it with non-DT as well. But either is fine with me.

I know the difference, but here it is not only about fixing an issue but have it cleanly removed later on. But I don't have a strong opinion on that and maybe Andrew or Jason should coordinate what must be fixed now and how we do it.

Quoted 4 lines · Simon Baatz

Quoted 3 lines · Sebastian Hesselbarth

- [PATCH v2 2/2] clk: mvebu: Do not gate runit clock on Kirkwood (no lockup for minimal kernel configs)

Quoted 8 lines

Quoted 5 lines

- [PATCH] NET: mv643xx: get smi clock from device tree (no lockup for modular DT ethernet)

- Some patch that adds MV643XX_ETH_SHARED_NAME ".0" and ".1" clk aliases (no lockup for modular non-DT ethernet)

I think your patch to get the smi clock is intended for device tree. Thus, the driver won't use these aliases, right?

Actually, both patches above will not fix modular ethernet for 3.8-rc as shared driver is probed before core driver and not requesting any clk at all. The "NET: mv643xx: get smi clock from device tree" patch is based on Jason's attempt to separate shared driver.

If we need to fix modular ethernet now, we also need to add a clk_get to shared ethernet.

But yes, DT doesn't need any clock aliases.

Quoted 4 lines

Quoted 3 lines

- Some patch that adds clk_prepare_enable to ge0/ge1 clocks to kirkwood_legacy_clk_init()
(retain MAC address for modular DT ethernet)

Quoted 3 lines

I like mine better, since it only enables the clocks of the interfaces that are initialized in the init code. I tested it with non-DT as well. But either is fine with me.

I know the difference, but here it is not only about fixing an issue but have it cleanly removed later on. But I don't have a strong opinion on that and maybe Andrew or Jason should coordinate what must be fixed now and how we do it.

Once we add proper DT support to mv643xx_eth, the local-mac-address will need to be defined in the dtb. We should probably poke the other Jason to see if there is a cooresponding u-boot patch to his local-mac-address patch.

Quoted 4 lines · Jason Cooper

Once we add proper DT support to mv643xx_eth, the local-mac-address will need to be defined in the dtb. We should probably poke the other Jason to see if there is a cooresponding u-boot patch to his local-mac-address patch.

Sorry I don't have a u-boot fix, we don't use u-boot here - but local-mac-address is common on other ARM drivers so I'd hope there is a u-boot facility for stuffing it already?

As it stands, there are three patches to fix this issue:

ARM: kirkwood: of_serial: fix clock gating by removing clock-frequency
ARM: kirkwood: provide ge clock aliases for shared smi
ARM: kirkwood: fix to retain gbe MAC addresses for DT kernels

wrt to ge losing mac addresses, both DT and non-DT booting are covered by Sebastian's patches, for non-DT aware mv643xx_eth.

Quoted 5 lines · Jason Cooper

As it stands, there are three patches to fix this issue:

ARM: kirkwood: of_serial: fix clock gating by removing clock-frequency
ARM: kirkwood: provide ge clock aliases for shared smi
ARM: kirkwood: fix to retain gbe MAC addresses for DT kernels

Actually, for the second patch I got distracted by the smi split patch set floating around. But that is not in current kernel and smi will not request any clock at all.

If Simon can hit another round of testing without second patch included and agrees, I suggest to keep it for next release.

Quoted 2 lines

wrt to ge losing mac addresses, both DT and non-DT booting are covered by Sebastian's patches, for non-DT aware mv643xx_eth.

non-DT already ungates ge0/1 clocks on registration and cannot loose its mac address, not my fix.

Quoted 11 lines · Sebastian Hesselbarth

Quoted 5 lines · Jason Cooper

As it stands, there are three patches to fix this issue:

ARM: kirkwood: of_serial: fix clock gating by removing clock-frequency
ARM: kirkwood: provide ge clock aliases for shared smi
ARM: kirkwood: fix to retain gbe MAC addresses for DT kernels

Actually, for the second patch I got distracted by the smi split patch set floating around. But that is not in current kernel and smi will not request any clock at all.

Quoted 2 lines

If Simon can hit another round of testing without second patch included and agrees, I suggest to keep it for next release.

Ok, I'll wait for Simon on this. fixes can go anytime, so no rush. Better to get it right the first (second?) time out.

wrt to runit gating, the only case we are not covering is if of_serial is a module (and so is everything else using the runit clk). That's *really* rare. If someone embarks down that path, they get the responsibility of not writing to all the deactivated registers. ;-)

Hi Jason, Sebastian,

Quoted 5 lines · Jason Cooper

wrt to runit gating, the only case we are not covering is if of_serial is a module (and so is everything else using the runit clk). That's *really* rare. If someone embarks down that path, they get the responsibility of not writing to all the deactivated registers. ;-)

With the serial driver now enabling runit it is really rare, but where is your enthusiasm to get to the bottom of it? At least we have indications that there really is something in "..." (my box stops somewhere when no driver enables runit)

Sebastian, are you still interested in the .flags stuff from the runit patch or do you see no need now since "ddr" is the only exception anyway?

Quoted 9 lines · Simon Baatz

Hi Jason, Sebastian,

Quoted 5 lines · Jason Cooper

wrt to runit gating, the only case we are not covering is if of_serial is a module (and so is everything else using the runit clk). That's *really* rare. If someone embarks down that path, they get the responsibility of not writing to all the deactivated registers. ;-)

Quoted 4 lines

With the serial driver now enabling runit it is really rare, but where is your enthusiasm to get to the bottom of it? At least we have indications that there really is something in "..." (my box stops somewhere when no driver enables runit)

I think watchdog could be an issue, Jason had it disabled and he was able to run it, right? I don't have a strong opinion on that, but I'd disable every clock you can - OTOH runit will be enabled anyway if you choose to have serial.

Quoted 3 lines

Sebastian, are you still interested in the .flags stuff from the runit patch or do you see no need now since "ddr" is the only exception anyway?

I don't expect to use it on Dove but it should be good to have for Kirkwood and maybe Armada XP/370 too. You prepare a patch?

Hi Jason, Sebastian,

Quoted 5 lines · Jason Cooper

wrt to runit gating, the only case we are not covering is if of_serial is a module (and so is everything else using the runit clk). That's *really* rare. If someone embarks down that path, they get the responsibility of not writing to all the deactivated registers. ;-)

With the serial driver now enabling runit it is really rare, but where is your enthusiasm to get to the bottom of it?

If you're interested, I still have a few ideas. One was to wire two USB serial adapters end to end to create a different console (console=/dev/ttyUSB0,115200, getty, etc). Since they would be going over usb, that's a different clock, so it should work and provide us with a safety net.

My guesses would be the RTC and/or GPIO blocks (the GPIO blinker needs a clock), based on table 94.

Quoted 2 lines

My guesses would be the RTC and/or GPIO blocks (the GPIO blinker needs a clock), based on table 94.

I've used AT91 parts where you can do GPO with the clock disabled, but GPI needed a ticking clock. So, yes, GPIO is a good candidate for ruint clock as well.

Looking through the data sheets, and comparing against the gated clocks, we have the following without their own clock:

RTC, I2C (a.k.a. TWI), UART, NAND, SPI, Watchdog, eFuse,

Quoted 5 lines · Jason Cooper

If you're interested, I still have a few ideas. One was to wire two USB serial adapters end to end to create a different console (console=/dev/ttyUSB0,115200, getty, etc). Since they would be going over usb, that's a different clock, so it should work and provide us with a safety net.

I can't recall, can you still use JTAG once the CPU has hung on a mbus access?

If so memory dumping the console ring, or cpu registers would get the answer pretty directly..

My guesses would be the RTC and/or GPIO blocks (the GPIO blinker needs a clock), based on table 94.

Hi Jason,

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Jason Cooper

If you're interested, I still have a few ideas. One was to wire two USB serial adapters end to end to create a different console (console=/dev/ttyUSB0,115200, getty, etc). Since they would be going over usb, that's a different clock, so it should work and provide us with a safety net.

Quoted 8 lines

I can't recall, can you still use JTAG once the CPU has hung on a mbus access?

If so memory dumping the console ring, or cpu registers would get the answer pretty directly..

My guesses would be the RTC and/or GPIO blocks (the GPIO blinker needs a clock), based on table 94.

These guesses seem to be dead on:

Moved the RTC module and GPIO modules (keys, leds) out of the way, to see whether they cause the boot with disabled runit to lock up.

System boots now and SSH login is possible!

# mount -t debugfs debugfs /sys/kernel/debug 
# cat /sys/kernel/debug/clk/tclk/runit/clk_enable_count 
0
# insmod ./gpio_keys.ko 

System locks up.

and, after a reboot:

# insmod ./rtc-mv.ko 

System locks up.

Bingo!

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Quoted 3 lines · Sebastian Hesselbarth

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

FWIW, there are two block power management controls on the kirkwood chips, one is documented to a clock gate, and one is documented to a be a power down.

I wouldn't expect the clock gate to have a problem
with the MAC address, but the powerdown I would expect to wipe it..

Quoted 6 lines · Jason Gunthorpe

Quoted 3 lines · Sebastian Hesselbarth

I just want Simon to confirm that Kirkwood's gbe is really loosing the contents of its MAC address registers during gated clocks, which is from a HW point of view very unlikely.

Quoted 3 lines

FWIW, there are two block power management controls on the kirkwood chips, one is documented to a clock gate, and one is documented to a be a power down.

Are you referring to

Memory Power Management Control Register
Offset: 0x20118

I agree that loosing the MAC address _is_ an issue but there must be another way to retain it during gated ge clocks than not gate the clocks at all.

I can think of some ways to retain it but don't know what is the most common with linux:
- make u-boot pass it through cmdline and let mv643xx get it from there
- have kirkwood's common code grab it before clk gates kick in

Quoted 8 lines · Sebastian Hesselbarth

I agree that loosing the MAC address _is_ an issue but there must be another way to retain it during gated ge clocks than not gate the clocks at all.

I can think of some ways to retain it but don't know what is the most common with linux:
- make u-boot pass it through cmdline and let mv643xx get it from there
- have kirkwood's common code grab it before clk gates kick in

The cannonical solution here is to have a DT attribute 'local-mac-address' that is filled in by the bootloader rather than attempting to pass the mac address to the kernel through the ethernet controller registers.

Until the bootloaders are fixed a reasonable hack is to have the platform startup code look for an all-zeros local-mac-address in the DT and if found then copy the MAC from the ethernet registers into the DT. Then the ethernet driver can safely be a module since the MAC is captured in the DT.

I've been using this patch here on top of the original Ian Molton patch.

Jason: Can you include something like this as well?

drivers/net/ethernet/marvell/mv643xx_eth.c +6 -0

index 7048d7c..2b2cfcb 100644
@@ -2891,6 +2891,8 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 	struct mv643xx_eth_private *mp;
 	struct net_device *dev;
 	struct resource *res;
+	const u8 *mac;
+	int len;
 	int err;
 
 	if (pdev->dev.of_node) {

@@ -2912,6 +2914,10 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 		else
 			pd->phy_addr = MV643XX_ETH_PHY_ADDR_DEFAULT;
 
+		mac = of_get_property(pdev->dev.of_node, "local-mac-address", &len);
+		if (mac && len == 6)
+		    memcpy(pd->mac_addr, mac, sizeof pd->mac_addr);
+
 		np = of_parse_phandle(pdev->dev.of_node, "mdio", 0);
 		if (np) {
 			pd->shared = of_find_device_by_node(np);

Signature · 1 line

1.7.5.4

Simon,

Can you try this in conjunction with enabling ARM_ATAG_DTB_COMPAT ? Does that solve your issue?

Quoted 4 lines · Jason Cooper

Simon,

Can you try this in conjunction with enabling ARM_ATAG_DTB_COMPAT ? Does that solve your issue?

I use

CONFIG_ARM_ATAG_DTB_COMPAT=y CONFIG_ARM_ATAG_DTB_COMPAT_CMDLINE_FROM_BOOTLOADER=y

in my standard configuration already. I don't use the original U-Boot that came with the device, but a mainline one which supports my box.

Quoted 8 lines · Sebastian Hesselbarth

I agree that loosing the MAC address _is_ an issue but there must be another way to retain it during gated ge clocks than not gate the clocks at all.

I can think of some ways to retain it but don't know what is the most common with linux:
- make u-boot pass it through cmdline and let mv643xx get it from there
- have kirkwood's common code grab it before clk gates kick in

The cannonical solution here is to have a DT attribute 'local-mac-address' that is filled in by the bootloader rather than attempting to pass the mac address to the kernel through the ethernet controller registers.

Until the bootloaders are fixed a reasonable hack is to have the platform startup code look for an all-zeros local-mac-address in the DT and if found then copy the MAC from the ethernet registers into the DT. Then the ethernet driver can safely be a module since the MAC is captured in the DT.

I've been using this patch here on top of the original Ian Molton patch.

Jason: Can you include something like this as well?

drivers/net/ethernet/marvell/mv643xx_eth.c +6 -0

index 7048d7c..2b2cfcb 100644
@@ -2891,6 +2891,8 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 	struct mv643xx_eth_private *mp;
 	struct net_device *dev;
 	struct resource *res;
+	const u8 *mac;
+	int len;
 	int err;
 
 	if (pdev->dev.of_node) {

@@ -2912,6 +2914,10 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 		else
 			pd->phy_addr = MV643XX_ETH_PHY_ADDR_DEFAULT;
 
+		mac = of_get_property(pdev->dev.of_node, "local-mac-address", &len);
+		if (mac && len == 6)
+		    memcpy(pd->mac_addr, mac, sizeof pd->mac_addr);
+
 		np = of_parse_phandle(pdev->dev.of_node, "mdio", 0);
 		if (np) {
 			pd->shared = of_find_device_by_node(np);

Signature · 7 lines

1.7.5.4


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

Commit f479db "ARM: Kirkwood: Ensure runit clock always ticks." made sure that the runit clock always ticks on Kirkwood.

When moving the clock gating to clk-gating-ctrl.c for Kirkwood DT devices, this change was disabled. Set the CLK_IGNORE_UNUSED flag for "runit" to ensure that it always ticks.

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

---
  drivers/clk/mvebu/clk-gating-ctrl.c |    5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/clk/mvebu/clk-gating-ctrl.c +0 -0

index 8fa5408..da5f807 100644
@@ -97,8 +97,11 @@ static void __init mvebu_clk_gating_setup(
  		 * isn't taken by any driver, but should anyway be
  		 * kept enabled, so we mark it as IGNORE_UNUSED for