[PATCH v5 12/14] riscv: sifive: fu540: enable all cache ways from u-boot proper

[Pragnesh Patel]

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/gene
>> ric/brd_main.c
>> https://github.com/avpatel/xvisor-next/blob/master/arch/arm/board/gene
>> 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.

>> >
>> > >
>> > > 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