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

Message ID 20231006003321.2100016-4-chris.packham@alliedtelesis.co.nz
State New
Headers
Series i2c: mv64xxx: Support for I2C unstuck |

Commit Message

Chris Packham Oct. 6, 2023, 12:33 a.m. UTC
  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(-)
  

Comments

Andi Shyti Oct. 12, 2023, 8:15 p.m. UTC | #1
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
  
Chris Packham Oct. 12, 2023, 8:25 p.m. UTC | #2
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
  
Wolfram Sang Oct. 13, 2023, 7:11 a.m. UTC | #3
> 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?
  
Chris Packham Oct. 15, 2023, 8:12 p.m. UTC | #4
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).
  

Patch

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;