[PATCH v4 0/6] Add generic set_rate clk_ops for PLL35xx and PLL36xx for samsung SoCs

[PATCH v4 0/6] Add generic set_rate clk_ops for PLL35xx and PLL36xx for samsung SoCs

[Yadwinder Singh Brar]

This patch series does the following:

 1) Unifies the clk strutures used for PLL35xx & PLL36xx and usues clk->base
    instead of clk->con0, to factor out possible common code.

 2) Defines a common rate_table which will contain recommended p, m, s and k
    values for supported rates that needs to be changed for changing
    corresponding PLL's rate

 3) Adds set_rate() and round_rate() clk_ops for PLL35xx and PLL36xx

changes since v3:
	- Used __clk_lookup() instead of adding alias for mout_vpllsrc
	- Added check for changing only M value in samsung_pll36xx_set_rate()
	- Modified samsung_pll35xx_mp_change() & samsung_pll35xx_set_rate()
	to improve readabilty.
	- Made the input rate_table as __init_data which is to be provided while
	registering PLL and made a copy of that table while registering, so
	that if multiple tables are their, they can be freed after getting the
	P, M, S, K setting values from required one.

changes since v2:
	- Added new patch to reorder the MUX registration for mout_vpllsrc MUX
	before the PLL registrations. And to add the alias for the mout_vpllsrc MUX.
        - Added a check to confirm parent rate while registrating the PLL
	rate tables.

changes since v1:
	- removed sorting and bsearch
	- modified the definition of struct "samsung_pll_rate_table"
	- added generic round_rate()
	- rectified the ops assignment for "rate table passed as NULL"
	  during PLL registration

Is rebased on branch kgene's "for-next"
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next

Vikas Sajjan (3):
  clk: samsung: Add set_rate() clk_ops for PLL36xx
  clk: samsung: Reorder MUX registration for mout_vpllsrc
  clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC

Yadwinder Singh Brar (3):
  clk: samsung: Use clk->base instead of directly using clk->con0 for
    PLL3xxx
  clk: samsung: Add support to register rate_table for PLL3xxx
  clk: samsung: Add set_rate() clk_ops for PLL35xx

 drivers/clk/samsung/clk-exynos4.c    |   10 +-
 drivers/clk/samsung/clk-exynos5250.c |   73 +++++++++--
 drivers/clk/samsung/clk-pll.c        |  255 ++++++++++++++++++++++++++++++----
 drivers/clk/samsung/clk-pll.h        |   35 +++++-
 drivers/clk/samsung/clk.h            |    2 +
 5 files changed, 332 insertions(+), 43 deletions(-)

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Yadwinder Singh Brar]

This patch unifies clk strutures used for PLL35xx & PLL36xx and uses clk->base
instead of directly using clk->con0, so that possible common code can be
factored out.
It also introdues common pll_[readl/writel] macros for the users of common
samsung_clk_pll struct.

Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/clk/samsung/clk-exynos4.c    |   10 ++++--
 drivers/clk/samsung/clk-exynos5250.c |   14 ++++----
 drivers/clk/samsung/clk-pll.c        |   54 ++++++++++++++++++---------------
 drivers/clk/samsung/clk-pll.h        |    4 +-
 4 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index addc738..ba33bc6 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -97,12 +97,14 @@
 #define GATE_IP_PERIL		0xc950
 #define E4210_GATE_IP_PERIR	0xc960
 #define GATE_BLOCK		0xc970
+#define E4X12_MPLL_LOCK		0x10008
 #define E4X12_MPLL_CON0		0x10108
 #define SRC_DMC			0x10200
 #define SRC_MASK_DMC		0x10300
 #define DIV_DMC0		0x10500
 #define DIV_DMC1		0x10504
 #define GATE_IP_DMC		0x10900
+#define APLL_LOCK		0x14000
 #define APLL_CON0		0x14100
 #define E4210_MPLL_CON0		0x14108
 #define SRC_CPU			0x14200
@@ -1026,13 +1028,13 @@ void __init exynos4_clk_init(struct device_node *np, enum exynos4_soc exynos4_so
 					reg_base + VPLL_CON0, pll_4650c);
 	} else {
 		apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
-					reg_base + APLL_CON0);
+					reg_base + APLL_LOCK);
 		mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-					reg_base + E4X12_MPLL_CON0);
+					reg_base + E4X12_MPLL_LOCK);
 		epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-					reg_base + EPLL_CON0);
+					reg_base + EPLL_LOCK);
 		vpll = samsung_clk_register_pll36xx("fout_vpll", "fin_pll",
-					reg_base + VPLL_CON0);
+					reg_base + VPLL_LOCK);
 	}
 
 	samsung_clk_add_lookup(apll, fout_apll);
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index 5c97e75..687b580 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -491,19 +491,19 @@ void __init exynos5250_clk_init(struct device_node *np)
 			ext_clk_match);
 
 	apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
-			reg_base + 0x100);
+			reg_base);
 	mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-			reg_base + 0x4100);
+			reg_base + 0x4000);
 	bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
-			reg_base + 0x20110);
+			reg_base + 0x20010);
 	gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
-			reg_base + 0x10150);
+			reg_base + 0x10050);
 	cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
-			reg_base + 0x10120);
+			reg_base + 0x10020);
 	epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-			reg_base + 0x10130);
+			reg_base + 0x10030);
 	vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
-			reg_base + 0x10140);
+			reg_base + 0x10040);
 
 	samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
 			ARRAY_SIZE(exynos5250_fixed_rate_clks));
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 89135f6..a7d8ad9 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -13,9 +13,24 @@
 #include "clk.h"
 #include "clk-pll.h"
 
+struct samsung_clk_pll {
+	struct clk_hw		hw;
+	const void __iomem	*base;
+};
+
+#define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
+
+#define pll_readl(pll, offset)						\
+		__raw_readl((void __iomem *)(pll->base + (offset)));
+#define pll_writel(pll, val, offset)					\
+		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
+
 /*
  * PLL35xx Clock Type
  */
+#define PLL35XX_LOCK_OFFSET	(0x0)
+#define PLL35XX_CON0_OFFSET	(0x100)
+#define PLL35XX_CON1_OFFSET	(0x104)
 
 #define PLL35XX_MDIV_MASK       (0x3FF)
 #define PLL35XX_PDIV_MASK       (0x3F)
@@ -24,21 +39,14 @@
 #define PLL35XX_PDIV_SHIFT      (8)
 #define PLL35XX_SDIV_SHIFT      (0)
 
-struct samsung_clk_pll35xx {
-	struct clk_hw		hw;
-	const void __iomem	*con_reg;
-};
-
-#define to_clk_pll35xx(_hw) container_of(_hw, struct samsung_clk_pll35xx, hw)
-
 static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
 {
-	struct samsung_clk_pll35xx *pll = to_clk_pll35xx(hw);
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
 	u32 mdiv, pdiv, sdiv, pll_con;
 	u64 fvco = parent_rate;
 
-	pll_con = __raw_readl(pll->con_reg);
+	pll_con = pll_readl(pll, PLL35XX_CON0_OFFSET);
 	mdiv = (pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK;
 	pdiv = (pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK;
 	sdiv = (pll_con >> PLL35XX_SDIV_SHIFT) & PLL35XX_SDIV_MASK;
@@ -54,9 +62,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops = {
 };
 
 struct clk * __init samsung_clk_register_pll35xx(const char *name,
-			const char *pname, const void __iomem *con_reg)
+			const char *pname, const void __iomem *base)
 {
-	struct samsung_clk_pll35xx *pll;
+	struct samsung_clk_pll *pll;
 	struct clk *clk;
 	struct clk_init_data init;
 
@@ -73,7 +81,7 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 	init.num_parents = 1;
 
 	pll->hw.init = &init;
-	pll->con_reg = con_reg;
+	pll->base = base;
 
 	clk = clk_register(NULL, &pll->hw);
 	if (IS_ERR(clk)) {
@@ -91,6 +99,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 /*
  * PLL36xx Clock Type
  */
+#define PLL36XX_LOCK_OFFSET	(0x0)
+#define PLL36XX_CON0_OFFSET	(0x100)
+#define PLL36XX_CON1_OFFSET	(0x104)
 
 #define PLL36XX_KDIV_MASK	(0xFFFF)
 #define PLL36XX_MDIV_MASK	(0x1FF)
@@ -100,22 +111,15 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 #define PLL36XX_PDIV_SHIFT	(8)
 #define PLL36XX_SDIV_SHIFT	(0)
 
-struct samsung_clk_pll36xx {
-	struct clk_hw		hw;
-	const void __iomem	*con_reg;
-};
-
-#define to_clk_pll36xx(_hw) container_of(_hw, struct samsung_clk_pll36xx, hw)
-
 static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
 {
-	struct samsung_clk_pll36xx *pll = to_clk_pll36xx(hw);
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
 	u32 mdiv, pdiv, sdiv, kdiv, pll_con0, pll_con1;
 	u64 fvco = parent_rate;
 
-	pll_con0 = __raw_readl(pll->con_reg);
-	pll_con1 = __raw_readl(pll->con_reg + 4);
+	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
 	mdiv = (pll_con0 >> PLL36XX_MDIV_SHIFT) & PLL36XX_MDIV_MASK;
 	pdiv = (pll_con0 >> PLL36XX_PDIV_SHIFT) & PLL36XX_PDIV_MASK;
 	sdiv = (pll_con0 >> PLL36XX_SDIV_SHIFT) & PLL36XX_SDIV_MASK;
@@ -133,9 +137,9 @@ static const struct clk_ops samsung_pll36xx_clk_ops = {
 };
 
 struct clk * __init samsung_clk_register_pll36xx(const char *name,
-			const char *pname, const void __iomem *con_reg)
+			const char *pname, const void __iomem *base)
 {
-	struct samsung_clk_pll36xx *pll;
+	struct samsung_clk_pll *pll;
 	struct clk *clk;
 	struct clk_init_data init;
 
@@ -152,7 +156,7 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 	init.num_parents = 1;
 
 	pll->hw.init = &init;
-	pll->con_reg = con_reg;
+	pll->base = base;
 
 	clk = clk_register(NULL, &pll->hw);
 	if (IS_ERR(clk)) {
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index f33786e..1329522 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -25,9 +25,9 @@ enum pll46xx_type {
 };
 
 extern struct clk * __init samsung_clk_register_pll35xx(const char *name,
-			const char *pname, const void __iomem *con_reg);
+			const char *pname, const void __iomem *base);
 extern struct clk * __init samsung_clk_register_pll36xx(const char *name,
-			const char *pname, const void __iomem *con_reg);
+			const char *pname, const void __iomem *base);
 extern struct clk * __init samsung_clk_register_pll45xx(const char *name,
 			const char *pname, const void __iomem *con_reg,
 			enum pll45xx_type type);
-- 
1.7.0.4

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Doug Anderson]

Yadwinder,

On Mon, Jun 3, 2013 at 8:09 AM, Yadwinder Singh Brar
<yadi.brar@samsung.com> wrote:
> This patch unifies clk strutures used for PLL35xx & PLL36xx and uses clk->base
> instead of directly using clk->con0, so that possible common code can be
> factored out.
> It also introdues common pll_[readl/writel] macros for the users of common
> samsung_clk_pll struct.
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c    |   10 ++++--
>  drivers/clk/samsung/clk-exynos5250.c |   14 ++++----
>  drivers/clk/samsung/clk-pll.c        |   54 ++++++++++++++++++---------------
>  drivers/clk/samsung/clk-pll.h        |    4 +-
>  4 files changed, 44 insertions(+), 38 deletions(-)

So.  We just found that this type of solution doesn't work on
exynos5420, since the LOCK and CON registers aren't always 0x100 away
from each other.  Perhaps you can adjust to use a solution like Andrew
proposed in <https://gerrit.chromium.org/gerrit/#/c/58411/>?  That way
we can avoid some churn of changing this code twice.

The number of parameters to the register PLL function is starting to
get unwieldy.  At some point we'll probably want to pass in a
structure.  I wonder if now would be the time?  Certainly it would be
easier to handle changes to the code without touching all of the
exynos variants...

Thanks!

-Doug

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Doug Anderson]

Yadwinder,

On Wed, Jun 12, 2013 at 1:33 PM, Doug Anderson <dianders@chromium.org> wrote:

> So.  We just found that this type of solution doesn't work on
> exynos5420, since the LOCK and CON registers aren't always 0x100 away
> from each other.  Perhaps you can adjust to use a solution like Andrew
> proposed in <https://gerrit.chromium.org/gerrit/#/c/58411/>?  That way
> we can avoid some churn of changing this code twice.
>
> The number of parameters to the register PLL function is starting to
> get unwieldy.  At some point we'll probably want to pass in a
> structure.  I wonder if now would be the time?  Certainly it would be
> easier to handle changes to the code without touching all of the
> exynos variants...

It's also probably wise to preemptively rebase atop
<https://patchwork.kernel.org/patch/2704761/> since that looks like it
will land in 3.10 and your series is destined for the release after.

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Tomasz Figa]

Hi,

On Wednesday 12 of June 2013 13:33:50 Doug Anderson wrote:
> Yadwinder,
> 
> On Mon, Jun 3, 2013 at 8:09 AM, Yadwinder Singh Brar
> 
> <yadi.brar@samsung.com> wrote:
> > This patch unifies clk strutures used for PLL35xx & PLL36xx and uses
> > clk->base instead of directly using clk->con0, so that possible
> > common code can be factored out.
> > It also introdues common pll_[readl/writel] macros for the users of
> > common samsung_clk_pll struct.
> > 
> > Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> > Tested-by: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> > ---
> > 
> >  drivers/clk/samsung/clk-exynos4.c    |   10 ++++--
> >  drivers/clk/samsung/clk-exynos5250.c |   14 ++++----
> >  drivers/clk/samsung/clk-pll.c        |   54
> >  ++++++++++++++++++--------------- drivers/clk/samsung/clk-pll.h     
> >    |    4 +-
> >  4 files changed, 44 insertions(+), 38 deletions(-)
> 
> So.  We just found that this type of solution doesn't work on
> exynos5420, since the LOCK and CON registers aren't always 0x100 away
> from each other.

Oops, this is what I've been afraid of, ever since we assumed this first 
time in our internal patches.

> Perhaps you can adjust to use a solution like Andrew
> proposed in <https://gerrit.chromium.org/gerrit/#/c/58411/>?  That way
> we can avoid some churn of changing this code twice.
> 
> The number of parameters to the register PLL function is starting to
> get unwieldy.  At some point we'll probably want to pass in a
> structure.  I wonder if now would be the time?  Certainly it would be
> easier to handle changes to the code without touching all of the
> exynos variants...

Hmm, if done properly, it could simplify PLL registration in SoC clock 
initialization code a lot.

I'm not sure if this is really the best solution (feel free to suggest 
anything better), but we could put PLLs in an array, like other clocks, 
e.g.

	... exynos4210_pll_clks[] = {
		CLK_PLL45XX(...),
		CLK_PLL45XX(...),
		CLK_PLL46XX(...),
		CLK_PLL46XX(...),
	};

and then just call a helper like

	samsung_clk_register_pll(exynos4210_pll_clks,
			ARRAY_SIZE(exynos4210_pll_clks));

Best regards,
Tomasz

> Thanks!
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Doug Anderson]

Tomasz,

On Wed, Jun 12, 2013 at 2:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hmm, if done properly, it could simplify PLL registration in SoC clock
> initialization code a lot.
>
> I'm not sure if this is really the best solution (feel free to suggest
> anything better), but we could put PLLs in an array, like other clocks,
> e.g.
>
>         ... exynos4210_pll_clks[] = {
>                 CLK_PLL45XX(...),
>                 CLK_PLL45XX(...),
>                 CLK_PLL46XX(...),
>                 CLK_PLL46XX(...),
>         };
>
> and then just call a helper like
>
>         samsung_clk_register_pll(exynos4210_pll_clks,
>                         ARRAY_SIZE(exynos4210_pll_clks));

Something like that looks like what I was thinking.  I'd have to see
it actually coded up to see if there's something I'm missing that
would prevent us from doing that, but I don't see anything.

-Doug

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Andrew Bresticker]

Doug,

>> Hmm, if done properly, it could simplify PLL registration in SoC clock
>> initialization code a lot.
>>
>> I'm not sure if this is really the best solution (feel free to suggest
>> anything better), but we could put PLLs in an array, like other clocks,
>> e.g.
>>
>>         ... exynos4210_pll_clks[] = {
>>                 CLK_PLL45XX(...),
>>                 CLK_PLL45XX(...),
>>                 CLK_PLL46XX(...),
>>                 CLK_PLL46XX(...),
>>         };
>>
>> and then just call a helper like
>>
>>         samsung_clk_register_pll(exynos4210_pll_clks,
>>                         ARRAY_SIZE(exynos4210_pll_clks));
>
> Something like that looks like what I was thinking.  I'd have to see
> it actually coded up to see if there's something I'm missing that
> would prevent us from doing that, but I don't see anything.

The only issue I see with this is that we may only want to register a
rate table with a PLL only if fin_pll is running at a certain rate.
On 5250 and 5420, for example, we have EPLL and VPLL rate tables that
should only be registered if fin_pll is 24Mhz.  We may have to
register those separately, but this approach seems fine otherwise.

-Andrew

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Yadwinder Singh Brar]

On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
<abrestic@chromium.org> wrote:
> Doug,
>
>>> Hmm, if done properly, it could simplify PLL registration in SoC clock
>>> initialization code a lot.
>>>
>>> I'm not sure if this is really the best solution (feel free to suggest
>>> anything better), but we could put PLLs in an array, like other clocks,
>>> e.g.
>>>
>>>         ... exynos4210_pll_clks[] = {
>>>                 CLK_PLL45XX(...),
>>>                 CLK_PLL45XX(...),
>>>                 CLK_PLL46XX(...),
>>>                 CLK_PLL46XX(...),
>>>         };
>>>
>>> and then just call a helper like
>>>
>>>         samsung_clk_register_pll(exynos4210_pll_clks,
>>>                         ARRAY_SIZE(exynos4210_pll_clks));
>>
>> Something like that looks like what I was thinking.  I'd have to see
>> it actually coded up to see if there's something I'm missing that
>> would prevent us from doing that, but I don't see anything.
>
> The only issue I see with this is that we may only want to register a
> rate table with a PLL only if fin_pll is running at a certain rate.
> On 5250 and 5420, for example, we have EPLL and VPLL rate tables that
> should only be registered if fin_pll is 24Mhz.  We may have to
> register those separately, but this approach seems fine otherwise.
>

As Andrew Bresticker said, we will face problem with different table,
and it will give some pain while handling such cases but I think
overall code may look better.

Similar thoughts were their in my mind also, but i didn't want to
disturb this series :).
Anyways, I think we can do it now only rather going for incremental
patches after this series.
I was thinking to make samsung_clk_register_pllxxxx itself  little
generic instead
of using helper, as we are almost duplicating code for most PLLs.

A rough picture in my mind was,
After implementing  generic samung_clk_register_pll(), code may look like
below. Its just an idea, please feel free to correct it.
Later we can factor out other common clk.ops for PLLs also.

this diff is over this series.
Assuming a generic samung_clk_register_pll() is their(which i think is
not impossible)
8<--------------------------------------------------------------------------

--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
epll_24mhz_tbl[] = {
        PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
 };

+struct __initdata samsung_pll_init_data samsung_plls[] = {
+       PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0, NULL),
+       PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK, MPLL_CON0, NULL),
+       PLL(pll_3500, "fout_bpll", "fin_pll",BPLL_LOCK, BPLL_CON0, NULL),
+       PLL(pll_3500, "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL),
+       PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL),
+};
+
+struct __initdata samsung_pll_init_data epll_init_data =
+       PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0, NULL);
+
+struct __initdata samsung_pll_init_data vpll_init_data =
+       PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0, NULL);
+
 /* register exynox5250 clocks */
 void __init exynos5250_clk_init(struct device_node *np)
 {
@@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct device_node *np)
        samsung_clk_register_mux(exynos5250_pll_pmux_clks,
                        ARRAY_SIZE(exynos5250_pll_pmux_clks));

-       fin_pll_rate = _get_rate("fin_pll");
+       samsung_clk_register_pll(samsung_plls, ARRAY_SIZE(samsung_plls));
+
        vpllsrc = __clk_lookup("mout_vpllsrc");
        if (vpllsrc)
                mout_vpllsrc_rate = clk_get_rate(vpllsrc);

-       apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
-                       reg_base, NULL, 0);
-       mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-                       reg_base + 0x4000, NULL, 0);
-       bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
-                       reg_base + 0x20010, NULL, 0);
-       gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
-                       reg_base + 0x10050, NULL, 0);
-       cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
-                       reg_base + 0x10020, NULL, 0);
-
+       fin_pll_rate = _get_rate("fin_pll");
        if (fin_pll_rate == (24 * MHZ)) {
-               epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-                               reg_base + 0x10030, epll_24mhz_tbl,
-                               ARRAY_SIZE(epll_24mhz_tbl));
-       } else {
-               pr_warn("%s: valid epll rate_table missing for\n"
-                       "parent fin_pll:%lu hz\n", __func__, fin_pll_rate);
-               epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-                               reg_base + 0x10030, NULL, 0);
+               epll_init_data.rate_table =  epll_24mhz_tb;
        }
+       samsung_clk_register_pll(&fout_epll_data, 1);

        if (mout_vpllsrc_rate == (24 * MHZ)) {
-               vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc"
-                       , reg_base + 0x10040, vpll_24mhz_tbl,
-                       ARRAY_SIZE(vpll_24mhz_tbl));
-       } else {
-               pr_warn("%s: valid vpll rate_table missing for\n"
-                       "parent mout_vpllsrc_rate:%lu hz\n", __func__,
-                       mout_vpllsrc_rate);
-               samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
-                       reg_base + 0x10040, NULL, 0);
+               vpll_init_data.rate_table =  epll_24mhz_tb;
        }
+       samsung_clk_register_pll(&fout_epll_data, 1);
        samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
                        ARRAY_SIZE(exynos5250_fixed_rate_clks));
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index 4780e6c..3b02dc5 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
        unsigned int kdiv;
 };

+#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)    \
+       {                                                               \
+               .type   =       _type,                                  \
+               .name   =       _name,                                  \
+               .parent_name =  _pname,                                 \
+               .flags  =       CLK_GET_RATE_NOCACHE,                   \
+               .rate_table =   _rate_table,                            \
+               .con_reg =      _con_reg,                               \
+               .lock_reg =     _lock_reg,                              \
+       }
+
+enum samsung_pll_type {
+       pll_3500,
+       pll_45xx,
+       pll_2550,
+       pll_3600,
+       pll_46xx,
+       pll_2660,
+};
+
+
+struct samsung_pll_init_data {
+       enum            samsung_pll_type type;
+       struct          samsung_pll_rate_table *rate_table;
+       const char      *name;
+       const char      *parent_name;
+       unsigned long   flags;
+       const void __iomem *con_reg;
+       const void __iomem *lock_reg;
+};
+
+extern int  __init samsung_clk_register_pll(struct samsung_pll_init_data *data,
+                               unsigned int nr_pll);
+

Regards,
Yadwinder

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Tomasz Figa]

On Thursday 13 of June 2013 12:32:05 Yadwinder Singh Brar wrote:
> On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
> 
> <abrestic@chromium.org> wrote:
> > Doug,
> > 
> >>> Hmm, if done properly, it could simplify PLL registration in SoC
> >>> clock
> >>> initialization code a lot.
> >>> 
> >>> I'm not sure if this is really the best solution (feel free to
> >>> suggest
> >>> anything better), but we could put PLLs in an array, like other
> >>> clocks,
> >>> e.g.
> >>> 
> >>>         ... exynos4210_pll_clks[] = {
> >>>         
> >>>                 CLK_PLL45XX(...),
> >>>                 CLK_PLL45XX(...),
> >>>                 CLK_PLL46XX(...),
> >>>                 CLK_PLL46XX(...),
> >>>         
> >>>         };
> >>> 
> >>> and then just call a helper like
> >>> 
> >>>         samsung_clk_register_pll(exynos4210_pll_clks,
> >>>         
> >>>                         ARRAY_SIZE(exynos4210_pll_clks));
> >> 
> >> Something like that looks like what I was thinking.  I'd have to see
> >> it actually coded up to see if there's something I'm missing that
> >> would prevent us from doing that, but I don't see anything.
> > 
> > The only issue I see with this is that we may only want to register a
> > rate table with a PLL only if fin_pll is running at a certain rate.
> > On 5250 and 5420, for example, we have EPLL and VPLL rate tables that
> > should only be registered if fin_pll is 24Mhz.  We may have to
> > register those separately, but this approach seems fine otherwise.
> 
> As Andrew Bresticker said, we will face problem with different table,
> and it will give some pain while handling such cases but I think
> overall code may look better.
> 
> Similar thoughts were their in my mind also, but i didn't want to
> disturb this series :).

Yes, I was thinking the same as well, but now that Exynos5420 doesn't 
follow the 0x100 register spacing, we have a problem :) .

> Anyways, I think we can do it now only rather going for incremental
> patches after this series.
> I was thinking to make samsung_clk_register_pllxxxx itself  little
> generic instead
> of using helper, as we are almost duplicating code for most PLLs.
> 
> A rough picture in my mind was,
> After implementing  generic samung_clk_register_pll(), code may look
> like below. Its just an idea, please feel free to correct it.
> Later we can factor out other common clk.ops for PLLs also.
> 
> this diff is over this series.
> Assuming a generic samung_clk_register_pll() is their(which i think is
> not impossible)
> 8<----------------------------------------------------------------------
> ----
> 
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
> epll_24mhz_tbl[] = {
>         PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
>  };
> 
> +struct __initdata samsung_pll_init_data samsung_plls[] = {
> +       PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0,
> NULL), +       PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK,
> MPLL_CON0, NULL), +       PLL(pll_3500, "fout_bpll",
> "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), +       PLL(pll_3500,
> "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), +      
> PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +};
> +
> +struct __initdata samsung_pll_init_data epll_init_data =
> +       PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0,
> NULL); +
> +struct __initdata samsung_pll_init_data vpll_init_data =
> +       PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0,
> NULL); +

This is mostly what I had in my mind. In addition I think I might have a 
solution for rate tables:

If we create another array

	struct samsung_pll_rate_table *rate_tables_24mhz[] = {
		apll_rate_table_24mhz,
		mpll_rate_table_24mhz, /* can be NULL as well, if no 
support for rate change */
		epll_rate_table_24mhz,
		vpll_rate_table_24mhz,
		/* ... */
	};

which lists rate tables for given input frequency. This relies on making 
rate tables end with a sentinel, to remove the need of passing array 
sizes.

>  /* register exynox5250 clocks */
>  void __init exynos5250_clk_init(struct device_node *np)
>  {
> @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct device_node
> *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks,
>                         ARRAY_SIZE(exynos5250_pll_pmux_clks));
> 
> -       fin_pll_rate = _get_rate("fin_pll");
> +       samsung_clk_register_pll(samsung_plls,
> ARRAY_SIZE(samsung_plls)); +

...and then pass it here like:

	if (fin_pll_rate == 24 * MHZ) {
		samsung_clk_register_pll(samsung_plls, 
ARRAY_SIZE(samsung_plls), rate_tables_24mhz);
	} else {
		/* warning or whatever */
		samsung_clk_register_pll(samsung_plls, 
ARRAY_SIZE(samsung_plls), NULL);
	}

This way we could specify different rate tables depending on input 
frequency for a whole group of PLLs.

The only thing I don't like here is having two separate arrays that need 
to have the same sizes. Feel free to improve (or discard) this idea, 
though.

Best regards,
Tomasz

>         vpllsrc = __clk_lookup("mout_vpllsrc");
>         if (vpllsrc)
>                 mout_vpllsrc_rate = clk_get_rate(vpllsrc);
> 
> -       apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> -                       reg_base, NULL, 0);
> -       mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> -                       reg_base + 0x4000, NULL, 0);
> -       bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
> -                       reg_base + 0x20010, NULL, 0);
> -       gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
> -                       reg_base + 0x10050, NULL, 0);
> -       cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
> -                       reg_base + 0x10020, NULL, 0);
> -
> +       fin_pll_rate = _get_rate("fin_pll");
>         if (fin_pll_rate == (24 * MHZ)) {
> -               epll = samsung_clk_register_pll36xx("fout_epll",
> "fin_pll", -                               reg_base + 0x10030,
> epll_24mhz_tbl, -                              
> ARRAY_SIZE(epll_24mhz_tbl));
> -       } else {
> -               pr_warn("%s: valid epll rate_table missing for\n"
> -                       "parent fin_pll:%lu hz\n", __func__,
> fin_pll_rate); -               epll =
> samsung_clk_register_pll36xx("fout_epll", "fin_pll", -                 
>              reg_base + 0x10030, NULL, 0);
> +               epll_init_data.rate_table =  epll_24mhz_tb;
>         }
> +       samsung_clk_register_pll(&fout_epll_data, 1);
> 
>         if (mout_vpllsrc_rate == (24 * MHZ)) {
> -               vpll = samsung_clk_register_pll36xx("fout_vpll",
> "mout_vpllsrc" -                       , reg_base + 0x10040,
> vpll_24mhz_tbl,
> -                       ARRAY_SIZE(vpll_24mhz_tbl));
> -       } else {
> -               pr_warn("%s: valid vpll rate_table missing for\n"
> -                       "parent mout_vpllsrc_rate:%lu hz\n", __func__,
> -                       mout_vpllsrc_rate);
> -               samsung_clk_register_pll36xx("fout_vpll",
> "mout_vpllsrc", -                       reg_base + 0x10040, NULL, 0);
> +               vpll_init_data.rate_table =  epll_24mhz_tb;
>         }
> +       samsung_clk_register_pll(&fout_epll_data, 1);
>         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
>                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
> diff --git a/drivers/clk/samsung/clk-pll.h
> b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
>         unsigned int kdiv;
>  };
> 
> +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)   
> \ +       {                                                            
>   \ +               .type   =       _type,                             
>     \ +               .name   =       _name,                           
>       \ +               .parent_name =  _pname,                        
>         \ +               .flags  =       CLK_GET_RATE_NOCACHE,        
>           \ +               .rate_table =   _rate_table,               
>             \ +               .con_reg =      _con_reg,                
>               \ +               .lock_reg =     _lock_reg,             
>                 \ +       }
> +
> +enum samsung_pll_type {
> +       pll_3500,
> +       pll_45xx,
> +       pll_2550,
> +       pll_3600,
> +       pll_46xx,
> +       pll_2660,
> +};
> +
> +
> +struct samsung_pll_init_data {
> +       enum            samsung_pll_type type;
> +       struct          samsung_pll_rate_table *rate_table;
> +       const char      *name;
> +       const char      *parent_name;
> +       unsigned long   flags;
> +       const void __iomem *con_reg;
> +       const void __iomem *lock_reg;
> +};
> +
> +extern int  __init samsung_clk_register_pll(struct
> samsung_pll_init_data *data, +                               unsigned
> int nr_pll);
> +
> 
> Regards,
> Yadwinder

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Yadwinder Singh Brar]

On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Thursday 13 of June 2013 12:32:05 Yadwinder Singh Brar wrote:
>> On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
>>
>> <abrestic@chromium.org> wrote:
>> > Doug,
>> >
>> >>> Hmm, if done properly, it could simplify PLL registration in SoC
>> >>> clock
>> >>> initialization code a lot.
>> >>>
>> >>> I'm not sure if this is really the best solution (feel free to
>> >>> suggest
>> >>> anything better), but we could put PLLs in an array, like other
>> >>> clocks,
>> >>> e.g.
>> >>>
>> >>>         ... exynos4210_pll_clks[] = {
>> >>>
>> >>>                 CLK_PLL45XX(...),
>> >>>                 CLK_PLL45XX(...),
>> >>>                 CLK_PLL46XX(...),
>> >>>                 CLK_PLL46XX(...),
>> >>>
>> >>>         };
>> >>>
>> >>> and then just call a helper like
>> >>>
>> >>>         samsung_clk_register_pll(exynos4210_pll_clks,
>> >>>
>> >>>                         ARRAY_SIZE(exynos4210_pll_clks));
>> >>
>> >> Something like that looks like what I was thinking.  I'd have to see
>> >> it actually coded up to see if there's something I'm missing that
>> >> would prevent us from doing that, but I don't see anything.
>> >
>> > The only issue I see with this is that we may only want to register a
>> > rate table with a PLL only if fin_pll is running at a certain rate.
>> > On 5250 and 5420, for example, we have EPLL and VPLL rate tables that
>> > should only be registered if fin_pll is 24Mhz.  We may have to
>> > register those separately, but this approach seems fine otherwise.
>>
>> As Andrew Bresticker said, we will face problem with different table,
>> and it will give some pain while handling such cases but I think
>> overall code may look better.
>>
>> Similar thoughts were their in my mind also, but i didn't want to
>> disturb this series :).
>
> Yes, I was thinking the same as well, but now that Exynos5420 doesn't
> follow the 0x100 register spacing, we have a problem :) .
>
>> Anyways, I think we can do it now only rather going for incremental
>> patches after this series.
>> I was thinking to make samsung_clk_register_pllxxxx itself  little
>> generic instead
>> of using helper, as we are almost duplicating code for most PLLs.
>>
>> A rough picture in my mind was,
>> After implementing  generic samung_clk_register_pll(), code may look
>> like below. Its just an idea, please feel free to correct it.
>> Later we can factor out other common clk.ops for PLLs also.
>>
>> this diff is over this series.
>> Assuming a generic samung_clk_register_pll() is their(which i think is
>> not impossible)
>> 8<----------------------------------------------------------------------
>> ----
>>
>> --- a/drivers/clk/samsung/clk-exynos5250.c
>> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
>> epll_24mhz_tbl[] = {
>>         PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
>>  };
>>
>> +struct __initdata samsung_pll_init_data samsung_plls[] = {
>> +       PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0,
>> NULL), +       PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK,
>> MPLL_CON0, NULL), +       PLL(pll_3500, "fout_bpll",
>> "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), +       PLL(pll_3500,
>> "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), +
>> PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +};
>> +
>> +struct __initdata samsung_pll_init_data epll_init_data =
>> +       PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0,
>> NULL); +
>> +struct __initdata samsung_pll_init_data vpll_init_data =
>> +       PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0,
>> NULL); +
>
> This is mostly what I had in my mind. In addition I think I might have a
> solution for rate tables:
>
> If we create another array
>
>         struct samsung_pll_rate_table *rate_tables_24mhz[] = {
>                 apll_rate_table_24mhz,
>                 mpll_rate_table_24mhz, /* can be NULL as well, if no
> support for rate change */
>                 epll_rate_table_24mhz,
>                 vpll_rate_table_24mhz,
>                 /* ... */
>         };
>
> which lists rate tables for given input frequency. This relies on making
> rate tables end with a sentinel, to remove the need of passing array
> sizes.
>

I think we may also have to make assumption that entries in the arrays
rate_tables_24mhz[] and samsung_plls[] should be in same order in
both arrays, and which may not be fair assumption, otherwise we
have to use some mechanism to identify which rate_table is for which PLL,
which will increase code and complexity.
Am I missed something or you are thinking something else?

Any thoughts from Doug or others ?

>>  /* register exynox5250 clocks */
>>  void __init exynos5250_clk_init(struct device_node *np)
>>  {
>> @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct device_node
>> *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks,
>>                         ARRAY_SIZE(exynos5250_pll_pmux_clks));
>>
>> -       fin_pll_rate = _get_rate("fin_pll");
>> +       samsung_clk_register_pll(samsung_plls,
>> ARRAY_SIZE(samsung_plls)); +
>
> ...and then pass it here like:
>
>         if (fin_pll_rate == 24 * MHZ) {
>                 samsung_clk_register_pll(samsung_plls,
> ARRAY_SIZE(samsung_plls), rate_tables_24mhz);
>         } else {
>                 /* warning or whatever */
>                 samsung_clk_register_pll(samsung_plls,
> ARRAY_SIZE(samsung_plls), NULL);
>         }
>
> This way we could specify different rate tables depending on input
> frequency for a whole group of PLLs.
>

I think code lines or complexity will be same.
Also all PLLs may not have same parent.

> The only thing I don't like here is having two separate arrays that need
> to have the same sizes. Feel free to improve (or discard) this idea,
> though.
>

Sorry, I didn't get your point. You are talking about which two arrays?

Regards,
Yadwinder

> Best regards,
> Tomasz
>
>>         vpllsrc = __clk_lookup("mout_vpllsrc");
>>         if (vpllsrc)
>>                 mout_vpllsrc_rate = clk_get_rate(vpllsrc);
>>
>> -       apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
>> -                       reg_base, NULL, 0);
>> -       mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
>> -                       reg_base + 0x4000, NULL, 0);
>> -       bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
>> -                       reg_base + 0x20010, NULL, 0);
>> -       gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
>> -                       reg_base + 0x10050, NULL, 0);
>> -       cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
>> -                       reg_base + 0x10020, NULL, 0);
>> -
>> +       fin_pll_rate = _get_rate("fin_pll");
>>         if (fin_pll_rate == (24 * MHZ)) {
>> -               epll = samsung_clk_register_pll36xx("fout_epll",
>> "fin_pll", -                               reg_base + 0x10030,
>> epll_24mhz_tbl, -
>> ARRAY_SIZE(epll_24mhz_tbl));
>> -       } else {
>> -               pr_warn("%s: valid epll rate_table missing for\n"
>> -                       "parent fin_pll:%lu hz\n", __func__,
>> fin_pll_rate); -               epll =
>> samsung_clk_register_pll36xx("fout_epll", "fin_pll", -
>>              reg_base + 0x10030, NULL, 0);
>> +               epll_init_data.rate_table =  epll_24mhz_tb;
>>         }
>> +       samsung_clk_register_pll(&fout_epll_data, 1);
>>
>>         if (mout_vpllsrc_rate == (24 * MHZ)) {
>> -               vpll = samsung_clk_register_pll36xx("fout_vpll",
>> "mout_vpllsrc" -                       , reg_base + 0x10040,
>> vpll_24mhz_tbl,
>> -                       ARRAY_SIZE(vpll_24mhz_tbl));
>> -       } else {
>> -               pr_warn("%s: valid vpll rate_table missing for\n"
>> -                       "parent mout_vpllsrc_rate:%lu hz\n", __func__,
>> -                       mout_vpllsrc_rate);
>> -               samsung_clk_register_pll36xx("fout_vpll",
>> "mout_vpllsrc", -                       reg_base + 0x10040, NULL, 0);
>> +               vpll_init_data.rate_table =  epll_24mhz_tb;
>>         }
>> +       samsung_clk_register_pll(&fout_epll_data, 1);
>>         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
>>                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
>> diff --git a/drivers/clk/samsung/clk-pll.h
>> b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644
>> --- a/drivers/clk/samsung/clk-pll.h
>> +++ b/drivers/clk/samsung/clk-pll.h
>> @@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
>>         unsigned int kdiv;
>>  };
>>
>> +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)
>> \ +       {
>>   \ +               .type   =       _type,
>>     \ +               .name   =       _name,
>>       \ +               .parent_name =  _pname,
>>         \ +               .flags  =       CLK_GET_RATE_NOCACHE,
>>           \ +               .rate_table =   _rate_table,
>>             \ +               .con_reg =      _con_reg,
>>               \ +               .lock_reg =     _lock_reg,
>>                 \ +       }
>> +
>> +enum samsung_pll_type {
>> +       pll_3500,
>> +       pll_45xx,
>> +       pll_2550,
>> +       pll_3600,
>> +       pll_46xx,
>> +       pll_2660,
>> +};
>> +
>> +
>> +struct samsung_pll_init_data {
>> +       enum            samsung_pll_type type;
>> +       struct          samsung_pll_rate_table *rate_table;
>> +       const char      *name;
>> +       const char      *parent_name;
>> +       unsigned long   flags;
>> +       const void __iomem *con_reg;
>> +       const void __iomem *lock_reg;
>> +};
>> +
>> +extern int  __init samsung_clk_register_pll(struct
>> samsung_pll_init_data *data, +                               unsigned
>> int nr_pll);
>> +
>>
>> Regards,
>> Yadwinder

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Tomasz Figa]

On Friday 14 of June 2013 00:05:31 Yadwinder Singh Brar wrote:
> On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > On Thursday 13 of June 2013 12:32:05 Yadwinder Singh Brar wrote:
> >> On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
> >> 
> >> <abrestic@chromium.org> wrote:
> >> > Doug,
> >> > 
> >> >>> Hmm, if done properly, it could simplify PLL registration in SoC
> >> >>> clock
> >> >>> initialization code a lot.
> >> >>> 
> >> >>> I'm not sure if this is really the best solution (feel free to
> >> >>> suggest
> >> >>> anything better), but we could put PLLs in an array, like other
> >> >>> clocks,
> >> >>> e.g.
> >> >>> 
> >> >>>         ... exynos4210_pll_clks[] = {
> >> >>>         
> >> >>>                 CLK_PLL45XX(...),
> >> >>>                 CLK_PLL45XX(...),
> >> >>>                 CLK_PLL46XX(...),
> >> >>>                 CLK_PLL46XX(...),
> >> >>>         
> >> >>>         };
> >> >>> 
> >> >>> and then just call a helper like
> >> >>> 
> >> >>>         samsung_clk_register_pll(exynos4210_pll_clks,
> >> >>>         
> >> >>>                         ARRAY_SIZE(exynos4210_pll_clks));
> >> >> 
> >> >> Something like that looks like what I was thinking.  I'd have to
> >> >> see
> >> >> it actually coded up to see if there's something I'm missing that
> >> >> would prevent us from doing that, but I don't see anything.
> >> > 
> >> > The only issue I see with this is that we may only want to register
> >> > a
> >> > rate table with a PLL only if fin_pll is running at a certain rate.
> >> > On 5250 and 5420, for example, we have EPLL and VPLL rate tables
> >> > that
> >> > should only be registered if fin_pll is 24Mhz.  We may have to
> >> > register those separately, but this approach seems fine otherwise.
> >> 
> >> As Andrew Bresticker said, we will face problem with different table,
> >> and it will give some pain while handling such cases but I think
> >> overall code may look better.
> >> 
> >> Similar thoughts were their in my mind also, but i didn't want to
> >> disturb this series :).
> > 
> > Yes, I was thinking the same as well, but now that Exynos5420 doesn't
> > follow the 0x100 register spacing, we have a problem :) .
> > 
> >> Anyways, I think we can do it now only rather going for incremental
> >> patches after this series.
> >> I was thinking to make samsung_clk_register_pllxxxx itself  little
> >> generic instead
> >> of using helper, as we are almost duplicating code for most PLLs.
> >> 
> >> A rough picture in my mind was,
> >> After implementing  generic samung_clk_register_pll(), code may look
> >> like below. Its just an idea, please feel free to correct it.
> >> Later we can factor out other common clk.ops for PLLs also.
> >> 
> >> this diff is over this series.
> >> Assuming a generic samung_clk_register_pll() is their(which i think
> >> is
> >> not impossible)
> >> 8<-------------------------------------------------------------------
> >> --- ----
> >> 
> >> --- a/drivers/clk/samsung/clk-exynos5250.c
> >> +++ b/drivers/clk/samsung/clk-exynos5250.c
> >> @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
> >> epll_24mhz_tbl[] = {
> >> 
> >>         PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
> >>  
> >>  };
> >> 
> >> +struct __initdata samsung_pll_init_data samsung_plls[] = {
> >> +       PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0,
> >> NULL), +       PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK,
> >> MPLL_CON0, NULL), +       PLL(pll_3500, "fout_bpll",
> >> "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), +       PLL(pll_3500,
> >> "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), +
> >> PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +};
> >> +
> >> +struct __initdata samsung_pll_init_data epll_init_data =
> >> +       PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0,
> >> NULL); +
> >> +struct __initdata samsung_pll_init_data vpll_init_data =
> >> +       PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0,
> >> NULL); +
> > 
> > This is mostly what I had in my mind. In addition I think I might have
> > a solution for rate tables:
> > 
> > If we create another array
> > 
> >         struct samsung_pll_rate_table *rate_tables_24mhz[] = {
> >         
> >                 apll_rate_table_24mhz,
> >                 mpll_rate_table_24mhz, /* can be NULL as well, if no
> > 
> > support for rate change */
> > 
> >                 epll_rate_table_24mhz,
> >                 vpll_rate_table_24mhz,
> >                 /* ... */
> >         
> >         };
> > 
> > which lists rate tables for given input frequency. This relies on
> > making rate tables end with a sentinel, to remove the need of passing
> > array sizes.
> 
> I think we may also have to make assumption that entries in the arrays
> rate_tables_24mhz[] and samsung_plls[] should be in same order in
> both arrays, and which may not be fair assumption, otherwise we
> have to use some mechanism to identify which rate_table is for which
> PLL, which will increase code and complexity.
> Am I missed something or you are thinking something else?

Yes, this is exactly what I thought. The order and size of 
rate_tables_24mhz[] would have to be the same as of samsung_plls[], which 
shouldn't be a problem technically, but adds another responsibility to the 
person who defines them.

> Any thoughts from Doug or others ?
> 
> >>  /* register exynox5250 clocks */
> >>  void __init exynos5250_clk_init(struct device_node *np)
> >>  {
> >> 
> >> @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct
> >> device_node *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks,
> >> 
> >>                         ARRAY_SIZE(exynos5250_pll_pmux_clks));
> >> 
> >> -       fin_pll_rate = _get_rate("fin_pll");
> >> +       samsung_clk_register_pll(samsung_plls,
> >> ARRAY_SIZE(samsung_plls)); +
> > 
> > ...and then pass it here like:
> >         if (fin_pll_rate == 24 * MHZ) {
> >         
> >                 samsung_clk_register_pll(samsung_plls,
> > 
> > ARRAY_SIZE(samsung_plls), rate_tables_24mhz);
> > 
> >         } else {
> >         
> >                 /* warning or whatever */
> >                 samsung_clk_register_pll(samsung_plls,
> > 
> > ARRAY_SIZE(samsung_plls), NULL);
> > 
> >         }
> > 
> > This way we could specify different rate tables depending on input
> > frequency for a whole group of PLLs.
> 
> I think code lines or complexity will be same.
> Also all PLLs may not have same parent.

Most of them usually do. Anyway, they can be grouped by the parent, e.g. 
all that have fin_pll as input can use one set of arrays and other can 
have their own set.

> > The only thing I don't like here is having two separate arrays that
> > need to have the same sizes. Feel free to improve (or discard) this
> > idea, though.
> 
> Sorry, I didn't get your point. You are talking about which two arrays?

I mean that the code would need to assume that

samsung_plls[] and rate_tables_24mhz[]

have the same sizes (and orders), which is not a perfect solution, but I 
can't think of anything better at the moment.

Best regards,
Tomasz

> Regards,
> Yadwinder
> 
> > Best regards,
> > Tomasz
> > 
> >>         vpllsrc = __clk_lookup("mout_vpllsrc");
> >>         if (vpllsrc)
> >>         
> >>                 mout_vpllsrc_rate = clk_get_rate(vpllsrc);
> >> 
> >> -       apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> >> -                       reg_base, NULL, 0);
> >> -       mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> >> -                       reg_base + 0x4000, NULL, 0);
> >> -       bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
> >> -                       reg_base + 0x20010, NULL, 0);
> >> -       gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
> >> -                       reg_base + 0x10050, NULL, 0);
> >> -       cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
> >> -                       reg_base + 0x10020, NULL, 0);
> >> -
> >> +       fin_pll_rate = _get_rate("fin_pll");
> >> 
> >>         if (fin_pll_rate == (24 * MHZ)) {
> >> 
> >> -               epll = samsung_clk_register_pll36xx("fout_epll",
> >> "fin_pll", -                               reg_base + 0x10030,
> >> epll_24mhz_tbl, -
> >> ARRAY_SIZE(epll_24mhz_tbl));
> >> -       } else {
> >> -               pr_warn("%s: valid epll rate_table missing for\n"
> >> -                       "parent fin_pll:%lu hz\n", __func__,
> >> fin_pll_rate); -               epll =
> >> samsung_clk_register_pll36xx("fout_epll", "fin_pll", -
> >> 
> >>              reg_base + 0x10030, NULL, 0);
> >> 
> >> +               epll_init_data.rate_table =  epll_24mhz_tb;
> >> 
> >>         }
> >> 
> >> +       samsung_clk_register_pll(&fout_epll_data, 1);
> >> 
> >>         if (mout_vpllsrc_rate == (24 * MHZ)) {
> >> 
> >> -               vpll = samsung_clk_register_pll36xx("fout_vpll",
> >> "mout_vpllsrc" -                       , reg_base + 0x10040,
> >> vpll_24mhz_tbl,
> >> -                       ARRAY_SIZE(vpll_24mhz_tbl));
> >> -       } else {
> >> -               pr_warn("%s: valid vpll rate_table missing for\n"
> >> -                       "parent mout_vpllsrc_rate:%lu hz\n",
> >> __func__,
> >> -                       mout_vpllsrc_rate);
> >> -               samsung_clk_register_pll36xx("fout_vpll",
> >> "mout_vpllsrc", -                       reg_base + 0x10040, NULL, 0);
> >> +               vpll_init_data.rate_table =  epll_24mhz_tb;
> >> 
> >>         }
> >> 
> >> +       samsung_clk_register_pll(&fout_epll_data, 1);
> >> 
> >>         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
> >>         
> >>                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
> >> 
> >> diff --git a/drivers/clk/samsung/clk-pll.h
> >> b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644
> >> --- a/drivers/clk/samsung/clk-pll.h
> >> +++ b/drivers/clk/samsung/clk-pll.h
> >> @@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
> >> 
> >>         unsigned int kdiv;
> >>  
> >>  };
> >> 
> >> +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)
> >> \ +       {
> >> 
> >>   \ +               .type   =       _type,
> >>   
> >>     \ +               .name   =       _name,
> >>     
> >>       \ +               .parent_name =  _pname,
> >>       
> >>         \ +               .flags  =       CLK_GET_RATE_NOCACHE,
> >>         
> >>           \ +               .rate_table =   _rate_table,
> >>           
> >>             \ +               .con_reg =      _con_reg,
> >>             
> >>               \ +               .lock_reg =     _lock_reg,
> >>               
> >>                 \ +       }
> >> 
> >> +
> >> +enum samsung_pll_type {
> >> +       pll_3500,
> >> +       pll_45xx,
> >> +       pll_2550,
> >> +       pll_3600,
> >> +       pll_46xx,
> >> +       pll_2660,
> >> +};
> >> +
> >> +
> >> +struct samsung_pll_init_data {
> >> +       enum            samsung_pll_type type;
> >> +       struct          samsung_pll_rate_table *rate_table;
> >> +       const char      *name;
> >> +       const char      *parent_name;
> >> +       unsigned long   flags;
> >> +       const void __iomem *con_reg;
> >> +       const void __iomem *lock_reg;
> >> +};
> >> +
> >> +extern int  __init samsung_clk_register_pll(struct
> >> samsung_pll_init_data *data, +                               unsigned
> >> int nr_pll);
> >> +
> >> 
> >> Regards,
> >> Yadwinder

[PATCH v4 1/6] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Yadwinder Singh Brar]

On Fri, Jun 14, 2013 at 12:13 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Friday 14 of June 2013 00:05:31 Yadwinder Singh Brar wrote:
>> On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com>
> wrote:
>> > On Thursday 13 of June 2013 12:32:05 Yadwinder Singh Brar wrote:
>> >> On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
>> >>
>> >> <abrestic@chromium.org> wrote:
>> >> > Doug,
>> >> >
>> >> >>> Hmm, if done properly, it could simplify PLL registration in SoC
>> >> >>> clock
>> >> >>> initialization code a lot.
>> >> >>>
>> >> >>> I'm not sure if this is really the best solution (feel free to
>> >> >>> suggest
>> >> >>> anything better), but we could put PLLs in an array, like other
>> >> >>> clocks,
>> >> >>> e.g.
>> >> >>>
>> >> >>>         ... exynos4210_pll_clks[] = {
>> >> >>>
>> >> >>>                 CLK_PLL45XX(...),
>> >> >>>                 CLK_PLL45XX(...),
>> >> >>>                 CLK_PLL46XX(...),
>> >> >>>                 CLK_PLL46XX(...),
>> >> >>>
>> >> >>>         };
>> >> >>>
>> >> >>> and then just call a helper like
>> >> >>>
>> >> >>>         samsung_clk_register_pll(exynos4210_pll_clks,
>> >> >>>
>> >> >>>                         ARRAY_SIZE(exynos4210_pll_clks));
>> >> >>
>> >> >> Something like that looks like what I was thinking.  I'd have to
>> >> >> see
>> >> >> it actually coded up to see if there's something I'm missing that
>> >> >> would prevent us from doing that, but I don't see anything.
>> >> >
>> >> > The only issue I see with this is that we may only want to register
>> >> > a
>> >> > rate table with a PLL only if fin_pll is running at a certain rate.
>> >> > On 5250 and 5420, for example, we have EPLL and VPLL rate tables
>> >> > that
>> >> > should only be registered if fin_pll is 24Mhz.  We may have to
>> >> > register those separately, but this approach seems fine otherwise.
>> >>
>> >> As Andrew Bresticker said, we will face problem with different table,
>> >> and it will give some pain while handling such cases but I think
>> >> overall code may look better.
>> >>
>> >> Similar thoughts were their in my mind also, but i didn't want to
>> >> disturb this series :).
>> >
>> > Yes, I was thinking the same as well, but now that Exynos5420 doesn't
>> > follow the 0x100 register spacing, we have a problem :) .
>> >
>> >> Anyways, I think we can do it now only rather going for incremental
>> >> patches after this series.
>> >> I was thinking to make samsung_clk_register_pllxxxx itself  little
>> >> generic instead
>> >> of using helper, as we are almost duplicating code for most PLLs.
>> >>
>> >> A rough picture in my mind was,
>> >> After implementing  generic samung_clk_register_pll(), code may look
>> >> like below. Its just an idea, please feel free to correct it.
>> >> Later we can factor out other common clk.ops for PLLs also.
>> >>
>> >> this diff is over this series.
>> >> Assuming a generic samung_clk_register_pll() is their(which i think
>> >> is
>> >> not impossible)
>> >> 8<-------------------------------------------------------------------
>> >> --- ----
>> >>
>> >> --- a/drivers/clk/samsung/clk-exynos5250.c
>> >> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> >> @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
>> >> epll_24mhz_tbl[] = {
>> >>
>> >>         PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
>> >>
>> >>  };
>> >>
>> >> +struct __initdata samsung_pll_init_data samsung_plls[] = {
>> >> +       PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0,
>> >> NULL), +       PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK,
>> >> MPLL_CON0, NULL), +       PLL(pll_3500, "fout_bpll",
>> >> "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), +       PLL(pll_3500,
>> >> "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), +
>> >> PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +};
>> >> +
>> >> +struct __initdata samsung_pll_init_data epll_init_data =
>> >> +       PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0,
>> >> NULL); +
>> >> +struct __initdata samsung_pll_init_data vpll_init_data =
>> >> +       PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0,
>> >> NULL); +
>> >
>> > This is mostly what I had in my mind. In addition I think I might have
>> > a solution for rate tables:
>> >
>> > If we create another array
>> >
>> >         struct samsung_pll_rate_table *rate_tables_24mhz[] = {
>> >
>> >                 apll_rate_table_24mhz,
>> >                 mpll_rate_table_24mhz, /* can be NULL as well, if no
>> >
>> > support for rate change */
>> >
>> >                 epll_rate_table_24mhz,
>> >                 vpll_rate_table_24mhz,
>> >                 /* ... */
>> >
>> >         };
>> >
>> > which lists rate tables for given input frequency. This relies on
>> > making rate tables end with a sentinel, to remove the need of passing
>> > array sizes.
>>
>> I think we may also have to make assumption that entries in the arrays
>> rate_tables_24mhz[] and samsung_plls[] should be in same order in
>> both arrays, and which may not be fair assumption, otherwise we
>> have to use some mechanism to identify which rate_table is for which
>> PLL, which will increase code and complexity.
>> Am I missed something or you are thinking something else?
>
> Yes, this is exactly what I thought. The order and size of
> rate_tables_24mhz[] would have to be the same as of samsung_plls[], which
> shouldn't be a problem technically, but adds another responsibility to the
> person who defines them.
>

OK. but I think this is not a fair assumption.

>> Any thoughts from Doug or others ?
>>
>> >>  /* register exynox5250 clocks */
>> >>  void __init exynos5250_clk_init(struct device_node *np)
>> >>  {
>> >>
>> >> @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct
>> >> device_node *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks,
>> >>
>> >>                         ARRAY_SIZE(exynos5250_pll_pmux_clks));
>> >>
>> >> -       fin_pll_rate = _get_rate("fin_pll");
>> >> +       samsung_clk_register_pll(samsung_plls,
>> >> ARRAY_SIZE(samsung_plls)); +
>> >
>> > ...and then pass it here like:
>> >         if (fin_pll_rate == 24 * MHZ) {
>> >
>> >                 samsung_clk_register_pll(samsung_plls,
>> >
>> > ARRAY_SIZE(samsung_plls), rate_tables_24mhz);
>> >
>> >         } else {
>> >
>> >                 /* warning or whatever */
>> >                 samsung_clk_register_pll(samsung_plls,
>> >
>> > ARRAY_SIZE(samsung_plls), NULL);
>> >
>> >         }
>> >
>> > This way we could specify different rate tables depending on input
>> > frequency for a whole group of PLLs.
>>
>> I think code lines or complexity will be same.
>> Also all PLLs may not have same parent.
>
> Most of them usually do. Anyway, they can be grouped by the parent, e.g.
> all that have fin_pll as input can use one set of arrays and other can
> have their own set.
>

Yes, but number of if() statements to handle them and overall lines in file,
 will be same(rather more) as compare to existing approach.

>> > The only thing I don't like here is having two separate arrays that
>> > need to have the same sizes. Feel free to improve (or discard) this
>> > idea, though.
>>
>> Sorry, I didn't get your point. You are talking about which two arrays?
>
> I mean that the code would need to assume that
>
> samsung_plls[] and rate_tables_24mhz[]
>
> have the same sizes (and orders), which is not a perfect solution, but I
> can't think of anything better at the moment.
>

Yes, that's what in my mind also, we will need an extra array of
pointers to rate_tables..
Also I can't see any considerable advantage of this approach over
existing(proposed) approach.

So at the moment, If anybody don't have any problem, I would like to
adopt the existing(simpler) approach.

Regards,
Yadwinder.

> Best regards,
> Tomasz
>
>> Regards,
>> Yadwinder
>>
>> > Best regards,
>> > Tomasz
>> >
>> >>         vpllsrc = __clk_lookup("mout_vpllsrc");
>> >>         if (vpllsrc)
>> >>
>> >>                 mout_vpllsrc_rate = clk_get_rate(vpllsrc);
>> >>
>> >> -       apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
>> >> -                       reg_base, NULL, 0);
>> >> -       mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
>> >> -                       reg_base + 0x4000, NULL, 0);
>> >> -       bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
>> >> -                       reg_base + 0x20010, NULL, 0);
>> >> -       gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
>> >> -                       reg_base + 0x10050, NULL, 0);
>> >> -       cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
>> >> -                       reg_base + 0x10020, NULL, 0);
>> >> -
>> >> +       fin_pll_rate = _get_rate("fin_pll");
>> >>
>> >>         if (fin_pll_rate == (24 * MHZ)) {
>> >>
>> >> -               epll = samsung_clk_register_pll36xx("fout_epll",
>> >> "fin_pll", -                               reg_base + 0x10030,
>> >> epll_24mhz_tbl, -
>> >> ARRAY_SIZE(epll_24mhz_tbl));
>> >> -       } else {
>> >> -               pr_warn("%s: valid epll rate_table missing for\n"
>> >> -                       "parent fin_pll:%lu hz\n", __func__,
>> >> fin_pll_rate); -               epll =
>> >> samsung_clk_register_pll36xx("fout_epll", "fin_pll", -
>> >>
>> >>              reg_base + 0x10030, NULL, 0);
>> >>
>> >> +               epll_init_data.rate_table =  epll_24mhz_tb;
>> >>
>> >>         }
>> >>
>> >> +       samsung_clk_register_pll(&fout_epll_data, 1);
>> >>
>> >>         if (mout_vpllsrc_rate == (24 * MHZ)) {
>> >>
>> >> -               vpll = samsung_clk_register_pll36xx("fout_vpll",
>> >> "mout_vpllsrc" -                       , reg_base + 0x10040,
>> >> vpll_24mhz_tbl,
>> >> -                       ARRAY_SIZE(vpll_24mhz_tbl));
>> >> -       } else {
>> >> -               pr_warn("%s: valid vpll rate_table missing for\n"
>> >> -                       "parent mout_vpllsrc_rate:%lu hz\n",
>> >> __func__,
>> >> -                       mout_vpllsrc_rate);
>> >> -               samsung_clk_register_pll36xx("fout_vpll",
>> >> "mout_vpllsrc", -                       reg_base + 0x10040, NULL, 0);
>> >> +               vpll_init_data.rate_table =  epll_24mhz_tb;
>> >>
>> >>         }
>> >>
>> >> +       samsung_clk_register_pll(&fout_epll_data, 1);
>> >>
>> >>         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
>> >>
>> >>                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
>> >>
>> >> diff --git a/drivers/clk/samsung/clk-pll.h
>> >> b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644
>> >> --- a/drivers/clk/samsung/clk-pll.h
>> >> +++ b/drivers/clk/samsung/clk-pll.h
>> >> @@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
>> >>
>> >>         unsigned int kdiv;
>> >>
>> >>  };
>> >>
>> >> +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)
>> >> \ +       {
>> >>
>> >>   \ +               .type   =       _type,
>> >>
>> >>     \ +               .name   =       _name,
>> >>
>> >>       \ +               .parent_name =  _pname,
>> >>
>> >>         \ +               .flags  =       CLK_GET_RATE_NOCACHE,
>> >>
>> >>           \ +               .rate_table =   _rate_table,
>> >>
>> >>             \ +               .con_reg =      _con_reg,
>> >>
>> >>               \ +               .lock_reg =     _lock_reg,
>> >>
>> >>                 \ +       }
>> >>
>> >> +
>> >> +enum samsung_pll_type {
>> >> +       pll_3500,
>> >> +       pll_45xx,
>> >> +       pll_2550,
>> >> +       pll_3600,
>> >> +       pll_46xx,
>> >> +       pll_2660,
>> >> +};
>> >> +
>> >> +
>> >> +struct samsung_pll_init_data {
>> >> +       enum            samsung_pll_type type;
>> >> +       struct          samsung_pll_rate_table *rate_table;
>> >> +       const char      *name;
>> >> +       const char      *parent_name;
>> >> +       unsigned long   flags;
>> >> +       const void __iomem *con_reg;
>> >> +       const void __iomem *lock_reg;
>> >> +};
>> >> +
>> >> +extern int  __init samsung_clk_register_pll(struct
>> >> samsung_pll_init_data *data, +                               unsigned
>> >> int nr_pll);
>> >> +
>> >>
>> >> Regards,
>> >> Yadwinder

[PATCH v4 2/6] clk: samsung: Add support to register rate_table for PLL3xxx

[Yadwinder Singh Brar]

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.

Reviewed-by: Doug Anderson <dianders@chromium.org>
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        |   22 +++++++++++++++++++-
 drivers/clk/samsung/clk-pll.h        |   35 ++++++++++++++++++++++++++++++++-
 4 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index ba33bc6..e02a342 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -1028,13 +1028,13 @@ void __init exynos4_clk_init(struct device_node *np, enum exynos4_soc exynos4_so
 					reg_base + VPLL_CON0, pll_4650c);
 	} else {
 		apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
-					reg_base + APLL_LOCK);
+					reg_base + APLL_LOCK, NULL, 0);
 		mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-					reg_base + E4X12_MPLL_LOCK);
+					reg_base + E4X12_MPLL_LOCK, NULL, 0);
 		epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-					reg_base + EPLL_LOCK);
+					reg_base + EPLL_LOCK, NULL, 0);
 		vpll = samsung_clk_register_pll36xx("fout_vpll", "fin_pll",
-					reg_base + VPLL_LOCK);
+					reg_base + VPLL_LOCK, NULL, 0);
 	}
 
 	samsung_clk_add_lookup(apll, fout_apll);
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index 687b580..ddf10ca 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -491,19 +491,19 @@ void __init exynos5250_clk_init(struct device_node *np)
 			ext_clk_match);
 
 	apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
-			reg_base);
+			reg_base, NULL, 0);
 	mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-			reg_base + 0x4000);
+			reg_base + 0x4000, NULL, 0);
 	bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
-			reg_base + 0x20010);
+			reg_base + 0x20010, NULL, 0);
 	gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
-			reg_base + 0x10050);
+			reg_base + 0x10050, NULL, 0);
 	cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
-			reg_base + 0x10020);
+			reg_base + 0x10020, NULL, 0);
 	epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-			reg_base + 0x10030);
+			reg_base + 0x10030, NULL, 0);
 	vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
-			reg_base + 0x10040);
+			reg_base + 0x10040, NULL, 0);
 
 	samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
 			ARRAY_SIZE(exynos5250_fixed_rate_clks));
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index a7d8ad9..cba73a4 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -16,6 +16,8 @@
 struct samsung_clk_pll {
 	struct clk_hw		hw;
 	const void __iomem	*base;
+	const struct samsung_pll_rate_table *rate_table;
+	unsigned int rate_count;
 };
 
 #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
@@ -62,7 +64,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops = {
 };
 
 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)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -80,6 +84,12 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 	init.parent_names = &pname;
 	init.num_parents = 1;
 
+	if (rate_table && rate_count) {
+		pll->rate_count = rate_count;
+		pll->rate_table = kmemdup(rate_table, rate_count *
+			sizeof(struct samsung_pll_rate_table), GFP_KERNEL);
+	}
+
 	pll->hw.init = &init;
 	pll->base = base;
 
@@ -137,7 +147,9 @@ static const struct clk_ops samsung_pll36xx_clk_ops = {
 };
 
 struct clk * __init samsung_clk_register_pll36xx(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)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -155,6 +167,12 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 	init.parent_names = &pname;
 	init.num_parents = 1;
 
+	if (rate_table && rate_count) {
+		pll->rate_count = rate_count;
+		pll->rate_table = kmemdup(rate_table, rate_count *
+			sizeof(struct samsung_pll_rate_table), GFP_KERNEL);
+	}
+
 	pll->hw.init = &init;
 	pll->base = base;
 
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index 1329522..4780e6c 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -12,6 +12,33 @@
 #ifndef __SAMSUNG_CLK_PLL_H
 #define __SAMSUNG_CLK_PLL_H
 
+#define PLL_35XX_RATE(_rate, _m, _p, _s)			\
+	{							\
+		.rate	=	(_rate),				\
+		.mdiv	=	(_m),				\
+		.pdiv	=	(_p),				\
+		.sdiv	=	(_s),				\
+	}
+
+#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)			\
+	{							\
+		.rate	=	(_rate),				\
+		.mdiv	=	(_m),				\
+		.pdiv	=	(_p),				\
+		.sdiv	=	(_s),				\
+		.kdiv	=	(_k),				\
+	}
+
+/* NOTE: Rate table should be kept sorted in descending order. */
+
+struct samsung_pll_rate_table {
+	unsigned int rate;
+	unsigned int pdiv;
+	unsigned int mdiv;
+	unsigned int sdiv;
+	unsigned int kdiv;
+};
+
 enum pll45xx_type {
 	pll_4500,
 	pll_4502,
@@ -25,9 +52,13 @@ enum pll46xx_type {
 };
 
 extern 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);
 extern struct clk * __init samsung_clk_register_pll36xx(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);
 extern struct clk * __init samsung_clk_register_pll45xx(const char *name,
 			const char *pname, const void __iomem *con_reg,
 			enum pll45xx_type type);
-- 
1.7.0.4

[PATCH v4 2/6] clk: samsung: Add support to register rate_table for PLL3xxx

[Doug Anderson]

Yadwinder,

On Mon, Jun 3, 2013 at 8:09 AM, Yadwinder Singh Brar
<yadi.brar@samsung.com> wrote:
> 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.
>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> 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        |   22 +++++++++++++++++++-
>  drivers/clk/samsung/clk-pll.h        |   35 ++++++++++++++++++++++++++++++++-
>  4 files changed, 64 insertions(+), 15 deletions(-)

Using something like patman
<http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;hb=refs/heads/master>
would really help here so you could get some version history.  I see
it in 0/6 but that's a bit of a pain...

Did you and Tomasz ever come to an agreement about whether the fin
freq needs to be specified with the PMSK values?


> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index ba33bc6..e02a342 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -1028,13 +1028,13 @@ void __init exynos4_clk_init(struct device_node *np, enum exynos4_soc exynos4_so
>                                         reg_base + VPLL_CON0, pll_4650c);
>         } else {
>                 apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> -                                       reg_base + APLL_LOCK);
> +                                       reg_base + APLL_LOCK, NULL, 0);
>                 mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> -                                       reg_base + E4X12_MPLL_LOCK);
> +                                       reg_base + E4X12_MPLL_LOCK, NULL, 0);
>                 epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
> -                                       reg_base + EPLL_LOCK);
> +                                       reg_base + EPLL_LOCK, NULL, 0);
>                 vpll = samsung_clk_register_pll36xx("fout_vpll", "fin_pll",
> -                                       reg_base + VPLL_LOCK);
> +                                       reg_base + VPLL_LOCK, NULL, 0);
>         }
>
>         samsung_clk_add_lookup(apll, fout_apll);
> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
> index 687b580..ddf10ca 100644
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -491,19 +491,19 @@ void __init exynos5250_clk_init(struct device_node *np)
>                         ext_clk_match);
>
>         apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> -                       reg_base);
> +                       reg_base, NULL, 0);
>         mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> -                       reg_base + 0x4000);
> +                       reg_base + 0x4000, NULL, 0);
>         bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
> -                       reg_base + 0x20010);
> +                       reg_base + 0x20010, NULL, 0);
>         gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
> -                       reg_base + 0x10050);
> +                       reg_base + 0x10050, NULL, 0);
>         cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
> -                       reg_base + 0x10020);
> +                       reg_base + 0x10020, NULL, 0);
>         epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
> -                       reg_base + 0x10030);
> +                       reg_base + 0x10030, NULL, 0);
>         vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
> -                       reg_base + 0x10040);
> +                       reg_base + 0x10040, NULL, 0);
>
>         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
>                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index a7d8ad9..cba73a4 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -16,6 +16,8 @@
>  struct samsung_clk_pll {
>         struct clk_hw           hw;
>         const void __iomem      *base;
> +       const struct samsung_pll_rate_table *rate_table;
> +       unsigned int rate_count;
>  };
>
>  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
> @@ -62,7 +64,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops = {
>  };
>
>  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)
>  {
>         struct samsung_clk_pll *pll;
>         struct clk *clk;
> @@ -80,6 +84,12 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
>         init.parent_names = &pname;
>         init.num_parents = 1;
>
> +       if (rate_table && rate_count) {
> +               pll->rate_count = rate_count;
> +               pll->rate_table = kmemdup(rate_table, rate_count *
> +                       sizeof(struct samsung_pll_rate_table), GFP_KERNEL);
> +       }
> +
>         pll->hw.init = &init;
>         pll->base = base;
>
> @@ -137,7 +147,9 @@ static const struct clk_ops samsung_pll36xx_clk_ops = {
>  };
>
>  struct clk * __init samsung_clk_register_pll36xx(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)
>  {
>         struct samsung_clk_pll *pll;
>         struct clk *clk;
> @@ -155,6 +167,12 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
>         init.parent_names = &pname;
>         init.num_parents = 1;
>
> +       if (rate_table && rate_count) {
> +               pll->rate_count = rate_count;
> +               pll->rate_table = kmemdup(rate_table, rate_count *
> +                       sizeof(struct samsung_pll_rate_table), GFP_KERNEL);

To me I'd rather see the tables left as "init", not "init_data" and
avoid the strdup().  The small amount of waste from multiple tables
doesn't seem worth the extra allocation.

...but I don't really care much either way.

[PATCH v4 2/6] clk: samsung: Add support to register rate_table for PLL3xxx

[Tomasz Figa]

Hi Doug,

On Wednesday 12 of June 2013 13:43:37 Doug Anderson wrote:
> Yadwinder,
> 
> On Mon, Jun 3, 2013 at 8:09 AM, Yadwinder Singh Brar
> 
> <yadi.brar@samsung.com> wrote:
> > 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.
> > 
> > Reviewed-by: Doug Anderson <dianders@chromium.org>
> > 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        |   22 +++++++++++++++++++-
> >  drivers/clk/samsung/clk-pll.h        |   35
> >  ++++++++++++++++++++++++++++++++- 4 files changed, 64 insertions(+),
> >  15 deletions(-)
> 
> Using something like patman
> <http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;hb=refs/h
> eads/master> would really help here so you could get some version
> history.  I see it in 0/6 but that's a bit of a pain...
> 
> Did you and Tomasz ever come to an agreement about whether the fin
> freq needs to be specified with the PMSK values?

No, not really.

But since
a) there seems to be no input from hardware guys,
b) we want to have this series merged,
c) otherwise this patch looks good
I think for now we can keep support for single input rate.

However, I think it would be resonable to recalculate all the output rates 
from PLL equation at registration stage anyway just to eliminate any typos 
or other errors in setting table, otherwise you might end up with having 
different rate set in the PLL and different assumed by the driver.

Best regards,
Tomasz

> > diff --git a/drivers/clk/samsung/clk-exynos4.c
> > b/drivers/clk/samsung/clk-exynos4.c index ba33bc6..e02a342 100644
> > --- a/drivers/clk/samsung/clk-exynos4.c
> > +++ b/drivers/clk/samsung/clk-exynos4.c
> > @@ -1028,13 +1028,13 @@ void __init exynos4_clk_init(struct
> > device_node *np, enum exynos4_soc exynos4_so> 
> >                                         reg_base + VPLL_CON0,
> >                                         pll_4650c);
> >         
> >         } else {
> >         
> >                 apll = samsung_clk_register_pll35xx("fout_apll",
> >                 "fin_pll",
> > 
> > -                                       reg_base + APLL_LOCK);
> > +                                       reg_base + APLL_LOCK, NULL,
> > 0);
> > 
> >                 mpll = samsung_clk_register_pll35xx("fout_mpll",
> >                 "fin_pll",
> > 
> > -                                       reg_base + E4X12_MPLL_LOCK);
> > +                                       reg_base + E4X12_MPLL_LOCK,
> > NULL, 0);> 
> >                 epll = samsung_clk_register_pll36xx("fout_epll",
> >                 "fin_pll",
> > 
> > -                                       reg_base + EPLL_LOCK);
> > +                                       reg_base + EPLL_LOCK, NULL,
> > 0);
> > 
> >                 vpll = samsung_clk_register_pll36xx("fout_vpll",
> >                 "fin_pll",
> > 
> > -                                       reg_base + VPLL_LOCK);
> > +                                       reg_base + VPLL_LOCK, NULL,
> > 0);
> > 
> >         }
> >         
> >         samsung_clk_add_lookup(apll, fout_apll);
> > 
> > diff --git a/drivers/clk/samsung/clk-exynos5250.c
> > b/drivers/clk/samsung/clk-exynos5250.c index 687b580..ddf10ca 100644
> > --- a/drivers/clk/samsung/clk-exynos5250.c
> > +++ b/drivers/clk/samsung/clk-exynos5250.c
> > @@ -491,19 +491,19 @@ void __init exynos5250_clk_init(struct
> > device_node *np)> 
> >                         ext_clk_match);
> >         
> >         apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> > 
> > -                       reg_base);
> > +                       reg_base, NULL, 0);
> > 
> >         mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> > 
> > -                       reg_base + 0x4000);
> > +                       reg_base + 0x4000, NULL, 0);
> > 
> >         bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
> > 
> > -                       reg_base + 0x20010);
> > +                       reg_base + 0x20010, NULL, 0);
> > 
> >         gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
> > 
> > -                       reg_base + 0x10050);
> > +                       reg_base + 0x10050, NULL, 0);
> > 
> >         cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
> > 
> > -                       reg_base + 0x10020);
> > +                       reg_base + 0x10020, NULL, 0);
> > 
> >         epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
> > 
> > -                       reg_base + 0x10030);
> > +                       reg_base + 0x10030, NULL, 0);
> > 
> >         vpll = samsung_clk_register_pll36xx("fout_vpll",
> >         "mout_vpllsrc",
> > 
> > -                       reg_base + 0x10040);
> > +                       reg_base + 0x10040, NULL, 0);
> > 
> >         samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
> >         
> >                         ARRAY_SIZE(exynos5250_fixed_rate_clks));
> > 
> > diff --git a/drivers/clk/samsung/clk-pll.c
> > b/drivers/clk/samsung/clk-pll.c index a7d8ad9..cba73a4 100644
> > --- a/drivers/clk/samsung/clk-pll.c
> > +++ b/drivers/clk/samsung/clk-pll.c
> > @@ -16,6 +16,8 @@
> > 
> >  struct samsung_clk_pll {
> >  
> >         struct clk_hw           hw;
> >         const void __iomem      *base;
> > 
> > +       const struct samsung_pll_rate_table *rate_table;
> > +       unsigned int rate_count;
> > 
> >  };
> >  
> >  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
> > 
> > @@ -62,7 +64,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops
> > = {> 
> >  };
> >  
> >  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)
> > 
> >  {
> >  
> >         struct samsung_clk_pll *pll;
> >         struct clk *clk;
> > 
> > @@ -80,6 +84,12 @@ struct clk * __init
> > samsung_clk_register_pll35xx(const char *name,> 
> >         init.parent_names = &pname;
> >         init.num_parents = 1;
> > 
> > +       if (rate_table && rate_count) {
> > +               pll->rate_count = rate_count;
> > +               pll->rate_table = kmemdup(rate_table, rate_count *
> > +                       sizeof(struct samsung_pll_rate_table),
> > GFP_KERNEL); +       }
> > +
> > 
> >         pll->hw.init = &init;
> >         pll->base = base;
> > 
> > @@ -137,7 +147,9 @@ static const struct clk_ops
> > samsung_pll36xx_clk_ops = {> 
> >  };
> >  
> >  struct clk * __init samsung_clk_register_pll36xx(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)
> > 
> >  {
> >  
> >         struct samsung_clk_pll *pll;
> >         struct clk *clk;
> > 
> > @@ -155,6 +167,12 @@ struct clk * __init
> > samsung_clk_register_pll36xx(const char *name,> 
> >         init.parent_names = &pname;
> >         init.num_parents = 1;
> > 
> > +       if (rate_table && rate_count) {
> > +               pll->rate_count = rate_count;
> > +               pll->rate_table = kmemdup(rate_table, rate_count *
> > +                       sizeof(struct samsung_pll_rate_table),
> > GFP_KERNEL);
> To me I'd rather see the tables left as "init", not "init_data" and
> avoid the strdup().  The small amount of waste from multiple tables
> doesn't seem worth the extra allocation.
> 
> ...but I don't really care much either way.
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/6] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Yadwinder Singh Brar]

This patch add set_rate() and round_rate() for PLL35xx

Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/clk/samsung/clk-pll.c |  104 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index cba73a4..319b52b 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -27,6 +27,37 @@ struct samsung_clk_pll {
 #define pll_writel(pll, val, offset)					\
 		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
 
+static const struct samsung_pll_rate_table *samsung_get_pll_settings(
+				struct samsung_clk_pll *pll, unsigned long rate)
+{
+	const struct samsung_pll_rate_table  *rate_table = pll->rate_table;
+	int i;
+
+	for (i = 0; i < pll->rate_count; i++) {
+		if (rate == rate_table[i].rate)
+			return &rate_table[i];
+	}
+
+	return NULL;
+}
+
+static long samsung_pll_round_rate(struct clk_hw *hw,
+			unsigned long drate, unsigned long *prate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	const struct samsung_pll_rate_table *rate_table = pll->rate_table;
+	int i;
+
+	/* Assumming rate_table is in descending order */
+	for (i = 0; i < pll->rate_count; i++) {
+		if (drate >= rate_table[i].rate)
+			return rate_table[i].rate;
+	}
+
+	/* return minimum supported value */
+	return rate_table[i - 1].rate;
+}
+
 /*
  * PLL35xx Clock Type
  */
@@ -34,12 +65,17 @@ struct samsung_clk_pll {
 #define PLL35XX_CON0_OFFSET	(0x100)
 #define PLL35XX_CON1_OFFSET	(0x104)
 
+/* Maximum lock time can be 270 * PDIV cycles */
+#define PLL35XX_LOCK_FACTOR	(270)
+
 #define PLL35XX_MDIV_MASK       (0x3FF)
 #define PLL35XX_PDIV_MASK       (0x3F)
 #define PLL35XX_SDIV_MASK       (0x7)
+#define PLL35XX_LOCK_STAT_MASK  (0x1)
 #define PLL35XX_MDIV_SHIFT      (16)
 #define PLL35XX_PDIV_SHIFT      (8)
 #define PLL35XX_SDIV_SHIFT      (0)
+#define PLL35XX_LOCK_STAT_SHIFT	(29)
 
 static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -59,8 +95,72 @@ static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
+static inline bool samsung_pll35xx_mp_change(
+		const struct samsung_pll_rate_table *rate, u32 pll_con)
+{
+	u32 old_mdiv, old_pdiv;
+
+	old_mdiv = (pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK;
+	old_pdiv = (pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK;
+
+	return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv);
+}
+
+static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long prate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	const struct samsung_pll_rate_table *rate;
+	u32 tmp;
+
+	/* Get required rate settings from table */
+	rate = samsung_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, __clk_get_name(hw->clk));
+		return -EINVAL;
+	}
+
+	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+
+	if (!(samsung_pll35xx_mp_change(rate, tmp))) {
+		/* If only s change, change just s value only*/
+		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
+		tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT;
+		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+	} else {
+		/* Set PLL lock time. */
+		pll_writel(pll, rate->pdiv * PLL35XX_LOCK_FACTOR,
+				PLL35XX_LOCK_OFFSET);
+
+		/* Change PLL PMS values */
+		tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
+				(PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) |
+				(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT));
+		tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) |
+				(rate->pdiv << PLL35XX_PDIV_SHIFT) |
+				(rate->sdiv << PLL35XX_SDIV_SHIFT);
+		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+
+		/* wait_lock_time */
+		do {
+			cpu_relax();
+			tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+		} while (!(tmp & (PLL35XX_LOCK_STAT_MASK
+				<< PLL35XX_LOCK_STAT_SHIFT)));
+	}
+
+	return 0;
+}
+
 static const struct clk_ops samsung_pll35xx_clk_ops = {
 	.recalc_rate = samsung_pll35xx_recalc_rate,
+	.round_rate = samsung_pll_round_rate,
+	.set_rate = samsung_pll35xx_set_rate,
+};
+
+static const struct clk_ops samsung_pll35xx_clk_min_ops = {
+	.recalc_rate = samsung_pll35xx_recalc_rate,
 };
 
 struct clk * __init samsung_clk_register_pll35xx(const char *name,
@@ -79,7 +179,6 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 	}
 
 	init.name = name;
-	init.ops = &samsung_pll35xx_clk_ops;
 	init.flags = CLK_GET_RATE_NOCACHE;
 	init.parent_names = &pname;
 	init.num_parents = 1;
@@ -88,6 +187,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 		pll->rate_count = rate_count;
 		pll->rate_table = kmemdup(rate_table, rate_count *
 			sizeof(struct samsung_pll_rate_table), GFP_KERNEL);
+		init.ops = &samsung_pll35xx_clk_ops;
+	} else {
+		init.ops = &samsung_pll35xx_clk_min_ops;
 	}
 
 	pll->hw.init = &init;
-- 
1.7.0.4

[PATCH v4 3/6] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Tomasz Figa]

On Monday 03 of June 2013 20:39:53 Yadwinder Singh Brar wrote:
> This patch add set_rate() and round_rate() for PLL35xx
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c |  104
> ++++++++++++++++++++++++++++++++++++++++- 1 files changed, 103
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c
> b/drivers/clk/samsung/clk-pll.c index cba73a4..319b52b 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -27,6 +27,37 @@ struct samsung_clk_pll {
>  #define pll_writel(pll, val, offset)					\
>  		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
> 
> +static const struct samsung_pll_rate_table *samsung_get_pll_settings(
> +				struct samsung_clk_pll *pll, unsigned long 
rate)
> +{
> +	const struct samsung_pll_rate_table  *rate_table = pll-
>rate_table;
> +	int i;
> +
> +	for (i = 0; i < pll->rate_count; i++) {
> +		if (rate == rate_table[i].rate)
> +			return &rate_table[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static long samsung_pll_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	const struct samsung_pll_rate_table *rate_table = pll->rate_table;
> +	int i;
> +
> +	/* Assumming rate_table is in descending order */
> +	for (i = 0; i < pll->rate_count; i++) {
> +		if (drate >= rate_table[i].rate)
> +			return rate_table[i].rate;
> +	}
> +
> +	/* return minimum supported value */
> +	return rate_table[i - 1].rate;
> +}
> +
>  /*
>   * PLL35xx Clock Type
>   */
> @@ -34,12 +65,17 @@ struct samsung_clk_pll {
>  #define PLL35XX_CON0_OFFSET	(0x100)
>  #define PLL35XX_CON1_OFFSET	(0x104)
> 
> +/* Maximum lock time can be 270 * PDIV cycles */
> +#define PLL35XX_LOCK_FACTOR	(270)
> +
>  #define PLL35XX_MDIV_MASK       (0x3FF)
>  #define PLL35XX_PDIV_MASK       (0x3F)
>  #define PLL35XX_SDIV_MASK       (0x7)
> +#define PLL35XX_LOCK_STAT_MASK  (0x1)
>  #define PLL35XX_MDIV_SHIFT      (16)
>  #define PLL35XX_PDIV_SHIFT      (8)
>  #define PLL35XX_SDIV_SHIFT      (0)
> +#define PLL35XX_LOCK_STAT_SHIFT	(29)
> 
>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>  				unsigned long parent_rate)
> @@ -59,8 +95,72 @@ static unsigned long
> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
> long)fvco;
>  }
> 
> +static inline bool samsung_pll35xx_mp_change(
> +		const struct samsung_pll_rate_table *rate, u32 pll_con)
> +{
> +	u32 old_mdiv, old_pdiv;
> +
> +	old_mdiv = (pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK;
> +	old_pdiv = (pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK;
> +
> +	return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv);
> +}
> +
> +static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long
> drate, +					unsigned long prate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	const struct samsung_pll_rate_table *rate;
> +	u32 tmp;
> +
> +	/* Get required rate settings from table */
> +	rate = samsung_get_pll_settings(pll, drate);
> +	if (!rate) {
> +		pr_err("%s: Invalid rate : %lu for pll clk %s\n", 
__func__,
> +			drate, __clk_get_name(hw->clk));
> +		return -EINVAL;
> +	}
> +
> +	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
> +
> +	if (!(samsung_pll35xx_mp_change(rate, tmp))) {
> +		/* If only s change, change just s value only*/
> +		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
> +		tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT;
> +		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);

I'd use the same coding style here as in case of PLL36xx, i.e. add return 
0 and move following code outside else.

This makes the code a bit more linear and lowers indentation level.

Otherwise looks good.

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

> +	} else {
> +		/* Set PLL lock time. */
> +		pll_writel(pll, rate->pdiv * PLL35XX_LOCK_FACTOR,
> +				PLL35XX_LOCK_OFFSET);
> +
> +		/* Change PLL PMS values */
> +		tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
> +				(PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) 
|
> +				(PLL35XX_SDIV_MASK << 
PLL35XX_SDIV_SHIFT));
> +		tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) |
> +				(rate->pdiv << PLL35XX_PDIV_SHIFT) |
> +				(rate->sdiv << PLL35XX_SDIV_SHIFT);
> +		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
> +
> +		/* wait_lock_time */
> +		do {
> +			cpu_relax();
> +			tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
> +		} while (!(tmp & (PLL35XX_LOCK_STAT_MASK
> +				<< PLL35XX_LOCK_STAT_SHIFT)));
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct clk_ops samsung_pll35xx_clk_ops = {
>  	.recalc_rate = samsung_pll35xx_recalc_rate,
> +	.round_rate = samsung_pll_round_rate,
> +	.set_rate = samsung_pll35xx_set_rate,
> +};
> +
> +static const struct clk_ops samsung_pll35xx_clk_min_ops = {
> +	.recalc_rate = samsung_pll35xx_recalc_rate,
>  };
> 
>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
> @@ -79,7 +179,6 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, }
> 
>  	init.name = name;
> -	init.ops = &samsung_pll35xx_clk_ops;
>  	init.flags = CLK_GET_RATE_NOCACHE;
>  	init.parent_names = &pname;
>  	init.num_parents = 1;
> @@ -88,6 +187,9 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, pll->rate_count =
> rate_count;
>  		pll->rate_table = kmemdup(rate_table, rate_count *
>  			sizeof(struct samsung_pll_rate_table), 
GFP_KERNEL);
> +		init.ops = &samsung_pll35xx_clk_ops;
> +	} else {
> +		init.ops = &samsung_pll35xx_clk_min_ops;
>  	}
> 
>  	pll->hw.init = &init;

[PATCH v4 4/6] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Yadwinder Singh Brar]

From: Vikas Sajjan <vikas.sajjan@linaro.org>

This patch adds set_rate and round_rate clk_ops for PLL36xx

Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/clk/samsung/clk-pll.c |   79 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 319b52b..42b60b5 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -215,6 +215,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 #define PLL36XX_CON0_OFFSET	(0x100)
 #define PLL36XX_CON1_OFFSET	(0x104)
 
+/* Maximum lock time can be 3000 * PDIV cycles */
+#define PLL36XX_LOCK_FACTOR	(3000)
+
 #define PLL36XX_KDIV_MASK	(0xFFFF)
 #define PLL36XX_MDIV_MASK	(0x1FF)
 #define PLL36XX_PDIV_MASK	(0x3F)
@@ -222,6 +225,8 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 #define PLL36XX_MDIV_SHIFT	(16)
 #define PLL36XX_PDIV_SHIFT	(8)
 #define PLL36XX_SDIV_SHIFT	(0)
+#define PLL36XX_KDIV_SHIFT	(0)
+#define PLL36XX_LOCK_STAT_SHIFT	(29)
 
 static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -244,8 +249,78 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
+static inline bool samsung_pll36xx_mpk_change(
+	const struct samsung_pll_rate_table *rate, u32 pll_con0, u32 pll_con1)
+{
+	u32 old_mdiv, old_pdiv, old_kdiv;
+
+	old_mdiv = (pll_con0 >> PLL36XX_MDIV_SHIFT) & PLL36XX_MDIV_MASK;
+	old_pdiv = (pll_con0 >> PLL36XX_PDIV_SHIFT) & PLL36XX_PDIV_MASK;
+	old_kdiv = (pll_con1 >> PLL36XX_KDIV_SHIFT) & PLL36XX_KDIV_MASK;
+
+	return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv ||
+		rate->kdiv != old_kdiv);
+}
+
+static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long parent_rate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 tmp, pll_con0, pll_con1;
+	const struct samsung_pll_rate_table *rate;
+
+	rate = samsung_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, __clk_get_name(hw->clk));
+		return -EINVAL;
+	}
+
+	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+	if (!(samsung_pll36xx_mpk_change(rate, pll_con0, pll_con1))) {
+		/* If only s change, change just s value only*/
+		pll_con0 &= ~(PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT);
+		pll_con0 |= (rate->sdiv << PLL36XX_SDIV_SHIFT);
+		pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+		return 0;
+	}
+
+	/* Set PLL lock time. */
+	pll_writel(pll, (rate->pdiv * PLL36XX_LOCK_FACTOR),
+			PLL36XX_LOCK_OFFSET);
+
+	 /* Change PLL PMS values */
+	pll_con0 &= ~((PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT) |
+			(PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT) |
+			(PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT));
+	pll_con0 |= (rate->mdiv << PLL36XX_MDIV_SHIFT) |
+			(rate->pdiv << PLL36XX_PDIV_SHIFT) |
+			(rate->sdiv << PLL36XX_SDIV_SHIFT);
+	pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+	pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+	pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
+	pll_writel(pll, pll_con1, PLL36XX_CON1_OFFSET);
+
+	/* wait_lock_time */
+	do {
+		cpu_relax();
+		tmp = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	} while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT)));
+
+	return 0;
+}
+
 static const struct clk_ops samsung_pll36xx_clk_ops = {
 	.recalc_rate = samsung_pll36xx_recalc_rate,
+	.set_rate = samsung_pll36xx_set_rate,
+	.round_rate = samsung_pll_round_rate,
+};
+
+static const struct clk_ops samsung_pll36xx_clk_min_ops = {
+	.recalc_rate = samsung_pll36xx_recalc_rate,
 };
 
 struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -264,7 +339,6 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 	}
 
 	init.name = name;
-	init.ops = &samsung_pll36xx_clk_ops;
 	init.flags = CLK_GET_RATE_NOCACHE;
 	init.parent_names = &pname;
 	init.num_parents = 1;
@@ -273,6 +347,9 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 		pll->rate_count = rate_count;
 		pll->rate_table = kmemdup(rate_table, rate_count *
 			sizeof(struct samsung_pll_rate_table), GFP_KERNEL);
+		init.ops = &samsung_pll36xx_clk_ops;
+	} else {
+		init.ops = &samsung_pll36xx_clk_min_ops;
 	}
 
 	pll->hw.init = &init;
-- 
1.7.0.4

[PATCH v4 4/6] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Tomasz Figa]

Hi Yadwinder, Vikas,

On Monday 03 of June 2013 20:39:54 Yadwinder Singh Brar wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> This patch adds set_rate and round_rate clk_ops for PLL36xx
> 
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  drivers/clk/samsung/clk-pll.c |   79
> ++++++++++++++++++++++++++++++++++++++++- 1 files changed, 78
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c
> b/drivers/clk/samsung/clk-pll.c index 319b52b..42b60b5 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -215,6 +215,9 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, #define
> PLL36XX_CON0_OFFSET	(0x100)
>  #define PLL36XX_CON1_OFFSET	(0x104)
> 
> +/* Maximum lock time can be 3000 * PDIV cycles */
> +#define PLL36XX_LOCK_FACTOR	(3000)
> +
>  #define PLL36XX_KDIV_MASK	(0xFFFF)
>  #define PLL36XX_MDIV_MASK	(0x1FF)
>  #define PLL36XX_PDIV_MASK	(0x3F)
> @@ -222,6 +225,8 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, #define
> PLL36XX_MDIV_SHIFT	(16)
>  #define PLL36XX_PDIV_SHIFT	(8)
>  #define PLL36XX_SDIV_SHIFT	(0)
> +#define PLL36XX_KDIV_SHIFT	(0)
> +#define PLL36XX_LOCK_STAT_SHIFT	(29)
> 
>  static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
>  				unsigned long parent_rate)
> @@ -244,8 +249,78 @@ static unsigned long
> samsung_pll36xx_recalc_rate(struct clk_hw *hw, return (unsigned
> long)fvco;
>  }
> 
> +static inline bool samsung_pll36xx_mpk_change(
> +	const struct samsung_pll_rate_table *rate, u32 pll_con0, u32 
pll_con1)
> +{
> +	u32 old_mdiv, old_pdiv, old_kdiv;
> +
> +	old_mdiv = (pll_con0 >> PLL36XX_MDIV_SHIFT) & PLL36XX_MDIV_MASK;
> +	old_pdiv = (pll_con0 >> PLL36XX_PDIV_SHIFT) & PLL36XX_PDIV_MASK;
> +	old_kdiv = (pll_con1 >> PLL36XX_KDIV_SHIFT) & PLL36XX_KDIV_MASK;
> +
> +	return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv ||
> +		rate->kdiv != old_kdiv);
> +}
> +
> +static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long
> drate, +					unsigned long parent_rate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 tmp, pll_con0, pll_con1;
> +	const struct samsung_pll_rate_table *rate;
> +
> +	rate = samsung_get_pll_settings(pll, drate);
> +	if (!rate) {
> +		pr_err("%s: Invalid rate : %lu for pll clk %s\n", 
__func__,
> +			drate, __clk_get_name(hw->clk));
> +		return -EINVAL;
> +	}
> +
> +	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
> +	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
> +
> +	if (!(samsung_pll36xx_mpk_change(rate, pll_con0, pll_con1))) {
> +		/* If only s change, change just s value only*/
> +		pll_con0 &= ~(PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT);
> +		pll_con0 |= (rate->sdiv << PLL36XX_SDIV_SHIFT);
> +		pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);

nit, I would put an empty line before the return statement here, to make 
it stand out a bit more.

Otherwise looks fine for me.

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

> +		return 0;
> +	}
> +
> +	/* Set PLL lock time. */
> +	pll_writel(pll, (rate->pdiv * PLL36XX_LOCK_FACTOR),
> +			PLL36XX_LOCK_OFFSET);
> +
> +	 /* Change PLL PMS values */
> +	pll_con0 &= ~((PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT) |
> +			(PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT) |
> +			(PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT));
> +	pll_con0 |= (rate->mdiv << PLL36XX_MDIV_SHIFT) |
> +			(rate->pdiv << PLL36XX_PDIV_SHIFT) |
> +			(rate->sdiv << PLL36XX_SDIV_SHIFT);
> +	pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
> +
> +	pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
> +	pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
> +	pll_writel(pll, pll_con1, PLL36XX_CON1_OFFSET);
> +
> +	/* wait_lock_time */
> +	do {
> +		cpu_relax();
> +		tmp = pll_readl(pll, PLL36XX_CON0_OFFSET);
> +	} while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT)));
> +
> +	return 0;
> +}
> +
>  static const struct clk_ops samsung_pll36xx_clk_ops = {
>  	.recalc_rate = samsung_pll36xx_recalc_rate,
> +	.set_rate = samsung_pll36xx_set_rate,
> +	.round_rate = samsung_pll_round_rate,
> +};
> +
> +static const struct clk_ops samsung_pll36xx_clk_min_ops = {
> +	.recalc_rate = samsung_pll36xx_recalc_rate,
>  };
> 
>  struct clk * __init samsung_clk_register_pll36xx(const char *name,
> @@ -264,7 +339,6 @@ struct clk * __init
> samsung_clk_register_pll36xx(const char *name, }
> 
>  	init.name = name;
> -	init.ops = &samsung_pll36xx_clk_ops;
>  	init.flags = CLK_GET_RATE_NOCACHE;
>  	init.parent_names = &pname;
>  	init.num_parents = 1;
> @@ -273,6 +347,9 @@ struct clk * __init
> samsung_clk_register_pll36xx(const char *name, pll->rate_count =
> rate_count;
>  		pll->rate_table = kmemdup(rate_table, rate_count *
>  			sizeof(struct samsung_pll_rate_table), 
GFP_KERNEL);
> +		init.ops = &samsung_pll36xx_clk_ops;
> +	} else {
> +		init.ops = &samsung_pll36xx_clk_min_ops;
>  	}
> 
>  	pll->hw.init = &init;

[PATCH v4 5/6] clk: samsung: Reorder MUX registration for mout_vpllsrc

[Yadwinder Singh Brar]

From: Vikas Sajjan <vikas.sajjan@linaro.org>

While trying to get rate of "mout_vpllsrc" MUX (parent) for registering the
"fout_vpll" (child), we found get rate was failing.

So this patch moves the mout_vpllsrc MUX out of the existing common list
and registers the mout_vpllsrc MUX before the PLL registrations.

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/clk/samsung/clk-exynos5250.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index ddf10ca..70cc6cf 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -207,6 +207,10 @@ struct samsung_fixed_factor_clock exynos5250_fixed_factor_clks[] __initdata = {
 	FFACTOR(none, "fout_bplldiv2", "fout_bpll", 1, 2, 0),
 };
 
+struct samsung_mux_clock exynos5250_pll_pmux_clks[] __initdata = {
+	MUX(none, "mout_vpllsrc", mout_vpllsrc_p, SRC_TOP2, 0, 1),
+};
+
 struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
 	MUX(none, "mout_apll", mout_apll_p, SRC_CPU, 0, 1),
 	MUX(none, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
@@ -214,7 +218,6 @@ struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
 	MUX(none, "sclk_mpll", mout_mpll_p, SRC_CORE1, 8, 1),
 	MUX(none, "mout_bpll_fout", mout_bpll_fout_p, PLL_DIV2_SEL, 0, 1),
 	MUX(none, "sclk_bpll", mout_bpll_p, SRC_CDREX, 0, 1),
-	MUX(none, "mout_vpllsrc", mout_vpllsrc_p, SRC_TOP2, 0, 1),
 	MUX(none, "sclk_vpll", mout_vpll_p, SRC_TOP2, 16, 1),
 	MUX(none, "sclk_epll", mout_epll_p, SRC_TOP2, 12, 1),
 	MUX(none, "sclk_cpll", mout_cpll_p, SRC_TOP2, 8, 1),
@@ -490,6 +493,9 @@ void __init exynos5250_clk_init(struct device_node *np)
 			ARRAY_SIZE(exynos5250_fixed_rate_ext_clks),
 			ext_clk_match);
 
+	samsung_clk_register_mux(exynos5250_pll_pmux_clks,
+			ARRAY_SIZE(exynos5250_pll_pmux_clks));
+
 	apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
 			reg_base, NULL, 0);
 	mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
-- 
1.7.0.4

[PATCH v4 5/6] clk: samsung: Reorder MUX registration for mout_vpllsrc

[Tomasz Figa]

On Monday 03 of June 2013 20:39:55 Yadwinder Singh Brar wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> While trying to get rate of "mout_vpllsrc" MUX (parent) for registering
> the "fout_vpll" (child), we found get rate was failing.
> 
> So this patch moves the mout_vpllsrc MUX out of the existing common list
> and registers the mout_vpllsrc MUX before the PLL registrations.
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5250.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

> diff --git a/drivers/clk/samsung/clk-exynos5250.c
> b/drivers/clk/samsung/clk-exynos5250.c index ddf10ca..70cc6cf 100644
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -207,6 +207,10 @@ struct samsung_fixed_factor_clock
> exynos5250_fixed_factor_clks[] __initdata = { FFACTOR(none,
> "fout_bplldiv2", "fout_bpll", 1, 2, 0),
>  };
> 
> +struct samsung_mux_clock exynos5250_pll_pmux_clks[] __initdata = {
> +	MUX(none, "mout_vpllsrc", mout_vpllsrc_p, SRC_TOP2, 0, 1),
> +};
> +
>  struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
>  	MUX(none, "mout_apll", mout_apll_p, SRC_CPU, 0, 1),
>  	MUX(none, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
> @@ -214,7 +218,6 @@ struct samsung_mux_clock exynos5250_mux_clks[]
> __initdata = { MUX(none, "sclk_mpll", mout_mpll_p, SRC_CORE1, 8, 1),
>  	MUX(none, "mout_bpll_fout", mout_bpll_fout_p, PLL_DIV2_SEL, 0, 1),
>  	MUX(none, "sclk_bpll", mout_bpll_p, SRC_CDREX, 0, 1),
> -	MUX(none, "mout_vpllsrc", mout_vpllsrc_p, SRC_TOP2, 0, 1),
>  	MUX(none, "sclk_vpll", mout_vpll_p, SRC_TOP2, 16, 1),
>  	MUX(none, "sclk_epll", mout_epll_p, SRC_TOP2, 12, 1),
>  	MUX(none, "sclk_cpll", mout_cpll_p, SRC_TOP2, 8, 1),
> @@ -490,6 +493,9 @@ void __init exynos5250_clk_init(struct device_node
> *np) ARRAY_SIZE(exynos5250_fixed_rate_ext_clks),
>  			ext_clk_match);
> 
> +	samsung_clk_register_mux(exynos5250_pll_pmux_clks,
> +			ARRAY_SIZE(exynos5250_pll_pmux_clks));
> +
>  	apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
>  			reg_base, NULL, 0);
>  	mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",

[PATCH v4 6/6] clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC

[Yadwinder Singh Brar]

Adds the EPLL and VPLL freq table for exynos5250 SoC.

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/clk/samsung/clk-exynos5250.c |   53 ++++++++++++++++++++++++++++++++--
 drivers/clk/samsung/clk.h            |    2 +
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index 70cc6cf..f98c19d 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -472,11 +472,34 @@ static __initdata struct of_device_id ext_clk_match[] = {
 	{ },
 };
 
+static __initdata struct samsung_pll_rate_table vpll_24mhz_tbl[] = {
+	/* sorted in descending order */
+	/* PLL_36XX_RATE(rate, m, p, s, k) */
+	PLL_36XX_RATE(266000000, 266, 3, 3, 0),
+	/* Not in UM, but need for eDP on snow */
+	PLL_36XX_RATE(70500000, 94, 2, 4, 0),
+};
+
+static __initdata struct samsung_pll_rate_table epll_24mhz_tbl[] = {
+	/* sorted in descending order */
+	/* PLL_36XX_RATE(rate, m, p, s, k) */
+	PLL_36XX_RATE(192000000, 48, 3, 1, 0),
+	PLL_36XX_RATE(180633600, 45, 3, 1, 10381),
+	PLL_36XX_RATE(180000000, 45, 3, 1, 0),
+	PLL_36XX_RATE(73728000, 73, 3, 3, 47710),
+	PLL_36XX_RATE(67737600, 90, 4, 3, 20762),
+	PLL_36XX_RATE(49152000, 49, 3, 3, 9962),
+	PLL_36XX_RATE(45158400, 45, 3, 3, 10381),
+	PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
+};
+
 /* register exynox5250 clocks */
 void __init exynos5250_clk_init(struct device_node *np)
 {
 	void __iomem *reg_base;
 	struct clk *apll, *mpll, *epll, *vpll, *bpll, *gpll, *cpll;
+	struct clk *vpllsrc;
+	unsigned long fin_pll_rate, mout_vpllsrc_rate = 0;
 
 	if (np) {
 		reg_base = of_iomap(np, 0);
@@ -496,6 +519,11 @@ void __init exynos5250_clk_init(struct device_node *np)
 	samsung_clk_register_mux(exynos5250_pll_pmux_clks,
 			ARRAY_SIZE(exynos5250_pll_pmux_clks));
 
+	fin_pll_rate = _get_rate("fin_pll");
+	vpllsrc = __clk_lookup("mout_vpllsrc");
+	if (vpllsrc)
+		mout_vpllsrc_rate = clk_get_rate(vpllsrc);
+
 	apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
 			reg_base, NULL, 0);
 	mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
@@ -506,10 +534,29 @@ void __init exynos5250_clk_init(struct device_node *np)
 			reg_base + 0x10050, NULL, 0);
 	cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
 			reg_base + 0x10020, NULL, 0);
-	epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
-			reg_base + 0x10030, NULL, 0);
-	vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
+
+	if (fin_pll_rate == (24 * MHZ)) {
+		epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
+				reg_base + 0x10030, epll_24mhz_tbl,
+				ARRAY_SIZE(epll_24mhz_tbl));
+	} else {
+		pr_warn("%s: valid epll rate_table missing for\n"
+			"parent fin_pll:%lu hz\n", __func__, fin_pll_rate);
+		epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
+				reg_base + 0x10030, NULL, 0);
+	}
+
+	if (mout_vpllsrc_rate == (24 * MHZ)) {
+		vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc"
+			, reg_base + 0x10040, vpll_24mhz_tbl,
+			ARRAY_SIZE(vpll_24mhz_tbl));
+	} else {
+		pr_warn("%s: valid vpll rate_table missing for\n"
+			"parent mout_vpllsrc_rate:%lu hz\n", __func__,
+			mout_vpllsrc_rate);
+		samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
 			reg_base + 0x10040, NULL, 0);
+	}
 
 	samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
 			ARRAY_SIZE(exynos5250_fixed_rate_clks));
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index e4ad6ea..c997649 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -20,6 +20,8 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 
+#define MHZ (1000*1000)
+
 /**
  * struct samsung_clock_alias: information about mux clock
  * @id: platform specific id of the clock.
-- 
1.7.0.4

[PATCH v4 6/6] clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC

[Doug Anderson]

Yadwinder,

On Mon, Jun 3, 2013 at 8:09 AM, Yadwinder Singh Brar
<yadi.brar@samsung.com> wrote:
> Adds the EPLL and VPLL freq table for exynos5250 SoC.
>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  drivers/clk/samsung/clk-exynos5250.c |   53 ++++++++++++++++++++++++++++++++--
>  drivers/clk/samsung/clk.h            |    2 +
>  2 files changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
> index 70cc6cf..f98c19d 100644
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -472,11 +472,34 @@ static __initdata struct of_device_id ext_clk_match[] = {
>         { },
>  };
>
> +static __initdata struct samsung_pll_rate_table vpll_24mhz_tbl[] = {
> +       /* sorted in descending order */
> +       /* PLL_36XX_RATE(rate, m, p, s, k) */
> +       PLL_36XX_RATE(266000000, 266, 3, 3, 0),
> +       /* Not in UM, but need for eDP on snow */
> +       PLL_36XX_RATE(70500000, 94, 2, 4, 0),
> +};
> +
> +static __initdata struct samsung_pll_rate_table epll_24mhz_tbl[] = {
> +       /* sorted in descending order */
> +       /* PLL_36XX_RATE(rate, m, p, s, k) */
> +       PLL_36XX_RATE(192000000, 48, 3, 1, 0),
> +       PLL_36XX_RATE(180633600, 45, 3, 1, 10381),
> +       PLL_36XX_RATE(180000000, 45, 3, 1, 0),
> +       PLL_36XX_RATE(73728000, 73, 3, 3, 47710),
> +       PLL_36XX_RATE(67737600, 90, 4, 3, 20762),
> +       PLL_36XX_RATE(49152000, 49, 3, 3, 9962),
> +       PLL_36XX_RATE(45158400, 45, 3, 3, 10381),
> +       PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
> +};
> +
>  /* register exynox5250 clocks */
>  void __init exynos5250_clk_init(struct device_node *np)
>  {
>         void __iomem *reg_base;
>         struct clk *apll, *mpll, *epll, *vpll, *bpll, *gpll, *cpll;
> +       struct clk *vpllsrc;
> +       unsigned long fin_pll_rate, mout_vpllsrc_rate = 0;
>
>         if (np) {
>                 reg_base = of_iomap(np, 0);
> @@ -496,6 +519,11 @@ void __init exynos5250_clk_init(struct device_node *np)
>         samsung_clk_register_mux(exynos5250_pll_pmux_clks,
>                         ARRAY_SIZE(exynos5250_pll_pmux_clks));
>
> +       fin_pll_rate = _get_rate("fin_pll");
> +       vpllsrc = __clk_lookup("mout_vpllsrc");
> +       if (vpllsrc)
> +               mout_vpllsrc_rate = clk_get_rate(vpllsrc);
> +
>         apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
>                         reg_base, NULL, 0);
>         mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> @@ -506,10 +534,29 @@ void __init exynos5250_clk_init(struct device_node *np)
>                         reg_base + 0x10050, NULL, 0);
>         cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
>                         reg_base + 0x10020, NULL, 0);
> -       epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
> -                       reg_base + 0x10030, NULL, 0);
> -       vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
> +
> +       if (fin_pll_rate == (24 * MHZ)) {
> +               epll = samsung_clk_register_pll36xx("fout_epll", "fin_pll",
> +                               reg_base + 0x10030, epll_24mhz_tbl,
> +                               ARRAY_SIZE(epll_24mhz_tbl));
> +       } else {
> +               pr_warn("%s: valid epll rate_table missing for\n"
> +                       "parent fin_pll:%lu hz\n", __func__, fin_pll_rate);

It seems like we could just have a warning once at the top of this
file.  ...and since we think nobody has designed a 5250 with a 26MHz
input clock we could even just consider it an error at the moment to
avoid adding a bunch of code.

You could also avoid all of these "if" statements with a level of indirection.

enum {
  EPLL, VPLL
};

samsung_pll_rate_table *plls_24mhz[] = { epll_24mhz_tbl, vpll_24mhz_tbl };
samsung_pll_rate_table *plls_default[] = { };

...of course you'd need a parallel table for sizes.  That does suggest
that Tomasz's thought of terminating the list with a sentinal would be
cleaner.

-Doug