Hi Yadwinder,
On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
Hi Tomasz,
On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa t.figa@samsung.com 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@chromium.org
wrote:
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan vikas.sajjan@linaro.org
wrote:
From: Yadwinder Singh Brar yadi.brar@samsung.com
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@samsung.com
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'm not sure what required rates you are talking about. For most PLLs the list of available frequencies seems to be defined by the set of supported PMS values and those are the frequencies that can be set by consumers, not the other way around.
There are some cases when some particular frequency must be generated, like PLLs used for audio or video clock sources, but this just means that there must be an entry in PMS table that gives PLL configuration which produces such frequency with used PLL input clock.
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.
This doesn't answer the question about using those PMS values with different input rate.
Kukjin, maybe you can give some information on this or point to a person who could?
Please let me know, why you think we are specific to current cases only ?
I don't like the idea of having separate tables for each input rate. Just imagine how much data would have to be added if boards with several different input rates showed up.
Sure, if each input rate requires different set of PMS values by hardware design, it can't be helped, but if the same PMS values can be reused (even giving different output rates, but this is usually not important), one table can be used for all cases.
Best regards,