* [PATCH 1/2] cache: add sifive composable cache driver
@ 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:23 ` [PATCH 1/2] cache: add sifive composable cache driver Sean Anderson
0 siblings, 2 replies; 9+ messages in thread
From: Zong Li @ 2021-07-27 8:54 UTC (permalink / raw)
To: rick, ycliang, bmeng.cn, seanga2, sjg, green.wan, paul.walmsley, u-boot
Cc: Zong Li
This driver is currently responsible for enabling all ccache ways.
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
+ 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
+
+#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;
+
+ 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);
+}
+
+static const struct cache_ops sifive_ccache_ops = {
+ .enable = sifive_ccache_enable,
+};
+
+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;
+
+ 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,
+};
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] board: sifive: use ccache driver instead of helper function
2021-07-27 8:54 [PATCH 1/2] cache: add sifive composable cache driver Zong Li
@ 2021-07-27 8:54 ` Zong Li
2021-07-28 4:29 ` Sean Anderson
2021-07-28 4:23 ` [PATCH 1/2] cache: add sifive composable cache driver Sean Anderson
1 sibling, 1 reply; 9+ messages in thread
From: Zong Li @ 2021-07-27 8:54 UTC (permalink / raw)
To: rick, ycliang, bmeng.cn, seanga2, sjg, green.wan, paul.walmsley, u-boot
Cc: Zong Li
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)
{
- 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;
+ }
+
+ 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/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;
- }
-
- 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__);
- return ret;
- }
- return 0;
+ return sifive_ccache_enable_ways();
}
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] cache: add sifive composable cache driver
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:23 ` Sean Anderson
2021-07-28 10:40 ` Zong Li
1 sibling, 1 reply; 9+ messages in thread
From: Sean Anderson @ 2021-07-28 4:23 UTC (permalink / raw)
To: Zong Li, rick, ycliang, bmeng.cn, sjg, green.wan, paul.walmsley, u-boot
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.
>
> 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.
> + 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?
> +
> +#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?
> +
> + 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?
> +}
> +
> +static const struct cache_ops sifive_ccache_ops = {
> + .enable = sifive_ccache_enable,
Please implement get_info as well. It should effectively just be
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].
--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,
> +};
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function
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
0 siblings, 1 reply; 9+ messages in thread
From: Sean Anderson @ 2021-07-28 4:29 UTC (permalink / raw)
To: Zong Li, rick, ycliang, bmeng.cn, sjg, green.wan, paul.walmsley, u-boot
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.
> - 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)
> + }
> +
> + ret = cache_enable(dev);
> + if (ret) {
> + pr_debug("%s: ccache enable filed\n", __func__);
failed; ditto
> + return ret;
> + }
>
> - /* memory barrier */
> - mb();
> - (*enable) = ways - 1;
> - /* memory barrier */
> - mb();
Is this handled by the __iowmb/__iormb in writel/readl in the driver?
> 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
> - }
> -
> - 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
> - return ret;
> - }
> - return 0;
> + return sifive_ccache_enable_ways();
> }
>
--Sean
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function
2021-07-28 4:29 ` Sean Anderson
@ 2021-07-28 7:25 ` Zong Li
2021-07-28 15:18 ` Sean Anderson
0 siblings, 1 reply; 9+ messages in thread
From: Zong Li @ 2021-07-28 7:25 UTC (permalink / raw)
To: Sean Anderson
Cc: rick, Leo Liang, Bin Meng, sjg, Green Wan, Paul Walmsley, u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] cache: add sifive composable cache driver
2021-07-28 4:23 ` [PATCH 1/2] cache: add sifive composable cache driver Sean Anderson
@ 2021-07-28 10:40 ` Zong Li
0 siblings, 0 replies; 9+ messages in thread
From: Zong Li @ 2021-07-28 10:40 UTC (permalink / raw)
To: Sean Anderson
Cc: rick, Leo Liang, Bin Meng, sjg, Green Wan, Paul Walmsley, u-boot
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,
> > +};
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function
2021-07-28 7:25 ` Zong Li
@ 2021-07-28 15:18 ` Sean Anderson
2021-07-29 12:26 ` Zong Li
0 siblings, 1 reply; 9+ messages in thread
From: Sean Anderson @ 2021-07-28 15:18 UTC (permalink / raw)
To: Zong Li; +Cc: rick, Leo Liang, Bin Meng, sjg, Green Wan, Paul Walmsley, u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function
2021-07-28 15:18 ` Sean Anderson
@ 2021-07-29 12:26 ` Zong Li
2021-07-30 9:16 ` Zong Li
0 siblings, 1 reply; 9+ messages in thread
From: Zong Li @ 2021-07-29 12:26 UTC (permalink / raw)
To: Sean Anderson
Cc: rick, Leo Liang, Bin Meng, sjg, Green Wan, Paul Walmsley, u-boot
On Wed, Jul 28, 2021 at 11:18 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> 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?
>
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?
> >
> >>
> >>> - 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] board: sifive: use ccache driver instead of helper function
2021-07-29 12:26 ` Zong Li
@ 2021-07-30 9:16 ` Zong Li
0 siblings, 0 replies; 9+ messages in thread
From: Zong Li @ 2021-07-30 9:16 UTC (permalink / raw)
To: Sean Anderson
Cc: rick, Leo Liang, Bin Meng, sjg, Green Wan, Paul Walmsley, u-boot
On Thu, Jul 29, 2021 at 8:26 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Wed, Jul 28, 2021 at 11:18 PM Sean Anderson <seanga2@gmail.com> wrote:
> >
> > 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?
> >
>
> 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 <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
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-30 9:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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 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.