[PATCH 0/6] Add support for GPUCC, CAMCC and VIDEOCC on Qualcomm QCS8300 platform

[PATCH 0/6] Add support for GPUCC, CAMCC and VIDEOCC on Qualcomm QCS8300 platform

[Imran Shaik]

This patch series add support for GPUCC, CAMCC and VIDEOCC on Qualcomm
QCS8300 platform.

Please note that this series is dependent on [1] and [2], which adds support
for QCS8300 GCC and SA8775P multi media clock controllers respectively.

[1] https://lore.kernel.org/all/20240822-qcs8300-gcc-v2-0-b310dfa70ad8@quicinc.com/
[2] https://lore.kernel.org/all/20241011-sa8775p-mm-v4-resend-patches-v5-0-4a9f17dc683a@quicinc.com/

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
Imran Shaik (6):
      dt-bindings: clock: qcom: Add GPU clocks for QCS8300
      clk: qcom: Add support for GPU Clock Controller on QCS8300
      dt-bindings: clock: qcom: Add CAMCC clocks for QCS8300
      clk: qcom: Add support for Camera Clock Controller on QCS8300
      dt-bindings: clock: qcom: Add QCS8300 video clock controller
      clk: qcom: Add support for Video Clock Controller on QCS8300

 .../devicetree/bindings/clock/qcom,gpucc.yaml      |  1 +
 .../bindings/clock/qcom,sa8775p-camcc.yaml         |  1 +
 .../bindings/clock/qcom,sa8775p-videocc.yaml       |  1 +
 drivers/clk/qcom/camcc-sa8775p.c                   | 99 +++++++++++++++++++++-
 drivers/clk/qcom/gpucc-sa8775p.c                   | 47 ++++++++++
 drivers/clk/qcom/videocc-sa8775p.c                 |  4 +
 include/dt-bindings/clock/qcom,sa8775p-camcc.h     |  1 +
 include/dt-bindings/clock/qcom,sa8775p-gpucc.h     |  4 +-
 8 files changed, 153 insertions(+), 5 deletions(-)
---
base-commit: 891a4dc5705df4de9a258accef31786b46700394
change-id: 20241016-qcs8300-mm-patches-fc01e8c75ed4

Best regards,
-- 
Imran Shaik <quic_imrashai@quicinc.com>

[PATCH 1/6] dt-bindings: clock: qcom: Add GPU clocks for QCS8300

[Imran Shaik]

Add support for qcom GPU clock controller bindings for QCS8300 platform.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,gpucc.yaml | 1 +
 include/dt-bindings/clock/qcom,sa8775p-gpucc.h          | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
index 0858fd635282..b2b8a1e0297f 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
@@ -27,6 +27,7 @@ description: |
 properties:
   compatible:
     enum:
+      - qcom,qcs8300-gpucc
       - qcom,sdm845-gpucc
       - qcom,sa8775p-gpucc
       - qcom,sc7180-gpucc
diff --git a/include/dt-bindings/clock/qcom,sa8775p-gpucc.h b/include/dt-bindings/clock/qcom,sa8775p-gpucc.h
index a5fd784b1ea2..54eaaf1c4e52 100644
--- a/include/dt-bindings/clock/qcom,sa8775p-gpucc.h
+++ b/include/dt-bindings/clock/qcom,sa8775p-gpucc.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
 /*
- * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved.
  * Copyright (c) 2023, Linaro Limited
  */
 
@@ -31,6 +31,8 @@
 #define GPU_CC_MEMNOC_GFX_CLK			20
 #define GPU_CC_SLEEP_CLK			21
 #define GPU_CC_XO_CLK_SRC			22
+#define GPU_CC_CX_ACCU_SHIFT_CLK		23
+#define GPU_CC_GX_ACCU_SHIFT_CLK		24
 
 /* GPU_CC resets */
 #define GPUCC_GPU_CC_ACD_BCR			0

-- 
2.25.1

[PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Imran Shaik]

Add support to the QCS8300 GPU clock controller by extending
the SA8775P GPU clock controller, which is mostly identical
but QCS8300 has few additional clocks and minor differences.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
index f8a8ac343d70..99a8344b00db 100644
--- a/drivers/clk/qcom/gpucc-sa8775p.c
+++ b/drivers/clk/qcom/gpucc-sa8775p.c
@@ -317,6 +317,24 @@ static struct clk_branch gpu_cc_crc_ahb_clk = {
 	},
 };
 
+static struct clk_branch gpu_cc_cx_accu_shift_clk = {
+	.halt_reg = 0x95e8,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x95e8,
+		.enable_mask = BIT(0),
+		.hw.init = &(const struct clk_init_data){
+			.name = "gpu_cc_cx_accu_shift_clk",
+			.parent_hws = (const struct clk_hw*[]){
+				&gpu_cc_xo_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gpu_cc_cx_ff_clk = {
 	.halt_reg = 0x914c,
 	.halt_check = BRANCH_HALT,
@@ -420,6 +438,24 @@ static struct clk_branch gpu_cc_demet_clk = {
 	},
 };
 
+static struct clk_branch gpu_cc_gx_accu_shift_clk = {
+	.halt_reg = 0x95e4,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x95e4,
+		.enable_mask = BIT(0),
+		.hw.init = &(const struct clk_init_data){
+			.name = "gpu_cc_gx_accu_shift_clk",
+			.parent_hws = (const struct clk_hw*[]){
+				&gpu_cc_xo_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct clk_branch gpu_cc_hlos1_vote_gpu_smmu_clk = {
 	.halt_reg = 0x7000,
 	.halt_check = BRANCH_HALT_VOTED,
@@ -499,6 +535,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
 	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
 	[GPU_CC_CB_CLK] = &gpu_cc_cb_clk.clkr,
 	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
+	[GPU_CC_CX_ACCU_SHIFT_CLK] = NULL,
 	[GPU_CC_CX_FF_CLK] = &gpu_cc_cx_ff_clk.clkr,
 	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
 	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
@@ -508,6 +545,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
 	[GPU_CC_DEMET_DIV_CLK_SRC] = &gpu_cc_demet_div_clk_src.clkr,
 	[GPU_CC_FF_CLK_SRC] = &gpu_cc_ff_clk_src.clkr,
 	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
+	[GPU_CC_GX_ACCU_SHIFT_CLK] = NULL,
 	[GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK] = &gpu_cc_hlos1_vote_gpu_smmu_clk.clkr,
 	[GPU_CC_HUB_AHB_DIV_CLK_SRC] = &gpu_cc_hub_ahb_div_clk_src.clkr,
 	[GPU_CC_HUB_AON_CLK] = &gpu_cc_hub_aon_clk.clkr,
@@ -583,6 +621,7 @@ static const struct qcom_cc_desc gpu_cc_sa8775p_desc = {
 };
 
 static const struct of_device_id gpu_cc_sa8775p_match_table[] = {
+	{ .compatible = "qcom,qcs8300-gpucc" },
 	{ .compatible = "qcom,sa8775p-gpucc" },
 	{ }
 };
@@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
+	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
+		gpu_cc_pll0_config.l = 0x31;
+		gpu_cc_pll0_config.alpha = 0xe555;
+
+		gpu_cc_sa8775p_clocks[GPU_CC_CX_ACCU_SHIFT_CLK] = &gpu_cc_cx_accu_shift_clk.clkr;
+		gpu_cc_sa8775p_clocks[GPU_CC_GX_ACCU_SHIFT_CLK] = &gpu_cc_gx_accu_shift_clk.clkr;
+	}
+
 	clk_lucid_evo_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
 	clk_lucid_evo_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
 

-- 
2.25.1

[PATCH 3/6] dt-bindings: clock: qcom: Add CAMCC clocks for QCS8300

[Imran Shaik]

Add support for qcom camera clock controller bindings for QCS8300 platform.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,sa8775p-camcc.yaml | 1 +
 include/dt-bindings/clock/qcom,sa8775p-camcc.h                  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sa8775p-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sa8775p-camcc.yaml
index 36a60d8f5ae3..18cbc23b9a07 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sa8775p-camcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sa8775p-camcc.yaml
@@ -18,6 +18,7 @@ description: |
 properties:
   compatible:
     enum:
+      - qcom,qcs8300-camcc
       - qcom,sa8775p-camcc
 
   clocks:
diff --git a/include/dt-bindings/clock/qcom,sa8775p-camcc.h b/include/dt-bindings/clock/qcom,sa8775p-camcc.h
index 38531acd699f..36ac587a981a 100644
--- a/include/dt-bindings/clock/qcom,sa8775p-camcc.h
+++ b/include/dt-bindings/clock/qcom,sa8775p-camcc.h
@@ -93,6 +93,7 @@
 #define CAM_CC_SM_OBS_CLK					83
 #define CAM_CC_XO_CLK_SRC					84
 #define CAM_CC_QDSS_DEBUG_XO_CLK				85
+#define CAM_CC_TITAN_TOP_ACCU_SHIFT_CLK				86
 
 /* CAM_CC power domains */
 #define CAM_CC_TITAN_TOP_GDSC					0

-- 
2.25.1

[PATCH 4/6] clk: qcom: Add support for Camera Clock Controller on QCS8300

[Imran Shaik]

Add support to the QCS8300 Camera clock controller by extending
the SA8775P Camera clock controller, which is mostly identical
but QCS8300 has few additional clocks and few other differences.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 drivers/clk/qcom/camcc-sa8775p.c | 99 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/camcc-sa8775p.c b/drivers/clk/qcom/camcc-sa8775p.c
index c04801a5af35..0ef3c6015c34 100644
--- a/drivers/clk/qcom/camcc-sa8775p.c
+++ b/drivers/clk/qcom/camcc-sa8775p.c
@@ -1682,6 +1682,24 @@ static struct clk_branch cam_cc_sm_obs_clk = {
 	},
 };
 
+static struct clk_branch cam_cc_titan_top_accu_shift_clk = {
+	.halt_reg = 0x131f0,
+	.halt_check = BRANCH_HALT_VOTED,
+	.clkr = {
+		.enable_reg = 0x131f0,
+		.enable_mask = BIT(0),
+		.hw.init = &(const struct clk_init_data) {
+			.name = "cam_cc_titan_top_accu_shift_clk",
+			.parent_hws = (const struct clk_hw*[]) {
+				&cam_cc_xo_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static struct gdsc cam_cc_titan_top_gdsc = {
 	.gdscr = 0x131bc,
 	.en_rest_wait_val = 0x2,
@@ -1776,6 +1794,7 @@ static struct clk_regmap *cam_cc_sa8775p_clocks[] = {
 	[CAM_CC_SLEEP_CLK_SRC] = &cam_cc_sleep_clk_src.clkr,
 	[CAM_CC_SLOW_AHB_CLK_SRC] = &cam_cc_slow_ahb_clk_src.clkr,
 	[CAM_CC_SM_OBS_CLK] = &cam_cc_sm_obs_clk.clkr,
+	[CAM_CC_TITAN_TOP_ACCU_SHIFT_CLK] = NULL,
 	[CAM_CC_XO_CLK_SRC] = &cam_cc_xo_clk_src.clkr,
 	[CAM_CC_QDSS_DEBUG_XO_CLK] = &cam_cc_qdss_debug_xo_clk.clkr,
 };
@@ -1812,6 +1831,7 @@ static struct qcom_cc_desc cam_cc_sa8775p_desc = {
 };
 
 static const struct of_device_id cam_cc_sa8775p_match_table[] = {
+	{ .compatible = "qcom,qcs8300-camcc" },
 	{ .compatible = "qcom,sa8775p-camcc" },
 	{ }
 };
@@ -1842,10 +1862,81 @@ static int cam_cc_sa8775p_probe(struct platform_device *pdev)
 	clk_lucid_evo_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
 	clk_lucid_evo_pll_configure(&cam_cc_pll5, regmap, &cam_cc_pll5_config);
 
-	/* Keep some clocks always enabled */
-	qcom_branch_set_clk_en(regmap, 0x13194); /* CAM_CC_CAMNOC_XO_CLK */
-	qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_GDSC_CLK */
-	qcom_branch_set_clk_en(regmap, 0x13208); /* CAM_CC_SLEEP_CLK */
+	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-camcc")) {
+		cam_cc_camnoc_axi_clk_src.cmd_rcgr = 0x13154;
+		cam_cc_camnoc_axi_clk.halt_reg = 0x1316c;
+		cam_cc_camnoc_axi_clk.clkr.enable_reg = 0x1316c;
+		cam_cc_camnoc_dcd_xo_clk.halt_reg = 0x13174;
+		cam_cc_camnoc_dcd_xo_clk.clkr.enable_reg = 0x13174;
+
+		cam_cc_csi0phytimer_clk_src.cmd_rcgr = 0x15054;
+		cam_cc_csi1phytimer_clk_src.cmd_rcgr = 0x15078;
+		cam_cc_csi2phytimer_clk_src.cmd_rcgr = 0x15098;
+		cam_cc_csid_clk_src.cmd_rcgr = 0x13134;
+
+		cam_cc_mclk0_clk_src.cmd_rcgr = 0x15000;
+		cam_cc_mclk1_clk_src.cmd_rcgr = 0x1501c;
+		cam_cc_mclk2_clk_src.cmd_rcgr = 0x15038;
+
+		cam_cc_fast_ahb_clk_src.cmd_rcgr = 0x13104;
+		cam_cc_slow_ahb_clk_src.cmd_rcgr = 0x1311c;
+		cam_cc_xo_clk_src.cmd_rcgr = 0x131b8;
+		cam_cc_sleep_clk_src.cmd_rcgr = 0x131d4;
+
+		cam_cc_core_ahb_clk.halt_reg = 0x131b4;
+		cam_cc_core_ahb_clk.clkr.enable_reg = 0x131b4;
+
+		cam_cc_cpas_ahb_clk.halt_reg = 0x130f4;
+		cam_cc_cpas_ahb_clk.clkr.enable_reg = 0x130f4;
+		cam_cc_cpas_fast_ahb_clk.halt_reg = 0x130fc;
+		cam_cc_cpas_fast_ahb_clk.clkr.enable_reg = 0x130fc;
+
+		cam_cc_csi0phytimer_clk.halt_reg = 0x1506c;
+		cam_cc_csi0phytimer_clk.clkr.enable_reg = 0x1506c;
+		cam_cc_csi1phytimer_clk.halt_reg = 0x15090;
+		cam_cc_csi1phytimer_clk.clkr.enable_reg = 0x15090;
+		cam_cc_csi2phytimer_clk.halt_reg = 0x150b0;
+		cam_cc_csi2phytimer_clk.clkr.enable_reg = 0x150b0;
+		cam_cc_csid_clk.halt_reg = 0x1314c;
+		cam_cc_csid_clk.clkr.enable_reg = 0x1314c;
+		cam_cc_csid_csiphy_rx_clk.halt_reg = 0x15074;
+		cam_cc_csid_csiphy_rx_clk.clkr.enable_reg = 0x15074;
+		cam_cc_csiphy0_clk.halt_reg = 0x15070;
+		cam_cc_csiphy0_clk.clkr.enable_reg = 0x15070;
+		cam_cc_csiphy1_clk.halt_reg = 0x15094;
+		cam_cc_csiphy1_clk.clkr.enable_reg = 0x15094;
+		cam_cc_csiphy2_clk.halt_reg = 0x150b4;
+		cam_cc_csiphy2_clk.clkr.enable_reg = 0x150b4;
+
+		cam_cc_mclk0_clk.halt_reg = 0x15018;
+		cam_cc_mclk0_clk.clkr.enable_reg = 0x15018;
+		cam_cc_mclk1_clk.halt_reg = 0x15034;
+		cam_cc_mclk1_clk.clkr.enable_reg = 0x15034;
+		cam_cc_mclk2_clk.halt_reg = 0x15050;
+		cam_cc_mclk2_clk.clkr.enable_reg = 0x15050;
+
+		cam_cc_titan_top_gdsc.gdscr = 0x131a0;
+
+		cam_cc_sa8775p_clocks[CAM_CC_CCI_3_CLK] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_CCI_3_CLK_SRC] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_CSI3PHYTIMER_CLK] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_CSI3PHYTIMER_CLK_SRC] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_CSIPHY3_CLK] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_MCLK3_CLK] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_MCLK3_CLK_SRC] = NULL;
+		cam_cc_sa8775p_clocks[CAM_CC_TITAN_TOP_ACCU_SHIFT_CLK] =
+				&cam_cc_titan_top_accu_shift_clk.clkr;
+
+		/* Keep some clocks always enabled */
+		qcom_branch_set_clk_en(regmap, 0x13178); /* CAM_CC_CAMNOC_XO_CLK */
+		qcom_branch_set_clk_en(regmap, 0x131d0); /* CAM_CC_GDSC_CLK */
+		qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_SLEEP_CLK */
+	} else {
+		/* Keep some clocks always enabled */
+		qcom_branch_set_clk_en(regmap, 0x13194); /* CAM_CC_CAMNOC_XO_CLK */
+		qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_GDSC_CLK */
+		qcom_branch_set_clk_en(regmap, 0x13208); /* CAM_CC_SLEEP_CLK */
+	}
 
 	ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_sa8775p_desc, regmap);
 

-- 
2.25.1

[PATCH 5/6] dt-bindings: clock: qcom: Add QCS8300 video clock controller

[Imran Shaik]

Add device tree bindings for the video clock controller on Qualcomm
QCS8300 platform.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
index 928131bff4c1..07e5d811d816 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
@@ -18,6 +18,7 @@ description: |
 properties:
   compatible:
     enum:
+      - qcom,qcs8300-videocc
       - qcom,sa8775p-videocc
 
   clocks:

-- 
2.25.1

[PATCH 6/6] clk: qcom: Add support for Video Clock Controller on QCS8300

[Imran Shaik]

Add support to the QCS8300 Video clock controller by extending
the SA8775P Video clock controller, which is mostly identical
but QCS8300 has minor difference.

Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
---
 drivers/clk/qcom/videocc-sa8775p.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/qcom/videocc-sa8775p.c b/drivers/clk/qcom/videocc-sa8775p.c
index bf5de411fd5d..d0494ba81f5f 100644
--- a/drivers/clk/qcom/videocc-sa8775p.c
+++ b/drivers/clk/qcom/videocc-sa8775p.c
@@ -524,6 +524,7 @@ static struct qcom_cc_desc video_cc_sa8775p_desc = {
 
 static const struct of_device_id video_cc_sa8775p_match_table[] = {
 	{ .compatible = "qcom,sa8775p-videocc" },
+	{ .compatible = "qcom,qcs8300-videocc" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, video_cc_sa8775p_match_table);
@@ -550,6 +551,9 @@ static int video_cc_sa8775p_probe(struct platform_device *pdev)
 	clk_lucid_evo_pll_configure(&video_pll0, regmap, &video_pll0_config);
 	clk_lucid_evo_pll_configure(&video_pll1, regmap, &video_pll1_config);
 
+	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-videocc"))
+		regmap_write(regmap, video_cc_mvs0c_div2_div_clk_src.reg, 2);
+
 	/* Keep some clocks always enabled */
 	qcom_branch_set_clk_en(regmap, 0x80ec); /* VIDEO_CC_AHB_CLK */
 	qcom_branch_set_clk_en(regmap, 0x8144); /* VIDEO_CC_SLEEP_CLK */

-- 
2.25.1

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Dmitry Baryshkov]

On Fri, Oct 18, 2024 at 04:42:30PM +0530, Imran Shaik wrote:
> Add support to the QCS8300 GPU clock controller by extending
> the SA8775P GPU clock controller, which is mostly identical
> but QCS8300 has few additional clocks and minor differences.
> 
> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> ---
>  drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 4/6] clk: qcom: Add support for Camera Clock Controller on QCS8300

[Dmitry Baryshkov]

On Fri, Oct 18, 2024 at 04:42:32PM +0530, Imran Shaik wrote:
> Add support to the QCS8300 Camera clock controller by extending
> the SA8775P Camera clock controller, which is mostly identical
> but QCS8300 has few additional clocks and few other differences.
> 
> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> ---
>  drivers/clk/qcom/camcc-sa8775p.c | 99 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/camcc-sa8775p.c b/drivers/clk/qcom/camcc-sa8775p.c
> index c04801a5af35..0ef3c6015c34 100644
> --- a/drivers/clk/qcom/camcc-sa8775p.c
> +++ b/drivers/clk/qcom/camcc-sa8775p.c
> @@ -1682,6 +1682,24 @@ static struct clk_branch cam_cc_sm_obs_clk = {
>  	},
>  };
>  
> +static struct clk_branch cam_cc_titan_top_accu_shift_clk = {
> +	.halt_reg = 0x131f0,
> +	.halt_check = BRANCH_HALT_VOTED,
> +	.clkr = {
> +		.enable_reg = 0x131f0,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "cam_cc_titan_top_accu_shift_clk",
> +			.parent_hws = (const struct clk_hw*[]) {
> +				&cam_cc_xo_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
>  static struct gdsc cam_cc_titan_top_gdsc = {
>  	.gdscr = 0x131bc,
>  	.en_rest_wait_val = 0x2,
> @@ -1776,6 +1794,7 @@ static struct clk_regmap *cam_cc_sa8775p_clocks[] = {
>  	[CAM_CC_SLEEP_CLK_SRC] = &cam_cc_sleep_clk_src.clkr,
>  	[CAM_CC_SLOW_AHB_CLK_SRC] = &cam_cc_slow_ahb_clk_src.clkr,
>  	[CAM_CC_SM_OBS_CLK] = &cam_cc_sm_obs_clk.clkr,
> +	[CAM_CC_TITAN_TOP_ACCU_SHIFT_CLK] = NULL,
>  	[CAM_CC_XO_CLK_SRC] = &cam_cc_xo_clk_src.clkr,
>  	[CAM_CC_QDSS_DEBUG_XO_CLK] = &cam_cc_qdss_debug_xo_clk.clkr,
>  };
> @@ -1812,6 +1831,7 @@ static struct qcom_cc_desc cam_cc_sa8775p_desc = {
>  };
>  
>  static const struct of_device_id cam_cc_sa8775p_match_table[] = {
> +	{ .compatible = "qcom,qcs8300-camcc" },
>  	{ .compatible = "qcom,sa8775p-camcc" },
>  	{ }
>  };
> @@ -1842,10 +1862,81 @@ static int cam_cc_sa8775p_probe(struct platform_device *pdev)
>  	clk_lucid_evo_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
>  	clk_lucid_evo_pll_configure(&cam_cc_pll5, regmap, &cam_cc_pll5_config);
>  
> -	/* Keep some clocks always enabled */
> -	qcom_branch_set_clk_en(regmap, 0x13194); /* CAM_CC_CAMNOC_XO_CLK */
> -	qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_GDSC_CLK */
> -	qcom_branch_set_clk_en(regmap, 0x13208); /* CAM_CC_SLEEP_CLK */
> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-camcc")) {
> +		cam_cc_camnoc_axi_clk_src.cmd_rcgr = 0x13154;
> +		cam_cc_camnoc_axi_clk.halt_reg = 0x1316c;
> +		cam_cc_camnoc_axi_clk.clkr.enable_reg = 0x1316c;
> +		cam_cc_camnoc_dcd_xo_clk.halt_reg = 0x13174;
> +		cam_cc_camnoc_dcd_xo_clk.clkr.enable_reg = 0x13174;
> +
> +		cam_cc_csi0phytimer_clk_src.cmd_rcgr = 0x15054;
> +		cam_cc_csi1phytimer_clk_src.cmd_rcgr = 0x15078;
> +		cam_cc_csi2phytimer_clk_src.cmd_rcgr = 0x15098;
> +		cam_cc_csid_clk_src.cmd_rcgr = 0x13134;
> +
> +		cam_cc_mclk0_clk_src.cmd_rcgr = 0x15000;
> +		cam_cc_mclk1_clk_src.cmd_rcgr = 0x1501c;
> +		cam_cc_mclk2_clk_src.cmd_rcgr = 0x15038;
> +
> +		cam_cc_fast_ahb_clk_src.cmd_rcgr = 0x13104;
> +		cam_cc_slow_ahb_clk_src.cmd_rcgr = 0x1311c;
> +		cam_cc_xo_clk_src.cmd_rcgr = 0x131b8;
> +		cam_cc_sleep_clk_src.cmd_rcgr = 0x131d4;
> +
> +		cam_cc_core_ahb_clk.halt_reg = 0x131b4;
> +		cam_cc_core_ahb_clk.clkr.enable_reg = 0x131b4;
> +
> +		cam_cc_cpas_ahb_clk.halt_reg = 0x130f4;
> +		cam_cc_cpas_ahb_clk.clkr.enable_reg = 0x130f4;
> +		cam_cc_cpas_fast_ahb_clk.halt_reg = 0x130fc;
> +		cam_cc_cpas_fast_ahb_clk.clkr.enable_reg = 0x130fc;
> +
> +		cam_cc_csi0phytimer_clk.halt_reg = 0x1506c;
> +		cam_cc_csi0phytimer_clk.clkr.enable_reg = 0x1506c;
> +		cam_cc_csi1phytimer_clk.halt_reg = 0x15090;
> +		cam_cc_csi1phytimer_clk.clkr.enable_reg = 0x15090;
> +		cam_cc_csi2phytimer_clk.halt_reg = 0x150b0;
> +		cam_cc_csi2phytimer_clk.clkr.enable_reg = 0x150b0;
> +		cam_cc_csid_clk.halt_reg = 0x1314c;
> +		cam_cc_csid_clk.clkr.enable_reg = 0x1314c;
> +		cam_cc_csid_csiphy_rx_clk.halt_reg = 0x15074;
> +		cam_cc_csid_csiphy_rx_clk.clkr.enable_reg = 0x15074;
> +		cam_cc_csiphy0_clk.halt_reg = 0x15070;
> +		cam_cc_csiphy0_clk.clkr.enable_reg = 0x15070;
> +		cam_cc_csiphy1_clk.halt_reg = 0x15094;
> +		cam_cc_csiphy1_clk.clkr.enable_reg = 0x15094;
> +		cam_cc_csiphy2_clk.halt_reg = 0x150b4;
> +		cam_cc_csiphy2_clk.clkr.enable_reg = 0x150b4;
> +
> +		cam_cc_mclk0_clk.halt_reg = 0x15018;
> +		cam_cc_mclk0_clk.clkr.enable_reg = 0x15018;
> +		cam_cc_mclk1_clk.halt_reg = 0x15034;
> +		cam_cc_mclk1_clk.clkr.enable_reg = 0x15034;
> +		cam_cc_mclk2_clk.halt_reg = 0x15050;
> +		cam_cc_mclk2_clk.clkr.enable_reg = 0x15050;
> +
> +		cam_cc_titan_top_gdsc.gdscr = 0x131a0;

Ugh. I see the point, but I can't say I like it.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> +
> +		cam_cc_sa8775p_clocks[CAM_CC_CCI_3_CLK] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_CCI_3_CLK_SRC] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_CSI3PHYTIMER_CLK] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_CSI3PHYTIMER_CLK_SRC] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_CSIPHY3_CLK] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_MCLK3_CLK] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_MCLK3_CLK_SRC] = NULL;
> +		cam_cc_sa8775p_clocks[CAM_CC_TITAN_TOP_ACCU_SHIFT_CLK] =
> +				&cam_cc_titan_top_accu_shift_clk.clkr;
> +
> +		/* Keep some clocks always enabled */
> +		qcom_branch_set_clk_en(regmap, 0x13178); /* CAM_CC_CAMNOC_XO_CLK */
> +		qcom_branch_set_clk_en(regmap, 0x131d0); /* CAM_CC_GDSC_CLK */
> +		qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_SLEEP_CLK */
> +	} else {
> +		/* Keep some clocks always enabled */
> +		qcom_branch_set_clk_en(regmap, 0x13194); /* CAM_CC_CAMNOC_XO_CLK */
> +		qcom_branch_set_clk_en(regmap, 0x131ec); /* CAM_CC_GDSC_CLK */
> +		qcom_branch_set_clk_en(regmap, 0x13208); /* CAM_CC_SLEEP_CLK */
> +	}
>  
>  	ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_sa8775p_desc, regmap);
>  
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] clk: qcom: Add support for Video Clock Controller on QCS8300

[Dmitry Baryshkov]

On Fri, Oct 18, 2024 at 04:42:34PM +0530, Imran Shaik wrote:
> Add support to the QCS8300 Video clock controller by extending
> the SA8775P Video clock controller, which is mostly identical
> but QCS8300 has minor difference.
> 
> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> ---
>  drivers/clk/qcom/videocc-sa8775p.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/qcom/videocc-sa8775p.c b/drivers/clk/qcom/videocc-sa8775p.c
> index bf5de411fd5d..d0494ba81f5f 100644
> --- a/drivers/clk/qcom/videocc-sa8775p.c
> +++ b/drivers/clk/qcom/videocc-sa8775p.c
> @@ -524,6 +524,7 @@ static struct qcom_cc_desc video_cc_sa8775p_desc = {
>  
>  static const struct of_device_id video_cc_sa8775p_match_table[] = {
>  	{ .compatible = "qcom,sa8775p-videocc" },
> +	{ .compatible = "qcom,qcs8300-videocc" },

Sorted, please.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, video_cc_sa8775p_match_table);
> @@ -550,6 +551,9 @@ static int video_cc_sa8775p_probe(struct platform_device *pdev)
>  	clk_lucid_evo_pll_configure(&video_pll0, regmap, &video_pll0_config);
>  	clk_lucid_evo_pll_configure(&video_pll1, regmap, &video_pll1_config);
>  

Comment?

> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-videocc"))
> +		regmap_write(regmap, video_cc_mvs0c_div2_div_clk_src.reg, 2);
> +
>  	/* Keep some clocks always enabled */
>  	qcom_branch_set_clk_en(regmap, 0x80ec); /* VIDEO_CC_AHB_CLK */
>  	qcom_branch_set_clk_en(regmap, 0x8144); /* VIDEO_CC_SLEEP_CLK */
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/6] dt-bindings: clock: qcom: Add GPU clocks for QCS8300

[Krzysztof Kozlowski]

On Fri, Oct 18, 2024 at 04:42:29PM +0530, Imran Shaik wrote:
> Add support for qcom GPU clock controller bindings for QCS8300 platform.

Why are you adding defines to SA8775p header? Commit msg should explain
non-obvious contents.

Best regards,
Krzysztof

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Krzysztof Kozlowski]

On Fri, Oct 18, 2024 at 04:42:30PM +0530, Imran Shaik wrote:
> Add support to the QCS8300 GPU clock controller by extending
> the SA8775P GPU clock controller, which is mostly identical
> but QCS8300 has few additional clocks and minor differences.
> 
> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> ---
>  drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
> index f8a8ac343d70..99a8344b00db 100644
> --- a/drivers/clk/qcom/gpucc-sa8775p.c
> +++ b/drivers/clk/qcom/gpucc-sa8775p.c
> @@ -317,6 +317,24 @@ static struct clk_branch gpu_cc_crc_ahb_clk = {
>  	},
>  };
>  
> +static struct clk_branch gpu_cc_cx_accu_shift_clk = {
> +	.halt_reg = 0x95e8,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x95e8,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data){
> +			.name = "gpu_cc_cx_accu_shift_clk",
> +			.parent_hws = (const struct clk_hw*[]){
> +				&gpu_cc_xo_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
>  static struct clk_branch gpu_cc_cx_ff_clk = {
>  	.halt_reg = 0x914c,
>  	.halt_check = BRANCH_HALT,
> @@ -420,6 +438,24 @@ static struct clk_branch gpu_cc_demet_clk = {
>  	},
>  };
>  
> +static struct clk_branch gpu_cc_gx_accu_shift_clk = {
> +	.halt_reg = 0x95e4,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x95e4,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data){
> +			.name = "gpu_cc_gx_accu_shift_clk",
> +			.parent_hws = (const struct clk_hw*[]){
> +				&gpu_cc_xo_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
>  static struct clk_branch gpu_cc_hlos1_vote_gpu_smmu_clk = {
>  	.halt_reg = 0x7000,
>  	.halt_check = BRANCH_HALT_VOTED,
> @@ -499,6 +535,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
>  	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
>  	[GPU_CC_CB_CLK] = &gpu_cc_cb_clk.clkr,
>  	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
> +	[GPU_CC_CX_ACCU_SHIFT_CLK] = NULL,
>  	[GPU_CC_CX_FF_CLK] = &gpu_cc_cx_ff_clk.clkr,
>  	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
>  	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
> @@ -508,6 +545,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
>  	[GPU_CC_DEMET_DIV_CLK_SRC] = &gpu_cc_demet_div_clk_src.clkr,
>  	[GPU_CC_FF_CLK_SRC] = &gpu_cc_ff_clk_src.clkr,
>  	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
> +	[GPU_CC_GX_ACCU_SHIFT_CLK] = NULL,
>  	[GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK] = &gpu_cc_hlos1_vote_gpu_smmu_clk.clkr,
>  	[GPU_CC_HUB_AHB_DIV_CLK_SRC] = &gpu_cc_hub_ahb_div_clk_src.clkr,
>  	[GPU_CC_HUB_AON_CLK] = &gpu_cc_hub_aon_clk.clkr,
> @@ -583,6 +621,7 @@ static const struct qcom_cc_desc gpu_cc_sa8775p_desc = {
>  };
>  
>  static const struct of_device_id gpu_cc_sa8775p_match_table[] = {
> +	{ .compatible = "qcom,qcs8300-gpucc" },
>  	{ .compatible = "qcom,sa8775p-gpucc" },

I just wanted to comment on your binding that devices should be made
compatible...

>  	{ }
>  };
> @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
>  
> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {

Why we cannot use match data? Seeing compatibles in the code is
unexpected and does not scale.

Best regards,
Krzysztof

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Dmitry Baryshkov]

On Mon, Oct 21, 2024 at 09:56:08AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Oct 18, 2024 at 04:42:30PM +0530, Imran Shaik wrote:
> > Add support to the QCS8300 GPU clock controller by extending
> > the SA8775P GPU clock controller, which is mostly identical
> > but QCS8300 has few additional clocks and minor differences.
> > 
> > Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> > ---
> >  drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
> > index f8a8ac343d70..99a8344b00db 100644
> > --- a/drivers/clk/qcom/gpucc-sa8775p.c
> > +++ b/drivers/clk/qcom/gpucc-sa8775p.c
> > @@ -317,6 +317,24 @@ static struct clk_branch gpu_cc_crc_ahb_clk = {
> >  	},
> >  };
> >  
> > +static struct clk_branch gpu_cc_cx_accu_shift_clk = {
> > +	.halt_reg = 0x95e8,
> > +	.halt_check = BRANCH_HALT,
> > +	.clkr = {
> > +		.enable_reg = 0x95e8,
> > +		.enable_mask = BIT(0),
> > +		.hw.init = &(const struct clk_init_data){
> > +			.name = "gpu_cc_cx_accu_shift_clk",
> > +			.parent_hws = (const struct clk_hw*[]){
> > +				&gpu_cc_xo_clk_src.clkr.hw,
> > +			},
> > +			.num_parents = 1,
> > +			.flags = CLK_SET_RATE_PARENT,
> > +			.ops = &clk_branch2_ops,
> > +		},
> > +	},
> > +};
> > +
> >  static struct clk_branch gpu_cc_cx_ff_clk = {
> >  	.halt_reg = 0x914c,
> >  	.halt_check = BRANCH_HALT,
> > @@ -420,6 +438,24 @@ static struct clk_branch gpu_cc_demet_clk = {
> >  	},
> >  };
> >  
> > +static struct clk_branch gpu_cc_gx_accu_shift_clk = {
> > +	.halt_reg = 0x95e4,
> > +	.halt_check = BRANCH_HALT,
> > +	.clkr = {
> > +		.enable_reg = 0x95e4,
> > +		.enable_mask = BIT(0),
> > +		.hw.init = &(const struct clk_init_data){
> > +			.name = "gpu_cc_gx_accu_shift_clk",
> > +			.parent_hws = (const struct clk_hw*[]){
> > +				&gpu_cc_xo_clk_src.clkr.hw,
> > +			},
> > +			.num_parents = 1,
> > +			.flags = CLK_SET_RATE_PARENT,
> > +			.ops = &clk_branch2_ops,
> > +		},
> > +	},
> > +};
> > +
> >  static struct clk_branch gpu_cc_hlos1_vote_gpu_smmu_clk = {
> >  	.halt_reg = 0x7000,
> >  	.halt_check = BRANCH_HALT_VOTED,
> > @@ -499,6 +535,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
> >  	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
> >  	[GPU_CC_CB_CLK] = &gpu_cc_cb_clk.clkr,
> >  	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
> > +	[GPU_CC_CX_ACCU_SHIFT_CLK] = NULL,
> >  	[GPU_CC_CX_FF_CLK] = &gpu_cc_cx_ff_clk.clkr,
> >  	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
> >  	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
> > @@ -508,6 +545,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
> >  	[GPU_CC_DEMET_DIV_CLK_SRC] = &gpu_cc_demet_div_clk_src.clkr,
> >  	[GPU_CC_FF_CLK_SRC] = &gpu_cc_ff_clk_src.clkr,
> >  	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
> > +	[GPU_CC_GX_ACCU_SHIFT_CLK] = NULL,
> >  	[GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK] = &gpu_cc_hlos1_vote_gpu_smmu_clk.clkr,
> >  	[GPU_CC_HUB_AHB_DIV_CLK_SRC] = &gpu_cc_hub_ahb_div_clk_src.clkr,
> >  	[GPU_CC_HUB_AON_CLK] = &gpu_cc_hub_aon_clk.clkr,
> > @@ -583,6 +621,7 @@ static const struct qcom_cc_desc gpu_cc_sa8775p_desc = {
> >  };
> >  
> >  static const struct of_device_id gpu_cc_sa8775p_match_table[] = {
> > +	{ .compatible = "qcom,qcs8300-gpucc" },
> >  	{ .compatible = "qcom,sa8775p-gpucc" },
> 
> I just wanted to comment on your binding that devices should be made
> compatible...
> 
> >  	{ }
> >  };
> > @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
> >  	if (IS_ERR(regmap))
> >  		return PTR_ERR(regmap);
> >  
> > +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
> 
> Why we cannot use match data? Seeing compatibles in the code is
> unexpected and does not scale.

Because using match data doesn't scale in such cases. We have been using
compatibles to patch clock trees for the platforms for quite a while.
You can see that each of the "tunings" is slightly different. From my
point of view, this approach provides a nice balance between having a
completely duplicate driver and having a driver which self-patches the
tree.

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Krzysztof Kozlowski]

On 21/10/2024 12:56, Dmitry Baryshkov wrote:
>>>  	{ }
>>>  };
>>> @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
>>>  	if (IS_ERR(regmap))
>>>  		return PTR_ERR(regmap);
>>>  
>>> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
>>
>> Why we cannot use match data? Seeing compatibles in the code is
>> unexpected and does not scale.
> 
> Because using match data doesn't scale in such cases. We have been using

I don't understand how it could not scale. That's the entire point of
match data - scaling.

> compatibles to patch clock trees for the platforms for quite a while.
> You can see that each of the "tunings" is slightly different. From my


You have one driver, where are these tunings which are supposed to be
different? You need here only enum or define, in the simplest choice.

> point of view, this approach provides a nice balance between having a
> completely duplicate driver and having a driver which self-patches the
> tree.

How duplicate driver got into this? I don't think we talk about the
same. I meant ID table match data.
> 

Best regards,
Krzysztof

Re: [PATCH 5/6] dt-bindings: clock: qcom: Add QCS8300 video clock controller

[Rob Herring]

On Fri, Oct 18, 2024 at 04:42:33PM +0530, Imran Shaik wrote:
> Add device tree bindings for the video clock controller on Qualcomm
> QCS8300 platform.

That's obvious reading the diff. How is it different from the sa8775p 
version? It must be different or you should have a fallback compatible.

> 
> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
> index 928131bff4c1..07e5d811d816 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
> @@ -18,6 +18,7 @@ description: |
>  properties:
>    compatible:
>      enum:
> +      - qcom,qcs8300-videocc
>        - qcom,sa8775p-videocc
>  
>    clocks:
> 
> -- 
> 2.25.1
> 

Re: [PATCH 1/6] dt-bindings: clock: qcom: Add GPU clocks for QCS8300

[Imran Shaik]

On 10/21/2024 1:24 PM, Krzysztof Kozlowski wrote:
> On Fri, Oct 18, 2024 at 04:42:29PM +0530, Imran Shaik wrote:
>> Add support for qcom GPU clock controller bindings for QCS8300 platform.
> 
> Why are you adding defines to SA8775p header? Commit msg should explain
> non-obvious contents.
> 

The QCS8300 GPU clock controller is mostly identical to SA8775P, but 
QCS8300 has few additional clocks and minor differences. Hence, re-using 
the SA8775P GPUCC for QCS8300. I will update the commit text with these 
details in next series.

Thanks,
Imran

> Best regards,
> Krzysztof
> 

Re: [PATCH 5/6] dt-bindings: clock: qcom: Add QCS8300 video clock controller

[Imran Shaik]

On 10/21/2024 11:46 PM, Rob Herring wrote:
> On Fri, Oct 18, 2024 at 04:42:33PM +0530, Imran Shaik wrote:
>> Add device tree bindings for the video clock controller on Qualcomm
>> QCS8300 platform.
> 
> That's obvious reading the diff. How is it different from the sa8775p
> version? It must be different or you should have a fallback compatible.
> 

The QCS8300 Video clock controller is mostly identical to SA8775P, but 
QCS8300 has minor difference. Hence, re-using the SA8775P VideoCC for 
QCS8300. I will update the commit text with the details and post next 
series.

Thanks,
Imran

>>
>> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
>> index 928131bff4c1..07e5d811d816 100644
>> --- a/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/qcom,sa8775p-videocc.yaml
>> @@ -18,6 +18,7 @@ description: |
>>   properties:
>>     compatible:
>>       enum:
>> +      - qcom,qcs8300-videocc
>>         - qcom,sa8775p-videocc
>>   
>>     clocks:
>>
>> -- 
>> 2.25.1
>>

Re: [PATCH 6/6] clk: qcom: Add support for Video Clock Controller on QCS8300

[Imran Shaik]

On 10/18/2024 5:38 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 18, 2024 at 04:42:34PM +0530, Imran Shaik wrote:
>> Add support to the QCS8300 Video clock controller by extending
>> the SA8775P Video clock controller, which is mostly identical
>> but QCS8300 has minor difference.
>>
>> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com>
>> ---
>>   drivers/clk/qcom/videocc-sa8775p.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/videocc-sa8775p.c b/drivers/clk/qcom/videocc-sa8775p.c
>> index bf5de411fd5d..d0494ba81f5f 100644
>> --- a/drivers/clk/qcom/videocc-sa8775p.c
>> +++ b/drivers/clk/qcom/videocc-sa8775p.c
>> @@ -524,6 +524,7 @@ static struct qcom_cc_desc video_cc_sa8775p_desc = {
>>   
>>   static const struct of_device_id video_cc_sa8775p_match_table[] = {
>>   	{ .compatible = "qcom,sa8775p-videocc" },
>> +	{ .compatible = "qcom,qcs8300-videocc" },
> 
> Sorted, please.
> 

Sure, I will fix this in next patch series.

>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(of, video_cc_sa8775p_match_table);
>> @@ -550,6 +551,9 @@ static int video_cc_sa8775p_probe(struct platform_device *pdev)
>>   	clk_lucid_evo_pll_configure(&video_pll0, regmap, &video_pll0_config);
>>   	clk_lucid_evo_pll_configure(&video_pll1, regmap, &video_pll1_config);
>>   
> 
> Comment?
> 

Sure, I will update the comment details for qcs8300-videocc in next 
patch series.

Thanks,
Imran

>> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-videocc"))
>> +		regmap_write(regmap, video_cc_mvs0c_div2_div_clk_src.reg, 2);
>> +
>>   	/* Keep some clocks always enabled */
>>   	qcom_branch_set_clk_en(regmap, 0x80ec); /* VIDEO_CC_AHB_CLK */
>>   	qcom_branch_set_clk_en(regmap, 0x8144); /* VIDEO_CC_SLEEP_CLK */
>>
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Imran Shaik]

On 10/21/2024 8:41 PM, Krzysztof Kozlowski wrote:
> On 21/10/2024 12:56, Dmitry Baryshkov wrote:
>>>>   	{ }
>>>>   };
>>>> @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
>>>>   	if (IS_ERR(regmap))
>>>>   		return PTR_ERR(regmap);
>>>>   
>>>> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
>>>
>>> Why we cannot use match data? Seeing compatibles in the code is
>>> unexpected and does not scale.
>>
>> Because using match data doesn't scale in such cases. We have been using
> 
> I don't understand how it could not scale. That's the entire point of
> match data - scaling.
> 
>> compatibles to patch clock trees for the platforms for quite a while.
>> You can see that each of the "tunings" is slightly different. From my
> 
> 
> You have one driver, where are these tunings which are supposed to be
> different? You need here only enum or define, in the simplest choice.
> 
>> point of view, this approach provides a nice balance between having a
>> completely duplicate driver and having a driver which self-patches the
>> tree.
> 
> How duplicate driver got into this? I don't think we talk about the
> same. I meant ID table match data.
>>

I agree with Dmitry. If I understand correctly, to add match data 
support, we need to define the gpu_cc_qcs8300_clocks struct by 
duplicating the entries from gpu_cc_sa8775p_clocks and then adding the 
additional qcs8300 clocks. The compatible approach is simpler and used 
across most existing platforms.

Thanks,
Imran

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Dmitry Baryshkov]

On Mon, 21 Oct 2024 at 18:11, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/10/2024 12:56, Dmitry Baryshkov wrote:
> >>>     { }
> >>>  };
> >>> @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
> >>>     if (IS_ERR(regmap))
> >>>             return PTR_ERR(regmap);
> >>>
> >>> +   if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
> >>
> >> Why we cannot use match data? Seeing compatibles in the code is
> >> unexpected and does not scale.
> >
> > Because using match data doesn't scale in such cases. We have been using
>
> I don't understand how it could not scale. That's the entire point of
> match data - scaling.
>
> > compatibles to patch clock trees for the platforms for quite a while.
> > You can see that each of the "tunings" is slightly different. From my
>
>
> You have one driver, where are these tunings which are supposed to be
> different? You need here only enum or define, in the simplest choice.

I think adding enum / define just for the sake of the match data is an
overkill. The driver checks for the compatible, tunes clock tree,
registers it and then it's done. There is no scalability issue - IOW
there are not so many compatible strings being handled by the driver,
the is_compatible check is limited to a single point, etc.

>
> > point of view, this approach provides a nice balance between having a
> > completely duplicate driver and having a driver which self-patches the
> > tree.
>
> How duplicate driver got into this? I don't think we talk about the
> same. I meant ID table match data.
> >
>
> Best regards,
> Krzysztof
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

[Krzysztof Kozlowski]

On 22/10/2024 08:34, Imran Shaik wrote:
> 
> 
> On 10/21/2024 8:41 PM, Krzysztof Kozlowski wrote:
>> On 21/10/2024 12:56, Dmitry Baryshkov wrote:
>>>>>   	{ }
>>>>>   };
>>>>> @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
>>>>>   	if (IS_ERR(regmap))
>>>>>   		return PTR_ERR(regmap);
>>>>>   
>>>>> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
>>>>
>>>> Why we cannot use match data? Seeing compatibles in the code is
>>>> unexpected and does not scale.
>>>
>>> Because using match data doesn't scale in such cases. We have been using
>>
>> I don't understand how it could not scale. That's the entire point of
>> match data - scaling.
>>
>>> compatibles to patch clock trees for the platforms for quite a while.
>>> You can see that each of the "tunings" is slightly different. From my
>>
>>
>> You have one driver, where are these tunings which are supposed to be
>> different? You need here only enum or define, in the simplest choice.
>>
>>> point of view, this approach provides a nice balance between having a
>>> completely duplicate driver and having a driver which self-patches the
>>> tree.
>>
>> How duplicate driver got into this? I don't think we talk about the
>> same. I meant ID table match data.
>>>
> 
> I agree with Dmitry. If I understand correctly, to add match data 
> support, we need to define the gpu_cc_qcs8300_clocks struct by 
> duplicating the entries from gpu_cc_sa8775p_clocks and then adding the 
> additional qcs8300 clocks. The compatible approach is simpler and used 
> across most existing platforms.
> 

You don't have to define any structs. You pass enum and retrieve it...

Best regards,
Krzysztof