Re: [PATCH RFC] timer: of: Create a platform_device before the framework is initialized

[William McVicker <willmcvicker@google.com>]

Hi Daniel,

On 06/25/2025, Daniel Lezcano wrote:
> In the context of the time keeping and the timers, some platforms have
> timers which need to be initialized very early. It is the case of the
> ARM platform which do not have the architected timers.
> 
> The macro TIMER_OF_DECLARE adds an entry in the timer init functions
> array at compile time and the function timer_probe is called from the
> timer_init() function in kernel/time.c
> 
> This array contains a t-uple with the init function and the compatible
> string.
> 
> The init function has a device node pointer parameter.
> 
> The timer_probe() function browses the of nodes and find the ones
> matching the compatible string given when using the TIMER_OF_DECLARE
> macro. It then calls the init function with the device node as a
> pointer.
> 
> But there are some platforms where there are multiple timers like the
> ARM64 with the architected timers. Those are always initialized very
> early and the other timers can be initialized later.
> 
> For this reason we find timer drivers with the platform_driver
> incarnation. Consequently their init functions are different, they
> have a platform_device pointer parameter and rely on the devm_
> function for rollbacking.
> 
> To summarize, we have:
>  - TIMER_OF_DECLARE with init function prototype:
>    int (*init)(struct device_node *np);
> 
>  - module_platform_driver (and variant) with the probe function
>    prototype:
>    int (*init)(struct platform_device *pdev);
> 
> The current situation with the timers is the following:
> 
>  - Two platforms can have the same timer hardware, hence the same
>    driver but one without alternate timers and the other with multiple
>    timers. For example, the Exynos platform has only the Exynos MCT on
>    ARM but has the architeched timers in addition on the ARM64.
> 
>  - The timer drivers can be modules now which was not the case until
>    recently. TIMER_OF_DECLARE do not allow the build as a module.
> 
> It results in duplicate init functions (one with rollback and one with
> devm_) and different way to declare the driver (TIMER_OF_DECLARE and
> module_platform_driver).
> 
> This proposed change is to unify the prototyping of the init functions
> to receive a platform_device pointer as parameter. Consequently, it
> will allow a smoother and nicer module conversion and a huge cleanup
> of the init functions by removing all the rollback code from all the
> timer drivers. It introduces a TIMER_OF_DECLARE_PDEV macro.
> 
> If the macro is used a platform_device is manually allocated and
> initialized with the needed information for the probe
> function. Otherwise module_platform_driver can be use instead with the
> same probe function without the timer_probe() initialization.
> 
> I don't have an expert knowledge of the platform_device internal
> subtilitie so I'm not sure if this approach is valid. However, it has
> been tested on a Rockchip board with the "rockchip,rk3288-timer" and
> verified the macro and the devm_ rollback work correctly.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Hans de Goede <hansg@kernel.org>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/clocksource/timer-probe.c | 61 ++++++++++++++++++++++++++++++-
>  include/asm-generic/vmlinux.lds.h |  2 +
>  include/linux/clocksource.h       |  3 ++
>  include/linux/of.h                |  5 +++
>  4 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index b7860bc0db4b..6b2b341b8c95 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -7,13 +7,18 @@
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/clocksource.h>
> +#include <linux/platform_device.h>
>  
>  extern struct of_device_id __timer_of_table[];
> +extern struct of_device_id __timer_pdev_of_table[];
>  
>  static const struct of_device_id __timer_of_table_sentinel
>  	__used __section("__timer_of_table_end");
>  
> -void __init timer_probe(void)
> +static const struct of_device_id __timer_pdev_of_table_sentinel
> +	__used __section("__timer_pdev_of_table_end");
> +
> +static int __init timer_of_probe(void)
>  {
>  	struct device_node *np;
>  	const struct of_device_id *match;
> @@ -38,6 +43,60 @@ void __init timer_probe(void)
>  		timers++;
>  	}
>  
> +	return timers;
> +}
> +
> +static int __init timer_pdev_of_probe(void)
> +{
> +	struct device_node *np;
> +	struct platform_device *pdev;
> +	const struct of_device_id *match;
> +	of_init_fn_pdev init_func;
> +	unsigned int timers = 0;
> +	int ret;
> +
> +	for_each_matching_node_and_match(np, __timer_pdev_of_table, &match) {
> +		if (!of_device_is_available(np))
> +			continue;
> +
> +		init_func = match->data;
> +
> +		pdev = platform_device_alloc(of_node_full_name(np), -1);
> +		if (!pdev)
> +			continue;
> +
> +		ret = device_add_of_node(&pdev->dev, np);
> +		if (ret) {
> +			platform_device_put(pdev);
> +			continue;
> +		}
> +
> +		dev_set_name(&pdev->dev, pdev->name);
> +
> +		ret = init_func(pdev);
> +		if (!ret) {
> +			timers++;
> +			continue;
> +		}
> +
> +		if (ret != -EPROBE_DEFER)
> +			pr_err("Failed to initialize '%pOF': %d\n", np,
> +			       ret);
> +
> +		device_remove_of_node(&pdev->dev);
> +
> +		platform_device_put(pdev);
> +	}
> +
> +	return timers;
> +}
> +
> +void __init timer_probe(void)
> +{
> +	unsigned timers = 0;
> +
> +	timers += timer_of_probe();
> +	timers += timer_pdev_of_probe();
>  	timers += acpi_probe_device_table(timer);
>  
>  	if (!timers)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index fa5f19b8d53a..97606499c8d7 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -318,6 +318,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
>  	KEEP(*(__##name##_of_table_end))
>  
>  #define TIMER_OF_TABLES()	OF_TABLE(CONFIG_TIMER_OF, timer)
> +#define TIMER_PDEV_OF_TABLES()	OF_TABLE(CONFIG_TIMER_OF, timer_pdev)
>  #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
>  #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
>  #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
> @@ -714,6 +715,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
>  	CLK_OF_TABLES()							\
>  	RESERVEDMEM_OF_TABLES()						\
>  	TIMER_OF_TABLES()						\
> +	TIMER_PDEV_OF_TABLES()						\
>  	CPU_METHOD_OF_TABLES()						\
>  	CPUIDLE_METHOD_OF_TABLES()					\
>  	KERNEL_DTB()							\
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 65b7c41471c3..0eeabd207040 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -289,6 +289,9 @@ extern int clocksource_i8253_init(void);
>  #define TIMER_OF_DECLARE(name, compat, fn) \
>  	OF_DECLARE_1_RET(timer, name, compat, fn)
>  
> +#define TIMER_OF_DECLARE_PDEV(name, compat, fn) \
> +	OF_DECLARE_PDEV(timer_pdev, name, compat, fn)
> +
>  #ifdef CONFIG_TIMER_PROBE
>  extern void timer_probe(void);
>  #else
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a62154aeda1b..a312a6f5ecc1 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1540,9 +1540,12 @@ static inline int of_get_available_child_count(const struct device_node *np)
>  	_OF_DECLARE_STUB(table, name, compat, fn, fn_type)
>  #endif
>  
> +struct platform_device;
> +
>  typedef int (*of_init_fn_2)(struct device_node *, struct device_node *);
>  typedef int (*of_init_fn_1_ret)(struct device_node *);
>  typedef void (*of_init_fn_1)(struct device_node *);
> +typedef int (*of_init_fn_pdev)(struct platform_device *);
>  
>  #define OF_DECLARE_1(table, name, compat, fn) \
>  		_OF_DECLARE(table, name, compat, fn, of_init_fn_1)
> @@ -1550,6 +1553,8 @@ typedef void (*of_init_fn_1)(struct device_node *);
>  		_OF_DECLARE(table, name, compat, fn, of_init_fn_1_ret)
>  #define OF_DECLARE_2(table, name, compat, fn) \
>  		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
> +#define OF_DECLARE_PDEV(table, name, compat, fn) \
> +		_OF_DECLARE(table, name, compat, fn, of_init_fn_pdev)

To support auto-module loading you'll need to also define the
MODULE_DEVICE_TABLE() as part of TIMER_OF_DECLARE_PDEV().

I haven't tested the patch yet, but aside from my comment above it LGTM.

Thanks,
Will

>  
>  /**
>   * struct of_changeset_entry	- Holds a changeset entry
> -- 
> 2.43.0
>