[edk2-devel] [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
Pete Batard
pete at akeo.ie
Tue May 5 12:09:41 UTC 2020
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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list