On Fri, Nov 04, 2011 at 02:42:54PM -0500, Scott Wood wrote:
> On 11/04/2011 07:36 AM, Zhao Chenhui wrote:
> > From: Li Yang <le...@freescale.com>
> >
> > Some 85xx silicons like MPC8536 and P1022 has the JOG PM feature.
> >
> > The patch adds the support to change CPU frequency using the standard
> > cpufreq interface. Add the all PLL ratio core support. The ratio CORE
> > to CCB can 1:1, 1.5, 2:1, 2.5:1, 3:1, 3.5:1 and 4:1
> >
> > Signed-off-by: Dave Liu <dave...@freescale.com>
> > Signed-off-by: Li Yang <le...@freescale.com>
> > Signed-off-by: Jerry Huang <chang-ming.hu...@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.z...@freescale.com>
> > ---
> > arch/powerpc/platforms/85xx/Makefile | 1 +
> > arch/powerpc/platforms/85xx/cpufreq.c | 255
> > +++++++++++++++++++++++++++++++++
> > arch/powerpc/platforms/Kconfig | 8 +
> > 3 files changed, 264 insertions(+), 0 deletions(-)
> > create mode 100644 arch/powerpc/platforms/85xx/cpufreq.c
>
> Please name this something more specific, such as 85xx/cpufreq-jog.c
>
> Other 85xx/qoriq chips, such as p4080, have different mechanisms for
> updating CPU frequency.
>
> > +static struct cpufreq_frequency_table mpc85xx_freqs[] = {
> > + {2, 0},
> > + {3, 0},
> > + {4, 0},
> > + {5, 0},
> > + {6, 0},
> > + {7, 0},
> > + {8, 0},
> > + {0, CPUFREQ_TABLE_END},
> > +};
>
> Only p1022 can handle 1:1 (index 2).
>
> > +static void set_pll(unsigned int pll, int cpu)
> > +{
> > + int shift;
> > + u32 busfreq, corefreq, val;
> > + u32 core_spd, mask, tmp;
> > +
> > + tmp = in_be32(guts + PMJCR);
> > + shift = (cpu == 1) ? CORE1_RATIO_SHIFT : CORE0_RATIO_SHIFT;
> > + busfreq = fsl_get_sys_freq();
> > + val = (pll & CORE_RATIO_MASK) << shift;
> > +
> > + corefreq = ((busfreq * pll) >> 1);
>
> Use "/ 2", not ">> 1". Same asm code, more readable.
>
> > + /* must set the bit[18/19] if the requested core freq > 533 MHz */
> > + core_spd = (cpu == 1) ? PMJCR_CORE1_SPD_MASK : PMJCR_CORE0_SPD_MASK;
> > + if (corefreq > FREQ_533MHz)
> > + val |= core_spd;
>
> this is the cutoff for p1022 -- on mpc8536 the manual says the cutoff is
> 800 MHz.
>
> > + mask = (cpu == 1) ? (PMJCR_CORE1_RATIO_MASK | PMJCR_CORE1_SPD_MASK) :
> > + (PMJCR_CORE0_RATIO_MASK | PMJCR_CORE0_SPD_MASK);
> > + tmp &= ~mask;
> > + tmp |= val;
> > + out_be32(guts + PMJCR, tmp);
>
> clrsetbits_be32()
>
> > + val = in_be32(guts + PMJCR);
> > + out_be32(guts + POWMGTCSR,
> > + POWMGTCSR_LOSSLESS_MASK | POWMGTCSR_JOG_MASK);
>
> setbits32()
>
> > + pr_debug("PMJCR request %08x at CPU %d\n", tmp, cpu);
> > +}
> > +
> > +static void verify_pll(int cpu)
> > +{
> > + int shift;
> > + u32 busfreq, pll, corefreq;
> > +
> > + shift = (cpu == 1) ? CORE1_RATIO_SHIFT : CORE0_RATIO_SHIFT;
> > + busfreq = fsl_get_sys_freq();
> > + pll = (in_be32(guts + PORPLLSR) >> shift) & CORE_RATIO_MASK;
> > +
> > + corefreq = (busfreq * pll) >> 1;
> > + corefreq /= 1000000;
> > + pr_debug("PORPLLSR core freq %dMHz at CPU %d\n", corefreq, cpu);
> > +}
>
> It looks like the entire point of this function is to make a debug
> print... #ifdef DEBUG the contents? Or if we mark fsl_get_sys_freq()
> as __pure (or better, read this once at init, since it involves
> searching the device tree), will it all get optimized away?
>
>
> > + /* initialize frequency table */
> > + pr_info("core %d frequency table:\n", policy->cpu);
> > + for (i = 0; mpc85xx_freqs[i].frequency != CPUFREQ_TABLE_END; i++) {
> > + mpc85xx_freqs[i].frequency =
> > + (busfreq * mpc85xx_freqs[i].index) >> 1;
> > + pr_info("%d: %dkHz\n", i, mpc85xx_freqs[i].frequency);
> > + }
>
> This should be pr_debug.
>
> > + /* the latency of a transition, the unit is ns */
> > + policy->cpuinfo.transition_latency = 2000;
> > +
> > + cur_pll = get_pll(policy->cpu);
> > + pr_debug("current pll is at %d\n", cur_pll);
> > +
> > + for (i = 0; mpc85xx_freqs[i].frequency != CPUFREQ_TABLE_END; i++) {
> > + if (mpc85xx_freqs[i].index == cur_pll)
> > + policy->cur = mpc85xx_freqs[i].frequency;
> > + }
>
> You could combine these loops.
>
> > + /* this ensures that policy->cpuinfo_min
> > + * and policy->cpuinfo_max are set correctly */
>
> comment style
>
> > +static int mpc85xx_cpufreq_target(struct cpufreq_policy *policy,
> > + unsigned int target_freq,
> > + unsigned int relation)
> > +{
> > + struct cpufreq_freqs freqs;
> > + unsigned int new;
> > +
> > + cpufreq_frequency_table_target(policy,
> > + mpc85xx_freqs,
> > + target_freq,
> > + relation,
> > + &new);
> > +
> > + freqs.old = policy->cur;
> > + freqs.new = mpc85xx_freqs[new].frequency;
> > + freqs.cpu = policy->cpu;
> > +
> > + mutex_lock(&mpc85xx_switch_mutex);
> > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +
> > + pr_info("Setting frequency for core %d to %d kHz, " \
> > + "PLL ratio is %d/2\n",
> > + policy->cpu,
> > + mpc85xx_freqs[new].frequency,
> > + mpc85xx_freqs[new].index);
> > +
> > + set_pll(mpc85xx_freqs[new].index, policy->cpu);
> > +
> > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > + mutex_unlock(&mpc85xx_switch_mutex);
> > +
> > + ppc_proc_freq = freqs.new * 1000ul;
>
> ppc_proc_freq is global -- can CPUs not have their frequencies adjusted
> separately?
>
> It should be under the lock, if the lock is needed at all.
>
There is only one ppc_proc_freq. no lock.
> > +/*
> > + * module init and destoy
> > + */
> > +static struct of_device_id mpc85xx_jog_ids[] __initdata = {
> > + { .compatible = "fsl,mpc8536-guts", },
> > + { .compatible = "fsl,p1022-guts", },
> > + {}
> > +};
> > +
> > +static int __init mpc85xx_cpufreq_init(void)
> > +{
> > + struct device_node *np;
> > +
> > + pr_info("Freescale MPC85xx CPU frequency switching driver\n");
>
> If you're going to print something here, print it after you find a node
> you can work with -- not on all 85xx/qoriq that have this driver enabled.
>
> -Scott
Thanks. I will fix them all.
-chenhui
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
