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

[Scott Wood]

On 01/04/2012 03:34 AM, Zhao Chenhui-B35336 wrote:
>> 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.

Is mpc8536 rev 1.0 supported by the kernel in general?  If so, and this
code doesn't work with it, it needs to check for that revision and not
register the cpufreq handler if found.

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

They're bits.  Sometimes you use it additively, to set this bit along
with others.  Sometimes you use it subtractively, to test whether the
bit has cleared -- you could argue that it's used as a mask in that
context, but I don't think adding _MASK to the name really adds anything
here (likewise for things like PMJCR_CORE0_SPD_MASK).

>>> +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)".

You seem to have a different version of the p1022 manual than I (and the
FSL docs website) do.  In my copy 25.4.1 is "Performance Monitor
Interrupt" and it has no subsections.

PMJCR is described in 26.4.1.11 and for CORE0_SPD says:

> 0 Core0 frequency at 400–500 MHz
> 1 Core0 frequency at 500–1067 MHz

>>> +	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.

My point was that this specific function isn't really doing anything
p1022-specific, it's just a way to get other CPUs in the system to halt
until signalled to continue.  I thought it would be nice to just write
it generically from the start, but it's up to you.

>>> +	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.

OK, I missed that you're clearing more bits than you checked in
spin_event_timeout().  Could you rename P1022_POWMGTCSR_MSK to something
more meaningful (especially since you use _MASK all over the place to
mean something else)?

-Scott