[PATCH v2 03/18] boot: Add UCLASS support for extension boards

[Kory Maincent]

On Fri, 10 Oct 2025 12:13:11 +0100
Simon Glass <sjg at chromium.org> wrote:

> Hi Kory,
> 
> On Thu, 9 Oct 2025 at 15:51, Kory Maincent (TI.com)
> <kory.maincent at bootlin.com> wrote:
> >
> > Introduce UCLASS-based extension board support to enable more
> > standardized and automatic loading of extension board device tree
> > overlays in preparation for integration with bootstd and pxe_utils.
> >
> > Signed-off-by: Kory Maincent (TI.com) <kory.maincent at bootlin.com>

...

> > +
> > +#include <alist.h>
> > +#include <command.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/uclass.h>
> > +#include <env.h>
> > +#include <extension_board.h>
> > +#include <fdt_support.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>  
> 
> nit: Please correct the ordering
> 
> https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files

Oh didn't know this rule.

> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/* For now, bind the extension device manually if none are found */
> > +static struct udevice *extension_get_dev(void)  
> 
> This should really return the error rather than gobbling it. Normally
> we use 'int' as the return value, so:
> 
> static int extension_get_dev(struct udevice *devp)

You mean rather this then:
static int extension_get_dev(struct udevice **devp)

> 
> > +{
> > +       struct driver *drv = ll_entry_start(struct driver, driver);
> > +       const int n_ents = ll_entry_count(struct driver, driver);
> > +       struct udevice *dev;
> > +       int i, ret;
> > +
> > +       /* These are not needed before relocation */
> > +       if (!(gd->flags & GD_FLG_RELOC))
> > +               return NULL;
> > +
> > +       uclass_first_device(UCLASS_EXTENSION, &dev);
> > +       if (dev)
> > +               return dev;
> > +
> > +       /* Create the extension device if not already bound */
> > +       for (i = 0; i < n_ents; i++, drv++) {
> > +               if (drv->id == UCLASS_EXTENSION) {  
> 
> If this is really what you want, you might as well just put a
> U_BOOT_DRVINFO() declaration in the source. Then driver model does it
> for you. We are supposed to use the devicetree, but it seems that that
> idea is WIP at best.

Oh I will use this MACRO instead.
 
> > +int dm_extension_scan(void)
> > +{
> > +       struct alist *extension_list = dm_extension_get_list();
> > +       struct udevice *dev = extension_get_dev();
> > +       const struct extension_ops *ops;
> > +
> > +       if (!dev || !extension_list)
> > +               return -ENODEV;
> > +
> > +       ops = device_get_ops(dev);  
> 
> extension_get_ops()

Is it really needed as it is used only once.

> 
> > +       if (!ops->scan)
> > +               return -ENODEV;  
> 
> That is normally -ENOSYS - at this point we have a device, it's just
> that it doesn't have the require method.
> 
> But it seem strange to me to allow a device that has no means to scan.

Yes, maybe this check is useless indeed, as it is the only action done by the
board code.

...

> > @@ -13,9 +14,28 @@ LIST_HEAD(extension_list);
> >  static int do_extension_list(struct cmd_tbl *cmdtp, int flag,
> >                              int argc, char *const argv[])
> >  {
> > +#if CONFIG_IS_ENABLED(SUPPORT_DM_EXTENSION_SCAN)
> > +       struct alist *dm_extension_list;
> > +#endif
> >         struct extension *extension;
> >         int i = 0;
> >
> > +#if CONFIG_IS_ENABLED(SUPPORT_DM_EXTENSION_SCAN)  
> 
> Is it possible to use if() ? Perhaps at the end of your series, when
> all the conversions are done and you can delete the old code?

As all these macro check are removed when the conversion is done I didn't
bother to use if() because I would have to take care of define but not used
warnings.

...

> > +/**
> > + * dm_extension_get_list - Get the extension list
> > + * Return: The extension alist pointer, or NULL if no such list exists.  
> 
> caller must free the list?

No.
 
> > + */
> > +struct alist *dm_extension_get_list(void);  
> 
> Is this the list of all extensions from all extension devices?

yes.

> > +/**
> > + * dm_extension_apply - Apply extension board overlay to the devicetree
> > + * @extension_num: Extension number to be applied
> > + * Return: Zero on success, negative on failure.
> > + */
> > +int dm_extension_apply(int extension_num);  
> 
> The uclass should have a method like apply(struct uclass *dev, int
> extension_num). Is the numbering global across all devices?

We currently support only one extension driver loaded at a time, therefore we
don't currently need this uclass parameter.
We will change the API when we will have several scan method possible at the
same time but I can't test it for now. I don't think we will have such cases
soon and maybe the devicetree WIP support will be ok at that time.

> > +
> > +/**
> > + * dm_extension_apply_all - Apply all extension board overlays to the
> > + *                         devicetree
> > + * Return: Zero on success, negative on failure.
> > + */
> > +int dm_extension_apply_all(void);
> > +
> >  struct extension {
> >         struct list_head list;
> >         char name[32];
> > @@ -20,6 +62,28 @@ struct extension {
> >         char other[32];
> >  };  
> 
> At some point in this series, please comment this struct

Ok I will add a patch to add comment on this.

> 
> >
> > +struct extension_ops {
> > +       /**
> > +        * scan - Add system-specific function to scan extension boards.
> > +        * @dev: extension device to use  
> 
> need to document extension_list here - I believe it is a list of
> struct extension? Or is it struct extension * ?

Oop some code leftover here.

> 
> > +        * Return: The number of extension or a negative value in case of
> > +        *         error.
> > +        */
> > +       int (*scan)(struct alist *extension_list);  
> 
> This is a bit of a strange uclass function! Since you are probing
> drivers in this uclass, you can iterate through them using
> uclass_foreach...() etc.
> 
> I am guessing that you just need to add a struct udevice * as the first arg.

As I said there is only one drivers loaded at a time supported for now.  

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com