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

[Alper Nebi Yasak]

Hi,

Sorry with the late reply. I am trying to get things enabled in Debian
for MT8173 and MT8183 Chromebooks, and needed to post this so I can
refer to this there [1]. Then I worked on things related to that, hence
the delay. Replying here mostly because I wanted to tell you as you
might be interested -- although I do have some module probe ordering
problems that could use some insight...

[1] https://salsa.debian.org/kernel-team/linux/-/merge_requests/906

On 2023-11-09 12:05 +03:00, AngeloGioacchino Del Regno wrote:
> 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! ;-)

I'm seeing similar blocks added for other SoCs and tried to match what
they did, hoping it's correct for MT8173 as well.

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

Thanks for making it possible to use them as modules, I appreciate it!
It does look like a maintainability vs performance trade off, and I
don't know enough about performance profiling to say it doesn't matter here.

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

Heh, I'm having trouble with titles recently. Sent a v2 just now, with a
second patch for device-tree.

> 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