[PATCH 3/3] rockchip: i2c: fix illegal I2C START/STOP condition

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz at cherry.de>

In the last message sent in rockchip_i2c_xfer, the controller is
disabled (see rk_i2c_disable() in rk_i2c_read()/rk_i2c_write()), then
the STOP condition is sent (see rk_i2c_send_stop_bit() in
rockchip_i2c_xfer()) and the controller is disabled once again (see
rk_i2c_disable() right after).

The issue is that re-enabling the controller just to send the STOP
condition doesn't work. When, the controller is disabled, the SCL and
SDA lanes are not driven anymore and thus enter the idle mode where they
are kept high by the external HW pull-up. To send a STOP condition, one
needs to drive the SDA line so that a rising edge happens while SCL is
high. Experimentally (on PX30 and RK3399), when enabling the controller
to send a STOP condition after it's been disabled, the controller only
drives the SDA line to trigger the rising edge for the STOP condition,
leaving SCL undriven (and thus, high). This means, that because SDA is
high before this happens and that we need a rising edge, the controller
drives the SDA line low and then releases it, meaning we trigger a START
condition followed by a STOP condition:

SCL
        _________
_____...
        __  _____
_____...  \/
SDA
	    ^ STOP
	  ^ START

This is illegal in I2C protocol[1]:

 5. A START condition immediately followed by a STOP condition (void
    message) is an illegal format. Many devices however are designed to
    operate properly under this condition.

My guess is that the I2C controller IP knows that it makes only sense to
send a STOP condition after a START condition, meaning the controller is
already driving the SCL line low and neither the device nor controller
drive the SDA line after the last ACK/NACK as there's no need to, then
it needs to drive SDA, release SCL to make it high and then release the
SDA line. However, after it's been disabled, the SCL is already released
so the controller only essentially drives SDA and then releases it.

It happens that this seems to be breaking the SE050 Secure Element after
a few transfers in the middle of a transfer where it starts clock
stretching the bus forever. It may be related to Errata 3.2[2] but the
description of the setup isn't an exact match to the current situation.

It seems to be required to disable the I2C controller between messages
as the Linux kernel states that "The HW is actually not capable of
REPEATED START. But we can get the intended effect by resetting its
internal state and issuing an ordinary START.". Between messages, this
logic seems fine as I get an Sr (repeated START condition) before
starting the next message in the transfer without a STOP condition.
However, we should NOT disable the controller after the last message in
the transfer otherwise we do this illegal START condition followed by
the STOP condition, hence the added check.

[1] https://www.nxp.com/docs/en/user-guide/UM10204.pdf 3.1.10 The target address and R/W bit point 5
[2] https://www.nxp.com/docs/en/errata/SE050_Erratasheet.pdf

Fixes: c9fca5ec8849 ("rockchip: i2c: don't sent stop bit after each message")
Signed-off-by: Quentin Schulz <quentin.schulz at cherry.de>
---
 drivers/i2c/rk_i2c.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
index 3c44d0e65f5..def07018148 100644
--- a/drivers/i2c/rk_i2c.c
+++ b/drivers/i2c/rk_i2c.c
@@ -255,8 +255,6 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 	}
 
 i2c_exit:
-	rk_i2c_disable(i2c);
-
 	return err;
 }
 
@@ -333,8 +331,6 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 	}
 
 i2c_exit:
-	rk_i2c_disable(i2c);
-
 	return err;
 }
 
@@ -359,6 +355,18 @@ static int rockchip_i2c_xfer(struct udevice *bus, struct i2c_msg *msg,
 			ret = -EREMOTEIO;
 			break;
 		}
+
+		/*
+		 * The HW is actually not capable of REPEATED START. But we can
+		 * get the intended effect by resetting its internal state
+		 * and issuing an ordinary START.
+		 *
+		 * Do NOT disable the controller after the last message (before
+		 * sending the STOP condition) as this triggers an illegal
+		 * START condition followed by a STOP condition.
+		 */
+		if (nmsgs > 1)
+			rk_i2c_disable(i2c);
 	}
 
 	rk_i2c_send_stop_bit(i2c);

-- 
2.51.1