Subject: [PATCH 3/3] ARM: EXYNOS: Support ASV for Exynos4210

From: [email-masked] (Sylwester Nawrocki)
Date: Fri, 28 Oct 2011 22:12:22 +0200

On 10/27/2011 03:12 AM, Jongpill Lee wrote:
> 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-masked]>
> ---
>   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
> 
> diff --git a/arch/arm/mach-exynos4/Makefile b/arch/arm/mach-exynos4/Makefile
> index 0f3affe..8627669 100644
> --- a/arch/arm/mach-exynos4/Makefile
> +++ b/arch/arm/mach-exynos4/Makefile
> @@ -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
> diff --git a/arch/arm/mach-exynos4/asv-4210.c b/arch/arm/mach-exynos4/asv-4210.c
> new file mode 100644
> index 0000000..81a1c67
> --- /dev/null
> +++ b/arch/arm/mach-exynos4/asv-4210.c
> @@ -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},
> +	{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.

> +
> +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);		
?

> +	} 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

> +					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;
> +		}
> +		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 ?

> +
> +	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);
> +
> +	/* 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...

> +
> +	return 0;
> +}
> +
> +static int exynos4210_find_group(struct samsung_asv *asv_info,
> +			      enum target_asv exynos4_target)
> +{
> +	unsigned int ret = 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.  

> +			}
> +		}
> +	} 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;
> +			}
> +		}
> +	}
> +
> +	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 '<<'.

> +
> +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...

> +
> +	if (!iem_base) {
> +		pr_info("EXYNOS: ioremap fail\n");

pr_info at an error level ?

> +		return -EPERM;
> +	}
> +
> +	/* Clock setting to get asv value */
> +	if (!asv_info->pre_clock_init) {
> +		pr_info("EXYNOS: No Pre-setup function\n");

ditto

> +		goto err;
> +	} else {

'else' is not needed

> +		if (asv_info->pre_clock_init()) {
> +			pr_info("EXYNOS: pre_clock_init function fail");
> +			goto err;

ditto

> +		} 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));
> +		}
> +	}
> +
> +	/* 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.

> +	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)&
> +							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.

> +
> +	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 ?

> +	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 ?

> +		support_freq = "1.4GHz";
> +		break;
> +	/* Defalut support 1.0GHz */

s/Defalut/Default

I'm just wondering what exactly 'support' refers here to..

> +	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);
> +
> +	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.

> +
> +	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;
> +}
> diff --git a/arch/arm/mach-exynos4/asv.c b/arch/arm/mach-exynos4/asv.c
> index 498fac0..5acc8b1 100644
> --- a/arch/arm/mach-exynos4/asv.c
> +++ b/arch/arm/mach-exynos4/asv.c
> @@ -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 */

Is this comment still relevant ?

> +	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()) {
> diff --git a/arch/arm/mach-exynos4/include/mach/asv.h b/arch/arm/mach-exynos4/include/mach/asv.h
> index 038a872..f84a11d 100644
> --- a/arch/arm/mach-exynos4/include/mach/asv.h
> +++ b/arch/arm/mach-exynos4/include/mach/asv.h
> @@ -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 */
> diff --git a/arch/arm/mach-exynos4/include/mach/map.h b/arch/arm/mach-exynos4/include/mach/map.h
> index 918a979..81fa158 100644
> --- a/arch/arm/mach-exynos4/include/mach/map.h
> +++ b/arch/arm/mach-exynos4/include/mach/map.h
> @@ -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
> 
> diff --git a/arch/arm/mach-exynos4/include/mach/regs-clock.h b/arch/arm/mach-exynos4/include/mach/regs-clock.h
> index 6c37ebe..23ee10f 100644
> --- a/arch/arm/mach-exynos4/include/mach/regs-clock.h
> +++ b/arch/arm/mach-exynos4/include/mach/regs-clock.h
> @@ -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)

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)

> +
> +#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) ?

> +
>   #define S5P_APLL_LOCKTIME		(0x1C20)	/* 300us */
> 
>   #define S5P_APLLCON0_ENABLE_SHIFT	(31)
> diff --git a/arch/arm/mach-exynos4/include/mach/regs-iem.h b/arch/arm/mach-exynos4/include/mach/regs-iem.h
> new file mode 100644
> index 0000000..d9bf177
> --- /dev/null
> +++ b/arch/arm/mach-exynos4/include/mach/regs-iem.h
> @@ -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 ?

> + *
> + * 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)

Is there any specific reason to have parentheses around the numbers ?