[U-Boot] [PATCH v2] riscv: cache: Implement i/dcache [status, enable, disable]
Rick Chen
rickchen36 at gmail.com
Tue Nov 6 02:28:00 UTC 2018
Auer, Lukas <lukas.auer at aisec.fraunhofer.de> 於 2018年11月4日 週日 下午10:21寫道：
>
> Hi Rick,
>
> On Thu, 2018-11-01 at 12:08 +0800, Andes wrote:
> > From: Rick Chen <rick at andestech.com>
> >
> > AndeStar RISC-V(V5) provide mcache_ctl register which
> > can configure I/D cache as enabled or disabled.
> >
> > This CSR will be encapsulated by CONFIG_RISCV_NDS.
> > If you want to configure cache on AndeStar V5
> > AE350 platform. YOu can enable [*] AndeStar V5 ISA support
> > by make menuconfig.
> >
> > This approach also provide the expansion when the
> > vender specific features are going to join in.
> >
> > Signed-off-by: Rick Chen <rick at andestech.com>
> > Cc: Greentime Hu <greentime at andestech.com>
> > ---
> > arch/riscv/Kconfig | 8 ++++
> > arch/riscv/cpu/ax25/Makefile | 1 +
> > arch/riscv/cpu/ax25/cache.c | 89
> > ++++++++++++++++++++++++++++++++++++++++++
> > arch/riscv/cpu/ax25/cpu.c | 4 ++
> > arch/riscv/cpu/qemu/cpu.c | 2 +-
> > arch/riscv/cpu/start.S | 6 +++
> > arch/riscv/include/asm/cache.h | 9 +++++
> > arch/riscv/lib/cache.c | 30 +++++++++-----
> > 8 files changed, 138 insertions(+), 11 deletions(-)
> > create mode 100644 arch/riscv/cpu/ax25/cache.c
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 371921b..a356729 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -74,4 +74,12 @@ config 32BIT
> > config 64BIT
> > bool
> >
> > +config RISCV_NDS
> > + bool "AndeStar V5 ISA support"
> > + default n
> > + 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/Makefile
> > b/arch/riscv/cpu/ax25/Makefile
> > index 2ab0342..318bacc 100644
> > --- a/arch/riscv/cpu/ax25/Makefile
> > +++ b/arch/riscv/cpu/ax25/Makefile
> > @@ -4,3 +4,4 @@
> > # Rick Chen, Andes Technology Corporation <rick at andestech.com>
> >
> > obj-y := cpu.o
> > +obj-y += cache.o
> > diff --git a/arch/riscv/cpu/ax25/cache.c
> > b/arch/riscv/cpu/ax25/cache.c
> > new file mode 100644
> > index 0000000..e0bcaa2
> > --- /dev/null
> > +++ b/arch/riscv/cpu/ax25/cache.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + * Rick Chen, Andes Technology Corporation <rick at andestech.com>
> > + */
> > +
> > +#include <common.h>
> > +
> > +void icache_enable(void)
> > +{
> > +#ifndef CONFIG_SYS_ICACHE_OFF
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "ori t0, t1, 0x1\n\t"
> > + "csrw mcache_ctl, t0\n\t"
> > + );
> > +#endif
> > +#endif
> > +}
> > +
> > +void icache_disable(void)
>
> Just wondering, why do you not have the same #ifndef
> CONFIG_SYS_ICACHE_OFF as above here?
>
I only add CONFIG_SYS_XXX_OFF in enable function, not in disable funtion.
This can control cache enable or disable when u-boot start-up, on other reason.
If you care about this, I can add CONFIG_SYS_XXX_OFF for all.
> > +{
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "andi t0, t1, ~0x1\n\t"
> > + "csrw mcache_ctl, t0\n\t"
> > + );
> > +#endif
> > +}
> > +
> > +void dcache_enable(void)
> > +{
> > +#ifndef CONFIG_SYS_ICACHE_OFF
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "ori t0, t1, 0x2\n\t"
> > + "csrw mcache_ctl, t0\n\t"
> > + );
> > +#endif
> > +#endif
> > +}
> > +
> > +void dcache_disable(void)
> > +{
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "andi t0, t1, ~0x2\n\t"
> > + "csrw mcache_ctl, t0\n\t"
> > + );
> > +#endif
> > +}
> > +
> > +int icache_status(void)
> > +{
> > + int ret = 0;
> > +
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "andi %0, t1, 0x01\n\t"
> > + : "=r" (ret)
> > + :
> > + : "memory"
> > + );
> > +#endif
> > +
> > + return ret;
> > +}
> > +
> > +int dcache_status(void)
> > +{
> > + int ret = 0;
> > +
> > +#ifdef CONFIG_RISCV_NDS
> > + asm volatile (
> > + "csrr t1, mcache_ctl\n\t"
> > + "andi %0, t1, 0x02\n\t"
> > + : "=r" (ret)
> > + :
> > + : "memory"
> > + );
> > +#endif
> > +
> > + return ret;
> > +}
> > 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();
> > + icache_disable();
> > + dcache_disable();
>
> This is a separate change from the one described in the commit message,
> please move this into a new patch.
>
> I think we should do the same thing as ARM here. This is the
> implementation on armv8, for example. This sequence is chosen so that
> any new entries to the cache just before it is disabled get invalidated
> as well.
This is RISC-V not armv8.
/lib/cache.c is for RISC-V generic cache implement.
/cpu/ax25/cache.c is for Andes RISC-V cache implement.
You can add a platform which is armv8 relative as below
And modify the necessary flow as you want
/cpu/armv8/cache.c
>
> /*
> * Turn off I-cache and invalidate it
> */
> icache_disable();
> invalidate_icache_all();
>
> /*
> * turn off D-cache
> * dcache_disable() in turn flushes the d-cache and disables MMU
> */
> dcache_disable();
> invalidate_dcache_all();
>
> >
> > return 0;
> > }
> > diff --git a/arch/riscv/cpu/qemu/cpu.c b/arch/riscv/cpu/qemu/cpu.c
> > index 6c7a327..25d97d0 100644
> > --- a/arch/riscv/cpu/qemu/cpu.c
> > +++ b/arch/riscv/cpu/qemu/cpu.c
> > @@ -15,7 +15,7 @@ int cleanup_before_linux(void)
> > {
> > disable_interrupts();
> >
> > - /* turn off I/D-cache */
> > + cache_flush();
> >
> > return 0;
> > }
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 331a534..0e21679 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -46,6 +46,10 @@ _start:
> > /* mask all interrupts */
> > csrw mie, zero
> >
> > +/* Enable cache */
> > +jal icache_enable
> > +jal dcache_enable
> > +
>
> I think we should also define a enable_caches() function, like on ARM,
> to enable the caches. This works as well, but I think it makes sense to
> copy what other architectures do. What do you think?
>
Both coding style are exist in whole U-Boot
I can not find enable_caches be used in start.S
But it can be found
bl dcache_enable
in arch\powerpc\cpu\mpc86xx\start.S
So I prefer to not modify this flow.
Unless it indeed affect your platform and cause some error.
And I modify it as jal icache_enable the coding flow.
It is because you do not like CONFIG_SYS_ICACHE_OFF.
It may let you make some mistakes.
So I move CONFIG_SYS_ICACHE_OFF in /cpi/ax25/cache.c
> > /*
> > * Set stackpointer in internal/ex RAM to call board_init_f
> > */
> > @@ -181,6 +185,8 @@ clbss_l:
> > * initialization, now running from RAM.
> > */
> > call_board_init_r:
> > + jal invalidate_icache_all
> > + jal flush_dcache_all
>
> Is this required, perhaps this should be called from
> icache/dcache_enable?
Yes. It is required.
It can not be called to icache/dcache_enable
Cache shall be inval and flush after relocation.
>
> > la t0, board_init_r
> > mv t4, t0 /* offset of board_init_r()
> > */
> > add t4, t4, t6 /* real address of
> > board_init_r() */
> > diff --git a/arch/riscv/include/asm/cache.h
> > b/arch/riscv/include/asm/cache.h
> > index ca83dd6..e76ca13 100644
> > --- a/arch/riscv/include/asm/cache.h
> > +++ b/arch/riscv/include/asm/cache.h
> > @@ -7,6 +7,15 @@
> > #ifndef _ASM_RISCV_CACHE_H
> > #define _ASM_RISCV_CACHE_H
> >
> > +/* cache */
> > +int icache_status(void);
> > +void icache_enable(void);
> > +void icache_disable(void);
> > +int dcache_status(void);
> > +void dcache_enable(void);
> > +void dcache_disable(void);
>
> These are not required, include/common.h already has the function
> prototypes.
>
I will remove them from cache.h.
> > +void cache_flush(void);
> > +
> > /*
> > * The current upper bound for RISCV L1 data cache line sizes is 32
> > bytes.
> > * We use that value for aligning DMA buffers unless the board
> > config has
> > diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> > index d642a38..121db09 100644
> > --- a/arch/riscv/lib/cache.c
> > +++ b/arch/riscv/lib/cache.c
> > @@ -6,6 +6,15 @@
> >
> > #include <common.h>
> >
> > +void invalidate_icache_all(void)
> > +{
> > + asm volatile ("fence.i" ::: "memory");
> > +}
> > +
> > +void flush_dcache_all(void)
> > +{
> > + asm volatile("fence" :::"memory");
>
> nit: there should be a space after "volatile" and ":::" to match the
> style from invalidate_icache_all :)
I will modify it.
>
> > +}
> > void flush_dcache_range(unsigned long start, unsigned long end)
>
> Can you also implement the flush_dcache_range and flush_cache
> functions? We might run into unexpected behavior if we don't flush the
> cache here.
>
I do not modify it for the reason:
I separate one cache.c to
/lib/cache.c
/cpu/ax25/cache.c
I plan /lib/cache.c will be maintained by you
That is why I do not implement this two function.
If you said that I will implement flush_dcache_range and flush_cache in V2
> > {
> > }
> > @@ -19,41 +28,42 @@ void invalidate_icache_range(unsigned long start,
> > unsigned long end)
> > invalidate_icache_all();
> > }
> >
> > -void invalidate_icache_all(void)
> > +void invalidate_dcache_range(unsigned long start, unsigned long end)
> > {
> > - asm volatile ("fence.i" ::: "memory");
> > }
> >
> > -void invalidate_dcache_range(unsigned long start, unsigned long end)
> > +void cache_flush(void)
> > {
> > + invalidate_icache_all();
> > + flush_dcache_all();
> > }
> >
> > -void flush_cache(unsigned long addr, unsigned long size)
> > +__weak void flush_cache(unsigned long addr, unsigned long size)
>
> You are not overwriting this function, so it doesn't have to be defined
> as weak.
>
Actually I will have a
flush_cache(unsigned long addr, unsigned long size)
in /cpu/ax25/cache.c in the future.
And I declare it as weak in /lib/cache.c
So next time when I send a patch
/lib/cache.c can't be modified anymore.
Maybe it is not a good idea.
I will recovery it as not weak here in V2.
> Thanks,
> Lukas
>
> > {
> > }
> >
> > -void icache_enable(void)
> > +__weak void icache_enable(void)
> > {
> > }
> >
> > -void icache_disable(void)
> > +__weak void icache_disable(void)
> > {
> > }
> >
> > -int icache_status(void)
> > +__weak int icache_status(void)
> > {
> > return 0;
> > }
> >
> > -void dcache_enable(void)
> > +__weak void dcache_enable(void)
> > {
> > }
> >
> > -void dcache_disable(void)
> > +__weak void dcache_disable(void)
> > {
> > }
> >
> > -int dcache_status(void)
> > +__weak int dcache_status(void)
> > {
> > return 0;
> > }
More information about the U-Boot
mailing list