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

["Arnd Bergmann" <arnd@arndb.de>]

On Tue, Jul 8, 2025, at 11:50, Daniel Lezcano wrote:
> On Tue, Jul 01, 2025 at 09:52:45AM +0200, Arnd Bergmann wrote:
>> On Tue, Jul 1, 2025, at 01:53, William McVicker wrote:
>> >> @@ -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.
>> 
>> The patch doesn't actually have a module_platform_driver_probe()
>> yet either, so loading the module wouldn't actually do anything.
>> 
>> I feel that this RFC by itself a good step in the direction we want, 
>> so Daniel should go ahead with prototyping the next two steps:
>> adding the platform_driver registration into OF_DECLARE_PDEV,
>> and converting a driver so it can be used either with the _OF_DECLARE()
>> or the platform_driver case.
>
> I'm questioning the relevance of adding the macro when the driver is
> not compiled as a module.
>
> The first step of this macro is to allow the existing init functions
> to be converted to the same signature as the module probe functions in
> order to share the same code and take benefit of the devm_ variants
> function which will considerably reduce the code size of the drivers.
>
> Then we have the following situations:
>
>  1. The driver has to be loaded very early TIMER_OF_DECLARE_PDEV
>  (MODULE=no) the function timer-probe() is used
>
>  2. The driver is a module_platform_driver() and MODULE=no, then it is
>  built as a builtin_platform_driver(), we do not care about having it
>  loaded by timer-probe()
>
>  3. The driver is a module_platform_driver() and MODULE=yes
>
> If we do the change to have the TIMER_OF_DECLARE_PDEV() adding the
> platform_driver registration when MODULE=yes but using timer-probe
> when MODULE=no, we change the initialization and we will have issues
> with timers needing resources like SCMI clocks and where the
> mechanisms rely on EPROBE_DEFER.
>
> IMO, module_platform_driver and timer_probe must be separated.

I assumed that the entire point of your work was to simplify
the code when you have both a early and platform_driver based
probing in a single driver.

> Let's assume the one possible future use case with the VF PIT. This
> timer is only used on ARM but now it is also supported for the ARM64
> s32g2. For the first platform we need it very early and in the second
> case, we need it later because the architected timers are there.
>
> We should endup with:
>
> static const struct of_device_id pit_timer_of_match[] = {
> 	{ .compatible = "nxp,s32g2-pit", .data = &s32g2_data },
> 	{ .compatible = "nxp,s32g3-pit", .data = &s32g3_data },
> 	{ }
> };
> MODULE_DEVICE_TABLE(of, pit_timer_of_match);
>
> static struct platform_driver nxp_pit_driver = {
> 	.driver = {
> 		.name = "nxp-pit",
> 		.of_match_table = pit_timer_of_match,
> 	},
> 	.probe = pit_timer_probe,
> };
> module_platform_driver(nxp_pit_driver);
>
> TIMER_OF_DECLARE_PDEV(vf610, "fsl,vf610-pit", pit_timer_probe);

If you think we still need a module_platform_driver(), I would
not bother adding TIMER_OF_DECLARE_PDEV() either, as the
probe functions can easily be shared as well, the way that
drivers/clocksource/renesas-ostm.c does.

The only thing that would lose is the ability to call devm_
functions because the device pointer is unavailable here.

> If we change the TIMER_OF_DECLARE_PDEV to a macro which relies on
> timer_probe when MODULE=no, then the "nxp-pit" on the s32gX will fail
> to initialize because of the SCMI clocks not ready and the routine
> won't reprobe the function. This issue won't happen with
> builtin_platform_driver

The way I had expected this to work was that TIMER_OF_DECLARE_PDEV()
always registers the platform_driver and just skips the
section magic when it's in a loadable module.

    Arnd