From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A25FFC433F5 for ; Wed, 29 Sep 2021 12:59:19 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EB2F761211 for ; Wed, 29 Sep 2021 12:59:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org EB2F761211 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C173A82DC1; Wed, 29 Sep 2021 14:59:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 6436B83238; Wed, 29 Sep 2021 14:59:15 +0200 (CEST) Received: from sibelius.xs4all.nl (sibelius.xs4all.nl [83.163.83.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 274D78048A for ; Wed, 29 Sep 2021 14:59:12 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=mark.kettenis@xs4all.nl Received: from localhost (bloch.sibelius.xs4all.nl [local]) by bloch.sibelius.xs4all.nl (OpenSMTPD) with ESMTPA id 93ab437a; Wed, 29 Sep 2021 14:59:10 +0200 (CEST) Date: Wed, 29 Sep 2021 14:59:10 +0200 (CEST) From: Mark Kettenis To: Ilias Apalodimas Cc: zong.li@sifive.com, trini@konsulko.com, bharat.gooty@broadcom.com, rayagonda.kokatanur@broadcom.com, rick@andestech.com, ycliang@andestech.com, fitzsim@fitzsim.org, sjg@chromium.org, bmeng.cn@gmail.com, marek.behun@nic.cz, green.wan@sifive.com, seanga2@gmail.com, lukas@auer.io, brad.kim@semifive.com, xypron.glpk@gmx.de, david.abdurachmanov@sifive.com, dimitri.ledkov@canonical.com, u-boot@lists.denx.de In-Reply-To: (message from Ilias Apalodimas on Wed, 29 Sep 2021 15:55:48 +0300) Subject: Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards References: <20210927064751.78591-1-ilias.apalodimas@linaro.org> Message-ID: <5614943fc1ec0ac8@bloch.sibelius.xs4all.nl> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean > Date: Wed, 29 Sep 2021 15:55:48 +0300 > From: Ilias Apalodimas > > > > > > > - if (IS_ENABLED(CONFIG_OF_SEPARATE)) { > > > > > > - if (gd->arch.firmware_fdt_addr) > > > > > > - return (ulong *)gd->arch.firmware_fdt_addr; > > > > > > - else > > > > > > - return (ulong *)&_end; > > > > > > - } > > > > > > + if (gd->arch.firmware_fdt_addr) > > > > > > + return (void *)gd->arch.firmware_fdt_addr; > > > > > > + else > > > > > > + return (void *)&_end; > > > > > > } > > > > > > > > > > I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure > > > > > whether we should distinguish the value of a1 register which is > > > > > meaningless. It means that if we don't expect the device tree to be > > > > > passed by prior stage, then the a1 register might be a trash value at > > > > > the beginning, so it would still return the arch.firmware_fdt_addr > > > > > here, rather than _end. > > > > > > > > I thought about it as well. Those boards were configured up to now with > > > > 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? > > > > IOW the device tree was passed as part of U-Boot, which would mean a1 would > > > > have had thrash as well. Maybe a1 always has a valid DT on those boards > > > > so we never noticed? > > > > > > > > > > > > > And do you think that we should enable the > > > > > CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me > > > > > that we actually pass the device tree by prior stage (i.e. OpenSBI). > > > > > > > > Yes in that case what you request makes sense for unmatched/unleashed. > > > > Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return > > > > _end (instead of the current check). > > > > If that sounds good to you I'll send a v2 > > > > > > Looking a bit more at it... > > > Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. > > > By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to > > > OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register > > > reading works for that config. > > > > > > In that case the pre-existing code is 'wrong' as well, since the DTB is > > > not at _end, but the bogus path is never taken... > > > (check the __weak board_fdt_blob_setup for details). > > > > > > > If I remember correctly, the SPL would calculate the size of u-boot > > proper, and then put the DTB at the end of u-boot proper, so the DTB > > would fortuitously be put at the _end location. > > I haven't yet seen the creation part, but looking into the default > board_fdt_blob_setup() the location seems to vary depending on > CONFIG_SPL_BUILD. If that's selected (which is the case for those boards), > then it depends on yet another SPL config for a separate .bsdd section. > > I don't have a board to verify my suspicion but I think reading the DTB > without looking into a1 is broken for these boards. > > > > > > So I think I'll send a v2, keeping the config as-is and fixing the return > > > address of the DTB in case OF_BOARD is ever selected. > > > > > > > Yes, it seems to me that we could use a config to separate the case > > between the prior stage and the _end. > > Untangling OF_SEPARATE and OF_BOARD is part of a bigger revamp I wanted to > do on the handover of a device tree from previous bootloaders, since we do > have similar 'problems' in Arm and TF-A. But in principle OF_SEPARATE > shouldn't have per board code to overwrite it. OF_BOARD should be used for > that. OF_SEPARATE should merely mean "The dtb is concatenated to my U-Boot > binary. > > Right now RISC-V uses OF_SEPARATE reads the DTB on SPL and then goes back > to using the a1 register for U-Boot proper. We could instead read the > U-Boot concatenated DTB always in that case. OF_BOARD would then be used in > case OpenSBI is compiled with a *different* DTB and you'd want to use that. > Any idea if OpenSBI performs fixups before handing over the dtb in a1? It does. One of the things it does is add a reserved memory entry for itself. > Unfortunately I don't have a board to test apart from QEMU. Let me respin > this, with a potential fix I have in mind and we can discuss further. > > > Just note that, there is a patch > > on the fly, it modifies the same snippet of code, you might need to > > update your code based on top of it. > > https://lists.denx.de/pipermail/u-boot/2021-September/460378.html > > I'll reply to that and see if the _end is indeed a problem. > > Thanks > /Ilias >