Devicetree
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock
@ 2026-10-16 17:06 Atanas Filipov
  2026-10-16 17:06 ` [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider Atanas Filipov
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Atanas Filipov @ 2026-10-16 17:06 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Dmitry Baryshkov, Konrad Dybcio, Gjorgji Rosikopulos,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel,
	atanas.filipov

CAM_CC_CAMNOC_AXI_CLK on SM8250 is shared by several independent
camera blocks, such as CAMSS and the JPEG encoder. Each of them needs a
minimum camera NoC AXI rate, and setting the rate of the shared clock
directly from each consumer driver means the last writer wins.

Following the review of v2, this version drops the custom CAMNOC
interconnect provider. The camera clock controller instead exports the
camera NoC AXI clock through the common icc-clk support
(qcom_cc_desc.icc_hws), as gcc-ipq9574 and gcc-ipq5210 do. Consumers
request bandwidth on one master/slave path, the interconnect core
aggregates the requests, and the clock runs at the highest requested
rate. A consumer can do this through opp-peak-kBps in its OPP table,
so the vote follows its own core clock OPP.

The icc-clk path is backed by the RCG CAM_CC_CAMNOC_AXI_CLK_SRC rather
than the branch CAM_CC_CAMNOC_AXI_CLK. icc-clk enables its clock while
the provider registers, when TITAN_TOP_GDSC is still off, and enabling
the branch at that point fails with "cam_cc_camnoc_axi_clk status stuck
at 'off'". The RCG can be enabled at probe; the branch has
CLK_SET_RATE_PARENT, so the rate ends up on the RCG either way, and each
consumer keeps enabling the branch itself after powering the GDSC.

The JPEG encoder and CAMSS will use this path; their changes will be
posted separately.

Tested on RB5 (QRB5165): the camera NoC AXI clock follows the
aggregated vote, votes drop to zero when the consumer is idle, and no
clock warnings are seen at boot.

Changes since v2:
- Drop the CAMNOC interconnect provider driver, its binding and DT
  node; export the camera NoC AXI clock through icc-clk in
  camcc-sm8250 instead (Krzysztof Kozlowski, Konrad Dybcio)
- Drop the per-master CAMNOC port IDs; icc-clk provides one
  master/slave pair per clock

Atanas Filipov (3):
  dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock
  arm64: dts: qcom: sm8250: add camcc interconnect cells

 .../devicetree/bindings/clock/qcom,camcc-sm8250.yaml |  4 ++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi                 |  1 +
 drivers/clk/qcom/camcc-sm8250.c                      | 12 ++++++++++++
 include/dt-bindings/interconnect/qcom,sm8250.h       |  3 +++
 4 files changed, 20 insertions(+)


base-commit: b810335a0699d1810683796b0e3ffe8827ddfe6c
-- 
2.34.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-16 17:06 [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock Atanas Filipov
@ 2026-10-16 17:06 ` Atanas Filipov
  2026-10-20  7:12   ` Krzysztof Kozlowski
  2026-10-16 17:07 ` [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock Atanas Filipov
  2026-10-16 17:07 ` [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells Atanas Filipov
  2 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-16 17:06 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Dmitry Baryshkov, Konrad Dybcio, Gjorgji Rosikopulos,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel,
	atanas.filipov

CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
rate directly from each consumer means the last writer wins.

Add #interconnect-cells so the camera clock controller can act as an
icc-clk provider, and add the camera NoC AXI master/slave IDs.
Consumers request bandwidth on this path and the interconnect core
aggregates the requests.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 .../devicetree/bindings/clock/qcom,camcc-sm8250.yaml          | 4 ++++
 include/dt-bindings/interconnect/qcom,sm8250.h                | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,camcc-sm8250.yaml b/Documentation/devicetree/bindings/clock/qcom,camcc-sm8250.yaml
index 5c3ff37ec0d75..9cc7e77ec89ae 100644
--- a/Documentation/devicetree/bindings/clock/qcom,camcc-sm8250.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,camcc-sm8250.yaml
@@ -36,6 +36,9 @@ properties:
       - const: bi_tcxo_ao
       - const: sleep_clk
 
+  '#interconnect-cells':
+    const: 1
+
   power-domains:
     items:
       - description: MMCX power domain
@@ -70,5 +73,6 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      #interconnect-cells = <1>;
     };
 ...
diff --git a/include/dt-bindings/interconnect/qcom,sm8250.h b/include/dt-bindings/interconnect/qcom,sm8250.h
index 2a656c02df4b1..96dc43e063cf0 100644
--- a/include/dt-bindings/interconnect/qcom,sm8250.h
+++ b/include/dt-bindings/interconnect/qcom,sm8250.h
@@ -173,4 +173,7 @@
 #define SLAVE_QUP_CORE_1		4
 #define SLAVE_QUP_CORE_2		5
 
+#define MASTER_CAMCC_CAMNOC_AXI		0
+#define SLAVE_CAMCC_CAMNOC_AXI		1
+
 #endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock
  2026-10-16 17:06 [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock Atanas Filipov
  2026-10-16 17:06 ` [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider Atanas Filipov
@ 2026-10-16 17:07 ` Atanas Filipov
  2026-10-16 17:22   ` sashiko-bot
  2026-10-19 14:49   ` Dmitry Baryshkov
  2026-10-16 17:07 ` [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells Atanas Filipov
  2 siblings, 2 replies; 22+ messages in thread
From: Atanas Filipov @ 2026-10-16 17:07 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Dmitry Baryshkov, Konrad Dybcio, Gjorgji Rosikopulos,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel,
	atanas.filipov

CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
rate directly from each consumer means the last writer wins.

Register the clock as an icc-clk path through the common icc_hws
support, as gcc-ipq9574 does. The interconnect core aggregates all
requests and the clock runs at the highest requested rate.

Use the RCG rather than the branch: icc-clk enables the clock at
registration, when TITAN_TOP_GDSC is still off. Consumers enable the
branch themselves.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 drivers/clk/qcom/camcc-sm8250.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c
index 21e9423676216..84a91103e5bef 100644
--- a/drivers/clk/qcom/camcc-sm8250.c
+++ b/drivers/clk/qcom/camcc-sm8250.c
@@ -4,11 +4,13 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/interconnect-provider.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,camcc-sm8250.h>
+#include <dt-bindings/interconnect/qcom,sm8250.h>
 
 #include "clk-alpha-pll.h"
 #include "clk-branch.h"
@@ -2405,6 +2407,12 @@ static const struct regmap_config cam_cc_sm8250_regmap_config = {
 	.fast_io = true,
 };
 
+#define SM8250_CAMCC_ICC_ID		8250	/* some unique value */
+
+static const struct qcom_icc_hws_data cam_cc_sm8250_icc_hws[] = {
+	{ MASTER_CAMCC_CAMNOC_AXI, SLAVE_CAMCC_CAMNOC_AXI, CAM_CC_CAMNOC_AXI_CLK_SRC },
+};
+
 static const struct qcom_cc_desc cam_cc_sm8250_desc = {
 	.config = &cam_cc_sm8250_regmap_config,
 	.clks = cam_cc_sm8250_clocks,
@@ -2413,6 +2421,9 @@ static const struct qcom_cc_desc cam_cc_sm8250_desc = {
 	.num_resets = ARRAY_SIZE(cam_cc_sm8250_resets),
 	.gdscs = cam_cc_sm8250_gdscs,
 	.num_gdscs = ARRAY_SIZE(cam_cc_sm8250_gdscs),
+	.icc_hws = cam_cc_sm8250_icc_hws,
+	.num_icc_hws = ARRAY_SIZE(cam_cc_sm8250_icc_hws),
+	.icc_first_node_id = SM8250_CAMCC_ICC_ID,
 };
 
 static const struct of_device_id cam_cc_sm8250_match_table[] = {
@@ -2443,6 +2454,7 @@ static struct platform_driver cam_cc_sm8250_driver = {
 	.driver = {
 		.name = "cam_cc-sm8250",
 		.of_match_table = cam_cc_sm8250_match_table,
+		.sync_state = icc_sync_state,
 	},
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-16 17:06 [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock Atanas Filipov
  2026-10-16 17:06 ` [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider Atanas Filipov
  2026-10-16 17:07 ` [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock Atanas Filipov
@ 2026-10-16 17:07 ` Atanas Filipov
  2026-10-19 14:50   ` Dmitry Baryshkov
  2 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-16 17:07 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Dmitry Baryshkov, Konrad Dybcio, Gjorgji Rosikopulos,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel,
	atanas.filipov

Add #interconnect-cells to the camera clock controller so camera
consumers can request camera NoC AXI bandwidth through the
interconnect framework.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index f975c2183269a..e923c74dc1435 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -4675,6 +4675,7 @@ camcc: clock-controller@ad00000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
+			#interconnect-cells = <1>;
 		};
 
 		mdss: display-subsystem@ae00000 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock
  2026-10-16 17:07 ` [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock Atanas Filipov
@ 2026-10-16 17:22   ` sashiko-bot
  2026-10-19 14:49   ` Dmitry Baryshkov
  1 sibling, 0 replies; 22+ messages in thread
From: sashiko-bot @ 2026-10-16 17:22 UTC (permalink / raw)
  To: Atanas Filipov; +Cc: linux-clk, devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: camcc-sm8250: premature icc_sync_state on old DT causes system hang
--

commit efec91c2d7f6b65bf67f582b282617ccb26d996e
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock

CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
rate directly from each consumer means the last writer wins.

Register the clock as an icc-clk path through the common icc_hws
support, as gcc-ipq9574 does. The interconnect core aggregates all
requests and the clock runs at the highest requested rate.

Use the RCG rather than the branch: icc-clk enables the clock at
registration, when TITAN_TOP_GDSC is still off. Consumers enable the
branch themselves.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

> diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c
> index 21e9423676216..84a91103e5bef 100644
> --- a/drivers/clk/qcom/camcc-sm8250.c
> +++ b/drivers/clk/qcom/camcc-sm8250.c

[ ... ]

> @@ -2443,6 +2454,7 @@ static struct platform_driver cam_cc_sm8250_driver = {
>  	.driver = {
>  		.name = "cam_cc-sm8250",
>  		.of_match_table = cam_cc_sm8250_match_table,
> +		.sync_state = icc_sync_state,

[Severity: High]
Could this cause premature state synchronization when booting with an older
Device Tree?

On older DTs where the camcc node lacks the #interconnect-cells property,
the interconnect core skips counting it during initialization:

drivers/interconnect/core.c:of_count_icc_providers() {
	...
	if (of_property_present(child, "#interconnect-cells") &&
	    likely(!of_match_node(ignore_list, child)))
		count++;
	...
}

Because of this, providers_count only reflects the other interconnect
providers. However, since the driver unconditionally registers
icc_sync_state, the driver core will still execute it once the device
probes.

When this executes, the extra call increments the static count in
icc_sync_state without a corresponding +1 in providers_count:

drivers/interconnect/core.c:icc_sync_state() {
	...
	count++;

	if (count < providers_count)
		return;
	...
}

Will this cause the interconnect core to reach providers_count prematurely,
forcing synced_state to true and dropping bandwidth requests on all other
system interconnects before their consumers have probed?

>  	},
>  };
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016170701.1504338-1-atanas.filipov@oss.qualcomm.com?part=2

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock
  2026-10-16 17:07 ` [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock Atanas Filipov
  2026-10-16 17:22   ` sashiko-bot
@ 2026-10-19 14:49   ` Dmitry Baryshkov
  2026-10-20  8:20     ` Atanas Filipov
  1 sibling, 1 reply; 22+ messages in thread
From: Dmitry Baryshkov @ 2026-10-19 14:49 UTC (permalink / raw)
  To: Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On Fri, Oct 16, 2026 at 08:07:00PM +0300, Atanas Filipov wrote:
> CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
> rate directly from each consumer means the last writer wins.
> 
> Register the clock as an icc-clk path through the common icc_hws
> support, as gcc-ipq9574 does. The interconnect core aggregates all
> requests and the clock runs at the highest requested rate.
> 
> Use the RCG rather than the branch: icc-clk enables the clock at
> registration, when TITAN_TOP_GDSC is still off. Consumers enable the
> branch themselves.
> 
> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
> ---
>  drivers/clk/qcom/camcc-sm8250.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

This is a nice approach. Several quesitons: if there are multiple
clients, should the clock track the max of the rates a sum of the rates?
Also, this clock needs a certain level on a rail. Are you going to add
the opp table to the clock controller or to the devices which use the
ICC?

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-16 17:07 ` [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells Atanas Filipov
@ 2026-10-19 14:50   ` Dmitry Baryshkov
  2026-10-20  8:26     ` Atanas Filipov
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Baryshkov @ 2026-10-19 14:50 UTC (permalink / raw)
  To: Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On Fri, Oct 16, 2026 at 08:07:01PM +0300, Atanas Filipov wrote:
> Add #interconnect-cells to the camera clock controller so camera
> consumers can request camera NoC AXI bandwidth through the
> interconnect framework.

I guess currently we have only one consumer, camss. Could you please
include patches converting it to the bus voting? Be sure to retain
compatibility with older DTs, which don't have ICC provided by the camcc
node.

> 
> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index f975c2183269a..e923c74dc1435 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -4675,6 +4675,7 @@ camcc: clock-controller@ad00000 {
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  			#power-domain-cells = <1>;
> +			#interconnect-cells = <1>;
>  		};
>  
>  		mdss: display-subsystem@ae00000 {
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-16 17:06 ` [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider Atanas Filipov
@ 2026-10-20  7:12   ` Krzysztof Kozlowski
  2026-10-20  7:40     ` Gjorgji Rosikopulos (Consultant)
  2026-10-20 12:44     ` Atanas Filipov
  0 siblings, 2 replies; 22+ messages in thread
From: Krzysztof Kozlowski @ 2026-10-20  7:12 UTC (permalink / raw)
  To: Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Dmitry Baryshkov,
	Konrad Dybcio, Gjorgji Rosikopulos, linux-arm-msm, linux-clk,
	linux-pm, devicetree, linux-kernel

On Fri, Oct 16, 2026 at 08:06:59PM +0300, Atanas Filipov wrote:
> CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its

Clock is not an interconnect, although AXI suggests this could be
something with bus, but it seems you solve driver problem with
interconnect description.

If you only need to solve "last writer" problem, why not fixing the
clock framework to accept the votes? Otherwise this should have
description why it is an interconnect.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-20  7:12   ` Krzysztof Kozlowski
@ 2026-10-20  7:40     ` Gjorgji Rosikopulos (Consultant)
  2026-10-20 12:44     ` Atanas Filipov
  1 sibling, 0 replies; 22+ messages in thread
From: Gjorgji Rosikopulos (Consultant) @ 2026-10-20  7:40 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Dmitry Baryshkov,
	Konrad Dybcio, linux-arm-msm, linux-clk, linux-pm, devicetree,
	linux-kernel

Hi Krzysztof,

On 10/20/2026 10:12 AM, Krzysztof Kozlowski wrote:
> On Fri, Oct 16, 2026 at 08:06:59PM +0300, Atanas Filipov wrote:
>> CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
> 
> Clock is not an interconnect, although AXI suggests this could be
> something with bus, but it seems you solve driver problem with
> interconnect description.
> 
> If you only need to solve "last writer" problem, why not fixing the
> clock framework to accept the votes? Otherwise this should have
> description why it is an interconnect.

There are two approaches which we have looked in to resolving the sharing of the AXI clock.

1. The drivers newer touch this clock but vote on new icc camera which controls this clock (The v1 and v2) patches.

2. Use icc-clk in the extension in clock framework same approach as: 
https://lore.kernel.org/all/20240430064214.2030013-6-quic_varada@quicinc.com/
This is already merged and i believe resolved similar issue. 

3. Which you are proposing to have clock framework to accept the votes.

Before we jump and implement the v4 it will be really good to have common agreement on which direction to go.

Regards,
~Gjorgji

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock
  2026-10-19 14:49   ` Dmitry Baryshkov
@ 2026-10-20  8:20     ` Atanas Filipov
  0 siblings, 0 replies; 22+ messages in thread
From: Atanas Filipov @ 2026-10-20  8:20 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On 10/19/2026 5:49 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 16, 2026 at 08:07:00PM +0300, Atanas Filipov wrote:
>> CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
>> rate directly from each consumer means the last writer wins.
>>
>> Register the clock as an icc-clk path through the common icc_hws
>> support, as gcc-ipq9574 does. The interconnect core aggregates all
>> requests and the clock runs at the highest requested rate.
>>
>> Use the RCG rather than the branch: icc-clk enables the clock at
>> registration, when TITAN_TOP_GDSC is still off. Consumers enable the
>> branch themselves.
>>
>> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
>> ---
>>   drivers/clk/qcom/camcc-sm8250.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
> 
> This is a nice approach. Several quesitons: if there are multiple
> clients, should the clock track the max of the rates a sum of the rates?

Max. icc-clk sets the clock from the aggregated peak bandwidth, and
icc_std_aggregate() takes the maximum of the peak requests. Each client
asks for the bandwidth it needs, so the highest request covers all of
them. A sum would overestimate the clock.

> Also, this clock needs a certain level on a rail. Are you going to add
> the opp table to the clock controller or to the devices which use the
> ICC?
> 
To the devices. Each consumer describes its own needs in its OPP
table: every entry carries the core clock rate, the camera NoC
bandwidth and the required rail level, and dev_pm_opp_set_opp()
applies all of them at once. Dropping the OPP on idle removes the
votes. For example, the JPEG encoder (to be posted separately):

         interconnects = <&camcc MASTER_CAMCC_CAMNOC_AXI
                          &camcc SLAVE_CAMCC_CAMNOC_AXI>;
         interconnect-names = "camnoc";
         operating-points-v2 = <&jpeg_opp_table>;

         jpeg_opp_table: opp-table {
                 compatible = "operating-points-v2";

                 opp-100000000 {
                         opp-hz = /bits/ 64 <100000000>;
                         opp-peak-kBps = <25000>;
                         required-opps = <&rpmhpd_opp_min_svs>;
                 };
                 ...
                 opp-600000000 {
                         opp-hz = /bits/ 64 <600000000>;
                         opp-peak-kBps = <400000>;
                         required-opps = <&rpmhpd_opp_turbo>;
                 };
         };

The consumer asks for the bandwidth it needs, independent of how many
rate levels the camera NoC clock has on a given platform; icc-clk
rounds the request to a rate the clock supports.

Best regards,
Atanas

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-19 14:50   ` Dmitry Baryshkov
@ 2026-10-20  8:26     ` Atanas Filipov
  2026-10-20  9:45       ` Dmitry Baryshkov
  0 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-20  8:26 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On 10/19/2026 5:50 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 16, 2026 at 08:07:01PM +0300, Atanas Filipov wrote:
>> Add #interconnect-cells to the camera clock controller so camera
>> consumers can request camera NoC AXI bandwidth through the
>> interconnect framework.
> 
> I guess currently we have only one consumer, camss. Could you please
> include patches converting it to the bus voting? Be sure to retain
> compatibility with older DTs, which don't have ICC provided by the camcc
> node.
>

Before converting CAMSS I would like to agree on the approach, since it
cannot simply follow JPEG. In either case the camcc path would be
optional, so older DTs keep working as before.

OPP is per device, and CAMSS is a single device containing several VFEs, 
each with its own core clock and its own load. If CAMSS used 
dev_pm_opp_set_opp() on its device like JPEG does, every VFE would 
overwrite the clock and bandwidth votes of the others, which is the same 
last-writer-wins problem in a different place.

I see two ways to keep the VFEs independent:

1. One OPP table in the CAMSS node, used only as a lookup. Each VFE
    finds the OPP for its own required rate, sets its own clock and
    votes the bandwidth from that OPP on its own ICC path. The driver
    requests a separate path per VFE from the same "interconnects"
    entry, so DT describes the hardware connection once and the
    interconnect core aggregates the VFE requests with the other
    consumers. The rail level is not applied by the OPP core in this
    case, so CAMSS keeps handling it as today.

2. An OPP table per VFE, attached to the per-VFE power domain device
    that CAMSS already creates, so each VFE calls dev_pm_opp_set_opp()
    on its own device. This applies the rail level as well, but the
    virtual device shares the CAMSS of_node, so it would pick up all
    CAMSS interconnect paths and need indexed OPP tables in DT, which I
    have not seen used for such devices.

Which one would you prefer, or do you see a better way?

Best regards,
Atanas

>>
>> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8250.dtsi | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> index f975c2183269a..e923c74dc1435 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> @@ -4675,6 +4675,7 @@ camcc: clock-controller@ad00000 {
>>   			#clock-cells = <1>;
>>   			#reset-cells = <1>;
>>   			#power-domain-cells = <1>;
>> +			#interconnect-cells = <1>;
>>   		};
>>   
>>   		mdss: display-subsystem@ae00000 {
>> -- 
>> 2.34.1
>>
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-20  8:26     ` Atanas Filipov
@ 2026-10-20  9:45       ` Dmitry Baryshkov
  2026-10-20 12:49         ` Atanas Filipov
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Baryshkov @ 2026-10-20  9:45 UTC (permalink / raw)
  To: Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On Tue, Oct 20, 2026 at 11:26:42AM +0300, Atanas Filipov wrote:
> On 10/19/2026 5:50 PM, Dmitry Baryshkov wrote:
> > On Fri, Oct 16, 2026 at 08:07:01PM +0300, Atanas Filipov wrote:
> > > Add #interconnect-cells to the camera clock controller so camera
> > > consumers can request camera NoC AXI bandwidth through the
> > > interconnect framework.
> > 
> > I guess currently we have only one consumer, camss. Could you please
> > include patches converting it to the bus voting? Be sure to retain
> > compatibility with older DTs, which don't have ICC provided by the camcc
> > node.
> > 
> 
> Before converting CAMSS I would like to agree on the approach, since it
> cannot simply follow JPEG. In either case the camcc path would be
> optional, so older DTs keep working as before.

How does the current camss vote on the clock?  JPEG is not (yet) merged,
so we can't reference it here. The order in which the patches can land
would be: icc-clk, camss using the (optional) interconnect, JPEG on top.

Please note Krzysztof's comments.

> OPP is per device, and CAMSS is a single device containing several VFEs,
> each with its own core clock and its own load. If CAMSS used
> dev_pm_opp_set_opp() on its device like JPEG does, every VFE would overwrite
> the clock and bandwidth votes of the others, which is the same
> last-writer-wins problem in a different place.

For this series just convert the existing approach as is. We have two
distinct questions here:
 - Letting camss and JPEG (and others) to vote independently on the
   clock
 - Letting camss correctly handle per-VFE votes

This series must address the first question only.

> I see two ways to keep the VFEs independent:
> 
> 1. One OPP table in the CAMSS node, used only as a lookup. Each VFE
>    finds the OPP for its own required rate, sets its own clock and
>    votes the bandwidth from that OPP on its own ICC path. The driver
>    requests a separate path per VFE from the same "interconnects"
>    entry, so DT describes the hardware connection once and the
>    interconnect core aggregates the VFE requests with the other
>    consumers. The rail level is not applied by the OPP core in this
>    case, so CAMSS keeps handling it as today.
> 
> 2. An OPP table per VFE, attached to the per-VFE power domain device
>    that CAMSS already creates, so each VFE calls dev_pm_opp_set_opp()
>    on its own device. This applies the rail level as well, but the
>    virtual device shares the CAMSS of_node, so it would pick up all
>    CAMSS interconnect paths and need indexed OPP tables in DT, which I
>    have not seen used for such devices.
> 
> Which one would you prefer, or do you see a better way?

3. Add minimal VFE nodes and let camss use those. Converge towards the
split VFE (IFE) device tree.

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-20  7:12   ` Krzysztof Kozlowski
  2026-10-20  7:40     ` Gjorgji Rosikopulos (Consultant)
@ 2026-10-20 12:44     ` Atanas Filipov
  2026-10-28 18:10       ` Atanas Filipov
  1 sibling, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-20 12:44 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Dmitry Baryshkov,
	Konrad Dybcio, Gjorgji Rosikopulos, linux-arm-msm, linux-clk,
	linux-pm, devicetree, linux-kernel

On 10/20/2026 10:12 AM, Krzysztof Kozlowski wrote:
> On Fri, Oct 16, 2026 at 08:06:59PM +0300, Atanas Filipov wrote:
>> CAM_CC_CAMNOC_AXI_CLK is shared by several camera blocks. Setting its
> 
> Clock is not an interconnect, although AXI suggests this could be
> something with bus, but it seems you solve driver problem with
> interconnect description.
> 
Agreed, the problem is in the drivers. Today each CAMSS VFE calls
clk_set_rate() on camnoc_axi based on its own load only. The last
caller wins, and the rate is never lowered after streaming stops.

> If you only need to solve "last writer" problem, why not fixing the
> clock framework to accept the votes? Otherwise this should have
> description why it is an interconnect.
> 
> Best regards,
> Krzysztof

I checked clk_set_min_rate() and clk_set_rate_range(). The clock core
takes the highest minimum of all users, but when a user drops its
request, the core only clamps the last requested rate to the new
range. The RCG keeps that rate and does not go to the lowest allowed
one, so the clock stays high. Fixing this needs a change in the clock
core.

icc-clk already does this: the interconnect core aggregates the
requests and icc-clk is the only one that sets the clock rate. The
camnoc AXI clock drives the camera NoC, the bus between the camera
blocks and the MMSS NoC, so a bandwidth request matches the hardware.
gcc-ipq9574 uses icc-clk for its NoC clocks in the same way.

Which direction do you prefer? If icc-clk stays, I will add this
explanation to the commit message in v4.

Best regards,
Atanas



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-20  9:45       ` Dmitry Baryshkov
@ 2026-10-20 12:49         ` Atanas Filipov
  2026-10-28 17:45           ` Atanas Filipov
  0 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-20 12:49 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On 10/20/2026 12:45 PM, Dmitry Baryshkov wrote:
> On Tue, Oct 20, 2026 at 11:26:42AM +0300, Atanas Filipov wrote:
>> On 10/19/2026 5:50 PM, Dmitry Baryshkov wrote:
>>> On Fri, Oct 16, 2026 at 08:07:01PM +0300, Atanas Filipov wrote:
>>>> Add #interconnect-cells to the camera clock controller so camera
>>>> consumers can request camera NoC AXI bandwidth through the
>>>> interconnect framework.
>>>
>>> I guess currently we have only one consumer, camss. Could you please
>>> include patches converting it to the bus voting? Be sure to retain
>>> compatibility with older DTs, which don't have ICC provided by the camcc
>>> node.
>>>
>>
>> Before converting CAMSS I would like to agree on the approach, since it
>> cannot simply follow JPEG. In either case the camcc path would be
>> optional, so older DTs keep working as before.
> 
> How does the current camss vote on the clock?  JPEG is not (yet) merged,
> so we can't reference it here. The order in which the patches can land
> would be: icc-clk, camss using the (optional) interconnect, JPEG on top.

It does not vote. Each VFE calls clk_set_rate() directly from
vfe_set_clock_rates(), based on the pixel rate of its own lines only.

Where clk_set_rate() is used on camnoc_axi (SC7280, SC8280XP, SA8775P),
the clock is shared by all VFEs, but:

  - the last VFE to power up wins and may lower the rate under a VFE
    that is already streaming;
  - vfe_check_clock_rates() checks only the VFE's own needs and does not
    notice changes made by other VFEs;
  - vfe_put() never drops the rate.

On the remaining SoCs the camnoc clocks have no levels and are only
enabled. On SM8250 the levels sit on "camnoc_axi_src", which
vfe_match_clock_names() does not match, so they are never applied.

> 
> Please note Krzysztof's comments.

Yes. I will reply to Krzysztof directly in 1/3. The CAMSS behaviour
above is part of that answer: today every consumer sets the shared
camnoc rate directly and nothing aggregates the requests.

> 
>> OPP is per device, and CAMSS is a single device containing several VFEs,
>> each with its own core clock and its own load. If CAMSS used
>> dev_pm_opp_set_opp() on its device like JPEG does, every VFE would overwrite
>> the clock and bandwidth votes of the others, which is the same
>> last-writer-wins problem in a different place.
> 
> For this series just convert the existing approach as is. We have two
> distinct questions here:
>   - Letting camss and JPEG (and others) to vote independently on the
>     clock
>   - Letting camss correctly handle per-VFE votes
> 
> This series must address the first question only.
> 
>> I see two ways to keep the VFEs independent:
>>
>> 1. One OPP table in the CAMSS node, used only as a lookup. Each VFE
>>     finds the OPP for its own required rate, sets its own clock and
>>     votes the bandwidth from that OPP on its own ICC path. The driver
>>     requests a separate path per VFE from the same "interconnects"
>>     entry, so DT describes the hardware connection once and the
>>     interconnect core aggregates the VFE requests with the other
>>     consumers. The rail level is not applied by the OPP core in this
>>     case, so CAMSS keeps handling it as today.
>>
>> 2. An OPP table per VFE, attached to the per-VFE power domain device
>>     that CAMSS already creates, so each VFE calls dev_pm_opp_set_opp()
>>     on its own device. This applies the rail level as well, but the
>>     virtual device shares the CAMSS of_node, so it would pick up all
>>     CAMSS interconnect paths and need indexed OPP tables in DT, which I
>>     have not seen used for such devices.
>>
>> Which one would you prefer, or do you see a better way?
> 
> 3. Add minimal VFE nodes and let camss use those. Converge towards the
> split VFE (IFE) device tree.
> 

Agreed, I will take that up separately. To get it right the first
time, could you clarify what "minimal" covers:

  - child nodes of the camss node, or standalone nodes like the split
    CSIPHY?
  - which resources move into them: only the per-VFE clocks, power
    domain, OPP table and interconnect path, or reg and interrupts as
    well?

--
Best regards,
Atanas


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-20 12:49         ` Atanas Filipov
@ 2026-10-28 17:45           ` Atanas Filipov
  2026-10-29  1:41             ` Dmitry Baryshkov
  0 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-28 17:45 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: atanas.filipov, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Konrad Dybcio, Gjorgji Rosikopulos, linux-arm-msm, linux-clk,
	linux-pm, devicetree, linux-kernel

On 10/20/2026 3:49 PM, Atanas Filipov wrote:
> On 10/20/2026 12:45 PM, Dmitry Baryshkov wrote:
>> 3. Add minimal VFE nodes and let camss use those. Converge towards the
>> split VFE (IFE) device tree.
>
> Agreed, I will take that up separately. To get it right the first
> time, could you clarify what "minimal" covers:

After going through the options discussed so far, I think the VFE
child nodes you suggested are the right approach for the per-VFE
votes. Below is a concrete proposal. It is separate from this series,
which stays limited to the icc-clk conversion as you asked.

Why a VFE node is needed, not only convenient:

 - The VFEs need independent votes for three things: their own clock
   rate, the MMCX level that rate requires, and their own bandwidth.
   OPP is per device, so this needs one device per VFE.
 - VFE and VFE Lite have different rate tables (on SM8250 vfe0/vfe1
   350-720 MHz, vfe_lite0/vfe_lite1 400/480 MHz). A single
   frequency-keyed OPP table in the CAMSS node cannot describe both.
 - Today nothing scales MMCX with the VFE rates: camcc only applies
   its fixed required-opps, and CAMSS sets no performance state.

Proposed minimal node, one per VFE, as a child of the camss node:

	camss: camss@ac6a000 {
		...
		vfe0: vfe-0 {
			compatible = "qcom,sm8250-vfe";
			clocks = <&camcc CAM_CC_IFE_0_CLK>;
			clock-names = "core";
			power-domains = <&camcc IFE_0_GDSC>,
					<&rpmhpd RPMHPD_MMCX>;
			power-domain-names = "ife", "mmcx";
			interconnects = <&mmss_noc MASTER_CAMNOC_HF 0
					 &mc_virt SLAVE_EBI_CH0 0>,
					<&camcc MASTER_CAMCC_CAMNOC_AXI
					 &camcc SLAVE_CAMCC_CAMNOC_AXI>;
			interconnect-names = "hf", "camnoc_axi";
			operating-points-v2 = <&vfe_opp_table>;
		};
		...
	};

 - Only what is needed for the votes moves: the core clock, the
   power domains, the OPP table with required-opps, and the
   interconnect paths. reg, interrupts and the remaining clocks stay
   in the camss node for now.
 - VFE and VFE Lite get separate compatibles and separate OPP tables;
   VFE nodes of the same kind share one table.
 - CAMSS populates the children and uses each child device for
   dev_pm_opp_set_rate() and its ICC path. The OPP core then sets the
   clock and the MMCX level, genpd takes the highest level across the
   VFEs and the other MMCX consumers, and the interconnect core
   aggregates the bandwidth with the other camera consumers.
 - Without the child nodes CAMSS keeps its current behaviour, so older
   DTs keep working.

Child nodes keep the camss node as the single entry point and keep
the change small for existing DTs. reg and interrupts are not needed
for the votes, so I would leave them in the camss node; moving them
would only make the child nodes look complete without changing what
the driver can do.

Does this match what you had in mind?

--
Best regards,
Atanas

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-20 12:44     ` Atanas Filipov
@ 2026-10-28 18:10       ` Atanas Filipov
  2026-10-28 22:00         ` Bryan O'Donoghue
  0 siblings, 1 reply; 22+ messages in thread
From: Atanas Filipov @ 2026-10-28 18:10 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: atanas.filipov, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Konrad Dybcio, Georgi Djakov, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Abel Vesa, Jonathan Marek, Bryan O'Donoghue,
	Dmitry Baryshkov, Konrad Dybcio, Gjorgji Rosikopulos,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel

On 10/20/2026 3:44 PM, Atanas Filipov wrote:
> On Tue, Oct 20, 2026 at 09:12:03AM +0200, Krzysztof Kozlowski wrote:
>> If you only need to solve "last writer" problem, why not fixing the
>> clock framework to accept the votes? Otherwise this should have
>> description why it is an interconnect.

A follow-up on the second part, since the problem turned out to be
more than the last writer.

The last-writer problem between the CAMSS VFEs is now handled in CAMSS
itself, in the direction you and Bryan suggested: each VFE sets a
clk_set_rate_range() floor, and on power off drops it and sets the
lowest rate explicitly, so the clock no longer stays high:

https://lore.kernel.org/all/20261028132403.3505037-1-atanas.filipov@oss.qualcomm.com/

What clock votes cannot express is why I think the camera NoC is an
interconnect:

 - The camera NoC is a bus with several independent masters: the VFE
   write ports, JPEG and OPE. They share it on the way to the MMSS NoC,
   and the camnoc AXI clock sets its throughput.
 - The consumers know their bandwidth, not the NoC rate that serves it.
   Converting one to the other depends on the NoC width, which is a
   property of the bus, not of each consumer.
 - Masters running at the same time load the NoC by the sum of their
   traffic. Clock rate requests can only be aggregated by maximum; a
   sum of rates has no meaning, a sum of bandwidth does.
 - The rest of the camera DMA path, from the MMSS NoC to DDR, is
   already described with interconnects, and CAMSS votes bandwidth on
   it. The camera NoC is its first hop.

The interconnect core already sums the average bandwidth of all
requests. icc-clk currently derives the rate from the aggregated peak
only, so it does not use that sum yet; that is a limitation of the
icc-clk translation, not of describing the camera NoC as an
interconnect. It can be addressed in icc-clk by taking the average
into account together with the bus width.

Is this a valid reason to describe the camera NoC as an interconnect?

--
Best regards,
Atanas

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-28 18:10       ` Atanas Filipov
@ 2026-10-28 22:00         ` Bryan O'Donoghue
  2026-10-29  1:50           ` Dmitry Baryshkov
  2026-10-29  7:25           ` Gjorgji Rosikopulos (Consultant)
  0 siblings, 2 replies; 22+ messages in thread
From: Bryan O'Donoghue @ 2026-10-28 22:00 UTC (permalink / raw)
  To: Atanas Filipov, Krzysztof Kozlowski
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Dmitry Baryshkov, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On 28/10/2026 19:10, Atanas Filipov wrote:
> What clock votes cannot express is why I think the camera NoC is an
> interconnect:
> 
>   - The camera NoC is a bus with several independent masters: the VFE
>     write ports, JPEG and OPE. They share it on the way to the MMSS NoC,
>     and the camnoc AXI clock sets its throughput.

I've always conceptually viewed the CamNoc as just another NoC that 
happens to live inside of the TITAN_TOP_GDSC power-island.

I think the trouble here is the CAMNOC_AXI_CLK is just one clock, it's 
not interconnecting any two things at all.

Noc entries in DT usually look like

interconnects = <&gem_noc MASTER_AMPSS_M0 0
		 &config_noc SLAVE_VENUS_CFG 0>,
		<&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI_CH0 0>;
		interconnect-names = "cpu-cfg", "video-mem";

In the second case we're connecting an MMSS port to a DRAM port i.e. two 
completely separate producers/consumers and we have specific ports that 
RPM/RPMh is involved in managing.

Which is not the case for CamNoc - here really we have some shared 
clocks which we need to calculate across components within the Camera 
power-island.

In downstream the CPAs code does that.

- icc-clk would I think over-provision clocks
- CCF would as you've observed under-provision

- But a CPA's analog in upstream which components set/consume ?

That would work.

After all what we are doing here _is_ specific to how camera works. We 
don't really have many - any ? other examples in Qcom SoCs where we lump 
a number of different semi-independent components into one area - ICP, 
CDM, VFE, JPEG and ask them to share aggregated clocks - in a way that 
is specific to - guessing but - specific to how these components get 
modeled in silicon test benches.

My vote is to replicate CPAs in upstream with a CPA's icc provider driver.

The bandwidth calculations are already proven out in downstream and if 
we get it right - potentially its something downstream could switch to.

We could even use the interconnect API inside of the CPA's driver but, I 
think just doing votes via interconnect and icc_set_bw() into icc-clk in 
the drivers directly won't produce the same bandwidth calculations we 
have in downstream - which you may loathe downstream code but the clock 
calculations we'd assume are ~ a pretty good starting point.

So; make the CPAs driver an icc provider.

cpas_icc_set() could/should absorb the downstream cam_cpas_hw.c 
heuristic for calculating the aggregate clocks.

Its not represented in generic icc-clk right now and as we add in more 
clocks/SoCs this disparity will grow.

---
bod

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-28 17:45           ` Atanas Filipov
@ 2026-10-29  1:41             ` Dmitry Baryshkov
  2026-10-29  7:50               ` Atanas Filipov
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Baryshkov @ 2026-10-29  1:41 UTC (permalink / raw)
  To: Atanas Filipov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On Wed, Oct 28, 2026 at 07:45:23PM +0200, Atanas Filipov wrote:
> On 10/20/2026 3:49 PM, Atanas Filipov wrote:
> > On 10/20/2026 12:45 PM, Dmitry Baryshkov wrote:
> >> 3. Add minimal VFE nodes and let camss use those. Converge towards the
> >> split VFE (IFE) device tree.
> >
> > Agreed, I will take that up separately. To get it right the first
> > time, could you clarify what "minimal" covers:
> 
> After going through the options discussed so far, I think the VFE
> child nodes you suggested are the right approach for the per-VFE
> votes. Below is a concrete proposal. It is separate from this series,
> which stays limited to the icc-clk conversion as you asked.
> 
> Why a VFE node is needed, not only convenient:
> 
>  - The VFEs need independent votes for three things: their own clock
>    rate, the MMCX level that rate requires, and their own bandwidth.
>    OPP is per device, so this needs one device per VFE.
>  - VFE and VFE Lite have different rate tables (on SM8250 vfe0/vfe1
>    350-720 MHz, vfe_lite0/vfe_lite1 400/480 MHz). A single
>    frequency-keyed OPP table in the CAMSS node cannot describe both.
>  - Today nothing scales MMCX with the VFE rates: camcc only applies
>    its fixed required-opps, and CAMSS sets no performance state.
> 
> Proposed minimal node, one per VFE, as a child of the camss node:
> 
> 	camss: camss@ac6a000 {
> 		...
> 		vfe0: vfe-0 {
> 			compatible = "qcom,sm8250-vfe";
> 			clocks = <&camcc CAM_CC_IFE_0_CLK>;
> 			clock-names = "core";
> 			power-domains = <&camcc IFE_0_GDSC>,
> 					<&rpmhpd RPMHPD_MMCX>;
> 			power-domain-names = "ife", "mmcx";

If IFE_0_GDSC is an (indirect) child of MMCX, this is an overkill. You
should not need to cast votes on MMCX, IFE -> TITAN_TOP -> MMCX will
work for you.

> 			interconnects = <&mmss_noc MASTER_CAMNOC_HF 0
> 					 &mc_virt SLAVE_EBI_CH0 0>,
> 					<&camcc MASTER_CAMCC_CAMNOC_AXI
> 					 &camcc SLAVE_CAMCC_CAMNOC_AXI>;
> 			interconnect-names = "hf", "camnoc_axi";
> 			operating-points-v2 = <&vfe_opp_table>;
> 		};
> 		...
> 	};
> 
>  - Only what is needed for the votes moves: the core clock, the
>    power domains, the OPP table with required-opps, and the
>    interconnect paths. reg, interrupts and the remaining clocks stay
>    in the camss node for now.
>  - VFE and VFE Lite get separate compatibles and separate OPP tables;
>    VFE nodes of the same kind share one table.
>  - CAMSS populates the children and uses each child device for
>    dev_pm_opp_set_rate() and its ICC path. The OPP core then sets the
>    clock and the MMCX level, genpd takes the highest level across the
>    VFEs and the other MMCX consumers, and the interconnect core
>    aggregates the bandwidth with the other camera consumers.
>  - Without the child nodes CAMSS keeps its current behaviour, so older
>    DTs keep working.
> 
> Child nodes keep the camss node as the single entry point and keep
> the change small for existing DTs. reg and interrupts are not needed
> for the votes, so I would leave them in the camss node; moving them
> would only make the child nodes look complete without changing what
> the driver can do.
> 
> Does this match what you had in mind?

Yes

> 
> --
> Best regards,
> Atanas

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-28 22:00         ` Bryan O'Donoghue
@ 2026-10-29  1:50           ` Dmitry Baryshkov
  2026-10-29  9:57             ` Bryan O'Donoghue
  2026-10-29  7:25           ` Gjorgji Rosikopulos (Consultant)
  1 sibling, 1 reply; 22+ messages in thread
From: Dmitry Baryshkov @ 2026-10-29  1:50 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: Atanas Filipov, Krzysztof Kozlowski, Bjorn Andersson,
	Michael Turquette, Stephen Boyd, Konrad Dybcio, Georgi Djakov,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Abel Vesa,
	Jonathan Marek, Konrad Dybcio, Gjorgji Rosikopulos, linux-arm-msm,
	linux-clk, linux-pm, devicetree, linux-kernel

On Wed, Oct 28, 2026 at 11:00:29PM +0100, Bryan O'Donoghue wrote:
> On 28/10/2026 19:10, Atanas Filipov wrote:
> > What clock votes cannot express is why I think the camera NoC is an
> > interconnect:
> > 
> >   - The camera NoC is a bus with several independent masters: the VFE
> >     write ports, JPEG and OPE. They share it on the way to the MMSS NoC,
> >     and the camnoc AXI clock sets its throughput.
> 
> I've always conceptually viewed the CamNoc as just another NoC that happens
> to live inside of the TITAN_TOP_GDSC power-island.
> 
> I think the trouble here is the CAMNOC_AXI_CLK is just one clock, it's not
> interconnecting any two things at all.
> 
> Noc entries in DT usually look like
> 
> interconnects = <&gem_noc MASTER_AMPSS_M0 0
> 		 &config_noc SLAVE_VENUS_CFG 0>,
> 		<&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI_CH0 0>;
> 		interconnect-names = "cpu-cfg", "video-mem";
> 
> In the second case we're connecting an MMSS port to a DRAM port i.e. two
> completely separate producers/consumers and we have specific ports that
> RPM/RPMh is involved in managing.

RPM / RPMh are Qualcomm-specific abstractions. If you look underneath,
you'd see NoC clocks, being managed by the RPMh cores. We also have OSM
device, which describes how each of the cores / clusters is connected to
the L3 cache. On MSM8996 similarly there is a CBF device, which
describes connection between two core clusters.

From my PoV, CAMNOX_AXI, is just the same NoC clock, just not being
wired through RPM/RPMh. From this point of view the clock can be
described as as an interconnect, it gives correct abstraction: no matter
how the camera blocks are connected or how the connection between them
is managed in hardware / firmware / clock devices, there is a bus (aka
interconnect) between those camera blocks. If at some point the clock
becomes managed by RPMh, it won't (significantly) change the hardware
description

> 
> Which is not the case for CamNoc - here really we have some shared clocks
> which we need to calculate across components within the Camera power-island.
> 
> In downstream the CPAs code does that.
> 
> - icc-clk would I think over-provision clocks
> - CCF would as you've observed under-provision
> 
> - But a CPA's analog in upstream which components set/consume ?
> 
> That would work.
> 
> After all what we are doing here _is_ specific to how camera works. We don't
> really have many - any ? other examples in Qcom SoCs where we lump a number
> of different semi-independent components into one area - ICP, CDM, VFE, JPEG
> and ask them to share aggregated clocks - in a way that is specific to -
> guessing but - specific to how these components get modeled in silicon test
> benches.
> 
> My vote is to replicate CPAs in upstream with a CPA's icc provider driver.
> 
> The bandwidth calculations are already proven out in downstream and if we
> get it right - potentially its something downstream could switch to.
> 
> We could even use the interconnect API inside of the CPA's driver but, I
> think just doing votes via interconnect and icc_set_bw() into icc-clk in the
> drivers directly won't produce the same bandwidth calculations we have in
> downstream - which you may loathe downstream code but the clock calculations
> we'd assume are ~ a pretty good starting point.
> 
> So; make the CPAs driver an icc provider.
> 
> cpas_icc_set() could/should absorb the downstream cam_cpas_hw.c heuristic
> for calculating the aggregate clocks.
> 
> Its not represented in generic icc-clk right now and as we add in more
> clocks/SoCs this disparity will grow.
> 
> ---
> bod

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-28 22:00         ` Bryan O'Donoghue
  2026-10-29  1:50           ` Dmitry Baryshkov
@ 2026-10-29  7:25           ` Gjorgji Rosikopulos (Consultant)
  1 sibling, 0 replies; 22+ messages in thread
From: Gjorgji Rosikopulos (Consultant) @ 2026-10-29  7:25 UTC (permalink / raw)
  To: Bryan O'Donoghue, Atanas Filipov, Krzysztof Kozlowski,
	Dmitry Baryshkov
  Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Konrad Dybcio,
	Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Abel Vesa, Jonathan Marek, Dmitry Baryshkov, Konrad Dybcio,
	linux-arm-msm, linux-clk, linux-pm, devicetree, linux-kernel



On 10/29/2026 12:00 AM, Bryan O'Donoghue wrote:
> On 28/10/2026 19:10, Atanas Filipov wrote:
>> What clock votes cannot express is why I think the camera NoC is an
>> interconnect:
>>
>>   - The camera NoC is a bus with several independent masters: the VFE
>>     write ports, JPEG and OPE. They share it on the way to the MMSS NoC,
>>     and the camnoc AXI clock sets its throughput.
> 
> I've always conceptually viewed the CamNoc as just another NoC that 
> happens to live inside of the TITAN_TOP_GDSC power-island.
> 
> I think the trouble here is the CAMNOC_AXI_CLK is just one clock, it's 
> not interconnecting any two things at all.
> 
> Noc entries in DT usually look like
> 
> interconnects = <&gem_noc MASTER_AMPSS_M0 0
> 		 &config_noc SLAVE_VENUS_CFG 0>,
> 		<&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI_CH0 0>;
> 		interconnect-names = "cpu-cfg", "video-mem";
> 
> In the second case we're connecting an MMSS port to a DRAM port i.e. two 
> completely separate producers/consumers and we have specific ports that 
> RPM/RPMh is involved in managing.
> 
> Which is not the case for CamNoc - here really we have some shared 
> clocks which we need to calculate across components within the Camera 
> power-island.
> 
> In downstream the CPAs code does that.
> 
> - icc-clk would I think over-provision clocks
> - CCF would as you've observed under-provision
> 
> - But a CPA's analog in upstream which components set/consume ?
> 
> That would work.
> 
> After all what we are doing here _is_ specific to how camera works. We 
> don't really have many - any ? other examples in Qcom SoCs where we lump 
> a number of different semi-independent components into one area - ICP, 
> CDM, VFE, JPEG and ask them to share aggregated clocks - in a way that 
> is specific to - guessing but - specific to how these components get 
> modeled in silicon test benches.
> 
> My vote is to replicate CPAs in upstream with a CPA's icc provider driver.
> 
> The bandwidth calculations are already proven out in downstream and if 
> we get it right - potentially its something downstream could switch to.
> 
> We could even use the interconnect API inside of the CPA's driver but, I 
> think just doing votes via interconnect and icc_set_bw() into icc-clk in 
> the drivers directly won't produce the same bandwidth calculations we 
> have in downstream - which you may loathe downstream code but the clock 
> calculations we'd assume are ~ a pretty good starting point.
> 
> So; make the CPAs driver an icc provider.
> 
> cpas_icc_set() could/should absorb the downstream cam_cpas_hw.c 
> heuristic for calculating the aggregate clocks.
> 
> Its not represented in generic icc-clk right now and as we add in more 
> clocks/SoCs this disparity will grow.

Just to add, there is a CAMNOC interconnect within the camera subsystem
that handles arbitration between the online and offline engines and routes
the data to MMNOC.

It also provides QoS settings, which are currently configured by the VFE driver
as part of set_qos(). Therefore, I agree with Bryan that, if implemented properly,
the downstream driver could switch to this approach in the future.

This raises the question: should we update CAMSS to use the CAMNOC provider and stop
voting directly on MMNOC? CAMNOC could then vote on MNOC based on the aggregated bandwidth
requests from the individual engines.

Regards,
~Gjorgji

> 
> ---
> bod


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells
  2026-10-29  1:41             ` Dmitry Baryshkov
@ 2026-10-29  7:50               ` Atanas Filipov
  0 siblings, 0 replies; 22+ messages in thread
From: Atanas Filipov @ 2026-10-29  7:50 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: atanas.filipov, Krzysztof Kozlowski, Bjorn Andersson,
	Michael Turquette, Stephen Boyd, Konrad Dybcio, Georgi Djakov,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Abel Vesa,
	Jonathan Marek, Bryan O'Donoghue, Konrad Dybcio,
	Gjorgji Rosikopulos, linux-arm-msm, linux-clk, linux-pm,
	devicetree, linux-kernel

On 10/29/2026 3:41 AM, Dmitry Baryshkov wrote:
> On Wed, Oct 28, 2026 at 07:45:23PM +0200, Atanas Filipov wrote:
>> 			power-domains = <&camcc IFE_0_GDSC>,
>> 					<&rpmhpd RPMHPD_MMCX>;
>> 			power-domain-names = "ife", "mmcx";
>
> If IFE_0_GDSC is an (indirect) child of MMCX, this is an overkill. You
> should not need to cast votes on MMCX, IFE -> TITAN_TOP -> MMCX will
> work for you.

Agreed, IFE_0_GDSC is a subdomain of TITAN_TOP_GDSC, which is a
subdomain of MMCX. I will keep only the IFE GDSC in the VFE node.

>> Does this match what you had in mind?
>
> Yes

Thanks.

--
Best regards,
Atanas

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider
  2026-10-29  1:50           ` Dmitry Baryshkov
@ 2026-10-29  9:57             ` Bryan O'Donoghue
  0 siblings, 0 replies; 22+ messages in thread
From: Bryan O'Donoghue @ 2026-10-29  9:57 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Atanas Filipov, Krzysztof Kozlowski, Bjorn Andersson,
	Michael Turquette, Stephen Boyd, Konrad Dybcio, Georgi Djakov,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Abel Vesa,
	Jonathan Marek, Konrad Dybcio, Gjorgji Rosikopulos, linux-arm-msm,
	linux-clk, linux-pm, devicetree, linux-kernel

On 29/10/2026 01:50, Dmitry Baryshkov wrote:
>  From my PoV, CAMNOX_AXI, is just the same NoC clock, just not being
> wired through RPM/RPMh. From this point of view the clock can be
> described as as an interconnect, it gives correct abstraction: no matter
> how the camera blocks are connected or how the connection between them
> is managed in hardware / firmware / clock devices, there is a bus (aka
> interconnect) between those camera blocks. If at some point the clock
> becomes managed by RPMh, it won't (significantly) change the hardware
> description

yeah I think it should be an icc provider - perhaps interconnect nodes 
make sense, not for one clock but for a number of clocks and components 
in the system JPEG, ICP voting on the bandwidth/clocks.

TBH my point here is more we need to soak up the CPAs clock logic in an 
putative cpas_icc_set() routine since I'm... as sure as I can be that 
icc_set_bw() on its own and CCF can't really capture that logic.

---
bod

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2026-10-29  9:57 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-16 17:06 [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock Atanas Filipov
2026-10-16 17:06 ` [PATCH v3 1/3] dt-bindings: clock: qcom,camcc-sm8250: add interconnect provider Atanas Filipov
2026-10-20  7:12   ` Krzysztof Kozlowski
2026-10-20  7:40     ` Gjorgji Rosikopulos (Consultant)
2026-10-20 12:44     ` Atanas Filipov
2026-10-28 18:10       ` Atanas Filipov
2026-10-28 22:00         ` Bryan O'Donoghue
2026-10-29  1:50           ` Dmitry Baryshkov
2026-10-29  9:57             ` Bryan O'Donoghue
2026-10-29  7:25           ` Gjorgji Rosikopulos (Consultant)
2026-10-16 17:07 ` [PATCH v3 2/3] clk: qcom: camcc-sm8250: use icc-clk for camera NoC AXI clock Atanas Filipov
2026-10-16 17:22   ` sashiko-bot
2026-10-19 14:49   ` Dmitry Baryshkov
2026-10-20  8:20     ` Atanas Filipov
2026-10-16 17:07 ` [PATCH v3 3/3] arm64: dts: qcom: sm8250: add camcc interconnect cells Atanas Filipov
2026-10-19 14:50   ` Dmitry Baryshkov
2026-10-20  8:26     ` Atanas Filipov
2026-10-20  9:45       ` Dmitry Baryshkov
2026-10-20 12:49         ` Atanas Filipov
2026-10-28 17:45           ` Atanas Filipov
2026-10-29  1:41             ` Dmitry Baryshkov
2026-10-29  7:50               ` Atanas Filipov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox