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