simplified RISC-V interrupt and clocksource handling v2

simplified RISC-V interrupt and clocksource handling v2

[Christoph Hellwig]

This series tries adds support for interrupt handling and timers
for the RISC-V architecture.

The basic per-hart interrupt handling implemented by the scause
and sie CSRs is extremely simple and implemented directly in
arch/riscv/kernel/irq.c.  In addition there is a irqchip driver
for the PLIC external interrupt controller, which is called through
the set_handle_irq API, and a clocksource driver that gets its
timer interrupt directly from the low-level interrupt handling.

Compared to previous iterations this version does not try to use an
irqchip driver for the low-level interrupt handling.  This saves
a couple indirect calls and an additional read of the scause CSR
in the hot path, makes the code much simpler and last but not least
avoid the dependency on a device tree for a mandatory architectural
feature.

A git tree is available here (contains a few more patches before
the ones in this series)

    git://git.infradead.org/users/hch/riscv.git riscv-irq-simple.2

Gitweb:

    http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple.2

Changes since v1:
 - rename the plic driver to irq-sifive-plic
 - switch to a default compatible of sifive,plic0 (still supporting the
   riscv,plic0 name for compatibility)
 - add a reference for the SiFive PLIC register layout
 - fix plic_toggle addressing for large numbers of hwirqs
 - remove the call to ack_bad_irq
 - use a raw spinlock for plic_toggle_lock
 - use the irq_desc cpumask in the plic enable/disable methods
 - add back OF contexid parsing in the plic driver
 - don't allow COMPILE_TEST builds of the clocksource driver, as it
   depends on <asm/sbi.h>
 - default the clocksource driver to y
 - clean up naming in the clocksource driver
 - remove the MINDELTA and MAXDELTA #defines
 - various DT binding fixes

[PATCH 01/11] dt-bindings: Correct RISC-V's timebase-frequency

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@sifive.com>

Someone must have read the device tree specification incorrectly,
because we were putting timebase-frequency in the wrong place.  This
corrects the issue, moving it from

/ {
        cpus {
                timebase-frequency = X;
        }
}

to

/ {
        cpus {
                cpu@0 {
                        timebase-frequency = X;
                }
        }
}

This is great, because the timer's frequency should really be a per-cpu
quantity on RISC-V systems since there's a timer per CPU.  This should
lead to some cleanups in our timer driver.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/devicetree/bindings/riscv/cpus.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.txt b/Documentation/devicetree/bindings/riscv/cpus.txt
index adf7b7af5dc3..b0b038d6c406 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.txt
+++ b/Documentation/devicetree/bindings/riscv/cpus.txt
@@ -93,9 +93,9 @@ Linux is allowed to run on.
         cpus {
                 #address-cells = <1>;
                 #size-cells = <0>;
-                timebase-frequency = <1000000>;
                 cpu@0 {
                         clock-frequency = <1600000000>;
+                        timebase-frequency = <1000000>;
                         compatible = "sifive,rocket0", "riscv";
                         device_type = "cpu";
                         i-cache-block-size = <64>;
@@ -113,6 +113,7 @@ Linux is allowed to run on.
                 };
                 cpu@1 {
                         clock-frequency = <1600000000>;
+                        timebase-frequency = <1000000>;
                         compatible = "sifive,rocket0", "riscv";
                         d-cache-block-size = <64>;
                         d-cache-sets = <64>;
@@ -145,6 +146,7 @@ Example: Spike ISA Simulator with 1 Hart
 This device tree matches the Spike ISA golden model as run with `spike -p1`.
 
         cpus {
+                timebase-frequency = <1000000>;
                 cpu@0 {
                         device_type = "cpu";
                         reg = <0x00000000>;
-- 
2.18.0

[PATCH 02/11] dt-bindings: Add an enable method to RISC-V

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@sifive.com>

RISC-V doesn't currently specify a mechanism for enabling or disabling
CPUs.  Instead, we assume that all CPUs are enabled on boot, and if
someone wants to save power we instead put a CPU to sleep via a WFI
loop.  Future systems may have an explicit mechanism for putting a CPU
to sleep, so we're standardizing the device tree entry for when that
happens.

We're not defining a spin-table based interface to the firmware, as the
plan is to handle this entirely within the kernel instead.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/devicetree/bindings/riscv/cpus.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.txt b/Documentation/devicetree/bindings/riscv/cpus.txt
index b0b038d6c406..6aa9cd075a5b 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.txt
+++ b/Documentation/devicetree/bindings/riscv/cpus.txt
@@ -82,6 +82,15 @@ described below.
                 Value type: <string>
                 Definition: Contains the RISC-V ISA string of this hart.  These
                             ISA strings are defined by the RISC-V ISA manual.
+        - cpu-enable-method:
+                Usage: optional
+                Value type: <stringlist>
+                Definition: When absent, default is either "always-disabled"
+                            "always-enabled", depending on the current state
+                            of the CPU.
+                            Must be one of:
+                                * "always-disabled": This CPU cannot be enabled.
+                                * "always-enabled": This CPU cannot be disabled.
 
 Example: SiFive Freedom U540G Development Kit
 ---------------------------------------------
-- 
2.18.0

[PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@dabbelt.com>

This patch adds documentation for the platform-level interrupt
controller (PLIC) found in all RISC-V systems.  This interrupt
controller routes interrupts from all the devices in the system to each
hart-local interrupt controller.

Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
want to change how we're specifying holes in the hart list.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: various fixes and updates]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
new file mode 100644
index 000000000000..c756cd208a93
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
@@ -0,0 +1,57 @@
+SiFive Platform-Level Interrupt Controller (PLIC)
+-------------------------------------------------
+
+SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
+(PLIC) high-level specification in the RISC-V Privileged Architecture
+specification.  The PLIC connects all external interrupts in the system to all
+hart contexts in the system, via the external interrupt source in each hart.
+
+A hart context is a privilege mode in a hardware execution thread.  For example,
+in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
+privilege modes per hart; machine mode and supervisor mode.
+
+Each interrupt can be enabled on per-context basis. Any context can claim
+a pending enabled interrupt and then release it once it has been handled.
+
+Each interrupt has a configurable priority. Higher priority interrupts are
+serviced first. Each context can specify a priority threshold. Interrupts
+with priority below this threshold will not cause the PLIC to raise its
+interrupt line leading to the context.
+
+While the PLIC supports both edge-triggered and level-triggered interrupts,
+interrupt handlers are oblivious to this distinction and therefore it is not
+specified in the PLIC device-tree binding.
+
+While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
+"sifive,plic0" device is a concrete implementation of the PLIC that contains a
+specific memory layout, which is documented in chapter 8 of the SiFive U5
+Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
+
+Required properties:
+- compatible : "sifive,plic0"
+- #address-cells : should be <0>
+- #interrupt-cells : should be <1>
+- interrupt-controller : Identifies the node as an interrupt controller
+- reg : Should contain 1 register range (address and length)
+- interrupts-extended : Specifies which contexts are connected to the PLIC,
+  with "-1" specifying that a context is not present.  The nodes pointed
+  to should be "riscv" HART nodes, or eventually be parented by such nodes.
+- riscv,ndev: Specifies how many external interrupts are supported by
+  this controller.
+
+Example:
+
+	plic: interrupt-controller@c000000 {
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+		compatible = "riscv,plic0";
+		interrupt-controller;
+		interrupts-extended = <
+			&cpu0-intc 11
+			&cpu1-intc 11 &cpu1-intc 9
+			&cpu2-intc 11 &cpu2-intc 9
+			&cpu3-intc 11 &cpu3-intc 9
+			&cpu4-intc 11 &cpu4-intc 9>;
+		reg = <0xc000000 0x4000000>;
+		riscv,ndev = <10>;
+	};
-- 
2.18.0

[PATCH 04/11] RISC-V: remove timer leftovers

[Christoph Hellwig]

This code is currently unused and will be added back later in a different
place with the real interrupt and clocksource support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/time.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 2463fcca719e..0df9b2cbd645 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -13,32 +13,11 @@
  */
 
 #include <linux/clocksource.h>
-#include <linux/clockchips.h>
 #include <linux/delay.h>
-
-#ifdef CONFIG_RISCV_TIMER
-#include <linux/timer_riscv.h>
-#endif
-
 #include <asm/sbi.h>
 
 unsigned long riscv_timebase;
 
-DECLARE_PER_CPU(struct clock_event_device, riscv_clock_event);
-
-void riscv_timer_interrupt(void)
-{
-#ifdef CONFIG_RISCV_TIMER
-	/*
-	 * FIXME: This needs to be cleaned up along with the rest of the IRQ
-	 * handling cleanup.  See irq.c for more details.
-	 */
-	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
-
-	evdev->event_handler(evdev);
-#endif
-}
-
 void __init init_clockevent(void)
 {
 	timer_probe();
-- 
2.18.0

[PATCH 05/11] RISC-V: simplify software interrupt / IPI code

[Christoph Hellwig]

Rename handle_ipi to riscv_software_interrupt, drop the unused return
value and move the prototype to irq.h together with riscv_timer_interupt.
This allows simplifying the upcoming interrupt handling support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/irq.h | 1 +
 arch/riscv/include/asm/smp.h | 3 ---
 arch/riscv/kernel/smp.c      | 6 ++----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 4dee9d4c13c0..c871661c9df4 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -22,6 +22,7 @@
 #define INTERRUPT_CAUSE_EXTERNAL    9
 
 void riscv_timer_interrupt(void);
+void riscv_software_interrupt(void);
 
 #include <asm-generic/irq.h>
 
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 85e4220839b0..c9395fff246f 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -44,9 +44,6 @@ void arch_send_call_function_single_ipi(int cpu);
  */
 #define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))
 
-/* Interprocessor interrupt handler */
-irqreturn_t handle_ipi(void);
-
 #endif /* CONFIG_SMP */
 
 #endif /* _ASM_RISCV_SMP_H */
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 6d3962435720..906fe21ea21b 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -45,7 +45,7 @@ int setup_profiling_timer(unsigned int multiplier)
 	return -EINVAL;
 }
 
-irqreturn_t handle_ipi(void)
+void riscv_software_interrupt(void)
 {
 	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
 
@@ -60,7 +60,7 @@ irqreturn_t handle_ipi(void)
 
 		ops = xchg(pending_ipis, 0);
 		if (ops == 0)
-			return IRQ_HANDLED;
+			return;
 
 		if (ops & (1 << IPI_RESCHEDULE))
 			scheduler_ipi();
@@ -73,8 +73,6 @@ irqreturn_t handle_ipi(void)
 		/* Order data access and bit testing. */
 		mb();
 	}
-
-	return IRQ_HANDLED;
 }
 
 static void
-- 
2.18.0

[PATCH 06/11] RISC-V: remove INTERRUPT_CAUSE_* defines from asm/irq.h

[Christoph Hellwig]

These are only of use to the local irq controller driver, so add them in
that driver implementation instead, which will be submitted soon.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/irq.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index c871661c9df4..996b6fbe17a6 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -17,10 +17,6 @@
 
 #define NR_IRQS         0
 
-#define INTERRUPT_CAUSE_SOFTWARE    1
-#define INTERRUPT_CAUSE_TIMER       5
-#define INTERRUPT_CAUSE_EXTERNAL    9
-
 void riscv_timer_interrupt(void);
 void riscv_software_interrupt(void);
 
-- 
2.18.0

[PATCH 07/11] RISC-V: add a definition for the SIE SEIE bit

[Christoph Hellwig]

This mirrors the SIE_SSIE and SETE bits that are used in a similar
fashion.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/csr.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 421fa3585798..28a0d1cb374c 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -54,6 +54,7 @@
 /* Interrupt Enable and Interrupt Pending flags */
 #define SIE_SSIE _AC(0x00000002, UL) /* Software Interrupt Enable */
 #define SIE_STIE _AC(0x00000020, UL) /* Timer Interrupt Enable */
+#define SIE_SEIE _AC(0x00000200, UL) /* External Interrupt Enable */
 
 #define EXC_INST_MISALIGNED     0
 #define EXC_INST_ACCESS         1
-- 
2.18.0

[PATCH 08/11] RISC-V: implement low-level interrupt handling

[Christoph Hellwig]

Add support for a routine that dispatches exceptions with the interrupt
flags set to either the IPI or irqdomain code (and the clock source in the
future).

Loosely based on the irq-riscv-int.c irqchip driver from the RISC-V tree.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/entry.S |  4 +--
 arch/riscv/kernel/irq.c   | 52 ++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 9aaf6c986771..fa2c08e3c05e 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -168,8 +168,8 @@ ENTRY(handle_exception)
 
 	/* Handle interrupts */
 	move a0, sp /* pt_regs */
-	REG_L a1, handle_arch_irq
-	jr a1
+	move a1, s4 /* scause */
+	tail do_IRQ
 1:
 	/* Exceptions run with interrupts enabled */
 	csrs sstatus, SR_SIE
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 7bcdaed15703..ab5f3e22c7cc 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -1,21 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Regents of the University of California
  * Copyright (C) 2017 SiFive
- *
- *   This program is free software; you can redistribute it and/or
- *   modify it under the terms of the GNU General Public License
- *   as published by the Free Software Foundation, version 2.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY; without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *   GNU General Public License for more details.
+ * Copyright (C) 2018 Christoph Hellwig
  */
 
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
 
+/*
+ * Possible interrupt causes:
+ */
+#define INTERRUPT_CAUSE_SOFTWARE    1
+#define INTERRUPT_CAUSE_TIMER       5
+#define INTERRUPT_CAUSE_EXTERNAL    9
+
+/*
+ * The high order bit of the trap cause register is always set for
+ * interrupts, which allows us to differentiate them from exceptions
+ * quickly.  The INTERRUPT_CAUSE_* macros don't contain that bit, so we
+ * need to mask it off.
+ */
+#define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
+
+asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	irq_enter();
+	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
+#ifdef CONFIG_SMP
+	case INTERRUPT_CAUSE_SOFTWARE:
+		/*
+		 * We only use software interrupts to pass IPIs, so if a non-SMP
+		 * system gets one, then we don't know what to do.
+		 */
+		riscv_software_interrupt();
+		break;
+#endif
+	case INTERRUPT_CAUSE_EXTERNAL:
+		handle_arch_irq(regs);
+		break;
+	default:
+		panic("unexpected interrupt cause");
+	}
+	irq_exit();
+
+	set_irq_regs(old_regs);
+}
+
 void __init init_IRQ(void)
 {
 	irqchip_init();
-- 
2.18.0

[PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@sifive.com>

Follow the updated DT specs and read the timebase-frequency from the
CPU 0 node.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: updated changelog]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/time.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 0df9b2cbd645..1bb01dc2d0f1 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -24,17 +24,24 @@ void __init init_clockevent(void)
 	csr_set(sie, SIE_STIE);
 }
 
-void __init time_init(void)
+static long __init timebase_frequency(void)
 {
 	struct device_node *cpu;
 	u32 prop;
 
 	cpu = of_find_node_by_path("/cpus");
-	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
-		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
-	riscv_timebase = prop;
+	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
+		return prop;
+	cpu = of_find_node_by_path("/cpus/cpu@0");
+	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
+		return prop;
 
-	lpj_fine = riscv_timebase / HZ;
+	panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
+}
 
+void __init time_init(void)
+{
+	riscv_timebase = timebase_frequency();
+	lpj_fine = riscv_timebase / HZ;
 	init_clockevent();
 }
-- 
2.18.0

[PATCH 10/11] irqchip: add a SiFive PLIC driver

[Christoph Hellwig]

Adds a driver for the SiFive implementation of the RISC-V Platform Level
Interrupt Controller (PLIC).  The PLIC connects global interrupt sources
to the local interrupt controller on each hart.

This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
but has been almost entirely rewritten since, and includes many fixes
from Atish Patra.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/configs/defconfig      |   1 +
 drivers/irqchip/Kconfig           |  12 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-sifive-plic.c | 259 ++++++++++++++++++++++++++++++
 4 files changed, 273 insertions(+)
 create mode 100644 drivers/irqchip/irq-sifive-plic.c

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 07326466871b..36473d7dbaac 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -76,3 +76,4 @@ CONFIG_ROOT_NFS=y
 CONFIG_CRYPTO_USER_API_HASH=y
 CONFIG_MODULES=y
 CONFIG_MODULE_UNLOAD=y
+CONFIG_SIFIVE_PLIC=y
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e9233db16e03..df345b878ac2 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -372,3 +372,15 @@ config QCOM_PDC
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
 endmenu
+
+config SIFIVE_PLIC
+	bool "SiFive Platform-Level Interrupt Controller"
+	depends on RISCV
+	help
+	   This enables support for the PLIC chip found in SiFive (and
+	   potentially other) RISC-V systems.  The PLIC controls devices
+	   interrupts and connects them to each core's local interrupt
+	   controller.  Aside from timer and software interrupts, all other
+	   interrupt sources are subordinate to the PLIC.
+
+	   If you don't know what to do here, say Y.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..fbd1ec8070ef 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -87,3 +87,4 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
new file mode 100644
index 000000000000..d2fb8364dec5
--- /dev/null
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ * Copyright (C) 2018 Christoph Hellwig
+ */
+#define pr_fmt(fmt) "plic: " fmt
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/*
+ * This driver implements a version of the RISC-V PLIC with the actual layout
+ * specified in chapter 8 of the SiFive U5 Coreplex Series Manual:
+ *
+ *     https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf
+ *
+ * The largest number supported by devices marked as 'riscv,plic0', is 1024, of
+ * which device 0 is defined as non-existant by the RISC-V Priviledged Spec.
+ */
+
+#define MAX_DEVICES			1024
+#define MAX_CONTEXTS			15872
+
+/*
+ * Each interrupt source has a priority register associated with it.
+ * We always hardwire it to one in Linux.
+ */
+#define PRIORITY_BASE			0
+#define     PRIORITY_PER_ID		4
+
+/*
+ * Each hart context has a vector of interupt enable bits associated with it.
+ * There's one bit for each interrupt source.
+ */
+#define ENABLE_BASE			0x2000
+#define     ENABLE_PER_HART		0x80
+
+/*
+ * Each hart context has a set of control registers associated with it.  Right
+ * now there's only two: a source priority threshold over which the hart will
+ * take an interrupt, and a register to claim interrupts.
+ */
+#define CONTEXT_BASE			0x200000
+#define     CONTEXT_PER_HART		0x1000
+#define     CONTEXT_THRESHOLD		0x00
+#define     CONTEXT_CLAIM		0x04
+
+static void __iomem *plic_regs;
+
+struct plic_handler {
+	bool			present;
+	int			ctxid;
+};
+static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
+
+static inline void __iomem *plic_hart_offset(int ctxid)
+{
+	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
+}
+
+static inline u32 __iomem *plic_enable_base(int ctxid)
+{
+	return plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
+}
+
+/*
+ * Protect mask operations on the registers given that we can't assume that
+ * atomic memory operations work on them.
+ */
+static DEFINE_RAW_SPINLOCK(plic_toggle_lock);
+
+static inline void plic_toggle(int ctxid, int hwirq, int enable)
+{
+	u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
+	u32 hwirq_mask = 1 << (hwirq % 32);
+
+	raw_spin_lock(&plic_toggle_lock);
+	if (enable)
+		writel(readl(reg) | hwirq_mask, reg);
+	else
+		writel(readl(reg) & ~hwirq_mask, reg);
+	raw_spin_unlock(&plic_toggle_lock);
+}
+
+static inline void plic_irq_toggle(struct irq_data *d, int enable)
+{
+	int cpu;
+
+	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
+	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
+		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
+
+		if (handler->present)
+			plic_toggle(handler->ctxid, d->hwirq, enable);
+	}
+}
+
+static void plic_irq_enable(struct irq_data *d)
+{
+	plic_irq_toggle(d, 1);
+}
+
+static void plic_irq_disable(struct irq_data *d)
+{
+	plic_irq_toggle(d, 0);
+}
+
+static struct irq_chip plic_chip = {
+	.name		= "SiFive PLIC",
+	/*
+	 * There is no need to mask/unmask PLIC interrupts.  They are "masked"
+	 * by reading claim and "unmasked" when writing it back.
+	 */
+	.irq_enable	= plic_irq_enable,
+	.irq_disable	= plic_irq_disable,
+};
+
+static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
+	irq_set_chip_data(irq, NULL);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static const struct irq_domain_ops plic_irqdomain_ops = {
+	.map		= plic_irqdomain_map,
+	.xlate		= irq_domain_xlate_onecell,
+};
+
+static struct irq_domain *plic_irqdomain;
+
+/*
+ * Handling an interrupt is a two-step process: first you claim the interrupt
+ * by reading the claim register, then you complete the interrupt by writing
+ * that source ID back to the same claim register.  This automatically enables
+ * and disables the interrupt, so there's nothing else to do.
+ */
+static void plic_handle_irq(struct pt_regs *regs)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+	void __iomem *claim = plic_hart_offset(handler->ctxid) + CONTEXT_CLAIM;
+	irq_hw_number_t hwirq;
+
+	WARN_ON_ONCE(!handler->present);
+
+	csr_clear(sie, SIE_STIE);
+	while ((hwirq = readl(claim))) {
+		int irq = irq_find_mapping(plic_irqdomain, hwirq);
+
+		if (unlikely(irq <= 0))
+			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
+					hwirq);
+		else
+			generic_handle_irq(irq);
+		writel(hwirq, claim);
+	}
+	csr_set(sie, SIE_STIE);
+}
+
+/*
+ * Walk up the DT tree until we find a active RISC-V core (HART) node and
+ * extract the cpuid from it.
+ */
+static int plic_find_hart_id(struct device_node *node)
+{
+	for (; node; node = node->parent) {
+		if (of_device_is_compatible(node, "riscv"))
+			return riscv_of_processor_hart(node);
+	}
+
+	return -1;
+}
+
+static int __init plic_init(struct device_node *node,
+		struct device_node *parent)
+{
+	int error = 0, nr_handlers, nr_mapped = 0, i;
+	u32 nr_irqs;
+
+	if (plic_regs) {
+		pr_warning("PLIC already present.\n");
+		return -ENXIO;
+	}
+
+	plic_regs = of_iomap(node, 0);
+	if (WARN_ON(!plic_regs))
+		return -EIO;
+
+	error = -EINVAL;
+	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
+	if (WARN_ON(!nr_irqs))
+		goto out_iounmap;
+
+	nr_handlers = of_irq_count(node);
+	if (WARN_ON(!nr_handlers))
+		goto out_iounmap;
+	if (WARN_ON(nr_handlers < num_possible_cpus()))
+		goto out_iounmap;
+
+	error = -ENOMEM;
+	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
+			&plic_irqdomain_ops, NULL);
+	if (WARN_ON(!plic_irqdomain))
+		goto out_iounmap;
+
+	for (i = 0; i < nr_handlers; i++) {
+		struct of_phandle_args parent;
+		struct plic_handler *handler;
+		irq_hw_number_t hwirq;
+		int cpu;
+
+		if (of_irq_parse_one(node, i, &parent)) {
+			pr_err("failed to parse parent for contxt %d.\n", i);
+			continue;
+		}
+
+		/* skip context holes */
+		if (parent.args[0] == -1)
+			continue;
+
+		cpu = plic_find_hart_id(parent.np->parent);
+		if (cpu < 0) {
+			pr_warn("failed to parse hart ID for context %d.\n", i);
+			continue;
+		}
+
+		handler = per_cpu_ptr(&plic_handlers, cpu);
+		handler->present = true;
+		handler->ctxid = i;
+
+		/* priority must be > threshold to trigger an interrupt */
+		writel(0, plic_hart_offset(i) + CONTEXT_THRESHOLD);
+		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
+			plic_toggle(i, hwirq, 0);
+		nr_mapped++;
+	}
+
+	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
+		nr_irqs, nr_mapped, nr_handlers);
+	set_handle_irq(plic_handle_irq);
+	return 0;
+
+out_iounmap:
+	iounmap(plic_regs);
+	return error;
+}
+
+IRQCHIP_DECLARE(sifive_plic0, "sifive,plic0", plic_init);
+IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
-- 
2.18.0

[PATCH 11/11] clocksource: new RISC-V SBI timer driver

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@dabbelt.com>

The RISC-V ISA defines a per-hart real-time clock and timer, which is
present on all systems.  The clock is accessed via the 'rdtime'
pseudo-instruction (which reads a CSR), and the timer is set via an SBI
call.

Contains various improvements from Atish Patra <atish.patra@wdc.com>.

Signed-off-by: Dmitriy Cherkasov <dmitriy@oss-tech.org>
Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: remove dead code, add SPDX tags, minor cleanups, merged
 hotplug cpu and other improvements from Atish]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/smp.h      |   3 -
 arch/riscv/kernel/irq.c           |   3 +
 arch/riscv/kernel/smpboot.c       |   1 -
 arch/riscv/kernel/time.c          |   8 +-
 drivers/clocksource/Kconfig       |  11 +++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/riscv_timer.c | 119 ++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h        |   1 +
 8 files changed, 136 insertions(+), 11 deletions(-)
 create mode 100644 drivers/clocksource/riscv_timer.c

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index c9395fff246f..36016845461d 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -24,9 +24,6 @@
 
 #ifdef CONFIG_SMP
 
-/* SMP initialization hook for setup_arch */
-void __init init_clockevent(void);
-
 /* SMP initialization hook for setup_arch */
 void __init setup_smp(void);
 
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index ab5f3e22c7cc..0cfac48a1272 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -30,6 +30,9 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
 
 	irq_enter();
 	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
+	case INTERRUPT_CAUSE_TIMER:
+		riscv_timer_interrupt();
+		break;
 #ifdef CONFIG_SMP
 	case INTERRUPT_CAUSE_SOFTWARE:
 		/*
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index f741458c5a3f..56abab6a9812 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -104,7 +104,6 @@ asmlinkage void __init smp_callin(void)
 	current->active_mm = mm;
 
 	trap_init();
-	init_clockevent();
 	notify_cpu_starting(smp_processor_id());
 	set_cpu_online(smp_processor_id(), 1);
 	local_flush_tlb_all();
diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 1bb01dc2d0f1..94e9ca18f5fa 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -18,12 +18,6 @@
 
 unsigned long riscv_timebase;
 
-void __init init_clockevent(void)
-{
-	timer_probe();
-	csr_set(sie, SIE_STIE);
-}
-
 static long __init timebase_frequency(void)
 {
 	struct device_node *cpu;
@@ -43,5 +37,5 @@ void __init time_init(void)
 {
 	riscv_timebase = timebase_frequency();
 	lpj_fine = riscv_timebase / HZ;
-	init_clockevent();
+	timer_probe();
 }
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index dec0dd88ec15..a11f4ba98b05 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -609,4 +609,15 @@ config ATCPIT100_TIMER
 	help
 	  This option enables support for the Andestech ATCPIT100 timers.
 
+config RISCV_TIMER
+	bool "Timer for the RISC-V platform"
+	depends on RISCV
+	default y
+	select TIMER_PROBE
+	select TIMER_OF
+	help
+	  This enables the per-hart timer built into all RISC-V systems, which
+	  is accessed via both the SBI and the rdcycle instruction.  This is
+	  required for all RISC-V systems.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 00caf37e52f9..ded31f720bd9 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
 obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
+obj-$(CONFIG_RISCV_TIMER)		+= riscv_timer.o
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
new file mode 100644
index 000000000000..c1e46170c068
--- /dev/null
+++ b/drivers/clocksource/riscv_timer.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ */
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/cpu.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <asm/sbi.h>
+
+/*
+ * All RISC-V systems have a timer attached to every hart.  These timers can be
+ * read by the 'rdcycle' pseudo instruction, and can use the SBI to setup
+ * events.  In order to abstract the architecture-specific timer reading and
+ * setting functions away from the clock event insertion code, we provide
+ * function pointers to the clockevent subsystem that perform two basic
+ * operations: rdtime() reads the timer on the current CPU, and
+ * next_event(delta) sets the next timer event to 'delta' cycles in the future.
+ * As the timers are inherently a per-cpu resource, these callbacks perform
+ * operations on the current hart.  There is guaranteed to be exactly one timer
+ * per hart on all RISC-V systems.
+ */
+
+static int riscv_clock_next_event(unsigned long delta,
+		struct clock_event_device *ce)
+{
+	csr_set(sie, SIE_STIE);
+	sbi_set_timer(get_cycles64() + delta);
+	return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
+	.name			= "riscv_timer_clockevent",
+	.features		= CLOCK_EVT_FEAT_ONESHOT,
+	.rating			= 100,
+	.set_next_event		= riscv_clock_next_event,
+};
+
+/*
+ * It is guarnteed that all the timers across all the harts are synchronized
+ * within one tick of each other, so while this could technically go
+ * backwards when hopping between CPUs, practically it won't happen.
+ */
+static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
+{
+	return get_cycles64();
+}
+
+static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+	.name		= "riscv_clocksource",
+	.rating		= 300,
+	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+	.read		= riscv_clocksource_rdtime,
+};
+
+static int riscv_timer_starting_cpu(unsigned int cpu)
+{
+	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
+
+	ce->cpumask = cpumask_of(cpu);
+	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);
+
+	csr_set(sie, SIE_STIE);
+	return 0;
+}
+
+static int riscv_timer_dying_cpu(unsigned int cpu)
+{
+	csr_clear(sie, SIE_STIE);
+	return 0;
+}
+
+/* called directly from the low-level interrupt handler */
+void riscv_timer_interrupt(void)
+{
+	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
+
+	csr_clear(sie, SIE_STIE);
+	evdev->event_handler(evdev);
+}
+
+static int riscv_timer_hart_id(struct device_node *dev)
+{
+	u32 hart;
+
+	if (!dev)
+		return -1;
+	if (!of_device_is_compatible(dev, "riscv"))
+		return -1;
+	if (of_property_read_u32(dev, "reg", &hart))
+		return -1;
+
+	return hart;
+}
+
+static int __init riscv_timer_init_dt(struct device_node *n)
+{
+	int cpu_id = riscv_timer_hart_id(n), error;
+	struct clocksource *cs;
+
+	if (cpu_id != smp_processor_id())
+		return 0;
+
+	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
+	clocksource_register_hz(cs, riscv_timebase);
+
+	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
+			 "clockevents/riscv/timer:starting",
+			 riscv_timer_starting_cpu, riscv_timer_dying_cpu);
+	if (error)
+		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
+		       error, cpu_id);
+	return error;
+}
+
+TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 8796ba387152..554c27f6cfbd 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -125,6 +125,7 @@ enum cpuhp_state {
 	CPUHP_AP_MARCO_TIMER_STARTING,
 	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 	CPUHP_AP_ARC_TIMER_STARTING,
+	CPUHP_AP_RISCV_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_STARTING,
-- 
2.18.0

Re: simplified RISC-V interrupt and clocksource handling v2

[Palmer Dabbelt]

On Thu, 02 Aug 2018 04:49:57 PDT (-0700), Christoph Hellwig wrote:
> This series tries adds support for interrupt handling and timers
> for the RISC-V architecture.
>
> The basic per-hart interrupt handling implemented by the scause
> and sie CSRs is extremely simple and implemented directly in
> arch/riscv/kernel/irq.c.  In addition there is a irqchip driver
> for the PLIC external interrupt controller, which is called through
> the set_handle_irq API, and a clocksource driver that gets its
> timer interrupt directly from the low-level interrupt handling.
>
> Compared to previous iterations this version does not try to use an
> irqchip driver for the low-level interrupt handling.  This saves
> a couple indirect calls and an additional read of the scause CSR
> in the hot path, makes the code much simpler and last but not least
> avoid the dependency on a device tree for a mandatory architectural
> feature.
>
> A git tree is available here (contains a few more patches before
> the ones in this series)
>
>     git://git.infradead.org/users/hch/riscv.git riscv-irq-simple.2
>
> Gitweb:
>
>     http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple.2
>
> Changes since v1:
>  - rename the plic driver to irq-sifive-plic
>  - switch to a default compatible of sifive,plic0 (still supporting the
>    riscv,plic0 name for compatibility)
>  - add a reference for the SiFive PLIC register layout
>  - fix plic_toggle addressing for large numbers of hwirqs
>  - remove the call to ack_bad_irq
>  - use a raw spinlock for plic_toggle_lock
>  - use the irq_desc cpumask in the plic enable/disable methods
>  - add back OF contexid parsing in the plic driver
>  - don't allow COMPILE_TEST builds of the clocksource driver, as it
>    depends on <asm/sbi.h>
>  - default the clocksource driver to y
>  - clean up naming in the clocksource driver
>  - remove the MINDELTA and MAXDELTA #defines
>  - various DT binding fixes

Ah, thank you so much.  This is great!  With this patch set applied on top of 
rc7 I can boot QEMU master and get to the Fedora root file system.  I'll review 
the patch set properly, but at least for now I think a

Tested-by: Palmer Dabbelt <palmer@sifive.com>

is warranted.  What's the best way to go about merging this?  There's quite a 
bit of arch/riscv diff here so I don't mind taking it through the RISC-V tree, 
but there's also some irqchip and clocksource stuff as well so I'm not sure if 
that's OK to do.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Atish Patra]

On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> This patch adds documentation for the platform-level interrupt
> controller (PLIC) found in all RISC-V systems.  This interrupt
> controller routes interrupts from all the devices in the system to each
> hart-local interrupt controller.
> 
> Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> want to change how we're specifying holes in the hart list.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: various fixes and updates]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>   1 file changed, 57 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> new file mode 100644
> index 000000000000..c756cd208a93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> @@ -0,0 +1,57 @@
> +SiFive Platform-Level Interrupt Controller (PLIC)
> +-------------------------------------------------
> +
> +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> +(PLIC) high-level specification in the RISC-V Privileged Architecture
> +specification.  The PLIC connects all external interrupts in the system to all
> +hart contexts in the system, via the external interrupt source in each hart.
> +
> +A hart context is a privilege mode in a hardware execution thread.  For example,
> +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> +privilege modes per hart; machine mode and supervisor mode.
> +
> +Each interrupt can be enabled on per-context basis. Any context can claim
> +a pending enabled interrupt and then release it once it has been handled.
> +
> +Each interrupt has a configurable priority. Higher priority interrupts are
> +serviced first. Each context can specify a priority threshold. Interrupts
> +with priority below this threshold will not cause the PLIC to raise its
> +interrupt line leading to the context.
> +
> +While the PLIC supports both edge-triggered and level-triggered interrupts,
> +interrupt handlers are oblivious to this distinction and therefore it is not
> +specified in the PLIC device-tree binding.
> +
> +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> +specific memory layout, which is documented in chapter 8 of the SiFive U5
> +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> +
> +Required properties:
> +- compatible : "sifive,plic0"
> +- #address-cells : should be <0>
> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- reg : Should contain 1 register range (address and length)

The one in the real device tree has two entries.
reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;

Is it intentional or just incorrect entry left over from earlier days?

Regards,
Atish
> +- interrupts-extended : Specifies which contexts are connected to the PLIC,
> +  with "-1" specifying that a context is not present.  The nodes pointed
> +  to should be "riscv" HART nodes, or eventually be parented by such nodes.
> +- riscv,ndev: Specifies how many external interrupts are supported by
> +  this controller.
> +
> +Example:
> +
> +	plic: interrupt-controller@c000000 {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		compatible = "riscv,plic0";
> +		interrupt-controller;
> +		interrupts-extended = <
> +			&cpu0-intc 11
> +			&cpu1-intc 11 &cpu1-intc 9
> +			&cpu2-intc 11 &cpu2-intc 9
> +			&cpu3-intc 11 &cpu3-intc 9
> +			&cpu4-intc 11 &cpu4-intc 9>;
> +		reg = <0xc000000 0x4000000>;
> +		riscv,ndev = <10>;
> +	};
> 

Re: [PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> From: Palmer Dabbelt <palmer@sifive.com>
> 
> Follow the updated DT specs and read the timebase-frequency from the
> CPU 0 node.
> 

However, the DT in the HighFive Unleashed has the entry at the wrong place.

Even the example in github also at wrong place.
https://github.com/riscv/riscv-device-tree-doc/pull/8/commits/2461d481329c55005fcbe684f0d6bdb3b7f0a432

DT should be consistent between Documentation and the one in the 
hardware. I can fix them in bbl & submit a bbl patch. But I am not sure 
if that's an acceptable way to do it.

Regards,
Atish
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: updated changelog]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/kernel/time.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> index 0df9b2cbd645..1bb01dc2d0f1 100644
> --- a/arch/riscv/kernel/time.c
> +++ b/arch/riscv/kernel/time.c
> @@ -24,17 +24,24 @@ void __init init_clockevent(void)
>   	csr_set(sie, SIE_STIE);
>   }
>   
> -void __init time_init(void)
> +static long __init timebase_frequency(void)
>   {
>   	struct device_node *cpu;
>   	u32 prop;
>   
>   	cpu = of_find_node_by_path("/cpus");
> -	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
> -		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
> -	riscv_timebase = prop;
> +	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
> +		return prop;
> +	cpu = of_find_node_by_path("/cpus/cpu@0");
> +	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
> +		return prop;
>   
> -	lpj_fine = riscv_timebase / HZ;
> +	panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
> +}
>   
> +void __init time_init(void)
> +{
> +	riscv_timebase = timebase_frequency();
> +	lpj_fine = riscv_timebase / HZ;
>   	init_clockevent();
>   }
> 

Re: [PATCH 10/11] irqchip: add a SiFive PLIC driver

[Atish Patra]

On 8/2/18 4:51 AM, Christoph Hellwig wrote:
> Adds a driver for the SiFive implementation of the RISC-V Platform Level
> Interrupt Controller (PLIC).  The PLIC connects global interrupt sources
> to the local interrupt controller on each hart.
> 
> This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
> but has been almost entirely rewritten since, and includes many fixes
> from Atish Patra.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/configs/defconfig      |   1 +
>   drivers/irqchip/Kconfig           |  12 ++
>   drivers/irqchip/Makefile          |   1 +
>   drivers/irqchip/irq-sifive-plic.c | 259 ++++++++++++++++++++++++++++++
>   4 files changed, 273 insertions(+)
>   create mode 100644 drivers/irqchip/irq-sifive-plic.c
> 
> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> index 07326466871b..36473d7dbaac 100644
> --- a/arch/riscv/configs/defconfig
> +++ b/arch/riscv/configs/defconfig
> @@ -76,3 +76,4 @@ CONFIG_ROOT_NFS=y
>   CONFIG_CRYPTO_USER_API_HASH=y
>   CONFIG_MODULES=y
>   CONFIG_MODULE_UNLOAD=y
> +CONFIG_SIFIVE_PLIC=y
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e9233db16e03..df345b878ac2 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -372,3 +372,15 @@ config QCOM_PDC
>   	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>   
>   endmenu
> +
> +config SIFIVE_PLIC
> +	bool "SiFive Platform-Level Interrupt Controller"
> +	depends on RISCV
> +	help
> +	   This enables support for the PLIC chip found in SiFive (and
> +	   potentially other) RISC-V systems.  The PLIC controls devices
> +	   interrupts and connects them to each core's local interrupt
> +	   controller.  Aside from timer and software interrupts, all other
> +	   interrupt sources are subordinate to the PLIC.
> +
> +	   If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..fbd1ec8070ef 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -87,3 +87,4 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>   obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>   obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>   obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> new file mode 100644
> index 000000000000..d2fb8364dec5
> --- /dev/null
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2018 Christoph Hellwig
> + */
> +#define pr_fmt(fmt) "plic: " fmt
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * This driver implements a version of the RISC-V PLIC with the actual layout
> + * specified in chapter 8 of the SiFive U5 Coreplex Series Manual:
> + *
> + *     https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf
> + *
> + * The largest number supported by devices marked as 'riscv,plic0', is 1024, of
> + * which device 0 is defined as non-existant by the RISC-V Priviledged Spec.
/s/existant/existent

/s/Priviledged/Privileged
> + */
> +
> +#define MAX_DEVICES			1024
> +#define MAX_CONTEXTS			15872
> +
> +/*
> + * Each interrupt source has a priority register associated with it.
> + * We always hardwire it to one in Linux.
> + */
> +#define PRIORITY_BASE			0
> +#define     PRIORITY_PER_ID		4
> +
> +/*
> + * Each hart context has a vector of interupt enable bits associated with it.
/s/interupt/interrupt

> + * There's one bit for each interrupt source.
> + */
> +#define ENABLE_BASE			0x2000
> +#define     ENABLE_PER_HART		0x80
> +
> +/*
> + * Each hart context has a set of control registers associated with it.  Right
> + * now there's only two: a source priority threshold over which the hart will
> + * take an interrupt, and a register to claim interrupts.
> + */
> +#define CONTEXT_BASE			0x200000
> +#define     CONTEXT_PER_HART		0x1000
> +#define     CONTEXT_THRESHOLD		0x00
> +#define     CONTEXT_CLAIM		0x04
> +
> +static void __iomem *plic_regs;
> +
> +struct plic_handler {
> +	bool			present;
> +	int			ctxid;
> +};
> +static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> +
> +static inline void __iomem *plic_hart_offset(int ctxid)
> +{
> +	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
> +}
> +
> +static inline u32 __iomem *plic_enable_base(int ctxid)
> +{
> +	return plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
> +}
> +
> +/*
> + * Protect mask operations on the registers given that we can't assume that
> + * atomic memory operations work on them.
> + */
> +static DEFINE_RAW_SPINLOCK(plic_toggle_lock);
> +
> +static inline void plic_toggle(int ctxid, int hwirq, int enable)
> +{
> +	u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
> +	u32 hwirq_mask = 1 << (hwirq % 32);
> +
> +	raw_spin_lock(&plic_toggle_lock);
> +	if (enable)
> +		writel(readl(reg) | hwirq_mask, reg);
> +	else
> +		writel(readl(reg) & ~hwirq_mask, reg);
> +	raw_spin_unlock(&plic_toggle_lock);
> +}
> +
> +static inline void plic_irq_toggle(struct irq_data *d, int enable)
> +{
> +	int cpu;
> +
> +	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> +	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
> +		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> +
> +		if (handler->present)
> +			plic_toggle(handler->ctxid, d->hwirq, enable);
> +	}
> +}
> +
> +static void plic_irq_enable(struct irq_data *d)
> +{
> +	plic_irq_toggle(d, 1);
> +}
> +
> +static void plic_irq_disable(struct irq_data *d)
> +{
> +	plic_irq_toggle(d, 0);
> +}
> +
> +static struct irq_chip plic_chip = {
> +	.name		= "SiFive PLIC",
> +	/*
> +	 * There is no need to mask/unmask PLIC interrupts.  They are "masked"
> +	 * by reading claim and "unmasked" when writing it back.
> +	 */
> +	.irq_enable	= plic_irq_enable,
> +	.irq_disable	= plic_irq_disable,
> +};
> +
> +static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, NULL);
> +	irq_set_noprobe(irq);
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops plic_irqdomain_ops = {
> +	.map		= plic_irqdomain_map,
> +	.xlate		= irq_domain_xlate_onecell,
> +};
> +
> +static struct irq_domain *plic_irqdomain;
> +
> +/*
> + * Handling an interrupt is a two-step process: first you claim the interrupt
> + * by reading the claim register, then you complete the interrupt by writing
> + * that source ID back to the same claim register.  This automatically enables
> + * and disables the interrupt, so there's nothing else to do.
> + */
> +static void plic_handle_irq(struct pt_regs *regs)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +	void __iomem *claim = plic_hart_offset(handler->ctxid) + CONTEXT_CLAIM;
> +	irq_hw_number_t hwirq;
> +
> +	WARN_ON_ONCE(!handler->present);
> +
> +	csr_clear(sie, SIE_STIE);

We should clear the SIE_SEIE not SIE_STIE. Correct ?

> +	while ((hwirq = readl(claim))) {
> +		int irq = irq_find_mapping(plic_irqdomain, hwirq);
> +
> +		if (unlikely(irq <= 0))
> +			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
> +					hwirq);
> +		else
> +			generic_handle_irq(irq);
> +		writel(hwirq, claim);
> +	}
> +	csr_set(sie, SIE_STIE);

Same as above.
> +}
> +
> +/*
> + * Walk up the DT tree until we find a active RISC-V core (HART) node and
> + * extract the cpuid from it.
> + */
> +static int plic_find_hart_id(struct device_node *node)
> +{
> +	for (; node; node = node->parent) {
> +		if (of_device_is_compatible(node, "riscv"))
> +			return riscv_of_processor_hart(node);
> +	}
> +
> +	return -1;
> +}
> +
> +static int __init plic_init(struct device_node *node,
> +		struct device_node *parent)
> +{
> +	int error = 0, nr_handlers, nr_mapped = 0, i;
> +	u32 nr_irqs;
> +
> +	if (plic_regs) {
> +		pr_warning("PLIC already present.\n");

Got a check-patch warning.

WARNING: Prefer pr_warn(... to pr_warning(...
#257: FILE: drivers/irqchip/irq-sifive-plic.c:191:
+               pr_warning("PLIC already present.\n");

> +		return -ENXIO;
> +	}
> +
> +	plic_regs = of_iomap(node, 0);
> +	if (WARN_ON(!plic_regs))
> +		return -EIO;
> +
> +	error = -EINVAL;
> +	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
> +	if (WARN_ON(!nr_irqs))
> +		goto out_iounmap;
> +
> +	nr_handlers = of_irq_count(node);
> +	if (WARN_ON(!nr_handlers))
> +		goto out_iounmap;
> +	if (WARN_ON(nr_handlers < num_possible_cpus()))
> +		goto out_iounmap;
> +
> +	error = -ENOMEM;
> +	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
> +			&plic_irqdomain_ops, NULL);
> +	if (WARN_ON(!plic_irqdomain))
> +		goto out_iounmap;
> +
> +	for (i = 0; i < nr_handlers; i++) {
> +		struct of_phandle_args parent;
> +		struct plic_handler *handler;
> +		irq_hw_number_t hwirq;
> +		int cpu;
> +
> +		if (of_irq_parse_one(node, i, &parent)) {
> +			pr_err("failed to parse parent for contxt %d.\n", i);
/s/contxt/context


> +			continue;
> +		}
> +
> +		/* skip context holes */
> +		if (parent.args[0] == -1)
> +			continue;
> +
> +		cpu = plic_find_hart_id(parent.np->parent);

Since plic_find_hart_id() is going to walk up the DT, we can pass only 
parent.np instead of parent.np->parent. It does not increase any 
efficiency either way. So not very necessary. Just thought of taking the 
advantage of plic_find_hart_id.

Regards,
Atish
> +		if (cpu < 0) {
> +			pr_warn("failed to parse hart ID for context %d.\n", i);
> +			continue;
> +		}
> +
> +		handler = per_cpu_ptr(&plic_handlers, cpu);
> +		handler->present = true;
> +		handler->ctxid = i;
> +
> +		/* priority must be > threshold to trigger an interrupt */
> +		writel(0, plic_hart_offset(i) + CONTEXT_THRESHOLD);
> +		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> +			plic_toggle(i, hwirq, 0);
> +		nr_mapped++;
> +	}
> +
> +	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
> +		nr_irqs, nr_mapped, nr_handlers);
> +	set_handle_irq(plic_handle_irq);
> +	return 0;
> +
> +out_iounmap:
> +	iounmap(plic_regs);
> +	return error;
> +}
> +
> +IRQCHIP_DECLARE(sifive_plic0, "sifive,plic0", plic_init);
> +IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
> 

Re: [PATCH 11/11] clocksource: new RISC-V SBI timer driver

[Atish Patra]

On 8/2/18 4:51 AM, Christoph Hellwig wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> The RISC-V ISA defines a per-hart real-time clock and timer, which is
> present on all systems.  The clock is accessed via the 'rdtime'
> pseudo-instruction (which reads a CSR), and the timer is set via an SBI
> call.
> 
> Contains various improvements from Atish Patra <atish.patra@wdc.com>.
> 
> Signed-off-by: Dmitriy Cherkasov <dmitriy@oss-tech.org>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: remove dead code, add SPDX tags, minor cleanups, merged
>   hotplug cpu and other improvements from Atish]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/include/asm/smp.h      |   3 -
>   arch/riscv/kernel/irq.c           |   3 +
>   arch/riscv/kernel/smpboot.c       |   1 -
>   arch/riscv/kernel/time.c          |   8 +-
>   drivers/clocksource/Kconfig       |  11 +++
>   drivers/clocksource/Makefile      |   1 +
>   drivers/clocksource/riscv_timer.c | 119 ++++++++++++++++++++++++++++++
>   include/linux/cpuhotplug.h        |   1 +
>   8 files changed, 136 insertions(+), 11 deletions(-)
>   create mode 100644 drivers/clocksource/riscv_timer.c
> 
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index c9395fff246f..36016845461d 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -24,9 +24,6 @@
>   
>   #ifdef CONFIG_SMP
>   
> -/* SMP initialization hook for setup_arch */
> -void __init init_clockevent(void);
> -
>   /* SMP initialization hook for setup_arch */
>   void __init setup_smp(void);
>   
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index ab5f3e22c7cc..0cfac48a1272 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -30,6 +30,9 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
>   
>   	irq_enter();
>   	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
> +	case INTERRUPT_CAUSE_TIMER:
> +		riscv_timer_interrupt();
> +		break;
>   #ifdef CONFIG_SMP
>   	case INTERRUPT_CAUSE_SOFTWARE:
>   		/*
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index f741458c5a3f..56abab6a9812 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -104,7 +104,6 @@ asmlinkage void __init smp_callin(void)
>   	current->active_mm = mm;
>   
>   	trap_init();
> -	init_clockevent();
>   	notify_cpu_starting(smp_processor_id());
>   	set_cpu_online(smp_processor_id(), 1);
>   	local_flush_tlb_all();
> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> index 1bb01dc2d0f1..94e9ca18f5fa 100644
> --- a/arch/riscv/kernel/time.c
> +++ b/arch/riscv/kernel/time.c
> @@ -18,12 +18,6 @@
>   
>   unsigned long riscv_timebase;
>   
> -void __init init_clockevent(void)
> -{
> -	timer_probe();
> -	csr_set(sie, SIE_STIE);
> -}
> -
>   static long __init timebase_frequency(void)
>   {
>   	struct device_node *cpu;
> @@ -43,5 +37,5 @@ void __init time_init(void)
>   {
>   	riscv_timebase = timebase_frequency();
>   	lpj_fine = riscv_timebase / HZ;
> -	init_clockevent();
> +	timer_probe();
>   }
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index dec0dd88ec15..a11f4ba98b05 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -609,4 +609,15 @@ config ATCPIT100_TIMER
>   	help
>   	  This option enables support for the Andestech ATCPIT100 timers.
>   
> +config RISCV_TIMER
> +	bool "Timer for the RISC-V platform"
> +	depends on RISCV
> +	default y
> +	select TIMER_PROBE
> +	select TIMER_OF
> +	help
> +	  This enables the per-hart timer built into all RISC-V systems, which
> +	  is accessed via both the SBI and the rdcycle instruction.  This is
> +	  required for all RISC-V systems.
> +
>   endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..ded31f720bd9 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>   obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>   obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>   obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> +obj-$(CONFIG_RISCV_TIMER)		+= riscv_timer.o
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> new file mode 100644
> index 000000000000..c1e46170c068
> --- /dev/null
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + */
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpu.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <asm/sbi.h>
> +
> +/*
> + * All RISC-V systems have a timer attached to every hart.  These timers can be
> + * read by the 'rdcycle' pseudo instruction, and can use the SBI to setup
> + * events.  In order to abstract the architecture-specific timer reading and
> + * setting functions away from the clock event insertion code, we provide
> + * function pointers to the clockevent subsystem that perform two basic
> + * operations: rdtime() reads the timer on the current CPU, and
> + * next_event(delta) sets the next timer event to 'delta' cycles in the future.
> + * As the timers are inherently a per-cpu resource, these callbacks perform
> + * operations on the current hart.  There is guaranteed to be exactly one timer
> + * per hart on all RISC-V systems.
> + */
> +
> +static int riscv_clock_next_event(unsigned long delta,
> +		struct clock_event_device *ce)
> +{
> +	csr_set(sie, SIE_STIE);
> +	sbi_set_timer(get_cycles64() + delta);
> +	return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
> +	.name			= "riscv_timer_clockevent",
> +	.features		= CLOCK_EVT_FEAT_ONESHOT,
> +	.rating			= 100,
> +	.set_next_event		= riscv_clock_next_event,
> +};
> +
> +/*
> + * It is guarnteed that all the timers across all the harts are synchronized

/s/guarnteed/guaranteed
> + * within one tick of each other, so while this could technically go
> + * backwards when hopping between CPUs, practically it won't happen.
> + */
> +static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
> +{
> +	return get_cycles64();
> +}
> +
> +static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
> +	.name		= "riscv_clocksource",
> +	.rating		= 300,
> +	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
> +	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> +	.read		= riscv_clocksource_rdtime,
> +};
> +
> +static int riscv_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
> +
> +	ce->cpumask = cpumask_of(cpu);
> +	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);
> +
> +	csr_set(sie, SIE_STIE);
> +	return 0;
> +}
> +
> +static int riscv_timer_dying_cpu(unsigned int cpu)
> +{
> +	csr_clear(sie, SIE_STIE);
> +	return 0;
> +}
> +
> +/* called directly from the low-level interrupt handler */
> +void riscv_timer_interrupt(void)
> +{
> +	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
> +
> +	csr_clear(sie, SIE_STIE);
> +	evdev->event_handler(evdev);
> +}
> +
> +static int riscv_timer_hart_id(struct device_node *dev)
> +{
> +	u32 hart;
> +
> +	if (!dev)
> +		return -1;
> +	if (!of_device_is_compatible(dev, "riscv"))
> +		return -1;
> +	if (of_property_read_u32(dev, "reg", &hart))
> +		return -1;
> +
> +	return hart;
> +}
> +
> +static int __init riscv_timer_init_dt(struct device_node *n)
> +{
> +	int cpu_id = riscv_timer_hart_id(n), error;
> +	struct clocksource *cs;
> +
> +	if (cpu_id != smp_processor_id())
> +		return 0;
> +
> +	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
> +	clocksource_register_hz(cs, riscv_timebase);
> +
> +	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
> +			 "clockevents/riscv/timer:starting",
> +			 riscv_timer_starting_cpu, riscv_timer_dying_cpu);
> +	if (error)
> +		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> +		       error, cpu_id);
> +	return error;
> +}
> +
> +TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 8796ba387152..554c27f6cfbd 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -125,6 +125,7 @@ enum cpuhp_state {
>   	CPUHP_AP_MARCO_TIMER_STARTING,
>   	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>   	CPUHP_AP_ARC_TIMER_STARTING,
> +	CPUHP_AP_RISCV_TIMER_STARTING,
>   	CPUHP_AP_KVM_STARTING,
>   	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
>   	CPUHP_AP_KVM_ARM_VGIC_STARTING,
> 

Otherwise, FWIW

Reviewed-by: Atish Patra <atish.patra@wdc.com>

Re: simplified RISC-V interrupt and clocksource handling v2

[Thomas Gleixner]

On Thu, 2 Aug 2018, Palmer Dabbelt wrote:
> bit of arch/riscv diff here so I don't mind taking it through the RISC-V tree,
> but there's also some irqchip and clocksource stuff as well so I'm not sure if
> that's OK to do.

I have no objections if that goes through the risc-v tree once the DT stuff
is sorted out.

For the clocksource and irqchip bits:

    Acked-by: Thomas Gleixner <tglx@linutronix.de>

Thanks,

	tglx

Re: [PATCH 10/11] irqchip: add a SiFive PLIC driver

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 04:13:26PM -0700, Atish Patra wrote:
>> + * which device 0 is defined as non-existant by the RISC-V Priviledged Spec.
> /s/existant/existent
>
> /s/Priviledged/Privileged
>> + * Each hart context has a vector of interupt enable bits associated with it.
> /s/interupt/interrupt

All fixed.

>> +	WARN_ON_ONCE(!handler->present);
>> +
>> +	csr_clear(sie, SIE_STIE);
>
> We should clear the SIE_SEIE not SIE_STIE. Correct ?

Yes, fixed.

>> +	if (plic_regs) {
>> +		pr_warning("PLIC already present.\n");
>
> Got a check-patch warning.
>
> WARNING: Prefer pr_warn(... to pr_warning(...
> #257: FILE: drivers/irqchip/irq-sifive-plic.c:191:
> +               pr_warning("PLIC already present.\n");

Fixed.

>> +
>> +		if (of_irq_parse_one(node, i, &parent)) {
>> +			pr_err("failed to parse parent for contxt %d.\n", i);
> /s/contxt/context

Fixed.

>> +			continue;
>> +		}
>> +
>> +		/* skip context holes */
>> +		if (parent.args[0] == -1)
>> +			continue;
>> +
>> +		cpu = plic_find_hart_id(parent.np->parent);
>
> Since plic_find_hart_id() is going to walk up the DT, we can pass only 
> parent.np instead of parent.np->parent. It does not increase any efficiency 
> either way. So not very necessary. Just thought of taking the advantage of 
> plic_find_hart_id.

Yeah, I'll update this.

Thanks for the review!

Re: [PATCH 11/11] clocksource: new RISC-V SBI timer driver

[Christoph Hellwig]

>> +/*
>> + * It is guarnteed that all the timers across all the harts are synchronized
>
> /s/guarnteed/guaranteed

Fixed.

Re: [PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 03:19:49PM -0700, Atish Patra wrote:
> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>> From: Palmer Dabbelt <palmer@sifive.com>
>>
>> Follow the updated DT specs and read the timebase-frequency from the
>> CPU 0 node.
>>
>
> However, the DT in the HighFive Unleashed has the entry at the wrong place.
>
> Even the example in github also at wrong place.
> https://github.com/riscv/riscv-device-tree-doc/pull/8/commits/2461d481329c55005fcbe684f0d6bdb3b7f0a432
>
> DT should be consistent between Documentation and the one in the hardware. 
> I can fix them in bbl & submit a bbl patch. But I am not sure if that's an 
> acceptable way to do it.

I'll need to have comments from Palmer and/or someone else at SiFive
here.  Personally I really don't care where we document the timebase,
as this patch supports both locations anywhere.  For now I'll just update
the commit log to state that more explicitly.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 03:08:15PM -0700, Atish Patra wrote:
> The one in the real device tree has two entries.
> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>
> Is it intentional or just incorrect entry left over from earlier days?

I'll need to SiFive folks to comment on that.  The driver itself uses
of_iomap, which as far as I can tell only needs two entries.

Re: [PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Christoph Hellwig]

Actually I had a slight change of heart: I'm going to drop both
timebase frequence patches and the enable method documentation one
from this series as they aren't strictly required to get a booting
qemu kernel.

Re: [PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Palmer Dabbelt]

On Fri, 03 Aug 2018 05:33:57 PDT (-0700), Christoph Hellwig wrote:
> On Thu, Aug 02, 2018 at 03:19:49PM -0700, Atish Patra wrote:
>> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>>> From: Palmer Dabbelt <palmer@sifive.com>
>>>
>>> Follow the updated DT specs and read the timebase-frequency from the
>>> CPU 0 node.
>>>
>>
>> However, the DT in the HighFive Unleashed has the entry at the wrong place.
>>
>> Even the example in github also at wrong place.
>> https://github.com/riscv/riscv-device-tree-doc/pull/8/commits/2461d481329c55005fcbe684f0d6bdb3b7f0a432
>>
>> DT should be consistent between Documentation and the one in the hardware.
>> I can fix them in bbl & submit a bbl patch. But I am not sure if that's an
>> acceptable way to do it.
>
> I'll need to have comments from Palmer and/or someone else at SiFive
> here.  Personally I really don't care where we document the timebase,
> as this patch supports both locations anywhere.  For now I'll just update
> the commit log to state that more explicitly.

You're welcome to submit a BBL patch to make this all match, but from my 
understanding of the device tree spec putting timebase-frequency in either 
place should be legal so it's not a critical fix.  That said, it's better to 
have them match than not match.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Rob Herring]

On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>
> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> > From: Palmer Dabbelt <palmer@dabbelt.com>
> >
> > This patch adds documentation for the platform-level interrupt
> > controller (PLIC) found in all RISC-V systems.  This interrupt
> > controller routes interrupts from all the devices in the system to each
> > hart-local interrupt controller.
> >
> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> > want to change how we're specifying holes in the hart list.
> >
> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > [hch: various fixes and updates]
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
> >   1 file changed, 57 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> > new file mode 100644
> > index 000000000000..c756cd208a93
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> > @@ -0,0 +1,57 @@
> > +SiFive Platform-Level Interrupt Controller (PLIC)
> > +-------------------------------------------------
> > +
> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
> > +specification.  The PLIC connects all external interrupts in the system to all
> > +hart contexts in the system, via the external interrupt source in each hart.
> > +
> > +A hart context is a privilege mode in a hardware execution thread.  For example,
> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> > +privilege modes per hart; machine mode and supervisor mode.
> > +
> > +Each interrupt can be enabled on per-context basis. Any context can claim
> > +a pending enabled interrupt and then release it once it has been handled.
> > +
> > +Each interrupt has a configurable priority. Higher priority interrupts are
> > +serviced first. Each context can specify a priority threshold. Interrupts
> > +with priority below this threshold will not cause the PLIC to raise its
> > +interrupt line leading to the context.
> > +
> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
> > +interrupt handlers are oblivious to this distinction and therefore it is not
> > +specified in the PLIC device-tree binding.
> > +
> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> > +
> > +Required properties:
> > +- compatible : "sifive,plic0"
> > +- #address-cells : should be <0>
> > +- #interrupt-cells : should be <1>
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- reg : Should contain 1 register range (address and length)
>
> The one in the real device tree has two entries.
> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>
> Is it intentional or just incorrect entry left over from earlier days?

> > +             reg = <0xc000000 0x4000000>;

Looks to me like one has #size-cells and #address-cells set to 2 and
the example is using 1.

Rob

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 02:59:48PM -0600, Rob Herring wrote:
> > > +Required properties:
> > > +- compatible : "sifive,plic0"
> > > +- #address-cells : should be <0>
> > > +- #interrupt-cells : should be <1>
> > > +- interrupt-controller : Identifies the node as an interrupt controller
> > > +- reg : Should contain 1 register range (address and length)
> >
> > The one in the real device tree has two entries.
> > reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
> >
> > Is it intentional or just incorrect entry left over from earlier days?
> 
> > > +             reg = <0xc000000 0x4000000>;
> 
> Looks to me like one has #size-cells and #address-cells set to 2 and
> the example is using 1.

Yes.  And it seems like the real life device tree is simply bogus.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Palmer Dabbelt]

On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
> On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>>
>> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>> > From: Palmer Dabbelt <palmer@dabbelt.com>
>> >
>> > This patch adds documentation for the platform-level interrupt
>> > controller (PLIC) found in all RISC-V systems.  This interrupt
>> > controller routes interrupts from all the devices in the system to each
>> > hart-local interrupt controller.
>> >
>> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
>> > want to change how we're specifying holes in the hart list.
>> >
>> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> > [hch: various fixes and updates]
>> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> > ---
>> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>> >   1 file changed, 57 insertions(+)
>> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> > new file mode 100644
>> > index 000000000000..c756cd208a93
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> > @@ -0,0 +1,57 @@
>> > +SiFive Platform-Level Interrupt Controller (PLIC)
>> > +-------------------------------------------------
>> > +
>> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
>> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
>> > +specification.  The PLIC connects all external interrupts in the system to all
>> > +hart contexts in the system, via the external interrupt source in each hart.
>> > +
>> > +A hart context is a privilege mode in a hardware execution thread.  For example,
>> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
>> > +privilege modes per hart; machine mode and supervisor mode.
>> > +
>> > +Each interrupt can be enabled on per-context basis. Any context can claim
>> > +a pending enabled interrupt and then release it once it has been handled.
>> > +
>> > +Each interrupt has a configurable priority. Higher priority interrupts are
>> > +serviced first. Each context can specify a priority threshold. Interrupts
>> > +with priority below this threshold will not cause the PLIC to raise its
>> > +interrupt line leading to the context.
>> > +
>> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
>> > +interrupt handlers are oblivious to this distinction and therefore it is not
>> > +specified in the PLIC device-tree binding.
>> > +
>> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
>> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
>> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
>> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
>> > +
>> > +Required properties:
>> > +- compatible : "sifive,plic0"

I think there was a thread bouncing around somewhere where decided to pick the 
official name of the compatible string to be "sifive,plic-1.0".  The idea here 
is that the PLIC is compatible across all of SiFive's current implementations, 
but there's some limitations in the memory map that will probably cause us to 
spin a version 2 at some point so we want major version number.  The minor 
number is just in case we find some errata in the PLIC.

>> > +- #address-cells : should be <0>
>> > +- #interrupt-cells : should be <1>
>> > +- interrupt-controller : Identifies the node as an interrupt controller
>> > +- reg : Should contain 1 register range (address and length)
>>
>> The one in the real device tree has two entries.
>> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>>
>> Is it intentional or just incorrect entry left over from earlier days?
>
>> > +             reg = <0xc000000 0x4000000>;
>
> Looks to me like one has #size-cells and #address-cells set to 2 and
> the example is using 1.

Yes.  For some background on how this works: we have a hardware generator that 
has a tree-of-busses abstraction, and each device is attached to some point on 
that tree.  When a device gets attached to the bus, we also generate a device 
tree entry.  For whatever system I generated the example PLIC device tree entry 
from, it must have been attached to a bus with addresses of 32 bits or less, 
which resulted in #address-cells and #size-cells being 1.

Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is 
probably not what I generated as an example -- the fu540-c000 is a complicated 
configuration, when I mess around with the hardware I tend to use simple ones 
as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging 
off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as 
the machine has 8GiB of memory and the PLIC is on a bus that's closer to the 
core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.

Is the inconsistency a problem?  I generally write device tree handling code to 
just respect whatever #*-fields says and don't consider that part of the 
specification of the binding.  I don't mind changing the example to have 
#size-fields and #address-fields to be 2, but since it's not wrong I also don't 
see any reason to change it.  We do have 32-bit devices with PLICs, and while 
they're not Linux-capable devices we're trying to adopt the Linux device tree 
bindings through the rest of the RISC-V software ecosystem as they tend to be 
pretty well thought out.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Atish Patra]

On 8/7/18 7:17 PM, Palmer Dabbelt wrote:
> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
>> On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>>>
>>> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>>>> From: Palmer Dabbelt <palmer@dabbelt.com>
>>>>
>>>> This patch adds documentation for the platform-level interrupt
>>>> controller (PLIC) found in all RISC-V systems.  This interrupt
>>>> controller routes interrupts from all the devices in the system to each
>>>> hart-local interrupt controller.
>>>>
>>>> Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
>>>> want to change how we're specifying holes in the hart list.
>>>>
>>>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>>>> [hch: various fixes and updates]
>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>> ---
>>>>    .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>>>>    1 file changed, 57 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>>>> new file mode 100644
>>>> index 000000000000..c756cd208a93
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>>>> @@ -0,0 +1,57 @@
>>>> +SiFive Platform-Level Interrupt Controller (PLIC)
>>>> +-------------------------------------------------
>>>> +
>>>> +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
>>>> +(PLIC) high-level specification in the RISC-V Privileged Architecture
>>>> +specification.  The PLIC connects all external interrupts in the system to all
>>>> +hart contexts in the system, via the external interrupt source in each hart.
>>>> +
>>>> +A hart context is a privilege mode in a hardware execution thread.  For example,
>>>> +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
>>>> +privilege modes per hart; machine mode and supervisor mode.
>>>> +
>>>> +Each interrupt can be enabled on per-context basis. Any context can claim
>>>> +a pending enabled interrupt and then release it once it has been handled.
>>>> +
>>>> +Each interrupt has a configurable priority. Higher priority interrupts are
>>>> +serviced first. Each context can specify a priority threshold. Interrupts
>>>> +with priority below this threshold will not cause the PLIC to raise its
>>>> +interrupt line leading to the context.
>>>> +
>>>> +While the PLIC supports both edge-triggered and level-triggered interrupts,
>>>> +interrupt handlers are oblivious to this distinction and therefore it is not
>>>> +specified in the PLIC device-tree binding.
>>>> +
>>>> +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
>>>> +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
>>>> +specific memory layout, which is documented in chapter 8 of the SiFive U5
>>>> +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
>>>> +
>>>> +Required properties:
>>>> +- compatible : "sifive,plic0"
> 
> I think there was a thread bouncing around somewhere where decided to pick the
> official name of the compatible string to be "sifive,plic-1.0".  The idea here
> is that the PLIC is compatible across all of SiFive's current implementations,
> but there's some limitations in the memory map that will probably cause us to
> spin a version 2 at some point so we want major version number.  The minor
> number is just in case we find some errata in the PLIC.
> 
>>>> +- #address-cells : should be <0>
>>>> +- #interrupt-cells : should be <1>
>>>> +- interrupt-controller : Identifies the node as an interrupt controller
>>>> +- reg : Should contain 1 register range (address and length)
>>>
>>> The one in the real device tree has two entries.
>>> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>>>
>>> Is it intentional or just incorrect entry left over from earlier days?
>>
>>>> +             reg = <0xc000000 0x4000000>;
>>
>> Looks to me like one has #size-cells and #address-cells set to 2 and
>> the example is using 1.
> 
> Yes.  For some background on how this works: we have a hardware generator that
> has a tree-of-busses abstraction, and each device is attached to some point on
> that tree.  When a device gets attached to the bus, we also generate a device
> tree entry.  For whatever system I generated the example PLIC device tree entry
> from, it must have been attached to a bus with addresses of 32 bits or less,
> which resulted in #address-cells and #size-cells being 1.
> 

Thanks Palmer for the detailed explanation.

> Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is
> probably not what I generated as an example -- the fu540-c000 is a complicated
> configuration, when I mess around with the hardware I tend to use simple ones
> as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging
> off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as
> the machine has 8GiB of memory and the PLIC is on a bus that's closer to the
> core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.
> 
> Is the inconsistency a problem?  I generally write device tree handling code to
> just respect whatever #*-fields says and don't consider that part of the
> specification of the binding.  I don't mind changing the example to have
> #size-fields and #address-fields to be 2, but since it's not wrong I also don't
> see any reason to change it.  We do have 32-bit devices with PLICs, and while
> they're not Linux-capable devices we're trying to adopt the Linux device tree
> bindings through the rest of the RISC-V software ecosystem as they tend to be
> pretty well thought out.
> 

Sounds good to me. IMHO, the inconsistencies and its reasoning are well 
documented which is good enough for now.

Regards,
Atish

Re: [PATCH 09/11] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

On 8/6/18 1:34 PM, Palmer Dabbelt wrote:
> On Fri, 03 Aug 2018 05:33:57 PDT (-0700), Christoph Hellwig wrote:
>> On Thu, Aug 02, 2018 at 03:19:49PM -0700, Atish Patra wrote:
>>> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>>>> From: Palmer Dabbelt <palmer@sifive.com>
>>>>
>>>> Follow the updated DT specs and read the timebase-frequency from the
>>>> CPU 0 node.
>>>>
>>>
>>> However, the DT in the HighFive Unleashed has the entry at the wrong place.
>>>
>>> Even the example in github also at wrong place.
>>> https://github.com/riscv/riscv-device-tree-doc/pull/8/commits/2461d481329c55005fcbe684f0d6bdb3b7f0a432
>>>
>>> DT should be consistent between Documentation and the one in the hardware.
>>> I can fix them in bbl & submit a bbl patch. But I am not sure if that's an
>>> acceptable way to do it.
>>
>> I'll need to have comments from Palmer and/or someone else at SiFive
>> here.  Personally I really don't care where we document the timebase,
>> as this patch supports both locations anywhere.  For now I'll just update
>> the commit log to state that more explicitly.
> 
> You're welcome to submit a BBL patch to make this all match, but from my
> understanding of the device tree spec putting timebase-frequency in either
> place should be legal so it's not a critical fix.  That said, it's better to
> have them match than not match.
> 

ok. I will add it my TODO list as a low priority task. Following DT 
entries can be fixed for now.

1. timebase-frequency
2. next-level-cache

Regards,
Atish

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Rob Herring]

On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
> >>
> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
> >> >
> >> > This patch adds documentation for the platform-level interrupt
> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
> >> > controller routes interrupts from all the devices in the system to each
> >> > hart-local interrupt controller.
> >> >
> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> >> > want to change how we're specifying holes in the hart list.
> >> >
> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> > [hch: various fixes and updates]
> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> > ---
> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
> >> >   1 file changed, 57 insertions(+)
> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> > new file mode 100644
> >> > index 000000000000..c756cd208a93
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> > @@ -0,0 +1,57 @@
> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
> >> > +-------------------------------------------------
> >> > +
> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
> >> > +specification.  The PLIC connects all external interrupts in the system to all
> >> > +hart contexts in the system, via the external interrupt source in each hart.
> >> > +
> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> >> > +privilege modes per hart; machine mode and supervisor mode.
> >> > +
> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
> >> > +a pending enabled interrupt and then release it once it has been handled.
> >> > +
> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
> >> > +serviced first. Each context can specify a priority threshold. Interrupts
> >> > +with priority below this threshold will not cause the PLIC to raise its
> >> > +interrupt line leading to the context.
> >> > +
> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
> >> > +specified in the PLIC device-tree binding.
> >> > +
> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> >> > +
> >> > +Required properties:
> >> > +- compatible : "sifive,plic0"
>
> I think there was a thread bouncing around somewhere where decided to pick the
> official name of the compatible string to be "sifive,plic-1.0".  The idea here
> is that the PLIC is compatible across all of SiFive's current implementations,
> but there's some limitations in the memory map that will probably cause us to
> spin a version 2 at some point so we want major version number.  The minor
> number is just in case we find some errata in the PLIC.

Is 1.0 an actual version number corresponding to an exact, revision
controlled version of the IP or just something you made up? Looks like
the latter to me and I'm not a fan of s/w folks making up version
numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
unless you have good reason to deviate (IP for FPGAs where version
numbers are exposed to customers is one example).

And defining a version 2 when you find a quirk doesn't work. You've
already shipped the DT. You need to be able to fix issues with just an
OS update. This is why you are supposed to define a compatible string
for each and every SoC (and use a fallback when they are "the
same"TM).

> >> > +- #address-cells : should be <0>
> >> > +- #interrupt-cells : should be <1>
> >> > +- interrupt-controller : Identifies the node as an interrupt controller
> >> > +- reg : Should contain 1 register range (address and length)
> >>
> >> The one in the real device tree has two entries.
> >> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
> >>
> >> Is it intentional or just incorrect entry left over from earlier days?
> >
> >> > +             reg = <0xc000000 0x4000000>;
> >
> > Looks to me like one has #size-cells and #address-cells set to 2 and
> > the example is using 1.
>
> Yes.  For some background on how this works: we have a hardware generator that
> has a tree-of-busses abstraction, and each device is attached to some point on
> that tree.  When a device gets attached to the bus, we also generate a device
> tree entry.  For whatever system I generated the example PLIC device tree entry
> from, it must have been attached to a bus with addresses of 32 bits or less,
> which resulted in #address-cells and #size-cells being 1.
>
> Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is
> probably not what I generated as an example -- the fu540-c000 is a complicated
> configuration, when I mess around with the hardware I tend to use simple ones
> as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging
> off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as
> the machine has 8GiB of memory and the PLIC is on a bus that's closer to the
> core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.
>
> Is the inconsistency a problem?  I generally write device tree handling code to
> just respect whatever #*-fields says and don't consider that part of the
> specification of the binding.  I don't mind changing the example to have
> #size-fields and #address-fields to be 2, but since it's not wrong I also don't
> see any reason to change it.  We do have 32-bit devices with PLICs, and while
> they're not Linux-capable devices we're trying to adopt the Linux device tree
> bindings through the rest of the RISC-V software ecosystem as they tend to be
> pretty well thought out.

The example is just an example and dts files can use either. For dts
files though, you should use the smallest size necessary and utilize
'ranges'. Some folks seem to think a 64-bit chip needs 64-bit address
and size everywhere. That's true at the top level typically, but
individual buses often don't span more than 4GB of address space. But
all that's out of scope of the example.

There are no "Linux device tree bindings". There are DT bindings that
happen to be hosting within the Linux tree for convenience.

Rob

Re: [PATCH 02/11] dt-bindings: Add an enable method to RISC-V

[Rob Herring]

On Thu, Aug 2, 2018 at 5:50 AM Christoph Hellwig <hch@lst.de> wrote:
>
> From: Palmer Dabbelt <palmer@sifive.com>
>
> RISC-V doesn't currently specify a mechanism for enabling or disabling
> CPUs.  Instead, we assume that all CPUs are enabled on boot, and if
> someone wants to save power we instead put a CPU to sleep via a WFI
> loop.  Future systems may have an explicit mechanism for putting a CPU
> to sleep, so we're standardizing the device tree entry for when that
> happens.
>
> We're not defining a spin-table based interface to the firmware, as the
> plan is to handle this entirely within the kernel instead.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/devicetree/bindings/riscv/cpus.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.txt b/Documentation/devicetree/bindings/riscv/cpus.txt
> index b0b038d6c406..6aa9cd075a5b 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.txt
> +++ b/Documentation/devicetree/bindings/riscv/cpus.txt
> @@ -82,6 +82,15 @@ described below.
>                  Value type: <string>
>                  Definition: Contains the RISC-V ISA string of this hart.  These
>                              ISA strings are defined by the RISC-V ISA manual.
> +        - cpu-enable-method:

Something wrong with "enable-method" as defined in the DT spec[1]?

> +                Usage: optional
> +                Value type: <stringlist>
> +                Definition: When absent, default is either "always-disabled"
> +                            "always-enabled", depending on the current state
> +                            of the CPU.
> +                            Must be one of:
> +                                * "always-disabled": This CPU cannot be enabled.
> +                                * "always-enabled": This CPU cannot be disabled.

To follow the spec, 'enable-method' should simply not be present in
the always-enabled case. I think the always disabled case should be
handled with:

status = "disabled";
enable-method = "none";

With "none" needing to be added to the spec.

[1] https://github.com/devicetree-org/devicetree-specification/blob/master/source/devicenodes.rst#general-properties-of-cpuscpu-nodes

Re: [PATCH 01/11] dt-bindings: Correct RISC-V's timebase-frequency

[Rob Herring]

On Thu, Aug 2, 2018 at 5:50 AM Christoph Hellwig <hch@lst.de> wrote:
>
> From: Palmer Dabbelt <palmer@sifive.com>
>
> Someone must have read the device tree specification incorrectly,
> because we were putting timebase-frequency in the wrong place.  This
> corrects the issue, moving it from
>
> / {
>         cpus {
>                 timebase-frequency = X;
>         }
> }
>
> to
>
> / {
>         cpus {
>                 cpu@0 {
>                         timebase-frequency = X;
>                 }
>         }
> }
>
> This is great, because the timer's frequency should really be a per-cpu
> quantity on RISC-V systems since there's a timer per CPU.  This should
> lead to some cleanups in our timer driver.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/devicetree/bindings/riscv/cpus.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

On Wed, Aug 08, 2018 at 08:16:14AM -0600, Rob Herring wrote:
> Is 1.0 an actual version number corresponding to an exact, revision
> controlled version of the IP or just something you made up? Looks like
> the latter to me and I'm not a fan of s/w folks making up version
> numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
> unless you have good reason to deviate (IP for FPGAs where version
> numbers are exposed to customers is one example).
> 
> And defining a version 2 when you find a quirk doesn't work. You've
> already shipped the DT. You need to be able to fix issues with just an
> OS update. This is why you are supposed to define a compatible string
> for each and every SoC (and use a fallback when they are "the
> same"TM).

Can you point to some existing examples of the multiple offered
compatible strings and what is actually matched for something that
largely hasn't changed?

For example the documentation for the arm GICv3 binding seems to just
match for arm,gic-v3.  On the other hand the GIC driver seems to match
for a lot of different strings.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Marc Zyngier]

On 08/08/18 16:09, Christoph Hellwig wrote:
> On Wed, Aug 08, 2018 at 08:16:14AM -0600, Rob Herring wrote:
>> Is 1.0 an actual version number corresponding to an exact, revision
>> controlled version of the IP or just something you made up? Looks like
>> the latter to me and I'm not a fan of s/w folks making up version
>> numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
>> unless you have good reason to deviate (IP for FPGAs where version
>> numbers are exposed to customers is one example).
>>
>> And defining a version 2 when you find a quirk doesn't work. You've
>> already shipped the DT. You need to be able to fix issues with just an
>> OS update. This is why you are supposed to define a compatible string
>> for each and every SoC (and use a fallback when they are "the
>> same"TM).
> 
> Can you point to some existing examples of the multiple offered
> compatible strings and what is actually matched for something that
> largely hasn't changed?
> 
> For example the documentation for the arm GICv3 binding seems to just
> match for arm,gic-v3.  On the other hand the GIC driver seems to match
> for a lot of different strings.

The original GIC driver deals with 2.5 revisions of the architecture
(yes, there was something pre-GICv1...), and implementers have been
creative to the extreme. Still, we could have done without most of these
compat strings. Hindsight and all that jazz.

GICv3 is a much more controlled architecture, and although people have
come up with a number of turds masquerading as implementations, it has
never been bad enough to mandate a different set of compat strings.
Also, you cannot describe that kind of stuff in ACPI, and we need to
support both, so we've come up with different ways of handling this.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

On Wed, Aug 08, 2018 at 05:47:40PM +0100, Marc Zyngier wrote:
> The original GIC driver deals with 2.5 revisions of the architecture
> (yes, there was something pre-GICv1...), and implementers have been
> creative to the extreme. Still, we could have done without most of these
> compat strings. Hindsight and all that jazz.
> 
> GICv3 is a much more controlled architecture, and although people have
> come up with a number of turds masquerading as implementations, it has
> never been bad enough to mandate a different set of compat strings.
> Also, you cannot describe that kind of stuff in ACPI, and we need to
> support both, so we've come up with different ways of handling this.

So the claim from SiFive is that all their current plic blocks are
the same.  Based on that I'd be really tempted to just match for
sifive,plic (or sifive,plic1 as suggested by them), but also require
each device to actually provide a board specific compatible string,
just in case that something goes wrong.  Which I suspect is what you
are doing with GICv3, right?

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Palmer Dabbelt]

On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
> On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
>> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>> >>
>> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
>> >> >
>> >> > This patch adds documentation for the platform-level interrupt
>> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
>> >> > controller routes interrupts from all the devices in the system to each
>> >> > hart-local interrupt controller.
>> >> >
>> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
>> >> > want to change how we're specifying holes in the hart list.
>> >> >
>> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> >> > [hch: various fixes and updates]
>> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> >> > ---
>> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>> >> >   1 file changed, 57 insertions(+)
>> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> > new file mode 100644
>> >> > index 000000000000..c756cd208a93
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> > @@ -0,0 +1,57 @@
>> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
>> >> > +-------------------------------------------------
>> >> > +
>> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
>> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
>> >> > +specification.  The PLIC connects all external interrupts in the system to all
>> >> > +hart contexts in the system, via the external interrupt source in each hart.
>> >> > +
>> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
>> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
>> >> > +privilege modes per hart; machine mode and supervisor mode.
>> >> > +
>> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
>> >> > +a pending enabled interrupt and then release it once it has been handled.
>> >> > +
>> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
>> >> > +serviced first. Each context can specify a priority threshold. Interrupts
>> >> > +with priority below this threshold will not cause the PLIC to raise its
>> >> > +interrupt line leading to the context.
>> >> > +
>> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
>> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
>> >> > +specified in the PLIC device-tree binding.
>> >> > +
>> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
>> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
>> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
>> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
>> >> > +
>> >> > +Required properties:
>> >> > +- compatible : "sifive,plic0"
>>
>> I think there was a thread bouncing around somewhere where decided to pick the
>> official name of the compatible string to be "sifive,plic-1.0".  The idea here
>> is that the PLIC is compatible across all of SiFive's current implementations,
>> but there's some limitations in the memory map that will probably cause us to
>> spin a version 2 at some point so we want major version number.  The minor
>> number is just in case we find some errata in the PLIC.
>
> Is 1.0 an actual version number corresponding to an exact, revision
> controlled version of the IP or just something you made up? Looks like
> the latter to me and I'm not a fan of s/w folks making up version
> numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
> unless you have good reason to deviate (IP for FPGAs where version
> numbers are exposed to customers is one example).

The hardware versioning scheme calls it "riscv,plic0", which is what we were 
originally using.  This PLIC isn't actually defined as a RISC-V spec, so we 
wanted to change it to a "sifive,*" name instead.  Since we were going to 
change the compat string anyway, I thought we'd just introduce a minor number 
to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.

I don't have a concrete idea of when the minor number would be used in the 
PLIC, but we do have a UART and I'd like to make a minor revision of that.  
This might be too much detail, but essentially the UART consists of two parts: 
a byte-wide FIFO that runs at the processor's clock, and then a bit-wide shift 
register that ruts at the UART clock.  The shift register is driven by a simple 
digital clock divider off the FIFO's clock, which means that whenever you 
change the FIFO's clock speed (for power management or whatever) you also need 
to change the clock divider to keep the UART's baud rate constant.

As a result, if you change the clock while the UART is in the middle of 
transmitting a byte then you get corruption.  There's a signal that says "the 
UART TX queue is empty" that can be read from software, but that signal points 
to the TX FIFO and doesn't account for the additional time to stream out the 
contents of the shift register.  There are configurations of the baud rate, bus 
latency, and clock speeds such that the "TX FIFO empty" poll can make it back 
to the core and the core's write to the clock controller can materialize at 
whatever magic makes the clocks change before the UART has serialized out every 
bit in the shift register, which manifests as corruption.

With the current UART hardware, which has a tentative compat string of 
"sifive,uart0", we need to determine that the shift registers has drained in an 
open-loop manner (ie, just wait for a bit).  This is ugly.  I'd like to spin a 
minor version of the UART that just has an extra control bit made available to 
software that tracks when the shift register drains, but since the drivers for 
the old version would be compatible it seems like calling that "sifive,uart1" 
is too big of a version jump.  Of course the Linux OF infrastructure assigns no 
semantic meaning to compat strings so it doesn't matter here, but we use device 
trees all over the place 

Thus, I thought that if we were going to change the naming scheme that we might 
as well go put in a minor version number just to be safe.  Sorry if that's a 
bit too much info... :)

Of course, this is all open source RTL so you can just see what's going on 
here in the PLIC

    https://github.com/freechipsproject/rocket-chip/blob/384096a6a73f0e94d6f7bd4bc9cc422e0a213e88/src/main/scala/devices/tilelink/Plic.scala#L69

and in the UART

    https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L91

We'll change the name generated by the hardware to match whatever's decided on 
here, so there won't be a rift between the hardware and the software.

This actually brings up an issue with the standard naming scheme, which in this 
cause would be something like "sifive,u540-c000-plic": the RTL is open source, 
so anyone can go build a chip with it at any time.  I've been operating under 
the impression that it will be impossible to maintain a central database of all 
implementations, so the fallback name will be the defacto name that becomes the 
interface.

I'd be happy to put a "sifive,u540-c000-plic" in there (I'd called in 
"sifive,u540-c000,plic" before, but that's just me not knowing the standard 
naming scheme) just to make sure we have a specific name.  We at SiFive can 
make sure we provide sane unique names for all our implementations, but I think 
we also really need to hammer out what the general compat string is because I 
don't think we can rely one everyone to do that.

There are devices that are very specific to a chip, and there we will have 
chip-specific names for them -- for example the PRCI (power, reset, clock and 
interrupt) block is different for pretty much every chip, so it'll be called 
something like "sifive,u540-c000-prci" (or maybe "sifive,aloe-prci", depending 
on whether we want to use engineering code names or marketing names in the 
bindings, which is a debate for later).  I'm less worried about these, though, 
as people building chips must pay a lot of attention to things like clock 
muxing so they're unlikely to accidentally end up with one floating around.

> And defining a version 2 when you find a quirk doesn't work. You've
> already shipped the DT. You need to be able to fix issues with just an
> OS update. This is why you are supposed to define a compatible string
> for each and every SoC (and use a fallback when they are "the
> same"TM).

Yes, of course -- we actually put the DTB in ROM on the chips, so there's 
really no option to change it aside from hacking up something nasty in the 
bootloader.  We're doing this for the u540-c000 because we haven't standardized 
the bindings.  There aren't that many u540-c000s in the wild so I'm OK 
shouldering the burden of making everyone upload their bootloaders with 
whatever workarounds are necessary to boot an upstream kernel.

I view that as just my punishment for not having properly discussed the 
bindings before shipping hardware (and out of tree kernels), but that won't be 
viable in the future.  I view this as just like any other ABI stabilization, 
where it's not officially stable until we're upstream.  That's why I'd like to 
get a good scheme for naming these in the future, as once this is in I'm 
stuck with any mistakes forever.

>> >> > +- #address-cells : should be <0>
>> >> > +- #interrupt-cells : should be <1>
>> >> > +- interrupt-controller : Identifies the node as an interrupt controller
>> >> > +- reg : Should contain 1 register range (address and length)
>> >>
>> >> The one in the real device tree has two entries.
>> >> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>> >>
>> >> Is it intentional or just incorrect entry left over from earlier days?
>> >
>> >> > +             reg = <0xc000000 0x4000000>;
>> >
>> > Looks to me like one has #size-cells and #address-cells set to 2 and
>> > the example is using 1.
>>
>> Yes.  For some background on how this works: we have a hardware generator that
>> has a tree-of-busses abstraction, and each device is attached to some point on
>> that tree.  When a device gets attached to the bus, we also generate a device
>> tree entry.  For whatever system I generated the example PLIC device tree entry
>> from, it must have been attached to a bus with addresses of 32 bits or less,
>> which resulted in #address-cells and #size-cells being 1.
>>
>> Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is
>> probably not what I generated as an example -- the fu540-c000 is a complicated
>> configuration, when I mess around with the hardware I tend to use simple ones
>> as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging
>> off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as
>> the machine has 8GiB of memory and the PLIC is on a bus that's closer to the
>> core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.
>>
>> Is the inconsistency a problem?  I generally write device tree handling code to
>> just respect whatever #*-fields says and don't consider that part of the
>> specification of the binding.  I don't mind changing the example to have
>> #size-fields and #address-fields to be 2, but since it's not wrong I also don't
>> see any reason to change it.  We do have 32-bit devices with PLICs, and while
>> they're not Linux-capable devices we're trying to adopt the Linux device tree
>> bindings through the rest of the RISC-V software ecosystem as they tend to be
>> pretty well thought out.
>
> The example is just an example and dts files can use either. For dts
> files though, you should use the smallest size necessary and utilize
> 'ranges'. Some folks seem to think a 64-bit chip needs 64-bit address
> and size everywhere. That's true at the top level typically, but
> individual buses often don't span more than 4GB of address space. But
> all that's out of scope of the example.

OK, thanks.  I think we're doing this where it's possible -- we emit ranges for 
busses, and I know that at least some of them (ie, the low-speed peripheral bus 
that essentially always fits into a 32-bit physical address) ends up with 
#{address,size}-cells=1.

I'll try to keep this in mind as we start to submit more bindings.

> There are no "Linux device tree bindings". There are DT bindings that
> happen to be hosting within the Linux tree for convenience.

Ah, OK.  By "Linux device tree bindings" I meant the ones stored in 
Documentation, which is what we considered the authoritative source and were 
planning on adopting everywhere we use device tree (ie, fixing the rest of our 
code as a result of the discussions we have submitting the bindings).  I'd 
heard some people refer to these as Linux specific, but I'm glad they're not as 
it means we won't be pushing upstream on getting everyone to agree on one set 
of bindings.

Does this mean I can submit bindings for devices that don't have a Linux 
driver?  How about devices where it doesn't even really make sense to ever have 
a Linux driver?

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Rob Herring]

On Wed, Aug 8, 2018 at 1:38 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
> > On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>
> >> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
> >> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
> >> >>
> >> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> >> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> >
> >> >> > This patch adds documentation for the platform-level interrupt
> >> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
> >> >> > controller routes interrupts from all the devices in the system to each
> >> >> > hart-local interrupt controller.
> >> >> >
> >> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> >> >> > want to change how we're specifying holes in the hart list.
> >> >> >
> >> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> > [hch: various fixes and updates]
> >> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> >> > ---
> >> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
> >> >> >   1 file changed, 57 insertions(+)
> >> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >
> >> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> > new file mode 100644
> >> >> > index 000000000000..c756cd208a93
> >> >> > --- /dev/null
> >> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> > @@ -0,0 +1,57 @@
> >> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
> >> >> > +-------------------------------------------------
> >> >> > +
> >> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> >> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
> >> >> > +specification.  The PLIC connects all external interrupts in the system to all
> >> >> > +hart contexts in the system, via the external interrupt source in each hart.
> >> >> > +
> >> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
> >> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> >> >> > +privilege modes per hart; machine mode and supervisor mode.
> >> >> > +
> >> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
> >> >> > +a pending enabled interrupt and then release it once it has been handled.
> >> >> > +
> >> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
> >> >> > +serviced first. Each context can specify a priority threshold. Interrupts
> >> >> > +with priority below this threshold will not cause the PLIC to raise its
> >> >> > +interrupt line leading to the context.
> >> >> > +
> >> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
> >> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
> >> >> > +specified in the PLIC device-tree binding.
> >> >> > +
> >> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> >> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> >> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
> >> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> >> >> > +
> >> >> > +Required properties:
> >> >> > +- compatible : "sifive,plic0"
> >>
> >> I think there was a thread bouncing around somewhere where decided to pick the
> >> official name of the compatible string to be "sifive,plic-1.0".  The idea here
> >> is that the PLIC is compatible across all of SiFive's current implementations,
> >> but there's some limitations in the memory map that will probably cause us to
> >> spin a version 2 at some point so we want major version number.  The minor
> >> number is just in case we find some errata in the PLIC.
> >
> > Is 1.0 an actual version number corresponding to an exact, revision
> > controlled version of the IP or just something you made up? Looks like
> > the latter to me and I'm not a fan of s/w folks making up version
> > numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
> > unless you have good reason to deviate (IP for FPGAs where version
> > numbers are exposed to customers is one example).
>
> The hardware versioning scheme calls it "riscv,plic0", which is what we were
> originally using.  This PLIC isn't actually defined as a RISC-V spec, so we
> wanted to change it to a "sifive,*" name instead.  Since we were going to
> change the compat string anyway, I thought we'd just introduce a minor number
> to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.

So I guess to answer my question, you are just making up version
numbers. Unless you are doing the IP verilog too, don't do that.

If you want to use just 'sifive,plic' then I'm fine with that. I've
given you the potential problems with that and they will be your
problems to deal with. Maybe you'll get lucky. Plus it won't be a
problem for the 1st implementation.

> I don't have a concrete idea of when the minor number would be used in the
> PLIC, but we do have a UART and I'd like to make a minor revision of that.
> This might be too much detail, but essentially the UART consists of two parts:
> a byte-wide FIFO that runs at the processor's clock, and then a bit-wide shift
> register that ruts at the UART clock.  The shift register is driven by a simple
> digital clock divider off the FIFO's clock, which means that whenever you
> change the FIFO's clock speed (for power management or whatever) you also need
> to change the clock divider to keep the UART's baud rate constant.
>
> As a result, if you change the clock while the UART is in the middle of
> transmitting a byte then you get corruption.  There's a signal that says "the
> UART TX queue is empty" that can be read from software, but that signal points
> to the TX FIFO and doesn't account for the additional time to stream out the
> contents of the shift register.  There are configurations of the baud rate, bus
> latency, and clock speeds such that the "TX FIFO empty" poll can make it back
> to the core and the core's write to the clock controller can materialize at
> whatever magic makes the clocks change before the UART has serialized out every
> bit in the shift register, which manifests as corruption.

I've experienced some broken UART clocking like that in the past.

That's a good example of why you may need SoC specific (or integration
specific) compatible strings. A future design could clock the FIFO
with a different clock that is fixed freq. A driver would distinguish
this quirk with different compatible strings.

> With the current UART hardware, which has a tentative compat string of
> "sifive,uart0", we need to determine that the shift registers has drained in an
> open-loop manner (ie, just wait for a bit).  This is ugly.  I'd like to spin a
> minor version of the UART that just has an extra control bit made available to
> software that tracks when the shift register drains, but since the drivers for
> the old version would be compatible it seems like calling that "sifive,uart1"
> is too big of a version jump.  Of course the Linux OF infrastructure assigns no
> semantic meaning to compat strings so it doesn't matter here, but we use device
> trees all over the place
>
> Thus, I thought that if we were going to change the naming scheme that we might
> as well go put in a minor version number just to be safe.  Sorry if that's a
> bit too much info... :)
>
> Of course, this is all open source RTL so you can just see what's going on
> here in the PLIC
>
>     https://github.com/freechipsproject/rocket-chip/blob/384096a6a73f0e94d6f7bd4bc9cc422e0a213e88/src/main/scala/devices/tilelink/Plic.scala#L69
>
> and in the UART
>
>     https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L91
>
> We'll change the name generated by the hardware to match whatever's decided on
> here, so there won't be a rift between the hardware and the software.

What exactly gets generated by the hardware?

> This actually brings up an issue with the standard naming scheme, which in this
> cause would be something like "sifive,u540-c000-plic": the RTL is open source,
> so anyone can go build a chip with it at any time.  I've been operating under
> the impression that it will be impossible to maintain a central database of all
> implementations, so the fallback name will be the defacto name that becomes the
> interface.
>
> I'd be happy to put a "sifive,u540-c000-plic" in there (I'd called in
> "sifive,u540-c000,plic" before, but that's just me not knowing the standard
> naming scheme) just to make sure we have a specific name.  We at SiFive can
> make sure we provide sane unique names for all our implementations, but I think
> we also really need to hammer out what the general compat string is because I
> don't think we can rely one everyone to do that.

Propose something. If compatible strings can be traced back to exactly
what versions of IP are used and there's a well defined process for
setting version numbers, then I'm fine with using version numbers in
compatible strings. Having version registers are helpful in that
regard. And if there are configuration options in the IP, registers to
read those too.

Presumably folks (including SiFive?) fork and modify the opensource
IP? Along the same lines as no one ships mainline kernels. Not sure
how you deal with that if you only use versions.

> There are devices that are very specific to a chip, and there we will have
> chip-specific names for them -- for example the PRCI (power, reset, clock and
> interrupt) block is different for pretty much every chip, so it'll be called
> something like "sifive,u540-c000-prci" (or maybe "sifive,aloe-prci", depending
> on whether we want to use engineering code names or marketing names in the
> bindings, which is a debate for later).  I'm less worried about these, though,
> as people building chips must pay a lot of attention to things like clock
> muxing so they're unlikely to accidentally end up with one floating around.
>
> > And defining a version 2 when you find a quirk doesn't work. You've
> > already shipped the DT. You need to be able to fix issues with just an
> > OS update. This is why you are supposed to define a compatible string
> > for each and every SoC (and use a fallback when they are "the
> > same"TM).
>
> Yes, of course -- we actually put the DTB in ROM on the chips, so there's
> really no option to change it aside from hacking up something nasty in the
> bootloader.  We're doing this for the u540-c000 because we haven't standardized
> the bindings.  There aren't that many u540-c000s in the wild so I'm OK
> shouldering the burden of making everyone upload their bootloaders with
> whatever workarounds are necessary to boot an upstream kernel.

I've been there too.

> I view that as just my punishment for not having properly discussed the
> bindings before shipping hardware (and out of tree kernels), but that won't be
> viable in the future.  I view this as just like any other ABI stabilization,
> where it's not officially stable until we're upstream.  That's why I'd like to
> get a good scheme for naming these in the future, as once this is in I'm
> stuck with any mistakes forever.

That's certainly reasonable and necessary. I'm just trying to make
sure it's understood how compatible is supposed to work.

> >> >> > +- #address-cells : should be <0>
> >> >> > +- #interrupt-cells : should be <1>
> >> >> > +- interrupt-controller : Identifies the node as an interrupt controller
> >> >> > +- reg : Should contain 1 register range (address and length)
> >> >>
> >> >> The one in the real device tree has two entries.
> >> >> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
> >> >>
> >> >> Is it intentional or just incorrect entry left over from earlier days?
> >> >
> >> >> > +             reg = <0xc000000 0x4000000>;
> >> >
> >> > Looks to me like one has #size-cells and #address-cells set to 2 and
> >> > the example is using 1.
> >>
> >> Yes.  For some background on how this works: we have a hardware generator that
> >> has a tree-of-busses abstraction, and each device is attached to some point on
> >> that tree.  When a device gets attached to the bus, we also generate a device
> >> tree entry.  For whatever system I generated the example PLIC device tree entry
> >> from, it must have been attached to a bus with addresses of 32 bits or less,
> >> which resulted in #address-cells and #size-cells being 1.
> >>
> >> Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is
> >> probably not what I generated as an example -- the fu540-c000 is a complicated
> >> configuration, when I mess around with the hardware I tend to use simple ones
> >> as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging
> >> off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as
> >> the machine has 8GiB of memory and the PLIC is on a bus that's closer to the
> >> core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.
> >>
> >> Is the inconsistency a problem?  I generally write device tree handling code to
> >> just respect whatever #*-fields says and don't consider that part of the
> >> specification of the binding.  I don't mind changing the example to have
> >> #size-fields and #address-fields to be 2, but since it's not wrong I also don't
> >> see any reason to change it.  We do have 32-bit devices with PLICs, and while
> >> they're not Linux-capable devices we're trying to adopt the Linux device tree
> >> bindings through the rest of the RISC-V software ecosystem as they tend to be
> >> pretty well thought out.
> >
> > The example is just an example and dts files can use either. For dts
> > files though, you should use the smallest size necessary and utilize
> > 'ranges'. Some folks seem to think a 64-bit chip needs 64-bit address
> > and size everywhere. That's true at the top level typically, but
> > individual buses often don't span more than 4GB of address space. But
> > all that's out of scope of the example.
>
> OK, thanks.  I think we're doing this where it's possible -- we emit ranges for
> busses, and I know that at least some of them (ie, the low-speed peripheral bus
> that essentially always fits into a 32-bit physical address) ends up with
> #{address,size}-cells=1.
>
> I'll try to keep this in mind as we start to submit more bindings.
>
> > There are no "Linux device tree bindings". There are DT bindings that
> > happen to be hosting within the Linux tree for convenience.
>
> Ah, OK.  By "Linux device tree bindings" I meant the ones stored in
> Documentation, which is what we considered the authoritative source and were
> planning on adopting everywhere we use device tree (ie, fixing the rest of our
> code as a result of the discussions we have submitting the bindings).  I'd
> heard some people refer to these as Linux specific, but I'm glad they're not as
> it means we won't be pushing upstream on getting everyone to agree on one set
> of bindings.
>
> Does this mean I can submit bindings for devices that don't have a Linux
> driver?

Certainly. ARM Mali GPU bindings don't have an (upstream) driver.

My main concern is if there is neither a driver nor a dts using a
binding, how do we know if it is used or not at some time in the
future.

> How about devices where it doesn't even really make sense to ever have
> a Linux driver?

Yes.

Rob

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Palmer Dabbelt]

On Wed, 08 Aug 2018 16:32:07 PDT (-0700), robh+dt@kernel.org wrote:
> On Wed, Aug 8, 2018 at 1:38 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
>> > On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >>
>> >> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
>> >> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>> >> >>
>> >> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>> >> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
>> >> >> >
>> >> >> > This patch adds documentation for the platform-level interrupt
>> >> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
>> >> >> > controller routes interrupts from all the devices in the system to each
>> >> >> > hart-local interrupt controller.
>> >> >> >
>> >> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
>> >> >> > want to change how we're specifying holes in the hart list.
>> >> >> >
>> >> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> >> >> > [hch: various fixes and updates]
>> >> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> >> >> > ---
>> >> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>> >> >> >   1 file changed, 57 insertions(+)
>> >> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> >
>> >> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> > new file mode 100644
>> >> >> > index 000000000000..c756cd208a93
>> >> >> > --- /dev/null
>> >> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> > @@ -0,0 +1,57 @@
>> >> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
>> >> >> > +-------------------------------------------------
>> >> >> > +
>> >> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
>> >> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
>> >> >> > +specification.  The PLIC connects all external interrupts in the system to all
>> >> >> > +hart contexts in the system, via the external interrupt source in each hart.
>> >> >> > +
>> >> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
>> >> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
>> >> >> > +privilege modes per hart; machine mode and supervisor mode.
>> >> >> > +
>> >> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
>> >> >> > +a pending enabled interrupt and then release it once it has been handled.
>> >> >> > +
>> >> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
>> >> >> > +serviced first. Each context can specify a priority threshold. Interrupts
>> >> >> > +with priority below this threshold will not cause the PLIC to raise its
>> >> >> > +interrupt line leading to the context.
>> >> >> > +
>> >> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
>> >> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
>> >> >> > +specified in the PLIC device-tree binding.
>> >> >> > +
>> >> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
>> >> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
>> >> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
>> >> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
>> >> >> > +
>> >> >> > +Required properties:
>> >> >> > +- compatible : "sifive,plic0"
>> >>
>> >> I think there was a thread bouncing around somewhere where decided to pick the
>> >> official name of the compatible string to be "sifive,plic-1.0".  The idea here
>> >> is that the PLIC is compatible across all of SiFive's current implementations,
>> >> but there's some limitations in the memory map that will probably cause us to
>> >> spin a version 2 at some point so we want major version number.  The minor
>> >> number is just in case we find some errata in the PLIC.
>> >
>> > Is 1.0 an actual version number corresponding to an exact, revision
>> > controlled version of the IP or just something you made up? Looks like
>> > the latter to me and I'm not a fan of s/w folks making up version
>> > numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
>> > unless you have good reason to deviate (IP for FPGAs where version
>> > numbers are exposed to customers is one example).
>>
>> The hardware versioning scheme calls it "riscv,plic0", which is what we were
>> originally using.  This PLIC isn't actually defined as a RISC-V spec, so we
>> wanted to change it to a "sifive,*" name instead.  Since we were going to
>> change the compat string anyway, I thought we'd just introduce a minor number
>> to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.
>
> So I guess to answer my question, you are just making up version
> numbers. Unless you are doing the IP verilog too, don't do that.

Well, in this case my proposal would be that we change the hardware team's 
versioning scheme to match whatever we decide on the versioning scheme should 
be as a part of this discussion.  I proposed accepting whatever versioning 
scheme is decided upon hereto the hardware team before discussing changing the 
naming scheme and they agreed to do so.

So we're really in the drivers' seat here.

> If you want to use just 'sifive,plic' then I'm fine with that. I've
> given you the potential problems with that and they will be your
> problems to deal with. Maybe you'll get lucky. Plus it won't be a
> problem for the 1st implementation.

I'd prefer to have some versioning scheme, that's why I'm talking so much about 
this :).  I really just want to learn how to get the right one, as I'm quite 
new to all this and we'll have many of these.

>> I don't have a concrete idea of when the minor number would be used in the
>> PLIC, but we do have a UART and I'd like to make a minor revision of that.
>> This might be too much detail, but essentially the UART consists of two parts:
>> a byte-wide FIFO that runs at the processor's clock, and then a bit-wide shift
>> register that ruts at the UART clock.  The shift register is driven by a simple
>> digital clock divider off the FIFO's clock, which means that whenever you
>> change the FIFO's clock speed (for power management or whatever) you also need
>> to change the clock divider to keep the UART's baud rate constant.
>>
>> As a result, if you change the clock while the UART is in the middle of
>> transmitting a byte then you get corruption.  There's a signal that says "the
>> UART TX queue is empty" that can be read from software, but that signal points
>> to the TX FIFO and doesn't account for the additional time to stream out the
>> contents of the shift register.  There are configurations of the baud rate, bus
>> latency, and clock speeds such that the "TX FIFO empty" poll can make it back
>> to the core and the core's write to the clock controller can materialize at
>> whatever magic makes the clocks change before the UART has serialized out every
>> bit in the shift register, which manifests as corruption.
>
> I've experienced some broken UART clocking like that in the past.
>
> That's a good example of why you may need SoC specific (or integration
> specific) compatible strings. A future design could clock the FIFO
> with a different clock that is fixed freq. A driver would distinguish
> this quirk with different compatible strings.

Ah, OK -- this has me a bit worried that I really fundamentally understand 
what's going on here.  In my model, if you have a UART that's missing the 
"actually done" signal then it's the same UART regardless of whether it's 
driver by a fixed or variable clock.  It's up to Linux to determine that this 
configuration doesn't require draining queues on clock changes (kind of an 
awkward example, as fixed clocks can't change), which we allow it to do by 
associating a clock with each UART.  This allows the UART driver to see the rest 
of the system, but in a manner rooted at the UART device as that's the part 
most likely to be relevant to the UART driver.

If the model is meant to be that "UART attached to a fixed clock" is different 
then "UART attached to a variable clock", then we must tag each device with the 
chip's fill name -- that's the only way to uniquely identify how the UART 
behaves in the context of the entire system.

My one worry is that there will be a lot of these.  I'm not even privy to any 
details about what's going on in RISC-V hardware land, but I can count a 
hundred today and they can all use the same driver.  There's then the 
additional headache that we'll never be able to publicly disclose most of these 
designs, which leaves all of those having unspecified behavior.

>> With the current UART hardware, which has a tentative compat string of
>> "sifive,uart0", we need to determine that the shift registers has drained in an
>> open-loop manner (ie, just wait for a bit).  This is ugly.  I'd like to spin a
>> minor version of the UART that just has an extra control bit made available to
>> software that tracks when the shift register drains, but since the drivers for
>> the old version would be compatible it seems like calling that "sifive,uart1"
>> is too big of a version jump.  Of course the Linux OF infrastructure assigns no
>> semantic meaning to compat strings so it doesn't matter here, but we use device
>> trees all over the place
>>
>> Thus, I thought that if we were going to change the naming scheme that we might
>> as well go put in a minor version number just to be safe.  Sorry if that's a
>> bit too much info... :)
>>
>> Of course, this is all open source RTL so you can just see what's going on
>> here in the PLIC
>>
>>     https://github.com/freechipsproject/rocket-chip/blob/384096a6a73f0e94d6f7bd4bc9cc422e0a213e88/src/main/scala/devices/tilelink/Plic.scala#L69
>>
>> and in the UART
>>
>>     https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L91
>>
>> We'll change the name generated by the hardware to match whatever's decided on
>> here, so there won't be a rift between the hardware and the software.
>
> What exactly gets generated by the hardware?

Sorry, by "generated by the hardware" I really meant "generated along with the 
hardware".  Essentially what happens is that whatever system collects the RTL 
blocks to be emitted also collects the device tree nodes to be emitted, thus 
ensuring they always match.  This is the ground truth for our device trees, and 
it's how we ensure the device tree always matches the RTL.  Whatever compatible 
string we agree on for the bindings will be reflected in this system, so the 
ground truth always matches the spec.

There's nothing we can do about what's already been shipped (ie, pre-spec 
hardware), and ensuring that RTL modifications that change software 
compatibility cause a change to the device tree node's compat string is 
something we at SiFive enforce by code review.

>> This actually brings up an issue with the standard naming scheme, which in this
>> cause would be something like "sifive,u540-c000-plic": the RTL is open source,
>> so anyone can go build a chip with it at any time.  I've been operating under
>> the impression that it will be impossible to maintain a central database of all
>> implementations, so the fallback name will be the defacto name that becomes the
>> interface.
>>
>> I'd be happy to put a "sifive,u540-c000-plic" in there (I'd called in
>> "sifive,u540-c000,plic" before, but that's just me not knowing the standard
>> naming scheme) just to make sure we have a specific name.  We at SiFive can
>> make sure we provide sane unique names for all our implementations, but I think
>> we also really need to hammer out what the general compat string is because I
>> don't think we can rely one everyone to do that.
>
> Propose something. If compatible strings can be traced back to exactly
> what versions of IP are used and there's a well defined process for
> setting version numbers, then I'm fine with using version numbers in
> compatible strings. Having version registers are helpful in that
> regard. And if there are configuration options in the IP, registers to
> read those too.
>
> Presumably folks (including SiFive?) fork and modify the opensource
> IP? Along the same lines as no one ships mainline kernels. Not sure
> how you deal with that if you only use versions.

We (and as far as I know, other vendors -- though I know little about anyone 
else' flow) ship RTL from the upstream blocks.  The blocks tend to just become 
stable enough that the only changes are bug fixes anyway, and as we approach 
tape out there are very few bugs.

The trend towards stability is particularly true of blocks like the PLIC that 
have been taped out multiple times, where pretty much all that changes is the 
interface to the generator.  There's a bunch of top-level stuff that's done out 
of tree, and we add our own special blocks in, but for the leaf blocks we 
generally match upstream closely.

Or at least that's what it looks like to me -- I don't do ECOs here, so maybe 
I'm just in the dark :)

>> There are devices that are very specific to a chip, and there we will have
>> chip-specific names for them -- for example the PRCI (power, reset, clock and
>> interrupt) block is different for pretty much every chip, so it'll be called
>> something like "sifive,u540-c000-prci" (or maybe "sifive,aloe-prci", depending
>> on whether we want to use engineering code names or marketing names in the
>> bindings, which is a debate for later).  I'm less worried about these, though,
>> as people building chips must pay a lot of attention to things like clock
>> muxing so they're unlikely to accidentally end up with one floating around.
>>
>> > And defining a version 2 when you find a quirk doesn't work. You've
>> > already shipped the DT. You need to be able to fix issues with just an
>> > OS update. This is why you are supposed to define a compatible string
>> > for each and every SoC (and use a fallback when they are "the
>> > same"TM).
>>
>> Yes, of course -- we actually put the DTB in ROM on the chips, so there's
>> really no option to change it aside from hacking up something nasty in the
>> bootloader.  We're doing this for the u540-c000 because we haven't standardized
>> the bindings.  There aren't that many u540-c000s in the wild so I'm OK
>> shouldering the burden of making everyone upload their bootloaders with
>> whatever workarounds are necessary to boot an upstream kernel.
>
> I've been there too.
>
>> I view that as just my punishment for not having properly discussed the
>> bindings before shipping hardware (and out of tree kernels), but that won't be
>> viable in the future.  I view this as just like any other ABI stabilization,
>> where it's not officially stable until we're upstream.  That's why I'd like to
>> get a good scheme for naming these in the future, as once this is in I'm
>> stuck with any mistakes forever.
>
> That's certainly reasonable and necessary. I'm just trying to make
> sure it's understood how compatible is supposed to work.

Yes, and thank you for spending so much time on this.  I really worry about 
getting these sort of interfaces well thought out, as I'm the one who is going 
to get burned by all the stupid mistakes I make :).

>> >> >> > +- #address-cells : should be <0>
>> >> >> > +- #interrupt-cells : should be <1>
>> >> >> > +- interrupt-controller : Identifies the node as an interrupt controller
>> >> >> > +- reg : Should contain 1 register range (address and length)
>> >> >>
>> >> >> The one in the real device tree has two entries.
>> >> >> reg = <0x00000000 0x0c000000 0x00000000 0x04000000>;
>> >> >>
>> >> >> Is it intentional or just incorrect entry left over from earlier days?
>> >> >
>> >> >> > +             reg = <0xc000000 0x4000000>;
>> >> >
>> >> > Looks to me like one has #size-cells and #address-cells set to 2 and
>> >> > the example is using 1.
>> >>
>> >> Yes.  For some background on how this works: we have a hardware generator that
>> >> has a tree-of-busses abstraction, and each device is attached to some point on
>> >> that tree.  When a device gets attached to the bus, we also generate a device
>> >> tree entry.  For whatever system I generated the example PLIC device tree entry
>> >> from, it must have been attached to a bus with addresses of 32 bits or less,
>> >> which resulted in #address-cells and #size-cells being 1.
>> >>
>> >> Christoph has a HiFive Unleashed, which has a fu540-c000 on it, which is
>> >> probably not what I generated as an example -- the fu540-c000 is a complicated
>> >> configuration, when I mess around with the hardware I tend to use simple ones
>> >> as I'm not really a hardware guy.  I suppose the bus that the PLIC is hanging
>> >> off on the fu540-c000 has addresses wider than 32 bits.  This makes sense, as
>> >> the machine has 8GiB of memory and the PLIC is on a bus that's closer to the
>> >> core than the DRAM is, so it'd need at least enough address bits to fit 8GiB.
>> >>
>> >> Is the inconsistency a problem?  I generally write device tree handling code to
>> >> just respect whatever #*-fields says and don't consider that part of the
>> >> specification of the binding.  I don't mind changing the example to have
>> >> #size-fields and #address-fields to be 2, but since it's not wrong I also don't
>> >> see any reason to change it.  We do have 32-bit devices with PLICs, and while
>> >> they're not Linux-capable devices we're trying to adopt the Linux device tree
>> >> bindings through the rest of the RISC-V software ecosystem as they tend to be
>> >> pretty well thought out.
>> >
>> > The example is just an example and dts files can use either. For dts
>> > files though, you should use the smallest size necessary and utilize
>> > 'ranges'. Some folks seem to think a 64-bit chip needs 64-bit address
>> > and size everywhere. That's true at the top level typically, but
>> > individual buses often don't span more than 4GB of address space. But
>> > all that's out of scope of the example.
>>
>> OK, thanks.  I think we're doing this where it's possible -- we emit ranges for
>> busses, and I know that at least some of them (ie, the low-speed peripheral bus
>> that essentially always fits into a 32-bit physical address) ends up with
>> #{address,size}-cells=1.
>>
>> I'll try to keep this in mind as we start to submit more bindings.
>>
>> > There are no "Linux device tree bindings". There are DT bindings that
>> > happen to be hosting within the Linux tree for convenience.
>>
>> Ah, OK.  By "Linux device tree bindings" I meant the ones stored in
>> Documentation, which is what we considered the authoritative source and were
>> planning on adopting everywhere we use device tree (ie, fixing the rest of our
>> code as a result of the discussions we have submitting the bindings).  I'd
>> heard some people refer to these as Linux specific, but I'm glad they're not as
>> it means we won't be pushing upstream on getting everyone to agree on one set
>> of bindings.
>>
>> Does this mean I can submit bindings for devices that don't have a Linux
>> driver?
>
> Certainly. ARM Mali GPU bindings don't have an (upstream) driver.
>
> My main concern is if there is neither a driver nor a dts using a
> binding, how do we know if it is used or not at some time in the
> future.

Well, I guess my argument would be: why does this matter?  If it's in the spec 
then it's in the spec, and we're not breaking anything that's compatible so we 
can't change it.

Though in practice I think any bindings that make their way into the spec will 
have a driver make it into at least some open source repository or be 
demonstrably in some physical object, as otherwise why would you bother going 
through the effort of adding it to the spec?  The best way to solve this seems 
to be at the review level: we just ensure that the bindings that are accepted 
into the standard are real enough that they're actually useful.

>> How about devices where it doesn't even really make sense to ever have
>> a Linux driver?
>
> Yes.
>
> Rob

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

On Wed, Aug 08, 2018 at 11:29:17PM -0700, Palmer Dabbelt wrote:
>> So I guess to answer my question, you are just making up version
>> numbers. Unless you are doing the IP verilog too, don't do that.
>
> Well, in this case my proposal would be that we change the hardware team's 
> versioning scheme to match whatever we decide on the versioning scheme 
> should be as a part of this discussion.  I proposed accepting whatever 
> versioning scheme is decided upon hereto the hardware team before 
> discussing changing the naming scheme and they agreed to do so.
>
> So we're really in the drivers' seat here.
>
>> If you want to use just 'sifive,plic' then I'm fine with that. I've
>> given you the potential problems with that and they will be your
>> problems to deal with. Maybe you'll get lucky. Plus it won't be a
>> problem for the 1st implementation.
>
> I'd prefer to have some versioning scheme, that's why I'm talking so much 
> about this :).  I really just want to learn how to get the right one, as 
> I'm quite new to all this and we'll have many of these.

Based on the discussion so far I think we should settle for sifive,plic +
an actual implementation string suggested by Palmer and Andrew.

This is what I have right now:

http://git.infradead.org/users/hch/riscv.git/commitdiff/1972707029f8f1216dbe14bd7791295e4b37f560

and which I'd like to send out before it is too late.

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Marc Zyngier]

On 08/08/18 17:57, Christoph Hellwig wrote:
> On Wed, Aug 08, 2018 at 05:47:40PM +0100, Marc Zyngier wrote:
>> The original GIC driver deals with 2.5 revisions of the architecture
>> (yes, there was something pre-GICv1...), and implementers have been
>> creative to the extreme. Still, we could have done without most of these
>> compat strings. Hindsight and all that jazz.
>>
>> GICv3 is a much more controlled architecture, and although people have
>> come up with a number of turds masquerading as implementations, it has
>> never been bad enough to mandate a different set of compat strings.
>> Also, you cannot describe that kind of stuff in ACPI, and we need to
>> support both, so we've come up with different ways of handling this.
> 
> So the claim from SiFive is that all their current plic blocks are
> the same.  Based on that I'd be really tempted to just match for
> sifive,plic (or sifive,plic1 as suggested by them), but also require
> each device to actually provide a board specific compatible string,
> just in case that something goes wrong.  Which I suspect is what you
> are doing with GICv3, right?

We do it slightly differently:

Each GICv3 implementation has a set of ID registers that uniquely
identifies the implementer and revision of the HW block. This allows us
to quirk individual implementations while only having a single matching
property, and be independent of a single firmware implementation (DT vs
ACPI).

We don't mandate anything at the board or even SoC level though (it
would quickly become unmanageable given the number of SOCs and devices).

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Rob Herring]

On Thu, Aug 9, 2018 at 12:29 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 08 Aug 2018 16:32:07 PDT (-0700), robh+dt@kernel.org wrote:
> > On Wed, Aug 8, 2018 at 1:38 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>
> >> On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
> >> > On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >>
> >> >> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
> >> >> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
> >> >> >>
> >> >> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> >> >> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> >> >
> >> >> >> > This patch adds documentation for the platform-level interrupt
> >> >> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
> >> >> >> > controller routes interrupts from all the devices in the system to each
> >> >> >> > hart-local interrupt controller.
> >> >> >> >
> >> >> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> >> >> >> > want to change how we're specifying holes in the hart list.
> >> >> >> >
> >> >> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> >> > [hch: various fixes and updates]
> >> >> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> >> >> > ---
> >> >> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
> >> >> >> >   1 file changed, 57 insertions(+)
> >> >> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> >
> >> >> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> > new file mode 100644
> >> >> >> > index 000000000000..c756cd208a93
> >> >> >> > --- /dev/null
> >> >> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> > @@ -0,0 +1,57 @@
> >> >> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
> >> >> >> > +-------------------------------------------------
> >> >> >> > +
> >> >> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> >> >> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
> >> >> >> > +specification.  The PLIC connects all external interrupts in the system to all
> >> >> >> > +hart contexts in the system, via the external interrupt source in each hart.
> >> >> >> > +
> >> >> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
> >> >> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> >> >> >> > +privilege modes per hart; machine mode and supervisor mode.
> >> >> >> > +
> >> >> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
> >> >> >> > +a pending enabled interrupt and then release it once it has been handled.
> >> >> >> > +
> >> >> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
> >> >> >> > +serviced first. Each context can specify a priority threshold. Interrupts
> >> >> >> > +with priority below this threshold will not cause the PLIC to raise its
> >> >> >> > +interrupt line leading to the context.
> >> >> >> > +
> >> >> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
> >> >> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
> >> >> >> > +specified in the PLIC device-tree binding.
> >> >> >> > +
> >> >> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> >> >> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> >> >> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
> >> >> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> >> >> >> > +
> >> >> >> > +Required properties:
> >> >> >> > +- compatible : "sifive,plic0"
> >> >>
> >> >> I think there was a thread bouncing around somewhere where decided to pick the
> >> >> official name of the compatible string to be "sifive,plic-1.0".  The idea here
> >> >> is that the PLIC is compatible across all of SiFive's current implementations,
> >> >> but there's some limitations in the memory map that will probably cause us to
> >> >> spin a version 2 at some point so we want major version number.  The minor
> >> >> number is just in case we find some errata in the PLIC.
> >> >
> >> > Is 1.0 an actual version number corresponding to an exact, revision
> >> > controlled version of the IP or just something you made up? Looks like
> >> > the latter to me and I'm not a fan of s/w folks making up version
> >> > numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
> >> > unless you have good reason to deviate (IP for FPGAs where version
> >> > numbers are exposed to customers is one example).
> >>
> >> The hardware versioning scheme calls it "riscv,plic0", which is what we were
> >> originally using.  This PLIC isn't actually defined as a RISC-V spec, so we
> >> wanted to change it to a "sifive,*" name instead.  Since we were going to
> >> change the compat string anyway, I thought we'd just introduce a minor number
> >> to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.
> >
> > So I guess to answer my question, you are just making up version
> > numbers. Unless you are doing the IP verilog too, don't do that.
>
> Well, in this case my proposal would be that we change the hardware team's
> versioning scheme to match whatever we decide on the versioning scheme should
> be as a part of this discussion.  I proposed accepting whatever versioning
> scheme is decided upon hereto the hardware team before discussing changing the
> naming scheme and they agreed to do so.
>
> So we're really in the drivers' seat here.

Okay. So you could have something like x.y.z versioning where a change
in X is change in register layout and/or major functional change, Y is
backwards compatible changes (i.e. superset of functionality), Z is
not s/w visible changes (though trusting that Z changes are really not
s/w visible makes me nervous). If you have a version register, then
the compatible string only needs to be specific enough to read that
register.

And if I can have anything I ask for, then any configuration option
needs to be s/w readable too. Things like FIFO sizes, number of
channels, etc. Sometimes these are DT properties, but generally the
preference is to make these implied by the compatible. There's no set
rule though. The problem with properties is they work well if you know
up front to have them, but often something is fixed and implied and
then becomes variable when a new version of the block is created.

Think of it this way, DT is a work-around for non-discoverable h/w.
The best solution is making the h/w more discoverable. If you look at
the ARM Primecell aka amba_bus drivers, we don't actually match on the
device compatible string (e.g. arm,pl011). Only "arm,primecell" is
used and then we match with standard ID registers from there. We only
need DT to discover where those blocks are, not what they are or what
version they are (and of course other things like clocks, regulators,
etc.).

So what your compatible string(s) need to look like depend on all the
above. It can be very generic if you can identify the block, version,
and capabilities in a standard way.

> > If you want to use just 'sifive,plic' then I'm fine with that. I've
> > given you the potential problems with that and they will be your
> > problems to deal with. Maybe you'll get lucky. Plus it won't be a
> > problem for the 1st implementation.
>
> I'd prefer to have some versioning scheme, that's why I'm talking so much about
> this :).  I really just want to learn how to get the right one, as I'm quite
> new to all this and we'll have many of these.
>
> >> I don't have a concrete idea of when the minor number would be used in the
> >> PLIC, but we do have a UART and I'd like to make a minor revision of that.
> >> This might be too much detail, but essentially the UART consists of two parts:
> >> a byte-wide FIFO that runs at the processor's clock, and then a bit-wide shift
> >> register that ruts at the UART clock.  The shift register is driven by a simple
> >> digital clock divider off the FIFO's clock, which means that whenever you
> >> change the FIFO's clock speed (for power management or whatever) you also need
> >> to change the clock divider to keep the UART's baud rate constant.
> >>
> >> As a result, if you change the clock while the UART is in the middle of
> >> transmitting a byte then you get corruption.  There's a signal that says "the
> >> UART TX queue is empty" that can be read from software, but that signal points
> >> to the TX FIFO and doesn't account for the additional time to stream out the
> >> contents of the shift register.  There are configurations of the baud rate, bus
> >> latency, and clock speeds such that the "TX FIFO empty" poll can make it back
> >> to the core and the core's write to the clock controller can materialize at
> >> whatever magic makes the clocks change before the UART has serialized out every
> >> bit in the shift register, which manifests as corruption.
> >
> > I've experienced some broken UART clocking like that in the past.
> >
> > That's a good example of why you may need SoC specific (or integration
> > specific) compatible strings. A future design could clock the FIFO
> > with a different clock that is fixed freq. A driver would distinguish
> > this quirk with different compatible strings.
>
> Ah, OK -- this has me a bit worried that I really fundamentally understand
> what's going on here.  In my model, if you have a UART that's missing the
> "actually done" signal then it's the same UART regardless of whether it's
> driver by a fixed or variable clock.  It's up to Linux to determine that this
> configuration doesn't require draining queues on clock changes (kind of an
> awkward example, as fixed clocks can't change), which we allow it to do by
> associating a clock with each UART.  This allows the UART driver to see the rest
> of the system, but in a manner rooted at the UART device as that's the part
> most likely to be relevant to the UART driver.
>
> If the model is meant to be that "UART attached to a fixed clock" is different
> then "UART attached to a variable clock", then we must tag each device with the
> chip's fill name -- that's the only way to uniquely identify how the UART
> behaves in the context of the entire system.

Certainly you can do it either way in this example. I think it is
simpler for the driver to just be told one way or the other rather
than have to figure out details of the clock tree.

> My one worry is that there will be a lot of these.  I'm not even privy to any
> details about what's going on in RISC-V hardware land, but I can count a
> hundred today and they can all use the same driver.  There's then the
> additional headache that we'll never be able to publicly disclose most of these
> designs, which leaves all of those having unspecified behavior.
>
> >> With the current UART hardware, which has a tentative compat string of
> >> "sifive,uart0", we need to determine that the shift registers has drained in an
> >> open-loop manner (ie, just wait for a bit).  This is ugly.  I'd like to spin a
> >> minor version of the UART that just has an extra control bit made available to
> >> software that tracks when the shift register drains, but since the drivers for
> >> the old version would be compatible it seems like calling that "sifive,uart1"
> >> is too big of a version jump.  Of course the Linux OF infrastructure assigns no
> >> semantic meaning to compat strings so it doesn't matter here, but we use device
> >> trees all over the place
> >>
> >> Thus, I thought that if we were going to change the naming scheme that we might
> >> as well go put in a minor version number just to be safe.  Sorry if that's a
> >> bit too much info... :)
> >>
> >> Of course, this is all open source RTL so you can just see what's going on
> >> here in the PLIC
> >>
> >>     https://github.com/freechipsproject/rocket-chip/blob/384096a6a73f0e94d6f7bd4bc9cc422e0a213e88/src/main/scala/devices/tilelink/Plic.scala#L69
> >>
> >> and in the UART
> >>
> >>     https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L91
> >>
> >> We'll change the name generated by the hardware to match whatever's decided on
> >> here, so there won't be a rift between the hardware and the software.
> >
> > What exactly gets generated by the hardware?
>
> Sorry, by "generated by the hardware" I really meant "generated along with the
> hardware".  Essentially what happens is that whatever system collects the RTL
> blocks to be emitted also collects the device tree nodes to be emitted, thus
> ensuring they always match.  This is the ground truth for our device trees, and
> it's how we ensure the device tree always matches the RTL.  Whatever compatible
> string we agree on for the bindings will be reflected in this system, so the
> ground truth always matches the spec.
>
> There's nothing we can do about what's already been shipped (ie, pre-spec
> hardware), and ensuring that RTL modifications that change software
> compatibility cause a change to the device tree node's compat string is
> something we at SiFive enforce by code review.
>
> >> This actually brings up an issue with the standard naming scheme, which in this
> >> cause would be something like "sifive,u540-c000-plic": the RTL is open source,
> >> so anyone can go build a chip with it at any time.  I've been operating under
> >> the impression that it will be impossible to maintain a central database of all
> >> implementations, so the fallback name will be the defacto name that becomes the
> >> interface.
> >>
> >> I'd be happy to put a "sifive,u540-c000-plic" in there (I'd called in
> >> "sifive,u540-c000,plic" before, but that's just me not knowing the standard
> >> naming scheme) just to make sure we have a specific name.  We at SiFive can
> >> make sure we provide sane unique names for all our implementations, but I think
> >> we also really need to hammer out what the general compat string is because I
> >> don't think we can rely one everyone to do that.
> >
> > Propose something. If compatible strings can be traced back to exactly
> > what versions of IP are used and there's a well defined process for
> > setting version numbers, then I'm fine with using version numbers in
> > compatible strings. Having version registers are helpful in that
> > regard. And if there are configuration options in the IP, registers to
> > read those too.
> >
> > Presumably folks (including SiFive?) fork and modify the opensource
> > IP? Along the same lines as no one ships mainline kernels. Not sure
> > how you deal with that if you only use versions.
>
> We (and as far as I know, other vendors -- though I know little about anyone
> else' flow) ship RTL from the upstream blocks.  The blocks tend to just become
> stable enough that the only changes are bug fixes anyway, and as we approach
> tape out there are very few bugs.
>
> The trend towards stability is particularly true of blocks like the PLIC that
> have been taped out multiple times, where pretty much all that changes is the
> interface to the generator.  There's a bunch of top-level stuff that's done out
> of tree, and we add our own special blocks in, but for the leaf blocks we
> generally match upstream closely.
>
> Or at least that's what it looks like to me -- I don't do ECOs here, so maybe
> I'm just in the dark :)

Okay, that's good to know. I guess there's at least some motivation to
not touch and break things you're going to set in stone (or Si
really).

[...]

> >> > There are no "Linux device tree bindings". There are DT bindings that
> >> > happen to be hosting within the Linux tree for convenience.
> >>
> >> Ah, OK.  By "Linux device tree bindings" I meant the ones stored in
> >> Documentation, which is what we considered the authoritative source and were
> >> planning on adopting everywhere we use device tree (ie, fixing the rest of our
> >> code as a result of the discussions we have submitting the bindings).  I'd
> >> heard some people refer to these as Linux specific, but I'm glad they're not as
> >> it means we won't be pushing upstream on getting everyone to agree on one set
> >> of bindings.
> >>
> >> Does this mean I can submit bindings for devices that don't have a Linux
> >> driver?
> >
> > Certainly. ARM Mali GPU bindings don't have an (upstream) driver.
> >
> > My main concern is if there is neither a driver nor a dts using a
> > binding, how do we know if it is used or not at some time in the
> > future.
>
> Well, I guess my argument would be: why does this matter?  If it's in the spec
> then it's in the spec, and we're not breaking anything that's compatible so we
> can't change it.

It is not always crystal clear what's valid or not in bindings and
having an actual dts or client implementation can help. Or there's
been times we've wanted to make incompatible changes (yes, it happens.
:)) and looking at dts files and drivers is an indication whether or
not we can get away with changes (it's only an incompatible change if
someone notices).

With anything, there's maintenance and refactorings that happen from
time to time. For example, I'm working on moving DT bindings to
json-schema[1]. It's easier if there aren't obsolete things we're
carrying.

Rob

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Palmer Dabbelt]

On Fri, 10 Aug 2018 09:57:03 PDT (-0700), robh+dt@kernel.org wrote:
> On Thu, Aug 9, 2018 at 12:29 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 08 Aug 2018 16:32:07 PDT (-0700), robh+dt@kernel.org wrote:
>> > On Wed, Aug 8, 2018 at 1:38 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >>
>> >> On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
>> >> > On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >> >>
>> >> >> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
>> >> >> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
>> >> >> >>
>> >> >> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
>> >> >> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
>> >> >> >> >
>> >> >> >> > This patch adds documentation for the platform-level interrupt
>> >> >> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
>> >> >> >> > controller routes interrupts from all the devices in the system to each
>> >> >> >> > hart-local interrupt controller.
>> >> >> >> >
>> >> >> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
>> >> >> >> > want to change how we're specifying holes in the hart list.
>> >> >> >> >
>> >> >> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> >> >> >> > [hch: various fixes and updates]
>> >> >> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> >> >> >> > ---
>> >> >> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
>> >> >> >> >   1 file changed, 57 insertions(+)
>> >> >> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> >> >
>> >> >> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> >> > new file mode 100644
>> >> >> >> > index 000000000000..c756cd208a93
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
>> >> >> >> > @@ -0,0 +1,57 @@
>> >> >> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
>> >> >> >> > +-------------------------------------------------
>> >> >> >> > +
>> >> >> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
>> >> >> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
>> >> >> >> > +specification.  The PLIC connects all external interrupts in the system to all
>> >> >> >> > +hart contexts in the system, via the external interrupt source in each hart.
>> >> >> >> > +
>> >> >> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
>> >> >> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
>> >> >> >> > +privilege modes per hart; machine mode and supervisor mode.
>> >> >> >> > +
>> >> >> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
>> >> >> >> > +a pending enabled interrupt and then release it once it has been handled.
>> >> >> >> > +
>> >> >> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
>> >> >> >> > +serviced first. Each context can specify a priority threshold. Interrupts
>> >> >> >> > +with priority below this threshold will not cause the PLIC to raise its
>> >> >> >> > +interrupt line leading to the context.
>> >> >> >> > +
>> >> >> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
>> >> >> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
>> >> >> >> > +specified in the PLIC device-tree binding.
>> >> >> >> > +
>> >> >> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
>> >> >> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
>> >> >> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
>> >> >> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
>> >> >> >> > +
>> >> >> >> > +Required properties:
>> >> >> >> > +- compatible : "sifive,plic0"
>> >> >>
>> >> >> I think there was a thread bouncing around somewhere where decided to pick the
>> >> >> official name of the compatible string to be "sifive,plic-1.0".  The idea here
>> >> >> is that the PLIC is compatible across all of SiFive's current implementations,
>> >> >> but there's some limitations in the memory map that will probably cause us to
>> >> >> spin a version 2 at some point so we want major version number.  The minor
>> >> >> number is just in case we find some errata in the PLIC.
>> >> >
>> >> > Is 1.0 an actual version number corresponding to an exact, revision
>> >> > controlled version of the IP or just something you made up? Looks like
>> >> > the latter to me and I'm not a fan of s/w folks making up version
>> >> > numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
>> >> > unless you have good reason to deviate (IP for FPGAs where version
>> >> > numbers are exposed to customers is one example).
>> >>
>> >> The hardware versioning scheme calls it "riscv,plic0", which is what we were
>> >> originally using.  This PLIC isn't actually defined as a RISC-V spec, so we
>> >> wanted to change it to a "sifive,*" name instead.  Since we were going to
>> >> change the compat string anyway, I thought we'd just introduce a minor number
>> >> to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.
>> >
>> > So I guess to answer my question, you are just making up version
>> > numbers. Unless you are doing the IP verilog too, don't do that.
>>
>> Well, in this case my proposal would be that we change the hardware team's
>> versioning scheme to match whatever we decide on the versioning scheme should
>> be as a part of this discussion.  I proposed accepting whatever versioning
>> scheme is decided upon hereto the hardware team before discussing changing the
>> naming scheme and they agreed to do so.
>>
>> So we're really in the drivers' seat here.
>
> Okay. So you could have something like x.y.z versioning where a change
> in X is change in register layout and/or major functional change, Y is
> backwards compatible changes (i.e. superset of functionality), Z is
> not s/w visible changes (though trusting that Z changes are really not
> s/w visible makes me nervous). If you have a version register, then
> the compatible string only needs to be specific enough to read that
> register.
>
> And if I can have anything I ask for, then any configuration option
> needs to be s/w readable too. Things like FIFO sizes, number of
> channels, etc. Sometimes these are DT properties, but generally the
> preference is to make these implied by the compatible. There's no set
> rule though. The problem with properties is they work well if you know
> up front to have them, but often something is fixed and implied and
> then becomes variable when a new version of the block is created.
>
> Think of it this way, DT is a work-around for non-discoverable h/w.
> The best solution is making the h/w more discoverable. If you look at
> the ARM Primecell aka amba_bus drivers, we don't actually match on the
> device compatible string (e.g. arm,pl011). Only "arm,primecell" is
> used and then we match with standard ID registers from there. We only
> need DT to discover where those blocks are, not what they are or what
> version they are (and of course other things like clocks, regulators,
> etc.).

Ya, good, that's generally been my model for it.  In the PLIC there isn't a 
whole lot in terms of configuration options to probe dynamically, as the 
register layout is fixed (determined by the device tree compat string) and the 
interrupt routings are fixed (determined by interrupt-parent and reg).  There's 
some amount of scheme for probing these, but since the routings aren't 
discoverable without the device tree they're not really useful in practice.

Our more modern blocks are becoming larger in scope, and these tend to have 
their options determined by software.  For example, SiFive's CLIC proposal (a 
proposal for a new RISC-V core-local interrupt controller) has many runtime 
discoverable properties, with pretty much only the base address being specified 
in the device tree -- here's an example of a binding

		L1: interrupt-controller@2000000 {
			compatible = "riscv,clic0";
			interrupts-extended = <&L2 3 &L2 7 &L2 11>;
			reg = <0x2000000 0x1000000>;
			reg-names = "control";
		};

I don't want to go too far down the CLIC path, though, as there's still a lot 
of work to be done there :)

> So what your compatible string(s) need to look like depend on all the
> above. It can be very generic if you can identify the block, version,
> and capabilities in a standard way.

For now I think the best bet is to go with "sifive,plic-1.0.0" as well as 
"sifive,fu540-c000-plic".  That way we've got plenty of room to avoid painting 
ourselves into a corner later.  In the interest of getting people a system they 
can start testing, I'm going to go ahead and bring the PLIC driver into my 
for-next branch with the "sifive,plic-1.0.0" binding.  I don't want to just 
unilaterally kill the discussion, but I figure we can always submit a fixup 
driver patch later if I'm wrong and we're not on the same page now.

I've also gone ahead and opened a PR against the RTL generator, but I'll wait 
on the bindings to be officially accepted before merging that:

    https://github.com/freechipsproject/rocket-chip/pull/1575

I'm assuming the bindings will go in through your tree?

>> > If you want to use just 'sifive,plic' then I'm fine with that. I've
>> > given you the potential problems with that and they will be your
>> > problems to deal with. Maybe you'll get lucky. Plus it won't be a
>> > problem for the 1st implementation.
>>
>> I'd prefer to have some versioning scheme, that's why I'm talking so much about
>> this :).  I really just want to learn how to get the right one, as I'm quite
>> new to all this and we'll have many of these.
>>
>> >> I don't have a concrete idea of when the minor number would be used in the
>> >> PLIC, but we do have a UART and I'd like to make a minor revision of that.
>> >> This might be too much detail, but essentially the UART consists of two parts:
>> >> a byte-wide FIFO that runs at the processor's clock, and then a bit-wide shift
>> >> register that ruts at the UART clock.  The shift register is driven by a simple
>> >> digital clock divider off the FIFO's clock, which means that whenever you
>> >> change the FIFO's clock speed (for power management or whatever) you also need
>> >> to change the clock divider to keep the UART's baud rate constant.
>> >>
>> >> As a result, if you change the clock while the UART is in the middle of
>> >> transmitting a byte then you get corruption.  There's a signal that says "the
>> >> UART TX queue is empty" that can be read from software, but that signal points
>> >> to the TX FIFO and doesn't account for the additional time to stream out the
>> >> contents of the shift register.  There are configurations of the baud rate, bus
>> >> latency, and clock speeds such that the "TX FIFO empty" poll can make it back
>> >> to the core and the core's write to the clock controller can materialize at
>> >> whatever magic makes the clocks change before the UART has serialized out every
>> >> bit in the shift register, which manifests as corruption.
>> >
>> > I've experienced some broken UART clocking like that in the past.
>> >
>> > That's a good example of why you may need SoC specific (or integration
>> > specific) compatible strings. A future design could clock the FIFO
>> > with a different clock that is fixed freq. A driver would distinguish
>> > this quirk with different compatible strings.
>>
>> Ah, OK -- this has me a bit worried that I really fundamentally understand
>> what's going on here.  In my model, if you have a UART that's missing the
>> "actually done" signal then it's the same UART regardless of whether it's
>> driver by a fixed or variable clock.  It's up to Linux to determine that this
>> configuration doesn't require draining queues on clock changes (kind of an
>> awkward example, as fixed clocks can't change), which we allow it to do by
>> associating a clock with each UART.  This allows the UART driver to see the rest
>> of the system, but in a manner rooted at the UART device as that's the part
>> most likely to be relevant to the UART driver.
>>
>> If the model is meant to be that "UART attached to a fixed clock" is different
>> then "UART attached to a variable clock", then we must tag each device with the
>> chip's fill name -- that's the only way to uniquely identify how the UART
>> behaves in the context of the entire system.
>
> Certainly you can do it either way in this example. I think it is
> simpler for the driver to just be told one way or the other rather
> than have to figure out details of the clock tree.
>
>> My one worry is that there will be a lot of these.  I'm not even privy to any
>> details about what's going on in RISC-V hardware land, but I can count a
>> hundred today and they can all use the same driver.  There's then the
>> additional headache that we'll never be able to publicly disclose most of these
>> designs, which leaves all of those having unspecified behavior.
>>
>> >> With the current UART hardware, which has a tentative compat string of
>> >> "sifive,uart0", we need to determine that the shift registers has drained in an
>> >> open-loop manner (ie, just wait for a bit).  This is ugly.  I'd like to spin a
>> >> minor version of the UART that just has an extra control bit made available to
>> >> software that tracks when the shift register drains, but since the drivers for
>> >> the old version would be compatible it seems like calling that "sifive,uart1"
>> >> is too big of a version jump.  Of course the Linux OF infrastructure assigns no
>> >> semantic meaning to compat strings so it doesn't matter here, but we use device
>> >> trees all over the place
>> >>
>> >> Thus, I thought that if we were going to change the naming scheme that we might
>> >> as well go put in a minor version number just to be safe.  Sorry if that's a
>> >> bit too much info... :)
>> >>
>> >> Of course, this is all open source RTL so you can just see what's going on
>> >> here in the PLIC
>> >>
>> >>     https://github.com/freechipsproject/rocket-chip/blob/384096a6a73f0e94d6f7bd4bc9cc422e0a213e88/src/main/scala/devices/tilelink/Plic.scala#L69
>> >>
>> >> and in the UART
>> >>
>> >>     https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L91
>> >>
>> >> We'll change the name generated by the hardware to match whatever's decided on
>> >> here, so there won't be a rift between the hardware and the software.
>> >
>> > What exactly gets generated by the hardware?
>>
>> Sorry, by "generated by the hardware" I really meant "generated along with the
>> hardware".  Essentially what happens is that whatever system collects the RTL
>> blocks to be emitted also collects the device tree nodes to be emitted, thus
>> ensuring they always match.  This is the ground truth for our device trees, and
>> it's how we ensure the device tree always matches the RTL.  Whatever compatible
>> string we agree on for the bindings will be reflected in this system, so the
>> ground truth always matches the spec.
>>
>> There's nothing we can do about what's already been shipped (ie, pre-spec
>> hardware), and ensuring that RTL modifications that change software
>> compatibility cause a change to the device tree node's compat string is
>> something we at SiFive enforce by code review.
>>
>> >> This actually brings up an issue with the standard naming scheme, which in this
>> >> cause would be something like "sifive,u540-c000-plic": the RTL is open source,
>> >> so anyone can go build a chip with it at any time.  I've been operating under
>> >> the impression that it will be impossible to maintain a central database of all
>> >> implementations, so the fallback name will be the defacto name that becomes the
>> >> interface.
>> >>
>> >> I'd be happy to put a "sifive,u540-c000-plic" in there (I'd called in
>> >> "sifive,u540-c000,plic" before, but that's just me not knowing the standard
>> >> naming scheme) just to make sure we have a specific name.  We at SiFive can
>> >> make sure we provide sane unique names for all our implementations, but I think
>> >> we also really need to hammer out what the general compat string is because I
>> >> don't think we can rely one everyone to do that.
>> >
>> > Propose something. If compatible strings can be traced back to exactly
>> > what versions of IP are used and there's a well defined process for
>> > setting version numbers, then I'm fine with using version numbers in
>> > compatible strings. Having version registers are helpful in that
>> > regard. And if there are configuration options in the IP, registers to
>> > read those too.
>> >
>> > Presumably folks (including SiFive?) fork and modify the opensource
>> > IP? Along the same lines as no one ships mainline kernels. Not sure
>> > how you deal with that if you only use versions.
>>
>> We (and as far as I know, other vendors -- though I know little about anyone
>> else' flow) ship RTL from the upstream blocks.  The blocks tend to just become
>> stable enough that the only changes are bug fixes anyway, and as we approach
>> tape out there are very few bugs.
>>
>> The trend towards stability is particularly true of blocks like the PLIC that
>> have been taped out multiple times, where pretty much all that changes is the
>> interface to the generator.  There's a bunch of top-level stuff that's done out
>> of tree, and we add our own special blocks in, but for the leaf blocks we
>> generally match upstream closely.
>>
>> Or at least that's what it looks like to me -- I don't do ECOs here, so maybe
>> I'm just in the dark :)
>
> Okay, that's good to know. I guess there's at least some motivation to
> not touch and break things you're going to set in stone (or Si
> really).
>
> [...]
>
>> >> > There are no "Linux device tree bindings". There are DT bindings that
>> >> > happen to be hosting within the Linux tree for convenience.
>> >>
>> >> Ah, OK.  By "Linux device tree bindings" I meant the ones stored in
>> >> Documentation, which is what we considered the authoritative source and were
>> >> planning on adopting everywhere we use device tree (ie, fixing the rest of our
>> >> code as a result of the discussions we have submitting the bindings).  I'd
>> >> heard some people refer to these as Linux specific, but I'm glad they're not as
>> >> it means we won't be pushing upstream on getting everyone to agree on one set
>> >> of bindings.
>> >>
>> >> Does this mean I can submit bindings for devices that don't have a Linux
>> >> driver?
>> >
>> > Certainly. ARM Mali GPU bindings don't have an (upstream) driver.
>> >
>> > My main concern is if there is neither a driver nor a dts using a
>> > binding, how do we know if it is used or not at some time in the
>> > future.
>>
>> Well, I guess my argument would be: why does this matter?  If it's in the spec
>> then it's in the spec, and we're not breaking anything that's compatible so we
>> can't change it.
>
> It is not always crystal clear what's valid or not in bindings and
> having an actual dts or client implementation can help. Or there's
> been times we've wanted to make incompatible changes (yes, it happens.
> :)) and looking at dts files and drivers is an indication whether or
> not we can get away with changes (it's only an incompatible change if
> someone notices).
>
> With anything, there's maintenance and refactorings that happen from
> time to time. For example, I'm working on moving DT bindings to
> json-schema[1]. It's easier if there aren't obsolete things we're
> carrying.

Make sense -- I'll just try to make sure we avoid doing anything stupid here so 
it's not a headache :)

Re: [PATCH 03/11] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Rob Herring]

On Fri, Aug 10, 2018 at 2:09 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Fri, 10 Aug 2018 09:57:03 PDT (-0700), robh+dt@kernel.org wrote:
> > On Thu, Aug 9, 2018 at 12:29 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>
> >> On Wed, 08 Aug 2018 16:32:07 PDT (-0700), robh+dt@kernel.org wrote:
> >> > On Wed, Aug 8, 2018 at 1:38 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >>
> >> >> On Wed, 08 Aug 2018 07:16:14 PDT (-0700), robh+dt@kernel.org wrote:
> >> >> > On Tue, Aug 7, 2018 at 8:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >> >>
> >> >> >> On Mon, 06 Aug 2018 13:59:48 PDT (-0700), robh+dt@kernel.org wrote:
> >> >> >> > On Thu, Aug 2, 2018 at 4:08 PM Atish Patra <atish.patra@wdc.com> wrote:
> >> >> >> >>
> >> >> >> >> On 8/2/18 4:50 AM, Christoph Hellwig wrote:
> >> >> >> >> > From: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> >> >> >
> >> >> >> >> > This patch adds documentation for the platform-level interrupt
> >> >> >> >> > controller (PLIC) found in all RISC-V systems.  This interrupt
> >> >> >> >> > controller routes interrupts from all the devices in the system to each
> >> >> >> >> > hart-local interrupt controller.
> >> >> >> >> >
> >> >> >> >> > Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> >> >> >> >> > want to change how we're specifying holes in the hart list.
> >> >> >> >> >
> >> >> >> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> >> >> >> >> > [hch: various fixes and updates]
> >> >> >> >> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> >> >> >> > ---
> >> >> >> >> >   .../interrupt-controller/sifive,plic0.txt     | 57 +++++++++++++++++++
> >> >> >> >> >   1 file changed, 57 insertions(+)
> >> >> >> >> >   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> >> >
> >> >> >> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> >> > new file mode 100644
> >> >> >> >> > index 000000000000..c756cd208a93
> >> >> >> >> > --- /dev/null
> >> >> >> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic0.txt
> >> >> >> >> > @@ -0,0 +1,57 @@
> >> >> >> >> > +SiFive Platform-Level Interrupt Controller (PLIC)
> >> >> >> >> > +-------------------------------------------------
> >> >> >> >> > +
> >> >> >> >> > +SiFive SOCs include an implementation of the Platform-Level Interrupt Controller
> >> >> >> >> > +(PLIC) high-level specification in the RISC-V Privileged Architecture
> >> >> >> >> > +specification.  The PLIC connects all external interrupts in the system to all
> >> >> >> >> > +hart contexts in the system, via the external interrupt source in each hart.
> >> >> >> >> > +
> >> >> >> >> > +A hart context is a privilege mode in a hardware execution thread.  For example,
> >> >> >> >> > +in an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> >> >> >> >> > +privilege modes per hart; machine mode and supervisor mode.
> >> >> >> >> > +
> >> >> >> >> > +Each interrupt can be enabled on per-context basis. Any context can claim
> >> >> >> >> > +a pending enabled interrupt and then release it once it has been handled.
> >> >> >> >> > +
> >> >> >> >> > +Each interrupt has a configurable priority. Higher priority interrupts are
> >> >> >> >> > +serviced first. Each context can specify a priority threshold. Interrupts
> >> >> >> >> > +with priority below this threshold will not cause the PLIC to raise its
> >> >> >> >> > +interrupt line leading to the context.
> >> >> >> >> > +
> >> >> >> >> > +While the PLIC supports both edge-triggered and level-triggered interrupts,
> >> >> >> >> > +interrupt handlers are oblivious to this distinction and therefore it is not
> >> >> >> >> > +specified in the PLIC device-tree binding.
> >> >> >> >> > +
> >> >> >> >> > +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> >> >> >> >> > +"sifive,plic0" device is a concrete implementation of the PLIC that contains a
> >> >> >> >> > +specific memory layout, which is documented in chapter 8 of the SiFive U5
> >> >> >> >> > +Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> >> >> >> >> > +
> >> >> >> >> > +Required properties:
> >> >> >> >> > +- compatible : "sifive,plic0"
> >> >> >>
> >> >> >> I think there was a thread bouncing around somewhere where decided to pick the
> >> >> >> official name of the compatible string to be "sifive,plic-1.0".  The idea here
> >> >> >> is that the PLIC is compatible across all of SiFive's current implementations,
> >> >> >> but there's some limitations in the memory map that will probably cause us to
> >> >> >> spin a version 2 at some point so we want major version number.  The minor
> >> >> >> number is just in case we find some errata in the PLIC.
> >> >> >
> >> >> > Is 1.0 an actual version number corresponding to an exact, revision
> >> >> > controlled version of the IP or just something you made up? Looks like
> >> >> > the latter to me and I'm not a fan of s/w folks making up version
> >> >> > numbers for h/w. Standard naming convention is <vendor>,<soc>-<block>
> >> >> > unless you have good reason to deviate (IP for FPGAs where version
> >> >> > numbers are exposed to customers is one example).
> >> >>
> >> >> The hardware versioning scheme calls it "riscv,plic0", which is what we were
> >> >> originally using.  This PLIC isn't actually defined as a RISC-V spec, so we
> >> >> wanted to change it to a "sifive,*" name instead.  Since we were going to
> >> >> change the compat string anyway, I thought we'd just introduce a minor number
> >> >> to be safe.  Since "0.0" is an awkward number, I figured "1.0" would be saner.
> >> >
> >> > So I guess to answer my question, you are just making up version
> >> > numbers. Unless you are doing the IP verilog too, don't do that.
> >>
> >> Well, in this case my proposal would be that we change the hardware team's
> >> versioning scheme to match whatever we decide on the versioning scheme should
> >> be as a part of this discussion.  I proposed accepting whatever versioning
> >> scheme is decided upon hereto the hardware team before discussing changing the
> >> naming scheme and they agreed to do so.
> >>
> >> So we're really in the drivers' seat here.
> >
> > Okay. So you could have something like x.y.z versioning where a change
> > in X is change in register layout and/or major functional change, Y is
> > backwards compatible changes (i.e. superset of functionality), Z is
> > not s/w visible changes (though trusting that Z changes are really not
> > s/w visible makes me nervous). If you have a version register, then
> > the compatible string only needs to be specific enough to read that
> > register.
> >
> > And if I can have anything I ask for, then any configuration option
> > needs to be s/w readable too. Things like FIFO sizes, number of
> > channels, etc. Sometimes these are DT properties, but generally the
> > preference is to make these implied by the compatible. There's no set
> > rule though. The problem with properties is they work well if you know
> > up front to have them, but often something is fixed and implied and
> > then becomes variable when a new version of the block is created.
> >
> > Think of it this way, DT is a work-around for non-discoverable h/w.
> > The best solution is making the h/w more discoverable. If you look at
> > the ARM Primecell aka amba_bus drivers, we don't actually match on the
> > device compatible string (e.g. arm,pl011). Only "arm,primecell" is
> > used and then we match with standard ID registers from there. We only
> > need DT to discover where those blocks are, not what they are or what
> > version they are (and of course other things like clocks, regulators,
> > etc.).
>
> Ya, good, that's generally been my model for it.  In the PLIC there isn't a
> whole lot in terms of configuration options to probe dynamically, as the
> register layout is fixed (determined by the device tree compat string) and the
> interrupt routings are fixed (determined by interrupt-parent and reg).  There's
> some amount of scheme for probing these, but since the routings aren't
> discoverable without the device tree they're not really useful in practice.
>
> Our more modern blocks are becoming larger in scope, and these tend to have
> their options determined by software.  For example, SiFive's CLIC proposal (a
> proposal for a new RISC-V core-local interrupt controller) has many runtime
> discoverable properties, with pretty much only the base address being specified
> in the device tree -- here's an example of a binding
>
>                 L1: interrupt-controller@2000000 {
>                         compatible = "riscv,clic0";
>                         interrupts-extended = <&L2 3 &L2 7 &L2 11>;
>                         reg = <0x2000000 0x1000000>;
>                         reg-names = "control";
>                 };
>
> I don't want to go too far down the CLIC path, though, as there's still a lot
> of work to be done there :)
>
> > So what your compatible string(s) need to look like depend on all the
> > above. It can be very generic if you can identify the block, version,
> > and capabilities in a standard way.
>
> For now I think the best bet is to go with "sifive,plic-1.0.0" as well as
> "sifive,fu540-c000-plic".  That way we've got plenty of room to avoid painting
> ourselves into a corner later.  In the interest of getting people a system they
> can start testing, I'm going to go ahead and bring the PLIC driver into my
> for-next branch with the "sifive,plic-1.0.0" binding.  I don't want to just
> unilaterally kill the discussion, but I figure we can always submit a fixup
> driver patch later if I'm wrong and we're not on the same page now.

Okay.

> I've also gone ahead and opened a PR against the RTL generator, but I'll wait
> on the bindings to be officially accepted before merging that:
>
>     https://github.com/freechipsproject/rocket-chip/pull/1575
>
> I'm assuming the bindings will go in through your tree?

No, they generally go thru the arch or subsystem trees with the code
that uses them (unless there isn't any and I'll take them).

Rob