On Mon, 14 Sep 2020, at 23:44, Guenter Roeck wrote:
> On 9/14/20 5:28 AM, Andrew Jeffery wrote:
> > 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.
> >
>
> Some Zilker labs devices have the same problem, though not as bad
> to need a 1ms delay. See zl6100.c. Various LTS devices have a similar
> problem, but there it is possible to poll the device until it is ready.
> See ltc2978.c.
>
> > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> > 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;
>
> Is that to be on the super-safe side ? Patch 0 talks about needing 250 uS.
>
> > +module_param(smbus_delay_us, ulong, 0664);
> > +
>
> I would not want to have this in user control, and it should not affect devices
> not known to be affected.
Can you clarify what you mean here? Initially I interpreted your statement as
meaning "Don't impose delays on the UCD90160 when the issues have only been
demonstrated with the UCD90320". But I've since looked at zl6100.c and its
delay is also exposed as a module parameter, which makes me wonder whether it
was unclear that smbus_delay_us here is specific to the driver's i2c_client and
is not a delay imposed on all SMBus accesses from the associated master. That
is, with the implementation I've posted here, other (non-UCD9000) devices on
the same bus are _not_ impacted by this value.
> I would suggest an implementation similar to other
> affected devices; again, see zl6100.c or ltc2978.c for examples.
I've had a look at these two examples. As you suggest the delays in zl6100.c
look pretty similar to what this series implements in the i2c core. I'm finding
it hard to dislodge the feeling that open-coding the waits is error prone, but
to avoid that and not implement the waits in the i2c core means having almost
duplicate implementations of handlers for i2c_smbus_{read,write}*() and
pmbus_{read,write}*() calls in the driver.
Andrew
