[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io ---
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
arch/arm/mach-sunxi/clock_sun6i.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-sunxi/clock_sun6i.c b/arch/arm/mach-sunxi/clock_sun6i.c index 4762fbf0c3..ce4291b30e 100644 --- a/arch/arm/mach-sunxi/clock_sun6i.c +++ b/arch/arm/mach-sunxi/clock_sun6i.c @@ -98,11 +98,10 @@ void clock_set_pll1(unsigned int clk) int k = 1; int m = 1;
- if (clk > 1152000000) { - k = 2; - } else if (clk > 768000000) { + if (clk >= 1368000000) { k = 3; - m = 2; + } else if (clk >= 768000000) { + k = 2; }
/* Switch to 24MHz clock while changing PLL1 */
Hello,
On Sun, Apr 9, 2017 at 6:19 PM, Icenowy Zheng icenowy@aosc.io wrote:
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Isn't adding a "From:" header line the way to reflect that ?
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
Maxime
在 2017-04-10 14:59，Maxime Ripard 写道：
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
Armbian has included this patch since several months ago, and it's proven that without it DVFS will be not stable. (And it proved at least it won't make the system more unstable). See [1].
And for Ondrej's research, please see [2].
I also verified it on my own board.
P.S. it's also the behavior of the clk-sun8iw7_tbl.c, in which the M part of PLL_CPUX is always 0.
[1] https://github.com/igorpecovnik/lib/blob/master/patch/u-boot/u-boot-sunxi/h3... [2] https://github.com/megous/h3-firmware
Maxime
On Mon, Apr 10, 2017 at 04:43:00PM +0800, icenowy@aosc.io wrote:
在 2017-04-10 14:59，Maxime Ripard 写道：
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
Armbian has included this patch since several months ago, and it's proven that without it DVFS will be not stable. (And it proved at least it won't make the system more unstable). See [1].
Then, this should have been your commit log.
Thanks! Maxime
Hi Maxime,
Maxime Ripard píše v Po 10. 04. 2017 v 08:59 +0200:
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
We already discussed this here: https://patchwork.kernel.org/patch/9446 365/
regards, o.j.
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
在 2017-04-10 18:06，Ondřej Jirman 写道：
Hi Maxime,
Maxime Ripard píše v Po 10. 04. 2017 v 08:59 +0200:
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
We already discussed this here: https://patchwork.kernel.org/patch/9446 365/
P.S. as Chen-Yu dig out, it's not the correct work to restrict factors, but we need to gate the PLL_CPUX when adjusting. (Of course the mux should also be switched to osc24M when PLL_CPUX is gated)
I tried this on your h3-firmware code, and it succeeded with even P overused (for any frequency) (and M is also free in this situation).
regards, o.j.
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
icenowy@aosc.io píše v Po 10. 04. 2017 v 18:15 +0800:
在 2017-04-10 18:06，Ondřej Jirman 写道：
Hi Maxime,
Maxime Ripard píše v Po 10. 04. 2017 v 08:59 +0200:
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
We already discussed this here: https://patchwork.kernel.org/patch/9446 365/
P.S. as Chen-Yu dig out, it's not the correct work to restrict factors, but we need to gate the PLL_CPUX when adjusting. (Of course the mux should also be switched to osc24M when PLL_CPUX is gated)
I tried this on your h3-firmware code, and it succeeded with even P overused (for any frequency) (and M is also free in this situation).
Interesting, I was only switching the source of the CPUX clock, and not gating the CPUX_PLL for the test. I guess the kernel still doesn't gate CPUX_PLL, so this whole situation should be fixed by adding the gating of PLL during the chnage and that would be it?
Sounds like the cleanest solution so far.
regards, o.j.
regards, o.j.
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
On Mon, Apr 10, 2017 at 12:06:27PM +0200, Ondřej Jirman wrote:
Hi Maxime,
Maxime Ripard píše v Po 10. 04. 2017 v 08:59 +0200:
On Mon, Apr 10, 2017 at 12:19:41AM +0800, Icenowy Zheng wrote:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
This doesn't mean anything. How has this been verified?
We already discussed this here: https://patchwork.kernel.org/patch/9446 365/
Yes, I know. I wasn't discussing the fact that this was the right thing to do or not, just that the wording didn't allow anyone to get an idea of *how* to reproduce that test on their own board.
Maxime
Hi Icenowy,
Icenowy Zheng píše v Po 10. 04. 2017 v 00:19 +0800:
According to the researching result of Ondrej Jirman, the factor M of PLL1 shouldn't be used and the factor P should be used only if the intended frequency is lower than 288MHz. This is proven by the clk-sun8iw7_tbl.c in the BSP source code -- in there the M value is always 0 and the maximum frequency that P is not 0 is 224MHz.
As P is ignored on sun6i, it's not currently used. This patch removed the usage of M.
This patch is an original work by Ondrej Jirman, however, he didn't add a Signed-off-by tag here to his commit. So I take this code and added my Signed-off-by.
Well, here it is if that helps:
Signed-off-by: Ondřej Jirman megous@megous.com
Though this might be somewhat difficult change to make. It works perfectly when combined with kernel that will not use dividers either. So it's ok with BSP kernel as you noted, but it might cause trouble (lockup on boot) with the mainline kernel unless the kernel is patched too to not use dividers. (it does now)
Current kernel patches are here: https://megous.com/dl/h3-cpufreq/
I have only tested that it breaks if kernel is patched and u-boot is not. I haven't tried to verify that it works the other way round, which is important for the mainlining of these changes.
That's also the reason it's been sitting in my trees for a long time and not being pushed out. It's not a problem when you control both u- boot and the kernel, but that will not be a case when mainlining. Though the current kernel will be broken anyway, when the kernel starts to enable cpufreq/DVFS, so it might not matter either way.
TLDR: - if we patch the kernel first without patching u-boot there will be lockups - if we patch u-boot first without kernel, that's not tested yet
regards, o.j.
Signed-off-by: Icenowy Zheng icenowy@aosc.io
This is a critical patch, and should be added to 2017.05.
It has been verified by the Armbian.
arch/arm/mach-sunxi/clock_sun6i.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-sunxi/clock_sun6i.c b/arch/arm/mach-sunxi/clock_sun6i.c index 4762fbf0c3..ce4291b30e 100644 --- a/arch/arm/mach-sunxi/clock_sun6i.c +++ b/arch/arm/mach-sunxi/clock_sun6i.c @@ -98,11 +98,10 @@ void clock_set_pll1(unsigned int clk) int k = 1; int m = 1;
- if (clk > 1152000000) {
k = 2;- } else if (clk > 768000000) {
- if (clk >= 1368000000) { k = 3;
m = 2;
} else if (clk >= 768000000) {
k = 2;}
/* Switch to 24MHz clock while changing PLL1 */
-- 2.12.2
participants (5)
-
Icenowy Zheng -
icenowy＠aosc.io
-
Maxime Ripard -
Ondřej Jirman -
Vincent Legoll