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

[Caleb Connolly]

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.

Kind regards,
> 
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20220422131555.123598-1-marex@denx.de/

-- 
// Caleb (they/them)