RE: [PATCH 3/3] ARM: EXYNOS4: Support ASV for Exynos4210

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.
> If you want to use asv result on your driver, please define
> exynos_result_of_asv with extern.
> 
> Signed-off-by: Jongpill Lee <boyko.lee@xxxxxxxxxxx>
> ---
>  arch/arm/mach-exynos4/Makefile                  |    2 +-
>  arch/arm/mach-exynos4/asv-4210.c                |  339

I would prefer, asv-exynos4210.c like others.

> +++++++++++++++++++++++
>  arch/arm/mach-exynos4/asv.c                     |    9 +-
>  arch/arm/mach-exynos4/include/mach/asv.h        |    3 +
>  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, 397 insertions(+), 3 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 587ce90..5422b01 100644
> --- a/arch/arm/mach-exynos4/Makefile
> +++ b/arch/arm/mach-exynos4/Makefile
> @@ -18,7 +18,7 @@ obj-$(CONFIG_CPU_EXYNOS4210)	+=
clock-exynos4210.o
>  obj-$(CONFIG_SOC_EXYNOS4212)	+= clock-exynos4212.o
>  obj-$(CONFIG_PM)		+= pm.o
>  obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
> -obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= asv.o
> +obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= asv.o asv-4210.o

If select 'cpufreq', asv should be enabled?

> 
>  obj-$(CONFIG_SMP)		+= platsmp.o headsmp.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..1260ab8
> --- /dev/null
> +++ b/arch/arm/mach-exynos4/asv-4210.c
> @@ -0,0 +1,339 @@
> +/* linux/arch/arm/mach-exynos/asv-4210.c
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com/
> + *
> + * EXYNOS4210 - ASV(Adaptive Support 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>
> +
> +/*
> + * exynos_result_of_asv is result of ASV group.
> + * Using by this value, other driver can adjust voltage.
> + */
> +unsigned int exynos_result_of_asv;
> +
> +enum target_asv {
> +	EXYNOS4210_1200,
> +	EXYNOS4210_1400,
> +	EXYNOS4210_SINGLE_1200,

As I know, _1200 and _1400 do not mean voltage but they looks like voltage
when I see above, target_a(daptive)s(upport)v(oltage).

> +};
> +
> +struct asv_judge_table exynos4210_1200_limit[] = {

static?

> +	/* HPM , IDS */
> +	{8 , 4},
> +	{11 , 8},
> +	{14 , 12},
> +	{18 , 17},
> +	{21 , 27},
> +	{23 , 45},
> +	{25 , 55},

And I think, needs comments about above.

> +};
> +
> +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_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))
> +		goto clock_fail;
> +	else {

No need 'else' here.

> +		clk_parent = clk_get(NULL, "xusbxti");
> +
> +		if (IS_ERR(clk_parent)) {
> +			clk_put(clk_copy);
> +
> +			goto clock_fail;
> +		}
> +		if (clk_set_parent(clk_copy, clk_parent))

Don't we need clk_put(clk_copy) and clk_put(clk_parent) here before out?

> +			goto clock_fail;
> +
> +		clk_put(clk_parent);
> +	}
> +	clk_set_rate(clk_copy, (48 * MHZ));
> +
> +	clk_put(clk_copy);
> +

I think, you need to sort out this function like following.

	a = clk_get A
	if (error a)
		goto fail1;

	b = clk_get B
	if (error b)
		goto fail2;

	c = clk_get C
	if (error c)
		goto fail3;

	...
	clk_put(c)

fail3:
	...
	clk_put(b)

fail2:
	...
	clk_put(a)

fail1:
	...


> +	/* HPM clock setting */
> +	clk_copy = clk_get(NULL, "dout_copy");
> +	if (IS_ERR(clk_copy))
> +		goto clock_fail;
> +	else {
> +		clk_parent = clk_get(NULL, "mout_mpll");
> +		if (IS_ERR(clk_parent)) {
> +			clk_put(clk_copy);
> +
> +			goto clock_fail;
> +		}
> +		if (clk_set_parent(clk_copy, clk_parent))
> +			goto clock_fail;
> +
> +		clk_put(clk_parent);
> +	}
> +
> +	clk_set_rate(clk_copy, (400 * MHZ));
> +
> +	clk_put(clk_copy);
> +
> +	clk_hpm = clk_get(NULL, "sclk_hpm");
> +	if (IS_ERR(clk_hpm))
> +		goto clock_fail;
> +
> +	clk_set_rate(clk_hpm, (200 * MHZ));
> +
> +	clk_put(clk_hpm);
> +

Same as above...

> +	return 0;
> +
> +clock_fail:
> +	pr_err("EXYNOS4210: ASV: Clock init fail\n");
> +
> +	return -EBUSY;
> +}
> +
> +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);
> +

Could you please add comments why this is needed?

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

How about to use 'switch' instead of 'if'?

> +		ret = ARRAY_SIZE(exynos4210_1200_limit);
> +
> +		for (i = 0; i < ARRAY_SIZE(exynos4210_1200_limit); i++) {

for (i = 0; i < ret; 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;

Just 'return i;' instead of 'ret = i; break; and return ret;'?

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

Hmm...

> +
> +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, SZ_128K);
> +
> +	if (!iem_base) {
> +		pr_err("EXYNOS4210: ASV: ioremap fail\n");
> +		return -EPERM;

Maybe -ENOMEM?

> +	}
> +
> +	/* Clock setting to get asv value */
> +	if (!asv_info->pre_clock_init)
> +		goto err;
> +	else {
> +		if (asv_info->pre_clock_init())
> +			goto err;
> +		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 < EXYNOS4_LOOP_CNT; i++) {
> +		tmp = __raw_readb(iem_base + EXYNOS4_APC_DBG_DLYCODE);
> +		hpm_delay += tmp;
> +	}
> +
> +	hpm_delay /= EXYNOS4_LOOP_CNT;
> +
> +	/* Store result of hpm value */
> +	asv_info->hpm_result = hpm_delay;

Don't we need 'iounmap(iem_base);' before out?

> +
> +	return 0;
> +
> +err:
> +	pr_err("EXYNOS4210: ASV: Failt to get hpm function\n");

Fail to ...?

and how about to add function name in error message?
pr_err("%s: ...\n", __func__);

> +
> +	iounmap(iem_base);
> +
> +	return -EPERM;

Why?

> +}
> +
> +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_err("EXYNOS4210: ASV: No ids_offset or No ids_mask\n");
> +
> +		return -EPERM;

Hmm...please check the return value again.

> +	}
> +
> +	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;
> +
> +	exynos_result_of_asv = 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 */
> +	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;
> +		support_freq = "1.4GHz";
> +		break;
> +	/* Defalut support 1.0GHz */
> +	default:
> +		result_grp = exynos4210_find_group(asv_info,
> EXYNOS4210_1200);
> +		result_grp |= SUPPORT_1000MHZ;
> +		support_freq = "1.0GHz";
> +		break;
> +	}
> +
> +set_reg:
> +	exynos_result_of_asv = result_grp;
> +
> +	pr_info("EXYNOS4: ASV: Support %s and Group is 0x%x\n",
> +					support_freq, 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;
> +
> +	asv_info->get_ids = exynos4210_get_ids;
> +	asv_info->get_hpm = exynos4210_get_hpm;
> +	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 b13d182..85d076e 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>
> 
> @@ -26,9 +28,12 @@ static int __init exynos_asv_init(void)
>  {
>  	exynos_asv = kzalloc(sizeof(struct samsung_asv), GFP_KERNEL);
>  	if (!exynos_asv)
> -		return -ENOMEM;
> +		goto out;

NO, see your 'out:' which includes 'kfree(exynos_asv)'

> 
> -	/* I will add asv driver of exynos4 series to regist */
> +	if (soc_is_exynos4210())
> +		exynos4210_asv_init(exynos_asv);
> +	else
> +		goto out;
> 
>  	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 9a4a8f4..cd43c48 100644
> --- a/arch/arm/mach-exynos4/include/mach/asv.h
> +++ b/arch/arm/mach-exynos4/include/mach/asv.h
> @@ -14,6 +14,7 @@
>  #define __ASM_ARCH_ASV_H __FILE__
> 
>  #define JUDGE_TABLE_END			NULL
> +#define EXYNOS4_LOOP_CNT		10

Is there any specific reason, 10?

> 
>  struct asv_judge_table {
>  	unsigned int hpm_limit; /* HPM value to decide group of target */
> @@ -37,4 +38,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)
> +
> +#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)
> 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
> + *
> + * 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)

above is not used.

> +#define EXYNOS4_APC_DBG_DLYCODE		(0x100E0)
> +
> +#define APC_HPM_EN			(1 << 4)
> +#define IEC_EN				(1 << 0)
> +
> +#endif /* __ASM_ARCH_REGS_IEM_H */
> --
> 1.7.1

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@xxxxxxxxxxx>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux