Re: [PATCH v3 0/4] P2040/P2041 i2c recovery erratum

[Chris Packham <Chris.Packham@alliedtelesis.co.nz>]

On 12/05/21 10:10 am, Joakim Tjernlund wrote:
> On Wed, 2021-05-12 at 09:20 +1200, Chris Packham wrote:
>> The P2040/P2041 has an erratum where the i2c recovery scheme
>> documented in the reference manual (and currently implemented
>> in the i2c-mpc.c driver) does not work. The errata document
>> provides an alternative that does work. This series implements
>> that alternative and uses a property in the devicetree to
>> decide when the alternative mechanism is needed.
>>
>> Chris Packham (4):
>>    dt-bindings: i2c: mpc: Add fsl,i2c-erratum-a004447 flag
>>    powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P2041 i2c
>>      controllers
>>    powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P1010 i2c
>>      controllers
>>    i2c: mpc: implement erratum A-004447 workaround
>>
>>   .../devicetree/bindings/i2c/i2c-mpc.yaml      |  7 ++
>>   arch/powerpc/boot/dts/fsl/p1010si-post.dtsi   |  8 ++
>>   arch/powerpc/boot/dts/fsl/p2041si-post.dtsi   | 16 ++++
>>   drivers/i2c/busses/i2c-mpc.c                  | 81 ++++++++++++++++++-
>>   4 files changed, 110 insertions(+), 2 deletions(-)
>>
> This now reminds me about the current I2C reset procedure, it didn't work for us and I came up with this one:
>    https://www.spinics.net/lists/linux-i2c/msg29490.html
> it never got in but we are still using it.

For those reading along the v2 mentioned in that thread was posted as 
https://lore.kernel.org/linux-i2c/20170511122033.22471-1-joakim.tjernlund@infinera.com/ 
there was a bit of discussion but it seemed to die out without reaching 
a conclusion.

The i2c-mpc driver is now using the generic recovery mechanism so that 
addresses one bit of feedback from the original thread.

I do wonder if the reason the recovery wasn't working for your case was 
because of the erratum. Do you happen to remember which SoC your issue 
was on?

I've been doing my recent work with a P2040 and prior to that I did test 
out the recovery on a T2081 (which isn't documented to have this 
erratum) when I was re-working the driver. The "new" recovery actually 
seems better but I don't have a reliably faulty i2c device so that's 
only based on me writing some code to manually trigger the recovery 
(using the snippet below) and observing it with an oscilloscope.

---8<---
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c85d6618723f..8fa4363414bc 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1217,6 +1217,25 @@ new_device_store(struct device *dev, struct 
device_attribute *attr,
  }
  static DEVICE_ATTR_WO(new_device);

+static ssize_t recover_store(struct device *dev, struct 
device_attribute *attr,
+                const char *buf, size_t count)
+{
+       struct i2c_adapter *adap = to_i2c_adapter(dev);
+       int ret;
+
+       dev_info(dev, "Recovering bus\n");
+
+       ret = i2c_recover_bus(adap);
+       if (ret == -EOPNOTSUPP)
+               dev_info(dev, "recovery not supported\n");
+       else if (ret)
+               dev_err(dev, "recovery failed %d\n", ret);
+
+       return count;
+}
+
+static DEVICE_ATTR_WO(recover);
+
  /*
   * And of course let the users delete the devices they instantiated, if
   * they got it wrong. This interface can only be used to delete devices
@@ -1277,6 +1296,7 @@ static struct attribute *i2c_adapter_attrs[] = {
         &dev_attr_name.attr,
         &dev_attr_new_device.attr,
         &dev_attr_delete_device.attr,
+       &dev_attr_recover.attr,
         NULL
  };
  ATTRIBUTE_GROUPS(i2c_adapter);
---8<---