Messages in this thread | | | Subject | Re: [PATCH v2 6/6] clk: qcom: Add video clock controller driver for SC7280 | From | Taniya Das <> | Date | Tue, 13 Jul 2021 08:30:39 +0530 |
| |
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.
--
| |