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

[Loic Poulain]

On Thu, Aug 27, 2026 at 11:31 AM Loic Poulain
<loic.poulain@xxxxxxxxxxxxxxxx> wrote:
>
> 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.

Maybe I replied too quickly, you're right. cci_get_required_rate()
already rejects any rate lacking a valid timing set for an active
master's mode, so the check here is redundant.

>
> >
> > > +             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