Re: [PATCH v2 6/6] clk: qcom: Add video clock controller driver for SC7280

[Stephen Boyd]

Quoting Taniya Das (2021-04-27 03:33:10)
> diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
> new file mode 100644
> index 0000000..3387154
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-sc7280.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,videocc-sc7280.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "common.h"
> +#include "reset.h"
> +#include "gdsc.h"
> +
> +enum {
> +       P_BI_TCXO,
> +       P_SLEEP_CLK,
> +       P_VIDEO_PLL0_OUT_EVEN,
> +};
> +
> +static struct pll_vco lucid_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
[...]
> +
> +static const struct parent_map video_cc_parent_map_0[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_VIDEO_PLL0_OUT_EVEN, 3 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_0[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .hw = &video_pll0.clkr.hw },
> +};
> +
> +static const struct parent_map video_cc_parent_map_1[] = {
> +       { P_SLEEP_CLK, 0 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_1[] = {
> +       { .fw_name = "sleep_clk" },
> +};
> +
> +static const struct parent_map video_cc_parent_map_2[] = {
> +       { P_BI_TCXO, 0 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_2_ao[] = {
> +       { .fw_name = "bi_tcxo_ao" },

This is new. Why would we want the video clk parent state to turn off
when the CPU is off? Does the video engine keep XO enabled for itself?
Can you please add some comment into the code explaining why it's ok to
use the ao clk here?

> +};
> +
> +static const struct freq_tbl ftbl_video_cc_iris_clk_src[] = {
> +       F(133333333, P_VIDEO_PLL0_OUT_EVEN, 3, 0, 0),
> +       F(240000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(335000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(424000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(460000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       { }
> +};
[...]
> +
> +static struct clk_branch video_cc_xo_clk = {
> +       .halt_reg = 0x7018,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x7018,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "video_cc_xo_clk",
> +                       .parent_hws = (const struct clk_hw*[]){
> +                               &video_cc_xo_clk_src.clkr.hw,
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,

Please add a comment why it is critical.

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +