* [PATCH v2 0/5] Add generic set_rate clk_ops for PLL35xx and PLL36xx for samsung SoCs
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
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 PLL36xx
changes since v1:
- removed sorting and bsearch
- modified the definition of struct "samsung_pll_rate_table"
- added generic round_rate()
- rectified the ops assignment for "rate table passed as NULL"
during PLL registration
Is rebased on branch kgene's "for-next"
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next
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 | 30 +++--
drivers/clk/samsung/clk-pll.c | 226 ++++++++++++++++++++++++++++++----
drivers/clk/samsung/clk-pll.h | 33 ++++-
4 files changed, 260 insertions(+), 39 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 0/5] Add generic set_rate clk_ops for PLL35xx and PLL36xx for samsung SoCs
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
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 PLL36xx
changes since v1:
- removed sorting and bsearch
- modified the definition of struct "samsung_pll_rate_table"
- added generic round_rate()
- rectified the ops assignment for "rate table passed as NULL"
during PLL registration
Is rebased on branch kgene's "for-next"
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next
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 | 30 +++--
drivers/clk/samsung/clk-pll.c | 226 ++++++++++++++++++++++++++++++----
drivers/clk/samsung/clk-pll.h | 33 ++++-
4 files changed, 260 insertions(+), 39 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 13:37 ` Vikas Sajjan
-1 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
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.
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
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.
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 13:37 ` Vikas Sajjan
-1 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
From: Yadwinder Singh Brar <yadi.brar@samsung.com>
This patch defines a common rate_table which will contain recommended p, m, s,
k values for supported rates that needs to be changed for changing
corresponding PLL's rate.
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
drivers/clk/samsung/clk-exynos4.c | 8 ++++----
drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
4 files changed, 54 insertions(+), 15 deletions(-)
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..4b3b3f5 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -16,6 +16,8 @@
struct samsung_clk_pll {
struct clk_hw hw;
const void __iomem *base;
+ const struct samsung_pll_rate_table *rate_table;
+ unsigned int rate_count;
};
#define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
@@ -62,7 +64,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops = {
};
struct clk * __init samsung_clk_register_pll35xx(const char *name,
- const char *pname, const void __iomem *base)
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count)
{
struct samsung_clk_pll *pll;
struct clk *clk;
@@ -82,6 +86,8 @@ 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;
clk = clk_register(NULL, &pll->hw);
if (IS_ERR(clk)) {
@@ -137,7 +143,9 @@ static const struct clk_ops samsung_pll36xx_clk_ops = {
};
struct clk * __init samsung_clk_register_pll36xx(const char *name,
- const char *pname, const void __iomem *base)
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count)
{
struct samsung_clk_pll *pll;
struct clk *clk;
@@ -157,6 +165,8 @@ 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;
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..a7c0f5a 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -12,6 +12,31 @@
#ifndef __SAMSUNG_CLK_PLL_H
#define __SAMSUNG_CLK_PLL_H
+#define PLL_35XX_RATE(_rate, _m, _p, _s) \
+ { \
+ .rate = (_rate), \
+ .mdiv = (_m), \
+ .pdiv = (_p), \
+ .sdiv = (_s), \
+ }
+
+#define PLL_36XX_RATE(_rate, _m, _p, _s, _k) \
+ { \
+ .rate = (_rate), \
+ .mdiv = (_m), \
+ .pdiv = (_p), \
+ .sdiv = (_s), \
+ .kdiv = (_k), \
+ }
+
+struct samsung_pll_rate_table {
+ unsigned int rate;
+ unsigned int pdiv;
+ unsigned int mdiv;
+ unsigned int sdiv;
+ unsigned int kdiv;
+};
+
enum pll45xx_type {
pll_4500,
pll_4502,
@@ -25,9 +50,13 @@ enum pll46xx_type {
};
extern struct clk * __init samsung_clk_register_pll35xx(const char *name,
- const char *pname, const void __iomem *base);
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count);
extern struct clk * __init samsung_clk_register_pll36xx(const char *name,
- const char *pname, const void __iomem *base);
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count);
extern struct clk * __init samsung_clk_register_pll45xx(const char *name,
const char *pname, const void __iomem *con_reg,
enum pll45xx_type type);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
From: Yadwinder Singh Brar <yadi.brar@samsung.com>
This patch defines a common rate_table which will contain recommended p, m, s,
k values for supported rates that needs to be changed for changing
corresponding PLL's rate.
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
drivers/clk/samsung/clk-exynos4.c | 8 ++++----
drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
4 files changed, 54 insertions(+), 15 deletions(-)
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..4b3b3f5 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -16,6 +16,8 @@
struct samsung_clk_pll {
struct clk_hw hw;
const void __iomem *base;
+ const struct samsung_pll_rate_table *rate_table;
+ unsigned int rate_count;
};
#define to_clk_pll(_hw) container_of(_hw, struct samsung_clk_pll, hw)
@@ -62,7 +64,9 @@ static const struct clk_ops samsung_pll35xx_clk_ops = {
};
struct clk * __init samsung_clk_register_pll35xx(const char *name,
- const char *pname, const void __iomem *base)
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count)
{
struct samsung_clk_pll *pll;
struct clk *clk;
@@ -82,6 +86,8 @@ 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;
clk = clk_register(NULL, &pll->hw);
if (IS_ERR(clk)) {
@@ -137,7 +143,9 @@ static const struct clk_ops samsung_pll36xx_clk_ops = {
};
struct clk * __init samsung_clk_register_pll36xx(const char *name,
- const char *pname, const void __iomem *base)
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count)
{
struct samsung_clk_pll *pll;
struct clk *clk;
@@ -157,6 +165,8 @@ 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;
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..a7c0f5a 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -12,6 +12,31 @@
#ifndef __SAMSUNG_CLK_PLL_H
#define __SAMSUNG_CLK_PLL_H
+#define PLL_35XX_RATE(_rate, _m, _p, _s) \
+ { \
+ .rate = (_rate), \
+ .mdiv = (_m), \
+ .pdiv = (_p), \
+ .sdiv = (_s), \
+ }
+
+#define PLL_36XX_RATE(_rate, _m, _p, _s, _k) \
+ { \
+ .rate = (_rate), \
+ .mdiv = (_m), \
+ .pdiv = (_p), \
+ .sdiv = (_s), \
+ .kdiv = (_k), \
+ }
+
+struct samsung_pll_rate_table {
+ unsigned int rate;
+ unsigned int pdiv;
+ unsigned int mdiv;
+ unsigned int sdiv;
+ unsigned int kdiv;
+};
+
enum pll45xx_type {
pll_4500,
pll_4502,
@@ -25,9 +50,13 @@ enum pll46xx_type {
};
extern struct clk * __init samsung_clk_register_pll35xx(const char *name,
- const char *pname, const void __iomem *base);
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count);
extern struct clk * __init samsung_clk_register_pll36xx(const char *name,
- const char *pname, const void __iomem *base);
+ const char *pname, const void __iomem *base,
+ const struct samsung_pll_rate_table *rate_table,
+ const unsigned int rate_count);
extern struct clk * __init samsung_clk_register_pll45xx(const char *name,
const char *pname, const void __iomem *con_reg,
enum pll45xx_type type);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 13:37 ` Vikas Sajjan
-1 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
From: Yadwinder Singh Brar <yadi.brar@samsung.com>
This patch add set_rate() and round_rate() for PLL35xx
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
drivers/clk/samsung/clk-pll.c | 103 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 4b3b3f5..1fba7ef 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -27,6 +27,36 @@ struct samsung_clk_pll {
#define pll_writel(pll, val, offset) \
__raw_writel(val, (void __iomem *)(pll->base + (offset)));
+static const struct samsung_pll_rate_table *samsung_get_pll_settings(
+ struct samsung_clk_pll *pll, unsigned long rate)
+{
+ const struct samsung_pll_rate_table *rate_table = pll->rate_table;
+ int i;
+
+ for (i = 0; i < pll->rate_count; i++) {
+ if (rate == rate_table[i].rate)
+ return &rate_table[i];
+ }
+
+ return NULL;
+}
+
+static long samsung_pll_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
+{
+ struct samsung_clk_pll *pll = to_clk_pll(hw);
+ const struct samsung_pll_rate_table *rate_table = pll->rate_table;
+ int i;
+
+ /* Assumming rate_table is in descending order */
+ for (i = 0; i < pll->rate_count; i++) {
+ if (drate >= rate_table[i].rate)
+ return rate_table[i].rate;
+ }
+
+ /* return minimum supported value */
+ return rate_table[i - 1].rate;
+}
/*
* PLL35xx Clock Type
*/
@@ -34,12 +64,17 @@ struct samsung_clk_pll {
#define PLL35XX_CON0_OFFSET 0x100
#define PLL35XX_CON1_OFFSET 0x104
+/* Maximum lock time can be 270 * PDIV cycles */
+#define PLL35XX_LOCK_FACTOR 270
+
#define PLL35XX_MDIV_MASK (0x3FF)
#define PLL35XX_PDIV_MASK (0x3F)
#define PLL35XX_SDIV_MASK (0x7)
+#define PLL35XX_LOCK_STAT_MASK (0x1)
#define PLL35XX_MDIV_SHIFT (16)
#define PLL35XX_PDIV_SHIFT (8)
#define PLL35XX_SDIV_SHIFT (0)
+#define PLL35XX_LOCK_STAT_SHIFT (29)
static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
@@ -59,8 +94,70 @@ static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
return (unsigned long)fvco;
}
+static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
+{
+ if ((mdiv != ((pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK)) ||
+ (pdiv != ((pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK)))
+ 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);
+ const struct samsung_pll_rate_table *rate;
+ u32 tmp;
+
+ /* Get required rate settings from table */
+ rate = samsung_get_pll_settings(pll, drate);
+ if (!rate) {
+ pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+ drate, __clk_get_name(hw->clk));
+ return -EINVAL;
+ }
+
+ tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+
+ if (!(samsung_pll35xx_mp_change(rate->mdiv, rate->pdiv, tmp))) {
+ /* If only s change, change just s value only*/
+ tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
+ tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT;
+ pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+ } else {
+ /* Set PLL lock time. */
+ pll_writel(pll, rate->pdiv * PLL35XX_LOCK_FACTOR,
+ PLL35XX_LOCK_OFFSET);
+
+ /* Change PLL PMS values */
+ tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
+ (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) |
+ (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT));
+ tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) |
+ (rate->pdiv << PLL35XX_PDIV_SHIFT) |
+ (rate->sdiv << PLL35XX_SDIV_SHIFT);
+ pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+
+ /* wait_lock_time */
+ do {
+ cpu_relax();
+ tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+ } while (!(tmp & (PLL35XX_LOCK_STAT_MASK
+ << PLL35XX_LOCK_STAT_SHIFT)));
+ }
+
+ return 0;
+}
+
static const struct clk_ops samsung_pll35xx_clk_ops = {
.recalc_rate = samsung_pll35xx_recalc_rate,
+ .round_rate = samsung_pll_round_rate,
+ .set_rate = samsung_pll35xx_set_rate,
+};
+
+static const struct clk_ops samsung_pll35xx_clk_min_ops = {
+ .recalc_rate = samsung_pll35xx_recalc_rate,
};
struct clk * __init samsung_clk_register_pll35xx(const char *name,
@@ -79,11 +176,15 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
}
init.name = name;
- init.ops = &samsung_pll35xx_clk_ops;
init.flags = CLK_GET_RATE_NOCACHE;
init.parent_names = &pname;
init.num_parents = 1;
+ if (rate_table && rate_count)
+ init.ops = &samsung_pll35xx_clk_ops;
+ else
+ init.ops = &samsung_pll35xx_clk_min_ops;
+
pll->hw.init = &init;
pll->base = base;
pll->rate_table = rate_table;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
From: Yadwinder Singh Brar <yadi.brar@samsung.com>
This patch add set_rate() and round_rate() for PLL35xx
Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
drivers/clk/samsung/clk-pll.c | 103 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 4b3b3f5..1fba7ef 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -27,6 +27,36 @@ struct samsung_clk_pll {
#define pll_writel(pll, val, offset) \
__raw_writel(val, (void __iomem *)(pll->base + (offset)));
+static const struct samsung_pll_rate_table *samsung_get_pll_settings(
+ struct samsung_clk_pll *pll, unsigned long rate)
+{
+ const struct samsung_pll_rate_table *rate_table = pll->rate_table;
+ int i;
+
+ for (i = 0; i < pll->rate_count; i++) {
+ if (rate == rate_table[i].rate)
+ return &rate_table[i];
+ }
+
+ return NULL;
+}
+
+static long samsung_pll_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
+{
+ struct samsung_clk_pll *pll = to_clk_pll(hw);
+ const struct samsung_pll_rate_table *rate_table = pll->rate_table;
+ int i;
+
+ /* Assumming rate_table is in descending order */
+ for (i = 0; i < pll->rate_count; i++) {
+ if (drate >= rate_table[i].rate)
+ return rate_table[i].rate;
+ }
+
+ /* return minimum supported value */
+ return rate_table[i - 1].rate;
+}
/*
* PLL35xx Clock Type
*/
@@ -34,12 +64,17 @@ struct samsung_clk_pll {
#define PLL35XX_CON0_OFFSET 0x100
#define PLL35XX_CON1_OFFSET 0x104
+/* Maximum lock time can be 270 * PDIV cycles */
+#define PLL35XX_LOCK_FACTOR 270
+
#define PLL35XX_MDIV_MASK (0x3FF)
#define PLL35XX_PDIV_MASK (0x3F)
#define PLL35XX_SDIV_MASK (0x7)
+#define PLL35XX_LOCK_STAT_MASK (0x1)
#define PLL35XX_MDIV_SHIFT (16)
#define PLL35XX_PDIV_SHIFT (8)
#define PLL35XX_SDIV_SHIFT (0)
+#define PLL35XX_LOCK_STAT_SHIFT (29)
static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
@@ -59,8 +94,70 @@ static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
return (unsigned long)fvco;
}
+static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
+{
+ if ((mdiv != ((pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK)) ||
+ (pdiv != ((pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK)))
+ 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);
+ const struct samsung_pll_rate_table *rate;
+ u32 tmp;
+
+ /* Get required rate settings from table */
+ rate = samsung_get_pll_settings(pll, drate);
+ if (!rate) {
+ pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+ drate, __clk_get_name(hw->clk));
+ return -EINVAL;
+ }
+
+ tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+
+ if (!(samsung_pll35xx_mp_change(rate->mdiv, rate->pdiv, tmp))) {
+ /* If only s change, change just s value only*/
+ tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
+ tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT;
+ pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+ } else {
+ /* Set PLL lock time. */
+ pll_writel(pll, rate->pdiv * PLL35XX_LOCK_FACTOR,
+ PLL35XX_LOCK_OFFSET);
+
+ /* Change PLL PMS values */
+ tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
+ (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT) |
+ (PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT));
+ tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) |
+ (rate->pdiv << PLL35XX_PDIV_SHIFT) |
+ (rate->sdiv << PLL35XX_SDIV_SHIFT);
+ pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
+
+ /* wait_lock_time */
+ do {
+ cpu_relax();
+ tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
+ } while (!(tmp & (PLL35XX_LOCK_STAT_MASK
+ << PLL35XX_LOCK_STAT_SHIFT)));
+ }
+
+ return 0;
+}
+
static const struct clk_ops samsung_pll35xx_clk_ops = {
.recalc_rate = samsung_pll35xx_recalc_rate,
+ .round_rate = samsung_pll_round_rate,
+ .set_rate = samsung_pll35xx_set_rate,
+};
+
+static const struct clk_ops samsung_pll35xx_clk_min_ops = {
+ .recalc_rate = samsung_pll35xx_recalc_rate,
};
struct clk * __init samsung_clk_register_pll35xx(const char *name,
@@ -79,11 +176,15 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
}
init.name = name;
- init.ops = &samsung_pll35xx_clk_ops;
init.flags = CLK_GET_RATE_NOCACHE;
init.parent_names = &pname;
init.num_parents = 1;
+ if (rate_table && rate_count)
+ init.ops = &samsung_pll35xx_clk_ops;
+ else
+ init.ops = &samsung_pll35xx_clk_min_ops;
+
pll->hw.init = &init;
pll->base = base;
pll->rate_table = rate_table;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 13:37 ` Vikas Sajjan
-1 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
This patch adds set_rate and round_rate clk_ops for PLL36xx
Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
drivers/clk/samsung/clk-pll.c | 59 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 1fba7ef..3872161 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -210,6 +210,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
#define PLL36XX_CON0_OFFSET 0x100
#define PLL36XX_CON1_OFFSET 0x104
+/* Maximum lock time can be 3000 * PDIV cycles */
+#define PLL36XX_LOCK_FACTOR 3000
+
#define PLL36XX_KDIV_MASK (0xFFFF)
#define PLL36XX_MDIV_MASK (0x1FF)
#define PLL36XX_PDIV_MASK (0x3F)
@@ -217,6 +220,8 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
#define PLL36XX_MDIV_SHIFT (16)
#define PLL36XX_PDIV_SHIFT (8)
#define PLL36XX_SDIV_SHIFT (0)
+#define PLL36XX_KDIV_SHIFT (0)
+#define PLL36XX_LOCK_STAT_SHIFT (29)
static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
@@ -239,8 +244,57 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
return (unsigned long)fvco;
}
+static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
+ unsigned long parent_rate)
+{
+ struct samsung_clk_pll *pll = to_clk_pll(hw);
+ u32 tmp, pll_con0, pll_con1;
+ const struct samsung_pll_rate_table *rate;
+
+ rate = samsung_get_pll_settings(pll, drate);
+ if (!rate) {
+ pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+ drate, __clk_get_name(hw->clk));
+ return -EINVAL;
+ }
+
+ pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+ pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+ /* Set PLL lock time. */
+ pll_writel(pll, (rate->pdiv * PLL36XX_LOCK_FACTOR),
+ PLL36XX_LOCK_OFFSET);
+
+ /* Change PLL PMS values */
+ pll_con0 &= ~((PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT) |
+ (PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT) |
+ (PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT));
+ pll_con0 |= (rate->mdiv << PLL36XX_MDIV_SHIFT) |
+ (rate->pdiv << PLL36XX_PDIV_SHIFT) |
+ (rate->sdiv << PLL36XX_SDIV_SHIFT);
+ pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+ pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+ pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
+ pll_writel(pll, pll_con1, PLL36XX_CON1_OFFSET);
+
+ /* wait_lock_time */
+ do {
+ cpu_relax();
+ tmp = pll_readl(pll, PLL36XX_CON0_OFFSET);
+ } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT)));
+
+ return 0;
+}
+
static const struct clk_ops samsung_pll36xx_clk_ops = {
.recalc_rate = samsung_pll36xx_recalc_rate,
+ .set_rate = samsung_pll36xx_set_rate,
+ .round_rate = samsung_pll_round_rate,
+};
+
+static const struct clk_ops samsung_pll36xx_clk_min_ops = {
+ .recalc_rate = samsung_pll36xx_recalc_rate,
};
struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -264,6 +318,11 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
init.parent_names = &pname;
init.num_parents = 1;
+ if (rate_table && rate_count)
+ init.ops = &samsung_pll36xx_clk_ops;
+ else
+ init.ops = &samsung_pll36xx_clk_min_ops;
+
pll->hw.init = &init;
pll->base = base;
pll->rate_table = rate_table;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds set_rate and round_rate clk_ops for PLL36xx
Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
drivers/clk/samsung/clk-pll.c | 59 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 1fba7ef..3872161 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -210,6 +210,9 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
#define PLL36XX_CON0_OFFSET 0x100
#define PLL36XX_CON1_OFFSET 0x104
+/* Maximum lock time can be 3000 * PDIV cycles */
+#define PLL36XX_LOCK_FACTOR 3000
+
#define PLL36XX_KDIV_MASK (0xFFFF)
#define PLL36XX_MDIV_MASK (0x1FF)
#define PLL36XX_PDIV_MASK (0x3F)
@@ -217,6 +220,8 @@ struct clk * __init samsung_clk_register_pll35xx(const char *name,
#define PLL36XX_MDIV_SHIFT (16)
#define PLL36XX_PDIV_SHIFT (8)
#define PLL36XX_SDIV_SHIFT (0)
+#define PLL36XX_KDIV_SHIFT (0)
+#define PLL36XX_LOCK_STAT_SHIFT (29)
static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
@@ -239,8 +244,57 @@ static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
return (unsigned long)fvco;
}
+static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
+ unsigned long parent_rate)
+{
+ struct samsung_clk_pll *pll = to_clk_pll(hw);
+ u32 tmp, pll_con0, pll_con1;
+ const struct samsung_pll_rate_table *rate;
+
+ rate = samsung_get_pll_settings(pll, drate);
+ if (!rate) {
+ pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+ drate, __clk_get_name(hw->clk));
+ return -EINVAL;
+ }
+
+ pll_con0 = pll_readl(pll, PLL36XX_CON0_OFFSET);
+ pll_con1 = pll_readl(pll, PLL36XX_CON1_OFFSET);
+
+ /* Set PLL lock time. */
+ pll_writel(pll, (rate->pdiv * PLL36XX_LOCK_FACTOR),
+ PLL36XX_LOCK_OFFSET);
+
+ /* Change PLL PMS values */
+ pll_con0 &= ~((PLL36XX_MDIV_MASK << PLL36XX_MDIV_SHIFT) |
+ (PLL36XX_PDIV_MASK << PLL36XX_PDIV_SHIFT) |
+ (PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT));
+ pll_con0 |= (rate->mdiv << PLL36XX_MDIV_SHIFT) |
+ (rate->pdiv << PLL36XX_PDIV_SHIFT) |
+ (rate->sdiv << PLL36XX_SDIV_SHIFT);
+ pll_writel(pll, pll_con0, PLL36XX_CON0_OFFSET);
+
+ pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT);
+ pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
+ pll_writel(pll, pll_con1, PLL36XX_CON1_OFFSET);
+
+ /* wait_lock_time */
+ do {
+ cpu_relax();
+ tmp = pll_readl(pll, PLL36XX_CON0_OFFSET);
+ } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT)));
+
+ return 0;
+}
+
static const struct clk_ops samsung_pll36xx_clk_ops = {
.recalc_rate = samsung_pll36xx_recalc_rate,
+ .set_rate = samsung_pll36xx_set_rate,
+ .round_rate = samsung_pll_round_rate,
+};
+
+static const struct clk_ops samsung_pll36xx_clk_min_ops = {
+ .recalc_rate = samsung_pll36xx_recalc_rate,
};
struct clk * __init samsung_clk_register_pll36xx(const char *name,
@@ -264,6 +318,11 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
init.parent_names = &pname;
init.num_parents = 1;
+ if (rate_table && rate_count)
+ init.ops = &samsung_pll36xx_clk_ops;
+ else
+ init.ops = &samsung_pll36xx_clk_min_ops;
+
pll->hw.init = &init;
pll->base = base;
pll->rate_table = rate_table;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 5/5] clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 13:37 ` Vikas Sajjan
-1 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-samsung-soc
Cc: kgene.kim, t.figa, yadi.brar01, dianders, mturquette,
linux-arm-kernel, thomas.abraham, patches, linaro-kernel
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 | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index ddf10ca..bf61091 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -469,6 +469,22 @@ static __initdata struct of_device_id ext_clk_match[] = {
{ },
};
+static const struct samsung_pll_rate_table vpll_tbl[] = {
+ PLL_36XX_RATE(70500000, 94, 2, 4, 0),
+};
+
+static const struct samsung_pll_rate_table epll_tbl[] = {
+ /* sorted in descending order */
+ PLL_36XX_RATE(192000000, 48, 3, 1, 0),
+ PLL_36XX_RATE(180633600, 45, 3, 1, 10381),
+ PLL_36XX_RATE(180000000, 45, 3, 1, 0),
+ PLL_36XX_RATE(73728000, 73, 3, 3, 47710),
+ PLL_36XX_RATE(67737600, 90, 4, 3, 20762),
+ PLL_36XX_RATE(49152000, 49, 3, 3, 9962),
+ PLL_36XX_RATE(45158400, 45, 3, 3, 10381),
+ PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
+};
+
/* register exynox5250 clocks */
void __init exynos5250_clk_init(struct device_node *np)
{
@@ -501,9 +517,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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 5/5] clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC
@ 2013-05-29 13:37 ` Vikas Sajjan
0 siblings, 0 replies; 36+ messages in thread
From: Vikas Sajjan @ 2013-05-29 13:37 UTC (permalink / raw)
To: linux-arm-kernel
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 | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index ddf10ca..bf61091 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -469,6 +469,22 @@ static __initdata struct of_device_id ext_clk_match[] = {
{ },
};
+static const struct samsung_pll_rate_table vpll_tbl[] = {
+ PLL_36XX_RATE(70500000, 94, 2, 4, 0),
+};
+
+static const struct samsung_pll_rate_table epll_tbl[] = {
+ /* sorted in descending order */
+ PLL_36XX_RATE(192000000, 48, 3, 1, 0),
+ PLL_36XX_RATE(180633600, 45, 3, 1, 10381),
+ PLL_36XX_RATE(180000000, 45, 3, 1, 0),
+ PLL_36XX_RATE(73728000, 73, 3, 3, 47710),
+ PLL_36XX_RATE(67737600, 90, 4, 3, 20762),
+ PLL_36XX_RATE(49152000, 49, 3, 3, 9962),
+ PLL_36XX_RATE(45158400, 45, 3, 3, 10381),
+ PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
+};
+
/* register exynox5250 clocks */
void __init exynos5250_clk_init(struct device_node *np)
{
@@ -501,9 +517,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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 17:58 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-29 17:58 UTC (permalink / raw)
To: Vikas Sajjan
Cc: linux-samsung-soc, Kukjin Kim, Tomasz Figa, yadi.brar01,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> To factor out possible common code, this patch unifies the clk strutures used
> for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> 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(-)
I've run with this patch atop our CCF backport on 3.8 on on
exynos5250-snow. It works and looks good to me. We're planning to
put this patch into our tree at
<https://gerrit.chromium.org/gerrit/#/c/56740>.
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx
@ 2013-05-29 17:58 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-29 17:58 UTC (permalink / raw)
To: linux-arm-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> To factor out possible common code, this patch unifies the clk strutures used
> for PLL35xx & PLL36xx and usues clk->base instead of clk->con0.
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> 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(-)
I've run with this patch atop our CCF backport on 3.8 on on
exynos5250-snow. It works and looks good to me. We're planning to
put this patch into our tree at
<https://gerrit.chromium.org/gerrit/#/c/56740>.
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-29 23:44 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-29 23:44 UTC (permalink / raw)
To: Vikas Sajjan
Cc: linux-samsung-soc, Kukjin Kim, Tomasz Figa, yadi.brar01,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> This patch defines a common rate_table which will contain recommended p, m, s,
> k values for supported rates that needs to be changed for changing
> corresponding PLL's rate.
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
> 4 files changed, 54 insertions(+), 15 deletions(-)
I also reviewed this in our gerrit
<https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
here for the list...
> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> - const char *pname, const void __iomem *base)
> + const char *pname, const void __iomem *base,
> + const struct samsung_pll_rate_table *rate_table,
> + const unsigned int rate_count)
Feels like you should document here that rate_table needs to be sorted
and the sort order.
> +struct samsung_pll_rate_table {
> + unsigned int rate;
nit: extra space before "int" should be removed.
Also: you can include rate here if you need a convenient place to
store it (which sadly means that this structure can't be const).
...but I do like Tomasz's idea of actually calculating it. You can't
know it at compile time since the parent rate comes from the device
tree.
compatible = "samsung,clock-xxti";
clock-frequency = <24000000>;
> + unsigned int pdiv;
> + unsigned int mdiv;
> + unsigned int sdiv;
> + unsigned int kdiv;
I think kdiv is signed.
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-05-29 23:44 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-29 23:44 UTC (permalink / raw)
To: linux-arm-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> This patch defines a common rate_table which will contain recommended p, m, s,
> k values for supported rates that needs to be changed for changing
> corresponding PLL's rate.
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
> 4 files changed, 54 insertions(+), 15 deletions(-)
I also reviewed this in our gerrit
<https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
here for the list...
> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> - const char *pname, const void __iomem *base)
> + const char *pname, const void __iomem *base,
> + const struct samsung_pll_rate_table *rate_table,
> + const unsigned int rate_count)
Feels like you should document here that rate_table needs to be sorted
and the sort order.
> +struct samsung_pll_rate_table {
> + unsigned int rate;
nit: extra space before "int" should be removed.
Also: you can include rate here if you need a convenient place to
store it (which sadly means that this structure can't be const).
...but I do like Tomasz's idea of actually calculating it. You can't
know it at compile time since the parent rate comes from the device
tree.
compatible = "samsung,clock-xxti";
clock-frequency = <24000000>;
> + unsigned int pdiv;
> + unsigned int mdiv;
> + unsigned int sdiv;
> + unsigned int kdiv;
I think kdiv is signed.
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-05-29 23:44 ` Doug Anderson
@ 2013-05-30 6:55 ` Yadwinder Singh Brar
-1 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-05-30 6:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Vikas Sajjan, linux-samsung-soc, Kukjin Kim, Tomasz Figa,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Hi Doug,
On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org> wrote:
> Vikas / Yadwinder,
>
> On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> This patch defines a common rate_table which will contain recommended p, m, s,
>> k values for supported rates that needs to be changed for changing
>> corresponding PLL's rate.
>>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
>> 4 files changed, 54 insertions(+), 15 deletions(-)
>
> I also reviewed this in our gerrit
> <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> here for the list...
>
>> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> - const char *pname, const void __iomem *base)
>> + const char *pname, const void __iomem *base,
>> + const struct samsung_pll_rate_table *rate_table,
>> + const unsigned int rate_count)
>
> Feels like you should document here that rate_table needs to be sorted
> and the sort order.
>
sure, we will add comment to sort the table in descending order.
>> +struct samsung_pll_rate_table {
>> + unsigned int rate;
>
> nit: extra space before "int" should be removed.
>
ok
> Also: you can include rate here if you need a convenient place to
> store it (which sadly means that this structure can't be const).
> ...but I do like Tomasz's idea of actually calculating it. You can't
> know it at compile time since the parent rate comes from the device
> tree.
>
> compatible = "samsung,clock-xxti";
> clock-frequency = <24000000>;
>
Actually this table should contain the recommended values
and if we see the user manual, the input(parent) rate is also a part
of recommended
table of different output rate for a particular PLL for that SoC.
So as Tomasz said input(parent) rate may change with board,
then, do those corresponding recommended p, m, s, k will be valid?
In case, input(parent) rate changes then we may need different set of p, m ,s, k
values recommended for new input rate to get required(recommended to
use) output rate.
So, we think its better that the p, m, s and k along with the parent
is known at the compile time ( or DT ?),
as these p, m, s, k values are very much coupled with the parent rate
to achieve the
required(recommended to use) output rate.
Also, since the sorted table is required (sorted based on "rate"),
its better to have the rate in a const rate table.
And the whole set of recommended values should come from same place(DT
or static table),
to keep the things simple and consistent.
Moreover, practically for a particular SoC , we use the recommended
input(parent) rate only for a PLL.
So we should keep the things simple here presently.
>> + unsigned int pdiv;
>> + unsigned int mdiv;
>> + unsigned int sdiv;
>> + unsigned int kdiv;
>
> I think kdiv is signed.
>
No, as these values should be the recommended values to be written in
corresponding register bits. So it should remain unsigned.
K value should be considered as negative only while recalculating rate.
As per exynos5250 user manual's section 7.3.2 :
" K value description "Postive value (Negative value)":
Postive values is that you should write EPLLCON/VPLLCON register.
Negative value is that you can calculate PLL output frequency with it."
> -Doug
Regards,
Yadwinder & Vikas.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-05-30 6:55 ` Yadwinder Singh Brar
0 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-05-30 6:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi Doug,
On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org> wrote:
> Vikas / Yadwinder,
>
> On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> This patch defines a common rate_table which will contain recommended p, m, s,
>> k values for supported rates that needs to be changed for changing
>> corresponding PLL's rate.
>>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> drivers/clk/samsung/clk-pll.h | 33 +++++++++++++++++++++++++++++++--
>> 4 files changed, 54 insertions(+), 15 deletions(-)
>
> I also reviewed this in our gerrit
> <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> here for the list...
>
>> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> - const char *pname, const void __iomem *base)
>> + const char *pname, const void __iomem *base,
>> + const struct samsung_pll_rate_table *rate_table,
>> + const unsigned int rate_count)
>
> Feels like you should document here that rate_table needs to be sorted
> and the sort order.
>
sure, we will add comment to sort the table in descending order.
>> +struct samsung_pll_rate_table {
>> + unsigned int rate;
>
> nit: extra space before "int" should be removed.
>
ok
> Also: you can include rate here if you need a convenient place to
> store it (which sadly means that this structure can't be const).
> ...but I do like Tomasz's idea of actually calculating it. You can't
> know it at compile time since the parent rate comes from the device
> tree.
>
> compatible = "samsung,clock-xxti";
> clock-frequency = <24000000>;
>
Actually this table should contain the recommended values
and if we see the user manual, the input(parent) rate is also a part
of recommended
table of different output rate for a particular PLL for that SoC.
So as Tomasz said input(parent) rate may change with board,
then, do those corresponding recommended p, m, s, k will be valid?
In case, input(parent) rate changes then we may need different set of p, m ,s, k
values recommended for new input rate to get required(recommended to
use) output rate.
So, we think its better that the p, m, s and k along with the parent
is known at the compile time ( or DT ?),
as these p, m, s, k values are very much coupled with the parent rate
to achieve the
required(recommended to use) output rate.
Also, since the sorted table is required (sorted based on "rate"),
its better to have the rate in a const rate table.
And the whole set of recommended values should come from same place(DT
or static table),
to keep the things simple and consistent.
Moreover, practically for a particular SoC , we use the recommended
input(parent) rate only for a PLL.
So we should keep the things simple here presently.
>> + unsigned int pdiv;
>> + unsigned int mdiv;
>> + unsigned int sdiv;
>> + unsigned int kdiv;
>
> I think kdiv is signed.
>
No, as these values should be the recommended values to be written in
corresponding register bits. So it should remain unsigned.
K value should be considered as negative only while recalculating rate.
As per exynos5250 user manual's section 7.3.2 :
" K value description "Postive value (Negative value)":
Postive values is that you should write EPLLCON/VPLLCON register.
Negative value is that you can calculate PLL output frequency with it."
> -Doug
Regards,
Yadwinder & Vikas.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-05-30 6:55 ` Yadwinder Singh Brar
@ 2013-05-30 15:55 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 15:55 UTC (permalink / raw)
To: Yadwinder Singh Brar
Cc: Vikas Sajjan, linux-samsung-soc, Kukjin Kim, Tomasz Figa,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Yadwinder and Vikas,
On Wed, May 29, 2013 at 11:55 PM, Yadwinder Singh Brar
<yadi.brar01@gmail.com> wrote:
>> Also: you can include rate here if you need a convenient place to
>> store it (which sadly means that this structure can't be const).
>> ...but I do like Tomasz's idea of actually calculating it. You can't
>> know it at compile time since the parent rate comes from the device
>> tree.
>>
>> compatible = "samsung,clock-xxti";
>> clock-frequency = <24000000>;
>>
>
> Actually this table should contain the recommended values
> and if we see the user manual, the input(parent) rate is also a part
> of recommended
> table of different output rate for a particular PLL for that SoC.
That's a reasonable point. It's not clear to me whether the
recommended PMSK values would be the same or different if the input
wasn't 24MHz. You're assuming that they would be different and that
seems fair given my (limited) understanding of the electrical
characteristics of PLLs.
...so if a given SoC needed to support multiple different parent clock
frequencies the presumably it would have multiple tables and pick the
right one to pass in.
> So as Tomasz said input(parent) rate may change with board,
> then, do those corresponding recommended p, m, s, k will be valid?
>
> In case, input(parent) rate changes then we may need different set of p, m ,s, k
> values recommended for new input rate to get required(recommended to
> use) output rate.
>
> So, we think its better that the p, m, s and k along with the parent
> is known at the compile time ( or DT ?),
> as these p, m, s, k values are very much coupled with the parent rate
> to achieve the
> required(recommended to use) output rate.
Having the rate in the table definitely makes it easier to understand,
too. We just have to hope someone doesn't mess up in their
calculation. Worst case we could put some error checking somewhere.
> Also, since the sorted table is required (sorted based on "rate"),
> its better to have the rate in a const rate table.
You _could_ still sort the table based on eventually rate even if the
actual rate wasn't in the table. ...but I agree that it's harder to
look at and figure out.
> And the whole set of recommended values should come from same place(DT
> or static table),
> to keep the things simple and consistent.
Agree that it's a little weird now that some bits of the clock code is
in the device tree and some bits are just static tables in code.
...but I think that's OK for now.
Perhaps a reasonable compromise is to include "24mhz" in the name of
the tables (in later patches) and then give a WARN_ON if the xxti
clock isn't 24MHz. Then you've effectively documented the plan of
action should someone eventually need a different xxti clock.
> Moreover, practically for a particular SoC , we use the recommended
> input(parent) rate only for a PLL.
> So we should keep the things simple here presently.
>From what I've seen so far I could totally believe Samsung releasing a
new user manual in the future that provides instructions for using a
non-24MHz xxti clock. ...but with all of the above I think we've got
a reasonable story so I think we're good.
>>> + unsigned int pdiv;
>>> + unsigned int mdiv;
>>> + unsigned int sdiv;
>>> + unsigned int kdiv;
>>
>> I think kdiv is signed.
>>
>
> No, as these values should be the recommended values to be written in
> corresponding register bits. So it should remain unsigned.
>
> K value should be considered as negative only while recalculating rate.
>
> As per exynos5250 user manual's section 7.3.2 :
> " K value description "Postive value (Negative value)":
> Postive values is that you should write EPLLCON/VPLLCON register.
> Negative value is that you can calculate PLL output frequency with it."
OK, fair point. We can keep it unsigned in this table.
To summarize where I think we are in this series, I'm hoping for one
more version that:
* Adds a comment about the rate table being sorted (and which direction)
* Adds "24mhz" to the variable name for the PLL tables in future patch series
* Adds a WARN_ON if the xxti clock isn't 24MHz
Does that sound like a plan? Tomasz: any thoughts on the above?
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-05-30 15:55 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 15:55 UTC (permalink / raw)
To: linux-arm-kernel
Yadwinder and Vikas,
On Wed, May 29, 2013 at 11:55 PM, Yadwinder Singh Brar
<yadi.brar01@gmail.com> wrote:
>> Also: you can include rate here if you need a convenient place to
>> store it (which sadly means that this structure can't be const).
>> ...but I do like Tomasz's idea of actually calculating it. You can't
>> know it at compile time since the parent rate comes from the device
>> tree.
>>
>> compatible = "samsung,clock-xxti";
>> clock-frequency = <24000000>;
>>
>
> Actually this table should contain the recommended values
> and if we see the user manual, the input(parent) rate is also a part
> of recommended
> table of different output rate for a particular PLL for that SoC.
That's a reasonable point. It's not clear to me whether the
recommended PMSK values would be the same or different if the input
wasn't 24MHz. You're assuming that they would be different and that
seems fair given my (limited) understanding of the electrical
characteristics of PLLs.
...so if a given SoC needed to support multiple different parent clock
frequencies the presumably it would have multiple tables and pick the
right one to pass in.
> So as Tomasz said input(parent) rate may change with board,
> then, do those corresponding recommended p, m, s, k will be valid?
>
> In case, input(parent) rate changes then we may need different set of p, m ,s, k
> values recommended for new input rate to get required(recommended to
> use) output rate.
>
> So, we think its better that the p, m, s and k along with the parent
> is known at the compile time ( or DT ?),
> as these p, m, s, k values are very much coupled with the parent rate
> to achieve the
> required(recommended to use) output rate.
Having the rate in the table definitely makes it easier to understand,
too. We just have to hope someone doesn't mess up in their
calculation. Worst case we could put some error checking somewhere.
> Also, since the sorted table is required (sorted based on "rate"),
> its better to have the rate in a const rate table.
You _could_ still sort the table based on eventually rate even if the
actual rate wasn't in the table. ...but I agree that it's harder to
look at and figure out.
> And the whole set of recommended values should come from same place(DT
> or static table),
> to keep the things simple and consistent.
Agree that it's a little weird now that some bits of the clock code is
in the device tree and some bits are just static tables in code.
...but I think that's OK for now.
Perhaps a reasonable compromise is to include "24mhz" in the name of
the tables (in later patches) and then give a WARN_ON if the xxti
clock isn't 24MHz. Then you've effectively documented the plan of
action should someone eventually need a different xxti clock.
> Moreover, practically for a particular SoC , we use the recommended
> input(parent) rate only for a PLL.
> So we should keep the things simple here presently.
>From what I've seen so far I could totally believe Samsung releasing a
new user manual in the future that provides instructions for using a
non-24MHz xxti clock. ...but with all of the above I think we've got
a reasonable story so I think we're good.
>>> + unsigned int pdiv;
>>> + unsigned int mdiv;
>>> + unsigned int sdiv;
>>> + unsigned int kdiv;
>>
>> I think kdiv is signed.
>>
>
> No, as these values should be the recommended values to be written in
> corresponding register bits. So it should remain unsigned.
>
> K value should be considered as negative only while recalculating rate.
>
> As per exynos5250 user manual's section 7.3.2 :
> " K value description "Postive value (Negative value)":
> Postive values is that you should write EPLLCON/VPLLCON register.
> Negative value is that you can calculate PLL output frequency with it."
OK, fair point. We can keep it unsigned in this table.
To summarize where I think we are in this series, I'm hoping for one
more version that:
* Adds a comment about the rate table being sorted (and which direction)
* Adds "24mhz" to the variable name for the PLL tables in future patch series
* Adds a WARN_ON if the xxti clock isn't 24MHz
Does that sound like a plan? Tomasz: any thoughts on the above?
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-30 16:25 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 16:25 UTC (permalink / raw)
To: Vikas Sajjan
Cc: linux-samsung-soc, Kukjin Kim, Tomasz Figa, Yadwinder Singh Brar,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> This patch add set_rate() and round_rate() for PLL35xx
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
> drivers/clk/samsung/clk-pll.c | 103 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 102 insertions(+), 1 deletion(-)
[ cut ]
> +#define PLL35XX_LOCK_STAT_MASK (0x1)
optional: Mask is _slightly_ overkill here. Could just assume that
it's a 1-bit thing and do (1 << PLL35XX_LOCK_STAT_SHIFT).
All comments are optional, so feel free to add my Reviewed-by:
Reviewed-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx
@ 2013-05-30 16:25 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 16:25 UTC (permalink / raw)
To: linux-arm-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>
> This patch add set_rate() and round_rate() for PLL35xx
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
> drivers/clk/samsung/clk-pll.c | 103 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 102 insertions(+), 1 deletion(-)
[ cut ]
> +#define PLL35XX_LOCK_STAT_MASK (0x1)
optional: Mask is _slightly_ overkill here. Could just assume that
it's a 1-bit thing and do (1 << PLL35XX_LOCK_STAT_SHIFT).
All comments are optional, so feel free to add my Reviewed-by:
Reviewed-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx
2013-05-29 13:37 ` Vikas Sajjan
@ 2013-05-30 16:50 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 16:50 UTC (permalink / raw)
To: Vikas Sajjan
Cc: linux-samsung-soc, Kukjin Kim, Tomasz Figa, Yadwinder Singh Brar,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> This patch adds set_rate and round_rate clk_ops for PLL36xx
>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
> drivers/clk/samsung/clk-pll.c | 59 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
Looks good to me. I one nit on space/vs tab after
PLL36XX_LOCK_STAT_SHIFT that would be nice to fix, but not major.
Reviewed-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx
@ 2013-05-30 16:50 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-05-30 16:50 UTC (permalink / raw)
To: linux-arm-kernel
Vikas / Yadwinder,
On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> This patch adds set_rate and round_rate clk_ops for PLL36xx
>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
> drivers/clk/samsung/clk-pll.c | 59 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
Looks good to me. I one nit on space/vs tab after
PLL36XX_LOCK_STAT_SHIFT that would be nice to fix, but not major.
Reviewed-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-05-30 6:55 ` Yadwinder Singh Brar
@ 2013-06-03 15:25 ` Tomasz Figa
-1 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-03 15:25 UTC (permalink / raw)
To: Yadwinder Singh Brar
Cc: Doug Anderson, Vikas Sajjan, linux-samsung-soc, Kukjin Kim,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Hi Yadwinder,
On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> Hi Doug,
>
> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
wrote:
> > Vikas / Yadwinder,
> >
> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
wrote:
> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >>
> >> This patch defines a common rate_table which will contain recommended p,
> >> m, s, k values for supported rates that needs to be changed for changing
> >> corresponding PLL's rate.
> >>
> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> ---
> >>
> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> drivers/clk/samsung/clk-pll.h | 33
> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15
> >> deletions(-)
> >
> > I also reviewed this in our gerrit
> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> > here for the list...
> >
> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> >>
> >> - const char *pname, const void __iomem *base)
> >> + const char *pname, const void __iomem *base,
> >> + const struct samsung_pll_rate_table *rate_table,
> >> + const unsigned int rate_count)
> >
> > Feels like you should document here that rate_table needs to be sorted
> > and the sort order.
>
> sure, we will add comment to sort the table in descending order.
>
> >> +struct samsung_pll_rate_table {
> >> + unsigned int rate;
> >
> > nit: extra space before "int" should be removed.
>
> ok
>
> > Also: you can include rate here if you need a convenient place to
> > store it (which sadly means that this structure can't be const).
> > ...but I do like Tomasz's idea of actually calculating it. You can't
> > know it at compile time since the parent rate comes from the device
> > tree.
> >
> > compatible = "samsung,clock-xxti";
> > clock-frequency = <24000000>;
>
> Actually this table should contain the recommended values
> and if we see the user manual, the input(parent) rate is also a part
> of recommended
> table of different output rate for a particular PLL for that SoC.
>From what I understood in the documentation is that there is a set of
recommended P, M, S (, K) tuples for each PLL and they are not dependent on
input frequency - f_in and f_out are provided in the table just for reference
to see the relation between output frequency and input frequency.
I think we should ask some H/W engineer about that to make sure and choose the
proper implementation, which will work properly for future cases, instead of
ending with something that works just with current cases.
Best regards,
--
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics
> So as Tomasz said input(parent) rate may change with board,
> then, do those corresponding recommended p, m, s, k will be valid?
>
> In case, input(parent) rate changes then we may need different set of p, m
> ,s, k values recommended for new input rate to get required(recommended to
> use) output rate.
>
> So, we think its better that the p, m, s and k along with the parent
> is known at the compile time ( or DT ?),
> as these p, m, s, k values are very much coupled with the parent rate
> to achieve the
> required(recommended to use) output rate.
>
> Also, since the sorted table is required (sorted based on "rate"),
> its better to have the rate in a const rate table.
>
> And the whole set of recommended values should come from same place(DT
> or static table),
> to keep the things simple and consistent.
>
> Moreover, practically for a particular SoC , we use the recommended
> input(parent) rate only for a PLL.
> So we should keep the things simple here presently.
>
> >> + unsigned int pdiv;
> >> + unsigned int mdiv;
> >> + unsigned int sdiv;
> >> + unsigned int kdiv;
> >
> > I think kdiv is signed.
>
> No, as these values should be the recommended values to be written in
> corresponding register bits. So it should remain unsigned.
>
> K value should be considered as negative only while recalculating rate.
>
> As per exynos5250 user manual's section 7.3.2 :
> " K value description "Postive value (Negative value)":
> Postive values is that you should write EPLLCON/VPLLCON register.
> Negative value is that you can calculate PLL output frequency with it."
>
> > -Doug
>
> Regards,
> Yadwinder & Vikas.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-03 15:25 ` Tomasz Figa
0 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-03 15:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi Yadwinder,
On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> Hi Doug,
>
> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
wrote:
> > Vikas / Yadwinder,
> >
> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
wrote:
> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >>
> >> This patch defines a common rate_table which will contain recommended p,
> >> m, s, k values for supported rates that needs to be changed for changing
> >> corresponding PLL's rate.
> >>
> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> ---
> >>
> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> drivers/clk/samsung/clk-pll.h | 33
> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15
> >> deletions(-)
> >
> > I also reviewed this in our gerrit
> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> > here for the list...
> >
> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> >>
> >> - const char *pname, const void __iomem *base)
> >> + const char *pname, const void __iomem *base,
> >> + const struct samsung_pll_rate_table *rate_table,
> >> + const unsigned int rate_count)
> >
> > Feels like you should document here that rate_table needs to be sorted
> > and the sort order.
>
> sure, we will add comment to sort the table in descending order.
>
> >> +struct samsung_pll_rate_table {
> >> + unsigned int rate;
> >
> > nit: extra space before "int" should be removed.
>
> ok
>
> > Also: you can include rate here if you need a convenient place to
> > store it (which sadly means that this structure can't be const).
> > ...but I do like Tomasz's idea of actually calculating it. You can't
> > know it at compile time since the parent rate comes from the device
> > tree.
> >
> > compatible = "samsung,clock-xxti";
> > clock-frequency = <24000000>;
>
> Actually this table should contain the recommended values
> and if we see the user manual, the input(parent) rate is also a part
> of recommended
> table of different output rate for a particular PLL for that SoC.
>From what I understood in the documentation is that there is a set of
recommended P, M, S (, K) tuples for each PLL and they are not dependent on
input frequency - f_in and f_out are provided in the table just for reference
to see the relation between output frequency and input frequency.
I think we should ask some H/W engineer about that to make sure and choose the
proper implementation, which will work properly for future cases, instead of
ending with something that works just with current cases.
Best regards,
--
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics
> So as Tomasz said input(parent) rate may change with board,
> then, do those corresponding recommended p, m, s, k will be valid?
>
> In case, input(parent) rate changes then we may need different set of p, m
> ,s, k values recommended for new input rate to get required(recommended to
> use) output rate.
>
> So, we think its better that the p, m, s and k along with the parent
> is known at the compile time ( or DT ?),
> as these p, m, s, k values are very much coupled with the parent rate
> to achieve the
> required(recommended to use) output rate.
>
> Also, since the sorted table is required (sorted based on "rate"),
> its better to have the rate in a const rate table.
>
> And the whole set of recommended values should come from same place(DT
> or static table),
> to keep the things simple and consistent.
>
> Moreover, practically for a particular SoC , we use the recommended
> input(parent) rate only for a PLL.
> So we should keep the things simple here presently.
>
> >> + unsigned int pdiv;
> >> + unsigned int mdiv;
> >> + unsigned int sdiv;
> >> + unsigned int kdiv;
> >
> > I think kdiv is signed.
>
> No, as these values should be the recommended values to be written in
> corresponding register bits. So it should remain unsigned.
>
> K value should be considered as negative only while recalculating rate.
>
> As per exynos5250 user manual's section 7.3.2 :
> " K value description "Postive value (Negative value)":
> Postive values is that you should write EPLLCON/VPLLCON register.
> Negative value is that you can calculate PLL output frequency with it."
>
> > -Doug
>
> Regards,
> Yadwinder & Vikas.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-06-03 15:25 ` Tomasz Figa
@ 2013-06-03 20:58 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-06-03 20:58 UTC (permalink / raw)
To: Tomasz Figa
Cc: Yadwinder Singh Brar, Vikas Sajjan, linux-samsung-soc,
Kukjin Kim, Mike Turquette, linux-arm-kernel, Thomas Abraham,
Patch Tracking, linaro-kernel
Tomasz,
On Mon, Jun 3, 2013 at 8:25 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> From what I understood in the documentation is that there is a set of
> recommended P, M, S (, K) tuples for each PLL and they are not dependent on
> input frequency - f_in and f_out are provided in the table just for reference
> to see the relation between output frequency and input frequency.
>
> I think we should ask some H/W engineer about that to make sure and choose the
> proper implementation, which will work properly for future cases, instead of
> ending with something that works just with current cases.
Hopefully you and Yadwinder can take this on? You probably have
better access to the right people to ask.
I did dig up my 4210 manual which actually has one place where it
shows two options for FIN of VPLL: 24MHz and 27MHz. They don't seem
to share any PMSK values that are the same.
It seems like the tables were generated by choosing a bunch of
frequencies that people would probably like to make and then finding
the best way to make that frequency (probably backed up by a bunch of
simulation and testing of that exact combination).
We have landed most of this series of patches (with a few
modifications to later patches to avoid adding a global alias)..
...but if you decided to go a different way we'll take your version of
course (either now with a revert / re-pick or when we rebase later).
:)
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-03 20:58 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2013-06-03 20:58 UTC (permalink / raw)
To: linux-arm-kernel
Tomasz,
On Mon, Jun 3, 2013 at 8:25 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> From what I understood in the documentation is that there is a set of
> recommended P, M, S (, K) tuples for each PLL and they are not dependent on
> input frequency - f_in and f_out are provided in the table just for reference
> to see the relation between output frequency and input frequency.
>
> I think we should ask some H/W engineer about that to make sure and choose the
> proper implementation, which will work properly for future cases, instead of
> ending with something that works just with current cases.
Hopefully you and Yadwinder can take this on? You probably have
better access to the right people to ask.
I did dig up my 4210 manual which actually has one place where it
shows two options for FIN of VPLL: 24MHz and 27MHz. They don't seem
to share any PMSK values that are the same.
It seems like the tables were generated by choosing a bunch of
frequencies that people would probably like to make and then finding
the best way to make that frequency (probably backed up by a bunch of
simulation and testing of that exact combination).
We have landed most of this series of patches (with a few
modifications to later patches to avoid adding a global alias)..
...but if you decided to go a different way we'll take your version of
course (either now with a revert / re-pick or when we rebase later).
:)
-Doug
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-06-03 15:25 ` Tomasz Figa
@ 2013-06-04 11:32 ` Yadwinder Singh Brar
-1 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-06-04 11:32 UTC (permalink / raw)
To: Tomasz Figa
Cc: Doug Anderson, Vikas Sajjan, linux-samsung-soc, Kukjin Kim,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
Hi Tomasz,
On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yadwinder,
>
> On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
>> Hi Doug,
>>
>> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
> wrote:
>> > Vikas / Yadwinder,
>> >
>> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
> wrote:
>> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >>
>> >> This patch defines a common rate_table which will contain recommended p,
>> >> m, s, k values for supported rates that needs to be changed for changing
>> >> corresponding PLL's rate.
>> >>
>> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> ---
>> >>
>> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> >> drivers/clk/samsung/clk-pll.h | 33
>> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15
>> >> deletions(-)
>> >
>> > I also reviewed this in our gerrit
>> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
>> > here for the list...
>> >
>> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> >>
>> >> - const char *pname, const void __iomem *base)
>> >> + const char *pname, const void __iomem *base,
>> >> + const struct samsung_pll_rate_table *rate_table,
>> >> + const unsigned int rate_count)
>> >
>> > Feels like you should document here that rate_table needs to be sorted
>> > and the sort order.
>>
>> sure, we will add comment to sort the table in descending order.
>>
>> >> +struct samsung_pll_rate_table {
>> >> + unsigned int rate;
>> >
>> > nit: extra space before "int" should be removed.
>>
>> ok
>>
>> > Also: you can include rate here if you need a convenient place to
>> > store it (which sadly means that this structure can't be const).
>> > ...but I do like Tomasz's idea of actually calculating it. You can't
>> > know it at compile time since the parent rate comes from the device
>> > tree.
>> >
>> > compatible = "samsung,clock-xxti";
>> > clock-frequency = <24000000>;
>>
>> Actually this table should contain the recommended values
>> and if we see the user manual, the input(parent) rate is also a part
>> of recommended
>> table of different output rate for a particular PLL for that SoC.
>
> From what I understood in the documentation is that there is a set of
> recommended P, M, S (, K) tuples for each PLL and they are not dependent on
> input frequency - f_in and f_out are provided in the table just for reference
> to see the relation between output frequency and input frequency.
>
If input rate(f_in) gets changed for PLL, then the corresponding
output rates(f_out)
will change by using same(common) set of recommended P, M, S (, K),
and the new set of output rates(f_out) may not be the _required_ set
of target rates.
So we need different set of P, M, S (,K) values for different f_in.
Table should contain set of P, M, S (,K) values for the _required_
target(f_out) rates
for a particular input(f_in) rate.
> I think we should ask some H/W engineer about that to make sure and choose the
> proper implementation, which will work properly for future cases, instead of
> ending with something that works just with current cases.
>
We already asked hardware engineer about PMS values for PLL,
and we got a table containing recommended P, M ,S values for a given
f_in(24 MHz)
and required f_out rates.
Please let me know, why you think we are specific to current cases only ?
Regards,
Yadwinder
> Best regards,
> --
> Tomasz Figa
> Linux Kernel Developer
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> So as Tomasz said input(parent) rate may change with board,
>> then, do those corresponding recommended p, m, s, k will be valid?
>>
>> In case, input(parent) rate changes then we may need different set of p, m
>> ,s, k values recommended for new input rate to get required(recommended to
>> use) output rate.
>>
>> So, we think its better that the p, m, s and k along with the parent
>> is known at the compile time ( or DT ?),
>> as these p, m, s, k values are very much coupled with the parent rate
>> to achieve the
>> required(recommended to use) output rate.
>>
>> Also, since the sorted table is required (sorted based on "rate"),
>> its better to have the rate in a const rate table.
>>
>> And the whole set of recommended values should come from same place(DT
>> or static table),
>> to keep the things simple and consistent.
>>
>> Moreover, practically for a particular SoC , we use the recommended
>> input(parent) rate only for a PLL.
>> So we should keep the things simple here presently.
>>
>> >> + unsigned int pdiv;
>> >> + unsigned int mdiv;
>> >> + unsigned int sdiv;
>> >> + unsigned int kdiv;
>> >
>> > I think kdiv is signed.
>>
>> No, as these values should be the recommended values to be written in
>> corresponding register bits. So it should remain unsigned.
>>
>> K value should be considered as negative only while recalculating rate.
>>
>> As per exynos5250 user manual's section 7.3.2 :
>> " K value description "Postive value (Negative value)":
>> Postive values is that you should write EPLLCON/VPLLCON register.
>> Negative value is that you can calculate PLL output frequency with it."
>>
>> > -Doug
>>
>> Regards,
>> Yadwinder & Vikas.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-04 11:32 ` Yadwinder Singh Brar
0 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-06-04 11:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tomasz,
On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yadwinder,
>
> On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
>> Hi Doug,
>>
>> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
> wrote:
>> > Vikas / Yadwinder,
>> >
>> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
> wrote:
>> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >>
>> >> This patch defines a common rate_table which will contain recommended p,
>> >> m, s, k values for supported rates that needs to be changed for changing
>> >> corresponding PLL's rate.
>> >>
>> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> ---
>> >>
>> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> >> drivers/clk/samsung/clk-pll.h | 33
>> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15
>> >> deletions(-)
>> >
>> > I also reviewed this in our gerrit
>> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
>> > here for the list...
>> >
>> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> >>
>> >> - const char *pname, const void __iomem *base)
>> >> + const char *pname, const void __iomem *base,
>> >> + const struct samsung_pll_rate_table *rate_table,
>> >> + const unsigned int rate_count)
>> >
>> > Feels like you should document here that rate_table needs to be sorted
>> > and the sort order.
>>
>> sure, we will add comment to sort the table in descending order.
>>
>> >> +struct samsung_pll_rate_table {
>> >> + unsigned int rate;
>> >
>> > nit: extra space before "int" should be removed.
>>
>> ok
>>
>> > Also: you can include rate here if you need a convenient place to
>> > store it (which sadly means that this structure can't be const).
>> > ...but I do like Tomasz's idea of actually calculating it. You can't
>> > know it at compile time since the parent rate comes from the device
>> > tree.
>> >
>> > compatible = "samsung,clock-xxti";
>> > clock-frequency = <24000000>;
>>
>> Actually this table should contain the recommended values
>> and if we see the user manual, the input(parent) rate is also a part
>> of recommended
>> table of different output rate for a particular PLL for that SoC.
>
> From what I understood in the documentation is that there is a set of
> recommended P, M, S (, K) tuples for each PLL and they are not dependent on
> input frequency - f_in and f_out are provided in the table just for reference
> to see the relation between output frequency and input frequency.
>
If input rate(f_in) gets changed for PLL, then the corresponding
output rates(f_out)
will change by using same(common) set of recommended P, M, S (, K),
and the new set of output rates(f_out) may not be the _required_ set
of target rates.
So we need different set of P, M, S (,K) values for different f_in.
Table should contain set of P, M, S (,K) values for the _required_
target(f_out) rates
for a particular input(f_in) rate.
> I think we should ask some H/W engineer about that to make sure and choose the
> proper implementation, which will work properly for future cases, instead of
> ending with something that works just with current cases.
>
We already asked hardware engineer about PMS values for PLL,
and we got a table containing recommended P, M ,S values for a given
f_in(24 MHz)
and required f_out rates.
Please let me know, why you think we are specific to current cases only ?
Regards,
Yadwinder
> Best regards,
> --
> Tomasz Figa
> Linux Kernel Developer
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> So as Tomasz said input(parent) rate may change with board,
>> then, do those corresponding recommended p, m, s, k will be valid?
>>
>> In case, input(parent) rate changes then we may need different set of p, m
>> ,s, k values recommended for new input rate to get required(recommended to
>> use) output rate.
>>
>> So, we think its better that the p, m, s and k along with the parent
>> is known at the compile time ( or DT ?),
>> as these p, m, s, k values are very much coupled with the parent rate
>> to achieve the
>> required(recommended to use) output rate.
>>
>> Also, since the sorted table is required (sorted based on "rate"),
>> its better to have the rate in a const rate table.
>>
>> And the whole set of recommended values should come from same place(DT
>> or static table),
>> to keep the things simple and consistent.
>>
>> Moreover, practically for a particular SoC , we use the recommended
>> input(parent) rate only for a PLL.
>> So we should keep the things simple here presently.
>>
>> >> + unsigned int pdiv;
>> >> + unsigned int mdiv;
>> >> + unsigned int sdiv;
>> >> + unsigned int kdiv;
>> >
>> > I think kdiv is signed.
>>
>> No, as these values should be the recommended values to be written in
>> corresponding register bits. So it should remain unsigned.
>>
>> K value should be considered as negative only while recalculating rate.
>>
>> As per exynos5250 user manual's section 7.3.2 :
>> " K value description "Postive value (Negative value)":
>> Postive values is that you should write EPLLCON/VPLLCON register.
>> Negative value is that you can calculate PLL output frequency with it."
>>
>> > -Doug
>>
>> Regards,
>> Yadwinder & Vikas.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-06-04 11:32 ` Yadwinder Singh Brar
@ 2013-06-05 13:40 ` Tomasz Figa
-1 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-05 13:40 UTC (permalink / raw)
To: Yadwinder Singh Brar, Kukjin Kim
Cc: Doug Anderson, Vikas Sajjan, linux-samsung-soc, Mike Turquette,
linux-arm-kernel, Thomas Abraham, Patch Tracking, linaro-kernel
Hi Yadwinder,
On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
> Hi Tomasz,
>
> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Yadwinder,
> >
> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> >> Hi Doug,
> >>
> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
> >
> > wrote:
> >> > Vikas / Yadwinder,
> >> >
> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
> >
> > wrote:
> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >>
> >> >> This patch defines a common rate_table which will contain recommended
> >> >> p,
> >> >> m, s, k values for supported rates that needs to be changed for
> >> >> changing
> >> >> corresponding PLL's rate.
> >> >>
> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> ---
> >> >>
> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> >> drivers/clk/samsung/clk-pll.h | 33
> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+),
> >> >> 15
> >> >> deletions(-)
> >> >
> >> > I also reviewed this in our gerrit
> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> >> > here for the list...
> >> >
> >> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> >> >>
> >> >> - const char *pname, const void __iomem *base)
> >> >> + const char *pname, const void __iomem *base,
> >> >> + const struct samsung_pll_rate_table
> >> >> *rate_table,
> >> >> + const unsigned int rate_count)
> >> >
> >> > Feels like you should document here that rate_table needs to be sorted
> >> > and the sort order.
> >>
> >> sure, we will add comment to sort the table in descending order.
> >>
> >> >> +struct samsung_pll_rate_table {
> >> >> + unsigned int rate;
> >> >
> >> > nit: extra space before "int" should be removed.
> >>
> >> ok
> >>
> >> > Also: you can include rate here if you need a convenient place to
> >> > store it (which sadly means that this structure can't be const).
> >> > ...but I do like Tomasz's idea of actually calculating it. You can't
> >> > know it at compile time since the parent rate comes from the device
> >> > tree.
> >> >
> >> > compatible = "samsung,clock-xxti";
> >> > clock-frequency = <24000000>;
> >>
> >> Actually this table should contain the recommended values
> >> and if we see the user manual, the input(parent) rate is also a part
> >> of recommended
> >> table of different output rate for a particular PLL for that SoC.
> >
> > From what I understood in the documentation is that there is a set of
> > recommended P, M, S (, K) tuples for each PLL and they are not dependent
> > on
> > input frequency - f_in and f_out are provided in the table just for
> > reference to see the relation between output frequency and input
> > frequency.
>
> If input rate(f_in) gets changed for PLL, then the corresponding
> output rates(f_out)
> will change by using same(common) set of recommended P, M, S (, K),
> and the new set of output rates(f_out) may not be the _required_ set
> of target rates.
> So we need different set of P, M, S (,K) values for different f_in.
>
> Table should contain set of P, M, S (,K) values for the _required_
> target(f_out) rates
> for a particular input(f_in) rate.
I'm not sure what required rates you are talking about. For most PLLs the list
of available frequencies seems to be defined by the set of supported PMS values
and those are the frequencies that can be set by consumers, not the other way
around.
There are some cases when some particular frequency must be generated, like
PLLs used for audio or video clock sources, but this just means that there
must be an entry in PMS table that gives PLL configuration which produces such
frequency with used PLL input clock.
> > I think we should ask some H/W engineer about that to make sure and choose
> > the proper implementation, which will work properly for future cases,
> > instead of ending with something that works just with current cases.
>
> We already asked hardware engineer about PMS values for PLL,
> and we got a table containing recommended P, M ,S values for a given
> f_in(24 MHz)
> and required f_out rates.
This doesn't answer the question about using those PMS values with different
input rate.
Kukjin, maybe you can give some information on this or point to a person who
could?
> Please let me know, why you think we are specific to current cases only ?
I don't like the idea of having separate tables for each input rate. Just
imagine how much data would have to be added if boards with several different
input rates showed up.
Sure, if each input rate requires different set of PMS values by hardware
design, it can't be helped, but if the same PMS values can be reused (even
giving different output rates, but this is usually not important), one table
can be used for all cases.
Best regards,
--
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics
> Regards,
> Yadwinder
>
> > Best regards,
> > --
> > Tomasz Figa
> > Linux Kernel Developer
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> >> So as Tomasz said input(parent) rate may change with board,
> >> then, do those corresponding recommended p, m, s, k will be valid?
> >>
> >> In case, input(parent) rate changes then we may need different set of p,
> >> m
> >> ,s, k values recommended for new input rate to get required(recommended
> >> to
> >> use) output rate.
> >>
> >> So, we think its better that the p, m, s and k along with the parent
> >> is known at the compile time ( or DT ?),
> >> as these p, m, s, k values are very much coupled with the parent rate
> >> to achieve the
> >>
> >> required(recommended to use) output rate.
> >>
> >> Also, since the sorted table is required (sorted based on "rate"),
> >> its better to have the rate in a const rate table.
> >>
> >> And the whole set of recommended values should come from same place(DT
> >> or static table),
> >> to keep the things simple and consistent.
> >>
> >> Moreover, practically for a particular SoC , we use the recommended
> >> input(parent) rate only for a PLL.
> >> So we should keep the things simple here presently.
> >>
> >> >> + unsigned int pdiv;
> >> >> + unsigned int mdiv;
> >> >> + unsigned int sdiv;
> >> >> + unsigned int kdiv;
> >> >
> >> > I think kdiv is signed.
> >>
> >> No, as these values should be the recommended values to be written in
> >> corresponding register bits. So it should remain unsigned.
> >>
> >> K value should be considered as negative only while recalculating rate.
> >>
> >> As per exynos5250 user manual's section 7.3.2 :
> >> " K value description "Postive value (Negative value)":
> >> Postive values is that you should write EPLLCON/VPLLCON register.
> >> Negative value is that you can calculate PLL output frequency with it."
> >>
> >> > -Doug
> >>
> >> Regards,
> >> Yadwinder & Vikas.
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >> linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-05 13:40 ` Tomasz Figa
0 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-05 13:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Yadwinder,
On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
> Hi Tomasz,
>
> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Yadwinder,
> >
> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> >> Hi Doug,
> >>
> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
> >
> > wrote:
> >> > Vikas / Yadwinder,
> >> >
> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
> >
> > wrote:
> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >>
> >> >> This patch defines a common rate_table which will contain recommended
> >> >> p,
> >> >> m, s, k values for supported rates that needs to be changed for
> >> >> changing
> >> >> corresponding PLL's rate.
> >> >>
> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> ---
> >> >>
> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> >> drivers/clk/samsung/clk-pll.h | 33
> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+),
> >> >> 15
> >> >> deletions(-)
> >> >
> >> > I also reviewed this in our gerrit
> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
> >> > here for the list...
> >> >
> >> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
> >> >>
> >> >> - const char *pname, const void __iomem *base)
> >> >> + const char *pname, const void __iomem *base,
> >> >> + const struct samsung_pll_rate_table
> >> >> *rate_table,
> >> >> + const unsigned int rate_count)
> >> >
> >> > Feels like you should document here that rate_table needs to be sorted
> >> > and the sort order.
> >>
> >> sure, we will add comment to sort the table in descending order.
> >>
> >> >> +struct samsung_pll_rate_table {
> >> >> + unsigned int rate;
> >> >
> >> > nit: extra space before "int" should be removed.
> >>
> >> ok
> >>
> >> > Also: you can include rate here if you need a convenient place to
> >> > store it (which sadly means that this structure can't be const).
> >> > ...but I do like Tomasz's idea of actually calculating it. You can't
> >> > know it at compile time since the parent rate comes from the device
> >> > tree.
> >> >
> >> > compatible = "samsung,clock-xxti";
> >> > clock-frequency = <24000000>;
> >>
> >> Actually this table should contain the recommended values
> >> and if we see the user manual, the input(parent) rate is also a part
> >> of recommended
> >> table of different output rate for a particular PLL for that SoC.
> >
> > From what I understood in the documentation is that there is a set of
> > recommended P, M, S (, K) tuples for each PLL and they are not dependent
> > on
> > input frequency - f_in and f_out are provided in the table just for
> > reference to see the relation between output frequency and input
> > frequency.
>
> If input rate(f_in) gets changed for PLL, then the corresponding
> output rates(f_out)
> will change by using same(common) set of recommended P, M, S (, K),
> and the new set of output rates(f_out) may not be the _required_ set
> of target rates.
> So we need different set of P, M, S (,K) values for different f_in.
>
> Table should contain set of P, M, S (,K) values for the _required_
> target(f_out) rates
> for a particular input(f_in) rate.
I'm not sure what required rates you are talking about. For most PLLs the list
of available frequencies seems to be defined by the set of supported PMS values
and those are the frequencies that can be set by consumers, not the other way
around.
There are some cases when some particular frequency must be generated, like
PLLs used for audio or video clock sources, but this just means that there
must be an entry in PMS table that gives PLL configuration which produces such
frequency with used PLL input clock.
> > I think we should ask some H/W engineer about that to make sure and choose
> > the proper implementation, which will work properly for future cases,
> > instead of ending with something that works just with current cases.
>
> We already asked hardware engineer about PMS values for PLL,
> and we got a table containing recommended P, M ,S values for a given
> f_in(24 MHz)
> and required f_out rates.
This doesn't answer the question about using those PMS values with different
input rate.
Kukjin, maybe you can give some information on this or point to a person who
could?
> Please let me know, why you think we are specific to current cases only ?
I don't like the idea of having separate tables for each input rate. Just
imagine how much data would have to be added if boards with several different
input rates showed up.
Sure, if each input rate requires different set of PMS values by hardware
design, it can't be helped, but if the same PMS values can be reused (even
giving different output rates, but this is usually not important), one table
can be used for all cases.
Best regards,
--
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics
> Regards,
> Yadwinder
>
> > Best regards,
> > --
> > Tomasz Figa
> > Linux Kernel Developer
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> >> So as Tomasz said input(parent) rate may change with board,
> >> then, do those corresponding recommended p, m, s, k will be valid?
> >>
> >> In case, input(parent) rate changes then we may need different set of p,
> >> m
> >> ,s, k values recommended for new input rate to get required(recommended
> >> to
> >> use) output rate.
> >>
> >> So, we think its better that the p, m, s and k along with the parent
> >> is known at the compile time ( or DT ?),
> >> as these p, m, s, k values are very much coupled with the parent rate
> >> to achieve the
> >>
> >> required(recommended to use) output rate.
> >>
> >> Also, since the sorted table is required (sorted based on "rate"),
> >> its better to have the rate in a const rate table.
> >>
> >> And the whole set of recommended values should come from same place(DT
> >> or static table),
> >> to keep the things simple and consistent.
> >>
> >> Moreover, practically for a particular SoC , we use the recommended
> >> input(parent) rate only for a PLL.
> >> So we should keep the things simple here presently.
> >>
> >> >> + unsigned int pdiv;
> >> >> + unsigned int mdiv;
> >> >> + unsigned int sdiv;
> >> >> + unsigned int kdiv;
> >> >
> >> > I think kdiv is signed.
> >>
> >> No, as these values should be the recommended values to be written in
> >> corresponding register bits. So it should remain unsigned.
> >>
> >> K value should be considered as negative only while recalculating rate.
> >>
> >> As per exynos5250 user manual's section 7.3.2 :
> >> " K value description "Postive value (Negative value)":
> >> Postive values is that you should write EPLLCON/VPLLCON register.
> >> Negative value is that you can calculate PLL output frequency with it."
> >>
> >> > -Doug
> >>
> >> Regards,
> >> Yadwinder & Vikas.
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >> linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-06-05 13:40 ` Tomasz Figa
@ 2013-06-06 9:01 ` Yadwinder Singh Brar
-1 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-06-06 9:01 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kukjin Kim, Doug Anderson, Vikas Sajjan, linux-samsung-soc,
Mike Turquette, linux-arm-kernel, Thomas Abraham, Patch Tracking,
linaro-kernel
On Wed, Jun 5, 2013 at 7:10 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yadwinder,
>
> On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
>> Hi Tomasz,
>>
>> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > Hi Yadwinder,
>> >
>> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
>> >> Hi Doug,
>> >>
>> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
>> >
>> > wrote:
>> >> > Vikas / Yadwinder,
>> >> >
>> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
>> >
>> > wrote:
>> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> >>
>> >> >> This patch defines a common rate_table which will contain recommended
>> >> >> p,
>> >> >> m, s, k values for supported rates that needs to be changed for
>> >> >> changing
>> >> >> corresponding PLL's rate.
>> >> >>
>> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> >> ---
>> >> >>
>> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> >> >> drivers/clk/samsung/clk-pll.h | 33
>> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+),
>> >> >> 15
>> >> >> deletions(-)
>> >> >
>> >> > I also reviewed this in our gerrit
>> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
>> >> > here for the list...
>> >> >
>> >> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> >> >>
>> >> >> - const char *pname, const void __iomem *base)
>> >> >> + const char *pname, const void __iomem *base,
>> >> >> + const struct samsung_pll_rate_table
>> >> >> *rate_table,
>> >> >> + const unsigned int rate_count)
>> >> >
>> >> > Feels like you should document here that rate_table needs to be sorted
>> >> > and the sort order.
>> >>
>> >> sure, we will add comment to sort the table in descending order.
>> >>
>> >> >> +struct samsung_pll_rate_table {
>> >> >> + unsigned int rate;
>> >> >
>> >> > nit: extra space before "int" should be removed.
>> >>
>> >> ok
>> >>
>> >> > Also: you can include rate here if you need a convenient place to
>> >> > store it (which sadly means that this structure can't be const).
>> >> > ...but I do like Tomasz's idea of actually calculating it. You can't
>> >> > know it at compile time since the parent rate comes from the device
>> >> > tree.
>> >> >
>> >> > compatible = "samsung,clock-xxti";
>> >> > clock-frequency = <24000000>;
>> >>
>> >> Actually this table should contain the recommended values
>> >> and if we see the user manual, the input(parent) rate is also a part
>> >> of recommended
>> >> table of different output rate for a particular PLL for that SoC.
>> >
>> > From what I understood in the documentation is that there is a set of
>> > recommended P, M, S (, K) tuples for each PLL and they are not dependent
>> > on
>> > input frequency - f_in and f_out are provided in the table just for
>> > reference to see the relation between output frequency and input
>> > frequency.
>>
>> If input rate(f_in) gets changed for PLL, then the corresponding
>> output rates(f_out)
>> will change by using same(common) set of recommended P, M, S (, K),
>> and the new set of output rates(f_out) may not be the _required_ set
>> of target rates.
>> So we need different set of P, M, S (,K) values for different f_in.
>>
>> Table should contain set of P, M, S (,K) values for the _required_
>> target(f_out) rates
>> for a particular input(f_in) rate.
>
> I'm not sure what required rates you are talking about. For most PLLs the list
> of available frequencies seems to be defined by the set of supported PMS values
> and those are the frequencies that can be set by consumers, not the other way
> around.
>
> There are some cases when some particular frequency must be generated, like
> PLLs used for audio or video clock sources, but this just means that there
> must be an entry in PMS table that gives PLL configuration which produces such
> frequency with used PLL input clock.
>
Yes, I was talking about same cases. Perhaps,I think it holds true in general
for PLLs, whose set_rate can be called.
>> > I think we should ask some H/W engineer about that to make sure and choose
>> > the proper implementation, which will work properly for future cases,
>> > instead of ending with something that works just with current cases.
>>
>> We already asked hardware engineer about PMS values for PLL,
>> and we got a table containing recommended P, M ,S values for a given
>> f_in(24 MHz)
>> and required f_out rates.
>
> This doesn't answer the question about using those PMS values with different
> input rate.
>
> Kukjin, maybe you can give some information on this or point to a person who
> could?
>
>> Please let me know, why you think we are specific to current cases only ?
>
> I don't like the idea of having separate tables for each input rate. Just
> imagine how much data would have to be added if boards with several different
> input rates showed up.
>
I think by declaring all these different table as __init,
and creating a copy to be used while registering, we can
save a little memory as well as the unnecessary cost which
we have to bear while doing set_rate() and round_rate()
by using single big table(for some time critical cases).
> Sure, if each input rate requires different set of PMS values by hardware
> design, it can't be helped, but if the same PMS values can be reused (even
> giving different output rates, but this is usually not important), one table
> can be used for all cases.
>
Its good to know whether same PMS values can be reused, from hardware guys.
Using different tables we can always blindly stick to recommended values without
bothering much every time, whether existing PMS can be reused or not
while adding new cases.
>> Regards,
>> Yadwinder
>>
>> > Best regards,
>> > --
>> > Tomasz Figa
>> > Linux Kernel Developer
>> > Samsung R&D Institute Poland
>> > Samsung Electronics
>> >
>> >> So as Tomasz said input(parent) rate may change with board,
>> >> then, do those corresponding recommended p, m, s, k will be valid?
>> >>
>> >> In case, input(parent) rate changes then we may need different set of p,
>> >> m
>> >> ,s, k values recommended for new input rate to get required(recommended
>> >> to
>> >> use) output rate.
>> >>
>> >> So, we think its better that the p, m, s and k along with the parent
>> >> is known at the compile time ( or DT ?),
>> >> as these p, m, s, k values are very much coupled with the parent rate
>> >> to achieve the
>> >>
>> >> required(recommended to use) output rate.
>> >>
>> >> Also, since the sorted table is required (sorted based on "rate"),
>> >> its better to have the rate in a const rate table.
>> >>
>> >> And the whole set of recommended values should come from same place(DT
>> >> or static table),
>> >> to keep the things simple and consistent.
>> >>
>> >> Moreover, practically for a particular SoC , we use the recommended
>> >> input(parent) rate only for a PLL.
>> >> So we should keep the things simple here presently.
>> >>
>> >> >> + unsigned int pdiv;
>> >> >> + unsigned int mdiv;
>> >> >> + unsigned int sdiv;
>> >> >> + unsigned int kdiv;
>> >> >
>> >> > I think kdiv is signed.
>> >>
>> >> No, as these values should be the recommended values to be written in
>> >> corresponding register bits. So it should remain unsigned.
>> >>
>> >> K value should be considered as negative only while recalculating rate.
>> >>
>> >> As per exynos5250 user manual's section 7.3.2 :
>> >> " K value description "Postive value (Negative value)":
>> >> Postive values is that you should write EPLLCON/VPLLCON register.
>> >> Negative value is that you can calculate PLL output frequency with it."
>> >>
>> >> > -Doug
>> >>
>> >> Regards,
>> >> Yadwinder & Vikas.
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe
>> >> linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-06 9:01 ` Yadwinder Singh Brar
0 siblings, 0 replies; 36+ messages in thread
From: Yadwinder Singh Brar @ 2013-06-06 9:01 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 5, 2013 at 7:10 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yadwinder,
>
> On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
>> Hi Tomasz,
>>
>> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > Hi Yadwinder,
>> >
>> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
>> >> Hi Doug,
>> >>
>> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson <dianders@chromium.org>
>> >
>> > wrote:
>> >> > Vikas / Yadwinder,
>> >> >
>> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan@linaro.org>
>> >
>> > wrote:
>> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> >>
>> >> >> This patch defines a common rate_table which will contain recommended
>> >> >> p,
>> >> >> m, s, k values for supported rates that needs to be changed for
>> >> >> changing
>> >> >> corresponding PLL's rate.
>> >> >>
>> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
>> >> >> ---
>> >> >>
>> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
>> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
>> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
>> >> >> drivers/clk/samsung/clk-pll.h | 33
>> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+),
>> >> >> 15
>> >> >> deletions(-)
>> >> >
>> >> > I also reviewed this in our gerrit
>> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
>> >> > here for the list...
>> >> >
>> >> >> struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> >> >>
>> >> >> - const char *pname, const void __iomem *base)
>> >> >> + const char *pname, const void __iomem *base,
>> >> >> + const struct samsung_pll_rate_table
>> >> >> *rate_table,
>> >> >> + const unsigned int rate_count)
>> >> >
>> >> > Feels like you should document here that rate_table needs to be sorted
>> >> > and the sort order.
>> >>
>> >> sure, we will add comment to sort the table in descending order.
>> >>
>> >> >> +struct samsung_pll_rate_table {
>> >> >> + unsigned int rate;
>> >> >
>> >> > nit: extra space before "int" should be removed.
>> >>
>> >> ok
>> >>
>> >> > Also: you can include rate here if you need a convenient place to
>> >> > store it (which sadly means that this structure can't be const).
>> >> > ...but I do like Tomasz's idea of actually calculating it. You can't
>> >> > know it at compile time since the parent rate comes from the device
>> >> > tree.
>> >> >
>> >> > compatible = "samsung,clock-xxti";
>> >> > clock-frequency = <24000000>;
>> >>
>> >> Actually this table should contain the recommended values
>> >> and if we see the user manual, the input(parent) rate is also a part
>> >> of recommended
>> >> table of different output rate for a particular PLL for that SoC.
>> >
>> > From what I understood in the documentation is that there is a set of
>> > recommended P, M, S (, K) tuples for each PLL and they are not dependent
>> > on
>> > input frequency - f_in and f_out are provided in the table just for
>> > reference to see the relation between output frequency and input
>> > frequency.
>>
>> If input rate(f_in) gets changed for PLL, then the corresponding
>> output rates(f_out)
>> will change by using same(common) set of recommended P, M, S (, K),
>> and the new set of output rates(f_out) may not be the _required_ set
>> of target rates.
>> So we need different set of P, M, S (,K) values for different f_in.
>>
>> Table should contain set of P, M, S (,K) values for the _required_
>> target(f_out) rates
>> for a particular input(f_in) rate.
>
> I'm not sure what required rates you are talking about. For most PLLs the list
> of available frequencies seems to be defined by the set of supported PMS values
> and those are the frequencies that can be set by consumers, not the other way
> around.
>
> There are some cases when some particular frequency must be generated, like
> PLLs used for audio or video clock sources, but this just means that there
> must be an entry in PMS table that gives PLL configuration which produces such
> frequency with used PLL input clock.
>
Yes, I was talking about same cases. Perhaps,I think it holds true in general
for PLLs, whose set_rate can be called.
>> > I think we should ask some H/W engineer about that to make sure and choose
>> > the proper implementation, which will work properly for future cases,
>> > instead of ending with something that works just with current cases.
>>
>> We already asked hardware engineer about PMS values for PLL,
>> and we got a table containing recommended P, M ,S values for a given
>> f_in(24 MHz)
>> and required f_out rates.
>
> This doesn't answer the question about using those PMS values with different
> input rate.
>
> Kukjin, maybe you can give some information on this or point to a person who
> could?
>
>> Please let me know, why you think we are specific to current cases only ?
>
> I don't like the idea of having separate tables for each input rate. Just
> imagine how much data would have to be added if boards with several different
> input rates showed up.
>
I think by declaring all these different table as __init,
and creating a copy to be used while registering, we can
save a little memory as well as the unnecessary cost which
we have to bear while doing set_rate() and round_rate()
by using single big table(for some time critical cases).
> Sure, if each input rate requires different set of PMS values by hardware
> design, it can't be helped, but if the same PMS values can be reused (even
> giving different output rates, but this is usually not important), one table
> can be used for all cases.
>
Its good to know whether same PMS values can be reused, from hardware guys.
Using different tables we can always blindly stick to recommended values without
bothering much every time, whether existing PMS can be reused or not
while adding new cases.
>> Regards,
>> Yadwinder
>>
>> > Best regards,
>> > --
>> > Tomasz Figa
>> > Linux Kernel Developer
>> > Samsung R&D Institute Poland
>> > Samsung Electronics
>> >
>> >> So as Tomasz said input(parent) rate may change with board,
>> >> then, do those corresponding recommended p, m, s, k will be valid?
>> >>
>> >> In case, input(parent) rate changes then we may need different set of p,
>> >> m
>> >> ,s, k values recommended for new input rate to get required(recommended
>> >> to
>> >> use) output rate.
>> >>
>> >> So, we think its better that the p, m, s and k along with the parent
>> >> is known at the compile time ( or DT ?),
>> >> as these p, m, s, k values are very much coupled with the parent rate
>> >> to achieve the
>> >>
>> >> required(recommended to use) output rate.
>> >>
>> >> Also, since the sorted table is required (sorted based on "rate"),
>> >> its better to have the rate in a const rate table.
>> >>
>> >> And the whole set of recommended values should come from same place(DT
>> >> or static table),
>> >> to keep the things simple and consistent.
>> >>
>> >> Moreover, practically for a particular SoC , we use the recommended
>> >> input(parent) rate only for a PLL.
>> >> So we should keep the things simple here presently.
>> >>
>> >> >> + unsigned int pdiv;
>> >> >> + unsigned int mdiv;
>> >> >> + unsigned int sdiv;
>> >> >> + unsigned int kdiv;
>> >> >
>> >> > I think kdiv is signed.
>> >>
>> >> No, as these values should be the recommended values to be written in
>> >> corresponding register bits. So it should remain unsigned.
>> >>
>> >> K value should be considered as negative only while recalculating rate.
>> >>
>> >> As per exynos5250 user manual's section 7.3.2 :
>> >> " K value description "Postive value (Negative value)":
>> >> Postive values is that you should write EPLLCON/VPLLCON register.
>> >> Negative value is that you can calculate PLL output frequency with it."
>> >>
>> >> > -Doug
>> >>
>> >> Regards,
>> >> Yadwinder & Vikas.
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe
>> >> linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
2013-06-06 9:01 ` Yadwinder Singh Brar
@ 2013-06-08 11:58 ` Tomasz Figa
-1 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-08 11:58 UTC (permalink / raw)
To: Yadwinder Singh Brar
Cc: Tomasz Figa, Kukjin Kim, Doug Anderson, Vikas Sajjan,
linux-samsung-soc, Mike Turquette, linux-arm-kernel,
Thomas Abraham, Patch Tracking, linaro-kernel
On Thursday 06 of June 2013 14:31:56 Yadwinder Singh Brar wrote:
> On Wed, Jun 5, 2013 at 7:10 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Yadwinder,
> >
> > On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
> >> Hi Tomasz,
> >>
> >> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com>
wrote:
> >> > Hi Yadwinder,
> >> >
> >> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> >> >> Hi Doug,
> >> >>
> >> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson
> >> >> <dianders@chromium.org>
> >> >
> >> > wrote:
> >> >> > Vikas / Yadwinder,
> >> >> >
> >> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan
> >> >> > <vikas.sajjan@linaro.org>
> >> >
> >> > wrote:
> >> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> >>
> >> >> >> This patch defines a common rate_table which will contain
> >> >> >> recommended
> >> >> >> p,
> >> >> >> m, s, k values for supported rates that needs to be changed for
> >> >> >> changing
> >> >> >> corresponding PLL's rate.
> >> >> >>
> >> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> >> ---
> >> >> >>
> >> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> >> >> drivers/clk/samsung/clk-pll.h | 33
> >> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54
> >> >> >> insertions(+),
> >> >> >> 15
> >> >> >> deletions(-)
> >> >> >
> >> >> > I also reviewed this in our gerrit
> >> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll
> >> >> > summarize
> >> >> > here for the list...
> >> >> >
> >> >> >> struct clk * __init samsung_clk_register_pll35xx(const char
> >> >> >> *name,
> >> >> >>
> >> >> >> - const char *pname, const void __iomem
> >> >> >> *base)
> >> >> >> + const char *pname, const void __iomem
> >> >> >> *base,
> >> >> >> + const struct samsung_pll_rate_table
> >> >> >> *rate_table,
> >> >> >> + const unsigned int rate_count)
> >> >> >
> >> >> > Feels like you should document here that rate_table needs to be
> >> >> > sorted
> >> >> > and the sort order.
> >> >>
> >> >> sure, we will add comment to sort the table in descending order.
> >> >>
> >> >> >> +struct samsung_pll_rate_table {
> >> >> >> + unsigned int rate;
> >> >> >
> >> >> > nit: extra space before "int" should be removed.
> >> >>
> >> >> ok
> >> >>
> >> >> > Also: you can include rate here if you need a convenient place
> >> >> > to
> >> >> > store it (which sadly means that this structure can't be const).
> >> >> > ...but I do like Tomasz's idea of actually calculating it. You
> >> >> > can't
> >> >> > know it at compile time since the parent rate comes from the
> >> >> > device
> >> >> > tree.
> >> >> >
> >> >> > compatible = "samsung,clock-xxti";
> >> >> > clock-frequency = <24000000>;
> >> >>
> >> >> Actually this table should contain the recommended values
> >> >> and if we see the user manual, the input(parent) rate is also a
> >> >> part
> >> >> of recommended
> >> >> table of different output rate for a particular PLL for that SoC.
> >> >
> >> > From what I understood in the documentation is that there is a set
> >> > of
> >> > recommended P, M, S (, K) tuples for each PLL and they are not
> >> > dependent on
> >> > input frequency - f_in and f_out are provided in the table just for
> >> > reference to see the relation between output frequency and input
> >> > frequency.
> >>
> >> If input rate(f_in) gets changed for PLL, then the corresponding
> >> output rates(f_out)
> >> will change by using same(common) set of recommended P, M, S (, K),
> >> and the new set of output rates(f_out) may not be the _required_ set
> >> of target rates.
> >> So we need different set of P, M, S (,K) values for different f_in.
> >>
> >> Table should contain set of P, M, S (,K) values for the _required_
> >> target(f_out) rates
> >> for a particular input(f_in) rate.
> >
> > I'm not sure what required rates you are talking about. For most PLLs
> > the list of available frequencies seems to be defined by the set of
> > supported PMS values and those are the frequencies that can be set by
> > consumers, not the other way around.
> >
> > There are some cases when some particular frequency must be generated,
> > like PLLs used for audio or video clock sources, but this just means
> > that there must be an entry in PMS table that gives PLL configuration
> > which produces such frequency with used PLL input clock.
>
> Yes, I was talking about same cases. Perhaps,I think it holds true in
> general for PLLs, whose set_rate can be called.
Not really. For PLLs used for general purpose clocking, like for CPU or
GPU, there is usually no need to achieve certain frequency. Actually you
might want to have as many as possible intermediate steps to improve
frequency scaling efficiency.
> >> > I think we should ask some H/W engineer about that to make sure and
> >> > choose the proper implementation, which will work properly for
> >> > future cases, instead of ending with something that works just
> >> > with current cases.>>
> >> We already asked hardware engineer about PMS values for PLL,
> >> and we got a table containing recommended P, M ,S values for a given
> >> f_in(24 MHz)
> >> and required f_out rates.
> >
> > This doesn't answer the question about using those PMS values with
> > different input rate.
> >
> > Kukjin, maybe you can give some information on this or point to a
> > person who could?
> >
> >> Please let me know, why you think we are specific to current cases
> >> only ?>
> > I don't like the idea of having separate tables for each input rate.
> > Just imagine how much data would have to be added if boards with
> > several different input rates showed up.
>
> I think by declaring all these different table as __init,
> and creating a copy to be used while registering, we can
> save a little memory as well as the unnecessary cost which
> we have to bear while doing set_rate() and round_rate()
> by using single big table(for some time critical cases).
Well, this is not really about memory, but rather the amount of code in
the driver. The amount of memory needed for such setting table is rather
insignificant.
> > Sure, if each input rate requires different set of PMS values by
> > hardware design, it can't be helped, but if the same PMS values can
> > be reused (even giving different output rates, but this is usually
> > not important), one table can be used for all cases.
>
> Its good to know whether same PMS values can be reused, from hardware
> guys.
Another thing worth knowing would be whether a different frequency of
input clock is really allowed or the SoC supports only 24 MHz. If the
latter is the case, then there is no need to bother supporting other
frequencies.
I'm waiting for some opinion from hardware engineers then.
Best regards,
Tomasz
> Using different tables we can always blindly stick to recommended values
> without bothering much every time, whether existing PMS can be reused
> or not while adding new cases.
>
> >> Regards,
> >> Yadwinder
> >>
> >> > Best regards,
> >> > --
> >> > Tomasz Figa
> >> > Linux Kernel Developer
> >> > Samsung R&D Institute Poland
> >> > Samsung Electronics
> >> >
> >> >> So as Tomasz said input(parent) rate may change with board,
> >> >> then, do those corresponding recommended p, m, s, k will be valid?
> >> >>
> >> >> In case, input(parent) rate changes then we may need different set
> >> >> of p, m
> >> >> ,s, k values recommended for new input rate to get
> >> >> required(recommended
> >> >> to
> >> >> use) output rate.
> >> >>
> >> >> So, we think its better that the p, m, s and k along with the
> >> >> parent
> >> >> is known at the compile time ( or DT ?),
> >> >> as these p, m, s, k values are very much coupled with the parent
> >> >> rate
> >> >> to achieve the
> >> >>
> >> >> required(recommended to use) output rate.
> >> >>
> >> >> Also, since the sorted table is required (sorted based on "rate"),
> >> >> its better to have the rate in a const rate table.
> >> >>
> >> >> And the whole set of recommended values should come from same
> >> >> place(DT
> >> >> or static table),
> >> >> to keep the things simple and consistent.
> >> >>
> >> >> Moreover, practically for a particular SoC , we use the
> >> >> recommended
> >> >> input(parent) rate only for a PLL.
> >> >> So we should keep the things simple here presently.
> >> >>
> >> >> >> + unsigned int pdiv;
> >> >> >> + unsigned int mdiv;
> >> >> >> + unsigned int sdiv;
> >> >> >> + unsigned int kdiv;
> >> >> >
> >> >> > I think kdiv is signed.
> >> >>
> >> >> No, as these values should be the recommended values to be written
> >> >> in
> >> >> corresponding register bits. So it should remain unsigned.
> >> >>
> >> >> K value should be considered as negative only while recalculating
> >> >> rate.
> >> >>
> >> >> As per exynos5250 user manual's section 7.3.2 :
> >> >> " K value description "Postive value (Negative value)":
> >> >> Postive values is that you should write EPLLCON/VPLLCON register.
> >> >> Negative value is that you can calculate PLL output frequency with
> >> >> it."
> >> >>
> >> >> > -Doug
> >> >>
> >> >> Regards,
> >> >> Yadwinder & Vikas.
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe
> >> >> linux-samsung-soc" in the body of a message to
> >> >> majordomo@vger.kernel.org More majordomo info at
> >> >> http://vger.kernel.org/majordomo-info.html>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >> linux-samsung-soc" in the body of a message to
> >> majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx
@ 2013-06-08 11:58 ` Tomasz Figa
0 siblings, 0 replies; 36+ messages in thread
From: Tomasz Figa @ 2013-06-08 11:58 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 06 of June 2013 14:31:56 Yadwinder Singh Brar wrote:
> On Wed, Jun 5, 2013 at 7:10 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Yadwinder,
> >
> > On Tuesday 04 of June 2013 17:02:48 Yadwinder Singh Brar wrote:
> >> Hi Tomasz,
> >>
> >> On Mon, Jun 3, 2013 at 8:55 PM, Tomasz Figa <t.figa@samsung.com>
wrote:
> >> > Hi Yadwinder,
> >> >
> >> > On Thursday 30 of May 2013 12:25:40 Yadwinder Singh Brar wrote:
> >> >> Hi Doug,
> >> >>
> >> >> On Thu, May 30, 2013 at 5:14 AM, Doug Anderson
> >> >> <dianders@chromium.org>
> >> >
> >> > wrote:
> >> >> > Vikas / Yadwinder,
> >> >> >
> >> >> > On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan
> >> >> > <vikas.sajjan@linaro.org>
> >> >
> >> > wrote:
> >> >> >> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> >>
> >> >> >> This patch defines a common rate_table which will contain
> >> >> >> recommended
> >> >> >> p,
> >> >> >> m, s, k values for supported rates that needs to be changed for
> >> >> >> changing
> >> >> >> corresponding PLL's rate.
> >> >> >>
> >> >> >> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> >> >> >> ---
> >> >> >>
> >> >> >> drivers/clk/samsung/clk-exynos4.c | 8 ++++----
> >> >> >> drivers/clk/samsung/clk-exynos5250.c | 14 +++++++-------
> >> >> >> drivers/clk/samsung/clk-pll.c | 14 ++++++++++++--
> >> >> >> drivers/clk/samsung/clk-pll.h | 33
> >> >> >> +++++++++++++++++++++++++++++++-- 4 files changed, 54
> >> >> >> insertions(+),
> >> >> >> 15
> >> >> >> deletions(-)
> >> >> >
> >> >> > I also reviewed this in our gerrit
> >> >> > <https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll
> >> >> > summarize
> >> >> > here for the list...
> >> >> >
> >> >> >> struct clk * __init samsung_clk_register_pll35xx(const char
> >> >> >> *name,
> >> >> >>
> >> >> >> - const char *pname, const void __iomem
> >> >> >> *base)
> >> >> >> + const char *pname, const void __iomem
> >> >> >> *base,
> >> >> >> + const struct samsung_pll_rate_table
> >> >> >> *rate_table,
> >> >> >> + const unsigned int rate_count)
> >> >> >
> >> >> > Feels like you should document here that rate_table needs to be
> >> >> > sorted
> >> >> > and the sort order.
> >> >>
> >> >> sure, we will add comment to sort the table in descending order.
> >> >>
> >> >> >> +struct samsung_pll_rate_table {
> >> >> >> + unsigned int rate;
> >> >> >
> >> >> > nit: extra space before "int" should be removed.
> >> >>
> >> >> ok
> >> >>
> >> >> > Also: you can include rate here if you need a convenient place
> >> >> > to
> >> >> > store it (which sadly means that this structure can't be const).
> >> >> > ...but I do like Tomasz's idea of actually calculating it. You
> >> >> > can't
> >> >> > know it at compile time since the parent rate comes from the
> >> >> > device
> >> >> > tree.
> >> >> >
> >> >> > compatible = "samsung,clock-xxti";
> >> >> > clock-frequency = <24000000>;
> >> >>
> >> >> Actually this table should contain the recommended values
> >> >> and if we see the user manual, the input(parent) rate is also a
> >> >> part
> >> >> of recommended
> >> >> table of different output rate for a particular PLL for that SoC.
> >> >
> >> > From what I understood in the documentation is that there is a set
> >> > of
> >> > recommended P, M, S (, K) tuples for each PLL and they are not
> >> > dependent on
> >> > input frequency - f_in and f_out are provided in the table just for
> >> > reference to see the relation between output frequency and input
> >> > frequency.
> >>
> >> If input rate(f_in) gets changed for PLL, then the corresponding
> >> output rates(f_out)
> >> will change by using same(common) set of recommended P, M, S (, K),
> >> and the new set of output rates(f_out) may not be the _required_ set
> >> of target rates.
> >> So we need different set of P, M, S (,K) values for different f_in.
> >>
> >> Table should contain set of P, M, S (,K) values for the _required_
> >> target(f_out) rates
> >> for a particular input(f_in) rate.
> >
> > I'm not sure what required rates you are talking about. For most PLLs
> > the list of available frequencies seems to be defined by the set of
> > supported PMS values and those are the frequencies that can be set by
> > consumers, not the other way around.
> >
> > There are some cases when some particular frequency must be generated,
> > like PLLs used for audio or video clock sources, but this just means
> > that there must be an entry in PMS table that gives PLL configuration
> > which produces such frequency with used PLL input clock.
>
> Yes, I was talking about same cases. Perhaps,I think it holds true in
> general for PLLs, whose set_rate can be called.
Not really. For PLLs used for general purpose clocking, like for CPU or
GPU, there is usually no need to achieve certain frequency. Actually you
might want to have as many as possible intermediate steps to improve
frequency scaling efficiency.
> >> > I think we should ask some H/W engineer about that to make sure and
> >> > choose the proper implementation, which will work properly for
> >> > future cases, instead of ending with something that works just
> >> > with current cases.>>
> >> We already asked hardware engineer about PMS values for PLL,
> >> and we got a table containing recommended P, M ,S values for a given
> >> f_in(24 MHz)
> >> and required f_out rates.
> >
> > This doesn't answer the question about using those PMS values with
> > different input rate.
> >
> > Kukjin, maybe you can give some information on this or point to a
> > person who could?
> >
> >> Please let me know, why you think we are specific to current cases
> >> only ?>
> > I don't like the idea of having separate tables for each input rate.
> > Just imagine how much data would have to be added if boards with
> > several different input rates showed up.
>
> I think by declaring all these different table as __init,
> and creating a copy to be used while registering, we can
> save a little memory as well as the unnecessary cost which
> we have to bear while doing set_rate() and round_rate()
> by using single big table(for some time critical cases).
Well, this is not really about memory, but rather the amount of code in
the driver. The amount of memory needed for such setting table is rather
insignificant.
> > Sure, if each input rate requires different set of PMS values by
> > hardware design, it can't be helped, but if the same PMS values can
> > be reused (even giving different output rates, but this is usually
> > not important), one table can be used for all cases.
>
> Its good to know whether same PMS values can be reused, from hardware
> guys.
Another thing worth knowing would be whether a different frequency of
input clock is really allowed or the SoC supports only 24 MHz. If the
latter is the case, then there is no need to bother supporting other
frequencies.
I'm waiting for some opinion from hardware engineers then.
Best regards,
Tomasz
> Using different tables we can always blindly stick to recommended values
> without bothering much every time, whether existing PMS can be reused
> or not while adding new cases.
>
> >> Regards,
> >> Yadwinder
> >>
> >> > Best regards,
> >> > --
> >> > Tomasz Figa
> >> > Linux Kernel Developer
> >> > Samsung R&D Institute Poland
> >> > Samsung Electronics
> >> >
> >> >> So as Tomasz said input(parent) rate may change with board,
> >> >> then, do those corresponding recommended p, m, s, k will be valid?
> >> >>
> >> >> In case, input(parent) rate changes then we may need different set
> >> >> of p, m
> >> >> ,s, k values recommended for new input rate to get
> >> >> required(recommended
> >> >> to
> >> >> use) output rate.
> >> >>
> >> >> So, we think its better that the p, m, s and k along with the
> >> >> parent
> >> >> is known at the compile time ( or DT ?),
> >> >> as these p, m, s, k values are very much coupled with the parent
> >> >> rate
> >> >> to achieve the
> >> >>
> >> >> required(recommended to use) output rate.
> >> >>
> >> >> Also, since the sorted table is required (sorted based on "rate"),
> >> >> its better to have the rate in a const rate table.
> >> >>
> >> >> And the whole set of recommended values should come from same
> >> >> place(DT
> >> >> or static table),
> >> >> to keep the things simple and consistent.
> >> >>
> >> >> Moreover, practically for a particular SoC , we use the
> >> >> recommended
> >> >> input(parent) rate only for a PLL.
> >> >> So we should keep the things simple here presently.
> >> >>
> >> >> >> + unsigned int pdiv;
> >> >> >> + unsigned int mdiv;
> >> >> >> + unsigned int sdiv;
> >> >> >> + unsigned int kdiv;
> >> >> >
> >> >> > I think kdiv is signed.
> >> >>
> >> >> No, as these values should be the recommended values to be written
> >> >> in
> >> >> corresponding register bits. So it should remain unsigned.
> >> >>
> >> >> K value should be considered as negative only while recalculating
> >> >> rate.
> >> >>
> >> >> As per exynos5250 user manual's section 7.3.2 :
> >> >> " K value description "Postive value (Negative value)":
> >> >> Postive values is that you should write EPLLCON/VPLLCON register.
> >> >> Negative value is that you can calculate PLL output frequency with
> >> >> it."
> >> >>
> >> >> > -Doug
> >> >>
> >> >> Regards,
> >> >> Yadwinder & Vikas.
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe
> >> >> linux-samsung-soc" in the body of a message to
> >> >> majordomo at vger.kernel.org More majordomo info at
> >> >> http://vger.kernel.org/majordomo-info.html>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >> linux-samsung-soc" in the body of a message to
> >> majordomo at vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2013-06-08 11:58 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-29 13:37 [PATCH v2 0/5] Add generic set_rate clk_ops for PLL35xx and PLL36xx for samsung SoCs Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
2013-05-29 13:37 ` [PATCH v2 1/5] clk: samsung: Use clk->base instead of directly using clk->con0 for PLL3xxx Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
2013-05-29 17:58 ` Doug Anderson
2013-05-29 17:58 ` Doug Anderson
2013-05-29 13:37 ` [PATCH v2 2/5] clk: samsung: Add support to register rate_table " Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
2013-05-29 23:44 ` Doug Anderson
2013-05-29 23:44 ` Doug Anderson
2013-05-30 6:55 ` Yadwinder Singh Brar
2013-05-30 6:55 ` Yadwinder Singh Brar
2013-05-30 15:55 ` Doug Anderson
2013-05-30 15:55 ` Doug Anderson
2013-06-03 15:25 ` Tomasz Figa
2013-06-03 15:25 ` Tomasz Figa
2013-06-03 20:58 ` Doug Anderson
2013-06-03 20:58 ` Doug Anderson
2013-06-04 11:32 ` Yadwinder Singh Brar
2013-06-04 11:32 ` Yadwinder Singh Brar
2013-06-05 13:40 ` Tomasz Figa
2013-06-05 13:40 ` Tomasz Figa
2013-06-06 9:01 ` Yadwinder Singh Brar
2013-06-06 9:01 ` Yadwinder Singh Brar
2013-06-08 11:58 ` Tomasz Figa
2013-06-08 11:58 ` Tomasz Figa
2013-05-29 13:37 ` [PATCH v2 3/5] clk: samsung: Add set_rate() clk_ops for PLL35xx Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
2013-05-30 16:25 ` Doug Anderson
2013-05-30 16:25 ` Doug Anderson
2013-05-29 13:37 ` [PATCH v2 4/5] clk: samsung: Add set_rate() clk_ops for PLL36xx Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
2013-05-30 16:50 ` Doug Anderson
2013-05-30 16:50 ` Doug Anderson
2013-05-29 13:37 ` [PATCH v2 5/5] clk: samsung: Add EPLL and VPLL freq table for exynos5250 SoC Vikas Sajjan
2013-05-29 13:37 ` Vikas Sajjan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.