On Mon, Feb 04, 2013 at 01:09:11PM +0800, Shawn Guo wrote:
> On Mon, Feb 04, 2013 at 11:06:22AM -0500, Anson Huang wrote:
> > > + /*
> > > + * The setpoints are selected per PLL/PDF frequencies, so we need to
> > > + * reprogram PLL for frequency scaling. The procedure of reprogramming
> > > + * PLL1 is as below.
> > > + *
> > > + * - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> > > + * - Disable pll1_sys_clk and reprogram it
> > > + * - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
> > > + * - Disable pll2_pfd2_396m_clk
> > > + */
> > > + clk_prepare_enable(pll2_pfd2_396m_clk);
> > > + clk_set_parent(step_clk, pll2_pfd2_396m_clk);
> > > + clk_set_parent(pll1_sw_clk, step_clk);
> > > + clk_prepare_enable(pll1_sys_clk);
> > > + if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
> > > + clk_disable_unprepare(pll1_sys_clk);
> > > + clk_set_rate(pll1_sys_clk, freqs.new * 1000);
> > > + clk_prepare_enable(pll1_sys_clk);
> > > + clk_set_parent(pll1_sw_clk, pll1_sys_clk);
> > > + clk_disable_unprepare(pll2_pfd2_396m_clk);
> > > + } else {
> > > + /*
> > > + * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient
> > > + * to provide the frequency.
> > > + */
> > > + clk_disable_unprepare(pll1_sys_clk);
> > > + }
> > Seems like we will get pll2_pfd2_396m_clk's use count mismatch? As every time cpu freq is changed, pll2_pfd2_396m_clk will be added at the beginning of this code piece, but only decreased when cpu freq > 396M, so everytime cpu freq changed to 396M, this pll2_pfd2_396m_clk will in increased?
>
> Ah, good catch. And pll1_sys_clk has the same problem. Since it's
> been verified by FSL kernel that we do not necessarily need to disable
> pll1_sys_clk before reprogramming it, I would choose to rewrite the
> code as below to make it cleaner and correct on clock usage.
>
> /*
> * The setpoints are selected per PLL/PDF frequencies, so we need to
> * reprogram PLL for frequency scaling. The procedure of reprogramming
> * PLL1 is as below.
> *
> * - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> * - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it
> * - Disable pll2_pfd2_396m_clk
> */
> clk_prepare_enable(pll2_pfd2_396m_clk);
> clk_set_parent(step_clk, pll2_pfd2_396m_clk);
> clk_set_parent(pll1_sw_clk, step_clk);
> if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
> clk_set_rate(pll1_sys_clk, freqs.new * 1000);
> /*
> * If we are leaving 396 MHz set-point, we need to enable
> * pll1_sys_clk and disable pll2_pfd2_396m_clk to keep
> * their use count correct.
> */
> if (freqs.old * 1000 <= clk_get_rate(pll2_pfd2_396m_clk)) {
> clk_prepare_enable(pll1_sys_clk);
> clk_disable_unprepare(pll2_pfd2_396m_clk);
> }
> clk_set_parent(pll1_sw_clk, pll1_sys_clk);
> clk_disable_unprepare(pll2_pfd2_396m_clk);
> } else {
> /*
> * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient
> * to provide the frequency.
> */
> clk_disable_unprepare(pll1_sys_clk);
> }
>
Looks good:)
> > > +
> > > + /* Ensure the arm clock divider is what we expect */
> > > + ret = clk_set_rate(arm_clk, freqs.new * 1000);
> > > + if (ret) {
> > > + dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
> > > + regulator_set_voltage_tol(arm_reg, volt_old, 0);
> > > + return ret;
> > > + }
> > > +
> > > + /* scaling down? scale voltage after frequency */
> > > + if (freqs.new < freqs.old) {
> > > + ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> > > + if (ret)
> > > + dev_warn(cpu_dev,
> > > + "failed to scale vddarm down: %d\n", ret);
> > > +
> > > + if (freqs.old == FREQ_1P2_GHZ / 1000) {
> > > + regulator_set_voltage_tol(pu_reg,
> > > + PU_SOC_VOLTAGE_NORMAL, 0);
> > > + regulator_set_voltage_tol(soc_reg,
> > > + PU_SOC_VOLTAGE_NORMAL, 0);
> > > + }
> > > + }
> > > +
> > > + for_each_online_cpu(cpu) {
> > > + freqs.cpu = cpu;
> > > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > + }
> > > +
> > Should we need to update the percpu loops_per_jiffy variable and global loops_per_jiffy? As the udelay and mdelay will rely on this global loops_per_jiffy? Or the latest kernel has handle it in other place such as cpufreq common driver? I remembered that common cpufreq driver only handle the noSMP case.
>
> No, it's not needed since commit ec971ea (ARM: add cpufreq transiton
> notifier to adjust loops_per_jiffy for smp) is in place.
>
> Shawn
I see, just check the latest kernel's implementation about global loops_per_jiffy, it is OK.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
