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.6345.1588680584572213294 for ; Tue, 05 May 2020 05:09:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=0gOHjS9Z; 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 s8so2417510wrt.9 for ; Tue, 05 May 2020 05:09:44 -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=XpDK2GmYS5P12IDK+YQlN4fCQnyQrRfqHS8lLQDRy7s=; b=0gOHjS9ZWFhiEo4g8+DFT2FwByKlzVfpPQeFEfww3clsZYDKX5lyvvi8S+cbgFPSZd lyKgWNBVGuWti57SpH2FbHsH4ByzUSGKWbo1mIj5y7afvz+Ocn/wtlKMh3PXeSTW3o2l x6Yd6nzkaQojU0edOJ+jQXi8+eGwM3MtjzOD9qvRpqJuj8mnrhkkgLTS4AwUBH4VxCgi +gOwtLK6Znii4ldjCV2vYRzxqH7K4JgBbhr+IqWou+fmMCjtOpdWTjlLkXutr/8HdlyY HQ7iEyuFWaOG4m5cE+K2VgnEGyu4xsjh1y4Hwu+TuPLeq4VlgdsicSPPf/wNr7IGQhMl 7yYg== 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=XpDK2GmYS5P12IDK+YQlN4fCQnyQrRfqHS8lLQDRy7s=; b=EgSUXJF6tEz7JwdsuCLvR2x8tJipbcrn7CZjAVPiPBml+mz2CY4ZTgNPNBA8Zw7Icg rZjweEzl2EGHw6MwaWQV0G5xQZ6LvO4BMWlri61eghPRMgB7N5m4oPPcWFy3F1zHcs/Y hwUIIQSr/zVqOhaS2dq6ZwJCKFylonZp2Kgi3vJZz8xhlAQSGhl/CCpgcS+JYxCx75iK jJfFoFHhj5jyfirT+6/q4ath7K84laNHC73eosOwf773CW5tMFZNOVhDRqW4Qvub2ejM 0qRCGO+l+fRwmQMm7Nz2V3RSfYE6pZ19xxHGpqeDINuVEuV3Jv0G9K/neZaN7mE/sQTf VLew== X-Gm-Message-State: AGi0Puax5TwP0rXHGZ639padvGCRk/WclxsxxnQg6r0kx1U6Oc6x0ilO n/pfnW+E9/l2k7x0SmkBgxTVzg== X-Google-Smtp-Source: APiQypLNwGitlEglZP2nt9AhZ9OApC0kDilAnLsTYfOsY6myWTeUpfeu3s1T2E8qdP4aIttQ7TOCiQ== X-Received: by 2002:adf:df01:: with SMTP id y1mr3325893wrl.401.1588680583069; Tue, 05 May 2020 05:09:43 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id o18sm2895686wrp.23.2020.05.05.05.09.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 05 May 2020 05:09:42 -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> <107b231b-e119-2e80-35d8-35ed27dc0cf7@akeo.ie> <0e6ad129-cb7d-7f25-077f-7a3bb7e8c050@arm.com> From: "Pete Batard" Message-ID: Date: Tue, 5 May 2020 13:09:41 +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: <0e6ad129-cb7d-7f25-077f-7a3bb7e8c050@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit 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 >>>> >>>> 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