| 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: <6441788.lOV4Wx5bFT@workhorse>
Date: Wed, 01 Oct 2025 15:17:13 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: 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>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.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
On Wednesday, 1 October 2025 13:49:54 Central European Summer Time AngeloGioacchino Del Regno wrote:
> 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"` <-----
>
> ???????
They're not children. A child would mean that they derive their
clock rate from the parent clock. This isn't the relationship here
though, their clock signal is completely independent of mfg_eb,
but the registers they access for clock control depend on mfg_eb to
be on. This means that even if mfgpll is off, and something wants to
check if they're on or not, the mfg_eb needs to be on for that
register access to happen. Similarly, when reconfiguring the mfgpll
rate, the clock rate of mfg_eb plays no role. It just needs to be on
for the register access.
mfg_eb here is a clock that drives a register, the register just
happens to be part of a clock controller.
Kind regards,
Nicolas Frattaroli
>
> 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