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

[PATCH 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/msm8953, 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 reverts the removal of cci_clk_rate, fixes the missing rate
for the msm8953 variant (added after the field was removed), and
replaces the passive warn-only approach with an active clk_set_rate()
at probe time, retaining the warning for cases where the rate cannot be
applied.

Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
Loic Poulain (3):
      Revert "i2c: qcom-cci: Remove unused struct member cci_clk_rate"
      i2c: qcom-cci: Add missing cci_clk_rate for msm8953
      i2c: qcom-cci: Fix CCI clock rate enforcement

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

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

[PATCH 1/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 4d64895a9e9e4e0bd5e0ccb5c3cc04b282b1e4d5..19e4719f13b29b2cf60e113565b1b63f19d0e669 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 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

[Loic Poulain]

The msm8953 CCI data was added after cci_clk_rate was removed from the
driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
were also calibrated for 19.2 MHz.

Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")
Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
 		.max_write_len = 11,
 		.max_read_len = 12,
 	},
+	.cci_clk_rate = 19200000,
 	.params[I2C_MODE_STANDARD] = {
 		.thigh = 78,
 		.tlow = 114,

-- 
2.34.1

[PATCH 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, actively set the clock to the
expected rate at probe time. Warn if the rate could not be applied so the
user is informed that timings may be incorrect.

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

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 5f74edde8558382e63e2c3d0fbe19489f325e8ce..e001c0d565cebd386fd67b1d9d532360b29c67d0 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -519,6 +519,7 @@ static int cci_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *child;
 	struct resource *r;
+	struct clk *cci_clk;
 	struct cci *cci;
 	int ret, i;
 	u32 val;
@@ -587,6 +588,15 @@ static int cci_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
 	cci->nclocks = ret;
 
+	cci_clk = devm_clk_get(dev, "cci");
+	if (IS_ERR(cci_clk))
+		return dev_err_probe(dev, PTR_ERR(cci_clk), "failed to get CCI clock\n");
+
+	clk_set_rate(cci_clk, cci->data->cci_clk_rate);
+	if (clk_get_rate(cci_clk) != cci->data->cci_clk_rate)
+		dev_warn(dev, "CCI clock is not at expected %lu Hz\n",
+			 cci->data->cci_clk_rate);
+
 	ret = cci_enable_clocks(cci);
 	if (ret < 0)
 		return ret;

-- 
2.34.1

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

[Konrad Dybcio]

On 7/21/26 4:58 PM, 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, actively set the clock to the
> expected rate at probe time. Warn if the rate could not be applied so the
> user is informed that timings may be incorrect.
> 
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 5f74edde8558382e63e2c3d0fbe19489f325e8ce..e001c0d565cebd386fd67b1d9d532360b29c67d0 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -519,6 +519,7 @@ static int cci_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *child;
>  	struct resource *r;
> +	struct clk *cci_clk;
>  	struct cci *cci;
>  	int ret, i;
>  	u32 val;
> @@ -587,6 +588,15 @@ static int cci_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
>  	cci->nclocks = ret;
>  
> +	cci_clk = devm_clk_get(dev, "cci");
> +	if (IS_ERR(cci_clk))
> +		return dev_err_probe(dev, PTR_ERR(cci_clk), "failed to get CCI clock\n");
> +
> +	clk_set_rate(cci_clk, cci->data->cci_clk_rate);
> +	if (clk_get_rate(cci_clk) != cci->data->cci_clk_rate)
> +		dev_warn(dev, "CCI clock is not at expected %lu Hz\n",
> +			 cci->data->cci_clk_rate);

This needs to go through OPP.

Feel free to take over and refresh:

https://lore.kernel.org/linux-arm-msm/20250904-topic-cci_updates-v1-0-d38559692703@oss.qualcomm.com/

Konrad

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

[Loic Poulain]

Hi Konrad,

On Tue, Jul 21, 2026 at 5:07 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 7/21/26 4:58 PM, 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, actively set the clock to the
> > expected rate at probe time. Warn if the rate could not be applied so the
> > user is informed that timings may be incorrect.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> > ---
> >  drivers/i2c/busses/i2c-qcom-cci.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > index 5f74edde8558382e63e2c3d0fbe19489f325e8ce..e001c0d565cebd386fd67b1d9d532360b29c67d0 100644
> > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -519,6 +519,7 @@ static int cci_probe(struct platform_device *pdev)
> >       struct device *dev = &pdev->dev;
> >       struct device_node *child;
> >       struct resource *r;
> > +     struct clk *cci_clk;
> >       struct cci *cci;
> >       int ret, i;
> >       u32 val;
> > @@ -587,6 +588,15 @@ static int cci_probe(struct platform_device *pdev)
> >               return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
> >       cci->nclocks = ret;
> >
> > +     cci_clk = devm_clk_get(dev, "cci");
> > +     if (IS_ERR(cci_clk))
> > +             return dev_err_probe(dev, PTR_ERR(cci_clk), "failed to get CCI clock\n");
> > +
> > +     clk_set_rate(cci_clk, cci->data->cci_clk_rate);
> > +     if (clk_get_rate(cci_clk) != cci->data->cci_clk_rate)
> > +             dev_warn(dev, "CCI clock is not at expected %lu Hz\n",
> > +                      cci->data->cci_clk_rate);
>
> This needs to go through OPP.
>
> Feel free to take over and refresh:
>
> https://lore.kernel.org/linux-arm-msm/20250904-topic-cci_updates-v1-0-d38559692703@oss.qualcomm.com/

Sorry for missing your series earlier. I still think we need this
series to restore the expected clock rate information in the driver
and enforce it at probe time, right now the timing tables are
calibrated for a specific frequency and the driver has no way to
verify or correct what it gets.

Then, I'm not against an OPP-based approach in principle, though I
think it should be built on top of proper foundations, we would need
to rework the driver so that the mode selection (standard/fast/fast+)
drives the clock rate selection, which in turn selects the appropriate
timing table.

Regards,
Loic

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

[Konrad Dybcio]

On 7/21/26 5:41 PM, Loic Poulain wrote:
> Hi Konrad,
> 
> On Tue, Jul 21, 2026 at 5:07 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 7/21/26 4:58 PM, 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, actively set the clock to the
>>> expected rate at probe time. Warn if the rate could not be applied so the
>>> user is informed that timings may be incorrect.
>>>
>>> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>>> ---

[...]

>>> +     cci_clk = devm_clk_get(dev, "cci");
>>> +     if (IS_ERR(cci_clk))
>>> +             return dev_err_probe(dev, PTR_ERR(cci_clk), "failed to get CCI clock\n");
>>> +
>>> +     clk_set_rate(cci_clk, cci->data->cci_clk_rate);
>>> +     if (clk_get_rate(cci_clk) != cci->data->cci_clk_rate)
>>> +             dev_warn(dev, "CCI clock is not at expected %lu Hz\n",
>>> +                      cci->data->cci_clk_rate);
>>
>> This needs to go through OPP.
>>
>> Feel free to take over and refresh:
>>
>> https://lore.kernel.org/linux-arm-msm/20250904-topic-cci_updates-v1-0-d38559692703@oss.qualcomm.com/
> 
> Sorry for missing your series earlier. I still think we need this
> series to restore the expected clock rate information in the driver
> and enforce it at probe time, right now the timing tables are
> calibrated for a specific frequency and the driver has no way to
> verify or correct what it gets.

What I'm saying is that you need to call dev_pm_opp_set_rate()
anyway, clk_set_rate() doesn't take into account the required
OPPs for a frequency, and those are very low, but non-zero for
CCI. Unfortunately that comes with the added headache of
maintaining an OPP table for each target anyway..

Konrad

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

[Wenmeng Liu]

On 7/21/2026 10:58 PM, Loic Poulain wrote:
> 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/msm8953, 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 reverts the removal of cci_clk_rate, fixes the missing rate
> for the msm8953 variant (added after the field was removed), and
> replaces the passive warn-only approach with an active clk_set_rate()
> at probe time, retaining the warning for cases where the rate cannot be
> applied.
> 
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
> Loic Poulain (3):
>        Revert "i2c: qcom-cci: Remove unused struct member cci_clk_rate"
>        i2c: qcom-cci: Add missing cci_clk_rate for msm8953
>        i2c: qcom-cci: Fix CCI clock rate enforcement
> 
>   drivers/i2c/busses/i2c-qcom-cci.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> ---
> base-commit: fce2dfa773ced15f27dd27cd0b482a7473cdcf2a
> change-id: 20260721-cci-clk-fix-fe145b2acfa7
> 
> Best regards,

For this patch series:

Tested-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

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

[Andi Shyti]

Hi Loic,

On Tue, Jul 21, 2026 at 04:58:19PM +0200, Loic Poulain wrote:
> 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>

Does this revert need a Fixes tag? I haven't seen very often
reverts with Fixes tag, but it makes sense if they need to be
backported.

I'm going to take patch 1 and 2 already while we discuss patch 3
separately, but please let me know first if you agree adding the
Fixes tag.

Andi

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

[Vladimir Zapolskiy]

On 7/25/26 01:57, Andi Shyti wrote:
> Hi Loic,
> 
> On Tue, Jul 21, 2026 at 04:58:19PM +0200, Loic Poulain wrote:
>> 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>
> 
> Does this revert need a Fixes tag? I haven't seen very often
> reverts with Fixes tag, but it makes sense if they need to be
> backported.

This revert commit does not need a Fixes tag, since there is no bug
fixed by the revert commit, I believe. But perhaps a series of commits
may need Fixes tags each, if the series is to be backported.

> I'm going to take patch 1 and 2 already while we discuss patch 3
> separately, but please let me know first if you agree adding the
> Fixes tag.
> 

-- 
Best wishes,
Vladimir

Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

[Vladimir Zapolskiy]

On 7/21/26 17:58, Loic Poulain wrote:
> The msm8953 CCI data was added after cci_clk_rate was removed from the
> driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
> GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
> were also calibrated for 19.2 MHz.
> 
> Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")

By the way this commit also does not fix anything, moreover its backport
won't solve any bugs, but may break the build, if 1/3 is not applied.

I'd ask to consider to remove the Fixes tag here.

> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
>   		.max_write_len = 11,
>   		.max_read_len = 12,
>   	},
> +	.cci_clk_rate = 19200000,
>   	.params[I2C_MODE_STANDARD] = {
>   		.thigh = 78,
>   		.tlow = 114,
> 

-- 
Best wishes,
Vladimir

Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

[Vladimir Zapolskiy]

Hi Loic, Luca,

On 7/21/26 17:58, Loic Poulain wrote:
> The msm8953 CCI data was added after cci_clk_rate was removed from the
> driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
> GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
> were also calibrated for 19.2 MHz.
> 
> Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
>   		.max_write_len = 11,
>   		.max_read_len = 12,
>   	},
> +	.cci_clk_rate = 19200000,
>   	.params[I2C_MODE_STANDARD] = {
>   		.thigh = 78,
>   		.tlow = 114,
> 

this simple change is wrong, but commit d202341d9b0c ("i2c: qcom-cci: Add
msm8953 compatible") causes it, but please note that the commit d202341d9b0c
has no explicit issues per se (if proper frequencies are set in dt), but
this particular commit 2/3 breaks the whole picture.

Let's see, MSM8953 I2C_MODE_STANDARD/I2C_MODE_FAST settings repeat the ones
for v1/v1.5 and 19.2MHz supply clock frequency, but I2C_MODE_FAST_PLUS
speed setting very close to v2 parameters and assumes 37.MHz supply clock
frequency.

So, at least one clock rate setting for all modes is invalid in this case.
Automatically it means the reverted commit 1/3 in the series does not
directly lead to the wanted and well-managed data, and the logic in 3/3
becomes invalid.

I would suggest to grasp the statement above carefully, design a bit
better solution, meanwhile postpone applying any commits from the
series, especially if a breaking change holds a Fixes tag.

-- 
Best wishes,
Vladimir

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

[Vladimir Zapolskiy]

Hi Andi,

On 7/25/26 01:57, Andi Shyti wrote:
> Hi Loic,
> 
> On Tue, Jul 21, 2026 at 04:58:19PM +0200, Loic Poulain wrote:
>> 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>
> 
> Does this revert need a Fixes tag? I haven't seen very often
> reverts with Fixes tag, but it makes sense if they need to be
> backported.
> 
> I'm going to take patch 1 and 2 already while we discuss patch 3
> separately, but please let me know first if you agree adding the
> Fixes tag.
> 

please put all commits from this series on hold due to my last finding:

https://lore.kernel.org/linux-arm-msm/8d20f3ab-696e-40a4-a05a-688779299071@linaro.org

-- 
Best wishes,
Vladimir

Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

[Loic Poulain]

Hi Vladimir,

On Sat, Jul 25, 2026 at 10:36 AM Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Hi Loic, Luca,
>
> On 7/21/26 17:58, Loic Poulain wrote:
> > The msm8953 CCI data was added after cci_clk_rate was removed from the
> > driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
> > GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
> > were also calibrated for 19.2 MHz.
> >
> > Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")
> > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> > ---
> >   drivers/i2c/busses/i2c-qcom-cci.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
> > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
> >               .max_write_len = 11,
> >               .max_read_len = 12,
> >       },
> > +     .cci_clk_rate = 19200000,
> >       .params[I2C_MODE_STANDARD] = {
> >               .thigh = 78,
> >               .tlow = 114,
> >
>
> this simple change is wrong, but commit d202341d9b0c ("i2c: qcom-cci: Add
> msm8953 compatible") causes it, but please note that the commit d202341d9b0c
> has no explicit issues per se (if proper frequencies are set in dt), but
> this particular commit 2/3 breaks the whole picture.
>
> Let's see, MSM8953 I2C_MODE_STANDARD/I2C_MODE_FAST settings repeat the ones
> for v1/v1.5 and 19.2MHz supply clock frequency, but I2C_MODE_FAST_PLUS
> speed setting very close to v2 parameters and assumes 37.MHz supply clock
> frequency.

Good catch, there's a real inconsistency in the table, and it's also
why configuring the clock rate in DT independently of the timing table
is fragile (and not even enforced by the bindings).

> So, at least one clock rate setting for all modes is invalid in this case.
> Automatically it means the reverted commit 1/3 in the series does not
> directly lead to the wanted and well-managed data, and the logic in 3/3
> becomes invalid.

Agreed on the analysis. To be precise though, this doesn't make things
worse than they already are upstream, msm8953.dtsi assigns 19.2 MHz,
and the Standard/Fast rows are identical to v1/v1.5 (calibrated for
19.2 MHz), so those two modes are fine today. Only Fast+ is broken
(without a DT change).

> I would suggest to grasp the statement above carefully, design a bit
> better solution, meanwhile postpone applying any commits from the
> series, especially if a breaking change holds a Fixes tag.

So if you agree? my short-term plan is submitting a V2 with a
switching of msm8953 to the CCI v2 params table, so running the CCI
clock at 37.5 MHz, which is what SDM630/MSM8996 already do (same HW
version). Moreover 37.5 MHz is supported by the msm8953 CCI RCG, so
this is a viable single rate that makes all three modes
self-consistent.

Longer term, clock scaling (probably OPP-based) could be an elegant
improvement, but it's a bigger change, also CCI block's masters share
one clock yet can run in different modes, the per-mode timing/rate
can't be chosen independently per master.

Regards,
Loic

Re: [PATCH 2/3] i2c: qcom-cci: Add missing cci_clk_rate for msm8953

[Vladimir Zapolskiy]

Hi Loic,

On 7/25/26 16:06, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Sat, Jul 25, 2026 at 10:36 AM Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> Hi Loic, Luca,
>>
>> On 7/21/26 17:58, Loic Poulain wrote:
>>> The msm8953 CCI data was added after cci_clk_rate was removed from the
>>> driver, so it never got a clock rate entry. The DT assigns 19.2 MHz to
>>> GCC_CAMSS_CCI_CLK and the hw_params values match those of v1/v1.5 which
>>> were also calibrated for 19.2 MHz.
>>>
>>> Fixes: d202341d9b0c ("i2c: qcom-cci: Add msm8953 compatible")
>>> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>>> ---
>>>    drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>>> index 19e4719f13b29b2cf60e113565b1b63f19d0e669..5f74edde8558382e63e2c3d0fbe19489f325e8ce 100644
>>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>>> @@ -783,6 +783,7 @@ static const struct cci_data cci_msm8953_data = {
>>>                .max_write_len = 11,
>>>                .max_read_len = 12,
>>>        },
>>> +     .cci_clk_rate = 19200000,
>>>        .params[I2C_MODE_STANDARD] = {
>>>                .thigh = 78,
>>>                .tlow = 114,
>>>
>>
>> this simple change is wrong, but commit d202341d9b0c ("i2c: qcom-cci: Add
>> msm8953 compatible") causes it, but please note that the commit d202341d9b0c
>> has no explicit issues per se (if proper frequencies are set in dt), but
>> this particular commit 2/3 breaks the whole picture.
>>
>> Let's see, MSM8953 I2C_MODE_STANDARD/I2C_MODE_FAST settings repeat the ones
>> for v1/v1.5 and 19.2MHz supply clock frequency, but I2C_MODE_FAST_PLUS
>> speed setting very close to v2 parameters and assumes 37.MHz supply clock
>> frequency.
> 
> Good catch, there's a real inconsistency in the table, and it's also
> why configuring the clock rate in DT independently of the timing table
> is fragile (and not even enforced by the bindings).
> 
>> So, at least one clock rate setting for all modes is invalid in this case.
>> Automatically it means the reverted commit 1/3 in the series does not
>> directly lead to the wanted and well-managed data, and the logic in 3/3
>> becomes invalid.
> 
> Agreed on the analysis. To be precise though, this doesn't make things
> worse than they already are upstream, msm8953.dtsi assigns 19.2 MHz,
> and the Standard/Fast rows are identical to v1/v1.5 (calibrated for
> 19.2 MHz), so those two modes are fine today. Only Fast+ is broken
> (without a DT change).
> 
>> I would suggest to grasp the statement above carefully, design a bit
>> better solution, meanwhile postpone applying any commits from the
>> series, especially if a breaking change holds a Fixes tag.
> 
> So if you agree? my short-term plan is submitting a V2 with a
> switching of msm8953 to the CCI v2 params table, so running the CCI
> clock at 37.5 MHz, which is what SDM630/MSM8996 already do (same HW
> version). Moreover 37.5 MHz is supported by the msm8953 CCI RCG, so
> this is a viable single rate that makes all three modes
> self-consistent.

please consider to add .cci_clk_rate property into 'struct hw_params',
since that is its proper and valid place, the rest of the logic can be
build similarly to this series, for instance. Basically that's why
1/3 revert change is not the right step towards a wanted data placement.

Next good (IMO) optimization/improvement might be to describe hardware
programming modes in their own list, and in v1/v1_5/v2/msm8953 cci_data
simply pick the wanted IP programming data up by an index parametrized
by mode speed/clock frequency/hardware revision.

> Longer term, clock scaling (probably OPP-based) could be an elegant
> improvement, but it's a bigger change, also CCI block's masters share
> one clock yet can run in different modes, the per-mode timing/rate
> can't be chosen independently per master.
> 

That's right.

-- 
Best wishes,
Vladimir

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

[Andi Shyti]

Hi Vladimir,

> > > 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>
> > 
> > Does this revert need a Fixes tag? I haven't seen very often
> > reverts with Fixes tag, but it makes sense if they need to be
> > backported.

There is a Requires tag (or similar, I don't remember).

> > I'm going to take patch 1 and 2 already while we discuss patch 3
> > separately, but please let me know first if you agree adding the
> > Fixes tag.
> > 
> 
> please put all commits from this series on hold due to my last finding:
> 
> https://lore.kernel.org/linux-arm-msm/8d20f3ab-696e-40a4-a05a-688779299071@linaro.org

Thanks Vladimir!

Andi