| Message ID | 20210511212052.27242-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | P2040/P2041 i2c recovery erratum | expand |
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. Jocke
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<---
On Wed, 2021-05-12 at 01:48 +0000, Chris Packham wrote: > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-i2c%2Fmsg29490.html&data=04%7C01%7CJoakim.Tjernlund%40infinera.com%7Cb85a6e9c3c8b469572da08d914e816b5%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637563809322419998%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4cwujNmAVlBa08Tt79hLYGJfJtn7wdz1Kgz0eW2VX9U%3D&reserved=0 > > it never got in but we are still using it. > > For those reading along the v2 mentioned in that thread was posted as > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-i2c%2F20170511122033.22471-1-joakim.tjernlund%40infinera.com%2F&data=04%7C01%7CJoakim.Tjernlund%40infinera.com%7Cb85a6e9c3c8b469572da08d914e816b5%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637563809322419998%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YDTX5L6J2ocHep5XutVN46jUpvJj7h1aDbHHwMqlrAs%3D&reserved=0 > 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? It could only be P2010 or MPC8321, I think it was MPC8321, you could try my solution on your CPU if you want to make sure. > > 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. You don't need a faulty device, just an aborted I2C read/write op. You could force one such I2C op. by py pulling down the clock/SDA in the middle of a byte transfer. Jocke
> > 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. > > You don't need a faulty device, just an aborted I2C read/write op. If you can wire GPIOs to the bus, you can use the I2C fault injector: Documentation/i2c/gpio-fault-injection.rst There are already two "incomplete transfer" injectors.
On 13/05/21 3:01 am, wsa@kernel.org wrote: >>> 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. >> You don't need a faulty device, just an aborted I2C read/write op. > If you can wire GPIOs to the bus, you can use the I2C fault injector: > > Documentation/i2c/gpio-fault-injection.rst > > There are already two "incomplete transfer" injectors. > Just giving this thread a poke. I have been looking at my options for triggering an i2c recovery but haven't really had time to do much. I think the best option given what I've got access to is a modified SFP that grounds the SDA line but I need to find a system where I can attach an oscilloscope (should be a few of these in the office when I can get on-site). I can confirm that when manually triggered the existing recovery and the new erratum workaround produce what I'd expect to observe on an oscilloscope. I haven't explored Joakim's alternative recovery but I don't think that should hold up these changes, any improvement to the existing recovery can be done later as a follow-up.
> 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. Yes, and the generic recovery has been improved since then. There is an "incomplete_write_byte" fault injector now and the generic recovery handles it correctly meanwhile. Before, it actually could cause a write to happen but we are sending STOPs now.
On Wed, May 12, 2021 at 09:20:48AM +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. The series looks good to me. Usually, I don't take DTS patches. This time I'd make an exception and apply all patches to for-current because this is clearly a bugfix. For that, I'd need an ack from PPC maintainers. Could I have those for patches 2+3?
Wolfram Sang <wsa@kernel.org> writes: > On Wed, May 12, 2021 at 09:20:48AM +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. > > The series looks good to me. Usually, I don't take DTS patches. This > time I'd make an exception and apply all patches to for-current because > this is clearly a bugfix. For that, I'd need an ack from PPC > maintainers. Could I have those for patches 2+3? Yep, done. cheers
On Wed, May 26, 2021 at 11:02:45AM +1000, Michael Ellerman wrote: > Wolfram Sang <wsa@kernel.org> writes: > > On Wed, May 12, 2021 at 09:20:48AM +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. > > > > The series looks good to me. Usually, I don't take DTS patches. This > > time I'd make an exception and apply all patches to for-current because > > this is clearly a bugfix. For that, I'd need an ack from PPC > > maintainers. Could I have those for patches 2+3? > > Yep, done. Thanks! Series applied to for-current.
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(-)