From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web12.6150.1588679692077512843 for ; Tue, 05 May 2020 04:54:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=bsqId887; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.67, mailfrom: pete@akeo.ie) Received: by mail-wr1-f67.google.com with SMTP id k1so2404905wrx.4 for ; Tue, 05 May 2020 04:54:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=BbIrtgLwnb1BQwXEJusNjb48AkyNdpEZLFnx5Nd2yzM=; b=bsqId887J9DaLT0eRwyLT5RS2ZgoTAeT9hNhVCjX6JNqebi87lSpZRrg5NEa1mkI2G exYSm7oyteuji6VE6yxZuR8svNcaQJIdKvJs9RR2JbKP+dpabEQZzetxuJsaRvKUWvsU kQFg0RvKyeYEfQIByRLNFck3WWMM4pdT+Ts5IM91DpARK+waMiYSwNe+IfCbq7zSh7ly dJS7NLF1WrFPYRj+/4DKKnXX5c+G4LDwp8oQuE2/it2EQ76ncUgsGGjQvFjRrHNcoYTF QDNLaR7NUj8fG3PI4n8TiCv95vKaOFivO9UKe+66jKutts7kkSB+uyrfrNCVr6ArtfFG t4PQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=BbIrtgLwnb1BQwXEJusNjb48AkyNdpEZLFnx5Nd2yzM=; b=k63P3xjfuQrhJeM7MKwXGGMN791f0nPTigN8S9sd7dbVnHlf30HHBDEWzhZ9XCnPFm u5yqZ5z/CvbBTKXV6ldUOfMtFRI9dVVJyBNcLRDXLdm9vZONjK+rN1h6R32N4GXitXwI o5Sp8r5AcBVtNNq6mztNjYmclTUblhyv40wJnEEJxweb3dJwP+bGYE91n+fSaiV5WI2U PiRHiIDMAYvhAnyTPEM1XQACjM7k0M13dTlejbhD8huVs8ADOeqkZ9esWrp1wi+hd5qb UHjZwOrJsCBpsJUT18n0fC7Vgv+k7Dwe1SlTZoJ4ufALh8OKZdZTct7WVoJOURvYqQ7T k30w== X-Gm-Message-State: AGi0Pua0As8wynoKU1msPDinMl66/rawgDfX4SA6A8jZQi2W9sKQN5+n uh1ke4boeCrrv6jot/IW/fuJQA== X-Google-Smtp-Source: APiQypJqHkhwBJiB9rgBwyC2Cx58ZmUvLaDGTmIwDPwwGpq+9U2bCn+S/wDeVxEKfuupDjU+PluX4Q== X-Received: by 2002:a05:6000:11cb:: with SMTP id i11mr3430575wrx.339.1588679690554; Tue, 05 May 2020 04:54:50 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id 145sm3677274wma.1.2020.05.05.04.54.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 05 May 2020 04:54:49 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation To: Ard Biesheuvel , 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> From: "Pete Batard" Message-ID: <107b231b-e119-2e80-35d8-35ed27dc0cf7@akeo.ie> Date: Tue, 5 May 2020 12:54:47 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <905f3a90-276d-85a8-d7ed-669d10f99c17@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Hi Ard, 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 >> 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