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