[U-Boot] [PATCH v5 4/4] riscv: Remove redundant a2 store on DRAM base in start.S

[Alexander Graf <agraf@suse.de>]

On 27.11.18 09:42, Anup Patel wrote:
> On Tue, Nov 27, 2018 at 1:26 PM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Tue, Nov 27, 2018 at 12:30 PM Rick Chen <rickchen36@gmail.com> wrote:
>>>
>>> Anup Patel <anup@brainfault.org> 於 2018年11月27日 週二 下午2:40寫道：
>>>>
>>>> On Tue, Nov 27, 2018 at 12:00 PM Rick Chen <rickchen36@gmail.com> wrote:
>>>>>
>>>>> Anup Patel <anup@brainfault.org> 於 2018年11月27日 週二 下午2:14寫道：
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, 27 Nov, 2018, 11:38 AM Rick Chen <rickchen36@gmail.com wrote:
>>>>>>>
>>>>>>> Anup Patel <anup@brainfault.org> 於 2018年11月27日 週二 下午1:47寫道：
>>>>>>>>
>>>>>>>> On Tue, Nov 27, 2018 at 11:14 AM Anup Patel <anup@brainfault.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Nov 27, 2018 at 10:50 AM Rick Chen <rickchen36@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Anup Patel <anup@brainfault.org> 於 2018年11月27日 週二 上午11:28寫道：
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Nov 27, 2018 at 8:50 AM Rick Chen <rickchen36@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@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 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));