Hello Stephen, Thanks for your review. On 6/2/2021 12:36 PM, Stephen Boyd wrote:
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?
Yes, next patch will have this updated.
+ { 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?
This clock is from HW design is enabled, thus our generation scripts have marked it to _ao. But I will be removing this in the next patch as it is not required to be modelled.
+}; + +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.
Next patch will remove this clock. As it is already enabled from HW, will not model the clock to not disable it from SW.
+ .ops = &clk_branch2_ops, + }, + }, +}; +
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
