[PATCH 5/5] ARM: S5PV210: Initial CPUFREQ Support
MyungJoo Ham
myungjoo.ham at samsung.com
Fri Jul 16 02:23:20 EDT 2010
Dear Marek,
On Fri, Jul 16, 2010 at 2:42 PM, Kyungmin Park
<kyungmin.park at samsung.com> wrote:
> On Fri, Jul 16, 2010 at 2:37 PM, MyungJoo Ham <myungjoo.ham at samsung.com> wrote:
>> 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.)
>
> Marek submitted the initial max8998 codes. and joonyoung implement the
> max8998-rtc.
>
> just explain the bug to marek.
>
> Thank you,
> Kyungmin Park
As mentioned in this thread, we need some corrections in the MAX8998
regulator driver.
1. max8998_set_voltage(rdev, min_uV, max_uV) max/min value
In the driver, the voltage value set by this function should be the
minimum possible value that is larger than min_uV. However, currently,
it sets the minimum possible value that is larger than or equal to
max_uV, not min_uV. Please correct this one.
2. max8998_set_voltage(rdev, min_uV, max_uV) ramp up delay
Depending on which rdev we use and RAMP_UP register (including the EN
bit) set_voltage, max8998_set_voltage can execute udelay()
accordingly.
Do you plan to keep updating MAX8998 driver from the old kernel
versions (such as 2.6.32 or 29)? We still have many missing features
in MAX8998 driver with mainline.
>
>>
>> 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
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Cheers!
- MyungJoo
--
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