mbox series

[linux,dev-5.8,0/3] i2c: Introduce transfer throttling

Message ID 20200902071736.2578715-1-andrew@aj.id.au
Headers show
Series i2c: Introduce transfer throttling | expand

Message

Andrew Jeffery Sept. 2, 2020, 7:17 a.m. UTC 
Hello,

It turns out that some I2C/SMBus devices get unhappy if we "hammer" them with
commands. A concrete example is the TI UCD90320 power sequencer, and I strongly
suspect the Maxim MAX31785 fan controller also suffers this issue. In the case
of the UCD90320 we see behaviours ranging across 35ms clock stretches (SMBus
limit), bus lockups where SDA, SCL or both become stuck low (must assert the
device RESET line to recover) or corruption of the volatile state of the device
(which can bring down rails).

Further, by "hammer" I don't even mean anything that might be coming close to
the SMBus tBUF requirements - the UCD90320 in particular starts getting upset
if we issue commands back-to-back with around 250us of delay between commands
addressing the device (for contrast the 100kHz tBUF requirement of SMBus is no
less than 4.7us idle time between STOP and START).

This series implements per-client throttling in the I2C/SMBus core as a proxy
for per-device throttling, and then patches the ucd9000 device driver to
throttle commands such that at least 1000ms passes between consecutive commands
to the target device.

The implementation augments `struct i2c_client` to track the time of the last
command to the associated device. While the "proxy" nature isn't ideal, the
implementation at least gets us past a hump of system instability.

So, TODO:

1. Remove the "proxy" nature by eliminating the schism between the client
   associated with a device's driver and accesses via /dev/i2c-*
2. Consider whether this feature should be expanded beyond the SMBus-specific
   parts of the i2c subsystem.

Nice-to-haves:

3. Implement devicetree support
4. Attach the debugfs attribute to where-ever is appropriate as after resolving
   1.

After I've thought a bit more about 1. I plan to send the patches upstream. I
think at that point I can take feedback from Wolfram on 2, but for the moment
I'd like this series to bake in the OpenBMC kernel tree.

The third patch is a bit WIP-y; it was mainly to perform experiments with the
UCD90320 in particular. If the attribute is to be exposed it probably should be
a property of either the client or the associated device once we solve 1 (as
mentioned in 4.) Anyway, it's at least in debugfs, so I don't think it matters
a great deal if we take it as there's no promise of ABI stability there.

The patches have been tested on several Rainier systems and the desired bus
behaviour confirmed with a logic analyser.

Please review!

Andrew

Andrew Jeffery (3):
  i2c: Allow throttling of transfers to client devices
  pmbus: (ucd9000) Throttle SMBus transfers to avoid poor behaviour
  ucd9000: Add a throttle delay attribute in debugfs

 drivers/hwmon/pmbus/ucd9000.c |  37 ++++++++
 drivers/i2c/i2c-core-base.c   |   8 +-
 drivers/i2c/i2c-core-smbus.c  | 169 ++++++++++++++++++++++++++++------
 drivers/i2c/i2c-core.h        |  21 +++++
 include/linux/i2c.h           |   5 +
 5 files changed, 209 insertions(+), 31 deletions(-)

Comments

Andrew Jeffery Sept. 2, 2020, 7:26 a.m. UTC | #1 
On Wed, 2 Sep 2020, at 16:47, Andrew Jeffery wrote:
> This series implements per-client throttling in the I2C/SMBus core as a proxy
> for per-device throttling, and then patches the ucd9000 device driver to
> throttle commands such that at least 1000ms passes between consecutive commands
> to the target device.

Ugh, typo. To clarify, the delay is 1000us (1ms), not 1000ms.