Re: [PATCH v2 1/3] i2c: qcom-cci: Switch msm8953 to the CCI v2 timing/rate config
From: Vladimir Zapolskiy
Date: Tue Jul 28 2026 - 05:50:35 EST
Hi Loic,
On 7/28/26 11:20, Loic Poulain wrote:
Hi Vladimir,
On Mon, Jul 27, 2026 at 9:21 PM Vladimir Zapolskiy
<vladimir.zapolskiy@xxxxxxxxxx> wrote:
Hi Loic,
On 7/27/26 12:21, Loic Poulain wrote:
The msm8953 CCI timing table is internally inconsistent. Its Standard
likely I was misunderstood in my v1 review comments, and I believe v3
will be needed...
If there is just one master, or two masters set in equal speed mode,
then there is no such issue as "msm8953 CCI timing table is internally
inconsistent". In other words generally it shall be permitted to have
supply clock frequency intermixed speed modes for any CCI variant.
The two masters sharing the supply clock are independently
configurable in terms of mode, so we simply can't guarantee they will
run in same mode.
There should be no restriction or guarantee to configure the same mode
for two masters. The supply clock frequency is one for both masters,
but speed mode selection is based on its presense in the driver by
the CCI frequency.
If there is no match in the driver for CCI clock frequency/speed mode,
return -EOPNOTSUPP for such master.
Think of it, right now for each CCI hardware revision 3 'struct hw_params'
are provided by the driver, some of them match 19.2MHz frequency, some of
them match 37.5MHz frequency or intermixed for msm8953 case. Likely there
is no restriction to provide 6 'struct hw_params' for each CCI hardware
revision covering both 19.2MHz and 37.5MHz supply clock frequencies, then
any combination of speed modes can be configured on any CCI hardware for
two masters.
The main point is that CCI clock frequency is not specific to CCI hardware,
but it is specific to 'struct hw_params' mode.
I'll repeat the same point as given in v1, namely supply clock frequency
is not a property of CCI revision (therefore v1 1/3 or v2 2/3 is invalid),
it is a property of the mode settings. It's correct to remove 'cci_clk_rate'
from 'struct cci_data', and it will be correct to add (or parameterize in
any other way) 'cci_clk_rate' to 'struct hw_params'. Each instance of
'struct hw_params' is strictly bound to a particular CCI clock frequency.
and Fast timings match v1/v1.5, which are calibrated for a 19.2 MHz CCI
clock, but its Fast+ timings are essentially the v2 values, which are
calibrated for 37.5 MHz. Since all masters share a single CCI clock,
So, you're right that the supply clock frequency is a property of the
mode/timing settings (though I'm not entirely sure timings are fully
hardware rev agnostic). I don't dispute that.
However, this reflects the current driver behavior. It's not
incorrect, it is a simplified approach that uses a single frequency
point across all operating modes of a given platform.
The driver should be fixed/improved. Reverting the link between CCI
clock frequency and CCI hardware revision is invalid, it should not
be done.
I added a brief paragraph in the V2 cover letter to explain why it's
out of scope.
My argument is about scope. Today the driver has no mechanism to pick
a rate per mode, a single CCI clock is shared by N masters (usually
two) that can run in different modes simultaneously, so per-mode
scaling requires (1) per-mode rate info in the params table, and (2)
vote/arbitration logic to select the highest required rate and still
apply correct timings for every master at that rate. Concretely, with
one master in Standard and another in Fast+, you'd need a 37.5 MHz
shared clock and a Standard-mode row calibrated for 37.5 MHz, which
This is a limitation of the driver only. For a selected CCI revision
please add 'struct hw_params' for Standard mode / 37.5MHz supply clock
frequency, and the problem is solved.
the msm8953 table simply does not provide today. So this isn't a small
tweak, it's a new capability backed by new table data. It would bring
more fine tuned supply rate, but without huge benefit, as cci clock is
gated most of the time due to runtime-pm.
But what if you have only one master?..
It's not important here? we have N master (usually two) and have to
deal with that.
It is important in sense that any one master usecases are properly
supported.
I think the msm8953 data is correct, it shall not be removed.
The msm8953 table as it stands is internally inconsistent (without
per-mode rate info), no single clock makes all three modes correct.
It is so, because there is msm8953 data is incomplete. But the present
msm8953 data is correct.
Moreover, msm8953 is the same HW version as msm8996/sdm630, which
already use cci_v2_data, there's no strong reason for msm8953 to keep
a bespoke, half-and-half table.
Okay, I read it as 'struct hw_params' data for msm8953 can be removed
and v2 data should be used for this CCI hardvare variant.
no single rate can satisfy all three modes with the current table, and
the DT assigns 19.2 MHz, so Fast+ timings are wrong.
If hypotetically 'struct hw_params' Fast+ timing for 19.2MHz supply
clock is added to the driver, this usecase will supported by the driver.
As for today the driver has a bug, no doubt.
This series intentionally does the minimal correct thing, aligning the
timings with a single supply clock and enforcing that clock in the
driver, which owns hw_params and has all the info to produce correct
timing. It fixes reported devices misbehaving when no (or incorrect)
assigned-clock-rate is set.
Since we're discussing it, this fix is not the only one possible,
and the fix which returns the link between CCI hardware revision and
CCI supply clock frequency is an invalid fix.
Not saying what you propose is not do-able, and I clearly understandInstead of a non-productive NAK I can provide a simple change adding
what you mean, but I'd prefer to land this simple
alignment/enforcement first and treat per-mode clock scaling as a
follow-up, rather than coupling a straightforward fix to a larger
refactor. Please clearly Nack if you do not agree with this first
step.
a proper link between speed mode and registers programming.
--
Best wishes,
Vladimir