From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Tue, 27 Nov 2018 11:41:55 +0100 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> <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 27.11.18 09:42, Anup Patel wrote: > 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. I feel like I'm missing something obvious here. Why can't we just preserve a2 in a callee saved register and then eventually write it straight into gd? For example, how about something like this (untested, probably mangled by mailer): diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index ae57fb8313..2281ec0537 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -47,3 +47,8 @@ int print_cpuinfo(void) return 0; } + +void arch_setup_gd(gd_t *new_gd, ulong arg) +{ + new_gd->fdt_blob = (const void *)arg; +} diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 7cd7755190..2acbd15f7c 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -44,8 +44,7 @@ trap_vector: .global trap_entry handle_reset: - li t0, CONFIG_SYS_SDRAM_BASE - SREG a2, 0(t0) + mv s0, a2 la t0, trap_entry csrw mtvec, t0 csrwi mstatus, 0 @@ -75,6 +74,7 @@ call_board_init_f_0: mv a0, sp jal board_init_f_alloc_reserve mv sp, a0 + mv a1, s0 jal board_init_f_init_reserve mv a0, zero /* a0 <-- boot_flags = 0 */ diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c index 208ef08562..c370ae0808 100644 --- a/arch/x86/cpu/i386/cpu.c +++ b/arch/x86/cpu/i386/cpu.c @@ -113,7 +113,7 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) asm volatile("lgdtl %0\n" : : "m" (gdt)); } -void arch_setup_gd(gd_t *new_gd) +void arch_setup_gd(gd_t *new_gd, ulong arg) { u64 *gdt_addr; diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c index 6c063e8200..a17ee5fd78 100644 --- a/arch/x86/cpu/x86_64/cpu.c +++ b/arch/x86/cpu/x86_64/cpu.c @@ -16,7 +16,7 @@ */ struct global_data *global_data_ptr = (struct global_data *)~0; -void arch_setup_gd(gd_t *new_gd) +void arch_setup_gd(gd_t *new_gd, ulong arg) { global_data_ptr = new_gd; } diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c index 7d290740bf..4313ff1e74 100644 --- a/arch/x86/lib/spl.c +++ b/arch/x86/lib/spl.c @@ -72,7 +72,7 @@ static int x86_spl_init(void) */ gd->new_gd = (struct global_data *)ptr; memcpy(gd->new_gd, gd, sizeof(*gd)); - arch_setup_gd(gd->new_gd); + arch_setup_gd(gd->new_gd, 0); gd->start_addr_sp = (ulong)ptr; /* Cache the SPI flash. Otherwise copying the code to RAM takes ages */ diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c index 5f4ca0f5a7..74f2d40ec5 100644 --- a/board/AndesTech/ax25-ae350/ax25-ae350.c +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c @@ -66,9 +66,8 @@ 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; - if (fdt_magic(*ptr) == FDT_MAGIC) - return (void *)*ptr; + if (fdt_magic(gd->fdt_blob) == FDT_MAGIC) + return (void *)gd->fdt_blob; return (void *)CONFIG_SYS_FDT_BASE; } diff --git a/common/board_f.c b/common/board_f.c index f1a1432d86..d853480ae7 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -726,7 +726,7 @@ static int jump_to_copy(void) * with the stack in SDRAM and Global Data in temporary memory * (CPU cache) */ - arch_setup_gd(gd->new_gd); + arch_setup_gd(gd->new_gd, 0); board_init_f_r_trampoline(gd->start_addr_sp); #else relocate_code(gd->start_addr_sp, gd->new_gd, gd->relocaddr); diff --git a/common/board_r.c b/common/board_r.c index c55e33eec2..39ac04d528 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -856,7 +856,7 @@ void board_init_r(gd_t *new_gd, ulong dest_addr) * dropping the new_gd parameter. */ #if CONFIG_IS_ENABLED(X86_64) - arch_setup_gd(new_gd); + arch_setup_gd(new_gd, 0); #endif #ifdef CONFIG_NEEDS_MANUAL_RELOC diff --git a/common/init/board_init.c b/common/init/board_init.c index 526fee35ff..be57b422f8 100644 --- a/common/init/board_init.c +++ b/common/init/board_init.c @@ -12,7 +12,7 @@ DECLARE_GLOBAL_DATA_PTR; /* Unfortunately x86 or ARM can't compile this code as gd cannot be assigned */ #if !defined(CONFIG_X86) && !defined(CONFIG_ARM) -__weak void arch_setup_gd(struct global_data *gd_ptr) +__weak void arch_setup_gd(struct global_data *gd_ptr, ulong arg) { gd = gd_ptr; } @@ -96,7 +96,7 @@ ulong board_init_f_alloc_reserve(ulong top) * (seemingly useless) incrementation causes no code increase. */ -void board_init_f_init_reserve(ulong base) +void board_init_f_init_reserve(ulong base, ulong arg) { struct global_data *gd_ptr; @@ -110,7 +110,7 @@ void board_init_f_init_reserve(ulong base) memset(gd_ptr, '\0', sizeof(*gd)); /* set GD unless architecture did it already */ #if !defined(CONFIG_ARM) - arch_setup_gd(gd_ptr); + arch_setup_gd(gd_ptr, arg); #endif /* next alloc will be higher by one GD plus 16-byte alignment */ base += roundup(sizeof(struct global_data), 16); diff --git a/include/init.h b/include/init.h index afc953d51e..d80fa75ed3 100644 --- a/include/init.h +++ b/include/init.h @@ -152,6 +152,7 @@ void board_init_f_init_reserve(ulong base); /** * arch_setup_gd() - Set up the global_data pointer * @gd_ptr: Pointer to global data + * @arg: Architecture specific opaque data * * This pointer is special in some architectures and cannot easily be assigned * to. For example on x86 it is implemented by adding a specific record to its @@ -160,7 +161,7 @@ void board_init_f_init_reserve(ulong base); * * gd = gd_ptr; */ -void arch_setup_gd(gd_t *gd_ptr); +void arch_setup_gd(gd_t *gd_ptr, ulong arg); /* common/board_r.c */ void board_init_r(gd_t *id, ulong dest_addr) __attribute__ ((noreturn));