[U-Boot] [PATCH v2 1/3] sunxi: sun4i: improve cpu clock selection method

[U-Boot] [PATCH v2 1/3] sunxi: sun4i: improve cpu clock selection method

[Iain Paton]

clock_set_pll1 would pick the next highest available cpu clock speed if
a value not in the pre defined table was selected. this potentially
results in overclocking the soc.

reverse the selection method so that we select the next lowest speed
and add the missing 912Mhz setting that's requested by sun7i which also
uses the sun4i clock code.

Signed-off-by: Iain Paton <ipaton0@gmail.com>
---
 arch/arm/cpu/armv7/sunxi/clock_sun4i.c | 35 ++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
index 49f4032..c3e04af 100644
--- a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
+++ b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
@@ -100,22 +100,23 @@ static struct {
 	unsigned int freq;
 } pll1_para[] = {
 	/* This array must be ordered by frequency. */
-	{ PLL1_CFG(16, 0, 0, 0), 384000000 },
-	{ PLL1_CFG(16, 1, 0, 0), 768000000 },
-	{ PLL1_CFG(20, 1, 0, 0), 960000000 },
-	{ PLL1_CFG(21, 1, 0, 0), 1008000000},
-	{ PLL1_CFG(22, 1, 0, 0), 1056000000},
-	{ PLL1_CFG(23, 1, 0, 0), 1104000000},
-	{ PLL1_CFG(24, 1, 0, 0), 1152000000},
-	{ PLL1_CFG(25, 1, 0, 0), 1200000000},
-	{ PLL1_CFG(26, 1, 0, 0), 1248000000},
-	{ PLL1_CFG(27, 1, 0, 0), 1296000000},
-	{ PLL1_CFG(28, 1, 0, 0), 1344000000},
-	{ PLL1_CFG(29, 1, 0, 0), 1392000000},
-	{ PLL1_CFG(30, 1, 0, 0), 1440000000},
 	{ PLL1_CFG(31, 1, 0, 0), 1488000000},
-	/* Final catchall entry */
-	{ PLL1_CFG(31, 1, 0, 0), ~0},
+	{ PLL1_CFG(30, 1, 0, 0), 1440000000},
+	{ PLL1_CFG(29, 1, 0, 0), 1392000000},
+	{ PLL1_CFG(28, 1, 0, 0), 1344000000},
+	{ PLL1_CFG(27, 1, 0, 0), 1296000000},
+	{ PLL1_CFG(26, 1, 0, 0), 1248000000},
+	{ PLL1_CFG(25, 1, 0, 0), 1200000000},
+	{ PLL1_CFG(24, 1, 0, 0), 1152000000},
+	{ PLL1_CFG(23, 1, 0, 0), 1104000000},
+	{ PLL1_CFG(22, 1, 0, 0), 1056000000},
+	{ PLL1_CFG(21, 1, 0, 0), 1008000000},
+	{ PLL1_CFG(20, 1, 0, 0), 960000000 },
+	{ PLL1_CFG(19, 1, 0, 0), 912000000 },
+	{ PLL1_CFG(16, 1, 0, 0), 768000000 },
+	/* Final catchall entry 384MHz*/
+	{ PLL1_CFG(16, 0, 0, 0), 0 },
+
 };
 
 void clock_set_pll1(unsigned int hz)
@@ -126,10 +127,12 @@ void clock_set_pll1(unsigned int hz)
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
 
 	/* Find target frequency */
-	while (pll1_para[i].freq < hz)
+	while (pll1_para[i].freq > hz)
 		i++;
 
 	hz = pll1_para[i].freq;
+	if (! hz)
+		hz = 384000000;
 
 	/* Calculate system clock divisors */
 	axi = DIV_ROUND_UP(hz, 432000000);	/* Max 450MHz */
-- 
2.1.3

[U-Boot] [PATCH v2 1/3] sunxi: sun4i: improve cpu clock selection method

[Hans de Goede]

Hi,

On 28-03-15 11:25, Iain Paton wrote:
> clock_set_pll1 would pick the next highest available cpu clock speed if
> a value not in the pre defined table was selected. this potentially
> results in overclocking the soc.
>
> reverse the selection method so that we select the next lowest speed
> and add the missing 912Mhz setting that's requested by sun7i which also
> uses the sun4i clock code.
>
> Signed-off-by: Iain Paton <ipaton0@gmail.com>

Thanks for the new set.

I've found one small issue with the second patch:
"sunxi: use CONFIG_SYS_CLK_FREQ to set cpu clock"

sun7i.h contained the following:

#define CONFIG_SYS_CLK_FREQ		24000000
#define CONFIG_TIMER_CLK_FREQ		CONFIG_SYS_CLK_FREQ

Which is a conflicting usage of CONFIG_SYS_CLK_FREQ compared to the new
usage introduced by your patch. I've fixed this by changing the above
to:

#define CONFIG_TIMER_CLK_FREQ		24000000

And run some compile and runtime tests.

Everything builds and works fine with this change added, so I've pushed
these 3 commits + one other fix to: u-boot-sunxi/master

And I will send out a pull-req shortly to get these fixes included into
the upcoming u-boot v2015.04 release.

Regards,

Hans


> ---
>   arch/arm/cpu/armv7/sunxi/clock_sun4i.c | 35 ++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> index 49f4032..c3e04af 100644
> --- a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> +++ b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> @@ -100,22 +100,23 @@ static struct {
>   	unsigned int freq;
>   } pll1_para[] = {
>   	/* This array must be ordered by frequency. */
> -	{ PLL1_CFG(16, 0, 0, 0), 384000000 },
> -	{ PLL1_CFG(16, 1, 0, 0), 768000000 },
> -	{ PLL1_CFG(20, 1, 0, 0), 960000000 },
> -	{ PLL1_CFG(21, 1, 0, 0), 1008000000},
> -	{ PLL1_CFG(22, 1, 0, 0), 1056000000},
> -	{ PLL1_CFG(23, 1, 0, 0), 1104000000},
> -	{ PLL1_CFG(24, 1, 0, 0), 1152000000},
> -	{ PLL1_CFG(25, 1, 0, 0), 1200000000},
> -	{ PLL1_CFG(26, 1, 0, 0), 1248000000},
> -	{ PLL1_CFG(27, 1, 0, 0), 1296000000},
> -	{ PLL1_CFG(28, 1, 0, 0), 1344000000},
> -	{ PLL1_CFG(29, 1, 0, 0), 1392000000},
> -	{ PLL1_CFG(30, 1, 0, 0), 1440000000},
>   	{ PLL1_CFG(31, 1, 0, 0), 1488000000},
> -	/* Final catchall entry */
> -	{ PLL1_CFG(31, 1, 0, 0), ~0},
> +	{ PLL1_CFG(30, 1, 0, 0), 1440000000},
> +	{ PLL1_CFG(29, 1, 0, 0), 1392000000},
> +	{ PLL1_CFG(28, 1, 0, 0), 1344000000},
> +	{ PLL1_CFG(27, 1, 0, 0), 1296000000},
> +	{ PLL1_CFG(26, 1, 0, 0), 1248000000},
> +	{ PLL1_CFG(25, 1, 0, 0), 1200000000},
> +	{ PLL1_CFG(24, 1, 0, 0), 1152000000},
> +	{ PLL1_CFG(23, 1, 0, 0), 1104000000},
> +	{ PLL1_CFG(22, 1, 0, 0), 1056000000},
> +	{ PLL1_CFG(21, 1, 0, 0), 1008000000},
> +	{ PLL1_CFG(20, 1, 0, 0), 960000000 },
> +	{ PLL1_CFG(19, 1, 0, 0), 912000000 },
> +	{ PLL1_CFG(16, 1, 0, 0), 768000000 },
> +	/* Final catchall entry 384MHz*/
> +	{ PLL1_CFG(16, 0, 0, 0), 0 },
> +
>   };
>
>   void clock_set_pll1(unsigned int hz)
> @@ -126,10 +127,12 @@ void clock_set_pll1(unsigned int hz)
>   		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>
>   	/* Find target frequency */
> -	while (pll1_para[i].freq < hz)
> +	while (pll1_para[i].freq > hz)
>   		i++;
>
>   	hz = pll1_para[i].freq;
> +	if (! hz)
> +		hz = 384000000;
>
>   	/* Calculate system clock divisors */
>   	axi = DIV_ROUND_UP(hz, 432000000);	/* Max 450MHz */
>

[U-Boot] [PATCH v2 1/3] sunxi: sun4i: improve cpu clock selection method

[Iain Paton]

On 29/03/15 13:39, Hans de Goede wrote:
> Hi,
> 
> On 28-03-15 11:25, Iain Paton wrote:
>> clock_set_pll1 would pick the next highest available cpu clock speed if
>> a value not in the pre defined table was selected. this potentially
>> results in overclocking the soc.
>>
>> reverse the selection method so that we select the next lowest speed
>> and add the missing 912Mhz setting that's requested by sun7i which also
>> uses the sun4i clock code.
>>
>> Signed-off-by: Iain Paton <ipaton0@gmail.com>
> 
> Thanks for the new set.
> 
> I've found one small issue with the second patch:
> "sunxi: use CONFIG_SYS_CLK_FREQ to set cpu clock"
> 
> sun7i.h contained the following:
> 
> #define CONFIG_SYS_CLK_FREQ        24000000
> #define CONFIG_TIMER_CLK_FREQ        CONFIG_SYS_CLK_FREQ
> 
> Which is a conflicting usage of CONFIG_SYS_CLK_FREQ compared to the new
> usage introduced by your patch. I've fixed this by changing the above
> to:
> 
> #define CONFIG_TIMER_CLK_FREQ        24000000
> 
> And run some compile and runtime tests.
> 
> Everything builds and works fine with this change added, so I've pushed
> these 3 commits + one other fix to: u-boot-sunxi/master

Apologies for missing that. Thanks for the fixup.

Did you see any compile or runtime errors?

I definately did build and boot the result several times before sending it, 
especially to verify the behaviour of the first patch.

So I'm curious as to how I missed it and you found the issue.

Rgds,
Iain

[U-Boot] [PATCH v2 1/3] sunxi: sun4i: improve cpu clock selection method

[Hans de Goede]

Hi,

On 30-03-15 09:52, Iain Paton wrote:
> On 29/03/15 13:39, Hans de Goede wrote:
>> Hi,
>>
>> On 28-03-15 11:25, Iain Paton wrote:
>>> clock_set_pll1 would pick the next highest available cpu clock speed if
>>> a value not in the pre defined table was selected. this potentially
>>> results in overclocking the soc.
>>>
>>> reverse the selection method so that we select the next lowest speed
>>> and add the missing 912Mhz setting that's requested by sun7i which also
>>> uses the sun4i clock code.
>>>
>>> Signed-off-by: Iain Paton <ipaton0@gmail.com>
>>
>> Thanks for the new set.
>>
>> I've found one small issue with the second patch:
>> "sunxi: use CONFIG_SYS_CLK_FREQ to set cpu clock"
>>
>> sun7i.h contained the following:
>>
>> #define CONFIG_SYS_CLK_FREQ        24000000
>> #define CONFIG_TIMER_CLK_FREQ        CONFIG_SYS_CLK_FREQ
>>
>> Which is a conflicting usage of CONFIG_SYS_CLK_FREQ compared to the new
>> usage introduced by your patch. I've fixed this by changing the above
>> to:
>>
>> #define CONFIG_TIMER_CLK_FREQ        24000000
>>
>> And run some compile and runtime tests.
>>
>> Everything builds and works fine with this change added, so I've pushed
>> these 3 commits + one other fix to: u-boot-sunxi/master
>
> Apologies for missing that. Thanks for the fixup.
>
> Did you see any compile or runtime errors?
>
> I definately did build and boot the result several times before sending it,
> especially to verify the behaviour of the first patch.
>
> So I'm curious as to how I missed it and you found the issue.

The problem caused compile time warnings when building for a sun7i (A20)
board, and likely would have also caused runtime problems, but I did not
test a build with the problem.

Regards,

Hans