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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA8EJppVUddvAp=3H7oGntE-5XqJkHc7=2mcgpBBnRcsHCDZQg@mail.gmail.com>
Date:   Fri, 14 Apr 2023 23:54:54 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Taniya Das <quic_tdas@...cinc.com>
Cc:     Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Taniya Das <tdas@...eaurora.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC

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

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