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

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

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

Obviously, that works for me. :)

Thanks!

/Pete