[PATCH 5/5] ARM: S5PV210: Initial CPUFREQ Support
MyungJoo Ham
myungjoo.ham at samsung.com
Fri Jul 16 01:37:04 EDT 2010
On Thu, Jul 15, 2010 at 8:59 PM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
> On Thu, Jul 15, 2010 at 06:06:08PM +0900, MyungJoo Ham wrote:
>
>> +/* Based on MAX8998 default RAMP time: 10mV/us */
>> +#define PMIC_RAMP_UP (10)
>
> This should be handled within the regulator API, not by hard coding
> numbers into the CPUfreq drivers. With many regulators there will be no
> need at all for a ramp delay, they can ramp effectively instantaneously.
> Doing this through the regulator API avoids hard coding details of an
> individual regulator in your driver and ensures that all users of this
> regulator can handle the ramp rate.
>
> I did have most of a patch to introduce this, I'll go and dust it off
> soon. You should just be able to remove the ramp rate code from the
> CPUfreq driver.
Making regulator API wait for the ramp up seems to be something like
"synchronous I/O", which in turn, forces CPU to do nothing while CPU
may execute some instructions while waiting for the ramp up. However,
we just do udelay() anyway; thus, it doesn't matter for us as well.
I'll try to put those udelay into MAX8998 PMIC drivers, which is for
S5PV210. I'll remove these udelay and PMIC_RAMP)UP from cpufreq driver
with the next version.
>
>> + if (s3c_freqs.freqs.new >= s3c_freqs.freqs.old) {
>> + /* Voltage up code: increase ARM first */
>> + if (!IS_ERR(arm_regulator) && !IS_ERR(internal_regulator)) {
>> + regulator_set_voltage(arm_regulator,
>> + arm_volt, arm_volt);
>> + regulator_set_voltage(internal_regulator,
>> + int_volt, int_volt);
>> + }
>
> You shouldn't be specifying an exact voltage here - there's no guarantee
> that the exact voltage you request will be delivered by the regulator,
> and some boards may wish to use constraints to eliminate some voltages
> (eg, to restrict the maximum operating point for the CPU).
>
> What you'd normally have here is that the minimum voltage would be the
> desired operating point for the CPU and the maximum voltage would be
> fixed at the maximum rated supply for the part. This will allow the
> regulator API to select the lowest voltage it is able to deliver.
Currently, there is a bug in MAX8998 PMIC driver that sets VDDARM to
minimum possible voltage that is LARGER THAN or EQUAL TO max_vol, not
min_vol. I'll correct set_voltage usage of CPUFREQ assuming that
bugfix for MAX8998 PMIC driver is sent. (Joonyoung Shim, who's just
started submitting MAX8998 PMIC driver, know about this issue and will
probably send a patch soon.)
Modifying set_voltage usage of CPUFREQ as you've told will also make
the driver compatible to AVS; I'm working on AVS - IEM as well. :)
>
>> +#ifdef CONFIG_REGULATOR
>> + arm_regulator = regulator_get_exclusive(NULL, "vddarm");
>> + if (IS_ERR(arm_regulator)) {
>> + printk(KERN_ERR "failed to get regulater resource vddarm\n");
>> + goto error;
>> + }
>> + internal_regulator = regulator_get_exclusive(NULL, "vddint");
>> + if (IS_ERR(internal_regulator)) {
>> + printk(KERN_ERR "failed to get regulater resource vddint\n");
>> + goto error;
>> + }
>> +#endif
>
> Remove these ifdefs, they're not needed - the regulator API will stub
> out the regulators. You should also have code to check that the voltage
> ranges for all the operating modes of the CPU can be supported. This is
> particularly important the way you've got the code at the minute
> specifying exact voltages.
Got it.
>
>> +#if defined(CONFIG_REGULATOR)
>> +#ifdef CONFIG_REGULATOR_MAX8998
>> + reg = __raw_readl(S5P_CLK_OUT);
>> + reg &= ~(0x1f << 12 | 0xf << 20); /* CLKSEL and DIVVAL*/
>> + apllreg = __raw_readl(S5P_APLL_CON);
>
> This ifdef for a particular regulator looks *very* suspicious. What's
> going on here?
Good point. That ifdefs should be removed.
Besides, the whole section enclosed by this CONFIG_REGULATOR will be removed.
A while ago, WM8994 had been using CLK_OUT as the clock source with
APLL dependent setting; thus "target" function accessed CLK_OUT when
APLL changed. However, it no longer uses APLL dependent clock source
and we no longer access CLK_OUT at the "target" function; thus, this
code in "init" function can be removed.
>
>> +#ifdef CPUFREQ_DISABLE_1GHZ
>> + reg |= (0x0 << 12 | 0x9 << 20); /* (800/4) / (9+1) = 20 */
>> +#else
>> + if (((apllreg << 16) & 0x3ff) == 0xfa) /* APLL=1GHz */
>> + reg |= (0x0 << 12 | 0xb << 20); /* (1000/4) / (11+1) = 20.833 */
>> + else /* APLL=800MHz */
>> + reg |= (0x0 << 12 | 0x9 << 20); /* (800/4) / (9+1) = 20 */
>> +#endif
>> + __raw_writel(reg, S5P_CLK_OUT);
>> +#endif
>> +#endif
>> +
>> + if (policy->cpu != 0) {
>> + printk(KERN_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(s5pv210_freq_table, policy->cpu);
>> +
>> + policy->cpuinfo.transition_latency = 40000; /*1us*/
>> +
>> + rate = clk_get_rate(mpu_clk);
>> + i = 0;
>> +
>> + while (s5pv210_freq_table[i].frequency != CPUFREQ_TABLE_END) {
>> + if (s5pv210_freq_table[i].frequency * 1000 == rate) {
>> + level = s5pv210_freq_table[i].index;
>> + break;
>> + }
>> + i++;
>> + }
>> +
>> + if (level == CPUFREQ_TABLE_END) { /* Not found */
>> + printk(KERN_ERR "[%s:%d] clock speed does not match: "
>> + "%d. Using L1 of 800MHz.\n",
>> + __FILE__, __LINE__, rate);
>> + level = L1;
>> + }
>> +
>> + printk(KERN_INFO "S5PV210 CPUFREQ Initialized.\n");
>> +
>> + memcpy(&s3c_freqs.old, &s5pv210_clk_info[level],
>> + sizeof(struct s3c_freq));
>> + previous_arm_volt = s5pv210_dvs_conf[level].arm_volt;
>> +
>> + return cpufreq_frequency_table_cpuinfo(policy, s5pv210_freq_table);
>> +}
>> +
>> +static struct cpufreq_driver s5pv210_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)
>> +{
>> + printk(KERN_INFO "S5PV210 CPUFREQ Init.\n");
>> +#ifdef CONFIG_REGULATOR
>> + arm_regulator = regulator_get_exclusive(NULL, "vddarm");
>> + if (IS_ERR(arm_regulator)) {
>> + printk(KERN_ERR "failed to get regulater resource vddarm\n");
>> + goto error;
>> + }
>> + internal_regulator = regulator_get_exclusive(NULL, "vddint");
>> + if (IS_ERR(internal_regulator)) {
>> + printk(KERN_ERR "failed to get regulater resource vddint\n");
>> + goto error;
>> + }
>> +#endif
>> + goto finish;
>> +error:
>> + printk(KERN_WARNING "Cannot get vddarm or vddint. CPUFREQ Will not"
>> + " change the voltage.\n");
>> +finish:
>> + return cpufreq_register_driver(&s5pv210_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..d3c31b2
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv210/include/mach/cpu-freq.h
>> @@ -0,0 +1,50 @@
>> +/* 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 _ARCH_ARM_MACH_S5PV210_INCLUDE_MACH_CPU_FREQ_H_
>> +#define _ARCH_ARM_MACH_S5PV210_INCLUDE_MACH_CPU_FREQ_H_
>> +
>> +#include <linux/cpufreq.h>
>> +
>> +#ifdef CONFIG_CPU_S5PV210
>> +
>> +#define USE_FREQ_TABLE
>> +
>> +#define KHZ_T 1000
>> +
>> +#define MPU_CLK "armclk"
>> +
>> +enum perf_level {
>> + L0 = 0,
>> + L1,
>> + L2,
>> + L3,
>> + L4,
>> +};
>> +
>> +#define CLK_DIV0_MASK ((0x7<<0)|(0x7<<8)|(0x7<<12)) /* APLL,HCLK_MSYS,PCLK_MSYS mask value */
>> +
>> +#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
>> +
>> +
>> +#endif /* CONFIG_CPU_S5PV210 */
>> +#endif /* _ARCH_ARM_MACH_S5PV210_INCLUDE_MACH_CPU_FREQ_H_ */
>> --
>> 1.6.3.3
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> --
> "You grabbed my hand and we fell into it, like a daydream - or a fever."
>
Thanks!
--
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