[PATCH 0/2] L2 cache controller support for SiFive FU540

[PATCH 0/2] L2 cache controller support for SiFive FU540

[Yash Shah]

This patch series adds an L2 cache controller driver with DT documentation
for SiFive FU540-C000.

These two patches were initially part of the patch series:
'L2 cache controller and EDAC support for SiFive SoCs'
https://lkml.org/lkml/2019/4/15/320
In order to merge L2 cache controller driver without any dependency on EDAC,
the L2 cache controller patches are re-posted seperately in this series.

The patchset is based on Linux 5.1-rc2 and tested on HiFive Unleashed
board with additional board related patches needed for testing can be
found at dev/yashs/L2_cache_controller branch of:
https://github.com/yashshah7/riscv-linux.git

Yash Shah (2):
  RISC-V: Add DT documentation for SiFive L2 Cache Controller
  RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive
    SoCs

 .../devicetree/bindings/riscv/sifive-l2-cache.txt  |  53 +++++
 arch/riscv/mm/Makefile                             |   1 +
 arch/riscv/mm/sifive_l2_cache.c                    | 224 +++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
 create mode 100644 arch/riscv/mm/sifive_l2_cache.c

-- 
1.9.1

[PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Yash Shah]

Add device tree bindings for SiFive FU540 L2 cache controller driver

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt

diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
new file mode 100644
index 0000000..15132e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
@@ -0,0 +1,53 @@
+SiFive L2 Cache Controller
+--------------------------
+The SiFive Level 2 Cache Controller is used to provide access to fast copies
+of memory for masters in a Core Complex. The Level 2 Cache Controller also
+acts as directory-based coherency manager.
+
+Required Properties:
+--------------------
+- compatible: Should be "sifive,fu540-c000-ccache"
+
+- cache-block-size: Specifies the block size in bytes of the cache
+
+- cache-level: Should be set to 2 for a level 2 cache
+
+- cache-sets: Specifies the number of associativity sets of the cache
+
+- cache-size: Specifies the size in bytes of the cache
+
+- cache-unified: Specifies the cache is a unified cache
+
+- interrupt-parent: Must be core interrupt controller
+
+- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
+
+- reg: Physical base address and size of L2 cache controller registers map
+
+- reg-names: Should be "control"
+
+Optional Properties:
+--------------------
+- next-level-cache: phandle to the next level cache if present.
+
+- memory-region: reference to the reserved-memory for the L2 Loosely Integrated
+  Memory region. The reserved memory node should be defined as per the bindings
+  in reserved-memory.txt
+
+
+Example:
+
+	cache-controller@2010000 {
+		compatible = "sifive,fu540-c000-ccache";
+		cache-block-size = <0x40>;
+		cache-level = <0x2>;
+		cache-sets = <0x400>;
+		cache-size = <0x100000>;
+		cache-unified;
+		interrupt-parent = <&plic0>;
+		interrupts = <1 2 3>;
+		reg = <0x0 0x2010000 0x0 0x1000>;
+		reg-names = "control";
+		next-level-cache = <&L25 &L40 &L36>;
+		memory-region = <&l2_lim>;
+	};
-- 
1.9.1

[PATCH 2/2] RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs

[Yash Shah]

The driver currently supports only SiFive FU540-C000 platform.

The initial version of L2 cache controller driver includes:
- Initial configuration reporting at boot up.
- Support for ECC related functionality.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 arch/riscv/mm/Makefile          |   1 +
 arch/riscv/mm/sifive_l2_cache.c | 224 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 225 insertions(+)
 create mode 100644 arch/riscv/mm/sifive_l2_cache.c

diff --git a/arch/riscv/mm/Makefile b/arch/riscv/mm/Makefile
index eb22ab4..1523ee5 100644
--- a/arch/riscv/mm/Makefile
+++ b/arch/riscv/mm/Makefile
@@ -3,3 +3,4 @@ obj-y += fault.o
 obj-y += extable.o
 obj-y += ioremap.o
 obj-y += cacheflush.o
+obj-y += sifive_l2_cache.o
diff --git a/arch/riscv/mm/sifive_l2_cache.c b/arch/riscv/mm/sifive_l2_cache.c
new file mode 100644
index 0000000..95f10e4
--- /dev/null
+++ b/arch/riscv/mm/sifive_l2_cache.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiFive L2 cache controller Driver
+ *
+ * Copyright (C) 2018-2019 SiFive, Inc.
+ *
+ */
+#include <linux/debugfs.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+
+#define SIFIVE_L2_DIRECCFIX_LOW 0x100
+#define SIFIVE_L2_DIRECCFIX_HIGH 0x104
+#define SIFIVE_L2_DIRECCFIX_COUNT 0x108
+
+#define SIFIVE_L2_DATECCFIX_LOW 0x140
+#define SIFIVE_L2_DATECCFIX_HIGH 0x144
+#define SIFIVE_L2_DATECCFIX_COUNT 0x148
+
+#define SIFIVE_L2_DATECCFAIL_LOW 0x160
+#define SIFIVE_L2_DATECCFAIL_HIGH 0x164
+#define SIFIVE_L2_DATECCFAIL_COUNT 0x168
+
+#define SIFIVE_L2_CONFIG 0x00
+#define SIFIVE_L2_WAYENABLE 0x08
+#define SIFIVE_L2_ECCINJECTERR 0x40
+
+#define SIFIVE_L2_ERR_TYPE_CE 0
+#define SIFIVE_L2_ERR_TYPE_UE 1
+#define SIFIVE_L2_MAX_ECCINTR 3
+
+static void __iomem *l2_base;
+static int g_irq[SIFIVE_L2_MAX_ECCINTR];
+
+enum {
+	DIR_CORR = 0,
+	DATA_CORR,
+	DATA_UNCORR,
+};
+
+static unsigned int l2_dirfix_addr_high(void)
+{
+	return readl(l2_base + SIFIVE_L2_DIRECCFIX_HIGH);
+}
+
+static unsigned int l2_dirfix_addr_low(void)
+{
+	return readl(l2_base + SIFIVE_L2_DIRECCFIX_LOW);
+}
+
+static unsigned int l2_dirfix_count(void)
+{
+	return readl(l2_base + SIFIVE_L2_DIRECCFIX_COUNT);
+}
+
+static unsigned int l2_datfix_addr_high(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFIX_HIGH);
+}
+
+static unsigned int l2_datfix_addr_low(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFIX_LOW);
+}
+
+static unsigned int l2_datfix_count(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFIX_COUNT);
+}
+
+static unsigned int l2_datfail_addr_high(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFAIL_HIGH);
+}
+
+static unsigned int l2_datfail_addr_low(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFAIL_LOW);
+}
+
+static unsigned int l2_datfail_count(void)
+{
+	return readl(l2_base + SIFIVE_L2_DATECCFAIL_COUNT);
+}
+
+#ifdef CONFIG_DEBUG_FS
+static struct dentry *sifive_test;
+
+static ssize_t l2_write(struct file *file, const char __user *data,
+			size_t count, loff_t *ppos)
+{
+	unsigned int val;
+
+	if (kstrtouint_from_user(data, count, 0, &val))
+		return -EINVAL;
+	if ((val >= 0 && val < 0xFF) || (val >= 0x10000 && val < 0x100FF))
+		writel(val, l2_base + SIFIVE_L2_ECCINJECTERR);
+	else
+		return -EINVAL;
+	return count;
+}
+
+static const struct file_operations l2_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.write = l2_write
+};
+
+static void setup_sifive_debug(void)
+{
+	sifive_test = debugfs_create_dir("sifive_l2_cache", NULL);
+	if (!sifive_test)
+		return;
+
+	if (!debugfs_create_file("sifive_debug_inject_error", 0200,
+				 sifive_test, NULL, &l2_fops))
+		debugfs_remove_recursive(sifive_test);
+}
+#endif
+
+static void l2_config_read(void)
+{
+	u32 regval, val;
+
+	regval = readl(l2_base + SIFIVE_L2_CONFIG);
+	val = regval & 0xFF;
+	pr_info("L2CACHE: No. of Banks in the cache: %d\n", val);
+	val = (regval & 0xFF00) >> 8;
+	pr_info("L2CACHE: No. of ways per bank: %d\n", val);
+	val = (regval & 0xFF0000) >> 16;
+	pr_info("L2CACHE: Sets per bank: %llu\n", (uint64_t)1 << val);
+	val = (regval & 0xFF000000) >> 24;
+	pr_info("L2CACHE: Bytes per cache block: %llu\n", (uint64_t)1 << val);
+
+	regval = readl(l2_base + SIFIVE_L2_WAYENABLE);
+	pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
+}
+
+static const struct of_device_id sifive_l2_ids[] = {
+	{ .compatible = "sifive,fu540-c000-ccache" },
+	{ /* end of table */ },
+};
+
+static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
+
+int register_sifive_l2_error_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&l2_err_chain, nb);
+}
+EXPORT_SYMBOL_GPL(register_sifive_l2_error_notifier);
+
+int unregister_sifive_l2_error_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&l2_err_chain, nb);
+}
+EXPORT_SYMBOL_GPL(unregister_sifive_l2_error_notifier);
+
+static irqreturn_t l2_int_handler(int irq, void *device)
+{
+	unsigned int regval, add_h, add_l;
+
+	if (irq == g_irq[DIR_CORR]) {
+		add_h = l2_dirfix_addr_high();
+		add_l = l2_dirfix_addr_low();
+		pr_err("L2CACHE: DirError @ 0x%08X.%08X\n", add_h, add_l);
+		regval = l2_dirfix_count();
+		atomic_notifier_call_chain(&l2_err_chain, SIFIVE_L2_ERR_TYPE_CE,
+					   "DirECCFix");
+	}
+	if (irq == g_irq[DATA_CORR]) {
+		add_h = l2_datfix_addr_high();
+		add_l = l2_datfix_addr_low();
+		pr_err("L2CACHE: DataError @ 0x%08X.%08X\n", add_h, add_l);
+		regval = l2_datfix_count();
+		atomic_notifier_call_chain(&l2_err_chain, SIFIVE_L2_ERR_TYPE_CE,
+					   "DatECCFix");
+	}
+	if (irq == g_irq[DATA_UNCORR]) {
+		add_h = l2_datfail_addr_high();
+		add_l = l2_datfail_addr_low();
+		pr_err("L2CACHE: DataFail @ 0x%08X.%08X\n", add_h, add_l);
+		regval = l2_datfail_count();
+		atomic_notifier_call_chain(&l2_err_chain, SIFIVE_L2_ERR_TYPE_UE,
+					   "DatECCFail");
+	}
+
+	return IRQ_HANDLED;
+}
+
+int __init sifive_l2_init(void)
+{
+	struct device_node *np;
+	struct resource res;
+	int i, rc;
+
+	np = of_find_matching_node(NULL, sifive_l2_ids);
+	if (!np)
+		return -ENODEV;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENODEV;
+
+	l2_base = ioremap(res.start, resource_size(&res));
+	if (!l2_base)
+		return -ENOMEM;
+
+	for (i = 0; i < SIFIVE_L2_MAX_ECCINTR; i++) {
+		g_irq[i] = irq_of_parse_and_map(np, i);
+		rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
+		if (rc) {
+			pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
+			return rc;
+		}
+	}
+
+	l2_config_read();
+
+#ifdef CONFIG_DEBUG_FS
+	setup_sifive_debug();
+#endif
+	return 0;
+}
+device_initcall(sifive_l2_init);
-- 
1.9.1

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Sudeep Holla]

On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> Add device tree bindings for SiFive FU540 L2 cache controller driver
> 
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> 
> diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> new file mode 100644
> index 0000000..15132e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> @@ -0,0 +1,53 @@
> +SiFive L2 Cache Controller
> +--------------------------
> +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> +acts as directory-based coherency manager.
> +
> +Required Properties:
> +--------------------
> +- compatible: Should be "sifive,fu540-c000-ccache"
> +
> +- cache-block-size: Specifies the block size in bytes of the cache
> +
> +- cache-level: Should be set to 2 for a level 2 cache
> +
> +- cache-sets: Specifies the number of associativity sets of the cache
> +
> +- cache-size: Specifies the size in bytes of the cache
> +
> +- cache-unified: Specifies the cache is a unified cache
> +
> +- interrupt-parent: Must be core interrupt controller
> +
> +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> +
> +- reg: Physical base address and size of L2 cache controller registers map
> +
> +- reg-names: Should be "control"
> +

It would be good if you mark the properties that are present in DT
specification and those that are added for sifive,fu540-c000-ccache
explicitly. Also I assume you can retain the stardard "cache" compatible
in addition to above. I am interested to see if the cacheinfo infrastructure
can be used without any issues.

--
Regards,
Sudeep

Re: [PATCH 2/2] RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs

[Sudeep Holla]

On Thu, Apr 25, 2019 at 11:24:56AM +0530, Yash Shah wrote:
> The driver currently supports only SiFive FU540-C000 platform.
> 
> The initial version of L2 cache controller driver includes:
> - Initial configuration reporting at boot up.
> - Support for ECC related functionality.
> 
> Signed-off-by: Yash Shah <yash.shah@sifive.com>

[....]

> +static const struct file_operations l2_fops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.write = l2_write
> +};
> +
> +static void setup_sifive_debug(void)
> +{
> +	sifive_test = debugfs_create_dir("sifive_l2_cache", NULL);
> +	if (!sifive_test)

Drop the conditional check above, Greg K H removed lots of them recently.
In his words: When calling debugfs functions, there is no need to ever
check the return value.  The function can work or not, but the code
logic should never do something different based on this.

He may not like to see this :)

> +		return;
> +
> +	if (!debugfs_create_file("sifive_debug_inject_error", 0200,
> +				 sifive_test, NULL, &l2_fops))

Ditto.

> +		debugfs_remove_recursive(sifive_test);
> +}

--
Regards,
Sudeep

Re: [PATCH 2/2] RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs

[Yash Shah]

On Thu, Apr 25, 2019 at 3:48 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Apr 25, 2019 at 11:24:56AM +0530, Yash Shah wrote:
> > The driver currently supports only SiFive FU540-C000 platform.
> >
> > The initial version of L2 cache controller driver includes:
> > - Initial configuration reporting at boot up.
> > - Support for ECC related functionality.
> >
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
>
> [....]
>
> > +static const struct file_operations l2_fops = {
> > +     .owner = THIS_MODULE,
> > +     .open = simple_open,
> > +     .write = l2_write
> > +};
> > +
> > +static void setup_sifive_debug(void)
> > +{
> > +     sifive_test = debugfs_create_dir("sifive_l2_cache", NULL);
> > +     if (!sifive_test)
>
> Drop the conditional check above, Greg K H removed lots of them recently.
> In his words: When calling debugfs functions, there is no need to ever
> check the return value.  The function can work or not, but the code
> logic should never do something different based on this.
>
> He may not like to see this :)

Sure, thanks for pointing it out. Will drop all the conditional check
in debugfs functions.

>
> > +             return;
> > +
> > +     if (!debugfs_create_file("sifive_debug_inject_error", 0200,
> > +                              sifive_test, NULL, &l2_fops))
>
> Ditto.
>
> > +             debugfs_remove_recursive(sifive_test);
> > +}
>
> --
> Regards,
> Sudeep

Thanks for your comments.

- Yash

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Yash Shah]

On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > Add device tree bindings for SiFive FU540 L2 cache controller driver
> >
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > new file mode 100644
> > index 0000000..15132e2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > @@ -0,0 +1,53 @@
> > +SiFive L2 Cache Controller
> > +--------------------------
> > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > +acts as directory-based coherency manager.
> > +
> > +Required Properties:
> > +--------------------
> > +- compatible: Should be "sifive,fu540-c000-ccache"
> > +
> > +- cache-block-size: Specifies the block size in bytes of the cache
> > +
> > +- cache-level: Should be set to 2 for a level 2 cache
> > +
> > +- cache-sets: Specifies the number of associativity sets of the cache
> > +
> > +- cache-size: Specifies the size in bytes of the cache
> > +
> > +- cache-unified: Specifies the cache is a unified cache
> > +
> > +- interrupt-parent: Must be core interrupt controller
> > +
> > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > +
> > +- reg: Physical base address and size of L2 cache controller registers map
> > +
> > +- reg-names: Should be "control"
> > +
>
> It would be good if you mark the properties that are present in DT
> specification and those that are added for sifive,fu540-c000-ccache

I believe there isn't any property which is added explicitly for
sifive,fu540-c000-ccache.

> explicitly. Also I assume you can retain the stardard "cache" compatible
> in addition to above. I am interested to see if the cacheinfo infrastructure
> can be used without any issues.

Yes, I will add the "cache" string to the compatible property.

>
> --
> Regards,
> Sudeep

Thanks for your comments.
- Yash

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Sudeep Holla]

On Fri, Apr 26, 2019 at 11:20:17AM +0530, Yash Shah wrote:
> On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > > Add device tree bindings for SiFive FU540 L2 cache controller driver
> > >
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> > >  1 file changed, 53 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > new file mode 100644
> > > index 0000000..15132e2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > @@ -0,0 +1,53 @@
> > > +SiFive L2 Cache Controller
> > > +--------------------------
> > > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > > +acts as directory-based coherency manager.
> > > +
> > > +Required Properties:
> > > +--------------------
> > > +- compatible: Should be "sifive,fu540-c000-ccache"
> > > +
> > > +- cache-block-size: Specifies the block size in bytes of the cache
> > > +
> > > +- cache-level: Should be set to 2 for a level 2 cache
> > > +
> > > +- cache-sets: Specifies the number of associativity sets of the cache
> > > +
> > > +- cache-size: Specifies the size in bytes of the cache
> > > +
> > > +- cache-unified: Specifies the cache is a unified cache
> > > +
> > > +- interrupt-parent: Must be core interrupt controller
> > > +
> > > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > > +
> > > +- reg: Physical base address and size of L2 cache controller registers map
> > > +
> > > +- reg-names: Should be "control"
> > > +
> >
> > It would be good if you mark the properties that are present in DT
> > specification and those that are added for sifive,fu540-c000-ccache
> 
> I believe there isn't any property which is added explicitly for
> sifive,fu540-c000-ccache.
> 

reg and interrupts are generally optional for normal cache and may be
required for cache controller like this. DT specification[1] covers
only caches and not cache controllers.

--
Regards,
Sudeep

[1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2/devicetree-specification-v0.2.pdf

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Yash Shah]

On Fri, Apr 26, 2019 at 3:04 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Apr 26, 2019 at 11:20:17AM +0530, Yash Shah wrote:
> > On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > > > Add device tree bindings for SiFive FU540 L2 cache controller driver
> > > >
> > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > ---
> > > >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> > > >  1 file changed, 53 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > new file mode 100644
> > > > index 0000000..15132e2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > @@ -0,0 +1,53 @@
> > > > +SiFive L2 Cache Controller
> > > > +--------------------------
> > > > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > > > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > > > +acts as directory-based coherency manager.
> > > > +
> > > > +Required Properties:
> > > > +--------------------
> > > > +- compatible: Should be "sifive,fu540-c000-ccache"
> > > > +
> > > > +- cache-block-size: Specifies the block size in bytes of the cache
> > > > +
> > > > +- cache-level: Should be set to 2 for a level 2 cache
> > > > +
> > > > +- cache-sets: Specifies the number of associativity sets of the cache
> > > > +
> > > > +- cache-size: Specifies the size in bytes of the cache
> > > > +
> > > > +- cache-unified: Specifies the cache is a unified cache
> > > > +
> > > > +- interrupt-parent: Must be core interrupt controller
> > > > +
> > > > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > > > +
> > > > +- reg: Physical base address and size of L2 cache controller registers map
> > > > +
> > > > +- reg-names: Should be "control"
> > > > +
> > >
> > > It would be good if you mark the properties that are present in DT
> > > specification and those that are added for sifive,fu540-c000-ccache
> >
> > I believe there isn't any property which is added explicitly for
> > sifive,fu540-c000-ccache.
> >
>
> reg and interrupts are generally optional for normal cache and may be
> required for cache controller like this. DT specification[1] covers
> only caches and not cache controllers.

Are you suggesting something like this:

Required Properties:
--------------------
Standard Properties:
- compatible: Should be "sifive,<chip>-ccache"
  Supported compatible strings are:
  "sifive,fu540-c000-ccache" and "sifive,fu740-c000-ccache"

- cache-block-size: Specifies the block size in bytes of the cache

- cache-level: Should be set to 2 for a level 2 cache

- cache-sets: Specifies the number of associativity sets of the cache

- cache-size: Specifies the size in bytes of the cache

- cache-unified: Specifies the cache is a unified cache

Non-Standard Properties:
- interrupt-parent: Must be core interrupt controller

- interrupts: Must contain 3 entries for FU540 (DirError, DataError and
  DataFail signals) or 4 entries for other chips (DirError, DirFail, DataError,
  DataFail signals)

- reg: Physical base address and size of L2 cache controller registers map

- reg-names: Should be "control"

- Yash
>
> --
> Regards,
> Sudeep
>
> [1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2/devicetree-specification-v0.2.pdf

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Rob Herring]

On Tue, Apr 30, 2019 at 09:50:45AM +0530, Yash Shah wrote:
> On Fri, Apr 26, 2019 at 3:04 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Apr 26, 2019 at 11:20:17AM +0530, Yash Shah wrote:
> > > On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > > > > Add device tree bindings for SiFive FU540 L2 cache controller driver
> > > > >
> > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > ---
> > > > >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> > > > >  1 file changed, 53 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > new file mode 100644
> > > > > index 0000000..15132e2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > @@ -0,0 +1,53 @@
> > > > > +SiFive L2 Cache Controller
> > > > > +--------------------------
> > > > > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > > > > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > > > > +acts as directory-based coherency manager.
> > > > > +
> > > > > +Required Properties:
> > > > > +--------------------
> > > > > +- compatible: Should be "sifive,fu540-c000-ccache"
> > > > > +
> > > > > +- cache-block-size: Specifies the block size in bytes of the cache
> > > > > +
> > > > > +- cache-level: Should be set to 2 for a level 2 cache
> > > > > +
> > > > > +- cache-sets: Specifies the number of associativity sets of the cache
> > > > > +
> > > > > +- cache-size: Specifies the size in bytes of the cache
> > > > > +
> > > > > +- cache-unified: Specifies the cache is a unified cache
> > > > > +
> > > > > +- interrupt-parent: Must be core interrupt controller
> > > > > +
> > > > > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > > > > +
> > > > > +- reg: Physical base address and size of L2 cache controller registers map
> > > > > +
> > > > > +- reg-names: Should be "control"
> > > > > +
> > > >
> > > > It would be good if you mark the properties that are present in DT
> > > > specification and those that are added for sifive,fu540-c000-ccache
> > >
> > > I believe there isn't any property which is added explicitly for
> > > sifive,fu540-c000-ccache.
> > >
> >
> > reg and interrupts are generally optional for normal cache and may be
> > required for cache controller like this. DT specification[1] covers
> > only caches and not cache controllers.
> 
> Are you suggesting something like this:
> 
> Required Properties:
> --------------------
> Standard Properties:

I don't think we need this separation.

> - compatible: Should be "sifive,<chip>-ccache"
>   Supported compatible strings are:
>   "sifive,fu540-c000-ccache" and "sifive,fu740-c000-ccache"
> 
> - cache-block-size: Specifies the block size in bytes of the cache
> 
> - cache-level: Should be set to 2 for a level 2 cache
> 
> - cache-sets: Specifies the number of associativity sets of the cache
> 
> - cache-size: Specifies the size in bytes of the cache

What are the possible valid values for these? That's what's important. 
What the properties mean are already defined in the spec.

> 
> - cache-unified: Specifies the cache is a unified cache
> 
> Non-Standard Properties:

I wouldn't call these non-standard.

> - interrupt-parent: Must be core interrupt controller

This is implied.

> 
> - interrupts: Must contain 3 entries for FU540 (DirError, DataError and
>   DataFail signals) or 4 entries for other chips (DirError, DirFail, DataError,
>   DataFail signals)
> 
> - reg: Physical base address and size of L2 cache controller registers map
> 
> - reg-names: Should be "control"

-names is not really needed when there is only 1 entry.

> 
> - Yash
> >
> > --
> > Regards,
> > Sudeep
> >
> > [1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2/devicetree-specification-v0.2.pdf

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Yash Shah]

On Thu, May 2, 2019 at 6:11 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Apr 30, 2019 at 09:50:45AM +0530, Yash Shah wrote:
> > On Fri, Apr 26, 2019 at 3:04 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Fri, Apr 26, 2019 at 11:20:17AM +0530, Yash Shah wrote:
> > > > On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > > > > > Add device tree bindings for SiFive FU540 L2 cache controller driver
> > > > > >
> > > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> > > > > >  1 file changed, 53 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..15132e2
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > > @@ -0,0 +1,53 @@
> > > > > > +SiFive L2 Cache Controller
> > > > > > +--------------------------
> > > > > > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > > > > > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > > > > > +acts as directory-based coherency manager.
> > > > > > +
> > > > > > +Required Properties:
> > > > > > +--------------------
> > > > > > +- compatible: Should be "sifive,fu540-c000-ccache"
> > > > > > +
> > > > > > +- cache-block-size: Specifies the block size in bytes of the cache
> > > > > > +
> > > > > > +- cache-level: Should be set to 2 for a level 2 cache
> > > > > > +
> > > > > > +- cache-sets: Specifies the number of associativity sets of the cache
> > > > > > +
> > > > > > +- cache-size: Specifies the size in bytes of the cache
> > > > > > +
> > > > > > +- cache-unified: Specifies the cache is a unified cache
> > > > > > +
> > > > > > +- interrupt-parent: Must be core interrupt controller
> > > > > > +
> > > > > > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > > > > > +
> > > > > > +- reg: Physical base address and size of L2 cache controller registers map
> > > > > > +
> > > > > > +- reg-names: Should be "control"
> > > > > > +
> > > > >
> > > > > It would be good if you mark the properties that are present in DT
> > > > > specification and those that are added for sifive,fu540-c000-ccache
> > > >
> > > > I believe there isn't any property which is added explicitly for
> > > > sifive,fu540-c000-ccache.
> > > >
> > >
> > > reg and interrupts are generally optional for normal cache and may be
> > > required for cache controller like this. DT specification[1] covers
> > > only caches and not cache controllers.
> >
> > Are you suggesting something like this:
> >
> > Required Properties:
> > --------------------
> > Standard Properties:
>
> I don't think we need this separation.

Ok. Won't include this "Standard/Non-standard properties" separation
in the next revision of this patch.

>
> > - cache-block-size: Specifies the block size in bytes of the cache
> >
> > - cache-level: Should be set to 2 for a level 2 cache
> >
> > - cache-sets: Specifies the number of associativity sets of the cache
> >
> > - cache-size: Specifies the size in bytes of the cache
>
> What are the possible valid values for these? That's what's important.
> What the properties mean are already defined in the spec.

Sure, will mention the valid values for these properties.

>
> >
> > - cache-unified: Specifies the cache is a unified cache
> >
> > Non-Standard Properties:
>
> I wouldn't call these non-standard.
>
> > - interrupt-parent: Must be core interrupt controller
>
> This is implied.

Will remove this redundant description.

>
> > - reg: Physical base address and size of L2 cache controller registers map
> >
> > - reg-names: Should be "control"
>
> -names is not really needed when there is only 1 entry.

Will remove this property.

>
> >
> > - Yash
> > >
> > > --
> > > Regards,
> > > Sudeep
> > >
> > > [1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2/devicetree-specification-v0.2.pdf

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Sudeep Holla]

On Thu, May 02, 2019 at 10:50:12AM +0530, Yash Shah wrote:
> On Thu, May 2, 2019 at 6:11 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Apr 30, 2019 at 09:50:45AM +0530, Yash Shah wrote:
> > > On Fri, Apr 26, 2019 at 3:04 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Fri, Apr 26, 2019 at 11:20:17AM +0530, Yash Shah wrote:
> > > > > On Thu, Apr 25, 2019 at 3:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Apr 25, 2019 at 11:24:55AM +0530, Yash Shah wrote:
> > > > > > > Add device tree bindings for SiFive FU540 L2 cache controller driver
> > > > > > >
> > > > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/riscv/sifive-l2-cache.txt  | 53 ++++++++++++++++++++++
> > > > > > >  1 file changed, 53 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > > > new file mode 100644
> > > > > > > index 0000000..15132e2
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.txt
> > > > > > > @@ -0,0 +1,53 @@
> > > > > > > +SiFive L2 Cache Controller
> > > > > > > +--------------------------
> > > > > > > +The SiFive Level 2 Cache Controller is used to provide access to fast copies
> > > > > > > +of memory for masters in a Core Complex. The Level 2 Cache Controller also
> > > > > > > +acts as directory-based coherency manager.
> > > > > > > +
> > > > > > > +Required Properties:
> > > > > > > +--------------------
> > > > > > > +- compatible: Should be "sifive,fu540-c000-ccache"
> > > > > > > +
> > > > > > > +- cache-block-size: Specifies the block size in bytes of the cache
> > > > > > > +
> > > > > > > +- cache-level: Should be set to 2 for a level 2 cache
> > > > > > > +
> > > > > > > +- cache-sets: Specifies the number of associativity sets of the cache
> > > > > > > +
> > > > > > > +- cache-size: Specifies the size in bytes of the cache
> > > > > > > +
> > > > > > > +- cache-unified: Specifies the cache is a unified cache
> > > > > > > +
> > > > > > > +- interrupt-parent: Must be core interrupt controller
> > > > > > > +
> > > > > > > +- interrupts: Must contain 3 entries (DirError, DataError and DataFail signals)
> > > > > > > +
> > > > > > > +- reg: Physical base address and size of L2 cache controller registers map
> > > > > > > +
> > > > > > > +- reg-names: Should be "control"
> > > > > > > +
> > > > > >
> > > > > > It would be good if you mark the properties that are present in DT
> > > > > > specification and those that are added for sifive,fu540-c000-ccache
> > > > >
> > > > > I believe there isn't any property which is added explicitly for
> > > > > sifive,fu540-c000-ccache.
> > > > >
> > > >
> > > > reg and interrupts are generally optional for normal cache and may be
> > > > required for cache controller like this. DT specification[1] covers
> > > > only caches and not cache controllers.
> > >
> > > Are you suggesting something like this:
> > >
> > > Required Properties:
> > > --------------------
> > > Standard Properties:
> >
> > I don't think we need this separation.
>
> Ok. Won't include this "Standard/Non-standard properties" separation
> in the next revision of this patch.
>

Sorry if I created confusion. I just wanted a note saying all the properties
in ePAPR/DeviceTree specification applies for this platform. That would
help me check if the standard cacheinfo infrastruction works as is or not.

--
Regards,
Sudeep

Re: [PATCH 1/2] RISC-V: Add DT documentation for SiFive L2 Cache Controller

[Yash Shah]

On Thu, May 2, 2019 at 2:40 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> Sorry if I created confusion. I just wanted a note saying all the properties
> in ePAPR/DeviceTree specification applies for this platform. That would
> help me check if the standard cacheinfo infrastruction works as is or not.

Sure, will add this note.

- Yash
>
> --
> Regards,
> Sudeep