Re: [PATCH 2/5] clk: qcom: videocc-sm8550: Add support for SM8650 videocc
From: Jagadeesh Kona
Date: Mon Feb 12 2024 - 08:07:50 EST
On 2/7/2024 12:49 PM, Dmitry Baryshkov wrote:
On Wed, 7 Feb 2024 at 08:59, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
On 2/6/2024 5:24 PM, Dmitry Baryshkov wrote:
On Tue, 6 Feb 2024 at 13:39, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
Add support to the SM8650 video clock controller by extending the
SM8550 video clock controller, which is mostly identical but SM8650
has few additional clocks and minor differences.
In the past we tried merging similar clock controllers. In the end
this results in the ugly source code. Please consider submitting a
separate driver.
Thanks Dmitry for your review. SM8650 has only few clock additions and
minor changes compared to SM8550, so I believe it is better to reuse
this existing driver and extend it.
I'd say, the final decision is on Bjorn and Konrad as maintainers.
Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
---
drivers/clk/qcom/videocc-sm8550.c | 160 +++++++++++++++++++++++++++++-
1 file changed, 156 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
index f3c9dfaee968..cdc08f5900fc 100644
--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <linux/clk-provider.h>
[skipping]
static struct gdsc video_cc_mvs0c_gdsc = {
.gdscr = 0x804c,
.en_rest_wait_val = 0x2,
@@ -354,15 +481,20 @@ static struct clk_regmap *video_cc_sm8550_clocks[] = {
[VIDEO_CC_MVS0_CLK] = &video_cc_mvs0_clk.clkr,
[VIDEO_CC_MVS0_CLK_SRC] = &video_cc_mvs0_clk_src.clkr,
[VIDEO_CC_MVS0_DIV_CLK_SRC] = &video_cc_mvs0_div_clk_src.clkr,
+ [VIDEO_CC_MVS0_SHIFT_CLK] = &video_cc_mvs0_shift_clk.clkr,
[VIDEO_CC_MVS0C_CLK] = &video_cc_mvs0c_clk.clkr,
[VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC] = &video_cc_mvs0c_div2_div_clk_src.clkr,
+ [VIDEO_CC_MVS0C_SHIFT_CLK] = &video_cc_mvs0c_shift_clk.clkr,
[VIDEO_CC_MVS1_CLK] = &video_cc_mvs1_clk.clkr,
[VIDEO_CC_MVS1_CLK_SRC] = &video_cc_mvs1_clk_src.clkr,
[VIDEO_CC_MVS1_DIV_CLK_SRC] = &video_cc_mvs1_div_clk_src.clkr,
+ [VIDEO_CC_MVS1_SHIFT_CLK] = &video_cc_mvs1_shift_clk.clkr,
[VIDEO_CC_MVS1C_CLK] = &video_cc_mvs1c_clk.clkr,
[VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC] = &video_cc_mvs1c_div2_div_clk_src.clkr,
+ [VIDEO_CC_MVS1C_SHIFT_CLK] = &video_cc_mvs1c_shift_clk.clkr,
[VIDEO_CC_PLL0] = &video_cc_pll0.clkr,
[VIDEO_CC_PLL1] = &video_cc_pll1.clkr,
+ [VIDEO_CC_XO_CLK_SRC] = &video_cc_xo_clk_src.clkr,
};
static struct gdsc *video_cc_sm8550_gdscs[] = {
@@ -380,6 +512,7 @@ static const struct qcom_reset_map video_cc_sm8550_resets[] = {
[CVP_VIDEO_CC_MVS1C_BCR] = { 0x8074 },
[VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
[VIDEO_CC_MVS1C_CLK_ARES] = { 0x8090, 2 },
+ [VIDEO_CC_XO_CLK_ARES] = { 0x8124, 2 },
Is this reset applicable to videocc-sm8550?
SM8550 also has above reset support in hardware, hence it is safe to
model above reset for both SM8550 and SM8650.
Then, separate commit, Fixes tag.
Sure, will separate and add Fixes tag in next series.
};
static const struct regmap_config video_cc_sm8550_regmap_config = {
@@ -402,6 +535,7 @@ static struct qcom_cc_desc video_cc_sm8550_desc = {
static const struct of_device_id video_cc_sm8550_match_table[] = {
{ .compatible = "qcom,sm8550-videocc" },
+ { .compatible = "qcom,sm8650-videocc" },
{ }
};
MODULE_DEVICE_TABLE(of, video_cc_sm8550_match_table);
@@ -410,6 +544,7 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
{
struct regmap *regmap;
int ret;
+ u32 offset;
ret = devm_pm_runtime_enable(&pdev->dev);
if (ret)
@@ -425,6 +560,23 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
return PTR_ERR(regmap);
}
+ if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8550-videocc")) {
+ video_cc_sm8550_clocks[VIDEO_CC_MVS0_SHIFT_CLK] = NULL;
+ video_cc_sm8550_clocks[VIDEO_CC_MVS0C_SHIFT_CLK] = NULL;
+ video_cc_sm8550_clocks[VIDEO_CC_MVS1_SHIFT_CLK] = NULL;
+ video_cc_sm8550_clocks[VIDEO_CC_MVS1C_SHIFT_CLK] = NULL;
+ video_cc_sm8550_clocks[VIDEO_CC_XO_CLK_SRC] = NULL;
Please invert the logic. Make video_cc_sm8550_clocks reflect SM8550
and patch in new clocks in the SM8650-specific branch below.
Sure, will add these clocks as NULL in video_cc_sm8550_clocks and patch
in new clocks here for SM8650. Then we can remove above check for SM8550.
No need to set them to NULL, it is the default value. Just add them to
the sm8650 branch.
The video_cc_sm8550_clocks[] array size is fixed and has memory
allocated only for current sm8550 clocks. To be able to accommodate
sm8650 clocks in the same array, we need to initialize the clocks to
NULL as below snippet to increase the array size.
static struct clk_regmap *video_cc_sm8550_clocks[] = {
....
[VIDEO_CC_XO_CLK_SRC] = NULL,
}
Thanks,
Jagadeesh
Thanks,
Jagadeesh
+ offset = 0x8140;
+ } else if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8650-videocc")) {
+ video_cc_pll0_config.l = 0x1e;
+ video_cc_pll0_config.alpha = 0xa000;
+ video_cc_pll1_config.l = 0x2b;
+ video_cc_pll1_config.alpha = 0xc000;
+ video_cc_mvs0_clk_src.freq_tbl = ftbl_video_cc_mvs0_clk_src_sm8650;
+ video_cc_mvs1_clk_src.freq_tbl = ftbl_video_cc_mvs1_clk_src_sm8650;
+ offset = 0x8150;
+ }
+
clk_lucid_ole_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
clk_lucid_ole_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
@@ -435,7 +587,7 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
* video_cc_xo_clk
*/
regmap_update_bits(regmap, 0x80f4, BIT(0), BIT(0));
- regmap_update_bits(regmap, 0x8140, BIT(0), BIT(0));
+ regmap_update_bits(regmap, offset, BIT(0), BIT(0));
regmap_update_bits(regmap, 0x8124, BIT(0), BIT(0));
ret = qcom_cc_really_probe(pdev, &video_cc_sm8550_desc, regmap);
--
2.43.0