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?
> +#define POWMGTCSR_LOSSLESS_MASK 0x00400000
> +#define POWMGTCSR_JOG_MASK 0x00200000
Are these really masks, or just values to use?
> +#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?
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?
> +
> + 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().
> + /*
> + * 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.
> + 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().
> + /* the latency of a transition, the unit is ns */
> + policy->cpuinfo.transition_latency = 2000;
Is this based on observation?
> diff --git a/arch/powerpc/platforms/85xx/sleep.S
> b/arch/powerpc/platforms/85xx/sleep.S
> index 763d2f2..919781d 100644
> --- a/arch/powerpc/platforms/85xx/sleep.S
> +++ b/arch/powerpc/platforms/85xx/sleep.S
> @@ -59,6 +59,7 @@ powmgtreq:
> * r5 = JOG or deep sleep request
> * JOG-0x00200000, deep sleep-0x00100000
> */
> +_GLOBAL(mpc85xx_enter_jog)
> _GLOBAL(mpc85xx_enter_deep_sleep)
> lis r6, ccsrbase_low@ha
> stw r4, ccsrbase_low@l(r6)
Why does this need two entry points rather than a more appropriate name?
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index 3fe6d92..1d0c4e0 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -200,6 +200,14 @@ config CPU_FREQ_PMAC64
> This adds support for frequency switching on Apple iMac G5,
> and some of the more recent desktop G5 machines as well.
>
> +config MPC85xx_CPUFREQ
> + bool "Support for Freescale MPC85xx CPU freq"
> + depends on PPC_85xx && PPC32
> + select CPU_FREQ_TABLE
> + help
> + This adds support for frequency switching on Freescale MPC85xx,
> + currently including P1022 and MPC8536.
default y, given the dependencies? Or wait for more testing before we
do that?
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
