> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
