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

[Kever Yang]

On 2025/11/7 19: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>
Reviewed-by: Kever Yang <kever.yang at rock-chips.com>

Thanks,
- Kever
> ---
>   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);
>