Hello,
While working with system designs making use of TI's UCD90320 Power
Sequencer we've found that communication with the device isn't terribly
reliable.
It appears that back-to-back transfers where commands addressed to the
device are put onto the bus with intervals between STOP and START in the
neighbourhood of 250us or less can cause bad behaviour. This primarily
happens during driver probe while scanning the device to determine its
capabilities.
We have observed the device causing excessive clock stretches and bus
lockups, and also corruption of the device's volatile state (requiring it
to be reset). The latter is particularly disruptive in that the controlled
rails are brought down either by:
1. The corruption causing a fault condition, or
2. Asserting the device's reset line to recover
A further observation is that pacing transfers to the device appears to
mitigate the bad behaviour. We're in discussion with TI to better
understand the limitations and at least get the behaviour documented.
This short series implements the mitigation in terms of a throttle in the
i2c_client associated with the device's driver. Before the first
communication with the device in the probe() of ucd9000 we configure the
i2c_client to throttle transfers with a minimum of a 1ms delay (with the
delay exposed as a module parameter).
The series is RFC for several reasons:
The first is to sus out feelings on the general direction. The problem is
pretty unfortunate - are there better ways to implement the mitigation?
If there aren't, then:
I'd like thoughts on whether we want to account for i2c-dev clients.
Implementing throttling in i2c_client feels like a solution-by-proxy as the
throttling is really a property of the targeted device, but we don't have a
coherent representation between platform devices and devices associated
with i2c-dev clients. At the moment we'd have to resort to address-based
lookups for platform data stashed in the transfer functions.
Next is that I've only implemented throttling for SMBus devices. I don't
yet have a use-case for throttling non-SMBus devices so I'm not sure it's
worth poking at it, but would appreciate thoughts there.
Further, I've had a bit of a stab at dealing with atomic transfers that's
not been tested. Hopefully it makes sense.
Finally I'm also interested in feedback on exposing the control in a little
more general manner than having to implement a module parameter in all
drivers that want to take advantage of throttling. This isn't a big problem
at the moment, but if anyone has thoughts there then I'm happy to poke at
those too.
Please review!
Andrew
Andrew Jeffery (2):
i2c: smbus: Allow throttling of transfers to client devices
hwmon: (pmbus/ucd9000) Throttle SMBus transfers to avoid poor
behaviour
drivers/hwmon/pmbus/ucd9000.c | 6 ++
drivers/i2c/i2c-core-base.c | 8 +-
drivers/i2c/i2c-core-smbus.c | 149 +++++++++++++++++++++++++++-------
drivers/i2c/i2c-core.h | 22 +++++
include/linux/i2c.h | 3 +
5 files changed, 157 insertions(+), 31 deletions(-)
--
2.25.1
Short turn-around times between transfers to e.g. the UCD90320 can lead
to problematic behaviour, including excessive clock stretching, bus
lockups and potential corruption of the device's volatile state.
Introduce transfer throttling for the device with a minimum access
delay of 1ms.
Signed-off-by: Andrew Jeffery <[email protected]>
---
drivers/hwmon/pmbus/ucd9000.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index 81f4c4f166cd..a0b97d035326 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -9,6 +9,7 @@
#include <linux/debugfs.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/moduleparam.h>
#include <linux/of_device.h>
#include <linux/init.h>
#include <linux/err.h>
@@ -18,6 +19,9 @@
#include <linux/gpio/driver.h>
#include "pmbus.h"
+static unsigned long smbus_delay_us = 1000;
+module_param(smbus_delay_us, ulong, 0664);
+
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd90320, ucd9090,
ucd90910 };
@@ -502,6 +506,8 @@ static int ucd9000_probe(struct i2c_client *client,
I2C_FUNC_SMBUS_BLOCK_DATA))
return -ENODEV;
+ i2c_smbus_throttle_client(client, smbus_delay_us);
+
ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID,
block_buffer);
if (ret < 0) {
--
2.25.1