All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zong Li <zong.li@sifive.com>
To: Sean Anderson <seanga2@gmail.com>
Cc: rick@andestech.com, Leo Liang <ycliang@andestech.com>,
	 Bin Meng <bmeng.cn@gmail.com>,
	sjg@chromium.org, Green Wan <green.wan@sifive.com>,
	 Paul Walmsley <paul.walmsley@sifive.com>,
	u-boot@lists.denx.de
Subject: Re: [PATCH 1/2] cache: add sifive composable cache driver
Date: Wed, 28 Jul 2021 18:40:59 +0800	[thread overview]
Message-ID: <CANXhq0p-WLXnAof=ipv6nS8uTPitpgZXBVHaPMAFDrQV0Rf3UQ@mail.gmail.com> (raw)
In-Reply-To: <b4907424-e160-14b7-9bec-a2ec7fb854dc@gmail.com>

On Wed, Jul 28, 2021 at 12:23 PM Sean Anderson <seanga2@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@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,
> > +};
> >
>

      reply	other threads:[~2021-07-28 10:41 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27  8:54 Zong Li
2021-07-27  8:54 ` [PATCH 2/2] board: sifive: use ccache driver instead of helper function Zong Li
2021-07-28  4:29   ` Sean Anderson
2021-07-28  7:25     ` Zong Li
2021-07-28 15:18       ` Sean Anderson
2021-07-29 12:26         ` Zong Li
2021-07-30  9:16           ` Zong Li
2021-07-28  4:23 ` [PATCH 1/2] cache: add sifive composable cache driver Sean Anderson
2021-07-28 10:40   ` Zong Li [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANXhq0p-WLXnAof=ipv6nS8uTPitpgZXBVHaPMAFDrQV0Rf3UQ@mail.gmail.com' \
    --to=zong.li@sifive.com \
    --cc=bmeng.cn@gmail.com \
    --cc=green.wan@sifive.com \
    --cc=paul.walmsley@sifive.com \
    --cc=rick@andestech.com \
    --cc=seanga2@gmail.com \
    --cc=sjg@chromium.org \
    --cc=u-boot@lists.denx.de \
    --cc=ycliang@andestech.com \
    --subject='Re: [PATCH 1/2] cache: add sifive composable cache driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.