[PATCH v3 3/3] common: Move autoprobe out to board init

[Marek Vasut]

On 11/20/24 5:55 PM, Caleb Connolly wrote:
> 
> 
> On 20/11/2024 17:03, Simon Glass wrote:
>> Hi Caleb,
>>
>> On Wed, 20 Nov 2024 at 08:51, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>>
>>> Hi Simon, Marex,
>>>
>>> On 20/11/2024 16:36, Simon Glass wrote:
>>>> Rather than doing autoprobe within the driver model code, move it out to
>>>> the board-init code. This makes it clear that it is a separate step from
>>>> binding devices.
>>>>
>>>> For now this is always done twice, before and after relocation, but we
>>>> should discuss whether it might be possible to drop the post-relocation
>>>> probe.
>>>>
>>>> For boards with SPL, the autoprobe is still done there as well.
>>>>
>>>> Note that with this change, autoprobe happens after the
>>>> EVT_DM_POST_INIT_R/F events are sent, rather than before.
>>>>
>>>> Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>
>>> [...]
>>>
>>>> +For some platforms, certain devices must be probed to get the platform into
>>>> +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND
>>>
>>> I've found the documentation around this flag lacking, and the
>>> implementation confusing. Here you say /drivers/ with this flag (which
>>> would imply I can add it to the U_BOOT_DRIVER() definition, but further
>>> below the function doc says /device/ with this flag.
>>>
>>> In practise, it seems like the flag has to be added to the device in the
>>> bind() function, and adding it to the driver definition doesn't work
>>> (this left me scrataching my head for a while). The commit message
>>> adding it explains this and it seems like the idea was to have a way to
>>> trigger probing a device from the .bind() callback.
>>>
>>> I don't think I'm the only one confused by this, since grepping for it
>>> reveals two watchdog drivers which set this on their U_BOOT_DRIVER()
>>> definition.
>>>
>>> I can only see one driver that ever enables this flag conditionally,
>>> that's the ARM PSCI driver. Every other user sets it unconditionally in
>>> the .bind() callback.
>>>
>>> Would it make sense to rework this into a driver flag?
>>>
>>> Otherwise, could you document this behaviour more explicitly in this series?
>>
>> I agree with you and I very-much want to document this better.
>>
>> The original patch[1] came with no documentation nor tests. It was
>> applied within 5 days, after Sean raised exactly the same objections
>> that I have with this feature. Those objections were overruled and the
>> patch was applied anyway.
>>
>> So here I am trying to clean this up.
> 
> Ok, good to hear. Then it would be great if at least the comment above
> the flag definition could be made more explicit (that it must be applied
> to a device not a driver).
>>
>> As to the driver thing, my main concern is that it is too easy for
>> people to add the flag, causing a large slowdown in U-Boot's
>> pre-relocation startup speed. Perhaps the solution to that could be to
>> enable bootstage everywhere and report the time in the banner?
> 
> I'm not sure what the problem is here. You're worried that people will
> set this flag when it isn't needed? This ought to be handled during
> review of the driver in question.
> 
> Certainly I don't think making features more obscure will point people
> towards better solution, more likely they'll just get frustrated,
> particularly when this flag actually IS the correct solution as is the
> case for many drivers.

The flag is per-device , see e.g. c438866b1674 ("led: Mark device 
instance with DM_FLAG_PROBE_AFTER_BIND") .

It was long needed for exactly that use case -- probe things like LEDs 
when they need to be preconfigured somehow.