ASV support for Exynos4 series
Subsystems (3)
cover ASV support for Exynos4 series
[PATCH 1/3] ARM: EXYNOS: Add ASV feature for Exynos4 series [PATCH 2/3] ARM: EXYNOS: Add clock for Exynos4210 asv feature [PATCH 3/3] ARM: EXYNOS: Support ASV for Exynos4210
PATCH 1/3 ARM: EXYNOS: Add ASV feature for Exynos4 series
This patch adds ASV feature for exynos4 seires.
Asv(Adoptive support voltage) feature support to get spec of SoC. And we can use to adjust voltage for operation SoC usign by ASV result.
--- arch/arm/mach-exynos4/Makefile | 2 +- arch/arm/mach-exynos4/asv.c | 78 ++++++++++++++++++++++++++++++ arch/arm/mach-exynos4/include/mach/asv.h | 42 ++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletions(-) create mode 100644 arch/arm/mach-exynos4/asv.c create mode 100644 arch/arm/mach-exynos4/include/mach/asv.h
arch/arm/mach-exynos4/Makefile +1 -1
@@ -12,7 +12,7 @@ obj- := # Core support for EXYNOS4 system -obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o +obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o obj-$(CONFIG_ARCH_EXYNOS4) += setup-i2c0.o irq-eint.o dma.o pmu.o obj-$(CONFIG_CPU_EXYNOS4210) += clock-exynos4210.o obj-$(CONFIG_SOC_EXYNOS4212) += clock-exynos4212.o
arch/arm/mach-exynos4/asv.c new file +78 -0
78 lines not on this page · raw message
arch/arm/mach-exynos4/include/mach/asv.h new file +42 -0
42 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH 1/3 ARM: EXYNOS: Add ASV feature for Exynos4 series
Hi,
Quoted 1 line · Jongpill Lee
This patch adds ASV feature for exynos4 seires.
s/seires/series
Quoted 1 line
Asv(Adoptive support voltage) feature support to get spec of SoC.
So is this "Adoptive support voltage" or Adaptive Supply Voltage as commented further in the patch ?
Quoted 1 line
And we can use to adjust voltage for operation SoC usign by ASV result.
s/usign/using
In general I find this commit description hard to understand. Could you please provide a bit more detailed description, perhaps in the cover letter, of what this patch series does and why ?
Quoted 48 lines
Signed-off-by Jongpill Lee<[email protected]>--- arch/arm/mach-exynos4/Makefile | 2 +- arch/arm/mach-exynos4/asv.c | 78 ++++++++++++++++++++++++++++++ arch/arm/mach-exynos4/include/mach/asv.h | 42 ++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletions(-)create mode 100644
arch/arm/mach-exynos4/asv.ccreate mode 100644arch/arm/mach-exynos4/include/mach/asv.harch/arm/mach-exynos4/Makefile +1
-1@@ -12,7 +12,7 @@ obj- := # Core support for EXYNOS4 system -obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o +obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o obj-$(CONFIG_ARCH_EXYNOS4) += setup-i2c0.o irq-eint.o dma.o pmu.o obj-$(CONFIG_CPU_EXYNOS4210) += clock-exynos4210.o obj-$(CONFIG_SOC_EXYNOS4212) += clock-exynos4212.oarch/arm/mach-exynos4/asv.c new file +19
-0@@ -0,0 +1,78 @@ +/* linux/arch/arm/mach-exynos4/asv.c +* +* Copyright (c) 2011 Samsung Electronics Co., Ltd. +* http://www.samsung.com/ +* +* EXYNOS4 - ASV(Adaptive Supply Voltage) driver +* +* 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. +*/ + +#include <linux/init.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/slab.h> +
arch/arm/mach-exynos4/asv.c line 29
Quoted 10 lines
arch/arm/mach-exynos4/asv.c new file +10
-0+#include <mach/map.h> +#include <mach/asv.h> + +static struct samsung_asv *exynos_asv; + +static int __init exynos_asv_init (void ) +{ + exynos_asv =kzalloc (sizeof (struct samsung_asv ),GFP_KERNEL ); +if (!exynos_asv) + goto out1;
Just return -ENOMEM here.
Quoted 9 lines
arch/arm/mach-exynos4/asv.c new file +8
-0+ +/* I will add asv driver of exynos4 series to regist */ + +if (exynos_asv->check_vdd_arm) { +if (exynos_asv->check_vdd_arm ()) { +pr_info ("EXYNOS: It is wrong vdd_arm\n" ); + goto out2; + }
arch/arm/mach-exynos4/asv.c line 47
Quoted 10 lines
arch/arm/mach-exynos4/asv.c new file +10
-0+ } + +/* Get HPM Delay value */ +if (exynos_asv->get_hpm) { +if (exynos_asv->get_hpm (exynos_asv)) { +pr_info ("EXYNOS: Fail to get HPM Value\n" ); + goto out2; + } + }else { +pr_info ("EXYNOS: There is no get hpm function\n" );
Are you sure all these info printks are needed in the mainline kernel?
Quoted 45 lines
arch/arm/mach-exynos4/asv.c new file +31
-0+ goto out2; + } + +/* Get IDS ARM Value */ +if (exynos_asv->get_ids) { +if (exynos_asv->get_ids (exynos_asv)) { +pr_info ("EXYNOS: Fail to get IDS Value\n" ); + goto out2; + } + }else { +pr_info ("EXYNOS: There is no get ids function\n" ); + goto out2; + } + +if (exynos_asv->store_result) { +if (exynos_asv->store_result (exynos_asv)) { +pr_info ("EXYNOS: Can not success to store result\n" ); + goto out2; + } + }else { +pr_info ("EXYNOS: No store_result function\n" ); + goto out2; + } + +return 0 ; +out2: +kfree (exynos_asv); +out1: +return -EINVAL ; +} +device_initcall_sync (exynos_asv_init);arch/arm/mach-exynos4/include/mach/asv.h new file +8
-0@@ -0,0 +1,42 @@ +/* linux/arch/arm/mach-exynos4/include/mach/asv.h +* +* Copyright (c) 2011 Samsung Electronics Co., Ltd. +* http://www.samsung.com/ +* +* EXYNOS4 - Adoptive Support Voltage Header file +* +* This program is free software; you can redistribute it and/or modify
arch/arm/mach-exynos4/include/mach/asv.h line 18
Quoted 10 lines
arch/arm/mach-exynos4/include/mach/asv.h new file +10
-0+* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#ifndef __ASM_ARCH_ASV_H +#define __ASM_ARCH_ASV_H __FILE__ + +#define JUDGE_TABLE_END NULL + +#define LOOP_CNT 10
These defines seem quite generic, if really needed perhaps it's worth to add a prefix, e.g. EXYNOS4_.
arch/arm/mach-exynos4/include/mach/asv.h line 25
Quoted 7 lines
arch/arm/mach-exynos4/include/mach/asv.h new file +7
-0+ +struct asv_judge_table { +unsigned int hpm_limit;/* HPM value to decide group of target */ +unsigned int ids_limit;/* IDS value to decide group of target */ +}; + +struct samsung_asv {
Wouldn't it be more appropriate to use more specific name, something like 'struct exynos_asv' ?
arch/arm/mach-exynos4/include/mach/asv.h line 26
Quoted 1 line
arch/arm/mach-exynos4/include/mach/asv.h new file +1
-0+unsigned int pkg_id;/* fused value for pakage */
^^^^^^
Quoted 6 lines
arch/arm/mach-exynos4/include/mach/asv.h new file +6
-0+unsigned int ids_offset;/* ids_offset of chip */ +unsigned int ids_mask;/* ids_mask of chip */ +unsigned int hpm_result;/* hpm value of chip */ +unsigned int ids_result;/* ids value of chip */ +int (*check_vdd_arm)(void );/* check vdd_arm value, this function is selectable */ +int (*pre_clock_init)(void );/* clock init function to get hpm */
arch/arm/mach-exynos4/include/mach/asv.h line 42
Quoted 10 lines
arch/arm/mach-exynos4/include/mach/asv.h new file +10
-0+int (*pre_clock_setup)(void );/* clock setup function to get hpm */ +/* specific get ids function */ +int (*get_ids)(struct samsung_asv *asv_info); +/* specific get hpm function */ +int (*get_hpm)(struct samsung_asv *asv_info); +/* store into some repository to send result of asv */ + int (*store_result)(struct samsung_asv *asv_info); +}; + +#endif /* __ASM_ARCH_ASV_H */
You're putting whole driver under arch/arm/, I'm wondering whether the functionality it provides could be handled by some generic framework, like cpufreq or devfreq. How this driver is different from cpufreq drivers ?
--
Thanks,
Sylwester
PATCH 1/3 ARM: EXYNOS: Add ASV feature for Exynos4 series
Quoted 4 lines · Sylwester Nawrocki
You're putting whole driver under
arch/arm/, I'm wondering whether the functionality it provides could be handled by some generic framework, like cpufreq or devfreq. How this driver is different from cpufreq drivers ?
I think there would be futher usage with ASV if IEM and proper PMICs are attaches; however, for now, ASV guides proper vddarm and vddint voltages for each freuquency.
Both drivers in cpufreq(vddarm and arm core freq) and devfreq(vddint and memory/bus freq.. and GPU or MMC later?) should use the result value of ASV because they both alter voltages according to the ASV value.
Thus, if this is going to be located in drivers/, it'd better be in elsewhere (drivers/misc? drivers/hwmon? ..???) and capable of providing values to other device drivers, at least to cpufreq and devfreq drivers.
Cheers!
MyungJoo
Quoted 9 lines
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
PATCH 1/3 ARM: EXYNOS: Add ASV feature for Exynos4 series
In reply to Jongpill LeeQuoted 4 lines · Jongpill Lee
This patch adds ASV feature for exynos4 seires.
Asv(Adoptive support voltage) feature support to get spec of SoC. And we can use to adjust voltage for operation SoC usign by ASV result.
Quoted 9 lines
Signed-off-by Jongpill Lee <[email protected]>---
?arch/arm/mach-exynos4/Makefile ? ? ? ? ? | ? ?2 +- ?arch/arm/mach-exynos4/asv.c ? ? ? ? ? ? ?| ? 78 ++++++++++++++++++++++++++++++ ?arch/arm/mach-exynos4/include/mach/asv.h | ? 42 ++++++++++++++++?3 files changed, 121 insertions(+), 1 deletions(-) ?create mode 100644
arch/arm/mach-exynos4/asv.c?create mode 100644arch/arm/mach-exynos4/include/mach/asv.h
Hello,
As long as we have <mach/asv.h> in our hands, we can have the ASV value stored at the common place declared by this asv.h rather than at INFORM2 as I've mentioned before at another thread. This location <mach/asv.h> seems the perfect location for the purpose. Besides, now, with Exynos4x12s, I start to hear cases where all the INFORMx registers are fully used; thus, it seems that it's time to remove unnecessary usage of the registers.
Cheers!
MyungJoo
Signature · 2 lines
MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
PATCH 2/3 ARM: EXYNOS: Add clock for Exynos4210 asv feature
In reply to Jongpill LeeThis patch adds clock for exynos4210 asv feature
which name is sclk_pwi,sclk_hpm,dout_copy.
These clock is used for HPM devices
--- arch/arm/mach-exynos4/clock.c | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-)
arch/arm/mach-exynos4/clock.c +34 -1
@@ -306,6 +306,25 @@ static struct clksrc_clk clk_periphclk = { .reg_div = { .reg = S5P_CLKDIV_CPU, .shift = 12, .size = 3 }, }; +static struct clk *clkset_mout_hpm_list[] = { + [0 ] = &clk_mout_apll.clk, + [1 ] = &clk_mout_mpll.clk, +}; + +static struct clksrc_sources clkset_sclk_hpm = { + .sources = clkset_mout_hpm_list, + .nr_sources =ARRAY_SIZE (clkset_mout_hpm_list), +}; + +static struct clksrc_clk clk_dout_copy = { + .clk = { + .name ="dout_copy" , + }, + .sources = &clkset_sclk_hpm, + .reg_src = { .reg =S5P_CLKSRC_CPU , .shift =20 , .size =1 }, + .reg_div = { .reg =S5P_CLKDIV_CPU1 , .shift =0 , .size =3 }, +}; +/* Core list of CMU_CORE side */ struct clk *clkset_corebus_list[] = {
@@ -1239,7 +1258,20 @@ static struct clksrc_clk clksrcs[] = { .ctrlbit = (1 << 16), }, .reg_div = { .reg = S5P_CLKDIV_FSYS3, .shift = 8, .size = 8 }, - } + }, { + .clk = { + .name = "sclk_hpm", + .parent = &clk_dout_copy.clk, + }, + .reg_div = { .reg = S5P_CLKDIV_CPU1, .shift = 4, .size = 3 }, + }, { + .clk = { + .name = "sclk_pwi", + }, + .sources = &clkset_group, + .reg_src = { .reg = S5P_CLKSRC_DMC, .shift = 16, .size = 4 }, + .reg_div = { .reg = S5P_CLKDIV_DMC1, .shift = 8, .size = 4 }, + }, };/* Clock initialization code */
@@ -1253,6 +1285,7 @@ static struct clksrc_clk *sysclks[] = { &clk_armclk, &clk_aclk_corem0, &clk_aclk_cores, + &clk_dout_copy, &clk_aclk_corem1, &clk_periphclk, &clk_mout_corebus,
Signature · 1 line
1.7.1
PATCH 3/3 ARM: EXYNOS: Support ASV for Exynos4210
In reply to Jongpill LeeThis patch adds function for exynos4210's asv driver. On Exynos4210, we can get ASV result using by HPM and IDS value. And asv result is sent through INFORM2 register.
--- arch/arm/mach-exynos4/Makefile | 2 +- arch/arm/mach-exynos4/asv-4210.c | 338 +++++++++++++++++++++++ arch/arm/mach-exynos4/asv.c | 8 + arch/arm/mach-exynos4/include/mach/asv.h | 2 + arch/arm/mach-exynos4/include/mach/map.h | 2 + arch/arm/mach-exynos4/include/mach/regs-clock.h | 18 ++ arch/arm/mach-exynos4/include/mach/regs-iem.h | 27 ++ 7 files changed, 396 insertions(+), 1 deletions(-) create mode 100644 arch/arm/mach-exynos4/asv-4210.c create mode 100644 arch/arm/mach-exynos4/include/mach/regs-iem.h
arch/arm/mach-exynos4/Makefile +1 -1
@@ -12,7 +12,7 @@ obj- := # Core support for EXYNOS4 system -obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o +obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o asv-4210.o obj-$(CONFIG_ARCH_EXYNOS4) += setup-i2c0.o irq-eint.o dma.o pmu.o obj-$(CONFIG_CPU_EXYNOS4210) += clock-exynos4210.o obj-$(CONFIG_SOC_EXYNOS4212) += clock-exynos4212.o
arch/arm/mach-exynos4/asv-4210.c new file +338 -0
338 lines not on this page · raw message
arch/arm/mach-exynos4/asv.c +8 -0
@@ -17,6 +17,8 @@#include <linux/io.h> #include <linux/slab.h> +#include <plat/cpu.h> +#include <mach/map.h> #include <mach/asv.h>
@@ -29,6 +31,12 @@ static int __init exynos_asv_init(void) goto out1;/* I will add asv driver of exynos4 series to regist */ +if (soc_is_exynos4210 ()) +exynos4210_asv_init (exynos_asv); +else { +pr_info ("EXYNOS: No MACH ASV driver\n" ); + goto out2; + }if (exynos_asv->check_vdd_arm) {if (exynos_asv->check_vdd_arm ()) {
arch/arm/mach-exynos4/include/mach/asv.h +2 -0
@@ -39,4 +39,6 @@ struct samsung_asv { int (*store_result)(struct samsung_asv *asv_info); }; +extern void exynos4210_asv_init (struct samsung_asv *asv_info); +#endif /* __ASM_ARCH_ASV_H */
arch/arm/mach-exynos4/include/mach/map.h +2 -0
@@ -60,6 +60,8 @@#define EXYNOS4_PA_COMBINER 0x10440000 +#define EXYNOS4_PA_IEM 0x10460000 +#define EXYNOS4_PA_GIC_CPU 0x10480000#define EXYNOS4_PA_GIC_DIST 0x10490000
arch/arm/mach-exynos4/include/mach/regs-clock.h +18 -0
@@ -130,6 +130,24 @@#define S5P_CLKGATE_SCLKCPU S5P_CLKREG(0x14800)#define S5P_CLKGATE_IP_CPU S5P_CLKREG(0x14900) +#define S5P_APLL_CON0L8 S5P_CLKREG(0x15100) +#define S5P_APLL_CON0L7 S5P_CLKREG(0x15104) +#define S5P_APLL_CON0L6 S5P_CLKREG(0x15108) +#define S5P_APLL_CON0L5 S5P_CLKREG(0x1510C) +#define S5P_APLL_CON0L4 S5P_CLKREG(0x15110) +#define S5P_APLL_CON0L3 S5P_CLKREG(0x15114) +#define S5P_APLL_CON0L2 S5P_CLKREG(0x15118) +#define S5P_APLL_CON0L1 S5P_CLKREG(0x1511C) + +#define S5P_CLKDIV_IEM_L8 S5P_CLKREG(0x15300) +#define S5P_CLKDIV_IEM_L7 S5P_CLKREG(0x15304) +#define S5P_CLKDIV_IEM_L6 S5P_CLKREG(0x15308) +#define S5P_CLKDIV_IEM_L5 S5P_CLKREG(0x1530C) +#define S5P_CLKDIV_IEM_L4 S5P_CLKREG(0x15310) +#define S5P_CLKDIV_IEM_L3 S5P_CLKREG(0x15314) +#define S5P_CLKDIV_IEM_L2 S5P_CLKREG(0x15318) +#define S5P_CLKDIV_IEM_L1 S5P_CLKREG(0x1531C) +#define S5P_APLL_LOCKTIME (0x1C20)/* 300us */ #define S5P_APLLCON0_ENABLE_SHIFT (31)
arch/arm/mach-exynos4/include/mach/regs-iem.h new file +27 -0
@@ -0,0 +1,27 @@ +/* linux/arch/arm/mach-exynos/include/mach/regs-iem.h +* +* Copyright (c) 2011 Samsung Electronics Co., Ltd. +* http://www.samsung.com/ +* +* EXYNOS4 - IEM(INTELLIGENT ENERGY MANAGEMENT) register discription +* +* 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. +*/ + +#ifndef __ASM_ARCH_REGS_IEM_H +#define __ASM_ARCH_REGS_IEM_H __FILE__ + +/* Register for IEC */ +#define EXYNOS4_IECDPCCR (0x00000) + +/* Register for APC */ +#define EXYNOS4_APC_CONTROL (0x10010) +#define EXYNOS4_APC_PREDLYSEL (0x10024) +#define EXYNOS4_APC_DBG_DLYCODE (0x100E0) + +#define APC_HPM_EN (1 << 4) +#define IEC_EN (1 << 0) + +#endif /* __ASM_ARCH_REGS_IEM_H */
Signature · 1 line
1.7.1
PATCH 3/3 ARM: EXYNOS: Support ASV for Exynos4210
Quoted 89 lines · Jongpill Lee
This patch adds function for exynos4210's asv driver. On Exynos4210, we can get ASV result using by HPM and IDS value. And asv result is sent through INFORM2 register.
Signed-off-by Jongpill Lee<[email protected]>--- arch/arm/mach-exynos4/Makefile | 2 +- arch/arm/mach-exynos4/asv-4210.c | 338 +++++++++++++++++++++++ arch/arm/mach-exynos4/asv.c | 8 + arch/arm/mach-exynos4/include/mach/asv.h | 2 + arch/arm/mach-exynos4/include/mach/map.h | 2 + arch/arm/mach-exynos4/include/mach/regs-clock.h | 18 ++ arch/arm/mach-exynos4/include/mach/regs-iem.h | 27 ++ 7 files changed, 396 insertions(+), 1 deletions(-)create mode 100644
arch/arm/mach-exynos4/asv-4210.ccreate mode 100644arch/arm/mach-exynos4/include/mach/regs-iem.harch/arm/mach-exynos4/Makefile +1
-1@@ -12,7 +12,7 @@ obj- := # Core support for EXYNOS4 system -obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o +obj-$(CONFIG_ARCH_EXYNOS4) += cpu.o init.o clock.o irq-combiner.o asv.o asv-4210.o obj-$(CONFIG_ARCH_EXYNOS4) += setup-i2c0.o irq-eint.o dma.o pmu.o obj-$(CONFIG_CPU_EXYNOS4210) += clock-exynos4210.o obj-$(CONFIG_SOC_EXYNOS4212) += clock-exynos4212.oarch/arm/mach-exynos4/asv-4210.c new file +53
-0@@ -0,0 +1,338 @@ +/* linux/arch/arm/mach-exynos/asv-4210.c +* +* Copyright (c) 2011 Samsung Electronics Co., Ltd. +* http://www.samsung.com/ +* +* EXYNOS4210 - ASV(Adaptive Supply Voltage) driver +* +* 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. +*/ + +#include <linux/init.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/io.h> + +#include <plat/clock.h> + +#include <mach/regs-iem.h> +#include <mach/regs-clock.h> +#include <mach/asv.h> + +enum target_asv { +EXYNOS4210_1200 , +EXYNOS4210_1400 , +EXYNOS4210_SINGLE_1200 , +}; + +struct asv_judge_table exynos4210_1200_limit[] = { +/* HPM , IDS */ + {8 ,4 }, + {11 ,8 }, + {14 ,12 }, + {18 ,17 }, + {21 ,27 }, + {23 ,45 }, + {25 ,55 }, +}; + +static struct asv_judge_table exynos4210_1400_limit[] = { +/* HPM , IDS */ + {13 ,8 }, + {17 ,12 }, + {22 ,32 }, + {26 ,52 }, +}; + +static struct asv_judge_table exynos4210_single_1200_limit[] = { +/* HPM , IDS */ + {8 ,4 },
arch/arm/mach-exynos4/asv-4210.c line 63
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ {14 , 12}, + {21 , 27}, + {25 , 55}, +}; + +static int exynos4210_check_vdd_arm (void ) +{ +/* It will be support */ +return 0 ; +}
If this function is empty might be better to have it removed now.
arch/arm/mach-exynos4/asv-4210.c line 75
Quoted 12 lines
arch/arm/mach-exynos4/asv-4210.c new file +12
-0+ +static int exynos4210_asv_pre_clock_init (void ) +{ +struct clk *clk_hpm; +struct clk *clk_copy; +struct clk *clk_parent; + +/* PWI clock setting */ + clk_copy =clk_get (NULL ,"sclk_pwi" ); +if (IS_ERR (clk_copy)) { +pr_info ("EXYNOS4210: ASV : SCLK_PWI clock get error\n" ); +return -EINVAL ;
How about just doing:
return ERR_PTR(clk_copy);
?
arch/arm/mach-exynos4/asv-4210.c line 85
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ }else { + clk_parent =clk_get (NULL ,"xusbxti" ); + +if (IS_ERR (clk_parent)) { +pr_info ("EXYNOS4210: ASV: MOUT_APLL clock get error\n" ); +clk_put (clk_copy); +return -EINVAL ; + } +if (clk_set_parent (clk_copy, clk_parent)) + pr_info("EXYNOS4210: ASV: Unable to set parent %s of clock %s.\n",
If I'm not mistaken, you could omit all occurrences of "EXYNOS4210: ASV: " string used with pr_<level> functions if you have added at the beginning of this file:
#define pr_fmt(fmt) "EXYNOS4210: ASV: " fmt
Quoted 20 lines
arch/arm/mach-exynos4/asv-4210.c new file +20
-0+ clk_parent->name, clk_copy->name); + +clk_put (clk_parent); + } +clk_set_rate (clk_copy,4800000 ); + +clk_put (clk_copy); + +/* HPM clock setting */ + clk_copy =clk_get (NULL ,"dout_copy" ); +if (IS_ERR (clk_copy)) { +pr_info ("EXYNOS4210: ASV: DOUT_COPY clock get error\n" ); +return -EINVAL ; + }else { + clk_parent =clk_get (NULL ,"mout_mpll" ); +if (IS_ERR (clk_parent)) { +pr_info ("EXYNOS4210: ASV: MOUT_APLL clock get error\n" ); +clk_put (clk_copy); +return -EINVAL ; + }
arch/arm/mach-exynos4/asv-4210.c line 115
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+if (clk_set_parent (clk_copy, clk_parent)) +pr_info ("EXYNOS4210: ASV: Unable to set parent %s of clock %s.\n" , + clk_parent->name, clk_copy->name); + +clk_put (clk_parent); + } + +clk_set_rate (clk_copy, (400 *1000 *1000 )); + +clk_put (clk_copy);
The above looks like 2 copies of same thing. Ever considered making a function out of one of these and using it here instead ?
Quoted 25 lines
arch/arm/mach-exynos4/asv-4210.c new file +25
-0+ + clk_hpm =clk_get (NULL ,"sclk_hpm" ); +if (IS_ERR (clk_hpm)) { +pr_info ("EXYNOS4210: ASV: Fail to get sclk_hpm\n" ); +return -EINVAL ; + } + +clk_set_rate (clk_hpm, (200 *1000 *1000 )); + +clk_put (clk_hpm); + +return 0 ; +} + +static int exynos4210_asv_pre_clock_setup (void ) +{ +/* APLL_CON0 level register */ +__raw_writel (0x80FA0601 ,S5P_APLL_CON0L8 ); +__raw_writel (0x80C80601 ,S5P_APLL_CON0L7 ); +__raw_writel (0x80C80602 ,S5P_APLL_CON0L6 ); +__raw_writel (0x80C80604 ,S5P_APLL_CON0L5 ); +__raw_writel (0x80C80601 ,S5P_APLL_CON0L4 ); +__raw_writel (0x80C80601 ,S5P_APLL_CON0L3 ); +__raw_writel (0x80C80601 ,S5P_APLL_CON0L2 ); +__raw_writel (0x80C80601 ,S5P_APLL_CON0L1 );
arch/arm/mach-exynos4/asv-4210.c line 150
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ +/* IEM Divider register */ +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L8 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L7 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L6 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L5 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L4 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L3 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L2 ); +__raw_writel (0x00500000 ,S5P_CLKDIV_IEM_L1 );
hmm... lots of magic numbers here...
Quoted 8 lines
arch/arm/mach-exynos4/asv-4210.c new file +8
-0+ +return 0 ; +} + +static int exynos4210_find_group (struct samsung_asv *asv_info, +enum target_asv exynos4_target) +{ +unsigned int ret =0 ;
arch/arm/mach-exynos4/asv-4210.c line 168
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+unsigned int i; + +if (exynos4_target ==EXYNOS4210_1200 ) { + ret =ARRAY_SIZE (exynos4210_1200_limit); + +for (i =0 ; i<ARRAY_SIZE (exynos4210_1200_limit); i++) { +if (asv_info->hpm_result<= exynos4210_1200_limit[i].hpm_limit || + asv_info->ids_result<= exynos4210_1200_limit[i].ids_limit) { + ret = i; +break ;
Perhaps it's just enough to return i directly.
Quoted 23 lines
arch/arm/mach-exynos4/asv-4210.c new file +23
-0+ } + } + }else if (exynos4_target ==EXYNOS4210_1400 ) { + ret =ARRAY_SIZE (exynos4210_1400_limit); + +for (i =0 ; i<ARRAY_SIZE (exynos4210_1400_limit); i++) { +if (asv_info->hpm_result<= exynos4210_1400_limit[i].hpm_limit || + asv_info->ids_result<= exynos4210_1400_limit[i].ids_limit) { + ret = i; +break ; + } + } + }else if (exynos4_target ==EXYNOS4210_SINGLE_1200 ) { + ret =ARRAY_SIZE (exynos4210_single_1200_limit); + +for (i =0 ; i<ARRAY_SIZE (exynos4210_single_1200_limit); i++) { +if (asv_info->hpm_result<= exynos4210_single_1200_limit[i].hpm_limit || + asv_info->ids_result<= exynos4210_single_1200_limit[i].ids_limit) { + ret = i; +break ; + } + } + }
arch/arm/mach-exynos4/asv-4210.c line 201
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ +return ret; +} + +#define PACK_ID 8 +#define PACK_MASK 0x3 + +#define SUPPORT_1400MHZ (1<<31) +#define SUPPORT_1200MHZ (1<<30) +#define SUPPORT_1000MHZ (1<<29)
? Do these happen to be S5P_INFORM2 register bit definitions ? If so the names should tell it explicitly. And there are whitespaces missing around '<<'.
arch/arm/mach-exynos4/asv-4210.c line 210
Quoted 9 lines
arch/arm/mach-exynos4/asv-4210.c new file +9
-0+ +static int exynos4210_get_hpm (struct samsung_asv *asv_info) +{ +unsigned int i; +unsigned int tmp; +unsigned int hpm_delay =0 ; +void __iomem *iem_base; + + iem_base =ioremap (EXYNOS4_PA_IEM , (128 *1024 ));
More magic numbers...
arch/arm/mach-exynos4/asv-4210.c line 213
Quoted 3 lines
arch/arm/mach-exynos4/asv-4210.c new file +3
-0+ +if (!iem_base) { +pr_info ("EXYNOS: ioremap fail\n" );
pr_info at an error level ?
arch/arm/mach-exynos4/asv-4210.c line 219
Quoted 6 lines
arch/arm/mach-exynos4/asv-4210.c new file +6
-0+return -EPERM ; + } + +/* Clock setting to get asv value */ +if (!asv_info->pre_clock_init) { +pr_info ("EXYNOS: No Pre-setup function\n" );
ditto
arch/arm/mach-exynos4/asv-4210.c line 221
Quoted 2 lines
arch/arm/mach-exynos4/asv-4210.c new file +2
-0+ goto err; + }else {
'else' is not needed
arch/arm/mach-exynos4/asv-4210.c line 224
Quoted 3 lines
arch/arm/mach-exynos4/asv-4210.c new file +3
-0+if (asv_info->pre_clock_init ()) { +pr_info ("EXYNOS: pre_clock_init function fail" ); + goto err;
ditto
Quoted 14 lines
arch/arm/mach-exynos4/asv-4210.c new file +14
-0+ }else { +/* HPM enable */ + tmp =__raw_readl (iem_base +EXYNOS4_APC_CONTROL ); + tmp |=APC_HPM_EN ; +__raw_writel (tmp, (iem_base +EXYNOS4_APC_CONTROL )); + + asv_info->pre_clock_setup (); + +/* IEM enable */ + tmp =__raw_readl (iem_base +EXYNOS4_IECDPCCR ); + tmp |=IEC_EN ; +__raw_writel (tmp, (iem_base +EXYNOS4_IECDPCCR )); + } + }
arch/arm/mach-exynos4/asv-4210.c line 248
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ +/* Get HPM Delay value */ +for (i =0 ; i<LOOP_CNT ; i++) { + tmp =__raw_readb (iem_base +EXYNOS4_APC_DBG_DLYCODE ); + hpm_delay += tmp; + } + + hpm_delay /=LOOP_CNT ; + +/* Store result of hpm value */
This type of comments doesn't seem to add much value.
Quoted 22 lines
arch/arm/mach-exynos4/asv-4210.c new file +22
-0+ asv_info->hpm_result = hpm_delay; + +return 0 ; + +err: +iounmap (iem_base); + +return -EPERM ; +} + +static int exynos4210_get_ids (struct samsung_asv *asv_info) +{ +unsigned int pkg_id_val; + +if (!asv_info->ids_offset || !asv_info->ids_mask) { +pr_info ("EXYNOS4: No ids_offset or No ids_mask\n" ); +return -EPERM ; + } + + pkg_id_val =__raw_readl (S5P_VA_CHIPID +0x4 ); + asv_info->pkg_id = pkg_id_val; + asv_info->ids_result = ((pkg_id_val>> asv_info->ids_offset)&
arch/arm/mach-exynos4/asv-4210.c line 280
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ asv_info->ids_mask); + +return 0 ; +} + +static int exynos4210_asv_store_result (struct samsung_asv *asv_info) +{ +unsigned int result_grp; +char *support_freq; +unsigned int exynos_idcode =0x0 ;
Assigment to 0 is unneeded, since exynos_idcode is overwritten right below. Also might be better to use u32 rather than 'unsigned int' in this case.
arch/arm/mach-exynos4/asv-4210.c line 293
Quoted 13 lines
arch/arm/mach-exynos4/asv-4210.c new file +13
-0+ + exynos_idcode =__raw_readl (S5P_VA_CHIPID ); + +/* Single chip is only support 1.2GHz */ +if (!((exynos_idcode>>PACK_ID )&PACK_MASK )) { + result_grp =exynos4210_find_group (asv_info,EXYNOS4210_SINGLE_1200 ); + result_grp |=SUPPORT_1200MHZ ; + support_freq ="1.2GHz" ; + + goto set_reg; + } + +/* Check support freq */
supported frequencies ?
arch/arm/mach-exynos4/asv-4210.c line 305
Quoted 12 lines
arch/arm/mach-exynos4/asv-4210.c new file +12
-0+switch (asv_info->pkg_id&0x7 ) { +/* Support 1.2GHz */ +case 1 : +case 7 : + result_grp =exynos4210_find_group (asv_info,EXYNOS4210_1200 ); + result_grp |=SUPPORT_1200MHZ ; + support_freq ="1.2GHz" ; +break ; +/* Support 1.4GHz */ +case 5 : + result_grp =exynos4210_find_group (asv_info,EXYNOS4210_1400 ); + result_grp |=SUPPORT_1200MHZ ;
typo ?
arch/arm/mach-exynos4/asv-4210.c line 308
Quoted 3 lines
arch/arm/mach-exynos4/asv-4210.c new file +3
-0+ support_freq ="1.4GHz" ; +break ; +/* Defalut support 1.0GHz */
s/Defalut/Default
I'm just wondering what exactly 'support' refers here to..
Quoted 12 lines
arch/arm/mach-exynos4/asv-4210.c new file +12
-0+default : + result_grp =exynos4210_find_group (asv_info,EXYNOS4210_1200 ); + result_grp |=SUPPORT_1000MHZ ; + support_freq ="1.0GHz" ; +break ; + } + +set_reg: +__raw_writel (result_grp,S5P_INFORM2 ); + +pr_info ("Support %s\n" , support_freq); +pr_info ("ASV Group for This Exynos4210 is 0x%x\n" , result_grp);
arch/arm/mach-exynos4/asv-4210.c line 330
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ +return 0 ; +} + +void exynos4210_asv_init (struct samsung_asv *asv_info) +{ +pr_info ("EXYNOS4210: Adaptive Support Voltage init\n" ); + + asv_info->ids_offset =24 ; + asv_info->ids_mask =0xFF ;
In the kernel lower case hex numbers are preferred.
Quoted 22 lines
arch/arm/mach-exynos4/asv-4210.c new file +8
-0+ + asv_info->get_ids = exynos4210_get_ids; + asv_info->get_hpm = exynos4210_get_hpm; + asv_info->check_vdd_arm = exynos4210_check_vdd_arm; + asv_info->pre_clock_init = exynos4210_asv_pre_clock_init; + asv_info->pre_clock_setup = exynos4210_asv_pre_clock_setup; + asv_info->store_result = exynos4210_asv_store_result; +}arch/arm/mach-exynos4/asv.c +2
-0@@ -17,6 +17,8 @@#include <linux/io.h> #include <linux/slab.h> +#include <plat/cpu.h> +#include <mach/map.h> #include <mach/asv.h>
arch/arm/mach-exynos4/asv.c static int __init exynos_asv_init(void) · line 33
Quoted 3 lines
arch/arm/mach-exynos4/asv.c +0
-0@@ -29,6 +31,12 @@ static int __init exynos_asv_init(void) goto out1;/* I will add asv driver of exynos4 series to regist */
Is this comment still relevant ?
Quoted 39 lines
arch/arm/mach-exynos4/asv.c +6
-0+if (soc_is_exynos4210 ()) +exynos4210_asv_init (exynos_asv); +else { +pr_info ("EXYNOS: No MACH ASV driver\n" ); + goto out2; + }if (exynos_asv->check_vdd_arm) {if (exynos_asv->check_vdd_arm ()) {arch/arm/mach-exynos4/include/mach/asv.h +2
-0@@ -39,4 +39,6 @@ struct samsung_asv { int (*store_result)(struct samsung_asv *asv_info); }; +extern void exynos4210_asv_init (struct samsung_asv *asv_info); +#endif /* __ASM_ARCH_ASV_H */ arch/arm/mach-exynos4/include/mach/map.h +2
-0@@ -60,6 +60,8 @@#define EXYNOS4_PA_COMBINER 0x10440000 +#define EXYNOS4_PA_IEM 0x10460000 +#define EXYNOS4_PA_GIC_CPU 0x10480000#define EXYNOS4_PA_GIC_DIST 0x10490000arch/arm/mach-exynos4/include/mach/regs-clock.h +0
-0@@ -130,6 +130,24 @@#define S5P_CLKGATE_SCLKCPU S5P_CLKREG(0x14800)
arch/arm/mach-exynos4/include/mach/regs-clock.h line 140
Quoted 10 lines
arch/arm/mach-exynos4/include/mach/regs-clock.h +8
-0#define S5P_CLKGATE_IP_CPU S5P_CLKREG(0x14900) +#define S5P_APLL_CON0L8 S5P_CLKREG(0x15100) +#define S5P_APLL_CON0L7 S5P_CLKREG(0x15104) +#define S5P_APLL_CON0L6 S5P_CLKREG(0x15108) +#define S5P_APLL_CON0L5 S5P_CLKREG(0x1510C) +#define S5P_APLL_CON0L4 S5P_CLKREG(0x15110) +#define S5P_APLL_CON0L3 S5P_CLKREG(0x15114) +#define S5P_APLL_CON0L2 S5P_CLKREG(0x15118) +#define S5P_APLL_CON0L1 S5P_CLKREG(0x1511C)
Do you think, instead of the above 8 lines, you could use something like:
#define S5P_APLL_CON0L(n) S5P_CLKREG(0x15100 + (8 - (n)) * 4)
arch/arm/mach-exynos4/include/mach/regs-clock.h line 149
Quoted 9 lines
arch/arm/mach-exynos4/include/mach/regs-clock.h +9
-0+ +#define S5P_CLKDIV_IEM_L8 S5P_CLKREG(0x15300) +#define S5P_CLKDIV_IEM_L7 S5P_CLKREG(0x15304) +#define S5P_CLKDIV_IEM_L6 S5P_CLKREG(0x15308) +#define S5P_CLKDIV_IEM_L5 S5P_CLKREG(0x1530C) +#define S5P_CLKDIV_IEM_L4 S5P_CLKREG(0x15310) +#define S5P_CLKDIV_IEM_L3 S5P_CLKREG(0x15314) +#define S5P_CLKDIV_IEM_L2 S5P_CLKREG(0x15318) +#define S5P_CLKDIV_IEM_L1 S5P_CLKREG(0x1531C)
and
#define S5P_CLKDIV_IEM_L(n) S5P_CLKREG(0x15300 + (8 - (n)) * 4)
(n = 1...8) ?
Quoted 10 lines
arch/arm/mach-exynos4/include/mach/regs-clock.h +1
-0+#define S5P_APLL_LOCKTIME (0x1C20)/* 300us */ #define S5P_APLLCON0_ENABLE_SHIFT (31)arch/arm/mach-exynos4/include/mach/regs-iem.h new file +0
-0
arch/arm/mach-exynos4/include/mach/regs-iem.h line 6
Quoted 6 lines
arch/arm/mach-exynos4/include/mach/regs-iem.h new file +6
-0@@ -0,0 +1,27 @@ +/* linux/arch/arm/mach-exynos/include/mach/regs-iem.h +* +* Copyright (c) 2011 Samsung Electronics Co., Ltd. +* http://www.samsung.com/ +* +* EXYNOS4 - IEM(INTELLIGENT ENERGY MANAGEMENT) register discription
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Really need this in upper case ?
Quoted 6 lines
arch/arm/mach-exynos4/include/mach/regs-iem.h new file +6
-0+* +* 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. +*/ +
arch/arm/mach-exynos4/include/mach/regs-iem.h line 22
Quoted 10 lines
arch/arm/mach-exynos4/include/mach/regs-iem.h new file +10
-0+#ifndef __ASM_ARCH_REGS_IEM_H +#define __ASM_ARCH_REGS_IEM_H __FILE__ + +/* Register for IEC */ +#define EXYNOS4_IECDPCCR (0x00000) + +/* Register for APC */ +#define EXYNOS4_APC_CONTROL (0x10010) +#define EXYNOS4_APC_PREDLYSEL (0x10024) +#define EXYNOS4_APC_DBG_DLYCODE (0x100E0)
Is there any specific reason to have parentheses around the numbers ?
PATCH 3/3 ARM: EXYNOS: Support ASV for Exynos4210
In reply to Jongpill LeeQuoted 18 lines · Jongpill Lee
arch/arm/mach-exynos4/asv-4210.c new file +18
-0+static int exynos4210_asv_pre_clock_init (void ) +{ +struct clk *clk_hpm; +struct clk *clk_copy; +struct clk *clk_parent; + +/* PWI clock setting */ + clk_copy =clk_get (NULL ,"sclk_pwi" ); +if (IS_ERR (clk_copy)) { +pr_info ("EXYNOS4210: ASV : SCLK_PWI clock get error\n" ); +return -EINVAL ; + }else { + clk_parent =clk_get (NULL ,"xusbxti" ); + +if (IS_ERR (clk_parent)) { +pr_info ("EXYNOS4210: ASV: MOUT_APLL clock get error\n" ); +clk_put (clk_copy); +return -EINVAL ;
arch/arm/mach-exynos4/asv-4210.c line 92
Quoted 10 lines
arch/arm/mach-exynos4/asv-4210.c new file +10
-0+ } +if (clk_set_parent (clk_copy, clk_parent)) +pr_info ("EXYNOS4210: ASV: Unable to set parent %s of clock %s.\n" , + clk_parent->name, clk_copy->name); + +clk_put (clk_parent); + } +clk_set_rate (clk_copy,4800000 ); + +clk_put (clk_copy);
This looks really weird - if something is using a clock, the reference to the struct clk is supposed to be held all the time that the clock is being used. Plus there's nothing to enable the clock itself (so it's probably disabled).