Received-SPF: pass (google.com: domain of linux-kernel+bounces-61600-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d88f0f42-c9ec-4638-8090-055bc4806574@quicinc.com>
Date: Mon, 12 Feb 2024 18:36:31 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/5] clk: qcom: videocc-sm8550: Add support for SM8650
 videocc
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio
	<konrad.dybcio@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh@kernel.org>,
        "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley
	<conor+dt@kernel.org>,
        Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
        Taniya Das <quic_tdas@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Imran Shaik <quic_imrashai@quicinc.com>,
        "Ajit Pandey" <quic_ajipan@quicinc.com>
References: <20240206113145.31096-1-quic_jkona@quicinc.com>
 <20240206113145.31096-3-quic_jkona@quicinc.com>
 <CAA8EJpqbKQS7Bp28xNZ0twu7BFLdOES9qS5xBvoonux8Ma4q6Q@mail.gmail.com>
 <e90522c1-7a2d-40ff-bf4e-c8f974722ddf@quicinc.com>
 <CAA8EJpqCDOE_5vg+4ew8H0HbhQM1w8reqU6Pu0MAYJtMw8zXUw@mail.gmail.com>
From: Jagadeesh Kona <quic_jkona@quicinc.com>
In-Reply-To: <CAA8EJpqCDOE_5vg+4ew8H0HbhQM1w8reqU6Pu0MAYJtMw8zXUw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/7/2024 12:49 PM, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 08:59, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 2/6/2024 5:24 PM, Dmitry Baryshkov wrote:
>>> On Tue, 6 Feb 2024 at 13:39, Jagadeesh Kona <quic_jkona@quicinc.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@quicinc.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,
}

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