> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter > Roeck > Sent: Thursday, July 15, 2021 10:40 PM > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > Cc: Rob Herring <robh@xxxxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Jean Delvare <jdelvare@xxxxxxxx> > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho > properties > > On Thu, Jul 15, 2021 at 10:26:05AM +0000, Sa, Nuno wrote: > > > From: Rob Herring <robh@xxxxxxxxxx> > > > Sent: Monday, July 12, 2021 7:27 PM > > > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > Guenter Roeck <linux@xxxxxxxxxxxx>; Jean Delvare > > > <jdelvare@xxxxxxxx> > > > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add > tacho > > > properties > > > > > > [External] > > > > > > On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote: > > > > Add the bindings for the tacho signal evaluation parameters > which > > > depend > > > > on the FAN being used. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > > > > --- > > > > .../bindings/hwmon/adi,axi-fan-control.yaml | 12 > > > ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi- > fan- > > > control.yaml > b/Documentation/devicetree/bindings/hwmon/adi,axi- > > > fan-control.yaml > > > > index 6747b870f297..0481eb34d9f1 100644 > > > > --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan- > > > control.yaml > > > > +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan- > > > control.yaml > > > > @@ -37,6 +37,18 @@ properties: > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > enum: [1, 2, 4] > > > > > > > > + adi,tacho-25-us: > > > > + description: Expected tacho signal when the PWM is at 25%. > > > > + > > > > + adi,tacho-50-us: > > > > + description: Expected tacho signal when the PWM is at 50%. > > > > + > > > > + adi,tacho-75-us: > > > > + description: Expected tacho signal when the PWM is at 75%. > > > > + > > > > + adi,tacho-100-us: > > > > + description: Expected tacho signal when the PWM is at 100%. > > > > > > This looks like it should be common. But having PWM percents in > the > > > property names doesn't scale. This is also a property of the fan, not > > > the fan controller. > > > > Yes, I see that these parameters are definitely a property of the > attached > > fan but the evaluation of these timings are very specific to this > controller > > (I think). The way it works is that the HW can fully operate without > any > > runtime SW configuration. In this case, it will use the values in these > > registers to evaluate the tacho signal coming from the FAN. And the > HW > > really uses the evaluation points like this (0, 25%, 50% and 100%). It > has > > some predefined values that work for the FAN that was used to > develop > > the IP but naturally the evaluation will fail as soon as other FAN is > attached > > (resulting in fan fault interrupts). And yes, writing these parameters > is > > already SW configuration but what I mean with "runtime" is after > probe :). > > > > Are you sure you can ever get this stable ? Each fan has its own > properties > and tolerances. If you replace a fan in a given system, you might get > different RPM numbers. The RPM will differ widely from system to > system > and from fan to fan. Anything that assumes a specific RPM in > devicetree > data seems to be quite vulnerable to failures. I have experienced that > recently with a different chip which also tries to correlate RPM and > PWM > and fails quite miserably. > > In my experience, anything other than minimum fan speed is really a > recipe > for instability and sporadic false failures. Even setting a minimum fan > speed > is tricky because it depends a lot on the fan. I see what you mean. So, I had to go through this process when testing this changes because the fan I'm using is different from the default one used to develop and stablish the default values in the IP core. The core provides you with a register which contains the tacho measurements in clock cycles. You can read that for all the PWM points of interest (with devmem2 for example) and make your own "calibration". I assume that people have to go through this process before putting some values in the devicetree. I'm aware this is not the neatest process but I guess it's acceptable... > > So, I honestly do not know how we could name this better... Maybe > a > > 'tacho-eval-points-us' array? The question would be the min/max > > elements? Do you have any suggestion for a more generic property? > > > I am guessing that the "us" refers to the time between pulses from the > fan. I think this is a bad value to start with - anything fan speed > related should really be expressed in RPM, not in time between > pulses. > > Overall I don't think this should be handled as generic set of > properties. > Whatever we come up with as standard set of pwm or fan related > properties > should not be an expected correlation between pwm and rpm. > Assuming such > a property is needed here (after all, the controller is what it is), > maybe a set of tuples makes sense, such as > > adi,pwm-rpm-map = < > 25, 250, > 50, 500, > 75, 750, > 100, 1000 > >; > > though I think that each of those should also include the tolerance > instead of just assuming that a 25% tolerance (as implemented in patch > 2/6) would work for all fans. Yes, this makes sense thanks. As the HW default tolerance is 25% I was somehow attached to that. Since all these values are correlated it makes complete sense to also give the tolerance here as it makes things more flexible. The map is also a good tip, I just have to see if there is a nice way to specify that the pwm column is constant... Thanks! - Nuno Sá
