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

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

[Loic Poulain]

The CCI hw_params timing values (thigh, tlow, etc.) are expressed in
clock ticks and were calibrated for a specific CCI clock frequency per
hardware variant (19.2 MHz for v1/v1.5, 37.5 MHz for v2). Running at the
wrong frequency causes incorrect I2C timings that may violate the I2C
specification.

A previous series removed the cci_clk_rate field and the associated
warning from the driver on the grounds that the warning was noisy and
the rate was already set via assigned-clock-rates in the DT. However,
enforcing a clock frequency configuration through DT is fragile and not
the right approach, the driver owns the hw_params and should own the
clock rate too.

This series first fixes the msm8953 timing table, which is internally
inconsistent (Standard/Fast calibrated for 19.2 MHz but Fast+ for
37.5 MHz), by reusing the v2 config (same hardware version as
msm8996/sdm630). It then reverts the removal of cci_clk_rate and
replaces the passive warn-only approach with an active rate enforcement
at probe time. The rate is applied through the OPP framework so that
boards describing an opp table also get the required power-domain and
regulator votes.

Note: Vladimir suggested selecting the CCI clock rate per I2C mode
(Standard/Fast/Fast+) rather than a single per-variant rate. That would
be a much larger change as it requires a table describing all the rates
supported for a given clock, plus proper vote/arbitration management,
since a single CCI clock is shared by multiple masters that may run in
different modes simultaneously. So this is out of scope for this series,
which keeps the simpler per-variant rate.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
Changes in v2:
- Instead of adding a 19.2 MHz cci_clk_rate for msm8953, switch it to
  the v2 config (37.5 MHz). The msm8953 table was internally
  inconsistent (Standard/Fast for 19.2 MHz, Fast+ for 37.5 MHz), so a
  single rate could not satisfy all modes. msm8953 is the same HW
  version as msm8996/sdm630, which already use v2. (Vladimir)
- Remove the msm8953 fix tag (Vladimir)
- Apply the rate via the OPP framework instead of a bare clk_set_rate(),
  managing the vote across runtime PM. (Konrad)
- Link to v1: https://lore.kernel.org/r/20260721-cci-clk-fix-v1-0-5eae78700da8@oss.qualcomm.com

---
Loic Poulain (3):
      i2c: qcom-cci: Switch msm8953 to the CCI v2 timing/rate config
      Revert "i2c: qcom-cci: Remove unused struct member cci_clk_rate"
      i2c: qcom-cci: Fix CCI clock rate enforcement

 drivers/i2c/busses/i2c-qcom-cci.c | 100 ++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 46 deletions(-)
---
base-commit: fce2dfa773ced15f27dd27cd0b482a7473cdcf2a
change-id: 20260721-cci-clk-fix-fe145b2acfa7

Best regards,
-- 
Loic Poulain <loic.poulain@oss.qualcomm.com>

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

[Loic Poulain]

The msm8953 CCI timing table is internally inconsistent. Its Standard
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,
no single rate can satisfy all three modes with the current table, and
the DT assigns 19.2 MHz, so Fast+ timings are wrong.

The msm8953 CCI is the same hardware version as msm8996/sdm630, which
already use the cci_v2_data config (37.5 MHz). 37.5 MHz is supported by
the msm8953 CCI RCG, so reuse cci_v2_data for msm8953 as well and drop
the redundant, inconsistent standalone table. This makes all three I2C
modes self-consistent under a single clock rate.

Note this requires the CCI clock to run at 37.5 MHz, the proper clock
rate will be set in an upcoming CCI patch.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 47 +--------------------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 4d64895a9e9e4e0bd5e0ccb5c3cc04b282b1e4d5..23cef922351ce44d71f1691cd7e4aab274fdd196 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -772,54 +772,9 @@ static const struct cci_data cci_v2_data = {
 	},
 };
 
-static const struct cci_data cci_msm8953_data = {
-	.num_masters = 2,
-	.queue_size = { 64, 16 },
-	.quirks = {
-		.max_write_len = 11,
-		.max_read_len = 12,
-	},
-	.params[I2C_MODE_STANDARD] = {
-		.thigh = 78,
-		.tlow = 114,
-		.tsu_sto = 28,
-		.tsu_sta = 28,
-		.thd_dat = 10,
-		.thd_sta = 77,
-		.tbuf = 118,
-		.scl_stretch_en = 0,
-		.trdhld = 6,
-		.tsp = 1
-	},
-	.params[I2C_MODE_FAST] = {
-		.thigh = 20,
-		.tlow = 28,
-		.tsu_sto = 21,
-		.tsu_sta = 21,
-		.thd_dat = 13,
-		.thd_sta = 18,
-		.tbuf = 32,
-		.scl_stretch_en = 0,
-		.trdhld = 6,
-		.tsp = 3
-	},
-	.params[I2C_MODE_FAST_PLUS] = {
-		.thigh = 16,
-		.tlow = 22,
-		.tsu_sto = 17,
-		.tsu_sta = 18,
-		.thd_dat = 16,
-		.thd_sta = 15,
-		.tbuf = 19,
-		.scl_stretch_en = 1,
-		.trdhld = 3,
-		.tsp = 3
-	},
-};
-
 static const struct of_device_id cci_dt_match[] = {
 	{ .compatible = "qcom,msm8226-cci", .data = &cci_v1_data},
-	{ .compatible = "qcom,msm8953-cci", .data = &cci_msm8953_data},
+	{ .compatible = "qcom,msm8953-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,msm8974-cci", .data = &cci_v1_5_data},
 	{ .compatible = "qcom,msm8996-cci", .data = &cci_v2_data},
 

-- 
2.34.1

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

[Vladimir Zapolskiy]

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.

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,

But what if you have only one master?..

I think the msm8953 data is correct, it shall not be removed.

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

They are not wrong, eh...

> 
> The msm8953 CCI is the same hardware version as msm8996/sdm630, which
> already use the cci_v2_data config (37.5 MHz). 37.5 MHz is supported by
> the msm8953 CCI RCG, so reuse cci_v2_data for msm8953 as well and drop
> the redundant, inconsistent standalone table. This makes all three I2C
> modes self-consistent under a single clock rate.
> 
> Note this requires the CCI clock to run at 37.5 MHz, the proper clock
> rate will be set in an upcoming CCI patch.
> 
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

-- 
Best wishes,
Vladimir

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

[Loic Poulain]

Hi Vladimir,

On Mon, Jul 27, 2026 at 9:21 PM Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> 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

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

[Vladimir Zapolskiy]

Hi Loic,

On 7/28/26 11:20, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Mon, Jul 27, 2026 at 9:21 PM Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> 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.

-- 
Best wishes,
Vladimir

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

[Loic Poulain]

On Tue, Jul 28, 2026 at 11:38 AM Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> 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
> > <vladimir.zapolskiy@linaro.org> 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

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

[Vladimir Zapolskiy]

Hi Loic,

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

* https://lore.kernel.org/linux-arm-msm/aL6Vp-3er71AJPJd@linaro.org/

On 7/28/26 13:26, Loic Poulain wrote:
> On Tue, Jul 28, 2026 at 11:38 AM Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> 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
>>> <vladimir.zapolskiy@linaro.org> 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/20260729112719.1151700-1-vladimir.zapolskiy@linaro.org

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

[PATCH v2 2/3] Revert "i2c: qcom-cci: Remove unused struct member cci_clk_rate"

[Loic Poulain]

This reverts commit b641af684fc28a07bee9c808eb6c433f9d9bcec6.

The cci_clk_rate field is needed to enforce the correct CCI clock rate
at probe time. The hw_params timing values are in clock ticks and were
calibrated for a specific frequency per hardware variant, so running at
the wrong rate would violate I2C timing specifications.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 23cef922351ce44d71f1691cd7e4aab274fdd196..2ed58b2a2f31c62ee1c972a53b71f86787144b84 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -117,6 +117,7 @@ struct cci_data {
 	unsigned int num_masters;
 	struct i2c_adapter_quirks quirks;
 	u16 queue_size[NUM_QUEUES];
+	unsigned long cci_clk_rate;
 	struct hw_params params[3];
 };
 
@@ -668,6 +669,7 @@ static const struct cci_data cci_v1_data = {
 		.max_write_len = 10,
 		.max_read_len = 12,
 	},
+	.cci_clk_rate =  19200000,
 	.params[I2C_MODE_STANDARD] = {
 		.thigh = 78,
 		.tlow = 114,
@@ -701,6 +703,7 @@ static const struct cci_data cci_v1_5_data = {
 		.max_write_len = 10,
 		.max_read_len = 12,
 	},
+	.cci_clk_rate =  19200000,
 	.params[I2C_MODE_STANDARD] = {
 		.thigh = 78,
 		.tlow = 114,
@@ -734,6 +737,7 @@ static const struct cci_data cci_v2_data = {
 		.max_write_len = 11,
 		.max_read_len = 12,
 	},
+	.cci_clk_rate =  37500000,
 	.params[I2C_MODE_STANDARD] = {
 		.thigh = 201,
 		.tlow = 174,

-- 
2.34.1

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

[Loic Poulain]

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.

Tested-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 2ed58b2a2f31c62ee1c972a53b71f86787144b84..87d64e308a8152b99ff4cfb4255e24edfb751e75 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_opp.h>
 
 #define CCI_HW_VERSION				0x0
 #define CCI_RESET_CMD				0x004
@@ -128,6 +129,7 @@ struct cci {
 	const struct cci_data *data;
 	struct clk_bulk_data *clocks;
 	int nclocks;
+	struct clk *cci_clk;
 	struct cci_master master[NUM_MASTERS];
 };
 
@@ -472,10 +474,35 @@ static void cci_disable_clocks(struct cci *cci)
 	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
 }
 
+static int cci_set_core_rate(struct cci *cci, unsigned long rate)
+{
+	struct device *dev = cci->dev;
+	int ret;
+
+	ret = dev_pm_opp_set_rate(dev, rate);
+	if (ret) {
+		dev_warn(dev, "CCI clock could not be set to %lu Hz\n", rate);
+		return ret;
+	}
+
+	if (!rate)
+		return 0;
+
+	/*
+	 * The hw_params timings are only valid at the expected rate,
+	 * so verify what actually landed on the hardware.
+	 */
+	if (clk_get_rate(cci->cci_clk) != rate)
+		dev_warn(dev, "CCI clock is not at expected %lu Hz\n", rate);
+
+	return 0;
+}
+
 static int __maybe_unused cci_suspend_runtime(struct device *dev)
 {
 	struct cci *cci = dev_get_drvdata(dev);
 
+	cci_set_core_rate(cci, 0);
 	cci_disable_clocks(cci);
 	return 0;
 }
@@ -485,6 +512,10 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
 	struct cci *cci = dev_get_drvdata(dev);
 	int ret;
 
+	ret = cci_set_core_rate(cci, cci->data->cci_clk_rate);
+	if (ret)
+		return ret;
+
 	ret = cci_enable_clocks(cci);
 	if (ret)
 		return ret;
@@ -587,6 +618,24 @@ static int cci_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
 	cci->nclocks = ret;
 
+	cci->cci_clk = devm_clk_get(dev, "cci");
+	if (IS_ERR(cci->cci_clk))
+		return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
+				     "failed to get CCI clock\n");
+
+	ret = devm_pm_opp_set_clkname(dev, "cci");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
+
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to add OPP table\n");
+
+	ret = cci_set_core_rate(cci, cci->data->cci_clk_rate);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");
+
 	ret = cci_enable_clocks(cci);
 	if (ret < 0)
 		return ret;

-- 
2.34.1

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

[Mukesh Savaliya]

On 7/27/2026 2:51 PM, Loic Poulain wrote:
[...]

> @@ -587,6 +618,24 @@ static int cci_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
>   	cci->nclocks = ret;
>   
> +	cci->cci_clk = devm_clk_get(dev, "cci");
> +	if (IS_ERR(cci->cci_clk))
> +		return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
> +				     "failed to get CCI clock\n");
> +
> +	ret = devm_pm_opp_set_clkname(dev, "cci");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
> +
> +	/* OPP table is optional */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to add OPP table\n");
> +

if this is an optional why to return with an error ? IMO, further 
operations should continue which are not opp dependent ?

> +	ret = cci_set_core_rate(cci, cci->data->cci_clk_rate);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");
> +
>   	ret = cci_enable_clocks(cci);
>   	if (ret < 0)
>   		return ret;
> 

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

[Loic Poulain]

Hi Mukesh,

On Mon, Jul 27, 2026 at 11:33 AM Mukesh Savaliya
<mukesh.savaliya@oss.qualcomm.com> wrote:
>
>
>
> On 7/27/2026 2:51 PM, Loic Poulain wrote:
> [...]
>
> > @@ -587,6 +618,24 @@ static int cci_probe(struct platform_device *pdev)
> >               return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
> >       cci->nclocks = ret;
> >
> > +     cci->cci_clk = devm_clk_get(dev, "cci");
> > +     if (IS_ERR(cci->cci_clk))
> > +             return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
> > +                                  "failed to get CCI clock\n");
> > +
> > +     ret = devm_pm_opp_set_clkname(dev, "cci");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
> > +
> > +     /* OPP table is optional */
> > +     ret = devm_pm_opp_of_add_table(dev);
> > +     if (ret && ret != -ENODEV)
> > +             return dev_err_probe(dev, ret, "failed to add OPP table\n");
> > +
>
> if this is an optional why to return with an error ? IMO, further
> operations should continue which are not opp dependent ?

The OPP table is optional only in the sense that it may be absent,
that's the -ENODEV case, which we deliberately don't treat as an
error. If a table is present but malformed, or can't be added yet
because a required resource isn't ready (-EPROBE_DEFER), we do want to
report/propagate the error rather than silently continue. This is the
established idiom (spi-qcom-qspi, qcom_geni_serial...).

Regards,
Loic


>
> > +     ret = cci_set_core_rate(cci, cci->data->cci_clk_rate);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");
> > +
> >       ret = cci_enable_clocks(cci);
> >       if (ret < 0)
> >               return ret;
> >
>

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

[Mukesh Savaliya]

Thanks Loic !

On 7/27/2026 3:15 PM, Loic Poulain wrote:
> Hi Mukesh,
> 
> On Mon, Jul 27, 2026 at 11:33 AM Mukesh Savaliya
> <mukesh.savaliya@oss.qualcomm.com> wrote:
>>
>>
>>
>> On 7/27/2026 2:51 PM, Loic Poulain wrote:
>> [...]
>>
>>> @@ -587,6 +618,24 @@ static int cci_probe(struct platform_device *pdev)
>>>                return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
>>>        cci->nclocks = ret;
>>>
>>> +     cci->cci_clk = devm_clk_get(dev, "cci");
>>> +     if (IS_ERR(cci->cci_clk))
>>> +             return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
>>> +                                  "failed to get CCI clock\n");
>>> +
>>> +     ret = devm_pm_opp_set_clkname(dev, "cci");
>>> +     if (ret)
>>> +             return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
>>> +
>>> +     /* OPP table is optional */
>>> +     ret = devm_pm_opp_of_add_table(dev);
>>> +     if (ret && ret != -ENODEV)
>>> +             return dev_err_probe(dev, ret, "failed to add OPP table\n");
>>> +
>>
>> if this is an optional why to return with an error ? IMO, further
>> operations should continue which are not opp dependent ?
> 
> The OPP table is optional only in the sense that it may be absent,
> that's the -ENODEV case, which we deliberately don't treat as an
> error. If a table is present but malformed, or can't be added yet
> because a required resource isn't ready (-EPROBE_DEFER), we do want to
> report/propagate the error rather than silently continue. This is the
> established idiom (spi-qcom-qspi, qcom_geni_serial...).
> 
Sorry, understood now. hence protected with both the conditions. Makes 
sense.

> Regards,
> Loic
> 
> 
>>
>>> +     ret = cci_set_core_rate(cci, cci->data->cci_clk_rate);
>>> +     if (ret)
>>> +             return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");
>>> +
>>>        ret = cci_enable_clocks(cci);
>>>        if (ret < 0)
>>>                return ret;
>>>
>>

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

[Vladimir Zapolskiy]

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?

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.

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.

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

> Tested-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

-- 
Best wishes,
Vladimir

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

[Loic Poulain]

Hi Vladimir,

On Tue, Jul 28, 2026 at 5:58 PM Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> 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 <wenmeng.liu@oss.qualcomm.com>
> > Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>
> --
> Best wishes,
> Vladimir

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

[Vladimir Zapolskiy]

Hi Loic.

On 7/29/26 14:35, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Tue, Jul 28, 2026 at 5:58 PM Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> 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