On Thu, Jul 29, 2021 at 8:26 PM Zong Li <zong...@sifive.com> wrote: > > On Wed, Jul 28, 2021 at 11:18 PM Sean Anderson <sean...@gmail.com> wrote: > > > > On 7/28/21 3:25 AM, Zong Li wrote: > > > On Wed, Jul 28, 2021 at 12:29 PM Sean Anderson <sean...@gmail.com> wrote: > > >> > > >> On 7/27/21 4:54 AM, Zong Li wrote: > > >>> Invokes the generic cache_enable interface to execute the relative > > >>> implementation in SiFive ccache driver. > > >>> > > >>> Signed-off-by: Zong Li <zong...@sifive.com> > > >>> --- > > >>> arch/riscv/cpu/fu540/Kconfig | 1 + > > >>> arch/riscv/cpu/fu540/cache.c | 62 > > >>> ++++++++--------------- > > >>> arch/riscv/cpu/fu740/Kconfig | 1 + > > >>> arch/riscv/cpu/fu740/cache.c | 60 +++++++--------------- > > >>> arch/riscv/include/asm/arch-fu540/cache.h | 2 +- > > >>> arch/riscv/include/asm/arch-fu740/cache.h | 2 +- > > >>> board/sifive/unleashed/unleashed.c | 10 +--- > > >>> board/sifive/unmatched/unmatched.c | 9 +--- > > >>> 8 files changed, 45 insertions(+), 102 deletions(-) > > >>> > > >>> diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig > > >>> index 05463b2625..1f50b823ed 100644 > > >>> --- a/arch/riscv/cpu/fu540/Kconfig > > >>> +++ b/arch/riscv/cpu/fu540/Kconfig > > >>> @@ -19,6 +19,7 @@ config SIFIVE_FU540 > > >>> imply SMP > > >>> imply CLK_SIFIVE > > >>> imply CLK_SIFIVE_PRCI > > >>> + imply SIFIVE_CACHE_CCACHE > > >>> imply SIFIVE_SERIAL > > >>> imply MACB > > >>> imply MII > > >>> diff --git a/arch/riscv/cpu/fu540/cache.c b/arch/riscv/cpu/fu540/cache.c > > >>> index 0fc4ef6c00..3c754c4614 100644 > > >>> --- a/arch/riscv/cpu/fu540/cache.c > > >>> +++ b/arch/riscv/cpu/fu540/cache.c > > >>> @@ -1,55 +1,33 @@ > > >>> // SPDX-License-Identifier: GPL-2.0+ > > >>> /* > > >>> - * Copyright (C) 2020 SiFive, Inc > > >>> + * Copyright (C) 2020 - 2021 SiFive, Inc > > >>> * > > >>> * Authors: > > >>> * Pragnesh Patel <pragnesh.pa...@sifive.com> > > >>> */ > > >>> > > >>> #include <common.h> > > >>> -#include <asm/global_data.h> > > >>> -#include <asm/io.h> > > >>> -#include <linux/bitops.h> > > >>> +#include <cache.h> > > >>> +#include <dm.h> > > >>> > > >>> -/* Register offsets */ > > >>> -#define L2_CACHE_CONFIG 0x000 > > >>> -#define L2_CACHE_ENABLE 0x008 > > >>> - > > >>> -#define MASK_NUM_WAYS GENMASK(15, 8) > > >>> -#define NUM_WAYS_SHIFT 8 > > >>> - > > >>> -DECLARE_GLOBAL_DATA_PTR; > > >>> - > > >>> -int cache_enable_ways(void) > > >>> +int sifive_ccache_enable_ways(void) > > >>> { > > >> > > >> Is there any reason this function is duplicated? See below for further > > >> comments. > > > > > > Sorry, I don't completely understand about duplication here. Do you > > > mean why we need this function? or why is it present in both unleashed > > > and unmatched? > > > > Why it is present in both. Shouldn't it be present in some shared file? > > > > I considered that before, the places to put this ccache function might > be either 'arch/riscv/lib/' or 'arch/riscv/cpu/generic', these two > folders put some common stuff for all riscv platforms, but this ccache > function is actually used in sifive platform only, I'm not sure if it > is suitable to put the function into them, so I just followed the > original tree. OTOH, the compiler will drop the unused code, so it > might be OK to put it into the folders above, and not influence other > platforms. Do you have any preference or any recommendation? >
I'm going to add a cache_init() or a weak enable_caches() which is declared in cpu_func.h into 'arch/riscv/lib/cache.c' for the next version. > > > > > >> > > >>> - const void *blob = gd->fdt_blob; > > >>> - int node; > > >>> - fdt_addr_t base; > > >>> - u32 config; > > >>> - u32 ways; > > >>> - > > >>> - volatile u32 *enable; > > >>> - > > >>> - node = fdt_node_offset_by_compatible(blob, -1, > > >>> - "sifive,fu540-c000-ccache"); > > >>> - > > >>> - if (node < 0) > > >>> - return node; > > >>> - > > >>> - base = fdtdec_get_addr_size_auto_parent(blob, 0, node, "reg", 0, > > >>> - NULL, false); > > >>> - if (base == FDT_ADDR_T_NONE) > > >>> - return FDT_ADDR_T_NONE; > > >>> - > > >>> - config = readl((volatile u32 *)base + L2_CACHE_CONFIG); > > >>> - ways = (config & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT; > > >>> - > > >>> - enable = (volatile u32 *)(base + L2_CACHE_ENABLE); > > >>> + struct udevice *dev; > > >>> + int ret; > > >>> + > > >>> + ret = uclass_get_device_by_driver(UCLASS_CACHE, > > >>> + DM_DRIVER_GET(sifive_ccache), > > >>> + &dev); > > >>> + if (ret) { > > >>> + pr_debug("%s: could not enable cache ways\n", __func__); > > >>> + return ret; > > >>> + } > > >>> + > > >>> + ret = cache_enable(dev); > > >>> + if (ret) { > > >>> + pr_debug("%s: ccache enable filed\n", __func__); > > >>> + return ret; > > >>> + } > > >>> > > >>> - /* memory barrier */ > > >>> - mb(); > > >>> - (*enable) = ways - 1; > > >>> - /* memory barrier */ > > >>> - mb(); > > >>> return 0; > > >>> } > > >>> diff --git a/arch/riscv/cpu/fu740/Kconfig b/arch/riscv/cpu/fu740/Kconfig > > >>> index 8e54310b9c..87d8016c88 100644 > > >>> --- a/arch/riscv/cpu/fu740/Kconfig > > >>> +++ b/arch/riscv/cpu/fu740/Kconfig > > >>> @@ -19,6 +19,7 @@ config SIFIVE_FU740 > > >>> imply SMP > > >>> imply CLK_SIFIVE > > >>> imply CLK_SIFIVE_PRCI > > >>> + imply SIFIVE_CACHE_CCACHE > > >>> imply SIFIVE_SERIAL > > >>> imply MACB > > >>> imply MII > > >>> diff --git a/arch/riscv/cpu/fu740/cache.c b/arch/riscv/cpu/fu740/cache.c > > >>> index 680955c9e3..1d6c295d98 100644 > > >>> --- a/arch/riscv/cpu/fu740/cache.c > > >>> +++ b/arch/riscv/cpu/fu740/cache.c > > >>> @@ -7,49 +7,27 @@ > > >>> */ > > >>> > > >>> #include <common.h> > > >>> -#include <asm/io.h> > > >>> -#include <linux/bitops.h> > > >>> -#include <asm/global_data.h> > > >>> +#include <cache.h> > > >>> +#include <dm.h> > > >>> > > >>> -/* Register offsets */ > > >>> -#define L2_CACHE_CONFIG 0x000 > > >>> -#define L2_CACHE_ENABLE 0x008 > > >>> - > > >>> -#define MASK_NUM_WAYS GENMASK(15, 8) > > >>> -#define NUM_WAYS_SHIFT 8 > > >>> - > > >>> -DECLARE_GLOBAL_DATA_PTR; > > >>> - > > >>> -int cache_enable_ways(void) > > >>> +int sifive_ccache_enable_ways(void) > > >>> { > > >>> - const void *blob = gd->fdt_blob; > > >>> - int node; > > >>> - fdt_addr_t base; > > >>> - u32 config; > > >>> - u32 ways; > > >>> - > > >>> - volatile u32 *enable; > > >>> - > > >>> - node = fdt_node_offset_by_compatible(blob, -1, > > >>> - "sifive,fu740-c000-ccache"); > > >>> - > > >>> - if (node < 0) > > >>> - return node; > > >>> - > > >>> - base = fdtdec_get_addr_size_auto_parent(blob, 0, node, "reg", 0, > > >>> - NULL, false); > > >>> - if (base == FDT_ADDR_T_NONE) > > >>> - return FDT_ADDR_T_NONE; > > >>> - > > >>> - config = readl((volatile u32 *)base + L2_CACHE_CONFIG); > > >>> - ways = (config & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT; > > >>> - > > >>> - enable = (volatile u32 *)(base + L2_CACHE_ENABLE); > > >>> + struct udevice *dev; > > >>> + int ret; > > >>> + > > >>> + ret = uclass_get_device_by_driver(UCLASS_CACHE, > > >>> + DM_DRIVER_GET(sifive_ccache), > > >>> + &dev); > > >>> + if (ret) { > > >>> + pr_debug("%s: could not enable cache ways\n", __func__); > > >>> + return ret; > > >> > > >> return log_msg_ret(...); > > >> > > >> (and you don't have to add a __func__ parameter if you use that function) > > > > > > Okay, let me change the function. Thanks. > > > > > >> > > >>> + } > > >>> + > > >>> + ret = cache_enable(dev); > > >>> + if (ret) { > > >>> + pr_debug("%s: ccache enable filed\n", __func__); > > >> > > >> failed; ditto > > > > > > okay. > > > > > >> > > >>> + return ret; > > >>> + } > > >>> > > >>> - /* memory barrier */ > > >>> - mb(); > > >>> - (*enable) = ways - 1; > > >>> - /* memory barrier */ > > >>> - mb(); > > >> > > >> Is this handled by the __iowmb/__iormb in writel/readl in the driver? > > >> > > > > > > Yes, the ccache driver uses writel to write the register, and writel > > > will invoke __iowmb. > > > > > >>> return 0; > > >>> } > > >>> diff --git a/arch/riscv/include/asm/arch-fu540/cache.h > > >>> b/arch/riscv/include/asm/arch-fu540/cache.h > > >>> index 135a17c679..c252eb64d1 100644 > > >>> --- a/arch/riscv/include/asm/arch-fu540/cache.h > > >>> +++ b/arch/riscv/include/asm/arch-fu540/cache.h > > >>> @@ -9,6 +9,6 @@ > > >>> #ifndef _CACHE_SIFIVE_H > > >>> #define _CACHE_SIFIVE_H > > >>> > > >>> -int cache_enable_ways(void); > > >>> +int sifive_ccache_enable_ways(void); > > >>> > > >>> #endif /* _CACHE_SIFIVE_H */ > > >>> diff --git a/arch/riscv/include/asm/arch-fu740/cache.h > > >>> b/arch/riscv/include/asm/arch-fu740/cache.h > > >>> index 7d4fe9942b..8c456e3658 100644 > > >>> --- a/arch/riscv/include/asm/arch-fu740/cache.h > > >>> +++ b/arch/riscv/include/asm/arch-fu740/cache.h > > >>> @@ -9,6 +9,6 @@ > > >>> #ifndef _CACHE_SIFIVE_H > > >>> #define _CACHE_SIFIVE_H > > >>> > > >>> -int cache_enable_ways(void); > > >>> +int sifive_ccache_enable_ways(void); > > >>> > > >>> #endif /* _CACHE_SIFIVE_H */ > > >>> diff --git a/board/sifive/unleashed/unleashed.c > > >>> b/board/sifive/unleashed/unleashed.c > > >>> index 43027f0b54..12e61ec85f 100644 > > >>> --- a/board/sifive/unleashed/unleashed.c > > >>> +++ b/board/sifive/unleashed/unleashed.c > > >>> @@ -126,14 +126,6 @@ void *board_fdt_blob_setup(void) > > >>> > > >>> int board_init(void) > > >>> { > > >>> - int ret; > > >>> - > > >>> /* enable all cache ways */ > > >>> - ret = cache_enable_ways(); > > >>> - if (ret) { > > >>> - debug("%s: could not enable cache ways\n", __func__); > > >>> - return ret; > > >> > > >> ditto, though here I would just skip the error message since you > > >> reported it above already > > >> > > > > > > It seems to me that this part was dropped in this patch. > > > > Ah, whoops. Ignore this comment, then. > > > > > > > > > > >>> - } > > >>> - > > >>> - return 0; > > >>> + return sifive_ccache_enable_ways(); > > >>> } > > >>> diff --git a/board/sifive/unmatched/unmatched.c > > >>> b/board/sifive/unmatched/unmatched.c > > >>> index 2f5629b578..d27c4d3e88 100644 > > >>> --- a/board/sifive/unmatched/unmatched.c > > >>> +++ b/board/sifive/unmatched/unmatched.c > > >>> @@ -23,13 +23,6 @@ void *board_fdt_blob_setup(void) > > >>> > > >>> int board_init(void) > > >>> { > > >>> - int ret; > > >>> - > > >>> /* enable all cache ways */ > > >>> - ret = cache_enable_ways(); > > >>> - if (ret) { > > >>> - debug("%s: could not enable cache ways\n", __func__); > > >> > > >> ditto > > > > > > Okay. > > > > > >> > > >>> - return ret; > > >>> - } > > >>> - return 0; > > >>> + return sifive_ccache_enable_ways(); > > >>> } > > >>> > > >> > > >> --Sean > >