From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0628.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe1f::628]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DB39C821B8 for ; Tue, 13 Dec 2016 07:32:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=u6KO1WIrB1NXcWXmMyfuYj2ZeDOM/YfVUdYghuayY3c=; b=ozb+GkdQ5TgQBn2/fLIjru/hrbNwoB5Sv4Tsm1vFZaA2mY2eVd4u9V/iZXGIghqEB+wqIO+yqWT2/I/Lsj3WeDpz8jtwjrnjRaK2X3OZ/Sax90r3Key9H1Whg+GrA/xMFtRuy/dGak0kP/rbZZgsodl5r3WHnQMtgpUShFw0eAg= Received: from DB6PR0801MB2054.eurprd08.prod.outlook.com (10.168.86.135) by DB6PR0801MB2054.eurprd08.prod.outlook.com (10.168.86.135) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.771.8; Tue, 13 Dec 2016 15:32:49 +0000 Received: from DB6PR0801MB2054.eurprd08.prod.outlook.com ([10.168.86.135]) by DB6PR0801MB2054.eurprd08.prod.outlook.com ([10.168.86.135]) with mapi id 15.01.0771.011; Tue, 13 Dec 2016 15:32:49 +0000 From: Daniil Egranov To: Leif Lindholm CC: "edk2-devel@lists.01.org" , "ryan.harkin@linaro.org" Thread-Topic: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set Marvell Yukon MAC address Thread-Index: AQHSVUlPGC0n6qHVxUyig7/oTxoV4aEF83Lw Date: Tue, 13 Dec 2016 15:32:49 +0000 Message-ID: References: <1481613864-114733-1-git-send-email-daniil.egranov@arm.com> <20161213140041.GF17590@bivouac.eciton.net> In-Reply-To: <20161213140041.GF17590@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Daniil.Egranov@arm.com; x-originating-ip: [73.136.41.219] x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr x-ms-office365-filtering-correlation-id: a06bdd1b-f6f3-4f7a-1266-08d4236d4bb9 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001); SRVR:DB6PR0801MB2054; x-microsoft-exchange-diagnostics: 1; DB6PR0801MB2054; 7:9h1Ttq7a3x2A3aR29webxkHKOgssrDpDmpWPr8aWIpjMvTcyRAOK6J5JiXGbwy4Rqoax6JjzpGr+qjMsuHrcQHXDj84lMcK/i4HOr/tAVfJYXzfpu3JHr+qbJkE3RjV2Cxy85ZFONgOFr4qPBJJwIRlaBSmX5ODIG4KwkjH1avbXGK5Sd9CuSlAmZ4pzbpjuNSoS/UM4SVN/klLZCqw4bxeqDkY3gFtNW/0rPBWLOfKcWtJ9iKPLtbyHtKXcOxNsSZ1+sbpfyajE1gKzyuZR0vACzCtNmOkqgdHLVJK4wRP0xOcBpKb+gHcQGq/J+oNw97H4XtnXGDGNemATCM2R9ORle3FP684yJ9SnDhxZTqKbAfs39p9Qs81IxT95lhfy7zyUKt8ho7nRnQ9PY3p6/srFTjVT3Ms4evXdbgIOrRxMzGic640a7+Mv8K8sMiRfZVipdj3xlapgMU249zwSNQ== x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(162533806227266); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026)(6041248)(20161123555025)(20161123560025)(20161123564025)(20161123562025)(6072148); SRVR:DB6PR0801MB2054; BCL:0; PCL:0; RULEID:; SRVR:DB6PR0801MB2054; x-forefront-prvs: 01559F388D x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(39450400003)(39410400002)(39850400002)(39840400002)(39860400002)(377454003)(40434004)(189002)(24454002)(199003)(13464003)(54534003)(4326007)(54356999)(50986999)(6116002)(102836003)(68736007)(3846002)(2906002)(5890100001)(33656002)(105586002)(9686002)(106116001)(76576001)(106356001)(122556002)(8936002)(81166006)(81156014)(2900100001)(97736004)(86362001)(66066001)(6436002)(6506006)(305945005)(110136003)(229853002)(92566002)(8676002)(7736002)(77096006)(101416001)(7696004)(76176999)(2950100002)(3660700001)(6916009)(38730400001)(3280700002)(189998001)(74316002)(5660300001); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR0801MB2054; H:DB6PR0801MB2054.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Dec 2016 15:32:49.3278 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB2054 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 15:32:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Leif, I understood your comments but I do not see a good reason to split a pretty= small function on even smaller functions which will not be used by any oth= er code. The code has comments or function calls explain themselves in the = name so it should not be a problem to understand the code. As I know, the = edk2 does not dictate a particular code structure or implementation way and= it's up to a developer how to organize and implement the code. If you insi= st to do such changes I can do it, but I am not sure if it's a reasonable r= equest. I agree, freeing allocated resources should be fixed. Thanks, Daniil -----Original Message----- From: Leif Lindholm [mailto:leif.lindholm@linaro.org] Sent: Tuesday, December 13, 2016 8:01 AM To: Daniil Egranov Cc: edk2-devel@lists.01.org; ryan.harkin@linaro.org Subject: Re: [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Set M= arvell Yukon MAC address 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-jugg= ling, and extremely difficult to review. I did ask for some helper functions to be created in my v1 review, but perh= aps not clearly enough. So below is an example of how this could be reworke= d to be something I could review and feel reasonably confident about in 5 m= inutes. 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 picki= er with this for the ARM Ltd. platforms, because we know from experience th= at 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 progra= mming of MAC address, and creating new EfiPciIoProtocol instances on each i= teration (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/Ar= mPlatformPkg/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 mPciRootCo= mplexDevicePath =3D { EFI_EVENT mAcpiRegistration =3D NULL; + +STATIC +BOOLEAN +IsMyYukon ( + IN EFI_PCI_IO_PROTOCOL *PciIo + ) +{ + EFI_STATUS Status; + UINT64 PciID; + + Status =3D PciIo->Pci.Read ( + PciIo, + EfiPciIoWidthUint32, + PCI_VENDOR_ID_OFFSET, + 1, + &PciID + ); + + if (EFI_ERROR (Status)) { + return FALSE; + } + + if ((PciID & 0xFFFFFFFF) !=3D JUNO_MARVELL_YUKON_ID) { + return FALSE; + } + + return TRUE; +} + +STATIC +VOID +GetJunoMacAddr ( + OUT UINT32 *MacHigh, + OUT UINT32 *MacLow + ) +{ + // Read MAC address from IOFPGA + *MacHigh =3D MmioRead32 (ARM_JUNO_SYS_PCIGBE_H); + *MacLow =3D MmioRead32 (ARM_JUNO_SYS_PCIGBE_L); + + // Convert to Marvell MAC Address register format + *MacHigh =3D SwapBytes32 ((*MacHigh & 0xFFFF) << 16 | + (*MacLow & 0xFFFF0000) >> 16); + *MacLow =3D SwapBytes32 (*MacLow) >> 16; } + +STATIC +BOOLEAN +IsSomehowValidToDoSomethingWith ( + IN EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddrSpaceDescriptor + ) +{ + return (AddrSpaceDescriptor->Desc =3D=3D ACPI_ADDRESS_SPACE_DESCRIPTOR &= & + AddrSpaceDescriptor->ResType =3D=3D 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 =3D 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 =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationGet, + 0, + OldPciAttributes + ); + + if (EFI_ERROR (Status)) { + return Status; + } + + Status =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSupported, + 0, + &AttrSupports + ); + + if (EFI_ERROR (Status)) { + return Status; + } + + AttrSupports &=3D EFI_PCI_DEVICE_ENABLE; Status =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationEnable, + AttrSupports, + NULL + ); + + Status =3D 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 =3D gBS->LocateHandleBuffer (ByProtocol, + &gEfiPciIoProtocolGuid, + NULL, &HandleCount, &HandleBuffer); + + if (EFI_ERROR (Status)) { + return NULL; + } + + for (HIndex =3D 0; HIndex < HandleCount; ++HIndex) { + Status =3D 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 =3D GetOnboardYukonPciIo (); + if (PciIo =3D=3D NULL) { + return EFI_NOT_FOUND; + } + + Status =3D 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 =3D gBS->ConnectController (Handle, NULL, PciRootComplexDevicePat= h, FALSE); ASSERT_EFI_ERROR (Status); + + Status =3D ArmJunoSetNetworkMAC(); + ASSERT_EFI_ERROR (Status); } STATIC diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxeInterna= l.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 I= D */ +#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 Registe= r */ +#define R_MAC 0x0100 /* MAC Address */ +#define R_MAC_MAINT 0x0110 /* MAC Address Maintenanc= e */ +#define R_MAC_LOW 0x04 /* MAC Address Low Regist= er Offset */ +#define R_TST_CTRL_1 0x0158 /* Test Control Register = 1 */ + + EFI_STATUS PciEmulationEntryPoint ( VOID -- 2.10.2 IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you.