[PATCH 0/5] Qualcomm CCI I2C clock requirements enforcement

[PATCH 0/5] Qualcomm CCI I2C clock requirements enforcement

[Konrad Dybcio]

The hardware requires the faster of the two (37.5 MHz as opposed to
19.2 MHz) clock rates to hit the required timings for I2C Fast+ Mode.

Additionally, the magic presets for electrical tuning registers on SoCs
supporting that faster mode ("cci_v2" in the driver) are calculated
based on that faster frequency.

Moreover, while its unlikely that it would ever exhibit as an issue
given CCI is a slow & tiny core, we do need to express a minimal voltage
level for any given clock rate, which is where the (optional -
backwards compat) OPP table addition comes in.

This series helps ensure all these requirements are met.

Patch 1 is a related but independent fix, can be picked right away
Patch 5 can be functionally merged as-is, but depends on patch 2 for
bindings

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
Konrad Dybcio (5):
      arm64: dts: qcom: sc8280xp: Fix CCI3 interrupt
      dt-bindings: i2c: qcom-cci: Allow operating-points-v2
      i2c: qcom-cci: Drop single-line wrappers
      i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements
      arm64: dts: qcom: sc8280xp: Add OPP table for CCI hosts

 .../devicetree/bindings/i2c/qcom,i2c-cci.yaml      |  2 +
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi             | 16 +++++++-
 drivers/i2c/busses/i2c-qcom-cci.c                  | 45 +++++++++++++++++-----
 3 files changed, 52 insertions(+), 11 deletions(-)
---
base-commit: 4ac65880ebca1b68495bd8704263b26c050ac010
change-id: 20250904-topic-cci_updates-800fdc9bada4

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH 1/5] arm64: dts: qcom: sc8280xp: Fix CCI3 interrupt

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

This was evidently wrong, as exemplified by the core failing to reset
at probe (which would be completed by the IRQ firing).

Fix it.

Fixes: 7cfa2e758bf4 ("arm64: dts: qcom: sc8280xp: camss: Add CCI definitions")
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 225233a37a4fd9f3d65735915c0338a993a322d1..18b5cb441f955f7a91204376e05536b203f3e28b 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -4292,7 +4292,7 @@ cci3: cci@ac4d000 {
 			compatible = "qcom,sc8280xp-cci", "qcom,msm8996-cci";
 			reg = <0 0x0ac4d000 0 0x1000>;
 
-			interrupts = <GIC_SPI 650 IRQ_TYPE_EDGE_RISING>;
+			interrupts = <GIC_SPI 771 IRQ_TYPE_EDGE_RISING>;
 
 			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
 				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,

-- 
2.51.0

[PATCH 2/5] dt-bindings: i2c: qcom-cci: Allow operating-points-v2

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

An OPP table is necessary to express combined voltage and frequency
requirements for the CCI hw block.

Allow passing one, without requiring its presence.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
index 73144473b9b24e574bfc6bd7d8908f2f3895e087..1bb9a70661a944c1bdc01d336475952221450dba 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
@@ -54,6 +54,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  operating-points-v2: true
+
   power-domains:
     maxItems: 1
 

-- 
2.51.0

[PATCH 3/5] i2c: qcom-cci: Drop single-line wrappers

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The CCI clock en/disable functions simply call bulk_ops, remove them.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index a3afa11a71a10dbb720ee9acb566991fe55b98a0..74fedfdec3ae4e034ec4d946179e963c783b5923 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -466,21 +466,12 @@ static const struct i2c_algorithm cci_algo = {
 	.functionality = cci_func,
 };
 
-static int cci_enable_clocks(struct cci *cci)
-{
-	return clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
-}
-
-static void cci_disable_clocks(struct cci *cci)
-{
-	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
-}
-
 static int __maybe_unused cci_suspend_runtime(struct device *dev)
 {
 	struct cci *cci = dev_get_drvdata(dev);
 
-	cci_disable_clocks(cci);
+	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
+
 	return 0;
 }
 
@@ -489,11 +480,12 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
 	struct cci *cci = dev_get_drvdata(dev);
 	int ret;
 
-	ret = cci_enable_clocks(cci);
+	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
 	if (ret)
 		return ret;
 
 	cci_init(cci);
+
 	return 0;
 }
 
@@ -592,7 +584,7 @@ static int cci_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
 	cci->nclocks = ret;
 
-	ret = cci_enable_clocks(cci);
+	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
 	if (ret < 0)
 		return ret;
 
@@ -651,7 +643,7 @@ static int cci_probe(struct platform_device *pdev)
 error:
 	disable_irq(cci->irq);
 disable_clocks:
-	cci_disable_clocks(cci);
+	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
 
 	return ret;
 }

-- 
2.51.0

[PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The CCI clock has voltage requirements, which need to be described
through an OPP table.

The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
(which is a value common across all SoCs), since it's not possible to
reach the required timings with the default 19.2 MHz rate.

Address both issues by introducing an OPP table and using it to vote
for the faster rate.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 74fedfdec3ae4e034ec4d946179e963c783b5923..d6192e2a5e3bc4d908cba594d1910a41f3a41e9c 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 
 #define CCI_HW_VERSION				0x0
@@ -121,6 +122,7 @@ struct cci_data {
 	struct i2c_adapter_quirks quirks;
 	u16 queue_size[NUM_QUEUES];
 	struct hw_params params[3];
+	bool fast_mode_plus_supported;
 };
 
 struct cci {
@@ -466,9 +468,22 @@ static const struct i2c_algorithm cci_algo = {
 	.functionality = cci_func,
 };
 
+static unsigned long cci_desired_clk_rate(struct cci *cci)
+{
+	if (cci->data->fast_mode_plus_supported)
+		return 37500000ULL;
+
+	return 19200000ULL;
+}
+
 static int __maybe_unused cci_suspend_runtime(struct device *dev)
 {
 	struct cci *cci = dev_get_drvdata(dev);
+	int ret;
+
+	ret = dev_pm_opp_set_rate(dev, 0);
+	if (ret)
+		return ret;
 
 	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
 
@@ -484,6 +499,10 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = dev_pm_opp_set_rate(dev, cci_desired_clk_rate(cci));
+	if (ret)
+		return ret;
+
 	cci_init(cci);
 
 	return 0;
@@ -588,6 +607,19 @@ static int cci_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	ret = devm_pm_opp_set_clkname(dev, "cci");
+	if (ret)
+		return ret;
+
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "invalid OPP table in device tree\n");
+
+	ret = dev_pm_opp_set_rate(dev, cci_desired_clk_rate(cci));
+	if (ret)
+		return ret;
+
 	/* Interrupt */
 
 	ret = platform_get_irq(pdev, 0);
@@ -775,6 +807,7 @@ static const struct cci_data cci_v2_data = {
 		.trdhld = 3,
 		.tsp = 3
 	},
+	.fast_mode_plus_supported = true,
 };
 
 static const struct of_device_id cci_dt_match[] = {

-- 
2.51.0

[PATCH 5/5] arm64: dts: qcom: sc8280xp: Add OPP table for CCI hosts

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The CCI hosts have both frequency and voltage requirements (which
happen to be common across instances on a given SoC, at least so far).

Express them by introducing an OPP table and linking it to the hosts.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 18b5cb441f955f7a91204376e05536b203f3e28b..c396186317d49f411d7162771a358563329a02a4 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -391,6 +391,15 @@ memory@80000000 {
 		reg = <0x0 0x80000000 0x0 0x0>;
 	};
 
+	cci_opp_table: opp-table-cci {
+		compatible = "operating-points-v2";
+
+		opp-37500000 {
+			opp-hz = /bits/ 64 <37500000>;
+			required-opps = <&rpmhpd_opp_low_svs>;
+		};
+	};
+
 	cpu0_opp_table: opp-table-cpu0 {
 		compatible = "operating-points-v2";
 		opp-shared;
@@ -4181,6 +4190,7 @@ cci0: cci@ac4a000 {
 				      "cpas_ahb",
 				      "cci";
 
+			operating-points-v2 = <&cci_opp_table>;
 			power-domains = <&camcc TITAN_TOP_GDSC>;
 
 			pinctrl-0 = <&cci0_default>;
@@ -4222,6 +4232,7 @@ cci1: cci@ac4b000 {
 				      "cpas_ahb",
 				      "cci";
 
+			operating-points-v2 = <&cci_opp_table>;
 			power-domains = <&camcc TITAN_TOP_GDSC>;
 
 			pinctrl-0 = <&cci1_default>;
@@ -4262,6 +4273,8 @@ cci2: cci@ac4c000 {
 				      "slow_ahb_src",
 				      "cpas_ahb",
 				      "cci";
+
+			operating-points-v2 = <&cci_opp_table>;
 			power-domains = <&camcc TITAN_TOP_GDSC>;
 
 			pinctrl-0 = <&cci2_default>;
@@ -4303,6 +4316,7 @@ cci3: cci@ac4d000 {
 				      "cpas_ahb",
 				      "cci";
 
+			operating-points-v2 = <&cci_opp_table>;
 			power-domains = <&camcc TITAN_TOP_GDSC>;
 
 			pinctrl-0 = <&cci3_default>;

-- 
2.51.0

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Fix CCI3 interrupt

[Bryan O'Donoghue]

On 04/09/2025 15:31, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> This was evidently wrong, as exemplified by the core failing to reset
> at probe (which would be completed by the IRQ firing).
> 
> Fix it.
> 
> Fixes: 7cfa2e758bf4 ("arm64: dts: qcom: sc8280xp: camss: Add CCI definitions")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 225233a37a4fd9f3d65735915c0338a993a322d1..18b5cb441f955f7a91204376e05536b203f3e28b 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -4292,7 +4292,7 @@ cci3: cci@ac4d000 {
>   			compatible = "qcom,sc8280xp-cci", "qcom,msm8996-cci";
>   			reg = <0 0x0ac4d000 0 0x1000>;
>   
> -			interrupts = <GIC_SPI 650 IRQ_TYPE_EDGE_RISING>;
> +			interrupts = <GIC_SPI 771 IRQ_TYPE_EDGE_RISING>;
>   
>   			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
>   				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

On 9/4/25 4:31 PM, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI clock has voltage requirements, which need to be described
> through an OPP table.
> 
> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> (which is a value common across all SoCs), since it's not possible to
> reach the required timings with the default 19.2 MHz rate.
> 
> Address both issues by introducing an OPP table and using it to vote
> for the faster rate.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 74fedfdec3ae4e034ec4d946179e963c783b5923..d6192e2a5e3bc4d908cba594d1910a41f3a41e9c 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  
>  #define CCI_HW_VERSION				0x0
> @@ -121,6 +122,7 @@ struct cci_data {
>  	struct i2c_adapter_quirks quirks;
>  	u16 queue_size[NUM_QUEUES];
>  	struct hw_params params[3];
> +	bool fast_mode_plus_supported;
>  };
>  
>  struct cci {
> @@ -466,9 +468,22 @@ static const struct i2c_algorithm cci_algo = {
>  	.functionality = cci_func,
>  };
>  
> +static unsigned long cci_desired_clk_rate(struct cci *cci)
> +{
> +	if (cci->data->fast_mode_plus_supported)
> +		return 37500000ULL;
> +
> +	return 19200000ULL;

Well this is embarrassing

ULL -> UL

Konrad

Re: [PATCH 2/5] dt-bindings: i2c: qcom-cci: Allow operating-points-v2

[Bryan O'Donoghue]

On 04/09/2025 15:31, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> An OPP table is necessary to express combined voltage and frequency
> requirements for the CCI hw block.
> 
> Allow passing one, without requiring its presence.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> index 73144473b9b24e574bfc6bd7d8908f2f3895e087..1bb9a70661a944c1bdc01d336475952221450dba 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> @@ -54,6 +54,8 @@ properties:
>     interrupts:
>       maxItems: 1
>   
> +  operating-points-v2: true
> +
>     power-domains:
>       maxItems: 1
>   
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 3/5] i2c: qcom-cci: Drop single-line wrappers

[Bryan O'Donoghue]

On 04/09/2025 15:31, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI clock en/disable functions simply call bulk_ops, remove them.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index a3afa11a71a10dbb720ee9acb566991fe55b98a0..74fedfdec3ae4e034ec4d946179e963c783b5923 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -466,21 +466,12 @@ static const struct i2c_algorithm cci_algo = {
>   	.functionality = cci_func,
>   };
>   
> -static int cci_enable_clocks(struct cci *cci)
> -{
> -	return clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
> -}
> -
> -static void cci_disable_clocks(struct cci *cci)
> -{
> -	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
> -}
> -
>   static int __maybe_unused cci_suspend_runtime(struct device *dev)
>   {
>   	struct cci *cci = dev_get_drvdata(dev);
>   
> -	cci_disable_clocks(cci);
> +	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
> +
>   	return 0;
>   }
>   
> @@ -489,11 +480,12 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
>   	struct cci *cci = dev_get_drvdata(dev);
>   	int ret;
>   
> -	ret = cci_enable_clocks(cci);
> +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
>   	if (ret)
>   		return ret;
>   
>   	cci_init(cci);
> +
>   	return 0;
>   }
>   
> @@ -592,7 +584,7 @@ static int cci_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
>   	cci->nclocks = ret;
>   
> -	ret = cci_enable_clocks(cci);
> +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clocks);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -651,7 +643,7 @@ static int cci_probe(struct platform_device *pdev)
>   error:
>   	disable_irq(cci->irq);
>   disable_clocks:
> -	cci_disable_clocks(cci);
> +	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
>   
>   	return ret;
>   }
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Bryan O'Donoghue]

On 04/09/2025 15:31, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI clock has voltage requirements, which need to be described
> through an OPP table.
> 
> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> (which is a value common across all SoCs), since it's not possible to
> reach the required timings with the default 19.2 MHz rate.
> 
> Address both issues by introducing an OPP table and using it to vote
> for the faster rate.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 74fedfdec3ae4e034ec4d946179e963c783b5923..d6192e2a5e3bc4d908cba594d1910a41f3a41e9c 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -10,6 +10,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>   #include <linux/pm_runtime.h>
>   
>   #define CCI_HW_VERSION				0x0
> @@ -121,6 +122,7 @@ struct cci_data {
>   	struct i2c_adapter_quirks quirks;
>   	u16 queue_size[NUM_QUEUES];
>   	struct hw_params params[3];
> +	bool fast_mode_plus_supported;

that is a very long name for a flag

>   };
>   
>   struct cci {
> @@ -466,9 +468,22 @@ static const struct i2c_algorithm cci_algo = {
>   	.functionality = cci_func,
>   };
>   
> +static unsigned long cci_desired_clk_rate(struct cci *cci)
> +{
> +	if (cci->data->fast_mode_plus_supported)
> +		return 37500000ULL;
> +
> +	return 19200000ULL;

what's 32 bits between friends ?

> +}
> +
>   static int __maybe_unused cci_suspend_runtime(struct device *dev)
>   {
>   	struct cci *cci = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(dev, 0);
> +	if (ret)
> +		return ret;
>   
>   	clk_bulk_disable_unprepare(cci->nclocks, cci->clocks);
>   
> @@ -484,6 +499,10 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> +	ret = dev_pm_opp_set_rate(dev, cci_desired_clk_rate(cci));
> +	if (ret)
> +		return ret;
> +
>   	cci_init(cci);
>   
>   	return 0;
> @@ -588,6 +607,19 @@ static int cci_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = devm_pm_opp_set_clkname(dev, "cci");
> +	if (ret)
> +		return ret;
> +
> +	/* OPP table is optional */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "invalid OPP table in device tree\n");
> +
> +	ret = dev_pm_opp_set_rate(dev, cci_desired_clk_rate(cci));
> +	if (ret)
> +		return ret;
> +
>   	/* Interrupt */
>   
>   	ret = platform_get_irq(pdev, 0);
> @@ -775,6 +807,7 @@ static const struct cci_data cci_v2_data = {
>   		.trdhld = 3,
>   		.tsp = 3
>   	},
> +	.fast_mode_plus_supported = true,
>   };
>   
>   static const struct of_device_id cci_dt_match[] = {
> 

LGTM

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 5/5] arm64: dts: qcom: sc8280xp: Add OPP table for CCI hosts

[Bryan O'Donoghue]

On 04/09/2025 15:31, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI hosts have both frequency and voltage requirements (which
> happen to be common across instances on a given SoC, at least so far).
> 
> Express them by introducing an OPP table and linking it to the hosts.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 18b5cb441f955f7a91204376e05536b203f3e28b..c396186317d49f411d7162771a358563329a02a4 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -391,6 +391,15 @@ memory@80000000 {
>   		reg = <0x0 0x80000000 0x0 0x0>;
>   	};
>   
> +	cci_opp_table: opp-table-cci {
> +		compatible = "operating-points-v2";
> +
> +		opp-37500000 {
> +			opp-hz = /bits/ 64 <37500000>;
> +			required-opps = <&rpmhpd_opp_low_svs>;
> +		};
> +	};
> +
>   	cpu0_opp_table: opp-table-cpu0 {
>   		compatible = "operating-points-v2";
>   		opp-shared;
> @@ -4181,6 +4190,7 @@ cci0: cci@ac4a000 {
>   				      "cpas_ahb",
>   				      "cci";
>   
> +			operating-points-v2 = <&cci_opp_table>;
>   			power-domains = <&camcc TITAN_TOP_GDSC>;
>   
>   			pinctrl-0 = <&cci0_default>;
> @@ -4222,6 +4232,7 @@ cci1: cci@ac4b000 {
>   				      "cpas_ahb",
>   				      "cci";
>   
> +			operating-points-v2 = <&cci_opp_table>;
>   			power-domains = <&camcc TITAN_TOP_GDSC>;
>   
>   			pinctrl-0 = <&cci1_default>;
> @@ -4262,6 +4273,8 @@ cci2: cci@ac4c000 {
>   				      "slow_ahb_src",
>   				      "cpas_ahb",
>   				      "cci";
> +
> +			operating-points-v2 = <&cci_opp_table>;
>   			power-domains = <&camcc TITAN_TOP_GDSC>;
>   
>   			pinctrl-0 = <&cci2_default>;
> @@ -4303,6 +4316,7 @@ cci3: cci@ac4d000 {
>   				      "cpas_ahb",
>   				      "cci";
>   
> +			operating-points-v2 = <&cci_opp_table>;
>   			power-domains = <&camcc TITAN_TOP_GDSC>;
>   
>   			pinctrl-0 = <&cci3_default>;
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 3/5] i2c: qcom-cci: Drop single-line wrappers

[Loic Poulain]

On Thu, Sep 4, 2025 at 4:31 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> The CCI clock en/disable functions simply call bulk_ops, remove them.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

Re: [PATCH 2/5] dt-bindings: i2c: qcom-cci: Allow operating-points-v2

[Rob Herring (Arm)]

On Thu, 04 Sep 2025 16:31:21 +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> An OPP table is necessary to express combined voltage and frequency
> requirements for the CCI hw block.
> 
> Allow passing one, without requiring its presence.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Stephan Gerhold]

On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI clock has voltage requirements, which need to be described
> through an OPP table.
> 
> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> (which is a value common across all SoCs), since it's not possible to
> reach the required timings with the default 19.2 MHz rate.
> 
> Address both issues by introducing an OPP table and using it to vote
> for the faster rate.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Using an OPP table for a single static rate that remains the same over
the whole lifetime of the driver feels like overkill to me. Couldn't you
just put the "required-opps" directly into the device node so that it is
automatically applied when the device goes in/out of runtime suspend?

And since you need to make DT additions anyway, couldn't you just use
"assigned-clock-rates" to avoid the need for a driver patch entirely? We
use that for e.g. USB clocks as well.

Thanks,
Stephan

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

On 9/8/25 10:36 AM, Stephan Gerhold wrote:
> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> The CCI clock has voltage requirements, which need to be described
>> through an OPP table.
>>
>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
>> (which is a value common across all SoCs), since it's not possible to
>> reach the required timings with the default 19.2 MHz rate.
>>
>> Address both issues by introducing an OPP table and using it to vote
>> for the faster rate.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Using an OPP table for a single static rate that remains the same over
> the whole lifetime of the driver feels like overkill to me. Couldn't you
> just put the "required-opps" directly into the device node so that it is
> automatically applied when the device goes in/out of runtime suspend?
> 
> And since you need to make DT additions anyway, couldn't you just use
> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
> use that for e.g. USB clocks as well.

This is futureproofing, in case someone invents FastMode++ with a higher
dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
clock which would (marginally) decrease power consumption

Konrad

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Stephan Gerhold]

On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
> > On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>
> >> The CCI clock has voltage requirements, which need to be described
> >> through an OPP table.
> >>
> >> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> >> (which is a value common across all SoCs), since it's not possible to
> >> reach the required timings with the default 19.2 MHz rate.
> >>
> >> Address both issues by introducing an OPP table and using it to vote
> >> for the faster rate.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > 
> > Using an OPP table for a single static rate that remains the same over
> > the whole lifetime of the driver feels like overkill to me. Couldn't you
> > just put the "required-opps" directly into the device node so that it is
> > automatically applied when the device goes in/out of runtime suspend?
> > 
> > And since you need to make DT additions anyway, couldn't you just use
> > "assigned-clock-rates" to avoid the need for a driver patch entirely? We
> > use that for e.g. USB clocks as well.
> 
> This is futureproofing, in case someone invents FastMode++ with a higher
> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
> clock which would (marginally) decrease power consumption
> 

If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
then I would expect a separate entry for 19.2 MHz in the OPP table of
PATCH 5/5? The DT is unrelated to what functionality you implement in
the driver, and that would make the OPP table look less useless. :-)

Thanks,
Stephan

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

On 9/8/25 10:46 AM, Stephan Gerhold wrote:
> On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
>> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
>>> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>
>>>> The CCI clock has voltage requirements, which need to be described
>>>> through an OPP table.
>>>>
>>>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
>>>> (which is a value common across all SoCs), since it's not possible to
>>>> reach the required timings with the default 19.2 MHz rate.
>>>>
>>>> Address both issues by introducing an OPP table and using it to vote
>>>> for the faster rate.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> Using an OPP table for a single static rate that remains the same over
>>> the whole lifetime of the driver feels like overkill to me. Couldn't you
>>> just put the "required-opps" directly into the device node so that it is
>>> automatically applied when the device goes in/out of runtime suspend?
>>>
>>> And since you need to make DT additions anyway, couldn't you just use
>>> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
>>> use that for e.g. USB clocks as well.
>>
>> This is futureproofing, in case someone invents FastMode++ with a higher
>> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
>> clock which would (marginally) decrease power consumption
>>
> 
> If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
> then I would expect a separate entry for 19.2 MHz in the OPP table of
> PATCH 5/5? The DT is unrelated to what functionality you implement in
> the driver, and that would make the OPP table look less useless. :-)

The frequency plan for 8280 does not recommend any rate != 37.5 MHz

For x1e80100 however, the lovsvs_d1 corner is recommended to be 30
(yes, thirty) MHz, sourced from CAM_PLL8 for $reasons

Konrad

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Stephan Gerhold]

On Mon, Sep 08, 2025 at 11:49:52AM +0200, Konrad Dybcio wrote:
> On 9/8/25 10:46 AM, Stephan Gerhold wrote:
> > On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
> >> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
> >>> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
> >>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>
> >>>> The CCI clock has voltage requirements, which need to be described
> >>>> through an OPP table.
> >>>>
> >>>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> >>>> (which is a value common across all SoCs), since it's not possible to
> >>>> reach the required timings with the default 19.2 MHz rate.
> >>>>
> >>>> Address both issues by introducing an OPP table and using it to vote
> >>>> for the faster rate.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>
> >>> Using an OPP table for a single static rate that remains the same over
> >>> the whole lifetime of the driver feels like overkill to me. Couldn't you
> >>> just put the "required-opps" directly into the device node so that it is
> >>> automatically applied when the device goes in/out of runtime suspend?
> >>>
> >>> And since you need to make DT additions anyway, couldn't you just use
> >>> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
> >>> use that for e.g. USB clocks as well.
> >>
> >> This is futureproofing, in case someone invents FastMode++ with a higher
> >> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
> >> clock which would (marginally) decrease power consumption
> >>
> > 
> > If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
> > then I would expect a separate entry for 19.2 MHz in the OPP table of
> > PATCH 5/5? The DT is unrelated to what functionality you implement in
> > the driver, and that would make the OPP table look less useless. :-)
> 
> The frequency plan for 8280 does not recommend any rate != 37.5 MHz
> 
> For x1e80100 however, the lovsvs_d1 corner is recommended to be 30
> (yes, thirty) MHz, sourced from CAM_PLL8 for $reasons
> 

The 37.5 MHz rate still exists on X1E I presume, or are you saying we
need more changes to support those odd 30 MHz?

Personally, I'm not fully convinced there is ever going to be a use case
of someone using a "non-standard" frequency. Even if "FastMode++" is
invented most devices will probably want to use it. And the voltage
requirements we're currently talking about here like "low svs" during
camera use cases are kind of negligible compared to others too.

But I'm fine with either solution, just wanted to mention it. :D

Thanks,
Stephan

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

On 9/8/25 11:57 AM, Stephan Gerhold wrote:
> On Mon, Sep 08, 2025 at 11:49:52AM +0200, Konrad Dybcio wrote:
>> On 9/8/25 10:46 AM, Stephan Gerhold wrote:
>>> On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
>>>> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
>>>>> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> The CCI clock has voltage requirements, which need to be described
>>>>>> through an OPP table.
>>>>>>
>>>>>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
>>>>>> (which is a value common across all SoCs), since it's not possible to
>>>>>> reach the required timings with the default 19.2 MHz rate.
>>>>>>
>>>>>> Address both issues by introducing an OPP table and using it to vote
>>>>>> for the faster rate.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>
>>>>> Using an OPP table for a single static rate that remains the same over
>>>>> the whole lifetime of the driver feels like overkill to me. Couldn't you
>>>>> just put the "required-opps" directly into the device node so that it is
>>>>> automatically applied when the device goes in/out of runtime suspend?
>>>>>
>>>>> And since you need to make DT additions anyway, couldn't you just use
>>>>> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
>>>>> use that for e.g. USB clocks as well.
>>>>
>>>> This is futureproofing, in case someone invents FastMode++ with a higher
>>>> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
>>>> clock which would (marginally) decrease power consumption
>>>>
>>>
>>> If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
>>> then I would expect a separate entry for 19.2 MHz in the OPP table of
>>> PATCH 5/5? The DT is unrelated to what functionality you implement in
>>> the driver, and that would make the OPP table look less useless. :-)
>>
>> The frequency plan for 8280 does not recommend any rate != 37.5 MHz
>>
>> For x1e80100 however, the lovsvs_d1 corner is recommended to be 30
>> (yes, thirty) MHz, sourced from CAM_PLL8 for $reasons
>>
> 
> The 37.5 MHz rate still exists on X1E I presume, or are you saying we
> need more changes to support those odd 30 MHz?

Yes, any corner over lowsvs_d1 is 37.5, sourced from cam_pll0

> Personally, I'm not fully convinced there is ever going to be a use case
> of someone using a "non-standard" frequency. Even if "FastMode++" is
> invented most devices will probably want to use it.

Not really, there's no reason to make your i2c bus go fastfastfast if
the devices on the other end can't cope with it

> And the voltage
> requirements we're currently talking about here like "low svs" during
> camera use cases are kind of negligible compared to others too.

Again, this is an I2C controller that seems to be associated with
cameras.. No image data has to actually be processed for the
communications to take place and you can attach any odd device

Konrad

> 
> But I'm fine with either solution, just wanted to mention it. :D
> 
> Thanks,
> Stephan

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Stephan Gerhold]

On Mon, Sep 08, 2025 at 12:00:13PM +0200, Konrad Dybcio wrote:
> On 9/8/25 11:57 AM, Stephan Gerhold wrote:
> > On Mon, Sep 08, 2025 at 11:49:52AM +0200, Konrad Dybcio wrote:
> >> On 9/8/25 10:46 AM, Stephan Gerhold wrote:
> >>> On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
> >>>> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
> >>>>> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
> >>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>
> >>>>>> The CCI clock has voltage requirements, which need to be described
> >>>>>> through an OPP table.
> >>>>>>
> >>>>>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
> >>>>>> (which is a value common across all SoCs), since it's not possible to
> >>>>>> reach the required timings with the default 19.2 MHz rate.
> >>>>>>
> >>>>>> Address both issues by introducing an OPP table and using it to vote
> >>>>>> for the faster rate.
> >>>>>>
> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>
> >>>>> Using an OPP table for a single static rate that remains the same over
> >>>>> the whole lifetime of the driver feels like overkill to me. Couldn't you
> >>>>> just put the "required-opps" directly into the device node so that it is
> >>>>> automatically applied when the device goes in/out of runtime suspend?
> >>>>>
> >>>>> And since you need to make DT additions anyway, couldn't you just use
> >>>>> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
> >>>>> use that for e.g. USB clocks as well.
> >>>>
> >>>> This is futureproofing, in case someone invents FastMode++ with a higher
> >>>> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
> >>>> clock which would (marginally) decrease power consumption
> >>>>
> >>>
> >>> If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
> >>> then I would expect a separate entry for 19.2 MHz in the OPP table of
> >>> PATCH 5/5? The DT is unrelated to what functionality you implement in
> >>> the driver, and that would make the OPP table look less useless. :-)
> >>
> >> The frequency plan for 8280 does not recommend any rate != 37.5 MHz
> >>
> >> For x1e80100 however, the lovsvs_d1 corner is recommended to be 30
> >> (yes, thirty) MHz, sourced from CAM_PLL8 for $reasons
> >>
> > 
> > The 37.5 MHz rate still exists on X1E I presume, or are you saying we
> > need more changes to support those odd 30 MHz?
> 
> Yes, any corner over lowsvs_d1 is 37.5, sourced from cam_pll0
> 
> > Personally, I'm not fully convinced there is ever going to be a use case
> > of someone using a "non-standard" frequency. Even if "FastMode++" is
> > invented most devices will probably want to use it.
> 
> Not really, there's no reason to make your i2c bus go fastfastfast if
> the devices on the other end can't cope with it
> 
> > And the voltage
> > requirements we're currently talking about here like "low svs" during
> > camera use cases are kind of negligible compared to others too.
> 
> Again, this is an I2C controller that seems to be associated with
> cameras.. No image data has to actually be processed for the
> communications to take place and you can attach any odd device
> 

My point is: In the unlikely case that support for faster I2C speeds is
added in newer SoCs, I think you'd just get a new "standard" base clock
frequency, add a new cci_data struct with adjusted timings and everyone
will use that (even for the lower I2C speeds). I doubt anyone will
bother adjusting and validating this for just one "corner"/voltage level
less. There are much more effective targets for power optimization than
the few bytes of I2C communication. :-)

Thanks,
Stephan

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Konrad Dybcio]

On 9/8/25 12:09 PM, Stephan Gerhold wrote:
> On Mon, Sep 08, 2025 at 12:00:13PM +0200, Konrad Dybcio wrote:
>> On 9/8/25 11:57 AM, Stephan Gerhold wrote:
>>> On Mon, Sep 08, 2025 at 11:49:52AM +0200, Konrad Dybcio wrote:
>>>> On 9/8/25 10:46 AM, Stephan Gerhold wrote:
>>>>> On Mon, Sep 08, 2025 at 10:43:50AM +0200, Konrad Dybcio wrote:
>>>>>> On 9/8/25 10:36 AM, Stephan Gerhold wrote:
>>>>>>> On Thu, Sep 04, 2025 at 04:31:23PM +0200, Konrad Dybcio wrote:
>>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>
>>>>>>>> The CCI clock has voltage requirements, which need to be described
>>>>>>>> through an OPP table.
>>>>>>>>
>>>>>>>> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
>>>>>>>> (which is a value common across all SoCs), since it's not possible to
>>>>>>>> reach the required timings with the default 19.2 MHz rate.
>>>>>>>>
>>>>>>>> Address both issues by introducing an OPP table and using it to vote
>>>>>>>> for the faster rate.
>>>>>>>>
>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>
>>>>>>> Using an OPP table for a single static rate that remains the same over
>>>>>>> the whole lifetime of the driver feels like overkill to me. Couldn't you
>>>>>>> just put the "required-opps" directly into the device node so that it is
>>>>>>> automatically applied when the device goes in/out of runtime suspend?
>>>>>>>
>>>>>>> And since you need to make DT additions anyway, couldn't you just use
>>>>>>> "assigned-clock-rates" to avoid the need for a driver patch entirely? We
>>>>>>> use that for e.g. USB clocks as well.
>>>>>>
>>>>>> This is futureproofing, in case someone invents FastMode++ with a higher
>>>>>> dvfs requirement or for when the driver adds presets for a 19.2 MHz CCI
>>>>>> clock which would (marginally) decrease power consumption
>>>>>>
>>>>>
>>>>> If 19.2 MHz CCI clock is feasible and has lower voltage requirements,
>>>>> then I would expect a separate entry for 19.2 MHz in the OPP table of
>>>>> PATCH 5/5? The DT is unrelated to what functionality you implement in
>>>>> the driver, and that would make the OPP table look less useless. :-)
>>>>
>>>> The frequency plan for 8280 does not recommend any rate != 37.5 MHz
>>>>
>>>> For x1e80100 however, the lovsvs_d1 corner is recommended to be 30
>>>> (yes, thirty) MHz, sourced from CAM_PLL8 for $reasons
>>>>
>>>
>>> The 37.5 MHz rate still exists on X1E I presume, or are you saying we
>>> need more changes to support those odd 30 MHz?
>>
>> Yes, any corner over lowsvs_d1 is 37.5, sourced from cam_pll0
>>
>>> Personally, I'm not fully convinced there is ever going to be a use case
>>> of someone using a "non-standard" frequency. Even if "FastMode++" is
>>> invented most devices will probably want to use it.
>>
>> Not really, there's no reason to make your i2c bus go fastfastfast if
>> the devices on the other end can't cope with it
>>
>>> And the voltage
>>> requirements we're currently talking about here like "low svs" during
>>> camera use cases are kind of negligible compared to others too.
>>
>> Again, this is an I2C controller that seems to be associated with
>> cameras.. No image data has to actually be processed for the
>> communications to take place and you can attach any odd device
>>
> 
> My point is: In the unlikely case that support for faster I2C speeds is
> added in newer SoCs, I think you'd just get a new "standard" base clock
> frequency, add a new cci_data struct with adjusted timings and everyone
> will use that (even for the lower I2C speeds). I doubt anyone will
> bother adjusting and validating this for just one "corner"/voltage level
> less. There are much more effective targets for power optimization than
> the few bytes of I2C communication. :-)

There are OEMs that customize some of the timings (e.g. Sony) and I would
expect SV efforts to at least cover the recommended frequency plan (which
like for x1e sometimes contains >1 frequency)..

That said, I do agree with you it's a rather niche/improbable corner of
the SoC to worry about.. But using required-opps in dt-bindings for
non-trivial hardware (CCI is a little bit more "fun" than the today's
state of the upstream driver makes it seem) simply feels like asking for
trouble (i.e. a ""real need"" for an opp table will probably come around
one day, so I don't think the additional 10 lines of code to support it
are that much of an issue either).

Konrad

Re: [PATCH 0/5] Qualcomm CCI I2C clock requirements enforcement

[Mukesh Savaliya]

On 9/4/2025 8:01 PM, Konrad Dybcio wrote:
> The hardware requires the faster of the two (37.5 MHz as opposed to
> 19.2 MHz) clock rates to hit the required timings for I2C Fast+ Mode.
Should mention "source clock rates" to not confuse with derived 
frequency/timings for Fast+ mode.>
> Additionally, the magic presets for electrical tuning registers on SoCs
> supporting that faster mode ("cci_v2" in the driver) are calculated
are/is calculated> based on that faster frequency.
> 
> Moreover, while its unlikely that it would ever exhibit as an issue
> given CCI is a slow & tiny core, we do need to express a minimal voltage
> level for any given clock rate, which is where the (optional -
> backwards compat) OPP table addition comes in.
> 
> This series helps ensure all these requirements are met.
> 
> Patch 1 is a related but independent fix, can be picked right away
> Patch 5 can be functionally merged as-is, but depends on patch 2 for
> bindings
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> Konrad Dybcio (5):
>        arm64: dts: qcom: sc8280xp: Fix CCI3 interrupt
>        dt-bindings: i2c: qcom-cci: Allow operating-points-v2
>        i2c: qcom-cci: Drop single-line wrappers
>        i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements
>        arm64: dts: qcom: sc8280xp: Add OPP table for CCI hosts
> 
>   .../devicetree/bindings/i2c/qcom,i2c-cci.yaml      |  2 +
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi             | 16 +++++++-
>   drivers/i2c/busses/i2c-qcom-cci.c                  | 45 +++++++++++++++++-----
>   3 files changed, 52 insertions(+), 11 deletions(-)
> ---
> base-commit: 4ac65880ebca1b68495bd8704263b26c050ac010
> change-id: 20250904-topic-cci_updates-800fdc9bada4
> 
> Best regards,

Re: [PATCH 4/5] i2c: qcom-cci: Add OPP table support and enforce FAST_PLUS requirements

[Mukesh Savaliya]

On 9/4/2025 8:01 PM, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The CCI clock has voltage requirements, which need to be described
> through an OPP table.
> 
> The 1 MHz FAST_PLUS mode requires the CCI core clock runs at 37,5 MHz
Typo: 37.5 MHz> (which is a value common across all SoCs), since it's 
not possible to
> reach the required timings with the default 19.2 MHz rate.
> 
> Address both issues by introducing an OPP table and using it to vote
> for the faster rate.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
[...]