From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web11.25.1580492906839397862 for ; Fri, 31 Jan 2020 09:48:27 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=IYKOiJRL; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.65, mailfrom: pete@akeo.ie) Received: by mail-wr1-f65.google.com with SMTP id g17so9708623wro.2 for ; Fri, 31 Jan 2020 09:48:26 -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=LJ/Xf1WWzMDe3g2n0wbAQTfLwO3S6BUPaDiiki5ARU4=; b=IYKOiJRLKSNmOHhA4W61G97DGPIjANHWLyOyUiCSBkQMpj5ZgWKCtCB+9OYV7alomu oNhpx2C4jyCG6rNLhVzhDvTzd+5gRzdc7najBntc6S2LSxxYVWekW7HIl+HN9o3PWEQk BdTcKXvz6tjUV6sHKjZD/+lhxnbRvJNJZjhfRrvHplSQ2qI/6T4SeCi4ly06ecQLz97Z XZaoHBs+oMiDfLms30MoQ6G00mbZaP2rrlW5/pdktsQbr+Jd8uSblkqN+n2Jye2tEhw2 NgaSyEw8ORNzmBcsJWsJvdH0yTuhFbq3V8XbSHV9oJXoAdyc79TnUCWJoG2fz0JWb5ck 4YIw== 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=LJ/Xf1WWzMDe3g2n0wbAQTfLwO3S6BUPaDiiki5ARU4=; b=gUbz+ca8JG+JHsxEAOj+kIdGZe0SspVB6zclHVLlUdbBXiSf+K8CmXb/2DHrz+4xwy YgIVgQPwvyhQgOE/K9kiM1R7gy4ZDl6uq5i+Oijr0lKgerXiFibmmXHdVc06Gum/v/za EZZLCqKIksF16dnhYhUWmHjOo9eO9mC/xgI6ck4xQmkh9VWJXeXfGNsiRfR0itSHPZne ddE1vMlUcAl7k8g8a3rXEpY9rpSg4DwCLYP4ElIUDyM65mBjY+pVi+5coVAxmboCSznC 5TF59LYOoWR6ZnrZWDU4TrZ6n+OMRXZq8g1qUPOvRhY56Hd873uHd0/lwpuwPS3foJwS XUew== X-Gm-Message-State: APjAAAXuSkDPt2DrURU2unVPPsEq6uf0VbXNJvtp95h10qiJMmE9EOoN ybZRYVABvHX7jbNFSw8XI7Sprw== X-Google-Smtp-Source: APXvYqw+44Svwh0YtY4TMnrs46LJDjyxEWLs/or0taMqxxAUddqNJ2RCoU2f1RP/iHM0xCxBjoPgFA== X-Received: by 2002:adf:ea0f:: with SMTP id q15mr13578261wrm.324.1580492905310; Fri, 31 Jan 2020 09:48:25 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.49.247]) by smtp.googlemail.com with ESMTPSA id a184sm11911178wmf.29.2020.01.31.09.48.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Jan 2020 09:48:24 -0800 (PST) Subject: Re: [edk2-platforms][PATCH v2 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: Ard Biesheuvel Cc: edk2-devel-groups-io , 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> From: "Pete Batard" Message-ID: <3ff9a4fc-ee2a-1dcb-e8d2-d95a0c191a59@akeo.ie> Date: Fri, 31 Jan 2020 17:48:23 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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, 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 >>