From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anup Patel Date: Tue, 27 Nov 2018 14:12:29 +0530 Subject: [U-Boot] [PATCH v5 4/4] riscv: Remove redundant a2 store on DRAM base in start.S In-Reply-To: References: <20181126103910.14457-1-anup@brainfault.org> <20181126103910.14457-5-anup@brainfault.org> <1285af47b99a60926626a52c0f124df80f01a2ed.camel@aisec.fraunhofer.de> <752D002CFF5D0F4FA35C0100F1D73F3FA3A50A96@ATCPCS16.andestech.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Tue, Nov 27, 2018 at 1:26 PM Anup Patel wrote: > > On Tue, Nov 27, 2018 at 12:30 PM Rick Chen wrote: > > > > Anup Patel 於 2018年11月27日 週二 下午2:40寫道： > > > > > > On Tue, Nov 27, 2018 at 12:00 PM Rick Chen wrote: > > > > > > > > Anup Patel 於 2018年11月27日 週二 下午2:14寫道： > > > > > > > > > > > > > > > > > > > > On Tue, 27 Nov, 2018, 11:38 AM Rick Chen > > > >> > > > > >> Anup Patel 於 2018年11月27日 週二 下午1:47寫道： > > > > >> > > > > > >> > On Tue, Nov 27, 2018 at 11:14 AM Anup Patel wrote: > > > > >> > > > > > > >> > > On Tue, Nov 27, 2018 at 10:50 AM Rick Chen wrote: > > > > >> > > > > > > > >> > > > Anup Patel 於 2018年11月27日 週二 上午11:28寫道： > > > > >> > > > > > > > > >> > > > > On Tue, Nov 27, 2018 at 8:50 AM Rick Chen wrote: > > > > >> > > > > > > > > > >> > > > > > > > > > Currently, the RISC-V U-Boot is saving a2 register at > > > > >> > > > > > > > > > CONFIG_SYS_DRAM_BASE in start.S which does not make sense because > > > > >> > > > > > > > > > there is no information passed by previous booting stage in a2 > > > > >> > > > > > > > > > register. > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > This patch removes redundant a2 store on DRAM base. > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > Signed-off-by: Anup Patel > > > > >> > > > > > > > > > --- > > > > >> > > > > > > > > > arch/riscv/cpu/start.S | 2 -- > > > > >> > > > > > > > > > 1 file changed, 2 deletions(-) > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index > > > > >> > > > > > > > > > 704190f946..e4276e8e19 100644 > > > > >> > > > > > > > > > --- a/arch/riscv/cpu/start.S > > > > >> > > > > > > > > > +++ b/arch/riscv/cpu/start.S > > > > >> > > > > > > > > > @@ -38,8 +38,6 @@ _start: > > > > >> > > > > > > > > > mv s0, a0 > > > > >> > > > > > > > > > mv s1, a1 > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > - li t0, CONFIG_SYS_SDRAM_BASE > > > > >> > > > > > > > > > - SREG a2, 0(t0) > > > > >> > > > > > > > > > la t0, trap_entry > > > > >> > > > > > > > > > #ifdef CONFIG_RISCV_SMODE > > > > >> > > > > > > > > > csrw stvec, t0 > > > > >> > > > > > > > > > -- > > > > >> > > > > > > > > > > > > >> > > > > > > > > This is weird. I remember these two lines were already removed by > > > > >> > > > > > > > > Lukas's patch series before? Did not have time to dig out the history > > > > >> > > > > > > > > though. > > > > >> > > > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > Regards, > > > > >> > > > > > > > > Bin > > > > >> > > > > > > > > > > > >> > > > > > > > You are correct, however I removed it again, because I did not want to break > > > > >> > > > > > > > Rick's board. He did add a commit to the last pull request that removes these > > > > >> > > > > > > > two lines and adjusts his board accordingly, but it is not in the current one. > > > > >> > > > > > > > > > > > >> > > > > > > > > > >> > > > > > Hi Likas > > > > >> > > > > > > > > > >> > > > > > Thanks for your explanation. > > > > >> > > > > > > > > > >> > > > > > RIck's commit as below > > > > >> > > > > > https://www.mail-archive.com/u-boot at lists.denx.de/msg305880.html > > > > >> > > > > > > > > >> > > > > When we run U-Boot in S-mode the BBL runs from 0x80000000 so this > > > > >> > > > > two lines corrupts BBL instructions. > > > > >> > > > > > > > > >> > > > > If this is important for some board then please have it around #ifdef. > > > > >> > > > > > > > > >> > > > > > > > >> > > > Hi Anup > > > > >> > > > > > > > >> > > > In the discussion as below : > > > > >> > > > https://www.mail-archive.com/u-boot at lists.denx.de/msg305880.html > > > > >> > > > > > > > >> > > > I try to solve this issue with the aptch > > > > >> > > > [PATCH] riscv: ax25-ae350: Pass dtb address to u-boot with a1 register > > > > >> > > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > > > > >> > > > - li t0, CONFIG_SYS_SDRAM_BASE > > > > >> > > > - SREG a2, 0(t0) > > > > >> > > > > > > > >> > > > diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c > > > > >> > > > b/board/AndesTech/ax25-ae350/ax25-ae350.c > > > > >> > > > void *board_fdt_blob_setup(void) > > > > >> > > > { > > > > >> > > > - void **ptr = (void *)CONFIG_SYS_SDRAM_BASE; > > > > >> > > > + void **ptr = (void *)&prior_stage_fdt_address; > > > > >> > > > > > > > >> > > > in the previous pull request. > > > > >> > > > > > > > >> > > > But Bin do not agree with that I use prior_stage_fdt_address in > > > > >> > > > board_fdt_blob_setup( ) > > > > >> > > > I try to explain why I use it like that way. > > > > >> > > > Then Bin have not any reply in the following mail. > > > > >> > > > Finally I decide to drop this patch in the next pull request. > > > > >> > > > > > > > >> > > > Hi Bin > > > > >> > > > > > > > >> > > > How do you think about I recovery this patch to fix this issue ? > > > > >> > > > > > > >> > > Actually, previous booting stage can pass location of FDT stored in flash > > > > >> > > to U-Boot. U-Boot requires FDT at a DRAM location which it can modify > > > > >> > > in-place before passing on to Linux kernel so we should relocate the FDT > > > > >> > > passed by previous booting stage to some board specific DRAM location. > > > > >> > > > > > > >> > > My suggestion is as follows: > > > > >> > > > > > > >> > > Instead of SDRAM_BASE, we can have new board specific config > > > > >> > > CONFIG_RISCV_PRIOR_FDT_BASE > > > > >> > > > > > > >> > > If CONFIG_RISCV_PRIOR_FDT_BASE is defined/selected by > > > > >> > > config then in start.S copy-over the FDT from location pointed by > > > > >> > > "a1" register to location pointed by CONFIG_RISCV_PRIOR_FDT_BASE. > > > > >> > > > > > >> > > > > >> Hi Anup > > > > >> > > > > >> It can not achieve dtb delivery at runtime. > > > > > > > > > > > > > > > Can you elaborate why? > > > > > > > > > > > > > CONFIG_RISCV_PRIOR_FDT_BASE is determined at compile time. > > > > I am wondering how it can be modified at run time ? > > > > > > I think you miss-understood my suggestion. I did not meant changing > > > CONFIG_RISCV_PRIOR_FDT_BASE at runtime. > > > > > > > > > > > > > > > > I am sure we can always relocate FDT passed by previous stage to some safer location and use it from there. > > > > > > > > My original patch also can achieve that dtb passed by a1 and relocated and boot. > > > > > > Great !!! > > > > > > Why not update your original patch to relocate FDT and use FDT from > > > safer location? > > > > > > > Good question. > > Above can see the question I ask for Bin : > > How do you think about I recovery this patch to fix this issue ? > > > > Before you talking about CONFIG_RISCV_PRIOR_FDT_BASE ! > > Can you explain why you need CONFIG_OF_BOARD ? > Why can you not use CONFIG_OF_PRIOR_STAGE ? I think I got your problem with CONFIG_OF_PRIOR_STAGE. U-Boot cannot update prior_stage_fdt_address when running from ROM/Flash. Instead of storing FDT pointer on CONFIG_SYS_SDRAM_BASE, you can have a board specific location to save FDT pointer when booting from flash. Here's a simple reference implementation (untested): diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 732a357a99..e83b9295d6 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -60,6 +60,19 @@ config RISCV_SMODE help Enable this option to build U-Boot for RISC-V S-Mode +config RISCV_FDTPTR_SAVE + bool "Save FDT pointer for flash booting" + help + Enable this option to save FDT pointer passed to U-Boot + booting from ROM or Flash. + +config RISCV_FDTPTR_SAVE_ADDR + hex "Location for saving FDT pointer" + depends on RISCV_FDTPTR_SAVE + help + Set this to provide board specific location for saving + FDT pointer. + config 32BIT bool diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index e4276e8e19..57c4070e0a 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -38,6 +38,11 @@ _start: mv s0, a0 mv s1, a1 +#ifdef CONFIG_RISCV_FDTPTR_SAVE + li t0, CONFIG_RISCV_FDTPTR_SAVE_ADDR + SREG s1, 0(t0) +#endif + la t0, trap_entry #ifdef CONFIG_RISCV_SMODE csrw stvec, t0 diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c index 5f4ca0f5a7..f3535a1d52 100644 --- a/board/AndesTech/ax25-ae350/ax25-ae350.c +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c @@ -66,9 +66,9 @@ ulong board_flash_get_legacy(ulong base, int banknum, flash_info_t *info) void *board_fdt_blob_setup(void) { - void **ptr = (void *)CONFIG_SYS_SDRAM_BASE; + void **ptr = (void *)CONFIG_RISCV_FDTPTR_SAVE_ADDR; if (fdt_magic(*ptr) == FDT_MAGIC) - return (void *)*ptr; + return (void *)*ptr; return (void *)CONFIG_SYS_FDT_BASE; } diff --git a/configs/ax25-ae350_64_defconfig b/configs/ax25-ae350_64_defconfig index b250d3fc7e..b4ba2ed18b 100644 --- a/configs/ax25-ae350_64_defconfig +++ b/configs/ax25-ae350_64_defconfig @@ -35,3 +35,5 @@ CONFIG_SYS_NS16550=y CONFIG_SPI=y CONFIG_ATCSPI200_SPI=y CONFIG_ATCPIT100_TIMER=y +CONFIG_RISCV_FDTPTR_SAVE=y +CONFIG_RISCV_FDTPTR_SAVE_ADDR=0x80000000 Regards, Anup