| 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: <CAA8EJpq9AE_B9rvXRa1Q803yWzmwZxwiF_hwokq8XJZgJy59PA@mail.gmail.com>
Date: Mon, 12 Feb 2024 15:18:27 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jagadeesh Kona <quic_jkona@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
Taniya Das <quic_tdas@...cinc.com>, linux-arm-msm@...r.kernel.org,
linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Imran Shaik <quic_imrashai@...cinc.com>,
Ajit Pandey <quic_ajipan@...cinc.com>
Subject: Re: [PATCH 2/5] clk: qcom: videocc-sm8550: Add support for SM8650 videocc
On Mon, 12 Feb 2024 at 15:07, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
>
>
>
> On 2/7/2024 12:49 PM, Dmitry Baryshkov wrote:
> > On Wed, 7 Feb 2024 at 08:59, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
> >>
> >>
> >>
> >> On 2/6/2024 5:24 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 6 Feb 2024 at 13:39, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
> >>>>
> >>>> Add support to the SM8650 video clock controller by extending the
> >>>> SM8550 video clock controller, which is mostly identical but SM8650
> >>>> has few additional clocks and minor differences.
> >>>
> >>> In the past we tried merging similar clock controllers. In the end
> >>> this results in the ugly source code. Please consider submitting a
> >>> separate driver.
> >>>
> >>
> >> Thanks Dmitry for your review. SM8650 has only few clock additions and
> >> minor changes compared to SM8550, so I believe it is better to reuse
> >> this existing driver and extend it.
> >
> > I'd say, the final decision is on Bjorn and Konrad as maintainers.
> >
> >>
> >>>>
> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.com>
> >>>> ---
> >>>> drivers/clk/qcom/videocc-sm8550.c | 160 +++++++++++++++++++++++++++++-
> >>>> 1 file changed, 156 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
> >>>> index f3c9dfaee968..cdc08f5900fc 100644
> >>>> --- a/drivers/clk/qcom/videocc-sm8550.c
> >>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
> >>>> @@ -1,6 +1,6 @@
> >>>> // SPDX-License-Identifier: GPL-2.0-only
> >>>> /*
> >>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> >>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> >>>> */
> >>>>
> >>>> #include <linux/clk-provider.h>
> >>>
> >>> [skipping]
> >>>
> >>>> static struct gdsc video_cc_mvs0c_gdsc = {
> >>>> .gdscr = 0x804c,
> >>>> .en_rest_wait_val = 0x2,
> >>>> @@ -354,15 +481,20 @@ static struct clk_regmap *video_cc_sm8550_clocks[] = {
> >>>> [VIDEO_CC_MVS0_CLK] = &video_cc_mvs0_clk.clkr,
> >>>> [VIDEO_CC_MVS0_CLK_SRC] = &video_cc_mvs0_clk_src.clkr,
> >>>> [VIDEO_CC_MVS0_DIV_CLK_SRC] = &video_cc_mvs0_div_clk_src.clkr,
> >>>> + [VIDEO_CC_MVS0_SHIFT_CLK] = &video_cc_mvs0_shift_clk.clkr,
> >>>> [VIDEO_CC_MVS0C_CLK] = &video_cc_mvs0c_clk.clkr,
> >>>> [VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC] = &video_cc_mvs0c_div2_div_clk_src.clkr,
> >>>> + [VIDEO_CC_MVS0C_SHIFT_CLK] = &video_cc_mvs0c_shift_clk.clkr,
> >>>> [VIDEO_CC_MVS1_CLK] = &video_cc_mvs1_clk.clkr,
> >>>> [VIDEO_CC_MVS1_CLK_SRC] = &video_cc_mvs1_clk_src.clkr,
> >>>> [VIDEO_CC_MVS1_DIV_CLK_SRC] = &video_cc_mvs1_div_clk_src.clkr,
> >>>> + [VIDEO_CC_MVS1_SHIFT_CLK] = &video_cc_mvs1_shift_clk.clkr,
> >>>> [VIDEO_CC_MVS1C_CLK] = &video_cc_mvs1c_clk.clkr,
> >>>> [VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC] = &video_cc_mvs1c_div2_div_clk_src.clkr,
> >>>> + [VIDEO_CC_MVS1C_SHIFT_CLK] = &video_cc_mvs1c_shift_clk.clkr,
> >>>> [VIDEO_CC_PLL0] = &video_cc_pll0.clkr,
> >>>> [VIDEO_CC_PLL1] = &video_cc_pll1.clkr,
> >>>> + [VIDEO_CC_XO_CLK_SRC] = &video_cc_xo_clk_src.clkr,
> >>>> };
> >>>>
> >>>> static struct gdsc *video_cc_sm8550_gdscs[] = {
> >>>> @@ -380,6 +512,7 @@ static const struct qcom_reset_map video_cc_sm8550_resets[] = {
> >>>> [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8074 },
> >>>> [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
> >>>> [VIDEO_CC_MVS1C_CLK_ARES] = { 0x8090, 2 },
> >>>> + [VIDEO_CC_XO_CLK_ARES] = { 0x8124, 2 },
> >>>
> >>> Is this reset applicable to videocc-sm8550?
> >>>
> >>
> >> SM8550 also has above reset support in hardware, hence it is safe to
> >> model above reset for both SM8550 and SM8650.
> >
> > Then, separate commit, Fixes tag.
> >
>
> Sure, will separate and add Fixes tag in next series.
>
> >>
> >>>> };
> >>>>
> >>>> static const struct regmap_config video_cc_sm8550_regmap_config = {
> >>>> @@ -402,6 +535,7 @@ static struct qcom_cc_desc video_cc_sm8550_desc = {
> >>>>
> >>>> static const struct of_device_id video_cc_sm8550_match_table[] = {
> >>>> { .compatible = "qcom,sm8550-videocc" },
> >>>> + { .compatible = "qcom,sm8650-videocc" },
> >>>> { }
> >>>> };
> >>>> MODULE_DEVICE_TABLE(of, video_cc_sm8550_match_table);
> >>>> @@ -410,6 +544,7 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >>>> {
> >>>> struct regmap *regmap;
> >>>> int ret;
> >>>> + u32 offset;
> >>>>
> >>>> ret = devm_pm_runtime_enable(&pdev->dev);
> >>>> if (ret)
> >>>> @@ -425,6 +560,23 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >>>> return PTR_ERR(regmap);
> >>>> }
> >>>>
> >>>> + if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8550-videocc")) {
> >>>> + video_cc_sm8550_clocks[VIDEO_CC_MVS0_SHIFT_CLK] = NULL;
> >>>> + video_cc_sm8550_clocks[VIDEO_CC_MVS0C_SHIFT_CLK] = NULL;
> >>>> + video_cc_sm8550_clocks[VIDEO_CC_MVS1_SHIFT_CLK] = NULL;
> >>>> + video_cc_sm8550_clocks[VIDEO_CC_MVS1C_SHIFT_CLK] = NULL;
> >>>> + video_cc_sm8550_clocks[VIDEO_CC_XO_CLK_SRC] = NULL;
> >>>
> >>> Please invert the logic. Make video_cc_sm8550_clocks reflect SM8550
> >>> and patch in new clocks in the SM8650-specific branch below.
> >>>
> >>
> >> Sure, will add these clocks as NULL in video_cc_sm8550_clocks and patch
> >> in new clocks here for SM8650. Then we can remove above check for SM8550.
> >
> > No need to set them to NULL, it is the default value. Just add them to
> > the sm8650 branch.
> >
>
> The video_cc_sm8550_clocks[] array size is fixed and has memory
> allocated only for current sm8550 clocks. To be able to accommodate
> sm8650 clocks in the same array, we need to initialize the clocks to
> NULL as below snippet to increase the array size.
>
> static struct clk_regmap *video_cc_sm8550_clocks[] = {
> .....
> [VIDEO_CC_XO_CLK_SRC] = NULL,
> }
The question/comment was regarding video_cc_sm8550_probe() rather than
video_cc_sm8550_clocks.
>
> Thanks,
> Jagadeesh
>
> >>
> >> Thanks,
> >> Jagadeesh
> >>
> >>>> + offset = 0x8140;
> >>>> + } else if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8650-videocc")) {
> >>>> + video_cc_pll0_config.l = 0x1e;
> >>>> + video_cc_pll0_config.alpha = 0xa000;
> >>>> + video_cc_pll1_config.l = 0x2b;
> >>>> + video_cc_pll1_config.alpha = 0xc000;
> >>>> + video_cc_mvs0_clk_src.freq_tbl = ftbl_video_cc_mvs0_clk_src_sm8650;
> >>>> + video_cc_mvs1_clk_src.freq_tbl = ftbl_video_cc_mvs1_clk_src_sm8650;
> >>>> + offset = 0x8150;
> >>>> + }
> >>>> +
> >>>> clk_lucid_ole_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
> >>>> clk_lucid_ole_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
> >>>>
> >>>> @@ -435,7 +587,7 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
> >>>> * video_cc_xo_clk
> >>>> */
> >>>> regmap_update_bits(regmap, 0x80f4, BIT(0), BIT(0));
> >>>> - regmap_update_bits(regmap, 0x8140, BIT(0), BIT(0));
> >>>> + regmap_update_bits(regmap, offset, BIT(0), BIT(0));
> >>>> regmap_update_bits(regmap, 0x8124, BIT(0), BIT(0));
> >>>>
> >>>> ret = qcom_cc_really_probe(pdev, &video_cc_sm8550_desc, regmap);
> >>>> --
> >>>> 2.43.0
> >>>>
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
Powered by blists - more mailing lists