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

[Milton Miller]

On Dec 16, 2005, at 6:47 PM, Arnd Bergmann wrote:

> This patch enables support for pause(0) power management state
> for the Cell Broadband Processor, which is import for power efficient
> operation. The pervasive infrastructure will in the future enable
> us to introduce more functionality specific to the Cell's
> pervasive unit.


Not to be a pain, but a few more comments mostly on things that have 
changed.


> +
> +static spinlock_t cbe_pervasive_lock;
>
= SPIN_LOCK_INIT

Initialize statically allocated locks statically.

> +	p = &cbe_pervasive[get_cpu()];
> +	spin_lock_irq(&cbe_pervasive_lock);
>

Since you are going atomic here, you could move the assignment inside
and use __get_cpu()

> +out:
> +	spin_unlock_irq(&cbe_pervasive_lock);
> +	put_cpu();

and then remove the put_cpu here.

> +}
> +
> +static void cbe_idle(void)
> +{
> +	unsigned long ctrl;
> +
> +	cbe_enable_pause_zero();

I'm guessing you don't need to do all of that work each time.  Maybe
set a flag in you pervasive structure to say that you did the work on 
this cpu.

> +
> +	while (1) {
> +		if (!need_resched()) {

you shoud disable interrupts here
> +			while (!need_resched()) {
> +				/* go into low thread priority */
> +				HMT_low();
> +
> +				/* go into low power mode */
> +				local_irq_disable();
> +				ctrl = mfspr(SPRN_CTRLF);
> +				ctrl &= ~(CTRL_RUNLATCH | CTRL_TE);
> +				mtspr(SPRN_CTRLT, ctrl);
> +				local_irq_enable();
> +			}
and not enable until here ...

so that you don't take an interrupt that sets need_resched() between 
your check and your sleep.   (Your sleep impliclity enables the 
interrupts to do the work).

> +			/* restore thread prio */
> +			HMT_medium();
> +		}
> +
> +		ppc64_runlatch_on();
> +		preempt_enable_no_resched();
> +		schedule();
> +		preempt_disable();
> +	}
> +}
> +

> +	for (node = NULL; (node = of_find_node_by_type(node, "cpu"));) {
> +		int_servers = (void *) get_property(node,
> +				"ibm,ppc-interrupt-server#s", &proplen);
> +		if (!int_servers) {
> +			printk(KERN_WARNING "CPU device misses "
> +				"ibm,ppc-interrupt-server#s property");
> +			continue;

%s, node->full_name


> +
> +void __init cell_pervasive_init(void)
> +{
> +	struct cbe_pervasive *p;
> +	int cpu;
> +	int ret;
> +
> +	spin_lock_init(&cbe_pervasive_lock);
> +

initialize staticly instead of at runtime as mentioned above.