From: Quentin Schulz Date: Fri, 7 Nov 2025 11:39:19 +0000 (+0100) Subject: rockchip: i2c: fix illegal I2C START/STOP condition X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0e5e98081943586b20a45f711bf3f4801191e762;p=thirdparty%2Fu-boot.git rockchip: i2c: fix illegal I2C START/STOP condition 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 Reviewed-by: Heiko Schocher Reviewed-by: Kever Yang --- 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);