On Tue, Mar 28, 2023 at 10:17:44AM +0200, Konrad Dybcio wrote:
> 
> 
> On 27.03.2023 21:31, Mark Kettenis wrote:
> >> From: Konrad Dybcio <konrad.dybcio@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.dybcio@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.

I think maybe a problem is save_boot_params_ret is poorly defined, or
too specifically defined as a point in the early boot code.

We're in a funny spot here since arch/arm/lib/save_prev_bl_data.c isn't
exactly generic (it might be previous BL is little kernel specific? not
100% sure), as well. It would be good, now that everything is in
Kconfig, to tighten the dependencies (this code is arm64 only, and
initramfs without fdt option doesn't make sense, so you can cleanup the
Makefile and caller logic) and preface is_addr_accessible with static as
it's only called there.

> >>  }
> >>  
> >> @@ -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?

Well, in the next iteration of this series just drop the apple m1
related changes.

-- 
Tom