Auer, Lukas <lukas.a...@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.a...@aisec.fraunhofer.de> 於 2018年10月27日 週六
> > 上午12:32寫道：
> > >
> > > Hi Rick,
> > >
> > > On Mon, 2018-10-22 at 16:16 +0800, Andes wrote:
> > > > From: Rick Chen <r...@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 <r...@andestech.com>
> > > > Cc: Greentime Hu <greent...@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.
> >
> > > > + icache_disable();
> > > > + dcache_disable();
> > > >
> > > > return 0;
> > > > }
> > > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > > > index 7cd7755..a8764df 100644
> > > > --- a/arch/riscv/cpu/start.S
> > > > +++ b/arch/riscv/cpu/start.S
> > > > @@ -51,6 +51,23 @@ handle_reset:
> > > > csrwi mstatus, 0
> > > > csrwi mie, 0
> > > >
> > > > +/* Enable cache */
> > > > +#ifndef CONFIG_SYS_ICACHE_OFF
> > > > +#ifdef CONFIG_NDS_V5
> > > > + csrr t1, mcache_ctl
> > > > + ori t0, t1, 0x1
> > > > + csrw mcache_ctl, t0
> > > > +#endif
> > > > +#endif
> > > > +
> > > > +#ifndef CONFIG_SYS_DCACHE_OFF
> > > > +#ifdef CONFIG_NDS_V5
> > > > + csrr t1, mcache_ctl
> > > > + ori t0, t1, 0x2
> > > > + csrw mcache_ctl, t0
> > > > +#endif
> > > > +#endif
> > > > +
> > > > /*
> > > > * Do CPU critical regs init only at reboot,
> > > > * not when booting from ram
> > > > @@ -191,6 +208,8 @@ clbss_l:
> > > > * initialization, now running from RAM.
> > > > */
> > > > call_board_init_r:
> > > > + jal invalidate_icache_all
> > > > + jal flush_dcache_all
> > > > 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);
> > > > +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 1d67c49..1837fb8 100644
> > > > --- a/arch/riscv/lib/cache.c
> > > > +++ b/arch/riscv/lib/cache.c
> > > > @@ -6,19 +6,39 @@
> > > >
> > > > #include <common.h>
> > > >
> > > > -void flush_dcache_range(unsigned long start, unsigned long end)
> > > > +#ifndef CONFIG_SYS_ICACHE_OFF
> > > > +void invalidate_icache_all(void)
> > > > {
> > > > + asm volatile("FENCE.I":::"memory");
> > > > }
> > >
> > > fence.i will likely still be needed even if the instruction cache
> > > is
> > > disabled. It should therefore not be removed using the #ifndef.
> > >
> >
> > I can remove here.
> > No matter i-cache is enabled or disabled, let invalidate_icache_all
> > still work with fence.i
> >
>
> I thought about this a bit more. We need invalidate_icache_all() to be
> always available, because there are systems, where the instruction
> cache can not be disabled. SiFive's U54-MC on the Freedom Unleashed
> board is an example of such a system. If CONFIG_SYS_ICACHE_OFF is
> defined by mistake, these systems might show unpredictable behavior.
>
> > > >
> > > > -void invalidate_icache_range(unsigned long start, unsigned long
> > > > end)
> > > > +void icache_enable(void)
> > > > {
> > > > +#ifndef CONFIG_SYS_ICACHE_OFF
> > >
> > > This #ifndef (also the ones below) is redundant since it is already
> > > tested above.
> > >
> >
> > I do not understand your words here.
> > If CONFIG_NDS_V5 is define, it still can be disabled by
> > CONFIG_SYS_ICACHE_OFF definition here.
> >
>
> I meant that the #ifndef CONFIG_SYS_ICACHE_OFF is redundant :)
>
> >
> > > > +#ifdef CONFIG_NDS_V5
> > > > + asm volatile (
> > > > + "csrr t1, mcache_ctl\n\t"
> > > > + "ori t0, t1, 0x1\n\t"
> > > > + "csrw mcache_ctl, t0\n\t"
> > > > + );
> > > > +#endif
> > > > +#endif
> > > > }
> > > >
> > >
> > > I think the cpu-specific functions should go into a separate file
> > > under
> > > cpu/ax25. In this case, all functions in this file (lib/cache.c)
> > > should
> > > be declared weak, so that they can be overwritten. In addition, we
> > > could also move the enable cache functions into that file. The
> > > functions could then be called, like with ARM, from board_init_r()
> > > by
> > > defining the function enable_caches / initr_caches.
> > > What do you think?
> > >
> >
> > It is a good advice.
> > I will try to separate it as you said.
> >
> > Rick
>
> Thanks!
>
> Lukas
>
> >
> > > Thanks,
> > > Lukas
> > >
> > > > -void invalidate_dcache_range(unsigned long start, unsigned long
> > > > end)
> > > > +void icache_disable(void)
> > > > {
> > > > +#ifndef CONFIG_SYS_ICACHE_OFF
> > > > +#ifdef CONFIG_NDS_V5
> > > > + asm volatile (
> > > > + "csrr t1, mcache_ctl\n\t"
> > > > + "andi t0, t1, ~0x1\n\t"
> > > > + "csrw mcache_ctl, t0\n\t"
> > > > + );
> > > > +#endif
> > > > +#endif
> > > > }
> > > > -
> > > > -void flush_cache(unsigned long addr, unsigned long size)
> > > > +#else
> > > > +void invalidate_icache_all(void)
> > > > {
> > > > }
> > > >
> > > > @@ -30,9 +50,44 @@ void icache_disable(void)
> > > > {
> > > > }
> > > >
> > > > -int icache_status(void)
> > > > +#endif
> > > > +
> > > > +#ifndef CONFIG_SYS_DCACHE_OFF
> > > > +void flush_dcache_all(void)
> > > > +{
> > > > + asm volatile("FENCE":::"memory");
> > > > +}
> > > > +
> > > > +void dcache_enable(void)
> > > > +{
> > > > +#ifndef CONFIG_SYS_DCACHE_OFF
> > > > +#ifdef CONFIG_NDS_V5
> > > > + 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)
> > > > +{
> > > > +#ifndef CONFIG_SYS_DCACHE_OFF
> > > > +#ifdef CONFIG_NDS_V5
> > > > + asm volatile (
> > > > + "csrr t1, mcache_ctl\n\t"
> > > > + "andi t0, t1, ~0x2\n\t"
> > > > + "csrw mcache_ctl, t0\n\t"
> > > > + );
> > > > +#endif
> > > > +#endif
> > > > +}
> > > > +
> > > > +
> > > > +#else
> > > > +void flush_dcache_all(void)
> > > > {
> > > > - return 0;
> > > > }
> > > >
> > > > void dcache_enable(void)
> > > > @@ -42,8 +97,68 @@ void dcache_enable(void)
> > > > void dcache_disable(void)
> > > > {
> > > > }
> > > > +#endif
> > > > +
> > > > +int icache_status(void)
> > > > +{
> > > > +#ifndef CONFIG_SYS_ICACHE_OFF
> > > > + int ret = 1;
> > > > +#else
> > > > + int ret = 0;
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_NDS_V5
> > > > + asm volatile (
> > > > + "csrr t1, mcache_ctl\n\t"
> > > > + "andi %0, t1, 0x01\n\t"
> > > > + : "=r" (ret)
> > > > + :
> > > > + : "memory"
> > > > + );
> > > > +#endif
> > > > +
> > > > + return ret;
> > > > +}
> > > >
> > > > int dcache_status(void)
> > > > {
> > > > - return 0;
> > > > +#ifndef CONFIG_SYS_DCACHE_OFF
> > > > + int ret = 1;
> > > > +#else
> > > > + int ret = 0;
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_NDS_V5
> > > > + asm volatile (
> > > > + "csrr t1, mcache_ctl\n\t"
> > > > + "andi %0, t1, 0x02\n\t"
> > > > + : "=r" (ret)
> > > > + :
> > > > + : "memory"
> > > > + );
> > > > +#endif
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +void invalidate_icache_range(unsigned long start, unsigned long
> > > > end)
> > > > +{
> > > > +}
> > > > +
> > > > +void invalidate_dcache_range(unsigned long start, unsigned long
> > > > end)
> > > > +{
> > > > +}
> > > > +
> > > > +void flush_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)
> > > > +{
> > > > }
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
