From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 030EC82182 for ; Tue, 13 Dec 2016 06:00:48 -0800 (PST) Received: by mail-wm0-x233.google.com with SMTP id a197so110915940wmd.0 for ; Tue, 13 Dec 2016 06:00:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=MVgH/QbVZ2mLEVf4iF4qr5gBCwuBkfMEpUzi2WHjUk4=; b=D9IlWlYAh+czVDX6UkwNZn77wm6rXmKrU0zspn6Rcf0Srj9V3ebv3BMfo7gZ6sDmh1 xqbAq0GhxLLdAhmI9quXvhIbCPeymgM0ulv1cnY58T44Ykj2+c9u5FLX8b8PVeAHRgqK v3l/r31a2VEDFb4rDWgKrBEUmyyLK9Y18AJpE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=MVgH/QbVZ2mLEVf4iF4qr5gBCwuBkfMEpUzi2WHjUk4=; b=A7aNs0sFygSu8xVoPha1TNuRKvr7NMM80TSLb8qMw2GHeDL66jcNKQlu32sjeYre1P SRkKG1ZQ4Ulo+DmYLx0q+cCdg3n5Ycpbe4W10Vjq+rMcK/bbxyJKSChAcNdjjUgLPOmx ALhv6sLwBaMwK2CaUdI2r0J47PDi6mrKWs5a+H9MHR6G0uIGz1yP9m3dGxszufaMp3kx 724Wffj1I4rF4XZFbu360Cq1iSeNpl0kLfNgib2NUb57jdsG4uXYHIlgSjfPqlkBG9YM US8cI6XbnTaF294MYLv1Spz+ZrZrazhLmOoph70LM7NTKuePVlf20zDSNuKgdjXGAv7a 9QIg== X-Gm-Message-State: AKaTC019i0wuFVv1YwLpbYOX5L4OkCygIXdiIOu0f+aBEEauKrmUzGSbGclQ3T/xyRZpApV+ X-Received: by 10.28.16.65 with SMTP id 62mr3052764wmq.81.1481637646066; Tue, 13 Dec 2016 06:00:46 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id n5sm2781153wmf.0.2016.12.13.06.00.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 13 Dec 2016 06:00:45 -0800 (PST) Date: Tue, 13 Dec 2016 14:00:42 +0000 From: Leif Lindholm To: Daniil Egranov Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org Message-ID: <20161213140041.GF17590@bivouac.eciton.net> References: <1481613864-114733-1-git-send-email-daniil.egranov@arm.com> MIME-Version: 1.0 In-Reply-To: <1481613864-114733-1-git-send-email-daniil.egranov@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Dec 2016 14:00:48 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 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 Signed-off-by: Leif Lindholm --- .../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 +#include #include +#include #include #include @@ -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 +#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