diff mbox

[4/7] clk: samsung: exynos5433: fix PLL rates

Message ID 20180213134032.30235-5-a.hajda@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Andrzej Hajda Feb. 13, 2018, 1:40 p.m. UTC 
Declared rates did not match rates generated by PLL.
As a result driver behaved inconsitently.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Chanwoo Choi Feb. 14, 2018, 5:45 a.m. UTC | #1 
Hi Sylwester and Andrzej,

Exynos5433 TRM shows different PLL frequency from the calculated value. Actually, I'm not sure which value is the correct value between TRM's value and real calculated value.

If we only consider the equation, I agree that we have to replace them with real calculated value with equation. But, TRM shows the diagram of clock domain which contains the multiple IPs. And each diagram specifies the required source clock rate of each child IP. Maybe, this source clock rate might be calculated from the PLL rate provided by TRM. (even if it's different from real calculated value).

Although the real rate is a little bit incorrect, we might better to keep PLL rate provided by TRM in order to get the required source clock of child IPs as the TRM. If we modify the PLL rate provided by TRM, the source clock rate of child IP might be different from TRM.

Basically, I have no objection of this patch. Just I think that need to check it.

Regards,
Chanwoo Choi

On 2018년 02월 13일 22:40, Andrzej Hajda wrote:
> Declared rates did not match rates generated by PLL.
> As a result driver behaved inconsitently.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index db270908037a..335bebfa21c0 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -729,7 +729,7 @@ static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
>  	PLL_35XX_RATE(800000000U,  400, 6,  1),
>  	PLL_35XX_RATE(733000000U,  733, 12, 1),
>  	PLL_35XX_RATE(700000000U,  175, 3,  1),
> -	PLL_35XX_RATE(667000000U,  222, 4,  1),
> +	PLL_35XX_RATE(666000000U,  222, 4,  1),
>  	PLL_35XX_RATE(633000000U,  211, 4,  1),
>  	PLL_35XX_RATE(600000000U,  500, 5,  2),
>  	PLL_35XX_RATE(552000000U,  460, 5,  2),
> @@ -757,12 +757,12 @@ static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
>  /* AUD_PLL */
>  static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initconst = {
>  	PLL_36XX_RATE(400000000U, 200, 3, 2,      0),
> -	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
> +	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),
>  	PLL_36XX_RATE(384000000U, 128, 2, 2,      0),
> -	PLL_36XX_RATE(368640000U, 246, 4, 2, -15729),
> -	PLL_36XX_RATE(361507200U, 181, 3, 2, -16148),
> -	PLL_36XX_RATE(338688000U, 113, 2, 2,  -6816),
> -	PLL_36XX_RATE(294912000U,  98, 1, 3,  19923),
> +	PLL_36XX_RATE(368639991U, 246, 4, 2, -15729),
> +	PLL_36XX_RATE(361507202U, 181, 3, 2, -16148),
> +	PLL_36XX_RATE(338687988U, 113, 2, 2,  -6816),
> +	PLL_36XX_RATE(294912002U,  98, 1, 3,  19923),
>  	PLL_36XX_RATE(288000000U,  96, 1, 3,      0),
>  	PLL_36XX_RATE(252000000U,  84, 1, 3,      0),
>  	{ /* sentinel */ }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda Feb. 14, 2018, 8:36 a.m. UTC | #2 
On 14.02.2018 06:45, Chanwoo Choi wrote:
> Hi Sylwester and Andrzej,
>
> Exynos5433 TRM shows different PLL frequency from the calculated value. Actually, I'm not sure which value is the correct value between TRM's value and real calculated value.
>
> If we only consider the equation, I agree that we have to replace them with real calculated value with equation. But, TRM shows the diagram of clock domain which contains the multiple IPs. And each diagram specifies the required source clock rate of each child IP. Maybe, this source clock rate might be calculated from the PLL rate provided by TRM. (even if it's different from real calculated value).
>
> Although the real rate is a little bit incorrect, we might better to keep PLL rate provided by TRM in order to get the required source clock of child IPs as the TRM. If we modify the PLL rate provided by TRM, the source clock rate of child IP might be different from TRM.
>
> Basically, I have no objection of this patch. Just I think that need to check it.

After verifying documentation it seems that in case of fractional PLLs
TRM shows rates with lower precision, so I wouldn't say in such case it
is different.
Only 667MHz frequency looks odd, but it seems to be a bug in TRM, I
guess copy/paste bug from Exyons5430, where 667MHz rate was generated
from different PMS coefficients.
Android code has also tables proving it should be 666MHz [1].


[1]:
https://github.com/googyanas/Googy-Max-N4-Kernel/blob/master/drivers/clk/samsung/clk-exynos5433.c#L3260

Regards
Andrzej

>
> Regards,
> Chanwoo Choi
>
> On 2018년 02월 13일 22:40, Andrzej Hajda wrote:
>> Declared rates did not match rates generated by PLL.
>> As a result driver behaved inconsitently.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5433.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>> index db270908037a..335bebfa21c0 100644
>> --- a/drivers/clk/samsung/clk-exynos5433.c
>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>> @@ -729,7 +729,7 @@ static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
>>  	PLL_35XX_RATE(800000000U,  400, 6,  1),
>>  	PLL_35XX_RATE(733000000U,  733, 12, 1),
>>  	PLL_35XX_RATE(700000000U,  175, 3,  1),
>> -	PLL_35XX_RATE(667000000U,  222, 4,  1),
>> +	PLL_35XX_RATE(666000000U,  222, 4,  1),
>>  	PLL_35XX_RATE(633000000U,  211, 4,  1),
>>  	PLL_35XX_RATE(600000000U,  500, 5,  2),
>>  	PLL_35XX_RATE(552000000U,  460, 5,  2),
>> @@ -757,12 +757,12 @@ static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
>>  /* AUD_PLL */
>>  static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initconst = {
>>  	PLL_36XX_RATE(400000000U, 200, 3, 2,      0),
>> -	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
>> +	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),
>>  	PLL_36XX_RATE(384000000U, 128, 2, 2,      0),
>> -	PLL_36XX_RATE(368640000U, 246, 4, 2, -15729),
>> -	PLL_36XX_RATE(361507200U, 181, 3, 2, -16148),
>> -	PLL_36XX_RATE(338688000U, 113, 2, 2,  -6816),
>> -	PLL_36XX_RATE(294912000U,  98, 1, 3,  19923),
>> +	PLL_36XX_RATE(368639991U, 246, 4, 2, -15729),
>> +	PLL_36XX_RATE(361507202U, 181, 3, 2, -16148),
>> +	PLL_36XX_RATE(338687988U, 113, 2, 2,  -6816),
>> +	PLL_36XX_RATE(294912002U,  98, 1, 3,  19923),
>>  	PLL_36XX_RATE(288000000U,  96, 1, 3,      0),
>>  	PLL_36XX_RATE(252000000U,  84, 1, 3,      0),
>>  	{ /* sentinel */ }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index db270908037a..335bebfa21c0 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -729,7 +729,7 @@  static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
 	PLL_35XX_RATE(800000000U,  400, 6,  1),
 	PLL_35XX_RATE(733000000U,  733, 12, 1),
 	PLL_35XX_RATE(700000000U,  175, 3,  1),
-	PLL_35XX_RATE(667000000U,  222, 4,  1),
+	PLL_35XX_RATE(666000000U,  222, 4,  1),
 	PLL_35XX_RATE(633000000U,  211, 4,  1),
 	PLL_35XX_RATE(600000000U,  500, 5,  2),
 	PLL_35XX_RATE(552000000U,  460, 5,  2),
@@ -757,12 +757,12 @@  static const struct samsung_pll_rate_table exynos5433_pll_rates[] __initconst =
 /* AUD_PLL */
 static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initconst = {
 	PLL_36XX_RATE(400000000U, 200, 3, 2,      0),
-	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
+	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),
 	PLL_36XX_RATE(384000000U, 128, 2, 2,      0),
-	PLL_36XX_RATE(368640000U, 246, 4, 2, -15729),
-	PLL_36XX_RATE(361507200U, 181, 3, 2, -16148),
-	PLL_36XX_RATE(338688000U, 113, 2, 2,  -6816),
-	PLL_36XX_RATE(294912000U,  98, 1, 3,  19923),
+	PLL_36XX_RATE(368639991U, 246, 4, 2, -15729),
+	PLL_36XX_RATE(361507202U, 181, 3, 2, -16148),
+	PLL_36XX_RATE(338687988U, 113, 2, 2,  -6816),
+	PLL_36XX_RATE(294912002U,  98, 1, 3,  19923),
 	PLL_36XX_RATE(288000000U,  96, 1, 3,      0),
 	PLL_36XX_RATE(252000000U,  84, 1, 3,      0),
 	{ /* sentinel */ }