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

[Anup Patel]

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?

I am sure we can always relocate FDT passed by previous stage to some safer
location and use it from there.

Regards,
Anup


> Rick
>
> > I think you can copy-over FDT in C code too. You don't need to do
> > in start.S.
> >
> > >
> > > In your board_fdt_blob_setup(), you can safely do:
> > > void **ptr = (void *)CONFIG_RISCV_PRIOR_FDT_BASE;
> > >
> > > Regards,
> > > Anup
>