Re: [edk2] [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address

[Leif Lindholm]

On Tue, Dec 13, 2016 at 07:56:09PM -0600, Daniil Egranov wrote:
> Changed code structure per Leif Lindholm's request

Needs a commit message describing what the patch does.

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov 
> ---
> Changelog:
> 
> v2
> Corrected style issues and code structure.
> 
> v1
> The patch reads a valid MAC address form the Juno IOFPGA registers 
> and pushes it into onboard Marvell Yukon NIC.
> 
>  .../ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 270 
> +
>  .../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h|  13 +
>  2 files changed, 283 insertions(+)
> 
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c 
> b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index b97f044..bc3a433 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -15,7 +15,9 @@
>  #include "ArmJunoDxeInternal.h"
>  #include 
>  
> +#include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -69,6 +71,271 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH 
> mPciRootComplexDevicePath = {
>  EFI_EVENT mAcpiRegistration = NULL;
>  
>  /**
> +  This function reads PCI ID of the controller.
> +
> +  @param[in]  PciIo   PCI IO protocol handle
> +  @param[in]  PciId   Looking for specified PCI ID Vendor/Device
> +**/
> +STATIC
> +EFI_STATUS
> +ReadMarvellYoukonPciId (
> +  IN EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN UINT32   PciId
> +  )
> +{
> +  UINT32  DevicePciId;
> +  EFI_STATUS  Status;
> +
> +  Status = PciIo->Pci.Read (
> +PciIo,
> +EfiPciIoWidthUint32,
> +PCI_VENDOR_ID_OFFSET,
> +1,
> +);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (DevicePciId != PciId) {
> +return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function searches for Marvell Yukon NIC on the Juno
> +  platform and returns PCI IO protocol handle for the controller.
> +
> +  @param[out]  PciIo   PCI IO protocol handle
> +**/
> +STATIC
> +EFI_STATUS
> +GetMarvellYukonPciIoProtocol (
> +  OUT EFI_PCI_IO_PROTOCOL  **PciIo
> +  )
> +{
> +  UINTN   HandleCount;
> +  EFI_HANDLE  *HandleBuffer;
> +  UINTN   HIndex;
> +  EFI_STATUS  Status;
> +
> +  Status = gBS->LocateHandleBuffer (
> +  ByProtocol,
> +  ,
> +  NULL,
> +  ,
> +  );
> +  if (EFI_ERROR (Status)) {
> +return (Status);
> +  }
> +
> +  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
> +Status = gBS->OpenProtocol (
> +HandleBuffer[HIndex],
> +,
> +(VOID **) PciIo,
> +NULL,
> +NULL,
> +EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +if (EFI_ERROR (Status)) {
> +  continue;
> +}
> +
> +Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
> +if (EFI_ERROR (Status)) {
> +  // According to UEFI Spec 2.6 for EFI_BOOT_SERVICES.OpenProtocol():
> +  // when protocol was opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL 
> attribute,
> +  // the caller is not required to close the protocol interface with
> +  // EFI_BOOT_SERVICES.CloseProtocol().

I stand corrected.
But the comment could be a lot less verbose here.

It may be useful to say
  // PciIo opened with GET_PROTOCOL, CloseProtocol not required
if you want to keep confused reviewers like me on the right track.

... but I would probably rather put that comment with the OpenProtocol
line, skip the redundant continue and do

if (Status == EFI_SUCCESS) {
  break;
}

> +  continue;
> +} else {
> +  break;
> +}
> +  }
> +
> +  gBS->FreePool (HandleBuffer);
> +
> +  return Status;
> +}
> +
> +/**
> + This function restore the original controller attributes and free resources
> +
> +   @param[in]   PciIo   PCI IO protocol handle
> +   @param[in]   PciAttr PCI controller attributes.
> +   @param[in]   AcpiResDescriptor   ACPI 2.0 resource descriptors for the BAR
> +**/
> +STATIC
> +VOID
> +FreePciResources (

This function is called
"Free Resources"...

> +  IN EFI_PCI_IO_PROTOCOL*PciIo,
> +  IN UINT64 PciAttr,

...but takes a parameter called "PCI Attributes" and calls
OperationSet, so the function name is misleading. This restores the
state before GetPciResources (which is also a misleading name).
Can we call it RestorePciDev()?

> +  IN EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *AcpiResDescriptor
> +  )
> +{
> +  // According to UEFI Spec 2.6 for EFI_BOOT_SERVICES.OpenProtocol():
> +  // when protocol was opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL attribute,
> +  // the caller is not required to close the 

[edk2] [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address

[Daniil Egranov]

Changed code structure per Leif Lindholm's request

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov 
---
Changelog:

v2
Corrected style issues and code structure.

v1
The patch reads a valid MAC address form the Juno IOFPGA registers 
and pushes it into onboard Marvell Yukon NIC.

 .../ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 270 +
 .../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h|  13 +
 2 files changed, 283 insertions(+)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c 
b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index b97f044..bc3a433 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -15,7 +15,9 @@
 #include "ArmJunoDxeInternal.h"
 #include 
 
+#include 
 #include 
+#include 
 #include 
 
 #include 
@@ -69,6 +71,271 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH 
mPciRootComplexDevicePath = {
 EFI_EVENT mAcpiRegistration = NULL;
 
 /**
+  This function reads PCI ID of the controller.
+
+  @param[in]  PciIo   PCI IO protocol handle
+  @param[in]  PciId   Looking for specified PCI ID Vendor/Device
+**/
+STATIC
+EFI_STATUS
+ReadMarvellYoukonPciId (
+  IN EFI_PCI_IO_PROTOCOL  *PciIo,
+  IN UINT32   PciId
+  )
+{
+  UINT32  DevicePciId;
+  EFI_STATUS  Status;
+
+  Status = PciIo->Pci.Read (
+PciIo,
+EfiPciIoWidthUint32,
+PCI_VENDOR_ID_OFFSET,
+1,
+);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  if (DevicePciId != PciId) {
+return EFI_NOT_FOUND;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  This function searches for Marvell Yukon NIC on the Juno
+  platform and returns PCI IO protocol handle for the controller.
+
+  @param[out]  PciIo   PCI IO protocol handle
+**/
+STATIC
+EFI_STATUS
+GetMarvellYukonPciIoProtocol (
+  OUT EFI_PCI_IO_PROTOCOL  **PciIo
+  )
+{
+  UINTN   HandleCount;
+  EFI_HANDLE  *HandleBuffer;
+  UINTN   HIndex;
+  EFI_STATUS  Status;
+
+  Status = gBS->LocateHandleBuffer (
+  ByProtocol,
+  ,
+  NULL,
+  ,
+  );
+  if (EFI_ERROR (Status)) {
+return (Status);
+  }
+
+  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
+Status = gBS->OpenProtocol (
+HandleBuffer[HIndex],
+,
+(VOID **) PciIo,
+NULL,
+NULL,
+EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+if (EFI_ERROR (Status)) {
+  continue;
+}
+
+Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
+if (EFI_ERROR (Status)) {
+  // According to UEFI Spec 2.6 for EFI_BOOT_SERVICES.OpenProtocol():
+  // when protocol was opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL 
attribute,
+  // the caller is not required to close the protocol interface with
+  // EFI_BOOT_SERVICES.CloseProtocol().
+  continue;
+} else {
+  break;
+}
+  }
+
+  gBS->FreePool (HandleBuffer);
+
+  return Status;
+}
+
+/**
+ This function restore the original controller attributes and free resources
+
+   @param[in]   PciIo   PCI IO protocol handle
+   @param[in]   PciAttr PCI controller attributes.
+   @param[in]   AcpiResDescriptor   ACPI 2.0 resource descriptors for the BAR
+**/
+STATIC
+VOID
+FreePciResources (
+  IN EFI_PCI_IO_PROTOCOL*PciIo,
+  IN UINT64 PciAttr,
+  IN EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *AcpiResDescriptor
+  )
+{
+  // According to UEFI Spec 2.6 for EFI_BOOT_SERVICES.OpenProtocol():
+  // when protocol was opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL attribute,
+  // the caller is not required to close the protocol interface with
+  // EFI_BOOT_SERVICES.CloseProtocol().
+
+  PciIo->Attributes (
+   PciIo,
+   EfiPciIoAttributeOperationSet,
+   PciAttr,
+   NULL
+   );
+
+  if (AcpiResDescriptor != NULL) {
+gBS->FreePool (AcpiResDescriptor);
+  }
+}
+
+/**
+ This function provides PCI MMIO base address, old PCI controller attributes
+ and ACPI resource descriptors for the BAR.
+
+   @param[in]   PciIo   PCI IO protocol handle
+   @param[out]  PciRegBase  PCI base MMIO address
+   @param[out]  OldPciAttr  Old PCI controller attributes.
+   @param[out]  AcpiResDescriptor   ACPI 2.0 resource descriptors for the BAR
+**/
+STATIC
+EFI_STATUS
+GetPciResources (
+  IN EFI_PCI_IO_PROTOCOL *PciIo,
+  OUT UINT32 *PciRegBase,
+  OUT UINT64 *OldPciAttr,
+  OUT EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  **AcpiResDescriptor
+  )
+{
+  UINT64  AttrSupports;
+  EFI_STATUS  Status;
+
+  Status = PciIo->Attributes (
+PciIo,
+