[PATCH v4 7/7] ARM: S5PV210: Initial CPUFREQ Support
MyungJoo Ham
myungjoo.ham at samsung.com
Mon Jul 26 02:15:50 EDT 2010
On Mon, Jul 26, 2010 at 1:54 PM, Kukjin Kim <kgene.kim at samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> S5PV210 CPUFREQ Support.
>>
>> This CPUFREQ may work without PMIC's DVS support. However, it is not
>> as effective without DVS support as supposed. AVS is not supported in
>> this version.
>>
>> Note that CLK_SRC of some clocks including ARMCLK, G3D, G2D, MFC,
>> and ONEDRAM are modified directly without updating clksrc_src
>> representations of the clocks. Because CPUFREQ reverts the CLK_SRC
>> to the supposed values, this does not affect the consistency as long as
>> there are no other modules that modifies clock sources of ARMCLK, G3D,
>> G2D, MFC, and ONEDRAM (and only CPUFREQ should have the control). As
>> soon as clock framework is settled, we may update source clocks
>> (.parent field) of those clocks accordingly later.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>
> Hi MyungJoo,
Hello,
>
>> --
>> v2:
>> - Ramp-up delay is removed. (let regulator framework do the job)
>> - Provide proper max values for regulator_set_voltage
>> - Removed unneccesary #ifdef's.
>> - Removed unnecessary initialiser for CLK_OUT
>> v3:
>> - Style corrections (pr_info/pr_err, ...)
>> - Revised dvs_conf struct
>> v4:
>> - Renamed cpufreq-s5pv210.c -> cpufreq.c
>> - Style corrections (less #ifdef's, comments)
>> - Removed unncessary codes
>> - Remove #ifdef for WORKAROUND, use s5pv210_workaround()
>> - Renamed some static variables (get rid of s5pv210 prefix)
>> - Added machine dependency to Kconfig/Makefile
>> - DMC0, DMC1 refresh counter is not updated by a hardcoded value,
> but
>> with the value given as the default and relative clock speeds.
> Besides,
>> the algorithm to update DMC0/1 refresh counter is reformed.
>>
>> ---
>> arch/arm/Kconfig | 1 +
>> arch/arm/mach-s5pv210/Kconfig | 5 +
>> arch/arm/mach-s5pv210/Makefile | 2 +
>> arch/arm/mach-s5pv210/cpufreq.c | 783
>> +++++++++++++++++++++++++
>> arch/arm/mach-s5pv210/include/mach/cpu-freq.h | 40 ++
>> 5 files changed, 831 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/mach-s5pv210/cpufreq.c
>> create mode 100644 arch/arm/mach-s5pv210/include/mach/cpu-freq.h
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 98922f7..d5a5916 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -701,6 +701,7 @@ config ARCH_S5PV210
>> select HAVE_CLK
>> select ARM_L1_CACHE_SHIFT_6
>> select ARCH_USES_GETTIMEOFFSET
>> + select ARCH_HAS_CPUFREQ
>> help
>> Samsung S5PV210/S5PC110 series based systems
>>
>> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
>> index 631019a..1c6d3bc 100644
>> --- a/arch/arm/mach-s5pv210/Kconfig
>> +++ b/arch/arm/mach-s5pv210/Kconfig
>> @@ -14,6 +14,7 @@ config CPU_S5PV210
>> select PLAT_S5P
>> select S3C_PL330_DMA
>> select S5P_EXT_INT
>> + select S5PV210_CPU_FREQ if CPU_FREQ
>> help
>> Enable S5PV210 CPU support
>>
>> @@ -101,4 +102,8 @@ config MACH_SMDKC110
>> Machine support for Samsung SMDKC110
>> S5PC110(MCP) is one of package option of S5PV210
>>
>> +config S5PV210_CPU_FREQ
>> + bool "S5PV210 CPU-FREQ Support"
>> + depends on CPU_S5PV210 && CPU_FREQ
>> +
>> endif
>> diff --git a/arch/arm/mach-s5pv210/Makefile
> b/arch/arm/mach-s5pv210/Makefile
>> index aae592a..72ca5ec 100644
>> --- a/arch/arm/mach-s5pv210/Makefile
>> +++ b/arch/arm/mach-s5pv210/Makefile
>> @@ -34,3 +34,5 @@ obj-$(CONFIG_S5PV210_SETUP_I2C2) += setup-i2c2.o
>> obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
>> obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
>> obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
>> +
>> +obj-$(CONFIG_S5PV210_CPU_FREQ) += cpufreq.o
>> diff --git a/arch/arm/mach-s5pv210/cpufreq.c
> b/arch/arm/mach-s5pv210/cpufreq.c
>> new file mode 100644
>> index 0000000..8ea1ce8
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv210/cpufreq.c
>> @@ -0,0 +1,783 @@
>> +/* linux/arch/arm/mach-s5pv210/cpufreq.c
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co., Ltd.
>> + *
>> + * CPU frequency scaling for S5PV210/S5PC110
>> + * Based on cpu-sa1110.c and s5pc11x-cpufreq.c
>> + *
>> + * 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/types.h>
>> +#include <linux/kernel.h>
>> +#include <linux/sched.h>
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/gpio.h>
>> +#include <asm/system.h>
>> +
>> +#include <mach/hardware.h>
>> +#include <mach/map.h>
>> +#include <mach/regs-clock.h>
>> +#include <mach/regs-gpio.h>
>> +#include <mach/cpu-freq.h>
>> +
>> +#include <plat/cpu-freq.h>
>> +#include <plat/pll.h>
>> +#include <plat/clock.h>
>> +#include <plat/gpio-cfg.h>
>> +#include <plat/regs-fb.h>
>> +#ifdef CONFIG_PM
>> +#include <plat/pm.h>
>> +#endif
>> +
>> +static struct clk *mpu_clk;
>> +static struct regulator *arm_regulator;
>> +static struct regulator *internal_regulator;
>> +
>> +struct s3c_cpufreq_freqs s3c_freqs;
>> +
>> +static unsigned long previous_arm_volt;
>> +
>> +static unsigned int backup_dmc0_reg;
>> +static unsigned int backup_dmc1_reg;
>> +static unsigned int backup_freq_level;
>> +static unsigned int mpll_freq; /* in MHz */
>> +static unsigned int apll_freq_max; /* in MHz */
>> +
>> +/* frequency */
>> +static struct cpufreq_frequency_table freq_table[] = {
>> + {L0, 1000*1000},
>> + {L1, 800*1000},
>> + {L2, 400*1000},
>> + {L3, 200*1000},
>> + {L4, 100*1000},
>> + {0, CPUFREQ_TABLE_END},
>> +};
>> +
>> +struct s5pv210_dvs_conf {
>> + unsigned long arm_volt; /* uV */
>> + unsigned long int_volt; /* uV */
>> +};
>> +
>> +const unsigned long arm_volt_max = 1350000;
>> +const unsigned long int_volt_max = 1250000;
>> +
>> +static struct s5pv210_dvs_conf dvs_conf_workaround[] = {
>
> Is this really workaround?
>
> This is just difference of voltage characteristic of CPU revision, not
> workaround.
When the user manual (even when EVT1 was not released) says it's 1.2V
for 1GHz and later we use 1.3V for 1GHz for the stability, it only
looks as if we are doing workaround for the stability of EVT0. That's
why I'm putting that name on it. However, changing the name of this
variable is nothing, so, it can be done with the next revision.
>
>> + [L0] = {
>> + .arm_volt = 1300000,
>> + .int_volt = 1200000,
>> + },
>> + [L1] = {
>> + .arm_volt = 1250000,
>> + .int_volt = 1200000,
>> +
>> + },
>> + [L2] = {
>> + .arm_volt = 1250000,
>> + .int_volt = 1200000,
>> +
>> + },
>> + [L3] = {
>> + .arm_volt = 1250000,
>> + .int_volt = 1200000,
>> + },
>> + [L4] = {
>> + .arm_volt = 1250000,
>> + .int_volt = 1200000,
>> + },
>> +};
>> +static struct s5pv210_dvs_conf dvs_conf_default[] = {
>> + [L0] = {
>> + .arm_volt = 1250000,
>> + .int_volt = 1100000,
>> + },
>> + [L1] = {
>> + .arm_volt = 1200000,
>> + .int_volt = 1100000,
>> + },
>> + [L2] = {
>> + .arm_volt = 1050000,
>> + .int_volt = 1100000,
>> + },
>> + [L3] = {
>> + .arm_volt = 950000,
>> + .int_volt = 1100000,
>> + },
>> + [L4] = {
>> + .arm_volt = 950000,
>> + .int_volt = 1000000,
>> + },
>> +};
>> +static struct s5pv210_dvs_conf *dvs_conf = dvs_conf_default;
>> +
>> +static u32 clkdiv_val[5][11] = {
>> + /*{ APLL, A2M, HCLK_MSYS, PCLK_MSYS,
>> + * HCLK_DSYS, PCLK_DSYS, HCLK_PSYS, PCLK_PSYS, ONEDRAM,
>> + * MFC, G3D }
>> + */
>> + /* L0 : [1000/200/200/100][166/83][133/66][200/200] */
>> + {0, 4, 4, 1, 3, 1, 4, 1, 3, 0, 0},
>> + /* L1 : [800/200/200/100][166/83][133/66][200/200] */
>> + {0, 3, 3, 1, 3, 1, 4, 1, 3, 0, 0},
>> + /* L2 : [400/200/200/100][166/83][133/66][200/200] */
>> + {1, 3, 1, 1, 3, 1, 4, 1, 3, 0, 0},
>> + /* L3 : [200/200/200/100][166/83][133/66][200/200] */
>> + {3, 3, 0, 1, 3, 1, 4, 1, 3, 0, 0},
>> + /* L4 : [100/100/100/100][83/83][66/66][100/100] */
>> + {7, 7, 0, 0, 7, 0, 9, 0, 7, 0, 0},
>> +};
>> +
>> +static struct s3c_freq clk_info[] = {
>> + [L0] = { /* L0: 1GHz */
>> + .fclk = 1000000,
>> + .armclk = 1000000,
>> + .hclk_tns = 0,
>> + .hclk = 133000,
>> + .pclk = 66000,
>> + .hclk_msys = 200000,
>> + .pclk_msys = 100000,
>> + .hclk_dsys = 166750,
>> + .pclk_dsys = 83375,
>> + },
>> + [L1] = { /* L1: 800MHz */
>> + .fclk = 800000,
>> + .armclk = 800000,
>> + .hclk_tns = 0,
>> + .hclk = 133000,
>> + .pclk = 66000,
>> + .hclk_msys = 200000,
>> + .pclk_msys = 100000,
>> + .hclk_dsys = 166750,
>> + .pclk_dsys = 83375,
>> + },
>> + [L2] = { /* L2: 400MHz */
>> + .fclk = 800000,
>> + .armclk = 400000,
>> + .hclk_tns = 0,
>> + .hclk = 133000,
>> + .pclk = 66000,
>> + .hclk_msys = 200000,
>> + .pclk_msys = 100000,
>> + .hclk_dsys = 166750,
>> + .pclk_dsys = 83375,
>> + },
>> + [L3] = { /* L3: 200MHz */
>> + .fclk = 800000,
>> + .armclk = 200000,
>> + .hclk_tns = 0,
>> + .hclk = 133000,
>> + .pclk = 66000,
>> + .hclk_msys = 200000,
>> + .pclk_msys = 100000,
>> + .hclk_dsys = 166750,
>> + .pclk_dsys = 83375,
>> + },
>> + [L4] = { /* L4: 100MHz */
>> + .fclk = 800000,
>> + .armclk = 100000,
>> + .hclk_tns = 0,
>> + .hclk = 66000,
>> + .pclk = 66000,
>> + .hclk_msys = 100000,
>> + .pclk_msys = 100000,
>> + .hclk_dsys = 83375,
>> + .pclk_dsys = 83375,
>> + },
>> +};
>> +
>> +static int s5pv210_verify_speed(struct cpufreq_policy *policy)
>> +{
>> +
>
> No need empty line.
Yes.
>
>> + if (policy->cpu)
>> + return -EINVAL;
>> +
>> + return cpufreq_frequency_table_verify(policy, freq_table);
>> +}
>> +
>> +static unsigned int s5pv210_getspeed(unsigned int cpu)
>> +{
>> + unsigned long rate;
>> +
>> + if (cpu)
>> + return 0;
>> +
>> + rate = clk_get_rate(mpu_clk) / 1000;
>> +
>> + return rate;
>> +}
>> +
>> +static void s5pv210_target_APLL2MPLL(unsigned int index,
>> + unsigned int bus_speed_changing)
>> +{
>> + unsigned int reg;
>> +
>> + /*
>> + * 1. Temporarily change divider for MFC and G3D
>> + * SCLKA2M(200/1=200)->(200/4=50)MHz
>> + */
>> + reg = __raw_readl(S5P_CLK_DIV2);
>> + reg &= ~(S5P_CLKDIV2_G3D_MASK | S5P_CLKDIV2_MFC_MASK);
>> + reg |= (0x3 << S5P_CLKDIV2_G3D_SHIFT) |
>> + (0x3 << S5P_CLKDIV2_MFC_SHIFT);
>
> How about following for reducing redundant definition?
> reg |= (0x3 << 0) | (0x3 << 4); /* G3D_SHIFT, MFC_SHIFT */
>
> Actually, you used same method in below...
> + } while (reg & ((1 << 16) | (1 << 17)));
>
We are accessing the same shift values with the same registers again
at another file (arch/arm/mach-s5pv210/clock.c). Isn't it normal to
use some name for such values? As long as we use the same names as in
clock.c (although clock.s is not patched to use names yet), they are
not defined "redundantly", aren't they?
>> + if (s5pv210_workaround()) {
>
> This is just from CPU revision...not workaround.
Yes, having no G2D is not "errata", but "feature not added". Do you
think a name such as "s5pv210_evt0_only" would fit? I think this name
would fit for both errata workaround code and feature-not-added.
>
>> + /*
>> + * Early version does NOT have G2D
>> + * Do Nothing about G2D.
>> + */
>> + } else {
>> + reg &= ~S5P_CLKDIV2_G2D_MASK;
>> + reg |= (0x2 << S5P_CLKDIV2_G2D_SHIFT);
>> + }
>> +
>> + __raw_writel(reg, S5P_CLK_DIV2);
>> +
>> + /* For MFC, G3D dividing */
>> + do {
>> + reg = __raw_readl(S5P_CLK_DIV_STAT0);
>> + } while (reg & ((1 << 16) | (1 << 17)));
>
>> +
>> + /*
>> + * 2. Change SCLKA2M(200MHz) to SCLKMPLL in MFC_MUX, G3D MUX
>> + * (100/4=50)->(667/4=166)MHz
>> + */
>> + reg = __raw_readl(S5P_CLK_SRC2);
>> + reg &= ~(S5P_CLKSRC2_G3D_MASK | S5P_CLKSRC2_MFC_MASK);
>> + reg |= (0x1 << S5P_CLKSRC2_G3D_SHIFT) |
>> + (0x1 << S5P_CLKSRC2_MFC_SHIFT);
>
>
>> + if (s5pv210_workaround()) {
>
> This is not workaround...just CPU revision...
>
>> + /*
>> + * Early version does NOT have G2D
>> + * Do Nothing about G2D.
>> + */
>> + } else {
>> + reg &= ~S5P_CLKSRC2_G2D_MASK;
>> + reg |= (0x1 << S5P_CLKSRC2_G2D_SHIFT);
>> + }
>> + __raw_writel(reg, S5P_CLK_SRC2);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_MUX_STAT1);
>> + } while (reg & ((1 << 7) | (1 << 3)));
>> +
>> + /* 3. msys: SCLKAPLL -> SCLKMPLL */
>> + reg = __raw_readl(S5P_CLK_SRC0);
>> + reg &= ~(S5P_CLKSRC0_MUX200_MASK);
>> + reg |= (0x1 << S5P_CLKSRC0_MUX200_SHIFT);
>> + __raw_writel(reg, S5P_CLK_SRC0);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_MUX_STAT0);
>> + } while (reg & (0x1 << 18));
>> +}
>> +
>> +static void s5pv210_target_MPLL2APLL(unsigned int index,
>> + unsigned int bus_speed_changing)
>> +{
>> + unsigned int reg;
>> +
>> + /*
>> + * 7. Set Lock time = 30us*24MHz = 02cf (EVT1)
>> + * WORKAROUND: Lock time = 300us*24Mhz = 7200(0x1c20)
>> + * Normal: Lock time = 30us*24Mhz = 0x2cf
>> + */
>> + if (s5pv210_workaround())
>
> Same....just EVT0...not workaround.
>
>> + __raw_writel(0x1c20, S5P_APLL_LOCK);
>> + else
>> + __raw_writel(0x2cf, S5P_APLL_LOCK);
>> +
>> + /*
>> + * 8. Turn on APLL
>> + * 8-1. Set PMS values
>> + * 8-3. Wait until the PLL is locked
>> + */
>> + if (index == L0)
>> + /* APLL FOUT becomes 1000 Mhz */
>> + __raw_writel(PLL45XX_APLL_VAL_1000, S5P_APLL_CON);
>> + else
>> + /* APLL FOUT becomes 800 Mhz */
>> + __raw_writel(PLL45XX_APLL_VAL_800, S5P_APLL_CON);
>> +
>> + do {
>> + reg = __raw_readl(S5P_APLL_CON);
>> + } while (!(reg & (0x1 << 29)));
>> +
>> + /*
>> + * 9. Change source clock from SCLKMPLL(667MHz)
>> + * to SCLKA2M(200MHz) in MFC_MUX and G3D_MUX
>> + * (667/4=166)->(200/4=50)MHz
>> + */
>> + reg = __raw_readl(S5P_CLK_SRC2);
>> + reg &= ~(S5P_CLKSRC2_G3D_MASK | S5P_CLKSRC2_MFC_MASK);
>> + reg |= (0 << S5P_CLKSRC2_G3D_SHIFT) | (0 <<
>> S5P_CLKSRC2_MFC_SHIFT);
>
>
>> + if (s5pv210_workaround()) {
>
> Same.
>
>> + /*
>> + * Early version does NOT have G2D
>> + * Do Nothing about G2D.
>> + */
>> + } else {
>> + reg &= ~S5P_CLKSRC2_G2D_MASK;
>> + reg |= 0x1 << S5P_CLKSRC2_G2D_SHIFT;
>> + }
>> + __raw_writel(reg, S5P_CLK_SRC2);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_MUX_STAT1);
>> + } while (reg & ((1 << 7) | (1 << 3)));
>> +
>> + /*
>> + * 10. Change divider for MFC and G3D
>> + * (200/4=50)->(200/1=200)MHz
>> + */
>> + reg = __raw_readl(S5P_CLK_DIV2);
>> + reg &= ~(S5P_CLKDIV2_G3D_MASK | S5P_CLKDIV2_MFC_MASK);
>> + reg |= (clkdiv_val[index][10] << S5P_CLKDIV2_G3D_SHIFT) |
>> + (clkdiv_val[index][9] << S5P_CLKDIV2_MFC_SHIFT);
>
>
>> + if (s5pv210_workaround()) {
>
> Same.
>
>> + /*
>> + * Early version does NOT have G2D
>> + * Do Nothing about G2D.
>> + */
>> + } else {
>> + reg &= ~S5P_CLKDIV2_G2D_MASK;
>> + reg |= 0x2 << S5P_CLKDIV2_G2D_SHIFT;
>> + }
>> + __raw_writel(reg, S5P_CLK_DIV2);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_DIV_STAT0);
>> + } while (reg & ((1 << 16) | (1 << 17)));
>> +
>> + /* 11. Change MPLL to APLL in MSYS_MUX */
>> + reg = __raw_readl(S5P_CLK_SRC0);
>> + reg &= ~(S5P_CLKSRC0_MUX200_MASK);
>> + reg |= (0x0 << S5P_CLKSRC0_MUX200_SHIFT); /* SCLKMPLL ->
>> SCLKAPLL */
>> + __raw_writel(reg, S5P_CLK_SRC0);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_MUX_STAT0);
>> + } while (reg & (0x1 << 18));
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int no_cpufreq_access;
>> +/*
>> + * s5pv210_target: relation has an additional symantics other than
>> + * the standard
>> + * [0x30]:
>> + * 1: disable further access to target until being re-enabled.
>> + * 2: re-enable access to target */
>> +#endif
>> +static int s5pv210_target(struct cpufreq_policy *policy,
>
> s5pv210_target what?...
I will rename names including this and the following ones.
>
>> + unsigned int target_freq,
>> + unsigned int relation)
>> +{
>> + static int initialized;
>> + int ret = 0;
>> + unsigned long arm_clk;
>> + unsigned int index, reg, arm_volt, int_volt;
>> + unsigned int pll_changing = 0;
>> + unsigned int bus_speed_changing = 0;
>> +
>> + cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, KERN_INFO,
>> + "cpufreq: Entering for %dkHz\n", target_freq);
>> +#ifdef CONFIG_PM
>> + if ((relation & ENABLE_FURTHER_CPUFREQ) &&
>> + (relation & DISABLE_FURTHER_CPUFREQ)) {
>> + /* Invalidate both if both marked */
>> + relation &= ~ENABLE_FURTHER_CPUFREQ;
>> + relation &= ~DISABLE_FURTHER_CPUFREQ;
>> + pr_err("%s:%d denied marking \"FURTHER_CPUFREQ\""
>> + " as both marked.\n",
>> + __FILE__, __LINE__);
>> + }
>> + if (relation & ENABLE_FURTHER_CPUFREQ)
>> + no_cpufreq_access = 0;
>> + if (no_cpufreq_access == 1) {
>> +#ifdef CONFIG_PM_VERBOSE
>> + pr_err("%s:%d denied access to %s as it is disabled"
>> + " temporarily\n", __FILE__, __LINE__,
> __func__);
>> +#endif
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + if (relation & DISABLE_FURTHER_CPUFREQ)
>> + no_cpufreq_access = 1;
>> + relation &= ~MASK_FURTHER_CPUFREQ;
>> +#endif
>> +
>> + s3c_freqs.freqs.old = s5pv210_getspeed(0);
>> +
>> + if (cpufreq_frequency_table_target(policy, freq_table,
>> + target_freq, relation, &index)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + arm_clk = freq_table[index].frequency;
>> +
>> + s3c_freqs.freqs.new = arm_clk;
>> + s3c_freqs.freqs.cpu = 0;
>> +
>> + /*
>> + * Run this function unconditionally until s3c_freqs.freqs.new
>> + * and s3c_freqs.freqs.old are both set by this function.
>> + */
>> + if (s3c_freqs.freqs.new == s3c_freqs.freqs.old &&
>> + initialized >= 2)
>> + return 0;
>> + else if (initialized < 2)
>> + initialized++;
>> +
>
> Where is initialization of 'initialized'?...
It's static. If we initialize it with 0, we get:
ERROR: do not initialise statics to 0 or NULL
#381: FILE: arm/mach-s5pv210/cpufreq.c:381:
+ static int initialized = 0;
total: 1 errors, 0 warnings, 783 lines checked
arch/arm/mach-s5pv210/cpufreq.c has style problems, please review. If
any of these errors
>
>> + arm_volt = dvs_conf[index].arm_volt;
>> + int_volt = dvs_conf[index].int_volt;
>> +
>> + /* iNew clock information update */
>
> 'iNew clock'?
> Hmm...I found this typo in the S.LSI's original code :-(
uh oh.. we've got so many copy-pasted codes.. :(
>
>> + memcpy(&s3c_freqs.new, &clk_info[index],
>> + sizeof(struct s3c_freq));
>> +
>> + if (s3c_freqs.freqs.new >= s3c_freqs.freqs.old) {
>> + /* Voltage up code: increase ARM first */
>> + if (!IS_ERR_OR_NULL(arm_regulator) &&
>> + !IS_ERR_OR_NULL(internal_regulator)) {
>> + regulator_set_voltage(arm_regulator,
>> + arm_volt, arm_volt_max);
>> + regulator_set_voltage(internal_regulator,
>> + int_volt, int_volt_max);
>> + }
>> + }
>> + cpufreq_notify_transition(&s3c_freqs.freqs, CPUFREQ_PRECHANGE);
>> +
>> + if (s3c_freqs.new.fclk != s3c_freqs.old.fclk || initialized < 2)
>> + pll_changing = 1;
>> +
>> + if (s3c_freqs.new.hclk_msys != s3c_freqs.old.hclk_msys ||
>> + initialized < 2)
>
> Hmm...why need 'initialized' checking?
I did it (counting the initializing executions of "target" function)
in order to guarantee s3c_freqs.new and s3c_freqs.old values correctly
reflect the status of CPU clocks as soon as possible because we do
_not_ have the guarantee that the bootloader has given the right
values for the clock settings (as in clkdiv_val[]). With this
checking, we forcibly set the clock settings as in clkdiv_val even if
the frequency is not being effectively changed; it is called when
CPUFREQ is loaded. If we do not check this, clock settings related
with clkdiv_val[] won't be initialized by CPUFREQ until the frequency
is changed from the previous one and both s3c_freqs.new and
s3c_freqs.old should reflect the actual settings. However, a slight
modification will let it do this in one cycle and I can do it with the
next revision (making initialized < 2 --> initialized < 1).
>
>> + bus_speed_changing = 1;
>> +
>> + /*
>> + * If ONEDRAM(DMC0)'s clock is getting slower, DMC0's
>> + * refresh counter should decrease before slowing down
>> + * DMC0 clock. We assume that DMC0's source clock never
>> + * changes. This is a temporary setting for the transition.
>> + * Stable setting is done at the end of this function.
>> + */
>
> Actually there are some S5PC110 MCP types which don't have ONEDRAM.
> ...so how can distinguish it? No need check it?
Is accessing DMC0's TIMINGAREF registers (0xF0000030) without the
existence of ONEDRAM problematic? In this patch, writing values of
DMC0 is based on the given (by bootloader) value of DMC0. It only sets
the refresh count of DMC0/DMC1. I'm not sure how the hardware would
work; however, I guess it would not be a problem if there exists no
corresponding ONEDRAM.
>
>> + reg = (__raw_readl(S5P_CLK_DIV6) & S5P_CLKDIV6_ONEDRAM_MASK)
>> + >> S5P_CLKDIV6_ONEDRAM_SHIFT;
>> + if (clkdiv_val[index][8] > reg) {
>> + reg = backup_dmc0_reg * (reg + 1) / (clkdiv_val[index][8] +
> 1);
>> + WARN_ON(reg > 0xFFFF);
>> + reg &= 0xFFFF;
>> + __raw_writel(reg, S5P_VA_DMC0 + 0x30);
>> + }
>> +
>> + /*
>> + * If hclk_msys (for DMC1) is getting slower, DMC1's
>> + * refresh counter should decrease before slowing down
>> + * hclk_msys in order to get rid of glitches in the
>> + * transition. This is temporary setting for the transition.
>> + * Stable setting is done at the end of this function.
>> + *
>> + * Besides, we need to consider the case when PLL speed changes,
>> + * where the DMC1's source clock hclk_msys is changed from ARMCLK
>> + * to MPLL temporarily. DMC1 needs to be ready for this
>> + * transition as well.
>> + */
>> + if (s3c_freqs.new.hclk_msys < s3c_freqs.old.hclk_msys) {
>> + /*
>> + * hclk_msys is up to 12bit. (200000)
>> + * reg is 16bit. so no overflow, yet.
>> + *
>> + * May need to use div64.h later with larger hclk_msys or
>> + * DMCx refresh counter. But, we have bugs in do_div and
>> + * that should be fixed before.
>> + */
>> + reg = backup_dmc1_reg * s3c_freqs.new.hclk_msys;
>> + reg /= clk_info[backup_freq_level].hclk_msys;
>> +
>> + /*
>> + * When ARM_CLK is absed on APLL->MPLL,
>> + * hclk_msys becomes hclk_msys *= MPLL/APLL;
>> + *
>> + * Based on the worst case scenario, we use MPLL/APLL_MAX
>> + * assuming that MPLL clock speed does not change.
>> + *
>> + * Multiplied first in order to reduce rounding error.
>> + * because reg has 15b length, using 64b should be enough to
>> + * prevent overflow.
>> + */
>> + if (pll_changing) {
>> + reg *= mpll_freq;
>> + reg /= apll_freq_max;
>> + }
>> + WARN_ON(reg > 0xFFFF);
>> + __raw_writel(reg & 0xFFFF, S5P_VA_DMC1 + 0x30);
>> + }
>> +
>> + /*
>> + * APLL should be changed in this level
>> + * APLL -> MPLL(for stable transition) -> APLL
>> + * Some clock source's clock API are not prepared. Do not use clock
>> API
>> + * in below code.
>> + */
>> + if (pll_changing)
>> + s5pv210_target_APLL2MPLL(index, bus_speed_changing);
>> +
>> + /* ARM MCS value changed */
>> + if (index != L4) {
>> + reg = __raw_readl(S5P_ARM_MCS_CON);
>> + reg &= ~0x3;
>> + reg |= 0x1;
>> + __raw_writel(reg, S5P_ARM_MCS_CON);
>> + }
>> +
>> + reg = __raw_readl(S5P_CLK_DIV0);
>> +
>> + reg &= ~(S5P_CLKDIV0_APLL_MASK | S5P_CLKDIV0_A2M_MASK
>> + | S5P_CLKDIV0_HCLK200_MASK |
>> S5P_CLKDIV0_PCLK100_MASK
>> + | S5P_CLKDIV0_HCLK166_MASK |
>> S5P_CLKDIV0_PCLK83_MASK
>> + | S5P_CLKDIV0_HCLK133_MASK |
>> S5P_CLKDIV0_PCLK66_MASK);
>> +
>> + reg |= ((clkdiv_val[index][0]<<S5P_CLKDIV0_APLL_SHIFT)
>> + | (clkdiv_val[index][1] << S5P_CLKDIV0_A2M_SHIFT)
>> + | (clkdiv_val[index][2] <<
>> S5P_CLKDIV0_HCLK200_SHIFT)
>> + | (clkdiv_val[index][3] <<
>> S5P_CLKDIV0_PCLK100_SHIFT)
>> + | (clkdiv_val[index][4] <<
>> S5P_CLKDIV0_HCLK166_SHIFT)
>> + | (clkdiv_val[index][5] << S5P_CLKDIV0_PCLK83_SHIFT)
>> + | (clkdiv_val[index][6] <<
>> S5P_CLKDIV0_HCLK133_SHIFT)
>> + | (clkdiv_val[index][7] <<
>> S5P_CLKDIV0_PCLK66_SHIFT));
>> +
>> + __raw_writel(reg, S5P_CLK_DIV0);
>> +
>> + do {
>> + reg = __raw_readl(S5P_CLK_DIV_STAT0);
>> + } while (reg & 0xff);
>> +
>> + /* ARM MCS value changed */
>> + if (index == L4) {
>> + reg = __raw_readl(S5P_ARM_MCS_CON);
>> + reg &= ~0x3;
>> + reg |= 0x3;
>> + __raw_writel(reg, S5P_ARM_MCS_CON);
>> + }
>> +
>> + if (pll_changing)
>> + s5pv210_target_MPLL2APLL(index, bus_speed_changing);
>> +
>> + /*
>> + * Adjust DMC0 refresh ratio according to the rate of DMC0
>> + * The DIV value of DMC0 clock changes and SRC value is not
> controlled.
>> + * We assume that no one changes SRC value of DMC0 clock, either.
>> + */
>> + reg = __raw_readl(S5P_CLK_DIV6);
>> + reg &= ~S5P_CLKDIV6_ONEDRAM_MASK;
>> + reg |= (clkdiv_val[index][8] << S5P_CLKDIV6_ONEDRAM_SHIFT);
>> + /* ONEDRAM(DMC0) Clock Divider Ratio: 7+1 for L4, 3+1 for Others */
>> + __raw_writel(reg, S5P_CLK_DIV6);
>> + do {
>> + reg = __raw_readl(S5P_CLK_DIV_STAT1);
>> + } while (reg & (1 << 15));
>> +
>> + /*
>> + * If DMC0 clock gets slower (by orginal clock speed / n),
>> + * then, the refresh rate should decrease
>> + * (by original refresh count / n) (n: divider)
>> + */
>> + reg = backup_dmc0_reg * (clkdiv_val[backup_freq_level][8] + 1)
>> + / (clkdiv_val[index][8] + 1);
>> + __raw_writel(reg & 0xFFFF, S5P_VA_DMC0 + 0x30);
>> +
>> + /*
>> + * Adjust DMC1 refresh ratio according to the rate of hclk_msys
>> + * (L0~L3: 200 <-> L4: 100)
>> + * If DMC1 clock gets slower (by original clock speed * n),
>> + * then, the refresh rate should decrease
>> + * (by original refresh count * n) (n : clock rate)
>> + */
>> + reg = backup_dmc1_reg * clk_info[index].hclk_msys;
>> + reg /= clk_info[backup_freq_level].hclk_msys;
>> + __raw_writel(reg & 0xFFFF, S5P_VA_DMC1 + 0x30);
>> +
>> + if (s3c_freqs.freqs.new < s3c_freqs.freqs.old) {
>> + /* Voltage down: decrease INT first.*/
>> + if (!IS_ERR_OR_NULL(arm_regulator) &&
>> + !IS_ERR_OR_NULL(internal_regulator)) {
>> + regulator_set_voltage(internal_regulator,
>> + int_volt, int_volt_max);
>> + regulator_set_voltage(arm_regulator,
>> + arm_volt, arm_volt_max);
>> + }
>> + }
>> + cpufreq_notify_transition(&s3c_freqs.freqs, CPUFREQ_POSTCHANGE);
>> +
>> + memcpy(&s3c_freqs.old, &s3c_freqs.new, sizeof(struct s3c_freq));
>> + cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, KERN_INFO,
>> + "cpufreq: Performance changed[L%d]\n", index);
>> + previous_arm_volt = dvs_conf[index].arm_volt;
>> +out:
>> + return ret;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int previous_frequency;
>> +
>> +static int s5pv210_cpufreq_suspend(struct cpufreq_policy *policy,
>> + pm_message_t pmsg)
>> +{
>> + int ret = 0;
>> + pr_info("cpufreq: Entering suspend.\n");
>> +
>> + previous_frequency = cpufreq_get(0);
>> + ret = __cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ,
>> + DISABLE_FURTHER_CPUFREQ);
>> + return ret;
>> +}
>> +
>> +static int s5pv210_cpufreq_resume(struct cpufreq_policy *policy)
>> +{
>> + int ret = 0;
>> + u32 rate;
>> + int level = CPUFREQ_TABLE_END;
>> + int i;
>
> Should be 'int i = 0;' for removing below 'i = 0;'
Ok. will do.
>
>> +
>> + pr_info("cpufreq: Waking up from a suspend.\n");
>> +
>> + __cpufreq_driver_target(cpufreq_cpu_get(0), previous_frequency,
>> + ENABLE_FURTHER_CPUFREQ);
>> +
>> + /* Clock information update with wakeup value */
>> + rate = clk_get_rate(mpu_clk);
>> +
>> + i = 0;
>
> ...
>
>> + while (freq_table[i].frequency != CPUFREQ_TABLE_END) {
>> + if (freq_table[i].frequency * 1000 == rate) {
>> + level = freq_table[i].index;
>> + break;
>> + }
>> + i++;
>> + }
>> +
>> + if (level == CPUFREQ_TABLE_END) { /* Not found */
>> + pr_err("[%s:%d] clock speed does not match: "
>> + "%d. Using L1 of 800MHz.\n",
>> + __FILE__, __LINE__, rate);
>> + level = L1;
>> + }
>> +
>> + memcpy(&s3c_freqs.old, &clk_info[level],
>> + sizeof(struct s3c_freq));
>> + previous_arm_volt = dvs_conf[level].arm_volt;
>> + return ret;
>> +}
>> +#endif
>> +
>> +static int __init s5pv210_cpu_init(struct cpufreq_policy *policy)
>
> s5pv210_cpu_init?...is this function cpu initialization?..No!
> hmm...s5pv210_cpufreq_init() is better...but used at below...
>
> ...need another name for easily reading
sure.
>
>> +{
>> + u32 rate ;
>> + int i, level = CPUFREQ_TABLE_END;
>> + struct clk *mpll_clk;
>> +
>> + pr_info("S5PV210 CPUFREQ Initialising...\n");
>> +#ifdef CONFIG_PM
>> + no_cpufreq_access = 0;
>> +#endif
>> + mpu_clk = clk_get(NULL, "armclk");
>> + if (IS_ERR(mpu_clk)) {
>> + pr_err("S5PV210 CPUFREQ cannot get armclk\n");
>> + return PTR_ERR(mpu_clk);
>> + }
>> +
>> + if (policy->cpu != 0) {
>> + pr_err("S5PV210 CPUFREQ cannot get proper cpu(%d)\n",
>> + policy->cpu);
>> + return -EINVAL;
>> + }
>> + policy->cur = policy->min = policy->max = s5pv210_getspeed(0);
>> +
>> + cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
>> +
>> + policy->cpuinfo.transition_latency = 40000; /* 1us */
>> +
>> + rate = clk_get_rate(mpu_clk);
>> + i = 0;
>> +
> i = 0?
>
>> + while (freq_table[i].frequency != CPUFREQ_TABLE_END) {
>> + if (freq_table[i].frequency * 1000 == rate) {
>> + level = freq_table[i].index;
>> + break;
>> + }
>> + i++;
>> + }
>> +
>> + if (level == CPUFREQ_TABLE_END) { /* Not found */
>> + pr_err("[%s:%d] clock speed does not match: "
>> + "%d. Using L1 of 800MHz.\n",
>> + __FILE__, __LINE__, rate);
>> + level = L1;
>> + }
>> +
>> + backup_dmc0_reg = __raw_readl(S5P_VA_DMC0 + 0x30) & 0xFFFF;
>> + backup_dmc1_reg = __raw_readl(S5P_VA_DMC1 + 0x30) & 0xFFFF;
>> + backup_freq_level = level;
>> + mpll_clk = clk_get(NULL, "mout_mpll");
>> + mpll_freq = clk_get_rate(mpll_clk) / 1000 / 1000; /* in MHz */
>> + clk_put(mpll_clk);
>> + i = 0;
>> + do {
>> + int index = freq_table[i].index;
>> + if (apll_freq_max < clk_info[index].fclk)
>> + apll_freq_max = clk_info[index].fclk;
>> + i++;
>> + } while (freq_table[i].frequency != CPUFREQ_TABLE_END);
>> + apll_freq_max /= 1000; /* in MHz */
>> +
>> + memcpy(&s3c_freqs.old, &clk_info[level],
>> + sizeof(struct s3c_freq));
>> + previous_arm_volt = dvs_conf[level].arm_volt;
>> +
>> + return cpufreq_frequency_table_cpuinfo(policy, freq_table);
>> +}
>> +
>> +static struct cpufreq_driver s5pv210_driver = {
>
> s5pv210_cpufreq_driver...
>
>> + .flags = CPUFREQ_STICKY,
>> + .verify = s5pv210_verify_speed,
>> + .target = s5pv210_target,
>> + .get = s5pv210_getspeed,
>> + .init = s5pv210_cpu_init,
>> + .name = "s5pv210",
>> +#ifdef CONFIG_PM
>> + .suspend = s5pv210_cpufreq_suspend,
>> + .resume = s5pv210_cpufreq_resume,
>> +#endif
>> +};
>> +
>> +static int __init s5pv210_cpufreq_init(void)
>> +{
>> + if (s5pv210_workaround())
>
> Same...just CPU revision...not workaround...
>
>> + dvs_conf = dvs_conf_workaround;
>> +
>> + arm_regulator = regulator_get_exclusive(NULL, "vddarm");
>> + if (IS_ERR(arm_regulator)) {
>> + pr_err("failed to get regulater resource vddarm\n");
>> + goto error;
>> + }
>> + internal_regulator = regulator_get_exclusive(NULL, "vddint");
>> + if (IS_ERR(internal_regulator)) {
>> + pr_err("failed to get regulater resource vddint\n");
>> + goto error;
>> + }
>> + goto finish;
>> +error:
>> + pr_warn("Cannot get vddarm or vddint. CPUFREQ Will not"
>> + " change the voltage.\n");
>> +finish:
>> + return cpufreq_register_driver(&s5pv210_driver);
>
> s5pv210_cpufreq_driver...
>
>> +}
>> +
>> +late_initcall(s5pv210_cpufreq_init);
>> diff --git a/arch/arm/mach-s5pv210/include/mach/cpu-freq.h
> b/arch/arm/mach-
>> s5pv210/include/mach/cpu-freq.h
>> new file mode 100644
>> index 0000000..e1a1c0c
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv210/include/mach/cpu-freq.h
>> @@ -0,0 +1,40 @@
>> +/* arch/arm/mach-s5pv210/include/mach/cpu-freq.h
>> + *
>> + * Copyright (c) 2010 Samsung Electronics
>> + *
>> + * MyungJoo Ham <myungjoo.ham at samsung.com>
>> + *
>> + * S5PV210/S5PC110 CPU frequency scaling support
>> + *
>> + * 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_CPU_FREQ_H
>> +#define __ASM_ARCH_CPU_FREQ_H
>> +
>> +#include <linux/cpufreq.h>
>> +
>> +#define USE_FREQ_TABLE
>
> Where is this used?
None. will be removed.
>
>> +
>> +enum perf_level {
>> + L0 = 0,
>> + L1,
>> + L2,
>> + L3,
>> + L4,
>> +};
>> +
>> +#ifdef CONFIG_PM
>> +#define SLEEP_FREQ (800 * 1000) /* Use 800MHz when entering sleep */
>> +
>> +/* additional symantics for "relation" in cpufreq with pm */
>> +#define DISABLE_FURTHER_CPUFREQ 0x10
>> +#define ENABLE_FURTHER_CPUFREQ 0x20
>> +#define MASK_FURTHER_CPUFREQ 0x30
>> +/* With 0x00(NOCHANGE), it depends on the previous "further" status */
>> +
>> +#endif /* CONFIG_PM */
>> +
>> +#endif /* __ASM_ARCH_CPU_FREQ_H */
>> --
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim at samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
More information about the linux-arm-kernel
mailing list