[PATCH v6 2/7] clk: samsung: Define a common samsung_clk_register_pll()
This patch defines a common samsung_clk_register_pll() and its migrating the
PLL35xx PLL36xx to use it. Other samsung PLL can also be migrated to it.
It also adds exynos5250 exynos5420 PLLs to unique id list of clocks.
Since pll2550 pll35xx and pll2650 pll36xx have exactly same clk ops
implementation, added pll2550 and pll2650 also.
Signed-off-by: Yadwinder Singh Brar yadi.b...@samsung.com
---
drivers/clk/samsung/clk-exynos4.c| 40 +++
drivers/clk/samsung/clk-exynos5250.c | 60 +++-
drivers/clk/samsung/clk-exynos5420.c | 86 +++---
drivers/clk/samsung/clk-pll.c| 132 --
drivers/clk/samsung/clk-pll.h| 11 ++-
drivers/clk/samsung/clk.h| 48
6 files changed, 242 insertions(+), 135 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos4.c
b/drivers/clk/samsung/clk-exynos4.c
index addc738..ba25a1b 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -17,7 +17,6 @@
#include linux/of_address.h
#include clk.h
-#include clk-pll.h
/* Exynos4 clock controller register offsets */
#define SRC_LEFTBUS0x4200
@@ -97,12 +96,14 @@
#define GATE_IP_PERIL 0xc950
#define E4210_GATE_IP_PERIR0xc960
#define GATE_BLOCK 0xc970
+#define E4X12_MPLL_LOCK0x10008
#define E4X12_MPLL_CON00x10108
#define SRC_DMC0x10200
#define SRC_MASK_DMC 0x10300
#define DIV_DMC0 0x10500
#define DIV_DMC1 0x10504
#define GATE_IP_DMC0x10900
+#define APLL_LOCK 0x14000
#define APLL_CON0 0x14100
#define E4210_MPLL_CON00x14108
#define SRC_CPU0x14200
@@ -121,6 +122,12 @@ enum exynos4_soc {
EXYNOS4X12,
};
+/* list of PLLs to be registered */
+enum exynos4_plls {
+ apll, mpll, epll, vpll,
+ nr_plls /* number of PLLs */
+};
+
/*
* Let each supported clock get a unique id. This id is used to lookup the
clock
* for device tree based platforms. The clocks are categorized into three
@@ -988,6 +995,17 @@ static __initdata struct of_device_id ext_clk_match[] = {
{},
};
+struct __initdata samsung_pll_clock exynos4_plls[nr_plls] = {
+ [apll] = PLL_A(pll_35xx, fout_apll, fout_apll, fin_pll, APLL_LOCK,
+ APLL_CON0, fout_apll),
+ [mpll] = PLL_A(pll_35xx, fout_mpll, fout_mpll, fin_pll,
+ E4X12_MPLL_LOCK, E4X12_MPLL_CON0, fout_mpll),
+ [epll] = PLL_A(pll_36xx, fout_epll, fout_epll, fin_pll, EPLL_LOCK,
+ EPLL_CON0, fout_epll),
+ [vpll] = PLL_A(pll_36xx, fout_vpll, fout_vpll, fin_pll, VPLL_LOCK,
+ VPLL_CON0, fout_vpll),
+};
+
/* register exynos4 clocks */
void __init exynos4_clk_init(struct device_node *np, enum exynos4_soc
exynos4_soc, void __iomem *reg_base, unsigned long xom)
{
@@ -1024,22 +1042,16 @@ void __init exynos4_clk_init(struct device_node *np,
enum exynos4_soc exynos4_so
reg_base + EPLL_CON0, pll_4600);
vpll = samsung_clk_register_pll46xx(fout_vpll, mout_vpllsrc,
reg_base + VPLL_CON0, pll_4650c);
+
+ samsung_clk_add_lookup(apll, fout_apll);
+ samsung_clk_add_lookup(mpll, fout_mpll);
+ samsung_clk_add_lookup(epll, fout_epll);
+ samsung_clk_add_lookup(vpll, fout_vpll);
} else {
- apll = samsung_clk_register_pll35xx(fout_apll, fin_pll,
- reg_base + APLL_CON0);
- mpll = samsung_clk_register_pll35xx(fout_mpll, fin_pll,
- reg_base + E4X12_MPLL_CON0);
- epll = samsung_clk_register_pll36xx(fout_epll, fin_pll,
- reg_base + EPLL_CON0);
- vpll = samsung_clk_register_pll36xx(fout_vpll, fin_pll,
- reg_base + VPLL_CON0);
+ samsung_clk_register_pll(exynos4_plls,
+ ARRAY_SIZE(exynos4_plls), reg_base);
}
- samsung_clk_add_lookup(apll, fout_apll);
- samsung_clk_add_lookup(mpll, fout_mpll);
- samsung_clk_add_lookup(epll, fout_epll);
- samsung_clk_add_lookup(vpll, fout_vpll);
-
samsung_clk_register_fixed_rate(exynos4_fixed_rate_clks,
ARRAY_SIZE(exynos4_fixed_rate_clks));
samsung_clk_register_mux(exynos4_mux_clks,
diff --git a/drivers/clk/samsung/clk-exynos5250.c
b/drivers/clk/samsung/clk-exynos5250.c
index 7c68850..dc6a700 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -17,11 +17,22 @@
#include linux/of_address.h
#include clk.h
-#include clk-pll.h
+#define APLL_LOCK 0x0
+#define APLL_CON0
Re: [PATCH v6 2/7] clk: samsung: Define a common samsung_clk_register_pll()
Hi Yadwinder,
Generally looks really good, but some comments inline.
On Monday 10 of June 2013 18:54:14 Yadwinder Singh Brar wrote:
This patch defines a common samsung_clk_register_pll() and its migrating
the PLL35xx PLL36xx to use it. Other samsung PLL can also be migrated
to it. It also adds exynos5250 exynos5420 PLLs to unique id list of
clocks. Since pll2550 pll35xx and pll2650 pll36xx have exactly same
clk ops implementation, added pll2550 and pll2650 also.
Signed-off-by: Yadwinder Singh Brar yadi.b...@samsung.com
---
drivers/clk/samsung/clk-exynos4.c| 40 +++
drivers/clk/samsung/clk-exynos5250.c | 60 +++-
drivers/clk/samsung/clk-exynos5420.c | 86 +++---
drivers/clk/samsung/clk-pll.c| 132
-- drivers/clk/samsung/clk-pll.h
| 11 ++-
drivers/clk/samsung/clk.h| 48
6 files changed, 242 insertions(+), 135 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos4.c
b/drivers/clk/samsung/clk-exynos4.c index addc738..ba25a1b 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -17,7 +17,6 @@
#include linux/of_address.h
#include clk.h
-#include clk-pll.h
/* Exynos4 clock controller register offsets */
#define SRC_LEFTBUS 0x4200
@@ -97,12 +96,14 @@
#define GATE_IP_PERIL0xc950
#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_LOCK0x14000
#define APLL_CON00x14100
#define E4210_MPLL_CON0 0x14108
#define SRC_CPU 0x14200
@@ -121,6 +122,12 @@ enum exynos4_soc {
EXYNOS4X12,
};
+/* list of PLLs to be registered */
+enum exynos4_plls {
+ apll, mpll, epll, vpll,
+ nr_plls /* number of PLLs */
+};
+
/*
* Let each supported clock get a unique id. This id is used to lookup
the clock * for device tree based platforms. The clocks are categorized
into three @@ -988,6 +995,17 @@ static __initdata struct of_device_id
ext_clk_match[] = { {},
};
+struct __initdata samsung_pll_clock exynos4_plls[nr_plls] = {
+ [apll] = PLL_A(pll_35xx, fout_apll, fout_apll, fin_pll,
APLL_LOCK,
+ APLL_CON0, fout_apll),
+ [mpll] = PLL_A(pll_35xx, fout_mpll, fout_mpll, fin_pll,
+ E4X12_MPLL_LOCK, E4X12_MPLL_CON0, fout_mpll),
+ [epll] = PLL_A(pll_36xx, fout_epll, fout_epll, fin_pll,
EPLL_LOCK,
+ EPLL_CON0, fout_epll),
+ [vpll] = PLL_A(pll_36xx, fout_vpll, fout_vpll, fin_pll,
VPLL_LOCK,
+ VPLL_CON0, fout_vpll),
+};
+
/* register exynos4 clocks */
void __init exynos4_clk_init(struct device_node *np, enum exynos4_soc
exynos4_soc, void __iomem *reg_base, unsigned long xom) {
@@ -1024,22 +1042,16 @@ void __init exynos4_clk_init(struct device_node
*np, enum exynos4_soc exynos4_so reg_base + EPLL_CON0, pll_4600);
vpll = samsung_clk_register_pll46xx(fout_vpll,
mout_vpllsrc,
reg_base + VPLL_CON0, pll_4650c);
+
+ samsung_clk_add_lookup(apll, fout_apll);
+ samsung_clk_add_lookup(mpll, fout_mpll);
+ samsung_clk_add_lookup(epll, fout_epll);
+ samsung_clk_add_lookup(vpll, fout_vpll);
} else {
- apll = samsung_clk_register_pll35xx(fout_apll,
fin_pll,
- reg_base + APLL_CON0);
- mpll = samsung_clk_register_pll35xx(fout_mpll,
fin_pll,
- reg_base + E4X12_MPLL_CON0);
- epll = samsung_clk_register_pll36xx(fout_epll,
fin_pll,
- reg_base + EPLL_CON0);
- vpll = samsung_clk_register_pll36xx(fout_vpll,
fin_pll,
- reg_base + VPLL_CON0);
+ samsung_clk_register_pll(exynos4_plls,
+ ARRAY_SIZE(exynos4_plls),
reg_base);
}
- samsung_clk_add_lookup(apll, fout_apll);
- samsung_clk_add_lookup(mpll, fout_mpll);
- samsung_clk_add_lookup(epll, fout_epll);
- samsung_clk_add_lookup(vpll, fout_vpll);
-
samsung_clk_register_fixed_rate(exynos4_fixed_rate_clks,
ARRAY_SIZE(exynos4_fixed_rate_clks));
samsung_clk_register_mux(exynos4_mux_clks,
diff --git a/drivers/clk/samsung/clk-exynos5250.c
b/drivers/clk/samsung/clk-exynos5250.c index 7c68850..dc6a700 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -17,11 +17,22 @@
#include linux/of_address.h
#include
Re: [PATCH v6 2/7] clk: samsung: Define a common samsung_clk_register_pll()
On Wed, Jun 19, 2013 at 10:24 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Yadwinder,
Generally looks really good, but some comments inline.
On Monday 10 of June 2013 18:54:14 Yadwinder Singh Brar wrote:
This patch defines a common samsung_clk_register_pll() and its migrating
the PLL35xx PLL36xx to use it. Other samsung PLL can also be migrated
to it. It also adds exynos5250 exynos5420 PLLs to unique id list of
clocks. Since pll2550 pll35xx and pll2650 pll36xx have exactly same
clk ops implementation, added pll2550 and pll2650 also.
+void __init samsung_clk_register_pll(struct samsung_pll_clock
*clk_list, + unsigned int nr_pll, void __iomem
*base)
+{
+ struct samsung_clk_pll *pll;
+ struct clk *clk;
+ struct clk_init_data init;
+ struct samsung_pll_clock *list = clk_list;
+ int cnt;
+
+ for (cnt = 0; cnt nr_pll; cnt++, list++) {
I'd suggest moving contents of this loop to a function like following?
static int __init _samsung_clk_register_pll(struct samsung_pll_clock *pll,
void __iomem *base)
This will make the code less indented and so more readable.
Yes, will do it.
+ pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+ if (!pll) {
+ pr_err(%s: could not allocate pll clk %s\n,
+ __func__, list-name);
+ continue;
+ }
+
+ init.name = list-name;
+ init.flags = list-flags;
+ init.parent_names = list-parent_name;
+ init.num_parents = 1;
+
+ switch (list-type) {
+ /* clk_ops for 35xx and 2550 are similar */
+ case pll_35xx:
+ case pll_2550:
+ init.ops = samsung_pll35xx_clk_ops;
+ break;
+ /* clk_ops for 36xx and 2650 are similar */
+ case pll_36xx:
+ case pll_2650:
+ init.ops = samsung_pll36xx_clk_ops;
+ break;
+ default:
+ pr_warn(%s: Unknown pll type for pll clk %s\n,
+ __func__, list-name);
+ }
+
+ pll-hw.init = init;
+ pll-type = list-type;
+ pll-lock_reg = base + list-lock_offset;
+ pll-con_reg = base + list-con_offset;
+
+ clk = clk_register(NULL, pll-hw);
+ if (IS_ERR(clk)) {
+ pr_err(%s: failed to register pll clock %s\n,
+ __func__, list-name);
+ kfree(pll);
+ continue;
+ }
+
+ samsung_clk_add_lookup(clk, list-id);
+
+ if (list-alias)
+ if (clk_register_clkdev(clk, list-alias,
+ list-dev_name))
What about
if (!list-alias)
return;
ret = clk_register_clkdev(clk, list-alias, list-
dev_name);
if (ret)
pr_err(%s: failed to register lookup for %s,
__func__, list-name);
its ok, but to me it looks more clear and precise inside if(){ },
as its length and indentation is not much deep.
If you really insist we can do it ?
+ pr_err(%s: failed to register lookup for
%s,
+ __func__, list-name);
+ }
+}
+struct samsung_pll_clock {
+ unsigned intid;
+ const char *dev_name;
+ const char *name;
+ const char *parent_name;
+ unsigned long flags;
+ const int con_offset;
+ const int lock_offset;
I don't see any point of having all those const qualifiers of non-
pointers. This makes the struct useless for creating and initializing at
runtime.
OK, will remove it.
+ enumsamsung_pll_type type;
IMHO the enum keyword shouldn't be separated from enum name like this.
Any specific reason? Just to keep indentation same for all members, I
used tabs :).
Otherwise the patch looks fine. Maybe it's a bit too big - things could be
done a bit more gradually, like:
1) first add required structs and functions,
2) then move existing clock drivers to use the new API, possibly one patch
per driver,
3) remove the old API.
This would make the whole change easier to review and, in case of any
regressions, easier to track down the problem.
OK, I will split it.
Thanks,
Yadwinder
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/7] clk: samsung: Define a common samsung_clk_register_pll()
On Wednesday 19 of June 2013 23:44:47 Yadwinder Singh Brar wrote:
On Wed, Jun 19, 2013 at 10:24 PM, Tomasz Figa tomasz.f...@gmail.com
wrote:
Hi Yadwinder,
Generally looks really good, but some comments inline.
On Monday 10 of June 2013 18:54:14 Yadwinder Singh Brar wrote:
This patch defines a common samsung_clk_register_pll() and its
migrating the PLL35xx PLL36xx to use it. Other samsung PLL can
also be migrated to it. It also adds exynos5250 exynos5420 PLLs to
unique id list of clocks. Since pll2550 pll35xx and pll2650
pll36xx have exactly same clk ops implementation, added pll2550 and
pll2650 also.
+void __init samsung_clk_register_pll(struct samsung_pll_clock
*clk_list, + unsigned int nr_pll, void
__iomem
*base)
+{
+ struct samsung_clk_pll *pll;
+ struct clk *clk;
+ struct clk_init_data init;
+ struct samsung_pll_clock *list = clk_list;
+ int cnt;
+
+ for (cnt = 0; cnt nr_pll; cnt++, list++) {
I'd suggest moving contents of this loop to a function like following?
static int __init _samsung_clk_register_pll(struct samsung_pll_clock
*pll,
void __iomem *base)
This will make the code less indented and so more readable.
Yes, will do it.
+ pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+ if (!pll) {
+ pr_err(%s: could not allocate pll clk %s\n,
+ __func__, list-name);
+ continue;
+ }
+
+ init.name = list-name;
+ init.flags = list-flags;
+ init.parent_names = list-parent_name;
+ init.num_parents = 1;
+
+ switch (list-type) {
+ /* clk_ops for 35xx and 2550 are similar */
+ case pll_35xx:
+ case pll_2550:
+ init.ops = samsung_pll35xx_clk_ops;
+ break;
+ /* clk_ops for 36xx and 2650 are similar */
+ case pll_36xx:
+ case pll_2650:
+ init.ops = samsung_pll36xx_clk_ops;
+ break;
+ default:
+ pr_warn(%s: Unknown pll type for pll clk
%s\n,
+ __func__, list-name);
+ }
+
+ pll-hw.init = init;
+ pll-type = list-type;
+ pll-lock_reg = base + list-lock_offset;
+ pll-con_reg = base + list-con_offset;
+
+ clk = clk_register(NULL, pll-hw);
+ if (IS_ERR(clk)) {
+ pr_err(%s: failed to register pll clock %s\n,
+ __func__, list-name);
+ kfree(pll);
+ continue;
+ }
+
+ samsung_clk_add_lookup(clk, list-id);
+
+ if (list-alias)
+ if (clk_register_clkdev(clk, list-alias,
+ list-dev_name))
What about
if (!list-alias)
return;
ret = clk_register_clkdev(clk, list-alias, list-
dev_name);
if (ret)
pr_err(%s: failed to register lookup for %s,
__func__, list-name);
its ok, but to me it looks more clear and precise inside if(){ },
as its length and indentation is not much deep.
If you really insist we can do it ?
Well, you can read the code as the CPU does - if you are not interested in
the alias you can see instantly that you don't have to read this function
any more, because it returns if alias is NULL.
And it's always good to have lower indentation level. :)
+ pr_err(%s: failed to register lookup
for
%s,
+ __func__, list-name);
+ }
+}
+struct samsung_pll_clock {
+ unsigned intid;
+ const char *dev_name;
+ const char *name;
+ const char *parent_name;
+ unsigned long flags;
+ const int con_offset;
+ const int lock_offset;
I don't see any point of having all those const qualifiers of non-
pointers. This makes the struct useless for creating and initializing
at runtime.
OK, will remove it.
+ enumsamsung_pll_type type;
IMHO the enum keyword shouldn't be separated from enum name like this.
Any specific reason? Just to keep indentation same for all members, I
used tabs :).
Well, enum samsung_pll_type is the type name, not enum and type is
name of the member, not samsung_pll_type type.
Just imagine unsigned long x separated to
