| 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 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?
Chris Packham <chris.packham@alliedtelesis.co.nz> writes: > The i2c controllers on the P2040/P2041 have an erratum where the > documented scheme for i2c bus recovery will not work (A-004447). A > different mechanism is needed which is documented in the P2040 Chip > Errata Rev Q (latest available at the time of writing). > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > arch/powerpc/boot/dts/fsl/p2041si-post.dtsi | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) Acked-by: Michael Ellerman <mpe@ellerman.id.au> cheers > diff --git a/arch/powerpc/boot/dts/fsl/p2041si-post.dtsi b/arch/powerpc/boot/dts/fsl/p2041si-post.dtsi > index 872e4485dc3f..ddc018d42252 100644 > --- a/arch/powerpc/boot/dts/fsl/p2041si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p2041si-post.dtsi > @@ -371,7 +371,23 @@ sdhc@114000 { > }; > > /include/ "qoriq-i2c-0.dtsi" > + i2c@118000 { > + fsl,i2c-erratum-a004447; > + }; > + > + i2c@118100 { > + fsl,i2c-erratum-a004447; > + }; > + > /include/ "qoriq-i2c-1.dtsi" > + i2c@119000 { > + fsl,i2c-erratum-a004447; > + }; > + > + i2c@119100 { > + fsl,i2c-erratum-a004447; > + }; > + > /include/ "qoriq-duart-0.dtsi" > /include/ "qoriq-duart-1.dtsi" > /include/ "qoriq-gpio-0.dtsi" > -- > 2.31.1
Chris Packham <chris.packham@alliedtelesis.co.nz> writes: > The i2c controllers on the P1010 have an erratum where the documented > scheme for i2c bus recovery will not work (A-004447). A different > mechanism is needed which is documented in the P1010 Chip Errata Rev L. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > > Notes: > Changes in v3: > - New > > arch/powerpc/boot/dts/fsl/p1010si-post.dtsi | 8 ++++++++ > 1 file changed, 8 insertions(+) Acked-by: Michael Ellerman <mpe@ellerman.id.au> cheers > diff --git a/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi > index c2717f31925a..ccda0a91abf0 100644 > --- a/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi > @@ -122,7 +122,15 @@ memory-controller@2000 { > }; > > /include/ "pq3-i2c-0.dtsi" > + i2c@3000 { > + fsl,i2c-erratum-a004447; > + }; > + > /include/ "pq3-i2c-1.dtsi" > + i2c@3100 { > + fsl,i2c-erratum-a004447; > + }; > + > /include/ "pq3-duart-0.dtsi" > /include/ "pq3-espi-0.dtsi" > spi0: spi@7000 { > -- > 2.31.1
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(-)