On 27.03.2023 21:31, Mark Kettenis wrote:
>> From: Konrad Dybcio <konrad.dyb...@linaro.org>
>> Date: Fri, 24 Mar 2023 01:40:40 +0100
>
> Hi Konrad,
>
>> Mark's and Dzmitry's approaches come down to the same thing.. Let's
>> unify them by first removing the static keyword from the common file
>> to allow the variable to be reused, then renaming "reg0" to the more
>> sensible fw_dtb_pointer coming from the Apple file and finally remove
>> the mach-apple implementation of this very thing and enable the common
>> approach in the respective defconfig.
>>
>> Only build-tested.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dyb...@linaro.org>
>> ---
>>
>> arch/arm/lib/save_prev_bl_data.c | 14 +++++++-------
>> arch/arm/mach-apple/Makefile | 1 -
>> arch/arm/mach-apple/lowlevel_init.S | 17 -----------------
>> configs/apple_m1_defconfig | 1 +
>> 4 files changed, 8 insertions(+), 25 deletions(-)
>> delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
>>
>> diff --git a/arch/arm/lib/save_prev_bl_data.c
>> b/arch/arm/lib/save_prev_bl_data.c
>> index f7b23faf0d66..a127fec1f149 100644
>> --- a/arch/arm/lib/save_prev_bl_data.c
>> +++ b/arch/arm/lib/save_prev_bl_data.c
>> @@ -15,7 +15,7 @@
>> #include <asm/system.h>
>> #include <asm/armv8/mmu.h>
>>
>> -static ulong reg0 __section(".data");
>> +ulong fw_dtb_pointer __section(".data");
>>
>> /**
>> * Save x0 register value, assuming previous bootloader set it to
>> @@ -23,7 +23,7 @@ static ulong reg0 __section(".data");
>> */
>> void save_boot_params(ulong r0)
>> {
>> - reg0 = r0;
>> + fw_dtb_pointer = r0;
>> save_boot_params_ret();
>
> I suppose this works, but it is a bit strange sice
> save_boot_params_ret is just a label that we're supposed to jump back
> to, not a function.
>
>> }
>>
>> @@ -51,24 +51,24 @@ int save_prev_bl_data(void)
>> int node;
>> u64 initrd_start_prop;
>>
>> - if (!is_addr_accessible((phys_addr_t)reg0))
>> + if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer))
>> return -ENODATA;
>>
>> - fdt_blob = (struct fdt_header *)reg0;
>> + fdt_blob = (struct fdt_header *)fw_dtb_pointer;
>> if (!fdt_valid(&fdt_blob)) {
>> - pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__,
>> reg0);
>> + pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__,
>> fw_dtb_pointer);
>> return -ENODATA;
>> }
>>
>> if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
>> - env_set_addr("prevbl_fdt_addr", (void *)reg0);
>> + env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);
>> if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR))
>> return 0;
>>
>> node = fdt_path_offset(fdt_blob, "/chosen");
>> if (!node) {
>> pr_warn("%s: chosen node not found in device tree at addr:
>> 0x%lx\n",
>> - __func__, reg0);
>> + __func__, fw_dtb_pointer);
>> return -ENODATA;
>> }
>> /*
>
> And we have no use for these additional environment variables and I'd
> prefer not to add them.
Okay, let's forget about it then.
Would you (or anybody responsible, I don't know who picks things up)
be willing to take this patch without the Apple part then, a.k.a.
renaming r0 to fw_dtb_pointer?
Konrad
>
>> diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile
>> index 50b465b9473f..a775d8866ad4 100644
>> --- a/arch/arm/mach-apple/Makefile
>> +++ b/arch/arm/mach-apple/Makefile
>> @@ -1,6 +1,5 @@
>> # SPDX-License-Identifier: GPL-2.0+
>>
>> obj-y += board.o
>> -obj-y += lowlevel_init.o
>> obj-y += rtkit.o
>> obj-$(CONFIG_NVME_APPLE) += sart.o
>> diff --git a/arch/arm/mach-apple/lowlevel_init.S
>> b/arch/arm/mach-apple/lowlevel_init.S
>> deleted file mode 100644
>> index e1c0d91cef2c..000000000000
>> --- a/arch/arm/mach-apple/lowlevel_init.S
>> +++ /dev/null
>> @@ -1,17 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0+ */
>> -/*
>> - * (C) Copyright 2021 Mark Kettenis <kette...@openbsd.org>
>> - */
>> -
>> -.align 8
>> -.global fw_dtb_pointer
>> -fw_dtb_pointer:
>> - .quad 0
>> -
>> -.global save_boot_params
>> -save_boot_params:
>> - /* Stash DTB pointer passed by m1n1 */
>> - adr x1, fw_dtb_pointer
>> - str x0, [x1]
>> -
>> - b save_boot_params_ret
>> diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig
>> index b4ecf73cbc78..eb0addb741c5 100644
>> --- a/configs/apple_m1_defconfig
>> +++ b/configs/apple_m1_defconfig
>> @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y
>> CONFIG_DEFAULT_DEVICE_TREE="t8103-j274"
>> CONFIG_SYS_LOAD_ADDR=0x0
>> CONFIG_USE_PREBOOT=y
>> +CONFIG_SAVE_PREV_BL_FDT_ADDR=y
>> # CONFIG_DISPLAY_CPUINFO is not set
>> CONFIG_DISPLAY_BOARDINFO_LATE=y
>> CONFIG_BOARD_LATE_INIT=y
>> --
>> 2.40.0
>>
>>
