[GIT PULL] OpenRISC updates for 6.10
Hello Linus, Please consider for pull, The following changes since commit 4cece764965020c22cff7665b18a012006359095: Linux 6.9-rc1 (2024-03-24 14:10:05 -0700) are available in the Git repository at: https://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 4dc70e1aadfadf968676d983587c6f5d455aba85: openrisc: Move FPU state out of pt_regs (2024-04-15 15:20:39 +0100) OpenRISC updates for 6.10 A few cleanups and fixups from me: - Add a few missing relocations to fix module loading. - Cleanup FPU state save and restore to be more efficient. - Cleanups to traps handling and logging. - Fix issue with poweroff being broken after recent power driver refactoings. Stafford Horne (8): openrisc: Use do_kernel_power_off() openrisc: Define openrisc relocation types openrisc: Add support for more module relocations openrisc: traps: Convert printks to pr_ macros openrisc: traps: Remove calls to show_registers before die openrisc: traps: Don't send signals to kernel mode threads openrisc: Add FPU config openrisc: Move FPU state out of pt_regs arch/openrisc/Kconfig | 9 +++ arch/openrisc/include/asm/fpu.h | 22 ++ arch/openrisc/include/asm/processor.h | 1 + arch/openrisc/include/asm/ptrace.h| 3 +- arch/openrisc/include/uapi/asm/elf.h | 75 +++--- arch/openrisc/kernel/entry.S | 15 +--- arch/openrisc/kernel/module.c | 18 - arch/openrisc/kernel/process.c| 13 +-- arch/openrisc/kernel/ptrace.c | 18 ++--- arch/openrisc/kernel/signal.c | 36 - arch/openrisc/kernel/traps.c | 144 ++ 11 files changed, 243 insertions(+), 111 deletions(-) create mode 100644 arch/openrisc/include/asm/fpu.h
Re: [PATCH] openrisc: Use do_kernel_power_off()
On Sun, Apr 14, 2024 at 07:52:03PM +0200, Sebastian Reichel wrote: > Hi, > > On Sun, Mar 31, 2024 at 08:02:28AM +0100, Stafford Horne wrote: > > After commit 14c5678720bd ("power: reset: syscon-poweroff: Use > > devm_register_sys_off_handler(POWER_OFF)") setting up of pm_power_off > > was removed from the driver, this causes OpenRISC platforms using > > syscon-poweroff to no longer shutdown. > > > > The kernel now supports chained power-off handlers. Use > > do_kernel_power_off() that invokes chained power-off handlers. All > > architectures have moved away from using pm_power_off except OpenRISC. > > > > This patch migrates openrisc to use do_kernel_power_off() instead of the > > legacy pm_power_off(). > > > > Fixes: 14c5678720bd ("power: reset: syscon-poweroff: Use > > devm_register_sys_off_handler(POWER_OFF)") > > Signed-off-by: Stafford Horne > > --- > > Reviewed-by: Sebastian Reichel Hello, Thank you for the review. -Stafford
[PATCH 5/5] openrisc: Move FPU state out of pt_regs
My original, naive, FPU support patch had the FPCSR register stored during both the *mode switch* and *context switch*. This is wasteful. Also, the original patches did not save the FPU state when handling signals during the system call fast path. We fix this by moving the FPCSR state to thread_struct in task_struct. We also introduce new helper functions save_fpu and restore_fpu which can be used to sync the FPU with thread_struct. These functions are now called when needed: - Setting up and restoring sigcontext when handling signals - Before and after __switch_to during context switches - When handling FPU exceptions - When reading and writing FPU register sets In the future we can further optimize this by doing lazy FPU save and restore. For example, FPU sync is not needed when switching to and from kernel threads (x86 does this). FPU save and restore does not need to be done two times if we have both rescheduling and signal work to do. However, since OpenRISC FPU state is a single register, I leave these optimizations for future consideration. Signed-off-by: Stafford Horne --- arch/openrisc/include/asm/fpu.h | 22 arch/openrisc/include/asm/processor.h | 1 + arch/openrisc/include/asm/ptrace.h| 3 +-- arch/openrisc/kernel/entry.S | 15 +-- arch/openrisc/kernel/process.c| 5 arch/openrisc/kernel/ptrace.c | 12 +++-- arch/openrisc/kernel/signal.c | 36 +-- arch/openrisc/kernel/traps.c | 14 +++ 8 files changed, 76 insertions(+), 32 deletions(-) create mode 100644 arch/openrisc/include/asm/fpu.h diff --git a/arch/openrisc/include/asm/fpu.h b/arch/openrisc/include/asm/fpu.h new file mode 100644 index ..57bc44d80d53 --- /dev/null +++ b/arch/openrisc/include/asm/fpu.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ASM_OPENRISC_FPU_H +#define __ASM_OPENRISC_FPU_H + +struct task_struct; + +#ifdef CONFIG_FPU +static inline void save_fpu(struct task_struct *task) +{ + task->thread.fpcsr = mfspr(SPR_FPCSR); +} + +static inline void restore_fpu(struct task_struct *task) +{ + mtspr(SPR_FPCSR, task->thread.fpcsr); +} +#else +#define save_fpu(tsk) do { } while (0) +#define restore_fpu(tsk) do { } while (0) +#endif + +#endif /* __ASM_OPENRISC_FPU_H */ diff --git a/arch/openrisc/include/asm/processor.h b/arch/openrisc/include/asm/processor.h index 3b736e74e6ed..e05d1b59e24e 100644 --- a/arch/openrisc/include/asm/processor.h +++ b/arch/openrisc/include/asm/processor.h @@ -44,6 +44,7 @@ struct task_struct; struct thread_struct { + long fpcsr; /* Floating point control status register. */ }; /* diff --git a/arch/openrisc/include/asm/ptrace.h b/arch/openrisc/include/asm/ptrace.h index 375147ff71fc..1da3e66292e2 100644 --- a/arch/openrisc/include/asm/ptrace.h +++ b/arch/openrisc/include/asm/ptrace.h @@ -59,7 +59,7 @@ struct pt_regs { * -1 for all other exceptions. */ long orig_gpr11; /* For restarting system calls */ - long fpcsr; /* Floating point control status register. */ + long dummy; /* Cheap alignment fix */ long dummy2;/* Cheap alignment fix */ }; @@ -115,6 +115,5 @@ static inline long regs_return_value(struct pt_regs *regs) #define PT_GPR31 124 #define PT_PC128 #define PT_ORIG_GPR11 132 -#define PT_FPCSR 136 #endif /* __ASM_OPENRISC_PTRACE_H */ diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S index c9f48e750b72..440711d7bf40 100644 --- a/arch/openrisc/kernel/entry.S +++ b/arch/openrisc/kernel/entry.S @@ -106,8 +106,6 @@ l.mtspr r0,r3,SPR_EPCR_BASE ;\ l.lwz r3,PT_SR(r1);\ l.mtspr r0,r3,SPR_ESR_BASE ;\ - l.lwz r3,PT_FPCSR(r1) ;\ - l.mtspr r0,r3,SPR_FPCSR ;\ l.lwz r2,PT_GPR2(r1) ;\ l.lwz r3,PT_GPR3(r1) ;\ l.lwz r4,PT_GPR4(r1) ;\ @@ -177,8 +175,6 @@ handler: ;\ /* r30 already save */ ;\ l.swPT_GPR31(r1),r31;\ TRACE_IRQS_OFF_ENTRY;\ - l.mfspr r30,r0,SPR_FPCSR;\ - l.swPT_FPCSR(r1),r30;\ /* Store -1 in orig_gpr11 for non-syscall exceptions */ ;\ l.addi r30,r0,-1 ;\ l.swPT_ORIG_GPR11(r1),r30 @@ -219,8 +215,6 @@ handler: ;\ /*
[PATCH 4/5] openrisc: Add FPU config
Allow disabling FPU related code sequences to save space. Signed-off-by: Stafford Horne --- arch/openrisc/Kconfig | 9 + arch/openrisc/kernel/ptrace.c | 6 ++ arch/openrisc/kernel/traps.c | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig index 3586cda55bde..69c0258700b2 100644 --- a/arch/openrisc/Kconfig +++ b/arch/openrisc/Kconfig @@ -188,6 +188,15 @@ config SMP If you don't know what to do here, say N. +config FPU + bool "FPU support" + default y + help + Say N here if you want to disable all floating-point related procedures + in the kernel and reduce binary size. + + If you don't know what to do here, say Y. + source "kernel/Kconfig.hz" config OPENRISC_NO_SPR_SR_DSX diff --git a/arch/openrisc/kernel/ptrace.c b/arch/openrisc/kernel/ptrace.c index 1eeac3b62e9d..cf410193095f 100644 --- a/arch/openrisc/kernel/ptrace.c +++ b/arch/openrisc/kernel/ptrace.c @@ -88,6 +88,7 @@ static int genregs_set(struct task_struct *target, return ret; } +#ifdef CONFIG_FPU /* * As OpenRISC shares GPRs and floating point registers we don't need to export * the floating point registers again. So here we only export the fpcsr special @@ -115,13 +116,16 @@ static int fpregs_set(struct task_struct *target, >fpcsr, 0, 4); return ret; } +#endif /* * Define the register sets available on OpenRISC under Linux */ enum or1k_regset { REGSET_GENERAL, +#ifdef CONFIG_FPU REGSET_FPU, +#endif }; static const struct user_regset or1k_regsets[] = { @@ -133,6 +137,7 @@ static const struct user_regset or1k_regsets[] = { .regset_get = genregs_get, .set = genregs_set, }, +#ifdef CONFIG_FPU [REGSET_FPU] = { .core_note_type = NT_PRFPREG, .n = sizeof(struct __or1k_fpu_state) / sizeof(long), @@ -141,6 +146,7 @@ static const struct user_regset or1k_regsets[] = { .regset_get = fpregs_get, .set = fpregs_set, }, +#endif }; static const struct user_regset_view user_or1k_native_view = { diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 211ddaa0c5fa..57e0d674eb04 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -182,6 +182,7 @@ asmlinkage void do_fpe_trap(struct pt_regs *regs, unsigned long address) { if (user_mode(regs)) { int code = FPE_FLTUNK; +#ifdef CONFIG_FPU unsigned long fpcsr = regs->fpcsr; if (fpcsr & SPR_FPCSR_IVF) @@ -197,7 +198,7 @@ asmlinkage void do_fpe_trap(struct pt_regs *regs, unsigned long address) /* Clear all flags */ regs->fpcsr &= ~SPR_FPCSR_ALLF; - +#endif force_sig_fault(SIGFPE, code, (void __user *)regs->pc); } else { pr_emerg("KERNEL: Illegal fpe exception 0x%.8lx\n", regs->pc); -- 2.44.0
[PATCH 3/5] openrisc: traps: Don't send signals to kernel mode threads
OpenRISC exception handling sends signals to user processes on floating point exceptions and trap instructions (for debugging) among others. There is a bug where the trap handling logic may send signals to kernel threads, we should not send these signals to kernel threads, if that happens we treat it as an error. This patch adds conditions to die if the kernel receives these exceptions in kernel mode code. Fixes: 27267655c531 ("openrisc: Support floating point user api") Signed-off-by: Stafford Horne --- arch/openrisc/kernel/traps.c | 48 ++-- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 88fe27e4c10c..211ddaa0c5fa 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -180,29 +180,39 @@ asmlinkage void unhandled_exception(struct pt_regs *regs, int ea, int vector) asmlinkage void do_fpe_trap(struct pt_regs *regs, unsigned long address) { - int code = FPE_FLTUNK; - unsigned long fpcsr = regs->fpcsr; - - if (fpcsr & SPR_FPCSR_IVF) - code = FPE_FLTINV; - else if (fpcsr & SPR_FPCSR_OVF) - code = FPE_FLTOVF; - else if (fpcsr & SPR_FPCSR_UNF) - code = FPE_FLTUND; - else if (fpcsr & SPR_FPCSR_DZF) - code = FPE_FLTDIV; - else if (fpcsr & SPR_FPCSR_IXF) - code = FPE_FLTRES; - - /* Clear all flags */ - regs->fpcsr &= ~SPR_FPCSR_ALLF; - - force_sig_fault(SIGFPE, code, (void __user *)regs->pc); + if (user_mode(regs)) { + int code = FPE_FLTUNK; + unsigned long fpcsr = regs->fpcsr; + + if (fpcsr & SPR_FPCSR_IVF) + code = FPE_FLTINV; + else if (fpcsr & SPR_FPCSR_OVF) + code = FPE_FLTOVF; + else if (fpcsr & SPR_FPCSR_UNF) + code = FPE_FLTUND; + else if (fpcsr & SPR_FPCSR_DZF) + code = FPE_FLTDIV; + else if (fpcsr & SPR_FPCSR_IXF) + code = FPE_FLTRES; + + /* Clear all flags */ + regs->fpcsr &= ~SPR_FPCSR_ALLF; + + force_sig_fault(SIGFPE, code, (void __user *)regs->pc); + } else { + pr_emerg("KERNEL: Illegal fpe exception 0x%.8lx\n", regs->pc); + die("Die:", regs, SIGFPE); + } } asmlinkage void do_trap(struct pt_regs *regs, unsigned long address) { - force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc); + if (user_mode(regs)) { + force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc); + } else { + pr_emerg("KERNEL: Illegal trap exception 0x%.8lx\n", regs->pc); + die("Die:", regs, SIGILL); + } } asmlinkage void do_unaligned_access(struct pt_regs *regs, unsigned long address) -- 2.44.0
[PATCH 2/5] openrisc: traps: Remove calls to show_registers before die
The die function calls show_registers unconditionally. Remove calls to show_registers before calling die to avoid printing all registers and stack status two times during a crash. This was found when testing kernel trap and floating point exception handling. Signed-off-by: Stafford Horne --- arch/openrisc/kernel/traps.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 6d0fee912747..88fe27e4c10c 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -212,7 +212,6 @@ asmlinkage void do_unaligned_access(struct pt_regs *regs, unsigned long address) force_sig_fault(SIGBUS, BUS_ADRALN, (void __user *)address); } else { pr_emerg("KERNEL: Unaligned Access 0x%.8lx\n", address); - show_registers(regs); die("Die:", regs, address); } @@ -225,7 +224,6 @@ asmlinkage void do_bus_fault(struct pt_regs *regs, unsigned long address) force_sig_fault(SIGBUS, BUS_ADRERR, (void __user *)address); } else {/* Kernel mode */ pr_emerg("KERNEL: Bus error (SIGBUS) 0x%.8lx\n", address); - show_registers(regs); die("Die:", regs, address); } } @@ -421,7 +419,6 @@ asmlinkage void do_illegal_instruction(struct pt_regs *regs, } else {/* Kernel mode */ pr_emerg("KERNEL: Illegal instruction (SIGILL) 0x%.8lx\n", address); - show_registers(regs); die("Die:", regs, address); } } -- 2.44.0
[PATCH 1/5] openrisc: traps: Convert printks to pr_ macros
The pr_* macros are the convention and my upcoming patches add even more printk's. Use this opportunity to convert the printks in this file to the pr_* macros to avoid patch check warnings. Signed-off-by: Stafford Horne --- arch/openrisc/kernel/traps.c | 88 ++-- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 9370888c9a7e..6d0fee912747 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -51,16 +51,16 @@ static void print_trace(void *data, unsigned long addr, int reliable) { const char *loglvl = data; - printk("%s[<%p>] %s%pS\n", loglvl, (void *) addr, reliable ? "" : "? ", - (void *) addr); + pr_info("%s[<%p>] %s%pS\n", loglvl, (void *) addr, reliable ? "" : "? ", + (void *) addr); } static void print_data(unsigned long base_addr, unsigned long word, int i) { if (i == 0) - printk("(%08lx:)\t%08lx", base_addr + (i * 4), word); + pr_info("(%08lx:)\t%08lx", base_addr + (i * 4), word); else - printk(" %08lx:\t%08lx", base_addr + (i * 4), word); + pr_info(" %08lx:\t%08lx", base_addr + (i * 4), word); } /* displays a short stack trace */ @@ -69,7 +69,7 @@ void show_stack(struct task_struct *task, unsigned long *esp, const char *loglvl if (esp == NULL) esp = (unsigned long *) - printk("%sCall trace:\n", loglvl); + pr_info("%sCall trace:\n", loglvl); unwind_stack((void *)loglvl, esp, print_trace); } @@ -83,57 +83,57 @@ void show_registers(struct pt_regs *regs) if (user_mode(regs)) in_kernel = 0; - printk("CPU #: %d\n" - " PC: %08lxSR: %08lxSP: %08lx FPCSR: %08lx\n", - smp_processor_id(), regs->pc, regs->sr, regs->sp, - regs->fpcsr); - printk("GPR00: %08lx GPR01: %08lx GPR02: %08lx GPR03: %08lx\n", - 0L, regs->gpr[1], regs->gpr[2], regs->gpr[3]); - printk("GPR04: %08lx GPR05: %08lx GPR06: %08lx GPR07: %08lx\n", - regs->gpr[4], regs->gpr[5], regs->gpr[6], regs->gpr[7]); - printk("GPR08: %08lx GPR09: %08lx GPR10: %08lx GPR11: %08lx\n", - regs->gpr[8], regs->gpr[9], regs->gpr[10], regs->gpr[11]); - printk("GPR12: %08lx GPR13: %08lx GPR14: %08lx GPR15: %08lx\n", - regs->gpr[12], regs->gpr[13], regs->gpr[14], regs->gpr[15]); - printk("GPR16: %08lx GPR17: %08lx GPR18: %08lx GPR19: %08lx\n", - regs->gpr[16], regs->gpr[17], regs->gpr[18], regs->gpr[19]); - printk("GPR20: %08lx GPR21: %08lx GPR22: %08lx GPR23: %08lx\n", - regs->gpr[20], regs->gpr[21], regs->gpr[22], regs->gpr[23]); - printk("GPR24: %08lx GPR25: %08lx GPR26: %08lx GPR27: %08lx\n", - regs->gpr[24], regs->gpr[25], regs->gpr[26], regs->gpr[27]); - printk("GPR28: %08lx GPR29: %08lx GPR30: %08lx GPR31: %08lx\n", - regs->gpr[28], regs->gpr[29], regs->gpr[30], regs->gpr[31]); - printk(" RES: %08lx oGPR11: %08lx\n", - regs->gpr[11], regs->orig_gpr11); - - printk("Process %s (pid: %d, stackpage=%08lx)\n", - current->comm, current->pid, (unsigned long)current); + pr_info("CPU #: %d\n" + " PC: %08lxSR: %08lxSP: %08lx FPCSR: %08lx\n", + smp_processor_id(), regs->pc, regs->sr, regs->sp, + regs->fpcsr); + pr_info("GPR00: %08lx GPR01: %08lx GPR02: %08lx GPR03: %08lx\n", + 0L, regs->gpr[1], regs->gpr[2], regs->gpr[3]); + pr_info("GPR04: %08lx GPR05: %08lx GPR06: %08lx GPR07: %08lx\n", + regs->gpr[4], regs->gpr[5], regs->gpr[6], regs->gpr[7]); + pr_info("GPR08: %08lx GPR09: %08lx GPR10: %08lx GPR11: %08lx\n", + regs->gpr[8], regs->gpr[9], regs->gpr[10], regs->gpr[11]); + pr_info("GPR12: %08lx GPR13: %08lx GPR14: %08lx GPR15: %08lx\n", + regs->gpr[12], regs->gpr[13], regs->gpr[14], regs->gpr[15]); + pr_info("GPR16: %08lx GPR17: %08lx GPR18: %08lx GPR19: %08lx\n", + regs->gpr[16], regs->gpr[17], regs->gpr[18], regs->gpr[19]); + pr_info("GPR20: %08lx GPR21: %08lx GPR22: %08lx GPR23: %08lx\n", + regs->gpr[20], regs->gpr[21], regs->gpr[22], regs->gpr[23]); + pr_info("GPR24: %08lx GPR2
[PATCH 0/5] OpenRISC FPU and Signal handling fixups
This series has some fixups found when I was doing a deep dive documentation of the OpenRISC FPU support which was added in 2023. http://stffrdhrn.github.io/hardware/embedded/openrisc/2023/08/24/or1k-fpu-linux-and-compilers.html The FPU handling has issues of being inefficient and also not providing the proper state if signals are received when handling syscalls. The series is small, so should be easy to see from the commit list but in summary does: - Fixup some issues with exception handling. - Adds CONFIG_FPU to allow disabling FPU support - Fixups to e FPU handling logic moving the FPCSR state out of the kernel stack pt_regs and into the task_struct. Stafford Horne (5): openrisc: traps: Convert printks to pr_ macros openrisc: traps: Remove calls to show_registers before die openrisc: traps: Don't send signals to kernel mode threads openrisc: Add FPU config openrisc: Move FPU state out of pt_regs arch/openrisc/Kconfig | 9 ++ arch/openrisc/include/asm/fpu.h | 22 arch/openrisc/include/asm/processor.h | 1 + arch/openrisc/include/asm/ptrace.h| 3 +- arch/openrisc/kernel/entry.S | 15 +-- arch/openrisc/kernel/process.c| 5 + arch/openrisc/kernel/ptrace.c | 18 ++-- arch/openrisc/kernel/signal.c | 36 ++- arch/openrisc/kernel/traps.c | 144 ++ 9 files changed, 160 insertions(+), 93 deletions(-) create mode 100644 arch/openrisc/include/asm/fpu.h -- 2.44.0
[PATCH v2 0/2] OpenRISC module fixups
This series implements some missing OpenRISC relocations to allow module loading to work. I tested a few modules and these relocations were enough to allow for several modules I tested with. In the series we: 1. Update the relocations to add all of the relocation types currently defined in gnu binutils. 2. Implement two of the missing relocations needed for module loading. Since v1: - Added patch suggested by Geert to rename all relocation types to the R_OR1K_* form. Stafford Horne (2): openrisc: Define openrisc relocation types openrisc: Add support for more module relocations arch/openrisc/include/uapi/asm/elf.h | 75 arch/openrisc/kernel/module.c| 18 +-- 2 files changed, 80 insertions(+), 13 deletions(-) -- 2.44.0
[PATCH v2 2/2] openrisc: Add support for more module relocations
When testing modules in OpenRISC I found R_OR1K_AHI16 (signed adjusted high 16-bit) and R_OR1K_SLO16 (split low 16-bit) relocations are used in modules but not implemented yet. This patch implements the relocations. I have tested with a few modules. Signed-off-by: Stafford Horne --- arch/openrisc/kernel/module.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/openrisc/kernel/module.c b/arch/openrisc/kernel/module.c index 292f0afe27b9..c9ff4c4a0b29 100644 --- a/arch/openrisc/kernel/module.c +++ b/arch/openrisc/kernel/module.c @@ -55,6 +55,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, value |= *location & 0xfc00; *location = value; break; + case R_OR1K_AHI16: + /* Adjust the operand to match with a signed LO16. */ + value += 0x8000; + *((uint16_t *)location + 1) = value >> 16; + break; + case R_OR1K_SLO16: + /* Split value lower 16-bits. */ + value = ((value & 0xf800) << 10) | (value & 0x7ff); + *location = (*location & ~0x3e007ff) | value; + break; default: pr_err("module %s: Unknown relocation: %u\n", me->name, ELF32_R_TYPE(rel[i].r_info)); -- 2.44.0
[PATCH v2 1/2] openrisc: Define openrisc relocation types
This defines the current OpenRISC relocation types using the current R_OR1K_* naming conventions. The old R_OR32_* definitions are left for backwards compatibility. Note, the R_OR32_VTENTRY and R_OR32_VTINHERIT macros were defined with the wrong values the have always been 7 and 8 respectively, not 8 and 7. They are not used for module loading and I have updated them to use the correct values. Signed-off-by: Stafford Horne --- arch/openrisc/include/uapi/asm/elf.h | 75 arch/openrisc/kernel/module.c| 8 +-- 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/arch/openrisc/include/uapi/asm/elf.h b/arch/openrisc/include/uapi/asm/elf.h index 6868f81c281e..441e343f8268 100644 --- a/arch/openrisc/include/uapi/asm/elf.h +++ b/arch/openrisc/include/uapi/asm/elf.h @@ -34,15 +34,72 @@ #include /* The OR1K relocation types... not all relevant for module loader */ -#define R_OR32_NONE0 -#define R_OR32_32 1 -#define R_OR32_16 2 -#define R_OR32_8 3 -#define R_OR32_CONST 4 -#define R_OR32_CONSTH 5 -#define R_OR32_JUMPTARG6 -#define R_OR32_VTINHERIT 7 -#define R_OR32_VTENTRY 8 +#define R_OR1K_NONE0 +#define R_OR1K_32 1 +#define R_OR1K_16 2 +#define R_OR1K_8 3 +#define R_OR1K_LO_16_IN_INSN 4 +#define R_OR1K_HI_16_IN_INSN 5 +#define R_OR1K_INSN_REL_26 6 +#define R_OR1K_GNU_VTENTRY 7 +#define R_OR1K_GNU_VTINHERIT 8 +#define R_OR1K_32_PCREL9 +#define R_OR1K_16_PCREL10 +#define R_OR1K_8_PCREL 11 +#define R_OR1K_GOTPC_HI16 12 +#define R_OR1K_GOTPC_LO16 13 +#define R_OR1K_GOT16 14 +#define R_OR1K_PLT26 15 +#define R_OR1K_GOTOFF_HI16 16 +#define R_OR1K_GOTOFF_LO16 17 +#define R_OR1K_COPY18 +#define R_OR1K_GLOB_DAT19 +#define R_OR1K_JMP_SLOT20 +#define R_OR1K_RELATIVE21 +#define R_OR1K_TLS_GD_HI16 22 +#define R_OR1K_TLS_GD_LO16 23 +#define R_OR1K_TLS_LDM_HI1624 +#define R_OR1K_TLS_LDM_LO1625 +#define R_OR1K_TLS_LDO_HI1626 +#define R_OR1K_TLS_LDO_LO1627 +#define R_OR1K_TLS_IE_HI16 28 +#define R_OR1K_TLS_IE_LO16 29 +#define R_OR1K_TLS_LE_HI16 30 +#define R_OR1K_TLS_LE_LO16 31 +#define R_OR1K_TLS_TPOFF 32 +#define R_OR1K_TLS_DTPOFF 33 +#define R_OR1K_TLS_DTPMOD 34 +#define R_OR1K_AHI16 35 +#define R_OR1K_GOTOFF_AHI1636 +#define R_OR1K_TLS_IE_AHI1637 +#define R_OR1K_TLS_LE_AHI1638 +#define R_OR1K_SLO16 39 +#define R_OR1K_GOTOFF_SLO1640 +#define R_OR1K_TLS_LE_SLO1641 +#define R_OR1K_PCREL_PG21 42 +#define R_OR1K_GOT_PG2143 +#define R_OR1K_TLS_GD_PG21 44 +#define R_OR1K_TLS_LDM_PG2145 +#define R_OR1K_TLS_IE_PG21 46 +#define R_OR1K_LO1347 +#define R_OR1K_GOT_LO1348 +#define R_OR1K_TLS_GD_LO13 49 +#define R_OR1K_TLS_LDM_LO1350 +#define R_OR1K_TLS_IE_LO13 51 +#define R_OR1K_SLO13 52 +#define R_OR1K_PLTA26 53 +#define R_OR1K_GOT_AHI16 54 + +/* Old relocation names */ +#define R_OR32_NONER_OR1K_NONE +#define R_OR32_32 R_OR1K_32 +#define R_OR32_16 R_OR1K_16 +#define R_OR32_8 R_OR1K_8 +#define R_OR32_CONST R_OR1K_LO_16_IN_INSN +#define R_OR32_CONSTH R_OR1K_HI_16_IN_INSN +#define R_OR32_JUMPTARGR_OR1K_INSN_REL_26 +#define R_OR32_VTENTRY R_OR1K_GNU_VTENTRY +#define R_OR32_VTINHERIT R_OR1K_GNU_VTINHERIT typedef unsigned long elf_greg_t; diff --git a/arch/openrisc/kernel/module.c b/arch/openrisc/kernel/module.c index 532013f523ac..292f0afe27b9 100644 --- a/arch/openrisc/kernel/module.c +++ b/arch/openrisc/kernel/module.c @@ -39,16 +39,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, value = sym->st_value + rel[i].r_addend; switch (ELF32_R_TYPE(rel[i].r_info)) { - case R_OR32_32: + case R_OR1K_32: *location = value; break; - case R_OR32_CONST: + case R_OR1K_LO_16_IN_INSN: *((uint16_t *)location + 1) = value; break; - case R_OR32_CONSTH: + case R_OR1K_HI_16_IN_INSN: *((uint16_t *)location + 1) = value >> 16; break; - case R_OR32_JUMPTARG: + case R_OR1K_INSN_REL_26: value -= (uint32_t)location; value >>= 2; value &= 0x03ff; -- 2.44.0
Re: [PATCH] openrisc: Add support for more module relocations
On Thu, Apr 11, 2024 at 02:12:59PM +0200, Geert Uytterhoeven wrote: > Hi Stafford, > > On Wed, Apr 10, 2024 at 10:52 PM Stafford Horne wrote: > > This patch adds the relocations. Note, we use the old naming R_OR32_* > > instead or the new naming R_OR1K_* to avoid change as this header is > > exported as a user api. > > > --- a/arch/openrisc/include/uapi/asm/elf.h > > +++ b/arch/openrisc/include/uapi/asm/elf.h > > @@ -43,6 +43,8 @@ > > #define R_OR32_JUMPTARG6 > > #define R_OR32_VTINHERIT 7 > > #define R_OR32_VTENTRY 8 > > +#define R_OR32_AHI16 35 > > +#define R_OR32_SLO16 39 > > Would it make sense to switch to the new names, e.g. > > #define R_OR1K_NONE0 > > and add definitions for backwards compatibility? > > #define R_OR32_NONER_OR1K_NONE > Hi Geert, Actually I had a patch doing this and added all 38 or so relocation definitions. But I dropped it at the last moment in favor of simplicity. Let me rework it and add it back. -Stafford
[PATCH] openrisc: Add support for more module relocations
When testing modules in OpenRISC I found R_OR32_AHI16 (signed adjusted high 16-bit) and R_OR32_SLO16 (split low 16-bit) relocations are used in modules but not implemented yet. This patch adds the relocations. Note, we use the old naming R_OR32_* instead or the new naming R_OR1K_* to avoid change as this header is exported as a user api. Signed-off-by: Stafford Horne --- arch/openrisc/include/uapi/asm/elf.h | 2 ++ arch/openrisc/kernel/module.c| 10 ++ 2 files changed, 12 insertions(+) diff --git a/arch/openrisc/include/uapi/asm/elf.h b/arch/openrisc/include/uapi/asm/elf.h index 6868f81c281e..0c882a388524 100644 --- a/arch/openrisc/include/uapi/asm/elf.h +++ b/arch/openrisc/include/uapi/asm/elf.h @@ -43,6 +43,8 @@ #define R_OR32_JUMPTARG6 #define R_OR32_VTINHERIT 7 #define R_OR32_VTENTRY 8 +#define R_OR32_AHI16 35 +#define R_OR32_SLO16 39 typedef unsigned long elf_greg_t; diff --git a/arch/openrisc/kernel/module.c b/arch/openrisc/kernel/module.c index 532013f523ac..01bda5616114 100644 --- a/arch/openrisc/kernel/module.c +++ b/arch/openrisc/kernel/module.c @@ -55,6 +55,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs, value |= *location & 0xfc00; *location = value; break; + case R_OR32_AHI16: + /* Adjust the operand to match with a signed LO16. */ + value += 0x8000; + *((uint16_t *)location + 1) = value >> 16; + break; + case R_OR32_SLO16: + /* Split value lower 16-bits. */ + value = ((value & 0xf800) << 10) | (value & 0x7ff); + *location = (*location & ~0x3e007ff) | value; + break; default: pr_err("module %s: Unknown relocation: %u\n", me->name, ELF32_R_TYPE(rel[i].r_info)); -- 2.44.0
[PATCH] openrisc: Use do_kernel_power_off()
After commit 14c5678720bd ("power: reset: syscon-poweroff: Use devm_register_sys_off_handler(POWER_OFF)") setting up of pm_power_off was removed from the driver, this causes OpenRISC platforms using syscon-poweroff to no longer shutdown. The kernel now supports chained power-off handlers. Use do_kernel_power_off() that invokes chained power-off handlers. All architectures have moved away from using pm_power_off except OpenRISC. This patch migrates openrisc to use do_kernel_power_off() instead of the legacy pm_power_off(). Fixes: 14c5678720bd ("power: reset: syscon-poweroff: Use devm_register_sys_off_handler(POWER_OFF)") Signed-off-by: Stafford Horne --- arch/openrisc/kernel/process.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/arch/openrisc/kernel/process.c b/arch/openrisc/kernel/process.c index 86e02929f3ac..3c27d1c72718 100644 --- a/arch/openrisc/kernel/process.c +++ b/arch/openrisc/kernel/process.c @@ -65,7 +65,7 @@ void machine_restart(char *cmd) } /* - * This is used if pm_power_off has not been set by a power management + * This is used if a sys-off handler was not set by a power management * driver, in this case we can assume we are on a simulator. On * OpenRISC simulators l.nop 1 will trigger the simulator exit. */ @@ -89,10 +89,8 @@ void machine_halt(void) void machine_power_off(void) { printk(KERN_INFO "*** MACHINE POWER OFF ***\n"); - if (pm_power_off != NULL) - pm_power_off(); - else - default_power_off(); + do_kernel_power_off(); + default_power_off(); } /* -- 2.44.0
[GIT PULL] OpenRISC updates for 6.9
Hello Linus, Please consider for pull, The following changes since commit b401b621758e46812da61fa58a67c3fd8d91de0d: Linux 6.8-rc5 (2024-02-18 12:56:25 -0800) are available in the Git repository at: https://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 7f1e2fc493480086fbb375f4f6d33cb93fc069d6: openrisc: Use asm-generic's version of fix_to_virt() & virt_to_fix() (2024-03-10 08:55:46 +) OpenRISC updates for 6.9 Just a few cleanups and updates that were sent in: - Replace asm/fixmap.h with asm-generic version - Fix to move memblock setup up before it's used during init Dawei Li (1): openrisc: Use asm-generic's version of fix_to_virt() & virt_to_fix() Oreoluwa Babatunde (1): openrisc: Call setup_memory() earlier in the init sequence arch/openrisc/include/asm/fixmap.h | 31 +-- arch/openrisc/kernel/setup.c | 6 +++--- 2 files changed, 4 insertions(+), 33 deletions(-)
Re: [PATCH] openrisc: Use asm-generic's version of fix_to_virt() & virt_to_fix()
On Sat, Mar 09, 2024 at 06:24:07PM +0800, Dawei Li wrote: > Openrisc's implementation of fix_to_virt() & virt_to_fix() share same > functionality with ones of asm generic. > > Plus, generic version of fix_to_virt() can trap invalid index at compile > time. > > Thus, Replace the arch-specific implementations with asm generic's ones. > > Signed-off-by: Dawei Li Thanks, I have reviewed and tested this and it looks good. I will queue for the next merge window. -Stafford
Re: [PATCH 2/3] openrisc: Call setup_memory() earlier in the init sequence
On Fri, Feb 09, 2024 at 04:29:30PM -0800, Oreoluwa Babatunde wrote: > The unflatten_and_copy_device_tree() function contains a call to > memblock_alloc(). This means that memblock is allocating memory before > any of the reserved memory regions are set aside in the setup_memory() > function which calls early_init_fdt_scan_reserved_mem(). Therefore, > there is a possibility for memblock to allocate from any of the > reserved memory regions. > > Hence, move the call to setup_memory() to be earlier in the init > sequence so that the reserved memory regions are set aside before any > allocations are done using memblock. > > Signed-off-by: Oreoluwa Babatunde > --- > arch/openrisc/kernel/setup.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c > index 9cf7fb6..be56eaa 100644 > --- a/arch/openrisc/kernel/setup.c > +++ b/arch/openrisc/kernel/setup.c > @@ -255,6 +255,9 @@ void calibrate_delay(void) > > void __init setup_arch(char **cmdline_p) > { > + /* setup memblock allocator */ > + setup_memory(); > + > unflatten_and_copy_device_tree(); > > setup_cpuinfo(); > @@ -278,9 +281,6 @@ void __init setup_arch(char **cmdline_p) > } > #endif > > - /* setup memblock allocator */ > - setup_memory(); > - > /* paging_init() sets up the MMU and marks all pages as reserved */ > paging_init(); This looks good. I will merge it via the openrisc queue as you requested. -Stafford
Re: [PATCH 0/3] Restructure init sequence to set aside reserved memory earlier
On Tue, Mar 05, 2024 at 10:59:20AM -0800, Oreoluwa Babatunde wrote: > > On 2/9/2024 4:29 PM, Oreoluwa Babatunde wrote: > > The loongarch, openric, and sh architectures allocate memory from > > memblock before it gets the chance to set aside reserved memory regions. > > This means that there is a possibility for memblock to allocate from > > memory regions that are supposed to be reserved. > > > > This series makes changes to the arch specific setup code to call the > > functions responsible for setting aside the reserved memory regions earlier > > in the init sequence. > > Hence, by the time memblock starts being used to allocate memory, the > > reserved memory regions should already be set aside, and it will no > > longer be possible for allocations to come from them. > > > > I am currnetly using an arm64 device, and so I will need assistance from > > the relevant arch maintainers to help check if this breaks anything from > > compilation to device bootup. > > > > Oreoluwa Babatunde (3): > > loongarch: Call arch_mem_init() before platform_init() in the init > > sequence > > openrisc: Call setup_memory() earlier in the init sequence > > sh: Call paging_init() earlier in the init sequence > > > > arch/loongarch/kernel/setup.c | 2 +- > > arch/openrisc/kernel/setup.c | 6 +++--- > > arch/sh/kernel/setup.c| 4 ++-- > > 3 files changed, 6 insertions(+), 6 deletions(-) > Hello, > > Loongarch patch has already merged for this, but review is still pending > from openrisc and sh architectures. > Could someone please comment on these? Hello, The OpenRISC patch looks fine to me. I will test it out. Sorry, I thought you were getting this merged via other means. -Stafford
Re: [RFC][PATCH] locking: Generic ticket-lock
On Wed, Apr 14, 2021 at 02:45:43PM +0200, Peter Zijlstra wrote: > On Wed, Apr 14, 2021 at 12:16:38PM +0200, Peter Zijlstra wrote: > > On Wed, Apr 14, 2021 at 11:05:24AM +0200, Peter Zijlstra wrote: > > > > > That made me look at the qspinlock code, and queued_spin_*lock() uses > > > atomic_try_cmpxchg_acquire(), which means any arch that uses qspinlock > > > and has RCpc atomics will give us massive pain. > > > > > > Current archs using qspinlock are: x86, arm64, power, sparc64, mips and > > > openrisc (WTF?!). > > > > > > Of those, x86 and sparc are TSO archs with SC atomics, arm64 has RCsc > > > atomics, power has RCtso atomics (and is the arch we all hate for having > > > RCtso locks). > > > > > > Now MIPS has all sorts of ill specified barriers, but last time looked > > > at it it didn't actually use any of that and stuck to using smp_mb(), so > > > it will have RCsc atomics. > > > > > > /me goes look at wth openrisc is.. doesn't even appear to have > > > asm/barrier.h :-/ Looking at wikipedia it also doesn't appear to > > > actually have hardware ... > > > > FWIW this is broken, anything SMP *MUST* define mb(), at the very least. > > As near as I can tell this should do. The arch spec only lists this one > instruction and the text makes it sound like a completion barrier. Yes, I will submit this patch. The l.msync instruction completes all load/store operations. The l.lwa/l.swa pair (used for xchg/cmpxchg) also implies l.msync. > --- > arch/openrisc/include/asm/barrier.h | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/arch/openrisc/include/asm/barrier.h > b/arch/openrisc/include/asm/barrier.h > new file mode 100644 > index ..7538294721be > --- /dev/null > +++ b/arch/openrisc/include/asm/barrier.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_BARRIER_H > +#define __ASM_BARRIER_H > + > +#define mb() asm volatile ("l.msync" ::: "memory") > + > +#include > + > +#endif /* __ASM_BARRIER_H */
Re: [RFC][PATCH] locking: Generic ticket-lock
On Wed, Apr 14, 2021 at 12:16:38PM +0200, Peter Zijlstra wrote: > On Wed, Apr 14, 2021 at 11:05:24AM +0200, Peter Zijlstra wrote: > > > That made me look at the qspinlock code, and queued_spin_*lock() uses > > atomic_try_cmpxchg_acquire(), which means any arch that uses qspinlock > > and has RCpc atomics will give us massive pain. > > > > Current archs using qspinlock are: x86, arm64, power, sparc64, mips and > > openrisc (WTF?!). > > > > Of those, x86 and sparc are TSO archs with SC atomics, arm64 has RCsc > > atomics, power has RCtso atomics (and is the arch we all hate for having > > RCtso locks). > > > > Now MIPS has all sorts of ill specified barriers, but last time looked > > at it it didn't actually use any of that and stuck to using smp_mb(), so > > it will have RCsc atomics. > > > > /me goes look at wth openrisc is.. doesn't even appear to have > > asm/barrier.h :-/ Looking at wikipedia it also doesn't appear to > > actually have hardware ... Yes, not hardware available to consumers directoy, my development is done on FPGAs. > FWIW this is broken, anything SMP *MUST* define mb(), at the very least. Oh, thats right, something missed, when we developed qspinlocks we discussed this and my point there was that l.swa/l.lwa implied a mem flush l.msync/barrier. But mb still needs to be added. > > I'm thinking openrisc is a prime candidate for this ticket_lock.h we're > > all talking about. > > How's this then? Compile tested only on openrisc/simple_smp_defconfig. I did my testing with this FPGA build SoC: https://github.com/stffrdhrn/de0_nano-multicore Note, the CPU timer sync logic uses mb() and is a bit flaky. So missing mb() might be a reason. I thought we had defined mb() and l.msync, but it seems to have gotten lost. With that said I could test out this ticket-lock implementation. How would I tell if its better than qspinlock? > --- > arch/openrisc/Kconfig | 1 - > arch/openrisc/include/asm/Kbuild | 5 +- > arch/openrisc/include/asm/spinlock.h | 3 +- > arch/openrisc/include/asm/spinlock_types.h | 2 +- > include/asm-generic/qspinlock.h| 30 +++ > include/asm-generic/ticket-lock-types.h| 11 > include/asm-generic/ticket-lock.h | 86 > ++ > 7 files changed, 131 insertions(+), 7 deletions(-) > > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig > index 591acc5990dc..1858cf309f1f 100644 > --- a/arch/openrisc/Kconfig > +++ b/arch/openrisc/Kconfig > @@ -32,7 +32,6 @@ config OPENRISC > select HAVE_DEBUG_STACKOVERFLOW > select OR1K_PIC > select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 > - select ARCH_USE_QUEUED_SPINLOCKS > select ARCH_USE_QUEUED_RWLOCKS > select OMPIC if SMP > select ARCH_WANT_FRAME_POINTERS > diff --git a/arch/openrisc/include/asm/Kbuild > b/arch/openrisc/include/asm/Kbuild > index ca5987e11053..cb260e7d73db 100644 > --- a/arch/openrisc/include/asm/Kbuild > +++ b/arch/openrisc/include/asm/Kbuild > @@ -1,9 +1,8 @@ > # SPDX-License-Identifier: GPL-2.0 > generic-y += extable.h > generic-y += kvm_para.h > -generic-y += mcs_spinlock.h > -generic-y += qspinlock_types.h > -generic-y += qspinlock.h > +generic-y += ticket-lock.h > +generic-y += ticket-lock-types.h > generic-y += qrwlock_types.h > generic-y += qrwlock.h > generic-y += user.h > diff --git a/arch/openrisc/include/asm/spinlock.h > b/arch/openrisc/include/asm/spinlock.h > index a8940bdfcb7e..0b839ed1f3a0 100644 > --- a/arch/openrisc/include/asm/spinlock.h > +++ b/arch/openrisc/include/asm/spinlock.h > @@ -15,8 +15,7 @@ > #ifndef __ASM_OPENRISC_SPINLOCK_H > #define __ASM_OPENRISC_SPINLOCK_H > > -#include > - > +#include > #include > > #define arch_read_lock_flags(lock, flags) arch_read_lock(lock) > diff --git a/arch/openrisc/include/asm/spinlock_types.h > b/arch/openrisc/include/asm/spinlock_types.h > index 7c6fb1208c88..58ea31fa65ce 100644 > --- a/arch/openrisc/include/asm/spinlock_types.h > +++ b/arch/openrisc/include/asm/spinlock_types.h > @@ -1,7 +1,7 @@ > #ifndef _ASM_OPENRISC_SPINLOCK_TYPES_H > #define _ASM_OPENRISC_SPINLOCK_TYPES_H > > -#include > +#include > #include > > #endif /* _ASM_OPENRISC_SPINLOCK_TYPES_H */ > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h > index d74b13825501..a7a1296b0b4d 100644 > --- a/include/asm-generic/qspinlock.h > +++ b/include/asm-generic/qspinlock.h > @@ -2,6 +2,36 @@ > /* > * Queued spinlock > * > + * A 'generic' spinlock implementation that is based on MCS locks. An > + * architecture that's looking for a 'generic' spinlock, please first > consider > + * ticket-lock.h and only come looking here when you've considered all the > + * constraints below and can show your hardware does actually perform better > + * with qspinlock. > + * > + * > + * It relies on atomic_*_release()/atomic_*_acquire() to be RCsc (or no > weaker > + * than RCtso if
Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Apr 07, 2021 at 11:47:49AM +0200, Peter Zijlstra wrote: > On Wed, Apr 07, 2021 at 08:52:08AM +0900, Stafford Horne wrote: > > Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For > > OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So > > one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we > > remove our > > xchg16/xchg8 emulation code? > > CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 is guaranteed crap. > > All the architectures that have wanted it are RISC style LL/SC archs, > and for them a cmpxchg loop is a daft thing to do, since it reduces the > chance of it behaving sanely. > > Why would we provide something that's known to be suboptimal? If an > architecture chooses to not care about determinism and or fwd progress, > then that's their choice. But not one, I feel, we should encourage. Thanks, this is the response I was hoping my comment would provoke. So not enabling CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 for architectures unless they really want it should be the way. -Stafford
Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Apr 07, 2021 at 12:51:56AM +0800, Boqun Feng wrote: > Hi, > > On Wed, Mar 31, 2021 at 02:30:32PM +, guo...@kernel.org wrote: > > From: Guo Ren > > > > Some architectures don't have sub-word swap atomic instruction, > > they only have the full word's one. > > > > The sub-word swap only improve the performance when: > > NR_CPUS < 16K > > * 0- 7: locked byte > > * 8: pending > > * 9-15: not used > > * 16-17: tail index > > * 18-31: tail cpu (+1) > > > > The 9-15 bits are wasted to use xchg16 in xchg_tail. > > > > Please let architecture select xchg16/xchg32 to implement > > xchg_tail. > > > > If the architecture doesn't have sub-word swap atomic, won't it generate > the same/similar code no matter which version xchg_tail() is used? That > is even CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, xchg_tail() acts > similar to an xchg16() implemented by cmpxchg(), which means we still > don't have forward progress guarantee. So this configuration doesn't > solve the problem. > > I think it's OK to introduce this config and don't provide xchg16() for > risc-v. But I don't see the point of converting other architectures to > use it. Hello, For OpenRISC I did ack the patch to convert to CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y. But I think you are right, the generic code in xchg_tail and the xchg16 emulation code in produced by OpenRISC using xchg32 would produce very similar code. I have not compared instructions, but it does seem like duplicate functionality. Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we remove our xchg16/xchg8 emulation code? -Stafford
Re: [PATCH v6 6/9] openrisc: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Mar 31, 2021 at 02:30:37PM +, guo...@kernel.org wrote: > From: Guo Ren > > We don't have native hw xchg16 instruction, so let qspinlock > generic code to deal with it. > > Using the full-word atomic xchg instructions implement xchg16 has > the semantic risk for atomic operations. > > This patch cancels the dependency of on qspinlock generic code on > architecture's xchg16. > > Signed-off-by: Guo Ren > Cc: Arnd Bergmann > Cc: Jonas Bonn > Cc: Stefan Kristiansson > Cc: Stafford Horne > Cc: openr...@lists.librecores.org Acked-by: Stafford Horne > --- > arch/openrisc/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig > index 591acc5990dc..b299e409429f 100644 > --- a/arch/openrisc/Kconfig > +++ b/arch/openrisc/Kconfig > @@ -33,6 +33,7 @@ config OPENRISC > select OR1K_PIC > select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1 > select ARCH_USE_QUEUED_SPINLOCKS > + select ARCH_USE_QUEUED_SPINLOCKS_XCHG32 > select ARCH_USE_QUEUED_RWLOCKS > select OMPIC if SMP > select ARCH_WANT_FRAME_POINTERS > -- > 2.17.1 >
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Apr 06, 2021 at 11:50:38AM +0800, Guo Ren wrote: > On Wed, Mar 31, 2021 at 3:23 PM Arnd Bergmann wrote: > > > > On Wed, Mar 31, 2021 at 12:35 AM Stafford Horne wrote: > > > > > > I just want to chime in here, there may be a better spot in the thread to > > > mention this but, for OpenRISC I did implement some generic 8/16-bit xchg > > > code > > > which I have on my todo list somwhere to replace the other generic > > > implementations like that in mips. > > > > > > arch/openrisc/include/asm/cmpxchg.h > > > > > > The idea would be that architectures just implement these methods: > > > > > > long cmpxchg_u32(*ptr,old,new) > > > long xchg_u32(*ptr,val) > > > > > > Then the rest of the generic header would implement cmpxchg. > > > > I like the idea of generalizing it a little further. I'd suggest staying a > > little closer to the existing naming here though, as we already have > > cmpxchg() for the type-agnostic version, and cmpxchg64() for the > > fixed-length 64-bit version. > > > > I think a nice interface between architecture-specific and architecture > > independent code would be to have architectures provide > > arch_cmpxchg32()/arch_xchg32() as the most basic version, as well > > as arch_cmpxchg8()/arch_cmpxchg16()/arch_xchg8()/arch_xchg16() > > if they have instructions for those. > > > > The common code can then build cmpxchg16()/xchg16() on top of > > either the 16-bit or the 32-bit primitives, and build the cmpxchg()/xchg() > > wrapper around those (or alternatively we can decide to have them > > only deal with fixed-32-bit and long/pointer sized atomics). > I think these emulation codes are suitable for some architectures but not > riscv. > > We shouldn't export xchg16/cmpxchg16(emulated by lr.w/sc.w) in riscv, > We should forbid these sub-word atomic primitive and lets the > programmers consider their atomic design. Fair enough, having the generic sub-word emulation would be something that an architecture can select to use/export. -Stafford
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Mar 31, 2021 at 11:10:53PM +0800, Guo Ren wrote: > Hi Stafford, > > How do think add ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in openrisc? > > https://lore.kernel.org/linux-riscv/1617201040-83905-7-git-send-email-guo...@kernel.org/T/#u Sorry I missed your mail here. It is true that OpenRISC doesn't have xchg16, so using the xchg32 code is better. Acked-by: Stafford Horne > On Wed, Mar 31, 2021 at 8:31 PM Stafford Horne wrote: > > > > On Wed, Mar 31, 2021 at 09:23:27AM +0200, Arnd Bergmann wrote: > > > On Wed, Mar 31, 2021 at 12:35 AM Stafford Horne wrote: > > > > > > > > I just want to chime in here, there may be a better spot in the thread > > > > to > > > > mention this but, for OpenRISC I did implement some generic 8/16-bit > > > > xchg code > > > > which I have on my todo list somwhere to replace the other generic > > > > implementations like that in mips. > > > > > > > > arch/openrisc/include/asm/cmpxchg.h > > > > > > > > The idea would be that architectures just implement these methods: > > > > > > > > long cmpxchg_u32(*ptr,old,new) > > > > long xchg_u32(*ptr,val) > > > > > > > > Then the rest of the generic header would implement cmpxchg. > > > > > > I like the idea of generalizing it a little further. I'd suggest staying a > > > little closer to the existing naming here though, as we already have > > > cmpxchg() for the type-agnostic version, and cmpxchg64() for the > > > fixed-length 64-bit version. > > > > OK. > > > > > I think a nice interface between architecture-specific and architecture > > > independent code would be to have architectures provide > > > arch_cmpxchg32()/arch_xchg32() as the most basic version, as well > > > as arch_cmpxchg8()/arch_cmpxchg16()/arch_xchg8()/arch_xchg16() > > > if they have instructions for those. > > > > Thanks for the name suggestions, it makes it easier for me. > > > > > The common code can then build cmpxchg16()/xchg16() on top of > > > either the 16-bit or the 32-bit primitives, and build the cmpxchg()/xchg() > > > wrapper around those (or alternatively we can decide to have them > > > only deal with fixed-32-bit and long/pointer sized atomics). > > > > Yeah, that was the idea. > > > > -Stafford > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
[GIT PULL] OpenRISC fixes for 5.12-rc5
Hi Linus, Please consider for pull, The following changes since commit a5e13c6df0e41702d2b2c77c8ad41677ebb065b3: Linux 5.12-rc5 (2021-03-28 15:48:16 -0700) are available in the Git repository at: git://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 1683f7de65dbf0a2c6a7d639173fe92430a28930: soc: litex: Remove duplicated header file inclusion (2021-04-04 05:46:46 +0900) OpenRISC fix for 5.12 Includes: - Fix duplicate header include in Litex SOC driver Zhen Lei (1): soc: litex: Remove duplicated header file inclusion drivers/soc/litex/litex_soc_ctrl.c | 1 - 1 file changed, 1 deletion(-)
Re: [PATCH] soc: litex: Remove duplicated header file inclusion
On Wed, Mar 31, 2021 at 03:06:43PM +0200, Mateusz Holenko wrote: > From: Zhen Lei > > The header file is already included above and can be > removed here. > > Signed-off-by: Zhen Lei > Signed-off-by: Mateusz Holenko Thanks, I have staged this. -Stafford
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Mar 31, 2021 at 09:23:27AM +0200, Arnd Bergmann wrote: > On Wed, Mar 31, 2021 at 12:35 AM Stafford Horne wrote: > > > > I just want to chime in here, there may be a better spot in the thread to > > mention this but, for OpenRISC I did implement some generic 8/16-bit xchg > > code > > which I have on my todo list somwhere to replace the other generic > > implementations like that in mips. > > > > arch/openrisc/include/asm/cmpxchg.h > > > > The idea would be that architectures just implement these methods: > > > > long cmpxchg_u32(*ptr,old,new) > > long xchg_u32(*ptr,val) > > > > Then the rest of the generic header would implement cmpxchg. > > I like the idea of generalizing it a little further. I'd suggest staying a > little closer to the existing naming here though, as we already have > cmpxchg() for the type-agnostic version, and cmpxchg64() for the > fixed-length 64-bit version. OK. > I think a nice interface between architecture-specific and architecture > independent code would be to have architectures provide > arch_cmpxchg32()/arch_xchg32() as the most basic version, as well > as arch_cmpxchg8()/arch_cmpxchg16()/arch_xchg8()/arch_xchg16() > if they have instructions for those. Thanks for the name suggestions, it makes it easier for me. > The common code can then build cmpxchg16()/xchg16() on top of > either the 16-bit or the 32-bit primitives, and build the cmpxchg()/xchg() > wrapper around those (or alternatively we can decide to have them > only deal with fixed-32-bit and long/pointer sized atomics). Yeah, that was the idea. -Stafford
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 06:08:40PM +0200, Peter Zijlstra wrote: > On Tue, Mar 30, 2021 at 11:13:55AM +0800, Guo Ren wrote: > > On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote: > > > > > > On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote: > > > > u32 a = 0x55aa66bb; > > > > u16 *ptr = > > > > > > > > CPU0 CPU1 > > > > = = > > > > xchg16(ptr, new) while(1) > > > > WRITE_ONCE(*(ptr + 1), x); > > > > > > > > When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock. > > > > > > Then I think your LL/SC is broken. > > > > > > That also means you really don't want to build super complex locking > > > primitives on top, because that live-lock will percolate through. > > > Do you mean the below implementation has live-lock risk? > > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail) > > +{ > > + u32 old, new, val = atomic_read(>val); > > + > > + for (;;) { > > + new = (val & _Q_LOCKED_PENDING_MASK) | tail; > > + old = atomic_cmpxchg(>val, val, new); > > + if (old == val) > > + break; > > + > > + val = old; > > + } > > + return old; > > +} > > That entirely depends on the architecture (and cmpxchg() impementation). > > There are a number of cases: > > * architecture has cmpxchg() instruction (x86, s390, sparc, etc.). > > - architecture provides fwd progress (x86) > - architecture requires backoff for progress (sparc) > > * architecture does not have cmpxchg, and implements it using LL/SC. > > and here things get *really* interesting, because while an > architecture can have LL/SC fwd progress, that does not translate into > cmpxchg() also having the same guarantees and all bets are off. > > The real bummer is that C can do cmpxchg(), but there is no way it can > do LL/SC. And even if we'd teach C how to do LL/SC, it couldn't be > generic because architectures lacking it can't emulate it using > cmpxchg() (there's a fun class of bugs there). > > So while the above code might be the best we can do in generic code, > it's really up to the architecture to make it work. I just want to chime in here, there may be a better spot in the thread to mention this but, for OpenRISC I did implement some generic 8/16-bit xchg code which I have on my todo list somwhere to replace the other generic implementations like that in mips. arch/openrisc/include/asm/cmpxchg.h The idea would be that architectures just implement these methods: long cmpxchg_u32(*ptr,old,new) long xchg_u32(*ptr,val) Then the rest of the generic header would implement cmpxchg. -Stafford
[GIT PULL] OpenRISC updates for 5.12
Hi Linus, Please consider for pull. Note the starting point shows up a bit strange as I had to merge in 5.11 fixes to resolve some conflicts pointed out by linux-next. The below are the changes since v5.11. The following changes since commit 031c7a8cd6fc565e90320bf08f22ee6e70f9d969: openrisc: io: Add missing __iomem annotation to iounmap() (2021-01-20 06:14:26 +0900) are available in the Git repository at: g...@github.com:openrisc/linux.git tags/for-linus for you to fetch changes up to 8f722f67452f4b28cd8d7acf1658daa5796437c2: openrisc: Use devicetree to determine present cpus (2021-02-09 05:38:50 +0900) OpenRISC updates for 5.12 Include: - Update for Litex SoC controller to support wider width registers as well as reset. - Refactor SMP code to use device tree to define possible cpus. - Updates build including generating vmlinux.bin Gabriel Somlo (5): drivers/soc/litex: move generic accessors to litex.h drivers/soc/litex: separate MMIO from subregister offset calculation drivers/soc/litex: s/LITEX_REG_SIZE/LITEX_SUBREG_ALIGN/g drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs drivers/soc/litex: make 'litex_[set|get]_reg()' methods private Geert Uytterhoeven (1): drivers/soc/litex: Add restart handler Jan Henrik Weinstock (1): openrisc: Use devicetree to determine present cpus Joel Stanley (2): openrisc: Add vmlinux.bin target openrisc: restart: Call common handlers before hanging Masahiro Yamada (1): openrisc: add arch/openrisc/Kbuild Stafford Horne (1): Merge remote-tracking branch 'openrisc/or1k-5.11-fixes' into or1k-5.12-updates arch/openrisc/Kbuild | 3 + arch/openrisc/Makefile | 21 +++--- arch/openrisc/boot/.gitignore | 2 + arch/openrisc/boot/Makefile| 10 +++ arch/openrisc/kernel/process.c | 13 +++- arch/openrisc/kernel/smp.c | 23 -- drivers/soc/litex/Kconfig | 14 +++- drivers/soc/litex/litex_soc_ctrl.c | 116 +++- include/linux/litex.h | 150 + 9 files changed, 211 insertions(+), 141 deletions(-) create mode 100644 arch/openrisc/Kbuild create mode 100644 arch/openrisc/boot/.gitignore create mode 100644 arch/openrisc/boot/Makefile
Re: [PATCH v2] Use devicetree to determine present cpus (v2)
Thanks Jan, I was able to take these 2 mails and merge them together to a single patch. I will send the result for review in a bit. Next time please learn some git/linux kernel developer basics. It will help make this more smooth inthe future. THe code changes you made were great and I hope to see more in the future. :) Some tips: - Use 'git rebase -i', and the fixup/squash command to merge two or more commits. Also, there you should add the summary as you did in your mail in the git commit message. - Use 'git format-patch -o patch-dir -v2 ' to create your patch. - Use './scripts/checkpatch.pl patch-dir/.patch' to check your patch before you send it. If any issues use 'git rebase' or 'git commit --amend' to fix up the checkpatch issues then test and create a new patch. - Use './scripts/get-maintainers.pl' with 'git send-email' like this below. Some links: - https://www.kernel.org/doc/html/latest/process/submitting-patches.html - http://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/ (explains --cc-cmd, you can also add --no-rolestats to .get_maintainer.conf` - https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history (explain rebase squash fixup) This is usually what I do after testing. $ git lo # my custom alias: lo = log --pretty=format:'%C(yellow)%cd %C(green)%h %C(blue)%<(16)%aN%Creset %s %C(auto)%d%Creset' --decorate --date=short -n10 2021-02-09 8f722f67452f Jan Henrik Weinstock openrisc: Use devicetree to determine present cpus (HEAD -> or1k-5.12-updates) 2021-01-25 2261352157a9 Stafford Horne Merge remote-tracking branch 'openrisc/or1k-5.11-fixes' into or1k-5.12-updates (shorne/or1k-5.12-updates, shorne/for-next, openrisc/for-next, for-next) 2021-01-21 3706f9f76a4f Geert Uytterhoeven drivers/soc/litex: Add restart handler 2021-01-20 031c7a8cd6fc Geert Uytterhoeven openrisc: io: Add missing __iomem annotation to iounmap() (shorne/or1k-5.11-fixes, openrisc/or1k-5.11-fixes, or1k-5.11-fixes) 2021-01-18 803c72c8547c Masahiro Yamada openrisc: add arch/openrisc/Kbuild 2021-01-14 4f70d150294b Gabriel Somlodrivers/soc/litex: make 'litex_[set|get]_reg()' methods private 2021-01-14 51f109228308 Gabriel Somlodrivers/soc/litex: support 32-bit subregisters, 64-bit CPUs 2021-01-14 ffa4ebc48971 Gabriel Somlodrivers/soc/litex: s/LITEX_REG_SIZE/LITEX_SUBREG_ALIGN/g 2021-01-14 b5d3061ea2e6 Gabriel Somlodrivers/soc/litex: separate MMIO from subregister offset calculation 2021-01-14 9d93a9e8aab3 Gabriel Somlodrivers/soc/litex: move generic accessors to litex.h $ git format-patch -v3 -o patches/ 2261352157a9 patches/v3-0001-openrisc-Use-devicetree-to-determine-present-cpus.patch # Below a warning is printed but I think its OK as we can use NR_CPUS in smp.c $ ./scripts/checkpatch.pl patches/v3-0001-openrisc-Use-devicetree-to-determine-present-cpus.patch WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc #45: FILE: arch/openrisc/kernel/smp.c:73: + if (cpu_id < NR_CPUS) total: 0 errors, 1 warnings, 45 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. patches/v3-0001-openrisc-Use-devicetree-to-determine-present-cpus.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. $ git send-email --to linux-kernel --cc-cmd ./scripts/get_maintainer.pl patches/v3-0001-openrisc-Use-devicetree-to-determine-present-cpus.patch patches/v3-0001-openrisc-Use-devicetree-to-determine-present-cpus.patch -Stafford On Mon, Feb 08, 2021 at 03:28:32PM +0100, Jan Henrik Weinstock wrote: > Signed-off-by: Jan Henrik Weinstock > --- > arch/openrisc/kernel/smp.c | 31 +++ > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c > index 75be7e34f..83cbf43d4 100644 > --- a/arch/openrisc/kernel/smp.c > +++ b/arch/openrisc/kernel/smp.c > @@ -61,32 +61,31 @@ void __init smp_prepare_boot_cpu(void) > > void __init smp_init_cpus(void) > { > - int i; > + struct device_node* cpu; > + u32 cpu_id; > + > + for_each_of_cpu_node(cpu) { > + if (of_property_read_u32(cpu, "reg", _id)) { > + pr_warn("%s missing reg property", cpu->full_name); > + continue; > + } > > - for (i = 0; i < NR_CPUS; i++) > - set_cpu_possible(i, true); > + if (cpu_id <
[PATCH v3] openrisc: Use devicetree to determine present cpus
From: Jan Henrik Weinstock Use the device tree to determine the present cpus instead of assuming all CONFIG_NRCPUS are actually present in the system. Signed-off-by: Jan Henrik Weinstock [shorne: Squashed 2 email commits and added summary from email] Cc: Geert Uytterhoeven Signed-off-by: Stafford Horne --- arch/openrisc/kernel/smp.c | 23 +-- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c index 29c82ef2e207..48e1092a64de 100644 --- a/arch/openrisc/kernel/smp.c +++ b/arch/openrisc/kernel/smp.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -60,22 +61,32 @@ void __init smp_prepare_boot_cpu(void) void __init smp_init_cpus(void) { - int i; + struct device_node *cpu; + u32 cpu_id; - for (i = 0; i < NR_CPUS; i++) - set_cpu_possible(i, true); + for_each_of_cpu_node(cpu) { + if (of_property_read_u32(cpu, "reg", _id)) { + pr_warn("%s missing reg property", cpu->full_name); + continue; + } + + if (cpu_id < NR_CPUS) + set_cpu_possible(cpu_id, true); + } } void __init smp_prepare_cpus(unsigned int max_cpus) { - int i; + unsigned int cpu; /* * Initialise the present map, which describes the set of CPUs * actually populated at the present time. */ - for (i = 0; i < max_cpus; i++) - set_cpu_present(i, true); + for_each_possible_cpu(cpu) { + if (cpu < max_cpus) + set_cpu_present(cpu, true); + } } void __init smp_cpus_done(unsigned int max_cpus) -- 2.26.2
Re: [PATCH v2] openrisc: use device tree to determine present cpus
On Fri, Feb 05, 2021 at 05:07:51PM +0100, Geert Uytterhoeven wrote: > Hi Stafford, > > On Fri, Feb 5, 2021 at 3:43 PM Stafford Horne wrote: > > On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote: > > > Use the device tree to determine the present cpus instead of assuming all > > > CONFIG_NRCPUS are actually present in the system. > > > > > > Signed-off-by: Jan Henrik Weinstock > > > > Hi Jan, > > > > I cannot apply this patch, it seems you somehow sent it signed as a > > multipart > > message via Thunderbird. > > > > This causes errors when trying to apply, even after I tried to manually fix > > the > > patch mail: > > > > Applying: openrisc: use device tree to determine present cpus > > error: sha1 information is lacking or useless > > (arch/openrisc/kernel/smp.c). > > error: could not build fake ancestor > > Patch failed at 0001 openrisc: use device tree to determine present cpus > > > > Can you send this using 'git send-email?' > > > > If not I can get it applied with some work, otherwise you can point me to a > > git > > repo which I can pull it from. > > "b4 am 6dbc27f8-5261-59c5-acba-70f6c6a74...@rwth-aachen.de" works > fine for me. > > git://git.kernel.org/pub/scm/utils/b4/b4.git Did it work? For me I got, base not found. Looking up https://lore.kernel.org/r/6dbc27f8-5261-59c5-acba-70f6c6a74ba1%40rwth-aachen.de Grabbing thread from lore.kernel.org/lkml Analyzing 9 messages in the thread Will use the latest revision: v2 You can pick other revisions using the -vN flag --- Writing ./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx [PATCH v2] openrisc: use device tree to determine present cpus --- Total patches: 1 --- Link: https://lore.kernel.org/r/6dbc27f8-5261-59c5-acba-70f6c6a74...@rwth-aachen.de Base: not found git am ./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx -Stafford
Re: [PATCH v2] openrisc: use device tree to determine present cpus
On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote: > Use the device tree to determine the present cpus instead of assuming all > CONFIG_NRCPUS are actually present in the system. > > Signed-off-by: Jan Henrik Weinstock Hi Jan, I cannot apply this patch, it seems you somehow sent it signed as a multipart message via Thunderbird. This causes errors when trying to apply, even after I tried to manually fix the patch mail: Applying: openrisc: use device tree to determine present cpus error: sha1 information is lacking or useless (arch/openrisc/kernel/smp.c). error: could not build fake ancestor Patch failed at 0001 openrisc: use device tree to determine present cpus Can you send this using 'git send-email?' If not I can get it applied with some work, otherwise you can point me to a git repo which I can pull it from. -Stafford
Re: [PATCH] openrisc: use device tree to determine present cpus
On Sun, Jan 31, 2021 at 09:22:31AM +0100, Jan Henrik Weinstock wrote: > On 31/01/2021 00:03, Stafford Horne wrote: > > > This looks good, one small comment below. Can you send the next patch as a > > v2? > > > > Using 'git format-patch -v2 ...' > > Sorry, was not aware of that, will do better next time! > > > Should we warn on the else case? > > I think it is fine for the kernel to have room for more CPUs than are > actually present (i.e. NR_CPUs > present_cpus is OK). Other Archs do not > show a warning in this case either, therefore I also omitted it. Fair enough. Reviewed-by: Stafford Horne I can queue this for 5.12 when you send v2. -Stafford
Re: [PATCH] openrisc: use device tree to determine present cpus
Sorry, Please disrecard the g...@lianli.shorne-pla.net address in the mail distribution list. I must have it a incorrect button. -Stafford
Re: [PATCH] openrisc: use device tree to determine present cpus
On Sat, Jan 30, 2021 at 12:00:10PM +0100, Jan Henrik Weinstock wrote: > Hi Stafford, Geert, > > thanks for your feedback. I have made the following changes to the patch: Hi, Thanks for the updates. > 1. use for_each_of_cpu_node > 2. possible_cpus is now what is in the devicetree, up to NR_CPUS > 3. present_cpus is now all possible cpus, up to max_cpus This looks good, one small comment below. Can you send the next patch as a v2? Using 'git format-patch -v2 ...' > Signed-off-by: Jan Henrik Weinstock > --- You can include the 'Changes since v1' in the space here after '---'. > arch/openrisc/kernel/smp.c | 23 +-- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c > index 29c82ef2e..83cbf43d4 100644 > --- a/arch/openrisc/kernel/smp.c > +++ b/arch/openrisc/kernel/smp.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -60,22 +61,32 @@ void __init smp_prepare_boot_cpu(void) > > void __init smp_init_cpus(void) > { > - int i; > + struct device_node* cpu; > + u32 cpu_id; > > - for (i = 0; i < NR_CPUS; i++) > - set_cpu_possible(i, true); > + for_each_of_cpu_node(cpu) { > + if (of_property_read_u32(cpu, "reg", _id)) { > + pr_warn("%s missing reg property", cpu->full_name); > + continue; > + } > + > + if (cpu_id < NR_CPUS) Should we warn on the else case? > + set_cpu_possible(cpu_id, true); > + } > } -Stafford
[GIT PULL] OpenRISC fixes for 5.11-rc6
Hi Linus, Please consider for pull: The following changes since commit 6ee1d745b7c9fd573fba142a2efdad76a9f1cb04: Linux 5.11-rc5 (2021-01-24 16:47:14 -0800) are available in the Git repository at: git://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 1bea2a937dadd188de70198b0cf3915e05a506e4: soc: litex: Properly depend on HAS_IOMEM (2021-01-30 06:36:10 +0900) OpenRISC fixes for 5.11-rc6 Fixes include: * Fix config dependencies for Litex SOC driver causing issues on um David Gow (1): soc: litex: Properly depend on HAS_IOMEM drivers/soc/litex/Kconfig | 1 + 1 file changed, 1 insertion(+) -Stafford
Re: [PATCH] openrisc: use device tree to determine present cpus
On Fri, Jan 29, 2021 at 07:29:31PM +0100, Jan Henrik Weinstock wrote: > This patch proposes to use the device tree to determine the present cpus > instead of assuming all CONFIG_NRCPUS are actually present in the system. > > Signed-off-by: Jan Henrik Weinstock > --- > arch/openrisc/kernel/smp.c | 18 +++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c > index 29c82ef2e..75be7e34f 100644 > --- a/arch/openrisc/kernel/smp.c > +++ b/arch/openrisc/kernel/smp.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -68,14 +69,25 @@ void __init smp_init_cpus(void) > > void __init smp_prepare_cpus(unsigned int max_cpus) > { > - int i; > + u32 cpu_id; > + struct device_node *cpu, *cpus; > > /* >* Initialise the present map, which describes the set of CPUs >* actually populated at the present time. >*/ > - for (i = 0; i < max_cpus; i++) > - set_cpu_present(i, true); > + cpus = of_find_node_by_path("/cpus"); > + for_each_child_of_node(cpus, cpu) { > + if (of_property_read_u32(cpu, "reg", _id)) { > + pr_warn("%s missing reg property", cpu->full_name); > + continue; > + } > + > + if (cpu_id >= max_cpus) > + continue; > + > + set_cpu_present(cpu_id, true); > + } Hello, I looked into what other architectures do. Risc-V does something similar but it does the setup in 2 parts: - it uses the device tree to set possible CPU's in setup_smp. Using for_each_of_cpu_node and set_cpu_possible. - Then in smp_prepare_cpus, it loops over possible cpus with for_each_possible_cpu. Note, it seems risc-v does't actually check max_cpus when setting set_cpu_present which may be a bug. I think the two part approach is what we want to do: - we should do set_cpu_possible in smp_init_cpus based on device tree. Basically the same as your loop above but using for_each_of_cpu_node and NR_CPUS. - we can then do set_cpu_present using for_each_possible_cpu in smp_prepare_cpus. What do you think? -Stafford > } > > void __init smp_cpus_done(unsigned int max_cpus) > -- > 2.17.1 >
Re: [PATCH] soc: litex: Properly depend on HAS_IOMEM
On Thu, Jan 28, 2021 at 12:49:56AM +0800, David Gow wrote: > On Wed, Jan 27, 2021 at 8:27 PM Stafford Horne wrote: > > > > On Tue, Jan 26, 2021 at 07:36:04PM -0800, David Gow wrote: > > > The LiteX SOC controller driver makes use of IOMEM functions like > > > devm_platform_ioremap_resource(), which are only available if > > > CONFIG_HAS_IOMEM is defined. > > > > > > This causes the driver not to be enable under make ARCH=um allyesconfig, > > > even though it won't build. > > > > Is this wording correct? I suspect it causes to driver TO BE enabled. > > > > Whoops! Yes: that should be "causes the driver to be enabled" -- sorry > about that. > > Let me know if you want me to re-send it with the fixed wording, or if > you just want to fix that yourself. OK, I'll fix it. > > > > > > By adding a dependency on HAS_IOMEM, the driver will not be enabled on > > > architectures which don't support it. > > > > > > Fixes: 22447a99c97e ("drivers/soc/litex: add LiteX SoC Controller driver") > > > Signed-off-by: David Gow a > > > > This looks ok to me. I can queue it for 5.11 fixes, if you can help with > > the > > wording above. > > Thanks: that'd be great! No problem. -Stafford
Re: [PATCH] soc: litex: Properly depend on HAS_IOMEM
On Tue, Jan 26, 2021 at 07:36:04PM -0800, David Gow wrote: > The LiteX SOC controller driver makes use of IOMEM functions like > devm_platform_ioremap_resource(), which are only available if > CONFIG_HAS_IOMEM is defined. > > This causes the driver not to be enable under make ARCH=um allyesconfig, > even though it won't build. Is this wording correct? I suspect it causes to driver TO BE enabled. > > By adding a dependency on HAS_IOMEM, the driver will not be enabled on > architectures which don't support it. > > Fixes: 22447a99c97e ("drivers/soc/litex: add LiteX SoC Controller driver") > Signed-off-by: David Gow a This looks ok to me. I can queue it for 5.11 fixes, if you can help with the wording above. -Stafford > --- > drivers/soc/litex/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig > index 7c6b009b6f6c..7a7c38282e11 100644 > --- a/drivers/soc/litex/Kconfig > +++ b/drivers/soc/litex/Kconfig > @@ -8,6 +8,7 @@ config LITEX > config LITEX_SOC_CONTROLLER > tristate "Enable LiteX SoC Controller driver" > depends on OF || COMPILE_TEST > + depends on HAS_IOMEM > select LITEX > help > This option enables the SoC Controller Driver which verifies > -- > 2.30.0.280.ga3ce27912f-goog >
Re: linux-next: manual merge of the openrisc tree with Linus' tree
On Mon, Jan 25, 2021 at 12:47:46PM +1100, Stephen Rothwell wrote: > Hi Stafford, > > On Mon, 25 Jan 2021 10:04:46 +0900 Stafford Horne wrote: > > > > Thank's I knew about this conflict but I was not sure the best way to > > handle, I > > was/am going to rebase the openrisc/for-next branch onto 5.11-rc5 once > > released. > > I will resolve the conflict during the rebase so you should be able to drop > > the > > conflict patch after that. > > Its a pretty trivial conflict, so I wouldn't do the rebase just for this. Alright, I will not rebase. > > The issue is I had a fix that went straight to 5.11. Should I usually put > > these > > kind of fixes on my for-next and my fixes branches in parallel, that way I > > can > > resolve conflicts on for-next before hand? > > I notice that the version in Linus' tree was merged from a separate > branch. The easiest that to do is for you to merge that same branch > into your for-next branch - that way you only get your own changes, not > any other stuff that might be in Linus' tree. > > > I don't usually do that as in my mind for next is for 5.12 and fixes for > > 5.11 go > > straight to 5.11. Also, I don't like putting the same patch in 2 queues. > > But > > if I got any advice on how to avoid this in the future it would be > > appreciated. > > Like I said, just merge your fixes branch into you for-next branch > when/if you think the fixes are important for further development, or > the conflicts become to great. That sounds like a good idea. Let me do that. > I can also add you fixes branch to linux-next if you like (I already > have 86 other "fixes" branches). I think that should be alright for now, I'll maintain merging the fixes branch myself when I think it's needed. Thank you, -Stafford
Re: linux-next: manual merge of the openrisc tree with Linus' tree
On Mon, Jan 25, 2021 at 09:05:06AM +1100, Stephen Rothwell wrote: > Hi all, > > Today's linux-next merge of the openrisc tree got a conflict in: > > drivers/soc/litex/litex_soc_ctrl.c > > between commit: > > e6dc077b7dff ("soc: litex: Fix compile warning when device tree is not > configured") > > from Linus' tree and commit: > > 3706f9f76a4f ("drivers/soc/litex: Add restart handler") > > from the openrisc tree. > > I fixed it up (see below) and can carry the fix as necessary. This > is now fixed as far as linux-next is concerned, but any non trivial > conflicts should be mentioned to your upstream maintainer when your tree > is submitted for merging. You may also want to consider cooperating > with the maintainer of the conflicting tree to minimise any particularly > complex conflicts. Hi Stephen, Thank's I knew about this conflict but I was not sure the best way to handle, I was/am going to rebase the openrisc/for-next branch onto 5.11-rc5 once released. I will resolve the conflict during the rebase so you should be able to drop the conflict patch after that. The issue is I had a fix that went straight to 5.11. Should I usually put these kind of fixes on my for-next and my fixes branches in parallel, that way I can resolve conflicts on for-next before hand? I don't usually do that as in my mind for next is for 5.12 and fixes for 5.11 go straight to 5.11. Also, I don't like putting the same patch in 2 queues. But if I got any advice on how to avoid this in the future it would be appreciated. Thank you, -Stafford
[GIT PULL] OpenRISC fixes for 5.11
Hi Linus, Please consider for pull: The following changes since commit 7c53f6b671f4aba70ff15e1b05148b10d58c2837: Linux 5.11-rc3 (2021-01-10 14:34:50 -0800) are available in the Git repository at: git://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 031c7a8cd6fc565e90320bf08f22ee6e70f9d969: openrisc: io: Add missing __iomem annotation to iounmap() (2021-01-20 06:14:26 +0900) OpenRISC fixes for 5.11 Fixes include: * Compiler warning fixup for new Litex SoC driver * Sparse warning fixup for iounmap Geert Uytterhoeven (1): openrisc: io: Add missing __iomem annotation to iounmap() Stafford Horne (1): soc: litex: Fix compile warning when device tree is not configured arch/openrisc/include/asm/io.h | 2 +- arch/openrisc/mm/ioremap.c | 2 +- drivers/soc/litex/litex_soc_ctrl.c | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) -Stafford
Re: [PATCH v4] drivers/soc/litex: Add restart handler
On Wed, Jan 20, 2021 at 09:20:39AM +0100, Geert Uytterhoeven wrote: > Hi Stafford, > > On Tue, Jan 19, 2021 at 11:11 PM Stafford Horne wrote: > > On Wed, Jan 20, 2021 at 06:34:44AM +0900, Stafford Horne wrote: > > > On Tue, Jan 19, 2021 at 09:09:38AM +0100, Geert Uytterhoeven wrote: > > > > Let the LiteX SoC Controller register a restart handler, which resets > > > > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > > > > > > > Signed-off-by: Geert Uytterhoeven > > > > > > Thanks, this looks good to me, queued to my linux-next branch. > > > > > > -Stafford > > > > > > > @@ -66,8 +71,19 @@ static int litex_check_csr_access(void __iomem > > > > *reg_addr) > > > > > > > > struct litex_soc_ctrl_device { > > > > void __iomem *base; > > > > + struct notifier_block reset_nb; > > > > }; > > > > > > > > +static int litex_reset_handler(struct notifier_block *this, unsigned > > > > long mode, > > > > + void *cmd) > > > > +{ > > > > + struct litex_soc_ctrl_device *soc_ctrl_dev = > > > > + container_of(this, struct litex_soc_ctrl_device, reset_nb); > > > > > > Nice. > > > > > > > + litex_write32(soc_ctrl_dev->base + RESET_REG_OFF, RESET_REG_VALUE); > > > > + return NOTIFY_DONE; > > > > +} > > > > + > > > > Actually, I tested this out on the latest (2-weeks ago) Litex and > > openrisc/for-next and it didn't seem to work correctly. > > > > I will look into it a bit closer, but if you see or can think of anything > > let > > me know. Note There are a few failures below related to network services > > as my > > for-next kernel doesnt have a network driver (yet). > > Hmmm, openrisc/for-next does have commit 131172a4a8ce3fcc ("openrisc: > restart: Call common handlers before hanging"). > > It's been a few years I used an OpenRISC setup. > Do you have a link to Linux on mor1kx/LiteX setup instructions? Actually, I rebuilt everything again and it works fine. FYI, my instructions for setup are here: - https://github.com/stffrdhrn/or1k-utils/tree/master/litex I use an arty dev board, you can see the reset working below. There are some things I need to fix in my dev rootfs shutdown scripts but it does reset. # shutdown -r now Broadcast message from root@buildroot (console) (Thu Jan 1 00:01:07 1970): The system is going down for reboot NOW! INIT: Switching to runlevel: 6 # mounting home work nfs ... mount: mounting 10.0.0.27:/home/shorne/work on /home/shorne/work failed: No such device enabling login for shorne ... setting coredumps ... Stopping dropbear sshd: FAIL Stopping ntpd: FAIL Nothing to do, sntp is not a daemon. Stopping network: ifdown: interface lo not configured ifdown: interface eth0 not configured OK Saving random seed: [ 70.96] random: dd: uninitialized urandom read (512 bytes read) OK Stopping klogd: OK Stopping syslogd: start-stop-daemon: warning: killing process 51: No such process FAIL umount: devtmpfs busy - remounted read-only umount: can't unmount /: Invalid argument [ 72.65] reboot: Res __ _ __ _ __ / / (_) / | |/_/ / /__/ / __/ -_)> < //_/\__/\__/_/|_| Build your hardware, easily! (c) Copyright 2012-2020 Enjoy-Digital (c) Copyright 2007-2015 M-Labs BIOS built on Jan 20 2021 21:18:10 BIOS CRC passed (b12f1de3) Migen git sha1: 40b1092 LiteX git sha1: 57289dd4 --=== SoC ==-- CPU:MOR1KX @ 100MHz BUS:WISHBONE 32-bit @ 4GiB -Stafford
Re: [PATCH v4] drivers/soc/litex: Add restart handler
On Wed, Jan 20, 2021 at 06:34:44AM +0900, Stafford Horne wrote: > On Tue, Jan 19, 2021 at 09:09:38AM +0100, Geert Uytterhoeven wrote: > > Let the LiteX SoC Controller register a restart handler, which resets > > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > > > Signed-off-by: Geert Uytterhoeven > > Thanks, this looks good to me, queued to my linux-next branch. > > -Stafford > > > @@ -66,8 +71,19 @@ static int litex_check_csr_access(void __iomem *reg_addr) > > > > struct litex_soc_ctrl_device { > > void __iomem *base; > > + struct notifier_block reset_nb; > > }; > > > > +static int litex_reset_handler(struct notifier_block *this, unsigned long > > mode, > > + void *cmd) > > +{ > > + struct litex_soc_ctrl_device *soc_ctrl_dev = > > + container_of(this, struct litex_soc_ctrl_device, reset_nb); > > Nice. > > > + litex_write32(soc_ctrl_dev->base + RESET_REG_OFF, RESET_REG_VALUE); > > + return NOTIFY_DONE; > > +} > > + Actually, I tested this out on the latest (2-weeks ago) Litex and openrisc/for-next and it didn't seem to work correctly. I will look into it a bit closer, but if you see or can think of anything let me know. Note There are a few failures below related to network services as my for-next kernel doesnt have a network driver (yet). Using my buildroot rootfs: http://shorne.noip.me/downloads/or1k-glibc-rootfs.cpio.gz # shutdown -r now Broadcast message from root@buildroot (console) (Thu Jan 1 00:00:48 1970): The system is going down for reboot NOW! INIT: Switching to runlevel: 6 # mounting home work nfs ... mount: mounting 10.0.0.27:/home/shorne/work on /home/shorne/work failed: No such device enabling login for shorne ... setting coredumps ... Stopping dropbear sshd: FAIL Stopping ntpd: FAIL Nothing to do, sntp is not a daemon. Stopping network: ifdown: interface lo not configured ifdown: interface eth0 not configured OK Saving random seed: [ 52.02] random: dd: uninitialized urandom read (512 bytes read) OK Stopping klogd: OK Stopping syslogd: start-stop-daemon: warning: killing process 51: No such process FAIL umount: devtmpfs busy - remounted read-only umount: can't unmount /: Invalid argument [ 53.71] reboot: Restarting system [ 54.71] Reboot failed -- System halted [ 76.04] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [reboot:131] [ 76.04] CPU: 0 PID: 131 Comm: reboot Not tainted 5.11.0-rc1-9-gff28dae0bc90 #418 [ 76.04] CPU #: 0 [ 76.04]PC: c00050b8SR: 827fSP: c180fdb0 [ 76.04] GPR00: GPR01: c180fdb0 GPR02: c180fdc0 GPR03: 827f [ 76.04] GPR04: c0348944 GPR05: GPR06: c180fc70 GPR07: [ 76.04] GPR08: c180fdb0 GPR09: c00050b8 GPR10: c180e000 GPR11: 001e [ 76.04] GPR12: GPR13: 0020 GPR14: 0001 GPR15: [ 76.04] GPR16: GPR17: c02e48d4 GPR18: 00418958 GPR19: c02e48d4 [ 76.04] GPR20: GPR21: GPR22: c02e4018 GPR23: fffe [ 76.04] GPR24: GPR25: GPR26: GPR27: [ 76.04] GPR28: GPR29: GPR30: GPR31: [ 76.04] RES: 001e oGPR11: [ 76.04] Process reboot (pid: 131, stackpage=c180a000) [ 76.04] [ 76.04] Stack: [ 76.04] Call trace: [ 76.04] [<(ptrval)>] machine_restart+0x44/0x5c [ 76.04] [<(ptrval)>] kernel_restart+0x78/0xa4 [ 76.04] [<(ptrval)>] ? mutex_lock+0x24/0x50 [ 76.04] [<(ptrval)>] __do_sys_reboot+0x1a8/0x21c [ 76.04] [<(ptrval)>] ? do_filp_open+0x40/0xa0 [ 76.04] [<(ptrval)>] ? slab_free_freelist_hook+0x6c/0x14c [ 76.04] [<(ptrval)>] ? arch_local_irq_save+0x24/0x3c [ 76.04] [<(ptrval)>] ? kmem_cache_free+0x130/0x194 [ 76.04] [<(ptrval)>] ? call_rcu+0x50/0x8c [ 76.04] [<(ptrval)>] ? __fput+0x2d0/0x2f4 [ 76.04] [<(ptrval)>] ? do_sys_openat2+0xd8/0x134 [ 76.04] [<(ptrval)>] ? task_work_run+0xbc/0xf4 [ 76.04] [<(ptrval)>] ? do_work_pending+0x60/0x12c [ 76.04] [<(ptrval)>] sys_reboot+0x14/0x24 [ 76.04] [<(ptrval)>] ? _syscall_return+0x0/0x4 -Stafford
Re: [PATCH v4] drivers/soc/litex: Add restart handler
On Tue, Jan 19, 2021 at 09:09:38AM +0100, Geert Uytterhoeven wrote: > Let the LiteX SoC Controller register a restart handler, which resets > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > Signed-off-by: Geert Uytterhoeven Thanks, this looks good to me, queued to my linux-next branch. -Stafford > @@ -66,8 +71,19 @@ static int litex_check_csr_access(void __iomem *reg_addr) > > struct litex_soc_ctrl_device { > void __iomem *base; > + struct notifier_block reset_nb; > }; > > +static int litex_reset_handler(struct notifier_block *this, unsigned long > mode, > +void *cmd) > +{ > + struct litex_soc_ctrl_device *soc_ctrl_dev = > + container_of(this, struct litex_soc_ctrl_device, reset_nb); Nice. > + litex_write32(soc_ctrl_dev->base + RESET_REG_OFF, RESET_REG_VALUE); > + return NOTIFY_DONE; > +} > +
Re: [PATCH] openrisc: io: Add missing __iomem annotation to iounmap()
On Mon, Dec 28, 2020 at 09:33:28AM +0100, Geert Uytterhoeven wrote: > With C=1: > > drivers/soc/renesas/rmobile-sysc.c:330:33: sparse: sparse: incorrect type > in argument 1 (different address spaces) @@ expected void *addr @@ > got void [noderef] __iomem *[assigned] base @@ > drivers/soc/renesas/rmobile-sysc.c:330:33: sparse: expected void *addr > drivers/soc/renesas/rmobile-sysc.c:330:33: sparse: got void [noderef] > __iomem *[assigned] base > > Fix this by adding the missing __iomem annotation to iounmap(). > > Reported-by: kernel test robot > Signed-off-by: Geert Uytterhoeven Reviewed and OK. I queued this to my 5.11 fixes branch. I will send a PR for this over the weekend. https://github.com/openrisc/linux/commits/or1k-5.11-fixes (Found this one in spam and hopefully fixed up my filters now) -Stafford
Re: [PATCH v2] drivers/soc/litex: Add restart handler
On Mon, Jan 18, 2021 at 01:27:32PM +0100, Geert Uytterhoeven wrote: > Hi Stafford, > > On Mon, Jan 18, 2021 at 12:43 PM Stafford Horne wrote: > > On Thu, Jan 14, 2021 at 02:48:49PM +0100, Geert Uytterhoeven wrote: > > > On Thu, Jan 14, 2021 at 3:03 AM Stafford Horne wrote: > > > > On Mon, Jan 04, 2021 at 05:49:03PM +0100, Geert Uytterhoeven wrote: > > > > > On Mon, Jan 4, 2021 at 5:45 PM Geert Uytterhoeven > > > > > wrote: > > > > > > Let the LiteX SoC Controller a register a restart handler, which > > > > > > resets I think there is a typo here: Let the LiteX SoC Controller a register a restart ... should remove the first 'a' and say Let the LiteX SoC Controller register a restart ... > > > > > > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > > > > > > > > > > > Signed-off-by: Geert Uytterhoeven > > > > > > --- > > > > > > Tested with linux-on-litex-vexriscv. > > > > > > > > > > > > This patch is based on upstream, i.e. not on top of Gabriel Somlo's > > > > > > "[PATCH v5 0/4] drivers/soc/litex: support 32-bit subregisters, > > > > > > 64-bit > > > > > > CPUs" > > > > > > (https://lore.kernel.org/lkml/20201227161320.2194830-1-gso...@gmail.com/) > > > > > > > > > > Bummer, and that's why the RESET_REG_* definitions are no longer > > > > > next to the SCRATCH_REG_* definitions :-( > > > > > > > > If it helps I have accepted Gabriel's patches and put them onto > > > > for-next. > > > > > > > > https://github.com/openrisc/linux/commits/for-next > > > > > > > > I am happy to take and test a patch based on that. Or I can do the > > > > adjustments > > > > to base the patch on that myself. Let me know. > > > > > > Thanks for letting me know! V3 sent. > > > > Hi Geert, > > > > I don't seem to see v3 anywhere. Where did you send it and what is the > > subject? > > https://lore.kernel.org/linux-riscv/20210114134813.2238587-1-ge...@linux-m68k.org/ > > So "b4 am 20210114134813.2238587-1-ge...@linux-m68k.org" should give you > a copy. Thanks I got it, I am not sure why it does not show up in my inbox anywhere, sometimes gmail drops mails. Hence, I am replying here. As per the typo above I can fix during applying or you could send during a v4. One more small nit is that you move soc_ctrl_dev out to a static instance it might help to mention. But it's easy to see why. -Stafford
Re: [PATCH] openrisc: add arch/openrisc/Kbuild
On Sun, Jan 17, 2021 at 05:03:32PM +0900, Masahiro Yamada wrote: > Describe the subdirectories under arch/openrisc/ in arch/openrisc/Kbuild > so you can use the standard obj-y syntax. > > I removed the CONFIG_OPENRISC_BUILTIN_DTB conditional because it is > already controlled by arch/openrisc/boot/dts/Makefile. > > Signed-off-by: Masahiro Yamada Thank you, This looks OK to me, I have queued this for next. There was a small conflict with the vmlinux.bin changes you reviewed earlier. I resolved them and did a test bild and it seems to all be fine. -Stafford
Re: [PATCH v2] drivers/soc/litex: Add restart handler
On Thu, Jan 14, 2021 at 02:48:49PM +0100, Geert Uytterhoeven wrote: > Hi Stafford, > > On Thu, Jan 14, 2021 at 3:03 AM Stafford Horne wrote: > > On Mon, Jan 04, 2021 at 05:49:03PM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 4, 2021 at 5:45 PM Geert Uytterhoeven > > > wrote: > > > > Let the LiteX SoC Controller a register a restart handler, which resets > > > > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > > > > > > > Signed-off-by: Geert Uytterhoeven > > > > --- > > > > Tested with linux-on-litex-vexriscv. > > > > > > > > This patch is based on upstream, i.e. not on top of Gabriel Somlo's > > > > "[PATCH v5 0/4] drivers/soc/litex: support 32-bit subregisters, 64-bit > > > > CPUs" > > > > (https://lore.kernel.org/lkml/20201227161320.2194830-1-gso...@gmail.com/) > > > > > > Bummer, and that's why the RESET_REG_* definitions are no longer > > > next to the SCRATCH_REG_* definitions :-( > > > > If it helps I have accepted Gabriel's patches and put them onto for-next. > > > > https://github.com/openrisc/linux/commits/for-next > > > > I am happy to take and test a patch based on that. Or I can do the > > adjustments > > to base the patch on that myself. Let me know. > > Thanks for letting me know! V3 sent. Hi Geert, I don't seem to see v3 anywhere. Where did you send it and what is the subject? -Stafford
Re: [PATCH v2] drivers/soc/litex: Add restart handler
On Mon, Jan 04, 2021 at 05:49:03PM +0100, Geert Uytterhoeven wrote: > On Mon, Jan 4, 2021 at 5:45 PM Geert Uytterhoeven > wrote: > > Let the LiteX SoC Controller a register a restart handler, which resets > > the LiteX SoC by writing 1 to CSR_CTRL_RESET_ADDR. > > > > Signed-off-by: Geert Uytterhoeven > > --- > > Tested with linux-on-litex-vexriscv. > > > > This patch is based on upstream, i.e. not on top of Gabriel Somlo's > > "[PATCH v5 0/4] drivers/soc/litex: support 32-bit subregisters, 64-bit > > CPUs" > > (https://lore.kernel.org/lkml/20201227161320.2194830-1-gso...@gmail.com/) > > Bummer, and that's why the RESET_REG_* definitions are no longer > next to the SCRATCH_REG_* definitions :-( If it helps I have accepted Gabriel's patches and put them onto for-next. https://github.com/openrisc/linux/commits/for-next I am happy to take and test a patch based on that. Or I can do the adjustments to base the patch on that myself. Let me know. -Stafford > Well, I assume that will be fixed by evolution ;-) > > > v2: > > - Rebase on top of v5.11-rc1, > > - Change reset handler priority to recommended default value of 128 > > (was 192). > > > > (v1 was not sent to a mailing list) > > --- > > drivers/soc/litex/litex_soc_ctrl.c | 33 +++--- > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/soc/litex/litex_soc_ctrl.c > > b/drivers/soc/litex/litex_soc_ctrl.c > > index 1217cafdfd4d1d2b..d729ad50d4ffca5e 100644 > > --- a/drivers/soc/litex/litex_soc_ctrl.c > > +++ b/drivers/soc/litex/litex_soc_ctrl.c > > @@ -15,6 +15,11 @@ > > #include > > #include > > #include > > +#include > > + > > +/* reset register located at the base address */ > > +#define RESET_REG_OFF 0x00 > > +#define RESET_REG_VALUE 0x0001 > > > > /* > > * LiteX SoC Generator, depending on the configuration, can split a single > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > ge...@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
Re: [PATCH v6 0/5] drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs
I have queued this for 5.12. On Tue, Jan 12, 2021 at 12:31:39PM -0500, Gabriel Somlo wrote: > This series expands on commit 22447a99c97e ("drivers/soc/litex: add LiteX > SoC Controller driver"), adding support for handling both 8- and 32-bit > LiteX CSR (MMIO) subregisters, on both 32- and 64-bit CPUs. > > Notes v6: > - split out s/LITEX_REG_SIZE/LITEX_SUBREG_ALIGN/g change > into its own dedicated (cosmetic-only) patch (3/5). > - fixed typos in "main patch" (now 4/5) changelog. > - fixed typos in comments added via patch 5/5. > > Notes v5: > - added patch (4/4) taking 'litex_[set|get]_reg()' private > - additional optimization of [_]litex_set_reg() in 3/4 > > Notes v4: > - improved "eloquence" of some 3/3 commit blurb paragraphs > - fixed instance of "disgusting" comment style :) > - litex_[get|set]_reg() now using size_t for 'reg_size' argument > - slightly tighter shift calculation in litex_set_reg() > > Notes v3: > - split into smaller, more self-explanatory patches > - more detailed commit blurb for "main payload" (3/3) patch > - eliminate compiler warning on 32-bit architectures > > Notes v2: > - fix typo (s/u32/u64/) in litex_read64(). > > Notes v1: > - LITEX_SUBREG_SIZE now provided by Kconfig. > - it's not LITEX_REG_SIZE, but rather LITEX_SUBREG_ALIGN! > - move litex_[get|set]_reg() to include/linux/litex.h and mark > them as "static inline"; > - redo litex_[read|write][8|16|32|64]() using litex_[get|set]_reg() > (compiler should produce code as efficient as hardcoded shifts, > but also automatically matching LITEX_SUBREG_SIZE). > > Gabriel Somlo (5): > drivers/soc/litex: move generic accessors to litex.h > drivers/soc/litex: separate MMIO from subregister offset calculation > drivers/soc/litex: rename LITEX_REG_SIZE to LITEX_SUBREG_ALIGN > drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs > drivers/soc/litex: make 'litex_[set|get]_reg()' methods private > > drivers/soc/litex/Kconfig | 14 ++- > drivers/soc/litex/litex_soc_ctrl.c | 76 +- > include/linux/litex.h | 154 +++-- > 3 files changed, 119 insertions(+), 125 deletions(-) > > -- > 2.26.2 >
[PATCH] soc: litex: Fix compile warning when device tree is not configured
The test robot reported: drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: unused variable 'litex_soc_ctrl_of_match' [-Wunused-const-variable] static const struct of_device_id litex_soc_ctrl_of_match[] = { ^ 1 warning generated. As per the random config device tree is not configured causing the litex_soc_ctrl_of_match match list to not be used. This would usually mean that we cannot even use this driver as it depends on device tree, but as we also have COMPILE_TEST configured we allow it. Fix the warning by surrounding the unused variable with an ifdef CONFIG_OF. Reported-by: kernel test robot Signed-off-by: Stafford Horne --- drivers/soc/litex/litex_soc_ctrl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c index 1217cafdfd4d..9b0766384570 100644 --- a/drivers/soc/litex/litex_soc_ctrl.c +++ b/drivers/soc/litex/litex_soc_ctrl.c @@ -140,12 +140,13 @@ struct litex_soc_ctrl_device { void __iomem *base; }; +#ifdef CONFIG_OF static const struct of_device_id litex_soc_ctrl_of_match[] = { {.compatible = "litex,soc-controller"}, {}, }; - MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match); +#endif /* CONFIG_OF */ static int litex_soc_ctrl_probe(struct platform_device *pdev) { -- 2.26.2
Re: drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: unused variable 'litex_soc_ctrl_of_match'
On Mon, Jan 11, 2021 at 09:43:34AM -0700, Nathan Chancellor wrote: > On Mon, Jan 11, 2021 at 09:30:55PM +0900, Stafford Horne wrote: > > On Thu, Jan 07, 2021 at 04:04:47AM +0800, kernel test robot wrote: > > > Hi Pawel, > > > > > > FYI, the error/warning still remains. > > > > > > tree: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > head: e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62 > > > commit: 22447a99c97e353bde8f90c2353873f27681d57c drivers/soc/litex: add > > > LiteX SoC Controller driver > > > date: 8 weeks ago > > > config: x86_64-randconfig-a001-20210107 (attached as .config) > > > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project > > > 5c951623bc8965fa1e89660f2f5f4a2944e4981a) > > > reproduce (this is a W=1 build): > > > wget > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross > > > -O ~/bin/make.cross > > > chmod +x ~/bin/make.cross > > > # install x86_64 cross compiling tool for clang build > > > # apt-get install binutils-x86-64-linux-gnu > > > # > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22447a99c97e353bde8f90c2353873f27681d57c > > > git remote add linus > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > > git fetch --no-tags linus master > > > git checkout 22447a99c97e353bde8f90c2353873f27681d57c > > > # save the attached .config to linux build tree > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross > > > ARCH=x86_64 > > > > > > If you fix the issue, kindly add following tag as appropriate > > > Reported-by: kernel test robot > > > > > > All warnings (new ones prefixed by >>): > > > > > > >> drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: unused variable > > > >> 'litex_soc_ctrl_of_match' [-Wunused-const-variable] > > >static const struct of_device_id litex_soc_ctrl_of_match[] = { > > > ^ > > >1 warning generated. > > > > > > > > > vim +/litex_soc_ctrl_of_match +143 drivers/soc/litex/litex_soc_ctrl.c > > > > > >142 > > > > 143static const struct of_device_id litex_soc_ctrl_of_match[] = { > > >144{.compatible = "litex,soc-controller"}, > > >145{}, > > >146}; > > >147 > > > > > > > I don't use clang but GCC, and I cannot reproduce this warning. > > > > $ make drivers/soc/litex/litex_soc_ctrl.o > > CALLscripts/checksyscalls.sh > > CALLscripts/atomic/check-atomics.sh > > DESCEND objtool > > CC drivers/soc/litex/litex_soc_ctrl.o > > > > Also, I can see litex_soc_ctrl_of_match is used. I am not sure what is > > going on > > here. > > > > -Stafford > > > > You need W=1 > > $ make -skj"$(nproc)" W=1 olddefconfig drivers/soc/litex/litex_soc_ctrl.o > drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: ‘litex_soc_ctrl_of_match’ > defined but not used [-Wunused-const-variable=] > 143 | static const struct of_device_id litex_soc_ctrl_of_match[] = { > | ^~~ > > $ rg "CONFIG_OF|CONFIG_LITEX_SOC_CONTROLLER" .config > 1124:# CONFIG_OF is not set > 4673:CONFIG_LITEX_SOC_CONTROLLER=y > > This variable is used in two places in that file, in the > MODULE_DEVICE_TABLE macro and the of_match_ptr macro. When CONFIG_OF is > disabled, of_match_ptr evaluates to NULL. When the file is built into > the kernel image, MODULE_DEVICE_TABLE evaluates to nothing, leaving this > variable defined but unused in the final preprocessed source. > > Hope that helps! That helps, I noticed it was only used in those macros so that was fishy. I forgot to add W=1. We will need to surround the definition in: #if defined(CONFIG_OF) #endif /* CONFIG_OF */
Re: drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: unused variable 'litex_soc_ctrl_of_match'
On Thu, Jan 07, 2021 at 04:04:47AM +0800, kernel test robot wrote: > Hi Pawel, > > FYI, the error/warning still remains. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62 > commit: 22447a99c97e353bde8f90c2353873f27681d57c drivers/soc/litex: add LiteX > SoC Controller driver > date: 8 weeks ago > config: x86_64-randconfig-a001-20210107 (attached as .config) > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project > 5c951623bc8965fa1e89660f2f5f4a2944e4981a) > reproduce (this is a W=1 build): > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # install x86_64 cross compiling tool for clang build > # apt-get install binutils-x86-64-linux-gnu > # > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22447a99c97e353bde8f90c2353873f27681d57c > git remote add linus > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git fetch --no-tags linus master > git checkout 22447a99c97e353bde8f90c2353873f27681d57c > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross > ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All warnings (new ones prefixed by >>): > > >> drivers/soc/litex/litex_soc_ctrl.c:143:34: warning: unused variable > >> 'litex_soc_ctrl_of_match' [-Wunused-const-variable] >static const struct of_device_id litex_soc_ctrl_of_match[] = { > ^ >1 warning generated. > > > vim +/litex_soc_ctrl_of_match +143 drivers/soc/litex/litex_soc_ctrl.c > >142 > > 143static const struct of_device_id litex_soc_ctrl_of_match[] = { >144{.compatible = "litex,soc-controller"}, >145{}, >146}; >147 > I don't use clang but GCC, and I cannot reproduce this warning. $ make drivers/soc/litex/litex_soc_ctrl.o CALLscripts/checksyscalls.sh CALLscripts/atomic/check-atomics.sh DESCEND objtool CC drivers/soc/litex/litex_soc_ctrl.o Also, I can see litex_soc_ctrl_of_match is used. I am not sure what is going on here. -Stafford
Re: [PATCH] openrisc: restart: Call common handlers before hanging
On Sun, Dec 27, 2020 at 07:44:46PM +1030, Joel Stanley wrote: > Currently openrisc will print a message and then hang in an infinite > loop when rebooting. > > This patch adopts some code from ARM, which calls the common restart > infrastructure and hangs after a small delay if the restart infra > doesn't do anything. > > Signed-off-by: Joel Stanley > --- > Geert has a patch[1] for the litex soc code that adds a restart hander. > Openrisc doesn't hit that code path, this patch fixes that. > > [1] > https://github.com/geertu/linux/commit/7d09dc0797a8208a11eb7c0c2156c1a4c120180f > > arch/openrisc/kernel/process.c | 13 ++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/arch/openrisc/kernel/process.c b/arch/openrisc/kernel/process.c > index 3c98728cce24..181448f74316 100644 > --- a/arch/openrisc/kernel/process.c > +++ b/arch/openrisc/kernel/process.c > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -49,10 +50,16 @@ > */ > struct thread_info *current_thread_info_set[NR_CPUS] = { _thread_info, > }; > > -void machine_restart(void) > +void machine_restart(char *cmd) > { > - printk(KERN_INFO "*** MACHINE RESTART ***\n"); > - __asm__("l.nop 1"); Just a note, this nop with argument 1, is used by the simulators to shutdown. I am happy to get rid of this though. The simulator should be simulating how real hardware is shut down not doing these tricks. > + do_kernel_restart(cmd); As you mentioned this depends on Geert's patch. Does he plan to submit it soon? Geert is CCed. > + > + /* Give a grace period for failure to restart of 1s */ > + mdelay(1000); > + > + /* Whoops - the platform was unable to reboot. Tell the user! */ > + pr_emerg("Reboot failed -- System halted\n"); > + while (1); > } > > /* > -- > 2.29.2 I am queing this for 5.11 anyway is it hurt's nothing without Geert's patch. -Stafford
Re: [PATCH v4 0/3] drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs
On Fri, Dec 25, 2020 at 07:16:46PM -0500, Gabriel Somlo wrote: > This series expands on commit 22447a99c97e ("drivers/soc/litex: add LiteX > SoC Controller driver"), adding support for handling both 8- and 32-bit > LiteX CSR (MMIO) subregisters, on both 32- and 64-bit CPUs. > > Notes v4: > - improved "eloquence" of some 3/3 commit blurb paragraphs > - fixed instance of "disgusting" comment style :) > - litex_[get|set]_reg() now using size_t for 'reg_size' argument > - slightly tighter shift calculation in litex_set_reg() > > Notes v3: > - split into smaller, more self-explanatory patches > - more detailed commit blurb for "main payload" (3/3) patch > - eliminate compiler warning on 32-bit architectures > > Notes v2: > - fix typo (s/u32/u64/) in litex_read64(). > > Notes v1: > - LITEX_SUBREG_SIZE now provided by Kconfig. > - it's not LITEX_REG_SIZE, but rather LITEX_SUBREG_ALIGN! > - move litex_[get|set]_reg() to include/linux/litex.h and mark > them as "static inline"; > - redo litex_[read|write][8|16|32|64]() using litex_[get|set]_reg() > (compiler should produce code as efficient as hardcoded shifts, > but also automatically matching LITEX_SUBREG_SIZE). > > Gabriel Somlo (3): > drivers/soc/litex: move generic accessors to litex.h > drivers/soc/litex: separate MMIO from subregister offset calculation > drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs Thanks, this look good to me right now. I will wait for other reviewers to chime in over the next week or two after the holidays. Some minor things: - It's customary to add "Changes since xx" on each patch after "---", See: https://kernelnewbies.org/PatchTipsAndTricks - Did you run your patches through "./scripts/checkpatch.pl"? - Did you use "./scripts/get_maintainer.pl" to create the cc list? I usually run: git send-email --to linux-kernel --cc-cmd ./scripts/get_maintainer.pl ... From your cc list it seems you did, but maybe did it manually. (I setup .get_maintainer.conf with --no-rolestats for this to work) - In general check out: https://www.kernel.org/doc/html/latest/process/submit-checklist.html -Stafford > drivers/soc/litex/Kconfig | 12 +++ > drivers/soc/litex/litex_soc_ctrl.c | 76 +-- > include/linux/litex.h | 151 +++-- > 3 files changed, 115 insertions(+), 124 deletions(-) > > -- > 2.26.2 >
Re: [PATCH v3 3/3] drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs
On Fri, Dec 25, 2020 at 09:21:20AM -0500, Gabriel Somlo wrote: > Upstream LiteX now defaults to using 32-bit CSR subregisters > (see https://github.com/enjoy-digital/litex/commit/a2b71fde). > > This patch expands on commit 22447a99c97e ("drivers/soc/litex: add > LiteX SoC Controller driver"), adding support for handling both 8- > and 32-bit LiteX CSR (MMIO) subregisters, as determined by the > LITEX_SUBREG_SIZE Kconfig option. > > NOTE that while LITEX_SUBREG_SIZE could theoretically be a device > tree property, defining it as a compile-time constant allows for > much better optimization of the resulting code. This is further > supported by the low expected usefulness of deploying the same > kernel across LiteX SoCs built with different CSR-Bus data widths. > > The constant LITEX_SUBREG_SIZE is renamed to the more descriptive > LITEX_SUBREG_ALIGN (LiteX CSR subregisters are located at 32-bit > aligned MMIO addresses). > > Finally, the litex_[read|write][8|16|32|64]() accessors are > redefined in terms of litex_[get|set]_reg(), which, after compiler > optimization, will result in code as efficient as hardcoded shifts, > but with the added benefit of automatically matching the appropriate > LITEX_SUBREG_SIZE. > > NOTE that litex_[get|set]_reg() nominally operate 64-bit data, but > that too will be optimized away by the compiler in situations where > narrower data is used. Hello, thank you for taking the time to split and expand on the commit message it makes it a lot easier to review. > Signed-off-by: Gabriel Somlo > --- > drivers/soc/litex/Kconfig | 12 +++ > drivers/soc/litex/litex_soc_ctrl.c | 3 +- > include/linux/litex.h | 139 - > 3 files changed, 70 insertions(+), 84 deletions(-) > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig > index 7c6b009b6f6c..973f8d2fe1a7 100644 > --- a/drivers/soc/litex/Kconfig > +++ b/drivers/soc/litex/Kconfig > @@ -16,4 +16,16 @@ config LITEX_SOC_CONTROLLER > All drivers that use functions from litex.h must depend on > LITEX. > > +config LITEX_SUBREG_SIZE > + int "Size of a LiteX CSR subregister, in bytes" > + depends on LITEX > + range 1 4 > + default 4 > + help > + LiteX MMIO registers (referred to as Configuration and Status > + registers, or CSRs) are spread across adjacent 8- or 32-bit > + subregisters, located at 32-bit aligned MMIO addresses. Use > + this to select the appropriate size (1 or 4 bytes) matching > + your particular LiteX build. > + > endmenu > diff --git a/drivers/soc/litex/litex_soc_ctrl.c > b/drivers/soc/litex/litex_soc_ctrl.c > index 65977526d68e..da17ba56b795 100644 > --- a/drivers/soc/litex/litex_soc_ctrl.c > +++ b/drivers/soc/litex/litex_soc_ctrl.c > @@ -58,7 +58,8 @@ static int litex_check_csr_access(void __iomem *reg_addr) > /* restore original value of the SCRATCH register */ > litex_write32(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_VALUE); > > - pr_info("LiteX SoC Controller driver initialized"); > + pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d", > + LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN); > > return 0; > } > diff --git a/include/linux/litex.h b/include/linux/litex.h > index 918bab45243c..53fb03a2f257 100644 > --- a/include/linux/litex.h > +++ b/include/linux/litex.h > @@ -10,20 +10,19 @@ > #define _LINUX_LITEX_H > > #include > -#include > -#include > > -/* > - * The parameters below are true for LiteX SoCs configured for 8-bit CSR Bus, > - * 32-bit aligned. > - * > - * Supporting other configurations will require extending the logic in this > - * header and in the LiteX SoC controller driver. > - */ > -#define LITEX_REG_SIZE 0x4 > -#define LITEX_SUBREG_SIZE0x1 > +/* LiteX SoCs support 8- or 32-bit CSR Bus data width (i.e., subreg. size) */ > +#if defined(CONFIG_LITEX_SUBREG_SIZE) && \ > + (CONFIG_LITEX_SUBREG_SIZE == 1 || CONFIG_LITEX_SUBREG_SIZE == 4) > +#define LITEX_SUBREG_SIZE CONFIG_LITEX_SUBREG_SIZE > +#else > +#error LiteX subregister size (LITEX_SUBREG_SIZE) must be 4 or 1! > +#endif > #define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8) > > +/* LiteX subregisters of any width are always aligned on a 4-byte boundary */ > +#define LITEX_SUBREG_ALIGN 0x4 > + > static inline void _write_litex_subregister(u32 val, void __iomem *addr) > { > writel((u32 __force)cpu_to_le32(val), addr); > @@ -34,25 +33,31 @@ static inline u32 _read_litex_subregister(void __iomem > *addr) > return le32_to_cpu((__le32 __force)readl(addr)); > } > > -#define WRITE_LITEX_SUBREGISTER(val, base_offset, subreg_id) \ > - _write_litex_subregister(val, (base_offset) + \ > - LITEX_REG_SIZE * (subreg_id)) > - > -#define READ_LITEX_SUBREGISTER(base_offset, subreg_id) \ > - _read_litex_subregister((base_offset) + \ > -
Re: [PATCH v2] drivers/soc/litex: support 32-bit subregisters, 64-bit CPUs
On Tue, Dec 22, 2020 at 05:04:46PM -0500, Gabriel Somlo wrote: > The upstream LiteX project now defaults to using 32-bit subregisters > (see https://github.com/enjoy-digital/litex/commit/a2b71fde). > > This patch expands on commit 22447a99c97e, adding support for handling > both 8 and 32 bit LiteX CSR (MMIO) subregisters, as controlled by the > LITEX_SUBREG_SIZE Kconfig option. The commit should be written in the format: 22447a99c97e ("drivers/soc/litex: add LiteX SoC Controller driver") > Signed-off-by: Gabriel Somlo Gabriel thanks for submitting this. > --- > > Notes v2: > - fix typo (s/u32/u64/) in litex_read64(). > > Notes v1: > - LITEX_SUBREG_SIZE now provided by Kconfig. > - it's not LITEX_REG_SIZE, but rather LITEX_SUBREG_ALIGN! > - move litex_[get|set]_reg() to include/linux/litex.h and mark > them as "static inline"; > - redo litex_[read|write][8|16|32|64]() using litex_[get|set]_reg() > (compiler should produce code as efficient as hardcoded shifts, >but also automatically matching LITEX_SUBREG_SIZE). > > drivers/soc/litex/Kconfig | 12 ++ > drivers/soc/litex/litex_soc_ctrl.c | 76 +--- > include/linux/litex.h | 179 + > 3 files changed, 143 insertions(+), 124 deletions(-) > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig > index 7c6b009b6f6c..973f8d2fe1a7 100644 > --- a/drivers/soc/litex/Kconfig > +++ b/drivers/soc/litex/Kconfig > @@ -16,4 +16,16 @@ config LITEX_SOC_CONTROLLER > All drivers that use functions from litex.h must depend on > LITEX. > > +config LITEX_SUBREG_SIZE > + int "Size of a LiteX CSR subregister, in bytes" > + depends on LITEX > + range 1 4 > + default 4 > + help > + LiteX MMIO registers (referred to as Configuration and Status > + registers, or CSRs) are spread across adjacent 8- or 32-bit > + subregisters, located at 32-bit aligned MMIO addresses. Use > + this to select the appropriate size (1 or 4 bytes) matching > + your particular LiteX build. Could this config be moved to device tree? I see not as you want to compiler to use hard coded shifts. Can you mention this in the commit message not just the v1 notes? > endmenu > diff --git a/drivers/soc/litex/litex_soc_ctrl.c > b/drivers/soc/litex/litex_soc_ctrl.c > index 1217cafdfd4d..da17ba56b795 100644 > --- a/drivers/soc/litex/litex_soc_ctrl.c > +++ b/drivers/soc/litex/litex_soc_ctrl.c > @@ -16,79 +16,6 @@ > #include > #include > > -/* > - * LiteX SoC Generator, depending on the configuration, can split a single > - * logical CSR (Control Register) into a series of consecutive > physical > - * registers. > - * > - * For example, in the configuration with 8-bit CSR Bus, 32-bit aligned (the > - * default one for 32-bit CPUs) a 32-bit logical CSR will be generated as > four > - * 32-bit physical registers, each one containing one byte of meaningful > data. > - * > - * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus > - * > - * The purpose of `litex_set_reg`/`litex_get_reg` is to implement the logic > - * of writing to/reading from the LiteX CSR in a single place that can be > - * then reused by all LiteX drivers. > - */ > - > -/** > - * litex_set_reg() - Writes the value to the LiteX CSR (Control > Register) > - * @reg: Address of the CSR > - * @reg_size: The width of the CSR expressed in the number of bytes > - * @val: Value to be written to the CSR > - * > - * In the currently supported LiteX configuration (8-bit CSR Bus, 32-bit > aligned), > - * a 32-bit LiteX CSR is generated as 4 consecutive 32-bit physical > registers, > - * each one containing one byte of meaningful data. > - * > - * This function splits a single possibly multi-byte write into a series of > - * single-byte writes with a proper offset. > - */ > -void litex_set_reg(void __iomem *reg, unsigned long reg_size, > - unsigned long val) > -{ > - unsigned long shifted_data, shift, i; > - > - for (i = 0; i < reg_size; ++i) { > - shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT); > - shifted_data = val >> shift; > - > - WRITE_LITEX_SUBREGISTER(shifted_data, reg, i); > - } > -} > -EXPORT_SYMBOL_GPL(litex_set_reg); > - > -/** > - * litex_get_reg() - Reads the value of the LiteX CSR (Control > Register) > - * @reg: Address of the CSR > - * @reg_size: The width of the CSR expressed in the number of bytes > - * > - * Return: Value read from the CSR > - * > - * In the currently supported LiteX configuration (8-bit CSR Bus, 32-bit > aligned), > - * a 32-bit LiteX CSR is generated as 4 consecutive 32-bit physical > registers, > - * each one containing one byte of meaningful data. > - * > - * This function generates a series of single-byte reads with a proper offset > - * and joins their results into a single multi-byte value. > - */ > -unsigned
Re: [PATCH v2] openrisc: Add vmlinux.bin target
On Tue, Dec 22, 2020 at 04:22:16PM +0900, Masahiro Yamada wrote: > On Tue, Dec 22, 2020 at 4:07 PM Joel Stanley wrote: > > > > Build it by default. This is commonly used by fpga targets. > > > > Signed-off-by: Joel Stanley > > --- > > v2: Address review from Masahiro > > > > - Add vmlinux.bin to phony target > > - simplfy vmlinux.bin rule > > - add cleanup rule > > - add vmlinux.bin to targets > > - Add gitignore > > > Reviewed-by: Masahiro Yamada > Thanks, I will queue this. I am just wondering if I should wait for 5.12 or just go ahead an send it in 5.11 as a fix. -Stafford
Re: [PATCH 3/5] dt-bindings: soc: add the required property 'additionalProperties'
On Fri, Dec 18, 2020 at 03:17:06PM -0600, Rob Herring wrote: > On Fri, 04 Dec 2020 17:38:11 +0800, Zhen Lei wrote: > > When I do dt_binding_check for any YAML file, below wanring is always > > reported: > > > > xxx/soc/litex/litex,soc-controller.yaml: 'additionalProperties' is a > > required property > > xxx/soc/litex/litex,soc-controller.yaml: ignoring, error in schema: > > warning: no schema found in file: xxx/soc/litex/litex,soc-controller.yaml > > > > Signed-off-by: Zhen Lei > > --- > > Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml | 2 > > ++ > > 1 file changed, 2 insertions(+) > > > > Applied, thanks! Thank you! -Stafford
[GIT PULL] OpenRISC updates for v5.11
Hi Linus, Please consider for pull. The following changes since commit 3cea11cd5e3b00d91caf0b4730194039b45c5891: Linux 5.10-rc2 (2020-11-01 14:43:51 -0800) are available in the Git repository at: g...@github.com:openrisc/linux.git tags/for-linus for you to fetch changes up to d8398bf840f8964220508aff7901c924e322f5e8: openrisc: add local64.h to fix blk-iocost build (2020-11-21 07:13:07 +0900) OpenRISC updates for 5.11 This series adds: * New drivers and OpenRISC support for the LiteX platform * A bug fix to support userspace gdb debugging * Fixes one compile issue with blk-iocost Filip Kokosinski (4): dt-bindings: vendor: add vendor prefix for LiteX dt-bindings: serial: document LiteUART bindings drivers/tty/serial: add LiteUART driver openrisc: add support for LiteX Pawel Czarnecki (2): dt-bindings: soc: document LiteX SoC Controller bindings drivers/soc/litex: add LiteX SoC Controller driver Stafford Horne (2): openrisc: fix trap for debugger breakpoint signalling openrisc: add local64.h to fix blk-iocost build .../devicetree/bindings/serial/litex,liteuart.yaml | 38 ++ .../bindings/soc/litex/litex,soc-controller.yaml | 39 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS| 10 + arch/openrisc/boot/dts/or1klitex.dts | 55 +++ arch/openrisc/configs/or1klitex_defconfig | 18 + arch/openrisc/include/asm/Kbuild | 1 + arch/openrisc/kernel/traps.c | 4 +- drivers/soc/Kconfig| 1 + drivers/soc/Makefile | 1 + drivers/soc/litex/Kconfig | 19 + drivers/soc/litex/Makefile | 3 + drivers/soc/litex/litex_soc_ctrl.c | 176 + drivers/tty/serial/Kconfig | 32 ++ drivers/tty/serial/Makefile| 1 + drivers/tty/serial/liteuart.c | 404 + include/linux/litex.h | 102 ++ 17 files changed, 903 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/serial/litex,liteuart.yaml create mode 100644 Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml create mode 100644 arch/openrisc/boot/dts/or1klitex.dts create mode 100644 arch/openrisc/configs/or1klitex_defconfig create mode 100644 drivers/soc/litex/Kconfig create mode 100644 drivers/soc/litex/Makefile create mode 100644 drivers/soc/litex/litex_soc_ctrl.c create mode 100644 drivers/tty/serial/liteuart.c create mode 100644 include/linux/litex.h
[PATCH] openrisc: add local64.h to fix blk-iocost build
As of 5.10 OpenRISC allyesconfig builds fail with the following error. $ make ARCH=openrisc CROSS_COMPILE=or1k-elf- block/blk-iocost.o CALLscripts/checksyscalls.sh CALLscripts/atomic/check-atomics.sh CC block/blk-iocost.o block/blk-iocost.c:183:10: fatal error: asm/local64.h: No such file or directory 183 | #include | ^~~ compilation terminated. The new include of local64.h was added in commit 5e124f74325d ("blk-iocost: use local[64]_t for percpu stat") by Tejun. Adding the generic version of local64.h to OpenRISC fixes the build issue. Cc: Tejun Heo Signed-off-by: Stafford Horne --- arch/openrisc/include/asm/Kbuild | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild index ca5987e11053..442f3d3bcd90 100644 --- a/arch/openrisc/include/asm/Kbuild +++ b/arch/openrisc/include/asm/Kbuild @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 generic-y += extable.h generic-y += kvm_para.h +generic-y += local64.h generic-y += mcs_spinlock.h generic-y += qspinlock_types.h generic-y += qspinlock.h -- 2.26.2
Re: [PATCH 03/27] blk-iocost: use local[64]_t for percpu stat
On Tue, Sep 01, 2020 at 02:52:33PM -0400, Tejun Heo wrote: > blk-iocost has been reading percpu stat counters from remote cpus which on > some archs can lead to torn reads in really rare occassions. Use local[64]_t > for those counters. > > Signed-off-by: Tejun Heo > --- > block/blk-iocost.c | 37 +++-- > 1 file changed, 27 insertions(+), 10 deletions(-) > > diff --git a/block/blk-iocost.c b/block/blk-iocost.c > index d37b55db2409..e2266e7692b4 100644 > --- a/block/blk-iocost.c > +++ b/block/blk-iocost.c > @@ -179,6 +179,8 @@ > #include > #include > #include > +#include > +#include Hi Tejun, FYI, I am just noticing this but this breaks my allyesconfig build on OpenRISC; as 32-bit arch/openrisc doesn't define local64.h In general local64 is slow on 32-bit architectures, would that be a problem with the usage here? Are the calls to local64_* below on critical paths? Either way I will submit a patch in include generic local64.h on OpenRISC, I confirmed it fixes the build. I do not know of anyone using cgroups on OpenRISC systems. -Stafford > #include "blk-rq-qos.h" > #include "blk-stat.h" > #include "blk-wbt.h" > @@ -373,8 +375,8 @@ struct ioc_params { > }; > > struct ioc_missed { > - u32 nr_met; > - u32 nr_missed; > + local_t nr_met; > + local_t nr_missed; > u32 last_met; > u32 last_missed; > }; > @@ -382,7 +384,7 @@ struct ioc_missed { > struct ioc_pcpu_stat { > struct ioc_missed missed[2]; > > - u64 rq_wait_ns; > + local64_t rq_wait_ns; > u64 last_rq_wait_ns; > }; > > @@ -1278,8 +1280,8 @@ static void ioc_lat_stat(struct ioc *ioc, u32 > *missed_ppm_ar, u32 *rq_wait_pct_p > u64 this_rq_wait_ns; > > for (rw = READ; rw <= WRITE; rw++) { > - u32 this_met = READ_ONCE(stat->missed[rw].nr_met); > - u32 this_missed = READ_ONCE(stat->missed[rw].nr_missed); > + u32 this_met = local_read(>missed[rw].nr_met); > + u32 this_missed = > local_read(>missed[rw].nr_missed); > > nr_met[rw] += this_met - stat->missed[rw].last_met; > nr_missed[rw] += this_missed - > stat->missed[rw].last_missed; > @@ -1287,7 +1289,7 @@ static void ioc_lat_stat(struct ioc *ioc, u32 > *missed_ppm_ar, u32 *rq_wait_pct_p > stat->missed[rw].last_missed = this_missed; > } > > - this_rq_wait_ns = READ_ONCE(stat->rq_wait_ns); > + this_rq_wait_ns = local64_read(>rq_wait_ns); > rq_wait_ns += this_rq_wait_ns - stat->last_rq_wait_ns; > stat->last_rq_wait_ns = this_rq_wait_ns; > } > @@ -1908,6 +1910,7 @@ static void ioc_rqos_done_bio(struct rq_qos *rqos, > struct bio *bio) > static void ioc_rqos_done(struct rq_qos *rqos, struct request *rq) > { > struct ioc *ioc = rqos_to_ioc(rqos); > + struct ioc_pcpu_stat *ccs; > u64 on_q_ns, rq_wait_ns, size_nsec; > int pidx, rw; > > @@ -1931,13 +1934,17 @@ static void ioc_rqos_done(struct rq_qos *rqos, struct > request *rq) > rq_wait_ns = rq->start_time_ns - rq->alloc_time_ns; > size_nsec = div64_u64(calc_size_vtime_cost(rq, ioc), VTIME_PER_NSEC); > > + ccs = get_cpu_ptr(ioc->pcpu_stat); > + > if (on_q_ns <= size_nsec || > on_q_ns - size_nsec <= ioc->params.qos[pidx] * NSEC_PER_USEC) > - this_cpu_inc(ioc->pcpu_stat->missed[rw].nr_met); > + local_inc(>missed[rw].nr_met); > else > - this_cpu_inc(ioc->pcpu_stat->missed[rw].nr_missed); > + local_inc(>missed[rw].nr_missed); > + > + local64_add(rq_wait_ns, >rq_wait_ns); > > - this_cpu_add(ioc->pcpu_stat->rq_wait_ns, rq_wait_ns); > + put_cpu_ptr(ccs); > } > > static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos) > @@ -1977,7 +1984,7 @@ static int blk_iocost_init(struct request_queue *q) > { > struct ioc *ioc; > struct rq_qos *rqos; > - int ret; > + int i, cpu, ret; > > ioc = kzalloc(sizeof(*ioc), GFP_KERNEL); > if (!ioc) > @@ -1989,6 +1996,16 @@ static int blk_iocost_init(struct request_queue *q) > return -ENOMEM; > } > > + for_each_possible_cpu(cpu) { > + struct ioc_pcpu_stat *ccs = per_cpu_ptr(ioc->pcpu_stat, cpu); > + > + for (i = 0; i < ARRAY_SIZE(ccs->missed); i++) { > + local_set(>missed[i].nr_met, 0); > + local_set(>missed[i].nr_missed, 0); > + } > + local64_set(>rq_wait_ns, 0); > + } > + > rqos = >rqos; > rqos->id = RQ_QOS_COST; > rqos->ops =
[PATCH] openrisc: add support for LiteX
From: Filip Kokosinski This adds support for a basic LiteX-based SoC with a mor1kx soft CPU. Signed-off-by: Filip Kokosinski Signed-off-by: Mateusz Holenko [shorne: Merged in soc-cntl patch, removed CROSS_COMPILE, sort MAINT.] Signed-off-by: Stafford Horne --- MAINTAINERS | 1 + arch/openrisc/boot/dts/or1klitex.dts | 55 +++ arch/openrisc/configs/or1klitex_defconfig | 18 3 files changed, 74 insertions(+) create mode 100644 arch/openrisc/boot/dts/or1klitex.dts create mode 100644 arch/openrisc/configs/or1klitex_defconfig diff --git a/MAINTAINERS b/MAINTAINERS index 0b1f39b3938d..9c55a54c9673 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10177,6 +10177,7 @@ M: Karol Gugala M: Mateusz Holenko S: Maintained F: Documentation/devicetree/bindings/*/litex,*.yaml +F: arch/openrisc/boot/dts/or1klitex.dts F: drivers/soc/litex/litex_soc_ctrl.c F: drivers/tty/serial/liteuart.c F: include/linux/litex.h diff --git a/arch/openrisc/boot/dts/or1klitex.dts b/arch/openrisc/boot/dts/or1klitex.dts new file mode 100644 index ..3f9867aa3844 --- /dev/null +++ b/arch/openrisc/boot/dts/or1klitex.dts @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LiteX-based System on Chip + * + * Copyright (C) 2019 Antmicro + */ + +/dts-v1/; +/ { + compatible = "opencores,or1ksim"; + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <>; + + aliases { + serial0 = + }; + + chosen { + bootargs = "console=liteuart"; + }; + + memory@0 { + device_type = "memory"; + reg = <0x 0x1000>; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + cpu@0 { + compatible = "opencores,or1200-rtlsvn481"; + reg = <0>; + clock-frequency = <1>; + }; + }; + + pic: pic { + compatible = "opencores,or1k-pic"; + #interrupt-cells = <1>; + interrupt-controller; + }; + + serial0: serial@e0002000 { + device_type = "serial"; + compatible = "litex,liteuart"; + reg = <0xe0002000 0x100>; + }; + + soc_ctrl0: soc_controller@e000 { + compatible = "litex,soc-controller"; + reg = <0xe000 0xc>; + status = "okay"; + }; +}; diff --git a/arch/openrisc/configs/or1klitex_defconfig b/arch/openrisc/configs/or1klitex_defconfig new file mode 100644 index ..3c2c70d3d740 --- /dev/null +++ b/arch/openrisc/configs/or1klitex_defconfig @@ -0,0 +1,18 @@ +CONFIG_BLK_DEV_INITRD=y +CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC=y +CONFIG_BUG_ON_DATA_CORRUPTION=y +CONFIG_CC_OPTIMIZE_FOR_SIZE=y +CONFIG_DEVTMPFS=y +CONFIG_DEVTMPFS_MOUNT=y +CONFIG_EMBEDDED=y +CONFIG_HZ_100=y +CONFIG_INITRAMFS_SOURCE="openrisc-rootfs.cpio.gz" +CONFIG_OF_OVERLAY=y +CONFIG_OPENRISC_BUILTIN_DTB="or1klitex" +CONFIG_PANIC_ON_OOPS=y +CONFIG_PRINTK_TIME=y +CONFIG_LITEX_SOC_CONTROLLER=y +CONFIG_SERIAL_LITEUART=y +CONFIG_SERIAL_LITEUART_CONSOLE=y +CONFIG_SOFTLOCKUP_DETECTOR=y +CONFIG_TTY_PRINTK=y -- 2.26.2
[PATCH] openrisc: fix trap for debugger breakpoint signalling
I have been working on getting native Linux GDB support working for the OpenRISC port. The trap signal handler here was wrong in a few ways. During trap handling address (from the EEAR register) is not set by the CPU, so it is not correct to use here. We want to use trap as a break-point so use TRAP_BRKPT. Adding 4 to the pc was incorrect and causing GDB to think the breakpoint was not hit. Fixing these allows GDB to work now. Signed-off-by: Stafford Horne --- arch/openrisc/kernel/traps.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 206e5325e61b..4d61333c2623 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -238,9 +238,7 @@ void __init trap_init(void) asmlinkage void do_trap(struct pt_regs *regs, unsigned long address) { - force_sig_fault(SIGTRAP, TRAP_TRACE, (void __user *)address); - - regs->pc += 4; + force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc); } asmlinkage void do_unaligned_access(struct pt_regs *regs, unsigned long address) -- 2.26.2
Re: [PATCH v12 0/5] LiteX SoC controller and LiteUART serial driver
On Tue, Oct 13, 2020 at 04:45:09PM +0200, Mateusz Holenko wrote: > This patchset introduces support for LiteX SoC Controller > and LiteUART - serial device from LiteX SoC builder > (https://github.com/enjoy-digital/litex). > > In the following patchset I will add > a new mor1kx-based (OpenRISC) platform that > uses this device. > > Later I plan to extend this platform by > adding support for more devices from LiteX suite. > > Changes in v12: > - fixed descriptions in yaml files > - simplified probe implementations > - introduced litex_{read,write}{8,16,32,64}() fast accessors > - added formal documentation of litex_get_reg()/litex_set_reg() > - fixed possible memory leaks > - removed spin locks from CSR accessors > > Changes in v11: > - added Reviewed-by tag > - reformatted some comments > - switched to WARN instead of BUG on CSR validation fail > > Changes in v10: > - added casting to avoid sparse warnings in the SoC Controller's driver > > Changes in v9: > - fixed the `reg` node notation in the DT example > - added exporting of the `litex_set_reg`/`litex_get_reg` symbols > > Changes in v8: > - fixed help messages in LiteUART's KConfig > - removed dependency between LiteUART and LiteX SoC drivers > - removed `litex_check_accessors()` helper function > - added crashing (BUG) on the failed LiteX CSR access test > > Changes in v7: > - added missing include directive in UART's driver > > Changes in v6: > - changed accessors in SoC Controller's driver > - reworked UART driver > > Changes in v5: > - added Reviewed-by tag > - removed custom accessors from SoC Controller's driver > - fixed error checking in SoC Controller's driver > > Changes in v4: > - fixed copyright headers > - fixed SoC Controller's yaml > - simplified SoC Controller's driver > > Changes in v3: > - added Acked-by and Reviewed-by tags > - introduced LiteX SoC Controller driver > - removed endianness detection (handled now by LiteX SoC Controller > driver) > - modified litex.h header > - DTS aliases for LiteUART made optional > - renamed SERIAL_LITEUART_NR_PORTS to SERIAL_LITEUART_MAX_PORTS > - changed PORT_LITEUART from 122 to 123 > > Changes in v2: > - binding description rewritten to a yaml schema file > - added litex.h header with common register access functions > > Filip Kokosinski (3): > dt-bindings: vendor: add vendor prefix for LiteX > dt-bindings: serial: document LiteUART bindings > drivers/tty/serial: add LiteUART driver > > Pawel Czarnecki (2): > dt-bindings: soc: document LiteX SoC Controller bindings > drivers/soc/litex: add LiteX SoC Controller driver > > .../bindings/serial/litex,liteuart.yaml | 38 ++ > .../soc/litex/litex,soc-controller.yaml | 39 ++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > MAINTAINERS | 9 + > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile | 1 + > drivers/soc/litex/Kconfig | 19 + > drivers/soc/litex/Makefile| 3 + > drivers/soc/litex/litex_soc_ctrl.c| 176 > drivers/tty/serial/Kconfig| 32 ++ > drivers/tty/serial/Makefile | 1 + > drivers/tty/serial/liteuart.c | 404 ++ > include/linux/litex.h | 103 + > 13 files changed, 831 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/serial/litex,liteuart.yaml > create mode 100644 > Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml > create mode 100644 drivers/soc/litex/Kconfig > create mode 100644 drivers/soc/litex/Makefile > create mode 100644 drivers/soc/litex/litex_soc_ctrl.c > create mode 100644 drivers/tty/serial/liteuart.c > create mode 100644 include/linux/litex.h (CCing Linus) Hello, As it looks like we have all required signoffs and no issues with the latest v12 series I have queued this in the OpenRISC 5.11 queue (where we are one of the first users in our platform). I am CCing Linus in case he has any concerns with these merging through the OpenRISC queue. In the LiteX ecosystem RISC-V and lm32 (lattice micro 32 soft core) FPGA SoC's are also used which take advantage of these drivers. There will be more to come in the future, probably the next being LiteETH (ethernet). -Stafford
Re: [linux-stable-rc:linux-5.4.y 665/2391] drivers/android/binder.c:3776: Error: unrecognized keyword/register name `l.lwz
On Fri, Oct 16, 2020 at 11:06:38AM +0200, Greg Kroah-Hartman wrote: > On Fri, Oct 16, 2020 at 08:05:17AM +0900, Stafford Horne wrote: > > On Fri, Oct 16, 2020, 6:46 AM Jann Horn wrote: > > > > > +openrisc folks > > > > > > On Thu, Oct 15, 2020 at 11:28 PM kernel test robot wrote: > > > > tree: > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git > > > linux-5.4.y > > > > head: 85b0841aab15c12948af951d477183ab3df7de14 > > > > commit: c5665cafbedd2e2a523fe933e452391a02d3adb3 [665/2391] binder: > > > Prevent context manager from incrementing ref 0 > > > > config: openrisc-randconfig-r002-20201014 (attached as .config) > > > > compiler: or1k-linux-gcc (GCC) 9.3.0 > > > > reproduce (this is a W=1 build): > > > > wget > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross > > > -O ~/bin/make.cross > > > > chmod +x ~/bin/make.cross > > > > # > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git/commit/?id=c5665cafbedd2e2a523fe933e452391a02d3adb3 > > > > git remote add linux-stable-rc > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git > > > > git fetch --no-tags linux-stable-rc linux-5.4.y > > > > git checkout c5665cafbedd2e2a523fe933e452391a02d3adb3 > > > > # save the attached .config to linux build tree > > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross > > > ARCH=openrisc > > > > > > > > If you fix the issue, kindly add following tag as appropriate > > > > Reported-by: kernel test robot > > > > > > > > All errors (new ones prefixed by >>): > > > > > > > >drivers/android/binder.c: Assembler messages: > > > > >> drivers/android/binder.c:3776: Error: unrecognized keyword/register > > > name `l.lwz ?ap,4(r25)' > > > >drivers/android/binder.c:3781: Error: unrecognized keyword/register > > > name `l.addi ?ap,r0,0' > > > > > > binder is basically doing this: > > > > > > u64 data_ptr; > > > if (get_user(data_ptr, (u64 __user *)ptr)) > > > return -EFAULT; > > > > > > and GCC complains that that doesn't turn into valid assembly on > > > openrisc, where get_user() of size 8 expands into this: > > > > > > #define __get_user_asm2(x, addr, err) \ > > > { \ > > > unsigned long long __gu_tmp;\ > > > __asm__ __volatile__( \ > > > "1: l.lwz %1,0(%2)\n" \ > > > "2: l.lwz %H1,4(%2)\n" \ > > > "3:\n" \ > > > ".section .fixup,\"ax\"\n" \ > > > "4: l.addi %0,r0,%3\n" \ > > > " l.addi %1,r0,0\n" \ > > > " l.addi %H1,r0,0\n" \ > > > " l.j 3b\n" \ > > > " l.nop\n"\ > > > ".previous\n" \ > > > ".section __ex_table,\"a\"\n" \ > > > " .align 2\n" \ > > > " .long 1b,4b\n" \ > > > " .long 2b,4b\n" \ > > > ".previous" \ > > > : "=r"(err), "="(__gu_tmp)\ > > > : "r"(addr), "i"(-EFAULT), "0"(err)); \ > > > (x) = (__typeof__(*(addr)))(\ > > > (__typeof__((x)-(x)))__gu_tmp); \ > > > } > > > > > > and apparently the "l.lwz %H1,4(%2)" and "l.addi %H1,r0,0" don't turn > > > into valid assembly when %H1 expands to "?ap"? > > > > > > I don't know anything about OpenRISC, but this seems like it's > > > probably an issue in the get_user() implementation. > > > > > > > This is fixed in 5.9. I think the patch can be cherry picked by itself. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/openrisc?h=v5.9=d877322bc1adcab9850732275670409e8bcca4c4 > > Does not apply cleanly to 5.8.y or 5.4.y, can someone please properly > backport it and send it to sta...@vger.kernel.org? I will do it. -Stafford
Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver
On Mon, Sep 14, 2020 at 12:33:11PM +0200, Mateusz Holenko wrote: > On Fri, Sep 11, 2020 at 2:57 AM Stafford Horne wrote: > > > > On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote: > > > From: Pawel Czarnecki > > > > > > This commit adds driver for the FPGA-based LiteX SoC > > > Controller from LiteX SoC builder. > > > > > > Co-developed-by: Mateusz Holenko > > > Signed-off-by: Mateusz Holenko > > > Signed-off-by: Pawel Czarnecki > > > --- > > > + node = dev->of_node; > > > + if (!node) > > > + return -ENODEV; We return here without BUG() if the setup fails. > > > + > > > + soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL); > > > + if (!soc_ctrl_dev) > > > + return -ENOMEM; We return here without BUG() if we are out of memory. > > > + > > > + soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(soc_ctrl_dev->base)) > > > + return PTR_ERR(soc_ctrl_dev->base); Etc. > > > + > > > + result = litex_check_csr_access(soc_ctrl_dev->base); > > > + if (result) { > > > + // LiteX CSRs access is broken which means that > > > + // none of LiteX drivers will most probably > > > + // operate correctly > > The comment format here with // is not usually used in the kernel, but its > > not > > forbidded. Could you use the /* */ multiline style? > > Sure, I'll change the commenting style here. > > > > > > + BUG(); > > Instead of stopping the system with BUG, could we just do: > > > > return litex_check_csr_access(soc_ctrl_dev->base); > > > > We already have failure for NODEV/NOMEM so might as well not call BUG() here > > too. > > It's true that litex_check_csr_accessors() already generates error > codes that could be > returned directly. > The point of using BUG() macro here, however, is to stop booting the > system so that it's visible > (and impossible to miss for the user) that an unresolvable HW issue > was encountered. > > CSR-accessors - the litex_{g,s}et_reg() functions - are intended to be > used by other LiteX drivers > and it's very unlikely that those drivers would work properly after > the fail of litex_check_csr_accessors(). > Since in such case the UART driver will be affected too (no boot logs > and error messages visible to the user), > I thought it'll be easier to spot and debug the problem if the system > stopped in the BUG loop. > Perhaps there are other, more linux-friendly, ways of achieving a > similar goal - I'm open for suggestions. I see your point, but I thought if failed with an exit status above, we could do the same here. But I guess failing here means that something is really wrong as validation failed. Some points: - If we return here, the system will still boot but there will be no UART - If we bail with BUG(), here the system stops, and there is no UART - Both cases the user can connect with a debugger and read "dmesg", to see what is wrong, but BUG() does not print an error message on all architectures. We could also use: - WARN(1, "Failed to validate CSR registers, the system is probably broken."); If you want to keep BUG() it may be fine. I am not an expert on handling these type of bailout's so other input is appreciated. -Stafford
Re: drivers/android/binder.c:3689: Error: unrecognized keyword/register name `l.lwz
On Fri, Sep 11, 2020 at 02:37:14PM +0800, kernel test robot wrote: > Hi Stafford, > > FYI, the error/warning still remains. A change was merged to master today that should fix this. Please let me know if it's still an issue. -Stafford
Re: drivers/android/binder.c:3689: Error: unrecognized keyword/register name `l.lwz
On Fri, Sep 11, 2020 at 02:37:14PM +0800, kernel test robot wrote: > Hi Stafford, > > FYI, the error/warning still remains. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: 581cb3a26baf846ee9636214afaa5333919875b1 > commit: af84b16e3423bd9c1c8d81c44bc0a217f763f6b7 openrisc: uaccess: Use > static inline function in access_ok Hello, I sent a patch series to fix this a few weeks ago, it was reviewed and I hope the Pull Request will be merged to mainline tomorrow. Please check after that. -Stafford > date: 5 weeks ago > config: openrisc-allmodconfig (attached as .config) > compiler: or1k-linux-gcc (GCC) 9.3.0 > reproduce (this is a W=1 build): > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > git checkout af84b16e3423bd9c1c8d81c44bc0a217f763f6b7 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross > ARCH=openrisc > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All errors (new ones prefixed by >>): > >drivers/android/binder.c: Assembler messages: > >> drivers/android/binder.c:3689: Error: unrecognized keyword/register name > >> `l.lwz ?ap,4(r17)' >drivers/android/binder.c:3694: Error: unrecognized keyword/register name > `l.addi ?ap,r0,0' > > # > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=af84b16e3423bd9c1c8d81c44bc0a217f763f6b7 > git remote add linus > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git fetch --no-tags linus master > git checkout af84b16e3423bd9c1c8d81c44bc0a217f763f6b7 > vim +3689 drivers/android/binder.c > > 44d8047f1d87ad drivers/android/binder.c Todd Kjos > 2018-08-28 3594 > fb07ebc3e82a98 drivers/staging/android/binder.c Bojan Prtvar > 2013-09-02 3595 static int binder_thread_write(struct binder_proc *proc, > fb07ebc3e82a98 drivers/staging/android/binder.c Bojan Prtvar > 2013-09-02 3596 struct binder_thread *thread, > da49889deb34d3 drivers/staging/android/binder.c Arve Hjønnevåg > 2014-02-21 3597 binder_uintptr_t binder_buffer, size_t > size, > da49889deb34d3 drivers/staging/android/binder.c Arve Hjønnevåg > 2014-02-21 3598 binder_size_t *consumed) > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3599 { > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3600 uint32_t cmd; > 342e5c90b60134 drivers/android/binder.c Martijn Coenen > 2017-02-03 3601 struct binder_context *context = proc->context; > da49889deb34d3 drivers/staging/android/binder.c Arve Hjønnevåg > 2014-02-21 3602 void __user *buffer = (void __user > *)(uintptr_t)binder_buffer; > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3603 void __user *ptr = buffer + *consumed; > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3604 void __user *end = buffer + size; > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3605 > 26549d17741035 drivers/android/binder.c Todd Kjos > 2017-06-29 3606 while (ptr < end && thread->return_error.cmd == BR_OK) > { > 372e3147df7016 drivers/android/binder.c Todd Kjos > 2017-06-29 3607 int ret; > 372e3147df7016 drivers/android/binder.c Todd Kjos > 2017-06-29 3608 > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3609 if (get_user(cmd, (uint32_t __user *)ptr)) > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3610 return -EFAULT; > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3611 ptr += sizeof(uint32_t); > 975a1ac9a9fe65 drivers/staging/android/binder.c Arve Hjønnevåg > 2012-10-16 3612 trace_binder_command(cmd); > 355b0502f6efea drivers/staging/android/binder.c Greg Kroah-Hartman > 2011-11-30 3613 if (_IOC_NR(cmd) < > ARRAY_SIZE(binder_stats.bc)) { > 0953c7976c36ce drivers/android/binder.c Badhri Jagan Sridharan > 2017-06-29 3614 > atomic_inc(_stats.bc[_IOC_NR(cmd)]); > 0953c7976c36ce drivers/android/binder.c Badhri Jagan Sridharan > 2017-06-29 3615 > atomic_inc(>stats.bc[_IOC_NR(cmd)]); > 0953c7976c36ce drivers/android/binder.c Badhri Jagan Sridharan > 2017-06-29 3616 > atomic_inc(>stats.bc[_IOC_NR(cmd)]); > 355b0502f6efea drivers/staging/android/binder.c
[GIT PULL] OpenRISC fixes for v5.9-rc4
Hi Linus, Please consider for pull. The following changes since commit d012a7190fc1fd72ed48911e77ca97ba4521bccd: Linux 5.9-rc2 (2020-08-23 14:08:43 -0700) are available in the Git repository at: git://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to d877322bc1adcab9850732275670409e8bcca4c4: openrisc: Fix issue with get_user for 64-bit values (2020-09-12 17:26:00 +0900) OpenRISC fixes for 5.9-rc4 Fixes for compile issues pointed out by kbuild and one bug I found in interd with the 5.9 patches. Stafford Horne (3): openrisc: Reserve memblock for initrd openrisc: Fix cache API compile issue when not inlining openrisc: Fix issue with get_user for 64-bit values arch/openrisc/include/asm/uaccess.h | 33 + arch/openrisc/kernel/setup.c| 10 ++ arch/openrisc/mm/cache.c| 2 +- 3 files changed, 32 insertions(+), 13 deletions(-)
Re: [PATCH v3 3/3] openrisc: Fix issue with get_user for 64-bit values
On Fri, Sep 11, 2020 at 10:55:26PM +0200, Luc Van Oostenryck wrote: > On Fri, Sep 11, 2020 at 08:39:40AM +0900, Stafford Horne wrote: > > A build failure was raised by kbuild with the following error. > > > > drivers/android/binder.c: Assembler messages: > > drivers/android/binder.c:3861: Error: unrecognized keyword/register > > name `l.lwz ?ap,4(r24)' > > drivers/android/binder.c:3866: Error: unrecognized keyword/register > > name `l.addi ?ap,r0,0' > > > > The issue is with 64-bit get_user() calls on openrisc. I traced this to > > a problem where in the internally in the get_user macros there is a cast > > to long __gu_val this causes GCC to think the get_user call is 32-bit. > > This binder code is really long and GCC allocates register r30, which > > triggers the issue. The 64-bit get_user asm tries to get the 64-bit pair > > register, which for r30 overflows the general register names and returns > > the dummy register ?ap. > > > > The fix here is to move the temporary variables into the asm macros. We > > use a 32-bit __gu_tmp for 32-bit and smaller macro and a 64-bit tmp in > > the 64-bit macro. The cast in the 64-bit macro has a trick of casting > > through __typeof__((x)-(x)) which avoids the below warning. This was > > barrowed from riscv. > > > > arch/openrisc/include/asm/uaccess.h:240:8: warning: cast to pointer > > from integer of different size > > > > I tested this is a small unit test to check reading between 64-bit and > > 32-bit pointers to 64-bit and 32-bit values in all combinations. Also I > > ran make C=1 to confirm no new sparse warnings came up. It all looks > > clean to me. > > It looks correct to me too now at C & assembly level. > Feel free to add my: > Reviewed-by: Luc Van Oostenryck Thanks a lot. -Stafford
Re: [PATCH v10 0/5] LiteX SoC controller and LiteUART serial driver
On Wed, Aug 12, 2020 at 02:33:46PM +0200, Mateusz Holenko wrote: > This patchset introduces support for LiteX SoC Controller > and LiteUART - serial device from LiteX SoC builder > (https://github.com/enjoy-digital/litex). > > In the following patchset I will add > a new mor1kx-based (OpenRISC) platform that > uses this device. > > Later I plan to extend this platform by > adding support for more devices from LiteX suite. Hello, as discussed offline I am planning to merge these via the OpenRISC tree during 5.10. If anyone has an issues let me know. The patches all have their reviews and look fine to me other than a few nit's on the soc controller patch. -Stafford
Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver
On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote: > From: Pawel Czarnecki > > This commit adds driver for the FPGA-based LiteX SoC > Controller from LiteX SoC builder. > > Co-developed-by: Mateusz Holenko > Signed-off-by: Mateusz Holenko > Signed-off-by: Pawel Czarnecki > --- ... > +static int litex_check_csr_access(void __iomem *reg_addr) > +{ > + unsigned long reg; > + > + reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE); > + > + if (reg != SCRATCH_REG_VALUE) { > + panic("Scratch register read error! Expected: 0x%x but got: > 0x%lx", > + SCRATCH_REG_VALUE, reg); > + return -EINVAL; > + } > + > + litex_set_reg(reg_addr + SCRATCH_REG_OFF, > + SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE); > + reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE); > + > + if (reg != SCRATCH_TEST_VALUE) { > + panic("Scratch register write error! Expected: 0x%x but got: > 0x%lx", > + SCRATCH_TEST_VALUE, reg); > + return -EINVAL; > + } > + > + /* restore original value of the SCRATCH register */ > + litex_set_reg(reg_addr + SCRATCH_REG_OFF, > + SCRATCH_REG_SIZE, SCRATCH_REG_VALUE); > + > + /* Set flag for other drivers */ What does this comment mean? > + pr_info("LiteX SoC Controller driver initialized"); > + > + return 0; > +} > + > +struct litex_soc_ctrl_device { > + void __iomem *base; > +}; > + > +static const struct of_device_id litex_soc_ctrl_of_match[] = { > + {.compatible = "litex,soc-controller"}, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match); > + > +static int litex_soc_ctrl_probe(struct platform_device *pdev) > +{ > + int result; > + struct device *dev; > + struct device_node *node; > + struct litex_soc_ctrl_device *soc_ctrl_dev; > + > + dev = >dev; > + node = dev->of_node; > + if (!node) > + return -ENODEV; > + > + soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL); > + if (!soc_ctrl_dev) > + return -ENOMEM; > + > + soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(soc_ctrl_dev->base)) > + return PTR_ERR(soc_ctrl_dev->base); > + > + result = litex_check_csr_access(soc_ctrl_dev->base); > + if (result) { > + // LiteX CSRs access is broken which means that > + // none of LiteX drivers will most probably > + // operate correctly The comment format here with // is not usually used in the kernel, but its not forbidded. Could you use the /* */ multiline style? > + BUG(); Instead of stopping the system with BUG, could we just do: return litex_check_csr_access(soc_ctrl_dev->base); We already have failure for NODEV/NOMEM so might as well not call BUG() here too. > + } > + > + return 0; > +} > + Other than that it looks ok to me. -Stafford
[PATCH v3 3/3] openrisc: Fix issue with get_user for 64-bit values
A build failure was raised by kbuild with the following error. drivers/android/binder.c: Assembler messages: drivers/android/binder.c:3861: Error: unrecognized keyword/register name `l.lwz ?ap,4(r24)' drivers/android/binder.c:3866: Error: unrecognized keyword/register name `l.addi ?ap,r0,0' The issue is with 64-bit get_user() calls on openrisc. I traced this to a problem where in the internally in the get_user macros there is a cast to long __gu_val this causes GCC to think the get_user call is 32-bit. This binder code is really long and GCC allocates register r30, which triggers the issue. The 64-bit get_user asm tries to get the 64-bit pair register, which for r30 overflows the general register names and returns the dummy register ?ap. The fix here is to move the temporary variables into the asm macros. We use a 32-bit __gu_tmp for 32-bit and smaller macro and a 64-bit tmp in the 64-bit macro. The cast in the 64-bit macro has a trick of casting through __typeof__((x)-(x)) which avoids the below warning. This was barrowed from riscv. arch/openrisc/include/asm/uaccess.h:240:8: warning: cast to pointer from integer of different size I tested this is a small unit test to check reading between 64-bit and 32-bit pointers to 64-bit and 32-bit values in all combinations. Also I ran make C=1 to confirm no new sparse warnings came up. It all looks clean to me. Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjq%25...@intel.com/ Signed-off-by: Stafford Horne Cc: Luc Van Oostenryck --- Changes since v2: - Add back temporary variables but move to a different place, as described in commit message. Changes since v1: - New arch/openrisc/include/asm/uaccess.h | 33 ++--- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index f0390211236b..120f5005461b 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -165,19 +165,19 @@ struct __large_struct { #define __get_user_nocheck(x, ptr, size) \ ({ \ - long __gu_err, __gu_val;\ - __get_user_size(__gu_val, (ptr), (size), __gu_err); \ - (x) = (__force __typeof__(*(ptr)))__gu_val; \ + long __gu_err; \ + __get_user_size((x), (ptr), (size), __gu_err); \ __gu_err; \ }) #define __get_user_check(x, ptr, size) \ ({ \ - long __gu_err = -EFAULT, __gu_val = 0; \ + long __gu_err = -EFAULT;\ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ - if (access_ok(__gu_addr, size)) \ - __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ - (x) = (__force __typeof__(*(ptr)))__gu_val; \ + if (access_ok(__gu_addr, size)) \ + __get_user_size((x), __gu_addr, (size), __gu_err); \ + else\ + (x) = (__typeof__(*(ptr))) 0; \ __gu_err; \ }) @@ -191,11 +191,13 @@ do { \ case 2: __get_user_asm(x, ptr, retval, "l.lhz"); break; \ case 4: __get_user_asm(x, ptr, retval, "l.lwz"); break; \ case 8: __get_user_asm2(x, ptr, retval); break; \ - default: (x) = __get_user_bad();\ + default: (x) = (__typeof__(*(ptr)))__get_user_bad();\ } \ } while (0) #define __get_user_asm(x, addr, err, op) \ +{ \ + unsigned long __gu_tmp; \ __asm__ __volatile__( \ "1: "op" %1,0(%2)\n"\ "2:\n" \ @@ -209,10 +211,14 @@ do { \ " .align 2\n" \ " .long 1b,3b\n" \ ".previous" \ - : "=r"(err), "=r"(x)\ - : "r"(addr), "i"(-EFAULT), "0"(err)) + :
[PATCH v3 2/3] openrisc: Fix cache API compile issue when not inlining
I found this when compiling a kbuild random config with GCC 11. The config enables CONFIG_DEBUG_SECTION_MISMATCH, which sets CFLAGS -fno-inline-functions-called-once. This causes the call to cache_loop in cache.c to not be inlined causing the below compile error. In file included from arch/openrisc/mm/cache.c:13: arch/openrisc/mm/cache.c: In function 'cache_loop': ./arch/openrisc/include/asm/spr.h:16:27: warning: 'asm' operand 0 probably does not match constraints 16 | #define mtspr(_spr, _val) __asm__ __volatile__ ( \ | ^~~ arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr' 25 | mtspr(reg, line); | ^ ./arch/openrisc/include/asm/spr.h:16:27: error: impossible constraint in 'asm' 16 | #define mtspr(_spr, _val) __asm__ __volatile__ ( \ | ^~~ arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr' 25 | mtspr(reg, line); | ^ make[1]: *** [scripts/Makefile.build:283: arch/openrisc/mm/cache.o] Error 1 The asm constraint "K" requires a immediate constant argument to mtspr, however because of no inlining a register argument is passed causing a failure. Fix this by using __always_inline. Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjq%25...@intel.com/ Signed-off-by: Stafford Horne --- Changes since v2: - None Changes since v1: - New arch/openrisc/mm/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c index 08f56af387ac..534a52ec5e66 100644 --- a/arch/openrisc/mm/cache.c +++ b/arch/openrisc/mm/cache.c @@ -16,7 +16,7 @@ #include #include -static void cache_loop(struct page *page, const unsigned int reg) +static __always_inline 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); -- 2.26.2
[PATCH v3 0/3] OpenRISC fixes for 5.9-rc4
Changes since v2: - Update the uaccess.h to address issues pointed out by Luc Changes since v1: - Now a series, v1 was only the "Reserve memblock for initrd" patch - Added fixes for compiler issues pointed out by the kbuild robot - Updated "Reserve memblock for initrd", as per Mike's comments This is a few fixes found during testing 5.9. Stafford Horne (3): openrisc: Reserve memblock for initrd openrisc: Fix cache API compile issue when not inlining openrisc: Fix issue with get_user for 64-bit values arch/openrisc/include/asm/uaccess.h | 33 ++--- arch/openrisc/kernel/setup.c| 10 + arch/openrisc/mm/cache.c| 2 +- 3 files changed, 32 insertions(+), 13 deletions(-) -- 2.26.2
[PATCH v3 1/3] openrisc: Reserve memblock for initrd
Recently OpenRISC added support for external initrd images, but I found some instability when using larger buildroot initrd images. It turned out that I forgot to reserve the memblock space for the initrd image. This patch fixes the instability issue by reserving memblock space. Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") Signed-off-by: Stafford Horne Reviewed-by: Mike Rapoport --- Changes since v2: - None Changes since v1: - Updated to use separate variables as suggested by Mike. arch/openrisc/kernel/setup.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c index b18e775f8be3..13c87f1f872b 100644 --- a/arch/openrisc/kernel/setup.c +++ b/arch/openrisc/kernel/setup.c @@ -80,6 +80,16 @@ static void __init setup_memory(void) */ memblock_reserve(__pa(_stext), _end - _stext); +#ifdef CONFIG_BLK_DEV_INITRD + /* Then reserve the initrd, if any */ + if (initrd_start && (initrd_end > initrd_start)) { + unsigned long aligned_start = ALIGN_DOWN(initrd_start, PAGE_SIZE); + unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE); + + memblock_reserve(__pa(aligned_start), aligned_end - aligned_start); + } +#endif /* CONFIG_BLK_DEV_INITRD */ + early_init_fdt_reserve_self(); early_init_fdt_scan_reserved_mem(); -- 2.26.2
Re: [OpenRISC] [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values
On Sun, Sep 06, 2020 at 02:22:28AM +0200, Luc Van Oostenryck wrote: > On Sun, Sep 06, 2020 at 06:34:08AM +0900, Stafford Horne wrote: > > On Sat, Sep 05, 2020 at 03:57:14PM +0200, Luc Van Oostenryck wrote: > > > On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote: > > > > > > Hi, > > > > > > The change for 64-bit get_user() looks good to me. > > > But I wonder, given that openrisc is big-endian, what will happen > > > you have the opposite situation: > > > u32 *ptr; > > > u64 val; > > > ... > > > get_user(val, ptr); > > > > > > Won't you end with the value in the most significant part of > > > the register pair? > > > > Hi Luc, > > > > The get_user function uses the size of the ptr to determine how to do the > > load , > > so this case would not use the 64-bit pair register logic. I think it > > should be > > ok, the end result would be the same as c code: > > > > var = *ptr; > > Hi, > > Sorry to insist but both won't give the same result. > The problem comes from the output part of the asm: "=r" (x). > > The following code: > u32 getp(u32 *ptr) > { > u64 val; > val = *ptr; > return val; > } > will compile to something like: > getp: > l.jrr9 > l.lwz r11, 0(r3) > > The load is written to r11, which is what is returned. OK. > > But the get_user() code with a u32 pointer *and* a u64 destination > is equivalent to something like: > u32 getl(u32 *ptr) > { > u64 val; > > asm("l.lwz %0,0(%1)" : "=r"(val) : "r"(ptr)); > return val; > } > and this compiles to: > getl: > l.lwz r17,0(r3) > l.jrr9 > l.orr11, r19, r19 > > The load is written to r17 but what is returned is the content of r19. > Not good. > > I think that, in the get_user() code: > * if the pointer is a pointer to a 64-bit quantity, then variable > used in as the output in the asm needs to be a 64-bit variable > * if the pointer is a pointer to a 32-bit quantity, then variable > used in as the output in the asm needs to be a 64-bit variable > At least one way to guarantee this is to use a temporary variable > that matches the size of the pointer. Hello, Thanks for taking the time to explain. I see your point, it makes sense I will fix this up. -Stafford
Re: [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values
On Sat, Sep 05, 2020 at 03:57:14PM +0200, Luc Van Oostenryck wrote: > On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote: > > Hi, > > The change for 64-bit get_user() looks good to me. > But I wonder, given that openrisc is big-endian, what will happen > you have the opposite situation: > u32 *ptr; > u64 val; > ... > get_user(val, ptr); > > Won't you end with the value in the most significant part of > the register pair? Hi Luc, The get_user function uses the size of the ptr to determine how to do the load , so this case would not use the 64-bit pair register logic. I think it should be ok, the end result would be the same as c code: var = *ptr; -Stafford
Re: [PATCH v2 1/3] openrisc: Reserve memblock for initrd
On Sat, Sep 05, 2020 at 10:19:33PM +0900, Stafford Horne wrote: > Recently OpenRISC added support for external initrd images, but I found > some instability when using larger buildroot initrd images. It turned > out that I forgot to reserve the memblock space for the initrd image. > > This patch fixes the instability issue by reserving memblock space. > > Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") > Signed-off-by: Stafford Horne > --- Forgot to mention: Changes since v1: - Updated to use separate variables as suggested by Mike. > arch/openrisc/kernel/setup.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c > index b18e775f8be3..13c87f1f872b 100644 > --- a/arch/openrisc/kernel/setup.c > +++ b/arch/openrisc/kernel/setup.c > @@ -80,6 +80,16 @@ static void __init setup_memory(void) >*/ > memblock_reserve(__pa(_stext), _end - _stext); > > +#ifdef CONFIG_BLK_DEV_INITRD > + /* Then reserve the initrd, if any */ > + if (initrd_start && (initrd_end > initrd_start)) { > + unsigned long aligned_start = ALIGN_DOWN(initrd_start, > PAGE_SIZE); > + unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE); > + > + memblock_reserve(__pa(aligned_start), aligned_end - > aligned_start); > + } > +#endif /* CONFIG_BLK_DEV_INITRD */ > + > early_init_fdt_reserve_self(); > early_init_fdt_scan_reserved_mem(); > > -- > 2.26.2 >
Re: [PATCH v2 0/3] OpenRISC fixes for 5.9
On Sat, Sep 05, 2020 at 10:19:32PM +0900, Stafford Horne wrote: > Changes since v1: > > - Now a series, v1 was only the "Reserve memblock for initrd" patch > - Added fixes for compiler issues pointed out by the kbuild robot Forgot to mention: - Updated "Reserve memblock for initrd", as per Mike's comments > This is a few fixes found during testing 5.9. > > Stafford Horne (3): > openrisc: Reserve memblock for initrd > openrisc: Fix cache API compile issue when not inlining > openrisc: Fix issue with get_user for 64-bit values > > arch/openrisc/include/asm/uaccess.h | 16 > arch/openrisc/kernel/setup.c| 10 ++ > arch/openrisc/mm/cache.c| 2 +- > 3 files changed, 19 insertions(+), 9 deletions(-) > > -- > 2.26.2 >
[PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values
A build failure was raised by kbuild with the following error. drivers/android/binder.c: Assembler messages: drivers/android/binder.c:3861: Error: unrecognized keyword/register name `l.lwz ?ap,4(r24)' drivers/android/binder.c:3866: Error: unrecognized keyword/register name `l.addi ?ap,r0,0' The issue is with 64-bit get_user() calls on openrisc. I traced this to a problem where in the internally in the get_user macros there is a cast to long __gu_val this causes GCC to think the get_user call is 32-bit. This binder code is really long and GCC allocates register r30, which triggers the issue. The 64-bit get_user asm tries to get the 64-bit pair register, which for r30 overflows the general register names and returns the dummy register ?ap. The fix is to just remove the assignment to the 32-bit temporary variable __gu_val. Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjq%25...@intel.com/ Signed-off-by: Stafford Horne --- arch/openrisc/include/asm/uaccess.h | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index f0390211236b..4a8976dda1a5 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -165,19 +165,19 @@ struct __large_struct { #define __get_user_nocheck(x, ptr, size) \ ({ \ - long __gu_err, __gu_val;\ - __get_user_size(__gu_val, (ptr), (size), __gu_err); \ - (x) = (__force __typeof__(*(ptr)))__gu_val; \ + long __gu_err; \ + __get_user_size((x), (ptr), (size), __gu_err); \ __gu_err; \ }) #define __get_user_check(x, ptr, size) \ ({ \ - long __gu_err = -EFAULT, __gu_val = 0; \ + long __gu_err = -EFAULT;\ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ - if (access_ok(__gu_addr, size)) \ - __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ - (x) = (__force __typeof__(*(ptr)))__gu_val; \ + if (access_ok(__gu_addr, size)) \ + __get_user_size((x), __gu_addr, (size), __gu_err); \ + else\ + (x) = 0;\ __gu_err; \ }) @@ -191,7 +191,7 @@ do { \ case 2: __get_user_asm(x, ptr, retval, "l.lhz"); break; \ case 4: __get_user_asm(x, ptr, retval, "l.lwz"); break; \ case 8: __get_user_asm2(x, ptr, retval); break; \ - default: (x) = __get_user_bad();\ + default: (x) = (__typeof__(x)) __get_user_bad();\ } \ } while (0) -- 2.26.2
[PATCH v2 2/3] openrisc: Fix cache API compile issue when not inlining
I found this when compiling a kbuild random config with GCC 11. The config enables CONFIG_DEBUG_SECTION_MISMATCH, which sets CFLAGS -fno-inline-functions-called-once. This causes the call to cache_loop in cache.c to not be inlined causing the below compile error. In file included from arch/openrisc/mm/cache.c:13: arch/openrisc/mm/cache.c: In function 'cache_loop': ./arch/openrisc/include/asm/spr.h:16:27: warning: 'asm' operand 0 probably does not match constraints 16 | #define mtspr(_spr, _val) __asm__ __volatile__ ( \ | ^~~ arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr' 25 | mtspr(reg, line); | ^ ./arch/openrisc/include/asm/spr.h:16:27: error: impossible constraint in 'asm' 16 | #define mtspr(_spr, _val) __asm__ __volatile__ ( \ | ^~~ arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr' 25 | mtspr(reg, line); | ^ make[1]: *** [scripts/Makefile.build:283: arch/openrisc/mm/cache.o] Error 1 The asm constraint "K" requires a immediate constant argument to mtspr, however because of no inlining a register argument is passed causing a failure. Fix this by using __always_inline. Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjq%25...@intel.com/ Signed-off-by: Stafford Horne --- arch/openrisc/mm/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c index 08f56af387ac..534a52ec5e66 100644 --- a/arch/openrisc/mm/cache.c +++ b/arch/openrisc/mm/cache.c @@ -16,7 +16,7 @@ #include #include -static void cache_loop(struct page *page, const unsigned int reg) +static __always_inline 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); -- 2.26.2
[PATCH v2 1/3] openrisc: Reserve memblock for initrd
Recently OpenRISC added support for external initrd images, but I found some instability when using larger buildroot initrd images. It turned out that I forgot to reserve the memblock space for the initrd image. This patch fixes the instability issue by reserving memblock space. Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") Signed-off-by: Stafford Horne --- arch/openrisc/kernel/setup.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c index b18e775f8be3..13c87f1f872b 100644 --- a/arch/openrisc/kernel/setup.c +++ b/arch/openrisc/kernel/setup.c @@ -80,6 +80,16 @@ static void __init setup_memory(void) */ memblock_reserve(__pa(_stext), _end - _stext); +#ifdef CONFIG_BLK_DEV_INITRD + /* Then reserve the initrd, if any */ + if (initrd_start && (initrd_end > initrd_start)) { + unsigned long aligned_start = ALIGN_DOWN(initrd_start, PAGE_SIZE); + unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE); + + memblock_reserve(__pa(aligned_start), aligned_end - aligned_start); + } +#endif /* CONFIG_BLK_DEV_INITRD */ + early_init_fdt_reserve_self(); early_init_fdt_scan_reserved_mem(); -- 2.26.2
[PATCH v2 0/3] OpenRISC fixes for 5.9
Changes since v1: - Now a series, v1 was only the "Reserve memblock for initrd" patch - Added fixes for compiler issues pointed out by the kbuild robot This is a few fixes found during testing 5.9. Stafford Horne (3): openrisc: Reserve memblock for initrd openrisc: Fix cache API compile issue when not inlining openrisc: Fix issue with get_user for 64-bit values arch/openrisc/include/asm/uaccess.h | 16 arch/openrisc/kernel/setup.c| 10 ++ arch/openrisc/mm/cache.c| 2 +- 3 files changed, 19 insertions(+), 9 deletions(-) -- 2.26.2
Re: [PATCH] openrisc: Reserve memblock for initrd
On Tue, Sep 01, 2020 at 03:11:30PM +0300, Mike Rapoport wrote: > On Tue, Sep 01, 2020 at 07:20:44PM +0900, Stafford Horne wrote: > > On Tue, Sep 01, 2020 at 08:59:24AM +0300, Mike Rapoport wrote: > > > On Tue, Sep 01, 2020 at 06:21:01AM +0900, Stafford Horne wrote: > > > > Recently OpenRISC added support for external initrd images, but I found > > > > some instability when using larger buildroot initrd images. It turned > > > > out that I forgot to reserve the memblock space for the initrd image. > > > > > > > > This patch fixes the instability issue by reserving memblock space. > > > > > > > > Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") > > > > Signed-off-by: Stafford Horne > > > > --- > > > > arch/openrisc/kernel/setup.c | 9 + > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c > > > > index b18e775f8be3..2c8aa53cc7ba 100644 > > > > --- a/arch/openrisc/kernel/setup.c > > > > +++ b/arch/openrisc/kernel/setup.c > > > > @@ -80,6 +80,15 @@ static void __init setup_memory(void) > > > > */ > > > > memblock_reserve(__pa(_stext), _end - _stext); > > > > > > > > +#ifdef CONFIG_BLK_DEV_INITRD > > > > + /* Then reserve the initrd, if any */ > > > > + if (initrd_start && (initrd_end > initrd_start)) { > > > > + memblock_reserve(ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE), > > > > + ALIGN(initrd_end, PAGE_SIZE) - > > > > + ALIGN_DOWN(initrd_start, PAGE_SIZE)); > > > > + } > > > > > > The core mm takes care of reserving the entrire pages for the memory > > > reserved with memblock, so it is not necessary to do it here. > > > > Thanks for the pointer > > > > I guess what you mean is it is not required to do the page alignment. > > Right. > > > I used other architectures as an example and most do the alignment, I > > think for most architectures this can be pulled out to generic code. > > You are more than welcome :) > We already have a generic free_initrd_mem(), maybe it's time to have a > generic reserve_initrd_mem(). > > I'm ok with openrisc doing the alignment, but I think using local > variables would make the core more readable, e.g > > if (initd_start && (initrd_end)) { > unsigned long aligned_start = ALIGN_DOWN(initrd_start, > PAGE_SIZE); > unsigned long aligned_end = ALIGN(end, PAGE_SIZE); > > memblock_reserve(aligned_start, aligned_end); > } > > What do you say? Well, if the alignment is not needed I rather not do it. That would make the code as simple as: if (initrd_start && (initrd_end > initrd_start)) { memblock_reserve(__pa(initrd_start), initrd_end - initrd_start); } Let me look what is involved in making a reserve_initrd_mem(). -Stafford > > -Stafford > > > > > > +#endif /* CONFIG_BLK_DEV_INITRD */ > > > > + > > > > early_init_fdt_reserve_self(); > > > > early_init_fdt_scan_reserved_mem(); > > > > > > > > -- > > > > 2.26.2 > > > > > > > > > > -- > > > Sincerely yours, > > > Mike. > > -- > Sincerely yours, > Mike.
Re: [PATCH] openrisc: Reserve memblock for initrd
On Tue, Sep 01, 2020 at 08:59:24AM +0300, Mike Rapoport wrote: > On Tue, Sep 01, 2020 at 06:21:01AM +0900, Stafford Horne wrote: > > Recently OpenRISC added support for external initrd images, but I found > > some instability when using larger buildroot initrd images. It turned > > out that I forgot to reserve the memblock space for the initrd image. > > > > This patch fixes the instability issue by reserving memblock space. > > > > Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") > > Signed-off-by: Stafford Horne > > --- > > arch/openrisc/kernel/setup.c | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c > > index b18e775f8be3..2c8aa53cc7ba 100644 > > --- a/arch/openrisc/kernel/setup.c > > +++ b/arch/openrisc/kernel/setup.c > > @@ -80,6 +80,15 @@ static void __init setup_memory(void) > > */ > > memblock_reserve(__pa(_stext), _end - _stext); > > > > +#ifdef CONFIG_BLK_DEV_INITRD > > + /* Then reserve the initrd, if any */ > > + if (initrd_start && (initrd_end > initrd_start)) { > > + memblock_reserve(ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE), > > + ALIGN(initrd_end, PAGE_SIZE) - > > + ALIGN_DOWN(initrd_start, PAGE_SIZE)); > > + } > > The core mm takes care of reserving the entrire pages for the memory > reserved with memblock, so it is not necessary to do it here. Thanks for the pointer I guess what you mean is it is not required to do the page alignment. I used other architectures as an example and most do the alignment, I think for most architectures this can be pulled out to generic code. -Stafford > > +#endif /* CONFIG_BLK_DEV_INITRD */ > > + > > early_init_fdt_reserve_self(); > > early_init_fdt_scan_reserved_mem(); > > > > -- > > 2.26.2 > > > > -- > Sincerely yours, > Mike.
[PATCH] openrisc: Reserve memblock for initrd
Recently OpenRISC added support for external initrd images, but I found some instability when using larger buildroot initrd images. It turned out that I forgot to reserve the memblock space for the initrd image. This patch fixes the instability issue by reserving memblock space. Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images") Signed-off-by: Stafford Horne --- arch/openrisc/kernel/setup.c | 9 + 1 file changed, 9 insertions(+) diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c index b18e775f8be3..2c8aa53cc7ba 100644 --- a/arch/openrisc/kernel/setup.c +++ b/arch/openrisc/kernel/setup.c @@ -80,6 +80,15 @@ static void __init setup_memory(void) */ memblock_reserve(__pa(_stext), _end - _stext); +#ifdef CONFIG_BLK_DEV_INITRD + /* Then reserve the initrd, if any */ + if (initrd_start && (initrd_end > initrd_start)) { + memblock_reserve(ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE), + ALIGN(initrd_end, PAGE_SIZE) - + ALIGN_DOWN(initrd_start, PAGE_SIZE)); + } +#endif /* CONFIG_BLK_DEV_INITRD */ + early_init_fdt_reserve_self(); early_init_fdt_scan_reserved_mem(); -- 2.26.2
[GIT PULL] OpenRISC updates for v5.9
Hi Linus, Please consider for pull: The following changes since commit bcf876870b95592b52519ed4aafcf9d95999bc9c: Linux 5.8 (2020-08-02 14:21:45 -0700) are available in the Git repository at: git://github.com/openrisc/linux.git tags/for-linus for you to fetch changes up to 55b2662ec665cc8b592809a011fe807b05370ab8: openrisc: uaccess: Add user address space check to access_ok (2020-08-09 07:57:21 +0900) OpenRISC updates for 5.9 A few patches all over the place during this cycle, mostly bug and sparse warning fixes for OpenRISC, but a few enhancements too. Note, there are 2 non OpenRISC specific fixups. Non OpenRISC fixes: - In init we need to align the init_task correctly to fix an issue with MUTEX_FLAGS, reviewed by Peter Z. No one picked this up so I kept it on my tree. - In asm-generic/io.h I fixed up some sparse warnings, OK'd by Arnd. Arnd asked to merge it via my tree. OpenRISC fixes: - Many fixes for OpenRISC sprase warnings. - Add support OpenRISC SMP tlb flushing rather than always flushing the entire TLB on every CPU. - Fix bug when dumping stack via /proc/xxx/stack of user threads. Luc Van Oostenryck (1): openrisc: fix __user in raw_copy_to_user()'s prototype Stafford Horne (11): init: Align init_task to avoid conflict with MUTEX_FLAGS openrisc: Add support for external initrd images openrisc: Fix oops caused when dumping stack openrisc: Implement proper SMP tlb flushing asm-generic/io.h: Fix sparse warnings on big-endian architectures openrisc: io: Fixup defines and move include to the end openrisc: uaccess: Fix sparse address space warnings openrisc: uaccess: Use static inline function in access_ok openrisc: uaccess: Remove unused macro __addr_ok openrisc: signal: Fix sparse address space warnings openrisc: uaccess: Add user address space check to access_ok arch/openrisc/include/asm/io.h | 9 +++- arch/openrisc/include/asm/uaccess.h | 23 +- arch/openrisc/kernel/setup.c| 8 ++-- arch/openrisc/kernel/signal.c | 14 +++--- arch/openrisc/kernel/smp.c | 85 + arch/openrisc/kernel/stacktrace.c | 18 +++- arch/openrisc/kernel/vmlinux.lds.S | 12 -- arch/openrisc/mm/tlb.c | 17 +--- include/asm-generic/io.h| 16 +++ init/init_task.c| 1 + 10 files changed, 145 insertions(+), 58 deletions(-)
Re: [PATCH v2 5/6] openrisc: signal: Fix sparse address space warnings
On Sun, Aug 09, 2020 at 01:08:42AM +0200, Luc Van Oostenryck wrote: > On Sun, Aug 09, 2020 at 07:48:22AM +0900, Stafford Horne wrote: > > On Thu, Aug 06, 2020 at 09:04:49PM +0200, Luc Van Oostenryck wrote: > > > On Thu, Aug 06, 2020 at 06:07:24AM +0900, Stafford Horne wrote: > > > > --- > > > > arch/openrisc/kernel/signal.c | 14 +++--- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/arch/openrisc/kernel/signal.c > > > > b/arch/openrisc/kernel/signal.c > > > > index 4f0754874d78..7ce0728412f6 100644 > > > > --- a/arch/openrisc/kernel/signal.c > > > > +++ b/arch/openrisc/kernel/signal.c > > > > @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs > > > > *regs) > > > > * then frame should be dword aligned here. If it's > > > > * not, then the user is trying to mess with us. > > > > */ > > > > - if (((long)frame) & 3) > > > > + if (((__force unsigned long)frame) & 3) > > > > goto badframe; > > > > > > Same as patch 6, the __force is not needed. > > > > Thanks, I thought this was complaining before, I tested now and there is no > > problem so I must have been mixed up with something else. > > Sparse should have complained with with the cast to long but > purposely doesn't with unsigned long (or uintptr_t). > So, no mix up, I think. You are right, I noticed that right after I sent that email. Thanks a lot. -Stafford
[PATCH v3 6/6] openrisc: uaccess: Add user address space check to access_ok
Now that __user annotations are fixed for openrisc uaccess api's we can add checking to the access_ok macro. This patch adds the __chk_user_ptr check, on normal builds the added check is a nop. Signed-off-by: Stafford Horne Reviewed-by: Luc Van Oostenryck --- Changes since v2: - Remove __force in cast suggsted by Luc arch/openrisc/include/asm/uaccess.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index 85a55359b244..7c5892f56765 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -57,6 +57,7 @@ static inline int __range_ok(unsigned long addr, unsigned long size) #define access_ok(addr, size) \ ({ \ + __chk_user_ptr(addr); \ __range_ok((unsigned long)(addr), (size)); \ }) -- 2.26.2
[PATCH v3 3/6] openrisc: uaccess: Use static inline function in access_ok
As suggested by Linus when reviewing commit 9cb2feb4d21d ("arch/openrisc: Fix issues with access_ok()") last year; making __range_ok an inline function also fixes the used twice issue that the commit was fixing. I agree it's a good cleanup. This patch addresses that as I am currently working on the access_ok macro to fixup sparse annotations in OpenRISC. Suggested-by: Linus Torvalds Signed-off-by: Stafford Horne Reviewed-by: Luc Van Oostenryck --- arch/openrisc/include/asm/uaccess.h | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index f2fc5c4b88c3..4b59dc9ad300 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -48,16 +48,19 @@ /* Ensure that the range from addr to addr+size is all within the process' * address space */ -#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs()-size)) +static inline int __range_ok(unsigned long addr, unsigned long size) +{ + const mm_segment_t fs = get_fs(); + + return size <= fs && addr <= (fs - size); +} /* Ensure that addr is below task's addr_limit */ #define __addr_ok(addr) ((unsigned long) addr < get_fs()) #define access_ok(addr, size) \ ({ \ - unsigned long __ao_addr = (unsigned long)(addr);\ - unsigned long __ao_size = (unsigned long)(size);\ - __range_ok(__ao_addr, __ao_size); \ + __range_ok((unsigned long)(addr), (size)); \ }) /* -- 2.26.2
[PATCH v3 1/6] openrisc: io: Fixup defines and move include to the end
This didn't seem to cause any issues, but while working on fixing up sparse annotations for OpenRISC I noticed this. This patch moves the include of asm-generic/io.h to the end of the file. Also, we add defines of ioremap and iounmap, that way we don't get duplicate definitions from asm-generic/io.h. Signed-off-by: Stafford Horne --- Changes since v2: - none Changes since v1: - Add linux/types.h include following report from kbuild arch/openrisc/include/asm/io.h | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h index db02fb2077d9..7d6b4a77b379 100644 --- a/arch/openrisc/include/asm/io.h +++ b/arch/openrisc/include/asm/io.h @@ -14,6 +14,8 @@ #ifndef __ASM_OPENRISC_IO_H #define __ASM_OPENRISC_IO_H +#include + /* * PCI: can we really do 0 here if we have no port IO? */ @@ -25,9 +27,12 @@ #define PIO_OFFSET 0 #define PIO_MASK 0 -#include - +#define ioremap ioremap void __iomem *ioremap(phys_addr_t offset, unsigned long size); + +#define iounmap iounmap extern void iounmap(void *addr); +#include + #endif -- 2.26.2
[PATCH v3 5/6] openrisc: signal: Fix sparse address space warnings
The __user annotations in signal.c were mostly missing. The missing annotations caused the warnings listed below. This patch fixes them up by adding the __user annotations. arch/openrisc/kernel/signal.c:71:38: warning: incorrect type in initializer (different address spaces) arch/openrisc/kernel/signal.c:71:38:expected struct rt_sigframe *frame arch/openrisc/kernel/signal.c:71:38:got struct rt_sigframe [noderef] __user * arch/openrisc/kernel/signal.c:82:14: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:82:14:expected void const volatile [noderef] __user * arch/openrisc/kernel/signal.c:82:14:got struct rt_sigframe *frame arch/openrisc/kernel/signal.c:84:37: warning: incorrect type in argument 2 (different address spaces) arch/openrisc/kernel/signal.c:84:37:expected void const [noderef] __user *from arch/openrisc/kernel/signal.c:84:37:got struct sigset_t * arch/openrisc/kernel/signal.c:89:39: warning: incorrect type in argument 2 (different address spaces) arch/openrisc/kernel/signal.c:89:39:expected struct sigcontext [noderef] __user *sc arch/openrisc/kernel/signal.c:89:39:got struct sigcontext * arch/openrisc/kernel/signal.c:92:31: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:92:31:expected struct sigaltstack const [noderef] [usertype] __user * arch/openrisc/kernel/signal.c:92:31:got struct sigaltstack * arch/openrisc/kernel/signal.c:158:15: warning: incorrect type in assignment (different address spaces) arch/openrisc/kernel/signal.c:158:15:expected struct rt_sigframe *frame arch/openrisc/kernel/signal.c:158:15:got void [noderef] __user * arch/openrisc/kernel/signal.c:160:14: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:160:14:expected void const volatile [noderef] __user * arch/openrisc/kernel/signal.c:160:14:got struct rt_sigframe *frame arch/openrisc/kernel/signal.c:165:46: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:165:46:expected struct siginfo [noderef] [usertype] __user *to arch/openrisc/kernel/signal.c:165:46:got struct siginfo * arch/openrisc/kernel/signal.c:170:33: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:170:33:expected struct sigaltstack [noderef] [usertype] __user * arch/openrisc/kernel/signal.c:170:33:got struct sigaltstack * arch/openrisc/kernel/signal.c:171:40: warning: incorrect type in argument 2 (different address spaces) arch/openrisc/kernel/signal.c:171:40:expected struct sigcontext [noderef] __user *sc arch/openrisc/kernel/signal.c:171:40:got struct sigcontext * arch/openrisc/kernel/signal.c:173:32: warning: incorrect type in argument 1 (different address spaces) arch/openrisc/kernel/signal.c:173:32:expected void [noderef] __user *to arch/openrisc/kernel/signal.c:173:32:got struct sigset_t * Signed-off-by: Stafford Horne Reviewed-by: Luc Van Oostenryck --- Changes since v2: - Remove __force in cast suggested by Luc Van Oostenryck arch/openrisc/kernel/signal.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c index 4f0754874d78..97804f21a40c 100644 --- a/arch/openrisc/kernel/signal.c +++ b/arch/openrisc/kernel/signal.c @@ -68,7 +68,7 @@ static int restore_sigcontext(struct pt_regs *regs, asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs) { - struct rt_sigframe *frame = (struct rt_sigframe __user *)regs->sp; + struct rt_sigframe __user *frame = (struct rt_sigframe __user *)regs->sp; sigset_t set; /* @@ -76,7 +76,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs) * then frame should be dword aligned here. If it's * not, then the user is trying to mess with us. */ - if (((long)frame) & 3) + if (((unsigned long)frame) & 3) goto badframe; if (!access_ok(frame, sizeof(*frame))) @@ -151,7 +151,7 @@ static inline void __user *get_sigframe(struct ksignal *ksig, static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs) { - struct rt_sigframe *frame; + struct rt_sigframe __user *frame; unsigned long return_ip; int err = 0; @@ -181,10 +181,10 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, l.ori r11,r0,__NR_sigreturn l.sys 1 */ - err |= __put_user(0xa960, (short *)(frame->retcode + 0)); - err |= __put_user(__NR_rt_sigreturn, (short *)(frame->retcode + 2)); - err |= __put_user(0x2001, (unsigned long *)(frame->retcode + 4)); - err |= __put_user(0x1500, (unsigned long *)(frame->retcode + 8)); +
[PATCH v3 4/6] openrisc: uaccess: Remove unused macro __addr_ok
Since commit b48b2c3e5043 ("openrisc: use generic strnlen_user() function") the macro __addr_ok is no longer used. It is safe to remove so this patch removes it. Signed-off-by: Stafford Horne --- arch/openrisc/include/asm/uaccess.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index 4b59dc9ad300..85a55359b244 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -55,9 +55,6 @@ static inline int __range_ok(unsigned long addr, unsigned long size) return size <= fs && addr <= (fs - size); } -/* Ensure that addr is below task's addr_limit */ -#define __addr_ok(addr) ((unsigned long) addr < get_fs()) - #define access_ok(addr, size) \ ({ \ __range_ok((unsigned long)(addr), (size)); \ -- 2.26.2
[PATCH v3 2/6] openrisc: uaccess: Fix sparse address space warnings
The OpenRISC user access functions put_user(), get_user() and clear_user() were missing proper sparse annotations. This generated warnings like the below. This patch adds the annotations to fix the warnings. Example warnings: net/ipv4/ip_sockglue.c:759:29: warning: incorrect type in argument 1 (different address spaces) net/ipv4/ip_sockglue.c:759:29:expected void const volatile [noderef] __user * net/ipv4/ip_sockglue.c:759:29:got int const *__gu_addr net/ipv4/ip_sockglue.c:764:29: warning: incorrect type in initializer (different address spaces) net/ipv4/ip_sockglue.c:764:29:expected unsigned char const *__gu_addr net/ipv4/ip_sockglue.c:764:29:got unsigned char [noderef] __user * Signed-off-by: Stafford Horne Reviewed-by: Luc Van Oostenryck --- arch/openrisc/include/asm/uaccess.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h index 46e31bb4a9ad..f2fc5c4b88c3 100644 --- a/arch/openrisc/include/asm/uaccess.h +++ b/arch/openrisc/include/asm/uaccess.h @@ -100,7 +100,7 @@ extern long __put_user_bad(void); #define __put_user_check(x, ptr, size) \ ({ \ long __pu_err = -EFAULT;\ - __typeof__(*(ptr)) *__pu_addr = (ptr); \ + __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ if (access_ok(__pu_addr, size)) \ __put_user_size((x), __pu_addr, (size), __pu_err); \ __pu_err; \ @@ -173,7 +173,7 @@ struct __large_struct { #define __get_user_check(x, ptr, size) \ ({ \ long __gu_err = -EFAULT, __gu_val = 0; \ - const __typeof__(*(ptr)) * __gu_addr = (ptr); \ + const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ if (access_ok(__gu_addr, size)) \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ @@ -248,10 +248,10 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long size) #define INLINE_COPY_FROM_USER #define INLINE_COPY_TO_USER -extern unsigned long __clear_user(void *addr, unsigned long size); +extern unsigned long __clear_user(void __user *addr, unsigned long size); static inline __must_check unsigned long -clear_user(void *addr, unsigned long size) +clear_user(void __user *addr, unsigned long size) { if (likely(access_ok(addr, size))) size = __clear_user(addr, size); -- 2.26.2
[PATCH v3 0/6] OpenRISC header and sparse warning fixes for 5.9
Hello, Changes since v1: - in "io: Fixup defines and move include to the end" added a linux/types.h include as there were compiler failurs pointed out by kbuild. - Remove some __force's suggested by Luc Van Oostenryck This a series of fixes for OpenRISC sparse warnings. The kbuild robots report many issues related to issues with OpenRISC headers having missing or incorrect sparse annotations. Example kdbuild-all report: net/ipv4/ip_sockglue.c:1489:13: sparse: sparse: incorrect type in initializer (different address spaces) https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org/thread/MB6SE7BX425ENFTSIL6KAOB3CVS4WJLH/ Also this includes a few cleanups which I noticed while working on the warning fixups. -Stafford Stafford Horne (6): openrisc: io: Fixup defines and move include to the end openrisc: uaccess: Fix sparse address space warnings openrisc: uaccess: Use static inline function in access_ok openrisc: uaccess: Remove unused macro __addr_ok openrisc: signal: Fix sparse address space warnings openrisc: uaccess: Add user address space check to access_ok arch/openrisc/include/asm/io.h | 9 +++-- arch/openrisc/include/asm/uaccess.h | 21 +++-- arch/openrisc/kernel/signal.c | 14 +++--- 3 files changed, 25 insertions(+), 19 deletions(-) -- 2.26.2