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

[Dmitry Baryshkov]

On Mon, 17 Apr 2023 at 21:10, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
>
>
> On 14.04.2023 22:54, Dmitry Baryshkov wrote:
> > On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 14.04.2023 18:31, Dmitry Baryshkov wrote:
> >>> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
> >>>>
> >>>> Add support for the Video Clock Controller found on the SM8350 SoC.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> >>>> ---
> >>
> >> [...]
> >>
> >>>> +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..

Ack. Fair enough.

>
> >
> >> [...]
> >>
> >>>> +};
> >>>> +
> >>>> +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?

I'd say, switch to VIDEO_CC prefix, please. We can not drop the
prefix, as we risc getting conflicts otherwise.

>
> >
> >>
> >>>
> >>>> +       [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"..

Let's just drop them for now.


>
> 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!
> >



-- 
With best wishes
Dmitry