[PATCH v5 12/14] riscv: sifive: fu540: enable all cache ways from u-boot proper
Rick Chen
rickchen36 at gmail.com
Wed Mar 18 03:27:49 CET 2020
Hi Pragnesh
> From: Pragnesh Patel [mailto:pragnesh.patel at 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 at gmail.com>
> >Sent: 13 March 2020 19:19
> >To: Anup Patel <anup at brainfault.org>
> >Cc: Pragnesh Patel <pragnesh.patel at sifive.com>; U-Boot Mailing List <u-
> >boot at lists.denx.de>; Atish Patra <atish.patra at wdc.com>; Palmer Dabbelt
> ><palmerdabbelt at google.com>; Paul Walmsley <paul.walmsley at sifive.com>;
> >Jagan Teki <jagan at amarulasolutions.com>; Troy Benjegerdes
> ><troy.benjegerdes at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
> >Kadam <sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>; Palmer
> >Dabbelt <palmer at 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 at brainfault.org> wrote:
> >>
> >> On Fri, Mar 13, 2020 at 3:52 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> >> >
> >> > Hi Anup,
> >> >
> >> > On Fri, Mar 13, 2020 at 6:02 PM Anup Patel <anup at brainfault.org> wrote:
> >> > >
> >> > > On Fri, Mar 13, 2020 at 2:31 PM Bin Meng <bmeng.cn at gmail.com>
> >wrote:
> >> > > >
> >> > > > On Wed, Mar 11, 2020 at 3:04 PM Pragnesh Patel
> >> > > > <pragnesh.patel at sifive.com> wrote:
> >> > > > >
> >> > > > > Enable all cache ways from u-boot proper.
> >> > > >
> >> > > > U-Boot
> >> > > >
> >> > > > >
> >> > > > > Signed-off-by: Pragnesh Patel <pragnesh.patel at 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
More information about the U-Boot
mailing list