| Message ID | 20250411-livetree-fixup-v2-1-1236823377bb@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm: cleanup OF_LIVE fixup and fix RB1/2 | expand |
On 11/04/2025 14:47, Caleb Connolly 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.connolly@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", "of_live_built" ? > }; > > _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. > + */ > + 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; > + } > + } > + > return ret; > } > > void of_live_free(struct device_node *root) >
Hi Caleb, On Fri, 11 Apr 2025 at 06:47, Caleb Connolly <caleb.connolly@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.connolly@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. 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. > + */ > + 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 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. 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
Hi Simon, On 4/11/25 20:27, Simon Glass wrote: > Hi Caleb, > > On Fri, 11 Apr 2025 at 06:47, Caleb Connolly <caleb.connolly@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.connolly@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. > > 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). > >> + */ >> + 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. > 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... > > 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
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. + */ + 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; + } + } + return ret; } void of_live_free(struct device_node *root)
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.connolly@linaro.org> --- common/event.c | 3 +++ include/event.h | 18 ++++++++++++++++++ lib/of_live.c | 11 +++++++++++ 3 files changed, 32 insertions(+)