Hi Tomasz, On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote: > Hi Yadwinder, > > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote: >> Hi Doug, >> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@xxxxxxxxxxxx> > wrote: >> > Vikas / Yadwinder, >> > >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@xxxxxxxxxx> > wrote: >> >> From: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx> >> >> >> >> This patch defines a common rate_table which will contain recommended p, >> >> m, s, k values for supported rates that needs to be changed for changing >> >> corresponding PLL's rate. >> >> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx> >> >> --- >> >> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++---- >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++------- >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++-- >> >> drivers/clk/samsung/clk-pll.h | 33 >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15 >> >> deletions(-) >> > >> > I also reviewed this in our gerrit >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize >> > here for the list... >> > >> >> struct clk * __init samsung_clk_register_pll35xx(const char *name, >> >> >> >> - const char *pname, const void __iomem *base) >> >> + const char *pname, const void __iomem *base, >> >> + const struct samsung_pll_rate_table *rate_table, >> >> + const unsigned int rate_count) >> > >> > Feels like you should document here that rate_table needs to be sorted >> > and the sort order. >> >> sure, we will add comment to sort the table in descending order. >> >> >> +struct samsung_pll_rate_table { >> >> + unsigned int rate; >> > >> > nit: extra space before "int" should be removed. >> >> ok >> >> > Also: you can include rate here if you need a convenient place to >> > store it (which sadly means that this structure can't be const). >> > ...but I do like Tomasz's idea of actually calculating it. You can't >> > know it at compile time since the parent rate comes from the device >> > tree. >> > >> > compatible = "samsung,clock-xxti"; >> > clock-frequency = <24000000>; >> >> Actually this table should contain the recommended values >> and if we see the user manual, the input(parent) rate is also a part >> of recommended >> table of different output rate for a particular PLL for that SoC. > > From what I understood in the documentation is that there is a set of > recommended P, M, S (, K) tuples for each PLL and they are not dependent on > input frequency - f_in and f_out are provided in the table just for reference > to see the relation between output frequency and input frequency. > If input rate(f_in) gets changed for PLL, then the corresponding output rates(f_out) will change by using same(common) set of recommended P, M, S (, K), and the new set of output rates(f_out) may not be the _required_ set of target rates. So we need different set of P, M, S (,K) values for different f_in. Table should contain set of P, M, S (,K) values for the _required_ target(f_out) rates for a particular input(f_in) rate. > I think we should ask some H/W engineer about that to make sure and choose the > proper implementation, which will work properly for future cases, instead of > ending with something that works just with current cases. > We already asked hardware engineer about PMS values for PLL, and we got a table containing recommended P, M ,S values for a given f_in(24 MHz) and required f_out rates. Please let me know, why you think we are specific to current cases only ? Regards, Yadwinder > Best regards, > -- > Tomasz Figa > Linux Kernel Developer > Samsung R&D Institute Poland > Samsung Electronics > >> So as Tomasz said input(parent) rate may change with board, >> then, do those corresponding recommended p, m, s, k will be valid? >> >> In case, input(parent) rate changes then we may need different set of p, m >> ,s, k values recommended for new input rate to get required(recommended to >> use) output rate. >> >> So, we think its better that the p, m, s and k along with the parent >> is known at the compile time ( or DT ?), >> as these p, m, s, k values are very much coupled with the parent rate >> to achieve the >> required(recommended to use) output rate. >> >> Also, since the sorted table is required (sorted based on "rate"), >> its better to have the rate in a const rate table. >> >> And the whole set of recommended values should come from same place(DT >> or static table), >> to keep the things simple and consistent. >> >> Moreover, practically for a particular SoC , we use the recommended >> input(parent) rate only for a PLL. >> So we should keep the things simple here presently. >> >> >> + unsigned int pdiv; >> >> + unsigned int mdiv; >> >> + unsigned int sdiv; >> >> + unsigned int kdiv; >> > >> > I think kdiv is signed. >> >> No, as these values should be the recommended values to be written in >> corresponding register bits. So it should remain unsigned. >> >> K value should be considered as negative only while recalculating rate. >> >> As per exynos5250 user manual's section 7.3.2 : >> " K value description "Postive value (Negative value)": >> Postive values is that you should write EPLLCON/VPLLCON register. >> Negative value is that you can calculate PLL output frequency with it." >> >> > -Doug >> >> Regards, >> Yadwinder & Vikas. >> -- >> 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 -- 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
 |