From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.6212.1588680349190691406 for ; Tue, 05 May 2020 05:05:49 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D942B30E; Tue, 5 May 2020 05:05:47 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.24]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A09303F68F; Tue, 5 May 2020 05:05:46 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation To: Pete Batard , devel@edk2.groups.io Cc: leif@nuviainc.com, awarkentin@vmware.com, Andrei Warkentin References: <20200504111548.11112-1-pete@akeo.ie> <20200504111548.11112-3-pete@akeo.ie> <905f3a90-276d-85a8-d7ed-669d10f99c17@arm.com> <107b231b-e119-2e80-35d8-35ed27dc0cf7@akeo.ie> From: "Ard Biesheuvel" Message-ID: <0e6ad129-cb7d-7f25-077f-7a3bb7e8c050@arm.com> Date: Tue, 5 May 2020 14:05:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <107b231b-e119-2e80-35d8-35ed27dc0cf7@akeo.ie> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/5/20 1:54 PM, Pete Batard wrote: > Hi Ard, >=20 > On 2020.05.05 11:05, Ard Biesheuvel wrote: >> On 5/4/20 1:15 PM, Pete Batard wrote: >>> From: Andrei Warkentin >>> >>> 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=20 >>> options. >>> >>> Unfortunately, it's painful to get - it's only available via the mail= box >>> interface. Additionally, SerialLib is a very limited environment, eve= n >>> 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 w= ork >>> either because it operates in both environments with MMU on (DXE phas= e) >>> 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=20 >> similar issue. >> >> So the problem is that SerialPortInitialize() is called before we know= =20 >> what value to program, and we cannot rely on other libraries to=20 >> discover this value because they may not work before their constructor= =20 >> has been called. >=20 > I wouldn't qualify the problem that way. >=20 > The problem is that we have everything we need to program the UART by=20 > the time we get into SerialPortInitialize(), because=20 > ArmPlatformPeiBootAction() has long retrieved that one variable/value w= e=20 > need, which everything downstream relies on, but sharing that one=20 > variable properly, so that it available in both PEI and DXE by the time= =20 > it is needed, is a headache. >=20 > For all intent and purposes, we always have the value we need, because=20 > that's pretty much the very first thing we query. On the other hand,=20 > directing that value to the places that consume it is problematic=20 > because most of the mechanisms we have to do that have some kind of=20 > reliance that serial output would already have been initialized... >=20 > So we have been treating it as mere problem of sharing a global variabl= e=20 > between PEI and DXE and nothing else, hence the proposal. As a matter o= f=20 > fact, we started with an alternate solution where we just added an extr= a=20 > page on top of the NvStorage reserved region, and stored the variable=20 > there at an address that DualSerialPortLib could easily retrieve in=20 > either PEI or DXE mode (with no need for the extra PlatformPeiLib then)= .=20 > Hob is just the natural progression of that, to achieve it in a more=20 > EDK2-like way. >=20 >> So can we break the contents of SerialPortInitialize() into things=20 >> that need to happen once (program the divisors etc) and things that=20 >> need to=C2=A0 > happen each and every time (figure out which UART we a= re=20 >> using in the >> first place)? >=20 > I'm trying to understand what you're getting at here. >=20 > By once and multiple time, I am assuming that you're referring to the=20 > various instantiations of the library, because I only expect=20 > SerialPortInitialize() to be called once per instance for DebugLib=20 > related uses, so technically, everything from SerialPortInitialize()=20 > could be considered a one-time operation. >=20 > So I guess what you're suggesting is that we somehow drop setting up th= e=20 > baudrate in DXE phase and just assume that it has already been set from= =20 > PEI. >=20 >> If the second set does not suffer from the same issue, can we just=20 >> move the entire logic that programs the UART block into PrePi, so that= =20 >> all subsequent users don't have to touch those registers at all? >=20 > Well, someone can and will want to switch baudrate from Shell, which=20 > means we need to compute a dynamic divisor from the base serial clock=20 > frequency. So the only way I can see your idea work is if we re-query=20 > the mbox to obtain the base serial clock frequency in=20 > SerialPortSetAttributes(), but that means that, for the compiler to be=20 > happy, it will need to set up a DXE dependency with RpiFirmwareDxe,=20 > which we can't have in PEI phase... >=20 >> This means we may need two versions of DualSerialPortLib, >=20 > Unless there exists a compile time macros that indicate whether code is= =20 > compiling in PEI or DXE phase (and even then I suspect that .inf=20 > sections will not cooperate that easily), then I don't see how we can=20 > avoid having two separate versions of DualSerialPortLib indeed, and I=20 > see that as becoming more of a headache than what we're proposing here.= .. >=20 >> where the one that PrePi uses may need to be attached to SerialDxe as=20 >> well, so that it can reprogram the baud rate as needed. But for all=20 >> the remaining DebugLib related uses, we don't really need to reprogram= =20 >> the UART at all afaics. >=20 > From a design standpoint, this may look fine, but from an=20 > implementation standpoint, when, again, the one problem we are really=20 > trying to solve is the sharing of a global variable, I fear that we are= =20 > going to shoot ourselves in the foot if we try to go in that direction,= =20 > because duplication of code, when it can be avoided, doesn't strike me=20 > like a good idea. >=20 > If we are going to push for something like that, I'd much rather work o= n=20 > another EDK2 library that solves the problem of early global variable=20 > sharing between DXE and PEI, than something that's custom to RPi and=20 > that's not going to help anybody else in the process. >=20 > However, considering the time that has already spent trying to solve=20 > this issue, I'd rather not have to do either of these, really, because = I=20 > don't think there is much to gain from adding bells and whistle to a=20 > problem that really boils down to "We need to share a global variable,=20 > that we set in early PEI, between PEI and DXE" and which we have solved= =20 > using what I believe to be the mandated EDK2 way of doing it (HOBs). >=20 > Now, if you are *really* that opposed to the current solution, I can se= e=20 > what's achievable. But I'd rather only have to do that on a major=20 > objection ("This proposal may create problems in the long run because X= ,=20 > Y...") rather than preference of how things may look be better organize= d=20 > if we did it in this other fashion... >=20 > Besides the need to add an extra PlatformPeiLib and the small hack we=20 > had to use to get to the Hob in early DXE phase, do you have specific=20 > concerns about the current proposal that you identify as reasonable=20 > ground to want to push it back? >=20 I'll have a stab at coding it up myself.