Hi Ard,
On 2020.01.31 17:05, Ard Biesheuvel wrote:
On Fri, 24 Jan 2020 at 12:54, Pete Batard <p...@akeo.ie> 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 <p...@akeo.ie>
---
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 <p...@akeo.ie>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PrintLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Protocol/RpiFirmware.h>
+
+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 <p...@akeo.ie>
+#
+# 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53615): https://edk2.groups.io/g/devel/message/53615
Mute This Topic: https://groups.io/mt/70068668/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
