| 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: <20250722105203.33151-1-laura.nao@collabora.com>
Date: Tue, 22 Jul 2025 12:52:03 +0200
From: Laura Nao <laura.nao@...labora.com>
To: wenst@...omium.org
Cc: angelogioacchino.delregno@...labora.com,
conor+dt@...nel.org,
devicetree@...r.kernel.org,
guangjie.song@...iatek.com,
kernel@...labora.com,
krzk+dt@...nel.org,
laura.nao@...labora.com,
linux-arm-kernel@...ts.infradead.org,
linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-mediatek@...ts.infradead.org,
matthias.bgg@...il.com,
mturquette@...libre.com,
netdev@...r.kernel.org,
p.zabel@...gutronix.de,
richardcochran@...il.com,
robh@...nel.org,
sboyd@...nel.org
Subject: Re: [PATCH v2 14/29] clk: mediatek: Add MT8196 vlpckgen clock support
On 7/18/25 10:31, Chen-Yu Tsai wrote:
> On Tue, Jul 15, 2025 at 3:28 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
>>
>> Hi,
>>
>>
>> On Tue, Jun 24, 2025 at 10:33 PM Laura Nao <laura.nao@...labora.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@...labora.com>
>>> Signed-off-by: Laura Nao <laura.nao@...labora.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
>>>
Powered by blists - more mailing lists