From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x235.google.com (mail-wm0-x235.google.com [IPv6:2a00:1450:400c:c09::235]) (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 9870D8192F for ; Wed, 4 Jan 2017 07:12:43 -0800 (PST) Received: by mail-wm0-x235.google.com with SMTP id t79so458878906wmt.0 for ; Wed, 04 Jan 2017 07:12:43 -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=ra9EEzFksf3A8fuTULcaNBGI/F3tWE6sUMCkkFZ+ARY=; b=V0pHKB6Yr9/Mi6NVFIkKgvIByj6y8tDc4rcn3TRoyKn01IuldvUeZyL/EgGJ846Id9 /wpItbkCydzsTLNYZ5gIM92IpRlTFg1jO4PlW/wRllmoEGDqg4KulPjJBBEC7uIotb07 Vgo0VU4Fz77CvsOAMFRD0VLIA5vE4yNZTJwvE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ra9EEzFksf3A8fuTULcaNBGI/F3tWE6sUMCkkFZ+ARY=; b=Ss3+EPVfRL+Jg5biE1hVQv7CsJz+L7asQiR7aFoP+cWXCytlNUF4lDzOjYXULlA1LH cZ1X1Kt9DjfWchiQNjaViA9PZducN2fyIQ+gAFW157M/H/JfzRxX272MMnVOc7ir+kil 5sualTztyGjG9JF4LMc8igjbNOTC87EA2+8JfN01AjLYveqJTVx9XxoM//SB7pYmR72P aGEIRgCURnZBVAEOgMn9V3Ez2LcYT3KXEEab9W2K0Qm1w7I9LVMwnlbSuriX2+U8kpzt pjUZ7Y+Qo0WfEg+iAGrGAYMZV2p38bvSK50pzj3I7s8ZgXYL/udojLQ6i+ZglnazTFPR 3H0A== X-Gm-Message-State: AIkVDXJEVZT0IGqdmuCPA+WfYqRBjQuqS4CnQArUCauYX1wQzgBkoJ7NM8QV2e1wkIS3LjXI X-Received: by 10.28.187.67 with SMTP id l64mr56400553wmf.114.1483542761579; Wed, 04 Jan 2017 07:12:41 -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 i10sm99002874wjd.15.2017.01.04.07.12.40 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Jan 2017 07:12:40 -0800 (PST) Date: Wed, 4 Jan 2017 15:12:39 +0000 From: Leif Lindholm To: Daniil Egranov Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org Message-ID: <20170104151238.GI16872@bivouac.eciton.net> References: <1481680569-89827-1-git-send-email-daniil.egranov@arm.com> MIME-Version: 1.0 In-Reply-To: <1481680569-89827-1-git-send-email-daniil.egranov@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v3] 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: Wed, 04 Jan 2017 15:12:44 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 > > +#include > #include > +#include > #include > > #include > @@ -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 > > +#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 >