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

[Leif Lindholm <leif.lindholm@linaro.org>]

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 <daniil.egranov@arm.com>
> ---
> 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 <ArmPlatform.h>
>  
> +#include <IndustryStandard/Pci.h>
>  #include <Protocol/DevicePathFromText.h>
> +#include <Protocol/PciIo.h>
>  #include <Protocol/PciRootBridgeIo.h>
>  
>  #include <Guid/EventGroup.h>
> @@ -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,
> +                        &DevicePciId);
> +  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,
> +                  &gEfiPciIoProtocolGuid,
> +                  NULL,
> +                  &HandleCount,
> +                  &HandleBuffer);
> +  if (EFI_ERROR (Status)) {
> +    return (Status);
> +  }
> +
> +  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
> +    Status = gBS->OpenProtocol (
> +                    HandleBuffer[HIndex],
> +                    &gEfiPciIoProtocolGuid,
> +                    (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 protocol interface with
> +  // EFI_BOOT_SERVICES.CloseProtocol().

Again, could be less verbose here.

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

This function is called GetPci Resources, but also saves and modifies
the PCI controller attributes. This is somewhat semantically
misleading, so how about renaming it InitPciDev()?

> +  IN EFI_PCI_IO_PROTOCOL                 *PciIo,
> +  OUT UINT32                             *PciRegBase,
> +  OUT UINT64                             *OldPciAttr,
> +  OUT EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  **AcpiResDescriptor
> +  )
> +{
> +  UINT64      AttrSupports;
> +  EFI_STATUS  Status;
> +

For documentation purpose (i.e., worth being a bit verbose in code for
ARM Ltd. platforms since we know it is likely to be used as
reference), I suggest a few comments.

  // Get controller's current attributes
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationGet,
> +                    0,
> +                    OldPciAttr);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +

  // Fetch supported attributes
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationSupported,
> +                    0,
> +                    &AttrSupports);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +

  // Enable all supported attributes
> +  AttrSupports &= EFI_PCI_DEVICE_ENABLE;
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationEnable,
> +                    AttrSupports,
> +                    NULL);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = PciIo->GetBarAttributes (PciIo, 0, &AttrSupports, (VOID**)AcpiResDescriptor);

Could this change from being BAR 0 if core enumeration code changed?
If not, please change to PCI_BAR_IDX0.

The code never calls SetBarAttributes, and Supports is an optional
parameter that can be NULL. It would be more clear if it was.

> +  if (EFI_ERROR (Status)) {
> +    FreePciResources (PciIo, *OldPciAttr, NULL);
> +    return Status;
> +  }
> +
> +  if ((*AcpiResDescriptor)->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR &&
> +      (*AcpiResDescriptor)->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM &&
> +      !((*AcpiResDescriptor)->SpecificFlag & ACPI_SPECFLAG_PREFETCHABLE)) {

The above test is unreadble unless I already know what the test is
intended to discern. This is why I proposed wrapping it into a helper
function with a descriptive name. In this form it still needs a
comment explaining what the test is trying to prove that this address
range is.

Now, companion reading the code together with the specification, it
says "The configuration of a BAR of a PCI Controller is described with
one or more QWORD Address Space Descriptors followed by an End Tag.".
How do we know we have only one descriptor, or we only care about the
first one, and how do we know that can't change?

So, to summarize, this seems to check that:
- We can read BAR 0 attributes.
- BAR 0 resources contain at least one QWORD Address Space Descriptor.
- The first of these refers to something memory mapped.
- That it is not (ARM terminology) Normal memory.

Now this all sounds like a plausible way of finding the configuration
space for the on-board Yukon chip, but what is needed is an
explanation for how we know these things are all true and
unchangeable.

My vote would still be for a helper function
If the above assumptions are correct, I propose:

STATIC
EFI_STATUS
BarIsDeviceMemory (
  IN   EFI_PCI_IO_PROTOCOL *PciIo,
  OUT  UINT32              *PciRegBase
  )

Including the call to GetBarAttributes, and since there does not
appear to be any further use of the Attributes, they can be freed
directly after the check.

No further comments, and I do appreciate the substantial rework (and
the amount of learning it has meant for me around the PciIo protocol,
which I had been steadfastly dodging so far).

I can fold all of these in before committing, but I would still need:
- A commit message.
- The statement of why the 4 checks are safe.

Regards,

Leif

> +    *PciRegBase = (*AcpiResDescriptor)->AddrRangeMin;
> +  } else {
> +    FreePciResources (PciIo, *OldPciAttr, *AcpiResDescriptor);
> +    EFI_ACPI_ADDRESS_SPAStatus = EFI_UNSUPPORTED;
> +  }
> +
> +  return Status;
> +}
> +
> +/**
> + This function reads MAC address from IOFPGA and writes it to Marvell Yukon NIC
> +
> +   @param[in]   PciRegBase   PCI base MMIO address
> +**/
> +STATIC
> +EFI_STATUS
> +WriteMacAddress (
> +  IN UINT32  PciRegBase
> +  )
> +{
> +  UINT32  MacHigh;
> +  UINT32  MacLow;
> +
> +  // Read MAC address from IOFPGA
> +  MacHigh= MmioRead32 (ARM_JUNO_SYS_PCIGBE_H);
> +  MacLow = MmioRead32 (ARM_JUNO_SYS_PCIGBE_L);
> +
> +  // Set software reset control register to protect from deactivation
> +  // the config write state
> +  MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_CLR);
> +
> +  // Convert to Marvell MAC Address register format
> +  MacHigh = SwapBytes32 ((MacHigh & 0xFFFF) << 16 |
> +                         (MacLow & 0xFFFF0000) >> 16);
> +  MacLow = SwapBytes32 (MacLow) >> 16;
> +
> +  // Set MAC Address
> +  MmioWrite8 (PciRegBase + R_TST_CTRL_1, TST_CFG_WRITE_ENABLE);
> +  MmioWrite32 (PciRegBase + R_MAC, MacHigh);
> +  MmioWrite32 (PciRegBase + R_MAC_MAINT, MacHigh);
> +  MmioWrite32 (PciRegBase + R_MAC + R_MAC_LOW, MacLow);
> +  MmioWrite32 (PciRegBase + R_MAC_MAINT + R_MAC_LOW, MacLow);
> +  MmioWrite8 (PciRegBase + R_TST_CTRL_1, TST_CFG_WRITE_DISABLE);
> +
> +  // Initiate device reset
> +  MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_SET);
> +  MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_CLR);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  The function reads MAC address from Juno IOFPGA registers and writes it
> +  into Marvell Yukon NIC.
> +**/
> +STATIC
> +EFI_STATUS
> +ArmJunoSetNicMacAddress ()
> +{
> +
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *AcpiResDescriptor;
> +  UINT64                              OldPciAttr;
> +  EFI_PCI_IO_PROTOCOL*                PciIo;
> +  UINT32                              PciRegBase;
> +  EFI_STATUS                          Status;
> +
> +  Status = GetMarvellYukonPciIoProtocol (&PciIo);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GetPciResources (PciIo, &PciRegBase, &OldPciAttr, &AcpiResDescriptor);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = WriteMacAddress (PciRegBase);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  FreePciResources (PciIo, OldPciAttr, AcpiResDescriptor);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Notification function of the event defined as belonging to the
>    EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
>    the entry point of the driver.
> @@ -106,6 +373,9 @@ OnEndOfDxe (
>  
>    Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE);
>    ASSERT_EFI_ERROR (Status);
> +
> +  Status = ArmJunoSetNicMacAddress ();
> +  ASSERT_EFI_ERROR (Status);
>  }
>  
>  STATIC
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> index 662c413..df02770 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> @@ -29,6 +29,19 @@
>  
>  #include <IndustryStandard/Acpi.h>
>  
> +#define ACPI_SPECFLAG_PREFETCHABLE    0x06
> +#define JUNO_MARVELL_YUKON_ID         0x438011AB /* Juno Marvell PCI Dev ID */
> +#define TST_CFG_WRITE_ENABLE          0x02       /* Enable Config Write */
> +#define TST_CFG_WRITE_DISABLE         0x00       /* Disable Config Write */
> +#define CS_RESET_CLR                  0x02       /* SW Reset Clear */
> +#define CS_RESET_SET                  0x00       /* SW Reset Set */
> +#define R_CONTROL_STATUS              0x0004     /* Control/Status Register */
> +#define R_MAC                         0x0100     /* MAC Address */
> +#define R_MAC_MAINT                   0x0110     /* MAC Address Maintenance */
> +#define R_MAC_LOW                     0x04       /* MAC Address Low Register Offset */
> +#define R_TST_CTRL_1                  0x0158     /* Test Control Register 1 */
> +
> +
>  EFI_STATUS
>  PciEmulationEntryPoint (
>    VOID
> -- 
> 2.7.4
>