Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function

[Sean Anderson <seanga2@gmail.com>]

On 7/28/21 3:25 AM, Zong Li wrote:
> 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?

Why it is present in both. Shouldn't it be present in some shared file?

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

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