Re: [edk2-platforms][PATCH 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Thu, 23 Jan 2020 at 13:00, Pete Batard <pete@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 <pete@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 <pete@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?

> +  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.

> +    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

> +    }
> +  }
> +#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

> 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 <pete@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.