Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Chanwoo Choi <cw00.choi@samsung.com>]

On 2018년 02월 07일 19:29, Sylwester Nawrocki wrote:
> On 02/06/2018 03:44 AM, Chanwoo Choi wrote:
>> When I developed the clk-exynos5433.c I referred to the following description.
>> TRM specified that "Samsung recommends only the values
>> between 252MH ~ 400MHz in the PMS2460X PMS value" for aud_pll.
> 
> Thanks, I somehow missed it. There is also another sentence pointing to
> a contact person if other values are needed.
> 
>> It looks like that you refer to clk-exynos5420.c driver.
>> But, I'm wondering exynos5433 might not guarantee the additional clock
>> of this patch as the stable clock.
> 
> I took the values from downstream SM-N910C_LL kernel, I would say they
> are well tested and reliable. How about adding just these 3 entries:
> 
> +	PLL_36XX_RATE(196608001U, 197, 3, 3, -25690),
> +	PLL_36XX_RATE(180633609U, 301, 5, 3,   3671),
> +	PLL_36XX_RATE(131072006U, 131, 3, 3,   4719),
> ?
>  
> They are needed for audio sample rates that are multiple of 32000, 44100
> or 48000 Hz. The AUD PLL frequency values which are currently defined
> are not useful for anything except fs 48000 (fpll = 393216000 Hz).

If you referred to SM-N910C released kernel, I agree.

> 
> We could add new PLL rates: 361267218, 262144000, but I'm not convinced
> these would be any better than values used in a shipped product. We would 
> need to confirm below values with the HW team, and I'm not sure what 
> would be the P, M, S, K set for 262144000 frequency.
> 
> 	PLL_36XX_RATE(361267218U, 301, 5, 2,   3671),
> 
> As it turns out, the 393216000 Hz entry needs correction, the actual 
> frequency value from the P, M, S, K equation is 393216003:
> 
> -	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
> +	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),

As you described, 393216003 might be more correct than 393216000,
IMHO, I think that TRM specified 393216000 instead of 393216003,
because the following equation is clean without any decimal point.
- "393216000 / 8192 = 48000"

I have no any objection, if 393216003 is correct result.

Could you share your equation?
because your result is a little bit different of my result.
- my equation : ((mdiv + kdiv/65535) x 24MHz) / (pdiv x POWER(2,sdiv))

> 
> Hmm, I have tested with the PLL frequency set to 393216003 Hz and there
> are some glitches during audio playback, I can't get it to work properly.
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics