Hi Kory,
On Fri, 10 Oct 2025 at 14:35, Kory Maincent <[email protected]> wrote:
>
> On Fri, 10 Oct 2025 12:13:11 +0100
> Simon Glass <[email protected]> wrote:
>
> > Hi Kory,
> >
> > On Thu, 9 Oct 2025 at 15:51, Kory Maincent (TI.com)
> > <[email protected]> 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) <[email protected]>
>
> ...
>
> > > +
> > > +#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)
Yes that's right. In fact you can probably drop this function and use
'uclass_first_device_err(UCLASS_EXTENSION, &dev)' ? The pre-relocation
check shouldn't be needed.
>
> >
> > > +{
> > > + 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.
Not needed, just a convention, which I'd like to keep if we can.
>
> >
> > > + 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.
Ah OK, then it doesn't matter. I had forgotten that.
>
> ...
>
> > > +/**
> > > + * 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.
OK, then perhaps just note that the caller must not free it
>
> > > + */
> > > +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.
OK, I hadn't picked that up but it makes sense. We can always change
it later, as you say. But I would like to see an apply() method in
extension_ops, so that it actually looks like a driver.
>
> > > +
> > > +/**
> > > + * 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.
There are a lot of things with only one device / driver, but passing
the device in is how these work. If you don't want to do this, we can
always worry about it later, if needed.
Regards,
Simon
