Re: [PATCH v4 4/5] i2c: qcom-cci: Share the timing table across CCI revisions

[Loic Poulain]

Hi Vlad,

On Wed, Aug 19, 2026 at 1:31 PM Vladimir Zapolskiy
<vladimir.zapolskiy@xxxxxxxxxx> wrote:
>
> On 8/1/26 23:10, Loic Poulain wrote:
> > The hw_params timing values only depend on the CCI clock rate and the
> > I2C mode, not on the hardware revision: every per-variant table used
> > identical values for a given [rate][mode]. Only the set of supported
> > modes differs between revisions.
> >
> > Move the timings into a single shared cci_hw_params[rate][mode] table
> > and describe each variant's highest supported mode in cci_data with
> > max_mode instead of duplicating the timing values. This removes the
> > per-variant timing tables without any functional change.
> >
> > Suggested-by: Vladimir Zapolskiy <vladimir.zapolskiy@xxxxxxxxxx>
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/i2c/busses/i2c-qcom-cci.c | 181 +++++++++++++++-----------------------
> >   1 file changed, 70 insertions(+), 111 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > index b6fa37a306758ec33ede6fcc39ce4412b86d9a84..21695c744502f7fb5d333f1272f3771e4d9d5508 100644
> > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -124,7 +124,8 @@ struct cci_data {
> >       unsigned int num_masters;
> >       struct i2c_adapter_quirks quirks;
> >       u16 queue_size[NUM_QUEUES];
> > -     struct hw_params params[NUM_CCI_CLK_RATES][NUM_I2C_MODES];
> > +     /* Highest I2C mode supported by this variant. */
> > +     u8 max_mode;
> >   };
> >
> >   struct cci {
> > @@ -249,13 +250,76 @@ static int cci_clk_rate_idx(unsigned long rate)
> >       return -EINVAL;
> >   }
> >
> > +static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] = {
> > +     [CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> > +             .thigh = 78,
> > +             .tlow = 114,
> > +             .tsu_sto = 28,
> > +             .tsu_sta = 28,
> > +             .thd_dat = 10,
> > +             .thd_sta = 77,
> > +             .tbuf = 118,
> > +             .scl_stretch_en = 0,
> > +             .trdhld = 6,
> > +             .tsp = 1
> > +     },
> > +     [CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> > +             .thigh = 20,
> > +             .tlow = 28,
> > +             .tsu_sto = 21,
> > +             .tsu_sta = 21,
> > +             .thd_dat = 13,
> > +             .thd_sta = 18,
> > +             .tbuf = 32,
> > +             .scl_stretch_en = 0,
> > +             .trdhld = 6,
> > +             .tsp = 3
> > +     },
> > +     [CCI_CLK_RATE_37_5MHZ][I2C_MODE_STANDARD] = {
> > +             .thigh = 201,
> > +             .tlow = 174,
> > +             .tsu_sto = 204,
> > +             .tsu_sta = 231,
> > +             .thd_dat = 22,
> > +             .thd_sta = 162,
> > +             .tbuf = 227,
> > +             .scl_stretch_en = 0,
> > +             .trdhld = 6,
> > +             .tsp = 3
> > +     },
> > +     [CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST] = {
> > +             .thigh = 38,
> > +             .tlow = 56,
> > +             .tsu_sto = 40,
> > +             .tsu_sta = 40,
> > +             .thd_dat = 22,
> > +             .thd_sta = 35,
> > +             .tbuf = 62,
> > +             .scl_stretch_en = 0,
> > +             .trdhld = 6,
> > +             .tsp = 3
> > +     },
> > +     [CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST_PLUS] = {
> > +             .thigh = 16,
> > +             .tlow = 22,
> > +             .tsu_sto = 17,
> > +             .tsu_sta = 18,
> > +             .thd_dat = 16,
> > +             .thd_sta = 15,
> > +             .tbuf = 24,
> > +             .scl_stretch_en = 0,
> > +             .trdhld = 3,
> > +             .tsp = 3
> > +     },
> > +};
> > +
> >   static const struct hw_params *cci_get_hw_params(struct cci *cci, int mode)
> >   {
> >       unsigned long rate = clk_get_rate(cci->cci_clk);
> >       int ri = cci_clk_rate_idx(rate);
> >
> > -     if (ri >= 0 && cci->data->params[ri][mode].thigh)
> > -             return &cci->data->params[ri][mode];
> > +     if (ri >= 0 && mode <= cci->data->max_mode && cci_hw_params[ri][mode].thigh)
>
> This earlier introduced sanity check for non-zero 'cci_hw_params[ri][mode].thigh'
> is not needed anymore, right?

It's still needed. max_mode captures the platform limitation, the
thigh check captures the clock-rate limitation. E.g. we don't define
Fast+ params for the 19.2 MHz entry, so the two checks aren't
equivalent.

>
> > +             return &cci_hw_params[ri][mode];
> >
> >       return NULL;
> >   }
> > @@ -696,30 +760,7 @@ static const struct cci_data cci_v1_data = {
> >               .max_write_len = 10,
> >               .max_read_len = 12,
> >       },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> > -             .thigh = 78,
> > -             .tlow = 114,
> > -             .tsu_sto = 28,
> > -             .tsu_sta = 28,
> > -             .thd_dat = 10,
> > -             .thd_sta = 77,
> > -             .tbuf = 118,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 1
> > -     },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> > -             .thigh = 20,
> > -             .tlow = 28,
> > -             .tsu_sto = 21,
> > -             .tsu_sta = 21,
> > -             .thd_dat = 13,
> > -             .thd_sta = 18,
> > -             .tbuf = 32,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 3
> > -     },
> > +     .max_mode = I2C_MODE_FAST,
> >   };
> >
> >   static const struct cci_data cci_v1_5_data = {
> > @@ -729,30 +770,7 @@ static const struct cci_data cci_v1_5_data = {
> >               .max_write_len = 10,
> >               .max_read_len = 12,
> >       },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> > -             .thigh = 78,
> > -             .tlow = 114,
> > -             .tsu_sto = 28,
> > -             .tsu_sta = 28,
> > -             .thd_dat = 10,
> > -             .thd_sta = 77,
> > -             .tbuf = 118,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 1
> > -     },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> > -             .thigh = 20,
> > -             .tlow = 28,
> > -             .tsu_sto = 21,
> > -             .tsu_sta = 21,
> > -             .thd_dat = 13,
> > -             .thd_sta = 18,
> > -             .tbuf = 32,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 3
> > -     },
> > +     .max_mode = I2C_MODE_FAST,
> >   };
> >
> >   static const struct cci_data cci_v2_data = {
> > @@ -762,66 +780,7 @@ static const struct cci_data cci_v2_data = {
> >               .max_write_len = 11,
> >               .max_read_len = 12,
> >       },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> > -             .thigh = 78,
> > -             .tlow = 114,
> > -             .tsu_sto = 28,
> > -             .tsu_sta = 28,
> > -             .thd_dat = 10,
> > -             .thd_sta = 77,
> > -             .tbuf = 118,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 1
> > -     },
> > -     .params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> > -             .thigh = 20,
> > -             .tlow = 28,
> > -             .tsu_sto = 21,
> > -             .tsu_sta = 21,
> > -             .thd_dat = 13,
> > -             .thd_sta = 18,
> > -             .tbuf = 32,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 3
> > -     },
> > -     .params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_STANDARD] = {
> > -             .thigh = 201,
> > -             .tlow = 174,
> > -             .tsu_sto = 204,
> > -             .tsu_sta = 231,
> > -             .thd_dat = 22,
> > -             .thd_sta = 162,
> > -             .tbuf = 227,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 3
> > -     },
> > -     .params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST] = {
> > -             .thigh = 38,
> > -             .tlow = 56,
> > -             .tsu_sto = 40,
> > -             .tsu_sta = 40,
> > -             .thd_dat = 22,
> > -             .thd_sta = 35,
> > -             .tbuf = 62,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 6,
> > -             .tsp = 3
> > -     },
> > -     .params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST_PLUS] = {
> > -             .thigh = 16,
> > -             .tlow = 22,
> > -             .tsu_sto = 17,
> > -             .tsu_sta = 18,
> > -             .thd_dat = 16,
> > -             .thd_sta = 15,
> > -             .tbuf = 24,
> > -             .scl_stretch_en = 0,
> > -             .trdhld = 3,
> > -             .tsp = 3
> > -     },
> > +     .max_mode = I2C_MODE_FAST_PLUS,
> >   };
> >
> >   static const struct of_device_id cci_dt_match[] = {
> >
>
> --
> Best wishes,
> Vladimir