Re: [PATCH v2 1/3] i2c: qcom-cci: Switch msm8953 to the CCI v2 timing/rate config

Hi Vladimir,

On Mon, Jul 27, 2026 at 9:21 PM Vladimir Zapolskiy
<[email protected]> 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.

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

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

> > no single rate can satisfy all three modes with the current table, and
> > the DT assigns 19.2 MHz, so Fast+ timings are wrong.

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.

Not saying what you propose is not do-able, and I clearly understand
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.

Cheers,
Loic