Re: [PATCH v2 3/3] i2c: qcom-cci: Fix CCI clock rate enforcement

Hi Loic.

On 7/29/26 14:35, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Tue, Jul 28, 2026 at 5:58 PM Vladimir Zapolskiy
> <[email protected]> wrote:
>>
>> Hi Loic.
>>
>> On 7/27/26 12:21, Loic Poulain wrote:
>>> The hw_params timing values (thigh, tlow, etc.) are in CCI clock ticks
>>> and were calibrated for a specific clock rate per hardware variant. If
>>> the clock is running at a different rate the I2C timings will be wrong,
>>> potentially violating the I2C specification.
>>>
>>> Rather than just warning about a mismatch like before, actively set
>>> the clock to the expected rate, using the OPP framework so that boards
>>> which (will) describe an opp table also get the required power-domain and
>>> regulator votes for that rate. The OPP table is optional, boards without
>>> one simply fall back to a plain clk_set_rate() behavior, so existing DTs
>>> keep working.
>>>
>>
>> Can you please share any deficiencies you see, if a regular mechanism
>> of 'assigned-clock-rates' is used in dt descriptions instead of setting
>> a supported clock rate from the driver? Or is opps mechanism supposed to
>> substitute it?
> 
> assigned-clock-rates describes an initial hardware clock configuration
> applied before probe, but is a bit unclear about when we should really
> use it. In our case the CCI core rate isn't a fixed hardware property,
> it's a software policy tied to the I2C mode(s) the driver programs.
> The driver is then component that knows which rate a given mode
> needs, so it should own that decision.

Sent you a verbose comment, but we can continue discussion here.

I'm fine with 'assigned-clocks' variant, but what you say is also fine
with me, if the CCI device driver forcibly sets the desired clock rate,
please note that it does not exclude to set 19.2MHz rate for CCI v2
revisions, for instance, that's why I believe my change shared with
you is a step towards the right direction. A change to implement
a policy mechanism and set the wanted rate can be done on top of it.

> Driving it from DT means the DT author has to know the per-mode rate
> constraints (effectively reading the driver) and keep the two in sync
> (the CCI/I2C bus rate and the CCI core rate), which is probably why
> several dts files got it wrong today. Enforcing the rate in the driver
> fixes all of those cases in one go.
> 
> OPP doesn't really substitute the  assigned-clock-rates, we just use
> it to describe the constraints/votes (power-domain, regulator)
> associated with each operating rate (picked at runtime), if any.
> 
>> For instance qcom,i2c-cci.yaml example uses 'assigned-clock-rates'
>> properly, in many .dtsi files (NB, but not all, which means for a number
>> of platforms CCI hw programming is done incorrectly today) CCI clock
>> rate is set this way.
> 
> Yes and that's fragile. I think we should remove that from the example.
> 

It's up to you, there will be no objections from my side. There might be
pro et contra arguments about 'assigned-clocks' mechanism though.

>>
>> It seems to be sufficient to get the clock rate in probe, compare it
>> against the supported clock rate associated with a wanted mode, return
>> -EOPNOTSUPP if there is no match.
> 
> Certainly valid, but I don't think it would hurt to complement it with
> additional enforcement in the driver. I don't see why the CCI driver
> should be prevented from configuring its own clock. It has all the
> information required to determine the correct rate and should not have
> to rely on or trust whatever clock state happens to be programmed
> beforehand. Instead of merely rejecting an unexpected rate, it can
> program the correct one itself, which is standard driver behavior and
> not particularly different from what many other drivers do.
> 
>> To get a better idea of my proposal please check this simple commit:
>>
>> * https://github.com/torvalds/linux/compare/master...vzapolskiy:linux-lpc32xx:cci-speed-modes
> 
> Thanks, I think I have a clear understanding of what you're advocating
> for. While it does expand the scope of the original series somewhat,
> now we've discussed it in enough detail, I've taken your suggestions
> into account for the upcoming v3. Let's continue the discussion on
> that version.
> 

Thank you!

-- 
Best wishes,
Vladimir