[edk2-platforms][PATCH v2 0/3] Platform/RPi4: Add Genet network driver stub

[edk2-platforms][PATCH v2 0/3] Platform/RPi4: Add Genet network driver stub

[Pete Batard]

Changes from v1:
* Use UINT64 for PcdBcmGenetRegistersAddress
* Use PcdSet64S ()
* Remove PlatformPcdLibDestructor ()
* Reference PcdLib library class and drop the gPcdProtocolGuid Depex

Regards,

/Pete

Jeremy Linton (2):
  Silicon/Broadcom/Net: Add Genet stub driver to setup MAC
  Platform/RPi4: Enable Broadcom Genet stub driver

Pete Batard (1):
  Platform/RPi: Add PlatformPcdLib to set the Genet MAC address

 Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c   |  51 +++++++++
 Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf |  43 ++++++++
 Platform/RaspberryPi/RPi4/RPi4.dsc                             |   5 +
 Platform/RaspberryPi/RPi4/RPi4.fdf                             |   1 +
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf       |  40 +++++++
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c               | 114 ++++++++++++++++++++
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h               |  20 ++++
 Silicon/Broadcom/Drivers/Net/BcmNet.dec                        |  22 ++++
 8 files changed, 296 insertions(+)
 create mode 100644 Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
 create mode 100644 Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
 create mode 100644 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
 create mode 100644 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c
 create mode 100644 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h
 create mode 100644 Silicon/Broadcom/Drivers/Net/BcmNet.dec

-- 
2.21.0.windows.1

[edk2-platforms][PATCH v2 1/3] Silicon/Broadcom/Net: Add Genet stub driver to setup MAC

[Pete Batard]

From: Jeremy Linton <lintonrjeremy@gmail.com>

Add a stub for the Broadcom Genet network interface, that is used
by the Raspberry Pi 4 (and that may be used by other platforms).

For the time being, the stub only performs UMAC init, using a MAC
address PCD, as, even without a working network interface in UEFI
environment, it is desirable if the hardware can describe itself
for booting in an ACPI environment, rather than pass parameters via
DSDT/DSD .

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf |  40 +++++++
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c         | 114 ++++++++++++++++++++
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h         |  20 ++++
 Silicon/Broadcom/Drivers/Net/BcmNet.dec                  |  22 ++++
 4 files changed, 196 insertions(+)

diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
new file mode 100644
index 000000000000..9e9301608f24
--- /dev/null
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
@@ -0,0 +1,40 @@
+## @file
+#
+# Copyright (c) 2020, Jeremy Linton All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = BcmGenetDxe
+  FILE_GUID                      = e2b1eaf3-50b7-4ae1-b79e-ec8020cb57ac
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 0.1
+  ENTRY_POINT                    = GenetEntryPoint
+
+[Sources]
+  Genet.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+  Silicon/Broadcom/Drivers/Net/BcmNet.dec
+
+[LibraryClasses]
+  ArmLib
+  BaseLib
+  IoLib
+  UefiDriverEntryPoint
+  UefiLib
+
+[FixedPcd]
+  gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress
+
+[Pcd]
+  gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress
+
+[Depex]
+  TRUE
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c
new file mode 100644
index 000000000000..9f29bc0c0894
--- /dev/null
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.c
@@ -0,0 +1,114 @@
+/** @file
+
+  Copyright (c) 2020, Jeremy Linton All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  This driver acts like a stub to set the Broadcom
+  Genet MAC address, until the actual network driver
+  is in place.
+
+**/
+
+#include <Library/ArmLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+
+#include <Genet.h>
+#include <PiDxe.h>
+
+STATIC
+VOID
+RMWRegister (
+  UINT32                Offset,
+  UINT32                Mask,
+  UINT32                In
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Addr;
+  UINT32                Data;
+  UINT32                Shift;
+
+  Addr = GENET_BASE_ADDRESS + Offset;
+  Data = 0;
+  Shift = 1;
+  if (In) {
+    while (!(Mask & Shift))
+      Shift <<= 1;
+    Data = (MmioRead32 (Addr) & ~Mask) | ((In * Shift) & Mask);
+  } else {
+    Data = MmioRead32 (Addr) & ~Mask;
+  }
+
+  MmioWrite32 (Addr, Data);
+
+  ArmDataMemoryBarrier ();
+}
+
+STATIC
+VOID
+WdRegister (
+  UINT32                Offset,
+  UINT32                In
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Base = GENET_BASE_ADDRESS;
+
+  MmioWrite32 (Base + Offset, In);
+
+  ArmDataMemoryBarrier ();
+}
+
+STATIC
+VOID
+SetMacAddress (
+  UINT8*                MacAddr
+)
+{
+  // Bring the UMAC out of reset
+  RMWRegister (GENET_SYS_RBUF_FLUSH_CTRL, 0x2, 1);
+  gBS->Stall (10);
+  RMWRegister (GENET_SYS_RBUF_FLUSH_CTRL, 0x2, 0);
+
+  // Update the MAC
+  DEBUG ((DEBUG_INFO, "Using MAC address %02X:%02X:%02X:%02X:%02X:%02X\n",
+    MacAddr[0], MacAddr[1], MacAddr[2], MacAddr[3], MacAddr[4], MacAddr[5]));
+
+  WdRegister (GENET_UMAC_MAC0, (MacAddr[0] << 24) | (MacAddr[1] << 16) |
+    (MacAddr[2] << 8) | MacAddr[3]);
+  WdRegister (GENET_UMAC_MAC1, (MacAddr[4] << 8) | MacAddr[5]);
+
+}
+
+/**
+  The entry point of Genet UEFI Driver.
+
+  @param  ImageHandle                The image handle of the UEFI Driver.
+  @param  SystemTable                A pointer to the EFI System Table.
+
+  @retval  EFI_SUCCESS               The Driver or UEFI Driver exited normally.
+  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
+                                     SystemTable->Hdr.Revision.
+
+**/
+EFI_STATUS
+EFIAPI
+GenetEntryPoint (
+  IN  EFI_HANDLE          ImageHandle,
+  IN  EFI_SYSTEM_TABLE    *SystemTable
+  )
+{
+  UINT64 MacAddr;
+
+  // Read the MAC address
+  MacAddr = PcdGet64 (PcdBcmGenetMacAddress);
+
+  if (MacAddr != 0) {
+    SetMacAddress ((UINT8*)&MacAddr);
+  }
+
+  return EFI_SUCCESS;
+}
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h
new file mode 100644
index 000000000000..4a3827c0e0d1
--- /dev/null
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/Genet.h
@@ -0,0 +1,20 @@
+/** @file
+
+  Copyright (c) 2020, Pete Batard <pete@akeo.ie>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef BCM_GENET_H__
+#define BCM_GENET_H__
+
+#include <Library/PcdLib.h>
+
+#define GENET_BASE_ADDRESS         (FixedPcdGet64 (PcdBcmGenetRegistersAddress))
+
+#define GENET_SYS_RBUF_FLUSH_CTRL  0x0008
+#define GENET_UMAC_MAC0            0x080c
+#define GENET_UMAC_MAC1            0x0810
+
+#endif /* BCM_GENET_H__ */
diff --git a/Silicon/Broadcom/Drivers/Net/BcmNet.dec b/Silicon/Broadcom/Drivers/Net/BcmNet.dec
new file mode 100644
index 000000000000..2a8688cb09a7
--- /dev/null
+++ b/Silicon/Broadcom/Drivers/Net/BcmNet.dec
@@ -0,0 +1,22 @@
+## @file
+#
+#  Copyright (c) 2020, Pete Batard <pete@akeo.ie>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  DEC_SPECIFICATION              = 0x0001001A
+  PACKAGE_NAME                   = BcmNetPkg
+  PACKAGE_GUID                   = 34E19823-D23A-41AB-9C09-ED1225B32DFF
+  PACKAGE_VERSION                = 1.0
+
+[Guids]
+  gBcmNetTokenSpaceGuid = {0x12b97d70, 0x9149, 0x4c2f, {0x82, 0xd5, 0xad, 0xa9, 0x1e, 0x92, 0x75, 0xa1}}
+
+[PcdsFixedAtBuild]
+  gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress|0x0|UINT64|0x00000001
+
+[PcdsDynamic]
+  gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress|0x0|UINT64|0x00000002
-- 
2.21.0.windows.1

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

[Pete Batard]

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

[edk2-platforms][PATCH v2 3/3] Platform/RPi4: Enable Broadcom Genet stub driver

[Pete Batard]

From: Jeremy Linton <lintonrjeremy@gmail.com>

This adds the required references to use the Genet stub driver in
order to dynamically populate the MAC address for OS consumption.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi4/RPi4.dsc | 5 +++++
 Platform/RaspberryPi/RPi4/RPi4.fdf | 1 +
 2 files changed, 6 insertions(+)

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index b3a114b6e0ed..bd3800c1d653 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -402,6 +402,7 @@ [PcdsFixedAtBuild.common]
   gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x40000000
   gBcm27xxTokenSpaceGuid.PcdBcm27xxRegistersAddress|0xfc000000
   gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0xfe000000
+  gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress|0xfd580000
 
   # PCIe specific addresses
   gBcm27xxTokenSpaceGuid.PcdBcm27xxPciRegBase|0xfd500000
@@ -648,6 +649,10 @@ [Components.common]
   # Networking stack
   #
 !include NetworkPkg/Network.dsc.inc
+  Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
+    <LibraryClasses>
+      NULL|Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
+  }
 
   #
   # RNG
diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf
index 2bcfdb3244f6..db393d47bcea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.fdf
+++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
@@ -269,6 +269,7 @@ [FV.FvMain]
   # Networking stack
   #
 !include NetworkPkg/Network.fdf.inc
+  INF Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
 
   #
   # RNG
-- 
2.21.0.windows.1

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

[Ard Biesheuvel]

On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)

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?

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

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

[Pete Batard]

Hi Ard,

On 2020.01.31 17:05, Ard Biesheuvel wrote:
> On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
> 
> 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 <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
>> +
>> +[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
>>

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

[Pete Batard]

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

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

[Ard Biesheuvel]

On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
>
> Hi Ard,
>
> On 2020.01.31 17:05, Ard Biesheuvel wrote:
> > On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
> >
> > 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.
>

I don't think a single PlatformPcdLib is realistic, since you'd need
to link it into different drivers via NULL resolution, all of which
would inherited the conjunction of all the DEPEX clauses, potentially
causing circular dependencies.

I'd much rather have either a named library class in BcmGenet.dec
scope that the platform can implement to provide the MAC, or one that
can be used via NULL resolution (in cases where it is imaginable that
the driver does not need the MAC to be provided at all)

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

No, I don't think this is something we should care about.

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

I don't see a single library that sets all kinds of unrelated PCDs as
something highly useful tbh.

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

[Pete Batard]

On 2020.01.31 17:53, Ard Biesheuvel wrote:
> On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
>>
>> Hi Ard,
>>
>> On 2020.01.31 17:05, Ard Biesheuvel wrote:
>>> On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
>>>
>>> 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.
>>
> 
> I don't think a single PlatformPcdLib is realistic, since you'd need
> to link it into different drivers via NULL resolution, all of which
> would inherited the conjunction of all the DEPEX clauses, potentially
> causing circular dependencies.
> 
> I'd much rather have either a named library class in BcmGenet.dec
> scope that the platform can implement to provide the MAC, or one that
> can be used via NULL resolution (in cases where it is imaginable that
> the driver does not need the MAC to be provided at all)

Yes, but the problem is, right now, I have no idea what the Genet driver 
is going to look like, so we need a solution that is unlikely to 
interfere with what's going to happen in terms of Genet driver 
expansion. And this is the least intrusive solution I identified (which 
means it will also be easiest to remove if we decide to supersede it).

I personally don't think a named library is the better solution because, 
ideally, we should have two independent drivers (not libraries), one for 
UMAC and one for Genet, considering that we really want is have the 
platform rely on a UMAC hardware service implementation since we are 
dealing with 2 separate hardware functions, that pertain to networking 
but that are not directly related. So if you want to go towards the 
"There has to be a better solution that this" route, I'm would actually 
push for a separate driver rather than a library. But then I expect that 
we're going to create all kinds of problems for Jeremy when he produces 
his Genet implementation, which is precisely why I steered away from 
doing that in the first place. There are also other alternatives, such 
as implementing Simple Network Driver protocol, since this appears to 
provide means to set the MAC, but this too is going to interfere with 
Jeremy's effort.

So, and I hope this doesn't come as offensive because this is not my 
intention here, I'm afraid that, unless someone else takes that matter 
into their own hand, the current proposal is the best you're gonna get, 
unless you want to produce that named library class yourself (which, 
again, I still don't view as the better solution if that's what we are 
looking for).

What also bothers me is that you did state at the end of [1] that:

 > I don't mind this approach, but in general, I'd be happier with a
 > specific library class to discover the MAC address.

which I took as a tacit approval that, even if you didn't exactly like 
it, you were still okay with applying this patchset as long as the other 
issues were addressed. So I hope you can appreciate that I can't go 
around with expecting approval on the general approach of a patchset, 
only to see it suddenly rejected by the same person one week later...

But more pragmatically, we need a way to get networking in Linux, which 
means UMAC setup in UEFI, now. Not in two weeks. Not in one month. And 
short of someone coming up with an alternative tomorrow, I'm still 
seeing PCDLib lib as the one means to achieve that, that is going to 
create the least trouble for all the parties involved.

Regards,

/Pete

[1] https://edk2.groups.io/g/devel/message/53447

>> 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?
>>
> 
> No, I don't think this is something we should care about.
> 
>> 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).
>>
> 
> I don't see a single library that sets all kinds of unrelated PCDs as
> something highly useful tbh.
> 

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

[Ard Biesheuvel]

On Fri, 31 Jan 2020 at 19:45, Pete Batard <pete@akeo.ie> wrote:
>
> On 2020.01.31 17:53, Ard Biesheuvel wrote:
> > On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> Hi Ard,
> >>
> >> On 2020.01.31 17:05, Ard Biesheuvel wrote:
> >>> On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
> >>>
> >>> 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.
> >>
> >
> > I don't think a single PlatformPcdLib is realistic, since you'd need
> > to link it into different drivers via NULL resolution, all of which
> > would inherited the conjunction of all the DEPEX clauses, potentially
> > causing circular dependencies.
> >
> > I'd much rather have either a named library class in BcmGenet.dec
> > scope that the platform can implement to provide the MAC, or one that
> > can be used via NULL resolution (in cases where it is imaginable that
> > the driver does not need the MAC to be provided at all)
>
> Yes, but the problem is, right now, I have no idea what the Genet driver
> is going to look like, so we need a solution that is unlikely to
> interfere with what's going to happen in terms of Genet driver
> expansion. And this is the least intrusive solution I identified (which
> means it will also be easiest to remove if we decide to supersede it).
>

If the genet stub driver morphs into something more elaborate, it will
still require a platform specific way to discover the MAC the
platforms wants it to use.

> I personally don't think a named library is the better solution because,
> ideally, we should have two independent drivers (not libraries), one for
> UMAC and one for Genet, considering that we really want is have the
> platform rely on a UMAC hardware service implementation since we are
> dealing with 2 separate hardware functions, that pertain to networking
> but that are not directly related. So if you want to go towards the
> "There has to be a better solution that this" route, I'm would actually
> push for a separate driver rather than a library. But then I expect that
> we're going to create all kinds of problems for Jeremy when he produces
> his Genet implementation, which is precisely why I steered away from
> doing that in the first place. There are also other alternatives, such
> as implementing Simple Network Driver protocol, since this appears to
> provide means to set the MAC, but this too is going to interfere with
> Jeremy's effort.
>
> So, and I hope this doesn't come as offensive because this is not my
> intention here, I'm afraid that, unless someone else takes that matter
> into their own hand, the current proposal is the best you're gonna get,
> unless you want to produce that named library class yourself (which,
> again, I still don't view as the better solution if that's what we are
> looking for).
>

OK

> What also bothers me is that you did state at the end of [1] that:
>
>  > I don't mind this approach, but in general, I'd be happier with a
>  > specific library class to discover the MAC address.
>
> which I took as a tacit approval that, even if you didn't exactly like
> it, you were still okay with applying this patchset as long as the other
> issues were addressed. So I hope you can appreciate that I can't go
> around with expecting approval on the general approach of a patchset,
> only to see it suddenly rejected by the same person one week later...
>

That was before you refused to remove the #if PcdGet64() == 0

> But more pragmatically, we need a way to get networking in Linux, which
> means UMAC setup in UEFI, now. Not in two weeks. Not in one month. And
> short of someone coming up with an alternative tomorrow, I'm still
> seeing PCDLib lib as the one means to achieve that, that is going to
> create the least trouble for all the parties involved.
>
> Regards,
>
> /Pete
>
> [1] https://edk2.groups.io/g/devel/message/53447
>
> >> 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?
> >>
> >
> > No, I don't think this is something we should care about.
> >
> >> 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).
> >>
> >
> > I don't see a single library that sets all kinds of unrelated PCDs as
> > something highly useful tbh.
> >
>

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

[Ard Biesheuvel]

On Sat, 1 Feb 2020 at 09:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 31 Jan 2020 at 19:45, Pete Batard <pete@akeo.ie> wrote:
> >
> > On 2020.01.31 17:53, Ard Biesheuvel wrote:
> > > On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
> > >>
> > >> Hi Ard,
> > >>
> > >> On 2020.01.31 17:05, Ard Biesheuvel wrote:
> > >>> On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
> > >>>
> > >>> 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.
> > >>
> > >
> > > I don't think a single PlatformPcdLib is realistic, since you'd need
> > > to link it into different drivers via NULL resolution, all of which
> > > would inherited the conjunction of all the DEPEX clauses, potentially
> > > causing circular dependencies.
> > >
> > > I'd much rather have either a named library class in BcmGenet.dec
> > > scope that the platform can implement to provide the MAC, or one that
> > > can be used via NULL resolution (in cases where it is imaginable that
> > > the driver does not need the MAC to be provided at all)
> >
> > Yes, but the problem is, right now, I have no idea what the Genet driver
> > is going to look like, so we need a solution that is unlikely to
> > interfere with what's going to happen in terms of Genet driver
> > expansion. And this is the least intrusive solution I identified (which
> > means it will also be easiest to remove if we decide to supersede it).
> >
>
> If the genet stub driver morphs into something more elaborate, it will
> still require a platform specific way to discover the MAC the
> platforms wants it to use.
>
> > I personally don't think a named library is the better solution because,
> > ideally, we should have two independent drivers (not libraries), one for
> > UMAC and one for Genet, considering that we really want is have the
> > platform rely on a UMAC hardware service implementation since we are
> > dealing with 2 separate hardware functions, that pertain to networking
> > but that are not directly related. So if you want to go towards the
> > "There has to be a better solution that this" route, I'm would actually
> > push for a separate driver rather than a library. But then I expect that
> > we're going to create all kinds of problems for Jeremy when he produces
> > his Genet implementation, which is precisely why I steered away from
> > doing that in the first place. There are also other alternatives, such
> > as implementing Simple Network Driver protocol, since this appears to
> > provide means to set the MAC, but this too is going to interfere with
> > Jeremy's effort.
> >
> > So, and I hope this doesn't come as offensive because this is not my
> > intention here, I'm afraid that, unless someone else takes that matter
> > into their own hand, the current proposal is the best you're gonna get,
> > unless you want to produce that named library class yourself (which,
> > again, I still don't view as the better solution if that's what we are
> > looking for).
> >
>
> OK
>
> > What also bothers me is that you did state at the end of [1] that:
> >
> >  > I don't mind this approach, but in general, I'd be happier with a
> >  > specific library class to discover the MAC address.
> >
> > which I took as a tacit approval that, even if you didn't exactly like
> > it, you were still okay with applying this patchset as long as the other
> > issues were addressed. So I hope you can appreciate that I can't go
> > around with expecting approval on the general approach of a patchset,
> > only to see it suddenly rejected by the same person one week later...
> >
>
> That was before you refused to remove the #if PcdGet64() == 0
>

Apologies if that came out grumpy.

The reality is that it is highly unlikely that the genet driver will
ever be usable in another platform without substantial rework, so the
kind of future proofing that we're doing here is both too much and too
little. If you are concerned about timelines, just keep it simple and
inflexible, and we can refactor it later. By that time, I will also
hopefully have more bandwidth to look at this stuff.

Unrelated: you don't happen to be at FOSDEM do you?

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

[Pete Batard]

On 2020.02.01 09:48, Ard Biesheuvel wrote:
> On Sat, 1 Feb 2020 at 09:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On Fri, 31 Jan 2020 at 19:45, Pete Batard <pete@akeo.ie> wrote:
>>>
>>> On 2020.01.31 17:53, Ard Biesheuvel wrote:
>>>> On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
>>>>>
>>>>> Hi Ard,
>>>>>
>>>>> On 2020.01.31 17:05, Ard Biesheuvel wrote:
>>>>>> On Fri, 24 Jan 2020 at 12:54, 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   | 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 <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)
>>>>>>
>>>>>> 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.
>>>>>
>>>>
>>>> I don't think a single PlatformPcdLib is realistic, since you'd need
>>>> to link it into different drivers via NULL resolution, all of which
>>>> would inherited the conjunction of all the DEPEX clauses, potentially
>>>> causing circular dependencies.
>>>>
>>>> I'd much rather have either a named library class in BcmGenet.dec
>>>> scope that the platform can implement to provide the MAC, or one that
>>>> can be used via NULL resolution (in cases where it is imaginable that
>>>> the driver does not need the MAC to be provided at all)
>>>
>>> Yes, but the problem is, right now, I have no idea what the Genet driver
>>> is going to look like, so we need a solution that is unlikely to
>>> interfere with what's going to happen in terms of Genet driver
>>> expansion. And this is the least intrusive solution I identified (which
>>> means it will also be easiest to remove if we decide to supersede it).
>>>
>>
>> If the genet stub driver morphs into something more elaborate, it will
>> still require a platform specific way to discover the MAC the
>> platforms wants it to use.
>>
>>> I personally don't think a named library is the better solution because,
>>> ideally, we should have two independent drivers (not libraries), one for
>>> UMAC and one for Genet, considering that we really want is have the
>>> platform rely on a UMAC hardware service implementation since we are
>>> dealing with 2 separate hardware functions, that pertain to networking
>>> but that are not directly related. So if you want to go towards the
>>> "There has to be a better solution that this" route, I'm would actually
>>> push for a separate driver rather than a library. But then I expect that
>>> we're going to create all kinds of problems for Jeremy when he produces
>>> his Genet implementation, which is precisely why I steered away from
>>> doing that in the first place. There are also other alternatives, such
>>> as implementing Simple Network Driver protocol, since this appears to
>>> provide means to set the MAC, but this too is going to interfere with
>>> Jeremy's effort.
>>>
>>> So, and I hope this doesn't come as offensive because this is not my
>>> intention here, I'm afraid that, unless someone else takes that matter
>>> into their own hand, the current proposal is the best you're gonna get,
>>> unless you want to produce that named library class yourself (which,
>>> again, I still don't view as the better solution if that's what we are
>>> looking for).
>>>
>>
>> OK
>>
>>> What also bothers me is that you did state at the end of [1] that:
>>>
>>>   > I don't mind this approach, but in general, I'd be happier with a
>>>   > specific library class to discover the MAC address.
>>>
>>> which I took as a tacit approval that, even if you didn't exactly like
>>> it, you were still okay with applying this patchset as long as the other
>>> issues were addressed. So I hope you can appreciate that I can't go
>>> around with expecting approval on the general approach of a patchset,
>>> only to see it suddenly rejected by the same person one week later...
>>>
>>
>> That was before you refused to remove the #if PcdGet64() == 0

I actually have no problem at all removing that line.

My take away until just now was that you did not understand why the line 
was there, not that you really wanted to see it gone.

For the current use of this driver (RPi4 platform only), I'm fine with 
not having the line, and only adding it at a later stage if/as needed.

> Apologies if that came out grumpy.

Same for me. I'm not that intractable about PlatformPcdLib, but if 
there's something you really don't want to see in it, please don't be 
afraid to explicitly state so.

Therefore, unless you are fine with removing it yourself during 
integration, I can send a v2 that removes the #ifdef condition.

> 
> The reality is that it is highly unlikely that the genet driver will
> ever be usable in another platform without substantial rework,

Well, I genuinely have no idea what the genet driver is going to look 
like, so I'm not willing to make any predictions one way or another. All 
I know is that Broadcom appear to have released 5 versions of the genet 
hardware, so they don't seem intent on retiring it, which means we might 
see it being used on other platforms.

> so the
> kind of future proofing that we're doing here is both too much and too
> little. If you are concerned about timelines, just keep it simple and
> inflexible, and we can refactor it later. By that time, I will also
> hopefully have more bandwidth to look at this stuff.
> 
> Unrelated: you don't happen to be at FOSDEM do you?

I wish I was, but I'm afraid not. Maybe next year...

Regards,

/Pete