Re: [PATCH linux dev-5.10 33/35] pmbus: (core) Add a one-shot retry in pmbus_set_page()

[Joel Stanley <joel@jms.id.au>]

On Mon, 8 Mar 2021 at 22:56, Eddie James <eajames@linux.ibm.com> wrote:
>
> From: Andrew Jeffery <andrew@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@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Andrew, please review the pmbus related changes and let me know how
you would like to proceed.

> ---
>  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;
> --
> 2.27.0
>