Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation

["Pete Batard" <pete@akeo.ie>]

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.warkentin@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?

Regards,

/Pete