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.egra...@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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
