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

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

On Mon, Jan 09, 2017 at 03:20:51PM -0600, Daniil Egranov wrote:
> From: Daniil Egranov <daniil.egranov@linaro.org>
> 
> The patch reads a valid MAC address form the Juno IOFPGA registers 
> and pushes it into onboard Marvell Yukon NIC.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov <daniil.egranov@linaro.org>

Thanks for sticking with it.

I fixed a typo in commit message an shortened the subject line
slightly, but:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as a8675a1

Regards,

Leif

> ---
> 
> Hi Leif,
> 
> The Marvell control registers should be accesable through the memory
> specified in BAR 0.
> 
> The new helper function should cover all 4 checks:
> - We can read BAR 0 attributes. 
>  We always read BAR 0
> - 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.
>  The BAR 0 for Marvell Yukon should always be a memory type. I am not 
>  sure if it's possible to have more than one descriptor for a BAR. The 
>  current implementation of PciIoGetBarAttributes() will return only one
>  descriptor based on BAR index following the End Tag one. This update 
>  will step through Address Space Descriptors anyway until the End Tag.  
> 
> Thanks,
> Daniil
> 
> Changelog:
> 
> v4
> Provided more comments on the code and corrected functions name.
> Created helper function for reading base memory address from BAR.
> Added stepping through ACPI address space descriptor until End Tag.
> 
> v3
> Changed code structure per Leif Lindholm's request
> 
> 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     | 289 +++++++++++++++++++++
>  .../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h        |  13 +
>  2 files changed, 302 insertions(+)
> 
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index b97f044..5847d0a 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,290 @@ 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) {
> +    // If PciIo opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL, the CloseProtocol() is not required
> +    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)) {
> +      continue;
> +    } else {
> +      break;
> +    }
> +  }
> +
> +  gBS->FreePool (HandleBuffer);
> +
> +  return Status;
> +}
> +
> +/**
> + This function restore the original controller attributes
> +
> +   @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
> +RestorePciDev (
> +  IN EFI_PCI_IO_PROTOCOL                *PciIo,
> +  IN UINT64                             PciAttr
> +  )
> +{
> +  PciIo->Attributes (
> +           PciIo,
> +           EfiPciIoAttributeOperationSet,
> +           PciAttr,
> +           NULL
> +           );
> +}
> +
> +/**
> + This function returns PCI MMIO base address for a controller
> +
> +   @param[in]   PciIo               PCI IO protocol handle
> +   @param[out]  PciRegBase          PCI base MMIO address
> +**/
> +STATIC
> +EFI_STATUS
> +BarIsDeviceMemory (
> +  IN   EFI_PCI_IO_PROTOCOL *PciIo,
> +  OUT  UINT32              *PciRegBase
> +  )
> +{
> +  EFI_STATUS                         Status;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *AcpiResDescriptor;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *AcpiCurrentDescriptor;
> +
> +  // Marvell Yukon's Bar0 provides base memory address for control registers
> +  Status = PciIo->GetBarAttributes (PciIo, PCI_BAR_IDX0, NULL, (VOID**)&AcpiResDescriptor);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  AcpiCurrentDescriptor = AcpiResDescriptor;
> +
> +  // Search for a memory type descriptor
> +  while (AcpiCurrentDescriptor->Desc != ACPI_END_TAG_DESCRIPTOR) {
> +
> +    // Check if Bar is memory type one and fetch a base address
> +    if (AcpiCurrentDescriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR &&
> +        AcpiCurrentDescriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM &&
> +        !(AcpiCurrentDescriptor->SpecificFlag & ACPI_SPECFLAG_PREFETCHABLE)) {
> +      *PciRegBase = AcpiCurrentDescriptor->AddrRangeMin;
> +      break;
> +    } else {
> +      Status = EFI_UNSUPPORTED;
> +    }
> +
> +    AcpiCurrentDescriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) (AcpiCurrentDescriptor + 1);
> +  }
> +
> +  gBS->FreePool (AcpiResDescriptor);
> +
> +  return Status;
> +}
> +
> +/**
> + This function provides PCI MMIO base address, old PCI controller attributes.
> +
> +   @param[in]   PciIo               PCI IO protocol handle
> +   @param[out]  PciRegBase          PCI base MMIO address
> +   @param[out]  OldPciAttr          Old PCI controller attributes.
> +**/
> +STATIC
> +EFI_STATUS
> +InitPciDev (
> +  IN EFI_PCI_IO_PROTOCOL                 *PciIo,
> +  OUT UINT32                             *PciRegBase,
> +  OUT UINT64                             *OldPciAttr
> +  )
> +{
> +  UINT64      AttrSupports;
> +  EFI_STATUS  Status;
> +
> +  // 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 EFI_PCI_IO_ATTRIBUTE_IO, EFI_PCI_IO_ATTRIBUTE_MEMORY and
> +  // EFI_PCI_IO_ATTRIBUTE_BUS_MASTER bits in the PCI Config Header
> +  AttrSupports &= EFI_PCI_DEVICE_ENABLE;
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationEnable,
> +                    AttrSupports,
> +                    NULL);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = BarIsDeviceMemory (PciIo, PciRegBase);
> +  if (EFI_ERROR (Status)) {
> +    RestorePciDev (PciIo, *OldPciAttr);
> +  }
> +
> +  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 ()
> +{
> +  UINT64                              OldPciAttr;
> +  EFI_PCI_IO_PROTOCOL*                PciIo;
> +  UINT32                              PciRegBase;
> +  EFI_STATUS                          Status;
> +
> +  Status = GetMarvellYukonPciIoProtocol (&PciIo);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = WriteMacAddress (PciRegBase);
> +
> +  RestorePciDev (PciIo, OldPciAttr);
> +
> +  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 +392,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
>