Re: [PATCH 2/5] clk: qcom: videocc-sm8550: Add support for SM8650 videocc

[Dmitry Baryshkov]

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.

>
> 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?

>  };
>
>  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.

> +               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
>
>


-- 
With best wishes
Dmitry