[U-Boot] [PATCH v5 4/4] riscv: Remove redundant a2 store on DRAM base in start.S
Bin Meng
bmeng.cn at gmail.com
Fri Nov 30 06:21:25 UTC 2018
Hi Rick,
On Fri, Nov 30, 2018 at 2:05 PM Rick Chen <rickchen36 at gmail.com> wrote:
>
> Bin Meng <bmeng.cn at gmail.com> 於 2018年11月30日 週五 上午9:48寫道：
> >
> > Hi Rick,
> >
> > On Thu, Nov 29, 2018 at 6:41 PM Rick Chen <rickchen36 at gmail.com> wrote:
> > >
> > > Bin Meng <bmeng.cn at gmail.com> 於 2018年11月27日 週二 下午6:07寫道：
> > > >
> > > > Hi Rick,
> > > >
> > > > On Tue, Nov 27, 2018 at 4:43 PM Rick Chen <rickchen36 at gmail.com> wrote:
> > > > >
> > > > > Anup Patel <anup at brainfault.org> 於 2018年11月27日 週二 下午3:56寫道：
> > > > > >
> > > > > > 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 ?
> > > > > >
> > > > >
> > > > > You can find the discussion as below:
> > > > > https://patchwork.ozlabs.org/patch/988884/
> > > > >
> > > >
> > > > If I understand correctly, so far we have two scenarios to support:
> > > >
> > > > 1. U-Boot is booting directly from reset vector from ROM. This
> > > > canonical way to support DT is via CONFIG_OF_EMBED or
> > > > CONFIG_OF_SEPARATE. In such configuration, there is no any previous
> > > > bootloader so the concept of CONFIG_OF_PRIOR_STAGE does not apply.
> > > >
> > > > 2. U-Boot is booting from previous bootloader, and DT is passed in a1
> > > > register. Such configuration we can use CONFIG_OF_PRIOR_STAGE.
> > > >
> > >
> > > I use the 3rd way, CONFIG_OF_BOARD.
> > > It can be found in README.
> > > And can be chosen by make menuconfig
> > >
> > > It is the most flexible way that I can implement my own board_fdt_blob_setup( )
> > > which can both support boot from RAM or ROM without any configuration change.
> > > I will indeed use CONFIG_OF_EMBED when u-boot is in debug stage.
> > >
> >
> > What I don't understand is that if U-Boot is booting directly from the
> > reset vector, there is no a1 passed by anyone. U-Boot itself should
> > figure out a way to determine the DT and that's how CONFIG_OF_EMBED or
> > CONFIG_OF_SEPARATE is doing. Can you elaborate this configuration?
> >
>
> I use CONFIG_OF_BOARD and board_fdt_blob_setup as below:
>
> void *board_fdt_blob_setup(void)
> {
> void **ptr = (void *)CONFIG_SYS_SDRAM_BASE;
> if (fdt_magic(*ptr) == FDT_MAGIC)
> return (void *)*ptr;
>
> return (void *)CONFIG_SYS_FDT_BASE;
> }
>
> 1. When booting from RAM
> a1 can be passed by loader and reserved in CONFIG_SYS_SDRAM_BASE
> temporarily.
>
> 2. When booting from FLASH
> a1 can NOT be passed by loader and reserved in
> CONFIG_SYS_SDRAM_BASE temporarily.
>
> So dtb shall be get from CONFIG_SYS_FDT_BASE (flash base) which
> the dtb shall be pre-burned in this flash space..
> Or CONFIG_SYS_FDT_BASE maybe a memory map space (NOT RAM or ROM)
> provided by HW.
>
> That is why I mean using CONFIG_OF_BOARD can support both boot from
> ram or rom with one configuration.
>
Thanks for the info. So with the latest info, I now understand you
wanted to use CONFIG_OF_BOARD to support both configurations. That
makes sense to me, however I still wanted to point out the canonical
way to support booting from ROM is via CONFIG_OF_SEPARATE or
CONFIG_OF_EMBED. Since your board can support booting from reset
vector directly, what's the need to support the booting from RAM
configuration? In that configuration, what is the first stage
bootloader?
Regards,
Bin
More information about the U-Boot
mailing list