[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <748f1176-c73c-48af-a1af-0b63d088e834@collabora.com>
Date: Wed, 1 Oct 2025 13:49:54 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
Guangjie Song <guangjie.song@...iatek.com>,
Laura Nao <laura.nao@...labora.com>,
NĂcolas F. R. A. Prado <nfraprado@...labora.com>,
Yassine Oudjana <y.oudjana@...tonmail.com>
Cc: kernel@...labora.com, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org>, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg
Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
> their control registers are touched in any way at all.
....so, the MFGPLL clocks are children of EB?
Why are you using such a convoluted way of adding a parent clock to the MFGPLL
instead of just
----> `.parent_name = "mfg_eb"` <-----
???????
Cheers,
Angelo
> Failing to ensure
> this results in a pleasant SError interrupt if the EB clock happens to
> be off.
>
> To achieve this, leverage the CCF core's runtime power management
> support. Define the necessary suspend/resume callbacks, add the
> necessary code to get RPM clocks from the DT, and make sure RPM is
> enabled before clock registration happens.
>
> For the RPM callbacks to really make much sense at all, we change the
> drvdata from clk_data to a new private struct, as is common in drivers
> across the Linux kernel.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> ---
> drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
> drivers/clk/mediatek/clk-pll.h | 2 +
> 2 files changed, 87 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
> index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
> --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
> +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
> @@ -13,6 +13,7 @@
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>
> #include "clk-mtk.h"
> #include "clk-pll.h"
> @@ -38,7 +39,7 @@
> _flags, _rst_bar_mask, \
> _pd_reg, _pd_shift, _tuner_reg, \
> _tuner_en_reg, _tuner_en_bit, \
> - _pcw_reg, _pcw_shift, _pcwbits) { \
> + _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) { \
> .id = _id, \
> .name = _name, \
> .reg = _reg, \
> @@ -58,26 +59,60 @@
> .pcw_shift = _pcw_shift, \
> .pcwbits = _pcwbits, \
> .pcwibits = MT8196_INTEGER_BITS, \
> + .rpm_clk_names = _rpm_clks, \
> + .num_rpm_clks = ARRAY_SIZE(_rpm_clks), \
> }
>
> +static const char * const mfgpll_rpm_clk_names[] = {
> + NULL
> +};
> +
> static const struct mtk_pll_data mfg_ao_plls[] = {
> PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
> - BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
> - MFGPLL_CON1, 0, 22),
> + BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
> + mfgpll_rpm_clk_names),
> };
>
> static const struct mtk_pll_data mfgsc0_ao_plls[] = {
> PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
> MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
> - MFGPLL_SC0_CON1, 0, 22),
> + MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
> };
>
> static const struct mtk_pll_data mfgsc1_ao_plls[] = {
> PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
> MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
> - MFGPLL_SC1_CON1, 0, 22),
> + MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
> };
>
> +struct clk_mt8196_mfg {
> + struct clk_hw_onecell_data *clk_data;
> + struct clk_bulk_data *rpm_clks;
> + unsigned int num_rpm_clks;
> +};
> +
> +static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
> +{
> + struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> +
> + if (!clk_mfg || !clk_mfg->rpm_clks)
> + return 0;
> +
> + return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> +}
> +
> +static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
> +{
> + struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> +
> + if (!clk_mfg || !clk_mfg->rpm_clks)
> + return 0;
> +
> + clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> +
> + return 0;
> +}
> +
> static const struct of_device_id of_match_clk_mt8196_mfg[] = {
> { .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
> .data = &mfg_ao_plls },
> @@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
> static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> {
> const struct mtk_pll_data *plls;
> - struct clk_hw_onecell_data *clk_data;
> + struct clk_mt8196_mfg *clk_mfg;
> struct device_node *node = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> const int num_plls = 1;
> - int r;
> + int r, i;
>
> - plls = of_device_get_match_data(&pdev->dev);
> + plls = of_device_get_match_data(dev);
> if (!plls)
> return -EINVAL;
>
> - clk_data = mtk_alloc_clk_data(num_plls);
> - if (!clk_data)
> + clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
> + if (!clk_mfg)
> return -ENOMEM;
>
> - r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
> + clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
> +
> + if (clk_mfg->num_rpm_clks) {
> + clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
> + sizeof(*clk_mfg->rpm_clks),
> + GFP_KERNEL);
> + if (!clk_mfg->rpm_clks)
> + return -ENOMEM;
> +
> + for (i = 0; i < clk_mfg->num_rpm_clks; i++)
> + clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
> +
> + r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
> + clk_mfg->rpm_clks);
> + if (r)
> + return r;
> + }
> +
> + clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
> + if (!clk_mfg->clk_data)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, clk_mfg);
> + pm_runtime_enable(dev);
> +
> + r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
> if (r)
> goto free_clk_data;
>
> - r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> + r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> + clk_mfg->clk_data);
> if (r)
> goto unregister_plls;
>
> - platform_set_drvdata(pdev, clk_data);
> -
> return r;
>
> unregister_plls:
> - mtk_clk_unregister_plls(plls, num_plls, clk_data);
> + mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
> free_clk_data:
> - mtk_free_clk_data(clk_data);
> + mtk_free_clk_data(clk_mfg->clk_data);
>
> return r;
> }
> @@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> static void clk_mt8196_mfg_remove(struct platform_device *pdev)
> {
> const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
> - struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
> + struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
> struct device_node *node = pdev->dev.of_node;
>
> of_clk_del_provider(node);
> - mtk_clk_unregister_plls(plls, 1, clk_data);
> - mtk_free_clk_data(clk_data);
> + mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
> + mtk_free_clk_data(clk_mfg->clk_data);
> }
>
> +static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
> + clk_mt8196_mfg_suspend,
> + clk_mt8196_mfg_resume,
> + NULL);
> +
> static struct platform_driver clk_mt8196_mfg_drv = {
> .probe = clk_mt8196_mfg_probe,
> .remove = clk_mt8196_mfg_remove,
> .driver = {
> .name = "clk-mt8196-mfg",
> .of_match_table = of_match_clk_mt8196_mfg,
> + .pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
> },
> };
> module_platform_driver(clk_mt8196_mfg_drv);
> diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
> --- a/drivers/clk/mediatek/clk-pll.h
> +++ b/drivers/clk/mediatek/clk-pll.h
> @@ -53,6 +53,8 @@ struct mtk_pll_data {
> u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
> u8 pcw_chg_bit;
> u8 fenc_sta_bit;
> + const char * const *rpm_clk_names;
> + unsigned int num_rpm_clks;
> };
>
> /*
>
Powered by blists - more mailing lists