Re: [PATCH v2 4/4] pmbus: max31785: Work around back-to-back writes with FAN_CONFIG_1_2

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6978 bytes --]

On Wed, 2017-08-02 at 16:45 +0930, Andrew Jeffery wrote:
> Testing of the pmbus max31785 driver implementation revealed occasional
> NACKs from the device. Attempting the same transaction immediately after
> the failure appears to always succeed. The NACK has consistently been
> observed to happen on the second write of back-to-back writes to the
> device, where the first write is to FAN_CONFIG_1_2. The NACK occurs
> against the first data byte transmitted by the master on the second
> write. The behaviour has the hallmarks of a PMBus Device Busy response,
> but the busy bit is not set in the status byte.
> 
> Work around this undocumented behaviour by retrying any back-to-back
> writes that occur after first writing FAN_CONFIG_1_2.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

I expect I'll be dropping this patch. At this point it looks like we
have another chip on the bus interfering with transactions to the
MAX31785. Checking the behaviour with a scope showed that SCL was being
held down by something that wasn't the master, but according to Maxim
the SCL pin on the MAX31785 is input-only and therefore it cannot
interfere in the manner we observed. Further testing has narrowed down
the list of candidates for the interference, but our investigation is
ongoing.

Andrew
 
> ---
>  drivers/hwmon/pmbus/max31785.c | 105 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 97 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index c2b693badcea..509b1a5a49b9 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -48,6 +48,55 @@ enum max31785_regs {
>  
> >  #define MAX31785_NR_PAGES		23
>  
> +/*
> + * MAX31785 dragons ahead
> + *
> + * It seems there's an undocumented timing constraint when performing
> + * back-to-back writes where the first write is to FAN_CONFIG_1_2. The device
> + * provides no indication of this besides NACK'ing master Txs; no bits are set
> + * in STATUS_BYTE to suggest anything has gone wrong.
> + *
> + * Given that, do a one-shot retry of the write.
> + *
> + * The max31785_*_write_*_data() functions should be used at any point where
> + * the prior write is to FAN_CONFIG_1_2.
> + */
> +static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client,
> > +					      int command, u16 data)
> +{
> > +	int ret;
> +
> > +	ret = i2c_smbus_write_byte_data(client, command, data);
> > +	if (ret == -EIO)
> > +		ret = i2c_smbus_write_byte_data(client, command, data);
> +
> > +	return ret;
> +}
> +
> +static int max31785_i2c_smbus_write_word_data(struct i2c_client *client,
> > +					      int command, u16 data)
> +{
> > +	int ret;
> +
> > +	ret = i2c_smbus_write_word_data(client, command, data);
> > +	if (ret == -EIO)
> > +		ret = i2c_smbus_write_word_data(client, command, data);
> +
> > +	return ret;
> +}
> +
> +static int max31785_pmbus_write_word_data(struct i2c_client *client, int page,
> > +					  int command, u16 data)
> +{
> > +	int ret;
> +
> > +	ret = pmbus_write_word_data(client, page, command, data);
> > +	if (ret == -EIO)
> > +		ret = pmbus_write_word_data(client, page, command, data);
> +
> > +	return ret;
> +}
> +
>  static int max31785_read_byte_data(struct i2c_client *client, int page,
> >  				   int reg)
>  {
> @@ -210,6 +259,31 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
> >  	return rv;
>  }
>  
> +static int max31785_update_fan(struct i2c_client *client, int page,
> > +			       u8 config, u8 mask, u16 command)
> +{
> > +	int from, rv;
> > +	u8 to;
> +
> > +	from = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
> > +	if (from < 0)
> > +		return from;
> +
> > +	to = (from & ~mask) | (config & mask);
> +
> > +	if (to != from) {
> > +		rv = pmbus_write_byte_data(client, page, PMBUS_FAN_CONFIG_12,
> > +					   to);
> > +		if (rv < 0)
> > +			return rv;
> > +	}
> +
> > +	rv = max31785_pmbus_write_word_data(client, page, PMBUS_FAN_COMMAND_1,
> > +					    command);
> +
> > +	return rv;
> +}
> +
>  static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0xffff };
>  
>  static int max31785_write_word_data(struct i2c_client *client, int page,
> @@ -219,12 +293,24 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
> >  		return -ENXIO;
>  
> >  	switch (reg) {
> > +	case PMBUS_VIRT_FAN_TARGET_1:
> > +		return max31785_update_fan(client, page, PB_FAN_1_RPM,
> > +					   PB_FAN_1_RPM, word);
> > +	case PMBUS_VIRT_PWM_1:
> > +	{
> > +		u32 command = word;
> +
> > +		command *= 100;
> > +		command /= 255;
> > +		return max31785_update_fan(client, page, 0, PB_FAN_1_RPM,
> > +					   command);
> > +	}
> >  	case PMBUS_VIRT_PWM_ENABLE_1:
> >  		if (word >= ARRAY_SIZE(max31785_pwm_modes))
> >  			return -EINVAL;
>  
> > -		return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
> > -					max31785_pwm_modes[word]);
> > +		return max31785_update_fan(client, page, 0, PB_FAN_1_RPM,
> > +					   max31785_pwm_modes[word]);
> >  	default:
> >  		break;
> >  	}
> @@ -262,7 +348,7 @@ static int max31785_of_fan_config(struct i2c_client *client,
> >  		return -ENXIO;
> >  	}
>  
> > -	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> > +	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> >  	if (ret < 0)
> >  		return ret;
>  
> @@ -419,7 +505,8 @@ static int max31785_of_fan_config(struct i2c_client *client,
> >  	if (ret < 0)
> >  		return ret;
>  
> > -	ret = i2c_smbus_write_word_data(client, MFR_FAN_CONFIG, mfr_cfg);
> > +	ret = max31785_i2c_smbus_write_word_data(client, MFR_FAN_CONFIG,
> > +						 mfr_cfg);
> >  	if (ret < 0)
> >  		return ret;
>  
> @@ -473,7 +560,7 @@ static int max31785_of_tmp_config(struct i2c_client *client,
> >  		return -ENXIO;
> >  	}
>  
> > -	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> > +	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> >  	if (ret < 0)
> >  		return ret;
>  
> @@ -631,7 +718,8 @@ static int max31785_probe(struct i2c_client *client,
> >  		if (!have_fan || fan_configured)
> >  			continue;
>  
> > -		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> > +		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE,
> > +							 i);
> >  		if (ret < 0)
> >  			return ret;
>  
> @@ -640,8 +728,9 @@ static int max31785_probe(struct i2c_client *client,
> >  			return ret;
>  
> >  		ret &= ~PB_FAN_1_INSTALLED;
> > -		ret = i2c_smbus_write_word_data(client, PMBUS_FAN_CONFIG_12,
> > -						ret);
> > +		ret = max31785_i2c_smbus_write_word_data(client,
> > +							 PMBUS_FAN_CONFIG_12,
> > +							 ret);
> >  		if (ret < 0)
> >  			return ret;
> >  	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]