| Message ID | 20230327-qc_cleanups-v2-9-9a80cc563c76@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm cleanups / preparations | expand |
Hi, On Mon, 27 Mar 2023 at 23:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > 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(-) Perhaps we could start using a bloblist (in preparation for the coming Firmware Handoff [1] [2]) to pass information between stages? Regards, Simon [1] https://github.com/FirmwareHandoff/firmware_handoff [2] https://developer.arm.com/documentation/den0135/a
> From: Simon Glass <sjg@chromium.org> > Date: Tue, 28 Mar 2023 08:02:24 +1300 > > Hi, > > On Mon, 27 Mar 2023 at 23:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > 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(-) > > Perhaps we could start using a bloblist (in preparation for the coming > Firmware Handoff [1] [2]) to pass information between stages? We don't have TPL or SPL on these machines, only U-Boot proper. And the handoff protocol between m1n1 and U-Boot is defined by [3]. I have zero interest in changing that and my guess is that Hector Martin feels the same. Cheers, Mark [3] https://docs.kernel.org/arm64/booting.html
Hi Mark, On Tue, 28 Mar 2023 at 08:19, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Tue, 28 Mar 2023 08:02:24 +1300 > > > > Hi, > > > > On Mon, 27 Mar 2023 at 23:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > 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(-) > > > > Perhaps we could start using a bloblist (in preparation for the coming > > Firmware Handoff [1] [2]) to pass information between stages? > > We don't have TPL or SPL on these machines, only U-Boot proper. And > the handoff protocol between m1n1 and U-Boot is defined by [3]. I > have zero interest in changing that and my guess is that Hector Martin > feels the same. This has nothing to do with TPL, nor SPL. It is a way of communicating between projects. I hope you can both find the time and inclination to support firmware standardisation as it evolves. Projects need to work together. Regards, Simon > > Cheers, > > Mark > > [3] https://docs.kernel.org/arm64/booting.html
> From: Simon Glass <sjg@chromium.org> > Date: Tue, 28 Mar 2023 08:32:45 +1300 > > Hi Mark, > > On Tue, 28 Mar 2023 at 08:19, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > Date: Tue, 28 Mar 2023 08:02:24 +1300 > > > > > > Hi, > > > > > > On Mon, 27 Mar 2023 at 23:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > > > 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(-) > > > > > > Perhaps we could start using a bloblist (in preparation for the coming > > > Firmware Handoff [1] [2]) to pass information between stages? > > > > We don't have TPL or SPL on these machines, only U-Boot proper. And > > the handoff protocol between m1n1 and U-Boot is defined by [3]. I > > have zero interest in changing that and my guess is that Hector Martin > > feels the same. > > This has nothing to do with TPL, nor SPL. It is a way of communicating > between projects. > > I hope you can both find the time and inclination to support firmware > standardisation as it evolves. > > Projects need to work together. https://xkcd.com/927/
On Tue, 28 Mar 2023 at 08:42, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Tue, 28 Mar 2023 08:32:45 +1300 > > > > Hi Mark, > > > > On Tue, 28 Mar 2023 at 08:19, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > From: Simon Glass <sjg@chromium.org> > > > > Date: Tue, 28 Mar 2023 08:02:24 +1300 > > > > > > > > Hi, > > > > > > > > On Mon, 27 Mar 2023 at 23:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > > > > > 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(-) > > > > > > > > Perhaps we could start using a bloblist (in preparation for the coming > > > > Firmware Handoff [1] [2]) to pass information between stages? > > > > > > We don't have TPL or SPL on these machines, only U-Boot proper. And > > > the handoff protocol between m1n1 and U-Boot is defined by [3]. I > > > have zero interest in changing that and my guess is that Hector Martin > > > feels the same. > > > > This has nothing to do with TPL, nor SPL. It is a way of communicating > > between projects. > > > > I hope you can both find the time and inclination to support firmware > > standardisation as it evolves. > > > > Projects need to work together. > > https://xkcd.com/927/ https://media.licdn.com/dms/image/C4D12AQH3JLVFSpPm8A/article-cover_image-shrink_423_752/0/1520204567503?e=1685577600&v=beta&t=nyxh02u39i3z3WxE2AyV3HqFA1gp1YsM6Qy_rcTwCeY
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
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(-)