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

[Simon Glass]

Hi Quentin,

On Wed, 6 Nov 2024 at 08:31, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> Nitpicks below.
>
> On 11/1/24 12:50 PM, 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.
> >
> > Update the docs a little, for this feature.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add autoprobe to SPL also
> > - Leave the function name the same
> > - Fix 'Prove' typo
> > - Update cover letter since SPL is now covered also
> > - Update the documentation too, with a discussion link
> >
> >   common/board_f.c                    |  4 ++++
> >   common/board_r.c                    |  4 ++++
> >   common/spl/spl.c                    |  4 ++++
> >   doc/develop/driver-model/design.rst | 15 +++++++++++++++
> >   drivers/core/root.c                 | 13 ++++++++++++-
> >   include/dm/root.h                   | 10 ++++++++++
> >   6 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/board_f.c b/common/board_f.c
> > index 1e1aa08530a..974eb4f39e9 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -822,6 +822,10 @@ static int initf_dm(void)
> >       if (ret)
> >               return ret;
> >
> > +     ret = dm_autoprobe();
> > +     if (ret)
> > +             return ret;
> > +
> >       if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
> >               ret = dm_timer_init();
> >               if (ret)
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 62228a723e1..d1507b06f8d 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -241,6 +241,10 @@ static int initr_dm(void)
> >       if (ret)
> >               return ret;
> >
> > +     ret = dm_autoprobe();
> > +     if (ret)
> > +             return ret;
> > +
> >       return 0;
>
> return dm_autoprobe();

OK

>
> ?
>
> >   }
> >   #endif
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 1ceb63daf31..c4256f237ee 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -500,6 +500,10 @@ static int spl_common_init(bool setup_malloc)
> >                       debug("dm_init_and_scan() returned error %d\n", ret);
> >                       return ret;
> >               }
> > +
> > +             ret = dm_autoprobe();
> > +             if (ret)
> > +                     return ret;
> >       }
> >
> >       return 0;
> > diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst
> > index 8c2c81d7ac9..1dcac1408ff 100644
> > --- a/doc/develop/driver-model/design.rst
> > +++ b/doc/develop/driver-model/design.rst
> > @@ -842,6 +842,21 @@ steps (see device_probe()):
> >         cause the uclass to do some housekeeping to record the device as
> >         activated and 'known' by the uclass.
> >
> > +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`` will be probed immediately after all devices are
> > +bound. For now, this happens in each xPL build as well as in U-Boot proper,
> > +both before relocation and after relocation.  See the call to ``dm_autoprobe()``
> > +for where this is done.
> > +
> > +Note that autoprobe happens after the ``EVT_DM_POST_INIT_R`` and
> > +``EVT_DM_POST_INIT_F`` events are sent, since these events relate to
> > +device-binding.
> > +
> > +See here for discussion of this feature:
> > +
> > +:Link: https://patchwork.ozlabs.org/project/uboot/patch/20240626235717.272219-1-marex@denx.de/
> > +
> >   Running stage
> >   ^^^^^^^^^^^^^
> >
> > diff --git a/drivers/core/root.c b/drivers/core/root.c
> > index 2d4f078f97f..371a37fe752 100644
> > --- a/drivers/core/root.c
> > +++ b/drivers/core/root.c
> > @@ -299,6 +299,17 @@ static int dm_probe_devices(struct udevice *dev)
> >       return 0;
> >   }
> >
> > +int dm_autoprobe(void)
> > +{
> > +     int ret;
> > +
> > +     ret = dm_probe_devices(gd->dm_root);
> > +     if (ret)
> > +             return log_msg_ret("pro", ret);
> > +
> > +     return 0;
>
> 1) Should be log_msg_retz since log_msg_ret will only print the message
> on < 0 and here we test for ret

Well we have this all over the place. I'll add a comment to
dm_probe_devices(), but log_msg_retz() is really only for cases where
the function can return a positive value (e.g. a size, or node
offset). Most functions just return 0 on success and -ve error, which
is what log_msg_ret() is for.

> 2) Simplify:
>
> return log_msg_retz("pro", dm_probe_devices(gd->dm_root));
>
> the message won't be printed if the return value is 0.

I don't like combining things like this, so tend to write it out in
full. It is easier to add to the code later. Also the semantics of
putting a required call inside a debugging call are not that clear.
Perhaps we could document this in log.h ? But I always like to have a
simple 'return 0' at the end of a function, after all the error cases
have been handled.

>
> > +}
> > +
> >   /**
> >    * dm_scan() - Scan tables to bind devices
> >    *
> > @@ -331,7 +342,7 @@ static int dm_scan(bool pre_reloc_only)
> >       if (ret)
> >               return ret;
> >
> > -     return dm_probe_devices(gd->dm_root);
> > +     return 0;
>
> Simplify with:
>
> return dm_scan_other(pre_reloc_only);

See above :-)

>
> >   }
> >
> >   int dm_init_and_scan(bool pre_reloc_only)
> > diff --git a/include/dm/root.h b/include/dm/root.h
> > index b2f30a842f5..cd240c6a716 100644
> > --- a/include/dm/root.h
> > +++ b/include/dm/root.h
> > @@ -136,6 +136,16 @@ int dm_scan_other(bool pre_reloc_only);
> >    */
> >   int dm_init_and_scan(bool pre_reloc_only);
> >
> > +/**
> > + * dm_autoprobe() - Probe devices which are marked for probe-after-bind
> > + *
> > + * This probes all devices with a DM_FLAG_PROBE_AFTER_BIND flag. It checks the
> > + * entire tree, so parent nodes need not have the flag set.
> > + *
>
> Can suggest:
> """
> It recursively probes parent nodes, so they do not need to have the flag
> set themselves.
> """
> which is what I believe you wanted to say? "Checking the entire tree"
> induces the opposite of "parent nodes need not have the flag set".

Thanks, added and I will expand a little, too.

Regards,
Simon