Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> The biggest change is that windfarm is ported to generally use
> the new model, and I've written a new set of windfarm modules to
> take over from the old therm_pm72 (which was mostly unfixable)
> on the PowerMac G5 AGP and Xserve G5 machines.

I have the impression that the new driver keeps the fans running faster
all the time.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Wed, 2012-04-25 at 10:00 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > The biggest change is that windfarm is ported to generally use
> > the new model, and I've written a new set of windfarm modules to
> > take over from the old therm_pm72 (which was mostly unfixable)
> > on the PowerMac G5 AGP and Xserve G5 machines.
> 
> I have the impression that the new driver keeps the fans running faster
> all the time.

There's a few things you can try. Both drivers put the various values &
speeds in sysfs, tho in different places. You can do a test such as
overnight idle or something like that and compare the averages.

Also, does the new driver properly react to load ?

The algorithm should be identical and the factors fed to it as well, but
there's always the possibility that I screwed up something.

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Also, does the new driver properly react to load ?

The old driver keeps the cpu fans running at 300 rpm (lowest speed?)
for much longer when the cpus are put busy.  Only when the cpus are back
idle it speeds them up to 1200 rpm or more for some time depending on
how long the cpus were busy.  The new driver is faster at speeding up
the fans to around 800 rpm when cpus get busy, and keeps them running
longer at that speed, but doesn't appear to select much higher speeds.

The old driver appears to be better suited to desktops, whereas the new
driver is probably better for servers.

But the most annoying sound appears to be coming from the slots fan.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Wed, 2012-04-25 at 12:29 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > Also, does the new driver properly react to load ?
> 
> The old driver keeps the cpu fans running at 300 rpm (lowest speed?)
> for much longer when the cpus are put busy.  Only when the cpus are back
> idle it speeds them up to 1200 rpm or more for some time depending on
> how long the cpus were busy.  The new driver is faster at speeding up
> the fans to around 800 rpm when cpus get busy, and keeps them running
> longer at that speed, but doesn't appear to select much higher speeds.
> 
> The old driver appears to be better suited to desktops, whereas the new
> driver is probably better for servers.

That's odd... as I said, the algorithm is supposed to be the same...

> But the most annoying sound appears to be coming from the slots fan.

Constant or changing ? The slots fan is set to a fixed speed, which I
thought was constant (well, I "tickle" it a bit but roughly it's
constant). Or is that not the case for you ?

Darwin has an algorithm for it based on getting some data from the video
driver in the AGP slot, but I don't have that.

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Constant or changing ? The slots fan is set to a fixed speed, which I
> thought was constant (well, I "tickle" it a bit but roughly it's
> constant).

This fuzzing is what makes it much more annoying.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Wed, 2012-04-25 at 23:54 +0200, Andreas Schwab wrote:
> 
> This fuzzing is what makes it much more annoying.
> 
Interesting, I didn't think that couple of percent of pwm would be
noticable (there's probably too much ambiant noise in the lab here for
me to notice).

What if you just comment out the tickle code ?

In theory it's only needed if we haven't changed any fan speed for a
long time (the FCU can then timeout and assume we aren't driving it,
ramping up all fans to full speed). I could implement that a bit more
intelligently.

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> What if you just comment out the tickle code ?

I haven't tried it yet, but I suspect it won't tickle the fcu any more
since wf_control_set avoids writing an unchanged value (unlike the old
driver).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Thu, 2012-04-26 at 11:13 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > What if you just comment out the tickle code ?
> 
> I haven't tried it yet, but I suspect it won't tickle the fcu any more
> since wf_control_set avoids writing an unchanged value (unlike the old
> driver).

Sure, the question is whether that fixes the "annoyance" :-)

I don't think we really need to tickle the FCU as long as we have that
#define set in windfarm_fcu to use the actual fan values rather than the
programmed one.

In fact, can you change that define around and see if it makes it behave
more like therm_pm72 overall ? IE That's the only -known- difference
between the old and new driver (+/- a bug / typo / etc..)

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> In fact, can you change that define around and see if it makes it behave
> more like therm_pm72 overall ? IE That's the only -known- difference
> between the old and new driver (+/- a bug / typo / etc..)

Yes, that's make the difference for the cpu fan control.  Note that
MacOS (at least 10.3, which is the only one I have) drives the fans the
same way as the old driver.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Thu, 2012-04-26 at 13:41 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > In fact, can you change that define around and see if it makes it behave
> > more like therm_pm72 overall ? IE That's the only -known- difference
> > between the old and new driver (+/- a bug / typo / etc..)
> 
> Yes, that's make the difference for the cpu fan control.  Note that
> MacOS (at least 10.3, which is the only one I have) drives the fans the
> same way as the old driver.

Ok, I'll switch that back then. It seemed more sensible to read the
actual fan values rather than the programmed ones (in fact I wonder if I
can just skip the read alltogether then and use a cached value but that
means I won't be able to detect failed fans...), but if you say it
behaves better, let's keep it the way it was.

As for the tickle, I'm not sure yet how to proceed. I'll look into it,
try various things. We can maybe just remove the tickle but that means
that a completely idle machine might start ramping up as the FCU times
out.

Finally, setting the slot fan from sysfs should be doable reasonably
easily. Stay tuned and thanks a lot for testing ! :-)

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Ok, I'll switch that back then. It seemed more sensible to read the
> actual fan values rather than the programmed ones (in fact I wonder if I
> can just skip the read alltogether then and use a cached value but that
> means I won't be able to detect failed fans...), but if you say it
> behaves better, let's keep it the way it was.

I don't actually care too much about this, since the most annoyance came
from the slots fan.

> As for the tickle, I'm not sure yet how to proceed. I'll look into it,
> try various things. We can maybe just remove the tickle but that means
> that a completely idle machine might start ramping up as the FCU times
> out.

The old driver gets away with it probably because it always writes to
the fcu even if the speed didn't change.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Thu, 2012-04-26 at 11:13 +0200, Andreas Schwab wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> 
>> > What if you just comment out the tickle code ?
>> 
>> I haven't tried it yet, but I suspect it won't tickle the fcu any more
>> since wf_control_set avoids writing an unchanged value (unlike the old
>> driver).
>
> Sure, the question is whether that fixes the "annoyance" :-)

Removing the tickling removes much of the annoyance.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Darwin has an algorithm for it based on getting some data from the video
> driver in the AGP slot, but I don't have that.

Perhaps the slots fan should be programmable from user space.  For my
use case I'm pretty sure I would never need to set it to more than the
minimum speed.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

The goal of this series is to convert a bulk of PowerMac i2c drivers
to the new "proper" i2c driver registration model. This series is
not complete in that there are still a few drivers to do but it
goes through the bulk of thermal control and some of the nastiest.

The biggest change is that windfarm is ported to generally use
the new model, and I've written a new set of windfarm modules to
take over from the old therm_pm72 (which was mostly unfixable)
on the PowerMac G5 AGP and Xserve G5 machines.

This had a bit of testing on a couple of PowerMac G5 AGP variants
but none at all on Xserve G5 since the one I have is unfortunately
dead.

If you want to help me test this, don't forget to disable therm_pm72,
and instead load windfarm_pm72 or windfarm_rm31 depending on your
machine (you can load or build-in the whole lot as well). As usual
windfarm don't properly auto-load yet, I still need to fix that.

So please test ! Especially if you have an XServe G5

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Jean Delvare]

On Thu, 19 Apr 2012 18:16:41 +1000, Benjamin Herrenschmidt wrote:
> The goal of this series is to convert a bulk of PowerMac i2c drivers
> to the new "proper" i2c driver registration model. This series is
> not complete in that there are still a few drivers to do but it
> goes through the bulk of thermal control and some of the nastiest.
> 
> The biggest change is that windfarm is ported to generally use
> the new model, and I've written a new set of windfarm modules to
> take over from the old therm_pm72 (which was mostly unfixable)
> on the PowerMac G5 AGP and Xserve G5 machines.
> 
> This had a bit of testing on a couple of PowerMac G5 AGP variants
> but none at all on Xserve G5 since the one I have is unfortunately
> dead.
> 
> If you want to help me test this, don't forget to disable therm_pm72,
> and instead load windfarm_pm72 or windfarm_rm31 depending on your
> machine (you can load or build-in the whole lot as well). As usual
> windfarm don't properly auto-load yet, I still need to fix that.
> 
> So please test ! Especially if you have an XServe G5

Benjamin, thanks a lot for doing this! These drivers were the last ones
blocking the removal of the legacy binding model in i2c-core. Testers
are very welcome. The earliest we can get these changes upstream, the
better.

Christian, can you please test this series? I'm sure Benjamin can send
the patches to you directly if needed.

-- 
Jean Delvare

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Christian Kujau]

Jean Delvare <khali@linux-fr.org> wrote:

>On Thu, 19 Apr 2012 18:16:41 +1000, Benjamin Herrenschmidt wrote:
>> The goal of this series is to convert a bulk of PowerMac i2c drivers
>> to the new "proper" i2c driver registration model. This series is
>> not complete in that there are still a few drivers to do but it
>> goes through the bulk of thermal control and some of the nastiest.
>> 
>> The biggest change is that windfarm is ported to generally use
>> the new model, and I've written a new set of windfarm modules to
>> take over from the old therm_pm72 (which was mostly unfixable)
>> on the PowerMac G5 AGP and Xserve G5 machines.
>> 
>> This had a bit of testing on a couple of PowerMac G5 AGP variants
>> but none at all on Xserve G5 since the one I have is unfortunately
>> dead.
>> 
>> If you want to help me test this, don't forget to disable therm_pm72,
>> and instead load windfarm_pm72 or windfarm_rm31 depending on your
>> machine (you can load or build-in the whole lot as well). As usual
>> windfarm don't properly auto-load yet, I still need to fix that.
>> 
>> So please test ! Especially if you have an XServe G5
>
>Benjamin, thanks a lot for doing this! These drivers were the last ones
>blocking the removal of the legacy binding model in i2c-core. Testers
>are very welcome. The earliest we can get these changes upstream, the
>better.
>
>Christian, can you please test this series? I'm sure Benjamin can send
>the patches to you directly if needed.
>
>-- 
>Jean Delvare

I may need a day or two to get to it, but I'll test it.

Thanks,
C.
-- 
make bzImage, not war

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Benjamin Herrenschmidt]

On Thu, 2012-04-19 at 11:37 +0200, Jean Delvare wrote:

> Benjamin, thanks a lot for doing this! These drivers were the last ones
> blocking the removal of the legacy binding model in i2c-core.

There's a couple left :-) I haven't done therm_windtunnel.c (here too, I
need testers when I'm done) and our sound drivers are still using the
old API afaik, that's next on my list.

>  Testers
> are very welcome. The earliest we can get these changes upstream, the
> better.
> 
> Christian, can you please test this series? I'm sure Benjamin can send
> the patches to you directly if needed.

The main problem is that I don't have a Xserve G5 anymore to test,
that's one driver that largely rewritten and totally untested...

Cheers,
Ben.

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Jean Delvare <khali@linux-fr.org> writes:

> These drivers were the last ones blocking the removal of the legacy
> binding model in i2c-core.

There are also still some uses in snd-aoa.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Jean Delvare]

Hallo Andreas,

On Thu, 19 Apr 2012 12:11:25 +0200, Andreas Schwab wrote:
> Jean Delvare <khali@linux-fr.org> writes:
> 
> > These drivers were the last ones blocking the removal of the legacy
> > binding model in i2c-core.
> 
> There are also still some uses in snd-aoa.

You're right, there are still 3 sound drivers which need to be
converted (aoa/onyx, aoa/tas and ppc/keywest.) I believe Benjamin will
take care of the latter when he is done with the thermal management
drivers. Not sure who can work on and/or test the aoa sound drivers.

-- 
Jean Delvare

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Jean Delvare <khali@linux-fr.org> writes:
>
>> These drivers were the last ones blocking the removal of the legacy
>> binding model in i2c-core.
>
> There are also still some uses in snd-aoa.

Never mind, I just found the commit in tiwai/sound.git.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates

[Andreas Schwab]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Jean Delvare <khali@linux-fr.org> writes:
>>
>>> These drivers were the last ones blocking the removal of the legacy
>>> binding model in i2c-core.
>>
>> There are also still some uses in snd-aoa.
>
> Never mind, I just found the commit in tiwai/sound.git.

Confused.... this was some old one which has nothing to do with the
legacy binding model conversion.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."