Received-SPF: pass (google.com: domain of linux-kernel+bounces-64749-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <6cb25cb0-9885-4b83-beb8-0270e21c978c@quicinc.com>
Date: Wed, 14 Feb 2024 10:59:01 +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>
 <d88f0f42-c9ec-4638-8090-055bc4806574@quicinc.com>
 <CAA8EJpq9AE_B9rvXRa1Q803yWzmwZxwiF_hwokq8XJZgJy59PA@mail.gmail.com>
From: Jagadeesh Kona <quic_jkona@quicinc.com>
In-Reply-To: <CAA8EJpq9AE_B9rvXRa1Q803yWzmwZxwiF_hwokq8XJZgJy59PA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/12/2024 6:48 PM, Dmitry Baryshkov wrote:
> On Mon, 12 Feb 2024 at 15:07, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> 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,
>> }
> 
> The question/comment was regarding video_cc_sm8550_probe() rather than
> video_cc_sm8550_clocks.
> 

Ok thanks, will update the change as per above comments in next series.

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