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

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

[Chris Packham]

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(-)

-- 
2.31.1

[PATCH v3 1/4] dt-bindings: i2c: mpc: Add fsl,i2c-erratum-a004447 flag

[Chris Packham]

Document the fsl,i2c-erratum-a004447 flag which indicates the presence
of an i2c erratum on some QorIQ SoCs.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Acked-by: Rob Herring <robh@kernel.org>
---

Notes:
    Changes in v3:
    - Add Ack from Rob

 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
index 7b553d559c83..98c6fcf7bf26 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
@@ -46,6 +46,13 @@ properties:
     description: |
       I2C bus timeout in microseconds
 
+  fsl,i2c-erratum-a004447:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      Indicates the presence of QorIQ erratum A-004447, which
+      says that the standard i2c recovery scheme mechanism does
+      not work and an alternate implementation is needed.
+
 required:
   - compatible
   - reg
-- 
2.31.1

[PATCH v3 2/4] powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P2041 i2c controllers

[Chris Packham]

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(+)

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

Re: [PATCH v3 2/4] powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P2041 i2c controllers

[Michael Ellerman]

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

[PATCH v3 3/4] powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P1010 i2c controllers

[Chris Packham]

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(+)

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

Re: [PATCH v3 3/4] powerpc/fsl: set fsl,i2c-erratum-a004447 flag for P1010 i2c controllers

[Michael Ellerman]

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

[PATCH v3 4/4] i2c: mpc: implement erratum A-004447 workaround

[Chris Packham]

The P2040/P2041 has an erratum where the normal i2c recovery mechanism
does not work. Implement the alternative recovery mechanism documented
in the P2040 Chip Errata Rev Q.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v3:
    - Further code reduction in i2c_mpc_wait_sr()
    Changes in v2:
    - Use readb_poll_timeout instead of open-coded loop

 drivers/i2c/busses/i2c-mpc.c | 81 +++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 30d9e89a3db2..dcca9c2396db 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -19,6 +19,7 @@
 
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/fsl_devices.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -45,6 +46,7 @@
 #define CCR_MTX  0x10
 #define CCR_TXAK 0x08
 #define CCR_RSTA 0x04
+#define CCR_RSVD 0x02
 
 #define CSR_MCF  0x80
 #define CSR_MAAS 0x40
@@ -97,7 +99,7 @@ struct mpc_i2c {
 	u32 block;
 	int rc;
 	int expect_rxack;
-
+	bool has_errata_A004447;
 };
 
 struct mpc_i2c_divider {
@@ -136,6 +138,75 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 	}
 }
 
+static int i2c_mpc_wait_sr(struct mpc_i2c *i2c, int mask)
+{
+	void __iomem *addr = i2c->base + MPC_I2C_SR;
+	u8 val;
+
+	return readb_poll_timeout(addr, val, val & mask, 0, 100);
+}
+
+/*
+ * Workaround for Erratum A004447. From the P2040CE Rev Q
+ *
+ * 1.  Set up the frequency divider and sampling rate.
+ * 2.  I2CCR - a0h
+ * 3.  Poll for I2CSR[MBB] to get set.
+ * 4.  If I2CSR[MAL] is set (an indication that SDA is stuck low), then go to
+ *     step 5. If MAL is not set, then go to step 13.
+ * 5.  I2CCR - 00h
+ * 6.  I2CCR - 22h
+ * 7.  I2CCR - a2h
+ * 8.  Poll for I2CSR[MBB] to get set.
+ * 9.  Issue read to I2CDR.
+ * 10. Poll for I2CSR[MIF] to be set.
+ * 11. I2CCR - 82h
+ * 12. Workaround complete. Skip the next steps.
+ * 13. Issue read to I2CDR.
+ * 14. Poll for I2CSR[MIF] to be set.
+ * 15. I2CCR - 80h
+ */
+static void mpc_i2c_fixup_A004447(struct mpc_i2c *i2c)
+{
+	int ret;
+	u32 val;
+
+	writeccr(i2c, CCR_MEN | CCR_MSTA);
+	ret = i2c_mpc_wait_sr(i2c, CSR_MBB);
+	if (ret) {
+		dev_err(i2c->dev, "timeout waiting for CSR_MBB\n");
+		return;
+	}
+
+	val = readb(i2c->base + MPC_I2C_SR);
+
+	if (val & CSR_MAL) {
+		writeccr(i2c, 0x00);
+		writeccr(i2c, CCR_MSTA | CCR_RSVD);
+		writeccr(i2c, CCR_MEN | CCR_MSTA | CCR_RSVD);
+		ret = i2c_mpc_wait_sr(i2c, CSR_MBB);
+		if (ret) {
+			dev_err(i2c->dev, "timeout waiting for CSR_MBB\n");
+			return;
+		}
+		val = readb(i2c->base + MPC_I2C_DR);
+		ret = i2c_mpc_wait_sr(i2c, CSR_MIF);
+		if (ret) {
+			dev_err(i2c->dev, "timeout waiting for CSR_MIF\n");
+			return;
+		}
+		writeccr(i2c, CCR_MEN | CCR_RSVD);
+	} else {
+		val = readb(i2c->base + MPC_I2C_DR);
+		ret = i2c_mpc_wait_sr(i2c, CSR_MIF);
+		if (ret) {
+			dev_err(i2c->dev, "timeout waiting for CSR_MIF\n");
+			return;
+		}
+		writeccr(i2c, CCR_MEN);
+	}
+}
+
 #if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
 static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
 	{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -670,7 +741,10 @@ static int fsl_i2c_bus_recovery(struct i2c_adapter *adap)
 {
 	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
 
-	mpc_i2c_fixup(i2c);
+	if (i2c->has_errata_A004447)
+		mpc_i2c_fixup_A004447(i2c);
+	else
+		mpc_i2c_fixup(i2c);
 
 	return 0;
 }
@@ -767,6 +841,9 @@ static int fsl_i2c_probe(struct platform_device *op)
 	}
 	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
 
+	if (of_property_read_bool(op->dev.of_node, "fsl,i2c-erratum-a004447"))
+		i2c->has_errata_A004447 = true;
+
 	i2c->adap = mpc_ops;
 	scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
 		  "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
-- 
2.31.1

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

[Joakim Tjernlund]

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

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

[Chris Packham]

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<---

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

[Joakim Tjernlund]

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

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

[wsa]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]


> > 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Chris Packham]

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.

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

[wsa]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Michael Ellerman]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]