Re: [linux-sh:03225] Re: SH SCI output weirdness...

On Tue, May 04, 2004 at 09:48:55PM +0100, Stuart MENEFY wrote:
> The problem is the idle loop has been modified to call cpu_relax(), and
> cpu_relax() now includes a "sleep" instruction. The hlt_counter, although
> still present, is ignored.
> 
That's an oversight, the hlt_counter should be added back in to cpu_idle().
Additionally, it's probably worth having cpu_relax() check hlt_counter
prior to emitting the sleep instruction.

Putting this into cpu_relax() was ultimately a power saving hack (in the same
capacity as x86 hlt). But this does depend on the power-down mode config.
I agree that cpu_relax() is a bit of a problematic case, but in the case
where a module is well aware of when it can sleep, it certainly helps in
the power consumption case. As such, the power-down mode configuration needs
to be quite precise.

In the event that sleep is possible, it's certainly nicer to have in a busy
loop than a do { } while (0), particularly if you're going to be there for
awhile.

Perhaps if DPM is ever merged we can setup some more intelligent policies for
things like this.

> On the ST40, with the default power management configuration, this appears
> to try and power down or reduce the clock speed of the serial port. So any
> data still in the FIFO gets corrupted.
> 
In that case, ST40 needs to setup the power management configuration in a way
that won't power down serial, or hlt_counter needs to be wrapped in and then
disabled for ST40.

> Personally I think doing a "sleep" in the middle of cpu_relax() is wrong,
> as it is typically used in busy loops like spinlocks where there is no
> guarantee that there will be something to wake you back up. It also has an
> overhead which you may not want. Using it in the idle loop is more
> appropriate, as typically worth while going to sleep there, and you can
> take appropriate measures to make sure it is safe.
> 
It's used in the idle loop as well. Notably, this is the first indication
that its usage is problematic (and in this case, it's due to issues with
the default configuration of the ST40), so I don't see a particular reason
to back it out at this point. Although perhaps we can wrap this under
CONFIG_PM if there's enough interest.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQFAmBhQ1K+teJFxZ9wRAgr0AJ9KKZ9cb3gjJe9abOYGxorEoV09KgCfXPBU
kIQ+C/PRxLoTv690uywt0X0=
=nqoi
-----END PGP SIGNATURE-----