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 <[email protected]>
---
V3 -> V4: Fixed warnings realted to this commit
V2 -> V3: Fixed the commit message
V1 -> V2:
- Changed the max31785_wait macro to a function, following the conventions
used in other drivers that had the same issue.
- Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
from max31785_pmbus_* to _max31785_*, making them more concise.
drivers/hwmon/pmbus/max31785.c | 187 +++++++++++++++++++++++++++++----
1 file changed, 166 insertions(+), 21 deletions(-)
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index f9aa576495a5..cc9acd7bac37 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,118 @@ 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.
+ */
+static inline void max31785_wait(const struct max31785_data *data)
{
- if (page < MAX31785_NR_PAGES)
- return -ENODATA;
+ s64 delta = ktime_us_delta(ktime_get(), data->access);
+
+ if (delta < MAX31785_WAIT_DELAY_US)
+ udelay(MAX31785_WAIT_DELAY_US - delta);
+}
+
+static int max31785_i2c_write_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int command, u16 data)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = i2c_smbus_write_byte_data(client, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int max31785_i2c_read_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int command)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = i2c_smbus_read_word_data(client, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_read_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = pmbus_read_byte_data(client, page, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_write_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command, u16 data)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = pmbus_write_byte_data(client, page, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_read_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int phase, int command)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = pmbus_read_word_data(client, page, phase, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_write_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command, u16 data)
+{
+ int rc;
+
+ max31785_wait(driver_data);
+ rc = pmbus_write_word_data(client, page, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+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_read_byte_data(client, driver_data,
+ page - MAX31785_NR_PAGES,
+ reg);
}
return -ENODATA;
@@ -102,16 +202,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_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_read_word_data(client, driver_data, page, 0xff,
+ PMBUS_FAN_COMMAND_1);
if (command < 0)
return command;
@@ -129,6 +232,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 +262,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 +293,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_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_write_byte_data(client, driver_data, page,
+ PMBUS_FAN_CONFIG_12, to);
+ if (rv < 0)
+ return rv;
+ }
+
+ rv = _max31785_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 +350,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 +441,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_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_read_word_data(client, driver_data,
+ MFR_FAN_CONFIG);
if (ret < 0)
return ret;
@@ -329,6 +470,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 +479,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_write_byte_data(client, driver_data,
+ PMBUS_PAGE, 255);
if (ret < 0)
return ret;
--
2.39.2
On 10/26/23 15:17, 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 <[email protected]>
> ---
> V3 -> V4: Fixed warnings realted to this commit
I also asked about the use of udelay() instead of usleep_range() or fsleep().
I see you did not change the code. Fine, but please explain why the use of
udelay() instead of the alternative is desirable or needed here, and don't
just ignore review feedback.
Thanks,
Guenter
On 10/26/23, 6:02 PM, "Guenter Roeck" <[email protected] <mailto:[email protected]> on behalf of [email protected] <mailto:[email protected]>> wrote:
On 10/26/23 15:17, 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 <[email protected] <mailto:[email protected]>>
> ---
> V3 -> V4: Fixed warnings realted to this commit
>I also asked about the use of udelay() instead of usleep_range() or fsleep().
>I see you did not change the code. Fine, but please explain why the use of
>udelay() instead of the alternative is desirable or needed here, and don't
>just ignore review feedback.
Sorry, I missed the comment, I will make changes and send it again.
Thanks,
Lakshmi
>Thanks,
>Guenter