diff mbox series

[7/7] arm: Migrate Apple M1 to save_prev_bl_data

Message ID 20230324004040.572525-7-konrad.dybcio@linaro.org
State Superseded
Headers show
Series [1/7] mmc: msm_sdhci: Match clocks through "clocks" property | expand

Commit Message

Konrad Dybcio March 24, 2023, 12:40 a.m. UTC 
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

Comments

Mark Kettenis March 27, 2023, 7:31 p.m. UTC | #1 
> 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.

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

> 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 <kettenis@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
> 
>
Konrad Dybcio March 28, 2023, 8:17 a.m. UTC | #2 
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.
> 
>>  }
>>  
>> @@ -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 <kettenis@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
>>
>>
Tom Rini March 28, 2023, 2:30 p.m. UTC | #3 
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.
diff mbox series

Patch

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();
 }
 
@@ -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;
 	}
 	/*
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 <kettenis@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