* [PATCH 2/2 v2] tracing: Document the stack trace algorithm in the comments
From: Steven Rostedt @ 2019-08-07 17:28 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
In-Reply-To: <20190807172826.352574408@goodmis.org>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
As the max stack tracer algorithm is not that easy to understand from the
code, add comments that explain the algorithm and mentions how
ARCH_RET_ADDR_AFTER_LOCAL_VARS affects it.
Link: http://lkml.kernel.org/r/20190806123455.487ac02b@gandalf.local.home
Suggested-by: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_stack.c | 98 ++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 40e4a88eea8f..f94a2fc567de 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -53,6 +53,104 @@ static void print_max_stack(void)
}
}
+/*
+ * The stack tracer looks for a maximum stack at each call from a function. It
+ * registers a callback from ftrace, and in that callback it examines the stack
+ * size. It determines the stack size from the variable passed in, which is the
+ * address of a local variable in the stack_trace_call() callback function.
+ * The stack size is calculated by the address of the local variable to the top
+ * of the current stack. If that size is smaller than the currently saved max
+ * stack size, nothing more is done.
+ *
+ * If the size of the stack is greater than the maximum recorded size, then the
+ * following algorithm takes place.
+ *
+ * For architectures (like x86) that store the function's return address before
+ * saving the function's local variables, the stack will look something like
+ * this:
+ *
+ * [ top of stack ]
+ * 0: sys call entry frame
+ * 10: return addr to entry code
+ * 11: start of sys_foo frame
+ * 20: return addr to sys_foo
+ * 21: start of kernel_func_bar frame
+ * 30: return addr to kernel_func_bar
+ * 31: [ do trace stack here ]
+ *
+ * The save_stack_trace() is called returning all the functions it finds in the
+ * current stack. Which would be (from the bottom of the stack to the top):
+ *
+ * return addr to kernel_func_bar
+ * return addr to sys_foo
+ * return addr to entry code
+ *
+ * Now to figure out how much each of these functions' local variable size is,
+ * a search of the stack is made to find these values. When a match is made, it
+ * is added to the stack_dump_trace[] array. The offset into the stack is saved
+ * in the stack_trace_index[] array. The above example would show:
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 30
+ * return addr to sys_foo | 20
+ * return addr to entry | 10
+ *
+ * The print_max_stack() function above, uses these values to print the size of
+ * each function's portion of the stack.
+ *
+ * for (i = 0; i < nr_entries; i++) {
+ * size = i == nr_entries - 1 ? stack_trace_index[i] :
+ * stack_trace_index[i] - stack_trace_index[i+1]
+ * print "%d %d %d %s\n", i, stack_trace_index[i], size, stack_dump_trace[i]);
+ * }
+ *
+ * The above shows
+ *
+ * depth size location
+ * ----- ---- --------
+ * 0 30 10 kernel_func_bar
+ * 1 20 10 sys_foo
+ * 2 10 10 entry code
+ *
+ * Now for architectures that might save the return address after the functions
+ * local variables (saving the link register before calling nested functions),
+ * this will cause the stack to look a little different:
+ *
+ * [ top of stack ]
+ * 0: sys call entry frame
+ * 10: start of sys_foo_frame
+ * 19: return addr to entry code << lr saved before calling kernel_func_bar
+ * 20: start of kernel_func_bar frame
+ * 29: return addr to sys_foo_frame << lr saved before calling next function
+ * 30: [ do trace stack here ]
+ *
+ * Although the functions returned by save_stack_trace() may be the same, the
+ * placement in the stack will be different. Using the same algorithm as above
+ * would yield:
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 30
+ * return addr to sys_foo | 29
+ * return addr to entry | 19
+ *
+ * Where the mapping is off by one:
+ *
+ * kernel_func_bar stack frame size is 29 - 19 not 30 - 29!
+ *
+ * To fix this, if the architecture sets ARCH_RET_ADDR_AFTER_LOCAL_VARS the
+ * values in stack_trace_index[] are shifted by one to and the number of
+ * stack trace entries is decremented by one.
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 29
+ * return addr to sys_foo | 19
+ *
+ * Although the entry function is not displayed, the first function (sys_foo)
+ * will still include the stack size of it.
+ */
static void check_stack(unsigned long ip, unsigned long *stack)
{
unsigned long this_size, flags; unsigned long *p, *top, *start;
--
2.20.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 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data
From: Steven Rostedt @ 2019-08-07 17:28 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
In-Reply-To: <20190807172826.352574408@goodmis.org>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.
Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.
Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.
Link: 20190802094103.163576-1-jiping.ma2@windriver.com
Reported-by: Jiping Ma <jiping.ma2@windriver.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
kernel/trace/trace_stack.c | 14 ++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..961e98618db4 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -14,6 +14,19 @@
#define MCOUNT_ADDR ((unsigned long)_mcount)
#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
+/*
+ * Currently, gcc tends to save the link register after the local variables
+ * on the stack. This causes the max stack tracer to report the function
+ * frame sizes for the wrong functions. By defining
+ * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
+ * to find the return address on the stack after the local variables have
+ * been set up.
+ *
+ * Note, this may change in the future, and we will need to deal with that
+ * if it were to happen.
+ */
+#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
+
#ifndef __ASSEMBLY__
#include <linux/compat.h>
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..40e4a88eea8f 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
i++;
}
+#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
+ /*
+ * Some archs will store the link register before calling
+ * nested functions. This means the saved return address
+ * comes after the local storage, and we need to shift
+ * for that.
+ */
+ if (x > 1) {
+ memmove(&stack_trace_index[0], &stack_trace_index[1],
+ sizeof(stack_trace_index[0]) * (x - 1));
+ x--;
+ }
+#endif
+
stack_trace_nr_entries = x;
if (task_stack_end_corrupted(current)) {
--
2.20.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 0/2 v2] tracing/arm: Fix the stack tracer when LR is saved after local storage
From: Steven Rostedt @ 2019-08-07 17:28 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
As arm64 saves the link register after a function's local variables are
stored, it causes the max stack tracer to be off by one in its output
of which function has the bloated stack frame.
The first patch fixes this by creating a ARCH_RET_ADDR_BEFORE_LOCAL_VARS
define that an achitecture (arm64) may set in asm/ftrace.h, and this
will cause the stack tracer to make the shift.
As it has been proven that the stack tracer isn't the most trivial
algorithm to understand by staring at the code, the second patch adds
comments to the code to explain the algorithm with and without the
ARCH_RET_ADDR_BEFORE_LOCAL_VARS.
Hmm, should this be sent to stable (and for inclusion now?)
-- Steve
Changes since v1:
- Fixed wrong value in stack_trace_index[] array in comment
- Added a comment about gcc currently saves the LR after local variables,
but there's no guarantee that it will be like that in the future.
(Notified of this by Mark Rutland)
Steven Rostedt (VMware) (2):
tracing/arm64: Have max stack tracer handle the case of return address after data
tracing: Document the stack trace algorithm in the comments
----
arch/arm64/include/asm/ftrace.h | 13 +++++
kernel/trace/trace_stack.c | 112 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 125 insertions(+)
_______________________________________________
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 0/2] tracing/arm: Fix the stack tracer when LR is saved after local storage
From: Steven Rostedt @ 2019-08-07 17:20 UTC (permalink / raw)
To: Mark Rutland
Cc: Jiping Ma, catalin.marinas, will.deacon, linux-kernel, mingo,
Joel Fernandes, linux-arm-kernel
In-Reply-To: <20190807170814.GA45351@lakrids.cambridge.arm.com>
On Wed, 7 Aug 2019 18:08:14 +0100
Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Steve,
>
> On Wed, Aug 07, 2019 at 12:34:01PM -0400, Steven Rostedt wrote:
> > As arm64 saves the link register after a function's local variables are
> > stored, it causes the max stack tracer to be off by one in its output
> > of which function has the bloated stack frame.
>
> For reference, it's a bit more complex than that. :/
Yeah, I know it is. ;-)
>
> Our procedure call standard (the AAPCS) says that the frame record may
> be placed anywhere within a stackframe, so we don't have a guarantee as
> to where the saved lr will fall w.r.t local variables.
Yep.
>
> Today, GCC happens to create the stack frame by creating the stack
> record, so the LR is saved at a lower addresss than the local variables.
Which is what breaks the current algorithm (without this update).
>
> However, I am aware that there are reasons why a compiler may choose to
> place the frame record at a different locations, e.g. using pointer
> authentication to provide an implicit stack canary, so this could change
> in future, or potentially differ across functions.
>
> Maybe that's a bridge we'll have to cross in future.
OK, how about I update the change log and add a comment that states
that this can change. But even if it does, it wont break anything but
show the wrong stack size, which is usually only important for us
kernel developers anyway ;-)
Let me send a v2.
-- Steve
_______________________________________________
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 v19 00/15] arm64: untag user pointers passed to the kernel
From: Andrey Konovalov @ 2019-08-07 17:17 UTC (permalink / raw)
To: Will Deacon, Andrew Morton
Cc: Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy,
Catalin Marinas, Will Deacon, dri-devel, Kostya Serebryany,
Khalid Aziz, Lee Smith, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Dave Martin, Evgeniy Stepanov, linux-media, Kees Cook,
Ruben Ayrapetyan, Kevin Brodsky, Alex Williamson,
Mauro Carvalho Chehab, Linux ARM, Linux Memory Management List,
Greg Kroah-Hartman, Felix Kuehling, LKML, Jens Wiklander,
Ramana Radhakrishnan, Alexander Deucher, enh, Robin Murphy,
Yishai Hadas, Luc Van Oostenryck
In-Reply-To: <20190806171335.4dzjex5asoertaob@willie-the-truck>
On Tue, Aug 6, 2019 at 7:13 PM Will Deacon <will@kernel.org> wrote:
>
> On Wed, Jul 24, 2019 at 03:20:59PM +0100, Will Deacon wrote:
> > On Wed, Jul 24, 2019 at 04:16:49PM +0200, Andrey Konovalov wrote:
> > > On Wed, Jul 24, 2019 at 4:02 PM Will Deacon <will@kernel.org> wrote:
> > > > On Tue, Jul 23, 2019 at 08:03:29PM +0200, Andrey Konovalov wrote:
> > > > > Should this go through the mm or the arm tree?
> > > >
> > > > I would certainly prefer to take at least the arm64 bits via the arm64 tree
> > > > (i.e. patches 1, 2 and 15). We also need a Documentation patch describing
> > > > the new ABI.
> > >
> > > Sounds good! Should I post those patches together with the
> > > Documentation patches from Vincenzo as a separate patchset?
> >
> > Yes, please (although as you say below, we need a new version of those
> > patches from Vincenzo to address the feedback on v5). The other thing I
> > should say is that I'd be happy to queue the other patches in the series
> > too, but some of them are missing acks from the relevant maintainers (e.g.
> > the mm/ and fs/ changes).
>
> Ok, I've queued patches 1, 2, and 15 on a stable branch here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/tbi
>
> which should find its way into -next shortly via our for-next/core branch.
> If you want to make changes, please send additional patches on top.
>
> This is targetting 5.4, but I will drop it before the merge window if
> we don't have both of the following in place:
>
> * Updated ABI documentation with Acks from Catalin and Kevin
Catalin has posted a new version today.
> * The other patches in the series either Acked (so I can pick them up)
> or queued via some other tree(s) for 5.4.
So we have the following patches in this series:
1. arm64: untag user pointers in access_ok and __uaccess_mask_ptr
2. arm64: Introduce prctl() options to control the tagged user addresses ABI
3. lib: untag user pointers in strn*_user
4. mm: untag user pointers passed to memory syscalls
5. mm: untag user pointers in mm/gup.c
6. mm: untag user pointers in get_vaddr_frames
7. fs/namespace: untag user pointers in copy_mount_options
8. userfaultfd: untag user pointers
9. drm/amdgpu: untag user pointers
10. drm/radeon: untag user pointers in radeon_gem_userptr_ioctl
11. IB/mlx4: untag user pointers in mlx4_get_umem_mr
12. media/v4l2-core: untag user pointers in videobuf_dma_contig_user_get
13. tee/shm: untag user pointers in tee_shm_register
14. vfio/type1: untag user pointers in vaddr_get_pfn
15. selftests, arm64: add a selftest for passing tagged pointers to kernel
1, 2 and 15 have been picked by Will.
11 has been picked up by Jason.
9, 10, 12, 13 and 14 have acks from their subsystem maintainers.
3 touches generic lib code, I'm not sure if there's a dedicated
maintainer for that.
The ones that are left are the mm ones: 4, 5, 6, 7 and 8.
Andrew, could you take a look and give your Acked-by or pick them up directly?
>
> Make sense?
>
> Cheers,
>
> Will
Thanks!
_______________________________________________
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 0/2] tracing/arm: Fix the stack tracer when LR is saved after local storage
From: Mark Rutland @ 2019-08-07 17:08 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jiping Ma, catalin.marinas, will.deacon, linux-kernel, mingo,
Joel Fernandes, linux-arm-kernel
In-Reply-To: <20190807163401.570339297@goodmis.org>
Hi Steve,
On Wed, Aug 07, 2019 at 12:34:01PM -0400, Steven Rostedt wrote:
> As arm64 saves the link register after a function's local variables are
> stored, it causes the max stack tracer to be off by one in its output
> of which function has the bloated stack frame.
For reference, it's a bit more complex than that. :/
Our procedure call standard (the AAPCS) says that the frame record may
be placed anywhere within a stackframe, so we don't have a guarantee as
to where the saved lr will fall w.r.t local variables.
Today, GCC happens to create the stack frame by creating the stack
record, so the LR is saved at a lower addresss than the local variables.
However, I am aware that there are reasons why a compiler may choose to
place the frame record at a different locations, e.g. using pointer
authentication to provide an implicit stack canary, so this could change
in future, or potentially differ across functions.
Maybe that's a bridge we'll have to cross in future.
Thanks,
Mark.
>
> The first patch fixes this by creating a ARCH_RET_ADDR_BEFORE_LOCAL_VARS
> define that an achitecture (arm64) may set in asm/ftrace.h, and this
> will cause the stack tracer to make the shift.
>
> As it has been proven that the stack tracer isn't the most trivial
> algorithm to understand by staring at the code, the second patch adds
> comments to the code to explain the algorithm with and without the
> ARCH_RET_ADDR_BEFORE_LOCAL_VARS.
>
> Hmm, should this be sent to stable (and for inclusion now?)
>
> -- Steve
>
> Steven Rostedt (VMware) (2):
> tracing/arm64: Have max stack tracer handle the case of return address after data
> tracing: Document the stack trace algorithm in the comments
>
> ----
> arch/arm64/include/asm/ftrace.h | 1 +
> kernel/trace/trace_stack.c | 112 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 113 insertions(+)
_______________________________________________
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] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors
From: Sudeep Holla @ 2019-08-07 17:08 UTC (permalink / raw)
To: David Laight
Cc: Philipp Zabel, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <4102ce79ef7a4f5ba819663d072bccc8@AcuMS.aculab.com>
On Wed, Aug 07, 2019 at 03:18:59PM +0000, David Laight wrote:
> From: Sudeep Holla
> > Sent: 07 August 2019 14:01
> >
> > Instead of type-casting the {tx,rx}.buf all over the place while
> > accessing them to read/write __le32 from/to the firmware, let's use
> > the nice existing {get,put}_unaligned_le32 accessors to hide all the
> > type cast ugliness.
>
> Why the 'unaligned' accessors?
>
Since the firmware run in LE, we do byte-swapping anyways.
> > - *(__le32 *)t->tx.buf = cpu_to_le32(id);
> > + put_unaligned_le32(id, t->tx.buf);
>
If you look at the generic definition for put_unaligned_le32, it's
exactly the same as what I am replacing with the call. So nothing
changes IIUC. In fact, I see that all the helper in unaligned/access_ok.h
just do the byte-swapping.
> These will be expensive if the cpu doesn't support them.
The SCMI is currently used only on ARM platforms which have
HAVE_EFFICIENT_UNALIGNED_ACCESS defined.
--
Regards,
Sudeep
_______________________________________________
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/6] tty: serial: Add linflexuart driver for S32V234
From: Stefan-gabriel Mirea @ 2019-08-07 17:05 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, corbet@lwn.net,
catalin.marinas@arm.com, linux-doc@vger.kernel.org,
Larisa Ileana Grigore, linux-kernel@vger.kernel.org, Leo Li,
Cosmin Stefan Stoica, robh+dt@kernel.org,
linux-serial@vger.kernel.org, jslaby@suse.com,
shawnguo@kernel.org, will@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190807165646.GA6584@kroah.com>
On 8/7/2019 7:56 PM, gregkh@linuxfoundation.org wrote:
> On Wed, Aug 07, 2019 at 04:42:17PM +0000, Stefan-gabriel Mirea wrote:
>> On 8/6/2019 9:40 PM, gregkh@linuxfoundation.org wrote:
>>> On Tue, Aug 06, 2019 at 05:11:17PM +0000, Stefan-gabriel Mirea wrote:
<snip>
>>>> Other than that, I do not see anything wrong with the addition of a
>>>> define in serial_core.h for this purpose (which is also what most of the
>>>> serial drivers do, including amba-pl011.c, mentioned in
>>>> Documentation/driver-api/serial/driver.rst as providing the reference
>>>> implementation), so please be more specific.
>>>
>>> I am getting tired of dealing with merge issues with that list, and no
>>> one seems to be able to find where they are really needed for userspace,
>>> especially for new devices. What happens if you do not have use it?
>>
>> I see. If I drop its usage completely and leave 'type' from the
>> uart_port as 0, uart_port_startup() will fail when finding that
>> uport->type == PORT_UNKNOWN at [1] (there may be other effects as well,
>> e.g. due to the check in uart_configure_port[2]).
>>
>> So I suppose that I need to define some nonzero 'PORT_KNOWN' macro in
>> the driver and use that one internally for 'type'. Is my understanding
>> correct? Will there be any problems if I define it to a positive integer
>> which is already assigned to another driver, according to serial_core.h?
>
> Ugh, ok, that's messy, nevermind. Keep the #define in there, I will try
> to figure out how to move all of these at once sometime in the future...
>
> sorry for the noise.
No problem, thank you for your time.
Regards,
Stefan
_______________________________________________
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/6] tty: serial: Add linflexuart driver for S32V234
From: gregkh @ 2019-08-07 16:56 UTC (permalink / raw)
To: Stefan-gabriel Mirea
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, corbet@lwn.net,
catalin.marinas@arm.com, linux-doc@vger.kernel.org,
Larisa Ileana Grigore, linux-kernel@vger.kernel.org, Leo Li,
Cosmin Stefan Stoica, robh+dt@kernel.org,
linux-serial@vger.kernel.org, jslaby@suse.com,
shawnguo@kernel.org, will@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR0402MB2863C4406C06B0BDA3581822DFD40@VI1PR0402MB2863.eurprd04.prod.outlook.com>
On Wed, Aug 07, 2019 at 04:42:17PM +0000, Stefan-gabriel Mirea wrote:
> On 8/6/2019 9:40 PM, gregkh@linuxfoundation.org wrote:
> > On Tue, Aug 06, 2019 at 05:11:17PM +0000, Stefan-gabriel Mirea wrote:
> >> On 8/5/2019 6:31 PM, gregkh@linuxfoundation.org wrote:
> >>> On Fri, Aug 02, 2019 at 07:47:23PM +0000, Stefan-gabriel Mirea wrote:
> >>>>
> >>>> +/* Freescale Linflex UART */
> >>>> +#define PORT_LINFLEXUART 121
> >>>
> >>> Do you really need this modified?
> >>
> >> Hello Greg,
> >>
> >> This macro is meant to be assigned to port->type in the config_port
> >> method from uart_ops, in order for verify_port to know if the received
> >> serial_struct structure was really targeted for a LINFlex port. It
> >> needs to be defined outside, to avoid "collisions" with other drivers.
> >
> > Yes, I know what it goes to, but does anyone in userspace actually use
> > it?
>
> No, we do not use it from userspace, but kept the pattern only for
> conformance.
>
> >> Other than that, I do not see anything wrong with the addition of a
> >> define in serial_core.h for this purpose (which is also what most of the
> >> serial drivers do, including amba-pl011.c, mentioned in
> >> Documentation/driver-api/serial/driver.rst as providing the reference
> >> implementation), so please be more specific.
> >
> > I am getting tired of dealing with merge issues with that list, and no
> > one seems to be able to find where they are really needed for userspace,
> > especially for new devices. What happens if you do not have use it?
>
> I see. If I drop its usage completely and leave 'type' from the
> uart_port as 0, uart_port_startup() will fail when finding that
> uport->type == PORT_UNKNOWN at [1] (there may be other effects as well,
> e.g. due to the check in uart_configure_port[2]).
>
> So I suppose that I need to define some nonzero 'PORT_KNOWN' macro in
> the driver and use that one internally for 'type'. Is my understanding
> correct? Will there be any problems if I define it to a positive integer
> which is already assigned to another driver, according to serial_core.h?
Ugh, ok, that's messy, nevermind. Keep the #define in there, I will try
to figure out how to move all of these at once sometime in the future...
sorry for the noise.
greg k-h
_______________________________________________
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] arm64: Clarify when cpu_enable() is called
From: Mark Brown @ 2019-08-07 16:51 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, linux-arm-kernel, suzuki.poulose
In-Reply-To: <20190807160107.fpanxo4iimhg743c@willie-the-truck>
[-- Attachment #1.1: Type: text/plain, Size: 2016 bytes --]
On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote:
> On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote:
> > - * Take the appropriate actions to enable this capability for this CPU.
> > - * For each successfully booted CPU, this method is called for each
> > - * globally detected capability.
> > + * Take the appropriate actions to configure this capability for this
> > + * CPU. This will be called on all CPUs in the system if the
> > + * capability is detected anywhere in the system.
> That's not quite right though either, is it? We need to take into account
> the scope of the capability/erratum as well, since we don't /always/ call
> this function for everybody.
I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we
match the feature on all CPUs so we could see the feature on some CPUs
but not detect it as we're requiring a match on all? Possibly the above
should be "detected and enabled" rather than just "detected"? I can see
how that might not be 100% clear, I was thinking of detection as passing
all the match requirements including cross-CPU requirements but that
could be more explicit. Perhaps something like:
If this is called for any CPU in the system then it will be
called for all of them.
might cover it?
> Suzuki, are there any cases where ->cpu_enable() may be called on a CPU
> without the feature outside of ARM64_CPUCAP_LOCAL_CPU_ERRATUM or
> ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE?
There's at least ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, that was what
caused me to notice what was happening (and get confused about why
cpu_enable() was being called on non-matching CPUs).
I don't see where we limit where cpu_enable() is called after we start
calling it. When we're looping through in cpu_enable_non_boot_scope()
we skip SCOPE_BOOT_CPU but those get cpu_enable() called in
enable_cpu_capabilites() or verify_local_cpu_capabilities() depending on
if it's the boot CPU or not. It's possible I'm missing something
though.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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/2] drm: add cache support for arm64
From: Mark Rutland @ 2019-08-07 16:49 UTC (permalink / raw)
To: Rob Clark
Cc: Sean Paul, Maxime Ripard, Catalin Marinas, Maarten Lankhorst,
LKML, dri-devel, David Airlie, Rob Clark, linux-arm-kernel,
Daniel Vetter, Greg Kroah-Hartman, Thomas Gleixner, Will Deacon,
Christoph Hellwig, Allison Randal
In-Reply-To: <CAJs_Fx5xU2-dn3iOVqWTzAjpTaQ8BBNP_Gn_iMc-eJpOX+iXoQ@mail.gmail.com>
On Wed, Aug 07, 2019 at 09:15:54AM -0700, Rob Clark wrote:
> On Wed, Aug 7, 2019 at 5:38 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, Aug 06, 2019 at 09:31:55AM -0700, Rob Clark wrote:
> > > On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
> > > > > On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig <hch@lst.de> wrote:
> > > > > >
> > > > > > This goes in the wrong direction. drm_cflush_* are a bad API we need to
> > > > > > get rid of, not add use of it. The reason for that is two-fold:
> > > > > >
> > > > > > a) it doesn't address how cache maintaince actually works in most
> > > > > > platforms. When talking about a cache we three fundamental operations:
> > > > > >
> > > > > > 1) write back - this writes the content of the cache back to the
> > > > > > backing memory
> > > > > > 2) invalidate - this remove the content of the cache
> > > > > > 3) write back + invalidate - do both of the above
> > > > >
> > > > > Agreed that drm_cflush_* isn't a great API. In this particular case
> > > > > (IIUC), I need wb+inv so that there aren't dirty cache lines that drop
> > > > > out to memory later, and so that I don't get a cache hit on
> > > > > uncached/wc mmap'ing.
> > > >
> > > > Is there a cacheable alias lying around (e.g. the linear map), or are
> > > > these addresses only mapped uncached/wc?
> > > >
> > > > If there's a cacheable alias, performing an invalidate isn't sufficient,
> > > > since a CPU can allocate a new (clean) entry at any point in time (e.g.
> > > > as a result of prefetching or arbitrary speculation).
> > >
> > > I *believe* that there are not alias mappings (that I don't control
> > > myself) for pages coming from
> > > shmem_file_setup()/shmem_read_mapping_page()..
> >
> > AFAICT, that's regular anonymous memory, so there will be a cacheable
> > alias in the linear/direct map.
>
> tbh, I'm not 100% sure whether there is a cacheable alias, or whether
> any potential linear map is torn down.
I'm fairly confident that the linear/direct map cacheable alias is not
torn down when pages are allocated. The gneeric page allocation code
doesn't do so, and I see nothing the shmem code to do so.
For arm64, we can tear down portions of the linear map, but that has to
be done explicitly, and this is only possible when using rodata_full. If
not using rodata_full, it is not possible to dynamically tear down the
cacheable alias.
> My understanding is that a cacheable alias is "ok", with some
> caveats.. ie. that the cacheable alias is not accessed.
Unfortunately, that is not true. You'll often get away with it in
practice, but that's a matter of probability rather than a guarantee.
You cannot prevent a CPU from accessing a VA arbitrarily (e.g. as the
result of wild speculation). The ARM ARM (ARM DDI 0487E.a) points this
out explicitly:
| Entries for addresses that are Normal Cacheable can be allocated to
| the cache at any time, and so the cache invalidate instruction cannot
| ensure that the address is not present in a cache.
... along with:
| Caches introduce a number of potential problems, mainly because:
|
| * Memory accesses can occur at times other than when the programmer
| would expect them.
;)
> I'm not entirely sure about pre-fetch from access to adjacent pages.
> We have been using shmem as a source for pages since the beginning,
> and I haven't seen it cause any problems in the last 6 years. (This
> is limited to armv7 and armv8, I'm not really sure what would happen
> on armv6, but that is a combo I don't have to care about.)
Over time, CPUs get more aggressive w.r.t. prefetching and speculation,
so having not seen such issues in the past does not imply we won't in
future.
Anecdotally, we had an issue a few years ago that we were only able to
reproduce under heavy stress testing. A CPU would make speculative
instruction fetches from a read-destructive MMIO register, despite the
PC never going near the corresponding VA, and despite that code having
(apparently) caused no issue for years before that.
See commit:
b6ccb9803e90c16b ("ARM: 7954/1: mm: remove remaining domain support from ARMv6")
... which unfortunately lacks the full war story.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v3 2/5] ASoC: SOF: imx: Add i.MX8 HW support
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190807164258.8306-1-daniel.baluta@nxp.com>
Add support for the audio DSP hardware found on NXP i.MX8 platform.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
sound/soc/sof/Kconfig | 1 +
sound/soc/sof/Makefile | 1 +
sound/soc/sof/imx/Kconfig | 22 +++
sound/soc/sof/imx/Makefile | 4 +
sound/soc/sof/imx/imx8.c | 394 +++++++++++++++++++++++++++++++++++++
5 files changed, 422 insertions(+)
create mode 100644 sound/soc/sof/imx/Kconfig
create mode 100644 sound/soc/sof/imx/Makefile
create mode 100644 sound/soc/sof/imx/imx8.c
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 73c455dacab5..cc592bcadae7 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -181,6 +181,7 @@ config SND_SOC_SOF_PROBE_WORK_QUEUE
When selected, the probe is handled in two steps, for example to
avoid lockdeps if request_module is used in the probe.
+source "sound/soc/sof/imx/Kconfig"
source "sound/soc/sof/intel/Kconfig"
source "sound/soc/sof/xtensa/Kconfig"
diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
index f605a02257e7..a37dbfabfa3b 100644
--- a/sound/soc/sof/Makefile
+++ b/sound/soc/sof/Makefile
@@ -20,4 +20,5 @@ obj-$(CONFIG_SND_SOC_SOF_OF) += sof-of-dev.o
obj-$(CONFIG_SND_SOC_SOF_PCI) += sof-pci-dev.o
obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
+obj-$(CONFIG_SND_SOC_SOF_IMX_TOPLEVEL) += imx/
obj-$(CONFIG_SND_SOC_SOF_XTENSA) += xtensa/
diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
new file mode 100644
index 000000000000..fd73d8402dbf
--- /dev/null
+++ b/sound/soc/sof/imx/Kconfig
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+
+config SND_SOC_SOF_IMX_TOPLEVEL
+ bool "SOF support for NXP i.MX audio DSPs"
+ depends on ARM64 && SND_SOC_SOF_OF || COMPILE_TEST
+ help
+ This adds support for Sound Open Firmware for NXP i.MX platforms.
+ Say Y if you have such a device.
+ If unsure select "N".
+
+if SND_SOC_SOF_IMX_TOPLEVEL
+
+config SND_SOC_SOF_IMX8
+ tristate "SOF support for i.MX8"
+ depends on IMX_SCU
+ depends on IMX_DSP
+ help
+ This adds support for Sound Open Firmware for NXP i.MX8 platforms
+ Say Y if you have such a device.
+ If unsure select "N".
+
+endif ## SND_SOC_SOF_IMX_IMX_TOPLEVEL
diff --git a/sound/soc/sof/imx/Makefile b/sound/soc/sof/imx/Makefile
new file mode 100644
index 000000000000..6ef908e8c807
--- /dev/null
+++ b/sound/soc/sof/imx/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+snd-sof-imx8-objs := imx8.o
+
+obj-$(CONFIG_SND_SOC_SOF_IMX8) += snd-sof-imx8.o
diff --git a/sound/soc/sof/imx/imx8.c b/sound/soc/sof/imx/imx8.c
new file mode 100644
index 000000000000..9869d783c31e
--- /dev/null
+++ b/sound/soc/sof/imx/imx8.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+//
+// Copyright 2019 NXP
+//
+// Author: Daniel Baluta <daniel.baluta@nxp.com>
+//
+// Hardware interface for audio DSP on i.MX8
+
+#include <linux/firmware.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pm_domain.h>
+
+#include <linux/module.h>
+#include <sound/sof.h>
+#include <sound/sof/xtensa.h>
+#include <linux/firmware/imx/ipc.h>
+#include <linux/firmware/imx/dsp.h>
+
+#include <linux/firmware/imx/svc/misc.h>
+#include <dt-bindings/firmware/imx/rsrc.h>
+#include "../ops.h"
+
+/* DSP memories */
+#define IRAM_OFFSET 0x10000
+#define IRAM_SIZE (2 * 1024)
+#define DRAM0_OFFSET 0x0
+#define DRAM0_SIZE (32 * 1024)
+#define DRAM1_OFFSET 0x8000
+#define DRAM1_SIZE (32 * 1024)
+#define SYSRAM_OFFSET 0x18000
+#define SYSRAM_SIZE (256 * 1024)
+#define SYSROM_OFFSET 0x58000
+#define SYSROM_SIZE (192 * 1024)
+
+#define RESET_VECTOR_VADDR 0x596f8000
+
+#define MBOX_OFFSET 0x800000
+#define MBOX_SIZE 0x1000
+
+struct imx8_priv {
+ struct device *dev;
+ struct snd_sof_dev *sdev;
+
+ /* DSP IPC handler */
+ struct imx_dsp_ipc *dsp_ipc;
+ struct platform_device *ipc_dev;
+
+ /* System Controller IPC handler */
+ struct imx_sc_ipc *sc_ipc;
+
+ /* Power domain handling */
+ int num_domains;
+ struct device **pd_dev;
+ struct device_link **link;
+
+};
+
+static void imx8_get_reply(struct snd_sof_dev *sdev)
+{
+ struct snd_sof_ipc_msg *msg = sdev->msg;
+ struct sof_ipc_reply reply;
+ int ret = 0;
+
+ if (!msg) {
+ dev_warn(sdev->dev, "unexpected ipc interrupt\n");
+ return;
+ }
+
+ /* get reply */
+ sof_mailbox_read(sdev, sdev->host_box.offset, &reply, sizeof(reply));
+
+ if (reply.error < 0) {
+ memcpy(msg->reply_data, &reply, sizeof(reply));
+ ret = reply.error;
+ } else {
+ /* reply has correct size? */
+ if (reply.hdr.size != msg->reply_size) {
+ dev_err(sdev->dev, "error: reply expected %zu got %u bytes\n",
+ msg->reply_size, reply.hdr.size);
+ ret = -EINVAL;
+ }
+
+ /* read the message */
+ if (msg->reply_size > 0)
+ sof_mailbox_read(sdev, sdev->host_box.offset,
+ msg->reply_data, msg->reply_size);
+ }
+
+ msg->reply_error = ret;
+}
+
+static int imx8_get_mailbox_offset(struct snd_sof_dev *sdev)
+{
+ return MBOX_OFFSET;
+}
+
+static int imx8_get_window_offset(struct snd_sof_dev *sdev, u32 id)
+{
+ return MBOX_OFFSET;
+}
+
+void imx8_dsp_handle_reply(struct imx_dsp_ipc *ipc)
+{
+ struct imx8_priv *priv = imx_dsp_get_data(ipc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->sdev->ipc_lock, flags);
+ imx8_get_reply(priv->sdev);
+ snd_sof_ipc_reply(priv->sdev, 0);
+ spin_unlock_irqrestore(&priv->sdev->ipc_lock, flags);
+}
+
+void imx8_dsp_handle_request(struct imx_dsp_ipc *ipc)
+{
+ struct imx8_priv *priv = imx_dsp_get_data(ipc);
+
+ snd_sof_ipc_msgs_rx(priv->sdev);
+}
+
+struct imx_dsp_ops dsp_ops = {
+ .handle_reply = imx8_dsp_handle_reply,
+ .handle_request = imx8_dsp_handle_request,
+};
+
+static int imx8_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg)
+{
+ struct imx8_priv *priv = (struct imx8_priv *)sdev->private;
+
+ sof_mailbox_write(sdev, sdev->host_box.offset, msg->msg_data,
+ msg->msg_size);
+ imx_dsp_ring_doorbell(priv->dsp_ipc, 0);
+
+ return 0;
+}
+
+/*
+ * DSP control.
+ */
+static int imx8_run(struct snd_sof_dev *sdev)
+{
+ int ret;
+ struct imx8_priv *dsp_priv = (struct imx8_priv *)sdev->private;
+
+ ret = imx_sc_misc_set_control(dsp_priv->sc_ipc, IMX_SC_R_DSP,
+ IMX_SC_C_OFS_SEL, 1);
+ if (ret < 0) {
+ dev_err(sdev->dev, "Error system address offset source select\n");
+ return ret;
+ }
+
+ ret = imx_sc_misc_set_control(dsp_priv->sc_ipc, IMX_SC_R_DSP,
+ IMX_SC_C_OFS_AUDIO, 0x80);
+ if (ret < 0) {
+ dev_err(sdev->dev, "Error system address offset of AUDIO\n");
+ return ret;
+ }
+
+ ret = imx_sc_misc_set_control(dsp_priv->sc_ipc, IMX_SC_R_DSP,
+ IMX_SC_C_OFS_PERIPH, 0x5A);
+ if (ret < 0) {
+ dev_err(sdev->dev, "Error system address offset of PERIPH %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = imx_sc_misc_set_control(dsp_priv->sc_ipc, IMX_SC_R_DSP,
+ IMX_SC_C_OFS_IRQ, 0x51);
+ if (ret < 0) {
+ dev_err(sdev->dev, "Error system address offset of IRQ\n");
+ return ret;
+ }
+
+ imx_sc_pm_cpu_start(dsp_priv->sc_ipc, IMX_SC_R_DSP, true,
+ RESET_VECTOR_VADDR);
+
+ return 0;
+}
+
+static int imx8_probe(struct snd_sof_dev *sdev)
+{
+ struct platform_device *pdev =
+ container_of(sdev->dev, struct platform_device, dev);
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *res_node;
+ struct resource *mmio;
+ struct imx8_priv *priv;
+ struct resource res;
+ u32 base, size;
+ int ret = 0;
+ int i;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ sdev->private = priv;
+ priv->dev = sdev->dev;
+ priv->sdev = sdev;
+
+ /* power up device associated power domains */
+ priv->num_domains = of_count_phandle_with_args(np, "power-domains",
+ "#power-domain-cells");
+ if (priv->num_domains < 0) {
+ dev_err(sdev->dev, "no power-domains property in %pOF\n", np);
+ return priv->num_domains;
+ }
+
+ priv->pd_dev = devm_kmalloc_array(&pdev->dev, priv->num_domains,
+ sizeof(*priv->pd_dev), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->link = devm_kmalloc_array(&pdev->dev, priv->num_domains,
+ sizeof(*priv->link), GFP_KERNEL);
+ if (!priv->link)
+ return -ENOMEM;
+
+ for (i = 0; i < priv->num_domains; i++) {
+ priv->pd_dev[i] = dev_pm_domain_attach_by_id(&pdev->dev, i);
+ if (IS_ERR(priv->pd_dev[i])) {
+ ret = PTR_ERR(priv->pd_dev[i]);
+ goto exit_unroll_pm;
+ }
+ priv->link[i] = device_link_add(&pdev->dev, priv->pd_dev[i],
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (IS_ERR(priv->link[i])) {
+ ret = PTR_ERR(priv->link[i]);
+ dev_pm_domain_detach(priv->pd_dev[i], false);
+ goto exit_unroll_pm;
+ }
+ }
+
+ ret = imx_scu_get_handle(&priv->sc_ipc);
+ if (ret) {
+ dev_err(sdev->dev, "Cannot obtain SCU handle (err = %d)\n",
+ ret);
+ goto exit_unroll_pm;
+ }
+
+ priv->ipc_dev = platform_device_register_data(sdev->dev, "imx-dsp",
+ PLATFORM_DEVID_NONE,
+ pdev, sizeof(*pdev));
+ if (IS_ERR(priv->ipc_dev)) {
+ ret = PTR_ERR(priv->ipc_dev);
+ goto exit_unroll_pm;
+ }
+
+ priv->dsp_ipc = dev_get_drvdata(&priv->ipc_dev->dev);
+ if (!priv->dsp_ipc) {
+ /* DSP IPC driver not probed yet, try later */
+ ret = -EPROBE_DEFER;
+ dev_err(sdev->dev, "Failed to get drvdata\n");
+ goto exit_pdev_unregister;
+ }
+
+ imx_dsp_set_data(priv->dsp_ipc, priv);
+ priv->dsp_ipc->ops = &dsp_ops;
+
+ /* DSP base */
+ mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mmio) {
+ base = mmio->start;
+ size = resource_size(mmio);
+ } else {
+ dev_err(sdev->dev, "error: failed to get DSP base at idx 0\n");
+ ret = -EINVAL;
+ goto exit_pdev_unregister;
+ }
+
+ sdev->bar[SOF_FW_BLK_TYPE_IRAM] = devm_ioremap(sdev->dev, base, size);
+ if (!sdev->bar[SOF_FW_BLK_TYPE_IRAM]) {
+ dev_err(sdev->dev, "failed to ioremap base 0x%x size 0x%x\n",
+ base, size);
+ ret = -ENODEV;
+ goto exit_pdev_unregister;
+ }
+ sdev->mmio_bar = SOF_FW_BLK_TYPE_IRAM;
+
+ res_node = of_parse_phandle(np, "memory-region", 0);
+ if (!res_node) {
+ dev_err(&pdev->dev, "failed to get memory region node\n");
+ ret = -ENODEV;
+ goto exit_pdev_unregister;
+ }
+
+ ret = of_address_to_resource(res_node, 0, &res);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to get reserved region address\n");
+ goto exit_pdev_unregister;
+ }
+
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap_wc(sdev->dev, res.start,
+ res.end - res.start +
+ 1);
+ if (IS_ERR(sdev->bar[SOF_FW_BLK_TYPE_SRAM])) {
+ dev_err(sdev->dev, "failed to ioremap mem 0x%x size 0x%x\n",
+ base, size);
+ ret = PTR_ERR(sdev->bar[SOF_FW_BLK_TYPE_SRAM]);
+ goto exit_pdev_unregister;
+ }
+ sdev->mailbox_bar = SOF_FW_BLK_TYPE_SRAM;
+
+ return 0;
+
+exit_pdev_unregister:
+ platform_device_unregister(priv->ipc_dev);
+exit_unroll_pm:
+ while (--i >= 0) {
+ device_link_del(priv->link[i]);
+ dev_pm_domain_detach(priv->pd_dev[i], false);
+ }
+
+ return ret;
+}
+
+static int imx8_remove(struct snd_sof_dev *sdev)
+{
+ int i;
+ struct imx8_priv *priv = (struct imx8_priv *)sdev->private;
+
+ platform_device_unregister(priv->ipc_dev);
+
+ for (i = 0; i < priv->num_domains; i++) {
+ device_link_del(priv->link[i]);
+ dev_pm_domain_detach(priv->pd_dev[i], false);
+ }
+
+ return 0;
+}
+
+/* on i.MX8 there is 1 to 1 match between type and BAR idx */
+int imx8_get_bar_index(struct snd_sof_dev *sdev, u32 type)
+{
+ return type;
+}
+
+void imx8_ipc_msg_data(struct snd_sof_dev *sdev,
+ struct snd_pcm_substream *substream,
+ void *p, size_t sz)
+{
+ sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
+}
+
+int imx8_ipc_pcm_params(struct snd_sof_dev *sdev,
+ struct snd_pcm_substream *substream,
+ const struct sof_ipc_pcm_params_reply *reply)
+{
+ return 0;
+}
+
+static struct snd_soc_dai_driver imx8_dai[] = {
+{
+ .name = "esai-port",
+},
+};
+
+/* i.MX8 ops */
+struct snd_sof_dsp_ops sof_imx8_ops = {
+ /* probe and remove */
+ .probe = imx8_probe,
+ .remove = imx8_remove,
+ /* DSP core boot */
+ .run = imx8_run,
+
+ /* Block IO */
+ .block_read = sof_block_read,
+ .block_write = sof_block_write,
+
+ /* ipc */
+ .send_msg = imx8_send_msg,
+ .fw_ready = sof_fw_ready,
+ .get_mailbox_offset = imx8_get_mailbox_offset,
+ .get_window_offset = imx8_get_window_offset,
+
+ .ipc_msg_data = imx8_ipc_msg_data,
+ .ipc_pcm_params = imx8_ipc_pcm_params,
+
+ /* module loading */
+ .load_module = snd_sof_parse_module_memcpy,
+ .get_bar_index = imx8_get_bar_index,
+ /* firmware loading */
+ .load_firmware = snd_sof_load_firmware_memcpy,
+
+ /* DAI drivers */
+ .drv = imx8_dai,
+ .num_drv = 1, /* we have only 1 ESAI interface on i.MX8 */
+};
+EXPORT_SYMBOL(sof_imx8_ops);
+
+MODULE_LICENSE("Dual BSD/GPL");
--
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 v3 5/5] dt-bindings: dsp: fsl: Add DSP core binding support
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190807164258.8306-1-daniel.baluta@nxp.com>
This describes the DSP device tree node.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/dsp/fsl,dsp.yaml | 88 +++++++++++++++++++
1 file changed, 88 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
new file mode 100644
index 000000000000..24b9fd64e3eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dsp/fsl,dsp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX8 DSP core
+
+maintainers:
+ - Daniel Baluta <daniel.baluta@nxp.com>
+
+description: |
+ Some boards from i.MX8 family contain a DSP core used for
+ advanced pre- and post- audio processing.
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx8qxp-dsp
+
+ reg:
+ description: Should contain register location and length
+
+ clocks:
+ items:
+ - description: ipg clock
+ - description: ocram clock
+ - description: core clock
+
+ clock-names:
+ items:
+ - const: ipg
+ - const: ocram
+ - const: core
+
+ power-domains:
+ description:
+ List of phandle and PM domain specifier as documented in
+ Documentation/devicetree/bindings/power/power_domain.txt
+ maxItems: 4
+
+ mboxes:
+ description:
+ List of <&phandle type channel> - 2 channels for TXDB, 2 channels for RXDB
+ (see mailbox/fsl,mu.txt)
+ maxItems: 4
+
+ mbox-names:
+ items:
+ - const: txdb0
+ - const: txdb1
+ - const: rxdb0
+ - const: rxdb1
+
+ memory-region:
+ description:
+ phandle to a node describing reserved memory (System RAM memory)
+ used by DSP (see bindings/reserved-memory/reserved-memory.txt)
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - power-domains
+ - mboxes
+ - mbox-names
+ - memory-region
+
+examples:
+ - |
+ #include <dt-bindings/firmware/imx/rsrc.h>
+ #include <dt-bindings/clock/imx8-clock.h>
+ dsp@596e8000 {
+ compatbile = "fsl,imx8qxp-dsp";
+ reg = <0x596e8000 0x88000>;
+ clocks = <&adma_lpcg IMX_ADMA_LPCG_DSP_IPG_CLK>,
+ <&adma_lpcg IMX_ADMA_LPCG_OCRAM_IPG_CLK>,
+ <&adma_lpcg IMX_ADMA_LPCG_DSP_CORE_CLK>;
+ clock-names = "ipg", "ocram", "core";
+ power-domains = <&pd IMX_SC_R_MU_13A>,
+ <&pd IMX_SC_R_MU_13B>,
+ <&pd IMX_SC_R_DSP>,
+ <&pd IMX_SC_R_DSP_RAM>;
+ mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
+ mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
+ };
--
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 v3 3/5] ASoC: SOF: topology: Add dummy support for i.MX8 DAIs
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190807164258.8306-1-daniel.baluta@nxp.com>
Add dummy support for SAI/ESAI digital audio interface
IPs found on i.MX8 boards.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
include/sound/sof/dai.h | 2 ++
include/uapi/sound/sof/tokens.h | 8 ++++++++
sound/soc/sof/topology.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 40 insertions(+)
diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h
index 3d174e20aa53..ec3b5c080537 100644
--- a/include/sound/sof/dai.h
+++ b/include/sound/sof/dai.h
@@ -50,6 +50,8 @@ enum sof_ipc_dai_type {
SOF_DAI_INTEL_DMIC, /**< Intel DMIC */
SOF_DAI_INTEL_HDA, /**< Intel HD/A */
SOF_DAI_INTEL_SOUNDWIRE, /**< Intel SoundWire */
+ SOF_DAI_IMX_SAI, /**< i.MX SAI */
+ SOF_DAI_IMX_ESAI, /**< i.MX ESAI */
};
/* general purpose DAI configuration */
diff --git a/include/uapi/sound/sof/tokens.h b/include/uapi/sound/sof/tokens.h
index 6435240cef13..8f996857fb24 100644
--- a/include/uapi/sound/sof/tokens.h
+++ b/include/uapi/sound/sof/tokens.h
@@ -106,4 +106,12 @@
/* for backward compatibility */
#define SOF_TKN_EFFECT_TYPE SOF_TKN_PROCESS_TYPE
+/* SAI */
+#define SOF_TKN_IMX_SAI_FIRST_TOKEN 1000
+/* TODO: Add SAI tokens */
+
+/* ESAI */
+#define SOF_TKN_IMX_ESAI_FIRST_TOKEN 1100
+/* TODO: Add ESAI tokens */
+
#endif
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 30b5638622dd..e3761657ecc0 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -346,6 +346,8 @@ static const struct sof_dai_types sof_dais[] = {
{"SSP", SOF_DAI_INTEL_SSP},
{"HDA", SOF_DAI_INTEL_HDA},
{"DMIC", SOF_DAI_INTEL_DMIC},
+ {"SAI", SOF_DAI_IMX_SAI},
+ {"ESAI", SOF_DAI_IMX_ESAI},
};
static enum sof_ipc_dai_type find_dai(const char *name)
@@ -2516,6 +2518,26 @@ static int sof_link_ssp_load(struct snd_soc_component *scomp, int index,
return ret;
}
+static int sof_link_sai_load(struct snd_soc_component *scomp, int index,
+ struct snd_soc_dai_link *link,
+ struct snd_soc_tplg_link_config *cfg,
+ struct snd_soc_tplg_hw_config *hw_config,
+ struct sof_ipc_dai_config *config)
+{
+ /*TODO: Add implementation */
+ return 0;
+}
+
+static int sof_link_esai_load(struct snd_soc_component *scomp, int index,
+ struct snd_soc_dai_link *link,
+ struct snd_soc_tplg_link_config *cfg,
+ struct snd_soc_tplg_hw_config *hw_config,
+ struct sof_ipc_dai_config *config)
+{
+ /*TODO: Add implementation */
+ return 0;
+}
+
static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
struct snd_soc_dai_link *link,
struct snd_soc_tplg_link_config *cfg,
@@ -2840,6 +2862,14 @@ static int sof_link_load(struct snd_soc_component *scomp, int index,
ret = sof_link_hda_load(scomp, index, link, cfg, hw_config,
&config);
break;
+ case SOF_DAI_IMX_SAI:
+ ret = sof_link_sai_load(scomp, index, link, cfg, hw_config,
+ &config);
+ break;
+ case SOF_DAI_IMX_ESAI:
+ ret = sof_link_esai_load(scomp, index, link, cfg, hw_config,
+ &config);
+ break;
default:
dev_err(sdev->dev, "error: invalid DAI type %d\n", config.type);
ret = -EINVAL;
--
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 v3 4/5] arm64: dts: imx8qxp: Add DSP DT node
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190807164258.8306-1-daniel.baluta@nxp.com>
This includes DSP reserved memory, ADMA DSP device and DSP MU
communication channels description.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 4 +++
arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 32 +++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
index bfdada2db176..19468058e6ae 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
@@ -230,3 +230,7 @@
>;
};
};
+
+&adma_dsp {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 05fa0b7f36bb..b6c408fb2b7f 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -113,6 +113,17 @@
interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
};
+ reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ dsp_reserved: dsp@92400000 {
+ reg = <0 0x92400000 0 0x2000000>;
+ no-map;
+ };
+ };
+
pmu {
compatible = "arm,armv8-pmuv3";
interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
@@ -204,6 +215,27 @@
#clock-cells = <1>;
};
+ adma_dsp: dsp@596e8000 {
+ compatible = "fsl,imx8qxp-dsp";
+ reg = <0x596e8000 0x88000>;
+ clocks = <&adma_lpcg IMX_ADMA_LPCG_DSP_IPG_CLK>,
+ <&adma_lpcg IMX_ADMA_LPCG_OCRAM_IPG_CLK>,
+ <&adma_lpcg IMX_ADMA_LPCG_DSP_CORE_CLK>;
+ clock-names = "ipg", "ocram", "core";
+ power-domains = <&pd IMX_SC_R_MU_13A>,
+ <&pd IMX_SC_R_MU_13B>,
+ <&pd IMX_SC_R_DSP>,
+ <&pd IMX_SC_R_DSP_RAM>;
+ mbox-names = "txdb0", "txdb1",
+ "rxdb0", "rxdb1";
+ mboxes = <&lsio_mu13 2 0>,
+ <&lsio_mu13 2 1>,
+ <&lsio_mu13 3 0>,
+ <&lsio_mu13 3 1>;
+ memory-region = <&dsp_reserved>;
+ status = "disabled";
+ };
+
adma_lpuart0: serial@5a060000 {
compatible = "fsl,imx8qxp-lpuart", "fsl,imx7ulp-lpuart";
reg = <0x5a060000 0x1000>;
--
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 v3 1/5] ASoC: SOF: Add OF DSP device support
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190807164258.8306-1-daniel.baluta@nxp.com>
Add support for device tree based SOF DSP devices.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
sound/soc/sof/Kconfig | 10 +++
sound/soc/sof/Makefile | 3 +
sound/soc/sof/sof-of-dev.c | 143 +++++++++++++++++++++++++++++++++++++
3 files changed, 156 insertions(+)
create mode 100644 sound/soc/sof/sof-of-dev.c
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 5b41628ad722..73c455dacab5 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -36,6 +36,16 @@ config SND_SOC_SOF_ACPI
Say Y if you need this option
If unsure select "N".
+config SND_SOC_SOF_OF
+ tristate "SOF OF enumeration support"
+ depends on OF || COMPILE_TEST
+ select SND_SOC_SOF
+ select SND_SOC_SOF_OPTIONS
+ help
+ This adds support for Device Tree enumeration. This option is
+ required to enable i.MX8 devices.
+ Say Y if you need this option. If unsure select "N".
+
config SND_SOC_SOF_OPTIONS
tristate
help
diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
index 33c3ded2b7e2..f605a02257e7 100644
--- a/sound/soc/sof/Makefile
+++ b/sound/soc/sof/Makefile
@@ -7,6 +7,8 @@ snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
snd-sof-pci-objs := sof-pci-dev.o
snd-sof-acpi-objs := sof-acpi-dev.o
+snd-sof-of-objs := sof-of-dev.o
+
snd-sof-nocodec-objs := nocodec.o
obj-$(CONFIG_SND_SOC_SOF) += snd-sof.o
@@ -14,6 +16,7 @@ obj-$(CONFIG_SND_SOC_SOF_NOCODEC) += snd-sof-nocodec.o
obj-$(CONFIG_SND_SOC_SOF_ACPI) += sof-acpi-dev.o
+obj-$(CONFIG_SND_SOC_SOF_OF) += sof-of-dev.o
obj-$(CONFIG_SND_SOC_SOF_PCI) += sof-pci-dev.o
obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
new file mode 100644
index 000000000000..28a9692974e5
--- /dev/null
+++ b/sound/soc/sof/sof-of-dev.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+//
+// Copyright 2019 NXP
+//
+// Author: Daniel Baluta <daniel.baluta@nxp.com>
+//
+
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <sound/sof.h>
+
+#include "ops.h"
+
+extern struct snd_sof_dsp_ops sof_imx8_ops;
+
+/* platform specific devices */
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_IMX8)
+static struct sof_dev_desc sof_of_imx8qxp_desc = {
+ .default_fw_path = "imx/sof",
+ .default_tplg_path = "imx/sof-tplg",
+ .nocodec_fw_filename = "sof-imx8.ri",
+ .nocodec_tplg_filename = "sof-imx8-nocodec.tplg",
+ .ops = &sof_imx8_ops,
+};
+#endif
+
+static const struct dev_pm_ops sof_of_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
+ SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
+ NULL)
+};
+
+static void sof_of_probe_complete(struct device *dev)
+{
+ /* allow runtime_pm */
+ pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_enable(dev);
+}
+
+static int sof_of_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const struct sof_dev_desc *desc;
+ /*TODO: create a generic snd_soc_xxx_mach */
+ struct snd_soc_acpi_mach *mach;
+ struct snd_sof_pdata *sof_pdata;
+ const struct snd_sof_dsp_ops *ops;
+ int ret;
+
+ dev_info(&pdev->dev, "DT DSP detected");
+
+ sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
+ if (!sof_pdata)
+ return -ENOMEM;
+
+ desc = device_get_match_data(dev);
+ if (!desc)
+ return -ENODEV;
+
+ /* get ops for platform */
+ ops = desc->ops;
+ if (!ops) {
+ dev_err(dev, "error: no matching DT descriptor ops\n");
+ return -ENODEV;
+ }
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)
+ /* force nocodec mode */
+ dev_warn(dev, "Force to use nocodec mode\n");
+ mach = devm_kzalloc(dev, sizeof(*mach), GFP_KERNEL);
+ if (!mach)
+ return -ENOMEM;
+ ret = sof_nocodec_setup(dev, sof_pdata, mach, desc, ops);
+ if (ret < 0)
+ return ret;
+#else
+ /* TODO: implement case where we actually have a codec */
+ return -ENODEV;
+#endif
+
+ if (mach)
+ mach->mach_params.platform = dev_name(dev);
+
+ sof_pdata->machine = mach;
+ sof_pdata->desc = desc;
+ sof_pdata->dev = &pdev->dev;
+ sof_pdata->platform = dev_name(dev);
+
+ /* TODO: read alternate fw and tplg filenames from DT */
+ sof_pdata->fw_filename_prefix = sof_pdata->desc->default_fw_path;
+ sof_pdata->tplg_filename_prefix = sof_pdata->desc->default_tplg_path;
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)
+ /* set callback to enable runtime_pm */
+ sof_pdata->sof_probe_complete = sof_of_probe_complete;
+#endif
+ /* call sof helper for DSP hardware probe */
+ ret = snd_sof_device_probe(dev, sof_pdata);
+ if (ret) {
+ dev_err(dev, "error: failed to probe DSP hardware\n");
+ return ret;
+ }
+
+#if !IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)
+ sof_of_probe_complete(dev);
+#endif
+
+ return ret;
+}
+
+static int sof_of_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+
+ /* call sof helper for DSP hardware remove */
+ snd_sof_device_remove(&pdev->dev);
+
+ return 0;
+}
+
+static const struct of_device_id sof_of_ids[] = {
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_IMX8)
+ { .compatible = "fsl,imx8qxp-dsp", .data = &sof_of_imx8qxp_desc},
+#endif
+ { }
+};
+MODULE_DEVICE_TABLE(of, sof_of_ids);
+
+/* DT driver definition */
+static struct platform_driver snd_sof_of_driver = {
+ .probe = sof_of_probe,
+ .remove = sof_of_remove,
+ .driver = {
+ .name = "sof-audio-of",
+ .pm = &sof_of_pm,
+ .of_match_table = sof_of_ids,
+ },
+};
+module_platform_driver(snd_sof_of_driver);
+
+MODULE_LICENSE("Dual BSD/GPL");
--
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 v3 0/5] Add DSP node for i.MX8QXP board to be used by DSP SOF driver
From: Daniel Baluta @ 2019-08-07 16:42 UTC (permalink / raw)
To: daniel.baluta, shawnguo
Cc: aisheng.dong, devicetree, peng.fan, anson.huang, shengjiu.wang,
linux-kernel, m.felsch, paul.olaru, robh+dt, linux-imx, kernel,
l.stach, pierre-louis.bossart, mark.rutland, leonard.crestez,
festevam, linux-arm-kernel, sound-open-firmware
i.MX 8QXP boards feature an Hifi4 DSP from Tensilica.
This patch series aims on adding the DT node describing the DSP,
but it also contains the Linux SOF DSP driver code that will use the DT node
for easier review.
Note that we switched to the new yaml format for bindings documentation.
The DSP will run SOF Firmware [1]. Patches 1,2,3 are adding support
for Linux DSP driver are already sent for review to SOF folks [2]
Ideally, patches 4/5 and 5/5 will go upstream through Shawn's tree
while 1-3/5 will go upstream via Pierre's tree -> ASoC tree.
Mind that SOF DSP support depends on IMX DSP communication protocol
up for review here: https://lkml.org/lkml/2019/8/1/260
Shawn, can you pick this up first?
Symbol dependencies are hopefully set correct so even if one of
the patches is not in a tree the compilation will not fail because
the symbols depending on that patches will not be selected.
[1] https://github.com/thesofproject/sof
[2] https://github.com/thesofproject/linux/pull/1048/commits
Daniel Baluta (5):
ASoC: SOF: Add OF DSP device support
ASoC: SOF: imx: Add i.MX8 HW support
ASoC: SOF: topology: Add dummy support for i.MX8 DAIs
arm64: dts: imx8qxp: Add DSP DT node
dt-bindings: dsp: fsl: Add DSP core binding support
.../devicetree/bindings/dsp/fsl,dsp.yaml | 88 ++++
arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 4 +
arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 32 ++
include/sound/sof/dai.h | 2 +
include/uapi/sound/sof/tokens.h | 8 +
sound/soc/sof/Kconfig | 11 +
sound/soc/sof/Makefile | 4 +
sound/soc/sof/imx/Kconfig | 22 +
sound/soc/sof/imx/Makefile | 4 +
sound/soc/sof/imx/imx8.c | 394 ++++++++++++++++++
sound/soc/sof/sof-of-dev.c | 143 +++++++
sound/soc/sof/topology.c | 30 ++
12 files changed, 742 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
create mode 100644 sound/soc/sof/imx/Kconfig
create mode 100644 sound/soc/sof/imx/Makefile
create mode 100644 sound/soc/sof/imx/imx8.c
create mode 100644 sound/soc/sof/sof-of-dev.c
--
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
* Re: [PATCH 5/6] tty: serial: Add linflexuart driver for S32V234
From: Stefan-gabriel Mirea @ 2019-08-07 16:42 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, corbet@lwn.net,
catalin.marinas@arm.com, linux-doc@vger.kernel.org,
Larisa Ileana Grigore, linux-kernel@vger.kernel.org, Leo Li,
Cosmin Stefan Stoica, robh+dt@kernel.org,
linux-serial@vger.kernel.org, jslaby@suse.com,
shawnguo@kernel.org, will@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190806184042.GA26041@kroah.com>
On 8/6/2019 9:40 PM, gregkh@linuxfoundation.org wrote:
> On Tue, Aug 06, 2019 at 05:11:17PM +0000, Stefan-gabriel Mirea wrote:
>> On 8/5/2019 6:31 PM, gregkh@linuxfoundation.org wrote:
>>> On Fri, Aug 02, 2019 at 07:47:23PM +0000, Stefan-gabriel Mirea wrote:
>>>>
>>>> +/* Freescale Linflex UART */
>>>> +#define PORT_LINFLEXUART 121
>>>
>>> Do you really need this modified?
>>
>> Hello Greg,
>>
>> This macro is meant to be assigned to port->type in the config_port
>> method from uart_ops, in order for verify_port to know if the received
>> serial_struct structure was really targeted for a LINFlex port. It
>> needs to be defined outside, to avoid "collisions" with other drivers.
>
> Yes, I know what it goes to, but does anyone in userspace actually use
> it?
No, we do not use it from userspace, but kept the pattern only for
conformance.
>> Other than that, I do not see anything wrong with the addition of a
>> define in serial_core.h for this purpose (which is also what most of the
>> serial drivers do, including amba-pl011.c, mentioned in
>> Documentation/driver-api/serial/driver.rst as providing the reference
>> implementation), so please be more specific.
>
> I am getting tired of dealing with merge issues with that list, and no
> one seems to be able to find where they are really needed for userspace,
> especially for new devices. What happens if you do not have use it?
I see. If I drop its usage completely and leave 'type' from the
uart_port as 0, uart_port_startup() will fail when finding that
uport->type == PORT_UNKNOWN at [1] (there may be other effects as well,
e.g. due to the check in uart_configure_port[2]).
So I suppose that I need to define some nonzero 'PORT_KNOWN' macro in
the driver and use that one internally for 'type'. Is my understanding
correct? Will there be any problems if I define it to a positive integer
which is already assigned to another driver, according to serial_core.h?
Regards,
Stefan
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_core.c?h=v5.3-rc1#n188
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_core.c?h=v5.3-rc1#n2348
_______________________________________________
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/2] tracing: Document the stack trace algorithm in the comments
From: Steven Rostedt @ 2019-08-07 16:40 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
In-Reply-To: <20190807163454.392141426@goodmis.org>
On Wed, 07 Aug 2019 12:34:03 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> + * To fix this, if the architecture sets ARCH_RET_ADDR_AFTER_LOCAL_VARS the
> + * values in stack_trace_index[] are shifted by one to and the number of
> + * stack trace entries is decremented by one.
> + *
> + * stack_dump_trace[] | stack_trace_index[]
> + * ------------------ + -------------------
> + * return addr to kernel_func_bar | 20
That should have been 29, not 20. I'll update it.
-- Steve
> + * return addr to sys_foo | 19
> + *
> + * Although the entry function is not displayed, the first function (sys_foo)
> + * will still include the stack size of it.
> + */
> static void check_stack(unsigned long ip, unsigned long *stack)
> {
> unsigned long this_size, flags; unsigned long *p, *top, *start;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/2] tracing: Document the stack trace algorithm in the comments
From: Steven Rostedt @ 2019-08-07 16:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
In-Reply-To: <20190807163401.570339297@goodmis.org>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
As the max stack tracer algorithm is not that easy to understand from the
code, add comments that explain the algorithm and mentions how
ARCH_RET_ADDR_AFTER_LOCAL_VARS affects it.
Link: http://lkml.kernel.org/r/20190806123455.487ac02b@gandalf.local.home
Suggested-by: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_stack.c | 98 ++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 40e4a88eea8f..7a9a62834af9 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -53,6 +53,104 @@ static void print_max_stack(void)
}
}
+/*
+ * The stack tracer looks for a maximum stack at each call from a function. It
+ * registers a callback from ftrace, and in that callback it examines the stack
+ * size. It determines the stack size from the variable passed in, which is the
+ * address of a local variable in the stack_trace_call() callback function.
+ * The stack size is calculated by the address of the local variable to the top
+ * of the current stack. If that size is smaller than the currently saved max
+ * stack size, nothing more is done.
+ *
+ * If the size of the stack is greater than the maximum recorded size, then the
+ * following algorithm takes place.
+ *
+ * For architectures (like x86) that store the function's return address before
+ * saving the function's local variables, the stack will look something like
+ * this:
+ *
+ * [ top of stack ]
+ * 0: sys call entry frame
+ * 10: return addr to entry code
+ * 11: start of sys_foo frame
+ * 20: return addr to sys_foo
+ * 21: start of kernel_func_bar frame
+ * 30: return addr to kernel_func_bar
+ * 31: [ do trace stack here ]
+ *
+ * The save_stack_trace() is called returning all the functions it finds in the
+ * current stack. Which would be (from the bottom of the stack to the top):
+ *
+ * return addr to kernel_func_bar
+ * return addr to sys_foo
+ * return addr to entry code
+ *
+ * Now to figure out how much each of these functions' local variable size is,
+ * a search of the stack is made to find these values. When a match is made, it
+ * is added to the stack_dump_trace[] array. The offset into the stack is saved
+ * in the stack_trace_index[] array. The above example would show:
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 30
+ * return addr to sys_foo | 20
+ * return addr to entry | 10
+ *
+ * The print_max_stack() function above, uses these values to print the size of
+ * each function's portion of the stack.
+ *
+ * for (i = 0; i < nr_entries; i++) {
+ * size = i == nr_entries - 1 ? stack_trace_index[i] :
+ * stack_trace_index[i] - stack_trace_index[i+1]
+ * print "%d %d %d %s\n", i, stack_trace_index[i], size, stack_dump_trace[i]);
+ * }
+ *
+ * The above shows
+ *
+ * depth size location
+ * ----- ---- --------
+ * 0 30 10 kernel_func_bar
+ * 1 20 10 sys_foo
+ * 2 10 10 entry code
+ *
+ * Now for architectures that might save the return address after the functions
+ * local variables (saving the link register before calling nested functions),
+ * this will cause the stack to look a little different:
+ *
+ * [ top of stack ]
+ * 0: sys call entry frame
+ * 10: start of sys_foo_frame
+ * 19: return addr to entry code << lr saved before calling kernel_func_bar
+ * 20: start of kernel_func_bar frame
+ * 29: return addr to sys_foo_frame << lr saved before calling next function
+ * 30: [ do trace stack here ]
+ *
+ * Although the functions returned by save_stack_trace() may be the same, the
+ * placement in the stack will be different. Using the same algorithm as above
+ * would yield:
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 30
+ * return addr to sys_foo | 29
+ * return addr to entry | 19
+ *
+ * Where the mapping is off by one:
+ *
+ * kernel_func_bar stack frame size is 29 - 19 not 30 - 29!
+ *
+ * To fix this, if the architecture sets ARCH_RET_ADDR_AFTER_LOCAL_VARS the
+ * values in stack_trace_index[] are shifted by one to and the number of
+ * stack trace entries is decremented by one.
+ *
+ * stack_dump_trace[] | stack_trace_index[]
+ * ------------------ + -------------------
+ * return addr to kernel_func_bar | 20
+ * return addr to sys_foo | 19
+ *
+ * Although the entry function is not displayed, the first function (sys_foo)
+ * will still include the stack size of it.
+ */
static void check_stack(unsigned long ip, unsigned long *stack)
{
unsigned long this_size, flags; unsigned long *p, *top, *start;
--
2.20.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 1/2] tracing/arm64: Have max stack tracer handle the case of return address after data
From: Steven Rostedt @ 2019-08-07 16:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
In-Reply-To: <20190807163401.570339297@goodmis.org>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.
Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.
Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.
Link: 20190802094103.163576-1-jiping.ma2@windriver.com
Reported-by: Jiping Ma <jiping.ma2@windriver.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
arch/arm64/include/asm/ftrace.h | 1 +
kernel/trace/trace_stack.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..13a4832cfb00 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -13,6 +13,7 @@
#define HAVE_FUNCTION_GRAPH_FP_TEST
#define MCOUNT_ADDR ((unsigned long)_mcount)
#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
+#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
#ifndef __ASSEMBLY__
#include <linux/compat.h>
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..40e4a88eea8f 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
i++;
}
+#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
+ /*
+ * Some archs will store the link register before calling
+ * nested functions. This means the saved return address
+ * comes after the local storage, and we need to shift
+ * for that.
+ */
+ if (x > 1) {
+ memmove(&stack_trace_index[0], &stack_trace_index[1],
+ sizeof(stack_trace_index[0]) * (x - 1));
+ x--;
+ }
+#endif
+
stack_trace_nr_entries = x;
if (task_stack_end_corrupted(current)) {
--
2.20.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 0/2] tracing/arm: Fix the stack tracer when LR is saved after local storage
From: Steven Rostedt @ 2019-08-07 16:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiping Ma, catalin.marinas, will.deacon, mingo, Joel Fernandes,
linux-arm-kernel
As arm64 saves the link register after a function's local variables are
stored, it causes the max stack tracer to be off by one in its output
of which function has the bloated stack frame.
The first patch fixes this by creating a ARCH_RET_ADDR_BEFORE_LOCAL_VARS
define that an achitecture (arm64) may set in asm/ftrace.h, and this
will cause the stack tracer to make the shift.
As it has been proven that the stack tracer isn't the most trivial
algorithm to understand by staring at the code, the second patch adds
comments to the code to explain the algorithm with and without the
ARCH_RET_ADDR_BEFORE_LOCAL_VARS.
Hmm, should this be sent to stable (and for inclusion now?)
-- Steve
Steven Rostedt (VMware) (2):
tracing/arm64: Have max stack tracer handle the case of return address after data
tracing: Document the stack trace algorithm in the comments
----
arch/arm64/include/asm/ftrace.h | 1 +
kernel/trace/trace_stack.c | 112 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 113 insertions(+)
_______________________________________________
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/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
From: Daniel Lezcano @ 2019-08-07 16:30 UTC (permalink / raw)
To: Lorenzo Pieralisi, linux-pm
Cc: Mark Rutland, Ulf Hansson, Catalin Marinas, Rafael J. Wysocki,
LKML, Sudeep Holla, Will Deacon, LAKML
In-Reply-To: <20190722153745.32446-5-lorenzo.pieralisi@arm.com>
On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> PSCI firmware is the standard power management control for
> all ARM64 based platforms and it is also deployed on some
> ARM 32 bit platforms to date.
>
> Idle state entry in PSCI is currently achieved by calling
> arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
> idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
> to relay the call into PSCI firmware if PSCI is the boot method.
>
> Given that PSCI is the standard idle entry method on ARM64 systems
> (which means that no other CPUidle driver are expected on ARM64
> platforms - so PSCI is already a generic idle driver), in order to
> simplify idle entry and code maintenance, it makes sense to have a PSCI
> specific idle driver so that idle code that it is currently living in
> drivers/firmware directory can be hoisted out of it and moved
> where it belongs, into a full-fledged PSCI driver, leaving PSCI code
> in drivers/firmware as a pure firmware interface, as it should be.
>
> Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
> which is left unselected, since it selection would clash with the
> generic ARM CPUidle driver that provides a PSCI based idle driver
> through the arm/arm64 arches back-ends CPU operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
Modulo Ulf and Sudeep comments,
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
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] arm64: Disable big endian builds with clang
From: Mark Brown @ 2019-08-07 16:19 UTC (permalink / raw)
To: Mark Rutland
Cc: Tri Vo, Catalin Marinas, Nick Desaulniers, clang-built-linux,
Matt Hart, Nathan Chancellor, Will Deacon, linux-arm-kernel
In-Reply-To: <20190807154314.GH54191@lakrids.cambridge.arm.com>
[-- Attachment #1.1: Type: text/plain, Size: 351 bytes --]
On Wed, Aug 07, 2019 at 04:43:14PM +0100, Mark Rutland wrote:
> That confirms what Robin suggested [1] from looking at the config: this
> is a little-endian kernel.
> The Image header flags the big-endian bit is 0, and it succcessfully
> boots with a little-endian rootfs; log below.
Right, I've confirmed this myself. Sorry about the noise here.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 0/6] hwspinlock: allow sharing of hwspinlocks
From: Suman Anna @ 2019-08-07 16:19 UTC (permalink / raw)
To: Fabien DESSENNE, Bjorn Andersson
Cc: Ohad Ben-Cohen, Mark Rutland, Alexandre TORGUE, Jonathan Corbet,
linux-doc@vger.kernel.org, linux-remoteproc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Rob Herring, Maxime Coquelin,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Benjamin GAIGNARD
In-Reply-To: <02329102-5571-c6c1-b78c-693747133f0e@st.com>
Hi Fabien,
On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
> Hi
>
> On 06/08/2019 11:30 PM, Suman Anna wrote:
>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>>
>>>> Hi Fabien,
>>>>
>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>>>
>>>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>>> [..]
>>>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>>>> usage. On the other side, request_specific() would request shared locks.
>>>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>>>
>>>>> There is already an inconsistency in between these; as with above any
>>>>> system that uses both request() and request_specific() will be suffering
>>>>> from intermittent failures due to probe ordering.
>>>>>
>>>>>> one:
>>>>>> -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>>> -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>>> -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>>>> in mind before we take ay decision
>>>> Wouldn't it be actually simpler to just introduce a new specific API
>>>> variant for this, similar to the reset core for example (it uses a
>>>> separate exclusive API), without having to modify the bindings at all.
>>>> It is just a case of your driver using the right API, and the core can
>>>> be modified to use the additional tag semantics based on the API. It
>>>> should avoid any confusion with say using a different second cell value
>>>> for the same lock in two different nodes.
>>>>
>>> But this implies that there is an actual need to hold these locks
>>> exclusively. Given that they are (except for the raw case) all wrapped
>>> by Linux locking primitives there shouldn't be a problem sharing a lock
>>> (except possibly for the raw case).
>> Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
>> am still trying to understand better the usecase to see if the same lock
>> is being multiplexed for different protection contexts, or if all of
>> them are protecting the same context.
>
>
> Here are two different examples that explain the need for changes.
> In both cases the Linux clients are talking to a single entity on the
> remote-side.
>
> Example 1:
> exti: interrupt-controller@5000d000 {
> compatible = "st,stm32mp1-exti", "syscon";
> interrupt-controller;
> #interrupt-cells = <2>;
> reg = <0x5000d000 0x400>;
> hwlocks = <&hsem 1>;
> };
> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> With the current hwspinlock implementation, only the first driver succeeds
> in requesting (hwspin_lock_request_specific) the hwlock. The second request
> fails.
> Here, we really need to share the hwlock between the two drivers.
> Note: hardware spinlock support for regmap was 'recently' introduced in 4.15
> see https://lore.kernel.org/patchwork/patch/845941/
>
>
>
> Example 2:
> Here it is more a question of optimization : we want to save the number of
> hwlocks used to protect resources, using an unique hwlock to protect all
> pinctrl resources:
> pinctrl: pin-controller@50002000 {
> compatible = "st,stm32mp157-pinctrl";
> ranges = <0 0x50002000 0xa400>;
> hwlocks = <&hsem 0 1>;
>
> pinctrl_z: pin-controller-z@54004000 {
> compatible = "st,stm32mp157-z-pinctrl";
> ranges = <0 0x54004000 0x400>;
> pins-are-numbered;
> hwlocks = <&hsem 0 1>;
Thanks for the examples.
>
>>
>>> I agree that we shouldn't specify this property in DT - if anything it
>>> should be a variant of the API.
>
>
> If we decide to add a 'shared' API, then, what about the generic regmap
> driver?
>
> In the context of above example1, this would require to update the
> regmap driver.
>
> But would this be acceptable for any driver using syscon/regmap?
>
>
> I think it is better to keep the existing API (modifying it so it always
> allows
>
> hwlocks sharing, so no need for bindings update) than adding another API.
For your usecases, you would definitely need the syscon/regmap behavior
to be shared right. Whether we introduce a 'shared' API or an
'exclusive' API and change the current API behavior to shared, it is
definitely a case-by-case usage scenario for the existing drivers and
usage right. The main contention point is what to do with the
unprotected usecases like Bjorn originally pointed out.
regards
Suman
>
>
>
>>>
>>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>>> be aware that it is a shared lock. The tag can be set during the first
>>>> request API, and you look through both tags when giving out a handle.
>>>>
>>> Why would the driver need to know about it?
>> Just the semantics if we were to support single user vs multiple users
>> on Linux-side to even get a handle. Your point is that this may be moot
>> since we have protection anyway other than the raw cases. But we need to
>> be able to have the same API work across all cases.
>>
>> So far, it had mostly been that there would be one user on Linux
>> competing with other equivalent peer entities on different processors.
>> It is not common to have multiple users since these protection schemes
>> are usually needed only at the lowest levels of a stack, so the
>> exclusive handle stuff had been sufficient.
>>
>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>>> implied additional need for communicating the lock id to the other peer
>>>> entity, so a realistic usage is most always the specific API variant. I
>>>> doubt this API would be of much use for the shared driver usage. This
>>>> also implies that the client user does not care about specifying a lock
>>>> in DT.
>>>>
>>> Afaict if the lock are shared then there shouldn't be a problem with
>>> some clients using the request API and others request_specific(). As any
>>> collisions would simply mean that there are more contention on the lock.
>>>
>>> With the current exclusive model that is not possible and the success of
>>> the request_specific will depend on probe order.
>>>
>>> But perhaps it should be explicitly prohibited to use both APIs on the
>>> same hwspinlock instance?
>> Yeah, they are meant to be complimentary usage, though I doubt we will
>> ever have any realistic users for the generic API if we haven't had a
>> usage so far. I had posted a concept of reserved locks long back [1] to
>> keep away certain locks from the generic requestor, but dropped it since
>> we did not have an actual use-case needing it.
>>
>> regards
>> Suman
>>
>> [1] https://lwn.net/Articles/611944/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
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