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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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