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

[Loic Poulain]

On Mon, Sep 21, 2026 at 3:20 PM Hitesh Patel <hitesh@xxxxxxxxxxxxxx> wrote:
>
> Every CCI timing table except the msm8953 fast-plus one leaves SCL
> clock stretching disabled in MISC_CTL. 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, SC7280 CCI v2 at 100 kHz). The deserializer acknowledges
> the address locally, forwards the transaction over the coax link and
> stretches SCL until the remote side has completed it, which takes well
> over one clock period. 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 it, so there
> is no reason to make it a per-table parameter. Set the bit
> unconditionally and drop the scl_stretch_en field.
>
> Signed-off-by: Hitesh Patel <hitesh@xxxxxxxxxxxxxx>

Looks good to me. In addition to Andi's point about using
--in-reply-to, don't forget to include a short changelog when
submitting a new series/patch revision.

Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>


> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 873e901..fbd5507 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -27,6 +27,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))
> @@ -97,7 +98,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 */
>  };
> @@ -263,7 +263,7 @@ static void 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));
>         }
>  }
> @@ -659,7 +659,6 @@ static const struct cci_data cci_v1_data = {
>                 .thd_dat = 10,
>                 .thd_sta = 77,
>                 .tbuf = 118,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 1
>         },
> @@ -671,7 +670,6 @@ static const struct cci_data cci_v1_data = {
>                 .thd_dat = 13,
>                 .thd_sta = 18,
>                 .tbuf = 32,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 3
>         },
> @@ -692,7 +690,6 @@ static const struct cci_data cci_v1_5_data = {
>                 .thd_dat = 10,
>                 .thd_sta = 77,
>                 .tbuf = 118,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 1
>         },
> @@ -704,7 +701,6 @@ static const struct cci_data cci_v1_5_data = {
>                 .thd_dat = 13,
>                 .thd_sta = 18,
>                 .tbuf = 32,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 3
>         },
> @@ -725,7 +721,6 @@ static const struct cci_data cci_v2_data = {
>                 .thd_dat = 22,
>                 .thd_sta = 162,
>                 .tbuf = 227,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 3
>         },
> @@ -737,7 +732,6 @@ static const struct cci_data cci_v2_data = {
>                 .thd_dat = 22,
>                 .thd_sta = 35,
>                 .tbuf = 62,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 3
>         },
> @@ -749,7 +743,6 @@ static const struct cci_data cci_v2_data = {
>                 .thd_dat = 16,
>                 .thd_sta = 15,
>                 .tbuf = 24,
> -               .scl_stretch_en = 0,
>                 .trdhld = 3,
>                 .tsp = 3
>         },
> @@ -770,7 +763,6 @@ static const struct cci_data cci_msm8953_data = {
>                 .thd_dat = 10,
>                 .thd_sta = 77,
>                 .tbuf = 118,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 1
>         },
> @@ -782,7 +774,6 @@ static const struct cci_data cci_msm8953_data = {
>                 .thd_dat = 13,
>                 .thd_sta = 18,
>                 .tbuf = 32,
> -               .scl_stretch_en = 0,
>                 .trdhld = 6,
>                 .tsp = 3
>         },
> @@ -794,7 +785,6 @@ static const struct cci_data cci_msm8953_data = {
>                 .thd_dat = 16,
>                 .thd_sta = 15,
>                 .tbuf = 19,
> -               .scl_stretch_en = 1,
>                 .trdhld = 3,
>                 .tsp = 3
>         },
> --
> 2.43.0
>
> base-commit: 68142f986ff04b2b70b31db00f719bf690f64a9a