Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1723688 > unrolled thread
| Started by | Stafford Horne <shorne@gmail.com> |
|---|---|
| First post | 2017-08-31 00:00 +0200 |
| Last post | 2017-08-31 00:10 +0200 |
| Articles | 20 on this page of 29 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH 00/13] OpenRISC SMP Support Stafford Horne <shorne@gmail.com> - 2017-08-31 00:00 +0200
[PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 13/13] openrisc: add tick timer multicore sync logic Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 12/13] openrisc: enable LOCKDEP_SUPPORT and irqflags tracing Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators Mark Rutland <mark.rutland@arm.com> - 2017-08-31 12:50 +0200
Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators Stafford Horne <shorne@gmail.com> - 2017-08-31 15:10 +0200
[PATCH 09/13] openrisc: add cacheflush support to fix icache aliasing Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 07/13] openrisc: fix initial preempt state for secondary cpu tasks Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 04/13] openrisc: use qspinlocks and qrwlocks Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 01/13] openrisc: use shadow registers to save regs on exception Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-01 10:10 +0200
Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception Stafford Horne <shorne@gmail.com> - 2017-09-01 10:30 +0200
Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-01 10:10 +0200
[PATCH 05/13] irqchip: add initial support for ompic Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Marc Zyngier <marc.zyngier@arm.com> - 2017-08-31 11:30 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Stafford Horne <shorne@gmail.com> - 2017-09-01 03:30 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Marc Zyngier <marc.zyngier@arm.com> - 2017-09-01 19:30 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Stafford Horne <shorne@gmail.com> - 2017-09-04 00:20 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Marc Zyngier <marc.zyngier@arm.com> - 2017-09-04 09:40 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Mark Rutland <mark.rutland@arm.com> - 2017-08-31 13:10 +0200
Re: [PATCH 05/13] irqchip: add initial support for ompic Stafford Horne <shorne@gmail.com> - 2017-09-01 16:00 +0200
[PATCH 03/13] openrisc: add 1 and 2 byte cmpxchg support Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Re: [PATCH 03/13] openrisc: add 1 and 2 byte cmpxchg support Peter Zijlstra <peterz@infradead.org> - 2017-08-31 09:50 +0200
Re: [PATCH 03/13] openrisc: add 1 and 2 byte cmpxchg support Stafford Horne <shorne@gmail.com> - 2017-08-31 11:10 +0200
[PATCH 08/13] openrisc: sleep instead of spin on secondary wait Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
[PATCH 02/13] openrisc: define CPU_BIG_ENDIAN as true Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Re: [OpenRISC] [PATCH 02/13] openrisc: define CPU_BIG_ENDIAN as true Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-01 10:10 +0200
Re: [OpenRISC] [PATCH 02/13] openrisc: define CPU_BIG_ENDIAN as true Stafford Horne <shorne@gmail.com> - 2017-09-01 10:30 +0200
[PATCH 06/13] openrisc: initial SMP support Stafford Horne <shorne@gmail.com> - 2017-08-31 00:10 +0200
Page 1 of 2 [1] 2 Next page →
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:00 +0200 |
| Subject | [PATCH 00/13] OpenRISC SMP Support |
| Message-ID | <ukjbI-5jw-5@gated-at.bofh.it> |
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
--
2.13.5
[toc] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators |
| Message-ID | <ukjln-5CT-7@gated-at.bofh.it> |
| In reply to | #1723688 |
From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
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 <stefan.kristiansson@saunalahti.fi>
[shorne@gmail.com: Added defconfig]
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
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
diff --git a/arch/openrisc/boot/dts/simple_smp.dts b/arch/openrisc/boot/dts/simple_smp.dts
new file mode 100644
index 000000000000..47c54101baae
--- /dev/null
+++ b/arch/openrisc/boot/dts/simple_smp.dts
@@ -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>;
+ };
+
+};
diff --git a/arch/openrisc/configs/simple_smp_defconfig b/arch/openrisc/configs/simple_smp_defconfig
new file mode 100644
index 000000000000..b6e3c7e158e7
--- /dev/null
+++ b/arch/openrisc/configs/simple_smp_defconfig
@@ -0,0 +1,66 @@
+CONFIG_CROSS_COMPILE="or1k-linux-"
+CONFIG_LOCALVERSION="-simple-smp"
+CONFIG_NO_HZ=y
+CONFIG_LOG_BUF_SHIFT=14
+CONFIG_BLK_DEV_INITRD=y
+# CONFIG_RD_GZIP is not set
+# CONFIG_RD_BZIP2 is not set
+# CONFIG_RD_LZMA is not set
+# CONFIG_RD_XZ is not set
+# CONFIG_RD_LZO is not set
+# CONFIG_RD_LZ4 is not set
+CONFIG_EXPERT=y
+# CONFIG_KALLSYMS is not set
+# CONFIG_EPOLL is not set
+# CONFIG_TIMERFD is not set
+# CONFIG_EVENTFD is not set
+# CONFIG_AIO is not set
+# CONFIG_VM_EVENT_COUNTERS is not set
+# CONFIG_COMPAT_BRK is not set
+CONFIG_SLOB=y
+CONFIG_MODULES=y
+# CONFIG_BLOCK is not set
+CONFIG_OPENRISC_BUILTIN_DTB="simple_smp"
+CONFIG_SMP=y
+CONFIG_HZ_100=y
+CONFIG_OPENRISC_HAVE_SHADOW_GPRS=y
+CONFIG_NET=y
+CONFIG_PACKET=y
+CONFIG_UNIX=y
+CONFIG_INET=y
+# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
+# CONFIG_INET_XFRM_MODE_TUNNEL is not set
+# CONFIG_INET_XFRM_MODE_BEET is not set
+# CONFIG_INET_DIAG is not set
+CONFIG_TCP_CONG_ADVANCED=y
+# CONFIG_TCP_CONG_BIC is not set
+# CONFIG_TCP_CONG_CUBIC is not set
+# CONFIG_TCP_CONG_WESTWOOD is not set
+# CONFIG_TCP_CONG_HTCP is not set
+# CONFIG_IPV6 is not set
+# CONFIG_WIRELESS is not set
+CONFIG_DEVTMPFS=y
+CONFIG_DEVTMPFS_MOUNT=y
+# CONFIG_PREVENT_FIRMWARE_BUILD is not set
+# CONFIG_FW_LOADER is not set
+CONFIG_NETDEVICES=y
+CONFIG_ETHOC=y
+CONFIG_MICREL_PHY=y
+# CONFIG_WLAN is not set
+# CONFIG_INPUT is not set
+# CONFIG_SERIO is not set
+# CONFIG_VT is not set
+# CONFIG_LEGACY_PTYS is not set
+CONFIG_SERIAL_8250=y
+CONFIG_SERIAL_8250_CONSOLE=y
+CONFIG_SERIAL_OF_PLATFORM=y
+# CONFIG_HW_RANDOM is not set
+# CONFIG_HWMON is not set
+# CONFIG_USB_SUPPORT is not set
+# CONFIG_DNOTIFY is not set
+CONFIG_TMPFS=y
+CONFIG_NFS_FS=y
+CONFIG_XZ_DEC=y
+# CONFIG_ENABLE_WARN_DEPRECATED is not set
+# CONFIG_ENABLE_MUST_CHECK is not set
+# CONFIG_RCU_TRACE is not set
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 13/13] openrisc: add tick timer multicore sync logic |
| Message-ID | <ukjlo-5CT-33@gated-at.bofh.it> |
| In reply to | #1723691 |
In 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.
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
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
diff --git a/arch/openrisc/include/asm/or1k-timer.h b/arch/openrisc/include/asm/or1k-timer.h
new file mode 100644
index 000000000000..744341b22336
--- /dev/null
+++ b/arch/openrisc/include/asm/or1k-timer.h
@@ -0,0 +1,27 @@
+/*
+ * OpenRISC timer API
+ *
+ * Copyright (C) 2008 by Ralf Baechle (ralf@linux-mips.org)
+ * Copyright (C) 2017 by Stafford Horne (shorne@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 */
diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
index b4b51a07016a..9028e5a1fdd7 100644
--- a/arch/openrisc/kernel/Makefile
+++ b/arch/openrisc/kernel/Makefile
@@ -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
diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index b349d6cb498f..a3f9f2f5d5d9 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -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();
diff --git a/arch/openrisc/kernel/sync-timer.c b/arch/openrisc/kernel/sync-timer.c
new file mode 100644
index 000000000000..7af9f20b7fad
--- /dev/null
+++ b/arch/openrisc/kernel/sync-timer.c
@@ -0,0 +1,120 @@
+/*
+ * OR1K timer synchronisation
+ *
+ * Based on work from MIPS implementation.
+ *
+ * All CPUs will have their count registers synchronised to the CPU0 next time
+ * value. This can cause a small timewarp for CPU0. All other CPU's should
+ * not have done anything significant (but they may have had interrupts
+ * enabled briefly - prom_smp_finish() should not be responsible for enabling
+ * interrupts...)
+ */
+
+#include <linux/kernel.h>
+#include <linux/irqflags.h>
+#include <linux/cpumask.h>
+
+#include <asm/or1k-timer.h>
+#include <asm/timex.h>
+#include <linux/atomic.h>
+#include <asm/barrier.h>
+
+#include <asm/spr.h>
+
+static unsigned int initcount;
+static atomic_t count_count_start = ATOMIC_INIT(0);
+static atomic_t count_count_stop = ATOMIC_INIT(0);
+
+#define COUNTON 100
+#define NR_LOOPS 3
+
+void synchronise_count_master(int cpu)
+{
+ int i;
+ unsigned long flags;
+
+ pr_info("Synchronize counters for CPU %u: ", cpu);
+
+ local_irq_save(flags);
+
+ /*
+ * We loop a few times to get a primed instruction cache,
+ * then the last pass is more or less synchronised and
+ * the master and slaves each set their cycle counters to a known
+ * value all at once. This reduces the chance of having random offsets
+ * between the processors, and guarantees that the maximum
+ * delay between the cycle counters is never bigger than
+ * the latency of information-passing (cachelines) between
+ * two CPUs.
+ */
+
+ for (i = 0; i < NR_LOOPS; i++) {
+ /* slaves loop on '!= 2' */
+ while (atomic_read(&count_count_start) != 1)
+ mb();
+ atomic_set(&count_count_stop, 0);
+ smp_wmb();
+
+ /* Let the slave writes its count register */
+ atomic_inc(&count_count_start);
+
+ /* Count will be initialised to current timer */
+ if (i == 1)
+ initcount = get_cycles();
+
+ /*
+ * Everyone initialises count in the last loop:
+ */
+ if (i == NR_LOOPS-1)
+ openrisc_timer_set(initcount);
+
+ /*
+ * Wait for slave to leave the synchronization point:
+ */
+ while (atomic_read(&count_count_stop) != 1)
+ mb();
+ atomic_set(&count_count_start, 0);
+ smp_wmb();
+ atomic_inc(&count_count_stop);
+ }
+ /* Arrange for an interrupt in a short while */
+ openrisc_timer_set_next(COUNTON);
+
+ local_irq_restore(flags);
+
+ /*
+ * i386 code reported the skew here, but the
+ * count registers were almost certainly out of sync
+ * so no point in alarming people
+ */
+ pr_cont("done.\n");
+}
+
+void synchronise_count_slave(int cpu)
+{
+ int i;
+
+ /*
+ * Not every cpu is online at the time this gets called,
+ * so we first wait for the master to say everyone is ready
+ */
+
+ for (i = 0; i < NR_LOOPS; i++) {
+ atomic_inc(&count_count_start);
+ while (atomic_read(&count_count_start) != 2)
+ mb();
+
+ /*
+ * Everyone initialises count in the last loop:
+ */
+ if (i == NR_LOOPS-1)
+ openrisc_timer_set(initcount);
+
+ atomic_inc(&count_count_stop);
+ while (atomic_read(&count_count_stop) != 2)
+ mb();
+ }
+ /* Arrange for an interrupt in a short while */
+ openrisc_timer_set_next(COUNTON);
+}
+#undef NR_LOOPS
diff --git a/arch/openrisc/kernel/time.c b/arch/openrisc/kernel/time.c
index ab04eaedbf8d..6baecea27080 100644
--- a/arch/openrisc/kernel/time.c
+++ b/arch/openrisc/kernel/time.c
@@ -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;
}
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 12/13] openrisc: enable LOCKDEP_SUPPORT and irqflags tracing |
| Message-ID | <ukjlq-5CT-69@gated-at.bofh.it> |
| In reply to | #1723691 |
Lockdep 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. Signed-off-by: Stafford Horne <shorne@gmail.com> --- arch/openrisc/Kconfig | 3 ++ arch/openrisc/kernel/entry.S | 74 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig index 862b642d380b..09d287db4f25 100644 --- a/arch/openrisc/Kconfig +++ b/arch/openrisc/Kconfig @@ -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" diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S index 1b7160c79646..690d55272ba6 100644 --- a/arch/openrisc/kernel/entry.S +++ b/arch/openrisc/kernel/entry.S @@ -42,6 +42,61 @@ /* =========================================================[ macros ]=== */ +#ifdef CONFIG_TRACE_IRQFLAGS +/* + * Trace irq on/off creating a stack frame. + */ +#define TRACE_IRQS_OP(trace_op) \ + l.sw -8(r1),r2 /* store frame pointer */ ;\ + l.sw -4(r1),r9 /* store return address */ ;\ + l.addi r2,r1,0 /* move sp to fp */ ;\ + l.jal trace_op ;\ + l.addi r1,r1,-8 ;\ + l.ori r1,r2,0 /* restore sp */ ;\ + l.lwz r9,-4(r1) /* restore return address */ ;\ + l.lwz r2,-8(r1) /* restore fp */ ;\ +/* + * Trace irq on/off and save registers we need that would otherwise be + * clobbered. + */ +#define TRACE_IRQS_SAVE(t1,trace_op) \ + l.sw -12(r1),t1 /* save extra reg */ ;\ + l.sw -8(r1),r2 /* store frame pointer */ ;\ + l.sw -4(r1),r9 /* store return address */ ;\ + l.addi r2,r1,0 /* move sp to fp */ ;\ + l.jal trace_op ;\ + l.addi r1,r1,-12 ;\ + l.ori r1,r2,0 /* restore sp */ ;\ + l.lwz r9,-4(r1) /* restore return address */ ;\ + l.lwz r2,-8(r1) /* restore fp */ ;\ + l.lwz t1,-12(r1) /* restore extra reg */ + +#define TRACE_IRQS_OFF TRACE_IRQS_OP(trace_hardirqs_off) +#define TRACE_IRQS_ON TRACE_IRQS_OP(trace_hardirqs_on) +#define TRACE_IRQS_ON_SYSCALL \ + TRACE_IRQS_SAVE(r10,trace_hardirqs_on) ;\ + l.lwz r3,PT_GPR3(r1) ;\ + l.lwz r4,PT_GPR4(r1) ;\ + l.lwz r5,PT_GPR5(r1) ;\ + l.lwz r6,PT_GPR6(r1) ;\ + l.lwz r7,PT_GPR7(r1) ;\ + l.lwz r8,PT_GPR8(r1) ;\ + l.lwz r11,PT_GPR11(r1) +#define TRACE_IRQS_OFF_ENTRY \ + l.lwz r5,PT_SR(r1) ;\ + l.andi r3,r5,(SPR_SR_IEE|SPR_SR_TEE) ;\ + l.sfeq r5,r0 /* skip trace if irqs were already off */;\ + l.bf 1f ;\ + l.nop ;\ + TRACE_IRQS_SAVE(r4,trace_hardirqs_off) ;\ +1: +#else +#define TRACE_IRQS_OFF +#define TRACE_IRQS_ON +#define TRACE_IRQS_OFF_ENTRY +#define TRACE_IRQS_ON_SYSCALL +#endif + /* * We need to disable interrupts at beginning of RESTORE_ALL * since interrupt might come in after we've loaded EPC return address @@ -124,6 +179,7 @@ handler: ;\ /* r30 already save */ ;\ /* l.sw PT_GPR30(r1),r30*/ ;\ l.sw PT_GPR31(r1),r31 ;\ + TRACE_IRQS_OFF_ENTRY ;\ /* Store -1 in orig_gpr11 for non-syscall exceptions */ ;\ l.addi r30,r0,-1 ;\ l.sw PT_ORIG_GPR11(r1),r30 @@ -557,9 +613,6 @@ _string_syscall_return: .align 4 ENTRY(_sys_call_handler) - /* syscalls run with interrupts enabled */ - ENABLE_INTERRUPTS(r29) // enable interrupts, r29 is temp - /* r1, EPCR, ESR a already saved */ l.sw PT_GPR2(r1),r2 /* r3-r8 must be saved because syscall restart relies @@ -597,6 +650,10 @@ ENTRY(_sys_call_handler) /* l.sw PT_GPR30(r1),r30 */ _syscall_check_trace_enter: + /* syscalls run with interrupts enabled */ + TRACE_IRQS_ON_SYSCALL + ENABLE_INTERRUPTS(r29) // enable interrupts, r29 is temp + /* If TIF_SYSCALL_TRACE is set, then we want to do syscall tracing */ l.lwz r30,TI_FLAGS(r10) l.andi r30,r30,_TIF_SYSCALL_TRACE @@ -657,6 +714,7 @@ _syscall_check_trace_leave: _syscall_check_work: /* Here we need to disable interrupts */ DISABLE_INTERRUPTS(r27,r29) + TRACE_IRQS_OFF l.lwz r30,TI_FLAGS(r10) l.andi r30,r30,_TIF_WORK_MASK l.sfne r30,r0 @@ -871,6 +929,7 @@ UNHANDLED_EXCEPTION(_vector_0x1f00,0x1f00) _resume_userspace: DISABLE_INTERRUPTS(r3,r4) + TRACE_IRQS_OFF l.lwz r4,TI_FLAGS(r10) l.andi r13,r4,_TIF_WORK_MASK l.sfeqi r13,0 @@ -909,6 +968,15 @@ _work_pending: l.lwz r8,PT_GPR8(r1) _restore_all: +#ifdef CONFIG_TRACE_IRQFLAGS + l.lwz r4,PT_SR(r1) + l.andi r3,r4,(SPR_SR_IEE|SPR_SR_TEE) + l.sfeq r3,r0 /* skip trace if irqs were off */ + l.bf skip_hardirqs_on + l.nop + TRACE_IRQS_ON +skip_hardirqs_on: +#endif RESTORE_ALL /* This returns to userspace code */ -- 2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-08-31 12:50 +0200 |
| Subject | Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators |
| Message-ID | <ukvcR-4vC-9@gated-at.bofh.it> |
| In reply to | #1723691 |
On Thu, Aug 31, 2017 at 07:03:11AM +0900, Stafford Horne wrote:
> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>
> 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 <stefan.kristiansson@saunalahti.fi>
> [shorne@gmail.com: Added defconfig]
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
> 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
>
> diff --git a/arch/openrisc/boot/dts/simple_smp.dts b/arch/openrisc/boot/dts/simple_smp.dts
> new file mode 100644
> index 000000000000..47c54101baae
> --- /dev/null
> +++ b/arch/openrisc/boot/dts/simple_smp.dts
> @@ -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?
> +
> + 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>;
> + };
> + };
No enable-method or similar?
Is your SMP bringup/teardown architected?
> +
> + ompic: ompic {
> + compatible = "ompic";
This needs a vendor prefix.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 15:10 +0200 |
| Subject | Re: [PATCH 10/13] openrisc: add simple_smp dts and defconfig for simulators |
| Message-ID | <ukxol-62o-7@gated-at.bofh.it> |
| In reply to | #1724092 |
On Thu, Aug 31, 2017 at 11:41:10AM +0100, Mark Rutland wrote:
> On Thu, Aug 31, 2017 at 07:03:11AM +0900, Stafford Horne wrote:
> > From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> >
> > 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 <stefan.kristiansson@saunalahti.fi>
> > [shorne@gmail.com: Added defconfig]
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> > 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
> >
> > diff --git a/arch/openrisc/boot/dts/simple_smp.dts b/arch/openrisc/boot/dts/simple_smp.dts
> > new file mode 100644
> > index 000000000000..47c54101baae
> > --- /dev/null
> > +++ b/arch/openrisc/boot/dts/simple_smp.dts
> > @@ -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?
I didn't know about stdout-path. I will add it.
> > +
> > + 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>;
> > + };
> > + };
>
> No enable-method or similar?
>
> 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.
>
> > +
> > + 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
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 09/13] openrisc: add cacheflush support to fix icache aliasing |
| Message-ID | <ukjln-5CT-11@gated-at.bofh.it> |
| In reply to | #1723688 |
From: Jan Henrik Weinstock <jan.weinstock@ice.rwth-aachen.de>
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.
Signed-off-by: Jan Henrik Weinstock <jan.weinstock@ice.rwth-aachen.de>
[shorne@gmail.com: Squashed patches and wrote commit message]
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
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
diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index b59e2ef84534..9e01dc6e1c0b 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -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 ""
diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild
index 5f066780d870..6eb16719549e 100644
--- a/arch/openrisc/include/asm/Kbuild
+++ b/arch/openrisc/include/asm/Kbuild
@@ -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
diff --git a/arch/openrisc/include/asm/cacheflush.h b/arch/openrisc/include/asm/cacheflush.h
new file mode 100644
index 000000000000..70f46fd7a074
--- /dev/null
+++ b/arch/openrisc/include/asm/cacheflush.h
@@ -0,0 +1,96 @@
+/*
+ * OpenRISC Linux
+ *
+ * Linux architectural port borrowing liberally from similar works of
+ * others. All original copyrights apply as per the original source
+ * declaration.
+ *
+ * OpenRISC implementation:
+ * Copyright (C) Jan Henrik Weinstock <jan.weinstock@rwth-aachen.de>
+ * et al.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __ASM_CACHEFLUSH_H
+#define __ASM_CACHEFLUSH_H
+
+#include <linux/mm.h>
+
+/*
+ * Helper function for flushing or invalidating entire pages from data
+ * and instruction caches. SMP needs a little extra work, since we need
+ * to flush the pages on all cpus.
+ */
+extern void local_dcache_page_flush(struct page *page);
+extern void local_icache_page_inv(struct page *page);
+
+/*
+ * Data cache flushing always happen on the local cpu. Instruction cache
+ * invalidations need to be broadcasted to all other cpu in the system in
+ * case of SMP configurations.
+ */
+#ifndef CONFIG_SMP
+#define dcache_page_flush(page) local_dcache_page_flush(page)
+#define icache_page_inv(page) local_icache_page_inv(page)
+#else /* CONFIG_SMP */
+#define dcache_page_flush(page) local_dcache_page_flush(page)
+#define icache_page_inv(page) smp_icache_page_inv(page)
+extern void smp_icache_page_inv(struct page *page);
+#endif /* CONFIG_SMP */
+
+/*
+ * Synchronizes caches. Whenever a cpu writes executable code to memory, this
+ * should be called to make sure the processor sees the newly written code.
+ */
+static inline void sync_icache_dcache(struct page *page)
+{
+ if (!IS_ENABLED(CONFIG_DCACHE_WRITETHROUGH))
+ dcache_page_flush(page);
+ icache_page_inv(page);
+}
+
+/*
+ * Pages with this bit set need not be flushed/invalidated, since
+ * they have not changed since last flush. New pages start with
+ * PG_arch_1 not set and are therefore dirty by default.
+ */
+#define PG_dc_clean PG_arch_1
+
+#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 1
+static inline void flush_dcache_page(struct page *page)
+{
+ clear_bit(PG_dc_clean, &page->flags);
+}
+
+/*
+ * Other interfaces are not required since we do not have virtually
+ * indexed or tagged caches. So we can use the default here.
+ */
+#define flush_cache_all() do { } while (0)
+#define flush_cache_mm(mm) do { } while (0)
+#define flush_cache_dup_mm(mm) do { } while (0)
+#define flush_cache_range(vma, start, end) do { } while (0)
+#define flush_cache_page(vma, vmaddr, pfn) do { } while (0)
+#define flush_dcache_mmap_lock(mapping) do { } while (0)
+#define flush_dcache_mmap_unlock(mapping) do { } while (0)
+#define flush_icache_range(start, end) do { } while (0)
+#define flush_icache_page(vma, pg) do { } while (0)
+#define flush_icache_user_range(vma, pg, adr, len) do { } while (0)
+#define flush_cache_vmap(start, end) do { } while (0)
+#define flush_cache_vunmap(start, end) do { } while (0)
+
+#define copy_to_user_page(vma, page, vaddr, dst, src, len) \
+ do { \
+ memcpy(dst, src, len); \
+ if (vma->vm_flags & VM_EXEC) \
+ sync_icache_dcache(page); \
+ } while (0)
+
+#define copy_from_user_page(vma, page, vaddr, dst, src, len) \
+ memcpy(dst, src, len)
+
+#endif /* __ASM_CACHEFLUSH_H */
diff --git a/arch/openrisc/include/asm/pgtable.h b/arch/openrisc/include/asm/pgtable.h
index 47eae2f78d2d..471821e4e810 100644
--- a/arch/openrisc/include/asm/pgtable.h
+++ b/arch/openrisc/include/asm/pgtable.h
@@ -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 */
diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index 7696f9274608..b349d6cb498f 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -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);
diff --git a/arch/openrisc/mm/Makefile b/arch/openrisc/mm/Makefile
index 324ba2634529..a31b2a42e966 100644
--- a/arch/openrisc/mm/Makefile
+++ b/arch/openrisc/mm/Makefile
@@ -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
diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c
new file mode 100644
index 000000000000..b747bf1fc1b6
--- /dev/null
+++ b/arch/openrisc/mm/cache.c
@@ -0,0 +1,61 @@
+/*
+ * OpenRISC cache.c
+ *
+ * Linux architectural port borrowing liberally from similar works of
+ * others. All original copyrights apply as per the original source
+ * declaration.
+ *
+ * Modifications for the OpenRISC architecture:
+ * Copyright (C) 2015 Jan Henrik Weinstock <jan.weinstock@rwth-aachen.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/spr.h>
+#include <asm/spr_defs.h>
+#include <asm/cache.h>
+#include <asm/cacheflush.h>
+#include <asm/tlbflush.h>
+
+static void cache_loop(struct page *page, const unsigned int reg)
+{
+ unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
+ unsigned long line = paddr & ~(L1_CACHE_BYTES - 1);
+
+ while (line < paddr + PAGE_SIZE) {
+ mtspr(reg, line);
+ line += L1_CACHE_BYTES;
+ }
+}
+
+void local_dcache_page_flush(struct page *page)
+{
+ cache_loop(page, SPR_DCBFR);
+}
+EXPORT_SYMBOL(local_dcache_page_flush);
+
+void local_icache_page_inv(struct page *page)
+{
+ cache_loop(page, SPR_ICBIR);
+}
+EXPORT_SYMBOL(local_icache_page_inv);
+
+void update_cache(struct vm_area_struct *vma, unsigned long address,
+ pte_t *pte)
+{
+ unsigned long pfn = pte_val(*pte) >> PAGE_SHIFT;
+ struct page *page = pfn_to_page(pfn);
+ int dirty = !test_and_set_bit(PG_dc_clean, &page->flags);
+
+ /*
+ * Since icaches do not snoop for updated data on OpenRISC, we
+ * must write back and invalidate any dirty pages manually. We
+ * can skip data pages, since they will not end up in icaches.
+ */
+ if ((vma->vm_flags & VM_EXEC) && dirty)
+ sync_icache_dcache(page);
+}
+
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 07/13] openrisc: fix initial preempt state for secondary cpu tasks |
| Message-ID | <ukjlp-5CT-47@gated-at.bofh.it> |
| In reply to | #1723688 |
During 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.
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
arch/openrisc/include/asm/thread_info.h | 2 +-
arch/openrisc/kernel/smp.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/openrisc/include/asm/thread_info.h b/arch/openrisc/include/asm/thread_info.h
index 6e619a79a401..c229aa6bb502 100644
--- a/arch/openrisc/include/asm/thread_info.h
+++ b/arch/openrisc/include/asm/thread_info.h
@@ -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, \
}
diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index ae93b84062ff..24f7aa45b0ba 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -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
*/
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 04/13] openrisc: use qspinlocks and qrwlocks |
| Message-ID | <ukjlp-5CT-53@gated-at.bofh.it> |
| In reply to | #1723688 |
Enable OpenRISC to use qspinlocks and qrwlocks for upcoming SMP support. Signed-off-by: Stafford Horne <shorne@gmail.com> --- 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 diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig index 8ddfc92ab813..214c837ce597 100644 --- a/arch/openrisc/Kconfig +++ b/arch/openrisc/Kconfig @@ -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 diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild index 5bea416a7792..5f066780d870 100644 --- a/arch/openrisc/include/asm/Kbuild +++ b/arch/openrisc/include/asm/Kbuild @@ -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 diff --git a/arch/openrisc/include/asm/spinlock.h b/arch/openrisc/include/asm/spinlock.h index fd00a3a24123..9b761e0e22c3 100644 --- a/arch/openrisc/include/asm/spinlock.h +++ b/arch/openrisc/include/asm/spinlock.h @@ -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 diff --git a/arch/openrisc/include/asm/spinlock_types.h b/arch/openrisc/include/asm/spinlock_types.h new file mode 100644 index 000000000000..7c6fb1208c88 --- /dev/null +++ b/arch/openrisc/include/asm/spinlock_types.h @@ -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 */ -- 2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 01/13] openrisc: use shadow registers to save regs on exception |
| Message-ID | <ukjlp-5CT-55@gated-at.bofh.it> |
| In reply to | #1723688 |
From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
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.
Signed-off-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
arch/openrisc/Kconfig | 10 +++++
arch/openrisc/kernel/head.S | 95 ++++++++++++++++++++++++++++++++-------------
2 files changed, 79 insertions(+), 26 deletions(-)
diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index 1e95920b0737..ba8ae92ec5fb 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -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 ""
diff --git a/arch/openrisc/kernel/head.S b/arch/openrisc/kernel/head.S
index 1e87913576e3..1e49895408f4 100644
--- a/arch/openrisc/kernel/head.S
+++ b/arch/openrisc/kernel/head.S
@@ -49,9 +49,31 @@
/* ============================================[ tmp store locations ]=== */
+#define SPR_SHADOW_GPR(x) ((x) + SPR_GPR_BASE + 32)
+
/*
* emergency_print temporary stores
*/
+#ifdef CONFIG_OPENRISC_HAVE_SHADOW_GPRS
+#define EMERGENCY_PRINT_STORE_GPR4 l.mtspr r0,r4,SPR_SHADOW_GPR(14)
+#define EMERGENCY_PRINT_LOAD_GPR4 l.mfspr r4,r0,SPR_SHADOW_GPR(14)
+
+#define EMERGENCY_PRINT_STORE_GPR5 l.mtspr r0,r5,SPR_SHADOW_GPR(15)
+#define EMERGENCY_PRINT_LOAD_GPR5 l.mfspr r5,r0,SPR_SHADOW_GPR(15)
+
+#define EMERGENCY_PRINT_STORE_GPR6 l.mtspr r0,r6,SPR_SHADOW_GPR(16)
+#define EMERGENCY_PRINT_LOAD_GPR6 l.mfspr r6,r0,SPR_SHADOW_GPR(16)
+
+#define EMERGENCY_PRINT_STORE_GPR7 l.mtspr r0,r7,SPR_SHADOW_GPR(7)
+#define EMERGENCY_PRINT_LOAD_GPR7 l.mfspr r7,r0,SPR_SHADOW_GPR(7)
+
+#define EMERGENCY_PRINT_STORE_GPR8 l.mtspr r0,r8,SPR_SHADOW_GPR(8)
+#define EMERGENCY_PRINT_LOAD_GPR8 l.mfspr r8,r0,SPR_SHADOW_GPR(8)
+
+#define EMERGENCY_PRINT_STORE_GPR9 l.mtspr r0,r9,SPR_SHADOW_GPR(9)
+#define EMERGENCY_PRINT_LOAD_GPR9 l.mfspr r9,r0,SPR_SHADOW_GPR(9)
+
+#else /* !CONFIG_OPENRISC_HAVE_SHADOW_GPRS */
#define EMERGENCY_PRINT_STORE_GPR4 l.sw 0x20(r0),r4
#define EMERGENCY_PRINT_LOAD_GPR4 l.lwz r4,0x20(r0)
@@ -70,13 +92,28 @@
#define EMERGENCY_PRINT_STORE_GPR9 l.sw 0x34(r0),r9
#define EMERGENCY_PRINT_LOAD_GPR9 l.lwz r9,0x34(r0)
+#endif
/*
* TLB miss handlers temorary stores
*/
-#define EXCEPTION_STORE_GPR9 l.sw 0x10(r0),r9
-#define EXCEPTION_LOAD_GPR9 l.lwz r9,0x10(r0)
+#ifdef CONFIG_OPENRISC_HAVE_SHADOW_GPRS
+#define EXCEPTION_STORE_GPR2 l.mtspr r0,r2,SPR_SHADOW_GPR(2)
+#define EXCEPTION_LOAD_GPR2 l.mfspr r2,r0,SPR_SHADOW_GPR(2)
+
+#define EXCEPTION_STORE_GPR3 l.mtspr r0,r3,SPR_SHADOW_GPR(3)
+#define EXCEPTION_LOAD_GPR3 l.mfspr r3,r0,SPR_SHADOW_GPR(3)
+#define EXCEPTION_STORE_GPR4 l.mtspr r0,r4,SPR_SHADOW_GPR(4)
+#define EXCEPTION_LOAD_GPR4 l.mfspr r4,r0,SPR_SHADOW_GPR(4)
+
+#define EXCEPTION_STORE_GPR5 l.mtspr r0,r5,SPR_SHADOW_GPR(5)
+#define EXCEPTION_LOAD_GPR5 l.mfspr r5,r0,SPR_SHADOW_GPR(5)
+
+#define EXCEPTION_STORE_GPR6 l.mtspr r0,r6,SPR_SHADOW_GPR(6)
+#define EXCEPTION_LOAD_GPR6 l.mfspr r6,r0,SPR_SHADOW_GPR(6)
+
+#else /* !CONFIG_OPENRISC_HAVE_SHADOW_GPRS */
#define EXCEPTION_STORE_GPR2 l.sw 0x64(r0),r2
#define EXCEPTION_LOAD_GPR2 l.lwz r2,0x64(r0)
@@ -92,26 +129,32 @@
#define EXCEPTION_STORE_GPR6 l.sw 0x74(r0),r6
#define EXCEPTION_LOAD_GPR6 l.lwz r6,0x74(r0)
+#endif
/*
* EXCEPTION_HANDLE temporary stores
*/
+#ifdef CONFIG_OPENRISC_HAVE_SHADOW_GPRS
+#define EXCEPTION_T_STORE_GPR30 l.mtspr r0,r30,SPR_SHADOW_GPR(30)
+#define EXCEPTION_T_LOAD_GPR30(reg) l.mfspr reg,r0,SPR_SHADOW_GPR(30)
+
+#define EXCEPTION_T_STORE_GPR10 l.mtspr r0,r10,SPR_SHADOW_GPR(10)
+#define EXCEPTION_T_LOAD_GPR10(reg) l.mfspr reg,r0,SPR_SHADOW_GPR(10)
+
+#define EXCEPTION_T_STORE_SP l.mtspr r0,r1,SPR_SHADOW_GPR(1)
+#define EXCEPTION_T_LOAD_SP(reg) l.mfspr reg,r0,SPR_SHADOW_GPR(1)
+
+#else /* !CONFIG_OPENRISC_HAVE_SHADOW_GPRS */
#define EXCEPTION_T_STORE_GPR30 l.sw 0x78(r0),r30
#define EXCEPTION_T_LOAD_GPR30(reg) l.lwz reg,0x78(r0)
#define EXCEPTION_T_STORE_GPR10 l.sw 0x7c(r0),r10
#define EXCEPTION_T_LOAD_GPR10(reg) l.lwz reg,0x7c(r0)
-#define EXCEPTION_T_STORE_SP l.sw 0x80(r0),r1
+#define EXCEPTION_T_STORE_SP l.sw 0x80(r0),r1
#define EXCEPTION_T_LOAD_SP(reg) l.lwz reg,0x80(r0)
-
-/*
- * For UNHANLDED_EXCEPTION
- */
-
-#define EXCEPTION_T_STORE_GPR31 l.sw 0x84(r0),r31
-#define EXCEPTION_T_LOAD_GPR31(reg) l.lwz reg,0x84(r0)
+#endif
/* =========================================================[ macros ]=== */
@@ -226,7 +269,7 @@
*
*/
#define UNHANDLED_EXCEPTION(handler) \
- EXCEPTION_T_STORE_GPR31 ;\
+ EXCEPTION_T_STORE_GPR30 ;\
EXCEPTION_T_STORE_GPR10 ;\
EXCEPTION_T_STORE_SP ;\
/* temporary store r3, r9 into r1, r10 */ ;\
@@ -255,35 +298,35 @@
/* r1: KSP, r10: current, r31: __pa(KSP) */ ;\
/* r12: temp, syscall indicator, r13 temp */ ;\
l.addi r1,r1,-(INT_FRAME_SIZE) ;\
- /* r1 is KSP, r31 is __pa(KSP) */ ;\
- tophys (r31,r1) ;\
- l.sw PT_GPR12(r31),r12 ;\
+ /* r1 is KSP, r30 is __pa(KSP) */ ;\
+ tophys (r30,r1) ;\
+ l.sw PT_GPR12(r30),r12 ;\
l.mfspr r12,r0,SPR_EPCR_BASE ;\
- l.sw PT_PC(r31),r12 ;\
+ l.sw PT_PC(r30),r12 ;\
l.mfspr r12,r0,SPR_ESR_BASE ;\
- l.sw PT_SR(r31),r12 ;\
+ l.sw PT_SR(r30),r12 ;\
/* save r31 */ ;\
- EXCEPTION_T_LOAD_GPR31(r12) ;\
- l.sw PT_GPR31(r31),r12 ;\
+ EXCEPTION_T_LOAD_GPR30(r12) ;\
+ l.sw PT_GPR30(r30),r12 ;\
/* save r10 as was prior to exception */ ;\
EXCEPTION_T_LOAD_GPR10(r12) ;\
- l.sw PT_GPR10(r31),r12 ;\
+ l.sw PT_GPR10(r30),r12 ;\
/* save PT_SP as was prior to exception */ ;\
EXCEPTION_T_LOAD_SP(r12) ;\
- l.sw PT_SP(r31),r12 ;\
- l.sw PT_GPR13(r31),r13 ;\
+ l.sw PT_SP(r30),r12 ;\
+ l.sw PT_GPR13(r30),r13 ;\
/* --> */ ;\
/* save exception r4, set r4 = EA */ ;\
- l.sw PT_GPR4(r31),r4 ;\
+ l.sw PT_GPR4(r30),r4 ;\
l.mfspr r4,r0,SPR_EEAR_BASE ;\
/* r12 == 1 if we come from syscall */ ;\
CLEAR_GPR(r12) ;\
/* ----- play a MMU trick ----- */ ;\
- l.ori r31,r0,(EXCEPTION_SR) ;\
- l.mtspr r0,r31,SPR_ESR_BASE ;\
+ l.ori r30,r0,(EXCEPTION_SR) ;\
+ l.mtspr r0,r30,SPR_ESR_BASE ;\
/* r31: EA address of handler */ ;\
- LOAD_SYMBOL_2_GPR(r31,handler) ;\
- l.mtspr r0,r31,SPR_EPCR_BASE ;\
+ LOAD_SYMBOL_2_GPR(r30,handler) ;\
+ l.mtspr r0,r30,SPR_EPCR_BASE ;\
l.rfe
/* =====================================================[ exceptions] === */
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Geert Uytterhoeven <geert@linux-m68k.org> |
|---|---|
| Date | 2017-09-01 10:10 +0200 |
| Subject | Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception |
| Message-ID | <ukPbA-1cR-1@gated-at.bofh.it> |
| In reply to | #1723698 |
On Fri, Sep 1, 2017 at 10:02 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Aug 30, 2017 at 11:58 PM, Stafford Horne <shorne@gmail.com> wrote:
>> --- a/arch/openrisc/Kconfig
>> +++ b/arch/openrisc/Kconfig
>> @@ -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
>> + 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@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
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-09-01 10:30 +0200 |
| Subject | Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception |
| Message-ID | <ukPuX-1kd-25@gated-at.bofh.it> |
| In reply to | #1724863 |
On Fri, Sep 01, 2017 at 10:03:44AM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 1, 2017 at 10:02 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Aug 30, 2017 at 11:58 PM, Stafford Horne <shorne@gmail.com> wrote:
> >> --- a/arch/openrisc/Kconfig
> >> +++ b/arch/openrisc/Kconfig
> >> @@ -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
Thanks yes, the help text says its mandatory. That should be forced as you
showed.
-Stafford
> >> + 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@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
[toc] | [prev] | [next] | [standalone]
| From | Geert Uytterhoeven <geert@linux-m68k.org> |
|---|---|
| Date | 2017-09-01 10:10 +0200 |
| Subject | Re: [OpenRISC] [PATCH 01/13] openrisc: use shadow registers to save regs on exception |
| Message-ID | <ukPbA-1cR-3@gated-at.bofh.it> |
| In reply to | #1723698 |
On Wed, Aug 30, 2017 at 11:58 PM, Stafford Horne <shorne@gmail.com> wrote:
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -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?
> + 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@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
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-08-31 00:10 +0200 |
| Subject | [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ukjlp-5CT-61@gated-at.bofh.it> |
| In reply to | #1723688 |
From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
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 <stefan.kristiansson@saunalahti.fi>
[shorne@gmail.com: converted ops to bitmask, wrote commit message]
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
.../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
diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
new file mode 100644
index 000000000000..4176ecc3366d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
@@ -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>;
+};
diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index 214c837ce597..dd7e55e7e42d 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -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
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..3fa60e6667a7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -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
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..123047d7a20d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -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
diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c
new file mode 100644
index 000000000000..438819f8a5a7
--- /dev/null
+++ b/drivers/irqchip/irq-ompic.c
@@ -0,0 +1,117 @@
+/*
+ * Open Multi-Processor Interrupt Controller driver
+ *
+ * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
+ *
+ * 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.
+ */
+
+#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>
+#include <linux/delay.h>
+
+#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)
+
+#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];
+
+static void __iomem *ompic_base;
+
+static inline u32 ompic_readreg(void __iomem *base, loff_t offset)
+{
+ return ioread32be(base + offset);
+}
+
+static void ompic_writereg(void __iomem *base, loff_t offset, u32 data)
+{
+ iowrite32be(data, base + offset);
+}
+
+#ifdef CONFIG_SMP
+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));
+ }
+}
+#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) {
+ do {
+ unsigned long ipi;
+
+ ipi = ops & -ops;
+ ops &= ~ipi;
+ ipi = __ffs(ipi);
+
+ handle_IPI(ipi);
+ } while (ops);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static struct irqaction ompi_ipi_irqaction = {
+ .handler = ompic_ipi_handler,
+ .flags = IRQF_PERCPU,
+ .name = "ompic_ipi",
+};
+
+#ifdef CONFIG_OF
+int __init ompic_of_init(struct device_node *node, struct device_node *parent)
+{
+ int irq;
+
+ if (WARN_ON(!node))
+ return -ENODEV;
+
+ memset(ipi_data, 0, sizeof(ipi_data));
+
+ ompic_base = of_iomap(node, 0);
+
+ irq = irq_of_parse_and_map(node, 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
--
2.13.5
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2017-08-31 11:30 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <uktXs-3NS-17@gated-at.bofh.it> |
| In reply to | #1723700 |
On 30/08/17 22:58, Stafford Horne wrote:
> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>
> 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 <stefan.kristiansson@saunalahti.fi>
> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
> .../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
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> new file mode 100644
> index 000000000000..4176ecc3366d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> @@ -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>;
> +};
> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> index 214c837ce597..dd7e55e7e42d 100644
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -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
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f1fd5f44d1d4..3fa60e6667a7 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -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...
> +
> config OR1K_PIC
> bool
> select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..123047d7a20d 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -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
> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c
> new file mode 100644
> index 000000000000..438819f8a5a7
> --- /dev/null
> +++ b/drivers/irqchip/irq-ompic.c
> @@ -0,0 +1,117 @@
> +/*
> + * Open Multi-Processor Interrupt Controller driver
> + *
> + * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> + *
> + * 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.
> + */
> +
> +#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.
> +#include <linux/delay.h>
Nor this.
> +
> +#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?).
> +
> +#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.
> +
> +static void __iomem *ompic_base;
> +
> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)
> +{
> + 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.
> +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?
> + 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.
> + }
> +}
> +#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?
> + 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?
> +
> + handle_IPI(ipi);
> + } while (ops);
> + }
> +
> + 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.
> +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?
> +
> + memset(ipi_data, 0, sizeof(ipi_data));
The kernel should do this for you already.
> +
> + ompic_base = of_iomap(node, 0);
> +
> + irq = irq_of_parse_and_map(node, 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.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-09-01 03:30 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ukIWt-5ac-7@gated-at.bofh.it> |
| In reply to | #1724029 |
On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:
> On 30/08/17 22:58, Stafford Horne wrote:
> > From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> >
> > 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 <stefan.kristiansson@saunalahti.fi>
> > [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> > .../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
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> > new file mode 100644
> > index 000000000000..4176ecc3366d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> > @@ -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>;
> > +};
> > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> > index 214c837ce597..dd7e55e7e42d 100644
> > --- a/arch/openrisc/Kconfig
> > +++ b/arch/openrisc/Kconfig
> > @@ -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
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index f1fd5f44d1d4..3fa60e6667a7 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -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...
Right, I will look to remove that.
> > +
> > config OR1K_PIC
> > bool
> > select IRQ_DOMAIN
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index e88d856cc09c..123047d7a20d 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -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
> > diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c
> > new file mode 100644
> > index 000000000000..438819f8a5a7
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-ompic.c
> > @@ -0,0 +1,117 @@
> > +/*
> > + * Open Multi-Processor Interrupt Controller driver
> > + *
> > + * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> > + *
> > + * 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.
> > + */
> > +
> > +#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.
>
> > +#include <linux/delay.h>
>
> Nor this.
OK on both.
> > +
> > +#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?).
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.
> > +
> > +#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.
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;
> > +
> > +static void __iomem *ompic_base;
> > +
> > +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)
> > +{
> > + 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.
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().
> > +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?
> > + 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.
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?
> > + }
> > +}
> > +#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?
Thanks, but some concern of above.
> > + 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?
Right, this should work, not sure what I was thinking above right now. Let
me give that a try.
> > +
> > + handle_IPI(ipi);
> > + } while (ops);
> > + }
> > +
> > + 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.
Thanks, right.
> > +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.
> > +
> > + memset(ipi_data, 0, sizeof(ipi_data));
>
> The kernel should do this for you already.
Right.
> > +
> > + ompic_base = of_iomap(node, 0);
> > +
> > + irq = irq_of_parse_and_map(node, 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
> >
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
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2017-09-01 19:30 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ukXVv-8aN-7@gated-at.bofh.it> |
| In reply to | #1724714 |
On 01/09/17 02:24, Stafford Horne wrote:
> On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:
>> On 30/08/17 22:58, Stafford Horne wrote:
>>> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>>>
>>> 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 <stefan.kristiansson@saunalahti.fi>
>>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
>>> Signed-off-by: Stafford Horne <shorne@gmail.com>
>>> ---
>>> .../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
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
>>> new file mode 100644
>>> index 000000000000..4176ecc3366d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
>>> @@ -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>;
>>> +};
>>> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
>>> index 214c837ce597..dd7e55e7e42d 100644
>>> --- a/arch/openrisc/Kconfig
>>> +++ b/arch/openrisc/Kconfig
>>> @@ -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
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index f1fd5f44d1d4..3fa60e6667a7 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -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...
>
> Right, I will look to remove that.
>
>>> +
>>> config OR1K_PIC
>>> bool
>>> select IRQ_DOMAIN
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index e88d856cc09c..123047d7a20d 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -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
>>> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c
>>> new file mode 100644
>>> index 000000000000..438819f8a5a7
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-ompic.c
>>> @@ -0,0 +1,117 @@
>>> +/*
>>> + * Open Multi-Processor Interrupt Controller driver
>>> + *
>>> + * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#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.
>>
>>> +#include <linux/delay.h>
>>
>> Nor this.
>
> OK on both.
>
>>> +
>>> +#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?).
>
> 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.
>
>>> +
>>> +#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.
>
> 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;
Yup, __cacheline_aligned is key here, as it will have the same effect as
the percpu stuff from that point of view.
>>> +
>>> +static void __iomem *ompic_base;
>>> +
>>> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)
>>> +{
>>> + 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.
>
> 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().
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.
>
>>> +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?
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.
>
>>> + 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.
>
> 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...
> 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.
[...]
> 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.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Stafford Horne <shorne@gmail.com> |
|---|---|
| Date | 2017-09-04 00:20 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ulLpg-5BV-9@gated-at.bofh.it> |
| In reply to | #1725247 |
On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:
> On 01/09/17 02:24, Stafford Horne wrote:
> > On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:
> >> On 30/08/17 22:58, Stafford Horne wrote:
> >>> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> >>>
> >>> 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 <stefan.kristiansson@saunalahti.fi>
> >>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> >>> Signed-off-by: Stafford Horne <shorne@gmail.com>
> >>> ---
> >>> .../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
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> >>> new file mode 100644
> >>> index 000000000000..4176ecc3366d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> >>> @@ -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>;
> >>> +};
> >>> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> >>> index 214c837ce597..dd7e55e7e42d 100644
> >>> --- a/arch/openrisc/Kconfig
> >>> +++ b/arch/openrisc/Kconfig
> >>> @@ -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
> >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> >>> index f1fd5f44d1d4..3fa60e6667a7 100644
> >>> --- a/drivers/irqchip/Kconfig
> >>> +++ b/drivers/irqchip/Kconfig
> >>> @@ -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...
> >
> > Right, I will look to remove that.
> >
> >>> +
> >>> config OR1K_PIC
> >>> bool
> >>> select IRQ_DOMAIN
> >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> >>> index e88d856cc09c..123047d7a20d 100644
> >>> --- a/drivers/irqchip/Makefile
> >>> +++ b/drivers/irqchip/Makefile
> >>> @@ -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
> >>> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c
> >>> new file mode 100644
> >>> index 000000000000..438819f8a5a7
> >>> --- /dev/null
> >>> +++ b/drivers/irqchip/irq-ompic.c
> >>> @@ -0,0 +1,117 @@
> >>> +/*
> >>> + * Open Multi-Processor Interrupt Controller driver
> >>> + *
> >>> + * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> >>> + *
> >>> + * 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.
> >>> + */
> >>> +
> >>> +#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.
> >>
> >>> +#include <linux/delay.h>
> >>
> >> Nor this.
> >
> > OK on both.
> >
> >>> +
> >>> +#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?).
> >
> > 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.
I will update that.
> >
> >>> +
> >>> +#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.
> >
> > 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;
>
> Yup, __cacheline_aligned is 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.
> >>> +
> >>> +static void __iomem *ompic_base;
> >>> +
> >>> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)
> >>> +{
> >>> + 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.
> >
> > 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().
>
> 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.
> >
> >>> +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?
>
> 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.
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.
> >
> >>> + 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.
> >
> > 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...
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
> > 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.
>
> [...]
>
> > 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.
> --
> Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2017-09-04 09:40 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ulU9b-2BO-7@gated-at.bofh.it> |
| In reply to | #1725767 |
On 03/09/17 23:12, Stafford Horne wrote:
> On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:
>> On 01/09/17 02:24, Stafford Horne wrote:
>>> On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:
>>>> On 30/08/17 22:58, Stafford Horne wrote:
>>>>> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
[...]
>>>>> + 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.
>>>
>>> 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...
>
> 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.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-08-31 13:10 +0200 |
| Subject | Re: [PATCH 05/13] irqchip: add initial support for ompic |
| Message-ID | <ukvwd-4SK-1@gated-at.bofh.it> |
| In reply to | #1723700 |
On Thu, Aug 31, 2017 at 06:58:36AM +0900, Stafford Horne wrote:
> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>
> 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?
> 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 <stefan.kristiansson@saunalahti.fi>
> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
> .../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
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> new file mode 100644
> index 000000000000..4176ecc3366d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt
> @@ -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).
> +- 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?
> +- 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?
> +- 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?
> +
> +Example:
> +
> +ompic: ompic {
> + compatible = "ompic";
> + reg = <0x98000000 16>;
> + #interrupt-cells = <1>;
> + interrupt-controller;
> + interrupts = <1>;
> +};
[...]
> +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?
[...]
> +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.
[...]
> +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?
> + memset(ipi_data, 0, sizeof(ipi_data));
As ipi_data was static, it is already zeroed.
> +
> + ompic_base = of_iomap(node, 0);
What if this failed?
> +
> + irq = irq_of_parse_and_map(node, 0);
What if this is missing?
> + 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.
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web