[RFC PATCH 0/5] New RISC-V Local Interrupt Controller Driver

[RFC PATCH 0/5] New RISC-V Local Interrupt Controller Driver

[Anup Patel]

This patchset provides a new RISC-V Local Interrupt Controller Driver
for managing per-CPU local interrupts. The overall approach is inspired
from the way per-CPU local interrupts are handled by Linux ARM64 and
ARM GICv3 driver.

Few advantages of having this new driver are as follows:
1. It registers all local interrupts as per-CPU interrupts
2. We can develop drivers for devices with per-CPU local interrupts
without changing arch code or this driver
3. It allows local interrupt controller DT node under each CPU DT node
as well as single system-wide DT node for local interrupt controller.

With this patchset, output of "cat /proc/interrupts" looks as follows:
           CPU0       CPU1       CPU2       CPU3       
  5:        995       1012        997       1015  RISC-V INTC   5 Edge      riscv_timer
  8:         23          6         10          7  SiFive PLIC   8 Edge      virtio0
 10:          9         10          5          4  SiFive PLIC  10 Edge      ttyS0

The patchset is based up Linux-4.19-rc2 and can be found at
riscv_intc_v1 branch of:
https://github.com/avpatel/linux.git

Anup Patel (5):
  RISC-V: Make IPI triggering flexible
  RISC-V: No need to pass scause as arg to do_IRQ()
  RISC-V: Select useful GENERIC_IRQ kconfig options
  irqchip: RISC-V Local Interrupt Controller Driver
  clocksource: riscv_timer: Make timer interrupt as a per-CPU interrupt

 arch/riscv/Kconfig                |   4 +
 arch/riscv/include/asm/irq.h      |  16 ++-
 arch/riscv/include/asm/smp.h      |  10 ++
 arch/riscv/kernel/entry.S         |   1 -
 arch/riscv/kernel/irq.c           |  45 +--------
 arch/riscv/kernel/smp.c           |  23 ++++-
 drivers/clocksource/riscv_timer.c |  76 ++++++++++++---
 drivers/irqchip/Kconfig           |  15 ++-
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-riscv-intc.c  | 156 ++++++++++++++++++++++++++++++
 drivers/irqchip/irq-sifive-plic.c |  21 +++-
 include/linux/cpuhotplug.h        |   1 +
 12 files changed, 301 insertions(+), 68 deletions(-)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

-- 
2.17.1

[RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Anup Patel]

The mechanism to trigger IPI is generally part of interrupt-controller
driver for various architectures. On RISC-V, we have an option to trigger
IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
triggered using SOC interrupt-controller (e.g. custom PLIC).

This patch makes IPI triggering flexible by providing a mechanism for
interrupt-controller driver to register IPI trigger function using
set_smp_ipi_trigger() function.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/include/asm/irq.h |  1 -
 arch/riscv/include/asm/smp.h | 10 ++++++++++
 arch/riscv/kernel/irq.c      | 26 +++++++++++++++++++++-----
 arch/riscv/kernel/smp.c      | 23 +++++++++++++++++++----
 4 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 996b6fbe17a6..93eb75eac4ff 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -18,7 +18,6 @@
 #define NR_IRQS         0
 
 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 36016845461d..72671580e1d4 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -27,6 +27,16 @@
 /* SMP initialization hook for setup_arch */
 void __init setup_smp(void);
 
+/*
+ * Called from C code, this handles an IPI.
+ */
+void handle_IPI(struct pt_regs *regs);
+
+/*
+ * Provide a function to raise an IPI on CPUs in callmap.
+ */
+void __init set_smp_ipi_trigger(void (*fn)(const struct cpumask *));
+
 /* Hook for the generic smp_call_function_many() routine. */
 void arch_send_call_function_ipi_mask(struct cpumask *mask);
 
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 0cfac48a1272..5532e7cedaec 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -9,6 +9,8 @@
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
 
+#include <asm/sbi.h>
+
 /*
  * Possible interrupt causes:
  */
@@ -26,12 +28,15 @@
 
 asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
 {
-	struct pt_regs *old_regs = set_irq_regs(regs);
+	struct pt_regs *old_regs;
 
-	irq_enter();
 	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
 	case INTERRUPT_CAUSE_TIMER:
+		old_regs = set_irq_regs(regs);
+		irq_enter();
 		riscv_timer_interrupt();
+		irq_exit();
+		set_irq_regs(old_regs);
 		break;
 #ifdef CONFIG_SMP
 	case INTERRUPT_CAUSE_SOFTWARE:
@@ -39,21 +44,32 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
 		 * 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();
+		handle_IPI(regs);
 		break;
 #endif
 	case INTERRUPT_CAUSE_EXTERNAL:
+		old_regs = set_irq_regs(regs);
+		irq_enter();
 		handle_arch_irq(regs);
+		irq_exit();
+		set_irq_regs(old_regs);
 		break;
 	default:
 		panic("unexpected interrupt cause");
 	}
-	irq_exit();
+}
 
-	set_irq_regs(old_regs);
+#ifdef CONFIG_SMP
+static void smp_ipi_trigger_sbi(const struct cpumask *to_whom)
+{
+	sbi_send_ipi(cpumask_bits(to_whom));
 }
+#endif
 
 void __init init_IRQ(void)
 {
 	irqchip_init();
+#ifdef CONFIG_SMP
+	set_smp_ipi_trigger(smp_ipi_trigger_sbi);
+#endif
 }
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 906fe21ea21b..04571d77eceb 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -38,17 +38,19 @@ enum ipi_message_type {
 	IPI_MAX
 };
 
-
 /* Unsupported */
 int setup_profiling_timer(unsigned int multiplier)
 {
 	return -EINVAL;
 }
 
-void riscv_software_interrupt(void)
+void handle_IPI(struct pt_regs *regs)
 {
+	struct pt_regs *old_regs = set_irq_regs(regs);
 	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
 
+	irq_enter();
+
 	/* Clear pending IPI */
 	csr_clear(sip, SIE_SSIE);
 
@@ -60,7 +62,7 @@ void riscv_software_interrupt(void)
 
 		ops = xchg(pending_ipis, 0);
 		if (ops == 0)
-			return;
+			goto done;
 
 		if (ops & (1 << IPI_RESCHEDULE))
 			scheduler_ipi();
@@ -73,6 +75,17 @@ void riscv_software_interrupt(void)
 		/* Order data access and bit testing. */
 		mb();
 	}
+
+done:
+	irq_exit();
+	set_irq_regs(old_regs);
+}
+
+static void (*__smp_ipi_trigger)(const struct cpumask *);
+
+void __init set_smp_ipi_trigger(void (*fn)(const struct cpumask *))
+{
+	__smp_ipi_trigger = fn;
 }
 
 static void
@@ -85,7 +98,9 @@ send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
 		set_bit(operation, &ipi_data[i].bits);
 
 	mb();
-	sbi_send_ipi(cpumask_bits(to_whom));
+
+	if (__smp_ipi_trigger)
+		__smp_ipi_trigger(to_whom);
 }
 
 void arch_send_call_function_ipi_mask(struct cpumask *mask)
-- 
2.17.1

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote:
> The mechanism to trigger IPI is generally part of interrupt-controller
> driver for various architectures. On RISC-V, we have an option to trigger
> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
> triggered using SOC interrupt-controller (e.g. custom PLIC).

Which is exactly what we want to avoid, and should not make it easy.

The last thing we need is non-standard whacky IPI mechanisms, and
that is why we habe SBI calls for it.  I think we should simply
stat that if an RISC-V cpu design bypasse the SBI for no good reason
we'll simply not support it.

So NAK for this patch.

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Anup Patel]

On Wed, Sep 5, 2018 at 12:20 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote:
>> The mechanism to trigger IPI is generally part of interrupt-controller
>> driver for various architectures. On RISC-V, we have an option to trigger
>> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
>> triggered using SOC interrupt-controller (e.g. custom PLIC).
>
> Which is exactly what we want to avoid, and should not make it easy.
>
> The last thing we need is non-standard whacky IPI mechanisms, and
> that is why we habe SBI calls for it.  I think we should simply
> stat that if an RISC-V cpu design bypasse the SBI for no good reason
> we'll simply not support it.

It's outrageous to call IPI mechanisms using interrupt-controller as "wacky".

Lot of architectures have well thought-out interrupt-controller designs with
IPI support.

In fact having IPIs through interrupt-controller drivers is much faster because
SBI call will have it's own overhead and M-mode code with eventually write
to some platform-specific/interrupt-controller register. The SBI call only makes
sense for very simple interrupt-controller (such as PLIC) which do not provide
IPI mechanism. It totally seems like SBI call for triggering IPIs was added as
workaround to address limitations of current PLIC.

RISC-V systems require a more mature and feature complete interrupt-controllers
which supports IPIs, PCI MSI, and Virtualization Extensions.

I am sure will see a much better interrupt controller (PLIC++ or something else)
soon.

>
> So NAK for this patch.

I think you jumped the gun to quickly here.

This patch does two things:
1. Adds a pluggable IPI triggering mechanism
2. Make IPI handling mechanism more generic so that we can
call IPI handler from interrupt-controller driver.

Your primary objection seems to be for point1 above. I will drop that
part only keep changes related to point2 above.

Regards,
Anup

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Christoph Hellwig]

On Wed, Sep 05, 2018 at 10:06:24AM +0530, Anup Patel wrote:
> It's outrageous to call IPI mechanisms using interrupt-controller as "wacky".

I call pluggable IPI whacky, and it's not outragous.  What is outragous
is your bullshit architecture astronaut patches.

There might be a nned to abstract IPI details even more in the future,
but the way to do it is either architectureally in the RISC-V privileged
spec, or in the SBI spec once we actually have one.

Having host OSes go through hoops to provide pluggable IPI
implementations is complete bullshit, and the argument that we already
have this in some architectures is not a good reason to repeat that
mistake.

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Palmer Dabbelt]

On Tue, 04 Sep 2018 11:50:02 PDT (-0700), Christoph Hellwig wrote:
> On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote:
>> The mechanism to trigger IPI is generally part of interrupt-controller
>> driver for various architectures. On RISC-V, we have an option to trigger
>> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
>> triggered using SOC interrupt-controller (e.g. custom PLIC).
>
> Which is exactly what we want to avoid, and should not make it easy.
>
> The last thing we need is non-standard whacky IPI mechanisms, and
> that is why we habe SBI calls for it.  I think we should simply
> stat that if an RISC-V cpu design bypasse the SBI for no good reason
> we'll simply not support it.

I agree.  Hiding this sort of stuff is the whole point of the SBI.

Anup: do you have some concrete reason for trying to avoid the SBI?  If it's 
just to add non-standard interrupt controllers then I don't think that's a 
sufficient reason, as you can just add support for whatever the non-standard 
interrupt mechanism is in the SBI implementation -- that's what we're doing 
with BBL's CLINT driver, though there's not a whole lot of wackiness there so 
at least the SBI implementation is pretty small.

> So NAK for this patch.

Certainly without a compelling reason, and even then I'd only want to take some 
standard interrupt controller -- for example, the CLIC (or whatever the result 
of the fast interrupts task group is called) could be a viable option.  Even 
with a standard interrupt controller, we'd need a really compelling reason to 
do so.

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Anup Patel]

On Thu, Sep 6, 2018 at 3:15 PM, Palmer Dabbelt <palmer@sifive.com> wrote:
> On Tue, 04 Sep 2018 11:50:02 PDT (-0700), Christoph Hellwig wrote:
>>
>> On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote:
>>>
>>> The mechanism to trigger IPI is generally part of interrupt-controller
>>> driver for various architectures. On RISC-V, we have an option to trigger
>>> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
>>> triggered using SOC interrupt-controller (e.g. custom PLIC).
>>
>>
>> Which is exactly what we want to avoid, and should not make it easy.
>>
>> The last thing we need is non-standard whacky IPI mechanisms, and
>> that is why we habe SBI calls for it.  I think we should simply
>> stat that if an RISC-V cpu design bypasse the SBI for no good reason
>> we'll simply not support it.
>
>
> I agree.  Hiding this sort of stuff is the whole point of the SBI.
>
> Anup: do you have some concrete reason for trying to avoid the SBI?  If it's
> just to add non-standard interrupt controllers then I don't think that's a
> sufficient reason, as you can just add support for whatever the non-standard
> interrupt mechanism is in the SBI implementation -- that's what we're doing
> with BBL's CLINT driver, though there's not a whole lot of wackiness there
> so at least the SBI implementation is pretty small.
>
>> So NAK for this patch.
>
>
> Certainly without a compelling reason, and even then I'd only want to take
> some standard interrupt controller -- for example, the CLIC (or whatever the
> result of the fast interrupts task group is called) could be a viable
> option.  Even with a standard interrupt controller, we'd need a really
> compelling reason to do so.

This patch is doing two things:
1. Allow IRQCHIP driver to provide IPI trigger mechanism
2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver
can call it

The main intention behind point1 was to allow interrupt-controller
specific IPI triggering mechanism. I am totally fine in dropping changes
related to point1. May be we can revisit this if we find compelling use-case.

I will revise this patch to have changes related to point2 only. These
changes are required for the new RISCV local interrupt controller
driver introduced by this patchset.

Regards,
Anup

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Christoph Hellwig]

On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote:
> This patch is doing two things:
> 1. Allow IRQCHIP driver to provide IPI trigger mechanism

And the big questions is why do we want that?  The last thing we
want is for people to "innovate" on how they deliver IPIs.  RISC-V
has defined an SBI interface for it to hide all the details, and
we should not try to handle systems that are not SBI compliant.

Eventuall we might want to revisit the SBI to improve on shortcomings
if there are any, but we should not allow random irqchip drivers to
override this.

> 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver
> can call it

And that is rather irrelevant without 1) above.

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Anup Patel]

On Mon, Sep 10, 2018 at 7:04 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote:
>> This patch is doing two things:
>> 1. Allow IRQCHIP driver to provide IPI trigger mechanism
>
> And the big questions is why do we want that?  The last thing we
> want is for people to "innovate" on how they deliver IPIs.  RISC-V
> has defined an SBI interface for it to hide all the details, and
> we should not try to handle systems that are not SBI compliant.
>
> Eventuall we might want to revisit the SBI to improve on shortcomings
> if there are any, but we should not allow random irqchip drivers to
> override this.

I have already dropped this part from the PATCH v2.

>
>> 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver
>> can call it
>
> And that is rather irrelevant without 1) above.

Nopes, this is required for the RISC-V INTC driver.

Regards,
Anup

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Palmer Dabbelt]

On Mon, 10 Sep 2018 06:34:18 PDT (-0700), Christoph Hellwig wrote:
> On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote:
>> This patch is doing two things:
>> 1. Allow IRQCHIP driver to provide IPI trigger mechanism
>
> And the big questions is why do we want that?  The last thing we
> want is for people to "innovate" on how they deliver IPIs.  RISC-V
> has defined an SBI interface for it to hide all the details, and
> we should not try to handle systems that are not SBI compliant.
>
> Eventuall we might want to revisit the SBI to improve on shortcomings
> if there are any, but we should not allow random irqchip drivers to
> override this.

I agree.  The whole point of the SBI is to provide an interface that everyone 
uses so we can the go figure out how to make this fast later.  If each platform 
has their own magic IPI hooks then this will end up being a mess.

We've got some schemes floating around to make the SBI fast (essentially an SBI 
VDSO), I'd prefer to push on that rather than adding a bunch of complexity 
here.

>
>> 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver
>> can call it
>
> And that is rather irrelevant without 1) above.

Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible

[Anup Patel]

On Sat, Sep 29, 2018 at 7:15 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Mon, 10 Sep 2018 06:34:18 PDT (-0700), Christoph Hellwig wrote:
> > On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote:
> >> This patch is doing two things:
> >> 1. Allow IRQCHIP driver to provide IPI trigger mechanism
> >
> > And the big questions is why do we want that?  The last thing we
> > want is for people to "innovate" on how they deliver IPIs.  RISC-V
> > has defined an SBI interface for it to hide all the details, and
> > we should not try to handle systems that are not SBI compliant.
> >
> > Eventuall we might want to revisit the SBI to improve on shortcomings
> > if there are any, but we should not allow random irqchip drivers to
> > override this.
>
> I agree.  The whole point of the SBI is to provide an interface that everyone
> uses so we can the go figure out how to make this fast later.  If each platform
> has their own magic IPI hooks then this will end up being a mess.
>
> We've got some schemes floating around to make the SBI fast (essentially an SBI
> VDSO), I'd prefer to push on that rather than adding a bunch of complexity
> here.

Yes, I have already removed the IPI triggering part from this patchset.

Regards,
Anup

[RFC PATCH 2/5] RISC-V: No need to pass scause as arg to do_IRQ()

[Anup Patel]

The scause is already part of pt_regs so no need to pass
scause as separate arg to do_IRQ().

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/kernel/entry.S | 1 -
 arch/riscv/kernel/irq.c   | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index fa2c08e3c05e..6eaacfa5b63d 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -168,7 +168,6 @@ ENTRY(handle_exception)
 
 	/* Handle interrupts */
 	move a0, sp /* pt_regs */
-	move a1, s4 /* scause */
 	tail do_IRQ
 1:
 	/* Exceptions run with interrupts enabled */
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 5532e7cedaec..c51c9b402e87 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -26,11 +26,11 @@
  */
 #define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
 
-asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
+asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs)
 {
 	struct pt_regs *old_regs;
 
-	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
+	switch (regs->scause & ~INTERRUPT_CAUSE_FLAG) {
 	case INTERRUPT_CAUSE_TIMER:
 		old_regs = set_irq_regs(regs);
 		irq_enter();
-- 
2.17.1

Re: [RFC PATCH 2/5] RISC-V: No need to pass scause as arg to do_IRQ()

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 06:15:11PM +0530, Anup Patel wrote:
> The scause is already part of pt_regs so no need to pass
> scause as separate arg to do_IRQ().
> 
> Signed-off-by: Anup Patel <anup@brainfault.org>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[RFC PATCH 3/5] RISC-V: Select useful GENERIC_IRQ kconfig options

[Anup Patel]

This patch selects following GENERIC_IRQ kconfig options:
GENERIC_IRQ_MULTI_HANDLER
GENERIC_IRQ_PROBE
GENERIC_IRQ_SHOW_LEVEL
HANDLE_DOMAIN_IRQ

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index a344980287a5..026abe3d6cb0 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -22,7 +22,10 @@ config RISCV
 	select DMA_DIRECT_OPS
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_CPU_DEVICES
+	select GENERIC_IRQ_MULTI_HANDLER
+	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
+	select GENERIC_IRQ_SHOW_LEVEL
 	select GENERIC_PCI_IOMAP
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
@@ -33,6 +36,7 @@ config RISCV
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_PERF_EVENTS
+	select HANDLE_DOMAIN_IRQ
 	select IRQ_DOMAIN
 	select NO_BOOTMEM
 	select RISCV_ISA_A if SMP
-- 
2.17.1

Re: [RFC PATCH 3/5] RISC-V: Select useful GENERIC_IRQ kconfig options

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 06:15:12PM +0530, Anup Patel wrote:
> This patch selects following GENERIC_IRQ kconfig options:
> GENERIC_IRQ_MULTI_HANDLER

This is already selected by arch/riscv/Kconfig.

> GENERIC_IRQ_PROBE

This is something only used by ISA drivers.  Why would we want that
on RISC-V?

> GENERIC_IRQ_SHOW_LEVEL

We don't really have any special level triggerd irq handling in
RISC-V.  That being said this is trivial and I don't see why it
even is a Kconfig option.  Please have a discussion with Thomas
and Marc on why we have this option instead of a default.

> HANDLE_DOMAIN_IRQ

We aren't using handle_domain_irq anywhere in RISC-V, no need to
build this.

Re: [RFC PATCH 3/5] RISC-V: Select useful GENERIC_IRQ kconfig options

[Anup Patel]

On Wed, Sep 5, 2018 at 12:26 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Sep 04, 2018 at 06:15:12PM +0530, Anup Patel wrote:
>> This patch selects following GENERIC_IRQ kconfig options:
>> GENERIC_IRQ_MULTI_HANDLER
>
> This is already selected by arch/riscv/Kconfig.
>
>> GENERIC_IRQ_PROBE
>
> This is something only used by ISA drivers.  Why would we want that
> on RISC-V?

Yes, thanks for pointing. GENERIC_IRQ_PROBE is not required at
this time. I will drop this selection. May be will re-consider later.

>
>> GENERIC_IRQ_SHOW_LEVEL
>
> We don't really have any special level triggerd irq handling in
> RISC-V.  That being said this is trivial and I don't see why it
> even is a Kconfig option.  Please have a discussion with Thomas
> and Marc on why we have this option instead of a default.

Most of MMIO device interrupts are level-interrupts. In fact, all
HW interrupt lines coming to PLIC will be level-interrupts. It's
just that PLIC does not implement state machine to sample
Level-IRQs and Edge-IRQs differently. Even the interrupt-controller
virtualization in hypervisors deal with Level and Edge interrupts
differently.

I am sure we will see both Level and Edge triggered interrupts
in RISC-V system. The MMIO device interrupts will be mostly
Level triggered and PCI MSIs will be mapped as Edge triggered
by MSI controller.

We should definitely select GENERIC_IRQ_SHOW_LEVEL
so that nature of IRQ interrupt line is evident in /proc/interrupts.

>
>> HANDLE_DOMAIN_IRQ
>
> We aren't using handle_domain_irq anywhere in RISC-V, no need to
> build this.

The new RISC-V local interrupt controller driver introduced by
this patchset uses handle_domain_irq().

The main advantage of handle_domain_irq() is that it helps reduce
few lines of code which is otherwise common across interrupt-controller
drivers (mostly code related to irq_enter(), irq_exit(), and set_irq_regs()).

Regards,
Anup

Re: [RFC PATCH 3/5] RISC-V: Select useful GENERIC_IRQ kconfig options

[Christoph Hellwig]

On Wed, Sep 05, 2018 at 10:22:27AM +0530, Anup Patel wrote:
> I am sure we will see both Level and Edge triggered interrupts
> in RISC-V system. The MMIO device interrupts will be mostly
> Level triggered and PCI MSIs will be mapped as Edge triggered
> by MSI controller.
> 
> We should definitely select GENERIC_IRQ_SHOW_LEVEL
> so that nature of IRQ interrupt line is evident in /proc/interrupts.

Please settle the argument with Thomas and Marc on what the default
for this option should be - in the end it just shows another line
in procfs, and I see no reason for RISC-V to ever deviated from the
global Linux default here, whatever that default is.

> >> HANDLE_DOMAIN_IRQ
> >
> > We aren't using handle_domain_irq anywhere in RISC-V, no need to
> > build this.
> 
> The new RISC-V local interrupt controller driver introduced by
> this patchset uses handle_domain_irq().

So select it in the patch that needs it, not anywhere else.

[RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Anup Patel]

The RISC-V local interrupt controller manages software interrupts,
timer interrupts, external interrupts (which are routed via the
platform level interrupt controller) and per-HART local interrupts.

This patch add a driver for RISC-V local interrupt controller. It's
a major re-write over perviously submitted version.
(Refer, https://www.spinics.net/lists/devicetree/msg241230.html)

Few advantages of this new driver over previous one are:
1. It registers all local interrupts as per-CPU interrupts
2. We can develop drivers for devices with per-CPU local interrupts
without changing arch code or this driver
3. It allows local interrupt controller DT node under each CPU DT node
as well as single system-wide DT node for local interrupt controller.

The RISC-V INTC driver is compliant with RISC-V Hart-Level Interrupt
Controller DT bindings located at:
Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt

Signed-off-by: Anup Patel <anup@brainfault.org>
Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 arch/riscv/include/asm/irq.h      |  15 ++-
 arch/riscv/kernel/irq.c           |  59 +----------
 drivers/irqchip/Kconfig           |  15 ++-
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-riscv-intc.c  | 164 ++++++++++++++++++++++++++++++
 drivers/irqchip/irq-sifive-plic.c |  21 +++-
 include/linux/cpuhotplug.h        |   1 +
 7 files changed, 214 insertions(+), 62 deletions(-)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 93eb75eac4ff..fe503d71876a 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -15,7 +15,20 @@
 #ifndef _ASM_RISCV_IRQ_H
 #define _ASM_RISCV_IRQ_H
 
-#define NR_IRQS         0
+/*
+ * 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))
 
 void riscv_timer_interrupt(void);
 
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index c51c9b402e87..46a311e5f0f6 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -7,69 +7,16 @@
 
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
-#include <linux/irqdomain.h>
-
-#include <asm/sbi.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)
 {
-	struct pt_regs *old_regs;
-
-	switch (regs->scause & ~INTERRUPT_CAUSE_FLAG) {
-	case INTERRUPT_CAUSE_TIMER:
-		old_regs = set_irq_regs(regs);
-		irq_enter();
-		riscv_timer_interrupt();
-		irq_exit();
-		set_irq_regs(old_regs);
-		break;
-#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.
-		 */
-		handle_IPI(regs);
-		break;
-#endif
-	case INTERRUPT_CAUSE_EXTERNAL:
-		old_regs = set_irq_regs(regs);
-		irq_enter();
+	if (handle_arch_irq)
 		handle_arch_irq(regs);
-		irq_exit();
-		set_irq_regs(old_regs);
-		break;
-	default:
-		panic("unexpected interrupt cause");
-	}
-}
-
-#ifdef CONFIG_SMP
-static void smp_ipi_trigger_sbi(const struct cpumask *to_whom)
-{
-	sbi_send_ipi(cpumask_bits(to_whom));
 }
-#endif
 
 void __init init_IRQ(void)
 {
 	irqchip_init();
-#ifdef CONFIG_SMP
-	set_smp_ipi_trigger(smp_ipi_trigger_sbi);
-#endif
+	if (!handle_arch_irq)
+		panic("No interrupt controller found.");
 }
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 383e7b70221d..885e182586f4 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -371,7 +371,18 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
-endmenu
+config RISCV_INTC
+	bool "RISC-V Interrupt Controller"
+	depends on RISCV
+	default y
+	help
+	   This enables support for the local interrupt controller found in
+	   standard RISC-V systems.  The local interrupt controller handles
+	   timer interrupts, software interrupts, and hardware interrupts.
+	   Without a local interrupt controller the system will be unable to
+	   handle any interrupts, including those passed via the PLIC.
+
+	   If you don't know what to do here, say Y.
 
 config SIFIVE_PLIC
 	bool "SiFive Platform-Level Interrupt Controller"
@@ -384,3 +395,5 @@ config SIFIVE_PLIC
 	   interrupt sources are subordinate to the PLIC.
 
 	   If you don't know what to do here, say Y.
+
+endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index fbd1ec8070ef..e638ee5c4452 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -87,4 +87,5 @@ 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_RISCV_INTC)		+= irq-riscv-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
new file mode 100644
index 000000000000..c80502a1e12b
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017-2018 SiFive
+ * Copyright (C) 2018 Anup Patel
+ */
+
+#define pr_fmt(fmt) "riscv-intc: " fmt
+#include <linux/atomic.h>
+#include <linux/bits.h>
+#include <linux/cpu.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/smp.h>
+#include <asm/sbi.h>
+
+static struct irq_domain *intc_domain;
+static atomic_t intc_init = ATOMIC_INIT(0);
+
+static void riscv_intc_irq(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs;
+	unsigned long cause = regs->scause & ~INTERRUPT_CAUSE_FLAG;
+
+	if (unlikely(cause >= BITS_PER_LONG))
+		panic("unexpected interrupt cause");
+
+	switch (cause) {
+	case INTERRUPT_CAUSE_TIMER:
+		old_regs = set_irq_regs(regs);
+		irq_enter();
+		riscv_timer_interrupt();
+		irq_exit();
+		set_irq_regs(old_regs);
+		break;
+#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.
+		 */
+		handle_IPI(regs);
+		break;
+#endif
+	default:
+		handle_domain_irq(intc_domain, cause, regs);
+		break;
+	}
+}
+
+/*
+ * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
+ * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
+ * hart, these functions can only be called on the hart that corresponds to the
+ * IRQ chip.  They are only called internally to this module, so they BUG_ON if
+ * this condition is violated rather than attempting to handle the error by
+ * forwarding to the target hart, as that's already expected to have been done.
+ */
+static void riscv_intc_irq_mask(struct irq_data *d)
+{
+	csr_clear(sie, 1 << (long)d->hwirq);
+}
+
+static void riscv_intc_irq_unmask(struct irq_data *d)
+{
+	csr_set(sie, 1 << (long)d->hwirq);
+}
+
+#ifdef CONFIG_SMP
+static void riscv_intc_ipi_trigger(const struct cpumask *to_whom)
+{
+	sbi_send_ipi(cpumask_bits(to_whom));
+}
+
+static int riscv_intc_cpu_starting(unsigned int cpu)
+{
+	csr_set(sie, 1 << INTERRUPT_CAUSE_SOFTWARE);
+	return 0;
+}
+
+static int riscv_intc_cpu_dying(unsigned int cpu)
+{
+	csr_clear(sie, 1 << INTERRUPT_CAUSE_SOFTWARE);
+	return 0;
+}
+
+static void riscv_intc_smp_init(void)
+{
+	csr_write(sie, 0);
+	csr_write(sip, 0);
+
+	set_smp_ipi_trigger(riscv_intc_ipi_trigger);
+
+	cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING,
+			  "irqchip/riscv/intc:starting",
+			  riscv_intc_cpu_starting,
+			  riscv_intc_cpu_dying);
+
+}
+#else
+static void riscv_intc_smp_init(void)
+{
+	csr_write(sie, 0);
+	csr_write(sip, 0);
+}
+#endif
+
+static struct irq_chip riscv_intc_chip = {
+	.name = "RISC-V INTC",
+	.irq_mask = riscv_intc_irq_mask,
+	.irq_unmask = riscv_intc_irq_unmask,
+};
+
+static int riscv_intc_domain_map(struct irq_domain *d, unsigned int irq,
+				 irq_hw_number_t hwirq)
+{
+	irq_set_percpu_devid(irq);
+	irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data,
+			    handle_percpu_devid_irq, NULL, NULL);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+
+	return 0;
+}
+
+static const struct irq_domain_ops riscv_intc_domain_ops = {
+	.map	= riscv_intc_domain_map,
+	.xlate	= irq_domain_xlate_onecell,
+};
+
+static int __init riscv_intc_init(struct device_node *node,
+				  struct device_node *parent)
+{
+	/*
+	 * RISC-V device trees can have one INTC DT node under
+	 * each CPU DT node so INTC init function will be called
+	 * once for each INTC DT node. We only need to do INTC
+	 * init once for boot CPU so we use atomic counter to
+	 * achieve this.
+	 */
+	if (atomic_inc_return(&intc_init) > 1)
+		return 0;
+
+	intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
+					    &riscv_intc_domain_ops, NULL);
+	if (!intc_domain)
+		goto error_add_linear;
+
+	set_handle_irq(&riscv_intc_irq);
+
+	riscv_intc_smp_init();
+
+	pr_info("%lu local interrupts mapped\n", (long)BITS_PER_LONG);
+
+	return 0;
+
+error_add_linear:
+	pr_warn("unable to add IRQ domain\n");
+	return -ENXIO;
+}
+
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 532e9d68c704..ab9614d5a2b4 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -8,6 +8,7 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -146,14 +147,17 @@ static struct irq_domain *plic_irqdomain;
  * 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)
+static void plic_handle_irq(struct irq_desc *desc)
 {
 	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 	void __iomem *claim = plic_hart_offset(handler->ctxid) + CONTEXT_CLAIM;
 	irq_hw_number_t hwirq;
 
 	WARN_ON_ONCE(!handler->present);
 
+	chained_irq_enter(chip, desc);
+
 	csr_clear(sie, SIE_SEIE);
 	while ((hwirq = readl(claim))) {
 		int irq = irq_find_mapping(plic_irqdomain, hwirq);
@@ -166,6 +170,8 @@ static void plic_handle_irq(struct pt_regs *regs)
 		writel(hwirq, claim);
 	}
 	csr_set(sie, SIE_SEIE);
+
+	chained_irq_exit(chip, desc);
 }
 
 /*
@@ -183,7 +189,7 @@ static int plic_find_hart_id(struct device_node *node)
 }
 
 static int __init plic_init(struct device_node *node,
-		struct device_node *parent)
+			    struct device_node *parent)
 {
 	int error = 0, nr_handlers, nr_mapped = 0, i;
 	u32 nr_irqs;
@@ -218,7 +224,7 @@ static int __init plic_init(struct device_node *node,
 		struct of_phandle_args parent;
 		struct plic_handler *handler;
 		irq_hw_number_t hwirq;
-		int cpu;
+		int cpu, parent_irq;
 
 		if (of_irq_parse_one(node, i, &parent)) {
 			pr_err("failed to parse parent for context %d.\n", i);
@@ -229,6 +235,13 @@ static int __init plic_init(struct device_node *node,
 		if (parent.args[0] == -1)
 			continue;
 
+		if (irq_find_host(parent.np)) {
+			parent_irq = irq_of_parse_and_map(node, i);
+			if (parent_irq)
+				irq_set_chained_handler(parent_irq,
+							plic_handle_irq);
+		}
+
 		cpu = plic_find_hart_id(parent.np);
 		if (cpu < 0) {
 			pr_warn("failed to parse hart ID for context %d.\n", i);
@@ -248,7 +261,7 @@ static int __init plic_init(struct device_node *node,
 
 	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:
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index caf40ad0bbc6..ca7268a38cef 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -100,6 +100,7 @@ enum cpuhp_state {
 	CPUHP_AP_IRQ_ARMADA_XP_STARTING,
 	CPUHP_AP_IRQ_BCM2836_STARTING,
 	CPUHP_AP_IRQ_MIPS_GIC_STARTING,
+	CPUHP_AP_IRQ_RISCV_STARTING,
 	CPUHP_AP_ARM_MVEBU_COHERENCY,
 	CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
 	CPUHP_AP_PERF_X86_STARTING,
-- 
2.17.1

Re: [RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 06:15:13PM +0530, Anup Patel wrote:
> Few advantages of this new driver over previous one are:
> 1. It registers all local interrupts as per-CPU interrupts

Please explain why this is an advantage.

> 2. We can develop drivers for devices with per-CPU local interrupts
> without changing arch code or this driver

Please explain why this is a good thing and not just a workaround for
the fact that some moron decided they need non-standard interrupts
and should only be done as a last resort.

> 3. It allows local interrupt controller DT node under each CPU DT node
> as well as single system-wide DT node for local interrupt controller.

Please explain why this is can't happen right now.

Downsides are that it is a heck lot more code and introduces indirect
calls in the interrupt fast path.

So for now a clear NAK.

> The RISC-V INTC driver is compliant with RISC-V Hart-Level Interrupt
> Controller DT bindings located at:
> Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> 
> Signed-off-by: Anup Patel <anup@brainfault.org>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  arch/riscv/include/asm/irq.h      |  15 ++-
>  arch/riscv/kernel/irq.c           |  59 +----------
>  drivers/irqchip/Kconfig           |  15 ++-
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-riscv-intc.c  | 164 ++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-sifive-plic.c |  21 +++-
>  include/linux/cpuhotplug.h        |   1 +
>  7 files changed, 214 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/irqchip/irq-riscv-intc.c
> 
> diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
> index 93eb75eac4ff..fe503d71876a 100644
> --- a/arch/riscv/include/asm/irq.h
> +++ b/arch/riscv/include/asm/irq.h
> @@ -15,7 +15,20 @@
>  #ifndef _ASM_RISCV_IRQ_H
>  #define _ASM_RISCV_IRQ_H
>  
> -#define NR_IRQS         0
> +/*
> + * 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))
>  
>  void riscv_timer_interrupt(void);
>  
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index c51c9b402e87..46a311e5f0f6 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -7,69 +7,16 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/irqchip.h>
> -#include <linux/irqdomain.h>
> -
> -#include <asm/sbi.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)
>  {
> -	struct pt_regs *old_regs;
> -
> -	switch (regs->scause & ~INTERRUPT_CAUSE_FLAG) {
> -	case INTERRUPT_CAUSE_TIMER:
> -		old_regs = set_irq_regs(regs);
> -		irq_enter();
> -		riscv_timer_interrupt();
> -		irq_exit();
> -		set_irq_regs(old_regs);
> -		break;
> -#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.
> -		 */
> -		handle_IPI(regs);
> -		break;
> -#endif
> -	case INTERRUPT_CAUSE_EXTERNAL:
> -		old_regs = set_irq_regs(regs);
> -		irq_enter();
> +	if (handle_arch_irq)
>  		handle_arch_irq(regs);
> -		irq_exit();
> -		set_irq_regs(old_regs);
> -		break;
> -	default:
> -		panic("unexpected interrupt cause");
> -	}
> -}
> -
> -#ifdef CONFIG_SMP
> -static void smp_ipi_trigger_sbi(const struct cpumask *to_whom)
> -{
> -	sbi_send_ipi(cpumask_bits(to_whom));
>  }
> -#endif
>  
>  void __init init_IRQ(void)
>  {
>  	irqchip_init();
> -#ifdef CONFIG_SMP
> -	set_smp_ipi_trigger(smp_ipi_trigger_sbi);
> -#endif
> +	if (!handle_arch_irq)
> +		panic("No interrupt controller found.");
>  }
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 383e7b70221d..885e182586f4 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -371,7 +371,18 @@ config QCOM_PDC
>  	  Power Domain Controller driver to manage and configure wakeup
>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>  
> -endmenu
> +config RISCV_INTC
> +	bool "RISC-V Interrupt Controller"
> +	depends on RISCV
> +	default y
> +	help
> +	   This enables support for the local interrupt controller found in
> +	   standard RISC-V systems.  The local interrupt controller handles
> +	   timer interrupts, software interrupts, and hardware interrupts.
> +	   Without a local interrupt controller the system will be unable to
> +	   handle any interrupts, including those passed via the PLIC.
> +
> +	   If you don't know what to do here, say Y.
>  
>  config SIFIVE_PLIC
>  	bool "SiFive Platform-Level Interrupt Controller"
> @@ -384,3 +395,5 @@ config SIFIVE_PLIC
>  	   interrupt sources are subordinate to the PLIC.
>  
>  	   If you don't know what to do here, say Y.
> +
> +endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index fbd1ec8070ef..e638ee5c4452 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -87,4 +87,5 @@ 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_RISCV_INTC)		+= irq-riscv-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> new file mode 100644
> index 000000000000..c80502a1e12b
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017-2018 SiFive
> + * Copyright (C) 2018 Anup Patel
> + */
> +
> +#define pr_fmt(fmt) "riscv-intc: " fmt
> +#include <linux/atomic.h>
> +#include <linux/bits.h>
> +#include <linux/cpu.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/smp.h>
> +#include <asm/sbi.h>
> +
> +static struct irq_domain *intc_domain;
> +static atomic_t intc_init = ATOMIC_INIT(0);
> +
> +static void riscv_intc_irq(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs;
> +	unsigned long cause = regs->scause & ~INTERRUPT_CAUSE_FLAG;
> +
> +	if (unlikely(cause >= BITS_PER_LONG))
> +		panic("unexpected interrupt cause");
> +
> +	switch (cause) {
> +	case INTERRUPT_CAUSE_TIMER:
> +		old_regs = set_irq_regs(regs);
> +		irq_enter();
> +		riscv_timer_interrupt();
> +		irq_exit();
> +		set_irq_regs(old_regs);
> +		break;
> +#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.
> +		 */
> +		handle_IPI(regs);
> +		break;
> +#endif
> +	default:
> +		handle_domain_irq(intc_domain, cause, regs);
> +		break;
> +	}
> +}
> +
> +/*
> + * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
> + * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
> + * hart, these functions can only be called on the hart that corresponds to the
> + * IRQ chip.  They are only called internally to this module, so they BUG_ON if
> + * this condition is violated rather than attempting to handle the error by
> + * forwarding to the target hart, as that's already expected to have been done.
> + */
> +static void riscv_intc_irq_mask(struct irq_data *d)
> +{
> +	csr_clear(sie, 1 << (long)d->hwirq);
> +}
> +
> +static void riscv_intc_irq_unmask(struct irq_data *d)
> +{
> +	csr_set(sie, 1 << (long)d->hwirq);
> +}
> +
> +#ifdef CONFIG_SMP
> +static void riscv_intc_ipi_trigger(const struct cpumask *to_whom)
> +{
> +	sbi_send_ipi(cpumask_bits(to_whom));
> +}
> +
> +static int riscv_intc_cpu_starting(unsigned int cpu)
> +{
> +	csr_set(sie, 1 << INTERRUPT_CAUSE_SOFTWARE);
> +	return 0;
> +}
> +
> +static int riscv_intc_cpu_dying(unsigned int cpu)
> +{
> +	csr_clear(sie, 1 << INTERRUPT_CAUSE_SOFTWARE);
> +	return 0;
> +}
> +
> +static void riscv_intc_smp_init(void)
> +{
> +	csr_write(sie, 0);
> +	csr_write(sip, 0);
> +
> +	set_smp_ipi_trigger(riscv_intc_ipi_trigger);
> +
> +	cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING,
> +			  "irqchip/riscv/intc:starting",
> +			  riscv_intc_cpu_starting,
> +			  riscv_intc_cpu_dying);
> +
> +}
> +#else
> +static void riscv_intc_smp_init(void)
> +{
> +	csr_write(sie, 0);
> +	csr_write(sip, 0);
> +}
> +#endif
> +
> +static struct irq_chip riscv_intc_chip = {
> +	.name = "RISC-V INTC",
> +	.irq_mask = riscv_intc_irq_mask,
> +	.irq_unmask = riscv_intc_irq_unmask,
> +};
> +
> +static int riscv_intc_domain_map(struct irq_domain *d, unsigned int irq,
> +				 irq_hw_number_t hwirq)
> +{
> +	irq_set_percpu_devid(irq);
> +	irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data,
> +			    handle_percpu_devid_irq, NULL, NULL);
> +	irq_set_status_flags(irq, IRQ_NOAUTOEN);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops riscv_intc_domain_ops = {
> +	.map	= riscv_intc_domain_map,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +
> +static int __init riscv_intc_init(struct device_node *node,
> +				  struct device_node *parent)
> +{
> +	/*
> +	 * RISC-V device trees can have one INTC DT node under
> +	 * each CPU DT node so INTC init function will be called
> +	 * once for each INTC DT node. We only need to do INTC
> +	 * init once for boot CPU so we use atomic counter to
> +	 * achieve this.
> +	 */
> +	if (atomic_inc_return(&intc_init) > 1)
> +		return 0;
> +
> +	intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
> +					    &riscv_intc_domain_ops, NULL);
> +	if (!intc_domain)
> +		goto error_add_linear;
> +
> +	set_handle_irq(&riscv_intc_irq);
> +
> +	riscv_intc_smp_init();
> +
> +	pr_info("%lu local interrupts mapped\n", (long)BITS_PER_LONG);
> +
> +	return 0;
> +
> +error_add_linear:
> +	pr_warn("unable to add IRQ domain\n");
> +	return -ENXIO;
> +}
> +
> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 532e9d68c704..ab9614d5a2b4 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -8,6 +8,7 @@
>  #include <linux/io.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -146,14 +147,17 @@ static struct irq_domain *plic_irqdomain;
>   * 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)
> +static void plic_handle_irq(struct irq_desc *desc)
>  {
>  	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	void __iomem *claim = plic_hart_offset(handler->ctxid) + CONTEXT_CLAIM;
>  	irq_hw_number_t hwirq;
>  
>  	WARN_ON_ONCE(!handler->present);
>  
> +	chained_irq_enter(chip, desc);
> +
>  	csr_clear(sie, SIE_SEIE);
>  	while ((hwirq = readl(claim))) {
>  		int irq = irq_find_mapping(plic_irqdomain, hwirq);
> @@ -166,6 +170,8 @@ static void plic_handle_irq(struct pt_regs *regs)
>  		writel(hwirq, claim);
>  	}
>  	csr_set(sie, SIE_SEIE);
> +
> +	chained_irq_exit(chip, desc);
>  }
>  
>  /*
> @@ -183,7 +189,7 @@ static int plic_find_hart_id(struct device_node *node)
>  }
>  
>  static int __init plic_init(struct device_node *node,
> -		struct device_node *parent)
> +			    struct device_node *parent)
>  {
>  	int error = 0, nr_handlers, nr_mapped = 0, i;
>  	u32 nr_irqs;
> @@ -218,7 +224,7 @@ static int __init plic_init(struct device_node *node,
>  		struct of_phandle_args parent;
>  		struct plic_handler *handler;
>  		irq_hw_number_t hwirq;
> -		int cpu;
> +		int cpu, parent_irq;
>  
>  		if (of_irq_parse_one(node, i, &parent)) {
>  			pr_err("failed to parse parent for context %d.\n", i);
> @@ -229,6 +235,13 @@ static int __init plic_init(struct device_node *node,
>  		if (parent.args[0] == -1)
>  			continue;
>  
> +		if (irq_find_host(parent.np)) {
> +			parent_irq = irq_of_parse_and_map(node, i);
> +			if (parent_irq)
> +				irq_set_chained_handler(parent_irq,
> +							plic_handle_irq);
> +		}
> +
>  		cpu = plic_find_hart_id(parent.np);
>  		if (cpu < 0) {
>  			pr_warn("failed to parse hart ID for context %d.\n", i);
> @@ -248,7 +261,7 @@ static int __init plic_init(struct device_node *node,
>  
>  	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:
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index caf40ad0bbc6..ca7268a38cef 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -100,6 +100,7 @@ enum cpuhp_state {
>  	CPUHP_AP_IRQ_ARMADA_XP_STARTING,
>  	CPUHP_AP_IRQ_BCM2836_STARTING,
>  	CPUHP_AP_IRQ_MIPS_GIC_STARTING,
> +	CPUHP_AP_IRQ_RISCV_STARTING,
>  	CPUHP_AP_ARM_MVEBU_COHERENCY,
>  	CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
>  	CPUHP_AP_PERF_X86_STARTING,
> -- 
> 2.17.1
> 
---end quoted text---

Re: [RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Anup Patel]

On Wed, Sep 5, 2018 at 12:27 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Sep 04, 2018 at 06:15:13PM +0530, Anup Patel wrote:
>> Few advantages of this new driver over previous one are:
>> 1. It registers all local interrupts as per-CPU interrupts
>
> Please explain why this is an advantage.

Previously submitted driver, registered separate irq_domain for
each CPU and local IRQs were registered as regular IRQs to IRQ
subsystem.
(Refer, https://www.spinics.net/lists/devicetree/msg241230.html)

The previously submitted driver had following sort-comings:
1. Required separate interrupt-controller DT node for each CPU
2. Wasted lot of IRQ numbers because each CPU will has its
own IRQ domain
3. irq_enable()/irq_disable() had to explicitly use smp_call_function_single()
to disable given IRQ on all CPUs

Instead of above, the new driver (this patch) registers only single
irq_domain common for each CPU and local IRQs are registered
as per-CPU IRQs to IRQ subsystem. Due to this we only need one
DT node for local interrupt controller and if multiple DT nodes are
present then we ignore them using atomic counter. We use same
IRQ number for local interrupts for all CPUs. Further, irq_enable()/
irq_disable() of per-CPU interrupts is handled internally by Linux
IRQ subsystem.

>
>> 2. We can develop drivers for devices with per-CPU local interrupts
>> without changing arch code or this driver
>
> Please explain why this is a good thing and not just a workaround for
> the fact that some moron decided they need non-standard interrupts
> and should only be done as a last resort.

Let me give you few examples of per-CPU interrupts from ARM world:

1. CPU PMU IRQs: Lot of ARM SoCs have PMU IRQ of a CPU mapped
as PPI (i.e. Per-CPU IRQ). This means PMU events on ARM generate
per-CPU IRQs

2. GICv2/GICv3 maintenance IRQs: The virtualization extensions in
GICv2/GICv3 help hypervisor inject virtual IRQs. The list registers using
which virtual IRQs are injected is limited resource and GICv2/GICv3
provides per-CPU maintenance IRQ to manage list registers. It is quite
possible that RISC-V SOC vendors will come-up with PLIC++ which
has virtualization extension requiring per-CPU IRQs.

3, MSI to local IRQs; The GICv3 has separate module called ITS which
implements a HW MSI controller. The GICv3 ITS translates MSI writes
from PCI devices to per-CPU interrupts called LPIs. It is quite possible
that RISC-V SOC vendors will come-up with PLIC++ which allows
mapping MSI writes to local per-CPU IRQ.

Apart from above, it is possible to have a CPU interconnect which
report bus errors of a CPU as per-CPU IRQs.

If you still think above examples are moronic then I cannot help
improve your understanding of per-CPU IRQs.

>
>> 3. It allows local interrupt controller DT node under each CPU DT node
>> as well as single system-wide DT node for local interrupt controller.
>
> Please explain why this is can't happen right now.

Currently, we don't have any local interrupt controller driver so
DT nodes for local interrupt controller are ignored.

The advantage point3 (above) is in-comparison to previously
submitted driver for RISC-V local interrupt controller.
(Refer, https://www.spinics.net/lists/devicetree/msg241230.html)

>
> Downsides are that it is a heck lot more code and introduces indirect
> calls in the interrupt fast path.

Without this patch IRQ handling flow is:
RISC-V entry.S --> do_IRQ() --> plic_handle_irq()

With this patch IRQ handling flow is:
RISC-V entry.S --> do_IRQ() --> riscv_intc_irq() --> plic_handle_irq()

I have an optimisation lined-up where RISC-V entry.S can
directly call handle_arch_irq (just like Linux ARM64). With
this optimisation IRQ handling flow will be:
RISC-V entry.S --> riscv_intc_irq() --> plic_handle_irq()

As you can see it is not "lot more code". In return, we get
to use per-CPU IRQs via Linux IRQ subsystem.

>
> So for now a clear NAK.
>

Again, you NAKed it too early without letting me provide
explanation.

Regards,
Anup

Re: [RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Christoph Hellwig]

On Wed, Sep 05, 2018 at 11:39:01AM +0530, Anup Patel wrote:
> Previously submitted driver, registered separate irq_domain for
> each CPU and local IRQs were registered as regular IRQs to IRQ
> subsystem.
> (Refer, https://www.spinics.net/lists/devicetree/msg241230.html)

And we reject that driver approach for good reason and are now
doing the architectualy low-level irq handling in common code
without any need whatsover to duplicate information in the
privileged spec in DT. 

Re: [RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Anup Patel]

On Thu, Sep 6, 2018 at 12:28 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Sep 05, 2018 at 11:39:01AM +0530, Anup Patel wrote:
>> Previously submitted driver, registered separate irq_domain for
>> each CPU and local IRQs were registered as regular IRQs to IRQ
>> subsystem.
>> (Refer, https://www.spinics.net/lists/devicetree/msg241230.html)
>
> And we reject that driver approach for good reason and are now
> doing the architectualy low-level irq handling in common code
> without any need whatsover to duplicate information in the
> privileged spec in DT.

In other words, the whole idea of separate RISCV local interrupt
controller driver was dropped due duplicate information in privilege
spec DT ??

Anyway, I think we should certainly have RISCV local interrupt
controller driver to manage local IRQs using Linux IRQ
subsystem. This gives us future flexibility in having more
per-CPU IRQ without changing any arch/riscv code.

Based on ARM examples which I had provided, it is very
likely that we will see more per-CPU IRQs in future. Some of
these will be device IRQs and some will be CPU specific
per-CPU IRQs (such as bus error interrupts).

Regards,
Anup

Re: [RFC PATCH 4/5] irqchip: RISC-V Local Interrupt Controller Driver

[Christoph Hellwig]

On Thu, Sep 06, 2018 at 05:23:07PM +0530, Anup Patel wrote:
> > And we reject that driver approach for good reason and are now
> > doing the architectualy low-level irq handling in common code
> > without any need whatsover to duplicate information in the
> > privileged spec in DT.
> 
> In other words, the whole idea of separate RISCV local interrupt
> controller driver was dropped due duplicate information in privilege
> spec DT ??

No.  We came to the conflusion that a few registers on a cpu
that allow enabling/disabling local vs external vs timer intterupts
aren't worth writing an irqchip (or DT entries) for.

[RFC PATCH 5/5] clocksource: riscv_timer: Make timer interrupt as a per-CPU interrupt

[Anup Patel]

Instead of directly calling RISC-V timer interrupt handler from
RISC-V local interrupt conntroller driver, this patch implements
RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs
of Linux IRQ subsystem.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/include/asm/irq.h      |  2 -
 drivers/clocksource/riscv_timer.c | 76 +++++++++++++++++++++++++------
 drivers/irqchip/irq-riscv-intc.c  |  8 ----
 3 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index fe503d71876a..7ff3751e7816 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -30,8 +30,6 @@
  */
 #define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
 
-void riscv_timer_interrupt(void);
-
 #include <asm-generic/irq.h>
 
 #endif /* _ASM_RISCV_IRQ_H */
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 4e8b347e43e2..c7ac60a82754 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -3,11 +3,17 @@
  * Copyright (C) 2012 Regents of the University of California
  * Copyright (C) 2017 SiFive
  */
+#define pr_fmt(fmt) "riscv-timer: " fmt
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/cpu.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
 #include <asm/sbi.h>
 
 /*
@@ -31,6 +37,7 @@ static int riscv_clock_next_event(unsigned long delta,
 	return 0;
 }
 
+static unsigned int riscv_clock_event_irq;
 static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
 	.name			= "riscv_timer_clockevent",
 	.features		= CLOCK_EVT_FEAT_ONESHOT,
@@ -38,6 +45,11 @@ static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
 	.set_next_event		= riscv_clock_next_event,
 };
 
+static u64 riscv_sched_clock(void)
+{
+	return get_cycles64();
+}
+
 /*
  * It is guaranteed that all the timers across all the harts are synchronized
  * within one tick of each other, so while this could technically go
@@ -48,7 +60,7 @@ static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
 	return get_cycles64();
 }
 
-static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+static struct clocksource riscv_clocksource = {
 	.name		= "riscv_clocksource",
 	.rating		= 300,
 	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
@@ -61,45 +73,81 @@ 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);
+	ce->irq = riscv_clock_event_irq;
 	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);
 
-	csr_set(sie, SIE_STIE);
+	enable_percpu_irq(riscv_clock_event_irq, IRQ_TYPE_NONE);
 	return 0;
 }
 
 static int riscv_timer_dying_cpu(unsigned int cpu)
 {
-	csr_clear(sie, SIE_STIE);
+	disable_percpu_irq(riscv_clock_event_irq);
 	return 0;
 }
 
-/* called directly from the low-level interrupt handler */
-void riscv_timer_interrupt(void)
+static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
 
 	csr_clear(sie, SIE_STIE);
 	evdev->event_handler(evdev);
+
+	return IRQ_HANDLED;
 }
 
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
-	int cpu_id = riscv_of_processor_hart(n), error;
-	struct clocksource *cs;
-
-	if (cpu_id != smp_processor_id())
+	int error;
+	struct irq_domain *domain;
+	struct of_phandle_args oirq;
+
+	/*
+	 * Either we have one INTC DT node under each CPU DT node
+	 * or a single system wide INTC DT node. Irrespective to
+	 * number of INTC DT nodes, we only proceed if we are able
+	 * to find irq_domain of INTC.
+	 *
+	 * Once we have INTC irq_domain, we create mapping for timer
+	 * interrupt HWIRQ and request_percpu_irq() on it.
+	 */
+
+	if (riscv_clock_event_irq)
 		return 0;
 
-	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
-	clocksource_register_hz(cs, riscv_timebase);
+	oirq.np = n;
+	oirq.args_count = 1;
+	oirq.args[0] = INTERRUPT_CAUSE_TIMER;
+
+	domain = irq_find_host(oirq.np);
+	if (!domain)
+		return -ENODEV;
+
+	riscv_clock_event_irq = irq_create_of_mapping(&oirq);
+	if (!riscv_clock_event_irq)
+		return -ENODEV;
+
+	clocksource_register_hz(&riscv_clocksource, riscv_timebase);
+	sched_clock_register(riscv_sched_clock,
+			     BITS_PER_LONG, riscv_timebase);
+
+	error = request_percpu_irq(riscv_clock_event_irq,
+				   riscv_timer_interrupt,
+				   "riscv_timer", &riscv_clock_event);
+	if (error)
+		return error;
 
 	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);
+		pr_err("RISCV timer register failed error %d\n", error);
+
+	pr_info("running at %lu.%02luMHz frequency\n",
+		(unsigned long)riscv_timebase / 1000000,
+		(unsigned long)(riscv_timebase / 10000) % 100);
+
 	return error;
 }
 
-TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
+TIMER_OF_DECLARE(riscv_timer, "riscv,cpu-intc", riscv_timer_init_dt);
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index c80502a1e12b..f40827a27227 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -22,20 +22,12 @@ static atomic_t intc_init = ATOMIC_INIT(0);
 
 static void riscv_intc_irq(struct pt_regs *regs)
 {
-	struct pt_regs *old_regs;
 	unsigned long cause = regs->scause & ~INTERRUPT_CAUSE_FLAG;
 
 	if (unlikely(cause >= BITS_PER_LONG))
 		panic("unexpected interrupt cause");
 
 	switch (cause) {
-	case INTERRUPT_CAUSE_TIMER:
-		old_regs = set_irq_regs(regs);
-		irq_enter();
-		riscv_timer_interrupt();
-		irq_exit();
-		set_irq_regs(old_regs);
-		break;
 #ifdef CONFIG_SMP
 	case INTERRUPT_CAUSE_SOFTWARE:
 		/*
-- 
2.17.1

Re: [RFC PATCH 5/5] clocksource: riscv_timer: Make timer interrupt as a per-CPU interrupt

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 06:15:14PM +0530, Anup Patel wrote:
> Instead of directly calling RISC-V timer interrupt handler from
> RISC-V local interrupt conntroller driver, this patch implements
> RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs
> of Linux IRQ subsystem.

And the point of that is?  Except for introducing lots of pointless
code of course..

Re: [RFC PATCH 5/5] clocksource: riscv_timer: Make timer interrupt as a per-CPU interrupt

[Anup Patel]

On Wed, Sep 5, 2018 at 12:28 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Sep 04, 2018 at 06:15:14PM +0530, Anup Patel wrote:
>> Instead of directly calling RISC-V timer interrupt handler from
>> RISC-V local interrupt conntroller driver, this patch implements
>> RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs
>> of Linux IRQ subsystem.
>
> And the point of that is?  Except for introducing lots of pointless
> code of course..

Instead of short-circuiting timer interrupt from low-level IRQ handler, we use
Linux per-CPU IRQ handling for timer interrupt.

Without this patch, output of "cat /proc/interrupts" looks as follows:
           CPU0       CPU1       CPU2       CPU3
  8:          8         12         18          6  SiFive PLIC   8  virtio0
 10:          9         11          3          5  SiFive PLIC  10  ttyS0

With this patchset, output of "cat /proc/interrupts" looks as follows:
           CPU0       CPU1       CPU2       CPU3
  5:        995       1012        997       1015  RISC-V INTC   5 Edge
     riscv_timer
  8:         23          6         10          7  SiFive PLIC   8 Edge
     virtio0
 10:          9         10          5          4  SiFive PLIC  10 Edge
     ttyS0

Regards,
Anup