Re: [PATCH v3] powerpc/85xx: add support to JOG feature using cpufreq interface
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_MASK0x0040
>>> +#define POWMGTCSR_JOG_MASK 0x0020
>>
>> 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 53334 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), 1, 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v3] powerpc/85xx: add support to JOG feature using cpufreq interface
> 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_MASK0x0040
> > +#define POWMGTCSR_JOG_MASK 0x0020
>
> Are these really masks, or just values to use?
They are masks.
>
> > +#define POWMGTCSR_CORE0_IRQ_MSK0x8000
> > +#define POWMGTCSR_CORE0_CI_MSK 0x4000
> > +#define POWMGTCSR_CORE0_DOZING 0x0008
> > +#define POWMGTCSR_CORE0_NAPPING0x0004
> > +
> > +#define POWMGTCSR_CORE_INT_MSK 0x0800
> > +#define POWMGTCSR_CORE_CINT_MSK0x0400
> > +#define POWMGTCSR_CORE_UDE_MSK 0x0200
> > +#define POWMGTCSR_CORE_MCP_MSK 0x0100
> > +#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 53334 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
Re: [PATCH v3] powerpc/85xx: add support to JOG feature using cpufreq interface
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 0x0040
> +#define POWMGTCSR_JOG_MASK 0x0020
Are these really masks, or just values to use?
> +#define POWMGTCSR_CORE0_IRQ_MSK 0x8000
> +#define POWMGTCSR_CORE0_CI_MSK 0x4000
> +#define POWMGTCSR_CORE0_DOZING 0x0008
> +#define POWMGTCSR_CORE0_NAPPING 0x0004
> +
> +#define POWMGTCSR_CORE_INT_MSK 0x0800
> +#define POWMGTCSR_CORE_CINT_MSK 0x0400
> +#define POWMGTCSR_CORE_UDE_MSK 0x0200
> +#define POWMGTCSR_CORE_MCP_MSK 0x0100
> +#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 53334 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), 1, 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
