From c9d27133159d0a84bf11052f588194a734cb48d4 Mon Sep 17 00:00:00 2001 From: Quentin Schulz Date: Fri, 7 Nov 2025 12:39:17 +0100 Subject: rockchip: i2c: fix incorrect STOP flag for the interrupt enable register I2C_CON_STOP is a flag to be used for the con register, where it is bit 4 to send the STOP condition. To enable the interrupt the controller sends to tell it's finished sending the STOP condition, it's the ien register at bit 5. Let's use the proper offset. My hunch is that enabling the interrupt is useless as the interrupt status register is always up-to-date and enabling the interrupt is just so that the interrupt is available via the GIC. However, U-Boot has no interrupt support and the logic was working well before this patch. This is just so people aren't side-tracked when debugging I2C issues on Rockchip by checking all writes are proper. Fixes: 3437469985df ("rockchip: Add I2C driver") Signed-off-by: Quentin Schulz Reviewed-by: Heiko Schocher Reviewed-by: Kever Yang --- drivers/i2c/rk_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c index fa167268ae7..fe09e75d3fb 100644 --- a/drivers/i2c/rk_i2c.c +++ b/drivers/i2c/rk_i2c.c @@ -137,7 +137,7 @@ static int rk_i2c_send_stop_bit(struct rk_i2c *i2c) writel(I2C_IPD_ALL_CLEAN, ®s->ipd); writel(I2C_CON_EN | I2C_CON_STOP, ®s->con); - writel(I2C_CON_STOP, ®s->ien); + writel(I2C_STOPIEN, ®s->ien); start = get_timer(0); while (1) { -- cgit v1.2.3 From 1cf8d0b68d6d853a2727bbb0a785fc7d93304ca4 Mon Sep 17 00:00:00 2001 From: Quentin Schulz Date: Fri, 7 Nov 2025 12:39:18 +0100 Subject: rockchip: i2c: move ACK comment where it applies The I2C_CON_LASTACK is kind of a misnomer as setting it means sending a NACK as last byte acknowledge when the controller is in receive mode. It should therefore be used only when there's no more data to transfer after this. Move the comment in the proper if block. Sync the comment with the Linux kernel's while at it so it's more explicit. Fixes: 5deaa530280f ("rockchip: i2c: fix >32 byte reads") Signed-off-by: Quentin Schulz Reviewed-by: Heiko Schocher Reviewed-by: Kever Yang --- drivers/i2c/rk_i2c.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c index fe09e75d3fb..3c44d0e65f5 100644 --- a/drivers/i2c/rk_i2c.c +++ b/drivers/i2c/rk_i2c.c @@ -195,13 +195,14 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len, while (bytes_remain_len) { if (bytes_remain_len > RK_I2C_FIFO_SIZE) { - con = I2C_CON_EN; - bytes_xferred = 32; - } else { /* * The hw can read up to 32 bytes at a time. If we need - * more than one chunk, send an ACK after the last byte. + * more than one chunk, send an ACK after the last byte + * of the current chunk. */ + con = I2C_CON_EN; + bytes_xferred = 32; + } else { con = I2C_CON_EN | I2C_CON_LASTACK; bytes_xferred = bytes_remain_len; } -- cgit v1.2.3 From 0e5e98081943586b20a45f711bf3f4801191e762 Mon Sep 17 00:00:00 2001 From: Quentin Schulz Date: Fri, 7 Nov 2025 12:39:19 +0100 Subject: 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 --- drivers/i2c/rk_i2c.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'drivers') 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); -- cgit v1.2.3