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

Hi Loic,

On 7/25/26 16:06, Loic Poulain wrote:
> 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.

please consider to add .cci_clk_rate property into 'struct hw_params',
since that is its proper and valid place, the rest of the logic can be
build similarly to this series, for instance. Basically that's why
1/3 revert change is not the right step towards a wanted data placement.

Next good (IMO) optimization/improvement might be to describe hardware
programming modes in their own list, and in v1/v1_5/v2/msm8953 cci_data
simply pick the wanted IP programming data up by an index parametrized
by mode speed/clock frequency/hardware revision.

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

That's right.

-- 
Best wishes,
Vladimir