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

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

On 14.04.2023 22:54, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> 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:
> 
> Excuse me, I got confused by all the syllables. I was looking at the
> video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I
> can not say that either one of those is incorrect, but such a
> difference looks a bit suspicious for me. Maybe Tanya or somebody else
> can comment here.
I'd say this could be a design decision, with div2 clocks being
treated differently, but it's how downstream does it on shipping
devices and while generally it's not a great thing to say, it seems
to be the "right enough" thing..

> 
>> [...]
>>
>>>> +};
>>>> +
>>>> +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.
> 
> Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones.
> I think we can follow that.
Or get rid of that, as it's always called with a phandle to videocc..

Thoughts?

> 
>>
>>>
>>>> +       [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.
> 
> Well, those are two separate questions. One is that w/o additional
> pm_clk calls this string is useless (and should be removed). Another
> on is a possible restructure of our cc drivers to use pm_clk for AHB
> clocks (which would require adding more than that).
Right, I had an impression that you needed any sort of pm ops at
all to be registered with pm_genpd correctly, but that seems not to
be the case.. With that commented out, I still see "suspended" / "active"
and not "unsupported"..

Konrad
> 
> 
>>
>>>
>>>> +};
>>>> +
>>>> +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!
> 
> Me too!
>