clock: mediatek: mt8173: Handle unallocated infracfg clock data

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

Posted by Alper Nebi Yasak 1 year ago

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@gmail.com>
---
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?

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

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

Posted by AngeloGioacchino Del Regno 1 year ago

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@gmail.com>
> ---
> 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

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

Posted by Alper Nebi Yasak 12 months ago

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@gmail.com>
>> ---
>> 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