[U-Boot] [PATCH v5 4/4] riscv: Remove redundant a2 store on DRAM base in start.S
Rick Chen
rickchen36 at gmail.com
Thu Nov 29 10:44:13 UTC 2018
Alexander Graf <agraf at suse.de> 於 2018年11月27日 週二 下午6:42寫道：
>
>
>
> 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));
Hi Alex
I will try and test it.
Thanks a lot
B.R
Rick
More information about the U-Boot
mailing list