From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web09.114.1580493199403807207 for ; Fri, 31 Jan 2020 09:53:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=wCQ2+pSx; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.66, mailfrom: pete@akeo.ie) Received: by mail-wm1-f66.google.com with SMTP id f129so9707946wmf.2 for ; Fri, 31 Jan 2020 09:53:19 -0800 (PST) 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=62EpEXOPHXSwyCJP9QhLdiTFsJT5jnOkqpmtjo+3lLs=; b=wCQ2+pSxIaR3EgbJzO+WWGbZhTW5waIES3mz0S0D7kMqK//MdHjSliSKbFeZWAMXFx 9gbVwFdmWwecOEYjfGMdwl47Ftd+KDVdvi9eN+86rxn8ksnKk/0/1KCuVtxU7MFaT58N 6F544fudrfAFNnVPwDq9etlRGAOF+Lb7bmavylqKNuuetKR3hsh4EbzOG3d7bWVRRZez uy5ScTjK0LnxrZWVgLmN7Q1fA7JXWF5AaKExltS2VzEnPcAkImCOnn5OsR/mk8aeE0VR w/aaWEzZ212BWfrNNJYit8Rc+FhJDupvG9kyqkTCY4zcjMcx59Ogxjklu7d5I8O5oEqW 9xiQ== 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=62EpEXOPHXSwyCJP9QhLdiTFsJT5jnOkqpmtjo+3lLs=; b=sXe/cNnPKE7FhSAbw+SJlhHsunoDMC5d0qdh/ksACC3wi/KIxZV/BBncNW57PZufKR X+slQPevGxua1pQ48/xDRMvGlPlRHJeXS7bZ0SND7bauO2HzyYApZn2y8Sf+5UkyUr5r bsjmW5UbBWjmESspOeyXqPq2pJcG7GKapOsUOfX6cUTHSTV0CAlFqas9/FqNteP97dcz uppkaRXdWu/NyxAvgwyyCnXSwjZKL9Bgo6n1RalN+HvkT7VUe8fqMwF9Kvp26blvUtMR W3a79n1yT2iQXytKWUVwkL+VuriCYyo3yVzv5T7vNBMFt9bL2/O5dMlp1sZ5PEJLSFKZ nDMw== X-Gm-Message-State: APjAAAUNOF7Qol77E13zmlh2XlbcICcq3k4uKV5n4i6laJ8wEe6gTD1f gJhzmgXtN1t7I68qCCzG4+GKQQ== X-Google-Smtp-Source: APXvYqx/qRkKLMiRVqzaAopZtm8ARL/ELrufn4nJ+4Z7PHWkyIxDekDo+KyS5kE1MmDUgcRri5CsSg== X-Received: by 2002:a7b:cb91:: with SMTP id m17mr12465656wmi.146.1580493197823; Fri, 31 Jan 2020 09:53:17 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.49.247]) by smtp.googlemail.com with ESMTPSA id y131sm12490988wmc.13.2020.01.31.09.53.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Jan 2020 09:53:17 -0800 (PST) Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: devel@edk2.groups.io, Ard Biesheuvel Cc: Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Jeremy Linton References: <20200124115411.9364-1-pete@akeo.ie> <20200124115411.9364-3-pete@akeo.ie> <15EF09ECF7E17D89.704@groups.io> From: "Pete Batard" Message-ID: <3796c45c-388e-dd3b-86da-16e6755984a0@akeo.ie> Date: Fri, 31 Jan 2020 17:53:15 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <15EF09ECF7E17D89.704@groups.io> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit On 2020.01.31 17:48, Pete Batard via Groups.Io 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. > > 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, Actually, that's the opposite that is true here. In this hypothetical scenario, the if () condition will always be true too, which is all the more reasons to want to guard the code with a #ifdef on PcdBcmGenetRegistersAddress. Else, we're gonna be querying the MAC address and setting a PCD which we have no use for. /Pete 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? > > 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). > > Regards, > > /Pete > >> >>> +  EFI_STATUS                       Status; >>> +  UINT64                           MacAddr; >>> +  RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol; >>> + >>> +  if (PcdGet64 (PcdBcmGenetMacAddress) == 0) { >>> +    Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >>> NULL, >>> +                    (VOID**)&mFwProtocol); >>> +    ASSERT_EFI_ERROR(Status); >>> + >>> +    // >>> +    // Get the MAC address from the firmware >>> +    // >>> +    Status = mFwProtocol->GetMacAddress ((UINT8*) &MacAddr); >>> +    if (EFI_ERROR (Status)) { >>> +      DEBUG ((DEBUG_WARN, "%a: failed to retrieve MAC address\n", >>> __FUNCTION__)); >>> +    } else { >>> +      PcdSet64S (PcdBcmGenetMacAddress, MacAddr); >>> +    } >>> +  } >>> +#endif >>> + >>> +  return EFI_SUCCESS; >>> +} >>> diff --git >>> a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf >>> b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf >>> new file mode 100644 >>> index 000000000000..2a207d2b3e54 >>> --- /dev/null >>> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf >>> @@ -0,0 +1,43 @@ >>> +#/** @file >>> +# >>> +#  Copyright (c) 2020, Pete Batard >>> +# >>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent >>> +# >>> +#**/ >>> + >>> +[Defines] >>> +  INF_VERSION                    = 0x0001001A >>> +  BASE_NAME                      = PlatformPcdLib >>> +  FILE_GUID                      = 3B8409D7-D3C7-4006-823B-BFB184435363 >>> +  MODULE_TYPE                    = DXE_DRIVER >>> +  VERSION_STRING                 = 1.0 >>> +  LIBRARY_CLASS                  = NULL|DXE_DRIVER UEFI_APPLICATION >>> +  CONSTRUCTOR                    = PlatformPcdLibConstructor >>> + >>> +[Sources] >>> +  PlatformPcdLib.c >>> + >>> +[Packages] >>> +  MdePkg/MdePkg.dec >>> +  MdeModulePkg/MdeModulePkg.dec >>> +  Platform/RaspberryPi/RaspberryPi.dec >>> +  Silicon/Broadcom/Drivers/Net/BcmNet.dec >>> + >>> +[LibraryClasses] >>> +  DebugLib >>> +  PcdLib >>> +  UefiLib >>> +  PrintLib >>> + >>> +[Protocols] >>> +  gRaspberryPiFirmwareProtocolGuid              ## CONSUMES >>> + >>> +[Pcd] >>> +  gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress   ## SOMETIMES_PRODUCES >>> + >>> +[FixedPcd] >>> +  gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress >>> + >>> +[Depex] >>> +  gRaspberryPiFirmwareProtocolGuid >>> -- >>> 2.21.0.windows.1 >>> > > > >