| Message ID | 20180205142230.9755-1-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sylwester, 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. 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. On 2018년 02월 05일 23:22, Sylwester Nawrocki wrote: > Add more definitions to the exynos5433_aud_pll_rates table so the > AUD_PLL can be used as a root clock source for the I2S1 and the HDMI > interface. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/clk/samsung/clk-exynos5433.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c > index db270908037a..74b70ddab4d6 100644 > --- a/drivers/clk/samsung/clk-exynos5433.c > +++ b/drivers/clk/samsung/clk-exynos5433.c > @@ -765,6 +765,14 @@ static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initcons > PLL_36XX_RATE(294912000U, 98, 1, 3, 19923), > PLL_36XX_RATE(288000000U, 96, 1, 3, 0), > PLL_36XX_RATE(252000000U, 84, 1, 3, 0), > + PLL_36XX_RATE(200000000U, 200, 3, 3, 0), > + PLL_36XX_RATE(196608001U, 197, 3, 3, -25690), > + PLL_36XX_RATE(180633609U, 301, 5, 3, 3671), > + PLL_36XX_RATE(131072006U, 131, 3, 3, 4719), > + PLL_36XX_RATE(100000000U, 200, 3, 4, 0), > + PLL_36XX_RATE(65536003U, 131, 3, 4, 4719), > + PLL_36XX_RATE(49152000U, 197, 3, 5, -25690), > + PLL_36XX_RATE(32768001U, 131, 3, 5, 4719), > { /* sentinel */ } > }; > > -- > 2.14.2 > > >
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). 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), 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. -- Regards, Sylwester
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. >
On 02/07/2018 12:24 PM, Chanwoo Choi wrote: > 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)) It resembles the code from samsung_pll36xx_recalc_rate(): (24 * 10^6 * (M * 2^16 + K)) / (P * 2^S) / 2^16 and a more accurate one ROUNDDOWN(ROUNDDOWN(24 * 10^6 * (M * 2^16 + K), 0) / ROUNDDOWN(P * 2^S, 0) / 2^16, 0) Shouldn't you substitute 65535 with 65536?
On 2018년 02월 07일 22:04, Sylwester Nawrocki wrote: > On 02/07/2018 12:24 PM, Chanwoo Choi wrote: >> 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)) > > It resembles the code from samsung_pll36xx_recalc_rate(): > > (24 * 10^6 * (M * 2^16 + K)) / (P * 2^S) / 2^16 > > and a more accurate one > > ROUNDDOWN(ROUNDDOWN(24 * 10^6 * (M * 2^16 + K), 0) / ROUNDDOWN(P * 2^S, 0) / 2^16, 0) > > Shouldn't you substitute 65535 with 65536? 65536 is right. It is my mistake using 65535. Thanks for your share.
diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c index db270908037a..74b70ddab4d6 100644 --- a/drivers/clk/samsung/clk-exynos5433.c +++ b/drivers/clk/samsung/clk-exynos5433.c @@ -765,6 +765,14 @@ static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initcons PLL_36XX_RATE(294912000U, 98, 1, 3, 19923), PLL_36XX_RATE(288000000U, 96, 1, 3, 0), PLL_36XX_RATE(252000000U, 84, 1, 3, 0), + PLL_36XX_RATE(200000000U, 200, 3, 3, 0), + PLL_36XX_RATE(196608001U, 197, 3, 3, -25690), + PLL_36XX_RATE(180633609U, 301, 5, 3, 3671), + PLL_36XX_RATE(131072006U, 131, 3, 3, 4719), + PLL_36XX_RATE(100000000U, 200, 3, 4, 0), + PLL_36XX_RATE(65536003U, 131, 3, 4, 4719), + PLL_36XX_RATE(49152000U, 197, 3, 5, -25690), + PLL_36XX_RATE(32768001U, 131, 3, 5, 4719), { /* sentinel */ } };
Add more definitions to the exynos5433_aud_pll_rates table so the AUD_PLL can be used as a root clock source for the I2S1 and the HDMI interface. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- drivers/clk/samsung/clk-exynos5433.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.14.2