[U-Boot] [PATCH] riscv: cache: Implement i/dcache [status, enable, disable]

[Auer, Lukas]

Hi Rick,

On Tue, 2018-10-30 at 10:48 +0800, Rick Chen wrote:
> Auer, Lukas <lukas.auer at aisec.fraunhofer.de> 於 2018年10月29日 週一
> 下午8:13寫道：
> > 
> > Hi Rick,
> > 
> > On Mon, 2018-10-29 at 11:16 +0800, Rick Chen wrote:
> > > Auer, Lukas <lukas.auer at aisec.fraunhofer.de> 於 2018年10月27日 週六
> > > 上午12:32寫道：
> > > > 
> > > > Hi Rick,
> > > > 
> > > > On Mon, 2018-10-22 at 16:16 +0800, Andes wrote:
> > > > > From: Rick Chen <rick at andestech.com>
> > > > > 
> > > > > AndeStar V5 provide mcache_ctl register which can configure
> > > > > I/D cache as enabled or disabled.
> > > > > 
> > > > > This CSR will be encapsulated by CONFIG_NDS_V5.
> > > > > If you want to configure cache on AndeStar V5
> > > > > AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> > > > > by make menuconfig.
> > > > > 
> > > > > Signed-off-by: Rick Chen <rick at andestech.com>
> > > > > Cc: Greentime Hu <greentime at andestech.com>
> > > > > ---
> > > > >  arch/riscv/Kconfig             |   8 +++
> > > > >  arch/riscv/cpu/ax25/cpu.c      |   4 ++
> > > > >  arch/riscv/cpu/start.S         |  19 ++++++
> > > > >  arch/riscv/include/asm/cache.h |   9 +++
> > > > >  arch/riscv/lib/cache.c         | 131
> > > > > ++++++++++++++++++++++++++++++++++++++---
> > > > >  5 files changed, 163 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > index 168ca3d..de7fd9e 100644
> > > > > --- a/arch/riscv/Kconfig
> > > > > +++ b/arch/riscv/Kconfig
> > > > > @@ -43,4 +43,12 @@ config 32BIT
> > > > >  config 64BIT
> > > > >       bool
> > > > > 
> > > > > +config NDS_V5
> > > > > +     bool "AndeStar V5 ISA support"
> > > > > +     def_bool n
> > > > 
> > > > You can use just "default n" instead of "def_bool n" here,
> > > > since
> > > > you
> > > > already have a "bool" above it.
> > > > 
> > > 
> > > I will reword it.
> > > 
> > > > > +     help
> > > > > +             Say Y here if you plan to run U-Boot on
> > > > > AndeStar v5
> > > > > +             platforms and use some specific features which
> > > > > are
> > > > > +             provided by Andes Technology AndeStar V5
> > > > > Families.
> > > > > +
> > > > >  endmenu
> > > > > diff --git a/arch/riscv/cpu/ax25/cpu.c
> > > > > b/arch/riscv/cpu/ax25/cpu.c
> > > > > index fddcc15..76689b2 100644
> > > > > --- a/arch/riscv/cpu/ax25/cpu.c
> > > > > +++ b/arch/riscv/cpu/ax25/cpu.c
> > > > > @@ -6,6 +6,7 @@
> > > > > 
> > > > >  /* CPU specific code */
> > > > >  #include <common.h>
> > > > > +#include <asm/cache.h>
> > > > > 
> > > > >  /*
> > > > >   * cleanup_before_linux() is called just before we call
> > > > > linux
> > > > > @@ -18,6 +19,9 @@ int cleanup_before_linux(void)
> > > > >       disable_interrupts();
> > > > > 
> > > > >       /* turn off I/D-cache */
> > > > > +     cache_flush();
> > > > 
> > > > I don't think you need to flush the data cache here. This
> > > > should be
> > > > handled by bootm_load_os() in common/bootm.c.
> > > > 
> > > 
> > > Do you mean
> > > flush_cache(flush_start, ALIGN(flush_len, ARCH_DMA_MINALIGN)); ?
> > > Ii is different cache API and implement.
> > > 
> > 
> > Yes I did mean flush_cache(), however I did not check if it is
> > implemented. Perhaps it's best to implement it and rely on it to
> > flush
> > the cache?
> > 
> > > If flush_cache is implemented, why you still add
> > > invalidate_icache_all(); in boot_jump_linux 0f bootm.c
> > > Do it have special reason ?
> > > 
> > 
> > RISC-V does not guarantee that stores to instruction memory are
> > visible
> > to instruction fetches (i.e. incoherent instruction caches). You
> > are
> > right, we do not need to flush the instruction cache explicitly to
> > make
> > the kernel visible to instruction fetches. We need it for a
> > separate
> > reason. Simplified, we do the following to start Linux.
> > 
> > kernel = kernel_address;
> > kernel();
> > 
> > Without a fence.i in between, we can not guarantee that kernel()
> > will
> > jump to the address specified in kernel_address. This is because
> > the
> > instruction cache is incoherent with the rest of the memory system,
> > it
> > must therefore be synchronized first.
> > This is the reason, why I added the instruction cache invalidation
> > to
> > the bootm code.
> > 
> 
> Yes
> I do agree with that i-cache invalidate is necessary before kernel()
> .
> 
> But U-Boot offer two cache hook function
> 
> the first one is as you said
> flush_cache(flush_start, ALIGN(flush_len, ARCH_DMA_MINALIGN));
> in common/bootm.c
> 
> the second one is
> cache_flush() which will be call in cleanup_before_linux()
> 
> and this two function can will be implemented as below in cache.c
> The both two also will be executed before kernel()
> 
> void cache_flush(void)
> {
>    invalidate_icache_all();
>    flush_dcache_all();
> }
> 
> void flush_cache(unsigned long addr, unsigned long size)
> {
>    invalidate_icache_all();
>    flush_dcache_all();
> }
> 
> But no mater what, I think separate them is a good idea.
> 

You are right, it's more to ensure new boards don't break if they
accidentally forget to include cache_flush() in cleanup_before_linux().

Thanks,
Lukas