From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22c.google.com (mail-wm0-x22c.google.com [IPv6:2a00:1450:400c:c09::22c]) (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 AE1A281EE6 for ; Thu, 8 Dec 2016 08:48:16 -0800 (PST) Received: by mail-wm0-x22c.google.com with SMTP id a197so225448512wmd.0 for ; Thu, 08 Dec 2016 08:48:16 -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=qefuhVUuY4jQ37vLlxLz/rbwDaI5gUnbgKt55g450H4=; b=XXU4klf1DtRZAponqlod3VBhDc4eVibL27vNnF2In5hC1Uw9HTCZV9h7kpg1bT2HaZ GGW0hMUUPrVNZD4qqY/Khi1f7PXwoLElrDhNweqe0np5kqIdFKdax90GxSE12GNRqPkv RQuhZl878XbkLy8mPw17O+nAu9YVpICKHfZrQ= 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=qefuhVUuY4jQ37vLlxLz/rbwDaI5gUnbgKt55g450H4=; b=ga+9hj9n+Fu38b+nqNp9Gn5GOrTGk092CECkZeEQ/74zz7be8pzpqumZkiYIhHhX3h pPnUQGiB0+BN93uqbjKEmsT0J/vO4O05cXKpDgx6RazmJTl7MyawqMEqo6DoiBOMoQHB pp7Ta8KmsFNhdDsjCnnplNV2WP571enAJV5mCiQ+ZceNNE/vH/xcX/J1LS33sRlpMBBu efgBGrDypmZkkNkhAlWYwa5Wj9rG641t24acBAw43AToXYPLQOZEaUNAS+GDQGf3XIX7 nAaMgsYv+tMgVqm4WfAv+ZOEqLrk0y0QG6l7WOtjTNNvbXCewxoNYiG8L1lG7hlDgu03 o+SA== X-Gm-Message-State: AKaTC03sgbG0W2BNHBjyy21oNVed/d9n2p238l5cErCVOhWNxFf/hsgta7LROSpeMeiDIvw3 X-Received: by 10.28.54.214 with SMTP id y83mr2977214wmh.45.1481215695045; Thu, 08 Dec 2016 08:48:15 -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 e5sm16168495wma.12.2016.12.08.08.48.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Dec 2016 08:48:14 -0800 (PST) Date: Thu, 8 Dec 2016 16:48:12 +0000 From: Leif Lindholm To: Daniil Egranov Cc: edk2-devel@lists.01.org, alan@softiron.co.uk Message-ID: <20161208164812.GX17590@bivouac.eciton.net> References: <1475718171-18204-1-git-send-email-daniil.egranov@arm.com> MIME-Version: 1.0 In-Reply-To: <1475718171-18204-1-git-send-email-daniil.egranov@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH] 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: Thu, 08 Dec 2016 16:48:17 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > .../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 > #include > +#include > +#include Could you insert these alphabetically sorted please? > > #include > #include > @@ -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 > > +#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 >