Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] i2c: mv64xxx: Add bus error recovery
@ 2020-07-07 21:10 Mark Tomlinson
  2020-07-29  3:11 ` Chris Packham
  0 siblings, 1 reply; 2+ messages in thread
From: Mark Tomlinson @ 2020-07-07 21:10 UTC (permalink / raw)
  To: gregory.clement; +Cc: linux-i2c, linux-kernel, Mark Tomlinson

This adds i2c bus recovery to the mv64xxx driver.

Implement bus recovery to recover from SCL/SDA stuck low.

This uses the generic recovery function, setting the clock/data lines as
GPIO pins, and sending 9 clocks to try and recover the bus.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 77 +++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 829b8c98ae51..e58853ba3ef0 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/err.h>
@@ -147,6 +148,10 @@ struct mv64xxx_i2c_data {
 	bool			irq_clear_inverted;
 	/* Clk div is 2 to the power n, not 2 to the power n + 1 */
 	bool			clk_n_base_0;
+	struct pinctrl		*pinctrl;
+	struct i2c_bus_recovery_info	rinfo;
+	struct pinctrl_state	*pin_default_state;
+	struct pinctrl_state	*pin_gpio_state;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -325,7 +330,8 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 			 drv_data->msg->flags);
 		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
 		mv64xxx_i2c_hw_init(drv_data);
-		drv_data->rc = -EIO;
+		i2c_recover_bus(&drv_data->adapter);
+		drv_data->rc = -EAGAIN;
 	}
 }
 
@@ -563,6 +569,7 @@ mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
 				"time_left: %d\n", drv_data->block,
 				(int)time_left);
 			mv64xxx_i2c_hw_init(drv_data);
+			i2c_recover_bus(&drv_data->adapter);
 		}
 	} else
 		spin_unlock_irqrestore(&drv_data->lock, flags);
@@ -872,6 +879,69 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 }
 #endif /* CONFIG_OF */
 
+/*
+ * Switch to bit bang mode to prepare for i2c generic recovery.
+ */
+static void mv64xxx_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(drv_data->pinctrl, drv_data->pin_gpio_state);
+}
+
+/*
+ * Return to normal i2c operation following recovery.
+ */
+static void mv64xxx_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(drv_data->pinctrl, drv_data->pin_default_state);
+}
+
+static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
+					  struct platform_device *pd)
+{
+	struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
+	struct device *dev = &pd->dev;
+
+	drv_data->pinctrl = devm_pinctrl_get(dev);
+	if (!drv_data->pinctrl || IS_ERR(drv_data->pinctrl)) {
+		dev_err(dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(drv_data->pinctrl);
+	}
+
+	drv_data->pin_default_state = pinctrl_lookup_state(drv_data->pinctrl,
+			PINCTRL_STATE_DEFAULT);
+	drv_data->pin_gpio_state = pinctrl_lookup_state(drv_data->pinctrl,
+			"gpio");
+	rinfo->scl_gpiod = devm_gpiod_get(dev, "scl",
+					  GPIOD_OUT_HIGH_OPEN_DRAIN);
+	rinfo->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
+	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER ||
+	    PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (IS_ERR(rinfo->sda_gpiod) ||
+	    IS_ERR(rinfo->scl_gpiod) ||
+	    IS_ERR(drv_data->pin_default_state) ||
+	    IS_ERR(drv_data->pin_gpio_state)) {
+		dev_dbg(dev, "recovery information incomplete\n");
+		return 0;
+	}
+
+	dev_dbg(dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
+		rinfo->scl_gpiod ? desc_to_gpio(rinfo->scl_gpiod) : -1,
+		rinfo->sda_gpiod ? desc_to_gpio(rinfo->sda_gpiod) : -1);
+
+	rinfo->prepare_recovery = mv64xxx_i2c_prepare_recovery;
+	rinfo->unprepare_recovery = mv64xxx_i2c_unprepare_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	drv_data->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 static int
 mv64xxx_i2c_probe(struct platform_device *pd)
 {
@@ -939,6 +1009,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 
 	mv64xxx_i2c_hw_init(drv_data);
 
+	rc = mv64xxx_i2c_init_recovery_info(drv_data, pd);
+	if (rc == -EPROBE_DEFER)
+		goto exit_reset;
+
 	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
 			 MV64XXX_I2C_CTLR_NAME, drv_data);
 	if (rc) {
@@ -951,6 +1025,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
 		goto exit_free_irq;
 	}
+	i2c_recover_bus(&drv_data->adapter);
 
 	return 0;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Add bus error recovery
  2020-07-07 21:10 [PATCH] i2c: mv64xxx: Add bus error recovery Mark Tomlinson
@ 2020-07-29  3:11 ` Chris Packham
  0 siblings, 0 replies; 2+ messages in thread
From: Chris Packham @ 2020-07-29  3:11 UTC (permalink / raw)
  To: Mark Tomlinson, gregory.clement; +Cc: linux-i2c, linux-kernel

Hi Mark,

On 8/07/20 9:10 am, Mark Tomlinson wrote:
> This adds i2c bus recovery to the mv64xxx driver.
>
> Implement bus recovery to recover from SCL/SDA stuck low.
>
> This uses the generic recovery function, setting the clock/data lines as
> GPIO pins, and sending 9 clocks to try and recover the bus.
>
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>

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

One additional comment below

> ---
>   drivers/i2c/busses/i2c-mv64xxx.c | 77 +++++++++++++++++++++++++++++++-
>   1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 829b8c98ae51..e58853ba3ef0 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -21,6 +21,7 @@
>   #include <linux/io.h>
>   #include <linux/of.h>
>   #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>   #include <linux/of_irq.h>
>   #include <linux/clk.h>
>   #include <linux/err.h>
> @@ -147,6 +148,10 @@ struct mv64xxx_i2c_data {
>   	bool			irq_clear_inverted;
>   	/* Clk div is 2 to the power n, not 2 to the power n + 1 */
>   	bool			clk_n_base_0;
> +	struct pinctrl		*pinctrl;
> +	struct i2c_bus_recovery_info	rinfo;
> +	struct pinctrl_state	*pin_default_state;
> +	struct pinctrl_state	*pin_gpio_state;
>   };
>   
>   static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> @@ -325,7 +330,8 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>   			 drv_data->msg->flags);
>   		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
>   		mv64xxx_i2c_hw_init(drv_data);
> -		drv_data->rc = -EIO;
> +		i2c_recover_bus(&drv_data->adapter);
> +		drv_data->rc = -EAGAIN;
>   	}
>   }
>   
> @@ -563,6 +569,7 @@ mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
>   				"time_left: %d\n", drv_data->block,
>   				(int)time_left);
>   			mv64xxx_i2c_hw_init(drv_data);
> +			i2c_recover_bus(&drv_data->adapter);
>   		}
>   	} else
>   		spin_unlock_irqrestore(&drv_data->lock, flags);
> @@ -872,6 +879,69 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>   }
>   #endif /* CONFIG_OF */
>   
> +/*
> + * Switch to bit bang mode to prepare for i2c generic recovery.
> + */
> +static void mv64xxx_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(drv_data->pinctrl, drv_data->pin_gpio_state);
> +}
> +
> +/*
> + * Return to normal i2c operation following recovery.
> + */
> +static void mv64xxx_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(drv_data->pinctrl, drv_data->pin_default_state);
> +}
> +
> +static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
> +					  struct platform_device *pd)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
> +	struct device *dev = &pd->dev;
> +
> +	drv_data->pinctrl = devm_pinctrl_get(dev);
> +	if (!drv_data->pinctrl || IS_ERR(drv_data->pinctrl)) {
> +		dev_err(dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(drv_data->pinctrl);
> +	}
> +
> +	drv_data->pin_default_state = pinctrl_lookup_state(drv_data->pinctrl,
> +			PINCTRL_STATE_DEFAULT);
> +	drv_data->pin_gpio_state = pinctrl_lookup_state(drv_data->pinctrl,
> +			"gpio");
> +	rinfo->scl_gpiod = devm_gpiod_get(dev, "scl",
> +					  GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	rinfo->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);

Should these be mentioned in 
Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml?

> +	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER ||
> +	    PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(rinfo->sda_gpiod) ||
> +	    IS_ERR(rinfo->scl_gpiod) ||
> +	    IS_ERR(drv_data->pin_default_state) ||
> +	    IS_ERR(drv_data->pin_gpio_state)) {
> +		dev_dbg(dev, "recovery information incomplete\n");
> +		return 0;
> +	}
> +
> +	dev_dbg(dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
> +		rinfo->scl_gpiod ? desc_to_gpio(rinfo->scl_gpiod) : -1,
> +		rinfo->sda_gpiod ? desc_to_gpio(rinfo->sda_gpiod) : -1);
> +
> +	rinfo->prepare_recovery = mv64xxx_i2c_prepare_recovery;
> +	rinfo->unprepare_recovery = mv64xxx_i2c_unprepare_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	drv_data->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +
>   static int
>   mv64xxx_i2c_probe(struct platform_device *pd)
>   {
> @@ -939,6 +1009,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>   
>   	mv64xxx_i2c_hw_init(drv_data);
>   
> +	rc = mv64xxx_i2c_init_recovery_info(drv_data, pd);
> +	if (rc == -EPROBE_DEFER)
> +		goto exit_reset;
> +
>   	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
>   			 MV64XXX_I2C_CTLR_NAME, drv_data);
>   	if (rc) {
> @@ -951,6 +1025,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>   			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
>   		goto exit_free_irq;
>   	}
> +	i2c_recover_bus(&drv_data->adapter);
>   
>   	return 0;
>   

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 21:10 [PATCH] i2c: mv64xxx: Add bus error recovery Mark Tomlinson
2020-07-29  3:11 ` Chris Packham

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git