Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953
From: Vladimir Zapolskiy
Date: Sat Jul 25 2026 - 10:22:41 EST
Hi Loic,
On 7/25/26 16:06, Loic Poulain wrote:
Hi Vladimir,
On Sat, Jul 25, 2026 at 10:36 AM Vladimir Zapolskiy
<vladimir.zapolskiy@xxxxxxxxxx> 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 <loic.poulain@xxxxxxxxxxxxxxxx>
---
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