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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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 {
>