On 2020.05.05 13:05, Ard Biesheuvel wrote:
On 5/5/20 1:54 PM, Pete Batard wrote:
Hi Ard,
On 2020.05.05 11:05, Ard Biesheuvel wrote:
On 5/4/20 1:15 PM, Pete Batard wrote:
From: Andrei Warkentin <andrey.warken...@gmail.com>
Fix for https://github.com/raspberrypi/firmware/issues/1376.
For the Pi 3, to properly configure miniUART, we need the core clock,
which can be vary between VideoCore firmare release or config.txt
options.
Unfortunately, it's painful to get - it's only available via the
mailbox
interface. Additionally, SerialLib is a very limited environment, even
when linked in a DXE-mode component, because as part of a DebugLib
implementation it ends being the base prerequisite of everything.
That means a "normal" mailbox implementation like the one from
RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
Using a basic implementation such as the one in PlatformLib doesn't
work
either because it operates in both environments with MMU on (DXE phase)
and MMU off (SEC/PrePi).
Ideally, we read the value via mbox exactly once at boot (PlatformLib),
and then somehow stash it. A GUID Hob sounds appropriate, yet when
SerialPortLib operates in DXE components, we can't use the HobLib to
*locate* the Hob list itself (remember, SerialPortLib initializes
before any of HobLib's dependencies, like UeflLib...).
Yeah, the gift that keeps on giving :-) NXP are struggling with a
similar issue.
So the problem is that SerialPortInitialize() is called before we
know what value to program, and we cannot rely on other libraries to
discover this value because they may not work before their
constructor has been called.
I wouldn't qualify the problem that way.
The problem is that we have everything we need to program the UART by
the time we get into SerialPortInitialize(), because
ArmPlatformPeiBootAction() has long retrieved that one variable/value
we need, which everything downstream relies on, but sharing that one
variable properly, so that it available in both PEI and DXE by the
time it is needed, is a headache.
For all intent and purposes, we always have the value we need, because
that's pretty much the very first thing we query. On the other hand,
directing that value to the places that consume it is problematic
because most of the mechanisms we have to do that have some kind of
reliance that serial output would already have been initialized...
So we have been treating it as mere problem of sharing a global
variable between PEI and DXE and nothing else, hence the proposal. As
a matter of fact, we started with an alternate solution where we just
added an extra page on top of the NvStorage reserved region, and
stored the variable there at an address that DualSerialPortLib could
easily retrieve in either PEI or DXE mode (with no need for the extra
PlatformPeiLib then). Hob is just the natural progression of that, to
achieve it in a more EDK2-like way.
So can we break the contents of SerialPortInitialize() into things
that need to happen once (program the divisors etc) and things that
need to > happen each and every time (figure out which UART we are
using in the
first place)?
I'm trying to understand what you're getting at here.
By once and multiple time, I am assuming that you're referring to the
various instantiations of the library, because I only expect
SerialPortInitialize() to be called once per instance for DebugLib
related uses, so technically, everything from SerialPortInitialize()
could be considered a one-time operation.
So I guess what you're suggesting is that we somehow drop setting up
the baudrate in DXE phase and just assume that it has already been set
from PEI.
If the second set does not suffer from the same issue, can we just
move the entire logic that programs the UART block into PrePi, so
that all subsequent users don't have to touch those registers at all?
Well, someone can and will want to switch baudrate from Shell, which
means we need to compute a dynamic divisor from the base serial clock
frequency. So the only way I can see your idea work is if we re-query
the mbox to obtain the base serial clock frequency in
SerialPortSetAttributes(), but that means that, for the compiler to be
happy, it will need to set up a DXE dependency with RpiFirmwareDxe,
which we can't have in PEI phase...
This means we may need two versions of DualSerialPortLib,
Unless there exists a compile time macros that indicate whether code
is compiling in PEI or DXE phase (and even then I suspect that .inf
sections will not cooperate that easily), then I don't see how we can
avoid having two separate versions of DualSerialPortLib indeed, and I
see that as becoming more of a headache than what we're proposing here...
where the one that PrePi uses may need to be attached to SerialDxe as
well, so that it can reprogram the baud rate as needed. But for all
the remaining DebugLib related uses, we don't really need to
reprogram the UART at all afaics.
From a design standpoint, this may look fine, but from an
implementation standpoint, when, again, the one problem we are really
trying to solve is the sharing of a global variable, I fear that we
are going to shoot ourselves in the foot if we try to go in that
direction, because duplication of code, when it can be avoided,
doesn't strike me like a good idea.
If we are going to push for something like that, I'd much rather work
on another EDK2 library that solves the problem of early global
variable sharing between DXE and PEI, than something that's custom to
RPi and that's not going to help anybody else in the process.
However, considering the time that has already spent trying to solve
this issue, I'd rather not have to do either of these, really, because
I don't think there is much to gain from adding bells and whistle to a
problem that really boils down to "We need to share a global variable,
that we set in early PEI, between PEI and DXE" and which we have
solved using what I believe to be the mandated EDK2 way of doing it
(HOBs).
Now, if you are *really* that opposed to the current solution, I can
see what's achievable. But I'd rather only have to do that on a major
objection ("This proposal may create problems in the long run because
X, Y...") rather than preference of how things may look be better
organized if we did it in this other fashion...
Besides the need to add an extra PlatformPeiLib and the small hack we
had to use to get to the Hob in early DXE phase, do you have specific
concerns about the current proposal that you identify as reasonable
ground to want to push it back?
I'll have a stab at coding it up myself.
Obviously, that works for me. :)
Thanks!
/Pete
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58608): https://edk2.groups.io/g/devel/message/58608
Mute This Topic: https://groups.io/mt/73972541/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
