Re: [PATCH 1/3] clk: socfpga: agilex: convert to CLK_OF_DECLARE()

["Ng, Adrian Ho Yin" <adrian.ho.yin.ng@altera.com>]

On 9/19/2026 6:42 AM, Brian Masney wrote:
> Hi Adrian,
> 
> On Fri, Sep 11, 2026 at 03:01:18PM +0800, adrian.ho.yin.ng@altera.com wrote:
>> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
>>
>> Register Agilex and eASIC N5X clocks at of_clk_init() so they are
>> available before platform devices probe.
>>
>> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
>> ---
>>   drivers/clk/socfpga/clk-agilex.c | 83 +++++++++++---------------------
>>   1 file changed, 28 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
>> index 2bdea1997b5e..64a20c727d16 100644
>> --- a/drivers/clk/socfpga/clk-agilex.c
>> +++ b/drivers/clk/socfpga/clk-agilex.c
>> @@ -4,8 +4,9 @@
>>    */
>>   #include <linux/slab.h>
>>   #include <linux/clk-provider.h>
>> +#include <linux/io.h>
>>   #include <linux/of.h>
>> -#include <linux/platform_device.h>
>> +#include <linux/of_address.h>
>>   
>>   #include <dt-bindings/clock/agilex-clock.h>
>>   
>> @@ -454,24 +455,26 @@ static int n5x_clk_register_pll(const struct stratix10_pll_clock *clks,
>>   	return 0;
>>   }
>>   
>> -static int agilex_clkmgr_init(struct platform_device *pdev)
>> +static void __init agilex_clkmgr_init(struct device_node *np)
>>   {
>> -	struct device_node *np = pdev->dev.of_node;
>> -	struct device *dev = &pdev->dev;
>>   	struct stratix10_clock_data *clk_data;
>>   	void __iomem *base;
>>   	int i, num_clks;
>>   
>> -	base = devm_platform_ioremap_resource(pdev, 0);
>> -	if (IS_ERR(base))
>> -		return PTR_ERR(base);
>> +	base = of_iomap(np, 0);
>> +	if (!base) {
>> +		pr_err("%s: failed to map clock registers\n", __func__);
>> +		return;
>> +	}
>>   
>>   	num_clks = AGILEX_NUM_CLKS;
>>   
>> -	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
>> -				num_clks), GFP_KERNEL);
>> -	if (!clk_data)
>> -		return -ENOMEM;
>> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, num_clks),
>> +			   GFP_KERNEL);
>> +	if (!clk_data) {
>> +		iounmap(base);
>> +		return;
>> +	}
>>   
>>   	clk_data->clk_data.num = num_clks;
>>   	clk_data->base = base;
>> @@ -491,27 +494,28 @@ static int agilex_clkmgr_init(struct platform_device *pdev)
>>   	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
>>   			      clk_data);
>>   	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
>> -	return 0;
>>   }
>>   
>> -static int n5x_clkmgr_init(struct platform_device *pdev)
>> +static void __init n5x_clkmgr_init(struct device_node *np)
>>   {
>> -	struct device_node *np = pdev->dev.of_node;
>> -	struct device *dev = &pdev->dev;
>>   	struct stratix10_clock_data *clk_data;
>>   	void __iomem *base;
>>   	int i, num_clks;
>>   
>> -	base = devm_platform_ioremap_resource(pdev, 0);
>> -	if (IS_ERR(base))
>> -		return PTR_ERR(base);
>> +	base = of_iomap(np, 0);
>> +	if (!base) {
>> +		pr_err("%s: failed to map clock registers\n", __func__);
>> +		return;
>> +	}
>>   
>>   	num_clks = AGILEX_NUM_CLKS;
>>   
>> -	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
>> -				num_clks), GFP_KERNEL);
>> -	if (!clk_data)
>> -		return -ENOMEM;
>> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, num_clks),
>> +			   GFP_KERNEL);
>> +	if (!clk_data) {
>> +		iounmap(base);
>> +		return;
>> +	}
>>   
>>   	clk_data->base = base;
>>   	clk_data->clk_data.num = num_clks;
>> @@ -531,38 +535,7 @@ static int n5x_clkmgr_init(struct platform_device *pdev)
>>   	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
>>   			      clk_data);
>>   	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
>> -	return 0;
>> -}
>> -
>> -static int agilex_clkmgr_probe(struct platform_device *pdev)
>> -{
>> -	int (*probe_func)(struct platform_device *init_func);
>> -
>> -	probe_func = of_device_get_match_data(&pdev->dev);
>> -	if (!probe_func)
>> -		return -ENODEV;
>> -	return	probe_func(pdev);
>>   }
>>   
>> -static const struct of_device_id agilex_clkmgr_match_table[] = {
>> -	{ .compatible = "intel,agilex-clkmgr",
>> -	  .data = agilex_clkmgr_init },
>> -	{ .compatible = "intel,easic-n5x-clkmgr",
>> -	  .data = n5x_clkmgr_init },
>> -	{ }
>> -};
>> -
>> -static struct platform_driver agilex_clkmgr_driver = {
>> -	.probe		= agilex_clkmgr_probe,
>> -	.driver		= {
>> -		.name	= "agilex-clkmgr",
>> -		.suppress_bind_attrs = true,
>> -		.of_match_table = agilex_clkmgr_match_table,
>> -	},
>> -};
>> -
>> -static int __init agilex_clk_init(void)
>> -{
>> -	return platform_driver_register(&agilex_clkmgr_driver);
>> -}
>> -core_initcall(agilex_clk_init);
>> +CLK_OF_DECLARE(agilex_clkmgr, "intel,agilex-clkmgr", agilex_clkmgr_init);
>> +CLK_OF_DECLARE(n5x_clkmgr, "intel,easic-n5x-clkmgr", n5x_clkmgr_init);
> 
> Why do all of these clk providers need to be registered so early?
> CLK_OF_DECLARE is abused quite a bit today and the majority of the clk
> drivers that use it don't actually need it.
> 
Hi Brian,

Thanks for the review.

subsys_initcall() / subsys_platform_driver() would not fix the failure 
we are hitting. Both still run after time_init(), while the DW APB timer 
is registered via TIMER_OF_DECLARE from timer_probe().

So moving from core_initcall() to subsys_initcall() changes nothing for
this consumer: the timer cannot defer, clk_get() still fails, and the 
timer is never brought up. That is why these patches use 
CLK_OF_DECLARE(): the provider must be registered from of_clk_init() so 
clocks exist before TIMER_OF_DECLARE runs.

On the broader point about CLK_OF_DECLARE abuse: I agree it should not 
be used merely to beat platform device probe. If the preferred approach 
is to keep the clkmgr as a platform driver (subsys_platform_driver()) 
and give the DW APB timer nodes a fixed clock-frequency instead of a 
clocks phandle, I can respin that way. Please let me know which you prefer.

Thank You
Adrian
> core_initcall() is pretty early as well. Can you use subsys_initcall
> instead so that it's available by time the platform devices probe?
> 
> I recently introduced a subsys_platform_driver() macro to simplify the
> code that should do what you need.
> 
> https://lore.kernel.org/linux-clk/20260908-subsys_initcall-v1-0-cbccf4cd4288@redhat.com/
> 
> Brian