Re: [PATCH] clock: mediatek: mt8173: Handle unallocated infracfg clock data

[AngeloGioacchino Del Regno]

Il 08/11/23 22:33, Alper Nebi Yasak ha scritto:
> The MT8173 infracfg clock driver does initialization in two steps, via a
> CLK_OF_DECLARE_DRIVER declaration. However its early init function
> doesn't get to run when it's built as a module, presumably since it's
> not loaded by the time it would have been called by of_clk_init(). This
> causes its second-step probe() to return -ENOMEM when trying to register
> clocks, as the necessary clock_data struct isn't initialized by the
> first step.
>
> MT2701 and MT6797 clock drivers also use this mechanism, but they try to
> allocate the necessary clock_data structure if missing in the second
> step. Mimic that for the MT8173 infracfg clock as well to make it work
> as a module.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@xxxxxxxxx>
> ---
> I've tried adding cpumux support to clk-mtk.c then switching this over
> to simple probe functions and it appears to work for me, though I don't
> know clock systems enough to recognize if it's subtly broken instead.
> That'd remove this piece of code, but this might still be worth applying
> to backport to stable kernels.
>
> If I'm reading things correctly, it looks like it would be possible to
> add cpumux & pll & pllfh support to clk-mtk.c, then move most if not
> every driver to simple probe, with one file per clock and module
> support. How much of that is desirable? In what order do the parts need
> to be registered?

Thing is, if (!infra_clk_data) at infracfg_probe time, then INFRA_CLK_13M==-ENOENT!
If you do this, you should at least also send a devicetree commit that adds

	clk13m: fixed-factor-clock-13m {
		compatible = "fixed-factor-clock";
		#clock-cells = <0>;
		clocks = <&clk26m>;
		clock-div = <2>;
		clock-mult = <1>;
		clock-output-names = "clk13m";
	};

....otherwise this solution is incomplete! ;-)

Regarding the CPUMUX support, when I've restructured the MediaTek clocks, I've also
been thinking about doing this, but decided not to do it because that'd be a check
done on ~10 clock drivers per SoC, of which only one is expected to succeed... I
see that as a waste of cycles at boot...

...but if anyone thinks otherwise, I'm fine with it...

Anyway.

Can you please fix the commit title to be consistent with the others and send a v2?

In this case, that would be
"clk: mediatek: mt8173-infracfg: Handle unallocated infracfg when module"

P.S.: Good job!

Cheers,
Angelo

>   drivers/clk/mediatek/clk-mt8173-infracfg.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173-infracfg.c b/drivers/clk/mediatek/clk-mt8173-infracfg.c
> index 2f2f074e231a..ecc8b0063ea5 100644
> --- a/drivers/clk/mediatek/clk-mt8173-infracfg.c
> +++ b/drivers/clk/mediatek/clk-mt8173-infracfg.c
> @@ -98,7 +98,17 @@ CLK_OF_DECLARE_DRIVER(mtk_infrasys, "mediatek,mt8173-infracfg",
>   static int clk_mt8173_infracfg_probe(struct platform_device *pdev)
>   {
>   	struct device_node *node = pdev->dev.of_node;
> -	int r;
> +	int r, i;
> +
> +	if (!infra_clk_data) {
> +		infra_clk_data = mtk_alloc_clk_data(CLK_INFRA_NR_CLK);
> +		if (!infra_clk_data)
> +			return -ENOMEM;
> +	} else {
> +		for (i = 0; i < CLK_INFRA_NR_CLK; i++)
> +			if (infra_clk_data->hws[i] == ERR_PTR(-EPROBE_DEFER))
> +				infra_clk_data->hws[i] = ERR_PTR(-ENOENT);
> +	}
>   
>   	r = mtk_clk_register_gates(&pdev->dev, node, infra_gates,
>   				   ARRAY_SIZE(infra_gates), infra_clk_data);
>
> base-commit: 2220f68f4504aa1ccce0fac721ccdb301e9da32f