[PATCH] clk: imx8mp: register driver at arch_initcall time

[PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

We have an imx8mp-based board with an external gpio-triggered
watchdog. Currently, we don't get to handle that in time before it
resets the board.

The probe of the watchdog device gets deferred because the SOC's GPIO
controller is not yet ready, and the probe of that in turn gets deferred
because its clock provider (namely, this driver) is not yet
ready. Altogether, the watchdog does not get handled until the late
initcall deferred_probe_initcall has made sure all leftover devices
have been probed, and that's way too late.

Aside from being necessary for our board, this also reduces total boot
time because fewer device probes get deferred.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
It would probably be reasonable to do the same to the other imx8m* clk
drivers, but I don't have any such hardware to test on.

 drivers/clk/imx/clk-imx8mp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index e89db568f5a8..9ddd39a664cc 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -734,7 +734,19 @@ static struct platform_driver imx8mp_clk_driver = {
 		.of_match_table = imx8mp_clk_of_match,
 	},
 };
-module_platform_driver(imx8mp_clk_driver);
+
+static int __init imx8mp_clk_init(void)
+{
+	return platform_driver_register(&imx8mp_clk_driver);
+}
+arch_initcall(imx8mp_clk_init);
+
+static void __exit imx8mp_clk_exit(void)
+{
+	platform_driver_unregister(&imx8mp_clk_driver);
+}
+module_exit(imx8mp_clk_exit);
+
 module_param(mcore_booted, bool, S_IRUGO);
 MODULE_PARM_DESC(mcore_booted, "See Cortex-M core is booted or not");
 
-- 
2.37.2

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 28/09/2022 14.41, Rasmus Villemoes wrote:
> We have an imx8mp-based board with an external gpio-triggered
> watchdog. Currently, we don't get to handle that in time before it
> resets the board.
> 
> The probe of the watchdog device gets deferred because the SOC's GPIO
> controller is not yet ready, and the probe of that in turn gets deferred
> because its clock provider (namely, this driver) is not yet
> ready. Altogether, the watchdog does not get handled until the late
> initcall deferred_probe_initcall has made sure all leftover devices
> have been probed, and that's way too late.
> 
> Aside from being necessary for our board, this also reduces total boot
> time because fewer device probes get deferred.

ping

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Ahmad Fatoum]

Hello Rasmus,

On 24.10.22 15:48, Rasmus Villemoes wrote:
> On 28/09/2022 14.41, Rasmus Villemoes wrote:
>> We have an imx8mp-based board with an external gpio-triggered
>> watchdog. Currently, we don't get to handle that in time before it
>> resets the board.
>>
>> The probe of the watchdog device gets deferred because the SOC's GPIO
>> controller is not yet ready, and the probe of that in turn gets deferred
>> because its clock provider (namely, this driver) is not yet
>> ready. Altogether, the watchdog does not get handled until the late
>> initcall deferred_probe_initcall has made sure all leftover devices
>> have been probed, and that's way too late.
>>
>> Aside from being necessary for our board, this also reduces total boot
>> time because fewer device probes get deferred.
> 
> ping

Module build is unaffected, because arch_initcall expands to module_init
if it's built as a module, right? Noting that in the commit message would
be good I think.

Also, did you try booting with fw_devlink=on? This should have resolved
your issue too. Not sure what other issues it may cause on i.MX8MP.

Cheers,
Ahmad

> 
> Rasmus
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 24/10/2022 16.00, Ahmad Fatoum wrote:
> Hello Rasmus,
> 
> On 24.10.22 15:48, Rasmus Villemoes wrote:
>> On 28/09/2022 14.41, Rasmus Villemoes wrote:
>>> We have an imx8mp-based board with an external gpio-triggered
>>> watchdog. Currently, we don't get to handle that in time before it
>>> resets the board.
>>>
>>> The probe of the watchdog device gets deferred because the SOC's GPIO
>>> controller is not yet ready, and the probe of that in turn gets deferred
>>> because its clock provider (namely, this driver) is not yet
>>> ready. Altogether, the watchdog does not get handled until the late
>>> initcall deferred_probe_initcall has made sure all leftover devices
>>> have been probed, and that's way too late.
>>>
>>> Aside from being necessary for our board, this also reduces total boot
>>> time because fewer device probes get deferred.
>>
>> ping
> 
> Module build is unaffected, because arch_initcall expands to module_init
> if it's built as a module, right?

Yes.

 Noting that in the commit message would
> be good I think.
> 
> Also, did you try booting with fw_devlink=on? This should have resolved
> your issue too. Not sure what other issues it may cause on i.MX8MP.

AFAICT, that's the default behaviour:

drivers/base/core.c:static u32 fw_devlink_flags = FW_DEVLINK_FLAGS_ON;
drivers/base/core.c-static int __init fw_devlink_setup(char *arg)
drivers/base/core.c-{
drivers/base/core.c-    if (!arg)
drivers/base/core.c-            return -EINVAL;
drivers/base/core.c-
drivers/base/core.c-    if (strcmp(arg, "off") == 0) {
drivers/base/core.c:            fw_devlink_flags = 0;
drivers/base/core.c-    } else if (strcmp(arg, "permissive") == 0) {
drivers/base/core.c:            fw_devlink_flags =
FW_DEVLINK_FLAGS_PERMISSIVE;
drivers/base/core.c-    } else if (strcmp(arg, "on") == 0) {
drivers/base/core.c:            fw_devlink_flags = FW_DEVLINK_FLAGS_ON;
drivers/base/core.c-    } else if (strcmp(arg, "rpm") == 0) {
drivers/base/core.c:            fw_devlink_flags = FW_DEVLINK_FLAGS_RPM;
drivers/base/core.c-    }
drivers/base/core.c-    return 0;
drivers/base/core.c-}
drivers/base/core.c-early_param("fw_devlink", fw_devlink_setup);

But I don't think the problem is that the driver core has inferred the
dependency chain upfront from parsing DT; the problem is simply that we
end up probing the clock controller, and hence all dependencies too late.

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Stephen Boyd]

Quoting Rasmus Villemoes (2022-09-28 05:41:08)
> We have an imx8mp-based board with an external gpio-triggered
> watchdog. Currently, we don't get to handle that in time before it
> resets the board.

How much time does your bootloader give you to pet the watchdog? Why is
the timeout short enough to trigger? Or is deferring probe slowing down
boot so significantly that boot times are bad?

> 
> The probe of the watchdog device gets deferred because the SOC's GPIO
> controller is not yet ready, and the probe of that in turn gets deferred
> because its clock provider (namely, this driver) is not yet
> ready. Altogether, the watchdog does not get handled until the late
> initcall deferred_probe_initcall has made sure all leftover devices
> have been probed, and that's way too late.
> 
> Aside from being necessary for our board, this also reduces total boot
> time because fewer device probes get deferred.

This is a game of whack-a-mole. If we decide to move device population
from DT (of_platform_default_populate_init) to device_initcall() level
we may run into a similar problem.

> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> It would probably be reasonable to do the same to the other imx8m* clk
> drivers, but I don't have any such hardware to test on.
> 
>  drivers/clk/imx/clk-imx8mp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index e89db568f5a8..9ddd39a664cc 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -734,7 +734,19 @@ static struct platform_driver imx8mp_clk_driver = {
>                 .of_match_table = imx8mp_clk_of_match,
>         },
>  };
> -module_platform_driver(imx8mp_clk_driver);
> +
> +static int __init imx8mp_clk_init(void)
> +{
> +       return platform_driver_register(&imx8mp_clk_driver);
> +}
> +arch_initcall(imx8mp_clk_init);

Furthermore, there isn't any comment about why this is arch_initcall
level. The next reader of this code can only assume why this was done or
go on a git archaeology dig to figure out that we're registering this
device early for some imx8mp-based board (is it upstream? What board is
it?). Please help people reading the code.

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 28/10/2022 01.29, Stephen Boyd wrote:
> Quoting Rasmus Villemoes (2022-09-28 05:41:08)
>> We have an imx8mp-based board with an external gpio-triggered
>> watchdog. Currently, we don't get to handle that in time before it
>> resets the board.
> 
> How much time does your bootloader give you to pet the watchdog? 

The bootloader has no say; it's a simple piece of hardware with a
hardcoded threshold. In this particular case 1 second. Most, if not all,
custom designed industrial boards I've worked with has always been
equipped with such an external watchdog (the threshold may be different,
but the basic functionality and requirement is the same). In some cases,
it's a matter of certifications, in others it's a requirement from
certain end customers. But the hardware designers certainly never add
this just for fun (obviously they want to keep complexity and BOM cost
down).

Why is
> the timeout short enough to trigger? Or is deferring probe slowing down
> boot so significantly that boot times are bad?

I wouldn't say that deferring probe slows down the boot as such (it
does, but not by a lot), but the fact that the watchdog device gets
deferred (because it depends on the gpio and in turn the clk IP blocks
in the SOC) is a problem.
>>
>> The probe of the watchdog device gets deferred because the SOC's GPIO
>> controller is not yet ready, and the probe of that in turn gets deferred
>> because its clock provider (namely, this driver) is not yet
>> ready. Altogether, the watchdog does not get handled until the late
>> initcall deferred_probe_initcall has made sure all leftover devices
>> have been probed, and that's way too late.
>>
>> Aside from being necessary for our board, this also reduces total boot
>> time because fewer device probes get deferred.
> 
> This is a game of whack-a-mole. If we decide to move device population
> from DT (of_platform_default_populate_init) to device_initcall() level
> we may run into a similar problem.

That's a red herring, because such a patch would be a regression for a
lot of existing and working boards with an external gpio-wdt watchdog.

>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> It would probably be reasonable to do the same to the other imx8m* clk
>> drivers, but I don't have any such hardware to test on.
>>
>>  drivers/clk/imx/clk-imx8mp.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>> index e89db568f5a8..9ddd39a664cc 100644
>> --- a/drivers/clk/imx/clk-imx8mp.c
>> +++ b/drivers/clk/imx/clk-imx8mp.c
>> @@ -734,7 +734,19 @@ static struct platform_driver imx8mp_clk_driver = {
>>                 .of_match_table = imx8mp_clk_of_match,
>>         },
>>  };
>> -module_platform_driver(imx8mp_clk_driver);
>> +
>> +static int __init imx8mp_clk_init(void)
>> +{
>> +       return platform_driver_register(&imx8mp_clk_driver);
>> +}
>> +arch_initcall(imx8mp_clk_init);
> 
> Furthermore, there isn't any comment about why this is arch_initcall
> level. The next reader of this code can only assume why this was done or
> go on a git archaeology dig to figure out that we're registering this
> device early for some imx8mp-based board (is it upstream? What board is
> it?). Please help people reading the code.

Sure, I could add a comment here.

But if we take a step back, doesn't it make sense in general to make
sure a central IP block like the SOC's primary clk source is probed
early, before various other dependent IP blocks and peripherals?
Initializing such a core part of the SOC certainly sounds to me like an
arch-level thing. And it's not like this would be the first SOC-specific
clk driver with init called at arch_initcall, without any comment why it
happens at that time. E.g. clk_mt7629_init. IMO, that doesn't require a
comment, it's really just common sense.

As I said, I think this change would make sense for at least all the
imx8m drivers, and I'm happy to extend the patch to cover those for
consistency.

It also gives a small but measurable improvement in total boot time, but
that by itself is not why I want to make this change - it's simply a
necessary patch to make my customer's new imx8mp-based boards boot.

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 28/09/2022 14.41, Rasmus Villemoes wrote:
> We have an imx8mp-based board with an external gpio-triggered
> watchdog. Currently, we don't get to handle that in time before it
> resets the board.
> 
> The probe of the watchdog device gets deferred because the SOC's GPIO
> controller is not yet ready, and the probe of that in turn gets deferred
> because its clock provider (namely, this driver) is not yet
> ready. Altogether, the watchdog does not get handled until the late
> initcall deferred_probe_initcall has made sure all leftover devices
> have been probed, and that's way too late.
> 
> Aside from being necessary for our board, this also reduces total boot
> time because fewer device probes get deferred.

Please advise on what I need to do in order to make progress here.

Thanks,
Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 07/11/2022 14.54, Rasmus Villemoes wrote:
> On 28/09/2022 14.41, Rasmus Villemoes wrote:
>> We have an imx8mp-based board with an external gpio-triggered
>> watchdog. Currently, we don't get to handle that in time before it
>> resets the board.
>>
>> The probe of the watchdog device gets deferred because the SOC's GPIO
>> controller is not yet ready, and the probe of that in turn gets deferred
>> because its clock provider (namely, this driver) is not yet
>> ready. Altogether, the watchdog does not get handled until the late
>> initcall deferred_probe_initcall has made sure all leftover devices
>> have been probed, and that's way too late.
>>
>> Aside from being necessary for our board, this also reduces total boot
>> time because fewer device probes get deferred.
> 
> Please advise on what I need to do in order to make progress here.

Ping.

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Fabio Estevam]

On Wed, Sep 28, 2022 at 9:41 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> We have an imx8mp-based board with an external gpio-triggered
> watchdog. Currently, we don't get to handle that in time before it
> resets the board.
>
> The probe of the watchdog device gets deferred because the SOC's GPIO
> controller is not yet ready, and the probe of that in turn gets deferred
> because its clock provider (namely, this driver) is not yet
> ready. Altogether, the watchdog does not get handled until the late
> initcall deferred_probe_initcall has made sure all leftover devices
> have been probed, and that's way too late.
>
> Aside from being necessary for our board, this also reduces total boot
> time because fewer device probes get deferred.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> It would probably be reasonable to do the same to the other imx8m* clk
> drivers, but I don't have any such hardware to test on.

Agreed.

> +static void __exit imx8mp_clk_exit(void)
> +{
> +       platform_driver_unregister(&imx8mp_clk_driver);
> +}
> +module_exit(imx8mp_clk_exit);

Isn't module_exit() unnecessary here, since we pass suppress_bind_attrs = true?

With module_exit() removed:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 19/11/2022 22.38, Fabio Estevam wrote:
> On Wed, Sep 28, 2022 at 9:41 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> We have an imx8mp-based board with an external gpio-triggered
>> watchdog. Currently, we don't get to handle that in time before it
>> resets the board.
>>
>> The probe of the watchdog device gets deferred because the SOC's GPIO
>> controller is not yet ready, and the probe of that in turn gets deferred
>> because its clock provider (namely, this driver) is not yet
>> ready. Altogether, the watchdog does not get handled until the late
>> initcall deferred_probe_initcall has made sure all leftover devices
>> have been probed, and that's way too late.
>>
>> Aside from being necessary for our board, this also reduces total boot
>> time because fewer device probes get deferred.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> It would probably be reasonable to do the same to the other imx8m* clk
>> drivers, but I don't have any such hardware to test on.
> 
> Agreed.
> 
>> +static void __exit imx8mp_clk_exit(void)
>> +{
>> +       platform_driver_unregister(&imx8mp_clk_driver);
>> +}
>> +module_exit(imx8mp_clk_exit);
> 
> Isn't module_exit() unnecessary here, since we pass suppress_bind_attrs = true?

Sorry, I don't follow. Before this patch, the driver also implicitly had
a module_exit() doing exactly this platform_driver_unregister(), it was
just hidden inside the module_platform_driver() macro. And I think
that's necessary if one wants to test that the module can be loaded and
unloaded (I don't think it's ever useful or even possible to have it be
a module on an actual imx8mp board).

For a modular build, this patch changes nothing since all foo_initcall
levels are translated to module_initcall for those. And when the driver
is built-in, the __exit code, both before and after this patch, is
discarded in the final image.

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Fabio Estevam]

On Sat, Nov 19, 2022 at 6:57 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> Sorry, I don't follow. Before this patch, the driver also implicitly had
> a module_exit() doing exactly this platform_driver_unregister(), it was
> just hidden inside the module_platform_driver() macro. And I think
> that's necessary if one wants to test that the module can be loaded and
> unloaded (I don't think it's ever useful or even possible to have it be
> a module on an actual imx8mp board).

You cannot load/unload it due to .suppress_bind_attrs = true, being passed.

> For a modular build, this patch changes nothing since all foo_initcall
> levels are translated to module_initcall for those. And when the driver
> is built-in, the __exit code, both before and after this patch, is
> discarded in the final image.

All I am suggesting is that you the patch only does:

--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -734,7 +734,19 @@ static struct platform_driver imx8mp_clk_driver = {
                .of_match_table = imx8mp_clk_of_match,
        },
 };
-module_platform_driver(imx8mp_clk_driver);
+
+static int __init imx8mp_clk_init(void)
+{
+       return platform_driver_register(&imx8mp_clk_driver);
+}
+arch_initcall(imx8mp_clk_init);

This is the same as in drivers/pinctrl/freescale/pinctrl-imx8mm.c for example.

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 19/11/2022 23.02, Fabio Estevam wrote:
> On Sat, Nov 19, 2022 at 6:57 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
>> Sorry, I don't follow. Before this patch, the driver also implicitly had
>> a module_exit() doing exactly this platform_driver_unregister(), it was
>> just hidden inside the module_platform_driver() macro. And I think
>> that's necessary if one wants to test that the module can be loaded and
>> unloaded (I don't think it's ever useful or even possible to have it be
>> a module on an actual imx8mp board).
> 
> You cannot load/unload it due to .suppress_bind_attrs = true, being passed.

That doesn't seem to be true. To test, I just built the imx8mq clk
driver as a module, and I could certainly both load and unload that on
my imx8mp platform. Sure, no "bind" attribute shows up in the
/sys/bus/platform/drivers/imx8mq-ccm/ directory, which is exactly what
one expects, but the module can be loaded just fine. And since it can be
loaded, it should also have a proper __exit call for deregistering the
driver on unload.

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Fabio Estevam]

On Mon, Nov 21, 2022 at 6:44 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> That doesn't seem to be true. To test, I just built the imx8mq clk
> driver as a module, and I could certainly both load and unload that on
> my imx8mp platform. Sure, no "bind" attribute shows up in the
> /sys/bus/platform/drivers/imx8mq-ccm/ directory, which is exactly what
> one expects, but the module can be loaded just fine. And since it can be
> loaded, it should also have a proper __exit call for deregistering the
> driver on unload.

You are right. Sorry for the confusion.

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 21/11/2022 11.59, Fabio Estevam wrote:
> On Mon, Nov 21, 2022 at 6:44 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
>> That doesn't seem to be true. To test, I just built the imx8mq clk
>> driver as a module, and I could certainly both load and unload that on
>> my imx8mp platform. Sure, no "bind" attribute shows up in the
>> /sys/bus/platform/drivers/imx8mq-ccm/ directory, which is exactly what
>> one expects, but the module can be loaded just fine. And since it can be
>> loaded, it should also have a proper __exit call for deregistering the
>> driver on unload.
> 
> You are right. Sorry for the confusion.

Thanks. Does that mean I can keep your R-b for the patch as-is?

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Fabio Estevam]

On Mon, Nov 21, 2022 at 9:29 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> Thanks. Does that mean I can keep your R-b for the patch as-is?

Yes, please keep it. Thanks

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Abel Vesa]

On 22-09-28 14:41:08, Rasmus Villemoes wrote:
> We have an imx8mp-based board with an external gpio-triggered
> watchdog. Currently, we don't get to handle that in time before it
> resets the board.
> 
> The probe of the watchdog device gets deferred because the SOC's GPIO
> controller is not yet ready, and the probe of that in turn gets deferred
> because its clock provider (namely, this driver) is not yet
> ready. Altogether, the watchdog does not get handled until the late
> initcall deferred_probe_initcall has made sure all leftover devices
> have been probed, and that's way too late.
> 
> Aside from being necessary for our board, this also reduces total boot
> time because fewer device probes get deferred.
> 

I'm gonna be honest here. I can't say I'm happy with this.
I would suggest finding a solution to disable the external watchdog
before booting the kernel, up until the driver probes, would be preferable
to me.

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> It would probably be reasonable to do the same to the other imx8m* clk
> drivers, but I don't have any such hardware to test on.
> 
>  drivers/clk/imx/clk-imx8mp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index e89db568f5a8..9ddd39a664cc 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -734,7 +734,19 @@ static struct platform_driver imx8mp_clk_driver = {
>  		.of_match_table = imx8mp_clk_of_match,
>  	},
>  };
> -module_platform_driver(imx8mp_clk_driver);
> +
> +static int __init imx8mp_clk_init(void)
> +{
> +	return platform_driver_register(&imx8mp_clk_driver);
> +}
> +arch_initcall(imx8mp_clk_init);
> +
> +static void __exit imx8mp_clk_exit(void)
> +{
> +	platform_driver_unregister(&imx8mp_clk_driver);
> +}
> +module_exit(imx8mp_clk_exit);
> +
>  module_param(mcore_booted, bool, S_IRUGO);
>  MODULE_PARM_DESC(mcore_booted, "See Cortex-M core is booted or not");
>  
> -- 
> 2.37.2
> 

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Rasmus Villemoes]

On 21/11/2022 16.43, Abel Vesa wrote:
> On 22-09-28 14:41:08, Rasmus Villemoes wrote:
>> We have an imx8mp-based board with an external gpio-triggered
>> watchdog. Currently, we don't get to handle that in time before it
>> resets the board.
>>
>> The probe of the watchdog device gets deferred because the SOC's GPIO
>> controller is not yet ready, and the probe of that in turn gets deferred
>> because its clock provider (namely, this driver) is not yet
>> ready. Altogether, the watchdog does not get handled until the late
>> initcall deferred_probe_initcall has made sure all leftover devices
>> have been probed, and that's way too late.
>>
>> Aside from being necessary for our board, this also reduces total boot
>> time because fewer device probes get deferred.
>>
> 
> I'm gonna be honest here. I can't say I'm happy with this.
> I would suggest finding a solution to disable the external watchdog
> before booting the kernel, up until the driver probes, would be preferable
> to me.

That's not an option (it would violate the very purpose of having an
external always-running watchdog), and also simply not possible on the
given hardware.

I don't understand why this simple patch can't just be applied. It hurts
nothing, it makes all imx8mp boards boot very slightly faster, there's
no maintenance burden associated with the boilerplate code, it allows
hardware that already exists to actually work with a mainline kernel
out-of-the-box. And in an alternate universe where the init function had
been arch_initcall in the initial commit (such as those in
drivers/clk/mediatek/), nobody would have asked any questions.

Rasmus

Re: [PATCH] clk: imx8mp: register driver at arch_initcall time

[Stephen Boyd]

Quoting Rasmus Villemoes (2022-11-21 23:49:50)
> On 21/11/2022 16.43, Abel Vesa wrote:
> > On 22-09-28 14:41:08, Rasmus Villemoes wrote:
> >> We have an imx8mp-based board with an external gpio-triggered
> >> watchdog. Currently, we don't get to handle that in time before it
> >> resets the board.
> >>
> >> The probe of the watchdog device gets deferred because the SOC's GPIO
> >> controller is not yet ready, and the probe of that in turn gets deferred
> >> because its clock provider (namely, this driver) is not yet
> >> ready. Altogether, the watchdog does not get handled until the late
> >> initcall deferred_probe_initcall has made sure all leftover devices
> >> have been probed, and that's way too late.
> >>
> >> Aside from being necessary for our board, this also reduces total boot
> >> time because fewer device probes get deferred.
> >>
> > 
> > I'm gonna be honest here. I can't say I'm happy with this.
> > I would suggest finding a solution to disable the external watchdog
> > before booting the kernel, up until the driver probes, would be preferable
> > to me.
> 
> That's not an option (it would violate the very purpose of having an
> external always-running watchdog), and also simply not possible on the
> given hardware.
> 
> I don't understand why this simple patch can't just be applied. It hurts
> nothing, it makes all imx8mp boards boot very slightly faster, there's
> no maintenance burden associated with the boilerplate code,

There is a maintenance burden. Moving the initcall around is papering
over the problem by not clearly describing the requirement to probe the
watchdog driver as soon as possible. I don't expect to remember years
from now that the watchdog driver needed this driver and the pinctrl
driver to avoid probe defer, otherwise the watchdog will bite because it
probes too late.

The problem is not being solved directly. That's why we're concerned.
Maybe if the problem statement was "don't allow probe defer", and that
was worked into the driver core so drivers can be marked as "panic when
this driver probe defers" then it would be more obvious what sort of
behavior we don't want.

Or to go further, maybe this board compatible needs to probe a board
driver that only adds the clk, pinctrl, and watchdog devices as a first
pass with a comment that these devices need to be probed as soon as
possible to avoid watchdog bites on that board. Then once those devices
are probed it can add the rest of the devices.

> it allows
> hardware that already exists to actually work with a mainline kernel
> out-of-the-box. And in an alternate universe where the init function had
> been arch_initcall in the initial commit (such as those in
> drivers/clk/mediatek/), nobody would have asked any questions.
> 

The usage of arch_initcall() and core_initcall() should be fixed.