Re: [PATCH v7 1/6] clk: samsung: add infrastructure to register cpu clocks
On 29.07.2014 07:35, Thomas Abraham wrote:
Hi Tomasz,
Thanks for your review comments. I have made most of the changes you
have suggested. The suggested modifications which I did not include is
marked below.
On Sat, Jul 19, 2014 at 6:25 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
[snip]
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..0d62968
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,576 @@
+/*
+ * 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.
I'd expect few words here or in separate comment on how the code works,
i.e. assumptions made, most important procedures, etc. This is a complex
piece of code for quite complex hardware, so proper documentation is
essential.
+*/
+
+#include linux/errno.h
+#include clk.h
+
+#define E4210_SRC_CPU0x0
+#define E4210_STAT_CPU 0x200
+#define E4210_DIV_CPU0 0x300
+#define E4210_DIV_CPU1 0x304
+#define E4210_DIV_STAT_CPU0 0x400
+#define E4210_DIV_STAT_CPU1 0x404
+
+#define MAX_DIV 8
+#define DIV_MASK 7
+#define DIV_MASK_ALL 0x
+#define MUX_MASK 7
+
+#define E4210_DIV0_RATIO0_MASK 0x7
+#define E4210_DIV1_HPM_MASK ((0x7 4) | (0x7 0))
This mask contains two fields, doesn't it? I'd say it would be better
for readability if you split it.
+#define E4210_MUX_HPM_MASK (1 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK (DIV_MASK E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0) \
+ (((apll) 24) | ((pclk_dbg) 20) | ((atb) 16) | \
+ ((periph) 12) | ((corem1) 8) | ((corem0) 4))
+#define E4210_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | ((copy) 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)
\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (periph 12) | (acp 8) | (cpud 4)))
+#define E5250_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud)\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (cpud 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)
\
+ (((kpll 24) | (pclk 20) | (aclk 4)))
Again, used macro arguments should always be surrounded with parentheses.
+
+enum cpuclk_type {
+ EXYNOS4210,
+ EXYNOS5250,
+ EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.
It seems like this could be used for all Exynos SoCs, so probably should
be called exynos_cpuclk_data.
+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the
CPU
+ * clock domain. The parent frequency at which these divider values are
valid is
+ * specified in @prate. The @prate is the frequency of the primary parent
clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct exynos_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.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock
+ * can use.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ * of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
+ * of the primary parent clock.
+ *
+ * This structure holds information required for programming the cpu clock
for
Re: [PATCH v7 1/6] clk: samsung: add infrastructure to register cpu clocks
Hi Tomasz,
Thanks for your review comments. I have made most of the changes you
have suggested. The suggested modifications which I did not include is
marked below.
On Sat, Jul 19, 2014 at 6:25 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
[snip]
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..0d62968
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,576 @@
+/*
+ * 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.
I'd expect few words here or in separate comment on how the code works,
i.e. assumptions made, most important procedures, etc. This is a complex
piece of code for quite complex hardware, so proper documentation is
essential.
+*/
+
+#include linux/errno.h
+#include clk.h
+
+#define E4210_SRC_CPU0x0
+#define E4210_STAT_CPU 0x200
+#define E4210_DIV_CPU0 0x300
+#define E4210_DIV_CPU1 0x304
+#define E4210_DIV_STAT_CPU0 0x400
+#define E4210_DIV_STAT_CPU1 0x404
+
+#define MAX_DIV 8
+#define DIV_MASK 7
+#define DIV_MASK_ALL 0x
+#define MUX_MASK 7
+
+#define E4210_DIV0_RATIO0_MASK 0x7
+#define E4210_DIV1_HPM_MASK ((0x7 4) | (0x7 0))
This mask contains two fields, doesn't it? I'd say it would be better
for readability if you split it.
+#define E4210_MUX_HPM_MASK (1 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK (DIV_MASK E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0) \
+ (((apll) 24) | ((pclk_dbg) 20) | ((atb) 16) | \
+ ((periph) 12) | ((corem1) 8) | ((corem0) 4))
+#define E4210_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | ((copy) 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)
\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (periph 12) | (acp 8) | (cpud 4)))
+#define E5250_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud)\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (cpud 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)
\
+ (((kpll 24) | (pclk 20) | (aclk 4)))
Again, used macro arguments should always be surrounded with parentheses.
+
+enum cpuclk_type {
+ EXYNOS4210,
+ EXYNOS5250,
+ EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.
It seems like this could be used for all Exynos SoCs, so probably should
be called exynos_cpuclk_data.
+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the
CPU
+ * clock domain. The parent frequency at which these divider values are
valid is
+ * specified in @prate. The @prate is the frequency of the primary parent
clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct exynos_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.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock
+ * can use.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ * of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
+ * of the primary parent clock.
+ *
+ * This structure holds information required for programming the cpu clock
for
+ * various clock speeds.
nit: s/cpu/CPU/
+
Re: [PATCH v7 1/6] clk: samsung: add infrastructure to register cpu clocks
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
[snip]
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..0d62968
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,576 @@
+/*
+ * 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.
I'd expect few words here or in separate comment on how the code works,
i.e. assumptions made, most important procedures, etc. This is a complex
piece of code for quite complex hardware, so proper documentation is
essential.
+*/
+
+#include linux/errno.h
+#include clk.h
+
+#define E4210_SRC_CPU0x0
+#define E4210_STAT_CPU 0x200
+#define E4210_DIV_CPU0 0x300
+#define E4210_DIV_CPU1 0x304
+#define E4210_DIV_STAT_CPU0 0x400
+#define E4210_DIV_STAT_CPU1 0x404
+
+#define MAX_DIV 8
+#define DIV_MASK 7
+#define DIV_MASK_ALL 0x
+#define MUX_MASK 7
+
+#define E4210_DIV0_RATIO0_MASK 0x7
+#define E4210_DIV1_HPM_MASK ((0x7 4) | (0x7 0))
This mask contains two fields, doesn't it? I'd say it would be better
for readability if you split it.
+#define E4210_MUX_HPM_MASK (1 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK (DIV_MASK E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0) \
+ (((apll) 24) | ((pclk_dbg) 20) | ((atb) 16) | \
+ ((periph) 12) | ((corem1) 8) | ((corem0) 4))
+#define E4210_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | ((copy) 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)
\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (periph 12) | (acp 8) | (cpud 4)))
+#define E5250_CPU_DIV1(hpm, copy)\
+ (((hpm) 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud)\
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+ (cpud 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)
\
+ (((kpll 24) | (pclk 20) | (aclk 4)))
Again, used macro arguments should always be surrounded with parentheses.
+
+enum cpuclk_type {
+ EXYNOS4210,
+ EXYNOS5250,
+ EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.
It seems like this could be used for all Exynos SoCs, so probably should
be called exynos_cpuclk_data.
+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the
CPU
+ * clock domain. The parent frequency at which these divider values are
valid is
+ * specified in @prate. The @prate is the frequency of the primary parent
clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct exynos_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.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock
+ * can use.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ * of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
+ * of the primary parent clock.
+ *
+ * This structure holds information required for programming the cpu clock
for
+ * various clock speeds.
nit: s/cpu/CPU/
+ */
+struct exynos_cpuclk {
+ struct clk_hw hw;
+ struct clk *alt_parent;
+ void __iomem*ctrl_base;
+ unsigned long offset;
+ spinlock_t *lock;
+ enum cpuclk_type
[PATCH v7 1/6] clk: samsung: add infrastructure to register cpu clocks
From: Thomas Abraham thomas...@samsung.com
This patch defines a new clock type for CPU clock provider and adds
infrastructure to register the CPU clock providers for Samsung platforms.
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 and so this new clock type provides a way to encapsulate these
blocks into CPU clock type.
Cc: Tomasz Figa t.f...@samsung.com
Signed-off-by: Thomas Abraham thomas...@samsung.com
Reviewed-by: Amit Daniel Kachhap amit.dan...@samsung.com
Tested-by: Arjun K.V arjun...@samsung.com
---
drivers/clk/samsung/Makefile |2 +-
drivers/clk/samsung/clk-cpu.c | 576 +
drivers/clk/samsung/clk.h |5 +
3 files changed, 582 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 69e8177..f4edd31 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_SOC_EXYNOS3250) += clk-exynos3250.o
obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..0d62968
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,576 @@
+/*
+ * 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 E4210_SRC_CPU 0x0
+#define E4210_STAT_CPU 0x200
+#define E4210_DIV_CPU0 0x300
+#define E4210_DIV_CPU1 0x304
+#define E4210_DIV_STAT_CPU00x400
+#define E4210_DIV_STAT_CPU10x404
+
+#define MAX_DIV8
+#define DIV_MASK 7
+#define DIV_MASK_ALL 0x
+#define MUX_MASK 7
+
+#define E4210_DIV0_RATIO0_MASK 0x7
+#define E4210_DIV1_HPM_MASK((0x7 4) | (0x7 0))
+#define E4210_MUX_HPM_MASK (1 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK(DIV_MASK E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0)\
+ (((apll) 24) | ((pclk_dbg) 20) | ((atb) 16) | \
+ ((periph) 12) | ((corem1) 8) | ((corem0) 4))
+#define E4210_CPU_DIV1(hpm, copy) \
+ (((hpm) 4) | ((copy) 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud) \
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+(periph 12) | (acp 8) | (cpud 4)))
+#define E5250_CPU_DIV1(hpm, copy) \
+ (((hpm) 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud) \
+ (((apll 24) | (pclk_dbg 20) | (atb 16) | \
+(cpud 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)
\
+ (((kpll 24) | (pclk 20) | (aclk 4)))
+
+enum cpuclk_type {
+ EXYNOS4210,
+ EXYNOS5250,
+ EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.
+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the CPU
+ * clock domain. The parent frequency at which these divider values are valid
is
+ * specified in @prate. The @prate is the frequency of the primary parent
clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct exynos_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.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock
+ * can use.
+ * @clk_nb: clock
