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

[Rick Chen <rickchen36@gmail.com>]

Bin Meng <bmeng.cn@gmail.com> 於 2018年11月30日 週五 下午2:57寫道：
>
> Hi Rick,
>
> On Fri, Nov 30, 2018 at 2:41 PM Rick Chen <rickchen36@gmail.com> wrote:
> >
> > Bin Meng <bmeng.cn@gmail.com> 於 2018年11月30日 週五 下午2:21寫道：
> > >
> > > Hi Rick,
> > >
> > > On Fri, Nov 30, 2018 at 2:05 PM Rick Chen <rickchen36@gmail.com> wrote:
> > > >
> > > > Bin Meng <bmeng.cn@gmail.com> 於 2018年11月30日 週五 上午9:48寫道：
> > > > >
> > > > > Hi Rick,
> > > > >
> > > > > On Thu, Nov 29, 2018 at 6:41 PM Rick Chen <rickchen36@gmail.com> wrote:
> > > > > >
> > > > > > Bin Meng <bmeng.cn@gmail.com> 於 2018年11月27日 週二 下午6:07寫道：
> > > > > > >
> > > > > > > Hi Rick,
> > > > > > >
> > > > > > > On Tue, Nov 27, 2018 at 4:43 PM Rick Chen <rickchen36@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Anup Patel <anup@brainfault.org> 於 2018年11月27日 週二 下午3:56寫道：
> > > > > > > > >
> > > > > > > > > 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 ?
> > > > > > > > >
> > > > > > > >
> > > > > > > > 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?
> >
> > I will develop u-boot via booting from ram. It will easy for debugging.
> > After develop complete.
> > It will be good for demo via booting from flash.
> > With one configuration, it can decrease to make mistake with wrong
> > configuration between 2 configuration
> > for ram or rom individually.
> >
> > In that configuration, what is the first stage
> > > bootloader?
> >
> > When booting from ram
> > GDB will be the first stage to load u-boot.
> > So it is not good for demo, because it need a pc to fire gdb.
> >
>
> OK, thanks for the info. For this patch, let's go with your proposal
> then, but please add some comments in the codes there for people to
> understand later.

Hi Bin

You say go with your proposal :
Does that mean you agree with the following modification ?
arch/riscv/cpu/start.S
-       li      t0, CONFIG_SYS_SDRAM_BASE
-       SREG    a2, 0(t0)

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;

And add some comments in this patch for people to understand it.

Or
I still can NOT borrow prior_stage_fdt_address and shall find another
way to overcome the problem
after remove the two line in start.S ?

B.R
Rick

>
> Regards,
> Bin