Hi Caleb,
On Mon, 14 Apr 2025 at 06:33, Caleb Connolly <caleb.conno...@linaro.org> wrote:
>
> Hi Simon,
>
> On 4/11/25 20:27, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Fri, 11 Apr 2025 at 06:47, Caleb Connolly <caleb.conno...@linaro.org>
> > wrote:
> >>
> >> OF_LIVE offers a variety of benefits, one of them being that the live
> >> tree can be modified without caring about the underlying FDT. This is
> >> particularly valuable for working around U-Boot limitations like lacking
> >> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >> peripherals like the sdcard being broken (and displaying potentially
> >> worrying error messages).
> >>
> >> Add an event to signal when the live tree has been built so that we can
> >> apply fixups to it directly before devices are bound.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org>
> >> ---
> >> common/event.c | 3 +++
> >> include/event.h | 18 ++++++++++++++++++
> >> lib/of_live.c | 11 +++++++++++
> >> 3 files changed, 32 insertions(+)
> >>
> >> diff --git a/common/event.c b/common/event.c
> >> index
> >> dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986
> >> 100644
> >> --- a/common/event.c
> >> +++ b/common/event.c
> >> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >> "ft_fixup",
> >>
> >> /* main loop events */
> >> "main_loop",
> >> +
> >> + /* livetree has been built */
> >> + "of_live_init",
> >> };
> >>
> >> _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name
> >> size");
> >> #endif
> >> diff --git a/include/event.h b/include/event.h
> >> index
> >> 75141a192a48b0931667632f41be8ff4d6139f7c..1d267f1d10547642d381fa287ab4981a2bf03543
> >> 100644
> >> --- a/include/event.h
> >> +++ b/include/event.h
> >> @@ -152,8 +152,17 @@ enum event_t {
> >> * A non-zero return value causes the boot to fail.
> >> */
> >> EVT_MAIN_LOOP,
> >>
> >> + /**
> >> + * @EVT_OF_LIVE_BUILT:
> >> + * This event is triggered immediately after the live device tree
> >> has been
> >> + * built. This allows for machine specific fixups to be done to
> >> the live tree
> >> + * (like disabling known-unsupported devices) before it is used.
> >> This
> >> + * event is only available if OF_LIVE is enabled and is only used
> >> after relocation.
> >
> > Add a bit more detail, e.g.: before it is used by U-Boot
> > post-relocation. This means it is possible to affect the devices bound
> > by driver model.
>
> I think this is really implied and made obvious by context clues. IF
> there comes a time where we build the live tree more than once (e.g. to
> build it as part of the DT_FIXUP_PROTOCOL for some reason) then the
> extra info you propose adding would no longer be correct, where the
> current wording would remain correct.
OK, leave it alone if you like.
>
> >
> > You should also note that the flattree is inactive when livetree is
> > used and any changes may eventually be passed onto the OS, if it
> > doesn't have its own devicetree. This is the point we were discussing
> > on the other thread.
>
> This information is totally irrelevant though, and there is no broad
> agreement with your proposed changes. The livetree API is also set up so
> that one could build multiple livetrees (for whatever reason), this
> event is modelled to reflect that (where you can see in my v1 patches
> that the event assumed you would only ever build the global tree).
Hmmm, if Linaro is going to block livetree fixups then we are going to
have a problem. I have not heard that, but let me know.
>
> >
> >> + */
> >> + EVT_OF_LIVE_BUILT,
> >> +
> >> /**
> >> * @EVT_COUNT:
> >> * This constants holds the maximum event number + 1 and is used
> >> when
> >> * looping over all event classes.
> >> @@ -202,8 +211,17 @@ union event_data {
> >> struct event_ft_fixup {
> >> oftree tree;
> >> struct bootm_headers *images;
> >> } ft_fixup;
> >> +
> >> + /**
> >> + * struct event_of_live_built - livetree has been built
> >> + *
> >> + * @root: The root node of the live device tree
> >> + */
> >> + struct event_of_live_built {
> >> + struct device_node *root;
> >> + } of_live_built;
> >> };
> >>
> >> /**
> >> * struct event - an event that can be sent and received
> >> diff --git a/lib/of_live.c b/lib/of_live.c
> >> index
> >> 90b9459ede313e492e28c8556c730f3bd8aaa9df..c1620616513c2e32448b4a6d156a9162d97c76b7
> >> 100644
> >> --- a/lib/of_live.c
> >> +++ b/lib/of_live.c
> >> @@ -10,8 +10,9 @@
> >>
> >> #define LOG_CATEGORY LOGC_DT
> >>
> >> #include <abuf.h>
> >> +#include <event.h>
> >> #include <log.h>
> >> #include <linux/libfdt.h>
> >> #include <of_live.h>
> >> #include <malloc.h>
> >> @@ -320,8 +321,9 @@ int unflatten_device_tree(const void *blob, struct
> >> device_node **mynodes)
> >>
> >> int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >> {
> >> int ret;
> >> + union event_data evt;
> >>
> >> debug("%s: start\n", __func__);
> >> ret = unflatten_device_tree(fdt_blob, rootp);
> >> if (ret) {
> >> @@ -334,8 +336,17 @@ int of_live_build(const void *fdt_blob, struct
> >> device_node **rootp)
> >> return ret;
> >> }
> >> debug("%s: stop\n", __func__);
> >>
> >> + if (CONFIG_IS_ENABLED(EVENT)) {
> >> + evt.of_live_built.root = *rootp;
> >> + ret = event_notify(EVT_OF_LIVE_BUILT, &evt, sizeof(evt));
> >> + if (ret) {
> >> + log_debug("Failed to notify livetree build event:
> >> err=%d\n", ret);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >
> > This should move to the caller of this function. We should also have a
>
> Why move it to the caller? Then every caller would need to signal the
> event and we'd pointlessly duplicate code. The event signals that /a/
> livetree has been built. Today this only happens once and we make the
> assumption that this livetree is to be used by U-Boot and only U-Boot.
>
> Should this assumption become incorrect in the future, it would be
> trivial to introduce a variable to the event_data that describes what
> the livetree is and/or where it will be used so that anyone catching the
> event can decide what to do.
>
> Since we don't yet know any other usecases, it would be overzealous to
> try and capture this information since we just don't know what's important.
There is (and will only ever be) one caller. At this point I really
don't mind what you do.
>
> > sandbox test for this feature, e.g. catch the event and add a
> > node/prop and check in a test in test-fdt.c (or similar) that it is
> > present.
>
> What would this test other than that the event is sent? We already have
> tests for the event framework...
If the CI tests pass, then it's fine.
>
> >
> > I'm not sure if you need to update test_event_dump.py but since CI
> > presumably passes, I guess not.>
> >> return ret;
> >> }
> >>
> >> void of_live_free(struct device_node *root)
> >>
> >> --
> >> 2.49.0
> >>
Regards,
Simon
