Re: [PATCH] fdt_support: add optional board_rng_seed() hook

[Simon Glass <sjg@chromium.org>]

Hi Rasmus,

On Tue, 23 Aug 2022 at 23:57, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 23/08/2022 16.35, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 23 Aug 2022 at 07:06, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 23/08/2022 15.38, Simon Glass wrote:
> >>
> >>>> +/**
> >>>> + * board_rng_seed() - Provide a seed to be passed via /chosen/rng-seed
> >>>> + *
> >>>> + * This function is called if CONFIG_BOARD_RNG_SEED is set, and must
> >>>> + * be provided by the board. It should return, via @buf, some suitable
> >>>> + * seed value to pass to the kernel.
> >>>> + *
> >>>> + * @param buf         A struct abuf for returning the seed and its size.
> >>>> + * @return            0 if ok, negative on error.
> >>>> + */
> >>>> +int board_rng_seed(struct abuf *buf);
> >>>
> >>> Instead of yet another hook, can we use EVT_FT_FIXUP? An even better
> >>> option might be to use EVT_FT_FIXUP and then call a UCLASS_BOARD
> >>> method to obtain the information.
> >>
> >> I didn't know there was anything called EVT_FT_FIXUP, and from grepping,
> >> it seems suffer the same problem as ft_board_setup() as I mention,
> >> namely running after the command line (aka /chosen/bootargs) has been
> >> set up.
> >
> > If that is the only problem, then you could add another event for
> > doing an earlier fixup.
>
> Then I'd much rather just add a board_fdt_chosen() hook called early in
> fdt_chosen(), rather than having to enable yet another overcomplicated
> generic framework. But this was very much specifically targeted at
> rng-seed, because that's a generic, defined binding in /chosen, and I
> wanted to support that explicitly rather than having each board
> implement the logic for populating that - even if, due to its nature,
> the board must supply the actual value to put there.

You can put the event spy in generic code if you like. I am trying to
think more generic ways to handle things, since we have so many
'hooks'.

>
> >> Also, I can't see how it can actually affect the blob being passed to
> >> the kernel, doesn't
> >>
> >>                 fixup.tree = oftree_default();
> >>                 ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> >>
> >> mean that fixup.tree points at U-Boot's control fdt rather than the blob
> >> that will be passed as the kernel's fdt? That seems wrong.
> >
> > Yes that is wrong for many platforms. We should probably just change
> > it, but there is a complication.
> >
> > My recent series made a start at supporting writing to a DT using the
> > ofnode interface. See vbe_simple_test_base() for some comments on the
> > current state. You could require OF_LIVE to be enabled for your new
> > feature.
> >
> > Ideally I'd like to see ofnode used for all devicetree access, but it
> > will need to be done in stages. In the meantime we should try to head
> > in that direction.
>
> Huh? You'd need to deserialize the blob we've loaded (from FIT or uImage
> or given directly to a bootm command),

yes

> then have _all_ the various fixup
> functions (setting mac addresses, populating /chosen, all the various
> arch and board fixups etc.) modify that deserialized tree,

Well they only need to use ofnode.

>  and then at
> the end of the day, you need to serialize the tree again to pass to
> linux. I don't see how that could happen incrementally, and I don't see
> what advantage this would bring anyway.

Yes that's right. It can actually happen incrementally.

Unless there is very little going on, it is faster to modify the live
tree and then flatten it before calling Linux.

>
> All that has nothing at all to do with how U-Boot code accesses U-Boot's
> control DT.

As I mentioned, ofnode can now access any tree, at least when OF_LIVE
is enabled. But as you point out, there is lots of work to do here.

Regards,
Simon