On 2020.01.23 13:50, Ard Biesheuvel wrote:
On Thu, 23 Jan 2020 at 13:00, 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 | 61
++++++++++++++++++++
Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 44
++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
new file mode 100644
index 000000000000..78f3679e2e48
--- /dev/null
+++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
@@ -0,0 +1,61 @@
+/** @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)
Can we drop this check? Why would you include this library in your
.DSC and set the fixed PCD to 0?
Well, the idea is that we may use this PCD library on the Pi 3 as well,
if we want to use a similar approach to this for other features, in
which case PcdBcmGenetRegistersAddress will be 0.
The fact that this library resides in Platform/RaspberryPi/Library/
rather than Platform/RaspberryPi/RPi4/Library/ means that it is designed
to be used on more than the Pi 4 platform if needed.
+ EFI_STATUS Status;
+ UINT64 MacAddr;
+ RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
+
+ if (PcdGet64 (PcdBcmGenetMacAddress) == 0) {
This is policy, which we don't need today, and may well be needed
elsewhere once we do need it. So I'd prefer it if we just drop this
check.
I'm not sure I understand your point.
This patchset is designed around the idea that someone may also provide
the MAC address at build time to force a specific MAC on their platform.
I can see specific scenarios for such a use (e.g. someone using Pi 4's
in a environment where they assign a specific IP according to the MAC
and planning for drop-in replacement in case of hardware failures) and
even outside of these, I can see people wanting for force a MAC that is
different than the hardware's (e.g. could also be handy for spoofing an
existing network device with an inexpensive Pi 4), without having to
touch the code. Granted, they should be able to perform the same in high
level OS, but once we add a full NIC driver in UEFI, we may announce a
different MAC prior to OS network init, and I think, since we can do it
here, and it will be picked up by the OS, we might as well do so since
it's easy to add.
As such, if someone did set the MAC PCD at build time, we shouldn't
override it.
+ 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 {
+ PcdSet64 (PcdBcmGenetMacAddress, MacAddr);
Please use PcdSet64S () instead - the unsafe ones are deprecated AFAIR
Will do.
+ }
+ }
+#endif
+
+ return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+PlatformPcdLibDestructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return EFI_SUCCESS;
+}
No need for a destructor
Yeah, I wasn't sure if that worked without one, and PlatformUiAppLib has
a similar empty destructor, so I preferred to ensure it was present.
I'll remove it form the patchset.
diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
new file mode 100644
index 000000000000..a564ddfbb2a3
--- /dev/null
+++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
@@ -0,0 +1,44 @@
+#/** @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
+ DESTRUCTOR = PlatformPcdLibDestructor
+
+[Sources]
+ PlatformPcdLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ Platform/RaspberryPi/RaspberryPi.dec
+ Silicon/Broadcom/Drivers/Net/BcmNet.dec
+
+[LibraryClasses]
+ UefiLib
+ DebugLib
+ PrintLib
+
+[Protocols]
+ gRaspberryPiFirmwareProtocolGuid ## CONSUMES
+
+[Pcd]
+ gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress ## SOMETIMES_PRODUCES
+
+[FixedPcd]
+ gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress
+
+[Depex]
+ gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
+
Use the PcdLib library class instead - this will depex on the
gPcdProtocolGuid if needed.
I don't mind this approach, but in general, I'd be happier with a
specific library class to discover the MAC address.
I.e., a library class in BcmNet.dec called BcmGenetPlatformLib, which
has [for now] a single function called BcmGenetGetMacFromPlatform().
The RPi4 implementation would provide that routine, and depex on the
firmware protocol, ensuring the ordering is correct. That way, we can
get rid of the PCDs entirely.
I get your point. But I'd rather not do that, since I do see some use
for a PCD provided MAC.
Regards,
/Pete
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53449): https://edk2.groups.io/g/devel/message/53449
Mute This Topic: https://groups.io/mt/70045881/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
