[U-Boot] [PATCH v5 4/4] riscv: Remove redundant a2 store on DRAM base in start.S
Alexander Graf
agraf at suse.de
Tue Nov 27 10:41:55 UTC 2018
On 27.11.18 09:42, Anup Patel wrote:
> On Tue, Nov 27, 2018 at 1:26 PM Anup Patel <anup at brainfault.org> wrote:
>>
>> On Tue, Nov 27, 2018 at 12:30 PM Rick Chen <rickchen36 at gmail.com> wrote:
>>>
>>> Anup Patel <anup at brainfault.org> 於 2018年11月27日 週二 下午2:40寫道：
>>>>
>>>> On Tue, Nov 27, 2018 at 12:00 PM Rick Chen <rickchen36 at gmail.com> wrote:
>>>>>
>>>>> Anup Patel <anup at brainfault.org> 於 2018年11月27日 週二 下午2:14寫道：
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, 27 Nov, 2018, 11:38 AM Rick Chen <rickchen36 at gmail.com wrote:
>>>>>>>
>>>>>>> Anup Patel <anup at brainfault.org> 於 2018年11月27日 週二 下午1:47寫道：
>>>>>>>>
>>>>>>>> On Tue, Nov 27, 2018 at 11:14 AM Anup Patel <anup at brainfault.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Nov 27, 2018 at 10:50 AM Rick Chen <rickchen36 at gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Anup Patel <anup at brainfault.org> 於 2018年11月27日 週二 上午11:28寫道：
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Nov 27, 2018 at 8:50 AM Rick Chen <rickchen36 at gmail.com> 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 <anup at brainfault.org>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>> 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@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@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));
More information about the U-Boot
mailing list