[PATCH v2 0/3] i2c: mv64xxx: Support for I2C unstuck

[PATCH v2 0/3] i2c: mv64xxx: Support for I2C unstuck

[Chris Packham]

Some newer Marvell SoCs natively support a recovery mechanisim. This can be
used as an alternative to the generic pinctrl/GPIO recovery mechanism used on
the older SoCs.

Chris Packham (3):
  dt-bindings: i2c: mv64xxx: update bindings for unstuck register
  arm64: dts: marvell: AC5: use I2C unstuck function
  i2c: mv64xxx: add support for FSM based recovery

 .../bindings/i2c/marvell,mv64xxx-i2c.yaml     |  5 +-
 arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++--
 drivers/i2c/busses/i2c-mv64xxx.c              | 68 ++++++++++++++++++-
 3 files changed, 73 insertions(+), 14 deletions(-)

-- 
2.42.0

[PATCH v2 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register

[Chris Packham]

Some newer Marvell SoCs support an "unstuck" function for I2C bus
recovery. This is an alternative to the generic GPIO based recovery that
the older SoCs use. The unstuck register falls outside of the usual
address block for the I2C controller so requires an additional cell in
the register property. This is optional and does not need to be
supplied.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---

Notes:
    Changes in v2:
    - Collect ack from Conor

 .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml         | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
index 984fc1ed3ec6..461d1c9ee3f7 100644
--- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
@@ -45,7 +45,10 @@ properties:
       auto-detects this and sets it appropriately.
 
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: I2C controller registers
+      - description: I2C unstuck register
 
   interrupts:
     maxItems: 1
-- 
2.42.0

[PATCH v2 2/3] arm64: dts: marvell: AC5: use I2C unstuck function

[Chris Packham]

The AC5 SoC supports using a controller based I2C unstuck function for
recovery. Use this instead of the generic GPIO recovery.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
index c9ce1010c415..e52d3c3496d5 100644
--- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
@@ -137,7 +137,7 @@ mdio: mdio@22004 {
 
 			i2c0: i2c@11000{
 				compatible = "marvell,mv78230-i2c";
-				reg = <0x11000 0x20>;
+				reg = <0x11000 0x20>, <0x110a0 0x4>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
@@ -146,17 +146,14 @@ i2c0: i2c@11000{
 				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
 				clock-frequency=<100000>;
 
-				pinctrl-names = "default", "gpio";
+				pinctrl-names = "default";
 				pinctrl-0 = <&i2c0_pins>;
-				pinctrl-1 = <&i2c0_gpio>;
-				scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
-				sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
 				status = "disabled";
 			};
 
 			i2c1: i2c@11100{
 				compatible = "marvell,mv78230-i2c";
-				reg = <0x11100 0x20>;
+				reg = <0x11100 0x20>, <0x110a4 0x4>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
@@ -165,11 +162,8 @@ i2c1: i2c@11100{
 				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
 				clock-frequency=<100000>;
 
-				pinctrl-names = "default", "gpio";
+				pinctrl-names = "default";
 				pinctrl-0 = <&i2c1_pins>;
-				pinctrl-1 = <&i2c1_gpio>;
-				scl-gpios = <&gpio0 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
-				sda-gpios = <&gpio0 21 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
 				status = "disabled";
 			};
 
-- 
2.42.0

[PATCH v2 3/3] i2c: mv64xxx: add support for FSM based recovery

[Chris Packham]

Some newer Marvell SoCs (AC5 and CN9130, possibly more) support a I2C
unstuck function. This provides a recovery function as part of the FSM
as an alternative to changing pinctrl modes and using the generic GPIO
based recovery. Allow for using this by adding an optional resource to
the platform data which contains the address of the I2C unstuck register
for the I2C controller.

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

Notes:
    Changes in v2:
    - shorted delay and timeout used with read_poll_timeout_atomic()
    - make use dev_dbg() for added messages
    - remove reference to "marvell,armada-8k-i2c"
    - I've elected to keep the behaviour that failing to ioremap the unstuck
      register (if supplied) is an error

 drivers/i2c/busses/i2c-mv64xxx.c | 68 ++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index fd8403b07fa6..efd28bbecf61 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -21,6 +21,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
@@ -82,6 +83,13 @@
 /* Bridge Status values */
 #define	MV64XXX_I2C_BRIDGE_STATUS_ERROR			BIT(0)
 
+/* Unstuck Register values */
+#define MV64XXX_I2C_UNSTUCK_TRIGGER			BIT(0)
+#define MV64XXX_I2C_UNSTUCK_ON_GOING			BIT(1)
+#define MV64XXX_I2C_UNSTUCK_ERROR			BIT(2)
+#define MV64XXX_I2C_UNSTUCK_COUNT(val)			((val & 0xf0) >> 4)
+#define MV64XXX_I2C_UNSTUCK_INPROGRESS (MV64XXX_I2C_UNSTUCK_TRIGGER|MV64XXX_I2C_UNSTUCK_ON_GOING)
+
 /* Driver states */
 enum {
 	MV64XXX_I2C_STATE_INVALID,
@@ -126,6 +134,7 @@ struct mv64xxx_i2c_data {
 	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
+	void __iomem		*unstuck_reg;
 	struct mv64xxx_i2c_regs	reg_offsets;
 	u32			addr1;
 	u32			addr2;
@@ -735,6 +744,33 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
 	return false;
 }
 
+static int
+mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
+{
+	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
+	int ret;
+	u32 val;
+
+	dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
+	writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
+	ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
+					!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
+					10, 1000);
+	if (ret) {
+		dev_err(&adap->dev, "recovery timeout\n");
+		return ret;
+	}
+
+	if (val & MV64XXX_I2C_UNSTUCK_ERROR) {
+		dev_err(&adap->dev, "recovery failed\n");
+		return -EBUSY;
+	}
+
+	dev_dbg(&adap->dev, "recovery complete after %d pulses\n", MV64XXX_I2C_UNSTUCK_COUNT(val));
+
+	return 0;
+}
+
 /*
  *****************************************************************************
  *
@@ -936,8 +972,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 }
 #endif /* CONFIG_OF */
 
-static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
-					  struct device *dev)
+static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,
+					      struct device *dev)
+{
+	struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
+
+	dev_dbg(dev, "using FSM for recovery\n");
+	rinfo->recover_bus = mv64xxx_i2c_recover_bus;
+	drv_data->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+
+}
+
+static int mv64xxx_i2c_init_gpio_recovery_info(struct mv64xxx_i2c_data *drv_data,
+					       struct device *dev)
 {
 	struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
 
@@ -986,6 +1035,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 {
 	struct mv64xxx_i2c_data		*drv_data;
 	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
+	struct resource *res;
 	int	rc;
 
 	if ((!pdata && !pd->dev.of_node))
@@ -1000,6 +1050,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	if (IS_ERR(drv_data->reg_base))
 		return PTR_ERR(drv_data->reg_base);
 
+	/* optional unstuck support */
+	res = platform_get_resource(pd, IORESOURCE_MEM, 1);
+	if (res) {
+		drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
+		if (IS_ERR(drv_data->unstuck_reg))
+			return PTR_ERR(drv_data->unstuck_reg);
+	}
+
 	strscpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		sizeof(drv_data->adapter.name));
 
@@ -1037,7 +1095,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 			return rc;
 	}
 
-	rc = mv64xxx_i2c_init_recovery_info(drv_data, &pd->dev);
+	if (drv_data->unstuck_reg)
+		rc = mv64xxx_i2c_init_fsm_recovery_info(drv_data, &pd->dev);
+	else
+		rc = mv64xxx_i2c_init_gpio_recovery_info(drv_data, &pd->dev);
+
 	if (rc == -EPROBE_DEFER)
 		return rc;
 
-- 
2.42.0

Re: [PATCH v2 3/3] i2c: mv64xxx: add support for FSM based recovery

[Andi Shyti]

Hi Chris,

...

> +static int
> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> +	int ret;
> +	u32 val;
> +
> +	dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
> +	writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
> +	ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
> +					!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
> +					10, 1000);

mmmhhh... still a bit skeptical about waiting 100 times 10us in
atomic.

I'm still of the opinion that this should run in a separate
thread. Any different opinion from the network?

BTW, first question, considering that you decreased the time
considerably... does it work?

Andi

Re: [PATCH v2 3/3] i2c: mv64xxx: add support for FSM based recovery

[Chris Packham]

On 13/10/23 09:15, Andi Shyti wrote:
> Hi Chris,
>
> ...
>
>> +static int
>> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> +	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
>> +	int ret;
>> +	u32 val;
>> +
>> +	dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
>> +	writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
>> +	ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
>> +					!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
>> +					10, 1000);
> mmmhhh... still a bit skeptical about waiting 100 times 10us in
> atomic.
>
> I'm still of the opinion that this should run in a separate
> thread. Any different opinion from the network?
>
> BTW, first question, considering that you decreased the time
> considerably... does it work?
Yes it still works. It did stop working with a really low timeout (10, 
100) but I didn't look hard for anything in-between.
>
> Andi

Re: [PATCH v2 3/3] i2c: mv64xxx: add support for FSM based recovery

[Wolfram Sang]

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


> mmmhhh... still a bit skeptical about waiting 100 times 10us in
> atomic.

Has it been discussed already why the non-atomic version of
read_poll_timeout is not enough?


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

Re: [PATCH v2 3/3] i2c: mv64xxx: add support for FSM based recovery

[Chris Packham]

On 13/10/23 20:11, Wolfram Sang wrote:
>> mmmhhh... still a bit skeptical about waiting 100 times 10us in
>> atomic.
> Has it been discussed already why the non-atomic version of
> read_poll_timeout is not enough?
>
For mv64xxx i2c_recovery() is called from two places. One would be fine 
with read_poll_timeout() but the other is in an interrupt handler so 
needs the atomic version (or something else that doesn't schedule).

Re: [PATCH v2 2/3] arm64: dts: marvell: AC5: use I2C unstuck function

[Gregory CLEMENT]

Hello Chris,

> The AC5 SoC supports using a controller based I2C unstuck function for
> recovery. Use this instead of the generic GPIO recovery.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> index c9ce1010c415..e52d3c3496d5 100644
> --- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> @@ -137,7 +137,7 @@ mdio: mdio@22004 {
>  
>  			i2c0: i2c@11000{
>  				compatible = "marvell,mv78230-i2c";
> -				reg = <0x11000 0x20>;
> +				reg = <0x11000 0x20>, <0x110a0 0x4>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> @@ -146,17 +146,14 @@ i2c0: i2c@11000{
>  				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
>  				clock-frequency=<100000>;
>  
> -				pinctrl-names = "default", "gpio";
> +				pinctrl-names = "default";
>  				pinctrl-0 = <&i2c0_pins>;
> -				pinctrl-1 = <&i2c0_gpio>;
> -				scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> -				sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;

By doing this then older kernel won't be able to do recovery, while if
you keep it, the new kernels will still use new way to support recovery
thanks to the new reg filed added and old kernels will continue to work.

However, what we try to maintain is running new kernel on old dtb not
the opposite which is just a nice to have. At the end it is up to you,
if you really want to remove this chunk I will apply it once the driver
part of the series will be accepted.

Gregory


>  				status = "disabled";
>  			};
>  
>  			i2c1: i2c@11100{
>  				compatible = "marvell,mv78230-i2c";
> -				reg = <0x11100 0x20>;
> +				reg = <0x11100 0x20>, <0x110a4 0x4>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> @@ -165,11 +162,8 @@ i2c1: i2c@11100{
>  				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>  				clock-frequency=<100000>;
>  
> -				pinctrl-names = "default", "gpio";
> +				pinctrl-names = "default";
>  				pinctrl-0 = <&i2c1_pins>;
> -				pinctrl-1 = <&i2c1_gpio>;
> -				scl-gpios = <&gpio0 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> -				sda-gpios = <&gpio0 21 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>  				status = "disabled";
>  			};
>  
> -- 
> 2.42.0
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v2 2/3] arm64: dts: marvell: AC5: use I2C unstuck function

[Chris Packham]

On 20/10/23 03:40, Gregory CLEMENT wrote:
> Hello Chris,
>
>> The AC5 SoC supports using a controller based I2C unstuck function for
>> recovery. Use this instead of the generic GPIO recovery.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
>> index c9ce1010c415..e52d3c3496d5 100644
>> --- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
>> +++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
>> @@ -137,7 +137,7 @@ mdio: mdio@22004 {
>>   
>>   			i2c0: i2c@11000{
>>   				compatible = "marvell,mv78230-i2c";
>> -				reg = <0x11000 0x20>;
>> +				reg = <0x11000 0x20>, <0x110a0 0x4>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>> @@ -146,17 +146,14 @@ i2c0: i2c@11000{
>>   				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
>>   				clock-frequency=<100000>;
>>   
>> -				pinctrl-names = "default", "gpio";
>> +				pinctrl-names = "default";
>>   				pinctrl-0 = <&i2c0_pins>;
>> -				pinctrl-1 = <&i2c0_gpio>;
>> -				scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> -				sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> By doing this then older kernel won't be able to do recovery, while if
> you keep it, the new kernels will still use new way to support recovery
> thanks to the new reg filed added and old kernels will continue to work.
>
> However, what we try to maintain is running new kernel on old dtb not
> the opposite which is just a nice to have. At the end it is up to you,
> if you really want to remove this chunk I will apply it once the driver
> part of the series will be accepted.

The GPIO recovery triggers an Erratum where the SoC locks up so I'd 
prefer to see it gone (basically a version of that offload Erratum from 
the early Armada-XPs).

I think it's all academic because I'm pretty sure I'm the only one 
actually running an upstream kernel on the AC5X. Marvell still ship a 
horribly out of date fork in their official SDK.

>
> Gregory
>
>
>>   				status = "disabled";
>>   			};
>>   
>>   			i2c1: i2c@11100{
>>   				compatible = "marvell,mv78230-i2c";
>> -				reg = <0x11100 0x20>;
>> +				reg = <0x11100 0x20>, <0x110a4 0x4>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>> @@ -165,11 +162,8 @@ i2c1: i2c@11100{
>>   				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>>   				clock-frequency=<100000>;
>>   
>> -				pinctrl-names = "default", "gpio";
>> +				pinctrl-names = "default";
>>   				pinctrl-0 = <&i2c1_pins>;
>> -				pinctrl-1 = <&i2c1_gpio>;
>> -				scl-gpios = <&gpio0 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> -				sda-gpios = <&gpio0 21 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>   				status = "disabled";
>>   			};
>>   
>> -- 
>> 2.42.0
>>