On 11/16/2011 03:55 AM, Zhao Chenhui wrote:
> From: Li Yang <le...@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?
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.
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).
> +static int mpc85xx_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq,
> + unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned int new;
> + int ret = 0;
> +
> + 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);
> +
> + ret = set_pll(policy->cpu, mpc85xx_freqs[new].index);
> + if (!ret) {
> + pr_info("cpufreq: Setting core%d frequency to %d kHz and " \
> + "PLL ratio to %d:2\n",
> + policy->cpu,
> + mpc85xx_freqs[new].frequency,
> + mpc85xx_freqs[new].index);
> +
> + ppc_proc_freq = freqs.new * 1000ul;
> + }
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + mutex_unlock(&mpc85xx_switch_mutex);
I still do not understand what sense it makes to set a global variable
(ppc_proc_freq) to the frequency of a specific CPU.
> +static int mpc85xx_job_probe(struct platform_device *ofdev)
> +{
> + struct device_node *np = ofdev->dev.of_node;
> +
> + if (of_device_is_compatible(np, "fsl,mpc8536-guts")) {
> + threshold_freq = FREQ_800MHz;
> + mpc85xx_freqs = mpc8536_freqs_table;
> + } else if (of_device_is_compatible(np, "fsl,p1022-guts")) {
> + threshold_freq = FREQ_533MHz;
> + mpc85xx_freqs = p1022_freqs_table;
> + }
Maybe use .data in the of_device_id table, similar to
arch/powerpc/platforms/83xx/suspend.c? Though it's slightly less
convenient now that we need to call of_match_device() again in order to
get a match pointer.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
