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 48B14C433FE for ; Tue, 9 Nov 2021 17:02:44 +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 5CCE861152 for ; Tue, 9 Nov 2021 17:02:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 5CCE861152 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com 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 1146683934; Tue, 9 Nov 2021 18:02:41 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com 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 B9E048393F; Tue, 9 Nov 2021 18:02:38 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id B9290836B0 for ; Tue, 9 Nov 2021 18:02:34 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=Peter.Hoyes@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A121FED1; Tue, 9 Nov 2021 09:02:33 -0800 (PST) Received: from [10.57.80.250] (unknown [10.57.80.250]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0B3EE3F800; Tue, 9 Nov 2021 09:02:32 -0800 (PST) Subject: Re: [PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64 To: Andre Przywara Cc: u-boot@lists.denx.de, diego.sueiro@arm.com References: <20211104165619.2684533-1-peter.hoyes@arm.com> <20211104165619.2684533-5-peter.hoyes@arm.com> <20211109150652.7d319d06@donnerap.cambridge.arm.com> From: Peter Hoyes Message-ID: <8bfa50b6-c15f-0f5b-30c0-ea8445214792@arm.com> Date: Tue, 9 Nov 2021 17:02:25 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20211109150652.7d319d06@donnerap.cambridge.arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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 Hi, On 09/11/2021 15:06, Andre Przywara wrote: > On Thu, 4 Nov 2021 16:56:18 +0000 > Peter Hoyes wrote: > > Hi, > >> From: Peter Hoyes >> >> Capture x0 in lowlevel_init.S as potential fdt address. Modify >> board_fdt_blob_setup to use fdt address from either vexpress_aemv8.h >> or lowlevel_init.S. >> >> Signed-off-by: Peter Hoyes >> --- >> board/armltd/vexpress64/Makefile | 5 +++++ >> board/armltd/vexpress64/lowlevel_init.S | 12 ++++++++++++ >> board/armltd/vexpress64/vexpress64.c | 24 ++++++++++++++++++++++++ >> 3 files changed, 41 insertions(+) >> create mode 100644 board/armltd/vexpress64/lowlevel_init.S >> >> diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile >> index 868dc4f629..5703e75967 100644 >> --- a/board/armltd/vexpress64/Makefile >> +++ b/board/armltd/vexpress64/Makefile >> @@ -5,3 +5,8 @@ >> >> obj-y := vexpress64.o >> obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o >> +ifdef CONFIG_OF_BOARD >> +ifndef CONFIG_TARGET_VEXPRESS64_JUNO >> +obj-y += lowlevel_init.o > I wonder if it hurts to avoid all these confusing conditionals and just > always include this, even for Juno? Maybe we can use x0 even on the Juno > at some day? > >> +endif >> +endif >> diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S >> new file mode 100644 >> index 0000000000..3dcfb85d0e >> --- /dev/null >> +++ b/board/armltd/vexpress64/lowlevel_init.S >> @@ -0,0 +1,12 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * (C) Copyright 2021 Arm Limited >> + */ >> + >> +.global save_boot_params >> +save_boot_params: >> + >> + adr x8, prior_stage_fdt_address >> + str x0, [x8] >> + >> + b save_boot_params_ret >> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c >> index d2f307cca5..bde6ef1ba3 100644 >> --- a/board/armltd/vexpress64/vexpress64.c >> +++ b/board/armltd/vexpress64/vexpress64.c >> @@ -86,6 +86,8 @@ int dram_init_banksize(void) >> } >> >> #ifdef CONFIG_OF_BOARD > Do we still need this? Every vexpress64 platform should now rely on OF_BOARD? BASE_FVP does not have OF_BOARD defined in its defconfig for now. I have done some basic testing with OF_BOARD enabled on the BASE_FVP and it seems OK at first glance, but I'm concerned about changes in behavior for existing users (e.g. if extra drivers are being automatically instantiated by nodes in the fdt). The other changes in this patch series (e.g. changing the env vars) are easier to reason about. > >> + >> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO >> #define JUNO_FLASH_SEC_SIZE (256 * 1024) >> static phys_addr_t find_dtb_in_nor_flash(const char *partname) >> { >> @@ -130,9 +132,19 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname) >> >> return ~0; >> } >> +#else > As suggested above, we probably should always include this variable below, so turn the #else into an #endif? > >> + >> +/* Assigned in lowlevel_init.S >> + * Push the variable into the .data section so that it >> + * does not get cleared later. >> + */ >> +unsigned long __section(".data") prior_stage_fdt_address; >> + >> +#endif >> >> void *board_fdt_blob_setup(int *err) >> { >> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO >> phys_addr_t fdt_rom_addr = find_dtb_in_nor_flash(CONFIG_JUNO_DTB_PART); >> >> *err = 0; >> @@ -142,6 +154,18 @@ void *board_fdt_blob_setup(int *err) >> } >> >> return (void *)fdt_rom_addr; >> +#else > Can you turn this into an #endif? > >> + if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) { >> + *err = 0; >> + return (void *)VEXPRESS_FDT_ADDR; > "else" after an unconditional return is somewhat frowned upon, since it > gives a wrong impression about the code flow. > > What about: > #ifdef VEXPRESS_FDT_ADDR > if (fdt_magic(VEXPRESS_FDT_ADDR) ... { > ... > return ...; > } > #endif > >> + } else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) { >> + *err = 0; >> + return (void *)prior_stage_fdt_address; >> + } else { > If we always include prior_stage_fdt_address (even though it may be > unused), you can just always include this piece. And lose the else here, > since we return inside the if branch. > > Cheers, > Andre > >> + *err = -ENXIO; >> + return NULL; >> + } >> +#endif >> } >> #endif >> Your other suggestions make sense to me. Peter