Messages in this thread | | | Date | Mon, 26 Jun 2023 16:40:09 +0300 | Subject | Re: [PATCH V5 4/5] clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks | From | Dmitry Baryshkov <> |
| |
On 26/06/2023 14:59, Jagadeesh Kona wrote: > > > On 6/24/2023 5:49 PM, Konrad Dybcio wrote: >> On 23.06.2023 18:46, Jagadeesh Kona wrote: >>> Add support for camera qdss, sleep and xo clocks. >>> >>> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> >>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> --- >>> Changes since v4: >>> - No changes. >>> Changes since v3: >>> - No changes. >>> Changes since v2: >>> - No changes. >>> Changes since v1: >>> - Newly added. >>> >>> drivers/clk/qcom/camcc-sm8550.c | 180 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 180 insertions(+) >>> >>> diff --git a/drivers/clk/qcom/camcc-sm8550.c >>> b/drivers/clk/qcom/camcc-sm8550.c >>> index 075bea32087c..7b4882444d58 100644 >>> --- a/drivers/clk/qcom/camcc-sm8550.c >>> +++ b/drivers/clk/qcom/camcc-sm8550.c >>> @@ -22,6 +22,8 @@ >>> enum { >>> DT_IFACE, >>> DT_BI_TCXO, >>> + DT_BI_TCXO_AO, >>> + DT_SLEEP_CLK, >>> }; >>> enum { >>> @@ -43,6 +45,7 @@ enum { >>> P_CAM_CC_PLL10_OUT_EVEN, >>> P_CAM_CC_PLL11_OUT_EVEN, >>> P_CAM_CC_PLL12_OUT_EVEN, >>> + P_SLEEP_CLK, >>> }; >>> static const struct pll_vco lucid_ole_vco[] = { >>> @@ -881,6 +884,22 @@ static const struct clk_parent_data >>> cam_cc_parent_data_11[] = { >>> { .hw = &cam_cc_pll7_out_even.clkr.hw }, >>> }; >>> +static const struct parent_map cam_cc_parent_map_12[] = { >>> + { P_SLEEP_CLK, 0 }, >>> +}; >>> + >>> +static const struct clk_parent_data cam_cc_parent_data_12[] = { >>> + { .index = DT_SLEEP_CLK }, >>> +}; >>> + >>> +static const struct parent_map cam_cc_parent_map_13[] = { >>> + { P_BI_TCXO, 0 }, >>> +}; >>> + >>> +static const struct clk_parent_data cam_cc_parent_data_13_ao[] = { >>> + { .index = DT_BI_TCXO_AO }, >>> +}; >>> + >>> static const struct freq_tbl ftbl_cam_cc_bps_clk_src[] = { >>> F(19200000, P_BI_TCXO, 1, 0, 0), >>> F(200000000, P_CAM_CC_PLL8_OUT_EVEN, 1, 0, 0), >>> @@ -1565,6 +1584,29 @@ static struct clk_rcg2 cam_cc_mclk7_clk_src = { >>> }, >>> }; >>> +static const struct freq_tbl ftbl_cam_cc_qdss_debug_clk_src[] = { >>> + F(19200000, P_BI_TCXO, 1, 0, 0), >>> + F(75000000, P_CAM_CC_PLL0_OUT_EVEN, 8, 0, 0), >>> + F(150000000, P_CAM_CC_PLL0_OUT_EVEN, 4, 0, 0), >>> + F(300000000, P_CAM_CC_PLL0_OUT_MAIN, 4, 0, 0), >>> + { } >>> +}; >>> + >>> +static struct clk_rcg2 cam_cc_qdss_debug_clk_src = { >>> + .cmd_rcgr = 0x13f24, >>> + .mnd_width = 0, >>> + .hid_width = 5, >>> + .parent_map = cam_cc_parent_map_0, >>> + .freq_tbl = ftbl_cam_cc_qdss_debug_clk_src, >>> + .clkr.hw.init = &(const struct clk_init_data) { >>> + .name = "cam_cc_qdss_debug_clk_src", >>> + .parent_data = cam_cc_parent_data_0, >>> + .num_parents = ARRAY_SIZE(cam_cc_parent_data_0), >>> + .flags = CLK_SET_RATE_PARENT, >>> + .ops = &clk_rcg2_shared_ops, >>> + }, >>> +}; >>> + >>> static const struct freq_tbl ftbl_cam_cc_sfe_0_clk_src[] = { >>> F(466000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0), >>> F(594000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0), >>> @@ -1611,6 +1653,26 @@ static struct clk_rcg2 cam_cc_sfe_1_clk_src = { >>> }, >>> }; >>> +static const struct freq_tbl ftbl_cam_cc_sleep_clk_src[] = { >>> + F(32000, P_SLEEP_CLK, 1, 0, 0), >>> + { } >>> +}; >>> + >>> +static struct clk_rcg2 cam_cc_sleep_clk_src = { >>> + .cmd_rcgr = 0x141a0, >>> + .mnd_width = 0, >>> + .hid_width = 5, >>> + .parent_map = cam_cc_parent_map_12, >>> + .freq_tbl = ftbl_cam_cc_sleep_clk_src, >>> + .clkr.hw.init = &(const struct clk_init_data) { >>> + .name = "cam_cc_sleep_clk_src", >>> + .parent_data = cam_cc_parent_data_12, >>> + .num_parents = ARRAY_SIZE(cam_cc_parent_data_12), >>> + .flags = CLK_SET_RATE_PARENT, >>> + .ops = &clk_rcg2_shared_ops, >>> + }, >>> +}; >>> + >>> static const struct freq_tbl ftbl_cam_cc_slow_ahb_clk_src[] = { >>> F(19200000, P_BI_TCXO, 1, 0, 0), >>> F(80000000, P_CAM_CC_PLL0_OUT_EVEN, 7.5, 0, 0), >>> @@ -1632,6 +1694,26 @@ static struct clk_rcg2 cam_cc_slow_ahb_clk_src >>> = { >>> }, >>> }; >>> +static const struct freq_tbl ftbl_cam_cc_xo_clk_src[] = { >>> + F(19200000, P_BI_TCXO, 1, 0, 0), >> You're overloading P_BI_TCXO with a different parent clock (XO_A). >> > > This RCG just requires active only voting, hence using XO_A as its parent. > > Both XO and XO_A are same clock in HW (BI_TCXO), hence we can reuse > P_BI_TCXO in frequency table for XO_A parent as well.
Please don't do such things, it complicates understanding the driver. The reviewer could have thought that here the driver was really referencing to the BI_TCXO rather than BI_TCXO_AO.
> > Thanks, > Jagadeesh > >> The rest lgtm >> >> Konrad
[skipped the rest]
-- With best wishes Dmitry
| |