| Message ID | 1359226864-28811-1-git-send-email-gmbnomis@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Simon, On Sat, Jan 26, 2013 at 08:01:04PM +0100, Simon Baatz wrote: > 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 <gmbnomis@gmail.com> > --- > > 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). 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@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@2011c { /* don't lose eth0 mac address */ ungateable = <0>; }; where in kirkwood.dtsi we had: gate_clk: clock-gating-control@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 thx, Jason.
On Sat, Jan 26, 2013 at 06:50:37PM -0500, Jason Cooper wrote: > Simon, > > On Sat, Jan 26, 2013 at 08:01:04PM +0100, Simon Baatz wrote: > > 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. > > > > > > 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. Me neither, the patch was intended to get 3.8 to work with modularized drivers. It was not intended to be the final solution. However, I think I found a better way for 3.8, see below. > > 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 did not notice it before, but looking at this, I realized why there is the problem with the ge[01] clocks: kirkwood_ge0[01]_init() in common.c depends on clk pointers that are only initialized in the non-DT case (kirkwood_clk_init()) but not in the DT case (kirkwood_legacy_clk_init() in board-dt.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) For 3.9 with a DT converted ethernet driver, we will need something more clever. If you agree, I can prepare patches for 3.8. > 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. ;-) > I like this idea. That fits my 'more clever' from above ;-) > eg in kirkwood-km_kirkwood.dts: > > gate_clk: clock-gating-control@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@2011c { > /* don't lose eth0 mac address */ > ungateable = <0>; > }; > > where in kirkwood.dtsi we had: > > gate_clk: clock-gating-control@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 Yes, probably better. - Simon
On 01/27/2013 02:31 AM, Simon Baatz wrote: > On Sat, Jan 26, 2013 at 06:50:37PM -0500, Jason Cooper wrote: >> 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). The statement in board-km_kirkwood.c is just not true. Andrew and I did some tests with Valentin Longchamps back when we introduced DT clk gating. Keymile board hangs not because of the accesses to clk gating ctrl registers but the phy gating that accessed the sata registers that are missing on the Kirkwood used on that board. Phy gates have not been used since DT clk gating at all. If we reintroduce them in a DT compatible way we can still disable them for keymile board in DT. > 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) >> and in any other board: >> >> gate_clk: clock-gating-control@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. Sebastian
Hi Sebastian, On Sun, Jan 27, 2013 at 11:15:59AM +0100, Sebastian Hesselbarth wrote: > > >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) 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. > > >>and in any other board: > >> > >>gate_clk: clock-gating-control@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. 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. - Simon
On 01/27/2013 11:56 AM, Simon Baatz wrote: > On Sun, Jan 27, 2013 at 11:15:59AM +0100, Sebastian Hesselbarth wrote: >> >>> 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) > > 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 prefer .flags even with the noise the patch will cause. It is a fix and there is no reason why we should fix that fix later just because of the amount of changes it will cause. > 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. 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(). >> 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. Again, then it is about fixing the driver not the clock gating. I understand that it will render the interface unusable but then a modular kirkwood kernel is just not supported in the current state. Sebastian
> 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(). Hi Sebastian 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. Andrew
On 01/27/2013 04:28 PM, Andrew Lunn wrote: >> 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(). > > 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. Andrew, if Simon confirms that gbe is really loosing its MAC address when clocks get gated, I still think that we should rely on other mechanisms to get a valid MAC address. Have a look at other DT ethernet drivers, they use local-mac-address property to pass MAC address from boot loader. IMHO that is what we should exploit on mv643xx_eth, too. If the property is set on boot, we can store it in some private data. If it is not set because some legacy u-boot without DT, we can rely on what is in the MAC address registers and just call modular eth on those platforms broken. If the easy fix is to update u-boot or compile it as built-in we shouldn't care at all. Without proper DT in u-boot you will always have a board specific kernel anyway. Sebastian
Dear Andrew Lunn, On Sun, 27 Jan 2013 16:28:37 +0100, Andrew Lunn wrote: > 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. Are you interested? Thomas
On Sun, Jan 27, 2013 at 04:41:04PM +0100, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Sun, 27 Jan 2013 16:28:37 +0100, Andrew Lunn wrote: > > > 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. Does this add on to ARM_ATAG_DTB_COMPAT [1]? Perhaps APPENDED_DTB should select it? Maybe the answer is as simple as having the driver pull the mac address from the dtb while ARM_ATAG_DTB_COMPAT is configured. thx, Jason. [1] arch/arm/Kconfig:1970
diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c index 8fa5408..917049c 100644 --- a/drivers/clk/mvebu/clk-gating-ctrl.c +++ b/drivers/clk/mvebu/clk-gating-ctrl.c @@ -28,6 +28,7 @@ struct mvebu_soc_descr { const char *name; const char *parent; int bit_idx; + unsigned long flags; }; #define to_clk_gate(_hw) container_of(_hw, struct clk_gate, hw) @@ -88,21 +89,12 @@ static void __init mvebu_clk_gating_setup( } for (n = 0; n < ctrl->num_gates; n++) { - u8 flags = 0; const char *parent = (descr[n].parent) ? descr[n].parent : default_parent; - - /* - * On Armada 370, the DDR clock is a special case: it - * isn't taken by any driver, but should anyway be - * kept enabled, so we mark it as IGNORE_UNUSED for - * now. - */ - if (!strcmp(descr[n].name, "ddr")) - flags |= CLK_IGNORE_UNUSED; - ctrl->gates[n] = clk_register_gate(NULL, descr[n].name, parent, - flags, base, descr[n].bit_idx, 0, &ctrl->lock); + descr[n].flags, base, + descr[n].bit_idx, + 0, &ctrl->lock); WARN_ON(IS_ERR(ctrl->gates[n])); } of_clk_add_provider(np, mvebu_clk_gating_get_src, ctrl); @@ -114,92 +106,99 @@ static void __init mvebu_clk_gating_setup( #ifdef CONFIG_MACH_ARMADA_370 static const struct mvebu_soc_descr __initconst armada_370_gating_descr[] = { - { "audio", NULL, 0 }, - { "pex0_en", NULL, 1 }, - { "pex1_en", NULL, 2 }, - { "ge1", NULL, 3 }, - { "ge0", NULL, 4 }, - { "pex0", NULL, 5 }, - { "pex1", NULL, 9 }, - { "sata0", NULL, 15 }, - { "sdio", NULL, 17 }, - { "tdm", NULL, 25 }, - { "ddr", NULL, 28 }, - { "sata1", NULL, 30 }, + { .name = "audio", .bit_idx = 0 }, + { .name = "pex0_en", .bit_idx = 1 }, + { .name = "pex1_en", .bit_idx = 2 }, + { .name = "ge1", .bit_idx = 3 }, + { .name = "ge0", .bit_idx = 4 }, + { .name = "pex0", .bit_idx = 5 }, + { .name = "pex1", .bit_idx = 9 }, + { .name = "sata0", .bit_idx = 15 }, + { .name = "sdio", .bit_idx = 17 }, + { .name = "tdm", .bit_idx = 25 }, + /* The DDR clock is a special case: it isn't taken by + * any driver, but should anyway be kept enabled. + */ + { .name = "ddr", .bit_idx = 28, .flags = CLK_IGNORE_UNUSED }, + { .name = "sata1", .bit_idx = 30 }, { } }; #endif #ifdef CONFIG_MACH_ARMADA_XP static const struct mvebu_soc_descr __initconst armada_xp_gating_descr[] = { - { "audio", NULL, 0 }, - { "ge3", NULL, 1 }, - { "ge2", NULL, 2 }, - { "ge1", NULL, 3 }, - { "ge0", NULL, 4 }, - { "pex0", NULL, 5 }, - { "pex1", NULL, 6 }, - { "pex2", NULL, 7 }, - { "pex3", NULL, 8 }, - { "bp", NULL, 13 }, - { "sata0lnk", NULL, 14 }, - { "sata0", "sata0lnk", 15 }, - { "lcd", NULL, 16 }, - { "sdio", NULL, 17 }, - { "usb0", NULL, 18 }, - { "usb1", NULL, 19 }, - { "usb2", NULL, 20 }, - { "xor0", NULL, 22 }, - { "crypto", NULL, 23 }, - { "tdm", NULL, 25 }, - { "xor1", NULL, 28 }, - { "sata1lnk", NULL, 29 }, - { "sata1", "sata1lnk", 30 }, + { .name = "audio", .bit_idx = 0 }, + { .name = "ge3", .bit_idx = 1 }, + { .name = "ge2", .bit_idx = 2 }, + { .name = "ge1", .bit_idx = 3 }, + { .name = "ge0", .bit_idx = 4 }, + { .name = "pex0", .bit_idx = 5 }, + { .name = "pex1", .bit_idx = 6 }, + { .name = "pex2", .bit_idx = 7 }, + { .name = "pex3", .bit_idx = 8 }, + { .name = "bp", .bit_idx = 13 }, + { .name = "sata0lnk", .bit_idx = 14 }, + { .name = "sata0", .bit_idx = 15, .parent = "sata0lnk" }, + { .name = "lcd", .bit_idx = 16 }, + { .name = "sdio", .bit_idx = 17 }, + { .name = "usb0", .bit_idx = 18 }, + { .name = "usb1", .bit_idx = 19 }, + { .name = "usb2", .bit_idx = 20 }, + { .name = "xor0", .bit_idx = 22 }, + { .name = "crypto", .bit_idx = 23 }, + { .name = "tdm", .bit_idx = 25 }, + { .name = "xor1", .bit_idx = 28 }, + { .name = "sata1lnk", .bit_idx = 29 }, + { .name = "sata1", .bit_idx = 30, .parent = "sata1lnk" }, { } }; #endif #ifdef CONFIG_ARCH_DOVE static const struct mvebu_soc_descr __initconst dove_gating_descr[] = { - { "usb0", NULL, 0 }, - { "usb1", NULL, 1 }, - { "ge", "gephy", 2 }, - { "sata", NULL, 3 }, - { "pex0", NULL, 4 }, - { "pex1", NULL, 5 }, - { "sdio0", NULL, 8 }, - { "sdio1", NULL, 9 }, - { "nand", NULL, 10 }, - { "camera", NULL, 11 }, - { "i2s0", NULL, 12 }, - { "i2s1", NULL, 13 }, - { "crypto", NULL, 15 }, - { "ac97", NULL, 21 }, - { "pdma", NULL, 22 }, - { "xor0", NULL, 23 }, - { "xor1", NULL, 24 }, - { "gephy", NULL, 30 }, + { .name = "usb0", .bit_idx = 0 }, + { .name = "usb1", .bit_idx = 1 }, + { .name = "ge", .bit_idx = 2, .parent = "gephy" }, + { .name = "sata", .bit_idx = 3 }, + { .name = "pex0", .bit_idx = 4 }, + { .name = "pex1", .bit_idx = 5 }, + { .name = "sdio0", .bit_idx = 8 }, + { .name = "sdio1", .bit_idx = 9 }, + { .name = "nand", .bit_idx = 10 }, + { .name = "camera", .bit_idx = 11 }, + { .name = "i2s0", .bit_idx = 12 }, + { .name = "i2s1", .bit_idx = 13 }, + { .name = "crypto", .bit_idx = 15 }, + { .name = "ac97", .bit_idx = 21 }, + { .name = "pdma", .bit_idx = 22 }, + { .name = "xor0", .bit_idx = 23 }, + { .name = "xor1", .bit_idx = 24 }, + { .name = "gephy", .bit_idx = 30 }, { } }; #endif #ifdef CONFIG_ARCH_KIRKWOOD static const struct mvebu_soc_descr __initconst kirkwood_gating_descr[] = { - { "ge0", NULL, 0 }, - { "pex0", NULL, 2 }, - { "usb0", NULL, 3 }, - { "sdio", NULL, 4 }, - { "tsu", NULL, 5 }, - { "runit", NULL, 7 }, - { "xor0", NULL, 8 }, - { "audio", NULL, 9 }, - { "sata0", NULL, 14 }, - { "sata1", NULL, 15 }, - { "xor1", NULL, 16 }, - { "crypto", NULL, 17 }, - { "pex1", NULL, 18 }, - { "ge1", NULL, 19 }, - { "tdm", NULL, 20 }, + /* Prevent that the Ethernet interfaces will forget their + * MAC addresses by keeping the "ge0/1" clocks running. + */ + { .name = "ge0", .bit_idx = 0, .flags = CLK_IGNORE_UNUSED }, + { .name = "pex0", .bit_idx = 2 }, + { .name = "usb0", .bit_idx = 3 }, + { .name = "sdio", .bit_idx = 4 }, + { .name = "tsu", .bit_idx = 5 }, + /* Devices will lock hard if the "runit" clock is gated. */ + { .name = "runit", .bit_idx = 7, .flags = CLK_IGNORE_UNUSED }, + { .name = "xor0", .bit_idx = 8 }, + { .name = "audio", .bit_idx = 9 }, + { .name = "sata0", .bit_idx = 14 }, + { .name = "sata1", .bit_idx = 15 }, + { .name = "xor1", .bit_idx = 16 }, + { .name = "crypto", .bit_idx = 17 }, + { .name = "pex1", .bit_idx = 18 }, + { .name = "ge1", .bit_idx = 19, .flags = CLK_IGNORE_UNUSED }, + { .name = "tdm", .bit_idx = 20 }, { } }; #endif
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 <gmbnomis@gmail.com> --- 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). The patch creates much noise, but is hopefully obvious. I have tested it on IB-NAS6210 (kirkwood, single Ethernet port). The other platforms are only compile tested. I don't know whether something similar is required for other platforms as well; I left them untouched. - Simon drivers/clk/mvebu/clk-gating-ctrl.c | 159 +++++++++++++++++------------------ 1 file changed, 79 insertions(+), 80 deletions(-)