Re: [PATCH v2 2/2] cpufreq: exynos4210: Use the common clock framework to set APLL clock rate

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Wednesday, October 09, 2013 02:08:43 PM Lukasz Majewski wrote:
> In the exynos4210_set_apll() function, the APLL frequency is set with
> direct register manipulation.
> 
> Such approach is not allowed in the common clock framework. The frequency
> is changed, but the corresponding clock value is not updated. This causes
> wrong frequency read from cpufreq's cpuinfo_cur_freq sysfs attribute.
> 
> Also direct manipulation with PLL's S parameter has been removed. It is
> already done at PLL35xx code.
> 
> Tested at:
> - Exynos4210 - Trats board (linux 3.12-rc4)
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

I need an ACK or Reviewed-by from someone in the CC list here.

Thanks!

> Changes for v2:
> - Remove PLL's S parameter setting via registers. It is now done with PLL35xx
>   code.
> - Remove exynos4210_pms_change() function
> 
> Change-Id: Ie5fb3c7946ba77b6f3d5e91af72eef2fd06770c1
> ---
>  drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++------------------------------
>  1 file changed, 8 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
> index add7fbe..f2c7506 100644
> --- a/drivers/cpufreq/exynos4210-cpufreq.c
> +++ b/drivers/cpufreq/exynos4210-cpufreq.c
> @@ -81,9 +81,9 @@ static void exynos4210_set_clkdiv(unsigned int div_index)
>  
>  static void exynos4210_set_apll(unsigned int index)
>  {
> -	unsigned int tmp;
> +	unsigned int tmp, freq = apll_freq_4210[index].freq;
>  
> -	/* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> +	/* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
>  	clk_set_parent(moutcore, mout_mpll);
>  
>  	do {
> @@ -92,21 +92,9 @@ static void exynos4210_set_apll(unsigned int index)
>  		tmp &= 0x7;
>  	} while (tmp != 0x2);
>  
> -	/* 2. Set APLL Lock time */
> -	__raw_writel(EXYNOS4_APLL_LOCKTIME, EXYNOS4_APLL_LOCK);
> -
> -	/* 3. Change PLL PMS values */
> -	tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -	tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
> -	tmp |= apll_freq_4210[index].mps;
> -	__raw_writel(tmp, EXYNOS4_APLL_CON0);
> +	clk_set_rate(mout_apll, freq * 1000);
>  
> -	/* 4. wait_lock_time */
> -	do {
> -		tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -	} while (!(tmp & (0x1 << EXYNOS4_APLLCON0_LOCKED_SHIFT)));
> -
> -	/* 5. MUX_CORE_SEL = APLL */
> +	/* MUX_CORE_SEL = APLL */
>  	clk_set_parent(moutcore, mout_apll);
>  
>  	do {
> @@ -115,53 +103,15 @@ static void exynos4210_set_apll(unsigned int index)
>  	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
>  }
>  
> -static bool exynos4210_pms_change(unsigned int old_index, unsigned int new_index)
> -{
> -	unsigned int old_pm = apll_freq_4210[old_index].mps >> 8;
> -	unsigned int new_pm = apll_freq_4210[new_index].mps >> 8;
> -
> -	return (old_pm == new_pm) ? 0 : 1;
> -}
> -
>  static void exynos4210_set_frequency(unsigned int old_index,
>  				     unsigned int new_index)
>  {
> -	unsigned int tmp;
> -
>  	if (old_index > new_index) {
> -		if (!exynos4210_pms_change(old_index, new_index)) {
> -			/* 1. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -
> -			/* 2. Change just s value in apll m,p,s value */
> -			tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -			tmp &= ~(0x7 << 0);
> -			tmp |= apll_freq_4210[new_index].mps & 0x7;
> -			__raw_writel(tmp, EXYNOS4_APLL_CON0);
> -		} else {
> -			/* Clock Configuration Procedure */
> -			/* 1. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -			/* 2. Change the apll m,p,s value */
> -			exynos4210_set_apll(new_index);
> -		}
> +		exynos4210_set_clkdiv(new_index);
> +		exynos4210_set_apll(new_index);
>  	} else if (old_index < new_index) {
> -		if (!exynos4210_pms_change(old_index, new_index)) {
> -			/* 1. Change just s value in apll m,p,s value */
> -			tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -			tmp &= ~(0x7 << 0);
> -			tmp |= apll_freq_4210[new_index].mps & 0x7;
> -			__raw_writel(tmp, EXYNOS4_APLL_CON0);
> -
> -			/* 2. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -		} else {
> -			/* Clock Configuration Procedure */
> -			/* 1. Change the apll m,p,s value */
> -			exynos4210_set_apll(new_index);
> -			/* 2. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -		}
> +		exynos4210_set_apll(new_index);
> +		exynos4210_set_clkdiv(new_index);
>  	}
>  }
>  
> @@ -194,7 +144,6 @@ int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
>  	info->volt_table = exynos4210_volt_table;
>  	info->freq_table = exynos4210_freq_table;
>  	info->set_freq = exynos4210_set_frequency;
> -	info->need_apll_change = exynos4210_pms_change;
>  
>  	return 0;
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.