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

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

Thanks to Ryan for reminding me :)

On Wed, Oct 05, 2016 at 08:42:51PM -0500, Daniil Egranov wrote:
> 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@arm.com>
> ---
>  .../ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c     | 141 +++++++++++++++++++++
>  .../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h        |  12 ++
>  2 files changed, 153 insertions(+)
> 
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index b97f044..0c5fbd0 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -17,6 +17,8 @@
>  
>  #include <Protocol/DevicePathFromText.h>
>  #include <Protocol/PciRootBridgeIo.h>
> +#include <Protocol/PciIo.h>
> +#include <IndustryStandard/Pci.h>

Could you insert these alphabetically sorted please?

>  
>  #include <Guid/EventGroup.h>
>  #include <Guid/GlobalVariable.h>
> @@ -68,6 +70,142 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
>  
>  EFI_EVENT mAcpiRegistration = NULL;
>  
> +UINT32 SwapUINT32(UINT32 value)

Use SwapBytes32 from BaseLib instead?

> +{
> +  value = ((value << 8) & 0xFF00FF00 ) | ((value >> 8) & 0xFF00FF );
> +  return (value << 16) | (value >> 16);
> +}
> +
> +/**
> +  The function reads MAC address from Juno IOFPGA registers and writes it
> +  into Marvell Yukon NIC.
> +**/
> +STATIC
> +EFI_STATUS
> +ArmJunoSetNetworkMAC()
> +{
> +
> +  EFI_STATUS                          Status;
> +  UINTN                               HandleCount;
> +  EFI_HANDLE                          *HandleBuffer;
> +  UINTN                               HIndex;
> +  EFI_PCI_IO_PROTOCOL*                PciIo;
> +  UINT64                              PciID;
> +  UINT32                              MacHigh;
> +  UINT32                              MacLow;
> +  UINT32                              PciRegBase;
> +  UINT64                              OldPciAttributes;
> +  UINT64                              AttrSupports;
> +  UINT8                               *PciBarAttributes;
> +
> +  Status = gBS->LocateHandleBuffer (ByProtocol,
> +                                    &gEfiPciIoProtocolGuid,
> +                                    NULL, &HandleCount, &HandleBuffer);
> +
> +  if (!EFI_ERROR (Status)) {

Better to bail out and reduce the indentation for the rest of the
function.

> +    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 = PciIo->Pci.Read (
> +            PciIo,

Inconcistent indentation - the OpenProtocol above looks better.

> +            EfiPciIoWidthUint32,
> +            PCI_VENDOR_ID_OFFSET,
> +            1,
> +            &PciID
> +            );
> +
> +      if (EFI_ERROR (Status)) {
> +        continue;
> +      }
> +
> +      if ((PciID & 0xFFFFFFFF) == JUNO_MARVELL_YUKON_ID) {

Invert test and continue, to reduce indentation.

The rest of this function would be nice to break out into a helper
function. (PciIo, 

> +
> +        // Read MAC address from IOFPGA
> +        MacHigh= MmioRead32 (ARM_JUNO_SYS_PCIGBE_H);
> +        MacLow = MmioRead32 (ARM_JUNO_SYS_PCIGBE_L);
> +
> +        Status = PciIo->Attributes (
> +              PciIo,

Indentation.

> +              EfiPciIoAttributeOperationGet,
> +              0,
> +              &OldPciAttributes
> +              );
> +
> +        if (EFI_ERROR (Status)) {
> +          continue;
> +        }
> +
> +        Status = PciIo->Attributes (
> +              PciIo,

Indentation.

> +              EfiPciIoAttributeOperationSupported,
> +              0,
> +              &AttrSupports
> +              );
> +
> +        if (!EFI_ERROR (Status)) {
> +          AttrSupports &= EFI_PCI_DEVICE_ENABLE;
> +          Status = PciIo->Attributes (
> +                PciIo,

Indentation.

> +                EfiPciIoAttributeOperationEnable,
> +                AttrSupports,
> +                NULL
> +                );
> +
> +          Status = PciIo->GetBarAttributes (PciIo, 0, &AttrSupports, (VOID**)&PciBarAttributes);
> +          if (!EFI_ERROR (Status) && (((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes)->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR)) {

First of all, it would be useful to have a temporary variable for
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes to make this
more readable.

Secondly, it would also be helpful to 

> +            if (((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes)->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> +              if (!(((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes)->SpecificFlag & ACPI_SPECFLAG_PREFETCHABLE)) {
> +                PciRegBase = ((EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes)->AddrRangeMin;
> +
> +                // Clear Software Reset

Of what? Presumably MAC, but be explicit.

> +                MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_CLR);
> +
> +                // Convert to Marvell MAC Address register format
> +                MacHigh = SwapUINT32 ((MacHigh & 0xFFFF) << 16 |
> +                                     (MacLow & 0xFFFF0000) >> 16);
> +                MacLow = SwapUINT32 (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 + 4, MacLow);
> +                MmioWrite32 (PciRegBase + R_MAC_MAINT + 4, MacLow);

What does 4 constitute in the above lines? (Is there not an R_MAC_LOW
and R_MAC_HIGH?)

> +                MmioWrite8 (PciRegBase + R_TST_CTRL_1, TST_CFG_WRITE_DISABLE);
> +
> +                // Soft reset

Of what? Presumably MAC, but be explicit.

> +                MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_SET);
> +                MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_CLR);
> +
> +                Status = EFI_SUCCESS;
> +              }
> +            }
> +          }
> +
> +          PciIo->Attributes (
> +                PciIo,

Indentation.

> +                EfiPciIoAttributeOperationSet,
> +                OldPciAttributes,
> +                NULL
> +                );
> +        }
> +      }
> +    }
> +  }
> +
> +  return Status;
> +}
> +
>  /**
>    Notification function of the event defined as belonging to the
>    EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
> @@ -106,6 +244,9 @@ OnEndOfDxe (
>  
>    Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE);
>    ASSERT_EFI_ERROR (Status);
> +
> +  Status = ArmJunoSetNetworkMAC();
> +  ASSERT_EFI_ERROR (Status);
>  }
>  
>  STATIC
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> index 662c413..cb8fdf6 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInternal.h
> @@ -29,6 +29,18 @@
>  
>  #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_TST_CTRL_1                  0x0158     /* Test Control Register 1 */
> +
> +
>  EFI_STATUS
>  PciEmulationEntryPoint (
>    VOID
> -- 
> 2.7.4
>