Bin Meng <bmeng...@gmail.com> 於 2018年11月30日 週五 下午3:26寫道： > > Hi Rick, > > On Fri, Nov 30, 2018 at 3:15 PM Rick Chen <rickche...@gmail.com> wrote: > > > > Bin Meng <bmeng...@gmail.com> 於 2018年11月30日 週五 下午2:57寫道： > > > > > > Hi Rick, > > > > > > On Fri, Nov 30, 2018 at 2:41 PM Rick Chen <rickche...@gmail.com> wrote: > > > > > > > > Bin Meng <bmeng...@gmail.com> 於 2018年11月30日 週五 下午2:21寫道： > > > > > > > > > > Hi Rick, > > > > > > > > > > On Fri, Nov 30, 2018 at 2:05 PM Rick Chen <rickche...@gmail.com> > > > > > wrote: > > > > > > > > > > > > Bin Meng <bmeng...@gmail.com> 於 2018年11月30日 週五 上午9:48寫道： > > > > > > > > > > > > > > Hi Rick, > > > > > > > > > > > > > > On Thu, Nov 29, 2018 at 6:41 PM Rick Chen <rickche...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > Bin Meng <bmeng...@gmail.com> 於 2018年11月27日 週二 下午6:07寫道： > > > > > > > > > > > > > > > > > > Hi Rick, > > > > > > > > > > > > > > > > > > On Tue, Nov 27, 2018 at 4:43 PM Rick Chen > > > > > > > > > <rickche...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Anup Patel <a...@brainfault.org> 於 2018年11月27日 週二 下午3:56寫道： > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 27, 2018 at 12:30 PM Rick Chen > > > > > > > > > > > <rickche...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Anup Patel <a...@brainfault.org> 於 2018年11月27日 週二 > > > > > > > > > > > > 下午2:40寫道： > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 27, 2018 at 12:00 PM Rick Chen > > > > > > > > > > > > > <rickche...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Anup Patel <a...@brainfault.org> 於 2018年11月27日 週二 > > > > > > > > > > > > > > 下午2:14寫道： > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 27 Nov, 2018, 11:38 AM Rick Chen > > > > > > > > > > > > > > > <rickche...@gmail.com wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> Anup Patel <a...@brainfault.org> 於 2018年11月27日 > > > > > > > > > > > > > > >> 週二 下午1:47寫道： > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > On Tue, Nov 27, 2018 at 11:14 AM Anup Patel > > > > > > > > > > > > > > >> > <a...@brainfault.org> wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > On Tue, Nov 27, 2018 at 10:50 AM Rick Chen > > > > > > > > > > > > > > >> > > <rickche...@gmail.com> wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > Anup Patel <a...@brainfault.org> 於 > > > > > > > > > > > > > > >> > > > 2018年11月27日 週二 上午11:28寫道： > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > On Tue, Nov 27, 2018 at 8:50 AM Rick > > > > > > > > > > > > > > >> > > > > Chen <rickche...@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 > > > > > > > > > > > > > > >> > > > > > > > > > <a...@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? > > > > > > > > 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. > > > > This one.
Hi Bin Thanks for your opinion. :) Rick > > > 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 ? > > > > Regards, > Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
