Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

Hi Vladimir,

On Sat, Jul 25, 2026 at 10:36 AM Vladimir Zapolskiy
<[email protected]> wrote:
>
> Hi Loic, Luca,
>
> On 7/21/26 17:58, Loic Poulain wrote:
> > The msm8953 CCI data was added after cci_clk_rate was removed from the
> > driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
> > GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
> > were also calibrated for 19.2 MHz.
> >
> > Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")
> > Signed-off-by: Loic Poulain <[email protected]>
> > ---
> >   drivers/i2c/busses/i2c-qcom-cci.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
> > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
> >               .max_write_len = 11,
> >               .max_read_len = 12,
> >       },
> > +     .cci_clk_rate = 19200000,
> >       .params[I2C_MODE_STANDARD] = {
> >               .thigh = 78,
> >               .tlow = 114,
> >
>
> this simple change is wrong, but commit d202341d9b0c ("i2c: qcom-cci: Add
> msm8953 compatible") causes it, but please note that the commit d202341d9b0c
> has no explicit issues per se (if proper frequencies are set in dt), but
> this particular commit 2/3 breaks the whole picture.
>
> Let's see, MSM8953 I2C_MODE_STANDARD/I2C_MODE_FAST settings repeat the ones
> for v1/v1.5 and 19.2MHz supply clock frequency, but I2C_MODE_FAST_PLUS
> speed setting very close to v2 parameters and assumes 37.MHz supply clock
> frequency.

Good catch, there's a real inconsistency in the table, and it's also
why configuring the clock rate in DT independently of the timing table
is fragile (and not even enforced by the bindings).

> So, at least one clock rate setting for all modes is invalid in this case.
> Automatically it means the reverted commit 1/3 in the series does not
> directly lead to the wanted and well-managed data, and the logic in 3/3
> becomes invalid.

Agreed on the analysis. To be precise though, this doesn't make things
worse than they already are upstream, msm8953.dtsi assigns 19.2 MHz,
and the Standard/Fast rows are identical to v1/v1.5 (calibrated for
19.2 MHz), so those two modes are fine today. Only Fast+ is broken
(without a DT change).

> I would suggest to grasp the statement above carefully, design a bit
> better solution, meanwhile postpone applying any commits from the
> series, especially if a breaking change holds a Fixes tag.

So if you agree? my short-term plan is submitting a V2 with a
switching of msm8953 to the CCI v2 params table, so running the CCI
clock at 37.5 MHz, which is what SDM630/MSM8996 already do (same HW
version). Moreover 37.5 MHz is supported by the msm8953 CCI RCG, so
this is a viable single rate that makes all three modes
self-consistent.

Longer term, clock scaling (probably OPP-based) could be an elegant
improvement, but it's a bigger change, also CCI block's masters share
one clock yet can run in different modes, the per-mode timing/rate
can't be chosen independently per master.

Regards,
Loic