Re: [PATCH 1/2] cache: add sifive composable cache driver

On 7/27/21 4:54 AM, Zong Li wrote:

This driver is currently responsible for enabling all ccache ways.

Can you expand on this a little? Perhaps describe the hardware a little. For example, you could describe what a way/bank is, and that they can't be disabled by the hardware.

Signed-off-by: Zong Li zong.li@sifive.com

drivers/cache/Kconfig | 7 +++ drivers/cache/Makefile | 1 + drivers/cache/cache-sifive-ccache.c | 69 +++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 drivers/cache/cache-sifive-ccache.c

diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig index 1e452ad6d9..b903e3e935 100644 --- a/drivers/cache/Kconfig +++ b/drivers/cache/Kconfig @@ -39,4 +39,11 @@ config NCORE_CACHE controller. The driver initializes cache directories and coherent agent interfaces.

+config SIFIVE_CACHE_CCACHE

Just SIFIVE_CCACHE (or SIFIVE_CACHE) please.

• bool "SiFive composable cache"
• select CACHE
• help

•  This driver is for SiFive Composable L2/L3 cache. It enables cache
  

•  ways of composable cache.
  

• endmenu

diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile index fed50be3f9..92c6c5a83f 100644 --- a/drivers/cache/Makefile +++ b/drivers/cache/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_SANDBOX) += sandbox_cache.o obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o obj-$(CONFIG_NCORE_CACHE) += cache-ncore.o obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o +obj-$(CONFIG_SIFIVE_CACHE_CCACHE) += cache-sifive-ccache.o diff --git a/drivers/cache/cache-sifive-ccache.c b/drivers/cache/cache-sifive-ccache.c new file mode 100644 index 0000000000..9ea064912f --- /dev/null +++ b/drivers/cache/cache-sifive-ccache.c @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2021 SiFive
• */

+#include <common.h> +#include <cache.h> +#include <dm.h> +#include <asm/io.h> +#include <dm/device.h>

+#define SIFIVE_CCACHE_CONFIG 0x000 +#define SIFIVE_CCACHE_ENABLE 0x008

WAY_ENABLE?

+#define SIFIVE_CCACHE_NUM_WAY_MASK GENMASK(15, 8) +#define SIFIVE_CCACHE_NUM_WAY_SHIFT 8

+struct sifive_ccache {

• void __iomem *base;

+};

+static int sifive_ccache_enable_all_ways(struct udevice *dev) +{

• struct sifive_ccache *priv = dev_get_priv(dev);
• u32 config;
• u32 ways;
• config = readl(priv->base + SIFIVE_CCACHE_CONFIG);
• ways = (config & SIFIVE_CCACHE_NUM_WAY_MASK) >> SIFIVE_CCACHE_NUM_WAY_SHIFT;

ways = FIELD_GET(SIFIVE_CCACHE_NUM_WAY_MASK, config);

and perhaps this should be named SIFIVE_CCACHE_CONFIG_WAYS to better match the datasheet?

• writel(ways - 1, priv->base + SIFIVE_CCACHE_ENABLE);
• return 0;

+}

+static int sifive_ccache_enable(struct udevice *dev) +{

• return sifive_ccache_enable_all_ways(dev);

Any reason to have this in a separate function?

+}

+static const struct cache_ops sifive_ccache_ops = {

• .enable = sifive_ccache_enable,

Please implement get_info as well. It should effectively just be

get_info() { struct sifive_ccache *priv = dev_get_priv(dev); info->base = priv->base; return 0; }

+};

+static int sifive_ccache_probe(struct udevice *dev) +{

• struct sifive_ccache *priv = dev_get_priv(dev);
• priv->base = dev_read_addr_ptr(dev);
• if (!priv->base)

• return -ENODEV;
  

Please return -EINVAL instead [1].

--Sean

[1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#err...

• return 0;

+}

+static const struct udevice_id sifive_ccache_ids[] = {

• { .compatible = "sifive,fu540-c000-ccache" },
• { .compatible = "sifive,fu740-c000-ccache" },
• {}

+};

+U_BOOT_DRIVER(sifive_ccache) = {

• .name = "sifive_ccache",
• .id = UCLASS_CACHE,
• .of_match = sifive_ccache_ids,
• .probe = sifive_ccache_probe,
• .priv_auto = sizeof(struct sifive_ccache),
• .ops = &sifive_ccache_ops,

+};