[PATCH linux dev-5.10 33/35] pmbus: (core) Add a one-shot retry in pmbus_set_page()
Andrei Kartashev
a.kartashev at yadro.com
Wed Mar 10 07:21:23 AEDT 2021
Hi,
I have a similar patch in our local tree, but it adds retry in more
places. I had to add retries for all i2c_smbus_* operations because pf
communication to PSUs sometime very unstable.
Here is it:
>From 7688b90c3e7e4986535a194a271509095534c3e7 Mon Sep 17 00:00:00 2001
From: Andrei Kartashev <a.kartashev at yadro.com>
Date: Tue, 9 Mar 2021 21:47:25 +0300
Subject: [PATCH] hwmon: (pmbus) Retry I2C request on failure
In real world I2C communication errors are possible. It was discovered
that pmbus read operation for some PSUs occasionally fails in random
places. For pmbus_set_page call there is already retry implemented, but
seems it is not enough.
Add retries for every i2c_smbus_read/i2c_smbus_write call to increase
robust.
Signed-off-by: Andrei Kartashev <a.kartashev at yadro.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 65 +++++++++++++++++++-------------
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 60ea917936a7..d98b52950022 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -27,6 +27,7 @@
*/
#define PMBUS_ATTR_ALLOC_SIZE 32
#define PMBUS_NAME_SIZE 24
+#define I2C_RETRIES 3
struct pmbus_sensor {
struct pmbus_sensor *next;
@@ -144,7 +145,7 @@ EXPORT_SYMBOL_GPL(pmbus_clear_cache);
int pmbus_set_page(struct i2c_client *client, int page, int phase)
{
struct pmbus_data *data = i2c_get_clientdata(client);
- int rv;
+ int rv, rtr;
if (page < 0)
return 0;
@@ -154,18 +155,12 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
dev_dbg(&client->dev, "Want page %u, %u cached\n", page,
data->currpage);
- rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
- if (rv < 0) {
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
page);
- dev_dbg(&client->dev,
- "Failed to set page %u, performed one-shot retry %s: %d\n",
- page, rv ? "and failed" : "with success", rv);
- if (rv < 0)
- return rv;
- }
- rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
if (rv < 0)
return rv;
@@ -176,8 +171,9 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
if (data->info->phases[page] && data->currphase != phase &&
!(data->info->func[page] & PMBUS_PHASE_VIRTUAL)) {
- rv = i2c_smbus_write_byte_data(client, PMBUS_PHASE,
- phase);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_write_byte_data(client, PMBUS_PHASE,
+ phase);
if (rv)
return rv;
}
@@ -189,13 +185,15 @@ EXPORT_SYMBOL_GPL(pmbus_set_page);
int pmbus_write_byte(struct i2c_client *client, int page, u8 value)
{
- int rv;
+ int rv, rtr;
rv = pmbus_set_page(client, page, 0xff);
if (rv < 0)
return rv;
- return i2c_smbus_write_byte(client, value);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_write_byte(client, value);
+ return rv;
}
EXPORT_SYMBOL_GPL(pmbus_write_byte);
@@ -220,13 +218,15 @@ static int _pmbus_write_byte(struct i2c_client *client, int page, u8 value)
int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
u16 word)
{
- int rv;
+ int rv, rtr;
rv = pmbus_set_page(client, page, 0xff);
if (rv < 0)
return rv;
- return i2c_smbus_write_word_data(client, reg, word);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_write_word_data(client, reg, word);
+ return rv;
}
EXPORT_SYMBOL_GPL(pmbus_write_word_data);
@@ -302,13 +302,15 @@ EXPORT_SYMBOL_GPL(pmbus_update_fan);
int pmbus_read_word_data(struct i2c_client *client, int page, int phase, u8 reg)
{
- int rv;
+ int rv, rtr;
rv = pmbus_set_page(client, page, phase);
if (rv < 0)
return rv;
- return i2c_smbus_read_word_data(client, reg);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_read_word_data(client, reg);
+ return rv;
}
EXPORT_SYMBOL_GPL(pmbus_read_word_data);
@@ -361,25 +363,29 @@ static int __pmbus_read_word_data(struct i2c_client *client, int page, int reg)
int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
{
- int rv;
+ int rv, rtr;
rv = pmbus_set_page(client, page, 0xff);
if (rv < 0)
return rv;
- return i2c_smbus_read_byte_data(client, reg);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_read_byte_data(client, reg);
+ return rv;
}
EXPORT_SYMBOL_GPL(pmbus_read_byte_data);
int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
{
- int rv;
+ int rv, rtr;
rv = pmbus_set_page(client, page, 0xff);
if (rv < 0)
return rv;
- return i2c_smbus_write_byte_data(client, reg, value);
+ for (rtr = 0, rv = -1; (rtr < I2C_RETRIES) && (rv < 0); rtr++)
+ rv = i2c_smbus_write_byte_data(client, reg, value);
+ return rv;
}
EXPORT_SYMBOL_GPL(pmbus_write_byte_data);
@@ -2193,7 +2199,7 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
struct pmbus_driver_info *info)
{
struct device *dev = &client->dev;
- int page, ret;
+ int page, ret, rtr;
/*
* Some PMBus chips don't support PMBUS_STATUS_WORD, so try
@@ -2201,10 +2207,13 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
* Bail out if both registers are not supported.
*/
data->read_status = pmbus_read_status_word;
- ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
+ for (rtr = 0, ret = -1; (rtr < I2C_RETRIES) && (ret < 0); rtr++)
+ ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
if (ret < 0 || ret == 0xffff) {
data->read_status = pmbus_read_status_byte;
- ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
+ for (rtr = 0, ret = -1; (rtr < I2C_RETRIES) && (ret < 0); rtr++)
+ ret = i2c_smbus_read_byte_data(client,
+ PMBUS_STATUS_BYTE);
if (ret < 0 || ret == 0xff) {
dev_err(dev, "PMBus status register not found\n");
return -ENODEV;
@@ -2214,7 +2223,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
}
/* Enable PEC if the controller supports it */
- ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
+ for (rtr = 0, ret = -1; (rtr < I2C_RETRIES) && (ret < 0); rtr++)
+ ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
client->flags |= I2C_CLIENT_PEC;
@@ -2223,7 +2233,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
* faults, and we should not try it. Also, in that case, writes into
* limit registers need to be disabled.
*/
- ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
+ for (rtr = 0, ret = -1; (rtr < I2C_RETRIES) && (ret < 0); rtr++)
+ ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
if (ret > 0 && (ret & PB_WP_ANY))
data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
--
2.26.2
On Mon, 2021-03-08 at 16:54 -0600, Eddie James wrote:
> From: Andrew Jeffery <andrew at aj.id.au>
>
> From extensive testing and tracing it was discovered that the
> MAX31785
> occasionally fails to switch pages despite ACK'ing the PAGE PMBus
> data
> write. I suspect this behaviour had been seen on other devices as
> well,
> as pmbus_set_page() already read-back the freshly set value and
> errored
> out if it wasn't what we requested.
>
> In the case of the MAX31785 it was shown that a one-shot retry was
> enough to get the PAGE write to stick if the inital command failed.
> To
> improve robustness, only error out if the one-shot retry also fails
> to
> stick.
>
> OpenBMC-Staging-Count: 1
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> Signed-off-by: Joel Stanley <joel at jms.id.au>
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 31 ++++++++++++++++++++--------
> ---
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c
> b/drivers/hwmon/pmbus/pmbus_core.c
> index 44c1a0a07509..dd4a09d18730 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -151,25 +151,34 @@ int pmbus_set_page(struct i2c_client *client,
> int page, int phase)
>
> if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL) &&
> data->info->pages > 1 && page != data->currpage) {
> + int i;
> +
> dev_dbg(&client->dev, "Want page %u, %u cached\n",
> page,
> data->currpage);
>
> - rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
> page);
> - if (rv < 0) {
> + for (i = 0; i < 2; i++) {
> rv = i2c_smbus_write_byte_data(client,
> PMBUS_PAGE,
> page);
> - dev_dbg(&client->dev,
> - "Failed to set page %u, performed one-
> shot retry %s: %d\n",
> - page, rv ? "and failed" : "with
> success", rv);
> + if (rv)
> + continue;
> +
> + rv = i2c_smbus_read_byte_data(client,
> PMBUS_PAGE);
> if (rv < 0)
> - return rv;
> - }
> + continue;
>
> - rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> - if (rv < 0)
> - return rv;
> + /* Success, exit loop */
> + if (rv == page)
> + break;
> +
> + rv = i2c_smbus_read_byte_data(client,
> PMBUS_STATUS_CML);
> + if (rv < 0)
> + continue;
> +
> + if (rv & PB_CML_FAULT_INVALID_DATA)
> + return -EIO;
> + }
>
> - if (rv != page)
> + if (i == 2)
> return -EIO;
> }
> data->currpage = page;
--
Best regards,
Andrei Kartashev
More information about the openbmc
mailing list