This patch series adds the SiFive Private L2 cache controller
driver and Performance Monitoring Unit (PMU) driver.
The Private L2 cache communicates with both the upstream L1
caches and downstream L3 cache or memory, enabling a high-
performance cache subsystem. It is also responsible for managing
requests from the L1 instruction and data caches of the core.
The Private L2 Performance Monitoring Unit (PMU) consists of a
set of event-programmable counters and their event selector registers.
The registers are available to control the behavior of the counters.
Eric Lin (2):
soc: sifive: Add SiFive private L2 cache support
dt-bindings: riscv: sifive: Add SiFive Private L2 cache controller
Greentime Hu (1):
soc: sifive: Add SiFive private L2 cache PMU driver
.../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++
drivers/soc/sifive/Kconfig | 17 +
drivers/soc/sifive/Makefile | 2 +
drivers/soc/sifive/sifive_pl2.h | 45 ++
drivers/soc/sifive/sifive_pl2_cache.c | 218 ++++++
drivers/soc/sifive/sifive_pl2_pmu.c | 669 ++++++++++++++++++
include/linux/cpuhotplug.h | 2 +
7 files changed, 1034 insertions(+)
create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
create mode 100644 drivers/soc/sifive/sifive_pl2.h
create mode 100644 drivers/soc/sifive/sifive_pl2_cache.c
create mode 100644 drivers/soc/sifive/sifive_pl2_pmu.c
--
2.40.1
This adds SiFive private L2 cache driver which will show
cache config information when booting and add cpu hotplug
callback functions.
Signed-off-by: Eric Lin <[email protected]>
Signed-off-by: Nick Hu <[email protected]>
Reviewed-by: Zong Li <[email protected]>
---
drivers/soc/sifive/Kconfig | 8 +
drivers/soc/sifive/Makefile | 1 +
drivers/soc/sifive/sifive_pl2.h | 25 ++++
drivers/soc/sifive/sifive_pl2_cache.c | 202 ++++++++++++++++++++++++++
include/linux/cpuhotplug.h | 1 +
5 files changed, 237 insertions(+)
create mode 100644 drivers/soc/sifive/sifive_pl2.h
create mode 100644 drivers/soc/sifive/sifive_pl2_cache.c
diff --git a/drivers/soc/sifive/Kconfig b/drivers/soc/sifive/Kconfig
index e86870be34c9..573564295058 100644
--- a/drivers/soc/sifive/Kconfig
+++ b/drivers/soc/sifive/Kconfig
@@ -7,4 +7,12 @@ config SIFIVE_CCACHE
help
Support for the composable cache controller on SiFive platforms.
+config SIFIVE_PL2
+ bool "Sifive private L2 Cache controller"
+ help
+ Support for the private L2 cache controller on SiFive platforms.
+ The SiFive Private L2 Cache Controller is per hart and communicates
+ with both the upstream L1 caches and downstream L3 cache or memory,
+ enabling a high-performance cache subsystem.
+
endif
diff --git a/drivers/soc/sifive/Makefile b/drivers/soc/sifive/Makefile
index 1f5dc339bf82..707493e1c691 100644
--- a/drivers/soc/sifive/Makefile
+++ b/drivers/soc/sifive/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_SIFIVE_CCACHE) += sifive_ccache.o
+obj-$(CONFIG_SIFIVE_PL2) += sifive_pl2_cache.o
diff --git a/drivers/soc/sifive/sifive_pl2.h b/drivers/soc/sifive/sifive_pl2.h
new file mode 100644
index 000000000000..57aa1019d5ed
--- /dev/null
+++ b/drivers/soc/sifive/sifive_pl2.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 SiFive, Inc.
+ *
+ */
+
+#ifndef _SIFIVE_PL2_H
+#define _SIFIVE_PL2_H
+
+#define SIFIVE_PL2_CONFIG1_OFFSET 0x1000
+#define SIFIVE_PL2_CONFIG0_OFFSET 0x1008
+#define SIFIVE_PL2_PMCLIENT_OFFSET 0x2800
+
+struct sifive_pl2_state {
+ void __iomem *pl2_base;
+ u32 config1;
+ u32 config0;
+ u64 pmclientfilter;
+};
+
+int sifive_pl2_pmu_init(void);
+int sifive_pl2_pmu_probe(struct device_node *pl2_node,
+ void __iomem *pl2_base, int cpu);
+
+#endif /*_SIFIVE_PL2_H */
diff --git a/drivers/soc/sifive/sifive_pl2_cache.c b/drivers/soc/sifive/sifive_pl2_cache.c
new file mode 100644
index 000000000000..aeb51d576af9
--- /dev/null
+++ b/drivers/soc/sifive/sifive_pl2_cache.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiFive private L2 cache controller Driver
+ *
+ * Copyright (C) 2018-2023 SiFive, Inc.
+ */
+
+#define pr_fmt(fmt) "pL2CACHE: " fmt
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/cpu_pm.h>
+#include <linux/cpuhotplug.h>
+#include "sifive_pl2.h"
+
+static DEFINE_PER_CPU(struct sifive_pl2_state, sifive_pl2_state);
+
+static void sifive_pl2_state_save(struct sifive_pl2_state *pl2_state)
+{
+ void __iomem *pl2_base = pl2_state->pl2_base;
+
+ if (!pl2_base)
+ return;
+
+ pl2_state->config1 = readl(pl2_base + SIFIVE_PL2_CONFIG1_OFFSET);
+ pl2_state->config0 = readl(pl2_base + SIFIVE_PL2_CONFIG0_OFFSET);
+ pl2_state->pmclientfilter = readq(pl2_base + SIFIVE_PL2_PMCLIENT_OFFSET);
+}
+
+static void sifive_pl2_state_restore(struct sifive_pl2_state *pl2_state)
+{
+ void __iomem *pl2_base = pl2_state->pl2_base;
+
+ if (!pl2_base)
+ return;
+
+ writel(pl2_state->config1, pl2_base + SIFIVE_PL2_CONFIG1_OFFSET);
+ writel(pl2_state->config0, pl2_base + SIFIVE_PL2_CONFIG0_OFFSET);
+ writeq(pl2_state->pmclientfilter, pl2_base + SIFIVE_PL2_PMCLIENT_OFFSET);
+}
+
+/*
+ * CPU Hotplug call back function
+ */
+static int sifive_pl2_online_cpu(unsigned int cpu)
+{
+ struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
+
+ sifive_pl2_state_restore(pl2_state);
+
+ return 0;
+}
+
+static int sifive_pl2_offline_cpu(unsigned int cpu)
+{
+ struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
+
+ /* Save the pl2 state */
+ sifive_pl2_state_save(pl2_state);
+
+ return 0;
+}
+
+/*
+ * PM notifer for suspend to ram
+ */
+#ifdef CONFIG_CPU_PM
+static int sifive_pl2_pm_notify(struct notifier_block *b, unsigned long cmd,
+ void *v)
+{
+ struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
+
+ switch (cmd) {
+ case CPU_PM_ENTER:
+ /* Save the pl2 state */
+ sifive_pl2_state_save(pl2_state);
+ break;
+ case CPU_PM_ENTER_FAILED:
+ case CPU_PM_EXIT:
+ sifive_pl2_state_restore(pl2_state);
+ break;
+ default:
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block sifive_pl2_pm_notifier_block = {
+ .notifier_call = sifive_pl2_pm_notify,
+};
+
+static inline void sifive_pl2_pm_init(void)
+{
+ cpu_pm_register_notifier(&sifive_pl2_pm_notifier_block);
+}
+
+#else
+static inline void sifive_pl2_pm_init(void) { }
+#endif /* CONFIG_CPU_PM */
+
+static const struct of_device_id sifive_pl2_cache_of_ids[] = {
+ { .compatible = "sifive,pL2Cache0" },
+ { .compatible = "sifive,pL2Cache1" },
+ { /* sentinel value */ }
+};
+
+static void pl2_config_read(void __iomem *pl2_base, int cpu)
+{
+ u32 regval, bank, way, set, cacheline;
+
+ regval = readl(pl2_base);
+ bank = regval & 0xff;
+ pr_info("in the CPU: %d\n", cpu);
+ pr_info("No. of Banks in the cache: %d\n", bank);
+ way = (regval & 0xff00) >> 8;
+ pr_info("No. of ways per bank: %d\n", way);
+ set = (regval & 0xff0000) >> 16;
+ pr_info("Total sets: %llu\n", (uint64_t)1 << set);
+ cacheline = (regval & 0xff000000) >> 24;
+ pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
+ pr_info("Size: %d\n", way << (set + cacheline));
+}
+
+static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ int cpu, ret = -EINVAL;
+ struct device_node *cpu_node, *pl2_node;
+ struct sifive_pl2_state *pl2_state = NULL;
+ void __iomem *pl2_base;
+
+ /* Traverse all cpu nodes to find the one mapping to its pl2 node. */
+ for_each_cpu(cpu, cpu_possible_mask) {
+ cpu_node = of_cpu_device_node_get(cpu);
+ pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
+
+ /* Found it! */
+ if (dev_of_node(&pdev->dev) == pl2_node) {
+ /* Use cpu to get its percpu data sifive_pl2_state. */
+ pl2_state = per_cpu_ptr(&sifive_pl2_state, cpu);
+ break;
+ }
+ }
+
+ if (!pl2_state) {
+ pr_err("Not found the corresponding cpu_node in dts.\n");
+ goto early_err;
+ }
+
+ /* Set base address of select and counter registers. */
+ pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+ if (IS_ERR(pl2_base)) {
+ ret = PTR_ERR(pl2_base);
+ goto early_err;
+ }
+
+ /* Print pL2 configs. */
+ pl2_config_read(pl2_base, cpu);
+ pl2_state->pl2_base = pl2_base;
+
+ return 0;
+
+early_err:
+ return ret;
+}
+
+static struct platform_driver sifive_pl2_cache_driver = {
+ .driver = {
+ .name = "SiFive-pL2-cache",
+ .of_match_table = sifive_pl2_cache_of_ids,
+ },
+ .probe = sifive_pl2_cache_dev_probe,
+};
+
+static int __init sifive_pl2_cache_init(void)
+{
+ int ret;
+
+ ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
+ "soc/sifive/pl2:online",
+ sifive_pl2_online_cpu,
+ sifive_pl2_offline_cpu);
+ if (ret < 0) {
+ pr_err("Failed to register CPU hotplug notifier %d\n", ret);
+ return ret;
+ }
+
+ ret = platform_driver_register(&sifive_pl2_cache_driver);
+ if (ret) {
+ pr_err("Failed to register sifive_pl2_cache_driver: %d\n", ret);
+ return ret;
+ }
+
+ sifive_pl2_pm_init();
+
+ return ret;
+}
+
+device_initcall(sifive_pl2_cache_init);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 0f1001dca0e0..35cd5ba0030b 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -207,6 +207,7 @@ enum cpuhp_state {
CPUHP_AP_IRQ_AFFINITY_ONLINE,
CPUHP_AP_BLK_MQ_ONLINE,
CPUHP_AP_ARM_MVEBU_SYNC_CLOCKS,
+ CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
CPUHP_AP_X86_INTEL_EPB_ONLINE,
CPUHP_AP_PERF_ONLINE,
CPUHP_AP_PERF_X86_ONLINE,
--
2.40.1
This add YAML DT binding documentation for SiFive Private L2
cache controller
Signed-off-by: Eric Lin <[email protected]>
Reviewed-by: Zong Li <[email protected]>
Reviewed-by: Nick Hu <[email protected]>
---
.../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
new file mode 100644
index 000000000000..b5d8d4a39dde
--- /dev/null
+++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) 2023 SiFive, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Private L2 Cache Controller
+
+maintainers:
+ - Greentime Hu <[email protected]>
+ - Eric Lin <[email protected]>
+
+description:
+ The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
+ L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
+ All the properties in ePAPR/DeviceTree specification applies for this platform.
+
+allOf:
+ - $ref: /schemas/cache-controller.yaml#
+
+select:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - sifive,pL2Cache0
+ - sifive,pL2Cache1
+
+ required:
+ - compatible
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - sifive,pL2Cache0
+ - sifive,pL2Cache1
+
+ cache-block-size:
+ const: 64
+
+ cache-level:
+ const: 2
+
+ cache-sets:
+ const: 512
+
+ cache-size:
+ const: 262144
+
+ cache-unified: true
+
+ reg:
+ maxItems: 1
+
+ next-level-cache: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - cache-block-size
+ - cache-level
+ - cache-sets
+ - cache-size
+ - cache-unified
+ - reg
+
+examples:
+ - |
+ pl2@10104000 {
+ compatible = "sifive,pL2Cache0";
+ cache-block-size = <64>;
+ cache-level = <2>;
+ cache-sets = <512>;
+ cache-size = <262144>;
+ cache-unified;
+ reg = <0x10104000 0x4000>;
+ next-level-cache = <&L4>;
+ };
--
2.40.1
Hey Eric,
On Fri, Jun 16, 2023 at 02:32:10PM +0800, Eric Lin wrote:
> This add YAML DT binding documentation for SiFive Private L2
> cache controller
>
> Signed-off-by: Eric Lin <[email protected]>
> Reviewed-by: Zong Li <[email protected]>
> Reviewed-by: Nick Hu <[email protected]>
Firstly, bindings need to come before the driver using them.
> ---
> .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
> 1 file changed, 81 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>
> diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> new file mode 100644
> index 000000000000..b5d8d4a39dde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
Cache bindings have moved to devicetree/bindings/cache.
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2023 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SiFive Private L2 Cache Controller
> +
> +maintainers:
> + - Greentime Hu <[email protected]>
> + - Eric Lin <[email protected]>
Drop the alignment here please.
> +
> +description:
> + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> + All the properties in ePAPR/DeviceTree specification applies for this platform.
Please wrap before 80 characters.
> +
> +allOf:
> + - $ref: /schemas/cache-controller.yaml#
> +
> +select:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - sifive,pL2Cache0
> + - sifive,pL2Cache1
Why is this select: required?
> + required:
> + - compatible
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - sifive,pL2Cache0
> + - sifive,pL2Cache1
What is the difference between these? (and drop the caps please)
Should this also not fall back to "cache"?
> +
> + cache-block-size:
> + const: 64
> +
> + cache-level:
> + const: 2
> +
> + cache-sets:
> + const: 512
> +
> + cache-size:
> + const: 262144
> +
> + cache-unified: true
> +
> + reg:
> + maxItems: 1
> +
> + next-level-cache: true
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - cache-block-size
> + - cache-level
> + - cache-sets
> + - cache-size
> + - cache-unified
> + - reg
> +
> +examples:
> + - |
> + pl2@10104000 {
cache-controller@
Cheers,
Conor.
> + compatible = "sifive,pL2Cache0";
> + cache-block-size = <64>;
> + cache-level = <2>;
> + cache-sets = <512>;
> + cache-size = <262144>;
> + cache-unified;
> + reg = <0x10104000 0x4000>;
> + next-level-cache = <&L4>;
> + };
> --
> 2.40.1
>
On 16/06/2023 08:32, Eric Lin wrote:
> This add YAML DT binding documentation for SiFive Private L2
> cache controller
>
> Signed-off-by: Eric Lin <[email protected]>
> Reviewed-by: Zong Li <[email protected]>
> Reviewed-by: Nick Hu <[email protected]>
> ---
> .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
> 1 file changed, 81 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>
> diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> new file mode 100644
> index 000000000000..b5d8d4a39dde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2023 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SiFive Private L2 Cache Controller
> +
> +maintainers:
> + - Greentime Hu <[email protected]>
> + - Eric Lin <[email protected]>
> +
> +description:
> + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> + All the properties in ePAPR/DeviceTree specification applies for this platform.
Drop the last sentence. Why specification would not apply?
> +
> +allOf:
> + - $ref: /schemas/cache-controller.yaml#
> +
> +select:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - sifive,pL2Cache0
> + - sifive,pL2Cache
> +
> + required:
> + - compatible
> +
> +properties:
> + compatible:
> + items:
You have only one item, so no need for items... unless you just missed
proper fallback.
> + - enum:
> + - sifive,pL2Cache0
> + - sifive,pL2Cache1
What is "0" and "1" here? What do these compatibles represent? Why they
do not have any SoC related part?
> +
> + cache-block-size:
> + const: 64
> +
> + cache-level:
> + const: 2
> +
> + cache-sets:
> + const: 512
> +
> + cache-size:
> + const: 262144
Are you sure? So all private L2 cache controllers will have fixed size
of cache?
> +
> + cache-unified: true
> +
> + reg:
> + maxItems: 1
> +
> + next-level-cache: true
> +
> +additionalProperties: false
> +
> +required:
required: goes before additionalProperties:.
Best regards,
Krzysztof
Le 16/06/2023 à 08:32, Eric Lin a écrit :
> This adds SiFive private L2 cache driver which will show
> cache config information when booting and add cpu hotplug
> callback functions.
>
> Signed-off-by: Eric Lin <[email protected]>
> Signed-off-by: Nick Hu <[email protected]>
> Reviewed-by: Zong Li <[email protected]>
[...]
> +static int __init sifive_pl2_cache_init(void)
> +{
> + int ret;
> +
> + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> + "soc/sifive/pl2:online",
> + sifive_pl2_online_cpu,
> + sifive_pl2_offline_cpu);
> + if (ret < 0) {
> + pr_err("Failed to register CPU hotplug notifier %d\n", ret);
> + return ret;
> + }
> +
> + ret = platform_driver_register(&sifive_pl2_cache_driver);
> + if (ret) {
> + pr_err("Failed to register sifive_pl2_cache_driver: %d\n", ret);
Blind guess: does cpuhp_remove_state() needs to be called?
> + return ret;
> + }
> +
> + sifive_pl2_pm_init();
> +
> + return ret;
If you send a v2, return 0; would be slighly nicer here.
CJ
> +}
[...]
Hey Eric,
On Fri, Jun 16, 2023 at 02:32:08PM +0800, Eric Lin wrote:
> This adds SiFive private L2 cache driver which will show
> cache config information when booting and add cpu hotplug
> callback functions.
>
> Signed-off-by: Eric Lin <[email protected]>
> Signed-off-by: Nick Hu <[email protected]>
Missing a Co-developed-by for Nick?
> +static void pl2_config_read(void __iomem *pl2_base, int cpu)
> +{
> + u32 regval, bank, way, set, cacheline;
> +
> + regval = readl(pl2_base);
> + bank = regval & 0xff;
> + pr_info("in the CPU: %d\n", cpu);
> + pr_info("No. of Banks in the cache: %d\n", bank);
> + way = (regval & 0xff00) >> 8;
> + pr_info("No. of ways per bank: %d\n", way);
> + set = (regval & 0xff0000) >> 16;
> + pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> + cacheline = (regval & 0xff000000) >> 24;
> + pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
> + pr_info("Size: %d\n", way << (set + cacheline));
> +}
Isn't this basically all information that we get anyway in sysfs based
on what gets put into the DT, except printed out once per CPU at
boottime?
If there's reason to keep it, please do as suggested by Ben and cut down
the number of lines emitted. Look at the ccache one for comparison:
static void ccache_config_read(void)
{
u32 cfg;
cfg = readl(ccache_base + SIFIVE_CCACHE_CONFIG);
pr_info("%llu banks, %llu ways, sets/bank=%llu, bytes/block=%llu\n",
FIELD_GET(SIFIVE_CCACHE_CONFIG_BANK_MASK, cfg),
FIELD_GET(SIFIVE_CCACHE_CONFIG_WAYS_MASK, cfg),
BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_SETS_MASK, cfg)),
BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_BLKS_MASK, cfg)));
cfg = readl(ccache_base + SIFIVE_CCACHE_WAYENABLE);
pr_info("Index of the largest way enabled: %u\n", cfg);
}
It'd also be good to print the same things as the ccache, no?
> +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + int cpu, ret = -EINVAL;
> + struct device_node *cpu_node, *pl2_node;
> + struct sifive_pl2_state *pl2_state = NULL;
> + void __iomem *pl2_base;
Please pick a sensible ordering for variables. IDC if it is reverse xmas
tree, or sorting by types, but this just seems quite random..
> + /* Traverse all cpu nodes to find the one mapping to its pl2 node. */
> + for_each_cpu(cpu, cpu_possible_mask) {
> + cpu_node = of_cpu_device_node_get(cpu);
> + pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
> +
> + /* Found it! */
> + if (dev_of_node(&pdev->dev) == pl2_node) {
> + /* Use cpu to get its percpu data sifive_pl2_state. */
> + pl2_state = per_cpu_ptr(&sifive_pl2_state, cpu);
> + break;
> + }
> + }
> +
> + if (!pl2_state) {
> + pr_err("Not found the corresponding cpu_node in dts.\n");
I don't think this error message is going to be helpful in figuring out
where the problem is on a machine with many of the caches. More
information about *which* cache caused it would be good.
Also it is not grammatically correct, it should read something like
"Failed to find CPU node for cache@abc" or something along those lines.
> + goto early_err;
early_err just returns ret. Why not just return the error directly?
> + }
> +
> + /* Set base address of select and counter registers. */
> + pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(pl2_base)) {
> + ret = PTR_ERR(pl2_base);
> + goto early_err;
> + }
> +
> + /* Print pL2 configs. */
> + pl2_config_read(pl2_base, cpu);
> + pl2_state->pl2_base = pl2_base;
> +
> + return 0;
> +
> +early_err:
> + return ret;
> +}
> +static struct platform_driver sifive_pl2_cache_driver = {
> + .driver = {
> + .name = "SiFive-pL2-cache",
> + .of_match_table = sifive_pl2_cache_of_ids,
> + },
> + .probe = sifive_pl2_cache_dev_probe,
> +};
> +
> +static int __init sifive_pl2_cache_init(void)
> +{
> + int ret;
> +
> + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> + "soc/sifive/pl2:online",
> + sifive_pl2_online_cpu,
> + sifive_pl2_offline_cpu);
Got some weird use of whitespace here & above, please remove the spaces.
Cheers,
Conor.
Hi Christophe,
On Sat, Jun 17, 2023 at 3:02 AM Christophe JAILLET
<[email protected]> wrote:
>
> Le 16/06/2023 à 08:32, Eric Lin a écrit :
> > This adds SiFive private L2 cache driver which will show
> > cache config information when booting and add cpu hotplug
> > callback functions.
> >
> > Signed-off-by: Eric Lin <[email protected]>
> > Signed-off-by: Nick Hu <[email protected]>
> > Reviewed-by: Zong Li <[email protected]>
>
> [...]
>
> > +static int __init sifive_pl2_cache_init(void)
> > +{
> > + int ret;
> > +
> > + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> > + "soc/sifive/pl2:online",
> > + sifive_pl2_online_cpu,
> > + sifive_pl2_offline_cpu);
> > + if (ret < 0) {
> > + pr_err("Failed to register CPU hotplug notifier %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = platform_driver_register(&sifive_pl2_cache_driver);
> > + if (ret) {
> > + pr_err("Failed to register sifive_pl2_cache_driver: %d\n", ret);
>
> Blind guess: does cpuhp_remove_state() needs to be called?
>
Yes, I'll fix this in v2. Thanks.
> > + return ret;
> > + }
> > +
> > + sifive_pl2_pm_init();
> > +
> > + return ret;
>
> If you send a v2, return 0; would be slighly nicer here.
>
OK, I'll fix it in v2.
Thanks for the review.
Best regards,
Eric Lin
> CJ
>
> > +}
>
> [...]
>
Hi Ben,
On Fri, Jun 16, 2023 at 4:30 PM Ben Dooks <[email protected]> wrote:
>
> On 16/06/2023 07:32, Eric Lin wrote:
> > This adds SiFive private L2 cache driver which will show
> > cache config information when booting and add cpu hotplug
> > callback functions.
> >
> > Signed-off-by: Eric Lin <[email protected]>
> > Signed-off-by: Nick Hu <[email protected]>
> > Reviewed-by: Zong Li <[email protected]>
> > ---
> > drivers/soc/sifive/Kconfig | 8 +
> > drivers/soc/sifive/Makefile | 1 +
> > drivers/soc/sifive/sifive_pl2.h | 25 ++++
> > drivers/soc/sifive/sifive_pl2_cache.c | 202 ++++++++++++++++++++++++++
> > include/linux/cpuhotplug.h | 1 +
> > 5 files changed, 237 insertions(+)
> > create mode 100644 drivers/soc/sifive/sifive_pl2.h
> > create mode 100644 drivers/soc/sifive/sifive_pl2_cache.c
> >
> > diff --git a/drivers/soc/sifive/Kconfig b/drivers/soc/sifive/Kconfig
> > index e86870be34c9..573564295058 100644
> > --- a/drivers/soc/sifive/Kconfig
> > +++ b/drivers/soc/sifive/Kconfig
> > @@ -7,4 +7,12 @@ config SIFIVE_CCACHE
> > help
> > Support for the composable cache controller on SiFive platforms.
> >
> > +config SIFIVE_PL2
> > + bool "Sifive private L2 Cache controller"
> > + help
> > + Support for the private L2 cache controller on SiFive platforms.
> > + The SiFive Private L2 Cache Controller is per hart and communicates
> > + with both the upstream L1 caches and downstream L3 cache or memory,
> > + enabling a high-performance cache subsystem.
> > +
> > endif
> > diff --git a/drivers/soc/sifive/Makefile b/drivers/soc/sifive/Makefile
> > index 1f5dc339bf82..707493e1c691 100644
> > --- a/drivers/soc/sifive/Makefile
> > +++ b/drivers/soc/sifive/Makefile
> > @@ -1,3 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> > obj-$(CONFIG_SIFIVE_CCACHE) += sifive_ccache.o
> > +obj-$(CONFIG_SIFIVE_PL2) += sifive_pl2_cache.o
> > diff --git a/drivers/soc/sifive/sifive_pl2.h b/drivers/soc/sifive/sifive_pl2.h
> > new file mode 100644
> > index 000000000000..57aa1019d5ed
> > --- /dev/null
> > +++ b/drivers/soc/sifive/sifive_pl2.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023 SiFive, Inc.
> > + *
> > + */
> > +
> > +#ifndef _SIFIVE_PL2_H
> > +#define _SIFIVE_PL2_H
> > +
> > +#define SIFIVE_PL2_CONFIG1_OFFSET 0x1000
> > +#define SIFIVE_PL2_CONFIG0_OFFSET 0x1008
> > +#define SIFIVE_PL2_PMCLIENT_OFFSET 0x2800
> > +
> > +struct sifive_pl2_state {
> > + void __iomem *pl2_base;
> > + u32 config1;
> > + u32 config0;
> > + u64 pmclientfilter;
> > +};
> > +
> > +int sifive_pl2_pmu_init(void);
> > +int sifive_pl2_pmu_probe(struct device_node *pl2_node,
> > + void __iomem *pl2_base, int cpu);
> > +
> > +#endif /*_SIFIVE_PL2_H */
> > diff --git a/drivers/soc/sifive/sifive_pl2_cache.c b/drivers/soc/sifive/sifive_pl2_cache.c
> > new file mode 100644
> > index 000000000000..aeb51d576af9
> > --- /dev/null
> > +++ b/drivers/soc/sifive/sifive_pl2_cache.c
> > @@ -0,0 +1,202 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SiFive private L2 cache controller Driver
> > + *
> > + * Copyright (C) 2018-2023 SiFive, Inc.
> > + */
> > +
> > +#define pr_fmt(fmt) "pL2CACHE: " fmt
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/cpu_pm.h>
> > +#include <linux/cpuhotplug.h>
> > +#include "sifive_pl2.h"
> > +
> > +static DEFINE_PER_CPU(struct sifive_pl2_state, sifive_pl2_state);
> > +
> > +static void sifive_pl2_state_save(struct sifive_pl2_state *pl2_state)
> > +{
> > + void __iomem *pl2_base = pl2_state->pl2_base;
> > +
> > + if (!pl2_base)
> > + return;
>
> is this test realy needed?
>
Yes,
The function cpuhp_setup_state() is called before sifive_pl2_cache_dev_probe().
When registering the CPU hotplug state, the kernel will issue the pl2
CPU hotplug callback.
However, the pl2_base is not yet being ioremap in sifive_pl2_cache_dev_probe().
Therefore, it is necessary to check pl2_base first to avoid such a scenario.
> > +
> > + pl2_state->config1 = readl(pl2_base + SIFIVE_PL2_CONFIG1_OFFSET);
> > + pl2_state->config0 = readl(pl2_base + SIFIVE_PL2_CONFIG0_OFFSET);
> > + pl2_state->pmclientfilter = readq(pl2_base + SIFIVE_PL2_PMCLIENT_OFFSET);
> > +}
> > +
> > +static void sifive_pl2_state_restore(struct sifive_pl2_state *pl2_state)
> > +{
> > + void __iomem *pl2_base = pl2_state->pl2_base;
> > +
> > + if (!pl2_base)
> > + return;
> > +
> > + writel(pl2_state->config1, pl2_base + SIFIVE_PL2_CONFIG1_OFFSET);
> > + writel(pl2_state->config0, pl2_base + SIFIVE_PL2_CONFIG0_OFFSET);
> > + writeq(pl2_state->pmclientfilter, pl2_base + SIFIVE_PL2_PMCLIENT_OFFSET);
> > +}
> > +
> > +/*
> > + * CPU Hotplug call back function
> > + */
> > +static int sifive_pl2_online_cpu(unsigned int cpu)
> > +{
> > + struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
> > +
> > + sifive_pl2_state_restore(pl2_state);
> > +
> > + return 0;
> > +}
> > +
> > +static int sifive_pl2_offline_cpu(unsigned int cpu)
> > +{
> > + struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
> > +
> > + /* Save the pl2 state */
> > + sifive_pl2_state_save(pl2_state);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * PM notifer for suspend to ram
> > + */
> > +#ifdef CONFIG_CPU_PM
> > +static int sifive_pl2_pm_notify(struct notifier_block *b, unsigned long cmd,
> > + void *v)
> > +{
> > + struct sifive_pl2_state *pl2_state = this_cpu_ptr(&sifive_pl2_state);
> > +
> > + switch (cmd) {
> > + case CPU_PM_ENTER:
> > + /* Save the pl2 state */
> > + sifive_pl2_state_save(pl2_state);
> > + break;
> > + case CPU_PM_ENTER_FAILED:
> > + case CPU_PM_EXIT:
> > + sifive_pl2_state_restore(pl2_state);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block sifive_pl2_pm_notifier_block = {
> > + .notifier_call = sifive_pl2_pm_notify,
> > +};
> > +
> > +static inline void sifive_pl2_pm_init(void)
> > +{
> > + cpu_pm_register_notifier(&sifive_pl2_pm_notifier_block);
> > +}
> > +
> > +#else
> > +static inline void sifive_pl2_pm_init(void) { }
> > +#endif /* CONFIG_CPU_PM */
> > +
> > +static const struct of_device_id sifive_pl2_cache_of_ids[] = {
> > + { .compatible = "sifive,pL2Cache0" },
> > + { .compatible = "sifive,pL2Cache1" },
>
> why the single cap here? I think that looks ugly.
>
OK, I'll fix it in v2.
> > + { /* sentinel value */ }
> > +};
> > +
> > +static void pl2_config_read(void __iomem *pl2_base, int cpu)
> > +{
> > + u32 regval, bank, way, set, cacheline;
> > +
> > + regval = readl(pl2_base);
> > + bank = regval & 0xff;
> > + pr_info("in the CPU: %d\n", cpu);
> > + pr_info("No. of Banks in the cache: %d\n", bank);
> > + way = (regval & 0xff00) >> 8;
> > + pr_info("No. of ways per bank: %d\n", way);
> > + set = (regval & 0xff0000) >> 16;
> > + pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> > + cacheline = (regval & 0xff000000) >> 24;
> > + pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
> > + pr_info("Size: %d\n", way << (set + cacheline));
>
>
> please either remove this or make it a single line, this is just going
> to spam the log with any system with more than one cpu core.
>
OK, I will make this log more simple in v2.
Thanks for the review.
Best Regards,
Eric Lin.
> > +}
> > +
> > +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + int cpu, ret = -EINVAL;
> > + struct device_node *cpu_node, *pl2_node;
> > + struct sifive_pl2_state *pl2_state = NULL;
> > + void __iomem *pl2_base;
> > +
> > + /* Traverse all cpu nodes to find the one mapping to its pl2 node. */
> > + for_each_cpu(cpu, cpu_possible_mask) {
> > + cpu_node = of_cpu_device_node_get(cpu);
> > + pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
> > +
> > + /* Found it! */
> > + if (dev_of_node(&pdev->dev) == pl2_node) {
> > + /* Use cpu to get its percpu data sifive_pl2_state. */
> > + pl2_state = per_cpu_ptr(&sifive_pl2_state, cpu);
> > + break;
> > + }
> > + }
> > +
> > + if (!pl2_state) {
> > + pr_err("Not found the corresponding cpu_node in dts.\n");
> > + goto early_err;
> > + }
> > +
> > + /* Set base address of select and counter registers. */
> > + pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(pl2_base)) {
> > + ret = PTR_ERR(pl2_base);
> > + goto early_err;
> > + }
> > +
> > + /* Print pL2 configs. */
> > + pl2_config_read(pl2_base, cpu);
> > + pl2_state->pl2_base = pl2_base;
> > +
> > + return 0;
> > +
> > +early_err:
> > + return ret;
> > +}
> > +
> > +static struct platform_driver sifive_pl2_cache_driver = {
> > + .driver = {
> > + .name = "SiFive-pL2-cache",
> > + .of_match_table = sifive_pl2_cache_of_ids,
> > + },
> > + .probe = sifive_pl2_cache_dev_probe,
> > +};
> > +
> > +static int __init sifive_pl2_cache_init(void)
> > +{
> > + int ret;
> > +
> > + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> > + "soc/sifive/pl2:online",
> > + sifive_pl2_online_cpu,
> > + sifive_pl2_offline_cpu);
> > + if (ret < 0) {
> > + pr_err("Failed to register CPU hotplug notifier %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = platform_driver_register(&sifive_pl2_cache_driver);
> > + if (ret) {
> > + pr_err("Failed to register sifive_pl2_cache_driver: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + sifive_pl2_pm_init();
> > +
> > + return ret;
> > +}
> > +
> > +device_initcall(sifive_pl2_cache_init);
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index 0f1001dca0e0..35cd5ba0030b 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -207,6 +207,7 @@ enum cpuhp_state {
> > CPUHP_AP_IRQ_AFFINITY_ONLINE,
> > CPUHP_AP_BLK_MQ_ONLINE,
> > CPUHP_AP_ARM_MVEBU_SYNC_CLOCKS,
> > + CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> > CPUHP_AP_X86_INTEL_EPB_ONLINE,
> > CPUHP_AP_PERF_ONLINE,
> > CPUHP_AP_PERF_X86_ONLINE,
>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html
>
Hi Conor,
On Sat, Jun 17, 2023 at 5:05 AM Conor Dooley <[email protected]> wrote:
>
> Hey Eric,
>
> On Fri, Jun 16, 2023 at 02:32:08PM +0800, Eric Lin wrote:
> > This adds SiFive private L2 cache driver which will show
> > cache config information when booting and add cpu hotplug
> > callback functions.
> >
> > Signed-off-by: Eric Lin <[email protected]>
> > Signed-off-by: Nick Hu <[email protected]>
>
> Missing a Co-developed-by for Nick?
Yes, I'll add Co-developed-by for Nick in v2. Thanks.
>
>
> > +static void pl2_config_read(void __iomem *pl2_base, int cpu)
> > +{
> > + u32 regval, bank, way, set, cacheline;
> > +
> > + regval = readl(pl2_base);
> > + bank = regval & 0xff;
> > + pr_info("in the CPU: %d\n", cpu);
> > + pr_info("No. of Banks in the cache: %d\n", bank);
> > + way = (regval & 0xff00) >> 8;
> > + pr_info("No. of ways per bank: %d\n", way);
> > + set = (regval & 0xff0000) >> 16;
> > + pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> > + cacheline = (regval & 0xff000000) >> 24;
> > + pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
> > + pr_info("Size: %d\n", way << (set + cacheline));
> > +}
>
> Isn't this basically all information that we get anyway in sysfs based
> on what gets put into the DT, except printed out once per CPU at
> boottime?
> If there's reason to keep it, please do as suggested by Ben and cut down
> the number of lines emitted. Look at the ccache one for comparison:
> static void ccache_config_read(void)
> {
> u32 cfg;
>
> cfg = readl(ccache_base + SIFIVE_CCACHE_CONFIG);
> pr_info("%llu banks, %llu ways, sets/bank=%llu, bytes/block=%llu\n",
> FIELD_GET(SIFIVE_CCACHE_CONFIG_BANK_MASK, cfg),
> FIELD_GET(SIFIVE_CCACHE_CONFIG_WAYS_MASK, cfg),
> BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_SETS_MASK, cfg)),
> BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_BLKS_MASK, cfg)));
>
> cfg = readl(ccache_base + SIFIVE_CCACHE_WAYENABLE);
> pr_info("Index of the largest way enabled: %u\n", cfg);
> }
> It'd also be good to print the same things as the ccache, no?
>
Yes, I'll cut down the number of lines as the ccache in v2. Thanks for
the suggestion.
> > +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + int cpu, ret = -EINVAL;
> > + struct device_node *cpu_node, *pl2_node;
> > + struct sifive_pl2_state *pl2_state = NULL;
> > + void __iomem *pl2_base;
>
> Please pick a sensible ordering for variables. IDC if it is reverse xmas
> tree, or sorting by types, but this just seems quite random..
>
Yes, I'll sort by type in v2.
> > + /* Traverse all cpu nodes to find the one mapping to its pl2 node. */
> > + for_each_cpu(cpu, cpu_possible_mask) {
> > + cpu_node = of_cpu_device_node_get(cpu);
> > + pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
> > +
> > + /* Found it! */
> > + if (dev_of_node(&pdev->dev) == pl2_node) {
> > + /* Use cpu to get its percpu data sifive_pl2_state. */
> > + pl2_state = per_cpu_ptr(&sifive_pl2_state, cpu);
> > + break;
> > + }
> > + }
> > +
> > + if (!pl2_state) {
> > + pr_err("Not found the corresponding cpu_node in dts.\n");
>
> I don't think this error message is going to be helpful in figuring out
> where the problem is on a machine with many of the caches. More
> information about *which* cache caused it would be good.
> Also it is not grammatically correct, it should read something like
> "Failed to find CPU node for cache@abc" or something along those lines.
>
OK, I'll rewrite the error message to make it more helpful for the user.
I'll fix it in v2. Thanks for the suggestion.
> > + goto early_err;
>
> early_err just returns ret. Why not just return the error directly?
>
Yeah, it can just return ret. I'll fix it in v2.
> > + }
> > +
> > + /* Set base address of select and counter registers. */
> > + pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(pl2_base)) {
> > + ret = PTR_ERR(pl2_base);
> > + goto early_err;
> > + }
> > +
> > + /* Print pL2 configs. */
> > + pl2_config_read(pl2_base, cpu);
> > + pl2_state->pl2_base = pl2_base;
> > +
> > + return 0;
> > +
> > +early_err:
> > + return ret;
> > +}
>
> > +static struct platform_driver sifive_pl2_cache_driver = {
> > + .driver = {
> > + .name = "SiFive-pL2-cache",
> > + .of_match_table = sifive_pl2_cache_of_ids,
> > + },
> > + .probe = sifive_pl2_cache_dev_probe,
> > +};
> > +
> > +static int __init sifive_pl2_cache_init(void)
> > +{
> > + int ret;
> > +
> > + ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> > + "soc/sifive/pl2:online",
> > + sifive_pl2_online_cpu,
> > + sifive_pl2_offline_cpu);
>
> Got some weird use of whitespace here & above, please remove the spaces.
>
Yes, I'll remove the whitespace in v2.
Thanks for the review.
Best Regards,
Eric Lin.
> Cheers,
> Conor.
Hi Conor,
On Fri, Jun 16, 2023 at 6:12 PM Conor Dooley <[email protected]> wrote:
>
> Hey Eric,
>
> On Fri, Jun 16, 2023 at 02:32:10PM +0800, Eric Lin wrote:
> > This add YAML DT binding documentation for SiFive Private L2
> > cache controller
> >
> > Signed-off-by: Eric Lin <[email protected]>
> > Reviewed-by: Zong Li <[email protected]>
> > Reviewed-by: Nick Hu <[email protected]>
>
> Firstly, bindings need to come before the driver using them.
>
OK, I'll fix it in v2.
> > ---
> > .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> > new file mode 100644
> > index 000000000000..b5d8d4a39dde
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>
> Cache bindings have moved to devicetree/bindings/cache.
>
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (C) 2023 SiFive, Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SiFive Private L2 Cache Controller
> > +
> > +maintainers:
> > + - Greentime Hu <[email protected]>
> > + - Eric Lin <[email protected]>
>
> Drop the alignment here please.
>
OK, I'll fix it in v2.
> > +
> > +description:
> > + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> > + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> > + All the properties in ePAPR/DeviceTree specification applies for this platform.
>
> Please wrap before 80 characters.
>
OK, I'll fix it in v2.
> > +
> > +allOf:
> > + - $ref: /schemas/cache-controller.yaml#
> > +
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - sifive,pL2Cache0
> > + - sifive,pL2Cache1
>
> Why is this select: required?
>
OK, I'll fix it in v2.
> > + required:
> > + - compatible
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - sifive,pL2Cache0
> > + - sifive,pL2Cache1
>
> What is the difference between these? (and drop the caps please)
The pL2Cache1 has minor changes in hardware, but it can use the same
pl2 cache driver.
OK, I'll drop the caps in v2.
>
> Should this also not fall back to "cache"?
>
Yes, I'll fix it in v2.
> > +
> > + cache-block-size:
> > + const: 64
> > +
> > + cache-level:
> > + const: 2
> > +
> > + cache-sets:
> > + const: 512
> > +
> > + cache-size:
> > + const: 262144
> > +
> > + cache-unified: true
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + next-level-cache: true
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - cache-block-size
> > + - cache-level
> > + - cache-sets
> > + - cache-size
> > + - cache-unified
> > + - reg
> > +
> > +examples:
> > + - |
> > + pl2@10104000 {
>
> cache-controller@
>
OK, I'll fix it in v2.
Thanks for the review.
Best Regards,
Eric Lin.
> Cheers,
> Conor.
>
> > + compatible = "sifive,pL2Cache0";
> > + cache-block-size = <64>;
> > + cache-level = <2>;
> > + cache-sets = <512>;
> > + cache-size = <262144>;
> > + cache-unified;
> > + reg = <0x10104000 0x4000>;
> > + next-level-cache = <&L4>;
> > + };
> > --
> > 2.40.1
> >
Hi Krzysztof,
On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 16/06/2023 08:32, Eric Lin wrote:
> > This add YAML DT binding documentation for SiFive Private L2
> > cache controller
> >
> > Signed-off-by: Eric Lin <[email protected]>
> > Reviewed-by: Zong Li <[email protected]>
> > Reviewed-by: Nick Hu <[email protected]>
> > ---
> > .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> > new file mode 100644
> > index 000000000000..b5d8d4a39dde
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (C) 2023 SiFive, Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SiFive Private L2 Cache Controller
> > +
> > +maintainers:
> > + - Greentime Hu <[email protected]>
> > + - Eric Lin <[email protected]>
> > +
> > +description:
> > + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> > + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> > + All the properties in ePAPR/DeviceTree specification applies for this platform.
>
> Drop the last sentence. Why specification would not apply?
>
OK, I'll drop it in v2.
> > +
> > +allOf:
> > + - $ref: /schemas/cache-controller.yaml#
> > +
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - sifive,pL2Cache0
> > + - sifive,pL2Cache
> > +
> > + required:
> > + - compatible
> > +
> > +properties:
> > + compatible:
> > + items:
>
>
> You have only one item, so no need for items... unless you just missed
> proper fallback.
OK, I'll fix it in v2.
>
> > + - enum:
> > + - sifive,pL2Cache0
> > + - sifive,pL2Cache1
>
> What is "0" and "1" here? What do these compatibles represent? Why they
> do not have any SoC related part?
The pL2Cache1 has minor changes in hardware, but it can use the same
pl2 cache driver.
May I ask, what do you mean about the SoC-related part? Thanks.
>
> > +
> > + cache-block-size:
> > + const: 64
> > +
> > + cache-level:
> > + const: 2
> > +
> > + cache-sets:
> > + const: 512
> > +
> > + cache-size:
> > + const: 262144
>
> Are you sure? So all private L2 cache controllers will have fixed size
> of cache?
The private L2 cache controllers will have different sizes.
OK, I'll fix in v2.
>
> > +
> > + cache-unified: true
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + next-level-cache: true
> > +
> > +additionalProperties: false
> > +
> > +required:
>
> required: goes before additionalProperties:.
>
OK, I'll fix it in v2.
Thanks for the review.
Best Regards,
Eric Lin.
>
> Best regards,
> Krzysztof
>
On 26/06/2023 05:26, Eric Lin wrote:
> Hi Krzysztof,
>
> On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 16/06/2023 08:32, Eric Lin wrote:
>>> This add YAML DT binding documentation for SiFive Private L2
>>> cache controller
>>>
>>> Signed-off-by: Eric Lin <[email protected]>
>>> Reviewed-by: Zong Li <[email protected]>
>>> Reviewed-by: Nick Hu <[email protected]>
>>> ---
>>> .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
>>> 1 file changed, 81 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>>> new file mode 100644
>>> index 000000000000..b5d8d4a39dde
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>>> @@ -0,0 +1,81 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +# Copyright (C) 2023 SiFive, Inc.
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: SiFive Private L2 Cache Controller
>>> +
>>> +maintainers:
>>> + - Greentime Hu <[email protected]>
>>> + - Eric Lin <[email protected]>
>>> +
>>> +description:
>>> + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
>>> + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
>>> + All the properties in ePAPR/DeviceTree specification applies for this platform.
>>
>> Drop the last sentence. Why specification would not apply?
>>
> OK, I'll drop it in v2.
>
>>> +
>>> +allOf:
>>> + - $ref: /schemas/cache-controller.yaml#
>>> +
>>> +select:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - sifive,pL2Cache0
>>> + - sifive,pL2Cache
>>> +
>>> + required:
>>> + - compatible
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>
>>
>> You have only one item, so no need for items... unless you just missed
>> proper fallback.
>
> OK, I'll fix it in v2.
>
>>
>>> + - enum:
>>> + - sifive,pL2Cache0
>>> + - sifive,pL2Cache1
>>
>> What is "0" and "1" here? What do these compatibles represent? Why they
>> do not have any SoC related part?
>
> The pL2Cache1 has minor changes in hardware, but it can use the same
> pl2 cache driver.
Then why aren't they compatible?
> May I ask, what do you mean about the SoC-related part? Thanks.
This is part of a SoC, right? We expect SoC blocks to have compatible
based on the SoC.
Best regards,
Krzysztof
Hi Krzysztof,
On Mon, Jun 26, 2023 at 2:19 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 26/06/2023 05:26, Eric Lin wrote:
> > Hi Krzysztof,
> >
> > On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 16/06/2023 08:32, Eric Lin wrote:
> >>> This add YAML DT binding documentation for SiFive Private L2
> >>> cache controller
> >>>
> >>> Signed-off-by: Eric Lin <[email protected]>
> >>> Reviewed-by: Zong Li <[email protected]>
> >>> Reviewed-by: Nick Hu <[email protected]>
> >>> ---
> >>> .../bindings/riscv/sifive,pL2Cache0.yaml | 81 +++++++++++++++++++
> >>> 1 file changed, 81 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >>> new file mode 100644
> >>> index 000000000000..b5d8d4a39dde
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >>> @@ -0,0 +1,81 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +# Copyright (C) 2023 SiFive, Inc.
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: SiFive Private L2 Cache Controller
> >>> +
> >>> +maintainers:
> >>> + - Greentime Hu <[email protected]>
> >>> + - Eric Lin <[email protected]>
> >>> +
> >>> +description:
> >>> + The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> >>> + L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> >>> + All the properties in ePAPR/DeviceTree specification applies for this platform.
> >>
> >> Drop the last sentence. Why specification would not apply?
> >>
> > OK, I'll drop it in v2.
> >
> >>> +
> >>> +allOf:
> >>> + - $ref: /schemas/cache-controller.yaml#
> >>> +
> >>> +select:
> >>> + properties:
> >>> + compatible:
> >>> + contains:
> >>> + enum:
> >>> + - sifive,pL2Cache0
> >>> + - sifive,pL2Cache
> >>> +
> >>> + required:
> >>> + - compatible
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + items:
> >>
> >>
> >> You have only one item, so no need for items... unless you just missed
> >> proper fallback.
> >
> > OK, I'll fix it in v2.
> >
> >>
> >>> + - enum:
> >>> + - sifive,pL2Cache0
> >>> + - sifive,pL2Cache1
> >>
> >> What is "0" and "1" here? What do these compatibles represent? Why they
> >> do not have any SoC related part?
> >
> > The pL2Cache1 has minor changes in hardware, but it can use the same
> > pl2 cache driver.
>
> Then why aren't they compatible?
>
The pL2Cache1 has removed some unused bits in the register compared to
pl2Cache0.
From the hardware perspective, they are not compatible but they can
share the same pl2 cache driver in software.
Thus, we would like to keep both. It would be great if you can provide
some suggestions. Thanks.
Best Regards,
Eric Lin.
> > May I ask, what do you mean about the SoC-related part? Thanks.
>
> This is part of a SoC, right? We expect SoC blocks to have compatible
> based on the SoC.
>
>
>
> Best regards,
> Krzysztof
>
On 28/06/2023 18:31, Eric Lin wrote:
>>>>
>>>>> + - enum:
>>>>> + - sifive,pL2Cache0
>>>>> + - sifive,pL2Cache1
>>>>
>>>> What is "0" and "1" here? What do these compatibles represent? Why they
>>>> do not have any SoC related part?
>>>
>>> The pL2Cache1 has minor changes in hardware, but it can use the same
>>> pl2 cache driver.
>>
>> Then why aren't they compatible?
>>
>
> The pL2Cache1 has removed some unused bits in the register compared to
> pl2Cache0.
> From the hardware perspective, they are not compatible but they can
> share the same pl2 cache driver in software.
So they are compatible... If they were not compatible, you wouldn't be
able to use the same match in the driver.
> Thus, we would like to keep both. It would be great if you can provide
> some suggestions. Thanks.
I propose to make them compatible, like every other piece of SoC. I
don't see any benefit of having them separate.
Best regards,
Krzysztof
On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
> On 28/06/2023 18:31, Eric Lin wrote:
>
> >>>>
> >>>>> + - enum:
> >>>>> + - sifive,pL2Cache0
> >>>>> + - sifive,pL2Cache1
> >>>>
> >>>> What is "0" and "1" here? What do these compatibles represent? Why they
> >>>> do not have any SoC related part?
> >>>
> >>> The pL2Cache1 has minor changes in hardware, but it can use the same
> >>> pl2 cache driver.
> >>
> >> Then why aren't they compatible?
> >>
> >
> > The pL2Cache1 has removed some unused bits in the register compared to
> > pl2Cache0.
> > From the hardware perspective, they are not compatible but they can
> > share the same pl2 cache driver in software.
>
> So they are compatible... If they were not compatible, you wouldn't be
> able to use the same match in the driver.
>
> > Thus, we would like to keep both. It would be great if you can provide
> > some suggestions. Thanks.
>
> I propose to make them compatible, like every other piece of SoC. I
> don't see any benefit of having them separate.
>
Hi Krzysztof,
Sorry for the late reply.
The pl2 cache is our internal platform IP and is not part of any SoC.
The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
is that it doesn't program the different parts of the config register
However, our internal software (e.g., bare-metal software) will program these different parts,
so it needs to rely on the different compatible string to identify the hardware.
Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
Best regards,
Eric Lin
> Best regards,
> Krzysztof
>
On 12/07/2023 13:09, Eric Lin wrote:
> On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
>> On 28/06/2023 18:31, Eric Lin wrote:
>>
>>>>>>
>>>>>>> + - enum:
>>>>>>> + - sifive,pL2Cache0
>>>>>>> + - sifive,pL2Cache1
>>>>>>
>>>>>> What is "0" and "1" here? What do these compatibles represent? Why they
>>>>>> do not have any SoC related part?
>>>>>
>>>>> The pL2Cache1 has minor changes in hardware, but it can use the same
>>>>> pl2 cache driver.
>>>>
>>>> Then why aren't they compatible?
>>>>
>>>
>>> The pL2Cache1 has removed some unused bits in the register compared to
>>> pl2Cache0.
>>> From the hardware perspective, they are not compatible but they can
>>> share the same pl2 cache driver in software.
>>
>> So they are compatible... If they were not compatible, you wouldn't be
>> able to use the same match in the driver.
>>
>>> Thus, we would like to keep both. It would be great if you can provide
>>> some suggestions. Thanks.
>>
>> I propose to make them compatible, like every other piece of SoC. I
>> don't see any benefit of having them separate.
>>
>
> Hi Krzysztof,
>
> Sorry for the late reply.
> The pl2 cache is our internal platform IP and is not part of any SoC.
>
> The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
> is that it doesn't program the different parts of the config register
> However, our internal software (e.g., bare-metal software) will program these different parts,
> so it needs to rely on the different compatible string to identify the hardware.
>
> Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
I don't understand how does it contradicts anything I said. So you do
agree with me? Or what?
Best regards,
Krzysztof
On Wed, Jul 12, 2023 at 02:30:06PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2023 13:09, Eric Lin wrote:
> > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
> >> On 28/06/2023 18:31, Eric Lin wrote:
> >>
> >>>>>>
> >>>>>>> + - enum:
> >>>>>>> + - sifive,pL2Cache0
> >>>>>>> + - sifive,pL2Cache1
> >>>>>>
> >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they
> >>>>>> do not have any SoC related part?
> >>>>>
> >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same
> >>>>> pl2 cache driver.
> >>>>
> >>>> Then why aren't they compatible?
> >>>>
> >>>
> >>> The pL2Cache1 has removed some unused bits in the register compared to
> >>> pl2Cache0.
> >>> From the hardware perspective, they are not compatible but they can
> >>> share the same pl2 cache driver in software.
> >>
> >> So they are compatible... If they were not compatible, you wouldn't be
> >> able to use the same match in the driver.
> >>
> >>> Thus, we would like to keep both. It would be great if you can provide
> >>> some suggestions. Thanks.
> >>
> >> I propose to make them compatible, like every other piece of SoC. I
> >> don't see any benefit of having them separate.
> >>
> > Sorry for the late reply.
> > The pl2 cache is our internal platform IP and is not part of any SoC.
> >
> > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
> > is that it doesn't program the different parts of the config register
> > However, our internal software (e.g., bare-metal software) will program these different parts,
> > so it needs to rely on the different compatible string to identify the hardware.
> >
> > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
>
> I don't understand how does it contradicts anything I said. So you do
> agree with me? Or what?
I probably should've been keeping a closer eye here, sorry.
I assume what Krzysztof means is why do you permit both
"sifive,pL2Cache0" and "sifive,pL2Cache1" appearing in isolation. IOW,
both of
compatible = "sifive,pl2cache0";
and
compatible = "sifive,pl2cache1";
are valid in your binding.
The hardware for both might be different, and their full featuresets may
be incompatible, but they implement a compatible subset of features. I
would expect that the following would be the permitted compatible setups:
compatible = "sifive,pl2cache0";
and
compatible = "sifive,pl2cache1", "sifive,pl2cache0";
A consumer of the DT that does care for the differences should be
looking for the specific compatible, and OS code that does not care can
always bind to the "0" version.
Do the "0" & "1" here refer to the IP version, as in
sifive-blocks-ip-versioning.txt? I didn't think the compatibles
containing those IP versions were supposed to appear in isolation,
without a soc-specific one?
Thanks,
Conor.
Hi Conor,
On Wed, Jul 12, 2023 at 8:49 PM Conor Dooley <[email protected]> wrote:
>
> On Wed, Jul 12, 2023 at 02:30:06PM +0200, Krzysztof Kozlowski wrote:
> > On 12/07/2023 13:09, Eric Lin wrote:
> > > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
> > >> On 28/06/2023 18:31, Eric Lin wrote:
> > >>
> > >>>>>>
> > >>>>>>> + - enum:
> > >>>>>>> + - sifive,pL2Cache0
> > >>>>>>> + - sifive,pL2Cache1
> > >>>>>>
> > >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they
> > >>>>>> do not have any SoC related part?
> > >>>>>
> > >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same
> > >>>>> pl2 cache driver.
> > >>>>
> > >>>> Then why aren't they compatible?
> > >>>>
> > >>>
> > >>> The pL2Cache1 has removed some unused bits in the register compared to
> > >>> pl2Cache0.
> > >>> From the hardware perspective, they are not compatible but they can
> > >>> share the same pl2 cache driver in software.
> > >>
> > >> So they are compatible... If they were not compatible, you wouldn't be
> > >> able to use the same match in the driver.
> > >>
> > >>> Thus, we would like to keep both. It would be great if you can provide
> > >>> some suggestions. Thanks.
> > >>
> > >> I propose to make them compatible, like every other piece of SoC. I
> > >> don't see any benefit of having them separate.
> > >>
> > > Sorry for the late reply.
> > > The pl2 cache is our internal platform IP and is not part of any SoC.
> > >
> > > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
> > > is that it doesn't program the different parts of the config register
> > > However, our internal software (e.g., bare-metal software) will program these different parts,
> > > so it needs to rely on the different compatible string to identify the hardware.
> > >
> > > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
> >
> > I don't understand how does it contradicts anything I said. So you do
> > agree with me? Or what?
>
> I probably should've been keeping a closer eye here, sorry.
>
> I assume what Krzysztof means is why do you permit both
> "sifive,pL2Cache0" and "sifive,pL2Cache1" appearing in isolation. IOW,
> both of
> compatible = "sifive,pl2cache0";
> and
> compatible = "sifive,pl2cache1";
> are valid in your binding.
>
> The hardware for both might be different, and their full featuresets may
> be incompatible, but they implement a compatible subset of features. I
> would expect that the following would be the permitted compatible setups:
> compatible = "sifive,pl2cache0";
> and
> compatible = "sifive,pl2cache1", "sifive,pl2cache0";
>
> A consumer of the DT that does care for the differences should be
> looking for the specific compatible, and OS code that does not care can
> always bind to the "0" version.
>
Yes, but I think the proper compatible string for hw pl2cache0 and hw
pl2cache1 should be as follows:
hw pl2cache0 -> compatible = "sifive,pl2cache0","sifive,pl2cache1";
hw pl2cache1 -> compatible = "sifive,pl2cache1";
Since the hw pl2cache0 implements more features than hw pl2cache1, it
can be compatible with the pl2cache1 driver.
However, hw pl2cache1 only implements a sub-feature of hw pl2cache0,
so it cannot be compatible with the pl2cache0 driver.
Thus, I'll keep only the compatible = "sifive,pl2cache1". in the
driver and dt-binding. Thanks for the suggestions.
> Do the "0" & "1" here refer to the IP version, as in
> sifive-blocks-ip-versioning.txt? I didn't think the compatibles
> containing those IP versions were supposed to appear in isolation,
> without a soc-specific one?
>
Yes, I think they refer to IP versions. OK, I'll fix it in v2.
Thanks for the review.
Best regards,
Eric Lin
> Thanks,
> Conor.
Hi Krzysztof,
On Wed, Jul 12, 2023 at 8:30 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 12/07/2023 13:09, Eric Lin wrote:
> > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
> >> On 28/06/2023 18:31, Eric Lin wrote:
> >>
> >>>>>>
> >>>>>>> + - enum:
> >>>>>>> + - sifive,pL2Cache0
> >>>>>>> + - sifive,pL2Cache1
> >>>>>>
> >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they
> >>>>>> do not have any SoC related part?
> >>>>>
> >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same
> >>>>> pl2 cache driver.
> >>>>
> >>>> Then why aren't they compatible?
> >>>>
> >>>
> >>> The pL2Cache1 has removed some unused bits in the register compared to
> >>> pl2Cache0.
> >>> From the hardware perspective, they are not compatible but they can
> >>> share the same pl2 cache driver in software.
> >>
> >> So they are compatible... If they were not compatible, you wouldn't be
> >> able to use the same match in the driver.
> >>
> >>> Thus, we would like to keep both. It would be great if you can provide
> >>> some suggestions. Thanks.
> >>
> >> I propose to make them compatible, like every other piece of SoC. I
> >> don't see any benefit of having them separate.
> >>
> >
> > Hi Krzysztof,
> >
> > Sorry for the late reply.
> > The pl2 cache is our internal platform IP and is not part of any SoC.
> >
> > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
> > is that it doesn't program the different parts of the config register
> > However, our internal software (e.g., bare-metal software) will program these different parts,
> > so it needs to rely on the different compatible string to identify the hardware.
> >
> > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
>
> I don't understand how does it contradicts anything I said. So you do
> agree with me? Or what?
>
Thanks for your suggestions. OK, I'll fix it in v2.
Best regards,
Eric Lin
> Best regards,
> Krzysztof
>