From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48200C4338F for ; Fri, 30 Jul 2021 09:17:09 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A5A8461019 for ; Fri, 30 Jul 2021 09:17:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A5A8461019 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6F27E83255; Fri, 30 Jul 2021 11:17:05 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=sifive.com header.i=@sifive.com header.b="lZuVC2Xy"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 64EC783271; Fri, 30 Jul 2021 11:17:02 +0200 (CEST) Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3D63C83255 for ; Fri, 30 Jul 2021 11:16:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=zong.li@sifive.com Received: by mail-lj1-x22e.google.com with SMTP id l4so11455354ljq.4 for ; Fri, 30 Jul 2021 02:16:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=x9U8TZxtoQfqmmLAeMljBNRll2klcrnw9xXkLmuQ8H0=; b=lZuVC2XyCrMVkMlln6Q/iueI5saD4Pv1TQfa3fviJABwZK9lAUgkGg99/jPjPsjOlj aBxA99ZxpBUNWlHpWKrR0ejrE7ShtoD4DyMjtKLQwuxKpT1iCfwO9gGyiWM55pyE1K6f 0DmfU2iVmJoGFxBpR3DclXHmO1K6cDOvYixlhMarW8r6bdR+VpiZ11WJTOYV2nBOrRxf kqxYR8QJK9384rXzPqHDAV9MRTop+bsMME+8jIIYNE9ZJ4DsSG0mSID/gL4HF7GuqT+O iF6bLItQrkJZJ160BES1qXLfP7uEKqfE0YmKVKDQhwpf6Rs59fRCPUjIJKIhG/Ya09zZ c6dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=x9U8TZxtoQfqmmLAeMljBNRll2klcrnw9xXkLmuQ8H0=; b=UPGrOyXKuyr5MlaSQy4RAQg2M3/ubIcZstYlx+9RWKIjrgI7Cpka44wPBRgSW+pNXZ Nw1ZzeGamFP/qgIeVedW7w2ehEnNEFKy1XtbsJ42gBRBiqWW2UFGSrhkjFRAC+6GW+BZ C8hN+WSi+MOXHCIoJZXPclKq7kRB7FHeXWkIgHiSraOgH+OXIupIB25FDh1UIAvpnPl7 ySeXftoWBoKX6MkYSpWB83eAke831JXsKZYbWY+/dSsHT9s9bRiEE5FmFTC7Xs+pywQR qjIgzalW6n2PLpFOyJdEFejMAjAUfKjgmxfK10keClfsZqGzk1x4lA/HHfEFT5t277Ll 8Qww== X-Gm-Message-State: AOAM5336deXifMMYXKsXIeXxWlJQIrVSOytDSLWk+cxrNNxJ3iW7+va/ ilOisfbOOIz5XeTley3cXiUoIXG+mwoWBiC1T0NKqQ== X-Google-Smtp-Source: ABdhPJxSPLYfiYZX2+Ab3ilCiN68uc8DZLoDIrki8nNLsFlm974/uXYI1T8EoG3YUdl86sCD3uW8WgBSYARc0/fCBIU= X-Received: by 2002:a2e:8106:: with SMTP id d6mr1037278ljg.261.1627636617531; Fri, 30 Jul 2021 02:16:57 -0700 (PDT) MIME-Version: 1.0 References: <20210727085452.25708-1-zong.li@sifive.com> <20210727085452.25708-2-zong.li@sifive.com> In-Reply-To: From: Zong Li Date: Fri, 30 Jul 2021 17:16:46 +0800 Message-ID: Subject: Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function To: Sean Anderson Cc: rick@andestech.com, Leo Liang , Bin Meng , sjg@chromium.org, Green Wan , Paul Walmsley , u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Thu, Jul 29, 2021 at 8:26 PM Zong Li wrote: > > On Wed, Jul 28, 2021 at 11:18 PM Sean Anderson wrote: > > > > On 7/28/21 3:25 AM, Zong Li wrote: > > > On Wed, Jul 28, 2021 at 12:29 PM Sean Anderson 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 > > >>> --- > > >>> 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 > > >>> */ > > >>> > > >>> #include > > >>> -#include > > >>> -#include > > >>> -#include > > >>> +#include > > >>> +#include > > >>> > > >>> -/* 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 > > >>> -#include > > >>> -#include > > >>> -#include > > >>> +#include > > >>> +#include > > >>> > > >>> -/* 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 > >