* [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
@ 2016-12-14 1:56 Daniil Egranov
2017-01-04 15:12 ` Leif Lindholm
0 siblings, 1 reply; 2+ messages in thread
From: Daniil Egranov @ 2016-12-14 1:56 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, ryan.harkin, Daniil Egranov
Changed code structure per Leif Lindholm's request
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().
+ 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 (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN UINT64 PciAttr,
+ 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().
+
+ 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 (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ OUT UINT32 *PciRegBase,
+ OUT UINT64 *OldPciAttr,
+ OUT EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR **AcpiResDescriptor
+ )
+{
+ UINT64 AttrSupports;
+ EFI_STATUS Status;
+
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationGet,
+ 0,
+ OldPciAttr);
+ 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);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = PciIo->GetBarAttributes (PciIo, 0, &AttrSupports, (VOID**)AcpiResDescriptor);
+ 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)) {
+ *PciRegBase = (*AcpiResDescriptor)->AddrRangeMin;
+ } else {
+ FreePciResources (PciIo, *OldPciAttr, *AcpiResDescriptor);
+ Status = 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address
2016-12-14 1:56 [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address Daniil Egranov
@ 2017-01-04 15:12 ` Leif Lindholm
0 siblings, 0 replies; 2+ messages in thread
From: Leif Lindholm @ 2017-01-04 15:12 UTC (permalink / raw)
To: Daniil Egranov; +Cc: edk2-devel, ryan.harkin
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-01-04 15:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-14 1:56 [PATCH v3] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address Daniil Egranov
2017-01-04 15:12 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox