Hi Simon,
On 4/10/25 16:15, Simon Glass wrote:
Hi Caleb,
On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.conno...@linaro.org> wrote:
On 4/10/25 15:07, Simon Glass wrote:
Hi Caleb,
On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.conno...@linaro.org> wrote:
Hi Simon,
On 4/10/25 13:27, Simon Glass wrote:
Hi Caleb,
On Wed, 9 Apr 2025 at 11:17, 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 | 9 +++++++++
lib/of_live.c | 3 +++
3 files changed, 15 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..3fc673ba635ed45467aae8587705d37bef1c2a3f
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_INIT:
+ * 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 DM init happens.
This
+ * event is only available if OF_LIVE is enabled and is only used after
relocation.
+ */
+ EVT_OF_LIVE_INIT,
+
/**
* @EVT_COUNT:
* This constants holds the maximum event number + 1 and is used when
* looping over all event classes.
diff --git a/lib/of_live.c b/lib/of_live.c
index
90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a
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>
@@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node
**rootp)
return ret;
}
debug("%s: stop\n", __func__);
+ event_notify_null(EVT_OF_LIVE_INIT);
+
This should go in initr_of_live() since the function you are dealing
with here is supposed to just do the live build.
Well, we only every call this function from one place right now, but if
it was called multiple times for some reason then I would want to be
able to re-apply fixups to the new live tree.... I guess it should
probably pass in *rootp to the event handler, let me rework that.>
There's no need to change the root, so what you have is find here.
Same for the EFI_STUB thing which I just noticed, actually
what EFI_STUB thing?>
Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
when you get to it, then.
Also please check for error
Otherwise this seems OK to me. I do wonder why we can't use
EVT_FT_FIXUP though. Could you add mention of that to your comment in
event.h?
Because FT_FIXUP is for fixing up the flat tree before starting the OS?
these are obviously different things imo, im not sure how i could
clarify this.>
Well, what is the purpose of your code? Are you saying that it is used
within U-Boot, but not passed to the OS?
Yes, please read the cover letter and commit messages. FT_FIXUP allows
for the FDT that is about to be passed to the OS to be fixed up,
OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
The livetree is obviously not used outside of U-Boot, being a totally
custom in-memory representation of the DT.
Oh, I wondered what '(which is not passed on to further boot stages)' meant.
If you don't understand what my patch series does, it would save us both
a lot of time if you asked me to clarify some specific point than to
reply with your stream-of-consciousness ponderings about what perhaps I
might be doing.
I've tried -- again -- to explain what we're doing here, comments below.
Sorry, no, we can't do that.
We have been for months already
We are moving towards using livetree in U-Boot
indeed, mach-snapdragon has been using it for 372 days now
commit 1534186f2953d99dcbc757a19b43e4fac644e8a9
Author: Caleb Connolly <caleb.conno...@linaro.org>
Date: Wed Apr 3 14:07:49 2024 +0200
qcom_defconfig: enable livetree
- there is already the fixup thing as mentioned.
The FT_FIXUP event which has absolutely nothing to do with livetree or
this patch series and which is triggered at a different point in the
boot process for an entirely different purpose (preparing for the OS
rather than preparing for U-Boot itself)?
We just don't yet have the flattening at the end before boot.
Which is exactly what we leverage in mach-snapdragon to bridge the gap
between U-Boot (a bootloader that intentionally has a simplified model
of the hardware) and upstream DT which contains a lot of additional
context which is (as far as U-Boot is concerned) noise that we have to
filter out.
> If you have time you could look at that.
this is something literally nobody wants that wouldn't even be used in
most cases (since EFI DT FIXUP protocol passes you an FDT anyway).
Your event to modify the livetree is fine, but any modifications
should be passed on to the OS.
As stated clearly in the commit message and the cover letter, making
modifications that don't get passed on to the OS is the entire point.
Assuming you didn't take the time to look at mach-snapdragon/of_fixup.c,
it is literally rewriting the livetree to patch out the USB superspeed
phy and only initialise USB in high-speed mode. This is quite simply
because U-Boot doesn't have PHY drivers for superspeed USB on Qualcomm
platforms yet.
We absolutely don't want this to be passed on to the OS because it is
entirely an implementation detail in U-Boot. It's functionally All this
additional complexiequivalent to adding a bunch of "#ifdef
ARCH_SNAPDRAGON" ... "skip the superspeed phy" in the dwc3 driver.
This is really basic abstraction, and hopefully you'll tell me that I
didn't need to explain all this because you did in fact take the time to
understand how mach-snapdragon is using the livetree before replying
(like by reading the big comment at the top of of_fixup.c).
If you don't want that, then your fixup would need to remove them, I suppose.
why why why???
> Does the OS actually care, though?
The OS would almost certainly be unhappy at the tree U-Boot is using
internally yes, because we don't bother fixing up stuff like the
usb-connector nodes which are unused in U-Boot.
I know, I seem very frustrated, and I am. I have no problem with you
misunderstanding a patch or asking for clarifications, but you keep
repeatedly misunderstanding fundamental technical realities of an issue
or patch series and then seemingly doubling down and just saying
whatever comes to mind without exerting any effort to understand the
other side.
----
You didn't ask, but I'm going to explain how I review patches to
hopefully convey some of my confusion here because I'm worried this is a
pattern that is repeating itself and it is starting to wear on me. I've
never been one to beat around the bush I guess.
My first thought when I don't understand some change a patch is making
is "huh I wonder why they did that instead of this", but you will never
see me say that verbatim on the lists because I immediately go and read
the code to remind myself what it is doing and validate any assumptions
I might be making, as well as to try to understand the thought process
of the author.
What you WILL see me say on the lists is either
1) A (usually backed up by references) explanation of why I think the
change is wrong, or
2) If I'm really confused I'll usually try to explain what I think
they're trying to do (again backed up by code references where relevant)
and ask for clarification.
This way in (1) the author can say "oh yeah my bad, thanks for noticing
that" or point out what I missed that makes their change make sense.
This is easy, painless, everyone can go about their day (unless there is
a technical disagreement, but really that's besides the point here)
In the second case, they can confirm that my understanding is correct
and respond to my (typically pointed) questions to clarify both how they
understand the code and whatever context I'm missing. Assuming I'm any
good at asking the right questions, this results in the same outcomes as
(1).
This works because I'm trying to bridge the gap between my understanding
and the patch authors, I have my own mental model of how things work but
it could be wrong, so I want to convey what my model is so that it can
be corrected in that case, and so that if the authors understanding is
wrong then they can correct it.
Your reviews don't do this, they leave me guessing wildly at what you
might possibly think my code is doing.
There is no polite way for me to say "this comment you made is so out of
left field that I think you fundamentally misunderstand this patch", but
if you tell me what you think I'm trying to do (if you're at all
unsure), so I know what foundation you're working on, then I can easily
and politely say "ah your understanding here is wrong", you can re-do
your review with a correct mental model and I don't have to try and
parse things the way I have done above.
If I were to go as far as to define a rule for patch review, it would be
"whenever the words 'i wonder' go through your head, try to find out
before sending your reply, and include your findings if you don't find a
satisfactory answer"
Kind regards,
Regards,
Simon
--
Caleb (they/them)
