| Message ID | 20231023135655.2964972-1-lakshmiy@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (pmbus/max31785) Add delay between bus accesses | expand |
On 10/23/23 06:56, Lakshmi Yadlapati wrote: > The MAX31785 has shown erratic behaviour across multiple system > designs, unexpectedly clock stretching and NAKing transactions. > > Experimentation shows that this seems to be triggered by a register access > directly back to back with a previous register write. Experimentation also > shows that inserting a small delay after register writes makes the issue go > away. > > Use a similar solution to what the max15301 driver does to solve the same > problem. Create a custom set of bus read and write functions that make sure > that the delay is added. > > Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com> > --- > drivers/hwmon/pmbus/max31785.c | 167 ++++++++++++++++++++++++++++----- > 1 file changed, 146 insertions(+), 21 deletions(-) > > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c > index f9aa576495a5..40fafb3b1867 100644 > --- a/drivers/hwmon/pmbus/max31785.c > +++ b/drivers/hwmon/pmbus/max31785.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2017 IBM Corp. > */ > > +#include <linux/delay.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/init.h> > @@ -23,19 +24,98 @@ enum max31785_regs { > > #define MAX31785_NR_PAGES 23 > #define MAX31785_NR_FAN_PAGES 6 > +#define MAX31785_WAIT_DELAY_US 250 > > -static int max31785_read_byte_data(struct i2c_client *client, int page, > - int reg) > +struct max31785_data { > + ktime_t access; /* Chip access time */ > + struct pmbus_driver_info info; > +}; > + > +#define to_max31785_data(x) container_of(x, struct max31785_data, info) > + > +/* > + * MAX31785 Driver Workaround > + * > + * The MAX31785 fan controller occasionally exhibits communication issues. > + * These issues are not indicated by the device itself, except for occasional > + * NACK responses during master transactions. No error bits are set in STATUS_BYTE. > + * > + * To address this, we introduce a delay of 250us between consecutive accesses > + * to the fan controller. This delay helps mitigate communication problems by > + * allowing sufficient time between accesses. > + */ > + > +#define max31785_wait(_func, _driver_data, ...) ({ \ > + int _ret; \ > + s64 delta = ktime_us_delta(ktime_get(), driver_data->access); \ > + if (delta < MAX31785_WAIT_DELAY_US) \ > + udelay(MAX31785_WAIT_DELAY_US - delta); \ > + /* All relevant functions return int */ \ > + _ret = _func(__VA_ARGS__); \ > + _driver_data->access = ktime_get(); \ > + _ret; \ > +}) Please no function macros. This can easily be implemented as function, like with all other drivers having the same problem. > + > +static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int command, u16 data) Please reduce the size of local function names. i2c_smbus -> i2c, and the _pmbus in the functions below adds no value. Guenter > { > - if (page < MAX31785_NR_PAGES) > - return -ENODATA; > + return max31785_wait(i2c_smbus_write_byte_data, driver_data, client, > + command, data); > +} > + > +static int max31785_i2c_smbus_read_word_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int command) > +{ > + return max31785_wait(i2c_smbus_read_word_data, driver_data, client, > + command); > +} > + > +static int max31785_pmbus_read_byte_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int page, int command) > +{ > + return max31785_wait(pmbus_read_byte_data, driver_data, client, page, > + command); > +} > + > +static int max31785_pmbus_write_byte_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int page, int command, u16 data) > +{ > + return max31785_wait(pmbus_write_byte_data, driver_data, client, page, > + command, data); > +} > + > +static int max31785_pmbus_read_word_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int page, int phase, int command) > +{ > + return max31785_wait(pmbus_read_word_data, driver_data, client, page, > + phase, command); > +} > + > +static int max31785_pmbus_write_word_data(struct i2c_client *client, > + struct max31785_data *driver_data, > + int page, int command, u16 data) > +{ > + return max31785_wait(pmbus_write_word_data, driver_data, client, page, > + command, data); > +} > + > +static int max31785_read_byte_data(struct i2c_client *client, int page, int reg) > +{ > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + struct max31785_data *driver_data = to_max31785_data(info); > > switch (reg) { > case PMBUS_VOUT_MODE: > return -ENOTSUPP; > case PMBUS_FAN_CONFIG_12: > - return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES, > - reg); > + return max31785_pmbus_read_byte_data(client, driver_data, > + page - MAX31785_NR_PAGES, > + reg); > } > > return -ENODATA; > @@ -102,16 +182,19 @@ static int max31785_get_pwm(struct i2c_client *client, int page) > return rv; > } > > -static int max31785_get_pwm_mode(struct i2c_client *client, int page) > +static int max31785_get_pwm_mode(struct i2c_client *client, > + struct max31785_data *driver_data, int page) > { > int config; > int command; > > - config = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12); > + config = max31785_pmbus_read_byte_data(client, driver_data, page, > + PMBUS_FAN_CONFIG_12); > if (config < 0) > return config; > > - command = pmbus_read_word_data(client, page, 0xff, PMBUS_FAN_COMMAND_1); > + command = max31785_pmbus_read_word_data(client, driver_data, page, 0xff, > + PMBUS_FAN_COMMAND_1); > if (command < 0) > return command; > > @@ -129,6 +212,8 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page) > static int max31785_read_word_data(struct i2c_client *client, int page, > int phase, int reg) > { > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + struct max31785_data *driver_data = to_max31785_data(info); > u32 val; > int rv; > > @@ -157,7 +242,7 @@ static int max31785_read_word_data(struct i2c_client *client, int page, > rv = max31785_get_pwm(client, page); > break; > case PMBUS_VIRT_PWM_ENABLE_1: > - rv = max31785_get_pwm_mode(client, page); > + rv = max31785_get_pwm_mode(client, driver_data, page); > break; > default: > rv = -ENODATA; > @@ -188,8 +273,36 @@ static inline u32 max31785_scale_pwm(u32 sensor_val) > return (sensor_val * 100) / 255; > } > > -static int max31785_pwm_enable(struct i2c_client *client, int page, > - u16 word) > +static int max31785_update_fan(struct i2c_client *client, > + struct max31785_data *driver_data, int page, > + u8 config, u8 mask, u16 command) > +{ > + int from, rv; > + u8 to; > + > + from = max31785_pmbus_read_byte_data(client, driver_data, page, > + PMBUS_FAN_CONFIG_12); > + if (from < 0) > + return from; > + > + to = (from & ~mask) | (config & mask); > + > + if (to != from) { > + rv = max31785_pmbus_write_byte_data(client, driver_data, page, > + PMBUS_FAN_CONFIG_12, to); > + if (rv < 0) > + return rv; > + } > + > + rv = max31785_pmbus_write_word_data(client, driver_data, page, > + PMBUS_FAN_COMMAND_1, command); > + > + return rv; > +} > + > +static int max31785_pwm_enable(struct i2c_client *client, > + struct max31785_data *driver_data, int page, > + u16 word) > { > int config = 0; > int rate; > @@ -217,18 +330,23 @@ static int max31785_pwm_enable(struct i2c_client *client, int page, > return -EINVAL; > } > > - return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate); > + return max31785_update_fan(client, driver_data, page, config, > + PB_FAN_1_RPM, rate); > } > > static int max31785_write_word_data(struct i2c_client *client, int page, > int reg, u16 word) > { > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + struct max31785_data *driver_data = to_max31785_data(info); > + > switch (reg) { > case PMBUS_VIRT_PWM_1: > - return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM, > - max31785_scale_pwm(word)); > + return max31785_update_fan(client, driver_data, page, 0, > + PB_FAN_1_RPM, > + max31785_scale_pwm(word)); > case PMBUS_VIRT_PWM_ENABLE_1: > - return max31785_pwm_enable(client, page, word); > + return max31785_pwm_enable(client, driver_data, page, word); > default: > break; > } > @@ -303,13 +421,16 @@ static int max31785_configure_dual_tach(struct i2c_client *client, > { > int ret; > int i; > + struct max31785_data *driver_data = to_max31785_data(info); > > for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { > - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); > + ret = max31785_i2c_smbus_write_byte_data(client, driver_data, > + PMBUS_PAGE, i); > if (ret < 0) > return ret; > > - ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG); > + ret = max31785_i2c_smbus_read_word_data(client, driver_data, > + MFR_FAN_CONFIG); > if (ret < 0) > return ret; > > @@ -329,6 +450,7 @@ static int max31785_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > struct pmbus_driver_info *info; > + struct max31785_data *driver_data; > bool dual_tach = false; > int ret; > > @@ -337,13 +459,16 @@ static int max31785_probe(struct i2c_client *client) > I2C_FUNC_SMBUS_WORD_DATA)) > return -ENODEV; > > - info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL); > - if (!info) > + driver_data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL); > + if (!driver_data) > return -ENOMEM; > > + info = &driver_data->info; > + driver_data->access = ktime_get(); > *info = max31785_info; > > - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); > + ret = max31785_i2c_smbus_write_byte_data(client,driver_data, > + PMBUS_PAGE, 255); > if (ret < 0) > return ret; >
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c index f9aa576495a5..40fafb3b1867 100644 --- a/drivers/hwmon/pmbus/max31785.c +++ b/drivers/hwmon/pmbus/max31785.c @@ -3,6 +3,7 @@ * Copyright (C) 2017 IBM Corp. */ +#include <linux/delay.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/init.h> @@ -23,19 +24,98 @@ enum max31785_regs { #define MAX31785_NR_PAGES 23 #define MAX31785_NR_FAN_PAGES 6 +#define MAX31785_WAIT_DELAY_US 250 -static int max31785_read_byte_data(struct i2c_client *client, int page, - int reg) +struct max31785_data { + ktime_t access; /* Chip access time */ + struct pmbus_driver_info info; +}; + +#define to_max31785_data(x) container_of(x, struct max31785_data, info) + +/* + * MAX31785 Driver Workaround + * + * The MAX31785 fan controller occasionally exhibits communication issues. + * These issues are not indicated by the device itself, except for occasional + * NACK responses during master transactions. No error bits are set in STATUS_BYTE. + * + * To address this, we introduce a delay of 250us between consecutive accesses + * to the fan controller. This delay helps mitigate communication problems by + * allowing sufficient time between accesses. + */ + +#define max31785_wait(_func, _driver_data, ...) ({ \ + int _ret; \ + s64 delta = ktime_us_delta(ktime_get(), driver_data->access); \ + if (delta < MAX31785_WAIT_DELAY_US) \ + udelay(MAX31785_WAIT_DELAY_US - delta); \ + /* All relevant functions return int */ \ + _ret = _func(__VA_ARGS__); \ + _driver_data->access = ktime_get(); \ + _ret; \ +}) + +static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client, + struct max31785_data *driver_data, + int command, u16 data) { - if (page < MAX31785_NR_PAGES) - return -ENODATA; + return max31785_wait(i2c_smbus_write_byte_data, driver_data, client, + command, data); +} + +static int max31785_i2c_smbus_read_word_data(struct i2c_client *client, + struct max31785_data *driver_data, + int command) +{ + return max31785_wait(i2c_smbus_read_word_data, driver_data, client, + command); +} + +static int max31785_pmbus_read_byte_data(struct i2c_client *client, + struct max31785_data *driver_data, + int page, int command) +{ + return max31785_wait(pmbus_read_byte_data, driver_data, client, page, + command); +} + +static int max31785_pmbus_write_byte_data(struct i2c_client *client, + struct max31785_data *driver_data, + int page, int command, u16 data) +{ + return max31785_wait(pmbus_write_byte_data, driver_data, client, page, + command, data); +} + +static int max31785_pmbus_read_word_data(struct i2c_client *client, + struct max31785_data *driver_data, + int page, int phase, int command) +{ + return max31785_wait(pmbus_read_word_data, driver_data, client, page, + phase, command); +} + +static int max31785_pmbus_write_word_data(struct i2c_client *client, + struct max31785_data *driver_data, + int page, int command, u16 data) +{ + return max31785_wait(pmbus_write_word_data, driver_data, client, page, + command, data); +} + +static int max31785_read_byte_data(struct i2c_client *client, int page, int reg) +{ + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); + struct max31785_data *driver_data = to_max31785_data(info); switch (reg) { case PMBUS_VOUT_MODE: return -ENOTSUPP; case PMBUS_FAN_CONFIG_12: - return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES, - reg); + return max31785_pmbus_read_byte_data(client, driver_data, + page - MAX31785_NR_PAGES, + reg); } return -ENODATA; @@ -102,16 +182,19 @@ static int max31785_get_pwm(struct i2c_client *client, int page) return rv; } -static int max31785_get_pwm_mode(struct i2c_client *client, int page) +static int max31785_get_pwm_mode(struct i2c_client *client, + struct max31785_data *driver_data, int page) { int config; int command; - config = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12); + config = max31785_pmbus_read_byte_data(client, driver_data, page, + PMBUS_FAN_CONFIG_12); if (config < 0) return config; - command = pmbus_read_word_data(client, page, 0xff, PMBUS_FAN_COMMAND_1); + command = max31785_pmbus_read_word_data(client, driver_data, page, 0xff, + PMBUS_FAN_COMMAND_1); if (command < 0) return command; @@ -129,6 +212,8 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page) static int max31785_read_word_data(struct i2c_client *client, int page, int phase, int reg) { + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); + struct max31785_data *driver_data = to_max31785_data(info); u32 val; int rv; @@ -157,7 +242,7 @@ static int max31785_read_word_data(struct i2c_client *client, int page, rv = max31785_get_pwm(client, page); break; case PMBUS_VIRT_PWM_ENABLE_1: - rv = max31785_get_pwm_mode(client, page); + rv = max31785_get_pwm_mode(client, driver_data, page); break; default: rv = -ENODATA; @@ -188,8 +273,36 @@ static inline u32 max31785_scale_pwm(u32 sensor_val) return (sensor_val * 100) / 255; } -static int max31785_pwm_enable(struct i2c_client *client, int page, - u16 word) +static int max31785_update_fan(struct i2c_client *client, + struct max31785_data *driver_data, int page, + u8 config, u8 mask, u16 command) +{ + int from, rv; + u8 to; + + from = max31785_pmbus_read_byte_data(client, driver_data, page, + PMBUS_FAN_CONFIG_12); + if (from < 0) + return from; + + to = (from & ~mask) | (config & mask); + + if (to != from) { + rv = max31785_pmbus_write_byte_data(client, driver_data, page, + PMBUS_FAN_CONFIG_12, to); + if (rv < 0) + return rv; + } + + rv = max31785_pmbus_write_word_data(client, driver_data, page, + PMBUS_FAN_COMMAND_1, command); + + return rv; +} + +static int max31785_pwm_enable(struct i2c_client *client, + struct max31785_data *driver_data, int page, + u16 word) { int config = 0; int rate; @@ -217,18 +330,23 @@ static int max31785_pwm_enable(struct i2c_client *client, int page, return -EINVAL; } - return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate); + return max31785_update_fan(client, driver_data, page, config, + PB_FAN_1_RPM, rate); } static int max31785_write_word_data(struct i2c_client *client, int page, int reg, u16 word) { + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); + struct max31785_data *driver_data = to_max31785_data(info); + switch (reg) { case PMBUS_VIRT_PWM_1: - return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM, - max31785_scale_pwm(word)); + return max31785_update_fan(client, driver_data, page, 0, + PB_FAN_1_RPM, + max31785_scale_pwm(word)); case PMBUS_VIRT_PWM_ENABLE_1: - return max31785_pwm_enable(client, page, word); + return max31785_pwm_enable(client, driver_data, page, word); default: break; } @@ -303,13 +421,16 @@ static int max31785_configure_dual_tach(struct i2c_client *client, { int ret; int i; + struct max31785_data *driver_data = to_max31785_data(info); for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); + ret = max31785_i2c_smbus_write_byte_data(client, driver_data, + PMBUS_PAGE, i); if (ret < 0) return ret; - ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG); + ret = max31785_i2c_smbus_read_word_data(client, driver_data, + MFR_FAN_CONFIG); if (ret < 0) return ret; @@ -329,6 +450,7 @@ static int max31785_probe(struct i2c_client *client) { struct device *dev = &client->dev; struct pmbus_driver_info *info; + struct max31785_data *driver_data; bool dual_tach = false; int ret; @@ -337,13 +459,16 @@ static int max31785_probe(struct i2c_client *client) I2C_FUNC_SMBUS_WORD_DATA)) return -ENODEV; - info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL); - if (!info) + driver_data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL); + if (!driver_data) return -ENOMEM; + info = &driver_data->info; + driver_data->access = ktime_get(); *info = max31785_info; - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); + ret = max31785_i2c_smbus_write_byte_data(client,driver_data, + PMBUS_PAGE, 255); if (ret < 0) return ret;
The MAX31785 has shown erratic behaviour across multiple system designs, unexpectedly clock stretching and NAKing transactions. Experimentation shows that this seems to be triggered by a register access directly back to back with a previous register write. Experimentation also shows that inserting a small delay after register writes makes the issue go away. Use a similar solution to what the max15301 driver does to solve the same problem. Create a custom set of bus read and write functions that make sure that the delay is added. Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com> --- drivers/hwmon/pmbus/max31785.c | 167 ++++++++++++++++++++++++++++----- 1 file changed, 146 insertions(+), 21 deletions(-)