[PATCH 08/14] cell: enable pause(0) in cpu_idle

[Arnd Bergmann]

On Middeweken 07 Dezember 2005 18:28, Milton Miller wrote:
> Hi Arnd.   Quite a few comments on this one.

Unfortunately, I can't test my changes here, because I'm still on
second generation hardware, while the code is written for the current
DD3.x CPUs. I hope Max can help me with some of this.

I'll send an updated patch for this to address your comments. It
will also have an update to not use pause(0) at all on DD2.x hardware.

> > +
> > +struct pmd {
> > +       struct pmd_regs __iomem *regs;
> > +       int power_management_enable;
> > +};
> 
> This name conflicts with the memory management system used throughout
> the kernel.  Please rename.

Ok, I'll change to 'struct cbe_pervasive'.
 
> > +       if (__get_cpu_var(pmd).power_management_enable)
> 
> How do you know __get_cpu_var is safe here? because this is only
> called in the idle loop which is bound?
> 
> > +       {
> > +               /* Disable EE during check for pause */
> > +               machine_state=mfmsr();
> > +               machine_state &= ~MSR_EE;
> > +               mtmsrd(machine_state);
> local_irq_disable() ?
> 
> > +               /* Pause the PU */
> > +               HMT_low();
> > +               multi_threading_control = 0;
> > +               mtspr(SPRN_CTRLT,multi_threading_control);
> > +
> > +               /* Re-enable EE after resuming */
> > +               machine_state=mfmsr();
> > +               machine_state |= MSR_EE;
> > +               mtmsrd(machine_state);
> local_irq_enable() ?

I'll put local_irq_{en,dis}able outside of the if(), that should take
care of both problems.


> > +       /* Enable DEC and EE interrupt request */
> > +       thread_switch_control  = mfspr(SPRN_TSC_CELL);
> > +       thread_switch_control |= TSCR_EE_ENABLE | TSCR_EE_BOOST;
> > +
> > +       if (smp_processor_id()%2)
> 
> smp_processor_id is software number, and does not necessarily
> correspond to the hardware thread id.   Either use the hw
> version, or better yet, read the PIR (spr 1023?) directly.
 
Hmm, who sets up the PIR? It would definitely be better to use
that for getting reliable information (e.g. when not using SMT),
but I'm not sure if PIR is available or correct on our boards.

> > +static struct pmd_regs __iomem *find_pmd_mmio(int cpu)
> > +{
> > +       struct device_node *node;
> > +       int node_number = cpu / 2;
> 
> hmm... so # threads / node hard coded in here ...

Yes, it really shouldn't, but we couldn't find a good way
around this. The association between logical CPUs, nodes and
threads is done through the ibm,ppc-interrupt-server#s
property of a CPU device node, right?
But how can I get back from a logical CPU number to the
thread number?
 
> > +       struct pmd_regs __iomem *pmd_mmio_area;
> > +       unsigned long real_address;
> > +
> > +       for (node = of_find_node_by_type(NULL, "cpu"); node;
> > +               node = of_find_node_by_type(node, "cpu")) {
> 
> perhaps
> for (node = NULL; node = of_find(..) ;) or =NULL then while
> 
> Somewhat long-winded, but ok the way it is.

I find Max' version more readable, even if it is a bit longer.
 
> > +                       if (node_number == *(int *)get_property(node, 
> > "node-id", NULL))
> > +                       break;
> > +       }
> > +
> > +       if (!node) {
> > +               printk(KERN_WARNING "PMD: CPU %d not found\n", cpu);
> > +               pmd_mmio_area = NULL;
> > +       } else {
> > +               real_address = *(long *)get_property(node, 
> > "pervasive", NULL);
> > +               pr_debug("PMD for CPU %d at %lx\n", cpu, real_address);
> > +               pmd_mmio_area = __ioremap(real_address, 0x1000, 
> > _PAGE_NO_CACHE);
> > +       }
> > +       return pmd_mmio_area;
> > +}
> > +
> > +void __init cell_pervasive_init(void)
> > +{
> > +       struct pmd *pmd;
> > +       int cpu;
> > +
> > +       if (!cpu_has_feature(CPU_FTR_PAUSE_ZERO))
> > +               return;
> > +
> > +       for_each_cpu(cpu) {
> > +               pmd = &per_cpu(pmd, cpu);
> > +               pmd->regs = find_pmd_mmio(cpu);
> > +       }
> 
> O(n**2) find loop, could combine to get O(n)

For the current generation, we won't have systems with more
than four CPU's so it should not matter at all.

> > +arch_initcall(enable_pause_zero_init);
> 
> arch_initcall functions should be static

right.
 
> > +
> > +void __init cell_pervasive_init(void);
> > +void enable_pause_zero(void *);
> > +void _pause_zero(void);
> 
> what is the single_underscore _pause_zero() ?
It's left over from an earlier version of the patch and can will go away.

> ohter functions are either arch_initcall or called by initcall
> in the same file and can be static.
ok.

> > +BEGIN_FTR_SECTION
> > +       mr      r22,r12    /* r12 has SRR1 saved */
> > +       srwi    r22,r22,16
> > +       andi.   r22,r22,MSR_WAKEMASK
> > +       cmpwi   r22,MSR_WAKEEE
> > +       beq     40f
> > +       cmpwi   r22,MSR_WAKEDEC
> > +       beq     42f
> > +       cmpwi   r22,MSR_WAKEMT
> > +       beq     43f
> > +END_FTR_SECTION_IFSET(CPU_FTR_PAUSE_ZERO)
> > +       b       system_reset_common
> > +40:    b       hardware_interrupt_common
> > +42:    b       decrementer_common
> > +43:    EXCEPTION_PROLOG_COMMON(0x100, PACA_EXGEN);
> > +       b       fast_exception_return
> > +
> 
> Branches to branches that must be in the same file, within the first
> 64k, currently within 32k.   Just make the conditional branches
> directly to the other routines.

ok.

> This could go inline with system_reset_common, except that it  would
> mean breaking apart the STD_EXCEPTION_COMMON macro for it.  Space
> optimization would then be to put the test for WAKEMT after
> PROLOG_COMMON at the expense of breaking up the tests.

Max, do you want to try doing this?

> We have multiple idle loops and ppc_md.idle_loop to avoid junk like 
> this.  Assign the idle-loop based on the cpu feature.  Place it in
> persavisive.c, then you can make pause_zero static, and it will be 
> inline.  All better for power (fewer tests and branches).

ok.

> >
> > +#ifdef CONFIG_PPC_CELL
> > +extern void pause_zero(void);
> > +#else
> > +static inline void pause_zero(void)
> > +{
> > +}
> > +#endif
> > +
> 
> and you can stop polluting processor.h with something you only want
> called in a pinned cpu context from your idle loop.

ok.

> > -#define        SPRN_TSCR       0x399   /* Thread switch control on BE 
> > */
> > -#define        SPRN_TTR        0x39A   /* Thread switch timeout on BE 
> > */
> > -#define          TSCR_DEC_ENABLE       0x200000 /* Decrementer 
> > Interrupt */
> > +#define        SPRN_TSC_CELL   0x399   /* Thread switch control on 
> > Cell */
> > +#define        SPRN_TTR        0x39A   /* Thread switch timeout on 
> > Cell */
> > +#define          TSC_DEC_ENABLE_0      0x400000 /* Decrementer 
> > Interrupt */
> > +#define          TSC_DEC_ENABLE_1      0x200000 /* Decrementer 
> > Interrupt */
> 
> The prefix should be the name of the register to which they apply and
> directly under that register.

Ok.

Thanks for reviewing this.

	Arnd <><