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

On Tue, Jul 28, 2026 at 11:38 AM Vladimir Zapolskiy
<[email protected]> wrote:
>
> Hi Loic,
>
> On 7/28/26 11:20, Loic Poulain wrote:
> > 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.
>
> 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 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.
>
> Instead of a non-productive NAK I can provide a simple change adding
> a proper link between speed mode and registers programming.

So I guess what you'd like is something along these lines:
https://github.com/loicpoulain/linux/commit/fdacebb22d914ca64b0c3d4e257a913dc0cd16dd

My intention was simply to move this 'disruptive' change out of the
series scope, as I don't believe this series itself is incorrect.

However, if you'd really rather have everything addressed in a single
series, I'd be inclined to either pick/rework the series in that
direction.

Regards,
Loic