On 2016년 09월 07일 17:27, Sylwester Nawrocki wrote:
> On 09/07/2016 06:14 AM, Chanwoo Choi wrote:
>> On 2016년 09월 06일 21:04, Sylwester Nawrocki wrote:
> ...
>>> -static const struct samsung_pll_clock exynos5410_plls[nr_plls] __initconst = {
>>> +static const struct samsung_pll_rate_table exynos5410_pll2550x_24mhz_tbl[] __initconst = {
>>> + PLL_36XX_RATE(400000000U, 200, 2, 2, 0),
>>
>> In the Exynos5410's TRM, the EPLL PMS table has the
>> 'P' state is 3 instead of 2 when target is 400MHz as following:
>>
>> PLL_36XX_RATE(400000000U, 200, 3, 2, 0),
>
> Indeed, thanks for pointing out.
>
>>> + PLL_36XX_RATE(333000000U, 111, 2, 2, 0),
>>> + PLL_36XX_RATE(300000000U, 100, 2, 2, 0),
>>> + PLL_36XX_RATE(266000000U, 266, 3, 3, 0),
>>> + PLL_36XX_RATE(200000000U, 200, 3, 3, 0),
>>> + PLL_36XX_RATE(192000000U, 192, 3, 3, 0),
>>> + PLL_36XX_RATE(166000000U, 166, 3, 3, 0),
>>> + PLL_36XX_RATE(133000000U, 266, 3, 4, 0),
>>> + PLL_36XX_RATE(100000000U, 200, 3, 4, 0),
>>> + PLL_36XX_RATE(66000000U, 176, 2, 5, 0),
>>
>> The all entry of EPLL PMS table has the zero(0) for 'K' value.
>> How about using the PLL_35XX_RATE() which has the P,M,S entry without K entry?
>
> Don't have a strong opinion on that, I'm inclined to stay with
> PLL_36XX_RATE() though, to indicate the K value is valid for this
> PLL type.
>
>>> @@ -237,6 +258,7 @@ static void __init exynos5410_clk_init(struct device_node *np)
>>> {
>>> struct samsung_clk_provider *ctx;
>>> void __iomem *reg_base;
>>> + struct clk *xxti;
>>>
>>> reg_base = of_iomap(np, 0);
>>> if (!reg_base)
>>> @@ -244,6 +266,10 @@ static void __init exynos5410_clk_init(struct device_node *np)
>>>
>>> ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
>>>
>>> + xxti = of_clk_get(np, 0);
>>> + if (!IS_ERR(xxti) && clk_get_rate(xxti) == 24 * MHZ)
>>> + exynos5410_plls[epll].rate_table = exynos5410_pll2550x_24mhz_tbl;
>>> +
>>
>> I sent the patch to use the samsung_cmu_register_one()
>>
>> and applied it[1]. I think that you need to rebase this patch on patch[1].
>> [1] https://lkml.org/lkml/2016/9/1/602
>> - Re: [PATCH 2/2] clk: samsung: exynos5410: Use samsung_cmu_register_one() to simplify code
>
> Yes, I need to rebase onto latest tree.
>
>>> +/* Maximum lock time can be 3000 * PDIV cycles */
>>> +#define PLL2650X_LOCK_FACTOR 3000
>>> +
>>> +#define PLL2650X_M_MASK 0x3FF
>>
>> 1FF instead of 0x3FF because the MDIV [24:16]?
>
> Right, my bad, I'll correct this.
>
>>> +#define PLL2650X_P_MASK 0x3F
>>> +#define PLL2650X_S_MASK 0x7
>>> +#define PLL2650X_K_MASK 0xFFFF
>>> +#define PLL2650X_LOCK_STAT_MASK 0x1
>>> +#define PLL2650X_M_SHIFT 16
>>> +#define PLL2650X_P_SHIFT 8
>>> +#define PLL2650X_S_SHIFT 0
>>> +#define PLL2650X_K_SHIFT 0
>>> +#define PLL2650X_LOCK_STAT_SHIFT 29
>>> +#define PLL2650X_PLL_ENABLE_SHIFT 31
>>> +
>>> +static unsigned long samsung_pll2650x_recalc_rate(struct clk_hw *hw,
>>> + unsigned long parent_rate)
>>> +{
>>> + struct samsung_clk_pll *pll = to_clk_pll(hw);
>>> + u64 fout = parent_rate;
>>> + u32 mdiv, pdiv, sdiv, pll_con0, pll_con1;
>>> + s16 kdiv;
>>> +
>>> + pll_con0 = readl_relaxed(pll->con_reg);
>>> + mdiv = (pll_con0 >> PLL2650X_M_SHIFT) & PLL2650X_M_MASK;
>>> + pdiv = (pll_con0 >> PLL2650X_P_SHIFT) & PLL2650X_P_MASK;
>>> + sdiv = (pll_con0 >> PLL2650X_S_SHIFT) & PLL2650X_S_MASK;
>>> +
>>> + pll_con1 = readl_relaxed(pll->con_reg + 4);
>>> + kdiv = (s16)((pll_con1 >> PLL2650X_K_SHIFT) & PLL2650X_K_MASK);
>>> +
>>> + fout *= (mdiv << 16) + kdiv;
>>> + do_div(fout, (pdiv << sdiv));
>>> + fout >>= 16;
>>> +
>>> + return (unsigned long)fout;
>>
>> I got some confusion because the EPLL_CON1 register don't include
>> the 'K' value. Instead, the name is 'DSM' which is PLL DSM(Delta-Sigma
>> Modulator).
>> But, I knew that DSM means the 'K' value as your implementation.
>
> Yes, the documentation seems incomplete, the K coefficient is used
> in the PLL output frequency equations and I found that register
> bit field used in some downstream kernel.
> Perhaps this is corrected in newer User Manual version than
> the "Preeliminary" I have.
OK. Thanks.
>
>> I have a question.
>> When I check the calculation fomula as following:
>> FFOUT = ((m+k/65536) x FFIN) / (p x 2^S);
>>
>> So, pll2560x might shift the kdiv instead of mdiv.
>> fout *= mdiv + (kdiv << 16);
>
> First we multiply both sides of the equation by 65536 (2^16):
>
> 65536 * FFOUT = 65536 * ((M + K/65536) x FFIN) / (P x 2^S);
>
> That means M needs to be multiplied by 2^16, not K:
>
> 65536 * FFOUT = (65536 * M + K) x FFIN) / (P x 2^S);
I understand. I was missing to check the "fout >>= 16;" equation.
Thanks for your explanation
>
> K is the "fractional" coefficient here, with least impact on FFOUT.
> Finally we divide fout by 2^16 to reverse previous multiplication.
>
> I tested this code with audio playback, if it had been incorrect
> in such a way the sampling rate would certainly have been incorrect
> and it would have been heard.
>
--
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
