Re: [PATCH v2 14/29] clk: mediatek: Add MT8196 vlpckgen clock support

[Laura Nao <laura.nao@collabora.com>]

On 7/18/25 10:31, Chen-Yu Tsai wrote:
> On Tue, Jul 15, 2025 at 3:28 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>>
>> Hi,
>>
>>
>> On Tue, Jun 24, 2025 at 10:33 PM Laura Nao <laura.nao@collabora.com> wrote:
>>>
>>> Add support for the MT8196 vlpckgen clock controller, which provides
>>> muxes and dividers for clock selection in other IP blocks.
>>>
>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>>> ---
>>>  drivers/clk/mediatek/Makefile              |   2 +-
>>>  drivers/clk/mediatek/clk-mt8196-vlpckgen.c | 769 +++++++++++++++++++++
>>>  2 files changed, 770 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/clk/mediatek/clk-mt8196-vlpckgen.c
>>>
>>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>>> index 0688d7bf4979..24683dd51783 100644
>>> --- a/drivers/clk/mediatek/Makefile
>>> +++ b/drivers/clk/mediatek/Makefile
>>> @@ -161,7 +161,7 @@ obj-$(CONFIG_COMMON_CLK_MT8195_VENCSYS) += clk-mt8195-venc.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8195_VPPSYS) += clk-mt8195-vpp0.o clk-mt8195-vpp1.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8195_WPESYS) += clk-mt8195-wpe.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o \
>>> -                                  clk-mt8196-topckgen2.o
>>> +                                  clk-mt8196-topckgen2.o clk-mt8196-vlpckgen.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8365_APU) += clk-mt8365-apu.o
>>>  obj-$(CONFIG_COMMON_CLK_MT8365_CAM) += clk-mt8365-cam.o
>>> diff --git a/drivers/clk/mediatek/clk-mt8196-vlpckgen.c b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
>>> new file mode 100644
>>> index 000000000000..23a673dd4c5c
>>> --- /dev/null
>>> +++ b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
>>> @@ -0,0 +1,769 @@
>>
>> [...]
>>
>>> +static const char * const vlp_camtg0_parents[] = {
>>> +       "clk26m",
>>> +       "univpll_192m_d32",
>>> +       "univpll_192m_d16",
>>> +       "clk13m",
>>> +       "osc_d40",
>>> +       "osc_d32",
>>> +       "univpll_192m_d10",
>>> +       "univpll_192m_d8",
>>> +       "univpll_d6_d16",
>>> +       "ulposc3",
>>> +       "osc_d20",
>>> +       "ck2_tvdpll1_d16",
>>> +       "univpll_d6_d8"
>>> +};
>>
>> It seems all the vlp_camtg* parents are the same. Please merge them
>> and just have one list.
>>
>>> +static const char * const vlp_sspm_26m_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d20"
>>> +};
>>> +
>>> +static const char * const vlp_ulposc_sspm_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d2",
>>> +       "mainpll_d4_d2"
>>> +};
>>> +
>>> +static const char * const vlp_vlp_pbus_26m_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d20"
>>> +};
>>> +
>>> +static const char * const vlp_debug_err_flag_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d20"
>>> +};
>>> +
>>> +static const char * const vlp_dpmsrdma_parents[] = {
>>> +       "clk26m",
>>> +       "mainpll_d7_d2"
>>> +};
>>> +
>>> +static const char * const vlp_vlp_pbus_156m_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d2",
>>> +       "mainpll_d7_d2",
>>> +       "mainpll_d7"
>>> +};
>>> +
>>> +static const char * const vlp_spm_parents[] = {
>>> +       "clk26m",
>>> +       "mainpll_d7_d4"
>>> +};
>>> +
>>> +static const char * const vlp_mminfra_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d4",
>>> +       "mainpll_d3"
>>> +};
>>> +
>>> +static const char * const vlp_usb_parents[] = {
>>> +       "clk26m",
>>> +       "mainpll_d9"
>>> +};
>>
>> The previous and the next one are the same.
>>
>>> +static const char * const vlp_usb_xhci_parents[] = {
>>> +       "clk26m",
>>> +       "mainpll_d9"
>>> +};
>>> +
>>> +static const char * const vlp_noc_vlp_parents[] = {
>>> +       "clk26m",
>>> +       "osc_d20",
>>> +       "mainpll_d9"
>>> +};
>>> +
>>> +static const char * const vlp_audio_h_parents[] = {
>>> +       "clk26m",
>>> +       "vlp_apll1",
>>> +       "vlp_apll2"
>>> +};
>>> +
>>> +static const char * const vlp_aud_engen1_parents[] = {
>>> +       "clk26m",
>>> +       "apll1_d8",
>>> +       "apll1_d4"
>>> +};
>>
>> The previous and the next one are the same.
>>
>>> +static const char * const vlp_aud_engen2_parents[] = {
>>> +       "clk26m",
>>> +       "apll2_d8",
>>> +       "apll2_d4"
>>> +};
>>> +
>>> +static const char * const vlp_aud_intbus_parents[] = {
>>> +       "clk26m",
>>> +       "mainpll_d7_d4",
>>> +       "mainpll_d4_d4"
>>> +};
>
> Also, all these audio related clocks (audio_h, aud_engen1, aud_engen2
> aud_intbus) have a "vlp_clk26m" clock as their parent. It should be:
>
>   - clk26m (clk26m from the top ckgen domain)
>   - vlp_clk26m (clk26m from the VLP domain)
>   - (from PLLs)
>   - (from PLLs)
>
> Moreover, an offline discussion with the audio owner suggests that
> of the two 26 MHz clock parents, we really just want the one from
> the VLP domain, as that one is usable even under suspend. This
> could be done by providing an index table.
>

Hi ChenYu,

Thanks for the feedback - I’ll make these changes along with the 
previous suggestions and submit a v3.

Best,

Laura

> ChenYu
>
>>> +
>>> +static const char * const vlp_spvlp_26m
>>
>> [...]
>>
>>> +static int clk_mt8196_vlp_probe(struct platform_device *pdev)
>>> +{
>>> +       struct clk_hw_onecell_data *clk_data;
>>> +       int r;
>>> +       struct device_node *node = pdev->dev.of_node;
>>> +
>>> +       clk_data = mtk_alloc_clk_data(ARRAY_SIZE(vlp_muxes) +
>>> +                                     ARRAY_SIZE(vlp_plls));
>>> +       if (!clk_data)
>>> +               return -ENOMEM;
>>> +
>>> +       r = mtk_clk_register_muxes(&pdev->dev, vlp_muxes, ARRAY_SIZE(vlp_muxes),
>>> +                                  node, &mt8196_clk_vlp_lock, clk_data);
>>> +       if (r)
>>> +               goto free_clk_data;
>>> +
>>> +       r = mtk_clk_register_plls(node, vlp_plls, ARRAY_SIZE(vlp_plls),
>>> +                                 clk_data);
>>> +       if (r)
>>> +               goto unregister_muxes;
>>> +
>>> +       r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
>>> +       if (r)
>>> +               goto unregister_plls;
>>> +
>>> +       platform_set_drvdata(pdev, clk_data);
>>> +
>>> +       return r;
>>> +
>>> +unregister_plls:
>>> +       mtk_clk_unregister_plls(vlp_plls, ARRAY_SIZE(vlp_plls), clk_data);
>>> +unregister_muxes:
>>> +       mtk_clk_unregister_muxes(vlp_muxes, ARRAY_SIZE(vlp_muxes), clk_data);
>>> +free_clk_data:
>>> +       mtk_free_clk_data(clk_data);
>>
>> The AFE driver sets some tuner parameters in the VLPCKGEN block at probe
>> time. Maybe we could do that here instead?
>>
>> /* vlp_cksys_clk: 0x1c016000 */
>> #define VLP_APLL1_TUNER_CON0 0x02a4
>> #define VLP_APLL2_TUNER_CON0 0x02a8
>>
>> /* vlp apll1 tuner default value*/
>> #define VLP_APLL1_TUNER_CON0_VALUE 0x6f28bd4d
>> /* vlp apll2 tuner default value + 1*/
>> #define VLP_APLL2_TUNER_CON0_VALUE 0x78fd5265
>>
>>        regmap_write(afe_priv->vlp_ck, VLP_APLL1_TUNER_CON0,
>> VLP_APLL1_TUNER_CON0_VALUE);
>>        regmap_write(afe_priv->vlp_ck, VLP_APLL2_TUNER_CON0,
>> VLP_APLL2_TUNER_CON0_VALUE);
>>
>> ChenYu
>>
>>> +
>>> +       return r;
>>> +}
>>> +
>>> +static void clk_mt8196_vlp_remove(struct platform_device *pdev)
>>> +{
>>> +       struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
>>> +       struct device_node *node = pdev->dev.of_node;
>>> +
>>> +       of_clk_del_provider(node);
>>> +       mtk_clk_unregister_plls(vlp_plls, ARRAY_SIZE(vlp_plls), clk_data);
>>> +       mtk_clk_unregister_muxes(vlp_muxes, ARRAY_SIZE(vlp_muxes), clk_data);
>>> +       mtk_free_clk_data(clk_data);
>>> +}
>>> +
>>> +static const struct of_device_id of_match_clk_mt8196_vlp_ck[] = {
>>> +       { .compatible = "mediatek,mt8196-vlpckgen" },
>>> +       { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_vlp_ck);
>>> +
>>> +static struct platform_driver clk_mt8196_vlp_drv = {
>>> +       .probe = clk_mt8196_vlp_probe,
>>> +       .remove = clk_mt8196_vlp_remove,
>>> +       .driver = {
>>> +               .name = "clk-mt8196-vlpck",
>>> +               .of_match_table = of_match_clk_mt8196_vlp_ck,
>>> +       },
>>> +};
>>> +
>>> +MODULE_DESCRIPTION("MediaTek MT8196 VLP clock generator driver");
>>> +module_platform_driver(clk_mt8196_vlp_drv);
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.39.5
>>>