* [PATCH] arm: skip nomap memblocks while finding the lowmem/highmem boundary
From: Chester Lin @ 2019-08-22 3:45 UTC (permalink / raw)
To: linux@armlinux.org.uk, rppt@linux.ibm.com,
ard.biesheuvel@linaro.org, akpm@linux-foundation.org,
geert@linux-m68k.org, tglx@linutronix.de, mark.rutland@arm.com
Cc: Joey Lee, guillaume.gardet@arm.com, linux-kernel@vger.kernel.org,
Chester Lin, Gary Lin, linux-arm-kernel@lists.infradead.org
adjust_lowmem_bounds() checks every memblocks in order to find the boundary
between lowmem and highmem. However some memblocks could be marked as NOMAP
so they are not used by kernel, which should be skipped while calculating
the boundary.
Signed-off-by: Chester Lin <clin@suse.com>
---
arch/arm/mm/mmu.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 426d9085396b..b86dba44d828 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1181,6 +1181,9 @@ void __init adjust_lowmem_bounds(void)
phys_addr_t block_start = reg->base;
phys_addr_t block_end = reg->base + reg->size;
+ if (memblock_is_nomap(reg))
+ continue;
+
if (reg->base < vmalloc_limit) {
if (block_end > lowmem_limit)
/*
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4] arm64: implement KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-22 3:45 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Masami Hiramatsu, Will Deacon,
Naveen N. Rao
Cc: Mark Rutland, linux-doc@vger.kernel.org, Peter Zijlstra,
linux-kernel@vger.kernel.org, Steven Rostedt, Ingo Molnar,
Thomas Gleixner, linux-arm-kernel@lists.infradead.org
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Tested on berlin arm64 platform.
~ # mount -t debugfs debugfs /sys/kernel/debug/
~ # cd /sys/kernel/debug/
/sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
before the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009fe28 k _do_fork+0x0 [DISABLED]
after the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Applied after arm64 FTRACE_WITH_REGS:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674404.html
Changes since v3:
- move kprobe_lookup_name() and arch_kprobe_on_func_entry to ftrace.c since
we only want to choose the ftrace entry for KPROBES_ON_FTRACE.
- only choose ftrace entry if (addr && !offset)
Changes since v2:
- remove patch1, make it a single cleanup patch
- remove "This patch" in the change log
- implement arm64's kprobe_lookup_name() and arch_kprobe_on_func_entry instead
of patching the common kprobes code
Changes since v1:
- make the kprobes/x86: use instruction_pointer and instruction_pointer_set
as patch1
- add Masami's ACK to patch1
- add some description about KPROBES_ON_FTRACE and why we need it on
arm64
- correct the log before the patch
- remove the consolidation patch, make it as TODO
- only adjust kprobe's addr when KPROBE_FLAG_FTRACE is set
- if KPROBES_ON_FTRACE, ftrace_call_adjust() the kprobe's addr before
calling ftrace_location()
- update the kprobes-on-ftrace/arch-support.txt in doc
.../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/probes/Makefile | 1 +
arch/arm64/kernel/probes/ftrace.c | 84 +++++++++++++++++++
4 files changed, 87 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/probes/ftrace.c
diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
index 68f266944d5f..e8358a38981c 100644
--- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
+++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
@@ -9,7 +9,7 @@
| alpha: | TODO |
| arc: | TODO |
| arm: | TODO |
- | arm64: | TODO |
+ | arm64: | ok |
| c6x: | TODO |
| csky: | TODO |
| h8300: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 663392d1eae2..928700f15e23 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -167,6 +167,7 @@ config ARM64
select HAVE_STACKPROTECTOR
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_KPROBES
+ select HAVE_KPROBES_ON_FTRACE
select HAVE_KRETPROBES
select HAVE_GENERIC_VDSO
select IOMMU_DMA if IOMMU_SUPPORT
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index 8e4be92e25b1..4020cfc66564 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
simulate-insn.o
obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
simulate-insn.o
+obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
new file mode 100644
index 000000000000..5989c57660f3
--- /dev/null
+++ b/arch/arm64/kernel/probes/ftrace.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dynamic Ftrace based Kprobes Optimization
+ *
+ * Copyright (C) Hitachi Ltd., 2012
+ * Copyright (C) 2019 Jisheng Zhang <jszhang@kernel.org>
+ * Synaptics Incorporated
+ */
+
+#include <linux/kprobes.h>
+
+/* Ftrace callback handler for kprobes -- called under preepmt disabed */
+void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *ops, struct pt_regs *regs)
+{
+ struct kprobe *p;
+ struct kprobe_ctlblk *kcb;
+
+ /* Preempt is disabled by ftrace */
+ p = get_kprobe((kprobe_opcode_t *)ip);
+ if (unlikely(!p) || kprobe_disabled(p))
+ return;
+
+ kcb = get_kprobe_ctlblk();
+ if (kprobe_running()) {
+ kprobes_inc_nmissed_count(p);
+ } else {
+ unsigned long orig_ip = instruction_pointer(regs);
+ /* Kprobe handler expects regs->pc = pc + 4 as breakpoint hit */
+ instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
+
+ __this_cpu_write(current_kprobe, p);
+ kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+ if (!p->pre_handler || !p->pre_handler(p, regs)) {
+ /*
+ * Emulate singlestep (and also recover regs->pc)
+ * as if there is a nop
+ */
+ instruction_pointer_set(regs,
+ (unsigned long)p->addr + MCOUNT_INSN_SIZE);
+ if (unlikely(p->post_handler)) {
+ kcb->kprobe_status = KPROBE_HIT_SSDONE;
+ p->post_handler(p, regs, 0);
+ }
+ instruction_pointer_set(regs, orig_ip);
+ }
+ /*
+ * If pre_handler returns !0, it changes regs->pc. We have to
+ * skip emulating post_handler.
+ */
+ __this_cpu_write(current_kprobe, NULL);
+ }
+}
+NOKPROBE_SYMBOL(kprobe_ftrace_handler);
+
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
+{
+ unsigned long addr = kallsyms_lookup_name(name);
+
+ if (addr && !offset) {
+ unsigned long faddr;
+ /*
+ * with -fpatchable-function-entry=2, the first 4 bytes is the
+ * LR saver, then the actual call insn. So ftrace location is
+ * always on the first 4 bytes offset.
+ */
+ faddr = ftrace_location_range(addr,
+ addr + AARCH64_INSN_SIZE);
+ if (faddr)
+ return (kprobe_opcode_t *)faddr;
+ }
+ return (kprobe_opcode_t *)addr;
+}
+
+bool arch_kprobe_on_func_entry(unsigned long offset)
+{
+ return offset <= AARCH64_INSN_SIZE;
+}
+
+int arch_prepare_kprobe_ftrace(struct kprobe *p)
+{
+ p->ainsn.api.insn = NULL;
+ return 0;
+}
--
2.23.0.rc1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Nick Desaulniers @ 2019-08-22 3:26 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: Tri Vo, Arnd Bergmann, Ard Biesheuvel, Russell King, LKML,
clang-built-linux, Miles Chen (陳民樺),
Linux ARM
In-Reply-To: <20190821174619.21935-1-nhuck@google.com>
On Wed, Aug 21, 2019 at 10:46 AM Nathan Huckleberry <nhuck@google.com> wrote:
>
> The stackframe setup when compiled with clang is different.
> Since the stack unwinder expects the gcc stackframe setup it
> fails to print backtraces. This patch adds support for the
> clang stackframe setup.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/35
> Cc: clang-built-linux@googlegroups.com
> Suggested-by: Tri Vo <trong@google.com>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
> Changes from v1->v2
> * Fix indentation in various files
> * Swap spaces for tabs
> * Rename Ldsi to Lopcode
> * Remove unused Ldsi entry
>
> arch/arm/Kconfig.debug | 2 +-
> arch/arm/Makefile | 5 +-
> arch/arm/lib/Makefile | 8 +-
> arch/arm/lib/backtrace-clang.S | 229 +++++++++++++++++++++++++++++++++
> 4 files changed, 241 insertions(+), 3 deletions(-)
> create mode 100644 arch/arm/lib/backtrace-clang.S
>
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 85710e078afb..b9c674ec19e0 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -56,7 +56,7 @@ choice
>
> config UNWINDER_FRAME_POINTER
> bool "Frame pointer unwinder"
> - depends on !THUMB2_KERNEL && !CC_IS_CLANG
> + depends on !THUMB2_KERNEL
> select ARCH_WANT_FRAME_POINTERS
> select FRAME_POINTER
> help
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index c3624ca6c0bc..6f251c201db0 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -36,7 +36,10 @@ KBUILD_CFLAGS += $(call cc-option,-mno-unaligned-access)
> endif
>
> ifeq ($(CONFIG_FRAME_POINTER),y)
> -KBUILD_CFLAGS +=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
> +KBUILD_CFLAGS +=-fno-omit-frame-pointer
> +ifeq ($(CONFIG_CC_IS_GCC),y)
> +KBUILD_CFLAGS += -mapcs -mno-sched-prolog
> +endif
> endif
>
> ifeq ($(CONFIG_CPU_BIG_ENDIAN),y)
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index b25c54585048..6d2ba454f25b 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -5,7 +5,7 @@
> # Copyright (C) 1995-2000 Russell King
> #
>
> -lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
> +lib-y := changebit.o csumipv6.o csumpartial.o \
> csumpartialcopy.o csumpartialcopyuser.o clearbit.o \
> delay.o delay-loop.o findbit.o memchr.o memcpy.o \
> memmove.o memset.o setbit.o \
> @@ -19,6 +19,12 @@ lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
> mmu-y := clear_user.o copy_page.o getuser.o putuser.o \
> copy_from_user.o copy_to_user.o
>
> +ifdef CONFIG_CC_IS_CLANG
> + lib-y += backtrace-clang.o
> +else
> + lib-y += backtrace.o
> +endif
> +
> # using lib_ here won't override already available weak symbols
> obj-$(CONFIG_UACCESS_WITH_MEMCPY) += uaccess_with_memcpy.o
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> new file mode 100644
> index 000000000000..6f2a8a57d0fb
> --- /dev/null
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -0,0 +1,229 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * linux/arch/arm/lib/backtrace-clang.S
> + *
> + * Copyright (C) 2019 Nathan Huckleberry
> + *
> + */
> +#include <linux/kern_levels.h>
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> + .text
> +
> +/* fp is 0 or stack frame */
> +
> +#define frame r4
> +#define sv_fp r5
> +#define sv_pc r6
> +#define mask r7
> +#define sv_lr r8
> +
> +ENTRY(c_backtrace)
> +
> +#if !defined(CONFIG_FRAME_POINTER) || !defined(CONFIG_PRINTK)
> + ret lr
> +ENDPROC(c_backtrace)
> +#else
> +
> +
> +/*
> + * Clang does not store pc or sp in function prologues
> + * so we don't know exactly where the function
> + * starts.
To quickly re-wrap text (if you're using vim) such as with comments like these:
shift+v (VISUAL LINE MODE)
j or k to highlight lines
gq (to rewrap)
You may need `set cc=80` (not sure).
> + *
> + * We can treat the current frame's lr as the saved pc and the
> + * preceding frame's lr as the current frame's lr,
> + * but we can't trace the most recent call.
> + * Inserting a false stack frame allows us to reference the
> + * function called last in the stacktrace.
> + *
> + * If the call instruction was a bl we can look at the callers
> + * branch instruction to calculate the saved pc.
> + * We can recover the pc in most cases, but in cases such as
> + * calling function pointers we cannot. In this
> + * case, default to using the lr. This will be
> + * some address in the function, but will not
> + * be the function start.
> + *
> + * Unfortunately due to the stack frame layout we can't dump
> + * r0 - r3, but these are less frequently saved.
I guess if they were spilled, but I'm ok with this; I'd rather have a
working unwinder than disabled config. The printing is a debug
feature that's nice to have, but the main focus should be unwinding.
We can always revisit improving support.
> + *
> + * Stack frame layout:
> + * <larger addresses>
> + * saved lr
> + * frame=> saved fp
> + * optionally saved caller registers (r4 - r10)
> + * optionally saved arguments (r0 - r3)
> + * <top of stack frame>
> + * <smaller addresses>
> + *
> + * Functions start with the following code sequence:
> + * corrected pc => stmfd sp!, {..., fp, lr}
> + * add fp, sp, #x
> + * stmfd sp!, {r0 - r3} (optional)
> + *
> + *
> + *
> + *
> + *
> + *
> + * The diagram below shows an example stack setup
> + * for dump_stack.
> + *
> + * The frame for c_backtrace has pointers to the
> + * code of dump_stack. This is why the frame of
> + * c_backtrace is used to for the pc calculation
> + * of dump_stack. This is why we must move back
> + * a frame to print dump_stack.
> + *
> + * The stored locals for dump_stack are in dump_stack's
> + * frame. This means that to fully print dump_stack's frame
> + * we need both the frame for dump_stack (for locals) and the
> + * frame that was called by dump_stack (for pc).
> + *
> + * To print locals we must know where the function start is. If
> + * we read the function prologue opcodes we can determine
> + * which variables are stored in the stack frame.
> + *
> + * To find the function start of dump_stack we can look at the
> + * stored LR of show_stack. It points at the instruction
> + * directly after the bl dump_stack. We can then read the
> + * offset from the bl opcode to determine where the branch takes us.
> + * The address calculated must be the start of dump_stack.
> + *
> + * c_backtrace frame dump_stack:
> + * {[LR] } ============| ...
> + * {[FP] } =======| | bl c_backtrace
> + * | |=> ...
> + * {[R4-R10]} |
> + * {[R0-R3] } | show_stack:
> + * dump_stack frame | ...
> + * {[LR] } =============| bl dump_stack
> + * {[FP] } <=======| |=> ...
> + * {[R4-R10]}
> + * {[R0-R3] }
> + */
> +
===>
> +stmfd sp!, {r4 - r9, fp, lr} @ Save an extra register
> + @ to ensure 8 byte alignment
> +movs frame, r0 @ if frame pointer is zero
> +beq no_frame @ we have no stack frames
> +
> +tst r1, #0x10 @ 26 or 32-bit mode?
> +moveq mask, #0xfc000003
> +movne mask, #0 @ mask for 32-bit
<== this section of the patch has weird indentation. The rest uses 2
tabs, this has none.
> +
> +/*
> + * Switches the current frame to be the frame for dump_stack.
> + */
> + add frame, sp, #24 @ switch to false frame
> +for_each_frame: tst frame, mask @ Check for address exceptions
> + bne no_frame
> +
> +/*
> + * sv_fp is the stack frame with the locals for the current considered
> + * function.
> + *
> + * sv_pc is the saved lr frame the frame above. This is a pointer to a
> + * code address within the current considered function, but
> + * it is not the function start. This value gets updated to be
> + * the function start later if it is possible.
> + */
> +1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> +1002: ldr sv_fp, [frame, #0] @ get saved fp
> +
> + teq sv_fp, mask @ make sure next frame exists
> + beq no_frame
> +
> +/*
> + * sv_lr is the lr from the function that called the current function. This
> + * is a pointer to a code address in the current function's caller.
> + * sv_lr-4 is the instruction used to call the current function.
> + *
> + * This sv_lr can be used to calculate the function start if the function
> + * was called using a bl instruction. If the function start
> + * can be recovered sv_pc is overwritten with the function start.
> + *
> + * If the current function was called using a function pointer we cannot
> + * recover the function start and instead continue with sv_pc as
> + * an arbitrary value within the current function. If this is the case
> + * we cannot print registers for the current function, but the stacktrace
> + * is still printed properly.
> + */
> +1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> +
> + ldr r0, [sv_lr, #-4] @ get call instruction
> + ldr r3, .Lopcode+4
> + and r2, r3, r0 @ is this a bl call
> + teq r2, r3
> + bne finished_setup @ give up if it's not
> + and r0, #0xffffff @ get call offset 24-bit int
> + lsl r0, r0, #8 @ sign extend offset
> + asr r0, r0, #8
> + ldr sv_pc, [sv_fp, #4] @ get lr address
> + add sv_pc, sv_pc, #-4 @ get call instruction address
> + add sv_pc, sv_pc, #8 @ take care of prefetch
> + add sv_pc, sv_pc, r0, lsl #2@ find function start
> +
> +finished_setup:
> +
> + bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
> +
> +/*
> + * Print the function (sv_pc) and where it was called
> + * from (sv_lr).
> + */
> +1004: mov r0, sv_pc
> +
> + mov r1, sv_lr
> + mov r2, frame
> + bic r1, r1, mask @ mask PC/LR for the mode
> + bl dump_backtrace_entry
> +
> +/*
> + * Test if the function start is a stmfd instruction
> + * to determine which registers were stored in the function
> + * prologue.
> + *
> + * If we could not recover the sv_pc because we were called through
> + * a function pointer the comparison will fail and no registers
> + * will print.
Will we still unwind though?
> + */
> +1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> + ldr r3, .Lopcode @ instruction exists,
> + teq r3, r1, lsr #11
> + ldr r0, [frame] @ locals are stored in
> + @ the preceding frame
> + subeq r0, r0, #4
> + bleq dump_backtrace_stm @ dump saved registers
> +
> +/*
> + * If we are out of frames or if the next frame is invalid.
> + */
> + teq sv_fp, #0 @ zero saved fp means
> + beq no_frame @ no further frames
> +
> + cmp sv_fp, frame @ next frame must be
> + mov frame, sv_fp @ above the current frame
> + bhi for_each_frame
> +
> +1006: adr r0, .Lbad
> + mov r1, frame
> + bl printk
> +no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> +ENDPROC(c_backtrace)
> + .pushsection __ex_table,"a"
> + .align 3
> + .long 1001b, 1006b
> + .long 1002b, 1006b
> + .long 1003b, 1006b
> + .long 1004b, 1006b
> + .long 1005b, 1006b
> + .popsection
> +
> +.Lbad: .asciz "Backtrace aborted due to bad frame pointer <%p>\n"
> + .align
> +.Lopcode: .word 0xe92d4800 >> 11 @ stmfd sp!, {... fp, lr}
> + .word 0x0b000000 @ bl if these bits are set
> +
> +#endif
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
OK, with you patch applied on today's Linux next,
CONFIG_UNWINDER_FRAME_POINTER, and ToT Clang:
$ qemu-system-arm -kernel arch/arm/boot/zImage -nographic -m 2048
--append "console=ttyAMA0 root=/dev/ram0" -machine virt
[ 0.000000] Linux version 5.3.0-rc5-07709-gac2d7d4a10c1-dirty
(ndesaulniers@ndesaulniers1.mtv.corp.google.com) (clang version 10.0.0
(https://github.com/llvm/llvm-project.git
da648ab8de3638ff82d6b9349c603b854a0224d6)) #53 SMP Wed Aug 21 20:05:15
PDT 2019
...
[ 0.957046] Kernel panic - not syncing: VFS: Unable to mount root
fs on unknown-block(1,0)
[ 0.957490] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.3.0-rc5-07709-gac2d7d4a10c1-dirty #53
[ 0.957808] Hardware name: Generic DT based system
[ 0.958046] Backtrace:
[ 0.958504] [<c030da98>] (dump_backtrace) from [<c030da94>]
(show_stack+0x14/0x18)
[ 0.958844] r10:c16f585c r6:00000000 r5:c198c7e4 r4:600000d3
[ 0.959085] [<c030da80>] (show_stack) from [<c106c2c8>]
(dump_stack+0xac/0xd8)
[ 0.959358] [<c106c21c>] (dump_stack) from [<c03504a0>] (panic+0x118/0x354)
[ 0.959568] r5:c19a61b5 r4:c1427ce5
[ 0.959722] [<c0350388>] (panic) from [<c1601624>]
(mount_block_root+0x13c/0x1f0)
[ 0.959947] [<c16014e8>] (mount_block_root) from [<c1601a48>]
(mount_root+0xb0/0xb4)
[ 0.960210] r10:00000000 r9:00000000 r8:00000000 r7:00000000
r6:00000000 r5:00100000
[ 0.960492] r4:c1427d49
[ 0.960600] [<c1601998>] (mount_root) from [<c1601d0c>]
(prepare_namespace+0x1ec/0x1f0)
[ 0.960886] r5:c19a3d30 r4:c16f5868
[ 0.961021] [<c1601b20>] (prepare_namespace) from [<c16011ac>]
(kernel_init_freeable+0xe0/0xf4)
[ 0.961330] r5:00000000 r4:c19a3d1c
[ 0.961468] [<c16010cc>] (kernel_init_freeable) from [<c10868d8>]
(kernel_init+0xc/0x2ac)
[ 0.961761] r5:c10868cc r4:00000000
[ 0.961913] [<c10868d8>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[ 0.962210] Exception stack(0xea09bf94 to 0xea09bfdc)
[ 0.962490] bf80:
c10868d8 00000000 c10868cc
[ 0.962883] bfa0: 00000000 00000000 00000000 c03010e8 00000000
00000000 00000000 00000000
[ 0.963202] bfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000
[ 0.963936] ---[ end Kernel panic - not syncing: VFS: Unable to
mount root fs on unknown-block(1,0) ]---
For comparison, the reference implementation:
[ 0.000000] Linux version 5.3.0-rc5-07709-gac2d7d4a10c1-dirty
(ndesaulniers@ndesaulniers1.mtv.corp.google.com) (gcc version 8.2.0
(Debian 8.2.0-14+build1)) #54 SMP Wed Aug 21 20:15:27 PDT 2019
...
[ 1.048134] Kernel panic - not syncing: VFS: Unable to mount root
fs on unknown-block(1,0)
[ 1.048617] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.3.0-rc5-07709-gac2d7d4a10c1-dirty #54
[ 1.048935] Hardware name: Generic DT based system
[ 1.049191] Backtrace:
[ 1.049663] [<c030dff8>] (dump_backtrace) from [<c030e368>]
(show_stack+0x18/0x1c)
[ 1.050038] r7:c16ed85c r6:600000d3 r5:00000000 r4:c198f804
[ 1.050292] [<c030e350>] (show_stack) from [<c0f550bc>]
(dump_stack+0xbc/0xd0)
[ 1.050581] [<c0f55000>] (dump_stack) from [<c0349fe8>] (panic+0x118/0x330)
[ 1.050834] r7:c16ed85c r6:c13404fc r5:00000000 r4:c19a5870
[ 1.051078] [<c0349ed4>] (panic) from [<c16017dc>]
(mount_block_root+0x264/0x284)
[ 1.051344] r3:0ed1c175 r2:0ed1c175 r1:ea09be84 r0:c13404fc
[ 1.051553] r7:c16ed85c
[ 1.051661] [<c1601578>] (mount_block_root) from [<c1601a64>]
(mount_root+0x124/0x140)
[ 1.051936] r10:ffffe000 r9:c16ed858 r8:c19a3400 r7:c1809100
r6:00000008 r5:c1804c48
[ 1.052238] r4:00100000
[ 1.052351] [<c1601940>] (mount_root) from [<c1601c04>]
(prepare_namespace+0x184/0x1cc)
[ 1.052638] r10:ffffe000 r9:c16ed858 r8:c19a3400 r7:c19a3400
r6:00000008 r5:c19a3430
[ 1.052915] r4:c16ed85c
[ 1.053025] [<c1601a80>] (prepare_namespace) from [<c1601308>]
(kernel_init_freeable+0x2f8/0x308)
[ 1.053337] r6:00000008 r5:c177b7c0 r4:c16ed838
[ 1.053507] [<c1601010>] (kernel_init_freeable) from [<c0f6ceb0>]
(kernel_init+0x10/0x118)
[ 1.053798] r10:00000000 r9:00000000 r8:00000000 r7:00000000
r6:00000000 r5:c0f6cea0
[ 1.054083] r4:00000000
[ 1.054190] [<c0f6cea0>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[ 1.054501] Exception stack(0xea09bfb0 to 0xea09bff8)
[ 1.054792] bfa0: 00000000
00000000 00000000 00000000
[ 1.055169] bfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 1.055526] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 1.055795] r5:c0f6cea0 r4:00000000
[ 1.056410] ---[ end Kernel panic - not syncing: VFS: Unable to
mount root fs on unknown-block(1,0) ]---
So the stack traces look comparable (same unwind "path"). Looks like
GCC spilled r0-r3 in panic(), but not much else. I guess Clang could
have spilled these anywhere and we simply wont be able to print them.
Maybe making the comment about this in you patch ALL CAPS might draw
attention to it in case someone ever notices a difference between the
unwind printout and the disassembly, but I assume that's unlikely, but
I also don't know if this functionality is relied upon heavily for
debugging.
In that sense:
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
(you can carry that on to v3)
With the above suggestions, I'd be happy to then add my reviewed by
tag. Thanks for all of the work that went into this.
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH V4 4/4] ARM: dts: imx7ulp: Add wdog1 node
From: Anson Huang @ 2019-08-22 2:37 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, schnitzeltony,
u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1566441463-11911-1-git-send-email-Anson.Huang@nxp.com>
Add wdog1 node to support watchdog driver.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
arch/arm/boot/dts/imx7ulp.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/imx7ulp.dtsi b/arch/arm/boot/dts/imx7ulp.dtsi
index 6859a3a..1fdb5a35 100644
--- a/arch/arm/boot/dts/imx7ulp.dtsi
+++ b/arch/arm/boot/dts/imx7ulp.dtsi
@@ -264,6 +264,16 @@
#clock-cells = <1>;
};
+ wdog1: wdog@403d0000 {
+ compatible = "fsl,imx7ulp-wdt";
+ reg = <0x403d0000 0x10000>;
+ interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
+ timeout-sec = <40>;
+ };
+
pcc2: clock-controller@403f0000 {
compatible = "fsl,imx7ulp-pcc2";
reg = <0x403f0000 0x10000>;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH V4 1/4] dt-bindings: watchdog: Add i.MX7ULP bindings
From: Anson Huang @ 2019-08-22 2:37 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, schnitzeltony,
u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
Add the watchdog bindings for Freescale i.MX7ULP.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
.../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
new file mode 100644
index 0000000..d83fc5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
@@ -0,0 +1,22 @@
+* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
+
+Required properties:
+- compatible : Should be "fsl,imx7ulp-wdt"
+- reg : Should contain WDT registers location and length
+- interrupts : Should contain WDT interrupt
+- clocks: Should contain a phandle pointing to the gated peripheral clock.
+
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
+Examples:
+
+wdog1: wdog@403d0000 {
+ compatible = "fsl,imx7ulp-wdt";
+ reg = <0x403d0000 0x10000>;
+ interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
+ timeout-sec = <40>;
+};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH V4 2/4] watchdog: Add i.MX7ULP watchdog support
From: Anson Huang @ 2019-08-22 2:37 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, schnitzeltony,
u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1566441463-11911-1-git-send-email-Anson.Huang@nxp.com>
The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
that is available for system use.
It provides a safety feature to ensure that software is executing
as planned and that the CPU is not stuck in an infinite loop or
executing unintended code. If the WDOG module is not serviced
(refreshed) within a certain period, it resets the MCU.
Add driver support for i.MX7ULP watchdog.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V3:
- pass clk directly for reset action to avoid dereference from structure;
- use constant instead of variable for wdog clock rate, as it is fixed as 1KHz.
---
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/imx7ulp_wdt.c | 243 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 drivers/watchdog/imx7ulp_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index a8f5c81..d68e5b5 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -724,6 +724,19 @@ config IMX_SC_WDT
To compile this driver as a module, choose M here: the
module will be called imx_sc_wdt.
+config IMX7ULP_WDT
+ tristate "IMX7ULP Watchdog"
+ depends on ARCH_MXC || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ This is the driver for the hardware watchdog on the Freescale
+ IMX7ULP and later processors. If you have one of these
+ processors and wish to have watchdog support enabled,
+ say Y, otherwise say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx7ulp_wdt.
+
config UX500_WATCHDOG
tristate "ST-Ericsson Ux500 watchdog"
depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b5a0aed..2ee352b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
+obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
new file mode 100644
index 0000000..5ce5102
--- /dev/null
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define WDOG_CS 0x0
+#define WDOG_CS_CMD32EN BIT(13)
+#define WDOG_CS_ULK BIT(11)
+#define WDOG_CS_RCS BIT(10)
+#define WDOG_CS_EN BIT(7)
+#define WDOG_CS_UPDATE BIT(5)
+
+#define WDOG_CNT 0x4
+#define WDOG_TOVAL 0x8
+
+#define REFRESH_SEQ0 0xA602
+#define REFRESH_SEQ1 0xB480
+#define REFRESH ((REFRESH_SEQ1 << 16) | REFRESH_SEQ0)
+
+#define UNLOCK_SEQ0 0xC520
+#define UNLOCK_SEQ1 0xD928
+#define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
+
+#define DEFAULT_TIMEOUT 60
+#define MAX_TIMEOUT 128
+#define WDOG_CLOCK_RATE 1000
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct imx7ulp_wdt_device {
+ struct notifier_block restart_handler;
+ struct watchdog_device wdd;
+ void __iomem *base;
+ struct clk *clk;
+};
+
+static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ writel(UNLOCK, base + WDOG_CNT);
+ if (enable)
+ writel(val | WDOG_CS_EN, base + WDOG_CS);
+ else
+ writel(val & ~WDOG_CS_EN, base + WDOG_CS);
+}
+
+static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ return val & WDOG_CS_EN;
+}
+
+static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ writel(REFRESH, wdt->base + WDOG_CNT);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_start(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, true);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, false);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
+ unsigned int timeout)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+ u32 val = WDOG_CLOCK_RATE * timeout;
+
+ writel(UNLOCK, wdt->base + WDOG_CNT);
+ writel(val, wdt->base + WDOG_TOVAL);
+
+ wdog->timeout = timeout;
+
+ return 0;
+}
+
+static const struct watchdog_ops imx7ulp_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = imx7ulp_wdt_start,
+ .stop = imx7ulp_wdt_stop,
+ .ping = imx7ulp_wdt_ping,
+ .set_timeout = imx7ulp_wdt_set_timeout,
+};
+
+static const struct watchdog_info imx7ulp_wdt_info = {
+ .identity = "i.MX7ULP watchdog timer",
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE,
+};
+
+static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+{
+ u32 val;
+
+ /* unlock the wdog for reconfiguration */
+ writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
+ writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+
+ /* set an initial timeout value in TOVAL */
+ writel(timeout, base + WDOG_TOVAL);
+ /* enable 32bit command sequence and reconfigure */
+ val = BIT(13) | BIT(8) | BIT(5);
+ writel(val, base + WDOG_CS);
+}
+
+static void imx7ulp_wdt_action(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int imx7ulp_wdt_probe(struct platform_device *pdev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt;
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdog;
+ int ret;
+
+ imx7ulp_wdt = devm_kzalloc(dev, sizeof(*imx7ulp_wdt), GFP_KERNEL);
+ if (!imx7ulp_wdt)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, imx7ulp_wdt);
+
+ imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(imx7ulp_wdt->base))
+ return PTR_ERR(imx7ulp_wdt->base);
+
+ imx7ulp_wdt->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(imx7ulp_wdt->clk)) {
+ dev_err(dev, "Failed to get watchdog clock\n");
+ return PTR_ERR(imx7ulp_wdt->clk);
+ }
+
+ ret = clk_prepare_enable(imx7ulp_wdt->clk);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, imx7ulp_wdt_action, imx7ulp_wdt->clk);
+ if (ret)
+ return ret;
+
+ wdog = &imx7ulp_wdt->wdd;
+ wdog->info = &imx7ulp_wdt_info;
+ wdog->ops = &imx7ulp_wdt_ops;
+ wdog->min_timeout = 1;
+ wdog->max_timeout = MAX_TIMEOUT;
+ wdog->parent = dev;
+ wdog->timeout = DEFAULT_TIMEOUT;
+
+ watchdog_init_timeout(wdog, 0, dev);
+ watchdog_stop_on_reboot(wdog);
+ watchdog_stop_on_unregister(wdog);
+ watchdog_set_drvdata(wdog, imx7ulp_wdt);
+ imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout * WDOG_CLOCK_RATE);
+
+ return devm_watchdog_register_device(dev, wdog);
+}
+
+static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
+
+ clk_disable_unprepare(imx7ulp_wdt->clk);
+
+ return 0;
+}
+
+static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+ u32 timeout = imx7ulp_wdt->wdd.timeout * WDOG_CLOCK_RATE;
+ int ret;
+
+ ret = clk_prepare_enable(imx7ulp_wdt->clk);
+ if (ret)
+ return ret;
+
+ if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
+ imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
+ imx7ulp_wdt_resume);
+
+static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
+ { .compatible = "fsl,imx7ulp-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
+
+static struct platform_driver imx7ulp_wdt_driver = {
+ .probe = imx7ulp_wdt_probe,
+ .driver = {
+ .name = "imx7ulp-wdt",
+ .pm = &imx7ulp_wdt_pm_ops,
+ .of_match_table = imx7ulp_wdt_dt_ids,
+ },
+};
+module_platform_driver(imx7ulp_wdt_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH V4 3/4] ARM: imx_v6_v7_defconfig: Enable CONFIG_IMX7ULP_WDT by default
From: Anson Huang @ 2019-08-22 2:37 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, schnitzeltony,
u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1566441463-11911-1-git-send-email-Anson.Huang@nxp.com>
Select CONFIG_IMX7ULP_WDT by default to support i.MX7ULP watchdog.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
arch/arm/configs/imx_v6_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 9bfffbe..be2a694 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -236,6 +236,7 @@ CONFIG_DA9062_WATCHDOG=y
CONFIG_DA9063_WATCHDOG=m
CONFIG_RN5T618_WATCHDOG=y
CONFIG_IMX2_WDT=y
+CONFIG_IMX7ULP_WDT=y
CONFIG_MFD_DA9052_I2C=y
CONFIG_MFD_DA9062=y
CONFIG_MFD_DA9063=y
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* RE: [PATCH V3 2/4] watchdog: Add i.MX7ULP watchdog support
From: Anson Huang @ 2019-08-22 2:44 UTC (permalink / raw)
To: Guenter Roeck
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, Leonard Crestez,
schnitzeltony@gmail.com, linux-watchdog@vger.kernel.org,
otavio@ossystems.com.br, festevam@gmail.com,
s.hauer@pengutronix.de, jan.tuerk@emtrion.com,
linux@armlinux.org.uk, linux-kernel@vger.kernel.org,
robh+dt@kernel.org, dl-linux-imx, kernel@pengutronix.de,
u.kleine-koenig@pengutronix.de, wim@linux-watchdog.org,
shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190821141355.GA10463@roeck-us.net>
Hi, Guenter
> On Tue, Aug 20, 2019 at 10:07:56PM -0400, Anson Huang wrote:
> > The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
> that
> > is available for system use.
> > It provides a safety feature to ensure that software is executing as
> > planned and that the CPU is not stuck in an infinite loop or executing
> > unintended code. If the WDOG module is not serviced
> > (refreshed) within a certain period, it resets the MCU.
> >
> > Add driver support for i.MX7ULP watchdog.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V2:
> > - add devm_add_action_or_reset to disable clk for remove action.
> > ---
> > drivers/watchdog/Kconfig | 13 +++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/imx7ulp_wdt.c | 246
> > +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 260 insertions(+)
> > create mode 100644 drivers/watchdog/imx7ulp_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > a8f5c81..d68e5b5 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -724,6 +724,19 @@ config IMX_SC_WDT
> > To compile this driver as a module, choose M here: the
> > module will be called imx_sc_wdt.
> >
> > +config IMX7ULP_WDT
> > + tristate "IMX7ULP Watchdog"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + select WATCHDOG_CORE
> > + help
> > + This is the driver for the hardware watchdog on the Freescale
> > + IMX7ULP and later processors. If you have one of these
> > + processors and wish to have watchdog support enabled,
> > + say Y, otherwise say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called imx7ulp_wdt.
> > +
> > config UX500_WATCHDOG
> > tristate "ST-Ericsson Ux500 watchdog"
> > depends on MFD_DB8500_PRCMU
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index b5a0aed..2ee352b 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -67,6 +67,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> > obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> > obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> > obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> > +obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
> > obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> > obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o diff --git
> > a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> new
> > file mode 100644 index 0000000..5d37957
> > --- /dev/null
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDOG_CS 0x0
> > +#define WDOG_CS_CMD32EN BIT(13)
> > +#define WDOG_CS_ULK BIT(11)
> > +#define WDOG_CS_RCS BIT(10)
> > +#define WDOG_CS_EN BIT(7)
> > +#define WDOG_CS_UPDATE BIT(5)
> > +
> > +#define WDOG_CNT 0x4
> > +#define WDOG_TOVAL 0x8
> > +
> > +#define REFRESH_SEQ0 0xA602
> > +#define REFRESH_SEQ1 0xB480
> > +#define REFRESH ((REFRESH_SEQ1 << 16) | REFRESH_SEQ0)
> > +
> > +#define UNLOCK_SEQ0 0xC520
> > +#define UNLOCK_SEQ1 0xD928
> > +#define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
> > +
> > +#define DEFAULT_TIMEOUT 60
> > +#define MAX_TIMEOUT 128
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > +bool, 0000); MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped
> > +once started (default="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +struct imx7ulp_wdt_device {
> > + struct notifier_block restart_handler;
> > + struct watchdog_device wdd;
> > + void __iomem *base;
> > + struct clk *clk;
> > + int rate;
> > +};
> > +
> > +static inline void imx7ulp_wdt_enable(void __iomem *base, bool
> > +enable) {
> > + u32 val = readl(base + WDOG_CS);
> > +
> > + writel(UNLOCK, base + WDOG_CNT);
> > + if (enable)
> > + writel(val | WDOG_CS_EN, base + WDOG_CS);
> > + else
> > + writel(val & ~WDOG_CS_EN, base + WDOG_CS); }
> > +
> > +static inline bool imx7ulp_wdt_is_enabled(void __iomem *base) {
> > + u32 val = readl(base + WDOG_CS);
> > +
> > + return val & WDOG_CS_EN;
> > +}
> > +
> > +static int imx7ulp_wdt_ping(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + writel(REFRESH, wdt->base + WDOG_CNT);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_start(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + imx7ulp_wdt_enable(wdt->base, true);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_stop(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + imx7ulp_wdt_enable(wdt->base, false);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> > + unsigned int timeout)
> > +{
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > + u32 val = wdt->rate * timeout;
> > +
> > + writel(UNLOCK, wdt->base + WDOG_CNT);
> > + writel(val, wdt->base + WDOG_TOVAL);
> > +
> > + wdog->timeout = timeout;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct watchdog_ops imx7ulp_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = imx7ulp_wdt_start,
> > + .stop = imx7ulp_wdt_stop,
> > + .ping = imx7ulp_wdt_ping,
> > + .set_timeout = imx7ulp_wdt_set_timeout, };
> > +
> > +static const struct watchdog_info imx7ulp_wdt_info = {
> > + .identity = "i.MX7ULP watchdog timer",
> > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > + WDIOF_MAGICCLOSE,
> > +};
> > +
> > +static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int
> > +timeout) {
> > + u32 val;
> > +
> > + /* unlock the wdog for reconfiguration */
> > + writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > + writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > +
> > + /* set an initial timeout value in TOVAL */
> > + writel(timeout, base + WDOG_TOVAL);
> > + /* enable 32bit command sequence and reconfigure */
> > + val = BIT(13) | BIT(8) | BIT(5);
> > + writel(val, base + WDOG_CS);
> > +}
> > +
> > +static void imx7ulp_wdt_action(void *data) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt = data;
> > +
> > + clk_disable_unprepare(imx7ulp_wdt->clk);
>
> If you had passed imx7ulp_wdt->clk as parameter, the dereference here
> would not be necessary.
Yup, I thought we may need other data later, so I just passed whole structure.
I agree with your idea, for now, we ONLY need the clk, so I will ONLY pass the clk
to save the dereference operation. Will send out V4.
>
> > +}
> > +
> > +static int imx7ulp_wdt_probe(struct platform_device *pdev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt;
> > + struct device *dev = &pdev->dev;
> > + struct watchdog_device *wdog;
> > + int ret;
> > +
> > + imx7ulp_wdt = devm_kzalloc(dev, sizeof(*imx7ulp_wdt),
> GFP_KERNEL);
> > + if (!imx7ulp_wdt)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, imx7ulp_wdt);
> > +
> > + imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(imx7ulp_wdt->base))
> > + return PTR_ERR(imx7ulp_wdt->base);
> > +
> > + imx7ulp_wdt->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(imx7ulp_wdt->clk)) {
> > + dev_err(dev, "Failed to get watchdog clock\n");
> > + return PTR_ERR(imx7ulp_wdt->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(imx7ulp_wdt->clk);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(dev, imx7ulp_wdt_action,
> imx7ulp_wdt);
> > + if (ret)
> > + return ret;
> > +
> > + imx7ulp_wdt->rate = 1000;
>
> I forgot to ask (sorry, I had noticed, but I guess I forgot).
>
> Why not clk_get_rate() ? If the clock rate is fixed, why bother with a 'rate'
> variable ? You could use a constant instead whereever it is used.
OK, the clock rate is fixed on 7ULP, I will use a constant instead of the variable.
Thanks,
Anson
>
> Thanks
> Guenter
>
> > + wdog = &imx7ulp_wdt->wdd;
> > + wdog->info = &imx7ulp_wdt_info;
> > + wdog->ops = &imx7ulp_wdt_ops;
> > + wdog->min_timeout = 1;
> > + wdog->max_timeout = MAX_TIMEOUT;
> > + wdog->parent = dev;
> > + wdog->timeout = DEFAULT_TIMEOUT;
> > +
> > + watchdog_init_timeout(wdog, 0, dev);
> > + watchdog_stop_on_reboot(wdog);
> > + watchdog_stop_on_unregister(wdog);
> > + watchdog_set_drvdata(wdog, imx7ulp_wdt);
> > + imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout *
> > +imx7ulp_wdt->rate);
> > +
> > + return devm_watchdog_register_device(dev, wdog); }
> > +
> > +static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(&imx7ulp_wdt->wdd))
> > + imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
> > +
> > + clk_disable_unprepare(imx7ulp_wdt->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused imx7ulp_wdt_resume(struct device *dev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > + u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(imx7ulp_wdt->clk);
> > + if (ret)
> > + return ret;
> > +
> > + if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
> > + imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
> > +
> > + if (watchdog_active(&imx7ulp_wdt->wdd))
> > + imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops,
> imx7ulp_wdt_suspend,
> > + imx7ulp_wdt_resume);
> > +
> > +static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> > + { .compatible = "fsl,imx7ulp-wdt", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
> > +
> > +static struct platform_driver imx7ulp_wdt_driver = {
> > + .probe = imx7ulp_wdt_probe,
> > + .driver = {
> > + .name = "imx7ulp-wdt",
> > + .pm = &imx7ulp_wdt_pm_ops,
> > + .of_match_table = imx7ulp_wdt_dt_ids,
> > + },
> > +};
> > +module_platform_driver(imx7ulp_wdt_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Nick Desaulniers @ 2019-08-22 2:33 UTC (permalink / raw)
To: Nathan Huckleberry, Catalin Marinas
Cc: Tri Vo, Arnd Bergmann, Ard Biesheuvel, Russell King, LKML,
clang-built-linux, Miles Chen (陳民樺),
Linux ARM
In-Reply-To: <CAJkfWY4cHz+i8kYg2i1Krs-32nh7-WQU+psT=DRGYnTje6yj4Q@mail.gmail.com>
On Wed, Aug 21, 2019 at 10:43 AM Nathan Huckleberry <nhuck@google.com> wrote:
>
> On Tue, Aug 20, 2019 at 2:39 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 12:44 PM Nathan Huckleberry <nhuck@google.com> wrote:
...snip...
> > > +tst r1, #0x10 @ 26 or 32-bit mode?
> > > +moveq mask, #0xfc000003
> >
> > Should we be using different masks for ARM vs THUMB as per the
> > reference implementation?
> The change that introduces the arm/thumb code looked like a script
> that was run over all arm in the kernel. Neither this code nor the
> reference solution is compatible with arm, so there's no need for the
> change.
Looks like you're referring to commit 8b592783a2e8 ("Thumb-2:
Implement the unified arch/arm/lib functions").
Currently, arch/arm/Kconfig.debug has:
57 config UNWINDER_FRAME_POINTER
58 bool "Frame pointer unwinder"
59 depends on !THUMB2_KERNEL && !CC_IS_CLANG
So it looks like UNWINDER_FRAME_POINTER and THUMB2_KERNEL are mutually
exclusive. Probably could send a patch cleaning that up. (ie.
removing the different masks; essentially removing the hunk from
arch/arm/lib/backtrace.S from 8b592783a2e8).
> > > +for_each_frame: tst frame, mask @ Check for address exceptions
> > > + bne no_frame
> > > +
> > > +/*
> > > + * sv_fp is the stack frame with the locals for the current considered
> > > + * function.
> > > + * sv_pc is the saved lr frame the frame above. This is a pointer to a
> > > + * code address within the current considered function, but
> > > + * it is not the function start. This value gets updated to be
> > > + * the function start later if it is possible.
> > > + */
> > > +1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > > +1002: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > The reference implementation applies the mask to sv_pc and sv_fp. I
> > assume we want to, too?
> The mask is already applied to both. See for_each_frame:
ah, under the finished_setup label.
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 04/14] media: rkisp1: add Rockchip MIPI Synopsys DPHY driver
From: Laurent Pinchart @ 2019-08-22 2:32 UTC (permalink / raw)
To: Helen Koike
Cc: devicetree, eddie.cai.linux, kernel, heiko, jacob2.chen,
jeffy.chen, zyc, linux-kernel, tfiga, linux-rockchip,
Sakari Ailus, sakari.ailus, zhengsq, hans.verkuil, mchehab,
ezequiel, linux-arm-kernel, linux-media
In-Reply-To: <28b05c0e-2f68-aecb-536a-c543bcd43de1@collabora.com>
Hi Helen,
On Wed, Aug 21, 2019 at 06:46:15PM -0300, Helen Koike wrote:
> On 8/15/19 2:54 PM, Laurent Pinchart wrote:
> > On Wed, Aug 07, 2019 at 10:37:55AM -0300, Helen Koike wrote:
> >> On 8/7/19 10:05 AM, Sakari Ailus wrote:
> >>> On Tue, Jul 30, 2019 at 03:42:46PM -0300, Helen Koike wrote:
> >>>> From: Jacob Chen <jacob2.chen@rock-chips.com>
> >>>>
> >>>> This commit adds a subdev driver for Rockchip MIPI Synopsys DPHY driver
> >>>>
> >>>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> >>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> >>>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> >>>> [migrate to phy framework]
> >>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>>> [update for upstream]
> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v8:
> >>>> - Remove boiler plate license text
> >>>>
> >>>> Changes in v7:
> >>>> - Migrate dphy specific code from
> >>>> drivers/media/platform/rockchip/isp1/mipi_dphy_sy.c
> >>>> to drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> - Drop support for rk3288
> >>>> - Drop support for dphy txrx
> >>>> - code styling and checkpatch fixes
> >>>>
> >>>> drivers/phy/rockchip/Kconfig | 8 +
> >>>> drivers/phy/rockchip/Makefile | 1 +
> >>>> drivers/phy/rockchip/phy-rockchip-dphy.c | 408 +++++++++++++++++++++++
> >>>> 3 files changed, 417 insertions(+)
> >>>> create mode 100644 drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>>
> >>>> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> >>>> index c454c90cd99e..afd072f135e6 100644
> >>>> --- a/drivers/phy/rockchip/Kconfig
> >>>> +++ b/drivers/phy/rockchip/Kconfig
> >>>> @@ -9,6 +9,14 @@ config PHY_ROCKCHIP_DP
> >>>> help
> >>>> Enable this to support the Rockchip Display Port PHY.
> >>>>
> >>>> +config PHY_ROCKCHIP_DPHY
> >>>> + tristate "Rockchip MIPI Synopsys DPHY driver"
> >
> > How much of this PHY is Rockchip-specific ? Would it make sense to turn
> > it into a Synopsys DPHY driver, with some Rockchip glue ? I suppose this
> > could always be done later, if needed (and I also suppose there's no
> > existing driver in drivers/phy/ that support the same Synopsys IP).
> >
> >>>> + depends on ARCH_ROCKCHIP && OF
> >>>
> >>> How about (...) || COMPILE_TEST ?
> >>>
> >>>> + select GENERIC_PHY_MIPI_DPHY
> >>>> + select GENERIC_PHY
> >>>> + help
> >>>> + Enable this to support the Rockchip MIPI Synopsys DPHY.
> >>>> +
> >>>> config PHY_ROCKCHIP_EMMC
> >>>> tristate "Rockchip EMMC PHY Driver"
> >>>> depends on ARCH_ROCKCHIP && OF
> >>>> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> >>>> index fd21cbaf40dd..f62e9010bcaf 100644
> >>>> --- a/drivers/phy/rockchip/Makefile
> >>>> +++ b/drivers/phy/rockchip/Makefile
> >>>> @@ -1,5 +1,6 @@
> >>>> # SPDX-License-Identifier: GPL-2.0
> >>>> obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
> >>>> +obj-$(CONFIG_PHY_ROCKCHIP_DPHY) += phy-rockchip-dphy.o
> >>>> obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
> >>>> obj-$(CONFIG_PHY_ROCKCHIP_INNO_HDMI) += phy-rockchip-inno-hdmi.o
> >>>> obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
> >>>> diff --git a/drivers/phy/rockchip/phy-rockchip-dphy.c b/drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> new file mode 100644
> >>>> index 000000000000..3a29976c2dff
> >>>> --- /dev/null
> >>>> +++ b/drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> @@ -0,0 +1,408 @@
> >>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>>> +/*
> >>>> + * Rockchip MIPI Synopsys DPHY driver
> >>>> + *
> >>>> + * Based on:
> >>>> + *
> >>>> + * Copyright (C) 2016 FuZhou Rockchip Co., Ltd.
> >>>> + * Author: Yakir Yang <ykk@@rock-chips.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/clk.h>
> >>>> +#include <linux/io.h>
> >>>> +#include <linux/mfd/syscon.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_device.h>
> >>>> +#include <linux/phy/phy.h>
> >>>> +#include <linux/phy/phy-mipi-dphy.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +#include <linux/regmap.h>
> >>>> +
> >>>> +#define RK3399_GRF_SOC_CON9 0x6224
> >>>> +#define RK3399_GRF_SOC_CON21 0x6254
> >>>> +#define RK3399_GRF_SOC_CON22 0x6258
> >>>> +#define RK3399_GRF_SOC_CON23 0x625c
> >>>> +#define RK3399_GRF_SOC_CON24 0x6260
> >>>> +#define RK3399_GRF_SOC_CON25 0x6264
> >>>> +#define RK3399_GRF_SOC_STATUS1 0xe2a4
> >>>> +
> >>>> +#define CLOCK_LANE_HS_RX_CONTROL 0x34
> >>>> +#define LANE0_HS_RX_CONTROL 0x44
> >>>> +#define LANE1_HS_RX_CONTROL 0x54
> >>>> +#define LANE2_HS_RX_CONTROL 0x84
> >>>> +#define LANE3_HS_RX_CONTROL 0x94
> >>>> +#define HS_RX_DATA_LANES_THS_SETTLE_CONTROL 0x75
> >>>> +
> >>>> +#define MAX_DPHY_CLK 8
> >>>> +
> >>>> +#define PHY_TESTEN_ADDR (0x1 << 16)
> >>>> +#define PHY_TESTEN_DATA (0x0 << 16)
> >>>> +#define PHY_TESTCLK (0x1 << 1)
> >>>> +#define PHY_TESTCLR (0x1 << 0)
> >
> > Maybe s/0x// for the previous four lines ?
> >
> >>>> +#define THS_SETTLE_COUNTER_THRESHOLD 0x04
> >>>> +
> >>>> +#define HIWORD_UPDATE(val, mask, shift) \
> >>>> + ((val) << (shift) | (mask) << ((shift) + 16))
> >
> > As you use this in a single place, I would inline it, possibly with a
> > small comment that explains what's happening.
> >
> >>>> +
> >>>> +#define GRF_SOC_CON12 0x0274
> >>>> +
> >>>> +#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(20)
> >>>> +#define GRF_EDP_REF_CLK_SEL_INTER BIT(4)
> >>>> +
> >>>> +#define GRF_EDP_PHY_SIDDQ_HIWORD_MASK BIT(21)
> >>>> +#define GRF_EDP_PHY_SIDDQ_ON 0
> >>>> +#define GRF_EDP_PHY_SIDDQ_OFF BIT(5)
> >
> > I would recommend aligning the value of of all macros in the same way.
> >
> >>>> +
> >>>> +struct hsfreq_range {
> >>>> + u32 range_h;
> >
> > The structure would be more compact if you turned this into a u16.
> >
> >>>> + u8 cfg_bit;
> >>>> +};
> >>>> +
> >>>> +static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> >>>> + { 89, 0x00}, { 99, 0x10}, { 109, 0x20}, { 129, 0x01},
> >>>> + { 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
> >>>> + { 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
> >>>> + { 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
> >>>> + { 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
> >>>> + { 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
> >>>> + { 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
> >>>> + { 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
> >>>> + {1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
> >>>> + {1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
> >
> > Maybe s/{/{ / and s/}/ }/ to give it a bit more air ? :-)
> >
> >>>> +};
> >>>> +
> >>>> +static const char * const rk3399_mipidphy_clks[] = {
> >>>> + "dphy-ref",
> >>>> + "dphy-cfg",
> >>>> + "grf",
> >>>> +};
> >>>> +
> >>>> +enum dphy_reg_id {
> >>>> + GRF_DPHY_RX0_TURNDISABLE = 0,
> >>>> + GRF_DPHY_RX0_FORCERXMODE,
> >>>> + GRF_DPHY_RX0_FORCETXSTOPMODE,
> >>>> + GRF_DPHY_RX0_ENABLE,
> >>>> + GRF_DPHY_RX0_TESTCLR,
> >>>> + GRF_DPHY_RX0_TESTCLK,
> >>>> + GRF_DPHY_RX0_TESTEN,
> >>>> + GRF_DPHY_RX0_TESTDIN,
> >>>> + GRF_DPHY_RX0_TURNREQUEST,
> >>>> + GRF_DPHY_RX0_TESTDOUT,
> >>>> + GRF_DPHY_TX0_TURNDISABLE,
> >>>> + GRF_DPHY_TX0_FORCERXMODE,
> >>>> + GRF_DPHY_TX0_FORCETXSTOPMODE,
> >>>> + GRF_DPHY_TX0_TURNREQUEST,
> >>>> + GRF_DPHY_TX1RX1_TURNDISABLE,
> >>>> + GRF_DPHY_TX1RX1_FORCERXMODE,
> >>>> + GRF_DPHY_TX1RX1_FORCETXSTOPMODE,
> >>>> + GRF_DPHY_TX1RX1_ENABLE,
> >>>> + GRF_DPHY_TX1RX1_MASTERSLAVEZ,
> >>>> + GRF_DPHY_TX1RX1_BASEDIR,
> >>>> + GRF_DPHY_TX1RX1_ENABLECLK,
> >>>> + GRF_DPHY_TX1RX1_TURNREQUEST,
> >>>> + GRF_DPHY_RX1_SRC_SEL,
> >>>> + /* rk3288 only */
> >>>> + GRF_CON_DISABLE_ISP,
> >>>> + GRF_CON_ISP_DPHY_SEL,
> >>>> + GRF_DSI_CSI_TESTBUS_SEL,
> >>>> + GRF_DVP_V18SEL,
> >>>> + /* below is for rk3399 only */
> >>>> + GRF_DPHY_RX0_CLK_INV_SEL,
> >>>> + GRF_DPHY_RX1_CLK_INV_SEL,
> >>>> +};
> >>>> +
> >>>> +struct dphy_reg {
> >>>> + u32 offset;
> >>>> + u32 mask;
> >>>> + u32 shift;
> >
> > The offset should hold in 16 bits and the mask and shift in 8 bits. That
> > would save space in the table below.
> >
> >>>> +};
> >>>> +
> >>>> +#define PHY_REG(_offset, _width, _shift) \
> >>>> + { .offset = _offset, .mask = BIT(_width) - 1, .shift = _shift, }
> >>>> +
> >>>> +static const struct dphy_reg rk3399_grf_dphy_regs[] = {
> >>>> + [GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON9, 4, 0),
> >>>> + [GRF_DPHY_RX0_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 10),
> >>>> + [GRF_DPHY_RX1_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 11),
> >>>> + [GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 0),
> >>>> + [GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 4),
> >>>> + [GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 8),
> >>>> + [GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 12),
> >>>> + [GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 0),
> >>>> + [GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 4),
> >>>> + [GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 8),
> >>>> + [GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 12),
> >>>> + [GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 0),
> >>>> + [GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 4),
> >>>> + [GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 8),
> >>>> + [GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 12),
> >>>> + [GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON24, 4, 0),
> >>>> + [GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 4),
> >>>> + [GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 5),
> >>>> + [GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 6),
> >>>> + [GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 7),
> >>>> + [GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3399_GRF_SOC_CON25, 8, 0),
> >>>> + [GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 8),
> >>>> + [GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 9),
> >>>> + [GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 10),
> >>>> + [GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3399_GRF_SOC_STATUS1, 8, 0),
> >
> > The annoying part with such an indirection is that you can't really
> > write multiple fields in a single register with a single operation.
> > Is the register mapping completely different between the rk3288 and the
> > rk3399, or are the fields grouped in registers in a similar way ? In the
> > latter case we could possibly optimise it.
>
> This would be the rk3288 version:
>
> +static const struct dphy_reg rk3288_grf_dphy_regs[] = {
> + [GRF_CON_DISABLE_ISP] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 0),
> + [GRF_CON_ISP_DPHY_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 1),
> + [GRF_DSI_CSI_TESTBUS_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 14),
> + [GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 0),
> + [GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 4),
> + [GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 8),
> + [GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 0),
> + [GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 4),
> + [GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 8),
> + [GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 12),
> + [GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 0),
> + [GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 4),
> + [GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 8),
> + [GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 12),
> + [GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 0),
> + [GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 1),
> + [GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 2),
> + [GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3288_GRF_SOC_CON14, 8, 3),
> + [GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 12),
> + [GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 13),
> + [GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 14),
> + [GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 15),
> + [GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 0),
> + [GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 4),
> + [GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 3, 8),
> + [GRF_DVP_V18SEL] = PHY_REG(RK3288_GRF_IO_VSEL, 1, 1),
> + [GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3288_GRF_SOC_STATUS21, 8, 0),
> +};
>
> Which seems different mask and shifts from rk3399. If you have any ideas in
> how to optimize this I would appreciate it.
It would be tricky indeed :-( Nevermind for now.
> >>>> +};
> >>>> +
> >>>> +struct dphy_drv_data {
> >>>> + const char * const *clks;
> >>>> + int num_clks;
> >
> > This is never negative, you can make it an unsigned int.
> >
> >>>> + const struct hsfreq_range *hsfreq_ranges;
> >>>> + int num_hsfreq_ranges;
> >
> > Same here.
> >
> >>>> + const struct dphy_reg *regs;
> >>>> +};
> >>>> +
> >>>> +struct rockchip_dphy {
> >>>> + struct device *dev;
> >>>> + struct regmap *grf;
> >>>> + const struct dphy_reg *grf_regs;
> >>>> + struct clk_bulk_data clks[MAX_DPHY_CLK];
> >>>> +
> >>>> + const struct dphy_drv_data *drv_data;
> >>>> + struct phy_configure_opts_mipi_dphy config;
> >>>> +};
> >>>> +
> >>>> +static inline void write_grf_reg(struct rockchip_dphy *priv,
> >>>> + int index, u8 value)
> >
> > Maybe unsigned int index ?
> >
> >>>> +{
> >>>> + const struct dphy_reg *reg = &priv->grf_regs[index];
> >>>> + unsigned int val = HIWORD_UPDATE(value, reg->mask, reg->shift);
> >>>> +
> >>>> + WARN_ON(!reg->offset);
> >>>> + regmap_write(priv->grf, reg->offset, val);
> >>>> +}
> >>>> +
> >>>> +static void mipidphy0_wr_reg(struct rockchip_dphy *priv,
> >>>> + u8 test_code, u8 test_data)
> >
> > Function (and structure) names have different prefixes, would it make
> > sense to standardise them ? Maybe rockchip_dphy_ ? Or rk_dphy_ for a
> > shorter version ? This could become rk_dphy_write_dphy(), and the
> > previous function rk_dphy_write_grf().
> >
> >>>> +{
> >>>> + /*
> >>>> + * With the falling edge on TESTCLK, the TESTDIN[7:0] signal content
> >>>> + * is latched internally as the current test code. Test data is
> >>>> + * programmed internally by rising edge on TESTCLK.
> >>>> + */
> >
> > I've never understood why PHYs tend to have a register named TEST that
> > contains way more than test data :-)
> >
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTDIN, test_code);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTEN, 1);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 0);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTEN, 0);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTDIN, test_data);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> +}
> >>>> +
> >>>> +/* should be move to power_on */
> >
> > s/move/moved/
> >
> > Do you mean merging the two functions together ? What prevents from
> > doing so ?
>
> Nothing really, this is a left over command as mipidphy_rx_stream_on() is already
> being called from power_on, and I don't think we should merge it because
> in the future we'll probably going to have mipidphy_txrx_stream_on() for dphy1.
Fine with me, let's just remove the comment then.
> >>>> +static int mipidphy_rx_stream_on(struct rockchip_dphy *priv)
> >>>> +{
> >>>> + const struct dphy_drv_data *drv_data = priv->drv_data;
> >>>> + const struct hsfreq_range *hsfreq_ranges = drv_data->hsfreq_ranges;
> >>>> + struct phy_configure_opts_mipi_dphy *config = &priv->config;
> >>>> + unsigned int i, hsfreq = 0, data_rate_mbps = config->hs_clk_rate;
> >>>> + int num_hsfreq_ranges = drv_data->num_hsfreq_ranges;
> >>>> +
> >>>> + do_div(data_rate_mbps, 1000 * 1000);
> >>>> +
> >>>> + dev_dbg(priv->dev, "%s: lanes %d - data_rate_mbps %u\n",
> >>>> + __func__, config->lanes, data_rate_mbps);
> >>>> +
> >>>> + for (i = 0; i < num_hsfreq_ranges; i++) {
> >>>> + if (hsfreq_ranges[i].range_h >= data_rate_mbps) {
> >>>> + hsfreq = hsfreq_ranges[i].cfg_bit;
> >>>> + break;
> >>>> + }
> >>>> + }
> >
> > As num_hsfreq_ranges and hsfreq_ranges are only used in this loop, I
> > would remove the local variables.
> >
> >>>> +
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_FORCERXMODE, 0);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_FORCETXSTOPMODE, 0);
> >>>> +
> >>>> + /* Disable lan turn around, which is ignored in receive mode */
> >
> > Is it "lan turn around", or "lane turn around" ?
> >
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TURNREQUEST, 0);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TURNDISABLE, 0xf);
> >>>> +
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_ENABLE, GENMASK(config->lanes - 1, 0));
> >>>> +
> >>>> + /* dphy start */
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLR, 1);
> >>>> + usleep_range(100, 150);
> >>>> + write_grf_reg(priv, GRF_DPHY_RX0_TESTCLR, 0);
> >>>> + usleep_range(100, 150);
> >>>> +
> >>>> + /* set clock lane */
> >>>> + /* HS hsfreq_range & lane 0 settle bypass */
> >>>> + mipidphy0_wr_reg(priv, CLOCK_LANE_HS_RX_CONTROL, 0);
> >>>> + /* HS RX Control of lane0 */
> >>>> + mipidphy0_wr_reg(priv, LANE0_HS_RX_CONTROL, hsfreq << 1);
> >>>> + /* HS RX Control of lane1 */
> >>>> + mipidphy0_wr_reg(priv, LANE1_HS_RX_CONTROL, 0);
> >>>> + /* HS RX Control of lane2 */
> >>>> + mipidphy0_wr_reg(priv, LANE2_HS_RX_CONTROL, 0);
> >>>> + /* HS RX Control of lane3 */
> >>>> + mipidphy0_wr_reg(priv, LANE3_HS_RX_CONTROL, 0);
> >
> > Does this hardcode usage of a single lane ?
>
> Rockchip seems to uses TEST* registers to set the hsfreqrange.
> It mentions the test code 0x44 (which is LANE0_HS_RX_CONTROL)
> but it doesn't mention the others lanes.
>
> Replacing those call by
> mipidphy0_wr_reg(priv, LANEx_HS_RX_CONTROL, hsfreq << 1);
> seems to be working.
>
> I can check if this changes the datarate (I just need to figure a proper
> way to test this or get some docs).
>
> Thanks for spotting this.
We've discussed this on IRC, it's not clear if the above code is
incorrect or not. Let's add this to a list of open issues.
> >>>> + /* HS RX Data Lanes Settle State Time Control */
> >>>> + mipidphy0_wr_reg(priv, HS_RX_DATA_LANES_THS_SETTLE_CONTROL,
> >>>> + THS_SETTLE_COUNTER_THRESHOLD);
> >>>> +
> >>>> + /* Normal operation */
> >>>> + mipidphy0_wr_reg(priv, 0x0, 0);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> >>>> +{
> >>>> + struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> + int ret;
> >>>> +
> >>>> + /* pass with phy_mipi_dphy_get_default_config (with pixel rate?) */
> >
> > I'm not sure to understand what this means.
>
> iirc, the question is if we should fail when phy_mipi_dphy_config_validate() fails,
> or if we should use a default config.
>
> Looking at other examples, is seems that only two drivers call
> phy_mipi_dphy_get_default_config() in a totally diferent context, not in mipi path.
> So I guess I would just remove this comment if this is ok with you.
OK.
> >>>> + ret = phy_mipi_dphy_config_validate(&opts->mipi_dphy);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + memcpy(&priv->config, opts, sizeof(priv->config));
> >>>
> >>> You could to:
> >>>
> >>> priv->config = *opts;
> >>>
> >>> Up to you. Some people like memcpy(). :-)
> >>
> >> your way is better thanks!
> >>
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_power_on(struct phy *phy)
> >>>> +{
> >>>> + struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> + int ret;
> >>>> +
> >>>> + ret = clk_bulk_enable(priv->drv_data->num_clks, priv->clks);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + return mipidphy_rx_stream_on(priv);
> >
> > Should you call clk_bulk_disable() if mipidphy_rx_stream_on() fails ?
> > Actually that function never fails, so I'd make it a void function, and
> > return 0 here.
>
> Ack, I made it void, I'll send it in the next version.
>
> > What happens if the clock rate is higher than the maximum supported by
> > the PHY ? Shouldn't rockchip_dphy_configure() fail in that case ?
>
> This is checked in function mipidphy_rx_stream_on(), if it is higher we just
> configure the maximum supported rate. Is this ok?
I think it would be better to reject that in rockchip_dphy_configure()
in order to let the user of the PHY handle the error as early as
possible.
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_power_off(struct phy *phy)
> >>>> +{
> >>>> + struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +
> >
> > No need to write any register ? That's scary, what will happen on the
> > next power on, when the clocks gets enabled ?
>
> Just for testing, I hacked the code to only call mipidphy_rx_stream_on() once,
> when streaming for the first time, then I don't call it anymore and starting/stopping
> streaming always works, so I guess it keeps the previous configuration when clocks
> get enabled.
> I wonder if this can be a problem when switching from dphy rx to txrx, but for now
> we just support rx.
>
> Maybe just calling rk_dphy_write_grf(priv, GRF_DPHY_RX0_ENABLE, 0) is enough.
If that works with disable/enable sequences I think it would be good to
include it.
> >>>> + clk_bulk_disable(priv->drv_data->num_clks, priv->clks);
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_init(struct phy *phy)
> >>>> +{
> >>>> + struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> + int ret;
> >>>> +
> >>>> + ret = clk_bulk_prepare(priv->drv_data->num_clks, priv->clks);
> >>>
> >>> return ...;
> >>>
> >>>> + if (ret)
> >>>> + return ret;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_exit(struct phy *phy)
> >>>> +{
> >>>> + struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +
> >>>> + clk_bulk_unprepare(priv->drv_data->num_clks, priv->clks);
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static const struct phy_ops rockchip_dphy_ops = {
> >>>> + .power_on = rockchip_dphy_power_on,
> >>>> + .power_off = rockchip_dphy_power_off,
> >>>> + .init = rockchip_dphy_init,
> >>>> + .exit = rockchip_dphy_exit,
> >>>> + .configure = rockchip_dphy_configure,
> >>>> + .owner = THIS_MODULE,
> >>>> +};
> >>>> +
> >>>> +static const struct dphy_drv_data rk3399_mipidphy_drv_data = {
> >>>> + .clks = rk3399_mipidphy_clks,
> >>>> + .num_clks = ARRAY_SIZE(rk3399_mipidphy_clks),
> >>>> + .hsfreq_ranges = rk3399_mipidphy_hsfreq_ranges,
> >>>> + .num_hsfreq_ranges = ARRAY_SIZE(rk3399_mipidphy_hsfreq_ranges),
> >>>> + .regs = rk3399_grf_dphy_regs,
> >>>
> >>> Do you expect to support more of the similar PHY(s) --- are there such? If
> >>> not, you could put these in the code that uses them.
> >>
> >> Yes, for rk3288 in the future.
> >>
> >>>> +};
> >>>> +
> >>>> +static const struct of_device_id rockchip_dphy_dt_ids[] = {
> >>>> + {
> >>>> + .compatible = "rockchip,rk3399-mipi-dphy",
> >>>> + .data = &rk3399_mipidphy_drv_data,
> >>>> + },
> >>>> + {}
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(of, rockchip_dphy_dt_ids);
> >>>> +
> >>>> +static int rockchip_dphy_probe(struct platform_device *pdev)
> >>>> +{
> >>>> + struct device *dev = &pdev->dev;
> >>>> + struct device_node *np = dev->of_node;
> >>>> + const struct dphy_drv_data *drv_data;
> >>>> + struct phy_provider *phy_provider;
> >>>> + const struct of_device_id *of_id;
> >>>> + struct rockchip_dphy *priv;
> >>>> + struct regmap *grf;
> >>>> + struct phy *phy;
> >>>> + unsigned int i;
> >>>> + int ret;
> >>>> +
> >>>> + if (!dev->parent || !dev->parent->of_node)
> >>>> + return -ENODEV;
> >>>> +
> >>>> + if (platform_get_resource(pdev, IORESOURCE_MEM, 0)) {
> >>>> + dev_err(&pdev->dev, "Rockchip DPHY driver only suports rx\n");
> >
> > You can replace pdev->dev with dev here and below.
> >
> > s/rx/RX mode/ ?
> >
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>>> + if (!priv)
> >>>> + return -ENOMEM;
> >>>> + priv->dev = dev;
> >>>> +
> >>>> + grf = syscon_node_to_regmap(dev->parent->of_node);
> >>>> + if (IS_ERR(grf)) {
> >>>> + grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>>> + "rockchip,grf");
> >>>> + if (IS_ERR(grf)) {
> >>>> + dev_err(dev, "Can't find GRF syscon\n");
> >>>> + return -ENODEV;
> >>>> + }
> >>>> + }
> >>>> + priv->grf = grf;
> >>>> +
> >>>> + of_id = of_match_device(rockchip_dphy_dt_ids, dev);
> >>>> + if (!of_id)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + drv_data = of_id->data;
> >>>> + priv->grf_regs = drv_data->regs;
> >
> > Do you have to store grf_regs in priv, or could it be accessed through
> > priv->drv_data->regs ?
> >
> >>>> + priv->drv_data = drv_data;
> >>>> + for (i = 0; i < drv_data->num_clks; i++)
> >>>> + priv->clks[i].id = drv_data->clks[i];
> >>>> + ret = devm_clk_bulk_get(&pdev->dev, drv_data->num_clks, priv->clks);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + phy = devm_phy_create(dev, np, &rockchip_dphy_ops);
> >>>> + if (IS_ERR(phy)) {
> >>>> + dev_err(dev, "failed to create phy\n");
> >>>> + return PTR_ERR(phy);
> >>>> + }
> >>>> + phy_set_drvdata(phy, priv);
> >>>> +
> >>>> + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> >>>> +
> >>>> + return PTR_ERR_OR_ZERO(phy_provider);
> >>>> +}
> >>>> +
> >>>> +static struct platform_driver rockchip_dphy_driver = {
> >>>> + .probe = rockchip_dphy_probe,
> >>>> + .driver = {
> >>>> + .name = "rockchip-mipi-dphy",
> >>>> + .of_match_table = rockchip_dphy_dt_ids,
> >>>> + },
> >>>> +};
> >>>> +module_platform_driver(rockchip_dphy_driver);
> >>>> +
> >>>> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel@collabora.com>");
> >>>> +MODULE_DESCRIPTION("Rockchip MIPI Synopsys DPHY driver");
> >>>> +MODULE_LICENSE("Dual MIT/GPL");
> >
> > Overall this is quite good, there are only small issues.
>
> Thank you a lot for your review
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/7] dt-bindings: arm: cpus: Add ASPEED SMP
From: Andrew Jeffery @ 2019-08-22 2:08 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-2-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:25, Joel Stanley wrote:
> The AST2600 SoC contains two CPUs and requires the operating system to
> bring the second one out of firmware.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 7/7] ARM: configs: aspeed_g5: Enable AST2600
From: Andrew Jeffery @ 2019-08-22 2:07 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-8-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> CONFIG_STRICT_KERNEL_RWX is enabled by default with ARMv7.
>
> Turn on HIGHMEM as the EVB has 2GB of RAM, and not all is usable without
> hihgmem.
>
> The SoC contains Cortex A7 supporting VFP and has two CPUs.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> arch/arm/configs/aspeed_g5_defconfig | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/configs/aspeed_g5_defconfig
> b/arch/arm/configs/aspeed_g5_defconfig
> index 426d8e0c9890..597536cc9573 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -21,21 +21,26 @@ CONFIG_PERF_EVENTS=y
> CONFIG_SLAB=y
> CONFIG_SLAB_FREELIST_RANDOM=y
> CONFIG_ARCH_MULTI_V6=y
> -# CONFIG_ARCH_MULTI_V7 is not set
> CONFIG_ARCH_ASPEED=y
> CONFIG_MACH_ASPEED_G5=y
> +CONFIG_MACH_ASPEED_G6=y
> # CONFIG_CACHE_L2X0 is not set
> +CONFIG_SMP=y
> +# CONFIG_ARM_CPU_TOPOLOGY is not set
> CONFIG_VMSPLIT_2G=y
> +CONFIG_NR_CPUS=2
> +CONFIG_HIGHMEM=y
> CONFIG_UACCESS_WITH_MEMCPY=y
> CONFIG_SECCOMP=y
> # CONFIG_ATAGS is not set
> CONFIG_ZBOOT_ROM_TEXT=0x0
> CONFIG_ZBOOT_ROM_BSS=0x0
> CONFIG_KEXEC=y
> -# CONFIG_SUSPEND is not set
> +CONFIG_VFP=y
> +CONFIG_NEON=y
> +CONFIG_KERNEL_MODE_NEON=y
> CONFIG_FIRMWARE_MEMMAP=y
> CONFIG_JUMP_LABEL=y
> -CONFIG_STRICT_KERNEL_RWX=y
> # CONFIG_BLK_DEV_BSG is not set
> # CONFIG_BLK_DEBUG_FS is not set
> # CONFIG_MQ_IOSCHED_DEADLINE is not set
> @@ -140,10 +145,12 @@ CONFIG_ASPEED_BT_IPMI_BMC=y
> CONFIG_HW_RANDOM_TIMERIOMEM=y
> # CONFIG_I2C_COMPAT is not set
> CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_MUX=y
> CONFIG_I2C_MUX_PCA9541=y
> CONFIG_I2C_MUX_PCA954x=y
> CONFIG_I2C_ASPEED=y
> CONFIG_I2C_FSI=y
> +CONFIG_SPI=y
> CONFIG_GPIOLIB=y
> CONFIG_GPIO_SYSFS=y
> CONFIG_GPIO_ASPEED=y
> @@ -194,6 +201,10 @@ CONFIG_USB_CONFIGFS_F_LB_SS=y
> CONFIG_USB_CONFIGFS_F_FS=y
> CONFIG_USB_CONFIGFS_F_HID=y
> CONFIG_USB_CONFIGFS_F_PRINTER=y
> +CONFIG_MMC=y
> +CONFIG_MMC_SDHCI=y
> +CONFIG_MMC_SDHCI_PLTFM=y
> +CONFIG_MMC_SDHCI_OF_ASPEED=y
The patches haven't yet been applied to the MMC tree, maybe we should
add this later?
Anyway,
Acked-by: Andrew Jeffery <andrew@aj.id.au>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/7] ARM: configs: multi_v7: Add ASPEED G6
From: Andrew Jeffery @ 2019-08-22 2:04 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-7-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> This adds the ASPEED AST2600 system and associated ASPEED devices so we
> get build coverage.
>
> The changes to the UART configuration to ensure the default console
> (UART5) works.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 5/7] ARM: dts: aspeed: Add AST2600 and EVB
From: Andrew Jeffery @ 2019-08-22 1:58 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-6-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> The AST2600 is a new SoC by ASPEED. It contains a dual core Cortex A7
> CPU and shares many periperhals with the existing AST2400 and AST2500.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/aspeed-ast2600-evb.dts | 44 ++++
> arch/arm/boot/dts/aspeed-g6.dtsi | 266 +++++++++++++++++++++++
> 3 files changed, 311 insertions(+)
> create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb.dts
> create mode 100644 arch/arm/boot/dts/aspeed-g6.dtsi
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 247e556de48e..2d8d29e5686d 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1276,6 +1276,7 @@ dtb-$(CONFIG_ARCH_MILBEAUT) +=
> milbeaut-m10v-evb.dtb
> dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
> dtb-$(CONFIG_ARCH_ASPEED) += \
> aspeed-ast2500-evb.dtb \
> + aspeed-ast2600-evb.dtb \
> aspeed-bmc-arm-centriq2400-rep.dtb \
> aspeed-bmc-arm-stardragon4800-rep2.dtb \
> aspeed-bmc-facebook-cmm.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> new file mode 100644
> index 000000000000..7f2528e084b5
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +
> +/ {
> + model = "AST2600 EVB";
> + compatible = "aspeed,ast2600";
> +
> + aliases {
> + serial4 = &uart5;
> + };
> +
> + chosen {
> + bootargs = "console=ttyS4,115200n8";
> + };
> +
> + memory@80000000 {
> + device_type = "memory";
> + reg = <0x80000000 0x80000000>;
> + };
> +};
> +
> +&mdio1 {
> + status = "okay";
> +
> + ethphy1: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +
> +&mac1 {
> + status = "okay";
> +
> + phy-mode = "rgmii";
> + phy-handle = <ðphy1>;
> +};
> +
> +&emmc {
> + status = "okay";
> +};
> diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi
> b/arch/arm/boot/dts/aspeed-g6.dtsi
> new file mode 100644
> index 000000000000..9f9931541060
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-g6.dtsi
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2019 IBM Corp.
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/ast2600-clock.h>
> +
> +/ {
> + model = "Aspeed BMC";
> + compatible = "aspeed,ast2600";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + interrupt-parent = <&gic>;
> +
> + aliases {
> + serial4 = &uart5;
> + };
> +
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + enable-method = "aspeed,ast2600-smp";
> +
> + cpu@f00 {
> + compatible = "arm,cortex-a7";
> + device_type = "cpu";
> + reg = <0xf00>;
> + };
> +
> + cpu@f01 {
> + compatible = "arm,cortex-a7";
> + device_type = "cpu";
> + reg = <0xf01>;
> + };
> + };
> +
> + timer {
> + compatible = "arm,armv7-timer";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) |
> IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>;
> + clocks = <&syscon ASPEED_CLK_HPLL>;
> + arm,cpu-registers-not-fw-configured;
> + };
> +
> + ahb {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + device_type = "soc";
> + ranges;
> +
> + gic: interrupt-controller@40461000 {
> + compatible = "arm,cortex-a7-gic";
> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) |
> IRQ_TYPE_LEVEL_HIGH)>;
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + reg = <0x40461000 0x1000>,
> + <0x40462000 0x1000>,
> + <0x40464000 0x2000>,
> + <0x40466000 0x2000>;
> + };
> +
> + mdio0: mdio@1e650000 {
> + compatible = "aspeed,ast2600-mdio";
> + reg = <0x1e650000 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + mdio1: mdio@1e650008 {
> + compatible = "aspeed,ast2600-mdio";
> + reg = <0x1e650008 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + mdio2: mdio@1e650010 {
> + compatible = "aspeed,ast2600-mdio";
> + reg = <0x1e650010 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + mdio3: mdio@1e650018 {
> + compatible = "aspeed,ast2600-mdio";
> + reg = <0x1e650018 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + mac0: ftgmac@1e660000 {
> + compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> + reg = <0x1e660000 0x180>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>;
> + status = "disabled";
> + };
> +
> + mac1: ftgmac@1e680000 {
> + compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> + reg = <0x1e680000 0x180>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_MAC2CLK>;
> + status = "disabled";
> + };
> +
> + mac2: ftgmac@1e670000 {
> + compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> + reg = <0x1e670000 0x180>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_MAC3CLK>;
> + status = "disabled";
> + };
> +
> + mac3: ftgmac@1e690000 {
> + compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> + reg = <0x1e690000 0x180>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_MAC4CLK>;
> + status = "disabled";
> + };
> +
> + apb {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + syscon: syscon@1e6e2000 {
> + compatible = "aspeed,ast2600-scu", "syscon", "simple-mfd";
> + reg = <0x1e6e2000 0x1000>;
> + ranges = <0 0x1e6e2000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> +
> + pinctrl: pinctrl {
> + compatible = "aspeed,ast2600-pinctrl";
> + };
> +
> + smp-memram@180 {
> + compatible = "aspeed,ast2600-smpmem";
> + reg = <0x180 0x40>;
> + };
> + };
> +
> + rng: hwrng@1e6e2524 {
> + compatible = "timeriomem_rng";
> + reg = <0x1e6e2524 0x4>;
> + period = <1>;
> + quality = <100>;
> + };
> +
> + rtc: rtc@1e781000 {
> + compatible = "aspeed,ast2600-rtc";
> + reg = <0x1e781000 0x18>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart5: serial@1e784000 {
> + compatible = "ns16550a";
> + reg = <0x1e784000 0x1000>;
> + reg-shift = <2>;
> + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_UART5CLK>;
> + no-loopback-test;
> + };
> +
> + wdt1: watchdog@1e785000 {
> + compatible = "aspeed,ast2600-wdt";
> + reg = <0x1e785000 0x40>;
> + };
> +
> + wdt2: watchdog@1e785040 {
> + compatible = "aspeed,ast2600-wdt";
> + reg = <0x1e785040 0x40>;
> + status = "disabled";
> + };
> +
> + wdt3: watchdog@1e785080 {
> + compatible = "aspeed,ast2600-wdt";
> + reg = <0x1e785080 0x40>;
> + status = "disabled";
> + };
> +
> + wdt4: watchdog@1e7850C0 {
> + compatible = "aspeed,ast2600-wdt";
> + reg = <0x1e7850C0 0x40>;
> + status = "disabled";
> + };
> +
> + sdc: sdc@1e740000 {
> + compatible = "aspeed,ast2600-sd-controller";
> + reg = <0x1e740000 0x100>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x1e740000 0x10000>;
> + clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> + status = "disabled";
> +
> + sdhci0: sdhci@1e740100 {
> + compatible = "aspeed,ast2600-sdhci", "sdhci";
> + reg = <0x100 0x100>;
> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> + sdhci,auto-cmd12;
> + clocks = <&syscon ASPEED_CLK_SDIO>;
> + status = "disabled";
> + };
> +
> + sdhci1: sdhci@1e740200 {
> + compatible = "aspeed,ast2600-sdhci", "sdhci";
> + reg = <0x200 0x100>;
> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> + sdhci,auto-cmd12;
> + clocks = <&syscon ASPEED_CLK_SDIO>;
> + status = "disabled";
> + };
> + };
> +
> + emmc: sdc@1e750000 {
> + compatible = "aspeed,ast2600-sd-controller";
> + reg = <0x1e750000 0x100>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x1e750000 0x10000>;
> + clocks = <&syscon ASPEED_CLK_GATE_EMMCCLK>;
> + status = "disabled";
> +
> + sdhci@1e750100 {
> + compatible = "aspeed,ast2600-sdhci";
> + reg = <0x100 0x100>;
> + sdhci,auto-cmd12;
> + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_EMMC>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_emmc_default>;
> + };
> + };
> + };
> + };
> +};
> +
> +&pinctrl {
> + pinctrl_emmc_default: emmc_default {
> + function = "SD3";
> + groups = "SD3";
> + };
I need to send some fixes for pinmux along with the dt patche, but this
will do for the moment.
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/7] ARM: aspeed: Add ASPEED AST2600 architecture
From: Andrew Jeffery @ 2019-08-22 1:28 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-4-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> The AST2600 is a Cortex A7 dual core CPU that uses the ARM GIC for
> interrupts and ARM timer as a clocksource.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/mach-aspeed/Kconfig | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 2979aa4daeea..56007b0b6120 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> menuconfig ARCH_ASPEED
> bool "Aspeed BMC architectures"
> - depends on ARCH_MULTI_V5 || ARCH_MULTI_V6
> + depends on ARCH_MULTI_V5 || ARCH_MULTI_V6 || ARCH_MULTI_V7
> select SRAM
> select WATCHDOG
> select ASPEED_WATCHDOG
> @@ -33,4 +33,16 @@ config MACH_ASPEED_G5
> Say yes if you intend to run on an Aspeed ast2500 or similar
> fifth generation Aspeed BMCs.
>
> +config MACH_ASPEED_G6
> + bool "Aspeed SoC 6th Generation"
> + depends on ARCH_MULTI_V7
> + select CPU_V7
> + select PINCTRL_ASPEED_G6
> + select ARM_GIC
> + select HAVE_ARM_ARCH_TIMER
> + select HAVE_SMP
> + help
> + Say yes if you intend to run on an Aspeed ast2600 or similar
> + sixth generation Aspeed BMCs.
> +
> endif
> --
> 2.23.0.rc1
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/7] ARM: aspeed: Select timer in each SoC
From: Andrew Jeffery @ 2019-08-22 1:27 UTC (permalink / raw)
To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-3-joel@jms.id.au>
On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> In preparation for adding the ast2600 which does not use this timer.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/mach-aspeed/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index a15c3a291386..2979aa4daeea 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -5,7 +5,6 @@ menuconfig ARCH_ASPEED
> select SRAM
> select WATCHDOG
> select ASPEED_WATCHDOG
> - select FTTMR010_TIMER
> select MFD_SYSCON
> select PINCTRL
> help
> @@ -18,6 +17,7 @@ config MACH_ASPEED_G4
> depends on ARCH_MULTI_V5
> select CPU_ARM926T
> select PINCTRL_ASPEED_G4
> + select FTTMR010_TIMER
> help
> Say yes if you intend to run on an Aspeed ast2400 or similar
> fourth generation BMCs, such as those used by OpenPower Power8
> @@ -28,6 +28,7 @@ config MACH_ASPEED_G5
> depends on ARCH_MULTI_V6
> select CPU_V6
> select PINCTRL_ASPEED_G5
> + select FTTMR010_TIMER
> help
> Say yes if you intend to run on an Aspeed ast2500 or similar
> fifth generation Aspeed BMCs.
> --
> 2.23.0.rc1
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4] kasan: add memory corruption identification for software tag-based mode
From: Walter Wu @ 2019-08-22 1:22 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: wsd_upstream, Vasily Gorbik, Arnd Bergmann, linux-mm,
Andrey Konovalov, linux-kernel, kasan-dev, Martin Schwidefsky,
Miles Chen, Alexander Potapenko, linux-arm-kernel,
Matthias Brugger, linux-mediatek, Andrew Morton, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <3318f9d7-a760-3cc8-b700-f06108ae745f@virtuozzo.com>
On Wed, 2019-08-21 at 20:52 +0300, Andrey Ryabinin wrote:
>
> On 8/20/19 8:37 AM, Walter Wu wrote:
> > On Tue, 2019-08-06 at 13:43 +0800, Walter Wu wrote:
> >> This patch adds memory corruption identification at bug report for
> >> software tag-based mode, the report show whether it is "use-after-free"
> >> or "out-of-bound" error instead of "invalid-access" error. This will make
> >> it easier for programmers to see the memory corruption problem.
> >>
> >> We extend the slab to store five old free pointer tag and free backtrace,
> >> we can check if the tagged address is in the slab record and make a
> >> good guess if the object is more like "use-after-free" or "out-of-bound".
> >> therefore every slab memory corruption can be identified whether it's
> >> "use-after-free" or "out-of-bound".
> >>
> >> ====== Changes
> >> Change since v1:
> >> - add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
> >> - change QUARANTINE_FRACTION to reduce quarantine size.
> >> - change the qlist order in order to find the newest object in quarantine
> >> - reduce the number of calling kmalloc() from 2 to 1 time.
> >> - remove global variable to use argument to pass it.
> >> - correct the amount of qobject cache->size into the byes of qlist_head.
> >> - only use kasan_cache_shrink() to shink memory.
> >>
> >> Change since v2:
> >> - remove the shinking memory function kasan_cache_shrink()
> >> - modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
> >> - optimize the quarantine_find_object() and qobject_free()
> >> - fix the duplicating function name 3 times in the header.
> >> - modify the function name set_track() to kasan_set_track()
> >>
> >> Change since v3:
> >> - change tag-based quarantine to extend slab to identify memory corruption
> >
> > Hi,Andrey,
> >
> > Would you review the patch,please?
>
>
> I didn't notice anything fundamentally wrong, but I find there are some
> questionable implementation choices that makes code look weirder than necessary
> and harder to understand. So I ended up with cleaning it up, see the diff bellow.
> I'll send v5 with that diff folded.
>
Thanks your review and suggestion.
Walter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 5/9] block: support diskcipher
From: boojin.kim @ 2019-08-22 0:54 UTC (permalink / raw)
To: axboe, linux-block, linux-kernel
Cc: 'Ulf Hansson', 'Mike Snitzer', dm-devel,
'Andreas Dilger', 'Alasdair Kergon',
'Eric Biggers', linux-samsung-soc, 'Herbert Xu',
'Krzysztof Kozlowski', 'Jaehoon Chung',
'Kukjin Kim', linux-ext4, 'Chao Yu',
linux-fscrypt, 'Jaegeuk Kim', linux-arm-kernel,
'Jens Axboe', 'Theodore Ts'o', linux-mmc,
linux-f2fs-devel, linux-crypto, linux-fsdevel,
'David S. Miller'
In-Reply-To: <CGME20190822005438epcas2p337aba06b328cdcdd1549395f0bbcfdbc@epcas2p3.samsung.com>
On 8/21/19 21:09 AM, Jens Axboe wrote:
> This isn't going to happen. With this, and the inline encryption
> proposed by Google, we'll bloat the bio even more. At least the Google
> approach didn't include bio iter changes as well.
> Please work it out between yourselves so we can have a single, clean
> abstraction that works for both.
I'm looking at inline encryption by Google.
I will find compatibility with inline encryption to avoid conflicts
in BIO/F2FS.
And changing bio iter has a benefit for diskcipher.
Without changing bio iter, diskcipher should control(alloc/free) a buffer
to hold a 'bi_dun' variable for every bio.
But changing bio iter is difficult to accept for block layer,
I will modify the diskcipher.
And, as you mentioned, inline encryption by Google has this control.
So I might be able to use it.
Thanks.
Boojin Kim.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal
From: Kevin Hilman @ 2019-08-21 23:40 UTC (permalink / raw)
To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
linux-pm
In-Reply-To: <20190821222421.30242-2-glaroque@baylibre.com>
Guillaume La Roque <glaroque@baylibre.com> writes:
> Adding the devicetree binding documentation for the Amlogic temperature
> sensor found in the Amlogic Meson G12 SoCs.
> the G12A and G12B SoCs are supported.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
nit: put your sign-off at the end. The tags you collect from
reviewers/testers should go first.
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 0/6] Add support of New Amlogic temperature sensor for G12 SoCs
From: Kevin Hilman @ 2019-08-21 23:39 UTC (permalink / raw)
To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>
Guillaume La Roque <glaroque@baylibre.com> writes:
> This patchs series add support of New Amlogic temperature sensor and minimal
> thermal zone for SEI510 and ODROID-N2 boards.
>
> First implementation was doing on IIO[1] but after comments i move on thermal framework.
> Formulas and calibration values come from amlogic.
>
> Changes since v3:
> - Add cooling map and trip point for hot type
> - move compatible on g12a instead of g12 to be aligned with others
> - add all reviewer, sorry for this mistake
>
> Changes since v2:
> - fix yaml documention
> - remove unneeded status variable for temperature-sensor node
> - rework driver after Martin review
> - add some information in commit message
>
> Changes since v1:
> - fix enum vs const in documentation
> - fix error with thermal-sensor-cells value set to 1 instead of 0
> - add some dependencies needed to add cooling-maps
>
> Dependencies :
> - patch 3,4 & 5: depends on Neil's patch and series :
> - missing dwc2 phy-names[2]
> - patchsets to add DVFS on G12a[3] which have deps on [4] and [5]
>
> [1] https://lore.kernel.org/linux-amlogic/20190604144714.2009-1-glaroque@baylibre.com/
> [2] https://lore.kernel.org/linux-amlogic/20190625123647.26117-1-narmstrong@baylibre.com/
> [3] https://lore.kernel.org/linux-amlogic/20190729132622.7566-1-narmstrong@baylibre.com/
> [4] https://lore.kernel.org/linux-amlogic/20190731084019.8451-5-narmstrong@baylibre.com/
> [5] https://lore.kernel.org/linux-amlogic/20190729132622.7566-3-narmstrong@baylibre.com/
>
>
> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
> Tested-by: Kevin Hilman <khilman@baylibre.com>
nit: you should put these on the individual patches, since the cover
letter does not get applied to any tree, any tags here get lost.
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support
From: Kevin Hilman @ 2019-08-21 23:31 UTC (permalink / raw)
To: Neil Armstrong, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-5-narmstrong@baylibre.com>
Neil Armstrong <narmstrong@baylibre.com> writes:
> Update compatible of the pwc-vpu node and add the HDMI support nodes
> for the Amlogic SM1 Based SEI610 Board.
I think this changelog is out of date. It's not doing anything with the
VPU pwrc node.
Kevin
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> .../boot/dts/amlogic/meson-sm1-sei610.dts | 23 +++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> index 12dab0ba2f26..66bd3bfbaf91 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> @@ -51,6 +51,17 @@
> };
> };
>
> + hdmi-connector {
> + compatible = "hdmi-connector";
> + type = "a";
> +
> + port {
> + hdmi_connector_in: endpoint {
> + remote-endpoint = <&hdmi_tx_tmds_out>;
> + };
> + };
> + };
> +
> leds {
> compatible = "gpio-leds";
>
> @@ -177,6 +188,18 @@
> phy-mode = "rmii";
> };
>
> +&hdmi_tx {
> + status = "okay";
> + pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
> + pinctrl-names = "default";
> +};
> +
> +&hdmi_tx_tmds_port {
> + hdmi_tx_tmds_out: endpoint {
> + remote-endpoint = <&hdmi_connector_in>;
> + };
> +};
> +
> &i2c3 {
> status = "okay";
> pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
> --
> 2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
From: Kevin Hilman @ 2019-08-21 23:29 UTC (permalink / raw)
To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
linux-pm
In-Reply-To: <20190821222421.30242-5-glaroque@baylibre.com>
Guillaume La Roque <glaroque@baylibre.com> writes:
> Add minimal thermal zone for two temperature sensor
> One is located close to the DDR and the other one is
> located close to the PLLs (between the CPU and GPU)
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> .../boot/dts/amlogic/meson-g12a-sei510.dts | 70 +++++++++++++++++++
> 1 file changed, 70 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> index c9fa23a56562..35d2ebbd6d4e 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> @@ -10,6 +10,7 @@
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/gpio/meson-g12a-gpio.h>
> #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
> +#include <dt-bindings/thermal/thermal.h>
>
> / {
> compatible = "seirobotics,sei510", "amlogic,g12a";
> @@ -33,6 +34,67 @@
> ethernet0 = ðmac;
> };
>
> + thermal-zones {
> + cpu-thermal {
> + polling-delay = <1000>;
> + polling-delay-passive = <100>;
> + thermal-sensors = <&cpu_temp>;
> +
> + trips {
> + cpu_hot: cpu-hot {
> + temperature = <85000>; /* millicelsius */
> + hysteresis = <2000>; /* millicelsius */
> + type = "hot";
> + };
> +
> + cpu_critical: cpu-critical {
> + temperature = <110000>; /* millicelsius */
> + hysteresis = <2000>; /* millicelsius */
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + map0 {
> + trip = <&cpu_hot>;
> + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> +
> + map1 {
> + trip = <&cpu_critical>;
> + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> +
> + ddr-thermal {
> + polling-delay = <1000>;
> + polling-delay-passive = <100>;
> + thermal-sensors = <&ddr_temp>;
> +
> + trips {
> + ddr_critical: ddr-critical {
> + temperature = <110000>; /* millicelsius */
> + hysteresis = <2000>; /* millicelsius */
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + map {
> + trip = <&ddr_critical>;
> + cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> + };
> +
> mono_dac: audio-codec-0 {
> compatible = "maxim,max98357a";
> #sound-dai-cells = <0>;
> @@ -321,6 +383,7 @@
> operating-points-v2 = <&cpu_opp_table>;
> clocks = <&clkc CLKID_CPU_CLK>;
> clock-latency = <50000>;
> + #cooling-cells = <2>;
> };
>
> &cpu1 {
> @@ -328,6 +391,7 @@
> operating-points-v2 = <&cpu_opp_table>;
> clocks = <&clkc CLKID_CPU_CLK>;
> clock-latency = <50000>;
> + #cooling-cells = <2>;
> };
>
> &cpu2 {
> @@ -335,6 +399,7 @@
> operating-points-v2 = <&cpu_opp_table>;
> clocks = <&clkc CLKID_CPU_CLK>;
> clock-latency = <50000>;
> + #cooling-cells = <2>;
> };
>
> &cpu3 {
> @@ -342,6 +407,7 @@
> operating-points-v2 = <&cpu_opp_table>;
> clocks = <&clkc CLKID_CPU_CLK>;
> clock-latency = <50000>;
> + #cooling-cells = <2>;
> };
>
> &cvbs_vdac_port {
> @@ -368,6 +434,10 @@
> status = "okay";
> };
>
> +&mali {
> + #cooling-cells = <2>;
> +};
> +
Is there a reason these #cooling-cells properties belong in the SoC
.dtsi and not the board .dts. Seems like you'll have to repeat this in
every board .dts which doesn't seem necessary.
Same comment for patch 5/6
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller
From: Kevin Hilman @ 2019-08-21 23:16 UTC (permalink / raw)
To: Neil Armstrong, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-3-narmstrong@baylibre.com>
Neil Armstrong <narmstrong@baylibre.com> writes:
> Add support for the General Purpose Amlogic Everything-Else Power controller,
> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
> USB, NNA, GE2D and Ethernet Power Domains.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Nice! Thanks for generalizing this.
A few comments/concerns below, but this is mostly ready.
> ---
> drivers/soc/amlogic/Kconfig | 11 +
> drivers/soc/amlogic/Makefile | 1 +
> drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
> 3 files changed, 572 insertions(+)
> create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index 23bfb8ef4fdb..bc2c912949bd 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
> Say yes to expose Amlogic Meson GX Power Domains as
> Generic Power Domains.
>
> +config MESON_EE_PM_DOMAINS
> + bool "Amlogic Meson Everything-Else Power Domains driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + depends on PM && OF
> + default ARCH_MESON
> + select PM_GENERIC_DOMAINS
> + select PM_GENERIC_DOMAINS_OF
> + help
> + Say yes to expose Amlogic Meson Everything-Else Power Domains as
> + Generic Power Domains.
> +
> config MESON_MX_SOCINFO
> bool "Amlogic Meson MX SoC Information driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index f2e4ed171297..de79d044b545 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
> obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
> obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
> obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> +obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
> new file mode 100644
> index 000000000000..7159888c850b
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
> @@ -0,0 +1,560 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset.h>
> +#include <linux/clk.h>
> +#include <dt-bindings/power/meson-g12a-power.h>
> +#include <dt-bindings/power/meson-sm1-power.h>
> +
> +/* AO Offsets */
> +
> +#define AO_RTI_GEN_PWR_SLEEP0 (0x3a << 2)
> +#define AO_RTI_GEN_PWR_ISO0 (0x3b << 2)
> +
> +/* HHI Offsets */
> +
> +#define HHI_MEM_PD_REG0 (0x40 << 2)
> +#define HHI_VPU_MEM_PD_REG0 (0x41 << 2)
> +#define HHI_VPU_MEM_PD_REG1 (0x42 << 2)
> +#define HHI_VPU_MEM_PD_REG3 (0x43 << 2)
> +#define HHI_VPU_MEM_PD_REG4 (0x44 << 2)
> +#define HHI_AUDIO_MEM_PD_REG0 (0x45 << 2)
> +#define HHI_NANOQ_MEM_PD_REG0 (0x46 << 2)
> +#define HHI_NANOQ_MEM_PD_REG1 (0x47 << 2)
> +#define HHI_VPU_MEM_PD_REG2 (0x4d << 2)
> +
> +struct meson_ee_pwrc;
> +struct meson_ee_pwrc_domain;
> +
> +struct meson_ee_pwrc_mem_domain {
> + unsigned int reg;
> + unsigned int mask;
> +};
> +
> +struct meson_ee_pwrc_top_domain {
> + unsigned int sleep_reg;
> + unsigned int sleep_mask;
> + unsigned int iso_reg;
> + unsigned int iso_mask;
> +};
> +
> +struct meson_ee_pwrc_domain_desc {
> + char *name;
> + char **reset_names;
> + unsigned int reset_names_count;
> + char **clk_names;
> + unsigned int clk_names_count;
> + struct meson_ee_pwrc_top_domain *top_pd;
> + unsigned int mem_pd_count;
> + struct meson_ee_pwrc_mem_domain *mem_pd;
> + bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
> +};
> +
> +struct meson_ee_pwrc_domain_data {
> + unsigned int count;
> + struct meson_ee_pwrc_domain_desc *domains;
> +};
> +
> +/* Clock and Resets lists */
> +
> +static char *g12a_pwrc_vpu_resets[] = {
> + "viu", "venc", "vcbus", "bt656",
> + "rdma", "venci", "vencp", "vdac",
> + "vdi6", "vencl", "vid_lock",
> +};
> +
> +static char *g12a_pwrc_vpu_clks[] = {
> + "vpu", "vapb",
> +};
> +
> +/* TOP Power Domains */
> +
> +static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
> + .sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
> + .sleep_mask = BIT(8),
> + .iso_reg = AO_RTI_GEN_PWR_SLEEP0,
> + .iso_mask = BIT(9),
> +};
> +
> +#define SM1_EE_PD(__bit) \
> + { \
> + .sleep_reg = AO_RTI_GEN_PWR_SLEEP0, \
> + .sleep_mask = BIT(__bit), \
> + .iso_reg = AO_RTI_GEN_PWR_ISO0, \
> + .iso_mask = BIT(__bit), \
> + }
> +
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
> +
> +/* Memory PD Domains */
> +
> +#define VPU_MEMPD(__reg) \
> + { __reg, GENMASK(1, 0) }, \
> + { __reg, GENMASK(3, 2) }, \
> + { __reg, GENMASK(5, 4) }, \
> + { __reg, GENMASK(7, 6) }, \
> + { __reg, GENMASK(9, 8) }, \
> + { __reg, GENMASK(11, 10) }, \
> + { __reg, GENMASK(13, 12) }, \
> + { __reg, GENMASK(15, 14) }, \
> + { __reg, GENMASK(17, 16) }, \
> + { __reg, GENMASK(19, 18) }, \
> + { __reg, GENMASK(21, 20) }, \
> + { __reg, GENMASK(23, 22) }, \
> + { __reg, GENMASK(25, 24) }, \
> + { __reg, GENMASK(27, 26) }, \
> + { __reg, GENMASK(29, 28) }, \
> + { __reg, GENMASK(31, 30) }
> +
> +#define VPU_HHI_MEMPD(__reg) \
> + { __reg, BIT(8) }, \
> + { __reg, BIT(9) }, \
> + { __reg, BIT(10) }, \
> + { __reg, BIT(11) }, \
> + { __reg, BIT(12) }, \
> + { __reg, BIT(13) }, \
> + { __reg, BIT(14) }, \
> + { __reg, BIT(15) }
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> + VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
> + { HHI_MEM_PD_REG0, GENMASK(3, 2) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> + VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
> + { HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
> + { HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
> + { HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
> + { HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
> + VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
> + { HHI_NANOQ_MEM_PD_REG0, 0xff },
> + { HHI_NANOQ_MEM_PD_REG1, 0xff },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
> + { HHI_MEM_PD_REG0, GENMASK(31, 30) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
> + { HHI_MEM_PD_REG0, GENMASK(29, 26) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
> + { HHI_MEM_PD_REG0, GENMASK(25, 18) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
> + { HHI_MEM_PD_REG0, GENMASK(5, 4) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
> + { HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
> +};
> +
> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power) \
> + { \
> + .name = __name, \
> + .reset_names_count = ARRAY_SIZE(__resets), \
> + .reset_names = __resets, \
> + .clk_names_count = ARRAY_SIZE(__clks), \
> + .clk_names = __clks, \
> + .top_pd = __top_pd, \
> + .mem_pd_count = ARRAY_SIZE(__mem), \
> + .mem_pd = __mem, \
> + .get_power = __get_power, \
> + }
> +
> +#define TOP_PD(__name, __top_pd, __mem) \
> + { \
> + .name = __name, \
> + .top_pd = __top_pd, \
> + .mem_pd_count = ARRAY_SIZE(__mem), \
> + .mem_pd = __mem, \
> + }
Why can't the TOP_PD domains also have a __get_power(). Shouldn't we
just be able to check the sleep_reg/sleep_mask like in the VPU case?
Also, for readability, I think the arguments to VPU_PD and TOP_PD to
have the same order, at least for the common ones. IOW, __name,
__top_pd, __mem should be first.
> +#define MEM_PD(__name, __mem) \
> + TOP_PD(__name, NULL, __mem)
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
> +
> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
> + [PWRC_G12A_VPU_ID] = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> + g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
> + g12a_pwrc_mem_vpu,
> + pwrc_vpu_get_power),
> + [PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
> + [PWRC_SM1_VPU_ID] = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> + g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
> + sm1_pwrc_mem_vpu,
> + pwrc_vpu_get_power),
> + [PWRC_SM1_NNA_ID] = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
> + [PWRC_SM1_USB_ID] = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
> + [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
> + [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
> + [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
> + [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +struct meson_ee_pwrc_domain {
> + struct generic_pm_domain base;
> + bool enabled;
> + struct meson_ee_pwrc *pwrc;
> + struct meson_ee_pwrc_domain_desc desc;
> + struct clk **clks;
> + int num_clks;
> + struct reset_control **rstc;
> + int num_rstc;
> +};
> +
> +struct meson_ee_pwrc {
> + struct regmap *regmap_ao;
> + struct regmap *regmap_hhi;
> + struct meson_ee_pwrc_domain *domains;
> + struct genpd_onecell_data xlate;
> +};
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> + u32 reg;
> +
> + regmap_read(pwrc_domain->pwrc->regmap_ao,
> + pwrc_domain->desc.top_pd->sleep_reg, ®);
> +
> + return (reg & pwrc_domain->desc.top_pd->sleep_mask);
> +}
> +
> +static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> + int i, ret;
> +
> + for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> + ret = reset_control_assert(pwrc_domain->rstc[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> + int i, ret;
> +
> + for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> + ret = reset_control_deassert(pwrc_domain->rstc[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
You should use the reset_array functions, then you don't need these helpers.
> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> + int i;
> +
> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> + clk_disable(pwrc_domain->clks[i]);
> +
> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> + clk_unprepare(pwrc_domain->clks[i]);
> +
> + return 0;
> +}
> +
> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> + int i, ret;
> +
> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> + ret = clk_prepare(pwrc_domain->clks[i]);
> + if (ret)
> + goto fail_prepare;
> + }
> +
> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> + ret = clk_enable(pwrc_domain->clks[i]);
> + if (ret)
> + goto fail_enable;
> + }
> +
> + return 0;
> +fail_enable:
> + while (--i)
> + clk_disable(pwrc_domain->clks[i]);
> +
> + /* Unprepare all clocks */
> + i = pwrc_domain->num_clks;
> +
> +fail_prepare:
> + while (--i)
> + clk_unprepare(pwrc_domain->clks[i]);
> +
> + return ret;
> +}
Both the clk enable and disable functions above are just open-coding of
the clk_bulk equivalents. Please use clk_bulk_*, then you don't need
these helpers. (c.f. the RFT patch I did to convert the old driver to
clk_bulk[1])
> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
> +{
> + struct meson_ee_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_ee_pwrc_domain, base);
> + int i;
> +
> + if (pwrc_domain->desc.top_pd)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> + pwrc_domain->desc.top_pd->sleep_reg,
> + pwrc_domain->desc.top_pd->sleep_mask,
> + pwrc_domain->desc.top_pd->sleep_mask);
> + udelay(20);
> +
> + for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> + pwrc_domain->desc.mem_pd[i].reg,
> + pwrc_domain->desc.mem_pd[i].mask,
> + pwrc_domain->desc.mem_pd[i].mask);
> +
> + udelay(20);
> +
> + if (pwrc_domain->desc.top_pd)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> + pwrc_domain->desc.top_pd->iso_reg,
> + pwrc_domain->desc.top_pd->iso_mask,
> + pwrc_domain->desc.top_pd->iso_mask);
> +
> + if (pwrc_domain->num_clks) {
> + msleep(20);
> + meson_ee_clk_disable(pwrc_domain);
> + }
> +
> + return 0;
> +}
> +
> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
> +{
> + struct meson_ee_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_ee_pwrc_domain, base);
> + int i, ret;
> +
> + if (pwrc_domain->desc.top_pd)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> + pwrc_domain->desc.top_pd->sleep_reg,
> + pwrc_domain->desc.top_pd->sleep_mask, 0);
> + udelay(20);
> +
> + for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> + pwrc_domain->desc.mem_pd[i].reg,
> + pwrc_domain->desc.mem_pd[i].mask, 0);
> +
> + udelay(20);
> +
> + ret = meson_ee_reset_assert(pwrc_domain);
> + if (ret)
> + return ret;
> +
> + if (pwrc_domain->desc.top_pd)
> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> + pwrc_domain->desc.top_pd->iso_reg,
> + pwrc_domain->desc.top_pd->iso_mask, 0);
> +
> + ret = meson_ee_reset_deassert(pwrc_domain);
> + if (ret)
> + return ret;
> +
> + return meson_ee_clk_enable(pwrc_domain);
> +}
> +
> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
> + struct meson_ee_pwrc *sm1_pwrc,
> + struct meson_ee_pwrc_domain *dom)
> +{
> + dom->pwrc = sm1_pwrc;
> + dom->num_rstc = dom->desc.reset_names_count;
> + dom->num_clks = dom->desc.clk_names_count;
> +
> + if (dom->num_rstc) {
> + int rst;
> +
> + dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
> + sizeof(struct reset_control *), GFP_KERNEL);
> + if (!dom->rstc)
> + return -ENOMEM;
> +
> + for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
> + dom->rstc[rst] = devm_reset_control_get_exclusive(
> + &pdev->dev,
> + dom->desc.reset_names[rst]);
> + if (IS_ERR(dom->rstc[rst]))
> + return PTR_ERR(dom->rstc[rst]);
> + }
Why not simplify and use the helpers that get multiple reset lines (like
devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?
You could also use reset_control_get_count() and compare to the expected
number (dom->num_rstc).
> + }
> +
> + if (dom->num_clks) {
> + int clk;
> +
> + dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
> + sizeof(struct clk *), GFP_KERNEL);
> + if (!dom->clks)
> + return -ENOMEM;
> +
> + for (clk = 0 ; clk < dom->num_clks ; ++clk) {
> + dom->clks[clk] = devm_clk_get(&pdev->dev,
> + dom->desc.clk_names[clk]);
> + if (IS_ERR(dom->clks[clk]))
> + return PTR_ERR(dom->clks[clk]);
> + }
> + }
Please use clk_bulk API, and then just double-check that the number of
clocks found matches the expected number.
> + dom->base.name = dom->desc.name;
> + dom->base.power_on = meson_ee_pwrc_on;
> + dom->base.power_off = meson_ee_pwrc_off;
> +
> + if (dom->desc.get_power) {
> + bool powered_off = dom->desc.get_power(dom);
nit: insert blank line here
More importantly, we defintely will have problem here in the
!powered_off case. TL;DR; the driver's state does not match the actual
hardware state.
When powered_off = false, you're telling the genpd core that this domain
is already turned on. However, you haven't called _pwrc_on() yet for
the domain, which means internal state of the driver for this domain
(e.g. clock enables, resets, etc.) is not in sync with the HW. On
SEI610 this case is happending for the VPU, which seems to be enabled by
u-boot, so this driver detects it as already on, which is fine. But...
Remember that the ->power_off() function will be called during suspend,
and will lead to the clk unprepare/disable calls. However, for domains
that are detected as on (!powered_off), clk prepare/enable will never
have been called, so that when suspend happens, you'll get "already
unprepared" errors from the clock core
IOW, I think you need something like this here:
if (!powered_off)
meson_ee_pwrc_on(&dom->base);
so that the internal state of clock fwk etc. matches the detected state
of the HW. The problem with that simple fix, at least for the VPU is
that it might cause us to lose any existing display or framebuffer
console that's on-going. Probably needs some testing.
Anyways, to see what I mean, try suspend/resume (you can test this
series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
see error splats from the clock core during suspend.
> + pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
> + powered_off);
> + } else
> + pm_genpd_init(&dom->base, NULL, true);
nit: the else clause should also have {} to match the if
(c.f. CodingStyle)
Why do you force the always-on governor in the case where ->get_power()
exists, but not the other?
If you force that, then for any devices connected to these domains that
use runtime PM, they will never turn off the domain when it's idle.
IOW, these domains will only ever be turned off on system-wide
suspend/resume.
IMO, unless there's a good reason not to, you should pass NULL for the
governor.
> + return 0;
> +}
> +
> +static int meson_ee_pwrc_probe(struct platform_device *pdev)
> +{
> + const struct meson_ee_pwrc_domain_data *match;
> + struct regmap *regmap_ao, *regmap_hhi;
> + struct meson_ee_pwrc *sm1_pwrc;
Why the sm1_ prefix throughout this code? This is now generalized for
more SoCs, so shouldn't be sm1. I'm assuming this is just left-over
from the original version. IMO, just called it pwrc.
> + int i, ret;
> +
> + match = of_device_get_match_data(&pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "failed to get match data\n");
> + return -ENODEV;
> + }
> +
> + sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
> + if (!sm1_pwrc)
> + return -ENOMEM;
> +
> + sm1_pwrc->xlate.domains =
> + devm_kcalloc(&pdev->dev,
> + match->count,
> + sizeof(*sm1_pwrc->xlate.domains),
> + GFP_KERNEL);
> + if (!sm1_pwrc->xlate.domains)
> + return -ENOMEM;
> +
> + sm1_pwrc->domains =
> + devm_kcalloc(&pdev->dev,
nit: this line probably doesn't need to be wrapped.
> + match->count,
> + sizeof(*sm1_pwrc->domains),
> + GFP_KERNEL);
> + if (!sm1_pwrc->domains)
> + return -ENOMEM;
> +
> + sm1_pwrc->xlate.num_domains = match->count;
> +
> + regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
> + if (IS_ERR(regmap_hhi)) {
> + dev_err(&pdev->dev, "failed to get HHI regmap\n");
> + return PTR_ERR(regmap_hhi);
> + }
> +
> + regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + "amlogic,ao-sysctrl");
> + if (IS_ERR(regmap_ao)) {
> + dev_err(&pdev->dev, "failed to get AO regmap\n");
> + return PTR_ERR(regmap_ao);
> + }
> +
> + sm1_pwrc->regmap_ao = regmap_ao;
> + sm1_pwrc->regmap_hhi = regmap_hhi;
> +
> + platform_set_drvdata(pdev, sm1_pwrc);
> +
> + for (i = 0 ; i < match->count ; ++i) {
> + struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
> +
> + memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
> +
> + ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
> + if (ret)
> + return ret;
> +
> + sm1_pwrc->xlate.domains[i] = &dom->base;
> + }
> +
> + of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
> +
> + return 0;
> +}
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
> + .count = ARRAY_SIZE(g12a_pwrc_domains),
> + .domains = g12a_pwrc_domains,
> +};
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
> + .count = ARRAY_SIZE(sm1_pwrc_domains),
> + .domains = sm1_pwrc_domains,
> +};
> +
> +static const struct of_device_id meson_ee_pwrc_match_table[] = {
> + {
> + .compatible = "amlogic,meson-g12a-pwrc",
> + .data = &meson_ee_g12a_pwrc_data,
> + },
> + {
> + .compatible = "amlogic,meson-sm1-pwrc",
> + .data = &meson_ee_sm1_pwrc_data,
> + },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver meson_ee_pwrc_driver = {
> + .probe = meson_ee_pwrc_probe,
> + .driver = {
> + .name = "meson_ee_pwrc",
> + .of_match_table = meson_ee_pwrc_match_table,
> + },
> +};
> +builtin_platform_driver(meson_ee_pwrc_driver);
> --
> 2.22.0
Kevin
[1] https://lore.kernel.org/linux-amlogic/20190809230904.28747-1-khilman@baylibre.com/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v4 6/6] MAINTAINERS: add entry for Amlogic Thermal driver
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
To: rui.zhang, edubezval, daniel.lezcano
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>
Add myself as maintainer for Amlogic Thermal driver.
Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fb2b12f75c37..299f27d11058 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15910,6 +15910,15 @@ F: Documentation/driver-api/thermal/cpu-cooling-api.rst
F: drivers/thermal/cpu_cooling.c
F: include/linux/cpu_cooling.h
+THERMAL DRIVER FOR AMLOGIC SOCS
+M: Guillaume La Roque <glaroque@baylibre.com>
+L: linux-pm@vger.kernel.org
+L: linux-amlogic@lists.infradead.org
+W: http://linux-meson.com/
+S: Supported
+F: drivers/thermal/amlogic_thermal.c
+F: Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
+
THINKPAD ACPI EXTRAS DRIVER
M: Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
L: ibm-acpi-devel@lists.sourceforge.net
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4 5/6] arm64: dts: amlogic: odroid-n2: add minimal thermal zone
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
To: rui.zhang, edubezval, daniel.lezcano
Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>
Add minimal thermal zone for two temperature sensor
One is located close to the DDR and the other one is
located close to the PLLs (between the CPU and GPU)
Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
.../boot/dts/amlogic/meson-g12b-odroid-n2.dts | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
index 777bfb938854..8d7c73bad4aa 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
@@ -10,6 +10,7 @@
#include <dt-bindings/input/input.h>
#include <dt-bindings/gpio/meson-g12a-gpio.h>
#include <dt-bindings/sound/meson-g12a-tohdmitx.h>
+#include <dt-bindings/thermal/thermal.h>
/ {
compatible = "hardkernel,odroid-n2", "amlogic,g12b";
@@ -20,6 +21,71 @@
ethernet0 = ðmac;
};
+ thermal-zones {
+ cpu-thermal {
+ polling-delay = <1000>;
+ polling-delay-passive = <100>;
+ thermal-sensors = <&cpu_temp>;
+
+ trips {
+ cpu_hot: cpu-hot {
+ temperature = <85000>; /* millicelsius */
+ hysteresis = <2000>; /* millicelsius */
+ type = "hot";
+ };
+
+ cpu_critical: cpu-critical {
+ temperature = <110000>; /* millicelsius */
+ hysteresis = <2000>; /* millicelsius */
+ type = "critical";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <&cpu_hot>;
+ cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu100 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu101 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu102 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu103 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+
+ map1 {
+ trip = <&cpu_critical>;
+ cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu100 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu101 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu102 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu103 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
+ };
+
+ ddr-thermal {
+ polling-delay = <1000>;
+ polling-delay-passive = <100>;
+ thermal-sensors = <&ddr_temp>;
+
+ trips {
+ ddr_critical: ddr-critical {
+ temperature = <110000>; /* millicelsius */
+ hysteresis = <2000>; /* millicelsius */
+ type = "critical";
+ };
+ };
+
+ cooling-maps {
+ map {
+ trip = <&ddr_critical>;
+ cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
+ };
+ };
+
chosen {
stdout-path = "serial0:115200n8";
};
@@ -289,6 +355,7 @@
operating-points-v2 = <&cpu_opp_table_0>;
clocks = <&clkc CLKID_CPU_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&cpu1 {
@@ -296,6 +363,7 @@
operating-points-v2 = <&cpu_opp_table_0>;
clocks = <&clkc CLKID_CPU_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&cpu100 {
@@ -303,6 +371,7 @@
operating-points-v2 = <&cpub_opp_table_1>;
clocks = <&clkc CLKID_CPUB_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&cpu101 {
@@ -310,6 +379,7 @@
operating-points-v2 = <&cpub_opp_table_1>;
clocks = <&clkc CLKID_CPUB_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&cpu102 {
@@ -317,6 +387,7 @@
operating-points-v2 = <&cpub_opp_table_1>;
clocks = <&clkc CLKID_CPUB_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&cpu103 {
@@ -324,6 +395,7 @@
operating-points-v2 = <&cpub_opp_table_1>;
clocks = <&clkc CLKID_CPUB_CLK>;
clock-latency = <50000>;
+ #cooling-cells = <2>;
};
&ext_mdio {
@@ -378,6 +450,10 @@
};
};
+&mali {
+ #cooling-cells = <2>;
+};
+
&hdmi_tx {
status = "okay";
pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox