Re: [PATCH v2 5/5] clk: mediatek: Add mfg_eb as parent to mt8196 mfgpll clocks

AngeloGioacchino Del Regno
2025-10-09T08:22:34Z

Il 08/10/25 18:05, Nicolas Frattaroli ha scritto:
> All the MFGPLL require MFG_EB to be on for any operation on them, and
> they only tick when MFG_EB is on as well, therefore making this a
> parent-child relationship.
>
> This dependency wasn't clear during the initial upstreaming of these
> clock controllers, as it only made itself known when I could observe
> the effects of the clock by bringing up a different piece of hardware.
>
> Add a new PLL_PARENT_EN flag to mediatek's clk-pll.h, and check for it
> when initialising the pll to then translate it into the actual
> CLK_OPS_PARENT_ENABLE flag.
>
> Then add the mfg_eb parent to the mfgpll clocks, and set the new
> PLL_PARENT_EN flag.
>
> Fixes: 03dc02f8c7dc ("clk: mediatek: Add MT8196 mfg clock support")
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> drivers/clk/mediatek/clk-mt8196-mfg.c | 3 ++-
> drivers/clk/mediatek/clk-pll.c | 3 +++
> drivers/clk/mediatek/clk-pll.h | 1 +
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
> index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..de6f426f148184e1bb95b5cfca590b1763fc0573 100644
> --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
> +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
> @@ -45,7 +45,7 @@
> .en_reg = _en_reg, \
> .en_mask = _en_mask, \
> .pll_en_bit = _pll_en_bit, \
> - .flags = _flags, \
> + .flags = (_flags) | PLL_PARENT_EN, \

In the event that we may want, one day, maybe commonize the PLL macro to all of the
PLL drivers, can you please add the PLL_PARENT_EN to the three clocks instead of
doing it in the macro itself?

static const struct mtk_pll_data mfg_ao_plls[] = {
PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0,
PLL_PARENT_EN, BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
MFGPLL_CON1, 0, 22),
};

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, PLL_PARENT_EN, BIT(0),
MFGPLL_SC0_CON1, 24, 0, 0, 0, MFGPLL_SC0_CON1, 0, 22),
};

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, PLL_PARENT_EN, BIT(0),
MFGPLL_SC1_CON1, 24, 0, 0, 0, MFGPLL_SC1_CON1, 0, 22),
};

After which,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> .rst_bar_mask = _rst_bar_mask, \
> .fmax = MT8196_PLL_FMAX, \
> .fmin = MT8196_PLL_FMIN, \
> @@ -58,6 +58,7 @@
> .pcw_shift = _pcw_shift, \
> .pcwbits = _pcwbits, \
> .pcwibits = MT8196_INTEGER_BITS, \
> + .parent_name = "mfg_eb", \
> }
>
> static const struct mtk_pll_data mfg_ao_plls[] = {
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index c4f9c06e5133dbc5902f261353c197fbde95e54d..0f3759fcd9d0228c23f4916d041d17b731a6c838 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -359,6 +359,9 @@ struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
>
> init.name = data->name;
> init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0;
> + if (data->flags & PLL_PARENT_EN)
> + init.flags |= CLK_OPS_PARENT_ENABLE;
> +
> init.ops = pll_ops;
> if (data->parent_name)
> init.parent_names = &data->parent_name;
> diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> index 0f2a1d19eea78b7390b221af47016eb9897f3596..492cad8ff80ba31a78a96085285cb938e9b978e9 100644
> --- a/drivers/clk/mediatek/clk-pll.h
> +++ b/drivers/clk/mediatek/clk-pll.h
> @@ -21,6 +21,7 @@ struct mtk_pll_div_table {
>
> #define HAVE_RST_BAR BIT(0)
> #define PLL_AO BIT(1)
> +#define PLL_PARENT_EN BIT(2)
> #define POSTDIV_MASK GENMASK(2, 0)
>
> struct mtk_pll_data {
>