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

[Heiko Schocher]

Hello Quentin,

On 07.11.25 12:39, Quentin Schulz wrote:
> 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(-)

Thanks for the detailed explanation!

Reviewed-by: Heiko Schocher <hs at nabladev.com>

bye,
Heiko
-- 
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office at nabladev.com
Geschäftsführer : Stefano Babic