Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 14.04.2023 18:31, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> Add support for the Video Clock Controller found on the SM8350 SoC.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---

[...]

>> +static struct clk_rcg2 video_cc_ahb_clk_src = {
>> +       .cmd_rcgr = 0xbd4,
>> +       .mnd_width = 0,
>> +       .hid_width = 5,
>> +       .parent_map = video_cc_parent_map_0,
>> +       .freq_tbl = ftbl_video_cc_ahb_clk_src,
>> +       .clkr.hw.init = &(const struct clk_init_data) {
>> +               .name = "video_cc_ahb_clk_src",
>> +               .parent_data = video_cc_parent_data_0,
>> +               .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
>> +               .flags = CLK_SET_RATE_PARENT,
>> +               .ops = &clk_rcg2_shared_ops,
>> +       },
>> +};
> 
> Do we need this clock at all? We don't have the child
> video_cc_ahb_clk, so potentially CCF can try disabling or modifying
> this clock.
Hm.. I see a few things:

1. downstream kona has it, upstream does not
2. it's shared so we never actually hard-shut it off..
2a. ..but it'd be good to ensure it's on when it's ready..
2b. ..but we never do anyway..
2c. ..but should we even? doesn't Venus govern it internally?


> 
>> +
>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = {
>> +       F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> +       F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> +       F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> +       F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0),
>> +       { }
>> +};
>> +

[...]

>> +static struct clk_branch video_cc_mvs1_clk = {
>> +       .halt_reg = 0xdb4,
>> +       .halt_check = BRANCH_HALT_VOTED,
> 
> As a note, sm8250 has BRANCH_HALT here.
No, it does on the div2 clk, and so do we:
[...]

>> +};
>> +
>> +static struct clk_branch video_cc_mvs1_div2_clk = {
>> +       .halt_reg = 0xdf4,
>> +       .halt_check = BRANCH_HALT_VOTED,
>> +       .hwcg_reg = 0xdf4,

[...]

>> +
>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = {
>> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 },
>> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 },
> 
> Would it be better to use common VIDEO_CC prefix here (IOW:
> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc.
My best guess would be that the ones prefixed with CVP_
are actual INTF/INSTANCEn(CORE) reset lines whereas
the ones containing _CLK_ reset their clock sub-branches.

> 
>> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 },
>> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 },
>> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 },
>> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 },
>> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 },
>> +};

[...]

>> +       ret = pm_runtime_resume_and_get(&pdev->dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc);
>> +       if (IS_ERR(regmap)) {
>> +               pm_runtime_put(&pdev->dev);
>> +               return PTR_ERR(regmap);
>> +       };
> 
> Extra semicolon
Ooeh!

> 
>> +
>> +       clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
>> +       clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
>> +
>> +       /*
>> +        * Keep clocks always enabled:
>> +        *      video_cc_ahb_clk
>> +        *      video_cc_xo_clk
>> +        */
>> +       regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
>> +       regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
>> +
>> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
>> +       pm_runtime_put(&pdev->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> 
> The driver doesn't use pm_clk at all. Are these PM_OPS correct?
I'm unsure. I see the pm state changing in debugfs when the clocks are
(not) consumed. But let's continue our discussion about using pm_clks
for AHB.

> 
>> +};
>> +
>> +static const struct of_device_id video_cc_sm8350_match_table[] = {
>> +       { .compatible = "qcom,sm8350-videocc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table);
>> +
>> +static struct platform_driver video_cc_sm8350_driver = {
>> +       .probe = video_cc_sm8350_probe,
>> +       .driver = {
>> +               .name = "sm8350-videocc",
>> +               .of_match_table = video_cc_sm8350_match_table,
>> +               .pm = &video_cc_sm8350_pm_ops,
>> +       },
>> +};
>> +module_platform_driver(video_cc_sm8350_driver);
>> +
>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.40.0
>>
> 
> Generic note: the register layout follows closely sm8250. However the
> existing differences probably do not warrant merging them.
No, I don't think merging any designs that are farther away
than 8150 and 8155 or 8992 and 8994 etc. is a good idea..

I don't want to ever look at something like dispcc-sm8[123]50.c
again!

Konrad
>