Re: [PATCH V4 2/4] clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550

Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> · Wed, 14 Jun 2023 17:25:29 +0530

On 6/9/2023 9:52 PM, Dmitry Baryshkov wrote:
On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:

Add support for the camera clock controller for camera clients to be
able to request for camcc clocks on SM8550 platform.

Co-developed-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
---
Changes since V3:
  - No changes
Changes since V2:
  - No changes
Changes since V1:
  - Sorted the PLL names in proper order
  - Updated all PLL configurations to lower case hex
  - Reused evo ops instead of adding new ops for ole pll
  - Moved few clocks to separate patch to fix patch too long error

  drivers/clk/qcom/Kconfig        |    7 +
  drivers/clk/qcom/Makefile       |    1 +
  drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++
  3 files changed, 3413 insertions(+)
  create mode 100644 drivers/clk/qcom/camcc-sm8550.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 9cd1f05d436b..85efed78dc9a 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -756,6 +756,13 @@ config SM_CAMCC_8450
           Support for the camera clock controller on SM8450 devices.
           Say Y if you want to support camera devices and camera functionality.

+config SM_CAMCC_8550
+       tristate "SM8550 Camera Clock Controller"
+       select SM_GCC_8550
+       help
+         Support for the camera clock controller on SM8550 devices.
+         Say Y if you want to support camera devices and camera functionality.
+
  config SM_DISPCC_6115
         tristate "SM6115 Display Clock Controller"
         depends on ARM64 || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 75d035150118..97c8cefc2fd0 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.o
  obj-$(CONFIG_SM_CAMCC_6350) += camcc-sm6350.o
  obj-$(CONFIG_SM_CAMCC_8250) += camcc-sm8250.o
  obj-$(CONFIG_SM_CAMCC_8450) += camcc-sm8450.o
+obj-$(CONFIG_SM_CAMCC_8550) += camcc-sm8550.o
  obj-$(CONFIG_SM_DISPCC_6115) += dispcc-sm6115.o
  obj-$(CONFIG_SM_DISPCC_6125) += dispcc-sm6125.o
  obj-$(CONFIG_SM_DISPCC_6350) += dispcc-sm6350.o
diff --git a/drivers/clk/qcom/camcc-sm8550.c b/drivers/clk/qcom/camcc-sm8550.c
new file mode 100644
index 000000000000..85f0c1e09b2b
--- /dev/null
+++ b/drivers/clk/qcom/camcc-sm8550.c
@@ -0,0 +1,3405 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,sm8550-camcc.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "common.h"
+#include "gdsc.h"
+#include "reset.h"
+
+enum {
+       DT_IFACE,
+       DT_BI_TCXO,
+};
+
+enum {
+       P_BI_TCXO,
+       P_CAM_CC_PLL0_OUT_EVEN,
+       P_CAM_CC_PLL0_OUT_MAIN,
+       P_CAM_CC_PLL0_OUT_ODD,
+       P_CAM_CC_PLL1_OUT_EVEN,
+       P_CAM_CC_PLL2_OUT_EVEN,
+       P_CAM_CC_PLL2_OUT_MAIN,
+       P_CAM_CC_PLL3_OUT_EVEN,
+       P_CAM_CC_PLL4_OUT_EVEN,
+       P_CAM_CC_PLL5_OUT_EVEN,
+       P_CAM_CC_PLL6_OUT_EVEN,
+       P_CAM_CC_PLL7_OUT_EVEN,
+       P_CAM_CC_PLL8_OUT_EVEN,
+       P_CAM_CC_PLL9_OUT_EVEN,
+       P_CAM_CC_PLL9_OUT_ODD,
+       P_CAM_CC_PLL10_OUT_EVEN,
+       P_CAM_CC_PLL11_OUT_EVEN,
+       P_CAM_CC_PLL12_OUT_EVEN,
+};
+
+static const struct pll_vco lucid_ole_vco[] = {
+       { 249600000, 2300000000, 0 },
+};
+
+static const struct pll_vco rivian_ole_vco[] = {
+       { 777000000, 1285000000, 0 },
+};
+
+static const struct alpha_pll_config cam_cc_pll0_config = {
+       /* .l includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */
+       .l = 0x4444003e,

I'd still insist on not touching the config.l field semantics.


We feel it is better to update config->l field and reuse existing code 
than adding separate function for lucid ole pll configure.

+       .alpha = 0x8000,
+       .config_ctl_val = 0x20485699,
+       .config_ctl_hi_val = 0x00182261,
+       .config_ctl_hi1_val = 0x82aa299c,
+       .test_ctl_val = 0x00000000,
+       .test_ctl_hi_val = 0x00000003,
+       .test_ctl_hi1_val = 0x00009000,
+       .test_ctl_hi2_val = 0x00000034,
+       .user_ctl_val = 0x00008400,
+       .user_ctl_hi_val = 0x00000005,
+};
+

[skipped the rest, LGTM]

+
+static struct platform_driver cam_cc_sm8550_driver = {
+       .probe = cam_cc_sm8550_probe,
+       .driver = {
+               .name = "cam_cc-sm8550",
+               .of_match_table = cam_cc_sm8550_match_table,
+       },
+};
+
+static int __init cam_cc_sm8550_init(void)
+{
+       return platform_driver_register(&cam_cc_sm8550_driver);
+}
+subsys_initcall(cam_cc_sm8550_init);

As it was pointed out, this driver is built as a module by default.
Please perform the tesing and cleanup before sending the driver and
use module_platform_driver.


We want clock drivers to be probed early in the bootup to avoid any 
probe deferrals of consumer drivers. If there is any scenario where 
clock drivers are built statically into kernel, then subsys_initcall() 
will ensure clock drivers are probed earlier. When built as module, 
subsys_initcall() will fallback to module_init() which is same as 
module_platform_driver().

Thanks,
Jagadeesh

+
+static void __exit cam_cc_sm8550_exit(void)
+{
+       platform_driver_unregister(&cam_cc_sm8550_driver);
+}
+module_exit(cam_cc_sm8550_exit);
+
+MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver");
+MODULE_LICENSE("GPL");
--
2.40.1









