Re: [PATCH V5 4/5] clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks

[Jagadeesh Kona <quic_jkona@quicinc.com>]

On 6/26/2023 7:10 PM, Dmitry Baryshkov wrote:
> 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.
> 

The enum in parent list indicates the actual HW clock, and since XO and 
XO_A are the same HW clock, parent enum needs to be a single one. Only 
parent_data needs to be updated with AO as we have been doing for all 
targets.

Thanks,
Jagadeesh

>>
>> Thanks,
>> Jagadeesh
>>
>>> The rest lgtm
>>>
>>> Konrad
> 
> [skipped the rest]
>