[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Icenowy Zheng]

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 */
-- 
2.12.2

[U-Boot] [linux-sunxi] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Vincent Legoll]

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 ?

-- 
Vincent Legoll

[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[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?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170410/bc7bb7c8/attachment.sig>

[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[icenowy at aosc.io]

在 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-Fix-PLL1-setup-to-never-use-dividers.patch
[2] https://github.com/megous/h3-firmware

> 
> Maxime

[U-Boot] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Maxime Ripard]

On Mon, Apr 10, 2017 at 04:43:00PM +0800, icenowy at 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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170411/7d450592/attachment.sig>

[U-Boot] [linux-sunxi] Re: [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[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/

regards,
  o.j.

> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170410/ac718063/attachment.sig>

[U-Boot] [linux-sunxi] Re: [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[icenowy at aosc.io]

在 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
>> 

[U-Boot] [linux-sunxi] Re: [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Ondřej Jirman]

icenowy at 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
> > > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170410/63542b44/attachment.sig>

[U-Boot] [linux-sunxi] Re: [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170411/c0df8c43/attachment.sig>

[U-Boot] [linux-sunxi] [PATCH] sunxi: set up PLL1 on sun6i+ without use dividers

[Ondřej Jirman]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170410/8819ebef/attachment.sig>