Re: [PATCH v3] i2c: qcom-cci: always enable SCL clock stretching

[hangxiang . ma]

On 10/7/26 1:01 PM, Hitesh Patel <hitesh@xxxxxxxxxxxxxx> wrote:
> The CCI timing tables leave SCL clock stretching disabled. A slave that
> holds SCL low is then not waited for: the master keeps its own clock
> timing and the transfer fails with a NACK or returns corrupt data.
>
> This is hit with a camera reached through a GMSL serializer/deserializer
> I2C tunnel (MAX9296A/MAX96717 on the RB3 Gen2 vision mezzanine). The
> deserializer acknowledges the address locally, but forwards the
> transaction over the coax link and stretches SCL until the remote side
> has completed it, which takes well over one clock period at 100 kHz.
> Without stretching the register reads of the sensor behind the link
> intermittently return garbage and writes are dropped, which shows up as
> random sensor init failures.
>
> Clock stretching is part of the I2C specification for every speed mode
> and a device that does not stretch is unaffected by enabling it, so set
> the bit unconditionally in cci_init() and drop the per-table
> scl_stretch_en field, which is zero in every table.
>
> Signed-off-by: Hitesh Patel <hitesh@xxxxxxxxxxxxxx>
> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> ---
> Changes in v3:
> - Rebased on i2c/i2c-next, which reworked the timing tables; v2 no longer
>    applied (Andi)
> - The msm8953 table is gone on that branch, so scl_stretch_en is now zero
>    in every table; commit message updated to match
> - Collected Reviewed-by from Loic
>
> Changes in v2:
> - Set the bit unconditionally in cci_init() and drop the per-table
>    scl_stretch_en field, instead of only fixing the v2 standard mode
>    table (Konrad)
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 6d8be7b..04e3053 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -28,6 +28,7 @@
>   #define CCI_I2C_Mm_SDA_CTL_1(m)			(0x108 + 0x100 * (m))
>   #define CCI_I2C_Mm_SDA_CTL_2(m)			(0x10c + 0x100 * (m))
>   #define CCI_I2C_Mm_MISC_CTL(m)			(0x110 + 0x100 * (m))
> +#define CCI_I2C_MISC_CTL_SCL_STRETCH_EN	BIT(8)
>   
>   #define CCI_I2C_Mm_READ_DATA(m)			(0x118 + 0x100 * (m))
>   #define CCI_I2C_Mm_READ_BUF_LEVEL(m)		(0x11c + 0x100 * (m))
> @@ -105,7 +106,6 @@ struct hw_params {
>   	u16 thd_dat; /* data hold time */
>   	u16 thd_sta; /* hold time (repeated) START condition */
>   	u16 tbuf; /* bus free time between a STOP and START condition */
> -	u8 scl_stretch_en;
>   	u16 trdhld;
>   	u16 tsp; /* pulse width of spikes suppressed by the input filter */
>   };
> @@ -260,7 +260,6 @@ static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] =
>   		.thd_dat = 10,
>   		.thd_sta = 77,
>   		.tbuf = 118,
> -		.scl_stretch_en = 0,
>   		.trdhld = 6,
>   		.tsp = 1
>   	},
> @@ -272,7 +271,6 @@ static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] =
>   		.thd_dat = 13,
>   		.thd_sta = 18,
>   		.tbuf = 32,
> -		.scl_stretch_en = 0,
>   		.trdhld = 6,
>   		.tsp = 3
>   	},
> @@ -284,7 +282,6 @@ static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] =
>   		.thd_dat = 22,
>   		.thd_sta = 162,
>   		.tbuf = 227,
> -		.scl_stretch_en = 0,
>   		.trdhld = 6,
>   		.tsp = 3
>   	},
> @@ -296,7 +293,6 @@ static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] =
>   		.thd_dat = 22,
>   		.thd_sta = 35,
>   		.tbuf = 62,
> -		.scl_stretch_en = 0,
>   		.trdhld = 6,
>   		.tsp = 3
>   	},
> @@ -308,7 +304,6 @@ static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] =
>   		.thd_dat = 16,
>   		.thd_sta = 15,
>   		.tbuf = 24,
> -		.scl_stretch_en = 0,
>   		.trdhld = 3,
>   		.tsp = 3
>   	},
> @@ -368,7 +363,7 @@ static int cci_init(struct cci *cci)
>   		val = hw->tbuf;
>   		writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i));
>   
> -		val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
> +		val = CCI_I2C_MISC_CTL_SCL_STRETCH_EN | hw->trdhld << 4 | hw->tsp;
>   		writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
>   	}
>   

Tested-by: Hangxiang Ma <hangxiang.ma@xxxxxxxxxxxxxxxx> # S5KJN5 on Kaanapali

---
Best Regards,
Hangxiang