[PATCH] cpufreq for freescale mx51

[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

Add'ed linaro-dev and linux-arm-kernel to CC.

Thanks Yong, some feeback follows inline.

On 10 Sep 29, Yong Shen wrote:
> From: Yong Shen <yong.shen@linaro.org>
> 
> ---
>  arch/arm/Kconfig                       |    6 +
>  arch/arm/mach-mx5/Kconfig              |    1 +
>  arch/arm/mach-mx5/board-mx51_babbage.c |   32 ++++
>  arch/arm/mach-mx5/clock-mx51.c         |   53 ++++++
>  arch/arm/plat-mxc/Makefile             |    2 +
>  arch/arm/plat-mxc/cpufreq.c            |  282 ++++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++
>  7 files changed, 396 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/plat-mxc/cpufreq.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 4db064e..64ebbc0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1517,6 +1517,12 @@ if ARCH_HAS_CPUFREQ
>  
>  source "drivers/cpufreq/Kconfig"
>  
> +config CPU_FREQ_IMX
> +	tristate "CPUfreq driver for i.MX CPUs"
> +	depends on ARCH_MXC && CPU_FREQ
> +	help
> +	This enables the CPUfreq driver for i.MX CPUs.
> +
>  config CPU_FREQ_SA1100
>  	bool
>  
> diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> index 1576d51..5956fee 100644
> --- a/arch/arm/mach-mx5/Kconfig
> +++ b/arch/arm/mach-mx5/Kconfig
> @@ -5,6 +5,7 @@ config ARCH_MX51
>  	default y
>  	select MXC_TZIC
>  	select ARCH_MXC_IOMUX_V3
> +	select ARCH_HAS_CPUFREQ
>  
>  comment "MX5 platforms:"
>  
> diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
> index ed885f9..e449e0b 100644
> --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> @@ -43,6 +43,31 @@
>  #define	MX51_USB_PLL_DIV_19_2_MHZ	0x01
>  #define	MX51_USB_PLL_DIV_24_MHZ	0x02
>  
> +struct cpu_wp *(*get_cpu_wp)(int *wp);
> +static int num_cpu_wp = 2;

use sizeof(array) instead of hard coding this.

> +/* working point(wp): 0 - 800MHz; 1 - 166.25MHz; */
> +static struct cpu_wp cpu_wp_auto[] = {
> +	{
> +	.pll_rate = 800000000,
> +	.cpu_rate = 800000000,
> +	.pdf = 0,
> +	.mfi = 8,
> +	.mfd = 2,
> +	.mfn = 1,
> +	.cpu_podf = 0,
> +	.cpu_voltage = 1100000,},
> +	{
> +	.pll_rate = 800000000,
> +	.cpu_rate = 160000000,
> +	.pdf = 4,
> +	.mfi = 8,
> +	.mfd = 2,
> +	.mfn = 1,
> +	.cpu_podf = 4,
> +	.cpu_voltage = 850000,},
> +};


This data should be moved out to a separate file (e.g. mx51_ratetable.h)
since it will be useful to other boards too.

If other boards can have different rate tables (and they can, depending on
the revision of the silicon), then we can either 'assemble' the correct rate
table based on a flag field or have explicit separate rate tables for each
silicon revision.

In any case, I suspect that there will be some core rates that will be common
across silicon revisions.

>  static struct platform_device *devices[] __initdata = {
>  	&mxc_fec_device,
>  };
> @@ -87,6 +112,12 @@ static struct imxuart_platform_data uart_pdata = {
>  	.flags = IMXUART_HAVE_RTSCTS,
>  };
>  
> +struct cpu_wp *mx51_babbage_get_cpu_wp(int *wp)
> +{
> +	*wp = num_cpu_wp;
> +	return cpu_wp_auto;
> +}
> +
>  static inline void mxc_init_imx_uart(void)
>  {
>  	mxc_register_device(&mxc_uart_device0, &uart_pdata);
> @@ -246,6 +277,7 @@ static void __init mxc_board_init(void)
>  
>  static void __init mx51_babbage_timer_init(void)
>  {
> +	get_cpu_wp = mx51_babbage_get_cpu_wp;
>  	mx51_clocks_init(32768, 24000000, 22579200, 0);
>  }
>  
> diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
> index d9f612d..f2488e6 100644
> --- a/arch/arm/mach-mx5/clock-mx51.c
> +++ b/arch/arm/mach-mx5/clock-mx51.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/time.h>
>  
>  #include <asm/clkdev.h>
>  #include <asm/div64.h>
> @@ -28,6 +29,11 @@
>  static unsigned long external_high_reference, external_low_reference;
>  static unsigned long oscillator_reference, ckih2_reference;
>  
> +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> +static int cpu_wp_nr;
> +static int cpu_curr_wp;
> +static struct cpu_wp *cpu_wp_tbl;
> +
>  static struct clk osc_clk;
>  static struct clk pll1_main_clk;
>  static struct clk pll1_sw_clk;
> @@ -38,7 +44,9 @@ static struct clk periph_apm_clk;
>  static struct clk ahb_clk;
>  static struct clk ipg_clk;
>  static struct clk usboh3_clk;
> +static void __iomem *pll1_base;
>  
> +#define SPIN_DELAY	1000000 /* in nanoseconds */
>  #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
>  
>  static int _clk_ccgr_enable(struct clk *clk)
> @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
>  	return 0;
>  }
>  
> +/*!
> + * Setup cpu clock based on working point.
> + * @param	wp	cpu freq working point
> + * @return		0 on success or error code on failure.
> + */
> +int cpu_clk_set_wp(int wp)
> +{
> +	struct cpu_wp *p;
> +	u32 reg;
> +
> +	if (wp == cpu_curr_wp)
> +		return 0;
> +
> +	p = &cpu_wp_tbl[wp];
> +
> +	/*use post divider to change freq
> +	 */
> +	reg = __raw_readl(MXC_CCM_CACRR);
> +	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> +	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> +	__raw_writel(reg, MXC_CCM_CACRR);
> +	cpu_curr_wp = wp;
> +
> +	return 0;
> +}
> +
>  static unsigned long clk_arm_get_rate(struct clk *clk)
>  {
>  	u32 cacrr, div;
> @@ -342,6 +376,20 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
>  	return parent_rate / div;
>  }
>  
> +int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	u32 i;
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		if (rate == cpu_wp_tbl[i].cpu_rate)
> +			break;
> +	}
> +	if (i >= cpu_wp_nr)
> +		return -EINVAL;
> +	cpu_clk_set_wp(i);
> +
> +	return 0;
> +}
> +
>  static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
>  {
>  	u32 reg, mux;
> @@ -694,6 +742,7 @@ static struct clk periph_apm_clk = {
>  static struct clk cpu_clk = {
>  	.parent = &pll1_sw_clk,
>  	.get_rate = clk_arm_get_rate,
> +	.set_rate = _clk_cpu_set_rate,
>  };
>  
>  static struct clk ahb_clk = {
> @@ -821,6 +870,7 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("mxc-ehci.1", "usb_ahb", ahb_clk)
>  	_REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
>  	_REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
> +	_REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
>  };
>  
>  static void clk_tree_init(void)
> @@ -848,10 +898,13 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
>  {
>  	int i;
>  
> +	pll1_base = ioremap(MX51_PLL1_BASE_ADDR, SZ_4K);
> +
>  	external_low_reference = ckil;
>  	external_high_reference = ckih1;
>  	ckih2_reference = ckih2;
>  	oscillator_reference = osc;
> +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
>  
>  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
>  		clkdev_add(&lookups[i]);
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index 895bc3c..c1bb400 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -17,6 +17,8 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci.o
>  obj-$(CONFIG_MXC_ULPI) += ulpi.o
>  obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
>  obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
> +# CPU FREQ support
> +obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
>  ifdef CONFIG_SND_IMX_SOC
>  obj-y += ssi-fiq.o
>  obj-y += ssi-fiq-ksym.o
> diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
> new file mode 100644
> index 0000000..cae40f1
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpufreq.c
> @@ -0,0 +1,282 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*!
> + * @file cpufreq.c
> + *
> + * @brief A driver for the Freescale Semiconductor i.MXC CPUfreq module.
> + *
> + * The CPUFREQ driver is for controling CPU frequency. It allows you to change
> + * the CPU clock speed on the fly.
> + *
> + * @ingroup PM
> + */

Fix these comments to follow the kernel commenting style.

> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <mach/hardware.h>
> +#include <asm/setup.h>
> +#include <mach/clock.h>
> +#include <asm/cacheflush.h>
> +#include <linux/hrtimer.h>
> +
> +int cpu_freq_khz_min;
> +int cpu_freq_khz_max;
> +int arm_lpm_clk;
> +int arm_normal_clk;
> +int cpufreq_suspended;
> +int cpufreq_trig_needed;
> +
> +static struct clk *cpu_clk;
> +static struct cpufreq_frequency_table imx_freq_table[4];
> +
> +extern int set_low_bus_freq(void);
> +extern int set_high_bus_freq(int high_bus_speed);
> +extern int low_freq_bus_used(void);
> +
> +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> +static int cpu_wp_nr;
> +static struct cpu_wp *cpu_wp_tbl;
> +
> +int set_cpu_freq(int freq)
> +{
> +	int ret = 0;
> +	int org_cpu_rate;
> +	int gp_volt = 0;
> +	int i;
> +
> +	org_cpu_rate = clk_get_rate(cpu_clk);
> +	if (org_cpu_rate == freq)
> +		return ret;
> +
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		if (freq == cpu_wp_tbl[i].cpu_rate)
> +			gp_volt = cpu_wp_tbl[i].cpu_voltage;
> +	}
> +
> +	if (gp_volt == 0)
> +		return ret;
> +	
> +	ret = clk_set_rate(cpu_clk, freq);
> +	if (ret != 0) {
> +		printk(KERN_DEBUG "cannot set CPU clock rate\n");
> +		return ret;
> +	}
> +	
> +	return ret;
> +}
> +
> +static int mxc_verify_speed(struct cpufreq_policy *policy)
> +{
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	return cpufreq_frequency_table_verify(policy, imx_freq_table);
> +}
> +
> +static unsigned int mxc_get_speed(unsigned int cpu)
> +{
> +	if (cpu)
> +		return 0;
> +
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int calc_frequency_khz(int target, unsigned int relation)
> +{
> +	int i;
> +
> +	if ((target * 1000) == clk_get_rate(cpu_clk))
> +		return target;
> +
> +	if (relation == CPUFREQ_RELATION_H) {
> +		for (i = cpu_wp_nr - 1; i >= 0; i--) {
> +			if (imx_freq_table[i].frequency <= target)
> +				return imx_freq_table[i].frequency;
> +		}
> +	} else if (relation == CPUFREQ_RELATION_L) {
> +		for (i = 0; i < cpu_wp_nr; i++) {
> +			if (imx_freq_table[i].frequency >= target)
> +				return imx_freq_table[i].frequency;
> +		}
> +	}
> +	printk(KERN_ERR "Error: No valid cpufreq relation\n");
> +	return cpu_freq_khz_max;
> +}
> +
> +static int mxc_set_target(struct cpufreq_policy *policy,
> +			  unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	int freq_Hz;
> +	int ret = 0;
> +
> +	if (cpufreq_suspended) {
> +		target_freq = clk_get_rate(cpu_clk) / 1000;
> +		freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> +		if (freq_Hz == arm_lpm_clk)
> +			freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate / 1000;
> +		else
> +			freqs.old = arm_lpm_clk / 1000;
> +
> +		freqs.new = freq_Hz / 1000;
> +		freqs.cpu = 0;
> +		freqs.flags = 0;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +		return ret;
> +	}
> +	/*
> +	 * Some governors do not respects CPU and policy lower limits
> +	 * which leads to bad things (division by zero etc), ensure
> +	 * that such things do not happen.
> +	 */

Isn't that a bug in the governor? Can you explain a bit?

> +	if (target_freq < policy->cpuinfo.min_freq)
> +		target_freq = policy->cpuinfo.min_freq;
> +
> +	if (target_freq < policy->min)
> +		target_freq = policy->min;
> +
> +	freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> +
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.cpu = 0;
> +	freqs.flags = 0;
> +	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +
> +	if (freqs.old != freqs.new)
> +		ret = set_cpu_freq(freq_Hz);
> +
> +	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +
> +	return ret;
> +}
> +
> +static int __init mxc_cpufreq_driver_init(struct cpufreq_policy *policy)
> +{
> +	int ret;
> +	int i;
> +
> +	printk(KERN_INFO "i.MXC CPU frequency driver\n");
> +
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	cpu_clk = clk_get(NULL, "cpu_clk");
> +	if (IS_ERR(cpu_clk)) {
> +		printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
> +		return PTR_ERR(cpu_clk);
> +	}
> +
> +	/* Set the current working point. */
> +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> +
> +	cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
> +	cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
> +
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;

cpu_wp_nr = 2 here

1st iteration of for loop:
    imx_freq_table[2 - 1 - 0].index = 2 - 0
so, imx_freq_table[1].index = 2

2nd iteration:
    imx_freq_table[2 - 1 - 1].index = 2 - 1
    imx_freq_table[0].index = 1

So you're trying to reverse the table order? Why not just sort the table date
in the way you want and add a comment on the top to keep it sorted.

> +		imx_freq_table[cpu_wp_nr - 1 - i].frequency =
> +		    cpu_wp_tbl[i].cpu_rate / 1000;
> +
> +		if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
> +			cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
> +
> +		if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
> +			cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
> +	}
> +
> +	imx_freq_table[i].index = 0;

imx_freq_table[i].index = i ?

> +	imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
> +
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
> +	policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
> +	policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
> +
> +	arm_lpm_clk = cpu_freq_khz_min * 1000;
> +	arm_normal_clk = cpu_freq_khz_max * 1000;
> +
> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = 10;
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
> +
> +	if (ret < 0) {
> +		clk_put(cpu_clk);
> +		printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
> +		       __func__);
> +		return ret;
> +	}
> +
> +	cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
> +	return 0;
> +}
> +
> +static int mxc_cpufreq_suspend(struct cpufreq_policy *policy,
> +				     pm_message_t state)
> +{
> +	return 0;
> +}

Get rid of these, since you don't use them.

> +static int mxc_cpufreq_resume(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}

Same here.

> +static int mxc_cpufreq_driver_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +
> +	/* Reset CPU to 665MHz */
> +	set_cpu_freq(arm_normal_clk);
> +	clk_put(cpu_clk);
> +	return 0;
> +}
> +
> +static struct cpufreq_driver mxc_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = mxc_verify_speed,
> +	.target = mxc_set_target,
> +	.get = mxc_get_speed,
> +	.init = mxc_cpufreq_driver_init,

mxc_cpufreq_init is ok. Lose the driver.

> +	.exit = mxc_cpufreq_driver_exit,

same.

> +	.suspend = mxc_cpufreq_suspend,
> +	.resume = mxc_cpufreq_resume,

Get rid of suspend/resume

> +	.name = "imx",
> +};
> +
> +static int __devinit mxc_cpufreq_init(void)
> +{
> +	return cpufreq_register_driver(&mxc_driver);
> +}
> +
> +static void mxc_cpufreq_exit(void)
> +{
> +	cpufreq_unregister_driver(&mxc_driver);
> +}
> +
> +module_init(mxc_cpufreq_init);
> +module_exit(mxc_cpufreq_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("CPUfreq driver for i.MX");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
> index a790bf2..14003b9 100644
> --- a/arch/arm/plat-mxc/include/mach/mxc.h
> +++ b/arch/arm/plat-mxc/include/mach/mxc.h
> @@ -133,6 +133,26 @@ extern unsigned int __mxc_cpu_type;
>  # define cpu_is_mxc91231()	(0)
>  #endif
>  
> +#ifndef __ASSEMBLY__
> +
> +struct cpu_wp {
> +	u32 pll_reg;
> +	u32 pll_rate;
> +	u32 cpu_rate;
> +	u32 pdr0_reg;
> +	u32 pdf;
> +	u32 mfi;
> +	u32 mfd;
> +	u32 mfn;
> +	u32 cpu_voltage;
> +	u32 cpu_podf;
> +};
> +
> +#ifndef CONFIG_ARCH_MX5
> +struct cpu_wp *get_cpu_wp(int *wp);
> +#endif
> +#endif
> +
>  #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
>  /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
>  #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
> -- 
> 1.6.3.3
> 

[PATCH] cpufreq for freescale mx51

[Yong Shen]

Hi Amit,

Please see my feedback embedded.

On Thu, Sep 30, 2010 at 6:48 PM, Amit Kucheria <amit.kucheria@linaro.org>wrote:

> Add'ed linaro-dev and linux-arm-kernel to CC.
>
> Thanks Yong, some feeback follows inline.
>
> On 10 Sep 29, Yong Shen wrote:
> > From: Yong Shen <yong.shen@linaro.org>
> >
> > ---
> >  arch/arm/Kconfig                       |    6 +
> >  arch/arm/mach-mx5/Kconfig              |    1 +
> >  arch/arm/mach-mx5/board-mx51_babbage.c |   32 ++++
> >  arch/arm/mach-mx5/clock-mx51.c         |   53 ++++++
> >  arch/arm/plat-mxc/Makefile             |    2 +
> >  arch/arm/plat-mxc/cpufreq.c            |  282
> ++++++++++++++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++
> >  7 files changed, 396 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/plat-mxc/cpufreq.c
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 4db064e..64ebbc0 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1517,6 +1517,12 @@ if ARCH_HAS_CPUFREQ
> >
> >  source "drivers/cpufreq/Kconfig"
> >
> > +config CPU_FREQ_IMX
> > +     tristate "CPUfreq driver for i.MX CPUs"
> > +     depends on ARCH_MXC && CPU_FREQ
> > +     help
> > +     This enables the CPUfreq driver for i.MX CPUs.
> > +
> >  config CPU_FREQ_SA1100
> >       bool
> >
> > diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> > index 1576d51..5956fee 100644
> > --- a/arch/arm/mach-mx5/Kconfig
> > +++ b/arch/arm/mach-mx5/Kconfig
> > @@ -5,6 +5,7 @@ config ARCH_MX51
> >       default y
> >       select MXC_TZIC
> >       select ARCH_MXC_IOMUX_V3
> > +     select ARCH_HAS_CPUFREQ
> >
> >  comment "MX5 platforms:"
> >
> > diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c
> b/arch/arm/mach-mx5/board-mx51_babbage.c
> > index ed885f9..e449e0b 100644
> > --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> > +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> > @@ -43,6 +43,31 @@
> >  #define      MX51_USB_PLL_DIV_19_2_MHZ       0x01
> >  #define      MX51_USB_PLL_DIV_24_MHZ 0x02
> >
> > +struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int num_cpu_wp = 2;
>
> use sizeof(array) instead of hard coding this.
>
yong: Agreed.

>
> > +/* working point(wp): 0 - 800MHz; 1 - 166.25MHz; */
> > +static struct cpu_wp cpu_wp_auto[] = {
> > +     {
> > +     .pll_rate = 800000000,
> > +     .cpu_rate = 800000000,
> > +     .pdf = 0,
> > +     .mfi = 8,
> > +     .mfd = 2,
> > +     .mfn = 1,
> > +     .cpu_podf = 0,
> > +     .cpu_voltage = 1100000,},
> > +     {
> > +     .pll_rate = 800000000,
> > +     .cpu_rate = 160000000,
> > +     .pdf = 4,
> > +     .mfi = 8,
> > +     .mfd = 2,
> > +     .mfn = 1,
> > +     .cpu_podf = 4,
> > +     .cpu_voltage = 850000,},
> > +};
>
>
> This data should be moved out to a separate file (e.g. mx51_ratetable.h)
> since it will be useful to other boards too.
>
> If other boards can have different rate tables (and they can, depending on
> the revision of the silicon), then we can either 'assemble' the correct
> rate
> table based on a flag field or have explicit separate rate tables for each
> silicon revision.
>
> In any case, I suspect that there will be some core rates that will be
> common
> across silicon revisions.
>
>
Yong: We keep it in a board related file because we may have different
voltage set to various boards. But like what you think, in most of case, we
can use this table for different boards.


> >  static struct platform_device *devices[] __initdata = {
> >       &mxc_fec_device,
> >  };
> > @@ -87,6 +112,12 @@ static struct imxuart_platform_data uart_pdata = {
> >       .flags = IMXUART_HAVE_RTSCTS,
> >  };
> >
> > +struct cpu_wp *mx51_babbage_get_cpu_wp(int *wp)
> > +{
> > +     *wp = num_cpu_wp;
> > +     return cpu_wp_auto;
> > +}
> > +
> >  static inline void mxc_init_imx_uart(void)
> >  {
> >       mxc_register_device(&mxc_uart_device0, &uart_pdata);
> > @@ -246,6 +277,7 @@ static void __init mxc_board_init(void)
> >
> >  static void __init mx51_babbage_timer_init(void)
> >  {
> > +     get_cpu_wp = mx51_babbage_get_cpu_wp;
> >       mx51_clocks_init(32768, 24000000, 22579200, 0);
> >  }
> >
> > diff --git a/arch/arm/mach-mx5/clock-mx51.c
> b/arch/arm/mach-mx5/clock-mx51.c
> > index d9f612d..f2488e6 100644
> > --- a/arch/arm/mach-mx5/clock-mx51.c
> > +++ b/arch/arm/mach-mx5/clock-mx51.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/time.h>
> >
> >  #include <asm/clkdev.h>
> >  #include <asm/div64.h>
> > @@ -28,6 +29,11 @@
> >  static unsigned long external_high_reference, external_low_reference;
> >  static unsigned long oscillator_reference, ckih2_reference;
> >
> > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int cpu_wp_nr;
> > +static int cpu_curr_wp;
> > +static struct cpu_wp *cpu_wp_tbl;
> > +
> >  static struct clk osc_clk;
> >  static struct clk pll1_main_clk;
> >  static struct clk pll1_sw_clk;
> > @@ -38,7 +44,9 @@ static struct clk periph_apm_clk;
> >  static struct clk ahb_clk;
> >  static struct clk ipg_clk;
> >  static struct clk usboh3_clk;
> > +static void __iomem *pll1_base;
> >
> > +#define SPIN_DELAY   1000000 /* in nanoseconds */
> >  #define MAX_DPLL_WAIT_TRIES  1000 /* 1000 * udelay(1) = 1ms */
> >
> >  static int _clk_ccgr_enable(struct clk *clk)
> > @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk,
> struct clk *parent)
> >       return 0;
> >  }
> >
> > +/*!
> > + * Setup cpu clock based on working point.
> > + * @param    wp      cpu freq working point
> > + * @return           0 on success or error code on failure.
> > + */
> > +int cpu_clk_set_wp(int wp)
> > +{
> > +     struct cpu_wp *p;
> > +     u32 reg;
> > +
> > +     if (wp == cpu_curr_wp)
> > +             return 0;
> > +
> > +     p = &cpu_wp_tbl[wp];
> > +
> > +     /*use post divider to change freq
> > +      */
> > +     reg = __raw_readl(MXC_CCM_CACRR);
> > +     reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > +     reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > +     __raw_writel(reg, MXC_CCM_CACRR);
> > +     cpu_curr_wp = wp;
> > +
> > +     return 0;
> > +}
> > +
> >  static unsigned long clk_arm_get_rate(struct clk *clk)
> >  {
> >       u32 cacrr, div;
> > @@ -342,6 +376,20 @@ static unsigned long clk_arm_get_rate(struct clk
> *clk)
> >       return parent_rate / div;
> >  }
> >
> > +int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
> > +{
> > +     u32 i;
> > +     for (i = 0; i < cpu_wp_nr; i++) {
> > +             if (rate == cpu_wp_tbl[i].cpu_rate)
> > +                     break;
> > +     }
> > +     if (i >= cpu_wp_nr)
> > +             return -EINVAL;
> > +     cpu_clk_set_wp(i);
> > +
> > +     return 0;
> > +}
> > +
> >  static int _clk_periph_apm_set_parent(struct clk *clk, struct clk
> *parent)
> >  {
> >       u32 reg, mux;
> > @@ -694,6 +742,7 @@ static struct clk periph_apm_clk = {
> >  static struct clk cpu_clk = {
> >       .parent = &pll1_sw_clk,
> >       .get_rate = clk_arm_get_rate,
> > +     .set_rate = _clk_cpu_set_rate,
> >  };
> >
> >  static struct clk ahb_clk = {
> > @@ -821,6 +870,7 @@ static struct clk_lookup lookups[] = {
> >       _REGISTER_CLOCK("mxc-ehci.1", "usb_ahb", ahb_clk)
> >       _REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
> >       _REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
> > +     _REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
> >  };
> >
> >  static void clk_tree_init(void)
> > @@ -848,10 +898,13 @@ int __init mx51_clocks_init(unsigned long ckil,
> unsigned long osc,
> >  {
> >       int i;
> >
> > +     pll1_base = ioremap(MX51_PLL1_BASE_ADDR, SZ_4K);
> > +
> >       external_low_reference = ckil;
> >       external_high_reference = ckih1;
> >       ckih2_reference = ckih2;
> >       oscillator_reference = osc;
> > +     cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> >
> >       for (i = 0; i < ARRAY_SIZE(lookups); i++)
> >               clkdev_add(&lookups[i]);
> > diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> > index 895bc3c..c1bb400 100644
> > --- a/arch/arm/plat-mxc/Makefile
> > +++ b/arch/arm/plat-mxc/Makefile
> > @@ -17,6 +17,8 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci.o
> >  obj-$(CONFIG_MXC_ULPI) += ulpi.o
> >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
> >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
> > +# CPU FREQ support
> > +obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
> >  ifdef CONFIG_SND_IMX_SOC
> >  obj-y += ssi-fiq.o
> >  obj-y += ssi-fiq-ksym.o
> > diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
> > new file mode 100644
> > index 0000000..cae40f1
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/cpufreq.c
> > @@ -0,0 +1,282 @@
> > +/*
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > + */
> > +
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/*!
> > + * @file cpufreq.c
> > + *
> > + * @brief A driver for the Freescale Semiconductor i.MXC CPUfreq module.
> > + *
> > + * The CPUFREQ driver is for controling CPU frequency. It allows you to
> change
> > + * the CPU clock speed on the fly.
> > + *
> > + * @ingroup PM
> > + */
>
> Fix these comments to follow the kernel commenting style.
>

Yong: OK

>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/init.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <mach/hardware.h>
> > +#include <asm/setup.h>
> > +#include <mach/clock.h>
> > +#include <asm/cacheflush.h>
> > +#include <linux/hrtimer.h>
> > +
> > +int cpu_freq_khz_min;
> > +int cpu_freq_khz_max;
> > +int arm_lpm_clk;
> > +int arm_normal_clk;
> > +int cpufreq_suspended;
> > +int cpufreq_trig_needed;
> > +
> > +static struct clk *cpu_clk;
> > +static struct cpufreq_frequency_table imx_freq_table[4];
> > +
> > +extern int set_low_bus_freq(void);
> > +extern int set_high_bus_freq(int high_bus_speed);
> > +extern int low_freq_bus_used(void);
> > +
> > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int cpu_wp_nr;
> > +static struct cpu_wp *cpu_wp_tbl;
> > +
> > +int set_cpu_freq(int freq)
> > +{
> > +     int ret = 0;
> > +     int org_cpu_rate;
> > +     int gp_volt = 0;
> > +     int i;
> > +
> > +     org_cpu_rate = clk_get_rate(cpu_clk);
> > +     if (org_cpu_rate == freq)
> > +             return ret;
> > +
> > +     for (i = 0; i < cpu_wp_nr; i++) {
> > +             if (freq == cpu_wp_tbl[i].cpu_rate)
> > +                     gp_volt = cpu_wp_tbl[i].cpu_voltage;
> > +     }
> > +
> > +     if (gp_volt == 0)
> > +             return ret;
> > +
> > +     ret = clk_set_rate(cpu_clk, freq);
> > +     if (ret != 0) {
> > +             printk(KERN_DEBUG "cannot set CPU clock rate\n");
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int mxc_verify_speed(struct cpufreq_policy *policy)
> > +{
> > +     if (policy->cpu != 0)
> > +             return -EINVAL;
> > +
> > +     return cpufreq_frequency_table_verify(policy, imx_freq_table);
> > +}
> > +
> > +static unsigned int mxc_get_speed(unsigned int cpu)
> > +{
> > +     if (cpu)
> > +             return 0;
> > +
> > +     return clk_get_rate(cpu_clk) / 1000;
> > +}
> > +
> > +static int calc_frequency_khz(int target, unsigned int relation)
> > +{
> > +     int i;
> > +
> > +     if ((target * 1000) == clk_get_rate(cpu_clk))
> > +             return target;
> > +
> > +     if (relation == CPUFREQ_RELATION_H) {
> > +             for (i = cpu_wp_nr - 1; i >= 0; i--) {
> > +                     if (imx_freq_table[i].frequency <= target)
> > +                             return imx_freq_table[i].frequency;
> > +             }
> > +     } else if (relation == CPUFREQ_RELATION_L) {
> > +             for (i = 0; i < cpu_wp_nr; i++) {
> > +                     if (imx_freq_table[i].frequency >= target)
> > +                             return imx_freq_table[i].frequency;
> > +             }
> > +     }
> > +     printk(KERN_ERR "Error: No valid cpufreq relation\n");
> > +     return cpu_freq_khz_max;
> > +}
> > +
> > +static int mxc_set_target(struct cpufreq_policy *policy,
> > +                       unsigned int target_freq, unsigned int relation)
> > +{
> > +     struct cpufreq_freqs freqs;
> > +     int freq_Hz;
> > +     int ret = 0;
> > +
> > +     if (cpufreq_suspended) {
> > +             target_freq = clk_get_rate(cpu_clk) / 1000;
> > +             freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > +             if (freq_Hz == arm_lpm_clk)
> > +                     freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate /
> 1000;
> > +             else
> > +                     freqs.old = arm_lpm_clk / 1000;
> > +
> > +             freqs.new = freq_Hz / 1000;
> > +             freqs.cpu = 0;
> > +             freqs.flags = 0;
> > +             cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +             cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +             return ret;
> > +     }
> > +     /*
> > +      * Some governors do not respects CPU and policy lower limits
> > +      * which leads to bad things (division by zero etc), ensure
> > +      * that such things do not happen.
> > +      */
>
> Isn't that a bug in the governor? Can you explain a bit?
>
Yong: the original driver writer might have concern that some governor
implementations will not care about the low limit suggested by cpu policy,
therefore it is a change to correct them.

>
> > +     if (target_freq < policy->cpuinfo.min_freq)
> > +             target_freq = policy->cpuinfo.min_freq;
> > +
> > +     if (target_freq < policy->min)
> > +             target_freq = policy->min;
> > +
> > +     freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > +
> > +     freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +     freqs.new = freq_Hz / 1000;
> > +     freqs.cpu = 0;
> > +     freqs.flags = 0;
> > +     cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +
> > +     if (freqs.old != freqs.new)
> > +             ret = set_cpu_freq(freq_Hz);
> > +
> > +     cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +
> > +     return ret;
> > +}
> > +
> > +static int __init mxc_cpufreq_driver_init(struct cpufreq_policy *policy)
> > +{
> > +     int ret;
> > +     int i;
> > +
> > +     printk(KERN_INFO "i.MXC CPU frequency driver\n");
> > +
> > +     if (policy->cpu != 0)
> > +             return -EINVAL;
> > +
> > +     cpu_clk = clk_get(NULL, "cpu_clk");
> > +     if (IS_ERR(cpu_clk)) {
> > +             printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
> > +             return PTR_ERR(cpu_clk);
> > +     }
> > +
> > +     /* Set the current working point. */
> > +     cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> > +
> > +     cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
> > +     cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
> > +
> > +     for (i = 0; i < cpu_wp_nr; i++) {
> > +             imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;
>
> cpu_wp_nr = 2 here
>
> 1st iteration of for loop:
>    imx_freq_table[2 - 1 - 0].index = 2 - 0
> so, imx_freq_table[1].index = 2
>
> 2nd iteration:
>    imx_freq_table[2 - 1 - 1].index = 2 - 1
>    imx_freq_table[0].index = 1
>
> So you're trying to reverse the table order? Why not just sort the table
> date
> in the way you want and add a comment on the top to keep it sorted.
>
Yong: my understanding is that the freq table defined in another file is
sorted in descending order, so the writer tends to make imx_freq_table in a
descending order.

>
> > +             imx_freq_table[cpu_wp_nr - 1 - i].frequency =
> > +                 cpu_wp_tbl[i].cpu_rate / 1000;
> > +
> > +             if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
> > +                     cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
> > +
> > +             if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
> > +                     cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
> > +     }
> > +
> > +     imx_freq_table[i].index = 0;
>
> imx_freq_table[i].index = i ?
>
Yong: this should be a place holder, no meaning.

>
> > +     imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
> > +
> > +     policy->cur = clk_get_rate(cpu_clk) / 1000;
> > +     policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
> > +     policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
> > +     policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
> > +
> > +     arm_lpm_clk = cpu_freq_khz_min * 1000;
> > +     arm_normal_clk = cpu_freq_khz_max * 1000;
> > +
> > +     /* Manual states, that PLL stabilizes in two CLK32 periods */
> > +     policy->cpuinfo.transition_latency = 10;
> > +
> > +     ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
> > +
> > +     if (ret < 0) {
> > +             clk_put(cpu_clk);
> > +             printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
> > +                    __func__);
> > +             return ret;
> > +     }
> > +
> > +     cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
> > +     return 0;
> > +}
> > +
> > +static int mxc_cpufreq_suspend(struct cpufreq_policy *policy,
> > +                                  pm_message_t state)
> > +{
> > +     return 0;
> > +}
>
> Get rid of these, since you don't use them.
>
> yong:Ok about this and below

> > +static int mxc_cpufreq_resume(struct cpufreq_policy *policy)
> > +{
> > +     return 0;
> > +}
>
> Same here.
>
> > +static int mxc_cpufreq_driver_exit(struct cpufreq_policy *policy)
> > +{
> > +     cpufreq_frequency_table_put_attr(policy->cpu);
> > +
> > +     /* Reset CPU to 665MHz */
> > +     set_cpu_freq(arm_normal_clk);
> > +     clk_put(cpu_clk);
> > +     return 0;
> > +}
> > +
> > +static struct cpufreq_driver mxc_driver = {
> > +     .flags = CPUFREQ_STICKY,
> > +     .verify = mxc_verify_speed,
> > +     .target = mxc_set_target,
> > +     .get = mxc_get_speed,
> > +     .init = mxc_cpufreq_driver_init,
>
> mxc_cpufreq_init is ok. Lose the driver.
>
> > +     .exit = mxc_cpufreq_driver_exit,
>
> same.
>
> > +     .suspend = mxc_cpufreq_suspend,
> > +     .resume = mxc_cpufreq_resume,
>
> Get rid of suspend/resume
>
> > +     .name = "imx",
> > +};
> > +
> > +static int __devinit mxc_cpufreq_init(void)
> > +{
> > +     return cpufreq_register_driver(&mxc_driver);
> > +}
> > +
> > +static void mxc_cpufreq_exit(void)
> > +{
> > +     cpufreq_unregister_driver(&mxc_driver);
> > +}
> > +
> > +module_init(mxc_cpufreq_init);
> > +module_exit(mxc_cpufreq_exit);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> > +MODULE_DESCRIPTION("CPUfreq driver for i.MX");
> > +MODULE_LICENSE("GPL");
> > diff --git a/arch/arm/plat-mxc/include/mach/mxc.h
> b/arch/arm/plat-mxc/include/mach/mxc.h
> > index a790bf2..14003b9 100644
> > --- a/arch/arm/plat-mxc/include/mach/mxc.h
> > +++ b/arch/arm/plat-mxc/include/mach/mxc.h
> > @@ -133,6 +133,26 @@ extern unsigned int __mxc_cpu_type;
> >  # define cpu_is_mxc91231()   (0)
> >  #endif
> >
> > +#ifndef __ASSEMBLY__
> > +
> > +struct cpu_wp {
> > +     u32 pll_reg;
> > +     u32 pll_rate;
> > +     u32 cpu_rate;
> > +     u32 pdr0_reg;
> > +     u32 pdf;
> > +     u32 mfi;
> > +     u32 mfd;
> > +     u32 mfn;
> > +     u32 cpu_voltage;
> > +     u32 cpu_podf;
> > +};
> > +
> > +#ifndef CONFIG_ARCH_MX5
> > +struct cpu_wp *get_cpu_wp(int *wp);
> > +#endif
> > +#endif
> > +
> >  #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
> >  /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
> >  #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
> > --
> > 1.6.3.3
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101001/3febe223/attachment-0001.html>

[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

More comments.

On Fri, Oct 1, 2010 at 7:06 AM, Yong Shen <yong.shen@linaro.org> wrote:
> Hi Amit,
>
> Please see my feedback embedded.
>
> On Thu, Sep 30, 2010 at 6:48 PM, Amit Kucheria <amit.kucheria@linaro.org>
> wrote:

[snip]

>> > +static int mxc_set_target(struct cpufreq_policy *policy,
>> > + ? ? ? ? ? ? ? ? ? ? ? unsigned int target_freq, unsigned int relation)
>> > +{
>> > + ? ? struct cpufreq_freqs freqs;
>> > + ? ? int freq_Hz;
>> > + ? ? int ret = 0;
>> > +
>> > + ? ? if (cpufreq_suspended) {
>> > + ? ? ? ? ? ? target_freq = clk_get_rate(cpu_clk) / 1000;
>> > + ? ? ? ? ? ? freq_Hz = calc_frequency_khz(target_freq, relation) *
>> > 1000;
>> > + ? ? ? ? ? ? if (freq_Hz == arm_lpm_clk)
>> > + ? ? ? ? ? ? ? ? ? ? freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate /
>> > 1000;
>> > + ? ? ? ? ? ? else
>> > + ? ? ? ? ? ? ? ? ? ? freqs.old = arm_lpm_clk / 1000;
>> > +
>> > + ? ? ? ? ? ? freqs.new = freq_Hz / 1000;
>> > + ? ? ? ? ? ? freqs.cpu = 0;
>> > + ? ? ? ? ? ? freqs.flags = 0;
>> > + ? ? ? ? ? ? cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>> > + ? ? ? ? ? ? cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>> > + ? ? ? ? ? ? return ret;
>> > + ? ? }
>> > + ? ? /*
>> > + ? ? ?* Some governors do not respects CPU and policy lower limits
>> > + ? ? ?* which leads to bad things (division by zero etc), ensure
>> > + ? ? ?* that such things do not happen.
>> > + ? ? ?*/
>>
>> Isn't that a bug in the governor? Can you explain a bit?
>
> Yong: the original driver writer might have concern that some governor
> implementations will not care about the low limit suggested by cpu policy,
> therefore it is a change to correct them.

Could you confirm if this is still the case, and if not, get rid of
this code/comment?

>> > + ? ? if (target_freq < policy->cpuinfo.min_freq)
>> > + ? ? ? ? ? ? target_freq = policy->cpuinfo.min_freq;
>> > +
>> > + ? ? if (target_freq < policy->min)
>> > + ? ? ? ? ? ? target_freq = policy->min;
>> > +
>> > + ? ? freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
>> > +

[snip]

>> > + ? ? /* Set the current working point. */
>> > + ? ? cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
>> > +
>> > + ? ? cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
>> > + ? ? cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
>> > +
>> > + ? ? for (i = 0; i < cpu_wp_nr; i++) {
>> > + ? ? ? ? ? ? imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;
>>
>> cpu_wp_nr = 2 here
>>
>> 1st iteration of for loop:
>> ? ?imx_freq_table[2 - 1 - 0].index = 2 - 0
>> so, imx_freq_table[1].index = 2
>>
>> 2nd iteration:
>> ? ?imx_freq_table[2 - 1 - 1].index = 2 - 1
>> ? ?imx_freq_table[0].index = 1
>>
>> So you're trying to reverse the table order? Why not just sort the table
>> date
>> in the way you want and add a comment on the top to keep it sorted.
>
> Yong: my understanding is that the freq table defined in another file is
> sorted in descending order, so the writer tends to make imx_freq_table in a
> descending order.

Just change the order of the wp_table instead of this confusing math
to re-sort it. Look at other cpufreq rate tables in the kernel.

>> > + ? ? ? ? ? ? imx_freq_table[cpu_wp_nr - 1 - i].frequency =
>> > + ? ? ? ? ? ? ? ? cpu_wp_tbl[i].cpu_rate / 1000;
>> > +
>> > + ? ? ? ? ? ? if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
>> > + ? ? ? ? ? ? ? ? ? ? cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
>> > +

[PATCH] cpufreq for freescale mx51

[Sascha Hauer]

On Thu, Sep 30, 2010 at 01:48:17PM +0300, Amit Kucheria wrote:
> Add'ed linaro-dev and linux-arm-kernel to CC.
> 
> Thanks Yong, some feeback follows inline.
> 
> On 10 Sep 29, Yong Shen wrote:
> > From: Yong Shen <yong.shen@linaro.org>
> > 
> > ---
> >  arch/arm/Kconfig                       |    6 +
> >  arch/arm/mach-mx5/Kconfig              |    1 +
> >  arch/arm/mach-mx5/board-mx51_babbage.c |   32 ++++
> >  arch/arm/mach-mx5/clock-mx51.c         |   53 ++++++
> >  arch/arm/plat-mxc/Makefile             |    2 +
> >  arch/arm/plat-mxc/cpufreq.c            |  282 ++++++++++++++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++
> >  7 files changed, 396 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/plat-mxc/cpufreq.c
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 4db064e..64ebbc0 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1517,6 +1517,12 @@ if ARCH_HAS_CPUFREQ
> >  
> >  source "drivers/cpufreq/Kconfig"
> >  
> > +config CPU_FREQ_IMX
> > +	tristate "CPUfreq driver for i.MX CPUs"
> > +	depends on ARCH_MXC && CPU_FREQ
> > +	help
> > +	This enables the CPUfreq driver for i.MX CPUs.
> > +
> >  config CPU_FREQ_SA1100
> >  	bool
> >  
> > diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> > index 1576d51..5956fee 100644
> > --- a/arch/arm/mach-mx5/Kconfig
> > +++ b/arch/arm/mach-mx5/Kconfig
> > @@ -5,6 +5,7 @@ config ARCH_MX51
> >  	default y
> >  	select MXC_TZIC
> >  	select ARCH_MXC_IOMUX_V3
> > +	select ARCH_HAS_CPUFREQ
> >  
> >  comment "MX5 platforms:"
> >  
> > diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
> > index ed885f9..e449e0b 100644
> > --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> > +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> > @@ -43,6 +43,31 @@
> >  #define	MX51_USB_PLL_DIV_19_2_MHZ	0x01
> >  #define	MX51_USB_PLL_DIV_24_MHZ	0x02
> >  
> > +struct cpu_wp *(*get_cpu_wp)(int *wp);

This should be moved to a generic place, otherwise it breaks the build
when multiple boards are selected.

> > +static int num_cpu_wp = 2;
> 
> use sizeof(array) instead of hard coding this.
> 
> > +/* working point(wp): 0 - 800MHz; 1 - 166.25MHz; */
> > +static struct cpu_wp cpu_wp_auto[] = {
> > +	{
> > +	.pll_rate = 800000000,
> > +	.cpu_rate = 800000000,
> > +	.pdf = 0,
> > +	.mfi = 8,
> > +	.mfd = 2,
> > +	.mfn = 1,
> > +	.cpu_podf = 0,
> > +	.cpu_voltage = 1100000,},
> > +	{
> > +	.pll_rate = 800000000,
> > +	.cpu_rate = 160000000,
> > +	.pdf = 4,
> > +	.mfi = 8,
> > +	.mfd = 2,
> > +	.mfn = 1,
> > +	.cpu_podf = 4,
> > +	.cpu_voltage = 850000,},
> > +};
> 
> 
> This data should be moved out to a separate file (e.g. mx51_ratetable.h)
> since it will be useful to other boards too.
> 
> If other boards can have different rate tables (and they can, depending on
> the revision of the silicon), then we can either 'assemble' the correct rate
> table based on a flag field or have explicit separate rate tables for each
> silicon revision.
> 
> In any case, I suspect that there will be some core rates that will be common
> across silicon revisions.
> 
> >  static struct platform_device *devices[] __initdata = {
> >  	&mxc_fec_device,
> >  };
> > @@ -87,6 +112,12 @@ static struct imxuart_platform_data uart_pdata = {
> >  	.flags = IMXUART_HAVE_RTSCTS,
> >  };
> >  
> > +struct cpu_wp *mx51_babbage_get_cpu_wp(int *wp)

static

> > +{
> > +	*wp = num_cpu_wp;
> > +	return cpu_wp_auto;
> > +}
> > +
> >  static inline void mxc_init_imx_uart(void)
> >  {
> >  	mxc_register_device(&mxc_uart_device0, &uart_pdata);
> > @@ -246,6 +277,7 @@ static void __init mxc_board_init(void)
> >  
> >  static void __init mx51_babbage_timer_init(void)
> >  {
> > +	get_cpu_wp = mx51_babbage_get_cpu_wp;

It looks strange to have a global function pointer which gets
overwritten by the boards. I would expect a

imx_add_cpu_workpoints(cpu_wp_auto, ARRAY_SIZE(cpu_wp_auto));

here.

> >  	mx51_clocks_init(32768, 24000000, 22579200, 0);
> >  }
> >  
> > diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
> > index d9f612d..f2488e6 100644
> > --- a/arch/arm/mach-mx5/clock-mx51.c
> > +++ b/arch/arm/mach-mx5/clock-mx51.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/time.h>

unneeded include

> >  
> >  #include <asm/clkdev.h>
> >  #include <asm/div64.h>
> > @@ -28,6 +29,11 @@
> >  static unsigned long external_high_reference, external_low_reference;
> >  static unsigned long oscillator_reference, ckih2_reference;
> >  
> > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int cpu_wp_nr;
> > +static int cpu_curr_wp;
> > +static struct cpu_wp *cpu_wp_tbl;
> > +
> >  static struct clk osc_clk;
> >  static struct clk pll1_main_clk;
> >  static struct clk pll1_sw_clk;
> > @@ -38,7 +44,9 @@ static struct clk periph_apm_clk;
> >  static struct clk ahb_clk;
> >  static struct clk ipg_clk;
> >  static struct clk usboh3_clk;
> > +static void __iomem *pll1_base;
> >  
> > +#define SPIN_DELAY	1000000 /* in nanoseconds */

unused

> >  #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
> >  
> >  static int _clk_ccgr_enable(struct clk *clk)
> > @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
> >  	return 0;
> >  }
> >  
> > +/*!
> > + * Setup cpu clock based on working point.
> > + * @param	wp	cpu freq working point
> > + * @return		0 on success or error code on failure.
> > + */
> > +int cpu_clk_set_wp(int wp)

static

> > +{
> > +	struct cpu_wp *p;
> > +	u32 reg;
> > +
> > +	if (wp == cpu_curr_wp)
> > +		return 0;
> > +
> > +	p = &cpu_wp_tbl[wp];
> > +
> > +	/*use post divider to change freq
> > +	 */
> > +	reg = __raw_readl(MXC_CCM_CACRR);
> > +	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > +	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > +	__raw_writel(reg, MXC_CCM_CACRR);

We have a simple divider here. Why do we need a reference to struct
cpu_wp then? This code could look much simpler.

(side note: I'm aware that the original Freescale code is also able
to change the cpu frequency using the pll instead of the divider, but is
this really necessary?)

> > +	cpu_curr_wp = wp;
> > +
> > +	return 0;
> > +}
> > +
> >  static unsigned long clk_arm_get_rate(struct clk *clk)
> >  {
> >  	u32 cacrr, div;
> > @@ -342,6 +376,20 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
> >  	return parent_rate / div;
> >  }
> >  
> > +int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	u32 i;
> > +	for (i = 0; i < cpu_wp_nr; i++) {
> > +		if (rate == cpu_wp_tbl[i].cpu_rate)
> > +			break;
> > +	}
> > +	if (i >= cpu_wp_nr)
> > +		return -EINVAL;
> > +	cpu_clk_set_wp(i);
> > +
> > +	return 0;
> > +}
> > +
> >  static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
> >  {
> >  	u32 reg, mux;
> > @@ -694,6 +742,7 @@ static struct clk periph_apm_clk = {
> >  static struct clk cpu_clk = {
> >  	.parent = &pll1_sw_clk,
> >  	.get_rate = clk_arm_get_rate,
> > +	.set_rate = _clk_cpu_set_rate,
> >  };
> >  
> >  static struct clk ahb_clk = {
> > @@ -821,6 +870,7 @@ static struct clk_lookup lookups[] = {
> >  	_REGISTER_CLOCK("mxc-ehci.1", "usb_ahb", ahb_clk)
> >  	_REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
> >  	_REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
> > +	_REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
> >  };
> >  
> >  static void clk_tree_init(void)
> > @@ -848,10 +898,13 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
> >  {
> >  	int i;
> >  
> > +	pll1_base = ioremap(MX51_PLL1_BASE_ADDR, SZ_4K);
> > +

this is unused.

> >  	external_low_reference = ckil;
> >  	external_high_reference = ckih1;
> >  	ckih2_reference = ckih2;
> >  	oscillator_reference = osc;
> > +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
> >  		clkdev_add(&lookups[i]);
> > diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> > index 895bc3c..c1bb400 100644
> > --- a/arch/arm/plat-mxc/Makefile
> > +++ b/arch/arm/plat-mxc/Makefile
> > @@ -17,6 +17,8 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci.o
> >  obj-$(CONFIG_MXC_ULPI) += ulpi.o
> >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
> >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
> > +# CPU FREQ support
> > +obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
> >  ifdef CONFIG_SND_IMX_SOC
> >  obj-y += ssi-fiq.o
> >  obj-y += ssi-fiq-ksym.o
> > diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
> > new file mode 100644
> > index 0000000..cae40f1
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/cpufreq.c
> > @@ -0,0 +1,282 @@
> > +/*
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > + */
> > +
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/*!
> > + * @file cpufreq.c
> > + *
> > + * @brief A driver for the Freescale Semiconductor i.MXC CPUfreq module.
> > + *
> > + * The CPUFREQ driver is for controling CPU frequency. It allows you to change
> > + * the CPU clock speed on the fly.
> > + *
> > + * @ingroup PM
> > + */
> 
> Fix these comments to follow the kernel commenting style.
> 
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/init.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <mach/hardware.h>
> > +#include <asm/setup.h>
> > +#include <mach/clock.h>
> > +#include <asm/cacheflush.h>
> > +#include <linux/hrtimer.h>
> > +
> > +int cpu_freq_khz_min;
> > +int cpu_freq_khz_max;
> > +int arm_lpm_clk;
> > +int arm_normal_clk;
> > +int cpufreq_suspended;
> > +int cpufreq_trig_needed;
> > +
> > +static struct clk *cpu_clk;
> > +static struct cpufreq_frequency_table imx_freq_table[4];

Three frequencies should be enough for everyone, right? This should be
dynamically allocated like in other cpufreq drivers.

> > +
> > +extern int set_low_bus_freq(void);
> > +extern int set_high_bus_freq(int high_bus_speed);
> > +extern int low_freq_bus_used(void);

declared but never used.

> > +
> > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int cpu_wp_nr;
> > +static struct cpu_wp *cpu_wp_tbl;
> > +
> > +int set_cpu_freq(int freq)
> > +{
> > +	int ret = 0;
> > +	int org_cpu_rate;
> > +	int gp_volt = 0;
> > +	int i;
> > +
> > +	org_cpu_rate = clk_get_rate(cpu_clk);
> > +	if (org_cpu_rate == freq)
> > +		return ret;
> > +
> > +	for (i = 0; i < cpu_wp_nr; i++) {
> > +		if (freq == cpu_wp_tbl[i].cpu_rate)
> > +			gp_volt = cpu_wp_tbl[i].cpu_voltage;
> > +	}
> > +
> > +	if (gp_volt == 0)
> > +		return ret;
> > +	
> > +	ret = clk_set_rate(cpu_clk, freq);
> > +	if (ret != 0) {
> > +		printk(KERN_DEBUG "cannot set CPU clock rate\n");
> > +		return ret;
> > +	}
> > +	

Please remove trailing whitespaces.

> > +	return ret;
> > +}
> > +
> > +static int mxc_verify_speed(struct cpufreq_policy *policy)
> > +{
> > +	if (policy->cpu != 0)
> > +		return -EINVAL;
> > +
> > +	return cpufreq_frequency_table_verify(policy, imx_freq_table);
> > +}
> > +
> > +static unsigned int mxc_get_speed(unsigned int cpu)
> > +{
> > +	if (cpu)
> > +		return 0;
> > +
> > +	return clk_get_rate(cpu_clk) / 1000;
> > +}
> > +
> > +static int calc_frequency_khz(int target, unsigned int relation)
> > +{
> > +	int i;
> > +
> > +	if ((target * 1000) == clk_get_rate(cpu_clk))
> > +		return target;
> > +
> > +	if (relation == CPUFREQ_RELATION_H) {
> > +		for (i = cpu_wp_nr - 1; i >= 0; i--) {
> > +			if (imx_freq_table[i].frequency <= target)
> > +				return imx_freq_table[i].frequency;
> > +		}
> > +	} else if (relation == CPUFREQ_RELATION_L) {
> > +		for (i = 0; i < cpu_wp_nr; i++) {
> > +			if (imx_freq_table[i].frequency >= target)
> > +				return imx_freq_table[i].frequency;
> > +		}
> > +	}

It looks like cpufreq_frequency_table_target could help here.

> > +	printk(KERN_ERR "Error: No valid cpufreq relation\n");
> > +	return cpu_freq_khz_max;
> > +}
> > +
> > +static int mxc_set_target(struct cpufreq_policy *policy,
> > +			  unsigned int target_freq, unsigned int relation)
> > +{
> > +	struct cpufreq_freqs freqs;
> > +	int freq_Hz;
> > +	int ret = 0;
> > +
> > +	if (cpufreq_suspended) {
> > +		target_freq = clk_get_rate(cpu_clk) / 1000;
> > +		freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > +		if (freq_Hz == arm_lpm_clk)
> > +			freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate / 1000;
> > +		else
> > +			freqs.old = arm_lpm_clk / 1000;
> > +
> > +		freqs.new = freq_Hz / 1000;
> > +		freqs.cpu = 0;
> > +		freqs.flags = 0;
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +		return ret;
> > +	}
> > +	/*
> > +	 * Some governors do not respects CPU and policy lower limits
> > +	 * which leads to bad things (division by zero etc), ensure
> > +	 * that such things do not happen.
> > +	 */
> 
> Isn't that a bug in the governor? Can you explain a bit?
> 
> > +	if (target_freq < policy->cpuinfo.min_freq)
> > +		target_freq = policy->cpuinfo.min_freq;
> > +
> > +	if (target_freq < policy->min)
> > +		target_freq = policy->min;
> > +
> > +	freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > +
> > +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +	freqs.new = freq_Hz / 1000;
> > +	freqs.cpu = 0;
> > +	freqs.flags = 0;
> > +	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +
> > +	if (freqs.old != freqs.new)
> > +		ret = set_cpu_freq(freq_Hz);
> > +
> > +	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __init mxc_cpufreq_driver_init(struct cpufreq_policy *policy)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	printk(KERN_INFO "i.MXC CPU frequency driver\n");
> > +
> > +	if (policy->cpu != 0)
> > +		return -EINVAL;
> > +
> > +	cpu_clk = clk_get(NULL, "cpu_clk");
> > +	if (IS_ERR(cpu_clk)) {
> > +		printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
> > +		return PTR_ERR(cpu_clk);
> > +	}
> > +
> > +	/* Set the current working point. */
> > +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> > +
> > +	cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
> > +	cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
> > +
> > +	for (i = 0; i < cpu_wp_nr; i++) {
> > +		imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;
> 
> cpu_wp_nr = 2 here
> 
> 1st iteration of for loop:
>     imx_freq_table[2 - 1 - 0].index = 2 - 0
> so, imx_freq_table[1].index = 2
> 
> 2nd iteration:
>     imx_freq_table[2 - 1 - 1].index = 2 - 1
>     imx_freq_table[0].index = 1
> 
> So you're trying to reverse the table order? Why not just sort the table date
> in the way you want and add a comment on the top to keep it sorted.
> 
> > +		imx_freq_table[cpu_wp_nr - 1 - i].frequency =
> > +		    cpu_wp_tbl[i].cpu_rate / 1000;
> > +
> > +		if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
> > +			cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
> > +
> > +		if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
> > +			cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
> > +	}
> > +
> > +	imx_freq_table[i].index = 0;
> 
> imx_freq_table[i].index = i ?
> 
> > +	imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
> > +
> > +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> > +	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
> > +	policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
> > +	policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
> > +
> > +	arm_lpm_clk = cpu_freq_khz_min * 1000;
> > +	arm_normal_clk = cpu_freq_khz_max * 1000;
> > +
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = 10;
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
> > +
> > +	if (ret < 0) {
> > +		clk_put(cpu_clk);
> > +		printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
> > +		       __func__);
> > +		return ret;
> > +	}
> > +
> > +	cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
> > +	return 0;
> > +}
> > +
> > +static int mxc_cpufreq_suspend(struct cpufreq_policy *policy,
> > +				     pm_message_t state)
> > +{
> > +	return 0;
> > +}
> 
> Get rid of these, since you don't use them.
> 
> > +static int mxc_cpufreq_resume(struct cpufreq_policy *policy)
> > +{
> > +	return 0;
> > +}
> 
> Same here.
> 
> > +static int mxc_cpufreq_driver_exit(struct cpufreq_policy *policy)
> > +{
> > +	cpufreq_frequency_table_put_attr(policy->cpu);
> > +
> > +	/* Reset CPU to 665MHz */
> > +	set_cpu_freq(arm_normal_clk);
> > +	clk_put(cpu_clk);
> > +	return 0;
> > +}
> > +
> > +static struct cpufreq_driver mxc_driver = {
> > +	.flags = CPUFREQ_STICKY,
> > +	.verify = mxc_verify_speed,
> > +	.target = mxc_set_target,
> > +	.get = mxc_get_speed,
> > +	.init = mxc_cpufreq_driver_init,
> 
> mxc_cpufreq_init is ok. Lose the driver.
> 
> > +	.exit = mxc_cpufreq_driver_exit,
> 
> same.
> 
> > +	.suspend = mxc_cpufreq_suspend,
> > +	.resume = mxc_cpufreq_resume,
> 
> Get rid of suspend/resume
> 
> > +	.name = "imx",
> > +};
> > +
> > +static int __devinit mxc_cpufreq_init(void)
> > +{
> > +	return cpufreq_register_driver(&mxc_driver);
> > +}
> > +
> > +static void mxc_cpufreq_exit(void)
> > +{
> > +	cpufreq_unregister_driver(&mxc_driver);
> > +}
> > +
> > +module_init(mxc_cpufreq_init);
> > +module_exit(mxc_cpufreq_exit);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> > +MODULE_DESCRIPTION("CPUfreq driver for i.MX");
> > +MODULE_LICENSE("GPL");
> > diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
> > index a790bf2..14003b9 100644
> > --- a/arch/arm/plat-mxc/include/mach/mxc.h
> > +++ b/arch/arm/plat-mxc/include/mach/mxc.h
> > @@ -133,6 +133,26 @@ extern unsigned int __mxc_cpu_type;
> >  # define cpu_is_mxc91231()	(0)
> >  #endif
> >  
> > +#ifndef __ASSEMBLY__
> > +
> > +struct cpu_wp {
> > +	u32 pll_reg;
> > +	u32 pll_rate;
> > +	u32 cpu_rate;
> > +	u32 pdr0_reg;
> > +	u32 pdf;
> > +	u32 mfi;
> > +	u32 mfd;
> > +	u32 mfn;
> > +	u32 cpu_voltage;
> > +	u32 cpu_podf;
> > +};
> > +
> > +#ifndef CONFIG_ARCH_MX5
> > +struct cpu_wp *get_cpu_wp(int *wp);
> > +#endif
> > +#endif
> > +
> >  #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
> >  /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
> >  #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
> > -- 
> > 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
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] cpufreq for freescale mx51

[Yong Shen]

Hi Sascha,

Thanks for your thorough comments.
I have already received comments from Arnd before yours arrived. So some of
the commends you two provided are common.
I acknowledge most of your opinions, except for two, I have some
explanations.

> > +    */
> > > +   reg = __raw_readl(MXC_CCM_CACRR);
> > > +   reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > > +   reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > > +   __raw_writel(reg, MXC_CCM_CACRR);
>
> We have a simple divider here. Why do we need a reference to struct
> cpu_wp then? This code could look much simpler.
>
> (side note: I'm aware that the original Freescale code is also able
> to change the cpu frequency using the pll instead of the divider, but is
> this really necessary?)
>
Using wp_tbl is because that it also contains information like regulator
voltage. Since the regulator driver for imx51 have not been upstreamed, you
don't see any regulator operation here. Also, like you mentioned, there are
two ways to change cpufreq, by post divider or pll change. And post divider
change is a simpler way and also the only way for babbage, since cpu freq
and ddr freq are all root from the same pll on babbage and they will
interfere each other.

> > +static struct cpufreq_frequency_table imx_freq_table[4];
>
> Three frequencies should be enough for everyone, right? This should be
> dynamically allocated like in other cpufreq drivers.
>

Yes, we only support 3 work points, which is for sure. Actually, we only
support 2 points on most mx51 chips. I supposed it is ok to use static array
here.

Thanks again for review. I will send out updated patch again.

Yong

On Wed, Oct 6, 2010 at 5:51 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Thu, Sep 30, 2010 at 01:48:17PM +0300, Amit Kucheria wrote:
> > Add'ed linaro-dev and linux-arm-kernel to CC.
> >
> > Thanks Yong, some feeback follows inline.
> >
> > On 10 Sep 29, Yong Shen wrote:
> > > From: Yong Shen <yong.shen@linaro.org>
> > >
> > > ---
> > >  arch/arm/Kconfig                       |    6 +
> > >  arch/arm/mach-mx5/Kconfig              |    1 +
> > >  arch/arm/mach-mx5/board-mx51_babbage.c |   32 ++++
> > >  arch/arm/mach-mx5/clock-mx51.c         |   53 ++++++
> > >  arch/arm/plat-mxc/Makefile             |    2 +
> > >  arch/arm/plat-mxc/cpufreq.c            |  282
> ++++++++++++++++++++++++++++++++
> > >  arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++
> > >  7 files changed, 396 insertions(+), 0 deletions(-)
> > >  create mode 100644 arch/arm/plat-mxc/cpufreq.c
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 4db064e..64ebbc0 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -1517,6 +1517,12 @@ if ARCH_HAS_CPUFREQ
> > >
> > >  source "drivers/cpufreq/Kconfig"
> > >
> > > +config CPU_FREQ_IMX
> > > +   tristate "CPUfreq driver for i.MX CPUs"
> > > +   depends on ARCH_MXC && CPU_FREQ
> > > +   help
> > > +   This enables the CPUfreq driver for i.MX CPUs.
> > > +
> > >  config CPU_FREQ_SA1100
> > >     bool
> > >
> > > diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> > > index 1576d51..5956fee 100644
> > > --- a/arch/arm/mach-mx5/Kconfig
> > > +++ b/arch/arm/mach-mx5/Kconfig
> > > @@ -5,6 +5,7 @@ config ARCH_MX51
> > >     default y
> > >     select MXC_TZIC
> > >     select ARCH_MXC_IOMUX_V3
> > > +   select ARCH_HAS_CPUFREQ
> > >
> > >  comment "MX5 platforms:"
> > >
> > > diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c
> b/arch/arm/mach-mx5/board-mx51_babbage.c
> > > index ed885f9..e449e0b 100644
> > > --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> > > +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> > > @@ -43,6 +43,31 @@
> > >  #define    MX51_USB_PLL_DIV_19_2_MHZ       0x01
> > >  #define    MX51_USB_PLL_DIV_24_MHZ 0x02
> > >
> > > +struct cpu_wp *(*get_cpu_wp)(int *wp);
>
> This should be moved to a generic place, otherwise it breaks the build
> when multiple boards are selected.
>
> > > +static int num_cpu_wp = 2;
> >
> > use sizeof(array) instead of hard coding this.
> >
> > > +/* working point(wp): 0 - 800MHz; 1 - 166.25MHz; */
> > > +static struct cpu_wp cpu_wp_auto[] = {
> > > +   {
> > > +   .pll_rate = 800000000,
> > > +   .cpu_rate = 800000000,
> > > +   .pdf = 0,
> > > +   .mfi = 8,
> > > +   .mfd = 2,
> > > +   .mfn = 1,
> > > +   .cpu_podf = 0,
> > > +   .cpu_voltage = 1100000,},
> > > +   {
> > > +   .pll_rate = 800000000,
> > > +   .cpu_rate = 160000000,
> > > +   .pdf = 4,
> > > +   .mfi = 8,
> > > +   .mfd = 2,
> > > +   .mfn = 1,
> > > +   .cpu_podf = 4,
> > > +   .cpu_voltage = 850000,},
> > > +};
> >
> >
> > This data should be moved out to a separate file (e.g. mx51_ratetable.h)
> > since it will be useful to other boards too.
> >
> > If other boards can have different rate tables (and they can, depending
> on
> > the revision of the silicon), then we can either 'assemble' the correct
> rate
> > table based on a flag field or have explicit separate rate tables for
> each
> > silicon revision.
> >
> > In any case, I suspect that there will be some core rates that will be
> common
> > across silicon revisions.
> >
> > >  static struct platform_device *devices[] __initdata = {
> > >     &mxc_fec_device,
> > >  };
> > > @@ -87,6 +112,12 @@ static struct imxuart_platform_data uart_pdata = {
> > >     .flags = IMXUART_HAVE_RTSCTS,
> > >  };
> > >
> > > +struct cpu_wp *mx51_babbage_get_cpu_wp(int *wp)
>
> static
>
> > > +{
> > > +   *wp = num_cpu_wp;
> > > +   return cpu_wp_auto;
> > > +}
> > > +
> > >  static inline void mxc_init_imx_uart(void)
> > >  {
> > >     mxc_register_device(&mxc_uart_device0, &uart_pdata);
> > > @@ -246,6 +277,7 @@ static void __init mxc_board_init(void)
> > >
> > >  static void __init mx51_babbage_timer_init(void)
> > >  {
> > > +   get_cpu_wp = mx51_babbage_get_cpu_wp;
>
> It looks strange to have a global function pointer which gets
> overwritten by the boards. I would expect a
>
> imx_add_cpu_workpoints(cpu_wp_auto, ARRAY_SIZE(cpu_wp_auto));
>
> here.
>
> > >     mx51_clocks_init(32768, 24000000, 22579200, 0);
> > >  }
> > >
> > > diff --git a/arch/arm/mach-mx5/clock-mx51.c
> b/arch/arm/mach-mx5/clock-mx51.c
> > > index d9f612d..f2488e6 100644
> > > --- a/arch/arm/mach-mx5/clock-mx51.c
> > > +++ b/arch/arm/mach-mx5/clock-mx51.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/io.h>
> > > +#include <linux/time.h>
>
> unneeded include
>
> > >
> > >  #include <asm/clkdev.h>
> > >  #include <asm/div64.h>
> > > @@ -28,6 +29,11 @@
> > >  static unsigned long external_high_reference, external_low_reference;
> > >  static unsigned long oscillator_reference, ckih2_reference;
> > >
> > > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > > +static int cpu_wp_nr;
> > > +static int cpu_curr_wp;
> > > +static struct cpu_wp *cpu_wp_tbl;
> > > +
> > >  static struct clk osc_clk;
> > >  static struct clk pll1_main_clk;
> > >  static struct clk pll1_sw_clk;
> > > @@ -38,7 +44,9 @@ static struct clk periph_apm_clk;
> > >  static struct clk ahb_clk;
> > >  static struct clk ipg_clk;
> > >  static struct clk usboh3_clk;
> > > +static void __iomem *pll1_base;
> > >
> > > +#define SPIN_DELAY 1000000 /* in nanoseconds */
>
> unused
>
> > >  #define MAX_DPLL_WAIT_TRIES        1000 /* 1000 * udelay(1) = 1ms */
> > >
> > >  static int _clk_ccgr_enable(struct clk *clk)
> > > @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk,
> struct clk *parent)
> > >     return 0;
> > >  }
> > >
> > > +/*!
> > > + * Setup cpu clock based on working point.
> > > + * @param  wp      cpu freq working point
> > > + * @return         0 on success or error code on failure.
> > > + */
> > > +int cpu_clk_set_wp(int wp)
>
> static
>
> > > +{
> > > +   struct cpu_wp *p;
> > > +   u32 reg;
> > > +
> > > +   if (wp == cpu_curr_wp)
> > > +           return 0;
> > > +
> > > +   p = &cpu_wp_tbl[wp];
> > > +
> > > +   /*use post divider to change freq
> > > +    */
> > > +   reg = __raw_readl(MXC_CCM_CACRR);
> > > +   reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > > +   reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > > +   __raw_writel(reg, MXC_CCM_CACRR);
>
> We have a simple divider here. Why do we need a reference to struct
> cpu_wp then? This code could look much simpler.
>
> (side note: I'm aware that the original Freescale code is also able
> to change the cpu frequency using the pll instead of the divider, but is
> this really necessary?)
>
> > > +   cpu_curr_wp = wp;
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  static unsigned long clk_arm_get_rate(struct clk *clk)
> > >  {
> > >     u32 cacrr, div;
> > > @@ -342,6 +376,20 @@ static unsigned long clk_arm_get_rate(struct clk
> *clk)
> > >     return parent_rate / div;
> > >  }
> > >
> > > +int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
> > > +{
> > > +   u32 i;
> > > +   for (i = 0; i < cpu_wp_nr; i++) {
> > > +           if (rate == cpu_wp_tbl[i].cpu_rate)
> > > +                   break;
> > > +   }
> > > +   if (i >= cpu_wp_nr)
> > > +           return -EINVAL;
> > > +   cpu_clk_set_wp(i);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  static int _clk_periph_apm_set_parent(struct clk *clk, struct clk
> *parent)
> > >  {
> > >     u32 reg, mux;
> > > @@ -694,6 +742,7 @@ static struct clk periph_apm_clk = {
> > >  static struct clk cpu_clk = {
> > >     .parent = &pll1_sw_clk,
> > >     .get_rate = clk_arm_get_rate,
> > > +   .set_rate = _clk_cpu_set_rate,
> > >  };
> > >
> > >  static struct clk ahb_clk = {
> > > @@ -821,6 +870,7 @@ static struct clk_lookup lookups[] = {
> > >     _REGISTER_CLOCK("mxc-ehci.1", "usb_ahb", ahb_clk)
> > >     _REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
> > >     _REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
> > > +   _REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
> > >  };
> > >
> > >  static void clk_tree_init(void)
> > > @@ -848,10 +898,13 @@ int __init mx51_clocks_init(unsigned long ckil,
> unsigned long osc,
> > >  {
> > >     int i;
> > >
> > > +   pll1_base = ioremap(MX51_PLL1_BASE_ADDR, SZ_4K);
> > > +
>
> this is unused.
>
> > >     external_low_reference = ckil;
> > >     external_high_reference = ckih1;
> > >     ckih2_reference = ckih2;
> > >     oscillator_reference = osc;
> > > +   cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> > >
> > >     for (i = 0; i < ARRAY_SIZE(lookups); i++)
> > >             clkdev_add(&lookups[i]);
> > > diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> > > index 895bc3c..c1bb400 100644
> > > --- a/arch/arm/plat-mxc/Makefile
> > > +++ b/arch/arm/plat-mxc/Makefile
> > > @@ -17,6 +17,8 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci.o
> > >  obj-$(CONFIG_MXC_ULPI) += ulpi.o
> > >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
> > >  obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
> > > +# CPU FREQ support
> > > +obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
> > >  ifdef CONFIG_SND_IMX_SOC
> > >  obj-y += ssi-fiq.o
> > >  obj-y += ssi-fiq-ksym.o
> > > diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
> > > new file mode 100644
> > > index 0000000..cae40f1
> > > --- /dev/null
> > > +++ b/arch/arm/plat-mxc/cpufreq.c
> > > @@ -0,0 +1,282 @@
> > > +/*
> > > + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights
> Reserved.
> > > + */
> > > +
> > > +/*
> > > + * The code contained herein is licensed under the GNU General Public
> > > + * License. You may obtain a copy of the GNU General Public License
> > > + * Version 2 or later at the following locations:
> > > + *
> > > + * http://www.opensource.org/licenses/gpl-license.html
> > > + * http://www.gnu.org/copyleft/gpl.html
> > > + */
> > > +
> > > +/*!
> > > + * @file cpufreq.c
> > > + *
> > > + * @brief A driver for the Freescale Semiconductor i.MXC CPUfreq
> module.
> > > + *
> > > + * The CPUFREQ driver is for controling CPU frequency. It allows you
> to change
> > > + * the CPU clock speed on the fly.
> > > + *
> > > + * @ingroup PM
> > > + */
> >
> > Fix these comments to follow the kernel commenting style.
> >
> > > +#include <linux/types.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/cpufreq.h>
> > > +#include <linux/init.h>
> > > +#include <linux/proc_fs.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/io.h>
> > > +#include <mach/hardware.h>
> > > +#include <asm/setup.h>
> > > +#include <mach/clock.h>
> > > +#include <asm/cacheflush.h>
> > > +#include <linux/hrtimer.h>
> > > +
> > > +int cpu_freq_khz_min;
> > > +int cpu_freq_khz_max;
> > > +int arm_lpm_clk;
> > > +int arm_normal_clk;
> > > +int cpufreq_suspended;
> > > +int cpufreq_trig_needed;
> > > +
> > > +static struct clk *cpu_clk;
> > > +static struct cpufreq_frequency_table imx_freq_table[4];
>
> Three frequencies should be enough for everyone, right? This should be
> dynamically allocated like in other cpufreq drivers.
>
> > > +
> > > +extern int set_low_bus_freq(void);
> > > +extern int set_high_bus_freq(int high_bus_speed);
> > > +extern int low_freq_bus_used(void);
>
> declared but never used.
>
> > > +
> > > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > > +static int cpu_wp_nr;
> > > +static struct cpu_wp *cpu_wp_tbl;
> > > +
> > > +int set_cpu_freq(int freq)
> > > +{
> > > +   int ret = 0;
> > > +   int org_cpu_rate;
> > > +   int gp_volt = 0;
> > > +   int i;
> > > +
> > > +   org_cpu_rate = clk_get_rate(cpu_clk);
> > > +   if (org_cpu_rate == freq)
> > > +           return ret;
> > > +
> > > +   for (i = 0; i < cpu_wp_nr; i++) {
> > > +           if (freq == cpu_wp_tbl[i].cpu_rate)
> > > +                   gp_volt = cpu_wp_tbl[i].cpu_voltage;
> > > +   }
> > > +
> > > +   if (gp_volt == 0)
> > > +           return ret;
> > > +
> > > +   ret = clk_set_rate(cpu_clk, freq);
> > > +   if (ret != 0) {
> > > +           printk(KERN_DEBUG "cannot set CPU clock rate\n");
> > > +           return ret;
> > > +   }
> > > +
>
> Please remove trailing whitespaces.
>
> > > +   return ret;
> > > +}
> > > +
> > > +static int mxc_verify_speed(struct cpufreq_policy *policy)
> > > +{
> > > +   if (policy->cpu != 0)
> > > +           return -EINVAL;
> > > +
> > > +   return cpufreq_frequency_table_verify(policy, imx_freq_table);
> > > +}
> > > +
> > > +static unsigned int mxc_get_speed(unsigned int cpu)
> > > +{
> > > +   if (cpu)
> > > +           return 0;
> > > +
> > > +   return clk_get_rate(cpu_clk) / 1000;
> > > +}
> > > +
> > > +static int calc_frequency_khz(int target, unsigned int relation)
> > > +{
> > > +   int i;
> > > +
> > > +   if ((target * 1000) == clk_get_rate(cpu_clk))
> > > +           return target;
> > > +
> > > +   if (relation == CPUFREQ_RELATION_H) {
> > > +           for (i = cpu_wp_nr - 1; i >= 0; i--) {
> > > +                   if (imx_freq_table[i].frequency <= target)
> > > +                           return imx_freq_table[i].frequency;
> > > +           }
> > > +   } else if (relation == CPUFREQ_RELATION_L) {
> > > +           for (i = 0; i < cpu_wp_nr; i++) {
> > > +                   if (imx_freq_table[i].frequency >= target)
> > > +                           return imx_freq_table[i].frequency;
> > > +           }
> > > +   }
>
> It looks like cpufreq_frequency_table_target could help here.
>
> > > +   printk(KERN_ERR "Error: No valid cpufreq relation\n");
> > > +   return cpu_freq_khz_max;
> > > +}
> > > +
> > > +static int mxc_set_target(struct cpufreq_policy *policy,
> > > +                     unsigned int target_freq, unsigned int relation)
> > > +{
> > > +   struct cpufreq_freqs freqs;
> > > +   int freq_Hz;
> > > +   int ret = 0;
> > > +
> > > +   if (cpufreq_suspended) {
> > > +           target_freq = clk_get_rate(cpu_clk) / 1000;
> > > +           freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > > +           if (freq_Hz == arm_lpm_clk)
> > > +                   freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate /
> 1000;
> > > +           else
> > > +                   freqs.old = arm_lpm_clk / 1000;
> > > +
> > > +           freqs.new = freq_Hz / 1000;
> > > +           freqs.cpu = 0;
> > > +           freqs.flags = 0;
> > > +           cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > > +           cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > +           return ret;
> > > +   }
> > > +   /*
> > > +    * Some governors do not respects CPU and policy lower limits
> > > +    * which leads to bad things (division by zero etc), ensure
> > > +    * that such things do not happen.
> > > +    */
> >
> > Isn't that a bug in the governor? Can you explain a bit?
> >
> > > +   if (target_freq < policy->cpuinfo.min_freq)
> > > +           target_freq = policy->cpuinfo.min_freq;
> > > +
> > > +   if (target_freq < policy->min)
> > > +           target_freq = policy->min;
> > > +
> > > +   freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
> > > +
> > > +   freqs.old = clk_get_rate(cpu_clk) / 1000;
> > > +   freqs.new = freq_Hz / 1000;
> > > +   freqs.cpu = 0;
> > > +   freqs.flags = 0;
> > > +   cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > > +
> > > +   if (freqs.old != freqs.new)
> > > +           ret = set_cpu_freq(freq_Hz);
> > > +
> > > +   cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +static int __init mxc_cpufreq_driver_init(struct cpufreq_policy
> *policy)
> > > +{
> > > +   int ret;
> > > +   int i;
> > > +
> > > +   printk(KERN_INFO "i.MXC CPU frequency driver\n");
> > > +
> > > +   if (policy->cpu != 0)
> > > +           return -EINVAL;
> > > +
> > > +   cpu_clk = clk_get(NULL, "cpu_clk");
> > > +   if (IS_ERR(cpu_clk)) {
> > > +           printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
> > > +           return PTR_ERR(cpu_clk);
> > > +   }
> > > +
> > > +   /* Set the current working point. */
> > > +   cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> > > +
> > > +   cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
> > > +   cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
> > > +
> > > +   for (i = 0; i < cpu_wp_nr; i++) {
> > > +           imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;
> >
> > cpu_wp_nr = 2 here
> >
> > 1st iteration of for loop:
> >     imx_freq_table[2 - 1 - 0].index = 2 - 0
> > so, imx_freq_table[1].index = 2
> >
> > 2nd iteration:
> >     imx_freq_table[2 - 1 - 1].index = 2 - 1
> >     imx_freq_table[0].index = 1
> >
> > So you're trying to reverse the table order? Why not just sort the table
> date
> > in the way you want and add a comment on the top to keep it sorted.
> >
> > > +           imx_freq_table[cpu_wp_nr - 1 - i].frequency =
> > > +               cpu_wp_tbl[i].cpu_rate / 1000;
> > > +
> > > +           if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
> > > +                   cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
> > > +
> > > +           if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
> > > +                   cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
> > > +   }
> > > +
> > > +   imx_freq_table[i].index = 0;
> >
> > imx_freq_table[i].index = i ?
> >
> > > +   imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
> > > +
> > > +   policy->cur = clk_get_rate(cpu_clk) / 1000;
> > > +   policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
> > > +   policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
> > > +   policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
> > > +
> > > +   arm_lpm_clk = cpu_freq_khz_min * 1000;
> > > +   arm_normal_clk = cpu_freq_khz_max * 1000;
> > > +
> > > +   /* Manual states, that PLL stabilizes in two CLK32 periods */
> > > +   policy->cpuinfo.transition_latency = 10;
> > > +
> > > +   ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
> > > +
> > > +   if (ret < 0) {
> > > +           clk_put(cpu_clk);
> > > +           printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
> > > +                  __func__);
> > > +           return ret;
> > > +   }
> > > +
> > > +   cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
> > > +   return 0;
> > > +}
> > > +
> > > +static int mxc_cpufreq_suspend(struct cpufreq_policy *policy,
> > > +                                pm_message_t state)
> > > +{
> > > +   return 0;
> > > +}
> >
> > Get rid of these, since you don't use them.
> >
> > > +static int mxc_cpufreq_resume(struct cpufreq_policy *policy)
> > > +{
> > > +   return 0;
> > > +}
> >
> > Same here.
> >
> > > +static int mxc_cpufreq_driver_exit(struct cpufreq_policy *policy)
> > > +{
> > > +   cpufreq_frequency_table_put_attr(policy->cpu);
> > > +
> > > +   /* Reset CPU to 665MHz */
> > > +   set_cpu_freq(arm_normal_clk);
> > > +   clk_put(cpu_clk);
> > > +   return 0;
> > > +}
> > > +
> > > +static struct cpufreq_driver mxc_driver = {
> > > +   .flags = CPUFREQ_STICKY,
> > > +   .verify = mxc_verify_speed,
> > > +   .target = mxc_set_target,
> > > +   .get = mxc_get_speed,
> > > +   .init = mxc_cpufreq_driver_init,
> >
> > mxc_cpufreq_init is ok. Lose the driver.
> >
> > > +   .exit = mxc_cpufreq_driver_exit,
> >
> > same.
> >
> > > +   .suspend = mxc_cpufreq_suspend,
> > > +   .resume = mxc_cpufreq_resume,
> >
> > Get rid of suspend/resume
> >
> > > +   .name = "imx",
> > > +};
> > > +
> > > +static int __devinit mxc_cpufreq_init(void)
> > > +{
> > > +   return cpufreq_register_driver(&mxc_driver);
> > > +}
> > > +
> > > +static void mxc_cpufreq_exit(void)
> > > +{
> > > +   cpufreq_unregister_driver(&mxc_driver);
> > > +}
> > > +
> > > +module_init(mxc_cpufreq_init);
> > > +module_exit(mxc_cpufreq_exit);
> > > +
> > > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> > > +MODULE_DESCRIPTION("CPUfreq driver for i.MX");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/arch/arm/plat-mxc/include/mach/mxc.h
> b/arch/arm/plat-mxc/include/mach/mxc.h
> > > index a790bf2..14003b9 100644
> > > --- a/arch/arm/plat-mxc/include/mach/mxc.h
> > > +++ b/arch/arm/plat-mxc/include/mach/mxc.h
> > > @@ -133,6 +133,26 @@ extern unsigned int __mxc_cpu_type;
> > >  # define cpu_is_mxc91231() (0)
> > >  #endif
> > >
> > > +#ifndef __ASSEMBLY__
> > > +
> > > +struct cpu_wp {
> > > +   u32 pll_reg;
> > > +   u32 pll_rate;
> > > +   u32 cpu_rate;
> > > +   u32 pdr0_reg;
> > > +   u32 pdf;
> > > +   u32 mfi;
> > > +   u32 mfd;
> > > +   u32 mfn;
> > > +   u32 cpu_voltage;
> > > +   u32 cpu_podf;
> > > +};
> > > +
> > > +#ifndef CONFIG_ARCH_MX5
> > > +struct cpu_wp *get_cpu_wp(int *wp);
> > > +#endif
> > > +#endif
> > > +
> > >  #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
> > >  /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
> > >  #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
> > > --
> > > 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
> >
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101007/4f4a5b6a/attachment-0001.html>

[PATCH] cpufreq for freescale mx51

[Sascha Hauer]

On Thu, Oct 07, 2010 at 11:36:07AM +0800, Yong Shen wrote:
> Hi Sascha,
> 
> Thanks for your thorough comments.
> I have already received comments from Arnd before yours arrived. So some of
> the commends you two provided are common.
> I acknowledge most of your opinions, except for two, I have some
> explanations.
> 
> > > +    */
> > > > +   reg = __raw_readl(MXC_CCM_CACRR);
> > > > +   reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > > > +   reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > > > +   __raw_writel(reg, MXC_CCM_CACRR);
> >
> > We have a simple divider here. Why do we need a reference to struct
> > cpu_wp then? This code could look much simpler.
> >
> > (side note: I'm aware that the original Freescale code is also able
> > to change the cpu frequency using the pll instead of the divider, but is
> > this really necessary?)
> >
> Using wp_tbl is because that it also contains information like regulator
> voltage.

The clock code does not handle the regulators, not even in the fsl
kernel.

> Since the regulator driver for imx51 have not been upstreamed, you
> don't see any regulator operation here. Also, like you mentioned, there are
> two ways to change cpufreq, by post divider or pll change. And post divider
> change is a simpler way and also the only way for babbage, since cpu freq
> and ddr freq are all root from the same pll on babbage and they will
> interfere each other.

I understand what you have to do, but the way you did really looks
strange. You introduce get_cpu_wp() to get the complete array of
workpoints. In the cpufreq driver you have this complete array, pick
the desired workpoint, pass the frequency to the clock layer which in
turn calls get_cpu_wp() to get the array and loop around it to get
the workpoint from the frequency again. Addionally you maintain a
pointer to what the clock code thinks the current workpoint is.

How about making the clock code agnostic of such workpoints and
calculate the pll values and dividers for a given frequency based
on the frequency. If that's to complicated you could maintain
a table in the clock code. If that's not flexible enough you could
pass a pointer to this table to mx51_clocks_init. This array could
carry information relevant only to the clock code and only relevant
to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct
for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields,
the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what
the next i.MX needs.

> 
> > > +static struct cpufreq_frequency_table imx_freq_table[4];
> >
> > Three frequencies should be enough for everyone, right? This should be
> > dynamically allocated like in other cpufreq drivers.
> >
> 
> Yes, we only support 3 work points, which is for sure. Actually, we only
> support 2 points on most mx51 chips. I supposed it is ok to use static array
> here.

Please just add dynamic allocation, then we are safe for any potential
future use.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] cpufreq for freescale mx51

[Yong Shen]

> > Using wp_tbl is because that it also contains information like regulator
> > voltage.
>
> The clock code does not handle the regulators, not even in the fsl
> kernel.
>
I did not mean to say clock code will handle regulators. I mean this table
also contains such information which may be needed in other freescale
drivers, like dvfs and busfreq.

>
>
> Since the regulator driver for imx51 have not been upstreamed, you
> > don't see any regulator operation here. Also, like you mentioned, there
> are
> > two ways to change cpufreq, by post divider or pll change. And post
> divider
> > change is a simpler way and also the only way for babbage, since cpu freq
> > and ddr freq are all root from the same pll on babbage and they will
> > interfere each other.
>
> I understand what you have to do, but the way you did really looks
> strange. You introduce get_cpu_wp() to get the complete array of
> workpoints. In the cpufreq driver you have this complete array, pick
> the desired workpoint, pass the frequency to the clock layer which in
> turn calls get_cpu_wp() to get the array and loop around it to get
> the workpoint from the frequency again. Addionally you maintain a
> pointer to what the clock code thinks the current workpoint is.
>
> How about making the clock code agnostic of such workpoints and
> calculate the pll values and dividers for a given frequency based
> on the frequency. If that's to complicated you could maintain
> a table in the clock code. If that's not flexible enough you could
> pass a pointer to this table to mx51_clocks_init. This array could
> carry information relevant only to the clock code and only relevant
> to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct
> for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields,
> the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what
> the next i.MX needs.
>

I strongly agree with you about this, and it is better to letter clock
calculate by itself.

>
>
>
> > > > +static struct cpufreq_frequency_table imx_freq_table[4];
> > >
> > > Three frequencies should be enough for everyone, right? This should be
> > > dynamically allocated like in other cpufreq drivers.
> > >
> >
> > Yes, we only support 3 work points, which is for sure. Actually, we only
> > support 2 points on most mx51 chips. I supposed it is ok to use static
> array
> > here.
>
> Please just add dynamic allocation, then we are safe for any potential
> future use.
>
> Ok! sorry for misunderstanding.

Here, I am not trying to find excuse for the cpufreq driver designer which
is my colleague, however the truth is that since in our company the schedule
is tight for all the tasks, that's why we always code with the first rough
idea in the head, and having less time to think about the flexibility and
other good conventions in programming. You can see that even freescale code
upstreaming is done by people out of freescale, because of we lacking of
resource.
Therefore, I would say we did not mean to have some short-sight design, we
also want to achieve a better life of coding like you guys are doing, which
is my direct feeling while I am in Linaro. :)
Thanks for comments.

Yong
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101007/62053234/attachment-0001.html>

[PATCHv2] cpufreq for freescale mx51

[Yong Shen]

Does anybody have more comments on [PATCHv2] ?

On Thu, Oct 7, 2010 at 8:00 PM, Yong Shen <yong.shen@linaro.org> wrote:

>
> > Using wp_tbl is because that it also contains information like regulator
>> > voltage.
>>
>> The clock code does not handle the regulators, not even in the fsl
>> kernel.
>>
> I did not mean to say clock code will handle regulators. I mean this table
> also contains such information which may be needed in other freescale
> drivers, like dvfs and busfreq.
>
>>
>>
> > Since the regulator driver for imx51 have not been upstreamed, you
>> > don't see any regulator operation here. Also, like you mentioned, there
>> are
>> > two ways to change cpufreq, by post divider or pll change. And post
>> divider
>> > change is a simpler way and also the only way for babbage, since cpu
>> freq
>> > and ddr freq are all root from the same pll on babbage and they will
>> > interfere each other.
>>
>> I understand what you have to do, but the way you did really looks
>> strange. You introduce get_cpu_wp() to get the complete array of
>> workpoints. In the cpufreq driver you have this complete array, pick
>> the desired workpoint, pass the frequency to the clock layer which in
>> turn calls get_cpu_wp() to get the array and loop around it to get
>> the workpoint from the frequency again. Addionally you maintain a
>> pointer to what the clock code thinks the current workpoint is.
>>
>> How about making the clock code agnostic of such workpoints and
>> calculate the pll values and dividers for a given frequency based
>> on the frequency. If that's to complicated you could maintain
>> a table in the clock code. If that's not flexible enough you could
>> pass a pointer to this table to mx51_clocks_init. This array could
>> carry information relevant only to the clock code and only relevant
>> to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct
>> for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields,
>> the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what
>> the next i.MX needs.
>>
>
> I strongly agree with you about this, and it is better to letter clock
> calculate by itself.
>
>>
>>
> >
>> > > > +static struct cpufreq_frequency_table imx_freq_table[4];
>> > >
>> > > Three frequencies should be enough for everyone, right? This should be
>> > > dynamically allocated like in other cpufreq drivers.
>> > >
>> >
>> > Yes, we only support 3 work points, which is for sure. Actually, we only
>> > support 2 points on most mx51 chips. I supposed it is ok to use static
>> array
>> > here.
>>
>> Please just add dynamic allocation, then we are safe for any potential
>> future use.
>>
>> Ok! sorry for misunderstanding.
>
> Here, I am not trying to find excuse for the cpufreq driver designer which
> is my colleague, however the truth is that since in our company the schedule
> is tight for all the tasks, that's why we always code with the first rough
> idea in the head, and having less time to think about the flexibility and
> other good conventions in programming. You can see that even freescale code
> upstreaming is done by people out of freescale, because of we lacking of
> resource.
> Therefore, I would say we did not mean to have some short-sight design, we
> also want to achieve a better life of coding like you guys are doing, which
> is my direct feeling while I am in Linaro. :)
> Thanks for comments.
>
> Yong
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101012/c5d94aa6/attachment-0001.html>

[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

On 10 Oct 07, Yong Shen wrote:
> 
> > > +static struct cpufreq_frequency_table imx_freq_table[4];
> >
> > Three frequencies should be enough for everyone, right? This should be
> > dynamically allocated like in other cpufreq drivers.
> >
> 
> Yes, we only support 3 work points, which is for sure. Actually, we only
> support 2 points on most mx51 chips. I supposed it is ok to use static array
> here.

This can become a common cpufreq driver for all i.MX platforms. We don't know
how many work points will be supported in future versions of the silicon.
That is why a static array is not ok.

I think Sascha was being ironic when he said "Three frequencies should be
enough for everyone, right?" :)

/Amit

[PATCH] cpufreq for freescale mx51

[Sascha Hauer]

On Thu, Oct 07, 2010 at 10:40:44AM +0300, Amit Kucheria wrote:
> On 10 Oct 07, Yong Shen wrote:
> > 
> > > > +static struct cpufreq_frequency_table imx_freq_table[4];
> > >
> > > Three frequencies should be enough for everyone, right? This should be
> > > dynamically allocated like in other cpufreq drivers.
> > >
> > 
> > Yes, we only support 3 work points, which is for sure. Actually, we only
> > support 2 points on most mx51 chips. I supposed it is ok to use static array
> > here.
> 
> This can become a common cpufreq driver for all i.MX platforms. We don't know
> how many work points will be supported in future versions of the silicon.
> That is why a static array is not ok.
> 
> I think Sascha was being ironic when he said "Three frequencies should be
> enough for everyone, right?" :)

Yes, the old 640k-is-enough-for-anyone joke.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

mx51 cpufreq driver

[yong.shen at linaro.org]

mx51 cpufreq driver, it passed test on mx51 babbage 3.0 board.

[PATCH] cpufreq for freescale mx51

[yong.shen at linaro.org]

From: Yong Shen <yong.shen@linaro.org>

it is tested on babbage 3.0

Signed-off-by: Yong Shen <yong.shen@linaro.org>
---
 arch/arm/Kconfig                       |    6 +
 arch/arm/mach-mx5/Kconfig              |    1 +
 arch/arm/mach-mx5/Makefile             |    2 +-
 arch/arm/mach-mx5/board-mx51_babbage.c |    7 +-
 arch/arm/mach-mx5/clock-mx51.c         |   53 +++++++
 arch/arm/mach-mx5/cpu_wp-mx51.c        |   43 ++++++
 arch/arm/plat-mxc/Makefile             |    2 +
 arch/arm/plat-mxc/cpufreq.c            |  254 ++++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/mxc.h   |   22 +++-
 9 files changed, 387 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-mx5/cpu_wp-mx51.c
 create mode 100644 arch/arm/plat-mxc/cpufreq.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4db064e..64ebbc0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1517,6 +1517,12 @@ if ARCH_HAS_CPUFREQ
 
 source "drivers/cpufreq/Kconfig"
 
+config CPU_FREQ_IMX
+	tristate "CPUfreq driver for i.MX CPUs"
+	depends on ARCH_MXC && CPU_FREQ
+	help
+	This enables the CPUfreq driver for i.MX CPUs.
+
 config CPU_FREQ_SA1100
 	bool
 
diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index 1576d51..5956fee 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -5,6 +5,7 @@ config ARCH_MX51
 	default y
 	select MXC_TZIC
 	select ARCH_MXC_IOMUX_V3
+	select ARCH_HAS_CPUFREQ
 
 comment "MX5 platforms:"
 
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index bf23f86..aaa13f8 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -3,7 +3,7 @@
 #
 
 # Object file lists.
-obj-y   := cpu.o mm.o clock-mx51.o devices.o
+obj-y   := cpu.o mm.o clock-mx51.o devices.o cpu_wp-mx51.o
 
 obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
 
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
index ed885f9..038760c 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
  * Copyright (C) 2009-2010 Amit Kucheria <amit.kucheria@canonical.com>
  *
  * The code contained herein is licensed under the GNU General Public
@@ -43,6 +43,10 @@
 #define	MX51_USB_PLL_DIV_19_2_MHZ	0x01
 #define	MX51_USB_PLL_DIV_24_MHZ	0x02
 
+
+extern struct cpu_wp *mx51_get_cpu_wp(int *wp);
+struct cpu_wp *(*get_cpu_wp)(int *wp);
+
 static struct platform_device *devices[] __initdata = {
 	&mxc_fec_device,
 };
@@ -246,6 +250,7 @@ static void __init mxc_board_init(void)
 
 static void __init mx51_babbage_timer_init(void)
 {
+	get_cpu_wp = mx51_get_cpu_wp;
 	mx51_clocks_init(32768, 24000000, 22579200, 0);
 }
 
diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
index d9f612d..f2488e6 100644
--- a/arch/arm/mach-mx5/clock-mx51.c
+++ b/arch/arm/mach-mx5/clock-mx51.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/time.h>
 
 #include <asm/clkdev.h>
 #include <asm/div64.h>
@@ -28,6 +29,11 @@
 static unsigned long external_high_reference, external_low_reference;
 static unsigned long oscillator_reference, ckih2_reference;
 
+extern struct cpu_wp *(*get_cpu_wp)(int *wp);
+static int cpu_wp_nr;
+static int cpu_curr_wp;
+static struct cpu_wp *cpu_wp_tbl;
+
 static struct clk osc_clk;
 static struct clk pll1_main_clk;
 static struct clk pll1_sw_clk;
@@ -38,7 +44,9 @@ static struct clk periph_apm_clk;
 static struct clk ahb_clk;
 static struct clk ipg_clk;
 static struct clk usboh3_clk;
+static void __iomem *pll1_base;
 
+#define SPIN_DELAY	1000000 /* in nanoseconds */
 #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
 
 static int _clk_ccgr_enable(struct clk *clk)
@@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
 	return 0;
 }
 
+/*!
+ * Setup cpu clock based on working point.
+ * @param	wp	cpu freq working point
+ * @return		0 on success or error code on failure.
+ */
+int cpu_clk_set_wp(int wp)
+{
+	struct cpu_wp *p;
+	u32 reg;
+
+	if (wp == cpu_curr_wp)
+		return 0;
+
+	p = &cpu_wp_tbl[wp];
+
+	/*use post divider to change freq
+	 */
+	reg = __raw_readl(MXC_CCM_CACRR);
+	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
+	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
+	__raw_writel(reg, MXC_CCM_CACRR);
+	cpu_curr_wp = wp;
+
+	return 0;
+}
+
 static unsigned long clk_arm_get_rate(struct clk *clk)
 {
 	u32 cacrr, div;
@@ -342,6 +376,20 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
 	return parent_rate / div;
 }
 
+int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
+{
+	u32 i;
+	for (i = 0; i < cpu_wp_nr; i++) {
+		if (rate == cpu_wp_tbl[i].cpu_rate)
+			break;
+	}
+	if (i >= cpu_wp_nr)
+		return -EINVAL;
+	cpu_clk_set_wp(i);
+
+	return 0;
+}
+
 static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
 {
 	u32 reg, mux;
@@ -694,6 +742,7 @@ static struct clk periph_apm_clk = {
 static struct clk cpu_clk = {
 	.parent = &pll1_sw_clk,
 	.get_rate = clk_arm_get_rate,
+	.set_rate = _clk_cpu_set_rate,
 };
 
 static struct clk ahb_clk = {
@@ -821,6 +870,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("mxc-ehci.1", "usb_ahb", ahb_clk)
 	_REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
 	_REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
+	_REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
 };
 
 static void clk_tree_init(void)
@@ -848,10 +898,13 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
 {
 	int i;
 
+	pll1_base = ioremap(MX51_PLL1_BASE_ADDR, SZ_4K);
+
 	external_low_reference = ckil;
 	external_high_reference = ckih1;
 	ckih2_reference = ckih2;
 	oscillator_reference = osc;
+	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
 
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)
 		clkdev_add(&lookups[i]);
diff --git a/arch/arm/mach-mx5/cpu_wp-mx51.c b/arch/arm/mach-mx5/cpu_wp-mx51.c
new file mode 100644
index 0000000..53f3de3
--- /dev/null
+++ b/arch/arm/mach-mx5/cpu_wp-mx51.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later@the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/types.h>
+#include <mach/hardware.h>
+
+/* working point(wp): 0 - 800MHz; 1 - 166.25MHz; */
+static struct cpu_wp cpu_wp_auto[] = {
+	{
+	.pll_rate = 800000000,
+	.cpu_rate = 160000000,
+	.pdf = 4,
+	.mfi = 8,
+	.mfd = 2,
+	.mfn = 1,
+	.cpu_podf = 4,
+	.cpu_voltage = 850000,},
+	{
+	.pll_rate = 800000000,
+	.cpu_rate = 800000000,
+	.pdf = 0,
+	.mfi = 8,
+	.mfd = 2,
+	.mfn = 1,
+	.cpu_podf = 0,
+	.cpu_voltage = 1100000,},
+};
+
+struct cpu_wp *mx51_get_cpu_wp(int *wp)
+{
+	*wp = sizeof(cpu_wp_auto) / sizeof(struct cpu_wp);
+	return cpu_wp_auto;
+}
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index 895bc3c..c1bb400 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -17,6 +17,8 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci.o
 obj-$(CONFIG_MXC_ULPI) += ulpi.o
 obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
 obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
+# CPU FREQ support
+obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
 ifdef CONFIG_SND_IMX_SOC
 obj-y += ssi-fiq.o
 obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
new file mode 100644
index 0000000..2e67c0a
--- /dev/null
+++ b/arch/arm/plat-mxc/cpufreq.c
@@ -0,0 +1,254 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/*
+ * A driver for the Freescale Semiconductor i.MXC CPUfreq module.
+ * The CPUFREQ driver is for controling CPU frequency. It allows you to change
+ * the CPU clock speed on the fly.
+ */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/proc_fs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <mach/hardware.h>
+#include <asm/setup.h>
+#include <mach/clock.h>
+#include <asm/cacheflush.h>
+#include <linux/hrtimer.h>
+
+int cpu_freq_khz_min;
+int cpu_freq_khz_max;
+int arm_lpm_clk;
+int arm_normal_clk;
+int cpufreq_suspended;
+int cpufreq_trig_needed;
+
+static struct clk *cpu_clk;
+static struct cpufreq_frequency_table imx_freq_table[4];
+
+extern int set_low_bus_freq(void);
+extern int set_high_bus_freq(int high_bus_speed);
+extern int low_freq_bus_used(void);
+
+extern struct cpu_wp *(*get_cpu_wp)(int *wp);
+static int cpu_wp_nr;
+static struct cpu_wp *cpu_wp_tbl;
+
+int set_cpu_freq(int freq)
+{
+	int ret = 0;
+	int org_cpu_rate;
+	int gp_volt = 0;
+	int i;
+
+	org_cpu_rate = clk_get_rate(cpu_clk);
+	if (org_cpu_rate == freq)
+		return ret;
+
+	for (i = 0; i < cpu_wp_nr; i++) {
+		if (freq == cpu_wp_tbl[i].cpu_rate)
+			gp_volt = cpu_wp_tbl[i].cpu_voltage;
+	}
+
+	if (gp_volt == 0)
+		return ret;
+
+	ret = clk_set_rate(cpu_clk, freq);
+	if (ret != 0) {
+		printk(KERN_DEBUG "cannot set CPU clock rate\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int mxc_verify_speed(struct cpufreq_policy *policy)
+{
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	return cpufreq_frequency_table_verify(policy, imx_freq_table);
+}
+
+static unsigned int mxc_get_speed(unsigned int cpu)
+{
+	if (cpu)
+		return 0;
+
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int calc_frequency_khz(int target, unsigned int relation)
+{
+	int i;
+
+	if ((target * 1000) == clk_get_rate(cpu_clk))
+		return target;
+
+	if (relation == CPUFREQ_RELATION_H) {
+		for (i = cpu_wp_nr - 1; i >= 0; i--) {
+			if (imx_freq_table[i].frequency <= target)
+				return imx_freq_table[i].frequency;
+		}
+	} else if (relation == CPUFREQ_RELATION_L) {
+		for (i = 0; i < cpu_wp_nr; i++) {
+			if (imx_freq_table[i].frequency >= target)
+				return imx_freq_table[i].frequency;
+		}
+	}
+	printk(KERN_ERR "Error: No valid cpufreq relation\n");
+	return cpu_freq_khz_max;
+}
+
+static int mxc_set_target(struct cpufreq_policy *policy,
+			  unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	int freq_Hz;
+	int ret = 0;
+
+	if (cpufreq_suspended) {
+		target_freq = clk_get_rate(cpu_clk) / 1000;
+		freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
+		if (freq_Hz == arm_lpm_clk)
+			freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate / 1000;
+		else
+			freqs.old = arm_lpm_clk / 1000;
+
+		freqs.new = freq_Hz / 1000;
+		freqs.cpu = 0;
+		freqs.flags = 0;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		return ret;
+	}
+
+	freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
+
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.cpu = 0;
+	freqs.flags = 0;
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+	if (freqs.old != freqs.new)
+		ret = set_cpu_freq(freq_Hz);
+
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+
+	return ret;
+}
+
+static int __init mxc_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+	int i;
+
+	printk(KERN_INFO "i.MXC CPU frequency driver\n");
+
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	cpu_clk = clk_get(NULL, "cpu_clk");
+	if (IS_ERR(cpu_clk)) {
+		printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
+		return PTR_ERR(cpu_clk);
+	}
+
+	/* Set the current working point. */
+	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
+
+	cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
+	cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
+
+	for (i = 0; i < cpu_wp_nr; i++) {
+		imx_freq_table[i].index = i;
+		imx_freq_table[i].frequency =
+		    cpu_wp_tbl[i].cpu_rate / 1000;
+
+		if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
+			cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
+
+		if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
+			cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
+	}
+
+	imx_freq_table[i].index = i;
+	imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
+	policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
+
+	arm_lpm_clk = cpu_freq_khz_min * 1000;
+	arm_normal_clk = cpu_freq_khz_max * 1000;
+
+	/* Manual states, that PLL stabilizes in two CLK32 periods */
+	policy->cpuinfo.transition_latency = 10;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
+
+	if (ret < 0) {
+		clk_put(cpu_clk);
+		printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
+		       __func__);
+		return ret;
+	}
+
+	cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
+	return 0;
+}
+
+static int mxc_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+
+	/* Reset CPU to 665MHz */
+	set_cpu_freq(arm_normal_clk);
+	clk_put(cpu_clk);
+	return 0;
+}
+
+static struct cpufreq_driver mxc_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = mxc_verify_speed,
+	.target = mxc_set_target,
+	.get = mxc_get_speed,
+	.init = mxc_cpufreq_init,
+	.exit = mxc_cpufreq_exit,
+	.name = "imx",
+};
+
+static int __devinit mxc_cpufreq_driver_init(void)
+{
+	return cpufreq_register_driver(&mxc_driver);
+}
+
+static void mxc_cpufreq_driver_exit(void)
+{
+	cpufreq_unregister_driver(&mxc_driver);
+}
+
+module_init(mxc_cpufreq_driver_init);
+module_exit(mxc_cpufreq_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("CPUfreq driver for i.MX");
+MODULE_LICENSE("GPL");
diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
index a790bf2..868655b 100644
--- a/arch/arm/plat-mxc/include/mach/mxc.h
+++ b/arch/arm/plat-mxc/include/mach/mxc.h
@@ -1,5 +1,5 @@
 /*
- * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2004-2010 Freescale Semiconductor, Inc. All Rights Reserved.
  * Copyright (C) 2008 Juergen Beisert (kernel at pengutronix.de)
  *
  * This program is free software; you can redistribute it and/or
@@ -133,6 +133,26 @@ extern unsigned int __mxc_cpu_type;
 # define cpu_is_mxc91231()	(0)
 #endif
 
+#ifndef __ASSEMBLY__
+
+struct cpu_wp {
+	u32 pll_reg;
+	u32 pll_rate;
+	u32 cpu_rate;
+	u32 pdr0_reg;
+	u32 pdf;
+	u32 mfi;
+	u32 mfd;
+	u32 mfn;
+	u32 cpu_voltage;
+	u32 cpu_podf;
+};
+
+#ifndef CONFIG_ARCH_MX5
+struct cpu_wp *get_cpu_wp(int *wp);
+#endif
+#endif
+
 #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
 /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
 #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
-- 
1.6.3.3

[PATCH] cpufreq for freescale mx51

[Arnd Bergmann]

> From: Yong Shen <yong.shen@linaro.org>
> 
> it is tested on babbage 3.0

One big comment and a couple of smaller ones:

It would be better to make this code a proper device driver,
probably a platform_driver unless you have a way to probe
the presence of the registers on another bus.

Making it a driver that registers to a bus lets you separate
the probing from the implementation, and gives you a structure
to add your private variables to.

> @@ -43,6 +43,10 @@
>  #define	MX51_USB_PLL_DIV_19_2_MHZ	0x01
>  #define	MX51_USB_PLL_DIV_24_MHZ	0x02
>  
> +
> +extern struct cpu_wp *mx51_get_cpu_wp(int *wp);
> +struct cpu_wp *(*get_cpu_wp)(int *wp);
> +
>  static struct platform_device *devices[] __initdata = {
>  	&mxc_fec_device,
>  };

Please put the extern declarations into a header file, not
in the implementation.

> @@ -246,6 +250,7 @@ static void __init mxc_board_init(void)
>  
>  static void __init mx51_babbage_timer_init(void)
>  {
> +	get_cpu_wp = mx51_get_cpu_wp;
>  	mx51_clocks_init(32768, 24000000, 22579200, 0);
>  }

It feels like mx51_babbage_timer_init() is not the right place
to initialize this. Why not the mxc_board_init function?

> +#define SPIN_DELAY	1000000 /* in nanoseconds */
>  #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
>  
>  static int _clk_ccgr_enable(struct clk *clk)

The SPIN_DELAY variable doesn't seem to be used anywhere.

> @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
>  	return 0;
>  }
>  
> +/*!
> + * Setup cpu clock based on working point.
> + * @param	wp	cpu freq working point
> + * @return		0 on success or error code on failure.
> + */
> +int cpu_clk_set_wp(int wp)
> +{

could be 'static'.

> +	struct cpu_wp *p;
> +	u32 reg;
> +
> +	if (wp == cpu_curr_wp)
> +		return 0;
> +
> +	p = &cpu_wp_tbl[wp];
> +
> +	/*use post divider to change freq
> +	 */
> +	reg = __raw_readl(MXC_CCM_CACRR);
> +	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> +	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> +	__raw_writel(reg, MXC_CCM_CACRR);
> +	cpu_curr_wp = wp;
> +
> +	return 0;
> +}

This might need a spinlock to protect concurrent register access.

Don't use __raw_readl but readl/ioread32, which have more well-defined
behaviour.

> +int cpu_freq_khz_min;
> +int cpu_freq_khz_max;
> +int arm_lpm_clk;
> +int arm_normal_clk;
> +int cpufreq_suspended;
> +int cpufreq_trig_needed;

You should not have private variables in the global name space like this.
At least mark everything static that is only used in one file. Any
global variable must (local variables should) be prefixed with the
name of the driver like:

static int mxc_cpufreq_khz_min;
static int mxc_cpufreq_khz_max;
static int mxc_lpm_clk;

This will become more important when we move to multiplatform kernels.

> +extern int set_low_bus_freq(void);
> +extern int set_high_bus_freq(int high_bus_speed);
> +extern int low_freq_bus_used(void);
> +
> +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> +static int cpu_wp_nr;
> +static struct cpu_wp *cpu_wp_tbl;

The same rules apply for functions.

Again, anything that can not be static should be declared in a
header file.

> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");

Please put your name and email address in here.

> +#ifndef CONFIG_ARCH_MX5
> +struct cpu_wp *get_cpu_wp(int *wp);
> +#endif

Do not enclose declarations in #ifdef. Anybody should be able to
just enable your driver anyway without getting conflicts.

	Arnd

[PATCH] cpufreq for freescale mx51

[Yong Shen]

Hi Arnd,

Really appreciate your valuable comments. Most of them are accepted. I have
different option about two comments.
1.

> It would be better to make this code a proper device driver,
> probably a platform_driver unless you have a way to probe
> the presence of the registers on another bus.
>
> Making it a driver that registers to a bus lets you separate
> the probing from the implementation, and gives you a structure
> to add your private variables to.
>
cpufreq is supposed to be registered using cpufreq_register_driver directly,
so no other platform data is needed. You can also find out other cpufreq
driver is designed this way, like omap cpufreq driver.
2.

> Don't use __raw_readl but readl/ioread32, which have more well-defined
> behaviour.
>
I think __raw_readl is ok here, since in the platform related code, we know
the register layout and length, this is more efficient. Also this is
extensively used in arch/arm/.

Again, thanks for your time for review. I will send out updated patch.

Yong

On Tue, Oct 5, 2010 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> > From: Yong Shen <yong.shen@linaro.org>
> >
> > it is tested on babbage 3.0
>
> One big comment and a couple of smaller ones:
>
> It would be better to make this code a proper device driver,
> probably a platform_driver unless you have a way to probe
> the presence of the registers on another bus.
>
> Making it a driver that registers to a bus lets you separate
> the probing from the implementation, and gives you a structure
> to add your private variables to.
>
> > @@ -43,6 +43,10 @@
> >  #define      MX51_USB_PLL_DIV_19_2_MHZ       0x01
> >  #define      MX51_USB_PLL_DIV_24_MHZ 0x02
> >
> > +
> > +extern struct cpu_wp *mx51_get_cpu_wp(int *wp);
> > +struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +
> >  static struct platform_device *devices[] __initdata = {
> >       &mxc_fec_device,
> >  };
>
> Please put the extern declarations into a header file, not
> in the implementation.
>
> > @@ -246,6 +250,7 @@ static void __init mxc_board_init(void)
> >
> >  static void __init mx51_babbage_timer_init(void)
> >  {
> > +     get_cpu_wp = mx51_get_cpu_wp;
> >       mx51_clocks_init(32768, 24000000, 22579200, 0);
> >  }
>
> It feels like mx51_babbage_timer_init() is not the right place
> to initialize this. Why not the mxc_board_init function?
>
> > +#define SPIN_DELAY   1000000 /* in nanoseconds */
> >  #define MAX_DPLL_WAIT_TRIES  1000 /* 1000 * udelay(1) = 1ms */
> >
> >  static int _clk_ccgr_enable(struct clk *clk)
>
> The SPIN_DELAY variable doesn't seem to be used anywhere.
>
> > @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk,
> struct clk *parent)
> >       return 0;
> >  }
> >
> > +/*!
> > + * Setup cpu clock based on working point.
> > + * @param    wp      cpu freq working point
> > + * @return           0 on success or error code on failure.
> > + */
> > +int cpu_clk_set_wp(int wp)
> > +{
>
> could be 'static'.
>
> > +     struct cpu_wp *p;
> > +     u32 reg;
> > +
> > +     if (wp == cpu_curr_wp)
> > +             return 0;
> > +
> > +     p = &cpu_wp_tbl[wp];
> > +
> > +     /*use post divider to change freq
> > +      */
> > +     reg = __raw_readl(MXC_CCM_CACRR);
> > +     reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> > +     reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> > +     __raw_writel(reg, MXC_CCM_CACRR);
> > +     cpu_curr_wp = wp;
> > +
> > +     return 0;
> > +}
>
> This might need a spinlock to protect concurrent register access.
>
> Don't use __raw_readl but readl/ioread32, which have more well-defined
> behaviour.
>
> > +int cpu_freq_khz_min;
> > +int cpu_freq_khz_max;
> > +int arm_lpm_clk;
> > +int arm_normal_clk;
> > +int cpufreq_suspended;
> > +int cpufreq_trig_needed;
>
> You should not have private variables in the global name space like this.
> At least mark everything static that is only used in one file. Any
> global variable must (local variables should) be prefixed with the
> name of the driver like:
>
> static int mxc_cpufreq_khz_min;
> static int mxc_cpufreq_khz_max;
> static int mxc_lpm_clk;
>
> This will become more important when we move to multiplatform kernels.
>
> > +extern int set_low_bus_freq(void);
> > +extern int set_high_bus_freq(int high_bus_speed);
> > +extern int low_freq_bus_used(void);
> > +
> > +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> > +static int cpu_wp_nr;
> > +static struct cpu_wp *cpu_wp_tbl;
>
> The same rules apply for functions.
>
> Again, anything that can not be static should be declared in a
> header file.
>
> > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
>
> Please put your name and email address in here.
>
> > +#ifndef CONFIG_ARCH_MX5
> > +struct cpu_wp *get_cpu_wp(int *wp);
> > +#endif
>
> Do not enclose declarations in #ifdef. Anybody should be able to
> just enable your driver anyway without getting conflicts.
>
>        Arnd
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101006/0b5618c5/attachment-0001.html>

[PATCH] cpufreq for freescale mx51

[Arnd Bergmann]

On Wednesday 06 October 2010, Yong Shen wrote:
> Hi Arnd,
> 
> Really appreciate your valuable comments. Most of them are accepted. I have
> different option about two comments.
> 1.
> 
> > It would be better to make this code a proper device driver,
> > probably a platform_driver unless you have a way to probe
> > the presence of the registers on another bus.
> >
> > Making it a driver that registers to a bus lets you separate
> > the probing from the implementation, and gives you a structure
> > to add your private variables to.
> >
> cpufreq is supposed to be registered using cpufreq_register_driver directly,
> so no other platform data is needed. You can also find out other cpufreq
> driver is designed this way, like omap cpufreq driver.

Ok, it is indeed different from what I thought.

My thought was that you have resources that need to be attached
to a device which then can get matched to a device_driver.

However, this is not how it works with the generic clock framework.
The device that you are controlling is not a random platform
device but the actual CPU, which has a node in the device tree
already (/sys/devices/system/cpu/*) and that gets controlled by
the generic cpufreq layer, while the resources are tied to the
struct clk that you are controlling.

So if anything, the clk is what needs to be a platform device,
not an artificial cpufreq device. You are only adding another
clock to the clock-mx51.c file and you are consistent with the
existing clocks in there, so I'm not asking you to change anything
here.

I wonder however if we can create a common cpufreq driver that
would work for all platforms that just need to call clk_set_rate
in the end and can operate from platform specific tables otherwise.
About half the cpufreq drivers in arm seem to be a variation of
this already, so we might be able to make the clock framework
good enough for this.

> 2.
> 
> > Don't use __raw_readl but readl/ioread32, which have more well-defined
> > behaviour.
> >
> I think __raw_readl is ok here, since in the platform related code, we know
> the register layout and length, this is more efficient. Also this is
> extensively used in arch/arm/.

I still disagree, but it's not important. IMHO most of the uses of
__raw_readl should be converted to readl or readl_relaxed if you are
worried about efficiency.

The main difference between __raw_readl and readl_relaxed is that the
endianess is well-defined on readl_relaxed.

	Arnd

[PATCH] cpufreq for freescale mx51

[Yong Shen]

> I still disagree, but it's not important. IMHO most of the uses of
> __raw_readl should be converted to readl or readl_relaxed if you are
> worried about efficiency.
>
> The main difference between __raw_readl and readl_relaxed is that the
> endianess is well-defined on readl_relaxed.
>
>        Arnd
>

I agree with you about that. I will keep that in mind and also notice my
colleages this in our future development.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101007/133e8673/attachment-0001.html>

mx51 cpufreq driver

[yong.shen at linaro.org]

2nd review. Updated according to comments from Sascha and Arnd.

[PATCH] cpufreq for freescale mx51

[yong.shen at linaro.org]

From: Yong Shen <yong.shen@linaro.org>

it is tested on babbage 3.0

Signed-off-by: Yong Shen <yong.shen@linaro.org>
---
 arch/arm/Kconfig                       |    6 +
 arch/arm/mach-mx5/Kconfig              |    1 +
 arch/arm/mach-mx5/Makefile             |    2 +-
 arch/arm/mach-mx5/board-mx51_babbage.c |   10 ++-
 arch/arm/mach-mx5/clock-mx51.c         |   54 ++++++++
 arch/arm/mach-mx5/cpu.c                |    2 +
 arch/arm/mach-mx5/cpu_wp-mx51.c        |   42 ++++++
 arch/arm/mach-mx5/cpu_wp-mx51.h        |   14 ++
 arch/arm/plat-mxc/Makefile             |    2 +
 arch/arm/plat-mxc/cpufreq.c            |  232 ++++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++-
 11 files changed, 382 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-mx5/cpu_wp-mx51.c
 create mode 100644 arch/arm/mach-mx5/cpu_wp-mx51.h
 create mode 100644 arch/arm/plat-mxc/cpufreq.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d203b84..9ea6c37 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1690,6 +1690,12 @@ if ARCH_HAS_CPUFREQ
 
 source "drivers/cpufreq/Kconfig"
 
+config CPU_FREQ_IMX
+	tristate "CPUfreq driver for i.MX CPUs"
+	depends on ARCH_MXC && CPU_FREQ
+	help
+	This enables the CPUfreq driver for i.MX CPUs.
+
 config CPU_FREQ_SA1100
 	bool
 
diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index 0848db5..7a621b4 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -5,6 +5,7 @@ config ARCH_MX51
 	default y
 	select MXC_TZIC
 	select ARCH_MXC_IOMUX_V3
+	select ARCH_HAS_CPUFREQ
 
 comment "MX5 platforms:"
 
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 86c66e7..673daba 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -3,7 +3,7 @@
 #
 
 # Object file lists.
-obj-y   := cpu.o mm.o clock-mx51.o devices.o
+obj-y   := cpu.o mm.o clock-mx51.o devices.o cpu_wp-mx51.o
 
 obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
 obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
index 6e384d9..74627d2 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
  * Copyright (C) 2009-2010 Amit Kucheria <amit.kucheria@canonical.com>
  *
  * The code contained herein is licensed under the GNU General Public
@@ -32,6 +32,7 @@
 #include <asm/mach/time.h>
 
 #include "devices.h"
+#include "cpu_wp-mx51.h"
 
 #define BABBAGE_USB_HUB_RESET	(0*32 + 7)	/* GPIO_1_7 */
 #define BABBAGE_USBH1_STP	(0*32 + 27)	/* GPIO_1_27 */
@@ -279,8 +280,15 @@ static struct sys_timer mxc_timer = {
 	.init	= mx51_babbage_timer_init,
 };
 
+static void __init fixup_mxc_board(struct machine_desc *desc, struct tag *tags,
+				   char **cmdline, struct meminfo *mi)
+{
+	get_cpu_wp = mx51_get_cpu_wp;
+}
+
 MACHINE_START(MX51_BABBAGE, "Freescale MX51 Babbage Board")
 	/* Maintainer: Amit Kucheria <amit.kucheria@canonical.com> */
+	.fixup = fixup_mxc_board,
 	.phys_io = MX51_AIPS1_BASE_ADDR,
 	.io_pg_offst = ((MX51_AIPS1_BASE_ADDR_VIRT) >> 18) & 0xfffc,
 	.boot_params = PHYS_OFFSET + 0x100,
diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
index 6af69de..0709a64 100644
--- a/arch/arm/mach-mx5/clock-mx51.c
+++ b/arch/arm/mach-mx5/clock-mx51.c
@@ -28,6 +28,10 @@
 static unsigned long external_high_reference, external_low_reference;
 static unsigned long oscillator_reference, ckih2_reference;
 
+static int cpu_wp_nr;
+static int cpu_curr_wp;
+static struct cpu_wp *cpu_wp_tbl;
+
 static struct clk osc_clk;
 static struct clk pll1_main_clk;
 static struct clk pll1_sw_clk;
@@ -39,6 +43,8 @@ static struct clk ahb_clk;
 static struct clk ipg_clk;
 static struct clk usboh3_clk;
 
+DEFINE_SPINLOCK(clk_lock);
+
 #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
 
 static int _clk_ccgr_enable(struct clk *clk)
@@ -330,6 +336,37 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
 	return 0;
 }
 
+/*!
+ * Setup cpu clock based on working point.
+ * @param	wp	cpu freq working point
+ * @return		0 on success or error code on failure.
+ */
+static int cpu_clk_set_wp(int wp)
+{
+	struct cpu_wp *p;
+	u32 reg;
+	unsigned long flags;
+
+	if (wp == cpu_curr_wp)
+		return 0;
+
+	p = &cpu_wp_tbl[wp];
+
+	/*use post divider to change freq
+	 */
+	spin_lock_irqsave(&clk_lock, flags);
+
+	reg = __raw_readl(MXC_CCM_CACRR);
+	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
+	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
+	__raw_writel(reg, MXC_CCM_CACRR);
+
+	spin_unlock_irqrestore(&clk_lock, flags);
+	cpu_curr_wp = wp;
+
+	return 0;
+}
+
 static unsigned long clk_arm_get_rate(struct clk *clk)
 {
 	u32 cacrr, div;
@@ -342,6 +379,20 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
 	return parent_rate / div;
 }
 
+static int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
+{
+	u32 i;
+	for (i = 0; i < cpu_wp_nr; i++) {
+		if (rate == cpu_wp_tbl[i].cpu_rate)
+			break;
+	}
+	if (i >= cpu_wp_nr)
+		return -EINVAL;
+	cpu_clk_set_wp(i);
+
+	return 0;
+}
+
 static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
 {
 	u32 reg, mux;
@@ -694,6 +745,7 @@ static struct clk periph_apm_clk = {
 static struct clk cpu_clk = {
 	.parent = &pll1_sw_clk,
 	.get_rate = clk_arm_get_rate,
+	.set_rate = _clk_cpu_set_rate,
 };
 
 static struct clk ahb_clk = {
@@ -837,6 +889,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
 	_REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
 	_REGISTER_CLOCK("imx-keypad.0", NULL, kpp_clk)
+	_REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
 };
 
 static void clk_tree_init(void)
@@ -868,6 +921,7 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
 	external_high_reference = ckih1;
 	ckih2_reference = ckih2;
 	oscillator_reference = osc;
+	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
 
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)
 		clkdev_add(&lookups[i]);
diff --git a/arch/arm/mach-mx5/cpu.c b/arch/arm/mach-mx5/cpu.c
index 2d37785..83add9c 100644
--- a/arch/arm/mach-mx5/cpu.c
+++ b/arch/arm/mach-mx5/cpu.c
@@ -22,6 +22,8 @@ static int cpu_silicon_rev = -1;
 
 #define SI_REV 0x48
 
+struct cpu_wp *(*get_cpu_wp)(int *wp);
+
 static void query_silicon_parameter(void)
 {
 	void __iomem *rom = ioremap(MX51_IROM_BASE_ADDR, MX51_IROM_SIZE);
diff --git a/arch/arm/mach-mx5/cpu_wp-mx51.c b/arch/arm/mach-mx5/cpu_wp-mx51.c
new file mode 100644
index 0000000..51bde45
--- /dev/null
+++ b/arch/arm/mach-mx5/cpu_wp-mx51.c
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later@the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/types.h>
+#include <mach/hardware.h>
+
+static struct cpu_wp cpu_wp_auto[] = {
+	{
+	.pll_rate = 800000000,
+	.cpu_rate = 160000000,
+	.pdf = 4,
+	.mfi = 8,
+	.mfd = 2,
+	.mfn = 1,
+	.cpu_podf = 4,
+	.cpu_voltage = 850000,},
+	{
+	.pll_rate = 800000000,
+	.cpu_rate = 800000000,
+	.pdf = 0,
+	.mfi = 8,
+	.mfd = 2,
+	.mfn = 1,
+	.cpu_podf = 0,
+	.cpu_voltage = 1100000,},
+};
+
+struct cpu_wp *mx51_get_cpu_wp(int *wp)
+{
+	*wp = sizeof(cpu_wp_auto) / sizeof(struct cpu_wp);
+	return cpu_wp_auto;
+}
diff --git a/arch/arm/mach-mx5/cpu_wp-mx51.h b/arch/arm/mach-mx5/cpu_wp-mx51.h
new file mode 100644
index 0000000..8038b62
--- /dev/null
+++ b/arch/arm/mach-mx5/cpu_wp-mx51.h
@@ -0,0 +1,14 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+extern struct cpu_wp *mx51_get_cpu_wp(int *wp);
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index 78d405e..0b8464f 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
 obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
 obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
 obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
+# CPU FREQ support
+obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
 ifdef CONFIG_SND_IMX_SOC
 obj-y += ssi-fiq.o
 obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
new file mode 100644
index 0000000..dfb1dde
--- /dev/null
+++ b/arch/arm/plat-mxc/cpufreq.c
@@ -0,0 +1,232 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/*
+ * A driver for the Freescale Semiconductor i.MXC CPUfreq module.
+ * The CPUFREQ driver is for controling CPU frequency. It allows you to change
+ * the CPU clock speed on the fly.
+ */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/proc_fs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <mach/hardware.h>
+#include <asm/setup.h>
+#include <mach/clock.h>
+#include <asm/cacheflush.h>
+#include <linux/hrtimer.h>
+
+static int cpu_freq_khz_min;
+static int cpu_freq_khz_max;
+static int arm_lpm_clk;
+static int arm_normal_clk;
+static int cpufreq_suspended;
+
+static struct clk *cpu_clk;
+static struct cpufreq_frequency_table imx_freq_table[4];
+
+static int cpu_wp_nr;
+static struct cpu_wp *cpu_wp_tbl;
+
+static int set_cpu_freq(int freq)
+{
+	int ret = 0;
+	int org_cpu_rate;
+	int gp_volt = 0;
+	int i;
+
+	org_cpu_rate = clk_get_rate(cpu_clk);
+	if (org_cpu_rate == freq)
+		return ret;
+
+	for (i = 0; i < cpu_wp_nr; i++) {
+		if (freq == cpu_wp_tbl[i].cpu_rate)
+			gp_volt = cpu_wp_tbl[i].cpu_voltage;
+	}
+
+	if (gp_volt == 0)
+		return ret;
+
+	ret = clk_set_rate(cpu_clk, freq);
+	if (ret != 0) {
+		printk(KERN_DEBUG "cannot set CPU clock rate\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int mxc_verify_speed(struct cpufreq_policy *policy)
+{
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	return cpufreq_frequency_table_verify(policy, imx_freq_table);
+}
+
+static unsigned int mxc_get_speed(unsigned int cpu)
+{
+	if (cpu)
+		return 0;
+
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int mxc_set_target(struct cpufreq_policy *policy,
+			  unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	int freq_Hz;
+	int ret = 0;
+	unsigned int index;
+
+	if (cpufreq_suspended) {
+		target_freq = clk_get_rate(cpu_clk) / 1000;
+		cpufreq_frequency_table_target(policy, imx_freq_table,
+				target_freq, relation, &index);
+		freq_Hz = imx_freq_table[index].frequency * 1000;
+
+		if (freq_Hz == arm_lpm_clk)
+			freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate / 1000;
+		else
+			freqs.old = arm_lpm_clk / 1000;
+
+		freqs.new = freq_Hz / 1000;
+		freqs.cpu = 0;
+		freqs.flags = 0;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		return ret;
+	}
+
+	cpufreq_frequency_table_target(policy, imx_freq_table,
+			target_freq, relation, &index);
+	freq_Hz = imx_freq_table[index].frequency * 1000;
+
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.cpu = 0;
+	freqs.flags = 0;
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+	if (freqs.old != freqs.new)
+		ret = set_cpu_freq(freq_Hz);
+
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+
+	return ret;
+}
+
+static int __init mxc_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+	int i;
+
+	printk(KERN_INFO "i.MXC CPU frequency driver\n");
+
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	cpu_clk = clk_get(NULL, "cpu_clk");
+	if (IS_ERR(cpu_clk)) {
+		printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
+		return PTR_ERR(cpu_clk);
+	}
+
+	/* Set the current working point. */
+	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
+
+	cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
+	cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
+
+	for (i = 0; i < cpu_wp_nr; i++) {
+		imx_freq_table[i].index = i;
+		imx_freq_table[i].frequency =
+		    cpu_wp_tbl[i].cpu_rate / 1000;
+
+		if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
+			cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
+
+		if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
+			cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
+	}
+
+	imx_freq_table[i].index = i;
+	imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
+	policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
+
+	arm_lpm_clk = cpu_freq_khz_min * 1000;
+	arm_normal_clk = cpu_freq_khz_max * 1000;
+
+	/* Manual states, that PLL stabilizes in two CLK32 periods */
+	policy->cpuinfo.transition_latency = 10;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
+
+	if (ret < 0) {
+		clk_put(cpu_clk);
+		printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
+		       __func__);
+		return ret;
+	}
+
+	cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
+	return 0;
+}
+
+static int mxc_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+
+	/* Reset CPU to 665MHz */
+	set_cpu_freq(arm_normal_clk);
+	clk_put(cpu_clk);
+	return 0;
+}
+
+static struct cpufreq_driver mxc_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = mxc_verify_speed,
+	.target = mxc_set_target,
+	.get = mxc_get_speed,
+	.init = mxc_cpufreq_init,
+	.exit = mxc_cpufreq_exit,
+	.name = "imx",
+};
+
+static int __devinit mxc_cpufreq_driver_init(void)
+{
+	return cpufreq_register_driver(&mxc_driver);
+}
+
+static void mxc_cpufreq_driver_exit(void)
+{
+	cpufreq_unregister_driver(&mxc_driver);
+}
+
+module_init(mxc_cpufreq_driver_init);
+module_exit(mxc_cpufreq_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc., Yong Shen <yong.shen@linaro.org>");
+MODULE_DESCRIPTION("CPUfreq driver for i.MX");
+MODULE_LICENSE("GPL");
diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
index a790bf2..31df991 100644
--- a/arch/arm/plat-mxc/include/mach/mxc.h
+++ b/arch/arm/plat-mxc/include/mach/mxc.h
@@ -1,5 +1,5 @@
 /*
- * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2004-2010 Freescale Semiconductor, Inc. All Rights Reserved.
  * Copyright (C) 2008 Juergen Beisert (kernel at pengutronix.de)
  *
  * This program is free software; you can redistribute it and/or
@@ -133,6 +133,24 @@ extern unsigned int __mxc_cpu_type;
 # define cpu_is_mxc91231()	(0)
 #endif
 
+#ifndef __ASSEMBLY__
+
+struct cpu_wp {
+	u32 pll_reg;
+	u32 pll_rate;
+	u32 cpu_rate;
+	u32 pdr0_reg;
+	u32 pdf;
+	u32 mfi;
+	u32 mfd;
+	u32 mfn;
+	u32 cpu_voltage;
+	u32 cpu_podf;
+};
+
+extern struct cpu_wp *(*get_cpu_wp)(int *wp);
+#endif
+
 #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
 /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
 #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
-- 
1.6.3.3

[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

Some comments inline.

On 10 Oct 07, Yong Shen wrote:
> From: Yong Shen <yong.shen@linaro.org>
> 
> it is tested on babbage 3.0
> 
> Signed-off-by: Yong Shen <yong.shen@linaro.org>
> ---
>  arch/arm/Kconfig                       |    6 +
>  arch/arm/mach-mx5/Kconfig              |    1 +
>  arch/arm/mach-mx5/Makefile             |    2 +-
>  arch/arm/mach-mx5/board-mx51_babbage.c |   10 ++-
>  arch/arm/mach-mx5/clock-mx51.c         |   54 ++++++++
>  arch/arm/mach-mx5/cpu.c                |    2 +
>  arch/arm/mach-mx5/cpu_wp-mx51.c        |   42 ++++++
>  arch/arm/mach-mx5/cpu_wp-mx51.h        |   14 ++
>  arch/arm/plat-mxc/Makefile             |    2 +
>  arch/arm/plat-mxc/cpufreq.c            |  232 ++++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/mxc.h   |   20 +++-
>  11 files changed, 382 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/mach-mx5/cpu_wp-mx51.c
>  create mode 100644 arch/arm/mach-mx5/cpu_wp-mx51.h
>  create mode 100644 arch/arm/plat-mxc/cpufreq.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d203b84..9ea6c37 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1690,6 +1690,12 @@ if ARCH_HAS_CPUFREQ
>  
>  source "drivers/cpufreq/Kconfig"
>  
> +config CPU_FREQ_IMX
> +	tristate "CPUfreq driver for i.MX CPUs"
> +	depends on ARCH_MXC && CPU_FREQ
> +	help
> +	This enables the CPUfreq driver for i.MX CPUs.
> +
>  config CPU_FREQ_SA1100
>  	bool
>  
> diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> index 0848db5..7a621b4 100644
> --- a/arch/arm/mach-mx5/Kconfig
> +++ b/arch/arm/mach-mx5/Kconfig
> @@ -5,6 +5,7 @@ config ARCH_MX51
>  	default y
>  	select MXC_TZIC
>  	select ARCH_MXC_IOMUX_V3
> +	select ARCH_HAS_CPUFREQ
>  
>  comment "MX5 platforms:"
>  
> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
> index 86c66e7..673daba 100644
> --- a/arch/arm/mach-mx5/Makefile
> +++ b/arch/arm/mach-mx5/Makefile
> @@ -3,7 +3,7 @@
>  #
>  
>  # Object file lists.
> -obj-y   := cpu.o mm.o clock-mx51.o devices.o
> +obj-y   := cpu.o mm.o clock-mx51.o devices.o cpu_wp-mx51.o

By hardcoding cpu_wp-mx51 here, you are making the assumption that even if
cpufreq is turned off, you'll still need the entire WP table to set the
cpu frequency. Why can't this be handled transparently in the clock code? If
the clock code does the calculations, every board doesn't need to first setup
calls to get_cpu_wp.

>  obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
>  obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
> diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c
> index 6e384d9..74627d2 100644
> --- a/arch/arm/mach-mx5/board-mx51_babbage.c
> +++ b/arch/arm/mach-mx5/board-mx51_babbage.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
>   * Copyright (C) 2009-2010 Amit Kucheria <amit.kucheria@canonical.com>
>   *
>   * The code contained herein is licensed under the GNU General Public
> @@ -32,6 +32,7 @@
>  #include <asm/mach/time.h>
>  
>  #include "devices.h"
> +#include "cpu_wp-mx51.h"
>  
>  #define BABBAGE_USB_HUB_RESET	(0*32 + 7)	/* GPIO_1_7 */
>  #define BABBAGE_USBH1_STP	(0*32 + 27)	/* GPIO_1_27 */
> @@ -279,8 +280,15 @@ static struct sys_timer mxc_timer = {
>  	.init	= mx51_babbage_timer_init,
>  };
>  
> +static void __init fixup_mxc_board(struct machine_desc *desc, struct tag *tags,
> +				   char **cmdline, struct meminfo *mi)
> +{
> +	get_cpu_wp = mx51_get_cpu_wp;
> +}
> +
>  MACHINE_START(MX51_BABBAGE, "Freescale MX51 Babbage Board")
>  	/* Maintainer: Amit Kucheria <amit.kucheria@canonical.com> */
> +	.fixup = fixup_mxc_board,
>  	.phys_io = MX51_AIPS1_BASE_ADDR,
>  	.io_pg_offst = ((MX51_AIPS1_BASE_ADDR_VIRT) >> 18) & 0xfffc,
>  	.boot_params = PHYS_OFFSET + 0x100,
> diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
> index 6af69de..0709a64 100644
> --- a/arch/arm/mach-mx5/clock-mx51.c
> +++ b/arch/arm/mach-mx5/clock-mx51.c
> @@ -28,6 +28,10 @@
>  static unsigned long external_high_reference, external_low_reference;
>  static unsigned long oscillator_reference, ckih2_reference;
>  
> +static int cpu_wp_nr;
> +static int cpu_curr_wp;
> +static struct cpu_wp *cpu_wp_tbl;
> +
>  static struct clk osc_clk;
>  static struct clk pll1_main_clk;
>  static struct clk pll1_sw_clk;
> @@ -39,6 +43,8 @@ static struct clk ahb_clk;
>  static struct clk ipg_clk;
>  static struct clk usboh3_clk;
>  
> +DEFINE_SPINLOCK(clk_lock);
> +
>  #define MAX_DPLL_WAIT_TRIES	1000 /* 1000 * udelay(1) = 1ms */
>  
>  static int _clk_ccgr_enable(struct clk *clk)
> @@ -330,6 +336,37 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)
>  	return 0;
>  }
>  
> +/*!
> + * Setup cpu clock based on working point.
> + * @param	wp	cpu freq working point
> + * @return		0 on success or error code on failure.
> + */
> +static int cpu_clk_set_wp(int wp)
> +{
> +	struct cpu_wp *p;
> +	u32 reg;
> +	unsigned long flags;
> +
> +	if (wp == cpu_curr_wp)
> +		return 0;
> +
> +	p = &cpu_wp_tbl[wp];
> +
> +	/*use post divider to change freq
> +	 */

Fix comment to one-line
>
> +	spin_lock_irqsave(&clk_lock, flags);
> +
> +	reg = __raw_readl(MXC_CCM_CACRR);
> +	reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;
> +	reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;
> +	__raw_writel(reg, MXC_CCM_CACRR);
> +
> +	spin_unlock_irqrestore(&clk_lock, flags);
> +	cpu_curr_wp = wp;
> +
> +	return 0;
> +}
> +
>  static unsigned long clk_arm_get_rate(struct clk *clk)
>  {
>  	u32 cacrr, div;
> @@ -342,6 +379,20 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
>  	return parent_rate / div;
>  }
>  
> +static int _clk_cpu_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	u32 i;
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		if (rate == cpu_wp_tbl[i].cpu_rate)
> +			break;
> +	}
> +	if (i >= cpu_wp_nr)
> +		return -EINVAL;
> +	cpu_clk_set_wp(i);
> +
> +	return 0;
> +}
> +

Why use cpu_clk_set_wp when CPUFREQ is not compiled in? You could do the
calculations here itself and remove the dependency on the WP table.

>  static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
>  {
>  	u32 reg, mux;
> @@ -694,6 +745,7 @@ static struct clk periph_apm_clk = {
>  static struct clk cpu_clk = {
>  	.parent = &pll1_sw_clk,
>  	.get_rate = clk_arm_get_rate,
> +	.set_rate = _clk_cpu_set_rate,
>  };
>  
>  static struct clk ahb_clk = {
> @@ -837,6 +889,7 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("fsl-usb2-udc", "usb", usboh3_clk)
>  	_REGISTER_CLOCK("fsl-usb2-udc", "usb_ahb", ahb_clk)
>  	_REGISTER_CLOCK("imx-keypad.0", NULL, kpp_clk)
> +	_REGISTER_CLOCK(NULL, "cpu_clk", cpu_clk)
>  };
>  
>  static void clk_tree_init(void)
> @@ -868,6 +921,7 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
>  	external_high_reference = ckih1;
>  	ckih2_reference = ckih2;
>  	oscillator_reference = osc;
> +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
>

>  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
>  		clkdev_add(&lookups[i]);
> diff --git a/arch/arm/mach-mx5/cpu.c b/arch/arm/mach-mx5/cpu.c
> index 2d37785..83add9c 100644
> --- a/arch/arm/mach-mx5/cpu.c
> +++ b/arch/arm/mach-mx5/cpu.c
> @@ -22,6 +22,8 @@ static int cpu_silicon_rev = -1;
>  
>  #define SI_REV 0x48
>  
> +struct cpu_wp *(*get_cpu_wp)(int *wp);
> +
>  static void query_silicon_parameter(void)
>  {
>  	void __iomem *rom = ioremap(MX51_IROM_BASE_ADDR, MX51_IROM_SIZE);
> diff --git a/arch/arm/mach-mx5/cpu_wp-mx51.c b/arch/arm/mach-mx5/cpu_wp-mx51.c
> new file mode 100644
> index 0000000..51bde45
> --- /dev/null
> +++ b/arch/arm/mach-mx5/cpu_wp-mx51.c
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/types.h>
> +#include <mach/hardware.h>
> +
> +static struct cpu_wp cpu_wp_auto[] = {
> +	{
> +	.pll_rate = 800000000,
> +	.cpu_rate = 160000000,
> +	.pdf = 4,
> +	.mfi = 8,
> +	.mfd = 2,
> +	.mfn = 1,
> +	.cpu_podf = 4,
> +	.cpu_voltage = 850000,},
> +	{
> +	.pll_rate = 800000000,
> +	.cpu_rate = 800000000,
> +	.pdf = 0,
> +	.mfi = 8,
> +	.mfd = 2,
> +	.mfn = 1,
> +	.cpu_podf = 0,
> +	.cpu_voltage = 1100000,},
> +};
> +
> +struct cpu_wp *mx51_get_cpu_wp(int *wp)
> +{
> +	*wp = sizeof(cpu_wp_auto) / sizeof(struct cpu_wp);
> +	return cpu_wp_auto;
> +}
> diff --git a/arch/arm/mach-mx5/cpu_wp-mx51.h b/arch/arm/mach-mx5/cpu_wp-mx51.h
> new file mode 100644
> index 0000000..8038b62
> --- /dev/null
> +++ b/arch/arm/mach-mx5/cpu_wp-mx51.h
> @@ -0,0 +1,14 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +extern struct cpu_wp *mx51_get_cpu_wp(int *wp);
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index 78d405e..0b8464f 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
>  obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
>  obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
>  obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
> +# CPU FREQ support
> +obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
>  ifdef CONFIG_SND_IMX_SOC
>  obj-y += ssi-fiq.o
>  obj-y += ssi-fiq-ksym.o
> diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
> new file mode 100644
> index 0000000..dfb1dde
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpufreq.c
> @@ -0,0 +1,232 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * A driver for the Freescale Semiconductor i.MXC CPUfreq module.
> + * The CPUFREQ driver is for controling CPU frequency. It allows you to change
> + * the CPU clock speed on the fly.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <mach/hardware.h>
> +#include <asm/setup.h>
> +#include <mach/clock.h>
> +#include <asm/cacheflush.h>
> +#include <linux/hrtimer.h>
> +
> +static int cpu_freq_khz_min;
> +static int cpu_freq_khz_max;
> +static int arm_lpm_clk;
> +static int arm_normal_clk;
> +static int cpufreq_suspended;
> +
> +static struct clk *cpu_clk;
> +static struct cpufreq_frequency_table imx_freq_table[4];
> +
> +static int cpu_wp_nr;
> +static struct cpu_wp *cpu_wp_tbl;
> +
> +static int set_cpu_freq(int freq)
> +{
> +	int ret = 0;
> +	int org_cpu_rate;
> +	int gp_volt = 0;
> +	int i;
> +
> +	org_cpu_rate = clk_get_rate(cpu_clk);
> +	if (org_cpu_rate == freq)
> +		return ret;
> +
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		if (freq == cpu_wp_tbl[i].cpu_rate)
> +			gp_volt = cpu_wp_tbl[i].cpu_voltage;
> +	}
> +
> +	if (gp_volt == 0)
> +		return ret;
> +
> +	ret = clk_set_rate(cpu_clk, freq);
> +	if (ret != 0) {
> +		printk(KERN_DEBUG "cannot set CPU clock rate\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int mxc_verify_speed(struct cpufreq_policy *policy)
> +{
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	return cpufreq_frequency_table_verify(policy, imx_freq_table);
> +}
> +
> +static unsigned int mxc_get_speed(unsigned int cpu)
> +{
> +	if (cpu)
> +		return 0;
> +
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int mxc_set_target(struct cpufreq_policy *policy,
> +			  unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	int freq_Hz;
> +	int ret = 0;
> +	unsigned int index;
> +
> +	if (cpufreq_suspended) {
> +		target_freq = clk_get_rate(cpu_clk) / 1000;
> +		cpufreq_frequency_table_target(policy, imx_freq_table,
> +				target_freq, relation, &index);
> +		freq_Hz = imx_freq_table[index].frequency * 1000;
> +
> +		if (freq_Hz == arm_lpm_clk)
> +			freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate / 1000;
> +		else
> +			freqs.old = arm_lpm_clk / 1000;
> +
> +		freqs.new = freq_Hz / 1000;
> +		freqs.cpu = 0;
> +		freqs.flags = 0;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +		return ret;
> +	}
> +
> +	cpufreq_frequency_table_target(policy, imx_freq_table,
> +			target_freq, relation, &index);
> +	freq_Hz = imx_freq_table[index].frequency * 1000;
> +
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.cpu = 0;
> +	freqs.flags = 0;
> +	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +
> +	if (freqs.old != freqs.new)
> +		ret = set_cpu_freq(freq_Hz);
> +
> +	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +
> +	return ret;
> +}
> +
> +static int __init mxc_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	int ret;
> +	int i;
> +
> +	printk(KERN_INFO "i.MXC CPU frequency driver\n");
> +
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	cpu_clk = clk_get(NULL, "cpu_clk");
> +	if (IS_ERR(cpu_clk)) {
> +		printk(KERN_ERR "%s: failed to get cpu clock\n", __func__);
> +		return PTR_ERR(cpu_clk);
> +	}
> +
> +	/* Set the current working point. */
> +	cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
> +
> +	cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
> +	cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
> +
> +	for (i = 0; i < cpu_wp_nr; i++) {
> +		imx_freq_table[i].index = i;
> +		imx_freq_table[i].frequency =
> +		    cpu_wp_tbl[i].cpu_rate / 1000;
> +
> +		if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
> +			cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
> +
> +		if ((cpu_wp_tbl[i].cpu_rate / 1000) > cpu_freq_khz_max)
> +			cpu_freq_khz_max = cpu_wp_tbl[i].cpu_rate / 1000;
> +	}
> +
> +	imx_freq_table[i].index = i;
> +	imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
> +
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
> +	policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min;
> +	policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
> +
> +	arm_lpm_clk = cpu_freq_khz_min * 1000;
> +	arm_normal_clk = cpu_freq_khz_max * 1000;
> +
> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = 10;
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table);
> +
> +	if (ret < 0) {
> +		clk_put(cpu_clk);
> +		printk(KERN_ERR "%s: failed to register i.MXC CPUfreq\n",
> +		       __func__);
> +		return ret;
> +	}
> +
> +	cpufreq_frequency_table_get_attr(imx_freq_table, policy->cpu);
> +	return 0;
> +}
> +
> +static int mxc_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +
> +	/* Reset CPU to 665MHz */
> +	set_cpu_freq(arm_normal_clk);
> +	clk_put(cpu_clk);
> +	return 0;
> +}
> +
> +static struct cpufreq_driver mxc_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = mxc_verify_speed,
> +	.target = mxc_set_target,
> +	.get = mxc_get_speed,
> +	.init = mxc_cpufreq_init,
> +	.exit = mxc_cpufreq_exit,
> +	.name = "imx",
> +};
> +
> +static int __devinit mxc_cpufreq_driver_init(void)
> +{
> +	return cpufreq_register_driver(&mxc_driver);
> +}
> +
> +static void mxc_cpufreq_driver_exit(void)
> +{
> +	cpufreq_unregister_driver(&mxc_driver);
> +}
> +
> +module_init(mxc_cpufreq_driver_init);
> +module_exit(mxc_cpufreq_driver_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc., Yong Shen <yong.shen@linaro.org>");
> +MODULE_DESCRIPTION("CPUfreq driver for i.MX");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
> index a790bf2..31df991 100644
> --- a/arch/arm/plat-mxc/include/mach/mxc.h
> +++ b/arch/arm/plat-mxc/include/mach/mxc.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright 2004-2010 Freescale Semiconductor, Inc. All Rights Reserved.
>   * Copyright (C) 2008 Juergen Beisert (kernel at pengutronix.de)
>   *
>   * This program is free software; you can redistribute it and/or
> @@ -133,6 +133,24 @@ extern unsigned int __mxc_cpu_type;
>  # define cpu_is_mxc91231()	(0)
>  #endif
>  
> +#ifndef __ASSEMBLY__
> +
> +struct cpu_wp {
> +	u32 pll_reg;
> +	u32 pll_rate;
> +	u32 cpu_rate;
> +	u32 pdr0_reg;
> +	u32 pdf;
> +	u32 mfi;
> +	u32 mfd;
> +	u32 mfn;
> +	u32 cpu_voltage;
> +	u32 cpu_podf;
> +};
> +
> +extern struct cpu_wp *(*get_cpu_wp)(int *wp);
> +#endif
> +
>  #if defined(CONFIG_ARCH_MX3) || defined(CONFIG_ARCH_MX2)
>  /* These are deprecated, use mx[23][157]_setup_weimcs instead. */
>  #define CSCR_U(n) (IO_ADDRESS(WEIM_BASE_ADDR + n * 0x10))
> -- 
> 1.6.3.3
> 
> 
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

[PATCH] cpufreq for freescale mx51

[Yong Shen]

> >  # Object file lists.
> > -obj-y   := cpu.o mm.o clock-mx51.o devices.o
> > +obj-y   := cpu.o mm.o clock-mx51.o devices.o cpu_wp-mx51.o
>
> By hardcoding cpu_wp-mx51 here, you are making the assumption that even if
> cpufreq is turned off, you'll still need the entire WP table to set the
> cpu frequency. Why can't this be handled transparently in the clock code?
> If
> the clock code does the calculations, every board doesn't need to first
> setup
> calls to get_cpu_wp.
>
>
It has history. Not only cpufreq needs this  table, other freescale drivers
also need it, like dvfs and busfreq drivers. So it is better to be there. I
do agree clock should do its calculation by itself.
Agree about the rest.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101007/5e9777fb/attachment-0001.html>

[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

On Thu, Oct 7, 2010 at 2:33 PM, Yong Shen <yong.shen@linaro.org> wrote:
>
>> > ?# Object file lists.
>> > -obj-y ? := cpu.o mm.o clock-mx51.o devices.o
>> > +obj-y ? := cpu.o mm.o clock-mx51.o devices.o cpu_wp-mx51.o
>>
>> By hardcoding cpu_wp-mx51 here, you are making the assumption that even if
>> cpufreq is turned off, you'll still need the entire WP table to set the
>> cpu frequency. Why can't this be handled transparently in the clock code?
>> If
>> the clock code does the calculations, every board doesn't need to first
>> setup
>> calls to get_cpu_wp.
>>
>
> It has history. Not only cpufreq needs this? table, other freescale drivers
> also need it, like dvfs and busfreq drivers. So it is better to be there. I
> do agree clock should do its calculation by itself.
> Agree about the rest.

Let's modify that history and do it the correct way then :)

When we come to the dvfs and bus drivers we'll fix them too.

Regards,
Amit