* [PATCH] i2c: mv64xxx: Add bus error recovery
@ 2020-07-07 21:10 Mark Tomlinson
2020-07-29 3:11 ` Chris Packham
2020-08-12 20:13 ` Wolfram Sang
0 siblings, 2 replies; 3+ 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 related [flat|nested] 3+ 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
2020-08-12 20:13 ` Wolfram Sang
1 sibling, 0 replies; 3+ 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] 3+ 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
@ 2020-08-12 20:13 ` Wolfram Sang
1 sibling, 0 replies; 3+ messages in thread
From: Wolfram Sang @ 2020-08-12 20:13 UTC (permalink / raw)
To: Mark Tomlinson; +Cc: gregory.clement, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5414 bytes --]
On Wed, Jul 08, 2020 at 09:10:36AM +1200, 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>
This came in pretty much the same time we worked on handling this stuff
in the core. Can you rebase this work on top of i2c/for-next and
especially commit 75820314de26 ("i2c: core: add generic I2C GPIO
recovery")? Most of the stuff here should be already included there. I
will try to get your updated patch into 5.9 still.
> ---
> 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-12 20:13 UTC | newest]
Thread overview: 3+ 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
2020-08-12 20:13 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).