Re: [PATCH 4/7] powerpc/85xx: add support to JOG feature using cpufreq interface

[Scott Wood <scottwood@freescale.com>]

On 11/04/2011 07:36 AM, Zhao Chenhui wrote:
> From: Li Yang <leoli@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 <daveliu@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@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.

> +/*
> + * 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