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

[Arnd Bergmann]

On Sünnavend 17 Dezember 2005 07:01, Milton Miller wrote:

> 
> > +
> > +static spinlock_t cbe_pervasive_lock;
> >
> = SPIN_LOCK_INIT
> 
> Initialize statically allocated locks statically.

No, there are efforts to remove SPIN_LOCK_INIT from the kernel,
although I could not find out what exactly the reason is.
I looked this up again and found that 

static DEFINE_SPINLOCK(cbe_pervasive_lock);

would be the preferred way to write this now.

> > +	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()
> 
Yes.

> > +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.

I moved this here when I discovered that the idle function is is called
only once on every CPU and is never left, only scheduled away.

> > +
> > +	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).

Ah, yes. That was the reason we have this obscure method of waking up
in the first place.

	Arnd <><