Re: [PATCH v5 12/14] riscv: sifive: fu540: enable all cache ways from u-boot proper
Hi Pragnesh
From: Pragnesh Patel [mailto:pragnesh.patel@sifive.com] Sent: Tuesday, March 17, 2020 5:52 PM To: Bin Meng; Anup Patel Cc: U-Boot Mailing List; Atish Patra; Palmer Dabbelt; Paul Walmsley; Jagan Teki; Troy Benjegerdes; Anup Patel; Sagar Kadam; Rick Jian-Zhi Chen(陳建志); Palmer Dabbelt Subject: RE: [PATCH v5 12/14] riscv: sifive: fu540: enable all cache ways from u-boot proper
Hi,
-----Original Message----- From: Bin Meng bmeng.cn@gmail.com Sent: 13 March 2020 19:19 To: Anup Patel anup@brainfault.org Cc: Pragnesh Patel pragnesh.patel@sifive.com; U-Boot Mailing List <u- boot@lists.denx.de>; Atish Patra atish.patra@wdc.com; Palmer Dabbelt palmerdabbelt@google.com; Paul Walmsley paul.walmsley@sifive.com; Jagan Teki jagan@amarulasolutions.com; Troy Benjegerdes troy.benjegerdes@sifive.com; Anup Patel anup.patel@wdc.com; Sagar Kadam sagar.kadam@sifive.com; Rick Chen rick@andestech.com; Palmer Dabbelt palmer@dabbelt.com Subject: Re: [PATCH v5 12/14] riscv: sifive: fu540: enable all cache ways from u-boot proper
Hi Anup,
On Fri, Mar 13, 2020 at 6:49 PM Anup Patel anup@brainfault.org wrote:
On Fri, Mar 13, 2020 at 3:52 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Anup,
On Fri, Mar 13, 2020 at 6:02 PM Anup Patel anup@brainfault.org wrote:
On Fri, Mar 13, 2020 at 2:31 PM Bin Meng bmeng.cn@gmail.com
wrote:
On Wed, Mar 11, 2020 at 3:04 PM Pragnesh Patel pragnesh.patel@sifive.com wrote: > > Enable all cache ways from u-boot proper.
U-Boot
> > Signed-off-by: Pragnesh Patel pragnesh.patel@sifive.com > --- > board/sifive/fu540/Makefile | 1 + > board/sifive/fu540/cache.c | 20 ++++++++++++++++++++ > board/sifive/fu540/cache.h | 13 +++++++++++++ > board/sifive/fu540/fu540.c | 6 ++++-- > 4 files changed, 38 insertions(+), 2 deletions(-) create > mode 100644 board/sifive/fu540/cache.c create mode 100644 > board/sifive/fu540/cache.h > > diff --git a/board/sifive/fu540/Makefile > b/board/sifive/fu540/Makefile index b05e2f5807..3b867bbd89 > 100644 > --- a/board/sifive/fu540/Makefile > +++ b/board/sifive/fu540/Makefile > @@ -3,6 +3,7 @@ > # Copyright (c) 2019 Western Digital Corporation or its affiliates. > > obj-y += fu540.o > +obj-y += cache.o > > ifdef CONFIG_SPL_BUILD > obj-y += spl.o > diff --git a/board/sifive/fu540/cache.c > b/board/sifive/fu540/cache.c new file mode 100644 index > 0000000000..a0bcd2ba48 > --- /dev/null > +++ b/board/sifive/fu540/cache.c
This should be put into arch/riscv/cpu/fu540/cache.c
> @@ -0,0 +1,20 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2019 SiFive, Inc */ #include <asm/io.h> > + > +/* Register offsets */ > +#define CACHE_ENABLE 0x008 > + > +/* Enable ways; allow cache to use these ways */ void > +cache_enable_ways(u64 base_addr, u8 value) { > + volatile u32 *enable = (volatile u32 *)(base_addr + > + CACHE_ENABLE); > + /* memory barrier */ > + mb(); > + (*enable) = value; > + /* memory barrier */ > + mb(); > +} > diff --git a/board/sifive/fu540/cache.h > b/board/sifive/fu540/cache.h new file mode 100644 index > 0000000000..425124a23b > --- /dev/null > +++ b/board/sifive/fu540/cache.h
arch/riscv/include/asm/arch-fu540/cache.h
Let's not entire FU540 directory under arch/riscv/cpu directory just to have cache functions. The arch/riscv/cpu/generic is perfectly suitable for FU540.
I doubt arch/riscv/cpu/generic can be generic if we consider U-Boot SPL phase. It can be generic for S-mode U-Boot though.
Its really very easy to do. We are already doing this in Xvisor.
As an example, of DT based operations in a generic board support refer: https://github.com/avpatel/xvisor-next/blob/master/arch/arm/board/gen e ric/brd_main.c https://github.com/avpatel/xvisor-next/blob/master/arch/arm/board/gen e ric/foundation-v8.c
Yes, this can be easy to do if we have everything written based on DT. But we cannot always assume DT is available in SPL. Some SoCs will not allow SPL with DT support to run due to constraint resources.
Even for DT, due to SPL initialization codes can be very low-level and specific to an SoC, not everything can be properly modeled by DT. Take a look at the u-boot/arch/arm directory. Things are not that easy.
Using the above approach, we are able to boot same Xvisor ARM/ARM64 binary on multiple boards.
Yes, I know. The same as what is done in the Linux kernel. Take x86 for example, the same kernel image can boot on almost every x86 desktop/laptop/server we have today. But we have to understand that this is built on top of BIOS which does all low-level processor / chipset initialization and hide that very well for OS.
IMHO just for cache, it's better not to add arch/riscv/cpu/fu540. If something that we can not handle with arch/riscv/cpu/generic then we will definitely add arch/riscv/cpu/fu540 dir.
Thanks Bin and Anup for the review.
You shall consider to put it in drivers/cache dir And parse the cache register base from DT instead of hard code (#define CACHE_CTRL_ADDR _AC(0x2010000, UL)) Also parse number of ways instead of hard code (15) // cache_enable_ways(CACHE_CTRL_ADDR, 15); It is a legacy way to define register base with hard code, better not doing that way.
Thanks Rick
If we re-use arch/riscv/cpu/generic as-much as possible then arch/riscv will be easy to maintain in future.
We can add arch/riscv/cpu/generic/cache.c which will do things FU540 specific based on "#ifdef" or "DT compatible string".
Regards, Bin
participants (1)
-
Rick Chen