| Message ID | 20230312160312.2227405-1-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (ucd90320) Add minimum delay between bus accesses | expand |
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@metafoo.de> 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);
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);
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@metafoo.de> --- drivers/hwmon/pmbus/ucd9000.c | 75 +++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+)