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
