* [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
@ 2016-12-13 7:24 Daniil Egranov
2016-12-13 14:00 ` Leif Lindholm
0 siblings, 1 reply; 4+ messages in thread
From: Daniil Egranov @ 2016-12-13 7:24 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, ryan.harkin, Daniil Egranov
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.
.../ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 147 +++++++++++++++++++++
.../Drivers/ArmJunoDxe/ArmJunoDxeInternal.h | 13 ++
2 files changed, 160 insertions(+)
diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index b97f044..a36c6ff 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,148 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
EFI_EVENT mAcpiRegistration = NULL;
/**
+ 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;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddrSpaceDescriptor;
+
+ 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 = PciIo->Pci.Read (
+ PciIo,
+ EfiPciIoWidthUint32,
+ PCI_VENDOR_ID_OFFSET,
+ 1,
+ &PciID
+ );
+
+ if (EFI_ERROR (Status)) {
+ continue;
+ }
+
+ if ((PciID & 0xFFFFFFFF) != JUNO_MARVELL_YUKON_ID) {
+ continue;
+ }
+
+ // Read MAC address from IOFPGA
+ MacHigh= MmioRead32 (ARM_JUNO_SYS_PCIGBE_H);
+ MacLow = MmioRead32 (ARM_JUNO_SYS_PCIGBE_L);
+
+ 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**)&PciBarAttributes);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ AddrSpaceDescriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)PciBarAttributes;
+
+ if (AddrSpaceDescriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR &&
+ AddrSpaceDescriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM &&
+ !(AddrSpaceDescriptor->SpecificFlag & ACPI_SPECFLAG_PREFETCHABLE)) {
+
+ PciRegBase = AddrSpaceDescriptor->AddrRangeMin;
+
+ // 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);
+
+ Status = EFI_SUCCESS;
+ }
+
+ PciIo->Attributes (
+ PciIo,
+ 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
the entry point of the driver.
@@ -106,6 +250,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.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
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
2016-12-13 15:32 ` Daniil Egranov
0 siblings, 1 reply; 4+ messages in thread
From: Leif Lindholm @ 2016-12-13 14:00 UTC (permalink / raw)
To: Daniil Egranov; +Cc: edk2-devel, ryan.harkin
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
2016-12-13 14:00 ` Leif Lindholm
@ 2016-12-13 15:32 ` Daniil Egranov
2016-12-13 15:53 ` Leif Lindholm
0 siblings, 1 reply; 4+ messages in thread
From: Daniil Egranov @ 2016-12-13 15:32 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org
Hi Leif,
I understood your comments but I do not see a good reason to split a pretty small function on even smaller functions which will not be used by any other code. The code has comments or function calls explain themselves in the name so it should not be a problem to understand the code. As I know, the edk2 does not dictate a particular code structure or implementation way and it's up to a developer how to organize and implement the code. If you insist to do such changes I can do it, but I am not sure if it's a reasonable request.
I agree, freeing allocated resources should be fixed.
Thanks,
Daniil
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Tuesday, December 13, 2016 8:01 AM
To: Daniil Egranov
Cc: edk2-devel@lists.01.org; ryan.harkin@linaro.org
Subject: Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
2016-12-13 15:32 ` Daniil Egranov
@ 2016-12-13 15:53 ` Leif Lindholm
0 siblings, 0 replies; 4+ messages in thread
From: Leif Lindholm @ 2016-12-13 15:53 UTC (permalink / raw)
To: Daniil Egranov; +Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org
On Tue, Dec 13, 2016 at 03:32:49PM +0000, Daniil Egranov wrote:
> I understood your comments but I do not see a good reason to split a
> pretty small function on even smaller functions which will not be
> used by any other code.
How do we find such reusability unless we start looking for it?
For squeezing things together efficiently, we have the compiler.
The source code is there to be readable.
> The code has comments or function calls
> explain themselves in the name so it should not be a problem to
> understand the code.
This isn't a tickbox "you should be able to figure it out" exercise -
it is about reducing the amount of time and effort that needs to be
spent on reviewing and reduce the amount of bugs that go into the code
in the first place.
> As I know, the edk2 does not dictate a
> particular code structure or implementation way and it's up to a
> developer how to organize and implement the code.
No, it's up to a maintainer to decide what code structure or
implementation is OK in the bit they are looking after and which is
not. That is their role, because it is their responsibility to look
after and maintain it.
And I agree that we should make a lot of this better codified globally
across the project.
> If you insist to do such changes I can do it, but I am not sure if
> it's a reasonable request.
Having found two bugs that would have been very unlikely to occur had
the code been broken down into more self-contained chunks, and taking
the time to provide an alternative version as my feedback had been
sliently ignored, I think my reqest is quite reasonable.
Regards,
Leif
> I agree, freeing allocated resources should be fixed.
>
> Thanks,
> Daniil
>
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, December 13, 2016 8:01 AM
> To: Daniil Egranov
> Cc: edk2-devel@lists.01.org; ryan.harkin@linaro.org
> Subject: Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
>
> 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
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-13 15:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2016-12-13 15:32 ` Daniil Egranov
2016-12-13 15:53 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox