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

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.

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

Regards,
Loic


>
> > Tested-by: Wenmeng Liu <[email protected]>
> > Suggested-by: Konrad Dybcio <[email protected]>
> > Signed-off-by: Loic Poulain <[email protected]>
>
> --
> Best wishes,
> Vladimir