Re: [PATCH 04/12] clk: qcom: camcc-qcs615: Add QCS615 camera clock controller driver

Taniya Das <quic_tdas@xxxxxxxxxxx> · Mon, 21 Oct 2024 13:48:51 +0530

On 10/19/2024 6:23 AM, Bryan O'Donoghue wrote:
On 18/10/2024 20:12, Taniya Das wrote:
Add support for the camera clock controller for camera clients to
be able to request for camcc clocks on QCS615 platform.

Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
---
  drivers/clk/qcom/Kconfig        |   10 +
  drivers/clk/qcom/Makefile       |    1 +
  drivers/clk/qcom/camcc-qcs615.c | 1588 
+++++++++++++++++++++++++++++++++++++++
  3 files changed, 1599 insertions(+)

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 
30eb8236c9d80071a87e0332cfac7b667a08824a..bdb1c672dd90d96814b214afd234341e37e3c470 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -460,6 +460,16 @@ config QCM_DISPCC_2290
        Say Y if you want to support display devices and functionality 
such as
        splash screen.
+config QCS_CAMCC_615
+    tristate "QCS615 Camera Clock Controller"
+    depends on ARM64 || COMPILE_TEST
+    select QCS_GCC_615
+    help
+      Support for the camera clock controller on Qualcomm 
Technologies, Inc
+      QCS615 devices.
+      Say Y if you want to support camera devices and functionality 
such as
+      capturing pictures.
+
  config QCS_GCC_404
      tristate "QCS404 Global Clock Controller"
      help
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 
2b378667a63ff6eca843d7bef638a5422d35c3d3..f69c1bc13d3eca1859d9e849399e55175df869c3 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
  obj-$(CONFIG_QCM_GCC_2290) += gcc-qcm2290.o
  obj-$(CONFIG_QCM_DISPCC_2290) += dispcc-qcm2290.o
+obj-$(CONFIG_QCS_CAMCC_615) += camcc-qcs615.o
  obj-$(CONFIG_QCS_GCC_404) += gcc-qcs404.o
  obj-$(CONFIG_QCS_Q6SSTOP_404) += q6sstop-qcs404.o
  obj-$(CONFIG_QCS_TURING_404) += turingcc-qcs404.o
diff --git a/drivers/clk/qcom/camcc-qcs615.c 
b/drivers/clk/qcom/camcc-qcs615.c
new file mode 100644
index 
0000000000000000000000000000000000000000..2341ddb57598eaaa7fa35300ae6635ff40da99ae
--- /dev/null
+++ b/drivers/clk/qcom/camcc-qcs615.c
@@ -0,0 +1,1588 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights 
reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,qcs615-camcc.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "clk-regmap-divider.h"
+#include "clk-regmap-mux.h"
+#include "common.h"
+#include "gdsc.h"
+#include "reset.h"
+
+enum {
+    DT_BI_TCXO,
+    DT_BI_TCXO_AO,
+};
+
+enum {
+    P_BI_TCXO,
+    P_CAM_CC_PLL0_OUT_AUX,
+    P_CAM_CC_PLL1_OUT_AUX,
+    P_CAM_CC_PLL2_OUT_AUX2,
+    P_CAM_CC_PLL2_OUT_EARLY,
+    P_CAM_CC_PLL3_OUT_MAIN,
+};
+
+static const struct pll_vco brammo_vco[] = {
+    { 500000000, 1250000000, 0 },
+};
+
+static const struct pll_vco spark_vco[] = {
+    { 1000000000, 2100000000, 0 },
+    { 750000000, 1500000000, 1 },
+    { 500000000, 1000000000, 2 },
+    { 300000000, 500000000, 3 },
+    { 550000000, 1100000000, 4 },
+};
+
+/* 600MHz configuration */
+static const struct alpha_pll_config cam_cc_pll0_config = {
+    .l = 0x1f,
+    .alpha_hi = 0x40,
+    .alpha_en_mask = BIT(24),
+    .vco_val = 0x2 << 20,
+    .vco_mask = 0x3 << 20,
+    .aux_output_mask = BIT(1),
+    .config_ctl_val = 0x4001055b,
+    .test_ctl_hi_val = 0x1,
+    .test_ctl_hi_mask = 0x1,
+};
+
+static struct clk_alpha_pll cam_cc_pll0 = {
+    .offset = 0x0,
+    .vco_table = spark_vco,
+    .num_vco = ARRAY_SIZE(spark_vco),
+    .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
+    .clkr = {
+        .hw.init = &(const struct clk_init_data) {
+            .name = "cam_cc_pll0",
+            .parent_data = &(const struct clk_parent_data) {
+                .index = DT_BI_TCXO,
+            },
+            .num_parents = 1,
+            .ops = &clk_alpha_pll_ops,
+        },
+    },
+};
+
+/* 808MHz configuration */
+static struct alpha_pll_config cam_cc_pll1_config = {
+    .l = 0x2A,
+    .alpha_hi = 0x15,
+    .alpha = 0x55555555,
+    .alpha_en_mask = BIT(24),
+    .vco_val = 0x2 << 20,
+    .vco_mask = 0x3 << 20,
+    .aux_output_mask = BIT(1),
+    .config_ctl_val = 0x4001055b,
+    .test_ctl_hi_val = 0x1,
+    .test_ctl_hi_mask = 0x1,
+};
+
+static struct clk_alpha_pll cam_cc_pll1 = {
+    .offset = 0x1000,
+    .vco_table = spark_vco,
+    .num_vco = ARRAY_SIZE(spark_vco),
+    .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
+    .clkr = {
+        .hw.init = &(const struct clk_init_data) {
+            .name = "cam_cc_pll1",
+            .parent_data = &(const struct clk_parent_data) {
+                .index = DT_BI_TCXO,
+            },
+            .num_parents = 1,
+            .ops = &clk_alpha_pll_ops,
+        },
+    },
+};
+
+/* 960MHz configuration */
+static struct alpha_pll_config cam_cc_pll2_config = {
+    .l = 0x32,
+    .vco_val = 0x0 << 20,

zero shifted any direction is still zero

zed.c

#include <stdio.h>
#include <stdint.h>

int main(int argc, char *argv[])
{
     uint32_t a = 0, b = 0 << 20;

     printf("a = %d b = %d\n", a, b);

     return 0;
}

gcc -o zed zed.c

a = 0 b = 0

+static struct gdsc bps_gdsc = {
+    .gdscr = 0x6004,
+    .en_rest_wait_val = 0x2,
+    .en_few_wait_val = 0x2,
+    .clk_dis_wait_val = 0xf,
+    .pd = {
+        .name = "bps_gdsc",
+    },
+    .pwrsts = PWRSTS_OFF_ON,
+    .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
+};
+
+static struct gdsc ife_0_gdsc = {
+    .gdscr = 0x9004,
+    .en_rest_wait_val = 0x2,
+    .en_few_wait_val = 0x2,
+    .clk_dis_wait_val = 0xf,
+    .pd = {
+        .name = "ife_0_gdsc",
+    },
+    .pwrsts = PWRSTS_OFF_ON,
+    .flags = POLL_CFG_GDSCR,
+};
+
+static struct gdsc ife_1_gdsc = {
+    .gdscr = 0xa004,
+    .en_rest_wait_val = 0x2,
+    .en_few_wait_val = 0x2,
+    .clk_dis_wait_val = 0xf,
+    .pd = {
+        .name = "ife_1_gdsc",
+    },
+    .pwrsts = PWRSTS_OFF_ON,
+    .flags = POLL_CFG_GDSCR,
+};

Shouldn't these have RETAIN flags ?

+
+static struct gdsc ipe_0_gdsc = {
+    .gdscr = 0x7004,
+    .en_rest_wait_val = 0x2,
+    .en_few_wait_val = 0x2,
+    .clk_dis_wait_val = 0xf,
+    .pd = {
+        .name = "ipe_0_gdsc",
+    },
+    .pwrsts = PWRSTS_OFF_ON,
+    .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
+};

I'd say those flags are very aspirational suggest POLL_CFG_GDSCR | 
RETAIN_FF_ENABLE.


+
+static struct gdsc titan_top_gdsc = {
+    .gdscr = 0xb134,
+    .en_rest_wait_val = 0x2,
+    .en_few_wait_val = 0x2,
+    .clk_dis_wait_val = 0xf,
+    .pd = {
+        .name = "titan_top_gdsc",
+    },
+    .pwrsts = PWRSTS_OFF_ON,
+    .flags = POLL_CFG_GDSCR,
+};


As Dmitry queried, TOP_GDSC should almost certainly be the parent of the 
IFE/IPE/BPS and others.

+static int cam_cc_qcs615_probe(struct platform_device *pdev)
+{
+    struct regmap *regmap;
+
+    regmap = qcom_cc_map(pdev, &cam_cc_qcs615_desc);
+    if (IS_ERR(regmap))
+        return PTR_ERR(regmap);
+
+    clk_alpha_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
+    clk_alpha_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
+    clk_alpha_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
+    clk_alpha_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);

Got to be missing something like

         /* Keep some clocks always-on */
         qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */

If the GDSC gets declocked everything beneath it - including the stuff 
in RETAIN goes away...

Smells wrong.

---
bod

Please help review: 
https://patchwork.kernel.org/project/linux-clk/cover/20241019-qcs615-mm-clockcontroller-v1-0-4cfb96d779ae@xxxxxxxxxxx/ 


--
Thanks & Regards,
Taniya Das.







