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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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

I'll have a stab at coding it up myself.