[PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs

[Andrew Jeffery]

Hi Matt,


On Thu, 2017-06-08 at 15:42 -0500, Matthew Barth wrote:
> 
> 
> On 06/08/17 1:12 AM, Andrew Jeffery wrote:
> > Non-standard attributes are an obstacle for userspace. Instead, use the
> > standard fanX_input attributes, and number the fast-rotor values in
> > [NR_CHANNEL, 2 * NR_CHANNEL - 1].

Uh, so this range should be [NR_CHANNEL + 1, 2 * NR_CHANNEL], given
hwmon uses 1-based indexing.

Joel: I'll send a v2 to fix this and another issue.

> > 
> > > > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> > Matt,
> > 
> > I have a couple of observations testing this on a Witherspoon system that has
> > the necessary modifications:
> > 
> > 1. The fast-rotor values are non-zero and vary despite the chassis power being
> >    off (the slow rotor values become non-zero when the chassis is powered-on).

>  I worked with Jordan on this issue and it was something pointed out
> to Maxim to fix where we had verified the fix using a sampling of
> updated max31785 chips in Oct 2016.

Okay.

>  He is working with them to find out if this is a pre-mfg batch we
> have that's not 0'd out at poweroff and what the current set of
> max31785 chips FW should do.

Right - please keep me up to date with the state of play.

>  (Along with this, he's again going to request an updated datasheet
> for rev 0x3040).

Great. Will it be a public release like the existing datasheet?

> > 2. The fast-rotor values seem to be 'sticky' - sometimes they don't
> > vary as
> >    often as I would expect (e.g. in fan ramp- up or down).


>  Not sure what you mean by 'sticky', but during Jordan and I
> investigating, we found that the fast rotor 2bytes were reading 0's
> during any speed change (up or down) and would only continue
> providing the tach feedback once the target speed had been deemed
> reached.

Right - the behaviour I'm seeing is that instead of 0's I read the last
reported value, until the target is reached. I hadn't understood
exactly *when* the state changed, but my gut feeling is that the value
change aligned with reaching the target as you suggest.

>  This is another point Jordan is bringing up to Maxim, however this
> should not preclude us from getting these fast rotor feedbacks
> enabled as we have application code in place that does not mark these
> as faulted due to this.

Okay, so it sounds like we're covered either way (zero, or the last
reported value).

> > 2.
> >  The fast-rotor attributes appear to correspond to the slow rotor
> > values.

>  What do you mean by attributes here?

hwmon sysfs attributes.

>  What were you seeing?

The attributes I assigned to the fast rotor values were reporting lower
values than the attributes I assigned to the slow rotor.

> Just for info...the faster of tach feedbacks (fanf_input) should
> always be in the 'upper' 2 bytes of the 0x90 reg and the slower
> (fans_input) should always be the 'lower' 2 bytes according to Maxim
> (and what I verified last year with this 0x3040 rev update).

I'll address this below in the patch where I've implemented the logic
for reading the 4-byte values off the bus.

>  So for the associated fanx_target, there will be differing speeds
> between the fans_input and fanf_input where these should be within a
> 15% deviation of the given fanx_target.
> 
> > On point 1, here's what I see out of the box:
> > 
> > > >     root at witherspoon:/sys/bus/i2c/drivers/max31785/3-0052/hwmon/hwmon1# grep '.*' fan*
> >     fan10_input:10190
> >     fan11_input:0
> >     fan12_input:0
> >     fan1_fault:0
> >     fan1_input:10714
> >     fan1_pulses:2
> >     fan1_target:10500
> >     fan2_fault:0
> >     fan2_input:10416
> >     fan2_pulses:2
> >     fan2_target:10500
> >     fan3_fault:0
> >     fan3_input:10744
> >     fan3_pulses:2
> >     fan3_target:10500
> >     fan4_fault:0
> >     fan4_input:10806
> >     fan4_pulses:2
> >     fan4_target:10500
> >     fan5_fault:0
> >     fan5_input:0
> >     fan5_pulses:1
> >     fan5_target:1638
> >     fan6_fault:0
> >     fan6_input:0
> >     fan6_pulses:1
> >     fan6_target:1638
> >     fan7_input:10135
> >     fan8_input:9920
> >     fan9_input:10162
> > 
> > Expanding on point 2, I issued `obmcutil chassison`, then proceeded to set
> > progressively increasing fan targets:
> > 
> > > >     root at witherspoon:/sys/bus/i2c/drivers/max31785/3-0052/hwmon/hwmon1# grep '.*' fan*
> >     fan10_input:7142
> >     fan11_input:0
> >     fan12_input:0
> >     fan1_fault:0
> >     fan1_input:6432
> >     fan1_pulses:2
> >     fan1_target:6500
> >     fan2_fault:0
> >     fan2_input:9816
> >     fan2_pulses:2
> >     fan2_target:9500
> >     fan3_fault:0
> >     fan3_input:8426
> >     fan3_pulses:2
> >     fan3_target:8500
> >     fan4_fault:0
> >     fan4_input:7515
> >     fan4_pulses:2
> >     fan4_target:7500
> >     fan5_fault:0
> >     fan5_input:0
> >     fan5_pulses:1
> >     fan5_target:1638
> >     fan6_fault:0
> >     fan6_input:0
> >     fan6_pulses:1
> >     fan6_target:1638

>  For witherspoon, fan5_target & fan6_target are not wired up so that
> 1638 target value is not valid and are never set (so it must be some
> default within the max31785).

Yes, understood.

>  So what did we decide on how these sysfs file names will be
> associated between _target and _input(s)? From our slack discussion,
> ie) fan1_input & fan2_input is the pair of feedbacks associated with
> fan1_target is that suppose to be true here?

No, I changed it as outlined in the commit message (modulo my off-by-
one error), as this made testing with the openbmc userspace easier.

> >     fan7_input:6009
> >     fan8_input:9351
> >     fan9_input:7944
> > 
> > The fanX_input values for X in [7, 10]

Again, due to some mental haziness, I was off-by-one here. That should
read [7, 11].

> >  are consistently lower than X in [1, 4].

>  There is a chance this is correct as until the thermal team examines
> the air flow, its not unheard of to have back pressure on fans
> causing them to actually spin slower, but regardless they should be
> within the 15% target deviation.

> It would be more beneficial to group each _input with its
> corresponding _target cuz it seems here, for example, that fan7_input
> & fan1_input are associated with fan1_target?

Is this a comment on how the driver is implemented or how I've
displayed the outputs for the analysis above?

> >  drivers/hwmon/max31785.c | 103 +++++++++++++++++++++++++----------------------
> >  1 file changed, 54 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
> > index ed47d08a0ada..ca69d0974c00 100644
> > --- a/drivers/hwmon/max31785.c
> > +++ b/drivers/hwmon/max31785.c
> > @@ -88,10 +88,14 @@ struct max31785 {
> > > > > >  	u8	mfr_fan_config[NR_CHANNEL];
> > > > > >  	u8	fault_status[NR_CHANNEL];
> > > > > >  	u16	pwm[NR_CHANNEL];
> > > > > > -	u16	tach_rpm[NR_CHANNEL];
> > > > > > -	u16	tach_rpm_fast[NR_CHANNEL];
> > > > > > +	u16	tach_rpm[NR_CHANNEL * 2];
> >  };
> > 
> > +static inline bool max31785_has_fast_rotor(struct max31785 *data)
> > +{
> > > > +	return !!(data->capabilities & MAX31785_CAP_FAST_ROTOR);
> > +}
> > +
> >  static int max31785_set_page(struct i2c_client *client,
> > > >  				u8 page)
> >  {
> > @@ -188,14 +192,14 @@ static int max31785_update_fan_speed(struct max31785 *data, u8 fan)
> > > >  	if (rc)
> > > >  		return rc;
> > 
> > > > -	if (data->capabilities & MAX31785_CAP_FAST_ROTOR) {
> > > > +	if (max31785_has_fast_rotor(data)) {
> > > >  		rc = max31785_smbus_read_long_data(data->client,
> > > >  				MAX31785_REG_FAN_SPEED_1);
> > > >  		if (rc < 0)
> > > >  			return rc;
> > 
> > > >  		data->tach_rpm[fan] = rc & 0xffff;
> > > > -		data->tach_rpm_fast[fan] = (rc >> 16) & 0xffff;
> > +		data->tach_rpm[NR_CHANNEL + fan] = (rc >> 16) & 0xffff;

This is where I interpret the top 16 bits as the fast fan value. For
reference, the implementation of max31785_smbus_read_long_data() is:

    /* Cut down version of i2c_smbus_xfer_emulated(), reading 4 bytes */
    static s64 max31785_smbus_read_long_data(struct i2c_client *client, u8 command)
    {
    	    unsigned char cmdbuf[1];
    	    unsigned char rspbuf[4];
    	    s64 rc;

    	    struct i2c_msg msg[2] = {
    	    	    {
    	    	    	    .addr = client->addr,
    	    	    	    .flags = 0,
    	    	    	    .len = sizeof(cmdbuf),
    	    	    	    .buf = cmdbuf,
    	    	    },
    	    	    {
    	    	    	    .addr = client->addr,
    	    	    	    .flags = I2C_M_RD,
    	    	    	    .len = sizeof(rspbuf),
    	    	    	    .buf = rspbuf,
    	    	    },
    	    };

    	    cmdbuf[0] = command;

    	    rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
    	    if (rc < 0)
    	    	    return rc;

    	    rc = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
    	         (rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));

    	    return rc;
    }

Particularly,

	rc = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
	     (rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));

Is derived from the i2c_smbus_xfer_emulated() implementation, expanding
it out to 4 bytes. From the SMBus specification the low byte is sent
first, followed by the high byte. Your suggestion was the slow rotor is
still reported in the first two bytes for the 0x3040 firmware, and I
assume the fast rotor word conforms to the SMBus standard, therefore we
arrive at the implementation above for rspbuf bytes 2 and 3.

However, as I reported, I'm seeing *lower* RPM values for the 'fast'
bytes (2 and 3 in rspbuf, or bits 31-16 in the returned 32-bit
quantity). Can you please try the patch yourself?

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170609/9756556d/attachment.sig>