Re: [PATCH v3 2/7] clk: samsung: add infrastructure to register cpu clocks
Hi Thomas,
On 07.02.2014 16:55, Thomas Abraham wrote:
From: Thomas Abraham thomas...@samsung.com
The CPU clock provider supplies the clock to the CPU clock domain. The
composition and organization of the CPU clock provider could vary among
Exynos SoCs. A CPU clock provider can be composed of clock mux, dividers
and gates. This patch defines a new clock type for CPU clock provider and
adds infrastructure to register the CPU clock providers for Samsung
platforms.
In addition to this, the arm cpu clock provider for Exynos4210 and
compatible SoCs is instantiated using the new cpu clock type. The clock
configuration data for this clock is obtained from device tree. This
implementation is reusable for Exynos4x12 and Exynos5250 SoCs as well.
Signed-off-by: Thomas Abraham thomas...@samsung.com
---
drivers/clk/samsung/Makefile |2 +-
drivers/clk/samsung/clk-cpu.c | 409 +
drivers/clk/samsung/clk.h |5 +
3 files changed, 415 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
[snip]
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..673f620
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,409 @@
[snip]
+#define SRC_CPU0x0
+#define STAT_CPU 0x200
+#define DIV_CPU0 0x300
+#define DIV_CPU1 0x304
+#define DIV_STAT_CPU0 0x400
+#define DIV_STAT_CPU1 0x404
+
+#define MAX_DIV8
+
+#define EXYNOS4210_ARM_DIV1(base) ((readl(base + DIV_CPU0) 0xf) + 1)
+#define EXYNOS4210_ARM_DIV2(base) (((readl(base + DIV_CPU0) 28) 0xf) + 1)
Those are 3-bit dividers, so the mask should be rather 0x7, shouldn't it?
Btw. Somehow I feel like this kind of macros is simply obfuscating code
without any real benefits. Could you rewrite them to simply take the
value of DIV_CPU0 register, without hiding readl behind the scenes?
+
+#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0)\
+ ((d5 24) | (d4 20) | (d3 16) | (d2 12) |\
+(d1 8) | (d0 4))
+#define EXYNOS4210_DIV_CPU1(d2, d1, d0)
\
+ ((d2 8) | (d1 4) | (d0 0))
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 0) | (0x7 4))
+#define EXYNOS4210_MUX_HPM_MASK(1 20)
[snip]
+/* common round rate callback useable for all types of cpu clocks */
+static long samsung_cpuclk_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
+{
+ struct clk *parent = __clk_get_parent(hw-clk);
+ unsigned long max_prate = __clk_round_rate(parent, UINT_MAX);
+ unsigned long t_prate, best_div = 1;
+ unsigned long delta, min_delta = UINT_MAX;
+
+ do {
+ t_prate = __clk_round_rate(parent, drate * best_div);
+ delta = drate - (t_prate / best_div);
+ if (delta min_delta) {
+ *prate = t_prate;
+ min_delta = delta;
+ }
+ if (!delta)
+ break;
+ best_div++;
+ } while ((drate * best_div) max_prate best_div = MAX_DIV);
+
+ return t_prate / best_div;
+}
I think there is something wrong with the code above. You increment
best_div in every iteration of the loop and use it to calculate the
final best frequency. Shouldn't best_div be the divisor which was found
to give the least delta and so the loop should rather iterate on a
different variable and store best_div only if (delta min_delta)
condition is true?
Anyway, I wonder if you couldn't somehow reuse the code from
drivers/clk/clk-divider.c...
+
+static unsigned long _calc_div(unsigned long prate, unsigned long drate)
+{
+ unsigned long div = prate / drate;
+
+ WARN_ON(div = MAX_DIV);
+ return (!(prate % drate)) ? div-- : div;
+}
+
+/* helper function to register a cpu clock */
+static int __init samsung_cpuclk_register(unsigned int lookup_id,
Isn't the name a bit too generic? I'd say it should be
exynos_cpuclk_register(), since the implementation is rather Exynos
specific anyway.
+ const char *name, const char **parents,
+ unsigned int num_parents, void __iomem *base,
+ const struct samsung_cpuclk_soc_data *soc_data,
+ struct device_node *np, const struct clk_ops *ops)
+{
+ struct samsung_cpuclk *cpuclk;
+ struct clk_init_data init;
+ struct clk *clk;
+ int ret;
+
+ cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
+ if (!cpuclk) {
+ pr_err(%s: could not allocate memory for %s clock\n,
+ __func__, name);
+ return -ENOMEM;
+ }
+
+ init.name = name;
+ init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_PARENT;
Why
Re: [PATCH v3 2/7] clk: samsung: add infrastructure to register cpu clocks
Hi Tomasz,
Thanks for your detailed review.
On Wed, Feb 12, 2014 at 11:55 PM, Tomasz Figa t.f...@samsung.com wrote:
Hi Thomas,
On 07.02.2014 16:55, Thomas Abraham wrote:
From: Thomas Abraham thomas...@samsung.com
The CPU clock provider supplies the clock to the CPU clock domain. The
composition and organization of the CPU clock provider could vary among
Exynos SoCs. A CPU clock provider can be composed of clock mux, dividers
and gates. This patch defines a new clock type for CPU clock provider and
adds infrastructure to register the CPU clock providers for Samsung
platforms.
In addition to this, the arm cpu clock provider for Exynos4210 and
compatible SoCs is instantiated using the new cpu clock type. The clock
configuration data for this clock is obtained from device tree. This
implementation is reusable for Exynos4x12 and Exynos5250 SoCs as well.
Signed-off-by: Thomas Abraham thomas...@samsung.com
---
drivers/clk/samsung/Makefile |2 +-
drivers/clk/samsung/clk-cpu.c | 409
+
drivers/clk/samsung/clk.h |5 +
3 files changed, 415 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
[snip]
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..673f620
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,409 @@
[snip]
+#define SRC_CPU0x0
+#define STAT_CPU 0x200
+#define DIV_CPU0 0x300
+#define DIV_CPU1 0x304
+#define DIV_STAT_CPU0 0x400
+#define DIV_STAT_CPU1 0x404
+
+#define MAX_DIV8
+
+#define EXYNOS4210_ARM_DIV1(base) ((readl(base + DIV_CPU0) 0xf) + 1)
+#define EXYNOS4210_ARM_DIV2(base) (((readl(base + DIV_CPU0) 28) 0xf)
+ 1)
Those are 3-bit dividers, so the mask should be rather 0x7, shouldn't it?
Yes, that is correct. I will fix this.
Btw. Somehow I feel like this kind of macros is simply obfuscating code
without any real benefits. Could you rewrite them to simply take the value
of DIV_CPU0 register, without hiding readl behind the scenes?
Okay. I will change this.
+
+#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0)\
+ ((d5 24) | (d4 20) | (d3 16) | (d2 12) |\
+(d1 8) | (d0 4))
+#define EXYNOS4210_DIV_CPU1(d2, d1, d0)
\
+ ((d2 8) | (d1 4) | (d0 0))
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 0) | (0x7 4))
+#define EXYNOS4210_MUX_HPM_MASK(1 20)
[snip]
+/* common round rate callback useable for all types of cpu clocks */
+static long samsung_cpuclk_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
+{
+ struct clk *parent = __clk_get_parent(hw-clk);
+ unsigned long max_prate = __clk_round_rate(parent, UINT_MAX);
+ unsigned long t_prate, best_div = 1;
+ unsigned long delta, min_delta = UINT_MAX;
+
+ do {
+ t_prate = __clk_round_rate(parent, drate * best_div);
+ delta = drate - (t_prate / best_div);
+ if (delta min_delta) {
+ *prate = t_prate;
+ min_delta = delta;
+ }
+ if (!delta)
+ break;
+ best_div++;
+ } while ((drate * best_div) max_prate best_div = MAX_DIV);
+
+ return t_prate / best_div;
+}
I think there is something wrong with the code above. You increment best_div
in every iteration of the loop and use it to calculate the final best
frequency. Shouldn't best_div be the divisor which was found to give the
least delta and so the loop should rather iterate on a different variable
and store best_div only if (delta min_delta) condition is true?
This function finds out the best parent frequency (APLL in this case)
which when divided with some divider value provides the the closest
possible drate. Probably, the name best_div is misleading since there
is no need to know the value of best_div outside this function.
Anyway, I wonder if you couldn't somehow reuse the code from
drivers/clk/clk-divider.c...
Yes, there are some similarities between these but they are not
entirely the same.
+
+static unsigned long _calc_div(unsigned long prate, unsigned long drate)
+{
+ unsigned long div = prate / drate;
+
+ WARN_ON(div = MAX_DIV);
+ return (!(prate % drate)) ? div-- : div;
+}
+
+/* helper function to register a cpu clock */
+static int __init samsung_cpuclk_register(unsigned int lookup_id,
Isn't the name a bit too generic? I'd say it should be
exynos_cpuclk_register(), since the implementation is rather Exynos specific
anyway.
The implementation of the cpu clock type is supposed to be usable on
all Samsung SoCs starting from s3c24xx onwards. I should have
Re: [PATCH v3 2/7] clk: samsung: add infrastructure to register cpu clocks
Hi Thomas,
From: Thomas Abraham thomas...@samsung.com
The CPU clock provider supplies the clock to the CPU clock domain. The
composition and organization of the CPU clock provider could vary
among Exynos SoCs. A CPU clock provider can be composed of clock mux,
dividers and gates. This patch defines a new clock type for CPU clock
provider and adds infrastructure to register the CPU clock providers
for Samsung platforms.
In addition to this, the arm cpu clock provider for Exynos4210 and
compatible SoCs is instantiated using the new cpu clock type. The
clock configuration data for this clock is obtained from device tree.
I'm a bit confused with the sentence:
This implementation is reusable for Exynos4x12 and Exynos5250 SoCs as
well.
It seems that the implementation for Exynos4210 is already reused in
this code for Exynos4412.
Is the name convention of exynos4210_* after the first SoC supporting
such setup? If yes, I think that it could be explicitly written in the
commit message.
Additionally, the above commit message could also emphasis, that the
implementation for other Exynos4 SoCs (like Exynos4412) is already
in place.
Signed-off-by: Thomas Abraham thomas...@samsung.com
---
drivers/clk/samsung/Makefile |2 +-
drivers/clk/samsung/clk-cpu.c | 409
+
drivers/clk/samsung/clk.h |5 + 3 files changed, 415
insertions(+), 1 deletion(-) create mode 100644
drivers/clk/samsung/clk-cpu.c
diff --git a/drivers/clk/samsung/Makefile
b/drivers/clk/samsung/Makefile index 8eb4799..e2b453f 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -2,7 +2,7 @@
# Samsung Clock specific Makefile
#
-obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
+obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o clk-cpu.o
obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
diff --git a/drivers/clk/samsung/clk-cpu.c
b/drivers/clk/samsung/clk-cpu.c new file mode 100644
index 000..673f620
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,409 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Thomas Abraham thomas...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or
modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This file contains the utility functions to register the cpu
clocks
+ * for samsung platforms.
+*/
+
+#include linux/errno.h
+#include clk.h
+
+#define SRC_CPU 0x0
+#define STAT_CPU 0x200
+#define DIV_CPU0 0x300
+#define DIV_CPU1 0x304
+#define DIV_STAT_CPU00x400
+#define DIV_STAT_CPU10x404
+
+#define MAX_DIV 8
+
+#define EXYNOS4210_ARM_DIV1(base) ((readl(base + DIV_CPU0) 0xf) +
1) +#define EXYNOS4210_ARM_DIV2(base) (((readl(base + DIV_CPU0)
28) 0xf) + 1) +
+#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1,
d0) \
+ ((d5 24) | (d4 20) | (d3 16) | (d2 12)
| \
+ (d1 8) | (d0 4))
+#define EXYNOS4210_DIV_CPU1(d2, d1,
d0) \
+ ((d2 8) | (d1 4) | (d0 0))
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 0) | (0x7 4))
+#define EXYNOS4210_MUX_HPM_MASK (1 20)
+
+/**
+ * struct exynos4210_armclk_data: config data to setup exynos4210
cpu clocks.
+ * @prate: frequency of the parent clock.
+ * @div0:value to be programmed in the div_cpu0 register.
+ * @div1:value to be programmed in the div_cpu1 register.
+ *
+ * This structure holds the divider configuration data for divider
clocks
+ * belonging to the CMU_CPU clock domain. The parent frequency at
which these
+ * divider values are vaild is specified in @prate.
+ */
+struct exynos4210_armclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct samsung_cpuclk: information about clock supplied to a CPU
core.
+ * @hw: handle between ccf and cpu clock.
+ * @alt_parent: alternate parent clock to use when switching
the speed
+ * of the primary parent clock.
+ * @ctrl_base: base address of the clock controller.
+ * @offset: offset from the ctrl_base address where the cpu
clock div/mux
+ * registers can be accessed.
+ * @clk_nb: clock notifier registered for changes in clock
speed of the
+ * primary parent clock.
+ * @data:optional data which the acutal instantiation of
this clock
+ * can use.
+ */
+struct samsung_cpuclk {
+ struct clk_hw hw;
+ struct clk *alt_parent;
+ void __iomem*ctrl_base;
+ unsigned long offset;
+ struct
