Message-ID: <e557503b-ccd5-46e2-b0b6-e8db30ad0ac4@linaro.org>
Date: Wed, 25 Jun 2025 11:20:47 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, gregkh@...uxfoundation.org
Cc: linux-kernel@...r.kernel.org, lorenzo.pieralisi@...aro.org,
 Hans de Goede <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Rob Herring <robh@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
 Arnd Bergmann <arnd@...db.de>, John Stultz <jstultz@...gle.com>,
 Stephen Boyd <sboyd@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
 "open list:GENERIC INCLUDE/ASM HEADER FILES" <linux-arch@...r.kernel.org>,
 "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE"
 <devicetree@...r.kernel.org>
Subject: Re: [PATCH RFC] timer: of: Create a platform_device before the
 framework is initialized

On 25/06/2025 09:57, 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.

tuple

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

Don't start a sentence with But.

"There are some platforms", "There are platforms" or "Some platforms"

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

architected

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

"It introduces" => "This change introduces"

I think, it would be nice to see an accompanying patch showing how this 
change achieves that IRL.

> 
> 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@...aro.org>
> Cc: Hans de Goede <hansg@...nel.org>
> Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> Cc: Rob Herring <robh@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.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;

Small nit.

Reverse Christmas tree the declarations.

> +
> +	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;

Shouldn't this be return -ENOMEM;

> +
> +		ret = device_add_of_node(&pdev->dev, np);
> +		if (ret) {
> +			platform_device_put(pdev);
> +			continue;

Why is this a continue ? you can get -EINVAL and -EBUSY from 
device_add_of_node() - can/should you really continue this loop after an 
-EINVAL ?

Understood for architected you want to keep going and get the system up 
at the very least I'd add a noisy message about it.

---
bod

Powered by blists - more mailing lists