Messages in this thread | | | Date | Wed, 7 Feb 2024 12:28:57 +0530 | Subject | Re: [PATCH 2/5] clk: qcom: videocc-sm8550: Add support for SM8650 videocc | From | Jagadeesh Kona <> |
| |
On 2/6/2024 5:24 PM, Dmitry Baryshkov wrote: > On Tue, 6 Feb 2024 at 13:39, Jagadeesh Kona <quic_jkona@quicinc.com> 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.
>> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> 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.
>> }; >> >> 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.
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 >> >> > >
| |