public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Leif Lindholm <leif.lindholm@linaro.org>
To: Daniil Egranov <daniil.egranov@arm.com>
Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org
Subject: Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
Date: Tue, 13 Dec 2016 14:00:42 +0000	[thread overview]
Message-ID: <20161213140041.GF17590@bivouac.eciton.net> (raw)
In-Reply-To: <1481613864-114733-1-git-send-email-daniil.egranov@arm.com>

On Tue, Dec 13, 2016 at 01:24:24AM -0600, Daniil Egranov wrote:
> Corrected style issues and code structure.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
> ---
> Changelog:
> 
> v1
> The patch reads a valid MAC address form the Juno IOFPGA registers 
> and pushes it into onboard Marvell Yukon NIC.

While an omprovement coding-style wise, this is still
wall-of-protocol-juggling, and extremely difficult to
review.

I did ask for some helper functions to be created in my v1 review, but
perhaps not clearly enough. So below is an example of how this could
be reworked to be something I could review and feel reasonably
confident about in 5 minutes.

I'm not saying my version is flawless (or even that it works), but it
is an indication of how I want new code to look like. And I am going
to be pickier with this for the ARM Ltd. platforms, because we know
from experience that people look at those for guidance.

What this reworking enabled me to see at a glance, for example, is how
the original kept iterating over the handle buffer even after
successful programming of MAC address, and creating new
EfiPciIoProtocol instances on each iteration (and never freeing them).

Regards,

Leif

>From e93bc891976d6e5a2efa46096d2faa5886f45711 Mon Sep 17 00:00:00 2001
From: Daniil Egranov <daniil.egranov@arm.com>
Date: Tue, 13 Dec 2016 01:24:24 -0600
Subject: [PATCH] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell
 Yukon MAC address

Leif's hacked-up version.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 .../ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c     | 235 +++++++++++++++++++++
 .../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h        |  13 ++
 2 files changed, 248 insertions(+)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index b97f044..3a35f8b 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>
@@ -68,6 +70,236 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
 
 EFI_EVENT mAcpiRegistration = NULL;
 
+
+STATIC
+BOOLEAN
+IsMyYukon (
+  IN EFI_PCI_IO_PROTOCOL  *PciIo
+  )
+{
+  EFI_STATUS Status;
+  UINT64     PciID;
+
+  Status = PciIo->Pci.Read (
+                        PciIo,
+                        EfiPciIoWidthUint32,
+                        PCI_VENDOR_ID_OFFSET,
+                        1,
+                        &PciID
+                        );
+
+  if (EFI_ERROR (Status)) {
+    return FALSE;
+  }
+
+  if ((PciID & 0xFFFFFFFF) != JUNO_MARVELL_YUKON_ID) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+STATIC
+VOID
+GetJunoMacAddr (
+  OUT UINT32  *MacHigh,
+  OUT UINT32  *MacLow
+  )
+{
+  // Read MAC address from IOFPGA
+  *MacHigh = MmioRead32 (ARM_JUNO_SYS_PCIGBE_H);
+  *MacLow  = MmioRead32 (ARM_JUNO_SYS_PCIGBE_L);
+
+  // Convert to Marvell MAC Address register format
+  *MacHigh = SwapBytes32 ((*MacHigh & 0xFFFF) << 16 |
+                          (*MacLow & 0xFFFF0000) >> 16);
+  *MacLow = SwapBytes32 (*MacLow) >> 16;
+}
+
+STATIC
+BOOLEAN
+IsSomehowValidToDoSomethingWith (
+  IN EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *AddrSpaceDescriptor
+  )
+{
+  return (AddrSpaceDescriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR &&
+          AddrSpaceDescriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM &&
+          !(AddrSpaceDescriptor->SpecificFlag & ACPI_SPECFLAG_PREFETCHABLE));
+}
+
+STATIC
+VOID
+WriteMacAddr (
+  IN OUT EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *AddrSpaceDescriptor
+  )
+{
+  UINT32  PciRegBase;
+  UINT32  MacHigh;
+  UINT32  MacLow;
+
+  GetJunoMacAddr (&MacHigh, &MacLow);
+
+  PciRegBase = AddrSpaceDescriptor->AddrRangeMin;
+
+  // Set software reset control register to protect from deactivation
+  // the config write state
+  MmioWrite16 (PciRegBase + R_CONTROL_STATUS, CS_RESET_CLR);
+
+  // 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);
+}
+
+STATIC
+EFI_STATUS
+GetAddressSpaceDescriptor (
+  IN OUT EFI_PCI_IO_PROTOCOL                *PciIo,
+  OUT    UINT64                             *OldPciAttributes,
+  OUT    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  **AddrSpaceDescriptor
+  )
+{
+  EFI_STATUS Status;
+  UINT64     AttrSupports;
+
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationGet,
+                    0,
+                    OldPciAttributes
+                    );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationSupported,
+                    0,
+                    &AttrSupports
+                    );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  AttrSupports &= EFI_PCI_DEVICE_ENABLE;
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationEnable,
+                    AttrSupports,
+                    NULL
+                    );
+
+  Status = PciIo->GetBarAttributes (PciIo, 0, &AttrSupports, (VOID**)AddrSpaceDescriptor);
+
+  return Status;
+}
+
+STATIC
+VOID
+RestorePciAttributes (
+  IN OUT EFI_PCI_IO_PROTOCOL  *PciIo,
+  IN     UINT64               OldPciAttributes
+  )
+{
+  PciIo->Attributes (
+           PciIo,
+           EfiPciIoAttributeOperationSet,
+           OldPciAttributes,
+           NULL
+           );
+}
+
+STATIC
+EFI_PCI_IO_PROTOCOL *
+GetOnboardYukonPciIo (
+  )
+{
+  EFI_STATUS           Status;
+  UINTN                HandleCount;
+  EFI_HANDLE           *HandleBuffer;
+  UINTN                HIndex;
+  EFI_PCI_IO_PROTOCOL  *PciIo;
+
+  Status = gBS->LocateHandleBuffer (ByProtocol,
+				    &gEfiPciIoProtocolGuid,
+				    NULL, &HandleCount, &HandleBuffer);
+
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+
+  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;
+    }
+
+    if (IsMyYukon (PciIo)) {
+      return PciIo;
+    }
+
+    // CloseProtocol?
+  }
+
+  return NULL;
+}
+
+/**
+  The function reads MAC address from Juno IOFPGA registers and writes it
+  into Marvell Yukon NIC.
+**/
+STATIC
+EFI_STATUS
+ArmJunoSetNetworkMAC ()
+{
+
+  EFI_STATUS                          Status;
+  EFI_PCI_IO_PROTOCOL                 *PciIo;
+  UINT64                              OldPciAttributes;
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *AddrSpaceDescriptor;
+
+
+  PciIo = GetOnboardYukonPciIo ();
+  if (PciIo == NULL) {
+    return EFI_NOT_FOUND;
+  }
+
+  Status = GetAddressSpaceDescriptor (
+             PciIo,
+             &OldPciAttributes,
+             &AddrSpaceDescriptor
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (IsSomehowValidToDoSomethingWith (AddrSpaceDescriptor)) {
+    WriteMacAddr (AddrSpaceDescriptor);
+  }
+
+  RestorePciAttributes (PciIo, OldPciAttributes);
+
+  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
@@ -106,6 +338,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..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.10.2



  reply	other threads:[~2016-12-13 14:00 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-13  7:24 [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address Daniil Egranov
2016-12-13 14:00 ` Leif Lindholm [this message]
2016-12-13 15:32   ` Daniil Egranov
2016-12-13 15:53     ` Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161213140041.GF17590@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox