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

[Andrew Jeffery]

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@xxxxxxxx>

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: signature.asc
Description: This is a digitally signed message part