[PATCH v2 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[PATCH v2 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

When implementing bus recovery for the i2c-rcar driver, two problems were
encountered: 1) When reading the SDA bit, not the SDA status was returned but
the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
consider the bus free again. SCL/SDA high is not enough, and there is no other
way known to reset the internal logic otherwise.

The obvious solution to just send STOP after recovery makes sense for the
generic case, too, IMO. If we made a device release SDA again, and are about
start a new transfer using START, then we should terminate the previous state
properly with STOP. This may help with some devices and shouldn't create any
drawback AFAICS.

For this, we need to introduce a 'set_sda' callback to the recovery
infrastructure. The first five patches may be interesting for anyone, so input
is greatly appreciated. Also, testing the new features with GPIO based recovery
would be awesome to have. Thanks to Phil for testing already!

This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
is documented here:

https://elinux.org/Tests:I2C-bus-recovery

A branch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery

Please let me know what you think.

Changes since V1:

* add more identifiers in patch 2
* don't use GPIOF_* flags but hardocded '0'
* added Phil's Tested-by



Wolfram Sang (6):
  i2c: make kerneldoc about bus recovery more precise
  i2c: add identifier in declarations for i2c_bus_recovery
  i2c: add 'set_sda' to bus_recovery_info
  i2c: ensure SDA is released in recovery if SDA is controllable
  i2c: send STOP after successful bus recovery
  i2c: rcar: implement bus recovery

 drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
 drivers/i2c/i2c-core-base.c   | 25 +++++++++++++++++++-
 include/linux/i2c.h           | 26 ++++++++++++---------
 3 files changed, 91 insertions(+), 14 deletions(-)

-- 
2.11.0

[PATCH v2 1/6] i2c: make kerneldoc about bus recovery more precise

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

"Used internally" is vague. What it actually means is that those fields
are populated by the core if valid GPIOs are provided. Change the
comments to reflect that.

Tested-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/i2c.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5d7f3c1853ae4b..f8a9d81e911e52 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -545,12 +545,12 @@ struct i2c_timings {
  * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
  *	i2c_generic_scl_recovery().
  * @get_scl: This gets current value of SCL line. Mandatory for generic SCL
- *      recovery. Used internally for generic GPIO recovery.
- * @set_scl: This sets/clears SCL line. Mandatory for generic SCL recovery. Used
- *      internally for generic GPIO recovery.
+ *      recovery. Populated internally for generic GPIO recovery.
+ * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
+ *      Populated internally for generic GPIO recovery.
  * @get_sda: This gets current value of SDA line. Optional for generic SCL
- *      recovery. Used internally, if sda_gpio is a valid GPIO, for generic GPIO
- *      recovery.
+ *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
+ *      GPIO recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
-- 
2.11.0

[PATCH v2 2/6] i2c: add identifier in declarations for i2c_bus_recovery

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

No reason to have them undefined, so let's add them.

Tested-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since V1:

* add more identifiers

 include/linux/i2c.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f8a9d81e911e52..fd87b806b5f165 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -559,14 +559,14 @@ struct i2c_timings {
  * @sda_gpiod: gpiod of the SDA line. Only required for GPIO recovery.
  */
 struct i2c_bus_recovery_info {
-	int (*recover_bus)(struct i2c_adapter *);
+	int (*recover_bus)(struct i2c_adapter *adap);
 
-	int (*get_scl)(struct i2c_adapter *);
-	void (*set_scl)(struct i2c_adapter *, int val);
-	int (*get_sda)(struct i2c_adapter *);
+	int (*get_scl)(struct i2c_adapter *adap);
+	void (*set_scl)(struct i2c_adapter *adap, int val);
+	int (*get_sda)(struct i2c_adapter *adap);
 
-	void (*prepare_recovery)(struct i2c_adapter *);
-	void (*unprepare_recovery)(struct i2c_adapter *);
+	void (*prepare_recovery)(struct i2c_adapter *adap);
+	void (*unprepare_recovery)(struct i2c_adapter *adap);
 
 	/* gpio recovery */
 	struct gpio_desc *scl_gpiod;
-- 
2.11.0

[PATCH v2 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This will be needed when we want to create STOP conditions, too, later.
Create the needed fields and populate them for the GPIO case if the GPIO
is set to output.

Tested-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since V1:

* don't use GPIOF_* flags but hardocded '0'

 drivers/i2c/i2c-core-base.c | 11 ++++++++++-
 include/linux/i2c.h         |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index bb34a5d4113331..a8b3fbe16fa08c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -147,6 +147,11 @@ static int get_sda_gpio_value(struct i2c_adapter *adap)
 	return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
 }
 
+static void set_sda_gpio_value(struct i2c_adapter *adap, int val)
+{
+	gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
+}
+
 /*
  * We are generating clock pulses. ndelay() determines durating of clk pulses.
  * We will generate clock with rate 100 KHz and so duration of both clock levels
@@ -225,8 +230,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 	if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
 		bri->get_scl = get_scl_gpio_value;
 		bri->set_scl = set_scl_gpio_value;
-		if (bri->sda_gpiod)
+		if (bri->sda_gpiod) {
 			bri->get_sda = get_sda_gpio_value;
+			/* FIXME: add proper flag instead of '0' once available */
+			if (gpiod_get_direction(bri->sda_gpiod) == 0)
+				bri->set_sda = set_sda_gpio_value;
+		}
 		return;
 	}
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fd87b806b5f165..419a38e7c315f1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -551,6 +551,9 @@ struct i2c_timings {
  * @get_sda: This gets current value of SDA line. Optional for generic SCL
  *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
  *      GPIO recovery.
+ * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
+ *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
+ *	recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
@@ -564,6 +567,7 @@ struct i2c_bus_recovery_info {
 	int (*get_scl)(struct i2c_adapter *adap);
 	void (*set_scl)(struct i2c_adapter *adap, int val);
 	int (*get_sda)(struct i2c_adapter *adap);
+	void (*set_sda)(struct i2c_adapter *adap, int val);
 
 	void (*prepare_recovery)(struct i2c_adapter *adap);
 	void (*unprepare_recovery)(struct i2c_adapter *adap);
-- 
2.11.0

[PATCH v2 4/6] i2c: ensure SDA is released in recovery if SDA is controllable

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

If we have a function to control SDA, we should ensure that SDA is not
held down by us. So, release the GPIO in this case.

Tested-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index a8b3fbe16fa08c..54cef80990a11a 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -169,6 +169,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		bri->prepare_recovery(adap);
 
 	bri->set_scl(adap, val);
+	if (bri->set_sda)
+		bri->set_sda(adap, 1);
 	ndelay(RECOVERY_NDELAY);
 
 	/*
-- 
2.11.0

[PATCH v2 5/6] i2c: send STOP after successful bus recovery

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

If we managed to get a client release SDA again, send a STOP afterwards
to make sure we have a consistent state on the bus again.

Tested-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 54cef80990a11a..23f353630db32f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -199,6 +199,18 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 	if (bri->get_sda && !bri->get_sda(adap))
 		ret = -EBUSY;
 
+	/* If all went well, send STOP for a sane bus state. */
+	if (ret == 0 && bri->set_sda) {
+		bri->set_scl(adap, 0);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_sda(adap, 0);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_scl(adap, 1);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_sda(adap, 1);
+		ndelay(RECOVERY_NDELAY / 2);
+	}
+
 	if (bri->unprepare_recovery)
 		bri->unprepare_recovery(adap);
 
-- 
2.11.0

[PATCH v2 6/6] i2c: rcar: implement bus recovery

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

We can force levels of SCL and SDA, so we can use that for bus recovery.
Note that we cannot read SDA back, because we will only get the internal
state of the bus free detection.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 8a2ae3e6c561c4..d4b7b5380c2983 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -132,6 +132,7 @@ struct rcar_i2c_priv {
 	int pos;
 	u32 icccr;
 	u32 flags;
+	u8 recovery_icmcr;	/* protected by adapter lock */
 	enum rcar_i2c_type devtype;
 	struct i2c_client *slave;
 
@@ -158,6 +159,46 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
 	return readl(priv->io + reg);
 }
 
+static int rcar_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
+
+};
+
+static void rcar_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	if (val)
+		priv->recovery_icmcr |= FSCL;
+	else
+		priv->recovery_icmcr &= ~FSCL;
+
+	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+/* No get_sda, because the HW only reports its bus free logic, not SDA itself */
+
+static void rcar_i2c_set_sda(struct i2c_adapter *adap, int val)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	if (val)
+		priv->recovery_icmcr |= FSDA;
+	else
+		priv->recovery_icmcr &= ~FSDA;
+
+	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+static struct i2c_bus_recovery_info rcar_i2c_bri = {
+	.get_scl = rcar_i2c_get_scl,
+	.set_scl = rcar_i2c_set_scl,
+	.set_sda = rcar_i2c_set_sda,
+	.recover_bus = i2c_generic_scl_recovery,
+};
 static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 {
 	/* reset master mode */
@@ -170,7 +211,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 
 static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 {
-	int i;
+	int i, ret;
 
 	for (i = 0; i < LOOP_TIMEOUT; i++) {
 		/* make sure that bus is not busy */
@@ -179,7 +220,15 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 		udelay(1);
 	}
 
-	return -EBUSY;
+	/* Waiting did not help, try to recover */
+	priv->recovery_icmcr = MDBS | OBPC | FSDA | FSCL;
+	ret = i2c_recover_bus(&priv->adap);
+
+	/* No failure when recovering, so check bus busy bit again */
+	if (ret == 0)
+		ret = (rcar_i2c_read(priv, ICMCR) & FSDA) ? -EBUSY : 0;
+
+	return ret;
 }
 
 static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv, struct i2c_timings *t)
@@ -851,6 +900,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	adap->retries = 3;
 	adap->dev.parent = dev;
 	adap->dev.of_node = dev->of_node;
+	adap->bus_recovery_info = &rcar_i2c_bri;
 	i2c_set_adapdata(adap, priv);
 	strlcpy(adap->name, pdev->name, sizeof(adap->name));
 
-- 
2.11.0

Re: [PATCH v2 3/6] i2c: add 'set_sda' to bus_recovery_info

[Linus Walleij]

On Tue, Jan 9, 2018 at 2:58 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> This will be needed when we want to create STOP conditions, too, later.
> Create the needed fields and populate them for the GPIO case if the GPIO
> is set to output.
>
> Tested-by: Phil Reid <preid@electromag.com.au>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Geert pointed out a bug in *_cansleep() so I have merged his fix
for v4.15.

Everything "should be smooth".

> +                       /* FIXME: add proper flag instead of '0' once available */
> +                       if (gpiod_get_direction(bri->sda_gpiod) == 0)

Yeah I should fix ... I suspect I need to do something kernel-wide
to shape this up.

Yours,
Linus Walleij

Re: [PATCH v2 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Wolfram Sang]

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

On Tue, Jan 09, 2018 at 02:58:53PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
> 
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
> 
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. The first five patches may be interesting for anyone, so input
> is greatly appreciated. Also, testing the new features with GPIO based recovery
> would be awesome to have. Thanks to Phil for testing already!
> 
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:
> 
> https://elinux.org/Tests:I2C-bus-recovery
> 
> A branch is available here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery
> 
> Please let me know what you think.
> 
> Changes since V1:
> 
> * add more identifiers in patch 2
> * don't use GPIOF_* flags but hardocded '0'
> * added Phil's Tested-by
> 
> 
> 
> Wolfram Sang (6):
>   i2c: make kerneldoc about bus recovery more precise
>   i2c: add identifier in declarations for i2c_bus_recovery
>   i2c: add 'set_sda' to bus_recovery_info
>   i2c: ensure SDA is released in recovery if SDA is controllable
>   i2c: send STOP after successful bus recovery
>   i2c: rcar: implement bus recovery
> 
>  drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/i2c/i2c-core-base.c   | 25 +++++++++++++++++++-
>  include/linux/i2c.h           | 26 ++++++++++++---------
>  3 files changed, 91 insertions(+), 14 deletions(-)

Applied to for-next, thanks!


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