From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5E80221DFA7B0 for ; Thu, 30 Mar 2017 11:28:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF8DE7EBA1; Thu, 30 Mar 2017 18:28:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BF8DE7EBA1 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com BF8DE7EBA1 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-143.phx2.redhat.com [10.3.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTP id 76160179CD; Thu, 30 Mar 2017 18:28:57 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org, leif.lindholm@linaro.org References: <20170329134833.12956-1-ard.biesheuvel@linaro.org> Cc: ryan.harkin@linaro.org From: Laszlo Ersek Message-ID: <54d3f0bc-47e5-760d-8222-ad5325b01722@redhat.com> Date: Thu, 30 Mar 2017 20:28:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170329134833.12956-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 30 Mar 2017 18:28:58 +0000 (UTC) Subject: Re: [PATCH 0/6] Embedded|ArmPlatformPkg: spring cleaning + DtPlatformDxe switch X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Mar 2017 18:28:59 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/29/17 15:48, Ard Biesheuvel wrote: > This implements the upstream part of switching VExpress TC2 and the AArch64 > FVP Foundation/Base models to the new DtPlatformDxe driver, which is much > simpler and only allows ACPI or DT to be enabled but never both. > > Patches #1 and #2 tweak the new DtPlatformDxe so it can choose from several > builtin DTBs depending on the actual platform detected at runtime. > > Patches #3, #4 and #5 are basically preparatory cleanup that allows patch #6 > to radically change ArmFvpDxe without affecting other users. > > Patch #6 removes all the handling of FDT device paths, string PCDs that > have to be initialized to 128 spaces and other awkwardness, and simply > sets the default DTB file section index based on the detected platform. > > Ard Biesheuvel (6): > EmbeddedPkg/DtPlatformDxe: allow multiple entries in DTB FV file > EmbeddedPkg/DtPlatformDxe: declare symbolic name for FILE_GUID > ArmPlatformPkg/ArmShellCmdRunAxf: remove BdsLib dependency > ArmPlatformPkg/ArmVExpressDxe: remove ARM support > ArmPlatformPkg/ArmVExpressDxe: remove unused cruft from ArmHwDxe > ArmPlatformPkg/ArmVExpressDxe: simply FDT handling in ArmFvpDxe > > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/AArch64/ArmFvpDxeAArch64.c | 60 +++------ > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c | 84 ------------ > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.c | 134 ++------------------ > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf | 42 ++---- > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.c | 43 +------ > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmHwDxe.inf | 3 - > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c | 48 ------- > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressInternal.h | 52 +------- > ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec | 28 ---- > ArmPlatformPkg/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf | 1 - > ArmPlatformPkg/Library/ArmShellCmdRunAxf/RunAxf.c | 58 ++++++++- > EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c | 5 +- > EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf | 6 +- > EmbeddedPkg/EmbeddedPkg.dec | 6 + > 14 files changed, 108 insertions(+), 462 deletions(-) > delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/Arm/ArmFvpDxeArm.c > delete mode 100644 ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmVExpressCommon.c > What do you think of the following proposal instead (I have no strong feelings about this, just picking your mind): (1) In commit 779cc439e881 ("EmbeddedPkg: add DT platform driver to select between DT and ACPI", 2017-03-27), a driver was added that, based on an HII checkbox, - either produces the platform-has-ACPI NULL protocol, - or immediately installs the DTB, found in any FV section. (Glossing over the details here, such as, if there is no DTB embedded in the firmware image, we always go with ACPI etc.) I reviewed that patch. I slightly disliked that in the DT case, we immediately installed the DT as a sysconfig table, but I figured, given that this driver actually "owned" the DTB, it was okay. Therefore, I didn't suggest producing the platform-has-DeviceTree NULL protocol, and then acting upon that protocol within the exact same driver (i.e., to install the DTB as a sysconfig table in a protocol notify). (2) I feel that, with this set, the DTB ownership is changing. I think the following restructuring would be an improvement: - DtPlatformDxe should only concern itself with translating the HII checkbox to the appropriate NULL protocol, namely platform-has-ACPI versus platform-has-DeviceTree. A corresponding rename for the driver might be in order too. - The owner of the (multiple possible) DTBs is now ArmVExpressDxe (or any similar driver included in any given platform DSC). IMO, this is the driver to look up the DTB within the firmware image, based on the platform type determination that it already performs. Then, ArmVExpressDxe should install the selected DTB as a sysconfig table, specifically in a protocol notify callback for platform-has-DeviceTree. (3) Here's an excerpt from the message of commit 65a69b214840, "EmbeddedPkg: introduce EDKII Platform Has Device Tree GUID", 2017-03-17: > In the DXE phase, the protocol is meant to be consumed by the platform > driver that > - owns the Device Tree description of the hardware, and > - is responsible for installing it as a system configuration table. I think that the above description matches the current situation 1:1, and that the proposed driver structuring would keep the responsibilities better separated. Plus, you could eliminate the butt-ugly BEFORE depex :) The "EmbeddedPkg/Drivers/DtPlatformDxe" driver is not yet used in any upstream edk2 platform DSC, so I think it's the appropriate time to set the responsibilities right. I haven't looked at your series [Linaro-uefi] [PATCH 0/6] EDK2 spring cleaning -- OpenPlatformPkg edition https://lists.linaro.org/pipermail/linaro-uefi/2017-March/004196.html yet, but there you call both patch sets inter-dependent, so I guess this is the one we should be discussing first. (4) I'm missing a whole lot of details about ArmVExpressDxe, so the above might not be feasible or desirable. For example, while it seems to identify and expose the DTB to install -- very elaborately, as you say --, ultimately it only sets PcdFdtDevicePaths. The PCD is then consumed by "EmbeddedPkg/Drivers/FdtPlatformDxe". As far as I can see, this driver reads the DTB from the EFI system partition, as directed by the PCD? In this patch set, you don't seem to touch FdtPlatformDxe, so I think that you are replacing all of the FdtPlatformDxe functionality by embedding the DTBs in the FV image. In that case, my proposal above should not conflict with (or require updates for) FdtPlatformDxe either. (5) Anyway, I just wanted to float the idea. What do you think of it? Thanks Laszlo