From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22a.google.com (mail-io0-x22a.google.com [IPv6:2607:f8b0:4001:c06::22a]) (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 1839C21A0483B for ; Fri, 31 Mar 2017 02:22:32 -0700 (PDT) Received: by mail-io0-x22a.google.com with SMTP id f84so35837493ioj.0 for ; Fri, 31 Mar 2017 02:22:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=gpltWIsIwTFpjawDcgTQqNHlQPtxlltaZWc5ZKdYkN8=; b=gTy0SXCSCXDCzi9ksRl5jLptXQ+MSD9Q53KhJ7INECvOIMSvWKMnk0y7H6CengWH6y UgS5ty3VIcTCxffTYqcwm9cmTRTldz4TWuzcK6FIBPWrYBViHGEk0z8qqDQacN9Kyxb0 9jKW1vAUCd5xMf4KjLyIg/SrM5BF0CPIWSCwI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=gpltWIsIwTFpjawDcgTQqNHlQPtxlltaZWc5ZKdYkN8=; b=lNrEbLprBR+7jUcuI9FLup8HkhZsErO57jZavid9VO/DzgM3Uow+4OYy4ck5FA/q9+ 1Z62JQ1drANB5pFU7b8HWcmTALzpNt0szbAUj2Iya3bgqzSM4IJ3QYF2lVVnYdncugrG Hh7Ex0qbLbxk51zL0ESDDIFTgHWqPqIISpjq/+vFIlJW+hina68xmPYNYliHIUXmQvcO FkvQ0pfCvrLVhpAo0veYqkdxUFdGUm7abTIgXMoT+a1JRViwIX2phH1ShLqWJYe9XYHG 8n6hq9tevPDRTIJxWIMlDYFKNHt9B3oUIQj9kDnX2dBpRlNbE2nQm8svc62lMBbT01iD HxQA== X-Gm-Message-State: AFeK/H2RuHUmW43yaLlat10C2ZjLzMDEp/DC22GslEeiVBRb4CeFkwg/hognVU0AhoNKJRfg/gsxUrxgSqbxPzfz X-Received: by 10.107.132.155 with SMTP id o27mr1915383ioi.138.1490952151102; Fri, 31 Mar 2017 02:22:31 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Fri, 31 Mar 2017 02:22:30 -0700 (PDT) In-Reply-To: <54d3f0bc-47e5-760d-8222-ad5325b01722@redhat.com> References: <20170329134833.12956-1-ard.biesheuvel@linaro.org> <54d3f0bc-47e5-760d-8222-ad5325b01722@redhat.com> From: Ard Biesheuvel Date: Fri, 31 Mar 2017 10:22:30 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Ryan Harkin 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: Fri, 31 Mar 2017 09:22:32 -0000 Content-Type: text/plain; charset=UTF-8 On 30 March 2017 at 19:28, Laszlo Ersek wrote: [...] > > 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. > Well, what I would like to avoid is having awareness of DT in more places than necessary. The nice thing of having a DtPlatformDxe driver that takes care of it all is that removing the module (and the PlatformHasAcpi NULL library class resolution) makes a platform completely ACPI only > (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? > I want to get rid of FdtPlatformDxe. It uses things like semihosting device paths to pull .dtb files that are known by name from various places. While this is nice for debugging in theory, nobody actually uses it. On it does not belong on a production system at all. Also, for a laugh, please check out 7aec2926b926ad90d09fb026af0ee04c4c831237, specifically the hunk that adds gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths. I agree about the butt-uglines of BEFORE depexes, so what I would prefer is an alternative way to allow DtPlatformDxe to 'own' DT installation entirely. To enforce the ordering, I could also update DtPlatformDxe to load the correct FV section at EndOfDxe. Would that be acceptable to you?