Message-ID: <a890fd4c-f15e-480a-64e3-c42c73584417@collabora.com>
Date:   Tue, 7 Mar 2023 10:27:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 7/7] clk: mediatek: mt8195: Add support for frequency
 hopping through FHCTL
To:     Chen-Yu Tsai <wenst@chromium.org>
Cc:     sboyd@kernel.org, mturquette@baylibre.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
        edward-jw.yang@mediatek.com, johnson.wang@mediatek.com,
        miles.chen@mediatek.com, chun-jie.chen@mediatek.com,
        rex-bc.chen@mediatek.com, jose.exposito89@gmail.com,
        linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com
References: <20230206100105.861720-1-angelogioacchino.delregno@collabora.com>
 <20230206100105.861720-8-angelogioacchino.delregno@collabora.com>
 <CAGXv+5Ey02hzNQA2O2ussAt9sXxxy9mOjze2GH90Ko9HeoETXQ@mail.gmail.com>
Content-Language: en-US
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <CAGXv+5Ey02hzNQA2O2ussAt9sXxxy9mOjze2GH90Ko9HeoETXQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Il 07/03/23 05:43, Chen-Yu Tsai ha scritto:
> On Mon, Feb 6, 2023 at 6:01 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Add FHCTL parameters and register PLLs through FHCTL to add support
>> for frequency hopping and SSC. FHCTL will be enabled only on PLLs
>> specified in devicetree.
>>
>> This commit brings functional changes only upon addition of
>> devicetree configuration.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/clk/mediatek/clk-mt8195-apmixedsys.c | 69 +++++++++++++++++++-
>>   1 file changed, 66 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8195-apmixedsys.c b/drivers/clk/mediatek/clk-mt8195-apmixedsys.c
>> index 1bc917f2667e..c0db31ce0741 100644
>> --- a/drivers/clk/mediatek/clk-mt8195-apmixedsys.c
>> +++ b/drivers/clk/mediatek/clk-mt8195-apmixedsys.c
>> @@ -3,9 +3,11 @@
>>   // Copyright (c) 2021 MediaTek Inc.
>>   // Author: Chun-Jie Chen <chun-jie.chen@mediatek.com>
>>
>> +#include "clk-fhctl.h"
>>   #include "clk-gate.h"
>>   #include "clk-mtk.h"
>>   #include "clk-pll.h"
>> +#include "clk-pllfh.h"
>>
>>   #include <dt-bindings/clock/mt8195-clk.h>
>>   #include <linux/of_device.h>
>> @@ -105,6 +107,61 @@ static const struct mtk_pll_data plls[] = {
>>              0, 0, 22, 0x0158, 24, 0, 0, 0, 0x0158, 0, 0x0158, 0, 9),
>>   };
>>
>> +enum fh_pll_id {
>> +       FH_ARMPLL_LL,
>> +       FH_ARMPLL_BL,
>> +       FH_MEMPLL,
>> +       FH_ADSPPLL,
>> +       FH_NNAPLL,
>> +       FH_CCIPLL,
>> +       FH_MFGPLL,
>> +       FH_TVDPLL2,
>> +       FH_MPLL,
>> +       FH_MMPLL,
>> +       FH_MAINPLL,
>> +       FH_MSDCPLL,
>> +       FH_IMGPLL,
>> +       FH_VDECPLL,
>> +       FH_TVDPLL1,
>> +       FH_NR_FH,
>> +};
>> +
>> +#define FH(_pllid, _fhid, _offset) {                                   \
>> +               .data = {                                               \
>> +                       .pll_id = _pllid,                               \
>> +                       .fh_id = _fhid,                                 \
>> +                       .fh_ver = FHCTL_PLLFH_V2,                       \
>> +                       .fhx_offset = _offset,                          \
>> +                       .dds_mask = GENMASK(21, 0),                     \
> 
>> +                       .slope0_value = 0x6003c97,                      \
>> +                       .slope1_value = 0x6003c97,                      \
> 
> Are these
> 
>> +                       .sfstrx_en = BIT(2),                            \
>> +                       .frddsx_en = BIT(1),                            \
>> +                       .fhctlx_en = BIT(0),                            \
>> +                       .tgl_org = BIT(31),                             \
>> +                       .dvfs_tri = BIT(31),                            \
>> +                       .pcwchg = BIT(31),                              \
> 
>> +                       .dt_val = 0x0,                                  \
>> +                       .df_val = 0x9,                                  \
> 
> and these just copied from MT8186?

Yes, and that's because they're really the same.

> 
>> +                       .updnlmt_shft = 16,                             \
>> +                       .msk_frddsx_dys = GENMASK(23, 20),              \
>> +                       .msk_frddsx_dts = GENMASK(19, 16),              \
>> +               },                                                      \
>> +       }
>> +
>> +static struct mtk_pllfh_data pllfhs[] = {
>> +       FH(CLK_APMIXED_ADSPPLL, FH_ADSPPLL, 0x78),
>> +       FH(CLK_APMIXED_NNAPLL, FH_NNAPLL, 0x8c),
>> +       FH(CLK_APMIXED_MFGPLL, FH_MFGPLL, 0xb4),
>> +       FH(CLK_APMIXED_TVDPLL2, FH_TVDPLL2, 0xc8),
>> +       FH(CLK_APMIXED_MMPLL, FH_MMPLL, 0xf0),
>> +       FH(CLK_APMIXED_MAINPLL, FH_MAINPLL, 0x104),
>> +       FH(CLK_APMIXED_MSDCPLL, FH_MSDCPLL, 0x118),
>> +       FH(CLK_APMIXED_IMGPLL, FH_IMGPLL, 0x12c),
>> +       FH(CLK_APMIXED_VDECPLL, FH_VDECPLL, 0x140),
>> +       FH(CLK_APMIXED_TVDPLL2, FH_TVDPLL1, 0x154),
>> +};
>> +
>>   static const struct of_device_id of_match_clk_mt8195_apmixed[] = {
>>          { .compatible = "mediatek,mt8195-apmixedsys", },
>>          {}
>> @@ -114,13 +171,17 @@ static int clk_mt8195_apmixed_probe(struct platform_device *pdev)
>>   {
>>          struct clk_hw_onecell_data *clk_data;
>>          struct device_node *node = pdev->dev.of_node;
>> +       const u8 *fhctl_node = "mediatek,mt8195-fhctl";
>>          int r;
>>
>>          clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
>>          if (!clk_data)
>>                  return -ENOMEM;
>>
>> -       r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
>> +       fhctl_parse_dt(fhctl_node, pllfhs, ARRAY_SIZE(pllfhs));
>> +
>> +       r = mtk_clk_register_pllfhs(node, plls, ARRAY_SIZE(plls),
>> +                                   pllfhs, ARRAY_SIZE(pllfhs), clk_data);
>>          if (r)
>>                  goto free_apmixed_data;
>>
>> @@ -140,7 +201,8 @@ static int clk_mt8195_apmixed_probe(struct platform_device *pdev)
>>   unregister_gates:
>>          mtk_clk_unregister_gates(apmixed_clks, ARRAY_SIZE(apmixed_clks), clk_data);
>>   unregister_plls:
>> -       mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
>> +       mtk_clk_unregister_pllfhs(plls, ARRAY_SIZE(plls), pllfhs,
>> +                                 ARRAY_SIZE(pllfhs), clk_data);
> 
> Nit: I think this could be squeezed into one line.
> 
>>   free_apmixed_data:
>>          mtk_free_clk_data(clk_data);
>>          return r;
>> @@ -153,7 +215,8 @@ static int clk_mt8195_apmixed_remove(struct platform_device *pdev)
>>
>>          of_clk_del_provider(node);
>>          mtk_clk_unregister_gates(apmixed_clks, ARRAY_SIZE(apmixed_clks), clk_data);
>> -       mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
>> +       mtk_clk_unregister_pllfhs(plls, ARRAY_SIZE(plls), pllfhs,
>> +                                 ARRAY_SIZE(pllfhs), clk_data);
> 
> Same here.
> 

That's the same on the others as well, but if I compress those lines I will have
to rebase the clocks cleanup series again and send the 54 patches again.. I'd like
to avoid that noise.

If you really want though, I can do that... what should I do?

Regards,
Angelo