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

[Zhao Chenhui-B35336]

> On 12/27/2011 05:25 AM, Zhao Chenhui wrote:
> >  * The driver doesn't support MPC8536 Rev 1.0 due to a JOG erratum.
> >    Subsequent revisions of MPC8536 have corrected the erratum.
> 
> Where do you check for this?

Nowhere. I just notify this patch don't support MPC8536 Rev 1.0.

> 
> > +#define POWMGTCSR_LOSSLESS_MASK	0x00400000
> > +#define POWMGTCSR_JOG_MASK	0x00200000
> 
> Are these really masks, or just values to use?

They are masks.

> 
> > +#define POWMGTCSR_CORE0_IRQ_MSK	0x80000000
> > +#define POWMGTCSR_CORE0_CI_MSK	0x40000000
> > +#define POWMGTCSR_CORE0_DOZING	0x00000008
> > +#define POWMGTCSR_CORE0_NAPPING	0x00000004
> > +
> > +#define POWMGTCSR_CORE_INT_MSK	0x00000800
> > +#define POWMGTCSR_CORE_CINT_MSK	0x00000400
> > +#define POWMGTCSR_CORE_UDE_MSK	0x00000200
> > +#define POWMGTCSR_CORE_MCP_MSK	0x00000100
> > +#define P1022_POWMGTCSR_MSK	(POWMGTCSR_CORE_INT_MSK | \
> > +				 POWMGTCSR_CORE_CINT_MSK | \
> > +				 POWMGTCSR_CORE_UDE_MSK | \
> > +				 POWMGTCSR_CORE_MCP_MSK)
> > +
> > +static void keep_waking_up(void *dummy)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	mb();
> > +
> > +	in_jog_process = 1;
> > +	mb();
> > +
> > +	while (in_jog_process != 0)
> > +		mb();
> > +
> > +	local_irq_restore(flags);
> > +}
> 
> Please document this.  Compare in_jog_process == 1, not != 0 -- it's
> unlikely, but what if the other cpu sees that in_jog_process has been
> set to 1, exits and sets in_jog_process to 0, then re-enters set_pll and
> sets in_jog_process to -1 again before this function does another load
> of in_jog_process?

Thanks. I'll fix it.

> 
> Do you really need all these mb()s?  I think this would suffice:
> 
> 	local_irq_save(flags);
> 
> 	in_jog_process = 1;
> 
> 	while (in_jog_process == 1)
> 		barrier();
> 
> 	local_irq_restore();
> 
> It's not really a performance issue, just simplicity.
> 
> > +static int p1022_set_pll(unsigned int cpu, unsigned int pll)
> > +{
> > +	int index, hw_cpu = get_hard_smp_processor_id(cpu);
> > +	int shift;
> > +	u32 corefreq, val, mask = 0;
> > +	unsigned int cur_pll = get_pll(hw_cpu);
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	if (pll == cur_pll)
> > +		return 0;
> > +
> > +	shift = hw_cpu * CORE_RATIO_BITS + CORE0_RATIO_SHIFT;
> > +	val = (pll & CORE_RATIO_MASK) << shift;
> > +
> > +	corefreq = sysfreq * pll / 2;
> > +	/*
> > +	 * Set the COREx_SPD bit if the requested core frequency
> > +	 * is larger than the threshold frequency.
> > +	 */
> > +	if (corefreq > FREQ_533MHz)
> > +		val |= PMJCR_CORE0_SPD_MASK << hw_cpu;
> 
> P1022 manual says the threshold is 500 MHz (but doesn't say how to set
> the bit if the frequency is exactly 500 MHz).  Where did 533340000 come
> from?

Please refer to Chapter 25 "25.4.1.11 Power Management Jog Control Register (PMJCR)".

> 
> > +
> > +	mask = (CORE_RATIO_MASK << shift) | (PMJCR_CORE0_SPD_MASK <<
> hw_cpu);
> > +	clrsetbits_be32(guts + PMJCR, mask, val);
> > +
> > +	/* readback to sync write */
> > +	val = in_be32(guts + PMJCR);
> 
> You don't use val after this -- just ignore the return value from
> in_be32().

OK.

> 
> > +	/*
> > +	 * A Jog request can not be asserted when any core is in a low
> > +	 * power state on P1022. Before executing a jog request, any
> > +	 * core which is in a low power state must be waked by a
> > +	 * interrupt, and keep waking up until the sequence is
> > +	 * finished.
> > +	 */
> > +	for_each_present_cpu(index) {
> > +		if (!cpu_online(index))
> > +			return -EFAULT;
> > +	}
> 
> EFAULT is not the appropriate error code -- it is for when userspace
> passes a bad virtual address.
> 
> Better, don't fail here -- bring the other core out of the low power
> state in order to do the jog.  cpufreq shouldn't stop working just
> because we took a core offline.
> 
> What prevents a core from going offline just after you check here?
> 
> > +	in_jog_process = -1;
> > +	mb();
> > +	smp_call_function(keep_waking_up, NULL, 0);
> 
> What does "keep waking up" mean?  Something like spin_while_jogging
> might be clearer.
> 
> > +	local_irq_save(flags);
> > +	mb();
> > +	/* Wait for the other core to wake. */
> > +	while (in_jog_process != 1)
> > +		mb();
> 
> Timeout?  And more unnecessary mb()s.
> 
> Might be nice to support more than two cores, even if this code isn't
> currently expected to be used on such hardware (it's just a generic
> "hold other cpus" loop; might as well make it reusable).  You could do
> this by using an atomic count for other cores to check in and out of the
> spin loop.

This is just for P1022, a dual-core chip. A separate patch will
support multi-core chips, such as P4080, etc.

> 
> > +	out_be32(guts + POWMGTCSR, POWMGTCSR_JOG_MASK |
> P1022_POWMGTCSR_MSK);
> > +
> > +	if (!spin_event_timeout(((in_be32(guts + POWMGTCSR) &
> > +	    POWMGTCSR_JOG_MASK) == 0), 10000, 10)) {
> > +		pr_err("%s: Fail to switch the core frequency.\n", __func__);
> > +		ret = -EFAULT;
> > +	}
> > +
> > +	clrbits32(guts + POWMGTCSR, P1022_POWMGTCSR_MSK);
> > +	in_jog_process = 0;
> > +	mb();
> 
> This mb() (or better, a readback of POWMGTCSR) should be before you
> clear in_jog_process.  For clarity of its purpose, the clearing of
> POWMGTCSR should go in the failure branch of spin_event_timeout().

According to the manual, P1022_POWMGTCSR_MSK should be reset
by software regardless of failure or success.

-chenhui