[RESEND PATCH 0/5] Add generic set_rate clk_ops for PLL35XX and PLL36XX for samsung SoCs

[RESEND PATCH 0/5] Add generic set_rate clk_ops for PLL35XX and PLL36XX for samsung SoCs

[Vikas Sajjan]

This patch series does the following: 

 1) Factors out possible common code, unifies the clk strutures used
    for PLL35XX & PLL36XX and usues clk->base instead of clk->con0

 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 PLL36XXX

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

And tested these patch on chromebook for EPLL settings for Audio on our chrome tree.

Vikas Sajjan (2):
  clk: samsung: Add set_rate() clk_ops for PLL36XX
  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 |   29 +++-
 drivers/clk/samsung/clk-pll.c        |  243 ++++++++++++++++++++++++++++++----
 drivers/clk/samsung/clk-pll.h        |   27 +++-
 4 files changed, 272 insertions(+), 37 deletions(-)

-- 
1.7.9.5

[RESEND PATCH 0/5] Add generic set_rate clk_ops for PLL35XX and PLL36XX for samsung SoCs

[Vikas Sajjan]

This patch series does the following: 

 1) Factors out possible common code, unifies the clk strutures used
    for PLL35XX & PLL36XX and usues clk->base instead of clk->con0

 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 PLL36XXX

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

And tested these patch on chromebook for EPLL settings for Audio on our chrome tree.

Vikas Sajjan (2):
  clk: samsung: Add set_rate() clk_ops for PLL36XX
  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 |   29 +++-
 drivers/clk/samsung/clk-pll.c        |  243 ++++++++++++++++++++++++++++++----
 drivers/clk/samsung/clk-pll.h        |   27 +++-
 4 files changed, 272 insertions(+), 37 deletions(-)

-- 
1.7.9.5

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

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

To factor out possible common code, this patch unifies the clk strutures used
for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.

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 d0940e6..cf7d4e7 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
@@ -1019,13 +1021,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..01f17cf 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.9.5

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

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

To factor out possible common code, this patch unifies the clk strutures used
for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.

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 d0940e6..cf7d4e7 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
@@ -1019,13 +1021,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..01f17cf 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.9.5

[RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

This patch 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.
It also sorts the rate table while registering the PLL rate table.
So that this sorted table can be used for making the searching of "required
rate" efficient.

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        |   35 ++++++++++++++++++++++++++++++++--
 drivers/clk/samsung/clk-pll.h        |   27 ++++++++++++++++++++++++--
 4 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index cf7d4e7..beff8a1 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -1021,13 +1021,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 01f17cf..b8c0260 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -10,12 +10,15 @@
 */
 
 #include <linux/errno.h>
+#include <linux/sort.h>
 #include "clk.h"
 #include "clk-pll.h"
 
 struct samsung_clk_pll {
 	struct clk_hw		hw;
 	const void __iomem	*base;
+	struct samsung_pll_rate_table *rate_table;
+	unsigned int rate_count;
 };
 
 #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
@@ -25,6 +28,14 @@ struct samsung_clk_pll {
 #define pll_writel(pll, val, offset)					\
 		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
 
+static int samsung_compare_rate(const void *_a, const void *_b)
+{
+	const struct samsung_pll_rate_table *a = _a;
+	const struct samsung_pll_rate_table *b = _b;
+
+	return a->rate - b->rate;
+}
+
 /*
  * PLL35xx Clock Type
  */
@@ -62,7 +73,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,
+			struct samsung_pll_rate_table *rate_table,
+			const unsigned int rate_count)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -82,6 +95,14 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 
 	pll->hw.init = &init;
 	pll->base = base;
+	pll->rate_table = rate_table;
+	pll->rate_count = rate_count;
+
+	if (pll->rate_table && pll->rate_count) {
+		sort(pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate, NULL);
+	}
 
 	clk = clk_register(NULL, &pll->hw);
 	if (IS_ERR(clk)) {
@@ -137,7 +158,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,
+			struct samsung_pll_rate_table *rate_table,
+			const unsigned int rate_count)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -157,6 +180,14 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 
 	pll->hw.init = &init;
 	pll->base = base;
+	pll->rate_table = rate_table;
+	pll->rate_count = rate_count;
+
+	if (pll->rate_table && pll->rate_count) {
+		sort(pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate, NULL);
+	}
 
 	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 1329522..b5e12430 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -12,6 +12,25 @@
 #ifndef __SAMSUNG_CLK_PLL_H
 #define __SAMSUNG_CLK_PLL_H
 
+#define PLL_35XX_RATE(_rate, _m, _p, _s)			\
+	{							\
+		.rate = _rate,					\
+		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
+	}
+
+#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)			\
+	{							\
+		.rate = _rate,					\
+		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
+		.pll_con1 = _k,					\
+	}
+
+struct samsung_pll_rate_table {
+	unsigned  int rate;
+	u32 pll_con0;
+	u32 pll_con1;
+};
+
 enum pll45xx_type {
 	pll_4500,
 	pll_4502,
@@ -25,9 +44,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,
+			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,
+			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.9.5

[RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

This patch 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.
It also sorts the rate table while registering the PLL rate table.
So that this sorted table can be used for making the searching of "required
rate" efficient.

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        |   35 ++++++++++++++++++++++++++++++++--
 drivers/clk/samsung/clk-pll.h        |   27 ++++++++++++++++++++++++--
 4 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index cf7d4e7..beff8a1 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -1021,13 +1021,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 01f17cf..b8c0260 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -10,12 +10,15 @@
 */
 
 #include <linux/errno.h>
+#include <linux/sort.h>
 #include "clk.h"
 #include "clk-pll.h"
 
 struct samsung_clk_pll {
 	struct clk_hw		hw;
 	const void __iomem	*base;
+	struct samsung_pll_rate_table *rate_table;
+	unsigned int rate_count;
 };
 
 #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
@@ -25,6 +28,14 @@ struct samsung_clk_pll {
 #define pll_writel(pll, val, offset)					\
 		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
 
+static int samsung_compare_rate(const void *_a, const void *_b)
+{
+	const struct samsung_pll_rate_table *a = _a;
+	const struct samsung_pll_rate_table *b = _b;
+
+	return a->rate - b->rate;
+}
+
 /*
  * PLL35xx Clock Type
  */
@@ -62,7 +73,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,
+			struct samsung_pll_rate_table *rate_table,
+			const unsigned int rate_count)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -82,6 +95,14 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 
 	pll->hw.init = &init;
 	pll->base = base;
+	pll->rate_table = rate_table;
+	pll->rate_count = rate_count;
+
+	if (pll->rate_table && pll->rate_count) {
+		sort(pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate, NULL);
+	}
 
 	clk = clk_register(NULL, &pll->hw);
 	if (IS_ERR(clk)) {
@@ -137,7 +158,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,
+			struct samsung_pll_rate_table *rate_table,
+			const unsigned int rate_count)
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
@@ -157,6 +180,14 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 
 	pll->hw.init = &init;
 	pll->base = base;
+	pll->rate_table = rate_table;
+	pll->rate_count = rate_count;
+
+	if (pll->rate_table && pll->rate_count) {
+		sort(pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate, NULL);
+	}
 
 	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 1329522..b5e12430 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -12,6 +12,25 @@
 #ifndef __SAMSUNG_CLK_PLL_H
 #define __SAMSUNG_CLK_PLL_H
 
+#define PLL_35XX_RATE(_rate, _m, _p, _s)			\
+	{							\
+		.rate = _rate,					\
+		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
+	}
+
+#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)			\
+	{							\
+		.rate = _rate,					\
+		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
+		.pll_con1 = _k,					\
+	}
+
+struct samsung_pll_rate_table {
+	unsigned  int rate;
+	u32 pll_con0;
+	u32 pll_con1;
+};
+
 enum pll45xx_type {
 	pll_4500,
 	pll_4502,
@@ -25,9 +44,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,
+			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,
+			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.9.5

[RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

Adds set_rate() and round_rate() clk_ops for PLL35xx

The round_rate() implemenation as of now is dummy, it returns the same rate
which is passed as input.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/clk/samsung/clk-pll.c |   95 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index b8c0260..291cc9e 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -11,6 +11,7 @@
 
 #include <linux/errno.h>
 #include <linux/sort.h>
+#include <linux/bsearch.h>
 #include "clk.h"
 #include "clk-pll.h"
 
@@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const void *_b)
 	return a->rate - b->rate;
 }
 
+static struct samsung_pll_rate_table *samsung_get_pll_settings(
+				struct samsung_clk_pll *pll, unsigned long rate)
+{
+	struct samsung_pll_rate_table req_rate, *tmp;
+
+	req_rate.rate = rate;
+	tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate);
+	if (tmp)
+		return tmp;
+
+	return NULL;
+}
+
 /*
  * PLL35xx Clock Type
  */
@@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const void *_b)
 #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)
+
+#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT))
+#define PLL35XX_PDIV(_tmp) ((_tmp) & (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT))
+#define PLL35XX_SDIV(_tmp) ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
 
 static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -68,8 +90,76 @@ static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
-static const struct clk_ops samsung_pll35xx_clk_ops = {
+static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
+{
+	if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv != PLL35XX_PDIV(pll_con)))
+		return 1;
+	else
+		return 0;
+}
+
+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);
+	struct samsung_pll_rate_table *rate;
+
+	u32 tmp, mdiv, pdiv, sdiv;
+
+	/* 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;
+	}
+
+	mdiv = PLL35XX_MDIV(rate->pll_con0);
+	pdiv = PLL35XX_PDIV(rate->pll_con0);
+	sdiv = PLL35XX_SDIV(rate->pll_con0);
+
+	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+
+	if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
+		/* If only s change, change just s value only*/
+		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
+		tmp |= sdiv;
+		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+	} else {
+		/* Set PLL lock time.
+		   Maximum lock time can be 270 * PDIV cycles */
+		pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
+				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 |= mdiv | pdiv | sdiv;
+		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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
+			unsigned long drate, unsigned long *prate)
+{
+	/* Clock framework cries without this, so implemented dummy */
+	return drate;
+}
+
+static struct clk_ops samsung_pll35xx_clk_ops = {
 	.recalc_rate = samsung_pll35xx_recalc_rate,
+	.round_rate = samsung_pll35xx_round_rate,
+	.set_rate = samsung_pll35xx_set_rate,
 };
 
 struct clk * __init samsung_clk_register_pll35xx(const char *name,
@@ -102,6 +192,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 		sort(pll->rate_table, pll->rate_count,
 			sizeof(struct samsung_pll_rate_table),
 			samsung_compare_rate, NULL);
+	} else {
+		samsung_pll35xx_clk_ops.round_rate = NULL;
+		samsung_pll35xx_clk_ops.set_rate = NULL;
 	}
 
 	clk = clk_register(NULL, &pll->hw);
-- 
1.7.9.5

[RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Vikas Sajjan]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

Adds set_rate() and round_rate() clk_ops for PLL35xx

The round_rate() implemenation as of now is dummy, it returns the same rate
which is passed as input.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/clk/samsung/clk-pll.c |   95 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index b8c0260..291cc9e 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -11,6 +11,7 @@
 
 #include <linux/errno.h>
 #include <linux/sort.h>
+#include <linux/bsearch.h>
 #include "clk.h"
 #include "clk-pll.h"
 
@@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const void *_b)
 	return a->rate - b->rate;
 }
 
+static struct samsung_pll_rate_table *samsung_get_pll_settings(
+				struct samsung_clk_pll *pll, unsigned long rate)
+{
+	struct samsung_pll_rate_table req_rate, *tmp;
+
+	req_rate.rate = rate;
+	tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
+			sizeof(struct samsung_pll_rate_table),
+			samsung_compare_rate);
+	if (tmp)
+		return tmp;
+
+	return NULL;
+}
+
 /*
  * PLL35xx Clock Type
  */
@@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const void *_b)
 #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)
+
+#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT))
+#define PLL35XX_PDIV(_tmp) ((_tmp) & (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT))
+#define PLL35XX_SDIV(_tmp) ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
 
 static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -68,8 +90,76 @@ static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
-static const struct clk_ops samsung_pll35xx_clk_ops = {
+static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
+{
+	if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv != PLL35XX_PDIV(pll_con)))
+		return 1;
+	else
+		return 0;
+}
+
+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);
+	struct samsung_pll_rate_table *rate;
+
+	u32 tmp, mdiv, pdiv, sdiv;
+
+	/* 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;
+	}
+
+	mdiv = PLL35XX_MDIV(rate->pll_con0);
+	pdiv = PLL35XX_PDIV(rate->pll_con0);
+	sdiv = PLL35XX_SDIV(rate->pll_con0);
+
+	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+
+	if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
+		/* If only s change, change just s value only*/
+		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
+		tmp |= sdiv;
+		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+	} else {
+		/* Set PLL lock time.
+		   Maximum lock time can be 270 * PDIV cycles */
+		pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
+				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 |= mdiv | pdiv | sdiv;
+		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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
+			unsigned long drate, unsigned long *prate)
+{
+	/* Clock framework cries without this, so implemented dummy */
+	return drate;
+}
+
+static struct clk_ops samsung_pll35xx_clk_ops = {
 	.recalc_rate = samsung_pll35xx_recalc_rate,
+	.round_rate = samsung_pll35xx_round_rate,
+	.set_rate = samsung_pll35xx_set_rate,
 };
 
 struct clk * __init samsung_clk_register_pll35xx(const char *name,
@@ -102,6 +192,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
 		sort(pll->rate_table, pll->rate_count,
 			sizeof(struct samsung_pll_rate_table),
 			samsung_compare_rate, NULL);
+	} else {
+		samsung_pll35xx_clk_ops.round_rate = NULL;
+		samsung_pll35xx_clk_ops.set_rate = NULL;
 	}
 
 	clk = clk_register(NULL, &pll->hw);
-- 
1.7.9.5

[RESEND PATCH 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Vikas Sajjan]

This patch adds set_rate and round_rate clk_ops for PLL36xx
The round_rate() implementation as of now is dummy, it returns the same rate
which is passed as input.

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/clk/samsung/clk-pll.c |   67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 291cc9e..55ff5fd 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -224,6 +224,13 @@ 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)
+
+#define PLL36XX_MDIV(_tmp) ((_tmp) & (PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT))
+#define PLL36XX_PDIV(_tmp) ((_tmp) & (PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT))
+#define PLL36XX_SDIV(_tmp) ((_tmp) & (PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT))
+#define PLL36XX_KDIV(_tmp) ((_tmp) & (PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT))
 
 static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -246,8 +253,65 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
+static long samsung_pll36xx_round_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long *prate)
+{
+	/* retruns the same 'drate' whichs comes as input */
+	return drate;
+}
+
+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, mdiv, pdiv, sdiv, kdiv;
+	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;
+	}
+
+	mdiv = PLL36XX_MDIV(rate->pll_con0);
+	pdiv = PLL36XX_PDIV(rate->pll_con0);
+	sdiv = PLL36XX_SDIV(rate->pll_con0);
+	kdiv = PLL36XX_KDIV(rate->pll_con1);
+
+	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+	pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+
+	/* Set PLL lock time.
+	   Maximum lock time can be 3000 * PDIV cycles */
+	pll_writel(pll, ((pdiv >> PLL36XX_PDIV_SHIFT) * 3000),
+			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 |= mdiv | pdiv | sdiv;
+	pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+	pll_con1 |= kdiv;
+	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_pll36xx_round_rate,
 };
 
 struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -280,6 +344,9 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 		sort(pll->rate_table, pll->rate_count,
 			sizeof(struct samsung_pll_rate_table),
 			samsung_compare_rate, NULL);
+	} else {
+		samsung_pll35xx_clk_ops.round_rate = NULL;
+		samsung_pll35xx_clk_ops.set_rate = NULL;
 	}
 
 	clk = clk_register(NULL, &pll->hw);
-- 
1.7.9.5

[RESEND PATCH 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Vikas Sajjan]

This patch adds set_rate and round_rate clk_ops for PLL36xx
The round_rate() implementation as of now is dummy, it returns the same rate
which is passed as input.

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/clk/samsung/clk-pll.c |   67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 291cc9e..55ff5fd 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -224,6 +224,13 @@ 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)
+
+#define PLL36XX_MDIV(_tmp) ((_tmp) & (PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT))
+#define PLL36XX_PDIV(_tmp) ((_tmp) & (PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT))
+#define PLL36XX_SDIV(_tmp) ((_tmp) & (PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT))
+#define PLL36XX_KDIV(_tmp) ((_tmp) & (PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT))
 
 static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -246,8 +253,65 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
+static long samsung_pll36xx_round_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long *prate)
+{
+	/* retruns the same 'drate' whichs comes as input */
+	return drate;
+}
+
+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, mdiv, pdiv, sdiv, kdiv;
+	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;
+	}
+
+	mdiv = PLL36XX_MDIV(rate->pll_con0);
+	pdiv = PLL36XX_PDIV(rate->pll_con0);
+	sdiv = PLL36XX_SDIV(rate->pll_con0);
+	kdiv = PLL36XX_KDIV(rate->pll_con1);
+
+	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+	pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+
+	/* Set PLL lock time.
+	   Maximum lock time can be 3000 * PDIV cycles */
+	pll_writel(pll, ((pdiv >> PLL36XX_PDIV_SHIFT) * 3000),
+			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 |= mdiv | pdiv | sdiv;
+	pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+	pll_con1 |= kdiv;
+	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_pll36xx_round_rate,
 };
 
 struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -280,6 +344,9 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 		sort(pll->rate_table, pll->rate_count,
 			sizeof(struct samsung_pll_rate_table),
 			samsung_compare_rate, NULL);
+	} else {
+		samsung_pll35xx_clk_ops.round_rate = NULL;
+		samsung_pll35xx_clk_ops.set_rate = NULL;
 	}
 
 	clk = clk_register(NULL, &pll->hw);
-- 
1.7.9.5

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

[Vikas Sajjan]

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 |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index ddf10ca..00d1fa6 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -469,6 +469,21 @@ static __initdata struct of_device_id ext_clk_match[] = {
 	{ },
 };
 
+static struct samsung_pll_rate_table vpll_tbl[] = {
+	PLL_36XX_RATE(70500000, 2, 94, 4, 0),
+};
+
+static struct samsung_pll_rate_table epll_tbl[] = {
+	PLL_36XX_RATE(192000000, 48, 3, 1, 0),
+	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(180633600, 45, 3, 1, 10381),
+	PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
+};
+
 /* register exynox5250 clocks */
 void __init exynos5250_clk_init(struct device_node *np)
 {
@@ -501,9 +516,9 @@ void __init exynos5250_clk_init(struct device_node *np)
 	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);
+			reg_base + 0x10030, epll_tbl, ARRAY_SIZE(epll_tbl));
 	vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
-			reg_base + 0x10040, NULL, 0);
+			reg_base + 0x10040, vpll_tbl, ARRAY_SIZE(vpll_tbl));
 
 	samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
 			ARRAY_SIZE(exynos5250_fixed_rate_clks));
-- 
1.7.9.5

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

[Vikas Sajjan]

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 |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index ddf10ca..00d1fa6 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -469,6 +469,21 @@ static __initdata struct of_device_id ext_clk_match[] = {
 	{ },
 };
 
+static struct samsung_pll_rate_table vpll_tbl[] = {
+	PLL_36XX_RATE(70500000, 2, 94, 4, 0),
+};
+
+static struct samsung_pll_rate_table epll_tbl[] = {
+	PLL_36XX_RATE(192000000, 48, 3, 1, 0),
+	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(180633600, 45, 3, 1, 10381),
+	PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
+};
+
 /* register exynox5250 clocks */
 void __init exynos5250_clk_init(struct device_node *np)
 {
@@ -501,9 +516,9 @@ void __init exynos5250_clk_init(struct device_node *np)
 	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);
+			reg_base + 0x10030, epll_tbl, ARRAY_SIZE(epll_tbl));
 	vpll = samsung_clk_register_pll36xx("fout_vpll", "mout_vpllsrc",
-			reg_base + 0x10040, NULL, 0);
+			reg_base + 0x10040, vpll_tbl, ARRAY_SIZE(vpll_tbl));
 
 	samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
 			ARRAY_SIZE(exynos5250_fixed_rate_clks));
-- 
1.7.9.5

Re: [RESEND PATCH 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:14 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> To factor out possible common code, this patch unifies the clk strutures
> used for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.
> 
> 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(-)
> 

Whether this patch really allows to factor out any significant amount of 
common code is rather discussible, but generally looks fine.

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

Best regards,
Tomasz

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

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:14 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> To factor out possible common code, this patch unifies the clk strutures
> used for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.
> 
> 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(-)
> 

Whether this patch really allows to factor out any significant amount of 
common code is rather discussible, but generally looks fine.

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

Best regards,
Tomasz

Re: [RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Tomasz Figa]

Hi,

Please see my comments inline.

On Friday 24 of May 2013 16:01:15 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> This patch 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.
> It also sorts the rate table while registering the PLL rate table.
> So that this sorted table can be used for making the searching of
> "required rate" efficient.
> 
> 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        |   35
> ++++++++++++++++++++++++++++++++-- drivers/clk/samsung/clk-pll.h       
> |   27 ++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 15
> deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c
> b/drivers/clk/samsung/clk-exynos4.c index cf7d4e7..beff8a1 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -1021,13 +1021,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 01f17cf..b8c0260 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -10,12 +10,15 @@
>  */
> 
>  #include <linux/errno.h>
> +#include <linux/sort.h>
>  #include "clk.h"
>  #include "clk-pll.h"
> 
>  struct samsung_clk_pll {
>  	struct clk_hw		hw;
>  	const void __iomem	*base;
> +	struct samsung_pll_rate_table *rate_table;
> +	unsigned int rate_count;
>  };
> 
>  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
> @@ -25,6 +28,14 @@ struct samsung_clk_pll {
>  #define pll_writel(pll, val, offset)					\
>  		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
> 
> +static int samsung_compare_rate(const void *_a, const void *_b)
> +{
> +	const struct samsung_pll_rate_table *a = _a;
> +	const struct samsung_pll_rate_table *b = _b;
> +
> +	return a->rate - b->rate;
> +}
> +
>  /*
>   * PLL35xx Clock Type
>   */
> @@ -62,7 +73,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,
> +			struct samsung_pll_rate_table *rate_table,
> +			const unsigned int rate_count)
>  {
>  	struct samsung_clk_pll *pll;
>  	struct clk *clk;
> @@ -82,6 +95,14 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name,
> 
>  	pll->hw.init = &init;
>  	pll->base = base;
> +	pll->rate_table = rate_table;
> +	pll->rate_count = rate_count;
> +
> +	if (pll->rate_table && pll->rate_count) {

You seem to assume here that the exact set of rates is known at 
compilation time. In other words, you assume that all boards have the same 
PLL input (xtal) frequency.

We have similar patches available in our internal tree that work in a bit 
different way:

1) they assume that passed array is already sorted, which is a good 
assumption, because you have full control of this array - it's a part of 
the clock driver and if it's incorrect it's your fault - you don't deal 
with data input by user

2) passed array don't have defined rates, but rather only all the relevant 
PLL coefficients; the frequency for each entry is calculated at 
registration time, based on rate of parent clock

3) the table is NULL terminated, without the need to pass its size, but 
it's just a matter of preference

> +		sort(pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate, NULL);
> +	}
> 
>  	clk = clk_register(NULL, &pll->hw);
>  	if (IS_ERR(clk)) {
> @@ -137,7 +158,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,
> +			struct samsung_pll_rate_table *rate_table,
> +			const unsigned int rate_count)
>  {
>  	struct samsung_clk_pll *pll;
>  	struct clk *clk;
> @@ -157,6 +180,14 @@ struct clk * __init
> samsung_clk_register_pll36xx(const char *name,
> 
>  	pll->hw.init = &init;
>  	pll->base = base;
> +	pll->rate_table = rate_table;
> +	pll->rate_count = rate_count;
> +
> +	if (pll->rate_table && pll->rate_count) {
> +		sort(pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate, NULL);
> +	}
> 
>  	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 1329522..b5e12430 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -12,6 +12,25 @@
>  #ifndef __SAMSUNG_CLK_PLL_H
>  #define __SAMSUNG_CLK_PLL_H
> 
> +#define PLL_35XX_RATE(_rate, _m, _p, _s)			\
> +	{							\
> +		.rate = _rate,					\
> +		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
> +	}
> +
> +#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)			\
> +	{							\
> +		.rate = _rate,					\
> +		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
> +		.pll_con1 = _k,					\
> +	}
> +
> +struct samsung_pll_rate_table {
> +	unsigned  int rate;
> +	u32 pll_con0;
> +	u32 pll_con1;

I don't like this struct. What about PLLs that don't have con0 or con1, 
but rather a completely weird register strukture like PLL_P_REG, 
PLL_M_REG, PLL_S_REG? The structure has generic name, which suggests that 
it should be able to handle any future Samsung PLLs as well.

I'd suggest storing all coefficients as separate fields of the struct, 
e.g.

struct samsung_pll_rate_table {
	unsigned  int rate;
	/* ... */
	unsigned int p;
	unsigned int m;
	unsigned int s;
	/* ... */
};

Best regards,
Tomasz

> +};
> +
>  enum pll45xx_type {
>  	pll_4500,
>  	pll_4502,
> @@ -25,9 +44,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,
> +			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,
> +			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);

[RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Tomasz Figa]

Hi,

Please see my comments inline.

On Friday 24 of May 2013 16:01:15 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> This patch 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.
> It also sorts the rate table while registering the PLL rate table.
> So that this sorted table can be used for making the searching of
> "required rate" efficient.
> 
> 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        |   35
> ++++++++++++++++++++++++++++++++-- drivers/clk/samsung/clk-pll.h       
> |   27 ++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 15
> deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c
> b/drivers/clk/samsung/clk-exynos4.c index cf7d4e7..beff8a1 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -1021,13 +1021,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 01f17cf..b8c0260 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -10,12 +10,15 @@
>  */
> 
>  #include <linux/errno.h>
> +#include <linux/sort.h>
>  #include "clk.h"
>  #include "clk-pll.h"
> 
>  struct samsung_clk_pll {
>  	struct clk_hw		hw;
>  	const void __iomem	*base;
> +	struct samsung_pll_rate_table *rate_table;
> +	unsigned int rate_count;
>  };
> 
>  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
> @@ -25,6 +28,14 @@ struct samsung_clk_pll {
>  #define pll_writel(pll, val, offset)					\
>  		__raw_writel(val, (void __iomem *)(pll->base + (offset)));
> 
> +static int samsung_compare_rate(const void *_a, const void *_b)
> +{
> +	const struct samsung_pll_rate_table *a = _a;
> +	const struct samsung_pll_rate_table *b = _b;
> +
> +	return a->rate - b->rate;
> +}
> +
>  /*
>   * PLL35xx Clock Type
>   */
> @@ -62,7 +73,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,
> +			struct samsung_pll_rate_table *rate_table,
> +			const unsigned int rate_count)
>  {
>  	struct samsung_clk_pll *pll;
>  	struct clk *clk;
> @@ -82,6 +95,14 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name,
> 
>  	pll->hw.init = &init;
>  	pll->base = base;
> +	pll->rate_table = rate_table;
> +	pll->rate_count = rate_count;
> +
> +	if (pll->rate_table && pll->rate_count) {

You seem to assume here that the exact set of rates is known at 
compilation time. In other words, you assume that all boards have the same 
PLL input (xtal) frequency.

We have similar patches available in our internal tree that work in a bit 
different way:

1) they assume that passed array is already sorted, which is a good 
assumption, because you have full control of this array - it's a part of 
the clock driver and if it's incorrect it's your fault - you don't deal 
with data input by user

2) passed array don't have defined rates, but rather only all the relevant 
PLL coefficients; the frequency for each entry is calculated at 
registration time, based on rate of parent clock

3) the table is NULL terminated, without the need to pass its size, but 
it's just a matter of preference

> +		sort(pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate, NULL);
> +	}
> 
>  	clk = clk_register(NULL, &pll->hw);
>  	if (IS_ERR(clk)) {
> @@ -137,7 +158,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,
> +			struct samsung_pll_rate_table *rate_table,
> +			const unsigned int rate_count)
>  {
>  	struct samsung_clk_pll *pll;
>  	struct clk *clk;
> @@ -157,6 +180,14 @@ struct clk * __init
> samsung_clk_register_pll36xx(const char *name,
> 
>  	pll->hw.init = &init;
>  	pll->base = base;
> +	pll->rate_table = rate_table;
> +	pll->rate_count = rate_count;
> +
> +	if (pll->rate_table && pll->rate_count) {
> +		sort(pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate, NULL);
> +	}
> 
>  	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 1329522..b5e12430 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -12,6 +12,25 @@
>  #ifndef __SAMSUNG_CLK_PLL_H
>  #define __SAMSUNG_CLK_PLL_H
> 
> +#define PLL_35XX_RATE(_rate, _m, _p, _s)			\
> +	{							\
> +		.rate = _rate,					\
> +		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
> +	}
> +
> +#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)			\
> +	{							\
> +		.rate = _rate,					\
> +		.pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),	\
> +		.pll_con1 = _k,					\
> +	}
> +
> +struct samsung_pll_rate_table {
> +	unsigned  int rate;
> +	u32 pll_con0;
> +	u32 pll_con1;

I don't like this struct. What about PLLs that don't have con0 or con1, 
but rather a completely weird register strukture like PLL_P_REG, 
PLL_M_REG, PLL_S_REG? The structure has generic name, which suggests that 
it should be able to handle any future Samsung PLLs as well.

I'd suggest storing all coefficients as separate fields of the struct, 
e.g.

struct samsung_pll_rate_table {
	unsigned  int rate;
	/* ... */
	unsigned int p;
	unsigned int m;
	unsigned int s;
	/* ... */
};

Best regards,
Tomasz

> +};
> +
>  enum pll45xx_type {
>  	pll_4500,
>  	pll_4502,
> @@ -25,9 +44,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,
> +			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,
> +			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);

Re: [RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:16 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> Adds set_rate() and round_rate() clk_ops for PLL35xx
> 
> The round_rate() implemenation as of now is dummy, it returns the same
> rate which is passed as input.
> 
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c |   95
> ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94
> insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c
> b/drivers/clk/samsung/clk-pll.c index b8c0260..291cc9e 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -11,6 +11,7 @@
> 
>  #include <linux/errno.h>
>  #include <linux/sort.h>
> +#include <linux/bsearch.h>
>  #include "clk.h"
>  #include "clk-pll.h"
> 
> @@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const
> void *_b) return a->rate - b->rate;
>  }
> 
> +static struct samsung_pll_rate_table *samsung_get_pll_settings(
> +				struct samsung_clk_pll *pll, unsigned long 
rate)
> +{
> +	struct samsung_pll_rate_table req_rate, *tmp;
> +
> +	req_rate.rate = rate;
> +	tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate);

Binary search over < 10 entries? Isn't it a bit of overkill?

> +	if (tmp)
> +		return tmp;
> +
> +	return NULL;
> +}
> +
>  /*
>   * PLL35xx Clock Type
>   */
> @@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const
> void *_b) #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)
> +
> +#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK <<
> PLL35XX_MDIV_SHIFT)) +#define PLL35XX_PDIV(_tmp) ((_tmp) &
> (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)) +#define PLL35XX_SDIV(_tmp)
> ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
> 
>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>  				unsigned long parent_rate)
> @@ -68,8 +90,76 @@ static unsigned long
> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
> long)fvco;
>  }
> 
> -static const struct clk_ops samsung_pll35xx_clk_ops = {
> +static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32
> pll_con) +{
> +	if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv !=
> PLL35XX_PDIV(pll_con))) +		return 1;
> +	else
> +		return 0;
> +}
> +
> +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);
> +	struct samsung_pll_rate_table *rate;
> +
> +	u32 tmp, mdiv, pdiv, sdiv;
> +
> +	/* 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;
> +	}
> +
> +	mdiv = PLL35XX_MDIV(rate->pll_con0);
> +	pdiv = PLL35XX_PDIV(rate->pll_con0);
> +	sdiv = PLL35XX_SDIV(rate->pll_con0);

You wouldn't need to use those macros if all coefficients were stored as 
separate fields in the struct.

> +
> +	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
> +
> +	if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
> +		/* If only s change, change just s value only*/
> +		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
> +		tmp |= sdiv;

This line is correct, but it looks like it wasn't, because:
a) the name suggests that it contains the raw value of S coefficient
b) it's real value is hidden between a macro, name of which suggests the 
same as in a) as well.

This makes the code hard to read.

> +		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
> +	} else {
> +		/* Set PLL lock time.
> +		   Maximum lock time can be 270 * PDIV cycles */
> +		pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
> +				PLL35XX_LOCK_OFFSET);

Hmm, magic constant in the code? Shouldn't it be defined as a macro?

> +
> +		/* Change PLL PMS values */
> +		tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
> +				(PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) 
|
> +				(PLL35XX_SDIV_MASK << 
PLL35XX_SDIV_SHIFT));
> +		tmp |= mdiv | pdiv | sdiv;

This looks strange as well, even if it's correct.

> +		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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate)
> +{
> +	/* Clock framework cries without this, so implemented dummy */

This is completely wrong. Have you tested this code or read how Common 
Clock Framework works?

clk_set_rate() first calls ->round_rate() to get a rate supported by the 
clock that is nearest and not higher than requested rate and only then it 
calls ->set_rate() with the rate returned by ->round_rate().

So the round_rate() callback must return the highest supported rate with 
parent clock at prate and not higher than drate.

> +	return drate;
> +}
> +
> +static struct clk_ops samsung_pll35xx_clk_ops = {
>  	.recalc_rate = samsung_pll35xx_recalc_rate,
> +	.round_rate = samsung_pll35xx_round_rate,
> +	.set_rate = samsung_pll35xx_set_rate,
>  };
> 
>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
> @@ -102,6 +192,9 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, sort(pll->rate_table,
> pll->rate_count,
>  			sizeof(struct samsung_pll_rate_table),
>  			samsung_compare_rate, NULL);
> +	} else {
> +		samsung_pll35xx_clk_ops.round_rate = NULL;
> +		samsung_pll35xx_clk_ops.set_rate = NULL;

This is completely wrong. You are changing a static structure that is used 
for all instances of PLL35xx, not only the one being registered at the 
moment.

Best regards,
Tomasz

>  	}
> 
>  	clk = clk_register(NULL, &pll->hw);

[RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:16 Vikas Sajjan wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> Adds set_rate() and round_rate() clk_ops for PLL35xx
> 
> The round_rate() implemenation as of now is dummy, it returns the same
> rate which is passed as input.
> 
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c |   95
> ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94
> insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c
> b/drivers/clk/samsung/clk-pll.c index b8c0260..291cc9e 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -11,6 +11,7 @@
> 
>  #include <linux/errno.h>
>  #include <linux/sort.h>
> +#include <linux/bsearch.h>
>  #include "clk.h"
>  #include "clk-pll.h"
> 
> @@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const
> void *_b) return a->rate - b->rate;
>  }
> 
> +static struct samsung_pll_rate_table *samsung_get_pll_settings(
> +				struct samsung_clk_pll *pll, unsigned long 
rate)
> +{
> +	struct samsung_pll_rate_table req_rate, *tmp;
> +
> +	req_rate.rate = rate;
> +	tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
> +			sizeof(struct samsung_pll_rate_table),
> +			samsung_compare_rate);

Binary search over < 10 entries? Isn't it a bit of overkill?

> +	if (tmp)
> +		return tmp;
> +
> +	return NULL;
> +}
> +
>  /*
>   * PLL35xx Clock Type
>   */
> @@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const
> void *_b) #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)
> +
> +#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK <<
> PLL35XX_MDIV_SHIFT)) +#define PLL35XX_PDIV(_tmp) ((_tmp) &
> (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)) +#define PLL35XX_SDIV(_tmp)
> ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
> 
>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>  				unsigned long parent_rate)
> @@ -68,8 +90,76 @@ static unsigned long
> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
> long)fvco;
>  }
> 
> -static const struct clk_ops samsung_pll35xx_clk_ops = {
> +static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32
> pll_con) +{
> +	if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv !=
> PLL35XX_PDIV(pll_con))) +		return 1;
> +	else
> +		return 0;
> +}
> +
> +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);
> +	struct samsung_pll_rate_table *rate;
> +
> +	u32 tmp, mdiv, pdiv, sdiv;
> +
> +	/* 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;
> +	}
> +
> +	mdiv = PLL35XX_MDIV(rate->pll_con0);
> +	pdiv = PLL35XX_PDIV(rate->pll_con0);
> +	sdiv = PLL35XX_SDIV(rate->pll_con0);

You wouldn't need to use those macros if all coefficients were stored as 
separate fields in the struct.

> +
> +	tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
> +
> +	if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
> +		/* If only s change, change just s value only*/
> +		tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
> +		tmp |= sdiv;

This line is correct, but it looks like it wasn't, because:
a) the name suggests that it contains the raw value of S coefficient
b) it's real value is hidden between a macro, name of which suggests the 
same as in a) as well.

This makes the code hard to read.

> +		pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
> +	} else {
> +		/* Set PLL lock time.
> +		   Maximum lock time can be 270 * PDIV cycles */
> +		pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
> +				PLL35XX_LOCK_OFFSET);

Hmm, magic constant in the code? Shouldn't it be defined as a macro?

> +
> +		/* Change PLL PMS values */
> +		tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
> +				(PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) 
|
> +				(PLL35XX_SDIV_MASK << 
PLL35XX_SDIV_SHIFT));
> +		tmp |= mdiv | pdiv | sdiv;

This looks strange as well, even if it's correct.

> +		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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate)
> +{
> +	/* Clock framework cries without this, so implemented dummy */

This is completely wrong. Have you tested this code or read how Common 
Clock Framework works?

clk_set_rate() first calls ->round_rate() to get a rate supported by the 
clock that is nearest and not higher than requested rate and only then it 
calls ->set_rate() with the rate returned by ->round_rate().

So the round_rate() callback must return the highest supported rate with 
parent clock at prate and not higher than drate.

> +	return drate;
> +}
> +
> +static struct clk_ops samsung_pll35xx_clk_ops = {
>  	.recalc_rate = samsung_pll35xx_recalc_rate,
> +	.round_rate = samsung_pll35xx_round_rate,
> +	.set_rate = samsung_pll35xx_set_rate,
>  };
> 
>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
> @@ -102,6 +192,9 @@ struct clk * __init
> samsung_clk_register_pll35xx(const char *name, sort(pll->rate_table,
> pll->rate_count,
>  			sizeof(struct samsung_pll_rate_table),
>  			samsung_compare_rate, NULL);
> +	} else {
> +		samsung_pll35xx_clk_ops.round_rate = NULL;
> +		samsung_pll35xx_clk_ops.set_rate = NULL;

This is completely wrong. You are changing a static structure that is used 
for all instances of PLL35xx, not only the one being registered at the 
moment.

Best regards,
Tomasz

>  	}
> 
>  	clk = clk_register(NULL, &pll->hw);

Re: [RESEND PATCH 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:17 Vikas Sajjan wrote:
> This patch adds set_rate and round_rate clk_ops for PLL36xx
> The round_rate() implementation as of now is dummy, it returns the same
> rate which is passed as input.
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  drivers/clk/samsung/clk-pll.c |   67
> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67
> insertions(+)
> 

I have exactly the same comments for this patch as for patch 3/5.

Best regards,
Tomasz

[RESEND PATCH 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Tomasz Figa]

Hi,

On Friday 24 of May 2013 16:01:17 Vikas Sajjan wrote:
> This patch adds set_rate and round_rate clk_ops for PLL36xx
> The round_rate() implementation as of now is dummy, it returns the same
> rate which is passed as input.
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  drivers/clk/samsung/clk-pll.c |   67
> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67
> insertions(+)
> 

I have exactly the same comments for this patch as for patch 3/5.

Best regards,
Tomasz

Re: [RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Yadwinder Singh Brar]

Hi Tomasz,

On Sat, May 25, 2013 at 3:34 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
> Please see my comments inline.
>
> On Friday 24 of May 2013 16:01:15 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> This patch 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.
>> It also sorts the rate table while registering the PLL rate table.
>> So that this sorted table can be used for making the searching of
>> "required rate" efficient.
>>
>> 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        |   35
>> ++++++++++++++++++++++++++++++++-- drivers/clk/samsung/clk-pll.h
>> |   27 ++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 15
>> deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c
>> b/drivers/clk/samsung/clk-exynos4.c index cf7d4e7..beff8a1 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -1021,13 +1021,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 01f17cf..b8c0260 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -10,12 +10,15 @@
>>  */
>>
>>  #include <linux/errno.h>
>> +#include <linux/sort.h>
>>  #include "clk.h"
>>  #include "clk-pll.h"
>>
>>  struct samsung_clk_pll {
>>       struct clk_hw           hw;
>>       const void __iomem      *base;
>> +     struct samsung_pll_rate_table *rate_table;
>> +     unsigned int rate_count;
>>  };
>>
>>  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
>> @@ -25,6 +28,14 @@ struct samsung_clk_pll {
>>  #define pll_writel(pll, val, offset)                                 \
>>               __raw_writel(val, (void __iomem *)(pll->base + (offset)));
>>
>> +static int samsung_compare_rate(const void *_a, const void *_b)
>> +{
>> +     const struct samsung_pll_rate_table *a = _a;
>> +     const struct samsung_pll_rate_table *b = _b;
>> +
>> +     return a->rate - b->rate;
>> +}
>> +
>>  /*
>>   * PLL35xx Clock Type
>>   */
>> @@ -62,7 +73,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,
>> +                     struct samsung_pll_rate_table *rate_table,
>> +                     const unsigned int rate_count)
>>  {
>>       struct samsung_clk_pll *pll;
>>       struct clk *clk;
>> @@ -82,6 +95,14 @@ struct clk * __init
>> samsung_clk_register_pll35xx(const char *name,
>>
>>       pll->hw.init = &init;
>>       pll->base = base;
>> +     pll->rate_table = rate_table;
>> +     pll->rate_count = rate_count;
>> +
>> +     if (pll->rate_table && pll->rate_count) {
>
> You seem to assume here that the exact set of rates is known at
> compilation time. In other words, you assume that all boards have the same
> PLL input (xtal) frequency.
>

Hmm .. not exactly same. I assumed that while registering a particular PLL,
required rates and parent of that PLL will be known(as in existing scenarios).
So we can provide a table containing valid supported rates with
recommended p, m, s values while registering  a particular PLL instance.
That table can also be provided at compilation time or can be initialized
before registering PLL depending upon SoC and parent.

> We have similar patches available in our internal tree that work in a bit
> different way:
>
> 1) they assume that passed array is already sorted, which is a good
> assumption, because you have full control of this array - it's a part of
> the clock driver and if it's incorrect it's your fault - you don't deal
> with data input by user
>

but the sorting is just one time job and we will be on safer side.

> 2) passed array don't have defined rates, but rather only all the relevant
> PLL coefficients; the frequency for each entry is calculated at
> registration time, based on rate of parent clock
>
> 3) the table is NULL terminated, without the need to pass its size, but
> it's just a matter of preference
>
>> +             sort(pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate, NULL);
>> +     }
>>
>>       clk = clk_register(NULL, &pll->hw);
>>       if (IS_ERR(clk)) {
>> @@ -137,7 +158,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,
>> +                     struct samsung_pll_rate_table *rate_table,
>> +                     const unsigned int rate_count)
>>  {
>>       struct samsung_clk_pll *pll;
>>       struct clk *clk;
>> @@ -157,6 +180,14 @@ struct clk * __init
>> samsung_clk_register_pll36xx(const char *name,
>>
>>       pll->hw.init = &init;
>>       pll->base = base;
>> +     pll->rate_table = rate_table;
>> +     pll->rate_count = rate_count;
>> +
>> +     if (pll->rate_table && pll->rate_count) {
>> +             sort(pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate, NULL);
>> +     }
>>
>>       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 1329522..b5e12430 100644
>> --- a/drivers/clk/samsung/clk-pll.h
>> +++ b/drivers/clk/samsung/clk-pll.h
>> @@ -12,6 +12,25 @@
>>  #ifndef __SAMSUNG_CLK_PLL_H
>>  #define __SAMSUNG_CLK_PLL_H
>>
>> +#define PLL_35XX_RATE(_rate, _m, _p, _s)                     \
>> +     {                                                       \
>> +             .rate = _rate,                                  \
>> +             .pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),    \
>> +     }
>> +
>> +#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)                 \
>> +     {                                                       \
>> +             .rate = _rate,                                  \
>> +             .pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),    \
>> +             .pll_con1 = _k,                                 \
>> +     }
>> +
>> +struct samsung_pll_rate_table {
>> +     unsigned  int rate;
>> +     u32 pll_con0;
>> +     u32 pll_con1;
>
> I don't like this struct. What about PLLs that don't have con0 or con1,
> but rather a completely weird register strukture like PLL_P_REG,
> PLL_M_REG, PLL_S_REG? The structure has generic name, which suggests that
> it should be able to handle any future Samsung PLLs as well.
>

We have derived  this struct looking at existing PLL3xxx and PLL4xxx registers.

> I'd suggest storing all coefficients as separate fields of the struct,
> e.g.
>
> struct samsung_pll_rate_table {
>         unsigned  int rate;
>         /* ... */
>         unsigned int p;
>         unsigned int m;
>         unsigned int s;
>         /* ... */
> };
>

Hmm .. it will fine, we can use it.

Regards,
Yadwinder

> Best regards,
> Tomasz
>
>> +};
>> +
>>  enum pll45xx_type {
>>       pll_4500,
>>       pll_4502,
>> @@ -25,9 +44,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,
>> +                     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,
>> +                     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);

[RESEND PATCH 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Yadwinder Singh Brar]

Hi Tomasz,

On Sat, May 25, 2013 at 3:34 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
> Please see my comments inline.
>
> On Friday 24 of May 2013 16:01:15 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> This patch 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.
>> It also sorts the rate table while registering the PLL rate table.
>> So that this sorted table can be used for making the searching of
>> "required rate" efficient.
>>
>> 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        |   35
>> ++++++++++++++++++++++++++++++++-- drivers/clk/samsung/clk-pll.h
>> |   27 ++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 15
>> deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c
>> b/drivers/clk/samsung/clk-exynos4.c index cf7d4e7..beff8a1 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -1021,13 +1021,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 01f17cf..b8c0260 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -10,12 +10,15 @@
>>  */
>>
>>  #include <linux/errno.h>
>> +#include <linux/sort.h>
>>  #include "clk.h"
>>  #include "clk-pll.h"
>>
>>  struct samsung_clk_pll {
>>       struct clk_hw           hw;
>>       const void __iomem      *base;
>> +     struct samsung_pll_rate_table *rate_table;
>> +     unsigned int rate_count;
>>  };
>>
>>  #define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
>> @@ -25,6 +28,14 @@ struct samsung_clk_pll {
>>  #define pll_writel(pll, val, offset)                                 \
>>               __raw_writel(val, (void __iomem *)(pll->base + (offset)));
>>
>> +static int samsung_compare_rate(const void *_a, const void *_b)
>> +{
>> +     const struct samsung_pll_rate_table *a = _a;
>> +     const struct samsung_pll_rate_table *b = _b;
>> +
>> +     return a->rate - b->rate;
>> +}
>> +
>>  /*
>>   * PLL35xx Clock Type
>>   */
>> @@ -62,7 +73,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,
>> +                     struct samsung_pll_rate_table *rate_table,
>> +                     const unsigned int rate_count)
>>  {
>>       struct samsung_clk_pll *pll;
>>       struct clk *clk;
>> @@ -82,6 +95,14 @@ struct clk * __init
>> samsung_clk_register_pll35xx(const char *name,
>>
>>       pll->hw.init = &init;
>>       pll->base = base;
>> +     pll->rate_table = rate_table;
>> +     pll->rate_count = rate_count;
>> +
>> +     if (pll->rate_table && pll->rate_count) {
>
> You seem to assume here that the exact set of rates is known at
> compilation time. In other words, you assume that all boards have the same
> PLL input (xtal) frequency.
>

Hmm .. not exactly same. I assumed that while registering a particular PLL,
required rates and parent of that PLL will be known(as in existing scenarios).
So we can provide a table containing valid supported rates with
recommended p, m, s values while registering  a particular PLL instance.
That table can also be provided at compilation time or can be initialized
before registering PLL depending upon SoC and parent.

> We have similar patches available in our internal tree that work in a bit
> different way:
>
> 1) they assume that passed array is already sorted, which is a good
> assumption, because you have full control of this array - it's a part of
> the clock driver and if it's incorrect it's your fault - you don't deal
> with data input by user
>

but the sorting is just one time job and we will be on safer side.

> 2) passed array don't have defined rates, but rather only all the relevant
> PLL coefficients; the frequency for each entry is calculated at
> registration time, based on rate of parent clock
>
> 3) the table is NULL terminated, without the need to pass its size, but
> it's just a matter of preference
>
>> +             sort(pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate, NULL);
>> +     }
>>
>>       clk = clk_register(NULL, &pll->hw);
>>       if (IS_ERR(clk)) {
>> @@ -137,7 +158,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,
>> +                     struct samsung_pll_rate_table *rate_table,
>> +                     const unsigned int rate_count)
>>  {
>>       struct samsung_clk_pll *pll;
>>       struct clk *clk;
>> @@ -157,6 +180,14 @@ struct clk * __init
>> samsung_clk_register_pll36xx(const char *name,
>>
>>       pll->hw.init = &init;
>>       pll->base = base;
>> +     pll->rate_table = rate_table;
>> +     pll->rate_count = rate_count;
>> +
>> +     if (pll->rate_table && pll->rate_count) {
>> +             sort(pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate, NULL);
>> +     }
>>
>>       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 1329522..b5e12430 100644
>> --- a/drivers/clk/samsung/clk-pll.h
>> +++ b/drivers/clk/samsung/clk-pll.h
>> @@ -12,6 +12,25 @@
>>  #ifndef __SAMSUNG_CLK_PLL_H
>>  #define __SAMSUNG_CLK_PLL_H
>>
>> +#define PLL_35XX_RATE(_rate, _m, _p, _s)                     \
>> +     {                                                       \
>> +             .rate = _rate,                                  \
>> +             .pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),    \
>> +     }
>> +
>> +#define PLL_36XX_RATE(_rate, _m, _p, _s, _k)                 \
>> +     {                                                       \
>> +             .rate = _rate,                                  \
>> +             .pll_con0 = ((_m) << 16 | (_p) << 8 | (_s)),    \
>> +             .pll_con1 = _k,                                 \
>> +     }
>> +
>> +struct samsung_pll_rate_table {
>> +     unsigned  int rate;
>> +     u32 pll_con0;
>> +     u32 pll_con1;
>
> I don't like this struct. What about PLLs that don't have con0 or con1,
> but rather a completely weird register strukture like PLL_P_REG,
> PLL_M_REG, PLL_S_REG? The structure has generic name, which suggests that
> it should be able to handle any future Samsung PLLs as well.
>

We have derived  this struct looking at existing PLL3xxx and PLL4xxx registers.

> I'd suggest storing all coefficients as separate fields of the struct,
> e.g.
>
> struct samsung_pll_rate_table {
>         unsigned  int rate;
>         /* ... */
>         unsigned int p;
>         unsigned int m;
>         unsigned int s;
>         /* ... */
> };
>

Hmm .. it will fine, we can use it.

Regards,
Yadwinder

> Best regards,
> Tomasz
>
>> +};
>> +
>>  enum pll45xx_type {
>>       pll_4500,
>>       pll_4502,
>> @@ -25,9 +44,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,
>> +                     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,
>> +                     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);

Re: [RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Yadwinder Singh Brar]

On Sat, May 25, 2013 at 3:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
> On Friday 24 of May 2013 16:01:16 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> Adds set_rate() and round_rate() clk_ops for PLL35xx
>>
>> The round_rate() implemenation as of now is dummy, it returns the same
>> rate which is passed as input.
>>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-pll.c |   95
>> ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94
>> insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-pll.c
>> b/drivers/clk/samsung/clk-pll.c index b8c0260..291cc9e 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -11,6 +11,7 @@
>>
>>  #include <linux/errno.h>
>>  #include <linux/sort.h>
>> +#include <linux/bsearch.h>
>>  #include "clk.h"
>>  #include "clk-pll.h"
>>
>> @@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const
>> void *_b) return a->rate - b->rate;
>>  }
>>
>> +static struct samsung_pll_rate_table *samsung_get_pll_settings(
>> +                             struct samsung_clk_pll *pll, unsigned long
> rate)
>> +{
>> +     struct samsung_pll_rate_table req_rate, *tmp;
>> +
>> +     req_rate.rate = rate;
>> +     tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate);
>
> Binary search over < 10 entries? Isn't it a bit of overkill?
>

Sorry, i couldn't see any over kill?  And it may NOT be always < 10.

>> +     if (tmp)
>> +             return tmp;
>> +
>> +     return NULL;
>> +}
>> +
>>  /*
>>   * PLL35xx Clock Type
>>   */
>> @@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const
>> void *_b) #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)
>> +
>> +#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK <<
>> PLL35XX_MDIV_SHIFT)) +#define PLL35XX_PDIV(_tmp) ((_tmp) &
>> (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)) +#define PLL35XX_SDIV(_tmp)
>> ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
>>
>>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>>                               unsigned long parent_rate)
>> @@ -68,8 +90,76 @@ static unsigned long
>> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
>> long)fvco;
>>  }
>>
>> -static const struct clk_ops samsung_pll35xx_clk_ops = {
>> +static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32
>> pll_con) +{
>> +     if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv !=
>> PLL35XX_PDIV(pll_con))) +             return 1;
>> +     else
>> +             return 0;
>> +}
>> +
>> +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);
>> +     struct samsung_pll_rate_table *rate;
>> +
>> +     u32 tmp, mdiv, pdiv, sdiv;
>> +
>> +     /* 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;
>> +     }
>> +
>> +     mdiv = PLL35XX_MDIV(rate->pll_con0);
>> +     pdiv = PLL35XX_PDIV(rate->pll_con0);
>> +     sdiv = PLL35XX_SDIV(rate->pll_con0);
>
> You wouldn't need to use those macros if all coefficients were stored as
> separate fields in the struct.
>

Agree, I will use that.

>> +
>> +     tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
>> +
>> +     if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
>> +             /* If only s change, change just s value only*/
>> +             tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
>> +             tmp |= sdiv;
>
> This line is correct, but it looks like it wasn't, because:
> a) the name suggests that it contains the raw value of S coefficient
> b) it's real value is hidden between a macro, name of which suggests the
> same as in a) as well.
>
> This makes the code hard to read.
>

We can rename sdiv to sdiv_regval ?? , to improve readability.

>> +             pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
>> +     } else {
>> +             /* Set PLL lock time.
>> +                Maximum lock time can be 270 * PDIV cycles */
>> +             pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
>> +                             PLL35XX_LOCK_OFFSET);
>
> Hmm, magic constant in the code? Shouldn't it be defined as a macro?
>

Ya, I will define it.

>> +
>> +             /* Change PLL PMS values */
>> +             tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
>> +                             (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)
> |
>> +                             (PLL35XX_SDIV_MASK <<
> PLL35XX_SDIV_SHIFT));
>> +             tmp |= mdiv | pdiv | sdiv;
>
> This looks strange as well, even if it's correct.
>
>> +             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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate)
>> +{
>> +     /* Clock framework cries without this, so implemented dummy */
>
> This is completely wrong. Have you tested this code or read how Common
> Clock Framework works?
>

As its mentioned in commit message, its just a dummy function to
make set_rate() work. We will implement it later in different patch.
In existing scenario users are requesting  valid rates provided
in table so it works fine with this dummy function.

> clk_set_rate() first calls ->round_rate() to get a rate supported by the
> clock that is nearest and not higher than requested rate and only then it
> calls ->set_rate() with the rate returned by ->round_rate().
>
> So the round_rate() callback must return the highest supported rate with
> parent clock at prate and not higher than drate.
>
>> +     return drate;
>> +}
>> +
>> +static struct clk_ops samsung_pll35xx_clk_ops = {
>>       .recalc_rate = samsung_pll35xx_recalc_rate,
>> +     .round_rate = samsung_pll35xx_round_rate,
>> +     .set_rate = samsung_pll35xx_set_rate,
>>  };
>>
>>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> @@ -102,6 +192,9 @@ struct clk * __init
>> samsung_clk_register_pll35xx(const char *name, sort(pll->rate_table,
>> pll->rate_count,
>>                       sizeof(struct samsung_pll_rate_table),
>>                       samsung_compare_rate, NULL);
>> +     } else {
>> +             samsung_pll35xx_clk_ops.round_rate = NULL;
>> +             samsung_pll35xx_clk_ops.set_rate = NULL;
>
> This is completely wrong. You are changing a static structure that is used
> for all instances of PLL35xx, not only the one being registered at the
> moment.
>

Yes,  will rectify it.

> Best regards,
> Tomasz
>
>>       }
>>
>>       clk = clk_register(NULL, &pll->hw);

Thanks for review.

Regards,
Yadwinder

[RESEND PATCH 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx

[Yadwinder Singh Brar]

On Sat, May 25, 2013 at 3:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
> On Friday 24 of May 2013 16:01:16 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> Adds set_rate() and round_rate() clk_ops for PLL35xx
>>
>> The round_rate() implemenation as of now is dummy, it returns the same
>> rate which is passed as input.
>>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-pll.c |   95
>> ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94
>> insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-pll.c
>> b/drivers/clk/samsung/clk-pll.c index b8c0260..291cc9e 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -11,6 +11,7 @@
>>
>>  #include <linux/errno.h>
>>  #include <linux/sort.h>
>> +#include <linux/bsearch.h>
>>  #include "clk.h"
>>  #include "clk-pll.h"
>>
>> @@ -36,6 +37,21 @@ static int samsung_compare_rate(const void *_a, const
>> void *_b) return a->rate - b->rate;
>>  }
>>
>> +static struct samsung_pll_rate_table *samsung_get_pll_settings(
>> +                             struct samsung_clk_pll *pll, unsigned long
> rate)
>> +{
>> +     struct samsung_pll_rate_table req_rate, *tmp;
>> +
>> +     req_rate.rate = rate;
>> +     tmp = bsearch(&req_rate, pll->rate_table, pll->rate_count,
>> +                     sizeof(struct samsung_pll_rate_table),
>> +                     samsung_compare_rate);
>
> Binary search over < 10 entries? Isn't it a bit of overkill?
>

Sorry, i couldn't see any over kill?  And it may NOT be always < 10.

>> +     if (tmp)
>> +             return tmp;
>> +
>> +     return NULL;
>> +}
>> +
>>  /*
>>   * PLL35xx Clock Type
>>   */
>> @@ -46,9 +62,15 @@ static int samsung_compare_rate(const void *_a, const
>> void *_b) #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)
>> +
>> +#define PLL35XX_MDIV(_tmp) ((_tmp) & (PLL35XX_MDIV_MASK <<
>> PLL35XX_MDIV_SHIFT)) +#define PLL35XX_PDIV(_tmp) ((_tmp) &
>> (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)) +#define PLL35XX_SDIV(_tmp)
>> ((_tmp) & (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT))
>>
>>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>>                               unsigned long parent_rate)
>> @@ -68,8 +90,76 @@ static unsigned long
>> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
>> long)fvco;
>>  }
>>
>> -static const struct clk_ops samsung_pll35xx_clk_ops = {
>> +static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32
>> pll_con) +{
>> +     if ((mdiv != PLL35XX_MDIV(pll_con)) || (pdiv !=
>> PLL35XX_PDIV(pll_con))) +             return 1;
>> +     else
>> +             return 0;
>> +}
>> +
>> +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);
>> +     struct samsung_pll_rate_table *rate;
>> +
>> +     u32 tmp, mdiv, pdiv, sdiv;
>> +
>> +     /* 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;
>> +     }
>> +
>> +     mdiv = PLL35XX_MDIV(rate->pll_con0);
>> +     pdiv = PLL35XX_PDIV(rate->pll_con0);
>> +     sdiv = PLL35XX_SDIV(rate->pll_con0);
>
> You wouldn't need to use those macros if all coefficients were stored as
> separate fields in the struct.
>

Agree, I will use that.

>> +
>> +     tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
>> +
>> +     if (!(samsung_pll35xx_mp_change(mdiv, pdiv, tmp))) {
>> +             /* If only s change, change just s value only*/
>> +             tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
>> +             tmp |= sdiv;
>
> This line is correct, but it looks like it wasn't, because:
> a) the name suggests that it contains the raw value of S coefficient
> b) it's real value is hidden between a macro, name of which suggests the
> same as in a) as well.
>
> This makes the code hard to read.
>

We can rename sdiv to sdiv_regval ?? , to improve readability.

>> +             pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
>> +     } else {
>> +             /* Set PLL lock time.
>> +                Maximum lock time can be 270 * PDIV cycles */
>> +             pll_writel(pll, (pdiv >> PLL35XX_PDIV_SHIFT) * 270,
>> +                             PLL35XX_LOCK_OFFSET);
>
> Hmm, magic constant in the code? Shouldn't it be defined as a macro?
>

Ya, I will define it.

>> +
>> +             /* Change PLL PMS values */
>> +             tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
>> +                             (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)
> |
>> +                             (PLL35XX_SDIV_MASK <<
> PLL35XX_SDIV_SHIFT));
>> +             tmp |= mdiv | pdiv | sdiv;
>
> This looks strange as well, even if it's correct.
>
>> +             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 long samsung_pll35xx_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate)
>> +{
>> +     /* Clock framework cries without this, so implemented dummy */
>
> This is completely wrong. Have you tested this code or read how Common
> Clock Framework works?
>

As its mentioned in commit message, its just a dummy function to
make set_rate() work. We will implement it later in different patch.
In existing scenario users are requesting  valid rates provided
in table so it works fine with this dummy function.

> clk_set_rate() first calls ->round_rate() to get a rate supported by the
> clock that is nearest and not higher than requested rate and only then it
> calls ->set_rate() with the rate returned by ->round_rate().
>
> So the round_rate() callback must return the highest supported rate with
> parent clock at prate and not higher than drate.
>
>> +     return drate;
>> +}
>> +
>> +static struct clk_ops samsung_pll35xx_clk_ops = {
>>       .recalc_rate = samsung_pll35xx_recalc_rate,
>> +     .round_rate = samsung_pll35xx_round_rate,
>> +     .set_rate = samsung_pll35xx_set_rate,
>>  };
>>
>>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> @@ -102,6 +192,9 @@ struct clk * __init
>> samsung_clk_register_pll35xx(const char *name, sort(pll->rate_table,
>> pll->rate_count,
>>                       sizeof(struct samsung_pll_rate_table),
>>                       samsung_compare_rate, NULL);
>> +     } else {
>> +             samsung_pll35xx_clk_ops.round_rate = NULL;
>> +             samsung_pll35xx_clk_ops.set_rate = NULL;
>
> This is completely wrong. You are changing a static structure that is used
> for all instances of PLL35xx, not only the one being registered at the
> moment.
>

Yes,  will rectify it.

> Best regards,
> Tomasz
>
>>       }
>>
>>       clk = clk_register(NULL, &pll->hw);

Thanks for review.

Regards,
Yadwinder

[RESEND PATCH 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx

[Vikas Sajjan]

This patch adds set_rate and round_rate clk_ops for PLL36xx
The round_rate() implementation as of now is dummy, it returns the same rate
which is passed as input.

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/clk/samsung/clk-pll.c |   67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 291cc9e..55ff5fd 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -224,6 +224,13 @@ 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)
+
+#define PLL36XX_MDIV(_tmp) ((_tmp) & (PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT))
+#define PLL36XX_PDIV(_tmp) ((_tmp) & (PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT))
+#define PLL36XX_SDIV(_tmp) ((_tmp) & (PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT))
+#define PLL36XX_KDIV(_tmp) ((_tmp) & (PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT))
 
 static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
@@ -246,8 +253,65 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
 	return (unsigned long)fvco;
 }
 
+static long samsung_pll36xx_round_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long *prate)
+{
+	/* retruns the same 'drate' whichs comes as input */
+	return drate;
+}
+
+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, mdiv, pdiv, sdiv, kdiv;
+	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;
+	}
+
+	mdiv = PLL36XX_MDIV(rate->pll_con0);
+	pdiv = PLL36XX_PDIV(rate->pll_con0);
+	sdiv = PLL36XX_SDIV(rate->pll_con0);
+	kdiv = PLL36XX_KDIV(rate->pll_con1);
+
+	pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+	pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+	pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+
+	/* Set PLL lock time.
+	   Maximum lock time can be 3000 * PDIV cycles */
+	pll_writel(pll, ((pdiv >> PLL36XX_PDIV_SHIFT) * 3000),
+			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 |= mdiv | pdiv | sdiv;
+	pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+	pll_con1 |= kdiv;
+	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_pll36xx_round_rate,
 };
 
 struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -280,6 +344,9 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
 		sort(pll->rate_table, pll->rate_count,
 			sizeof(struct samsung_pll_rate_table),
 			samsung_compare_rate, NULL);
+	} else {
+		samsung_pll35xx_clk_ops.round_rate = NULL;
+		samsung_pll35xx_clk_ops.set_rate = NULL;
 	}
 
 	clk = clk_register(NULL, &pll->hw);
-- 
1.7.9.5