On Sun, Mar 12, 2023 at 09:03:12AM -0700, Lars-Peter Clausen wrote:
> When probing the ucd90320 access to some of the registers randomly fails.
> Sometimes it NACKs a transfer, sometimes it returns just random data and
> the PEC check fails.
>
> 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: Lars-Peter Clausen <lars@xxxxxxxxxx>
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/pmbus/ucd9000.c | 75 +++++++++++++++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 75fc770c9e40..3daaf2237832 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> @@ -16,6 +17,7 @@
> #include <linux/i2c.h>
> #include <linux/pmbus.h>
> #include <linux/gpio/driver.h>
> +#include <linux/timekeeping.h>
> #include "pmbus.h"
>
> enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd90320, ucd9090,
> @@ -65,6 +67,7 @@ struct ucd9000_data {
> struct gpio_chip gpio;
> #endif
> struct dentry *debugfs;
> + ktime_t write_time;
> };
> #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
>
> @@ -73,6 +76,73 @@ struct ucd9000_debugfs_entry {
> u8 index;
> };
>
> +/*
> + * It has been observed that the UCD90320 randomly fails register access when
> + * doing another access right on the back of a register write. To mitigate this
> + * make sure that there is a minimum delay between a write access and the
> + * following access. The 250us is based on experimental data. At a delay of
> + * 200us the issue seems to go away. Add a bit of extra margin to allow for
> + * system to system differences.
> + */
> +#define UCD90320_WAIT_DELAY_US 250
> +
> +static inline void ucd90320_wait(const struct ucd9000_data *data)
> +{
> + s64 delta = ktime_us_delta(ktime_get(), data->write_time);
> +
> + if (delta < UCD90320_WAIT_DELAY_US)
> + udelay(UCD90320_WAIT_DELAY_US - delta);
> +}
> +
> +static int ucd90320_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 ucd9000_data *data = to_ucd9000_data(info);
> +
> + if (reg >= PMBUS_VIRT_BASE)
> + return -ENXIO;
> +
> + ucd90320_wait(data);
> + return pmbus_read_word_data(client, page, phase, reg);
> +}
> +
> +static int ucd90320_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + struct ucd9000_data *data = to_ucd9000_data(info);
> +
> + ucd90320_wait(data);
> + return pmbus_read_byte_data(client, page, reg);
> +}
> +
> +static int ucd90320_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 ucd9000_data *data = to_ucd9000_data(info);
> + int ret;
> +
> + ucd90320_wait(data);
> + ret = pmbus_write_word_data(client, page, reg, word);
> + data->write_time = ktime_get();
> +
> + return ret;
> +}
> +
> +static int ucd90320_write_byte(struct i2c_client *client, int page, u8 value)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + struct ucd9000_data *data = to_ucd9000_data(info);
> + int ret;
> +
> + ucd90320_wait(data);
> + ret = pmbus_write_byte(client, page, value);
> + data->write_time = ktime_get();
> +
> + return ret;
> +}
> +
> static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
> {
> int fan_config = 0;
> @@ -598,6 +668,11 @@ static int ucd9000_probe(struct i2c_client *client)
> info->read_byte_data = ucd9000_read_byte_data;
> info->func[0] |= PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12
> | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
> + } else if (mid->driver_data == ucd90320) {
> + info->read_byte_data = ucd90320_read_byte_data;
> + info->read_word_data = ucd90320_read_word_data;
> + info->write_byte = ucd90320_write_byte;
> + info->write_word_data = ucd90320_write_word_data;
> }
>
> ucd9000_probe_gpio(client, mid, data);
