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

[Zhao Chenhui <chenhui.zhao@freescale.com>]

On Wed, Nov 16, 2011 at 06:17:56PM -0600, Scott Wood wrote:
> On 11/16/2011 03:55 AM, Zhao Chenhui wrote:
> > From: Li Yang <leoli@freescale.com>
> > 
> > Some 85xx silicons like MPC8536 and P1022 has the JOG PM feature.
> 
> P1023 as well -- any plan to support?
> 
> I see this in the p1022 and mpc8536 manuals:
> 
> > The system operates as if a request to enter sleep mode has occurred, with the exception that the
> > values written into the PMCDR register (clock disable register for sleep/ deep sleep modes) are
> > ignored, and it is treated as if every bit in PMCDR is a logic 1. This means that the eTSECs, USB
> > controllers, DDR and eLBC will be stopped.
> 
> ...which doesn't sound good.
> 
> > 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(except MPC8536), 3:2, 2:1, 5:2, 3:1, 7:2 and 4:1.
> 
> The ratios supported are implementation-specific.  Only p1022 supports
> 1:1.  p1023 supports only 3:2, 2:1, 5:2, and 3:1 (assuming the
> preliminary manual I have is accurate).
> 
> > +	local_irq_save(flags);
> > +	/*
> > +	 * A Jog request can not be asserted when any core is in a low power
> > +	 * state. Before executing a jog request, any core which is in
> > +	 * a low power state must be waked by a interrupt.
> > +	 */
> > +	if (mpc85xx_freqs == p1022_freqs_table) {
> > +		powersave = ppc_md.power_save;
> > +		ppc_md.power_save = NULL;
> > +		wmb();
> > +		val = in_be32(guts + POWMGTCSR);
> > +		for_each_online_cpu(i) {
> > +			if (val & ((POWMGTCSR_CORE0_DOZING |
> > +					POWMGTCSR_CORE0_NAPPING) << (i * 2)))
> > +				smp_send_reschedule(i);
> > +		}
> > +	}
> 
> This is racy, what if another core read ppc_md.power_save just before
> you wrote NULL, but hasn't yet entered a low power state?
> 

Yes, It's rare but it is possible. Perhaps I can check if the core is
in ppc_md.power_save() by the flag _TLF_NAPPING.

> You should send a reschedule to all cores regardless of what you see in
> POWMGTCSR.
> 
> The p1022 also says that MSR[EE] should be zero -- it is on this core,
> but what about the other?
> 
> > +	setbits32(guts + POWMGTCSR, POWMGTCSR_JOG_MASK);
> 
> This might work on p1022, but don't you have to go through a core reset
> on mpc8536?  In that case, you can't just set the bit, you have to go
> through the deep sleep code to save/restore state.
> 
> P1022 also says, "Mask all the interrupts to the cores by setting the
> bits CORE_UDE_MSK, CORE_MCP_MSK, CORE_INT_MSK and CORE_CINT_MSK in the
> POWMGTCSR," which I don't see happening.

I will fix them.

> 
> Though, this directly contradicts where it later says, "The user must
> not issue a jog request at the same time as issuing a request
> for another low power mode, or while the system is in the process of
> entering a low power mode. This means that a jog request must not be
> asserted when any other bit of POWMGTCSR is non-zero. If the user tries
> to do this, the jog request is ignored."
> 
> POWMGTCSR must be zero except for the JOG bit, but you must set other
> POWMGTCSR bits.  Lovely. :-P  I assume that the "This means..."
> statement is just wrong, and you really are supposed to set those other
> bits.  P1023 refines the statement to, "This means that POWMGTCSR[JOG]
> must not be asserted when any of the other power management request bits
> (COREn_DOZ, SLP) in POWMGTCSR are set."
> 
> > +	if (powersave) {
> > +		ppc_md.power_save = powersave;
> > +		wmb();
> > +	}
> 
> How do you know the jog has happened at this point?  Just because you've
> issued a store that requests it doesn't mean it has taken effect by the
> time you execute the next instruction.
> 
> > +	local_irq_restore(flags);
> > +
> > +	/* verify */
> > +	if (!spin_event_timeout(get_pll(hw_cpu) == pll, 10000, 10)) {
> > +		pr_err("%s: Fail to switch the core frequency. "
> > +			"The current PLL of core %d is %d instead of %d.\n",
> > +				__func__, hw_cpu, get_pll(hw_cpu), pll);
> > +		ret = -EINVAL;
> > +	}
> 
> Shouldn't the pll be where it's supposed to be as soon as we resume
> execution?  I don't see a need to spin here, provided we properly wait
> for the jog to happen earlier (which we want to do so that we don't
> enable power_save and EE early).

I found some delay is needed to wait the pll to update in tests.

-chenhui