From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web11.124.1580493234614427205 for ; Fri, 31 Jan 2020 09:53:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=jdv2NITc; spf=pass (domain: linaro.org, ip: 209.85.128.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f65.google.com with SMTP id p17so9707528wma.1 for ; Fri, 31 Jan 2020 09:53:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XlZesUVelH7WSDJREVqxS3OWwjKZYrAwfOz0sGvy4tU=; b=jdv2NITc+V/KQR2Hvo1WSyP5yFkx+tNWcaUreJj2AXKLCqoW+9+etaAlB2J+09w6GG pkDV77w5DKZiws4mzl1T7EToe6cIiWV9zUfc3vEKMN7+cwPhw83207KXYoGHKcuHlyT4 fzQvt+Sa1EjM4b3VVAG3grxrcnI2cmMNkVlJSYsxArH9UkBizhdfN6hptJkrqpPlnHOJ hvSZzAcO7uE62gAEZb+b7x9/xotBXKRPX8W9qvwdQoeHHthuIBL93Hct332/1HJvjX2e r+/SVx6v5qHod3wCxDQaAWIGW4rD9UM6GkmRbHogixtqPlLRL9dboXoLGOm0fSAaBiGn FbkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XlZesUVelH7WSDJREVqxS3OWwjKZYrAwfOz0sGvy4tU=; b=WD82GqBOmOfjFGdATOuzpo9ACyO4Y7wOXYNGXmxE87Ee2WWbJZrDO2qwiB0cJy3+wQ ccFI2qpFV7bbEHOPDsAvpozwVgrki2Vw6QLTiAID8O34k/NxmJShjsp4S/gnny5pI6yr w+7J/+XwP8VQAHci+FJXZ/v+89QD42Yv+ORKyHoVvRpGfgefBJhBx/8F6GKbIhtQIOPP q03M1imSr8n/Vrl7BWrkgblTI+xKbPPSXnlTdXBmUFeLLVfzTHz0S1a1ZFGtkjaiGYsE 58n6lRUqihEVV4v09f/rqMiAAC/l8w+VG8oEHGel2mFJ1NVglbv8gC4X8sAO7TIH5eEP 3Zfw== X-Gm-Message-State: APjAAAV2e2mwH8VTBE48lvcFpMnkUGksjICieJFWvlpIoRugPTPWL3Bo HxkfPeG84P5CrZK6ZBOTAyNyb35tqHwYHXOBXFtPYw== X-Google-Smtp-Source: APXvYqwZ7CUPpLdvG6owG8Au+fIrcQ1GfV7ljjNv5a7FNmg/Oe2hq/aktsxfiC604Ien+pEgApehv5ntQQbyfEp7cJ0= X-Received: by 2002:a7b:cc81:: with SMTP id p1mr12816719wma.62.1580493232911; Fri, 31 Jan 2020 09:53:52 -0800 (PST) MIME-Version: 1.0 References: <20200124115411.9364-1-pete@akeo.ie> <20200124115411.9364-3-pete@akeo.ie> <3ff9a4fc-ee2a-1dcb-e8d2-d95a0c191a59@akeo.ie> In-Reply-To: <3ff9a4fc-ee2a-1dcb-e8d2-d95a0c191a59@akeo.ie> From: "Ard Biesheuvel" Date: Fri, 31 Jan 2020 18:53:41 +0100 Message-ID: Subject: Re: [edk2-platforms][PATCH v2 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: Pete Batard Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Jeremy Linton Content-Type: text/plain; charset="UTF-8" On Fri, 31 Jan 2020 at 18:48, Pete Batard wrote: > > Hi Ard, > > On 2020.01.31 17:05, Ard Biesheuvel wrote: > > On Fri, 24 Jan 2020 at 12:54, Pete Batard wrote: > >> > >> The Genet driver stub used by the Raspberry Pi 4 platform is > >> designed to set the MAC address according to a PCD. > >> > >> To be able to set that PCD at runtime, by using the Raspberry > >> Pi firmware interface, that has a dedicated call to retrieve > >> the MAC address, and satisfy driver dependencies in a generic > >> manner, we create a new PlatformPcdLib that can be referenced > >> by the Genet driver, to set the MAC PCD before use there. > >> > >> While it is currently only tailored around MAC PCD population > >> for Genet, we do expect this PCD library to be extended in the > >> future, to provide additional PCD facilities for other drivers. > >> > >> Signed-off-by: Pete Batard > >> --- > >> Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c | 51 ++++++++++++++++++++ > >> Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 43 +++++++++++++++++ > >> 2 files changed, 94 insertions(+) > >> > >> diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > >> new file mode 100644 > >> index 000000000000..792073a903e9 > >> --- /dev/null > >> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > >> @@ -0,0 +1,51 @@ > >> +/** @file > >> + * > >> + * Copyright (c) 2020, Pete Batard > >> + * > >> + * SPDX-License-Identifier: BSD-2-Clause-Patent > >> + * > >> + **/ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +EFI_STATUS > >> +EFIAPI > >> +PlatformPcdLibConstructor ( > >> + IN EFI_HANDLE ImageHandle, > >> + IN EFI_SYSTEM_TABLE *SystemTable > >> + ) > >> +{ > >> + > >> + // > >> + // If Genet is in use and a MAC address PCD has not been set, do it here. > >> + // > >> +#if (FixedPcdGet64 (PcdBcmGenetRegistersAddress) != 0) > > > > I still don't see why we need this conditional. This is a fixed PCD, > > so it must be set in the .DSC. If you are going to set it in the .DSC, > > why include this driver in the first place? > > Let's consider this: > > 1. We expand on PlatformPcdLib to do more than set the MAC Address PCD > for the Pi 4. For instance, we use it to set some other PCD, that is > specific to the Pi 3, or for an upcoming Pi. In this usage, we would be > using PlatformPcdLib with a .DSC where PcdBcmGenetRegistersAddress is > not set and therefore defaults to 0. > I don't think a single PlatformPcdLib is realistic, since you'd need to link it into different drivers via NULL resolution, all of which would inherited the conjunction of all the DEPEX clauses, potentially causing circular dependencies. I'd much rather have either a named library class in BcmGenet.dec scope that the platform can implement to provide the MAC, or one that can be used via NULL resolution (in cases where it is imaginable that the driver does not need the MAC to be provided at all) > 2. Since the whole PcdBcmGenetMacAddress setup section would not apply > then, we prefer ensuring that it cannot interfere or create issues, by > dropping it altogether at build time. Granted, the > if (PcdGet64 (PcdBcmGenetMacAddress) == 0) > condition could also be seen as enough of a guard, but I don't see much > of a reason not to go further, so that further alteration of the code > does not risk impacting the Pi 3 or other Pi platforms with future > developments. > > Does that make sense to you? Or do you see this as too far fetched a > scenario? > No, I don't think this is something we should care about. > I'm basically trying to make this whole library easier to maintain in > the long run, by adding some pre-emptive provisions to drop code that > may not apply (even if, in the current scenario, this library is only > used for the Pi 4 platform, where PcdBcmGenetRegistersAddress is always > set). > I don't see a single library that sets all kinds of unrelated PCDs as something highly useful tbh.