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

Hi Loic,

cc: Stephan, Konrad as participants of this old discussion:

* https://lore.kernel.org/linux-arm-msm/[email protected]/

On 7/28/26 13:26, Loic Poulain wrote:
> 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
> 

I shared another and personally more preferred variant yesterday:
* https://github.com/vzapolskiy/linux-lpc32xx/commit/e3dee612da90

The main differences:
* all CCI programming data is consolidated and removed from driver
   data specific to CCI revisions,
* as an immediate positive side effect 19.2MHz supply clock frequency
   is supported for v2 revision of CCI.

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

If you find it possible to incorporate the change above, it would
be very much appreciated.

The next question to solve is to fix CCI mode programming for all
platforms with CCI v2 revision, which are obviously broken right
now, and partially are fixed by the change proposed by me above.

These platform .dtsi files contain CCI clock rate assignment:
* agatti.dtsi
* msm8916.dtsi
* msm8939.dtsi
* msm8953.dtsi
* msm8996.dtsi
* sdm630.dtsi
* sdm845.dtsi
* sm6350.dtsi

These platform .dtsi files do not contain CCI clock rate assignments:
* kodiak.dtsi
* lemans.dtsi
* milos.dtsi
* monaco.dtsi
* sc8280xp.dtsi
* sdm670.dtsi
* sm8250.dtsi
* sm8450.dtsi
* sm8550.dtsi
* sm8650.dtsi
* talos.dtsi

One proposed "fix" (note that formally it is not a fix, but it compliments
a today's flaw in the driver) is to add CCI clock rate assigments like this,
it seems Stephan and me support such an approach, its benefit is that it
does not require any driver changes:
* https://lore.kernel.org/linux-arm-msm/[email protected]

Another and likely more preferred approach suggested by Konrad is to
use OPP tables. If this one is selected, then, as Konrad said, it would
be needed to resurrect/rebase his series, it would be important to update
the dt binding documentation etc. Please note, that a forcible setting
of CCI clock rate over OPP mechanism can also be done independently on
top of my non-intrusive "read-only" change.

-- 
Best wishes,
Vladimir