OpenRISC SMP Support
Stafford Horne
maintainercover OpenRISC SMP Support
Hello,
This series adds SMP support for OpenRISC. The OpenRISC multicore platform and SMP Linux support is based on the work that Stefan Kristiansson did around 2012. The platform was implemented in Verilog and run on FPGAs. I have been working to upstream this work. I have additionally tested this on QEMU, which I patched for OpenRISC multicore support, as well as FPGA.
I have documented the architecture in the OpenRISC 1.2 specification proposal available here:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
The QEMU patches are still under review but are available here for testers and anyone interested:
https://github.com/stffrdhrn/qemu.git openrisc-smp-v1
This series contains a bit of a mix of patches to get everything working.
o First the "use shadow registers" and "define CPU_BIG_ENDIAN as true" get the architecture ready for SMP.
o The "add 1 and 2 byte cmpxchg support" and "use qspinlocks and qrwlocks" add the SMP locking infrastructure as needed. Using the qspinlocks and qrwlocks as suggested by Peter Z while reviewing the original spinlocks implementation which I posted to the list a few months back [0].
o The "support for ompic" adds a new irqchip device which is used for IPI communication to support SMP. (Perhaps this patch should go via another route but included here for completeness)
o The "initial SMP support" adds smp.c and makes changes to all of the necessary data-structures to be per-cpu.
o The remaining patches are bug fixes and debug helpers which I wanted to keep separate from the "initial SMP support" in order to allow them to be reviewed on their own. This includes:
- add cacheflush support to fix icache aliasing
- fix initial preempt state for secondary cpu tasks
- sleep instead of spin on secondary wait
- support framepointers and STACKTRACE_SUPPORT
- enable LOCKDEP_SUPPORT and irqflags tracing
- timer sync: Add tick timer sync logic
[0] https://lkml.org/lkml/2017/2/21/659
-Stafford
Jan Henrik Weinstock (1):
openrisc: add cacheflush support to fix icache aliasing
Stafford Horne (8):
openrisc: define CPU_BIG_ENDIAN as true
openrisc: add 1 and 2 byte cmpxchg support
openrisc: use qspinlocks and qrwlocks
openrisc: fix initial preempt state for secondary cpu tasks
openrisc: sleep instead of spin on secondary wait
openrisc: support framepointers and STACKTRACE_SUPPORT
openrisc: enable LOCKDEP_SUPPORT and irqflags tracing
openrisc: add tick timer multicore sync logic
Stefan Kristiansson (4):
openrisc: use shadow registers to save regs on exception
irqchip: add initial support for ompic
openrisc: initial SMP support
openrisc: add simple_smp dts and defconfig for simulators
.../bindings/interrupt-controller/ompic.txt | 22 ++ arch/openrisc/Kconfig | 51 +++- arch/openrisc/boot/dts/simple_smp.dts | 58 +++++ arch/openrisc/configs/simple_smp_defconfig | 66 ++++++ arch/openrisc/include/asm/Kbuild | 5 +- arch/openrisc/include/asm/cacheflush.h | 96 ++++++++ arch/openrisc/include/asm/cmpxchg.h | 147 +++++++++--- arch/openrisc/include/asm/cpuinfo.h | 5 +- arch/openrisc/include/asm/mmu_context.h | 2 +- arch/openrisc/include/asm/or1k-timer.h | 27 +++ arch/openrisc/include/asm/pgtable.h | 18 +- arch/openrisc/include/asm/serial.h | 2 +- arch/openrisc/include/asm/smp.h | 26 +++ arch/openrisc/include/asm/spinlock.h | 12 +- arch/openrisc/include/asm/spinlock_types.h | 7 + arch/openrisc/include/asm/spr_defs.h | 14 ++ arch/openrisc/include/asm/thread_info.h | 2 +- arch/openrisc/include/asm/tlbflush.h | 25 +- arch/openrisc/include/asm/unwinder.h | 20 ++ arch/openrisc/kernel/Makefile | 4 +- arch/openrisc/kernel/dma.c | 14 +- arch/openrisc/kernel/entry.S | 74 +++++- arch/openrisc/kernel/head.S | 232 +++++++++++++++--- arch/openrisc/kernel/setup.c | 155 +++++++----- arch/openrisc/kernel/smp.c | 260 +++++++++++++++++++++ arch/openrisc/kernel/stacktrace.c | 86 +++++++ arch/openrisc/kernel/sync-timer.c | 120 ++++++++++ arch/openrisc/kernel/time.c | 66 ++++-- arch/openrisc/kernel/traps.c | 54 +---- arch/openrisc/kernel/unwinder.c | 105 +++++++++ arch/openrisc/lib/delay.c | 2 +- arch/openrisc/mm/Makefile | 2 +- arch/openrisc/mm/cache.c | 61 +++++ arch/openrisc/mm/fault.c | 4 +- arch/openrisc/mm/init.c | 2 +- arch/openrisc/mm/tlb.c | 16 +- drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 ++++++++++ 39 files changed, 1754 insertions(+), 230 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 arch/openrisc/boot/dts/simple_smp.dts create mode 100644 arch/openrisc/configs/simple_smp_defconfig create mode 100644 arch/openrisc/include/asm/cacheflush.h create mode 100644 arch/openrisc/include/asm/or1k-timer.h create mode 100644 arch/openrisc/include/asm/smp.h create mode 100644 arch/openrisc/include/asm/spinlock_types.h create mode 100644 arch/openrisc/include/asm/unwinder.h create mode 100644 arch/openrisc/kernel/smp.c create mode 100644 arch/openrisc/kernel/stacktrace.c create mode 100644 arch/openrisc/kernel/sync-timer.c create mode 100644 arch/openrisc/kernel/unwinder.c create mode 100644 arch/openrisc/mm/cache.c create mode 100644 drivers/irqchip/irq-ompic.c
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 1/13 openrisc: use shadow registers to save regs on exception
From: Stefan Kristiansson <[email protected]>
Previously, the area between 0x0-0x100 have been used as a "scratch" memory area to temporarily store regs during exception entry.
In a multi-core environment, this will not work.
This change is to use shadow registers for nested context.
Currently only the "critical" temp load/stores are covered, the EMERGENCY_PRINT ones are left as is (when they are used, it's game over anyway), they need to be handled as well in the future.
--- arch/openrisc/Kconfig | 10 +++++ arch/openrisc/kernel/head.S | 95 ++++++++++++++++++++++++++++++++------------- 2 files changed, 79 insertions(+), 26 deletions(-)
arch/openrisc/Kconfig +10 -0
@@ -121,6 +121,16 @@ config OPENRISC_NO_SPR_SR_DSX Say N here if you know that your OpenRISC processor has SPR_SR_DSX bit implemented. Say Y if you are unsure. +config OPENRISC_HAVE_SHADOW_GPRS + bool "Support for shadow gpr files" + help + Say Y here if your OpenRISC processor features shadowed + register files. They will in such case be used as a + scratch reg storage on exception entry. + + On SMP systems, this feature is mandatory. + On a unicore system it's safe to say N here if you are unsure. + config CMDLINE string "Default kernel command string" default ""
arch/openrisc/kernel/head.S +69 -26
159 lines not on this page · raw message
Signature · 1 line
2.13.5
Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 125
Quoted 8 lines · Stafford Horne
arch/openrisc/Kconfig +2
-0@@ -121,6 +121,16 @@ config OPENRISC_NO_SPR_SR_DSX Say N here if you know that your OpenRISC processor has SPR_SR_DSX bit implemented. Say Y if you are unsure. +config OPENRISC_HAVE_SHADOW_GPRS + bool "Support for shadow gpr files"
default y if SMP?
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 133
Quoted 8 lines
arch/openrisc/Kconfig +8
-0+ help + Say Y here if your OpenRISC processor features shadowed + register files. They will in such case be used as a + scratch reg storage on exception entry. + + On SMP systems, this feature is mandatory. + On a unicore system it's safe to say N here if you are unsure. +
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception
Quoted 11 lines · Geert Uytterhoeven
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 125
Quoted 8 lines · Stafford Horne
arch/openrisc/Kconfig +2
-0@@ -121,6 +121,16 @@ config OPENRISC_NO_SPR_SR_DSX Say N here if you know that your OpenRISC processor has SPR_SR_DSX bit implemented. Say Y if you are unsure. +config OPENRISC_HAVE_SHADOW_GPRS + bool "Support for shadow gpr files"default y if SMP?
Oops, that's not sufficient.
bool "Support for shadow gpr files" if !SMP
default y if SMP
Quoted 8 lines
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 133
Quoted 8 lines
arch/openrisc/Kconfig +8
-0+ help + Say Y here if your OpenRISC processor features shadowed + register files. They will in such case be used as a + scratch reg storage on exception entry. + + On SMP systems, this feature is mandatory. + On a unicore system it's safe to say N here if you are unsure. +
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Stafford Horne
maintainerRe: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception
Quoted 14 lines · Geert Uytterhoeven
Quoted 11 lines · Geert Uytterhoeven
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 125
Quoted 8 lines · Stafford Horne
arch/openrisc/Kconfig +2
-0@@ -121,6 +121,16 @@ config OPENRISC_NO_SPR_SR_DSX Say N here if you know that your OpenRISC processor has SPR_SR_DSX bit implemented. Say Y if you are unsure. +config OPENRISC_HAVE_SHADOW_GPRS + bool "Support for shadow gpr files"default y if SMP?
Quoted 4 lines
Oops, that's not sufficient.
bool "Support for shadow gpr files" if !SMP
default y if SMP
Thanks yes, the help text says its mandatory. That should be forced as you showed.
-Stafford
Quoted 19 lines
Quoted 8 lines
arch/openrisc/Kconfig config OPENRISC_NO_SPR_SR_DSX · line 133
Quoted 8 lines
arch/openrisc/Kconfig +8
-0+ help + Say Y here if your OpenRISC processor features shadowed + register files. They will in such case be used as a + scratch reg storage on exception entry. + + On SMP systems, this feature is mandatory. + On a unicore system it's safe to say N here if you are unsure. +Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.orgIn personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Stafford Horne
maintainerPATCH 2/13 openrisc: define CPU_BIG_ENDIAN as true
In reply to Stafford HorneWhile working on SMP for OpenRISC I found this is needed for qrwlocks to work correctly. OpenRISC is big endian so this should have been here already.
--- arch/openrisc/Kconfig | 3 +++ 1 file changed, 3 insertions(+)
arch/openrisc/Kconfig +3 -0
@@ -29,6 +29,9 @@ config OPENRISC select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 select NO_BOOTMEM +config CPU_BIG_ENDIAN + def_bool y + config MMU def_bool y
Signature · 1 line
2.13.5
Re: [OpenRISC] [PATCH 02/13] openrisc: define CPU_BIG_ENDIAN as true
Quoted 14 lines · Stafford Horne
While working on SMP for OpenRISC I found this is needed for qrwlocks to work correctly. OpenRISC is big endian so this should have been here already.
Signed-off-by Stafford Horne <[email protected]>--- arch/openrisc/Kconfig | 3 +++ 1 file changed, 3 insertions(+)arch/openrisc/Kconfig +0
-0
arch/openrisc/Kconfig config OPENRISC · line 34
Quoted 6 lines
arch/openrisc/Kconfig +3
-0@@ -29,6 +29,9 @@ config OPENRISC select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 select NO_BOOTMEM +config CPU_BIG_ENDIAN + def_bool y +
This is already in Andrew's tree and linux-next, as part of commit 410ae809fee6c792 ("arch: define CPU_BIG_ENDIAN for all fixed big endian archs").
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Stafford Horne
maintainerRe: [OpenRISC] [PATCH 02/13] openrisc: define CPU_BIG_ENDIAN as true
Quoted 18 lines · Geert Uytterhoeven
Quoted 17 lines · Stafford Horne
While working on SMP for OpenRISC I found this is needed for qrwlocks to work correctly. OpenRISC is big endian so this should have been here already.
Signed-off-by Stafford Horne <[email protected]>--- arch/openrisc/Kconfig | 3 +++ 1 file changed, 3 insertions(+)arch/openrisc/Kconfig +0
-0@@ -29,6 +29,9 @@ config OPENRISC select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 select NO_BOOTMEM
Quoted 7 lines
arch/openrisc/Kconfig config OPENRISC · line 34
Quoted 3 lines
arch/openrisc/Kconfig +3
-0+config CPU_BIG_ENDIAN + def_bool y +This is already in Andrew's tree and linux-next, as part of commit
410ae809fee6c792("arch: defineCPU_BIG_ENDIANfor all fixed big endian archs").
Thank you, Yes I am cc'd on those patches. I had added this patch myself some time before 10ae809fee6c792 ("arch: define CPU_BIG_ENDIAN for all fixed big endian archs") appeared.
Once that gets merged I will rebase and this will disappear.
-Stafford
Quoted 10 lines
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.orgIn personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Stafford Horne
maintainerPATCH 3/13 openrisc: add 1 and 2 byte cmpxchg support
In reply to Stafford HorneOpenRISC only supports hardware instructions that perform 4 byte atomic operations. For enabling qrwlocks for upcoming SMP support 1 and 2 byte implementations are needed. To do this we leverage the 4 byte atomic operations and shift/mask the 1 and 2 byte areas as needed.
This heavily borrows ideas and routines from sh and mips, which do something similar.
--- arch/openrisc/include/asm/cmpxchg.h | 147 ++++++++++++++++++++++++++++-------- 1 file changed, 115 insertions(+), 32 deletions(-)
arch/openrisc/include/asm/cmpxchg.h +115 -32
@@ -1,32 +1,29 @@/* +* 1,2 and 4 byte cmpxchg and xchg implementations for OpenRISC. +* * Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* Copyright (C) 2017 Stafford Horne <[email protected]> * * This file is licensed under the terms of the GNU General Public License * version 2. This program is licensed "as is" without any warranty of any * kind, whether express or implied. +* +* Note: +* The portable implementations of 1 and 2 byte xchg and cmpxchg using a 4 +* byte cmpxchg is sourced heavily from the sh and mips implementations. */ #ifndef __ASM_OPENRISC_CMPXCHG_H#define __ASM_OPENRISC_CMPXCHG_H#include <linux/types.h> - -/* -* This function doesn't exist, so you'll get a linker error -* if something tries to do an invalid cmpxchg(). -*/ -extern void __cmpxchg_called_with_bad_pointer (void ); +#include <linux/bitops.h> #define __HAVE_ARCH_CMPXCHG 1 -static inline unsigned long -__cmpxchg (volatile void *ptr,unsigned long old,unsigned long new,int size) +static inline unsigned long cmpxchg_u32 (volatile void *ptr, +unsigned long old,unsigned long new) { -if (size !=4 ) { -__cmpxchg_called_with_bad_pointer (); -return old; - } - __asm__ __volatile__("1: l.lwa %0, 0(%1) \n" " l.sfeq %0, %2 \n"
@@ -43,6 +40,97 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new, int size)return old; } +static inline unsigned long xchg_u32 (volatile void *ptr, +unsigned long val) +{ + __asm__ __volatile__( +"1: l.lwa %0, 0(%1) \n" +" l.swa 0(%1), %2 \n" +" l.bnf 1b \n" +" l.nop \n" + :"=&r" (val) + :"r" (ptr),"r" (val) + :"cc" ,"memory" ); + +return val; +} + +static inline u32 cmpxchg_small (volatile void *ptr,u32 old,u32 new, +int size) +{ +int off = (unsigned long )ptr %sizeof (u32); +volatile u32 *p = ptr - off; +#ifdef __BIG_ENDIAN +int bitoff = (sizeof (u32) - size - off) *BITS_PER_BYTE ; +#else +int bitoff = off *BITS_PER_BYTE ; +#endif +u32 bitmask = ((0x1 << size *BITS_PER_BYTE ) -1 ) << bitoff; +u32 load32, old32, new32; +u32 ret; + + load32 =READ_ONCE (*p); + +while (true) { + ret = (load32 & bitmask) >> bitoff; +if (old != ret) +return ret; + + old32 = (load32 & ~bitmask) | (old << bitoff); + new32 = (load32 & ~bitmask) | (new << bitoff); + +/* Do 32 bit cmpxchg */ + load32 =cmpxchg_u32 (p, old32, new32); +if (load32 == old32) +return old; + } +} + +/* xchg */ + +static inline u32 xchg_small (volatile void *ptr,u32 x,int size) +{ +int off = (unsigned long )ptr %sizeof (u32); +volatile u32 *p = ptr - off; +#ifdef __BIG_ENDIAN +int bitoff = (sizeof (u32) - size - off) *BITS_PER_BYTE ; +#else +int bitoff = off *BITS_PER_BYTE ; +#endif +u32 bitmask = ((0x1 << size *BITS_PER_BYTE ) -1 ) << bitoff; +u32 oldv, newv; +u32 ret; + +do { + oldv =READ_ONCE (*p); + ret = (oldv & bitmask) >> bitoff; + newv = (oldv & ~bitmask) | (x << bitoff); + }while (cmpxchg_u32 (p, oldv, newv) != oldv); + +return ret; +} + +/* +* This function doesn't exist, so you'll get a linker error +* if something tries to do an invalid cmpxchg(). +*/ +extern unsigned long __cmpxchg_called_with_bad_pointer(void) + __compiletime_error("Bad argument size for cmpxchg"); + +static inline unsigned long __cmpxchg (volatile void *ptr,unsigned long old, +unsigned long new,int size) +{ +switch (size) { +case 1 : +case 2 : +return cmpxchg_small (ptr, old, new, size); +case 4 : +return cmpxchg_u32 (ptr, old, new); +default : +return __cmpxchg_called_with_bad_pointer (); + } +} +#define cmpxchg (ptr, o, n) \ ({ \ (__typeof__(*(ptr))) __cmpxchg((ptr), \
@@ -55,32 +143,27 @@ __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new, int size)* This function doesn't exist, so you'll get a linker error if * something tries to do an invalidly-sized xchg(). */ -extern void __xchg_called_with_bad_pointer (void ); +extern unsigned long __xchg_called_with_bad_pointer(void) + __compiletime_error("Bad argument size for xchg"); -static inline unsigned long __xchg (unsigned long val,volatile void *ptr, -int size) +static inline unsigned long __xchg (volatile void *ptr,unsigned long with, +int size) { -if (size !=4 ) { -__xchg_called_with_bad_pointer (); -return val; +switch (size) { +case 1 : +case 2 : +return xchg_small (ptr, with, size); +case 4 : +return xchg_u32 (ptr, with); +default : +return __xchg_called_with_bad_pointer (); } - - __asm__ __volatile__( -"1: l.lwa %0, 0(%1) \n" -" l.swa 0(%1), %2 \n" -" l.bnf 1b \n" -" l.nop \n" - :"=&r" (val) - :"r" (ptr),"r" (val) - :"cc" ,"memory" ); - -return val; }#define xchg (ptr, with) \ ({ \ - (__typeof__(*(ptr))) __xchg((unsigned long)(with), \ - (ptr), \ + (__typeof__(*(ptr))) __xchg((ptr), \ + (unsigned long)(with), \ sizeof(*(ptr))); \ })
Signature · 1 line
2.13.5
Peter Zijlstra
maintainerRe: [PATCH 03/13] openrisc: add 1 and 2 byte cmpxchg support
Quoted 7 lines · Stafford Horne
OpenRISC only supports hardware instructions that perform 4 byte atomic operations. For enabling qrwlocks for upcoming SMP support 1 and 2 byte implementations are needed. To do this we leverage the 4 byte atomic operations and shift/mask the 1 and 2 byte areas as needed.
This heavily borrows ideas and routines from sh and mips, which do something similar.
Is there value in lifting them into something common?
Stafford Horne
maintainerRe: [PATCH 03/13] openrisc: add 1 and 2 byte cmpxchg support
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines · Stafford Horne
OpenRISC only supports hardware instructions that perform 4 byte atomic operations. For enabling qrwlocks for upcoming SMP support 1 and 2 byte implementations are needed. To do this we leverage the 4 byte atomic operations and shift/mask the 1 and 2 byte areas as needed.
This heavily borrows ideas and routines from sh and mips, which do something similar.
Is there value in lifting them into something common?
Actually, I was planning on that as a followup patch series.
Also, the timer sync patch [13/13] could me made common which is on my todo list.
Stafford Horne
maintainerPATCH 4/13 openrisc: use qspinlocks and qrwlocks
In reply to Stafford HorneEnable OpenRISC to use qspinlocks and qrwlocks for upcoming SMP support.
--- arch/openrisc/Kconfig | 2 ++ arch/openrisc/include/asm/Kbuild | 4 ++++ arch/openrisc/include/asm/spinlock.h | 12 +++++++++++- arch/openrisc/include/asm/spinlock_types.h | 7 +++++++ 4 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 arch/openrisc/include/asm/spinlock_types.h
arch/openrisc/Kconfig +2 -0
@@ -28,6 +28,8 @@ config OPENRISC select OR1K_PIC select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 select NO_BOOTMEM + select ARCH_USE_QUEUED_SPINLOCKS + select ARCH_USE_QUEUED_RWLOCKS config CPU_BIG_ENDIAN def_bool y
arch/openrisc/include/asm/Kbuild +4 -0
@@ -28,6 +28,10 @@ generic-y += module.h generic-y += pci.h generic-y += percpu.h generic-y += preempt.h +generic-y += qspinlock_types.h +generic-y += qspinlock.h +generic-y += qrwlock_types.h +generic-y += qrwlock.h generic-y += sections.h generic-y += segment.h generic-y += string.h
arch/openrisc/include/asm/spinlock.h +11 -1
@@ -19,6 +19,16 @@#ifndef __ASM_OPENRISC_SPINLOCK_H#define __ASM_OPENRISC_SPINLOCK_H -#error "or32 doesn't do SMP yet" +#include <asm/qspinlock.h> + +#include <asm/qrwlock.h> + +#define arch_read_lock_flags (lock, flags) arch_read_lock(lock) +#define arch_write_lock_flags (lock, flags) arch_write_lock(lock) + +#define arch_spin_relax (lock) cpu_relax() +#define arch_read_relax (lock) cpu_relax() +#define arch_write_relax (lock) cpu_relax() +#endif
arch/openrisc/include/asm/spinlock_types.h new file +7 -0
@@ -0,0 +1,7 @@ +#ifndef _ASM_OPENRISC_SPINLOCK_TYPES_H +#define _ASM_OPENRISC_SPINLOCK_TYPES_H + +#include <asm/qspinlock_types.h> +#include <asm/qrwlock_types.h> + +#endif /* _ASM_OPENRISC_SPINLOCK_TYPES_H */
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 5/13 irqchip: add initial support for ompic
In reply to Stafford HorneFrom: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.c
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +22 -0
@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic" +- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- interrupts : Specifies the interrupt line to which the ompic is wired. + +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};
arch/openrisc/Kconfig +1 -0
@@ -30,6 +30,7 @@ config OPENRISC select NO_BOOTMEM select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS + select OMPIC if SMP config CPU_BIG_ENDIAN def_bool y
drivers/irqchip/Kconfig +4 -0
@@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP select SPARSE_IRQ default y +config OMPIC + bool + select IRQ_DOMAIN + config OR1K_PIC bool select IRQ_DOMAIN
drivers/irqchip/Makefile +1 -0
@@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o +obj-$(CONFIG_OMPIC) += irq-ompic.o obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.o
drivers/irqchip/irq-ompic.c new file +117 -0
117 lines not on this page · raw message
Signature · 6 lines
2.13.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 74 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +22
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic" +- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- interrupts : Specifies the interrupt line to which the ompic is wired. + +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};arch/openrisc/Kconfig +1
-0@@ -30,6 +30,7 @@ config OPENRISC select NO_BOOTMEM select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS + select OMPIC if SMP config CPU_BIG_ENDIAN def_bool ydrivers/irqchip/Kconfig +0
-0
drivers/irqchip/Kconfig config CLPS711X_IRQCHIP · line 150
Quoted 6 lines
drivers/irqchip/Kconfig +3
-0@@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP select SPARSE_IRQ default y +config OMPIC + bool + select IRQ_DOMAIN
Why do you need to select IRQ_DOMAIN? This driver doesn't seem to use any...
Quoted 29 lines
drivers/irqchip/Kconfig +1
-0+ config OR1K_PIC bool select IRQ_DOMAINdrivers/irqchip/Makefile +1
-0@@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o +obj-$(CONFIG_OMPIC) += irq-ompic.o obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.odrivers/irqchip/irq-ompic.c new file +7
-0@@ -0,0 +1,117 @@ +/* +* Open Multi-Processor Interrupt Controller driver +* +* Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any
drivers/irqchip/irq-ompic.c line 17
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+* kind, whether express or implied. +*/ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/smp.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/irqchip/chained_irq.h>
Don't think you need this.
drivers/irqchip/irq-ompic.c line 18
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#include <linux/delay.h>
Nor this.
drivers/irqchip/irq-ompic.c line 24
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+ +#include <linux/irqchip.h> + +#define OMPIC_IPI_BASE 0x0 +#define OMPIC_IPI_CTRL (cpu) (OMPIC_IPI_BASE + 0x0 + (cpu)*8) +#define OMPIC_IPI_STAT (cpu) (OMPIC_IPI_BASE + 0x4 + (cpu)*8)
In the DT binding you say that "size can be arbitrary based on the number of cores the controller has been configured to handle, typically 8 bytes per core". Here, this is 8 bytes, always, which is not exactly the same. What is the architectural value, if any? If there is none, then the per-core size should either come from DT or some other mean (register?).
drivers/irqchip/irq-ompic.c line 36
Quoted 12 lines
drivers/irqchip/irq-ompic.c new file +12
-0+ +#define OMPIC_IPI_CTRL_IRQ_ACK (1 << 31) +#define OMPIC_IPI_CTRL_IRQ_GEN (1 << 30) +#define OMPIC_IPI_CTRL_DST (cpu) (((cpu) & 0x3fff) << 16) + +#define OMPIC_IPI_STAT_IRQ_PENDING (1 << 30) + +#define OMPIC_IPI_DATA (x) ((x) & 0xffff) + +static struct { +unsigned long ops; +} ipi_data[NR_CPUS ];
In general, a per cpu data structure is better expressed as a percpu data structure (yes, I'm in a funny mood this morning). Otherwise, you're going to thrash more than just the receiver and the sender, but also all the other CPUs that have their data in the same cache line.
Quoted 4 lines
drivers/irqchip/irq-ompic.c new file +4
-0+ +static void __iomem *ompic_base; + +static inline u32 ompic_readreg (void __iomem *base,loff_t offset)
drivers/irqchip/irq-ompic.c line 50
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+{ +return ioread32be (base + offset); +} + +static void ompic_writereg (void __iomem *base,loff_t offset,u32 data) +{ +iowrite32be (data, base + offset); +} + +#ifdef CONFIG_SMP
This code is only selected when CONFIG_SMP=y.
drivers/irqchip/irq-ompic.c line 52
Quoted 2 lines
drivers/irqchip/irq-ompic.c new file +2
-0+void ompic_raise_softirq (const struct cpumask *mask,unsigned int irq) +{
What is "irq" here? How is it guaranteed to fit in an unsigned long?
drivers/irqchip/irq-ompic.c line 62
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+unsigned int dst_cpu; +unsigned int src_cpu =smp_processor_id (); + +for_each_cpu (dst_cpu, mask) { +set_bit (irq, &ipi_data[dst_cpu].ops); + +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (src_cpu), +OMPIC_IPI_CTRL_IRQ_GEN | +OMPIC_IPI_CTRL_DST (dst_cpu) | +OMPIC_IPI_DATA (1 ));
What guarantees that the action of set_bit can be observed by the target CPU? set-bit gives you atomicity, but no barrier.
drivers/irqchip/irq-ompic.c line 74
Quoted 12 lines
drivers/irqchip/irq-ompic.c new file +12
-0+ } +} +#endif + +irqreturn_t ompic_ipi_handler (int irq,void *dev_id) +{ +unsigned int cpu =smp_processor_id (); +unsigned long *pending_ops = &ipi_data[cpu].ops; +unsigned long ops; + +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (cpu),OMPIC_IPI_CTRL_IRQ_ACK ); +while ((ops =xchg (pending_ops,0 )) !=0 ) {
Barrier again?
drivers/irqchip/irq-ompic.c line 80
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+ do { + unsigned long ipi; + + ipi = ops & -ops; + ops &= ~ipi; + ipi = __ffs(ipi);
This feels pointlessly convoluted. Is there any reason why you can't write it as:
ipi = __ffs(ops); ops &= ~(1UL << ipi);
which feels like a much more common idiom?
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +handle_IPI (ipi); + }while (ops); + } +
drivers/irqchip/irq-ompic.c line 95
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+return IRQ_HANDLED ; +} + +static struct irqaction ompi_ipi_irqaction = { + .handler = ompic_ipi_handler, + .flags =IRQF_PERCPU , + .name ="ompic_ipi" , +}; + +#ifdef CONFIG_OF
This code is useless if you don't have CONFIG_OF. So either you make the driver depend on CONFIG_OF, or you actively select it. And given that CONFIG_OPENRISC already selects CONFIG_OF, you can happily get rid of all the #ifdefery here.
drivers/irqchip/irq-ompic.c line 101
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+int __init ompic_of_init (struct device_node *node,struct device_node *parent) +{ +int irq; + +if (WARN_ON (!node)) +return -ENODEV ;
How do you end-up here if node == NULL?
drivers/irqchip/irq-ompic.c line 103
Quoted 2 lines
drivers/irqchip/irq-ompic.c new file +2
-0+ +memset (ipi_data,0 ,sizeof (ipi_data));
The kernel should do this for you already.
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +4
-0+ + ompic_base =of_iomap (node,0 ); + + irq =irq_of_parse_and_map (node,0 );
drivers/irqchip/irq-ompic.c line 117
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+setup_irq (irq, &ompi_ipi_irqaction); + +#ifdef CONFIG_SMP +set_smp_cross_call (ompic_raise_softirq); +#endif + + return 0; +} +IRQCHIP_DECLARE (ompic,"ompic" , ompic_of_init); +#endif
Thanks,
M.
Signature · 5 lines
Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Stafford Horne
maintainerRe: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 78 lines · Marc Zyngier
Quoted 77 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +22
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic" +- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- interrupts : Specifies the interrupt line to which the ompic is wired. + +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};arch/openrisc/Kconfig +1
-0@@ -30,6 +30,7 @@ config OPENRISC select NO_BOOTMEM select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS + select OMPIC if SMP config CPU_BIG_ENDIAN def_bool ydrivers/irqchip/Kconfig +0
-0@@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP select SPARSE_IRQ default y
Quoted 5 lines
drivers/irqchip/Kconfig config CLPS711X_IRQCHIP · line 150
Quoted 3 lines
drivers/irqchip/Kconfig +3
-0+config OMPIC + bool + select IRQ_DOMAINWhy do you need to select
IRQ_DOMAIN? This driver doesn't seem to use any...
Right, I will look to remove that.
Quoted 42 lines
Quoted 29 lines
drivers/irqchip/Kconfig +1
-0+ config OR1K_PIC bool select IRQ_DOMAINdrivers/irqchip/Makefile +1
-0@@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o +obj-$(CONFIG_OMPIC) += irq-ompic.o obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.odrivers/irqchip/irq-ompic.c new file +7
-0@@ -0,0 +1,117 @@ +/* +* Open Multi-Processor Interrupt Controller driver +* +* Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any drivers/irqchip/irq-ompic.c line 17
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+* kind, whether express or implied. +*/ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/smp.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/irqchip/chained_irq.h> Don't think you need this.
Quoted 3 lines
drivers/irqchip/irq-ompic.c line 18
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#include <linux/delay.h> Nor this.
OK on both.
Quoted 5 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +#include <linux/irqchip.h> + +#define OMPIC_IPI_BASE 0x0 +#define OMPIC_IPI_CTRL (cpu) (OMPIC_IPI_BASE + 0x0 + (cpu)*8)
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 24
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#define OMPIC_IPI_STAT (cpu) (OMPIC_IPI_BASE + 0x4 + (cpu)*8)In the DT binding you say that "size can be arbitrary based on the number of cores the controller has been configured to handle, typically 8 bytes per core". Here, this is 8 bytes, always, which is not exactly the same. What is the architectural value, if any? If there is none, then the per-core size should either come from DT or some other mean (register?).
I mean the address space is 8 bytes x number-of-cores. Thats what I meant by arbitrary, I guess its better for me to be explicit. There is no register that we can check to confirm the configuration of ompic. But I guess we can check the CPU NUMCORES register and compare it to the DT address space to do a sanity check.
Quoted 9 lines
Quoted 9 lines
drivers/irqchip/irq-ompic.c new file +9
-0+ +#define OMPIC_IPI_CTRL_IRQ_ACK (1 << 31) +#define OMPIC_IPI_CTRL_IRQ_GEN (1 << 30) +#define OMPIC_IPI_CTRL_DST (cpu) (((cpu) & 0x3fff) << 16) + +#define OMPIC_IPI_STAT_IRQ_PENDING (1 << 30) + +#define OMPIC_IPI_DATA (x) ((x) & 0xffff) +
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 36
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +3
-0+static struct { +unsigned long ops; +} ipi_data[NR_CPUS ];In general, a per cpu data structure is better expressed as a percpu data structure (yes, I'm in a funny mood this morning). Otherwise, you're going to thrash more than just the receiver and the sender, but also all the other CPUs that have their data in the same cache line.
Right, that makes sense, I am not sure why that was done this way. I think I borrowed from alpha which has the extra __cacheline_aligned annotations. I forgot to come back and fix this. Ill do as you suggest, thank you.
Excerpt from alpha:
static struct {
unsigned long bits ____cacheline_aligned;
} ipi_data[NR_CPUS] __cacheline_aligned;Quoted 8 lines
Quoted 8 lines
drivers/irqchip/irq-ompic.c new file +8
-0+ +static void __iomem *ompic_base; + +static inline u32 ompic_readreg (void __iomem *base,loff_t offset) +{ +return ioread32be (base + offset); +} +
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 50
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+static void ompic_writereg (void __iomem *base,loff_t offset,u32 data) +{ +iowrite32be (data, base + offset); +} + +#ifdef CONFIG_SMP This code is only selected when
CONFIG_SMP=y.
Yes, that is right, as below:
set_smp_cross_call(ompic_raise_softirq);
The set_smp_cross_call() function from smp.c is only defined for smp. Do you think thats wrong or needed extra comments? This is similar to other chips in irqchip/ for archs which use set_smp_cross_call().
Quoted 4 lines
drivers/irqchip/irq-ompic.c line 52
Quoted 2 lines
drivers/irqchip/irq-ompic.c new file +2
-0+void ompic_raise_softirq (const struct cpumask *mask,unsigned int irq) +{What is "irq" here? How is it guaranteed to fit in an unsigned long?
Here irq is the enum ipi_msg_type which is guaranteed to fit in unsigned long. Porbably its better to rename as msg or ipi_msg?
Quoted 5 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+unsigned int dst_cpu; +unsigned int src_cpu =smp_processor_id (); + +for_each_cpu (dst_cpu, mask) { +set_bit (irq, &ipi_data[dst_cpu].ops);
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 62
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (src_cpu), +OMPIC_IPI_CTRL_IRQ_GEN | +OMPIC_IPI_CTRL_DST (dst_cpu) | +OMPIC_IPI_DATA (1 ));What guarantees that the action of
set_bitcan be observed by the target CPU? set-bit gives you atomicity, but no barrier.
The bit will not be read by target CPUs until after the ompic_writereg() which will trigger the target CPU interrupt to be raised. OpenRISC stores are ordered.
This will work on OpenRISC, but should I be thinking of other architectures as well for all drivers? Or do you think this will be an issue on OpenRISC?
Quoted 6 lines
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+ } +} +#endif + +irqreturn_t ompic_ipi_handler (int irq,void *dev_id) +{
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 74
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+unsigned int cpu =smp_processor_id (); +unsigned long *pending_ops = &ipi_data[cpu].ops; +unsigned long ops; + +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (cpu),OMPIC_IPI_CTRL_IRQ_ACK ); +while ((ops =xchg (pending_ops,0 )) !=0 ) {Barrier again?
Thanks, but some concern of above.
Quoted 10 lines
drivers/irqchip/irq-ompic.c line 80
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+ do { + unsigned long ipi; + + ipi = ops & -ops; + ops &= ~ipi; + ipi = __ffs(ipi);This feels pointlessly convoluted. Is there any reason why you can't write it as:
Quoted 4 lines
ipi = __ffs(ops); ops &= ~(1UL << ipi);which feels like a much more common idiom?
Right, this should work, not sure what I was thinking above right now. Let me give that a try.
Quoted 12 lines
Quoted 12 lines
drivers/irqchip/irq-ompic.c new file +12
-0+ +handle_IPI (ipi); + }while (ops); + } + +return IRQ_HANDLED ; +} + +static struct irqaction ompi_ipi_irqaction = { + .handler = ompic_ipi_handler, + .flags =IRQF_PERCPU , + .name ="ompic_ipi" ,
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 95
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +3
-0+}; + +#ifdef CONFIG_OF This code is useless if you don't have
CONFIG_OF. So either you make the driver depend onCONFIG_OF, or you actively select it. And given thatCONFIG_OPENRISCalready selectsCONFIG_OF, you can happily get rid of all the #ifdefery here.
Thanks, right.
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 101
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+int __init ompic_of_init (struct device_node *node,struct device_node *parent) +{ +int irq; + +if (WARN_ON (!node)) +return -ENODEV ;How do you end-up here if node == NULL?
Right.
Quoted 4 lines
drivers/irqchip/irq-ompic.c line 103
Quoted 2 lines
drivers/irqchip/irq-ompic.c new file +2
-0+ +memset (ipi_data,0 ,sizeof (ipi_data));The kernel should do this for you already.
Right.
Quoted 7 lines
Quoted 7 lines
drivers/irqchip/irq-ompic.c new file +7
-0+ + ompic_base =of_iomap (node,0 ); + + irq =irq_of_parse_and_map (node,0 ); +setup_irq (irq, &ompi_ipi_irqaction); + +#ifdef CONFIG_SMP
Quoted 8 lines
drivers/irqchip/irq-ompic.c line 117
Quoted 8 lines
drivers/irqchip/irq-ompic.c new file +7
-0+set_smp_cross_call (ompic_raise_softirq); +#endif + + return 0; +} +IRQCHIP_DECLARE (ompic,"ompic" , ompic_of_init); +#endif
Thank you for the feedback, I will clean this up and resubmit with the comments on the other thread.
In terms of commit path, do you think its ok for this to go in via the OpenRISC arch path?
-Stafford
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 149 lines · Stafford Horne
Quoted 78 lines · Marc Zyngier
Quoted 77 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +22
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic" +- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- interrupts : Specifies the interrupt line to which the ompic is wired. + +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};arch/openrisc/Kconfig +1
-0@@ -30,6 +30,7 @@ config OPENRISC select NO_BOOTMEM select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS + select OMPIC if SMP config CPU_BIG_ENDIAN def_bool ydrivers/irqchip/Kconfig +0
-0@@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP select SPARSE_IRQ default yQuoted 5 lines
drivers/irqchip/Kconfig config CLPS711X_IRQCHIP · line 150
Quoted 3 lines
drivers/irqchip/Kconfig +3
-0+config OMPIC + bool + select IRQ_DOMAINWhy do you need to select
IRQ_DOMAIN? This driver doesn't seem to use any...Right, I will look to remove that.
Quoted 42 lines
Quoted 29 lines
drivers/irqchip/Kconfig +1
-0+ config OR1K_PIC bool select IRQ_DOMAINdrivers/irqchip/Makefile +1
-0@@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o +obj-$(CONFIG_OMPIC) += irq-ompic.o obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.odrivers/irqchip/irq-ompic.c new file +7
-0@@ -0,0 +1,117 @@ +/* +* Open Multi-Processor Interrupt Controller driver +* +* Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any drivers/irqchip/irq-ompic.c line 17
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+* kind, whether express or implied. +*/ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/smp.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/irqchip/chained_irq.h> Don't think you need this.
Quoted 3 lines
drivers/irqchip/irq-ompic.c line 18
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#include <linux/delay.h> Nor this.
OK on both.
Quoted 5 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +#include <linux/irqchip.h> + +#define OMPIC_IPI_BASE 0x0 +#define OMPIC_IPI_CTRL (cpu) (OMPIC_IPI_BASE + 0x0 + (cpu)*8)Quoted 8 lines
drivers/irqchip/irq-ompic.c line 24
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#define OMPIC_IPI_STAT (cpu) (OMPIC_IPI_BASE + 0x4 + (cpu)*8)In the DT binding you say that "size can be arbitrary based on the number of cores the controller has been configured to handle, typically 8 bytes per core". Here, this is 8 bytes, always, which is not exactly the same. What is the architectural value, if any? If there is none, then the per-core size should either come from DT or some other mean (register?).
Quoted 5 lines
I mean the address space is 8 bytes x number-of-cores. Thats what I meant by arbitrary, I guess its better for me to be explicit. There is no register that we can check to confirm the configuration of ompic. But I guess we can check the CPU NUMCORES register and compare it to the DT address space to do a sanity check.
Thanks for the explanation. So the code is OK, but the DT should be more rigorous in saying that there is 8 bytes per CPU. And yes to the check if that can be done at this stage.
Quoted 23 lines
Quoted 10 lines
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +9
-0+ +#define OMPIC_IPI_CTRL_IRQ_ACK (1 << 31) +#define OMPIC_IPI_CTRL_IRQ_GEN (1 << 30) +#define OMPIC_IPI_CTRL_DST (cpu) (((cpu) & 0x3fff) << 16) + +#define OMPIC_IPI_STAT_IRQ_PENDING (1 << 30) + +#define OMPIC_IPI_DATA (x) ((x) & 0xffff) +Quoted 8 lines
drivers/irqchip/irq-ompic.c line 36
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +3
-0+static struct { +unsigned long ops; +} ipi_data[NR_CPUS ];In general, a per cpu data structure is better expressed as a percpu data structure (yes, I'm in a funny mood this morning). Otherwise, you're going to thrash more than just the receiver and the sender, but also all the other CPUs that have their data in the same cache line.
Right, that makes sense, I am not sure why that was done this way. I think I borrowed from alpha which has the extra
__cacheline_alignedannotations. I forgot to come back and fix this. Ill do as you suggest, thank you.
Quoted 5 lines
Excerpt from alpha:
static struct { unsigned long bits ____cacheline_aligned; } ipi_data[NR_CPUS] __cacheline_aligned;
Yup, __cacheline_aligned is key here, as it will have the same effect as the percpu stuff from that point of view.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/irqchip/irq-ompic.c new file +8
-0+ +static void __iomem *ompic_base; + +static inline u32 ompic_readreg (void __iomem *base,loff_t offset) +{ +return ioread32be (base + offset); +} +Quoted 8 lines
drivers/irqchip/irq-ompic.c line 50
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+static void ompic_writereg (void __iomem *base,loff_t offset,u32 data) +{ +iowrite32be (data, base + offset); +} + +#ifdef CONFIG_SMP This code is only selected when
CONFIG_SMP=y.
Quoted 7 lines
Yes, that is right, as below:
set_smp_cross_call(ompic_raise_softirq);The
set_smp_cross_call()function fromsmp.cis only defined for smp. Do you think thats wrong or needed extra comments? This is similar to other chips in irqchip/ for archs which useset_smp_cross_call().
Other irqchips can often be compiled for both SMP and !SMP, hence the need of a #ifdef. In your case, this driver is only compiled when SMP is selected, so you're pretty much guaranteed that this symbol is available.
Quoted 8 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c line 52
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +2
-0+void ompic_raise_softirq (const struct cpumask *mask,unsigned int irq) +{What is "irq" here? How is it guaranteed to fit in an unsigned long?
Here irq is the
enum ipi_msg_typewhich is guaranteed to fit in unsigned long. Porbably its better to rename as msg oripi_msg?
You should certainly make sure your "enum ipi_msg_type" is capped at the number of bits of unsigned long. And yes to ipi_msg, which is a better name than irq. Also, you could change the types of ompic_raise_softirq and set_smp_cross_call, so that you use the enum instead of an int here.
Quoted 15 lines
Quoted 14 lines
drivers/irqchip/irq-ompic.c line 62
Quoted 11 lines
drivers/irqchip/irq-ompic.c new file +10
-0+unsigned int dst_cpu; +unsigned int src_cpu =smp_processor_id (); + +for_each_cpu (dst_cpu, mask) { +set_bit (irq, &ipi_data[dst_cpu].ops); + +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (src_cpu), +OMPIC_IPI_CTRL_IRQ_GEN | +OMPIC_IPI_CTRL_DST (dst_cpu) | +OMPIC_IPI_DATA (1 ));What guarantees that the action of
set_bitcan be observed by the target CPU? set-bit gives you atomicity, but no barrier.
Quoted 3 lines
The bit will not be read by target CPUs until after the
ompic_writereg()which will trigger the target CPU interrupt to be raised. OpenRISC stores are ordered.
Are they really ordered irrespective of the memory type (one is cacheable RAM, the other is a device...)?
And assuming they are (which I find a bit odd), that doesn't necessarily guarantee that the interrupt will only be delivered once the effect of set_bit can be seen on the target CPU...
Quoted 3 lines
This will work on OpenRISC, but should I be thinking of other architectures as well for all drivers? Or do you think this will be an issue on OpenRISC?
I definitely think this could be an issue with OpenRISC, but only someone familiar with the OpenRISC architecture can say whether I'm right or wrong. I'm just guessing at the moment.
[...]
Quoted 5 lines
Thank you for the feedback, I will clean this up and resubmit with the comments on the other thread.
In terms of commit path, do you think its ok for this to go in via the OpenRISC arch path?
Sure, that's fine by me.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Stafford Horne
maintainerRe: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 156 lines · Marc Zyngier
Quoted 149 lines · Stafford Horne
Quoted 78 lines · Marc Zyngier
Quoted 77 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +22
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic" +- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- interrupts : Specifies the interrupt line to which the ompic is wired. + +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};arch/openrisc/Kconfig +1
-0@@ -30,6 +30,7 @@ config OPENRISC select NO_BOOTMEM select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS + select OMPIC if SMP config CPU_BIG_ENDIAN def_bool ydrivers/irqchip/Kconfig +0
-0@@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP select SPARSE_IRQ default yQuoted 5 lines
drivers/irqchip/Kconfig config CLPS711X_IRQCHIP · line 150
Quoted 3 lines
drivers/irqchip/Kconfig +3
-0+config OMPIC + bool + select IRQ_DOMAINWhy do you need to select
IRQ_DOMAIN? This driver doesn't seem to use any...Right, I will look to remove that.
Quoted 42 lines
Quoted 29 lines
drivers/irqchip/Kconfig +1
-0+ config OR1K_PIC bool select IRQ_DOMAINdrivers/irqchip/Makefile +1
-0@@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o +obj-$(CONFIG_OMPIC) += irq-ompic.o obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.odrivers/irqchip/irq-ompic.c new file +7
-0@@ -0,0 +1,117 @@ +/* +* Open Multi-Processor Interrupt Controller driver +* +* Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any drivers/irqchip/irq-ompic.c line 17
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +10
-0+* kind, whether express or implied. +*/ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/smp.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/irqchip/chained_irq.h> Don't think you need this.
Quoted 3 lines
drivers/irqchip/irq-ompic.c line 18
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#include <linux/delay.h> Nor this.
OK on both.
Quoted 5 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +#include <linux/irqchip.h> + +#define OMPIC_IPI_BASE 0x0 +#define OMPIC_IPI_CTRL (cpu) (OMPIC_IPI_BASE + 0x0 + (cpu)*8)Quoted 8 lines
drivers/irqchip/irq-ompic.c line 24
Quoted 1 line
drivers/irqchip/irq-ompic.c new file +1
-0+#define OMPIC_IPI_STAT (cpu) (OMPIC_IPI_BASE + 0x4 + (cpu)*8)In the DT binding you say that "size can be arbitrary based on the number of cores the controller has been configured to handle, typically 8 bytes per core". Here, this is 8 bytes, always, which is not exactly the same. What is the architectural value, if any? If there is none, then the per-core size should either come from DT or some other mean (register?).
Quoted 5 lines
I mean the address space is 8 bytes x number-of-cores. Thats what I meant by arbitrary, I guess its better for me to be explicit. There is no register that we can check to confirm the configuration of ompic. But I guess we can check the CPU NUMCORES register and compare it to the DT address space to do a sanity check.
Quoted 3 lines
Thanks for the explanation. So the code is OK, but the DT should be more rigorous in saying that there is 8 bytes per CPU. And yes to the check if that can be done at this stage.
I will update that.
Quoted 23 lines
Quoted 23 lines
Quoted 10 lines
Quoted 10 lines
drivers/irqchip/irq-ompic.c new file +9
-0+ +#define OMPIC_IPI_CTRL_IRQ_ACK (1 << 31) +#define OMPIC_IPI_CTRL_IRQ_GEN (1 << 30) +#define OMPIC_IPI_CTRL_DST (cpu) (((cpu) & 0x3fff) << 16) + +#define OMPIC_IPI_STAT_IRQ_PENDING (1 << 30) + +#define OMPIC_IPI_DATA (x) ((x) & 0xffff) +Quoted 8 lines
drivers/irqchip/irq-ompic.c line 36
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +3
-0+static struct { +unsigned long ops; +} ipi_data[NR_CPUS ];In general, a per cpu data structure is better expressed as a percpu data structure (yes, I'm in a funny mood this morning). Otherwise, you're going to thrash more than just the receiver and the sender, but also all the other CPUs that have their data in the same cache line.
Right, that makes sense, I am not sure why that was done this way. I think I borrowed from alpha which has the extra
__cacheline_alignedannotations. I forgot to come back and fix this. Ill do as you suggest, thank you.
Quoted 8 lines
Quoted 5 lines
Excerpt from alpha:
static struct { unsigned long bits ____cacheline_aligned; } ipi_data[NR_CPUS] __cacheline_aligned;Yup,
__cacheline_alignedis key here, as it will have the same effect as the percpu stuff from that point of view.
Right, I think in this case I rather use the percpu API rather then depending on how well our compiler implements the __cacheline_aligned annotations.
Quoted 21 lines
Quoted 21 lines
Quoted 8 lines
Quoted 8 lines
drivers/irqchip/irq-ompic.c new file +8
-0+ +static void __iomem *ompic_base; + +static inline u32 ompic_readreg (void __iomem *base,loff_t offset) +{ +return ioread32be (base + offset); +} +Quoted 8 lines
drivers/irqchip/irq-ompic.c line 50
Quoted 6 lines
drivers/irqchip/irq-ompic.c new file +6
-0+static void ompic_writereg (void __iomem *base,loff_t offset,u32 data) +{ +iowrite32be (data, base + offset); +} + +#ifdef CONFIG_SMP This code is only selected when
CONFIG_SMP=y.Yes, that is right, as below:
set_smp_cross_call(ompic_raise_softirq);
Quoted 7 lines
Quoted 3 lines
The
set_smp_cross_call()function fromsmp.cis only defined for smp. Do you think thats wrong or needed extra comments? This is similar to other chips in irqchip/ for archs which useset_smp_cross_call().Other irqchips can often be compiled for both SMP and !SMP, hence the need of a #ifdef. In your case, this driver is only compiled when SMP is selected, so you're pretty much guaranteed that this symbol is available.
Right, I'll remove it.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c line 52
Quoted 3 lines
drivers/irqchip/irq-ompic.c new file +2
-0+void ompic_raise_softirq (const struct cpumask *mask,unsigned int irq) +{What is "irq" here? How is it guaranteed to fit in an unsigned long?
Quoted 7 lines
Quoted 2 lines
Here irq is the
enum ipi_msg_typewhich is guaranteed to fit in unsigned long. Porbably its better to rename as msg oripi_msg?You should certainly make sure your "enum
ipi_msg_type" is capped at the number of bits of unsigned long. And yes toipi_msg, which is a better name than irq. Also, you could change the types ofompic_raise_softirqandset_smp_cross_call, so that you use the enum instead of an int here.
OK.
I had a go at changing the type to the enum, but I realize that would require moving the enum definition into our asm/smp.h which I rather not do at the moment for sake of keeping it private inside of smp.c. This follows what other architectures do as well.
Quoted 19 lines
Quoted 15 lines
Quoted 14 lines
drivers/irqchip/irq-ompic.c line 62
Quoted 11 lines
drivers/irqchip/irq-ompic.c new file +10
-0+unsigned int dst_cpu; +unsigned int src_cpu =smp_processor_id (); + +for_each_cpu (dst_cpu, mask) { +set_bit (irq, &ipi_data[dst_cpu].ops); + +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (src_cpu), +OMPIC_IPI_CTRL_IRQ_GEN | +OMPIC_IPI_CTRL_DST (dst_cpu) | +OMPIC_IPI_DATA (1 ));What guarantees that the action of
set_bitcan be observed by the target CPU? set-bit gives you atomicity, but no barrier.Quoted 3 lines
The bit will not be read by target CPUs until after the
ompic_writereg()which will trigger the target CPU interrupt to be raised. OpenRISC stores are ordered.
Quoted 6 lines
Are they really ordered irrespective of the memory type (one is cacheable RAM, the other is a device...)?
And assuming they are (which I find a bit odd), that doesn't necessarily guarantee that the interrupt will only be delivered once the effect of
set_bitcan be seen on the target CPU...
OpenRISC might be a bit odd here, but I think this is correct. On OpenRISC the atomic instructions also imply a pipeline flush for stores and loads (l.swa/l.lwa). This is highlighted in the architecture spec section 7.3 [0].
[0] https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
-Stafford
Quoted 13 lines
Quoted 3 lines
This will work on OpenRISC, but should I be thinking of other architectures as well for all drivers? Or do you think this will be an issue on OpenRISC?
I definitely think this could be an issue with OpenRISC, but only someone familiar with the OpenRISC architecture can say whether I'm right or wrong. I'm just guessing at the moment.
[...]
Quoted 3 lines
Thank you for the feedback, I will clean this up and resubmit with the comments on the other thread.
Quoted 8 lines
Quoted 2 lines
In terms of commit path, do you think its ok for this to go in via the OpenRISC arch path?
Sure, that's fine by me.
M.Signature · 1 line
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 5 lines · Stafford Horne
Quoted 4 lines · Marc Zyngier
Quoted 3 lines · Stafford Horne
Quoted 2 lines · Marc Zyngier
Quoted 1 line · Stafford Horne
From: Stefan Kristiansson <[email protected]>
[...]
Quoted 21 lines
Quoted 21 lines
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+unsigned int dst_cpu; +unsigned int src_cpu =smp_processor_id (); + +for_each_cpu (dst_cpu, mask) { +set_bit (irq, &ipi_data[dst_cpu].ops);Quoted 8 lines
drivers/irqchip/irq-ompic.c line 62
Quoted 5 lines
drivers/irqchip/irq-ompic.c new file +5
-0+ +ompic_writereg (ompic_base,OMPIC_IPI_CTRL (src_cpu), +OMPIC_IPI_CTRL_IRQ_GEN | +OMPIC_IPI_CTRL_DST (dst_cpu) | +OMPIC_IPI_DATA (1 ));What guarantees that the action of
set_bitcan be observed by the target CPU? set-bit gives you atomicity, but no barrier.Quoted 3 lines
The bit will not be read by target CPUs until after the
ompic_writereg()which will trigger the target CPU interrupt to be raised. OpenRISC stores are ordered.Are they really ordered irrespective of the memory type (one is cacheable RAM, the other is a device...)?
Quoted 7 lines
Quoted 3 lines
And assuming they are (which I find a bit odd), that doesn't necessarily guarantee that the interrupt will only be delivered once the effect of
set_bitcan be seen on the target CPU...OpenRISC might be a bit odd here, but I think this is correct. On OpenRISC the atomic instructions also imply a pipeline flush for stores and loads (l.swa/l.lwa). This is highlighted in the architecture spec section 7.3 [0].
OK, fair enough. This is quite unusual (and I'm prepared to bet that this will be relaxed in future evolutions of the architecture), but that's indeed the gospel.
Please add a comment between set_bit and writereg so that we know we've been through this discussion already.
Thanks,
M.
Signature · 5 lines
Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/13] irqchip: add initial support for ompic
In reply to Stafford HorneQuoted 11 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
I'm a little confused. Is this device the whole "ompic", a sub-device thereof, or an independent IP block connected to it?
Quoted 23 lines
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +0
-0
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 5
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +5
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic"
This needs a vendor prefix.
Presumably, this should be "opencores,ompic"? (pending whether this is actually the whole ompic, as above).
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 8
Quoted 3 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +3
-0+- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core.
How are those regions correlated with CPUs?
e.g. is there a fixed relationship with a physical CPU id, or some mechanism by which the relationship can be probed dynamically?
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 11
Quoted 3 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +3
-0+- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1.
No flags? Does this not need edge/level configuration or active high/low configuration?
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 12
Quoted 1 line
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+- interrupts : Specifies the interrupt line to which the ompic is wired.
What is this interrupt used for?
Is this some percpu interrupt used to proxy the IPI? It feels very odd to assume such a thing from the parent irqchip. Surely this is intimately coupled with that?
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +10
-0+ +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};
[...]
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +5
-0+static struct { + unsigned long ops; +} ipi_data[NR_CPUS]; + +static void __iomem *ompic_base;
Is there guaranteed to only be one of these system-wide?
[...]
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 13 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +13
-0+void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq) +{ + unsigned int dst_cpu; + unsigned int src_cpu = smp_processor_id(); + + for_each_cpu(dst_cpu, mask) { + set_bit(irq, &ipi_data[dst_cpu].ops); + + ompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu), + OMPIC_IPI_CTRL_IRQ_GEN | + OMPIC_IPI_CTRL_DST(dst_cpu) | + OMPIC_IPI_DATA(1)); + }
Here you assume that the mapping is big enough to cover these registers, but the ompic_of_init() function didn't sanity-check the size, so this is not guaranteed.
[...]
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +6
-0+int __init ompic_of_init(struct device_node *node, struct device_node *parent) +{ + int irq; + + if (WARN_ON(!node)) + return -ENODEV;
Given this function is only invoked if the kernel found a node with a matching compatible string, how can this possibly happen?
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 1 line
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+ memset(ipi_data, 0, sizeof(ipi_data));
As ipi_data was static, it is already zeroed.
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +2
-0+ + ompic_base = of_iomap(node, 0);
What if this failed?
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +2
-0+ + irq = irq_of_parse_and_map(node, 0);
What if this is missing?
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 1 line
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+ setup_irq(irq, &ompi_ipi_irqaction);
As covered earlier, I;m confused by this. I'd expect that your root irqchip would handle the IPIs, and hence need to probe nothing from the DT for those.
Here we're assuming the IRQ is wired up to some per-cpu interrupt that we can treat as an IPI, and that the parent irqchip handles that appropriately, which seems very odd.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Stafford Horne
maintainerRe: [PATCH 05/13] irqchip: add initial support for ompic
Quoted 7 lines · Mark Rutland
Quoted 6 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
IPI driver for OpenRISC Multicore programmable interrupt controller as described in the Multicore support section of the OpenRISC 1.2 proposed architecture specification:
Quoted 8 lines
Quoted 5 lines
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
Each OpenRISC core contains a full interrupt controller which is used in the SMP architecture for interrupt balancing. This IPI device is the only external device required for enabling SMP on OpenRISC.
I'm a little confused. Is this device the whole "ompic", a sub-device thereof, or an independent IP block connected to it?
This device *is* the ompic, it currently just handles IPI communication, but was originally thought it should do more, i.e. distribute device interrupts, hence the name. For now, the ompic device only handles IPI.
Perhaps we can change the name? If you think that's needed I can discuss with Stefan (the creator).
This is documented in the pdf, but here is an ascii diagram to help.
Notes
o The ompic generates a level interrupt to the CPU PIC when a message is ready. Messages are delivered via the memory bus.
o The ompic does not have any interrupt input lines.
o The ompic must be wired to the same irq line on each core.
o Devices must be wired to the same irq line on each core.
o This seems strange, but in the end very little extra hardware is needed to enable smp.
+---------+ +---------+
| CPU | | CPU |
| Core 0 |<==\ (memory access) /==>| Core 1 |
| [ PIC ]| | | | [ PIC ]|
+----^-^--+ | | +----^-^--+
| | v v | |
<====|=|=================================|=|==> (memory bus)
| | ^ ^ | |
(ipi | +------|---------+--------|-------|-+ (device irq)
irq | | | | |
core0)| +------|---------|--------|-------+ (ipi irq core1)
| | | | |
+----o-o-+ | +--------+ |
| ompic |<===/ | Device |<===/
| IPI | +--------+
+--------+Quoted 31 lines
Quoted 23 lines
Pending ops are stored in a memory bit mask which can allow multiple pending operations to be set and serviced at a time. This is mostly borrowed from the alpha IPI implementation.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: converted ops to bitmask, wrote commit message]Signed-off-by Stafford Horne <[email protected]>--- .../bindings/interrupt-controller/ompic.txt | 22 ++++ arch/openrisc/Kconfig | 1 + drivers/irqchip/Kconfig | 4 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt create mode 100644 drivers/irqchip/irq-ompic.cDocumentation/devicetree/bindings/interrupt-controller/ompic.txt new file +0
-0Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 5
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +5
-0@@ -0,0 +1,22 @@ +OpenRISC Multicore Programmable Interrupt Controller + +Required properties: + +- compatible : This should be "ompic"This needs a vendor prefix.
Quoted 2 lines
Presumably, this should be "opencores,ompic"? (pending whether this is actually the whole ompic, as above).
As discussed above this is the ompic.
In terms of vendor, this and further openrisc development is no longer associated with opencores, that company just hosts the old opencores.org website but is not assisting in openrisc development any longer. Perhaps "openrisc,ompic" or "openrisc,ipi".
This also I would like to get final say from Stefan, but any suggestions are welcome.
Quoted 8 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 8
Quoted 3 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +3
-0+- reg : Specifies base physical address and size of the register space. The + size can be arbitrary based on the number of cores the controller has + been configured to handle, typically 8 bytes per core.How are those regions correlated with CPUs?
e.g. is there a fixed relationship with a physical CPU id, or some mechanism by which the relationship can be probed dynamically?
There should only be one region.
core id 0 is associated with registers at address 0x0 and 0x4 core id 1 is associated with registers at address 0x8 and 0xc etc.
This is can't be probed. Should this be documented here? That seems more of something that is internal to the driver. I think the only thing some one would want to set in TD is the register space size which should be be 8 x number-of-cores.
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 11
Quoted 3 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +3
-0+- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1.No flags? Does this not need edge/level configuration or active high/low configuration?
No flags, this maybe I am confused on, the ompic does not receive any interrupts. The input to the ompic are register writes which raise/clear interrupts on the target CPUs. So maybe it should not define an 'interrupt-controller' or '#interrupt-cells' tag. But then should it be an irqchip? Since its facilitates the IPI I think this is the right place.
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt line 12
Quoted 1 line
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+- interrupts : Specifies the interrupt line to which the ompic is wired.What is this interrupt used for?
Is this some percpu interrupt used to proxy the IPI? It feels very odd to assume such a thing from the parent irqchip. Surely this is intimately coupled with that?
This represents which IRQ line on each cpu the IPI is connected to (it must be connected to the same IRQ line on each CPU.
i.e.
interrupt-parent = <&pic>;
Quoted 13 lines
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +10
-0+ +Example: + +ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; +};[...]
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +5
-0+static struct { + unsigned long ops; +} ipi_data[NR_CPUS]; + +static void __iomem *ompic_base;Is there guaranteed to only be one of these system-wide?
Yes, that should be guaranteed, but I can look for moving this into a per device structure.
Quoted 11 lines
[...]
Quoted 9 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +9
-0+void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq) +{ + unsigned int dst_cpu; + unsigned int src_cpu = smp_processor_id(); + + for_each_cpu(dst_cpu, mask) { + set_bit(irq, &ipi_data[dst_cpu].ops); + + ompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),
Quoted 8 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 4 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +4
-0+ OMPIC_IPI_CTRL_IRQ_GEN | + OMPIC_IPI_CTRL_DST(dst_cpu) | + OMPIC_IPI_DATA(1)); + }Here you assume that the mapping is big enough to cover these registers, but the
ompic_of_init()function didn't sanity-check the size, so this is not guaranteed.
OK, I will add some sanity checking.
Quoted 11 lines
[...]
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +6
-0+int __init ompic_of_init(struct device_node *node, struct device_node *parent) +{ + int irq; + + if (WARN_ON(!node)) + return -ENODEV;Given this function is only invoked if the kernel found a node with a matching compatible string, how can this possibly happen?
Right, I think now. Will fix.
Quoted 3 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 1 line
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+ memset(ipi_data, 0, sizeof(ipi_data));As
ipi_datawas static, it is already zeroed.
Right, will fix.
Quoted 4 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +2
-0+ + ompic_base = of_iomap(node, 0);What if this failed?
Right will fix.
Quoted 4 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +2
-0+ + irq = irq_of_parse_and_map(node, 0);What if this is missing?
OK, I will do some error checking.
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt
Quoted 2 lines
Documentation/devicetree/bindings/interrupt-controller/ompic.txt new file +1
-0+ setup_irq(irq, &ompi_ipi_irqaction);As covered earlier, I;m confused by this. I'd expect that your root irqchip would handle the IPIs, and hence need to probe nothing from the DT for those.
Here we're assuming the IRQ is wired up to some per-cpu interrupt that we can treat as an IPI, and that the parent irqchip handles that appropriately, which seems very odd.
I hope the comments above help to clarify this. By appropriately you mean level/edge active high/low? The ompic and openrisc internal pic have been designed to be compatible.
Thanks for the review. Between this and the comments from Marc I have a bit of work to do. Please let me know if you have any further questions.
-Stafford
Stafford Horne
maintainerPATCH 6/13 openrisc: initial SMP support
In reply to Stafford HorneFrom: Stefan Kristiansson <[email protected]>
This patch introduces the SMP support for the OpenRISC architecture. The SMP architecture requires cores which have multicore features which have been introduced a few years back including:
- New SPRS SPR_COREID SPR_NUMCORES
- Shadow SPRs
- Atomic Instructions
- Cache Coherency
- A wired in IPI controller
This patch adds all of the SMP specific changes to core infrastructure, it looks big but it needs to go all together as its hard to split this one up.
Boot loader spinning of second cpu is not supported yet, it's assumed that Linux is booted straight after cpu reset.
The bulk of these changes are trivial changes to refactor to use per cpu data structures throughout. The addition of the smp.c and changes in time.c are the changes. Some specific notes:
MM changes
----------
The reason why this is created as an array, and not with DEFINE_PER_CPU is that doing it this way, we'll save a load in the tlb-miss handler (the load from __per_cpu_offset).
TLB Flush
---------
The SMP implementation of flush_tlb_* works by sending out a function-call IPI to all the non-local cpus by using the generic on_each_cpu() function.
Currently, all flush_tlb_* functions will result in a flush_tlb_all(), which has always been the behaviour in the UP case.
CPU INFO
--------
This creates a per cpu cpuinfo struct and fills it out accordingly for each activated cpu. show_cpuinfo is also updated to reflect new version information in later versions of the spec.
SMP API
-------
This imitates the arm64 implementation by having a smp_cross_call callback that can be set by set_smp_cross_call to initiate an IPI and a handle_IPI function that is expected to be called from an IPI irqchip driver.
--- arch/openrisc/Kconfig | 17 ++- arch/openrisc/include/asm/cpuinfo.h | 5 +- arch/openrisc/include/asm/mmu_context.h | 2 +- arch/openrisc/include/asm/pgtable.h | 2 +- arch/openrisc/include/asm/serial.h | 2 +- arch/openrisc/include/asm/smp.h | 26 ++++ arch/openrisc/include/asm/spr_defs.h | 14 ++ arch/openrisc/include/asm/tlbflush.h | 25 +++- arch/openrisc/kernel/Makefile | 1 + arch/openrisc/kernel/dma.c | 14 +- arch/openrisc/kernel/head.S | 97 ++++++++++++- arch/openrisc/kernel/setup.c | 155 +++++++++++++-------- arch/openrisc/kernel/smp.c | 234 ++++++++++++++++++++++++++++++++ arch/openrisc/kernel/time.c | 51 ++++--- arch/openrisc/lib/delay.c | 2 +- arch/openrisc/mm/fault.c | 4 +- arch/openrisc/mm/init.c | 2 +- arch/openrisc/mm/tlb.c | 16 +-- 18 files changed, 560 insertions(+), 109 deletions(-) create mode 100644 arch/openrisc/include/asm/smp.h create mode 100644 arch/openrisc/kernel/smp.c
arch/openrisc/Kconfig +15 -2
@@ -21,8 +21,10 @@ config OPENRISC select HAVE_UID16 select GENERIC_ATOMIC64 select GENERIC_CLOCKEVENTS + select GENERIC_CLOCKEVENTS_BROADCAST select GENERIC_STRNCPY_FROM_USER select GENERIC_STRNLEN_USER + select GENERIC_SMP_IDLE_THREAD select MODULES_USE_ELF_RELA select HAVE_DEBUG_STACKOVERFLOW select OR1K_PIC
@@ -107,8 +109,19 @@ config OPENRISC_HAVE_INST_DIV endmenu config NR_CPUS - int - default "1" + int "Maximum number of CPUs (2-32)" + range 2 32 + depends on SMP + default "2" + +config SMP + bool "Symmetric Multi-Processing support" + help + This enables support for systems with more than one CPU. If you have + a system with only one CPU, say N. If you have a system with more + than one CPU, say Y. + + If you don't know what to do here, say N. source kernel/Kconfig.hz source kernel/Kconfig.preempt
arch/openrisc/include/asm/cpuinfo.h +3 -2
@@ -19,7 +19,7 @@ #ifndef __ASM_OPENRISC_CPUINFO_H #define __ASM_OPENRISC_CPUINFO_H -struct cpuinfo { +struct cpuinfo_or1k { u32 clock_frequency; u32 icache_size;
@@ -31,6 +31,7 @@ struct cpuinfo { u32 dcache_ways; }; -extern struct cpuinfo cpuinfo; +extern struct cpuinfo_or1k cpuinfo_or1k[NR_CPUS ]; +extern void setup_cpuinfo (void );#endif /* __ASM_OPENRISC_CPUINFO_H */
arch/openrisc/include/asm/mmu_context.h +1 -1
@@ -34,7 +34,7 @@ extern void switch_mm(struct mm_struct *prev, struct mm_struct *next,* registers like cr3 on the i386 */ -extern volatile pgd_t *current_pgd;/* defined in arch/openrisc/mm/fault.c */ +extern volatile pgd_t *current_pgd[];/* defined in arch/openrisc/mm/fault.c */ static inline void enter_lazy_tlb (struct mm_struct *mm,struct task_struct *tsk) {
arch/openrisc/include/asm/pgtable.h +1 -1
@@ -94,7 +94,7 @@ extern void paging_init(void);* 64 MB of vmalloc area is comparable to what's available on other arches. */ -#define VMALLOC_START (PAGE_OFFSET-0x04000000) +#define VMALLOC_START (PAGE_OFFSET-0x04000000UL)#define VMALLOC_END (PAGE_OFFSET)#define VMALLOC_VMADDR (x) ((unsigned long)(x))
arch/openrisc/include/asm/serial.h +1 -1
@@ -29,7 +29,7 @@* it needs to be correct to get the early console working. */ -#define BASE_BAUD (cpuinfo.clock_frequency/16) +#define BASE_BAUD (cpuinfo_or1k[smp_processor_id()].clock_frequency/16)#endif /* __KERNEL__ */
arch/openrisc/include/asm/smp.h new file +26 -0
@@ -0,0 +1,26 @@ +/* +* Copyright (C) 2014 Stefan Kristiansson <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any +* kind, whether express or implied. +*/ + +#ifndef __ASM_OPENRISC_SMP_H +#define __ASM_OPENRISC_SMP_H + +#include <asm/spr.h> +#include <asm/spr_defs.h> + +#define raw_smp_processor_id () (current_thread_info()->cpu) +#define hard_smp_processor_id () mfspr(SPR_COREID) + +extern void smp_init_cpus (void ); + +extern void arch_send_call_function_single_ipi (int cpu); +extern void arch_send_call_function_ipi_mask (const struct cpumask *mask); + +extern void set_smp_cross_call (void (*)(const struct cpumask *,unsigned int )); +extern void handle_IPI (int ipinr); + +#endif /* __ASM_OPENRISC_SMP_H */
arch/openrisc/include/asm/spr_defs.h +14 -0
@@ -51,6 +51,11 @@#define SPR_ICCFGR (SPRGROUP_SYS + 6)#define SPR_DCFGR (SPRGROUP_SYS + 7)#define SPR_PCCFGR (SPRGROUP_SYS + 8) +#define SPR_VR2 (SPRGROUP_SYS + 9) +#define SPR_AVR (SPRGROUP_SYS + 10) +#define SPR_EVBAR (SPRGROUP_SYS + 11) +#define SPR_AECR (SPRGROUP_SYS + 12) +#define SPR_AESR (SPRGROUP_SYS + 13)#define SPR_NPC (SPRGROUP_SYS + 16)/* CZ 21/06/01 */ #define SPR_SR (SPRGROUP_SYS + 17)/* CZ 21/06/01 */ #define SPR_PPC (SPRGROUP_SYS + 18)/* CZ 21/06/01 */
@@ -61,6 +66,8 @@#define SPR_EEAR_LAST (SPRGROUP_SYS + 63)#define SPR_ESR_BASE (SPRGROUP_SYS + 64)#define SPR_ESR_LAST (SPRGROUP_SYS + 79) +#define SPR_COREID (SPRGROUP_SYS + 128) +#define SPR_NUMCORES (SPRGROUP_SYS + 129)#define SPR_GPR_BASE (SPRGROUP_SYS + 1024)/* Data MMU group */
@@ -135,12 +142,19 @@#define SPR_VR_CFG 0x00ff0000/* Processor configuration */ #define SPR_VR_RES 0x0000ffc0/* Reserved */ #define SPR_VR_REV 0x0000003f/* Processor revision */ +#define SPR_VR_UVRP 0x00000040/* Updated Version Registers Present */ #define SPR_VR_VER_OFF 24#define SPR_VR_CFG_OFF 16#define SPR_VR_REV_OFF 0/* +* Bit definitions for the Version Register 2 +*/ +#define SPR_VR2_CPUID 0xff000000/* Processor ID */ +#define SPR_VR2_VER 0x00ffffff/* Processor version */ + +/* * Bit definitions for the Unit Present Register * */
arch/openrisc/include/asm/tlbflush.h +19 -6
@@ -33,13 +33,26 @@* - flush_tlb_page(vma, vmaddr) flushes one page * - flush_tlb_range(mm, start, end) flushes a range of pages */ +extern void local_flush_tlb_all (void ); +extern void local_flush_tlb_mm (struct mm_struct *mm); +extern void local_flush_tlb_page (struct vm_area_struct *vma, +unsigned long addr); +extern void local_flush_tlb_range (struct vm_area_struct *vma, +unsigned long start, +unsigned long end); -void flush_tlb_all (void ); -void flush_tlb_mm (struct mm_struct *mm); -void flush_tlb_page (struct vm_area_struct *vma,unsigned long addr); -void flush_tlb_range (struct vm_area_struct *vma, -unsigned long start, -unsigned long end); +#ifndef CONFIG_SMP +#define flush_tlb_all local_flush_tlb_all +#define flush_tlb_mm local_flush_tlb_mm +#define flush_tlb_page local_flush_tlb_page +#define flush_tlb_range local_flush_tlb_range +#else +extern void flush_tlb_all (void ); +extern void flush_tlb_mm (struct mm_struct *mm); +extern void flush_tlb_page (struct vm_area_struct *vma,unsigned long addr); +extern void flush_tlb_range (struct vm_area_struct *vma,unsigned long start, +unsigned long end); +#endif static inline void flush_tlb (void ) {
arch/openrisc/kernel/Makefile +1 -0
@@ -8,6 +8,7 @@ obj-y := setup.o or32_ksyms.o process.o dma.o \ traps.o time.o irq.o entry.o ptrace.o signal.o \ sys_call_table.o +obj-$(CONFIG_SMP) += smp.o obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_OF) += prom.o
arch/openrisc/kernel/dma.c +9 -5
@@ -32,6 +32,7 @@ page_set_nocache(pte_t *pte, unsigned long addr, unsigned long next, struct mm_walk *walk) { unsigned long cl; +struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id ()];pte_val (*pte) |= _PAGE_CI;
@@ -42,7 +43,7 @@ page_set_nocache(pte_t *pte, unsigned long addr,flush_tlb_page (NULL , addr);/* Flush page out of dcache */ -for (cl =__pa (addr); cl <__pa (next); cl += cpuinfo.dcache_block_size) +for (cl =__pa (addr); cl <__pa (next); cl += cpuinfo->dcache_block_size)mtspr (SPR_DCBFR , cl);return 0 ;
@@ -140,6 +141,7 @@ or1k_map_page(struct device *dev, struct page *page, {unsigned long cl;dma_addr_t addr =page_to_phys (page) + offset; +struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id ()];if (attrs &DMA_ATTR_SKIP_CPU_SYNC )return addr;
@@ -148,13 +150,13 @@ or1k_map_page(struct device *dev, struct page *page,case DMA_TO_DEVICE :/* Flush the dcache for the requested range */ for (cl = addr; cl < addr + size; - cl += cpuinfo.dcache_block_size) + cl += cpuinfo->dcache_block_size)mtspr (SPR_DCBFR , cl);break ;case DMA_FROM_DEVICE :/* Invalidate the dcache for the requested range */ for (cl = addr; cl < addr + size; - cl += cpuinfo.dcache_block_size) + cl += cpuinfo->dcache_block_size)mtspr (SPR_DCBIR , cl);break ;default :
@@ -213,9 +215,10 @@ or1k_sync_single_for_cpu(struct device *dev, {unsigned long cl;dma_addr_t addr = dma_handle; +struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id ()];/* Invalidate the dcache for the requested range */ -for (cl = addr; cl < addr + size; cl += cpuinfo.dcache_block_size) +for (cl = addr; cl < addr + size; cl += cpuinfo->dcache_block_size)mtspr (SPR_DCBIR , cl); }
@@ -226,9 +229,10 @@ or1k_sync_single_for_device(struct device *dev, {unsigned long cl;dma_addr_t addr = dma_handle; +struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id ()];/* Flush the dcache for the requested range */ -for (cl = addr; cl < addr + size; cl += cpuinfo.dcache_block_size) +for (cl = addr; cl < addr + size; cl += cpuinfo->dcache_block_size)mtspr (SPR_DCBFR , cl); }
arch/openrisc/kernel/head.S +92 -5
126 lines not on this page · raw message
arch/openrisc/kernel/setup.c +101 -54
229 lines not on this page · raw message
arch/openrisc/kernel/smp.c new file +234 -0
234 lines not on this page · raw message
arch/openrisc/kernel/time.c +31 -20
82 lines not on this page · raw message
arch/openrisc/lib/delay.c +1 -1
@@ -25,7 +25,7 @@int read_current_timer (unsigned long *timer_value) { - *timer_value =mfspr (SPR_TTCR ); + *timer_value =get_cycles ();return 0 ; }
arch/openrisc/mm/fault.c +2 -2
@@ -33,7 +33,7 @@ unsigned long pte_errors; /* updated by do_page_fault() *//* __PHX__ :: - check the vmalloc_fault in do_page_fault() * - also look into include/asm-or32/mmu_context.h */ -volatile pgd_t *current_pgd; +volatile pgd_t *current_pgd[NR_CPUS ];extern void die (char *,struct pt_regs *,long );
@@ -319,7 +319,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,phx_mmu("vmalloc_fault"); */ - pgd = (pgd_t *)current_pgd + offset; + pgd = (pgd_t *)current_pgd[smp_processor_id ()] + offset; pgd_k = init_mm.pgd + offset;/* Since we're two-level, we don't need to do both
arch/openrisc/mm/init.c +1 -1
@@ -147,7 +147,7 @@ void __init paging_init(void)* (even if it is most probably not used until the next * switch_mm) */ - current_pgd = init_mm.pgd; + current_pgd[smp_processor_id ()] = init_mm.pgd; end = (unsigned long )__va (max_low_pfn *PAGE_SIZE );
arch/openrisc/mm/tlb.c +8 -8
@@ -49,7 +49,7 @@* */ -void flush_tlb_all (void ) +void local_flush_tlb_all (void ) {int i;unsigned long num_tlb_sets;
@@ -86,7 +86,7 @@ void flush_tlb_all(void)#define flush_itlb_page_no_eir (addr) \ mtspr_off(SPR_ITLBMR_BASE(0), ITLB_OFFSET(addr), 0); -void flush_tlb_page (struct vm_area_struct *vma,unsigned long addr) +void local_flush_tlb_page (struct vm_area_struct *vma,unsigned long addr) {if (have_dtlbeir)flush_dtlb_page_eir (addr);
@@ -99,8 +99,8 @@ void flush_tlb_page(struct vm_area_struct *vma, unsigned long addr)flush_itlb_page_no_eir (addr); } -void flush_tlb_range (struct vm_area_struct *vma, -unsigned long start,unsigned long end) +void local_flush_tlb_range (struct vm_area_struct *vma, +unsigned long start,unsigned long end) {int addr;bool dtlbeir;
@@ -129,13 +129,13 @@ void flush_tlb_range(struct vm_area_struct *vma,* This should be changed to loop over over mm and call flush_tlb_range. */ -void flush_tlb_mm (struct mm_struct *mm) +void local_flush_tlb_mm (struct mm_struct *mm) {/* Was seeing bugs with the mm struct passed to us. Scrapped most of this function. */ /* Several architctures do this */ -flush_tlb_all (); +local_flush_tlb_all (); }/* called in schedule() just before actually doing the switch_to */
@@ -149,14 +149,14 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next,* might be invalid at points where we still need to derefer * the pgd. */ - current_pgd = next->pgd; + current_pgd[smp_processor_id ()] = next->pgd;/* We don't have context support implemented, so flush all * entries belonging to previous map */ if (prev != next) -flush_tlb_mm (prev); +local_flush_tlb_mm (prev); }
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 7/13 openrisc: fix initial preempt state for secondary cpu tasks
In reply to Stafford HorneDuring SMP testing we were getting the below warning after booting the secondary cpu:
[ 0.060000] BUG: scheduling while atomic: swapper/1/0/0x00000000
This change follows similar patterns from other architectures to start the schduler with preempt disabled.
--- arch/openrisc/include/asm/thread_info.h | 2 +- arch/openrisc/kernel/smp.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
arch/openrisc/include/asm/thread_info.h +1 -1
@@ -74,7 +74,7 @@ struct thread_info { .task = &tsk, \ .flags = 0, \ .cpu = 0, \ - .preempt_count = 1, \ + .preempt_count = INIT_PREEMPT_COUNT, \ .addr_limit = KERNEL_DS, \ .ksp = 0, \ }
arch/openrisc/kernel/smp.c +1 -0
@@ -125,6 +125,7 @@ asmlinkage __init void secondary_start_kernel(void)local_irq_enable (); +preempt_disable ();/* * OK, it's off to the idle thread for us */
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 8/13 openrisc: sleep instead of spin on secondary wait
In reply to Stafford HorneCurrently we do a spin on secondary cpus when waiting to boot. This causes issues with power consumption as well as qemu cycle burning (it starves cpu 0 from actually being able to boot.)
The secondary interrupt handler is stored in the init section so as to allow it to be reclaimed after boot. However, if we ever want to support hotplug this may need to change.
--- arch/openrisc/kernel/head.S | 44 ++++++++++++++++++++++++++++++++++++++++++-- arch/openrisc/kernel/smp.c | 9 +++++++-- 2 files changed, 49 insertions(+), 4 deletions(-)
arch/openrisc/kernel/head.S +42 -2
@@ -712,9 +712,37 @@ _flush_tlb: #ifdef CONFIG_SMP secondary_wait: + /* Doze the cpu until we are asked to run */ + /* Setup special secondary exception handler */ + LOAD_SYMBOL_2_GPR(r3, _secondary_evbar) + tophys(r25,r3) + l.mtspr r0,r25,SPR_EVBAR + + /* Enable Interrupts */ + l.mfspr r25,r0,SPR_SR + l.ori r25,r25,SPR_SR_IEE + l.mtspr r0,r25,SPR_SR + + /* Unmask interrupts interrupts */ + l.mfspr r25,r0,SPR_PICMR + l.ori r25,r25,0xffff + l.mtspr r0,r25,SPR_PICMR + + /* Doze */ + l.mfspr r25,r0,SPR_PMR + LOAD_SYMBOL_2_GPR(r3, SPR_PMR_DME) + l.or r25,r25,r3 + l.mtspr r0,r25,SPR_PMR + + /* Wakeup - Restore exception handler */ + l.mtspr r0,r0,SPR_EVBAR + + /* + * Check if we actually got the wake signal, if not go-back to + * sleep. + */ l.mfspr r25,r0,SPR_COREID - l.movhi r3,hi(secondary_release) - l.ori r3,r3,lo(secondary_release) + LOAD_SYMBOL_2_GPR(r3, secondary_release) tophys(r4, r3) l.lwz r3,0(r4) l.sfeq r25,r3
@@ -1663,6 +1691,18 @@ ENTRY(_early_uart_init) l.jr r9 l.nop + .section .init.text, "ax" + .align 0x1000 + .global _secondary_evbar +_secondary_evbar: + + .space 0x800 + /* Just disable interrupts and Return */ + l.ori r3,r0,SPR_SR_SM + l.mtspr r0,r3,SPR_ESR_BASE + l.rfe + + .section .rodata _string_unhandled_exception: .string "\n\rRunarunaround: Unhandled exception 0x\0"
arch/openrisc/kernel/smp.c +7 -2
@@ -18,10 +18,13 @@#include <asm/mmu_context.h> #include <asm/tlbflush.h> +static void (*smp_cross_call)(const struct cpumask *,unsigned int ); +volatile unsigned long secondary_release =-1 ;struct thread_info *secondary_thread_info;enum ipi_msg_type { +IPI_WAKEUP ,IPI_RESCHEDULE ,IPI_CALL_FUNC ,IPI_CALL_FUNC_SINGLE ,
@@ -38,6 +41,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)spin_lock (&boot_lock); secondary_release = cpu; +smp_cross_call (cpumask_of (cpu),IPI_WAKEUP );/* * now the secondary core is starting up let it run its
@@ -137,6 +141,9 @@ void handle_IPI(int ipinr)unsigned int cpu =smp_processor_id ();switch (ipinr) { +case IPI_WAKEUP : +break ; +case IPI_RESCHEDULE :scheduler_ipi ();break ;
@@ -155,8 +162,6 @@ void handle_IPI(int ipinr) } } -static void (*smp_cross_call)(const struct cpumask *,unsigned int ); -void smp_send_reschedule (int cpu) {smp_cross_call (cpumask_of (cpu),IPI_RESCHEDULE );
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 9/13 openrisc: add cacheflush support to fix icache aliasing
In reply to Stafford HorneFrom: Jan Henrik Weinstock <[email protected]>
On OpenRISC the icache does not snoop data stores. This can cause aliasing as reported by Jan. This patch fixes the issue to ensure icache is properly synchronized when code is written to memory. It supports both SMP and UP flushing.
This supports dcache flush as well for architectures that do not support write-through caches; most OpenRISC implementations do implement write-through cache however. Dcache flushes are done only on a single core as OpenRISC dcaches all support snooping of bus stores.
--- arch/openrisc/Kconfig | 11 ++++ arch/openrisc/include/asm/Kbuild | 1 - arch/openrisc/include/asm/cacheflush.h | 96 ++++++++++++++++++++++++++++++++++ arch/openrisc/include/asm/pgtable.h | 16 +++--- arch/openrisc/kernel/smp.c | 15 ++++++ arch/openrisc/mm/Makefile | 2 +- arch/openrisc/mm/cache.c | 61 +++++++++++++++++++++ 7 files changed, 194 insertions(+), 8 deletions(-) create mode 100644 arch/openrisc/include/asm/cacheflush.h create mode 100644 arch/openrisc/mm/cache.c
arch/openrisc/Kconfig +11 -0
@@ -77,6 +77,17 @@ config OR1K_1200 endchoice +config DCACHE_WRITETHROUGH + bool "Have write through data caches" + default n + help + Select this if your implementation features write through data caches. + Selecting 'N' here will allow the kernel to force flushing of data + caches at relevant times. Most OpenRISC implementations support write- + through data caches. + + If unsure say N here + config OPENRISC_BUILTIN_DTB string "Builtin DTB" default ""
arch/openrisc/include/asm/Kbuild +0 -1
@@ -1,7 +1,6 @@ generic-y += barrier.h generic-y += bug.h generic-y += bugs.h -generic-y += cacheflush.h generic-y += checksum.h generic-y += clkdev.h generic-y += current.h
arch/openrisc/include/asm/cacheflush.h new file +96 -0
96 lines not on this page · raw message
arch/openrisc/include/asm/pgtable.h +10 -6
@@ -414,15 +414,19 @@ static inline void pmd_set(pmd_t *pmdp, pte_t *ptep)extern pgd_t swapper_pg_dir[PTRS_PER_PGD ];/* defined in head.S */ -/* -* or32 doesn't have any external MMU info: the kernel page -* tables contain all the necessary information. -* -* Actually I am not sure on what this could be used for. -*/ +static inline void update_tlb (struct vm_area_struct *vma, +unsigned long address,pte_t *pte) +{ +} + +extern void update_cache (struct vm_area_struct *vma, +unsigned long address,pte_t *pte); +static inline void update_mmu_cache (struct vm_area_struct *vma,unsigned long address,pte_t *pte) { +update_tlb (vma, address, pte); +update_cache (vma, address, pte); }/* __PHX__ FIXME, SWAP, this probably doesn't work */
arch/openrisc/kernel/smp.c +15 -0
@@ -17,6 +17,7 @@#include <asm/cpuinfo.h> #include <asm/mmu_context.h> #include <asm/tlbflush.h> +#include <asm/cacheflush.h> static void (*smp_cross_call)(const struct cpumask *,unsigned int );
@@ -238,3 +239,17 @@ void flush_tlb_range(struct vm_area_struct *vma, {on_each_cpu (ipi_flush_tlb_all,NULL ,1 ); } + +/* Instruction cache invalidate - performed on each cpu */ +static void ipi_icache_page_inv (void *arg) +{ +struct page *page = arg; + +local_icache_page_inv (page); +} + +void smp_icache_page_inv (struct page *page) +{ +on_each_cpu (ipi_icache_page_inv, page,1 ); +} +EXPORT_SYMBOL (smp_icache_page_inv);
arch/openrisc/mm/Makefile +1 -1
@@ -2,4 +2,4 @@ # Makefile for the linux openrisc-specific parts of the memory manager. # -obj-y := fault.o tlb.o init.o ioremap.o +obj-y := fault.o cache.o tlb.o init.o ioremap.o
arch/openrisc/mm/cache.c new file +61 -0
61 lines not on this page · raw message
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 10/13 openrisc: add simple_smp dts and defconfig for simulators
In reply to Stafford HorneFrom: Stefan Kristiansson <[email protected]>
Simple enough to be compatible with simulation environments, such as verilated systems, QEMU and other targets supporting OpenRISC SMP. This also supports our base FPGA SoC's if the cpu frequency is upped to 50Mhz.
--- arch/openrisc/boot/dts/simple_smp.dts | 58 ++++++++++++++++++++++++++ arch/openrisc/configs/simple_smp_defconfig | 66 ++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 arch/openrisc/boot/dts/simple_smp.dts create mode 100644 arch/openrisc/configs/simple_smp_defconfig
arch/openrisc/boot/dts/simple_smp.dts new file +58 -0
@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim"; + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <&pic>; + + chosen { + bootargs = "console=uart,mmio,0x90000000,115200"; + }; + + memory@0 { + device_type = "memory"; + reg = <0x00000000 0x02000000>; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + cpu@0 { + compatible = "opencores,or1200-rtlsvn481"; + reg = <0>; + clock-frequency = <20000000>; + }; + cpu@1 { + compatible = "opencores,or1200-rtlsvn481"; + reg = <1>; + clock-frequency = <20000000>; + }; + }; + + ompic: ompic { + compatible = "ompic"; + reg = <0x98000000 16>; + #interrupt-cells = <1>; + interrupt-controller; + interrupts = <1>; + }; + + /* + * OR1K PIC is built into CPU and accessed via special purpose + * registers. It is not addressable and, hence, has no 'reg' + * property. + */ + pic: pic { + compatible = "opencores,or1k-pic-level"; + #interrupt-cells = <1>; + interrupt-controller; + }; + + serial0: serial@90000000 { + compatible = "opencores,uart16550-rtlsvn105", "ns16550a"; + reg = <0x90000000 0x100>; + interrupts = <2>; + clock-frequency = <20000000>; + }; + +};
arch/openrisc/configs/simple_smp_defconfig new file +66 -0
66 lines not on this page · raw message
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 11/13 openrisc: support framepointers and STACKTRACE_SUPPORT
For lockdep support a reliable stack trace mechanism is needed. This patch adds support in OpenRISC for the stacktrace framework, implemented by a simple unwinder api. The unwinder api supports both framepointer and basic stack tracing.
The unwinder is now used to replace the stack_dump() implementation as well. The new traces are inline with other architectures trace format:
Call trace: [<c0004448>] show_stack+0x3c/0x58 [<c031c940>] dump_stack+0xa8/0xe4 [<c0008104>] __cpu_up+0x64/0x130 [<c000d268>] bringup_cpu+0x3c/0x178 [<c000d038>] cpuhp_invoke_callback+0xa8/0x1fc [<c000d680>] cpuhp_up_callbacks+0x44/0x14c [<c000e400>] cpu_up+0x14c/0x1bc [<c041da60>] smp_init+0x104/0x15c [<c033843c>] ? kernel_init+0x0/0x140 [<c0415e04>] kernel_init_freeable+0xbc/0x25c [<c033843c>] ? kernel_init+0x0/0x140 [<c0338458>] kernel_init+0x1c/0x140 [<c003a174>] ? schedule_tail+0x18/0xa0 [<c0006b80>] ret_from_fork+0x1c/0x9c
--- arch/openrisc/Kconfig | 4 ++ arch/openrisc/include/asm/unwinder.h | 20 +++++++ arch/openrisc/kernel/Makefile | 3 +- arch/openrisc/kernel/stacktrace.c | 86 ++++++++++++++++++++++++++++ arch/openrisc/kernel/traps.c | 54 +++--------------- arch/openrisc/kernel/unwinder.c | 105 +++++++++++++++++++++++++++++++++++ 6 files changed, 224 insertions(+), 48 deletions(-) create mode 100644 arch/openrisc/include/asm/unwinder.h create mode 100644 arch/openrisc/kernel/stacktrace.c create mode 100644 arch/openrisc/kernel/unwinder.c
arch/openrisc/Kconfig +4 -0
@@ -33,6 +33,7 @@ config OPENRISC select ARCH_USE_QUEUED_SPINLOCKS select ARCH_USE_QUEUED_RWLOCKS select OMPIC if SMP + select ARCH_WANT_FRAME_POINTERS config CPU_BIG_ENDIAN def_bool y
@@ -60,6 +61,9 @@ config TRACE_IRQFLAGS_SUPPORT config GENERIC_CSUM def_bool y +config STACKTRACE_SUPPORT + def_bool y + source "init/Kconfig" source "kernel/Kconfig.freezer"
arch/openrisc/include/asm/unwinder.h new file +20 -0
@@ -0,0 +1,20 @@ +/* +* OpenRISC unwinder.h +* +* Architecture API for unwinding stacks. +* +* Copyright (C) 2017 Stafford Horne <[email protected]> +* +* This file is licensed under the terms of the GNU General Public License +* version 2. This program is licensed "as is" without any warranty of any +* kind, whether express or implied. +*/ + +#ifndef __ASM_OPENRISC_UNWINDER_H +#define __ASM_OPENRISC_UNWINDER_H + +void unwind_stack (void *data,unsigned long *stack, +void (*trace)(void *data,unsigned long addr, +int reliable)); + +#endif /* __ASM_OPENRISC_UNWINDER_H */
arch/openrisc/kernel/Makefile +2 -1
@@ -6,9 +6,10 @@ extra-y := head.o vmlinux.lds obj-y := setup.o or32_ksyms.o process.o dma.o \ traps.o time.o irq.o entry.o ptrace.o signal.o \ - sys_call_table.o + sys_call_table.o unwinder.o obj-$(CONFIG_SMP) += smp.o +obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_OF) += prom.o
arch/openrisc/kernel/stacktrace.c new file +86 -0
86 lines not on this page · raw message
arch/openrisc/kernel/traps.c +7 -47
81 lines not on this page · raw message
arch/openrisc/kernel/unwinder.c new file +105 -0
105 lines not on this page · raw message
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 12/13 openrisc: enable LOCKDEP_SUPPORT and irqflags tracing
In reply to Stafford HorneLockdep is needed for proving the spinlocks and rwlocks work fine on our platform. It also requires calling the trace_hardirqs_off() and trace_hardirqs_on() pair of routines when entering and exiting an interrupt.
For OpenRISC the interrupt stack frame does not support frame pointers, so to call trace_hardirqs_on() and trace_hardirqs_off() here the macro's build up a stack frame each time.
There is one necessary small change in _sys_call_handler to move interrupt enabling later so they can get re-enabled during syscall restart. This was done to fix lockdep warnings that are now possible due to this
patch.
--- arch/openrisc/Kconfig | 3 ++ arch/openrisc/kernel/entry.S | 74 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 3 deletions(-)
arch/openrisc/Kconfig +3 -0
@@ -64,6 +64,9 @@ config GENERIC_CSUM config STACKTRACE_SUPPORT def_bool y +config LOCKDEP_SUPPORT + def_bool y + source "init/Kconfig" source "kernel/Kconfig.freezer"
arch/openrisc/kernel/entry.S +71 -3
116 lines not on this page · raw message
Signature · 1 line
2.13.5
Stafford Horne
maintainerPATCH 13/13 openrisc: add tick timer multicore sync logic
In reply to Stafford HorneIn case timers are not in sync when cpus start (i.e. hot plug / offset resets) we need to synchronize the secondary cpus internal timer with the main cpu. This is needed as in OpenRISC SMP there is only one clocksource registered which reads from the same ttcr register on each cpu.
This synchronization routine heavily borrows from mips implementation that does something similar.
--- arch/openrisc/include/asm/or1k-timer.h | 27 ++++++++ arch/openrisc/kernel/Makefile | 2 +- arch/openrisc/kernel/smp.c | 15 +++-- arch/openrisc/kernel/sync-timer.c | 120 +++++++++++++++++++++++++++++++++ arch/openrisc/kernel/time.c | 15 ++++- 5 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 arch/openrisc/include/asm/or1k-timer.h create mode 100644 arch/openrisc/kernel/sync-timer.c
arch/openrisc/include/asm/or1k-timer.h new file +27 -0
@@ -0,0 +1,27 @@ +/* +* OpenRISC timer API +* +* Copyright (C) 2008 by Ralf Baechle (ralf at linux-mips.org) +* Copyright (C) 2017 by Stafford Horne (shorne at gmail.com) +* +* This file is subject to the terms and conditions of the GNU General Public +* License. See the file "COPYING" in the main directory of this archive +* for more details. +*/ +#ifndef __ASM_OR1K_TIMER_H +#define __ASM_OR1K_TIMER_H + +#include <linux/compiler.h> + +void openrisc_timer_set (unsigned long count); +void openrisc_timer_set_next (unsigned long delta); + +#ifdef CONFIG_SMP +void synchronise_count_master (int cpu); +void synchronise_count_slave (int cpu); +#else +static inline void synchronise_count_master (int cpu) {} +static inline void synchronise_count_slave (int cpu) {} +#endif + +#endif /* __ASM_OR1K_TIMER_H */
arch/openrisc/kernel/Makefile +1 -1
@@ -8,7 +8,7 @@ obj-y := setup.o or32_ksyms.o process.o dma.o \ traps.o time.o irq.o entry.o ptrace.o signal.o \ sys_call_table.o unwinder.o -obj-$(CONFIG_SMP) += smp.o +obj-$(CONFIG_SMP) += smp.o sync-timer.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_OF) += prom.o
arch/openrisc/kernel/smp.c +10 -5
@@ -18,6 +18,7 @@#include <asm/mmu_context.h> #include <asm/tlbflush.h> #include <asm/cacheflush.h> +#include <asm/or1k-timer.h> static void (*smp_cross_call)(const struct cpumask *,unsigned int );
@@ -92,13 +93,15 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) ret =boot_secondary (cpu, idle);if (ret ==0 ) { -wait_for_completion_timeout (&cpu_running, -msecs_to_jiffies (1000 )); -if (!cpu_online (cpu)) - ret = -EIO ; +if (!wait_for_completion_timeout (&cpu_running, +msecs_to_jiffies (1000 ))) { +pr_crit ("CPU%u: failed to start\n" , cpu); +return -EIO ; + } +synchronise_count_master (cpu); } -return ret; +return 0 ; }extern void openrisc_clockevent_init (void );
@@ -128,6 +131,8 @@ asmlinkage __init void secondary_start_kernel(void)set_cpu_online (cpu, true);complete (&cpu_running); +synchronise_count_slave (cpu); +local_irq_enable ();preempt_disable ();
arch/openrisc/kernel/sync-timer.c new file +120 -0
120 lines not on this page · raw message
arch/openrisc/kernel/time.c +13 -2
@@ -27,8 +27,14 @@#include <asm/cpuinfo.h> -static int openrisc_timer_set_next_event (unsigned long delta, -struct clock_event_device *dev) +/* Test the timer ticks to count, used in sync routine */ +inline void openrisc_timer_set (unsigned long count) +{ +mtspr (SPR_TTCR , count); +} + +/* Set the timer to trigger in delta cycles */ +inline void openrisc_timer_set_next (unsigned long delta) {u32 c;
@@ -44,7 +50,12 @@ static int openrisc_timer_set_next_event(unsigned long delta,* Keep timer in continuous mode always. */ mtspr (SPR_TTMR ,SPR_TTMR_CR |SPR_TTMR_IE | c); +} +static int openrisc_timer_set_next_event (unsigned long delta, +struct clock_event_device *dev) +{ +openrisc_timer_set_next (delta);return 0 ; }
Signature · 1 line
2.13.5
Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
In reply to Stafford HorneQuoted 23 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
Simple enough to be compatible with simulation environments, such as verilated systems, QEMU and other targets supporting OpenRISC SMP. This also supports our base FPGA SoC's if the cpu frequency is upped to 50Mhz.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: Added defconfig]Signed-off-by Stafford Horne <[email protected]>--- arch/openrisc/boot/dts/simple_smp.dts | 58 ++++++++++++++++++++++++++ arch/openrisc/configs/simple_smp_defconfig | 66 ++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 arch/openrisc/boot/dts/simple_smp.dts create mode 100644 arch/openrisc/configs/simple_smp_defconfigarch/openrisc/boot/dts/simple_smp.dts new file +0
-0
arch/openrisc/boot/dts/simple_smp.dts line 10
Quoted 10 lines
arch/openrisc/boot/dts/simple_smp.dts new file +10
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim"; + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <&pic>; + + chosen { + bootargs = "console=uart,mmio,0x90000000,115200"; + };
Any reason this isn't using stdout-path?
Quoted 10 lines
arch/openrisc/boot/dts/simple_smp.dts new file +10
-0+ + memory@0 { + device_type = "memory"; + reg = <0x00000000 0x02000000>; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + cpu@0 {
arch/openrisc/boot/dts/simple_smp.dts line 30
Quoted 10 lines
arch/openrisc/boot/dts/simple_smp.dts new file +10
-0+ compatible = "opencores,or1200-rtlsvn481"; + reg = <0>; + clock-frequency = <20000000>; + }; + cpu@1 { + compatible = "opencores,or1200-rtlsvn481"; + reg = <1>; + clock-frequency = <20000000>; + }; + };
No enable-method or similar?
Is your SMP bringup/teardown architected?
arch/openrisc/boot/dts/simple_smp.dts line 33
Quoted 3 lines
arch/openrisc/boot/dts/simple_smp.dts new file +3
-0+ + ompic: ompic { + compatible = "ompic";
This needs a vendor prefix.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Stafford Horne
maintainerRe: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
Quoted 28 lines · Mark Rutland
Quoted 27 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
Simple enough to be compatible with simulation environments, such as verilated systems, QEMU and other targets supporting OpenRISC SMP. This also supports our base FPGA SoC's if the cpu frequency is upped to 50Mhz.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: Added defconfig]Signed-off-by Stafford Horne <[email protected]>--- arch/openrisc/boot/dts/simple_smp.dts | 58 ++++++++++++++++++++++++++ arch/openrisc/configs/simple_smp_defconfig | 66 ++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 arch/openrisc/boot/dts/simple_smp.dts create mode 100644 arch/openrisc/configs/simple_smp_defconfigarch/openrisc/boot/dts/simple_smp.dts new file +4
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim"; + #address-cells = <1>;
Quoted 8 lines
arch/openrisc/boot/dts/simple_smp.dts line 10
Quoted 6 lines
arch/openrisc/boot/dts/simple_smp.dts new file +6
-0+ #size-cells = <1>; + interrupt-parent = <&pic>; + + chosen { + bootargs = "console=uart,mmio,0x90000000,115200"; + };Any reason this isn't using stdout-path?
I didn't know about stdout-path. I will add it.
Quoted 23 lines
Quoted 10 lines
arch/openrisc/boot/dts/simple_smp.dts new file +10
-0+ + memory@0 { + device_type = "memory"; + reg = <0x00000000 0x02000000>; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + cpu@0 {arch/openrisc/boot/dts/simple_smp.dts line 30
Quoted 10 lines
arch/openrisc/boot/dts/simple_smp.dts new file +10
-0+ compatible = "opencores,or1200-rtlsvn481"; + reg = <0>; + clock-frequency = <20000000>; + }; + cpu@1 { + compatible = "opencores,or1200-rtlsvn481"; + reg = <1>; + clock-frequency = <20000000>; + }; + };No enable-method or similar?
Quoted 1 line
Is your SMP bringup/teardown architected?
There is no configurable enable-method yet. The current SMP bringup method is described in the architecture manual (SMP still under review). If you have any pointers that would be great. See section 10.4
https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf
In brief, Currently for SMP bringup both CPU's are expected to reset directly to the startup vectors. The main CPU (coreid == 0) will start initialization where the secondary cpu's either spin or sleep until signalled by the main CPU.
Original patch had cpu's spinning, but after [8/13] secondary cpus will go into Doze mode.
Quoted 6 lines
arch/openrisc/boot/dts/simple_smp.dts line 33
Quoted 4 lines
arch/openrisc/boot/dts/simple_smp.dts new file +3
-0+ + ompic: ompic { + compatible = "ompic";This needs a vendor prefix.
This has also been brought up on [5/13], the question of vendor was anticipated and I discussed with Stefan before sending the patch. OpenRISC is not part of opencores any longer and this ompic was definitely not implemented with an existing vendor in mind.
Perhaps we can just call it openrisc,ompic. I will comment more on the other thread.
Thanks for the review.
-Stafford
Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
In reply to Stafford HorneQuoted 23 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
Simple enough to be compatible with simulation environments, such as verilated systems, QEMU and other targets supporting OpenRISC SMP. This also supports our base FPGA SoC's if the cpu frequency is upped to 50Mhz.
Signed-off-by Stefan Kristiansson <[email protected]>[[email protected]: Added defconfig]Signed-off-by Stafford Horne <[email protected]>--- arch/openrisc/boot/dts/simple_smp.dts | 58 ++++++++++++++++++++++++++ arch/openrisc/configs/simple_smp_defconfig | 66 ++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 arch/openrisc/boot/dts/simple_smp.dts create mode 100644 arch/openrisc/configs/simple_smp_defconfigarch/openrisc/boot/dts/simple_smp.dts new file +0
-0
arch/openrisc/boot/dts/simple_smp.dts line 3
Quoted 3 lines
arch/openrisc/boot/dts/simple_smp.dts new file +3
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim";
You may want to add some comment on top, explaining what this is... and perhaps link to some page documenting how to set up qemu/FPGAs?
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Stafford Horne
maintainerRe: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
Quoted 3 lines · Pavel Machek
Quoted 2 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
[...]
Quoted 9 lines
arch/openrisc/boot/dts/simple_smp.dts line 3
Quoted 5 lines
+++
b/arch/openrisc/boot/dts/simple_smp.dtsarch/openrisc/boot/dts/simple_smp.dts new file +3
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim";You may want to add some comment on top, explaining what this is... and perhaps link to some page documenting how to set up qemu/FPGAs?
Sure, I can create an entry in Documentation/devicetree/binding documenting this since its been there for a while but was never properly documented.
But, just to be clear, what do you mean by page? Are you suggesting linking to a web page? Or do you mean something in Documentation/...?
-Stafford
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
Quoted 15 lines · Stafford Horne
Quoted 3 lines · Pavel Machek
Quoted 2 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
[...]
Quoted 9 lines
arch/openrisc/boot/dts/simple_smp.dts line 3
Quoted 5 lines
+++
b/arch/openrisc/boot/dts/simple_smp.dtsarch/openrisc/boot/dts/simple_smp.dts new file +3
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim";You may want to add some comment on top, explaining what this is... and perhaps link to some page documenting how to set up qemu/FPGAs?
Quoted 3 lines
Sure, I can create an entry in
Documentation/devicetree/bindingdocumenting this since its been there for a while but was never properly documented.
Yep, Documentation/devicetree should be mandatory.
Quoted 2 lines
But, just to be clear, what do you mean by page? Are you suggesting linking to a web page? Or do you mean something in
Documentation/...?
Whatever works for you. Maybe Documentation/ would be better...
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Stafford Horne
maintainerRe: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators
Quoted 20 lines · Pavel Machek
Quoted 15 lines · Stafford Horne
Quoted 3 lines · Pavel Machek
Quoted 2 lines · Stafford Horne
From: Stefan Kristiansson <[email protected]>
[...]
Quoted 9 lines
arch/openrisc/boot/dts/simple_smp.dts line 3
Quoted 5 lines
+++
b/arch/openrisc/boot/dts/simple_smp.dtsarch/openrisc/boot/dts/simple_smp.dts new file +3
-0@@ -0,0 +1,58 @@ +/dts-v1/; +/ { + compatible = "opencores,or1ksim";You may want to add some comment on top, explaining what this is... and perhaps link to some page documenting how to set up qemu/FPGAs?
Quoted 3 lines
Sure, I can create an entry in
Documentation/devicetree/bindingdocumenting this since its been there for a while but was never properly documented.
Quoted 6 lines
Yep,
Documentation/devicetreeshould be mandatory.Quoted 2 lines
But, just to be clear, what do you mean by page? Are you suggesting linking to a web page? Or do you mean something in
Documentation/...?Whatever works for you. Maybe
Documentation/would be better...
Actually, I forgot, we have something but its here:
arch/openrisc/README.openrisc
I'll work on some patches to move that to Documentation/ and clean it up.
-Stafford