| 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
| ||
|
Message-ID: <34797b11-b654-a9a4-ac26-5287ca582a82@linaro.org>
Date: Fri, 14 Apr 2023 19:48:03 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
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 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:
[...]
>> +};
>> +
>> +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
>
Powered by blists - more mailing lists