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 2/2] board: sifive: use ccache driver instead of helper function
Date: Wed, 28 Jul 2021 15:25:00 +0800	[thread overview]
Message-ID: <CANXhq0qEvHcBSS0MFPwn6xKGdZPVnfJCRCKUwX1FoSHRUwZmoA@mail.gmail.com> (raw)
In-Reply-To: <f25cc9e6-8a7e-5302-a751-310998b6f429@gmail.com>

On Wed, Jul 28, 2021 at 12:29 PM Sean Anderson <seanga2@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.li@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.patel@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?

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


> > -     }
> > -
> > -     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

  reply	other threads:[~2021-07-28  7:25 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27  8:54 [PATCH 1/2] cache: add sifive composable cache driver 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 [this message]
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

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=CANXhq0qEvHcBSS0MFPwn6xKGdZPVnfJCRCKUwX1FoSHRUwZmoA@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 2/2] board: sifive: use ccache driver instead of helper function' \
    /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.