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

[Zong Li]

On Wed, Jul 28, 2021 at 12:23 PM Sean Anderson <seanga2 at gmail.com> wrote:
>
> 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.

Certainly, let me give more details there.

>
> >
> > Signed-off-by: Zong Li <zong.li at 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.

The idea is that the configuration name needs to be able to
distinguish multiple cache devices. For example, if there are other
two devices related to cache, we could give the following three
configurations:
 - SIFIVE_CACHE_CCACHE
 - SIFIVE_CACHE_XXX
 - SIFIVE_CACHE_YYY

SIFIVE_CCACHE is also ok to me, then we would get the following
configuration names in the future.
 - SIFIVE_CCACHE
 - SIFIVE_XXX
 - SIFIVE_YYY

>
> > +     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?

Changes in the next version.

>
> > +
> > +#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?
>

Yes, change it in the next version.

> > +
> > +     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?
>

sifive_ccache_enable isn't clear enough to me, we couldn't be
straightforward to know what to enable.

> > +}
> > +
> > +static const struct cache_ops sifive_ccache_ops = {
> > +     .enable = sifive_ccache_enable,
>
> Please implement get_info as well. It should effectively just be
>

Add get_info in the next version. Thanks.

> 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].
>

Thanks. Modify it in the next version.

> --Sean
>
> [1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#error-codes
>
> > +
> > +     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,
> > +};
> >
>