Hi Arnd,<br><br>Really appreciate your valuable comments. Most of them are accepted. I have different option about two comments.<br>1. <br><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">
It would be better to make this code a proper device driver,<br>
probably a platform_driver unless you have a way to probe<br>
the presence of the registers on another bus.<br><br>
Making it a driver that registers to a bus lets you separate<br>
the probing from the implementation, and gives you a structure<br>
to add your private variables to.<br></blockquote>
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.<br>2. <br><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">
Don't use __raw_readl but readl/ioread32, which have more well-defined<br>
behaviour.<br></blockquote>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/.<br><br>Again, thanks for your time for review. I will send out updated patch.<br>
<br>Yong<br><br><div class="gmail_quote">On Tue, Oct 5, 2010 at 8:25 PM, Arnd Bergmann <span dir="ltr"><<a href="mailto:arnd@arndb.de" target="_blank">arnd@arndb.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div>> From: Yong Shen <<a href="mailto:yong.shen@linaro.org" target="_blank">yong.shen@linaro.org</a>><br>
><br>
> it is tested on babbage 3.0<br>
<br>
</div>One big comment and a couple of smaller ones:<br>
<br>
It would be better to make this code a proper device driver,<br>
probably a platform_driver unless you have a way to probe<br>
the presence of the registers on another bus.<br>
<br>
Making it a driver that registers to a bus lets you separate<br>
the probing from the implementation, and gives you a structure<br>
to add your private variables to.<br>
<div><br>
> @@ -43,6 +43,10 @@<br>
> #define MX51_USB_PLL_DIV_19_2_MHZ 0x01<br>
> #define MX51_USB_PLL_DIV_24_MHZ 0x02<br>
><br>
> +<br>
> +extern struct cpu_wp *mx51_get_cpu_wp(int *wp);<br>
> +struct cpu_wp *(*get_cpu_wp)(int *wp);<br>
> +<br>
> static struct platform_device *devices[] __initdata = {<br>
> &mxc_fec_device,<br>
> };<br>
<br>
</div>Please put the extern declarations into a header file, not<br>
in the implementation.<br>
<div><br>
> @@ -246,6 +250,7 @@ static void __init mxc_board_init(void)<br>
><br>
> static void __init mx51_babbage_timer_init(void)<br>
> {<br>
> + get_cpu_wp = mx51_get_cpu_wp;<br>
> mx51_clocks_init(32768, 24000000, 22579200, 0);<br>
> }<br>
<br>
</div>It feels like mx51_babbage_timer_init() is not the right place<br>
to initialize this. Why not the mxc_board_init function?<br>
<div><br>
> +#define SPIN_DELAY 1000000 /* in nanoseconds */<br>
> #define MAX_DPLL_WAIT_TRIES 1000 /* 1000 * udelay(1) = 1ms */<br>
><br>
> static int _clk_ccgr_enable(struct clk *clk)<br>
<br>
</div>The SPIN_DELAY variable doesn't seem to be used anywhere.<br>
<div><br>
> @@ -330,6 +338,32 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct clk *parent)<br>
> return 0;<br>
> }<br>
><br>
> +/*!<br>
> + * Setup cpu clock based on working point.<br>
> + * @param wp cpu freq working point<br>
> + * @return 0 on success or error code on failure.<br>
> + */<br>
> +int cpu_clk_set_wp(int wp)<br>
> +{<br>
<br>
</div>could be 'static'.<br>
<div><br>
> + struct cpu_wp *p;<br>
> + u32 reg;<br>
> +<br>
> + if (wp == cpu_curr_wp)<br>
> + return 0;<br>
> +<br>
> + p = &cpu_wp_tbl[wp];<br>
> +<br>
> + /*use post divider to change freq<br>
> + */<br>
> + reg = __raw_readl(MXC_CCM_CACRR);<br>
> + reg &= ~MXC_CCM_CACRR_ARM_PODF_MASK;<br>
> + reg |= cpu_wp_tbl[wp].cpu_podf << MXC_CCM_CACRR_ARM_PODF_OFFSET;<br>
> + __raw_writel(reg, MXC_CCM_CACRR);<br>
> + cpu_curr_wp = wp;<br>
> +<br>
> + return 0;<br>
> +}<br>
<br>
</div>This might need a spinlock to protect concurrent register access.<br>
<br>
Don't use __raw_readl but readl/ioread32, which have more well-defined<br>
behaviour.<br>
<div><br>
> +int cpu_freq_khz_min;<br>
> +int cpu_freq_khz_max;<br>
> +int arm_lpm_clk;<br>
> +int arm_normal_clk;<br>
> +int cpufreq_suspended;<br>
> +int cpufreq_trig_needed;<br>
<br>
</div>You should not have private variables in the global name space like this.<br>
At least mark everything static that is only used in one file. Any<br>
global variable must (local variables should) be prefixed with the<br>
name of the driver like:<br>
<br>
static int mxc_cpufreq_khz_min;<br>
static int mxc_cpufreq_khz_max;<br>
static int mxc_lpm_clk;<br>
<br>
This will become more important when we move to multiplatform kernels.<br>
<div><br>
> +extern int set_low_bus_freq(void);<br>
> +extern int set_high_bus_freq(int high_bus_speed);<br>
> +extern int low_freq_bus_used(void);<br>
> +<br>
> +extern struct cpu_wp *(*get_cpu_wp)(int *wp);<br>
> +static int cpu_wp_nr;<br>
> +static struct cpu_wp *cpu_wp_tbl;<br>
<br>
</div>The same rules apply for functions.<br>
<br>
Again, anything that can not be static should be declared in a<br>
header file.<br>
<div><br>
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");<br>
<br>
</div>Please put your name and email address in here.<br>
<div><br>
> +#ifndef CONFIG_ARCH_MX5<br>
> +struct cpu_wp *get_cpu_wp(int *wp);<br>
> +#endif<br>
<br>
</div>Do not enclose declarations in #ifdef. Anybody should be able to<br>
just enable your driver anyway without getting conflicts.<br>
<font color="#888888"><br>
Arnd<br>
</font></blockquote></div><br>