* Re: [GIT PULL] soc: amlogic: updates for v5.4 (round 2)
From: Kevin Hilman @ 2019-09-03 22:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:ARM/Amlogic Meson SoC support, SoC Team, arm-soc,
Linux ARM
In-Reply-To: <CAK8P3a0_HEhvVk8Onk-9MBhnaBQT9B39+t6AGA3FRrH-_yMqVg@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> writes:
> On Fri, Aug 30, 2019 at 1:34 AM Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> OK, here's the respin (round 2.1)
>>
>> The previous version was missing the bindings for the new driver, which
>> I had mistakenly put in the DT branch instead of here. Without the
>> bindings and associated headers, this branch did not build stanalone
>> (found by kbuild robot.)
>>
>> All that is fixed by this branch.
>>
>> As a result, I also needed to respin the DT64 pull. Since I moved the
>> bindings/header patche here, the respin of the DT64 pull will now have a
>> dependency merge of this branch.
>
> I've pulled round 2.1 into arm/drivers, but it seems that the
> patchwork integration
> failed to deal with the way this was sent:
>
> - https://patchwork.kernel.org/patch/11122205/ shows both the original
> pull request, and the updated one. It was meant to detect both pull
> requests as the same thing and mark the old one as superseded, but that
> did not happen.
>
> - Using pwclient to get the pull request only shows the original one
>
> - I actually tried pulling that after looking at it with pwclient instead of
> the email client. Thankfully, you had removed the original tag, so that
> failed and I took a closer look.
>
> I suspect it would have worked the way it did for
> https://patchwork.kernel.org/patch/11119171/ if you had specified
> the subject as
>
> [GIT PULL, v2] soc: amlogic: updates for v5.4 (round 2)
>
> i.e. kept the subject the same but the version inside of the [].
Ah, ok. Good to know.
Thanks,
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 v5 03/10] arm64: atomics: avoid out-of-line ll/sc atomics
From: Andrew Murray @ 2019-09-03 22:04 UTC (permalink / raw)
To: Will Deacon
Cc: mark.rutland, peterz, catalin.marinas, ndesaulniers,
Ard.Biesheuvel, Nathan Chancellor, robin.murphy, linux-arm-kernel
In-Reply-To: <20190903163753.huk5sjg4m27qu2zu@willie-the-truck>
On Tue, Sep 03, 2019 at 05:37:55PM +0100, Will Deacon wrote:
> On Tue, Sep 03, 2019 at 04:31:20PM +0100, Andrew Murray wrote:
> > On Tue, Sep 03, 2019 at 04:15:44PM +0100, Andrew Murray wrote:
> > > On Tue, Sep 03, 2019 at 03:45:34PM +0100, Will Deacon wrote:
> > > > Does it work if the only thing you change is the toolchain, and use GCC
> > > > instead?
> > >
> > > Yup.
> >
> > Also this is Clang generation:
> >
> > ffff8000100f2700 <__ptrace_link>:
> > ffff8000100f2700: f9426009 ldr x9, [x0, #1216]
> > ffff8000100f2704: 91130008 add x8, x0, #0x4c0
> > ffff8000100f2708: eb09011f cmp x8, x9
> > ffff8000100f270c: 540002a1 b.ne ffff8000100f2760 <__ptrace_link+0x60> // b.any
> > ffff8000100f2710: f9425829 ldr x9, [x1, #1200]
> > ffff8000100f2714: 9112c02a add x10, x1, #0x4b0
> > ffff8000100f2718: f9000528 str x8, [x9, #8]
> > ffff8000100f271c: f9026009 str x9, [x0, #1216]
> > ffff8000100f2720: f902640a str x10, [x0, #1224]
> > ffff8000100f2724: f9025828 str x8, [x1, #1200]
> > ffff8000100f2728: f9024001 str x1, [x0, #1152]
> > ffff8000100f272c: b4000162 cbz x2, ffff8000100f2758 <__ptrace_link+0x58>
> > ffff8000100f2730: b900985f str wzr, [x2, #152]
> > ffff8000100f2734: 14000004 b ffff8000100f2744 <__ptrace_link+0x44>
> > ffff8000100f2738: 14000001 b ffff8000100f273c <__ptrace_link+0x3c>
> > ffff8000100f273c: 14000006 b ffff8000100f2754 <__ptrace_link+0x54>
> > ffff8000100f2740: 14000001 b ffff8000100f2744 <__ptrace_link+0x44>
> > ffff8000100f2744: 52800028 mov w8, #0x1 // #1
> > ffff8000100f2748: b828005f stadd w8, [x2]
> > ffff8000100f274c: f9030002 str x2, [x0, #1536]
> > ffff8000100f2750: d65f03c0 ret
> > ffff8000100f2754: 140007fd b ffff8000100f4748 <ptrace_check_attach+0xf8>
> > ...
> >
> > This looks like the default path (before we write over it) will take you to
> > the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at
> > least not what we expected to see. Also why 4 branches?
>
> So I reproduced this with a silly atomic_inc wrapper:
>
> void will_atomic_inc(atomic_t *v)
> {
> atomic_inc(v);
> }
>
> Compiles to:
>
> 0000000000000018 <will_atomic_inc>:
> 18: 14000004 b 28 <will_atomic_inc+0x10>
> 1c: 14000001 b 20 <will_atomic_inc+0x8>
> 20: 14000005 b 34 <will_atomic_inc+0x1c>
> 24: 14000001 b 28 <will_atomic_inc+0x10>
> 28: 52800028 mov w8, #0x1 // #1
> 2c: b828001f stadd w8, [x0]
> 30: d65f03c0 ret
> 34: 14000027 b d0 <dump_kernel_offset+0x60>
> 38: d65f03c0 ret
>
> which is going to explode.
I've come up with a simple reproducer for this issue:
static bool branch_jump()
{
asm_volatile_goto(
"1: b %l[l_yes2]"
: : : : l_yes2);
return false;
l_yes2:
return true;
}
static bool branch_test()
{
return (!branch_jump() && !branch_jump());
}
void andy_test(int *v)
{
if (branch_test())
*v = 0xff;
}
This leads to the following (it shouldn't do anything):
0000000000000000 <andy_test>:
0: 14000004 b 10 <andy_test+0x10>
4: 14000001 b 8 <andy_test+0x8>
8: 14000004 b 18 <andy_test+0x18>
c: 14000001 b 10 <andy_test+0x10>
10: 52801fe8 mov w8, #0xff // #255
14: b9000008 str w8, [x0]
18: d65f03c0 ret
The issue goes away with any of the following hunks:
@@ -55,7 +55,7 @@ static bool branch_jump()
static bool branch_test()
{
- return (!branch_jump() && !branch_jump());
+ return (!branch_jump());
}
void andy_test(int *v)
or:
@@ -53,14 +53,10 @@ static bool branch_jump()
return true;
}
-static bool branch_test()
-{
- return (!branch_jump() && !branch_jump());
-}
void andy_test(int *v)
{
- if (branch_test())
+ if (!branch_jump() && !branch_jump())
*v = 0xff;
}
Thanks,
Andrew Murray
>
> Will
_______________________________________________
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 v1 2/3] perf cs-etm: Add callchain to instruction sample
From: Mathieu Poirier @ 2019-09-03 22:07 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Alexander Shishkin, linux-kernel,
Arnaldo Carvalho de Melo, Adrian Hunter, Namhyung Kim,
Robert Walker, Jiri Olsa, linux-arm-kernel, Mike Leach
In-Reply-To: <20190830062421.31275-3-leo.yan@linaro.org>
On Fri, Aug 30, 2019 at 02:24:20PM +0800, Leo Yan wrote:
> Firstly, this patch adds support for the thread stack; when every branch
> packet is coming we will push or pop the stack based on the sample
> flags.
>
> Secondly, based on the thread stack we can synthesize call chain for the
> instruction sample, this can be used by itrace option '--itrace=g'.
>
In most cases using the word "secondly" is a good indication the patch should be
split.
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/util/cs-etm.c | 74 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 882a0718033d..ad573d3bd305 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -17,6 +17,7 @@
> #include <stdlib.h>
>
> #include "auxtrace.h"
> +#include "callchain.h"
> #include "color.h"
> #include "cs-etm.h"
> #include "cs-etm-decoder/cs-etm-decoder.h"
> @@ -69,6 +70,7 @@ struct cs_etm_traceid_queue {
> size_t last_branch_pos;
> union perf_event *event_buf;
> struct thread *thread;
> + struct ip_callchain *chain;
> struct branch_stack *last_branch;
> struct branch_stack *last_branch_rb;
> struct cs_etm_packet *prev_packet;
> @@ -246,6 +248,16 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
> if (!tidq->prev_packet)
> goto out_free;
>
> + if (etm->synth_opts.callchain) {
> + size_t sz = sizeof(struct ip_callchain);
> +
> + /* Add 1 to callchain_sz for callchain context */
> + sz += (etm->synth_opts.callchain_sz + 1) * sizeof(u64);
> + tidq->chain = zalloc(sz);
> + if (!tidq->chain)
> + goto out_free;
> + }
> +
> if (etm->synth_opts.last_branch) {
> size_t sz = sizeof(struct branch_stack);
>
> @@ -270,6 +282,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
> zfree(&tidq->last_branch);
> zfree(&tidq->prev_packet);
> zfree(&tidq->packet);
> + zfree(&tidq->chain);
> out:
> return rc;
> }
> @@ -541,6 +554,7 @@ static void cs_etm__free_traceid_queues(struct cs_etm_queue *etmq)
> zfree(&tidq->last_branch_rb);
> zfree(&tidq->prev_packet);
> zfree(&tidq->packet);
> + zfree(&tidq->chain);
> zfree(&tidq);
>
> /*
> @@ -1121,6 +1135,41 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> sample->insn_len, (void *)sample->insn);
> }
>
> +static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
> + struct cs_etm_traceid_queue *tidq)
> +{
> + struct cs_etm_auxtrace *etm = etmq->etm;
> + u8 trace_chan_id = tidq->trace_chan_id;
> + int insn_len;
> + u64 from_ip, to_ip;
> +
> + if (etm->synth_opts.callchain || etm->synth_opts.thread_stack) {
> +
> + from_ip = cs_etm__last_executed_instr(tidq->prev_packet);
> + to_ip = cs_etm__first_executed_instr(tidq->packet);
> +
> + /*
> + * T32 instruction size might be 32-bit or 16-bit, decide by
> + * calling cs_etm__t32_instr_size().
> + */
> + if (tidq->prev_packet->isa == CS_ETM_ISA_T32)
> + insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
> + from_ip);
> + /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> + else
> + insn_len = 4;
> +
> + thread_stack__event(tidq->thread, tidq->prev_packet->cpu,
> + tidq->prev_packet->flags,
> + from_ip, to_ip, insn_len,
> + etmq->buffer->buffer_nr);
> + } else {
> + thread_stack__set_trace_nr(tidq->thread,
> + tidq->prev_packet->cpu,
> + etmq->buffer->buffer_nr);
Please add a comment on what the above does. As a rule of thumb I add a comment
per addition in a patch in order to help people understand what is happening and
some of the reasonning behing the code.
> + }
> +}
> +
> static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
> struct cs_etm_traceid_queue *tidq,
> u64 addr, u64 period)
> @@ -1146,6 +1195,14 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>
> cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->packet, &sample);
>
> + if (etm->synth_opts.callchain) {
> + thread_stack__sample(tidq->thread, tidq->packet->cpu,
> + tidq->chain,
> + etm->synth_opts.callchain_sz + 1,
> + sample.ip, etm->kernel_start);
> + sample.callchain = tidq->chain;
> + }
> +
> if (etm->synth_opts.last_branch) {
> cs_etm__copy_last_branch_rb(etmq, tidq);
> sample.branch_stack = tidq->last_branch;
> @@ -1329,6 +1386,8 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm,
> attr.sample_type &= ~(u64)PERF_SAMPLE_ADDR;
> }
>
> + if (etm->synth_opts.callchain)
> + attr.sample_type |= PERF_SAMPLE_CALLCHAIN;
> if (etm->synth_opts.last_branch)
> attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>
> @@ -1397,6 +1456,9 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
> tidq->period_instructions = instrs_over;
> }
>
> + if (tidq->prev_packet->last_instr_taken_branch)
> + cs_etm__add_stack_event(etmq, tidq);
> +
> if (etm->sample_branches) {
> bool generate_sample = false;
>
> @@ -2596,7 +2658,17 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> } else {
> itrace_synth_opts__set_default(&etm->synth_opts,
> session->itrace_synth_opts->default_no_sample);
> - etm->synth_opts.callchain = false;
> +
> + etm->synth_opts.thread_stack =
> + session->itrace_synth_opts->thread_stack;
> + }
> +
> + if (etm->synth_opts.callchain && !symbol_conf.use_callchain) {
> + symbol_conf.use_callchain = true;
> + if (callchain_register_param(&callchain_param) < 0) {
> + symbol_conf.use_callchain = false;
> + etm->synth_opts.callchain = false;
> + }
> }
>
> err = cs_etm__synth_events(etm, session);
> --
> 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 08/13] swiotlb-xen: always use dma-direct helpers to alloc coherent pages
From: Boris Ostrovsky @ 2019-09-03 22:15 UTC (permalink / raw)
To: Christoph Hellwig, Stefano Stabellini, Konrad Rzeszutek Wilk,
gross
Cc: xen-devel, iommu, x86, linux-kernel, linux-arm-kernel
In-Reply-To: <20190902130339.23163-9-hch@lst.de>
On 9/2/19 9:03 AM, Christoph Hellwig wrote:
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index b8808677ae1d..f9dd4cb6e4b3 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -299,8 +299,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
> * address. In fact on ARM virt_to_phys only works for kernel direct
> * mapped RAM memory. Also see comment below.
> */
> - ret = xen_alloc_coherent_pages(hwdev, size, dma_handle, flags, attrs);
> -
> + ret = dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
If I am reading __dma_direct_alloc_pages() correctly there is a path
that will force us to use GFP_DMA32 and as Juergen pointed out in
another message that may not be desirable.
-boris
_______________________________________________
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 v1 1/3] perf cs-etm: Refactor instruction size handling
From: Mathieu Poirier @ 2019-09-03 22:22 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Alexander Shishkin, linux-kernel,
Arnaldo Carvalho de Melo, Adrian Hunter, Namhyung Kim,
Robert Walker, Jiri Olsa, linux-arm-kernel, Mike Leach
In-Reply-To: <20190830062421.31275-2-leo.yan@linaro.org>
On Fri, Aug 30, 2019 at 02:24:19PM +0800, Leo Yan wrote:
> There has several code pieces need to know the instruction size, but
> now every place calculates the instruction size separately.
>
> This patch refactors to create a new function cs_etm__instr_size() as
> a central place to analyze the instruction length based on ISA type
> and instruction value.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/util/cs-etm.c | 44 +++++++++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index b3a5daaf1a8f..882a0718033d 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -914,6 +914,26 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
> return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;
> }
>
> +static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,
> + u8 trace_chan_id,
> + enum cs_etm_isa isa,
> + u64 addr)
> +{
> + int insn_len;
> +
> + /*
> + * T32 instruction size might be 32-bit or 16-bit, decide by calling
> + * cs_etm__t32_instr_size().
> + */
> + if (isa == CS_ETM_ISA_T32)
> + insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);
> + /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> + else
> + insn_len = 4;
> +
> + return insn_len;
> +}
> +
> static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
> {
> /* Returns 0 for the CS_ETM_DISCONTINUITY packet */
> @@ -938,19 +958,23 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
> const struct cs_etm_packet *packet,
> u64 offset)
> {
> + int insn_len;
> +
> if (packet->isa == CS_ETM_ISA_T32) {
> u64 addr = packet->start_addr;
>
> while (offset > 0) {
> - addr += cs_etm__t32_instr_size(etmq,
> - trace_chan_id, addr);
> + addr += cs_etm__instr_size(etmq, trace_chan_id,
> + packet->isa, addr);
> offset--;
> }
> return addr;
> }
>
> - /* Assume a 4 byte instruction size (A32/A64) */
> - return packet->start_addr + offset * 4;
> + /* Return instruction size for A32/A64 */
> + insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> + packet->isa, packet->start_addr);
> + return packet->start_addr + offset * insn_len;
This patch will work but from where I stand it makes things difficult to
understand more than anything else. It is also adding coupling between function
cs_etm__instr_addr() and cs_etm__instr_size(), meaning the code needs to be
carefully inspected in order to make changes to either one.
Last but not least function cs_etm__instr_size() isn't used in the upcoming
patches. I really don't see what is gained here.
Thanks,
Mathieu
> }
>
> static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq,
> @@ -1090,16 +1114,8 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> return;
> }
>
> - /*
> - * T32 instruction size might be 32-bit or 16-bit, decide by calling
> - * cs_etm__t32_instr_size().
> - */
> - if (packet->isa == CS_ETM_ISA_T32)
> - sample->insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
> - sample->ip);
> - /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> - else
> - sample->insn_len = 4;
> + sample->insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> + packet->isa, sample->ip);
>
> cs_etm__mem_access(etmq, trace_chan_id, sample->ip,
> sample->insn_len, (void *)sample->insn);
> --
> 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 08/13] swiotlb-xen: always use dma-direct helpers to alloc coherent pages
From: Boris Ostrovsky @ 2019-09-03 22:25 UTC (permalink / raw)
To: Christoph Hellwig, Stefano Stabellini, Konrad Rzeszutek Wilk,
Juergen Gross
Cc: xen-devel, iommu, x86, linux-kernel, linux-arm-kernel
In-Reply-To: <5799ca4b-7993-b1c5-73fa-396a560f58e8@oracle.com>
(Now with correct address for Juergen)
On 9/3/19 6:15 PM, Boris Ostrovsky wrote:
> On 9/2/19 9:03 AM, Christoph Hellwig wrote:
>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> index b8808677ae1d..f9dd4cb6e4b3 100644
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -299,8 +299,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>> * address. In fact on ARM virt_to_phys only works for kernel direct
>> * mapped RAM memory. Also see comment below.
>> */
>> - ret = xen_alloc_coherent_pages(hwdev, size, dma_handle, flags, attrs);
>> -
>> + ret = dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
>
> If I am reading __dma_direct_alloc_pages() correctly there is a path
> that will force us to use GFP_DMA32 and as Juergen pointed out in
> another message that may not be desirable.
>
> -boris
>
>
>
_______________________________________________
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 v5 03/10] arm64: atomics: avoid out-of-line ll/sc atomics
From: Nick Desaulniers @ 2019-09-03 22:35 UTC (permalink / raw)
To: Andrew Murray
Cc: Mark Rutland, Peter Zijlstra, Catalin Marinas, Robin Murphy,
Ard.Biesheuvel, Nathan Chancellor, Will Deacon, Linux ARM
In-Reply-To: <20190903220412.GU9720@e119886-lin.cambridge.arm.com>
On Tue, Sep 3, 2019 at 3:04 PM Andrew Murray <andrew.murray@arm.com> wrote:
>
> On Tue, Sep 03, 2019 at 05:37:55PM +0100, Will Deacon wrote:
> > On Tue, Sep 03, 2019 at 04:31:20PM +0100, Andrew Murray wrote:
> > > On Tue, Sep 03, 2019 at 04:15:44PM +0100, Andrew Murray wrote:
> > > > On Tue, Sep 03, 2019 at 03:45:34PM +0100, Will Deacon wrote:
> > > > > Does it work if the only thing you change is the toolchain, and use GCC
> > > > > instead?
> > > >
> > > > Yup.
> > >
> > > Also this is Clang generation:
> > >
> > > ffff8000100f2700 <__ptrace_link>:
> > > ffff8000100f2700: f9426009 ldr x9, [x0, #1216]
> > > ffff8000100f2704: 91130008 add x8, x0, #0x4c0
> > > ffff8000100f2708: eb09011f cmp x8, x9
> > > ffff8000100f270c: 540002a1 b.ne ffff8000100f2760 <__ptrace_link+0x60> // b.any
> > > ffff8000100f2710: f9425829 ldr x9, [x1, #1200]
> > > ffff8000100f2714: 9112c02a add x10, x1, #0x4b0
> > > ffff8000100f2718: f9000528 str x8, [x9, #8]
> > > ffff8000100f271c: f9026009 str x9, [x0, #1216]
> > > ffff8000100f2720: f902640a str x10, [x0, #1224]
> > > ffff8000100f2724: f9025828 str x8, [x1, #1200]
> > > ffff8000100f2728: f9024001 str x1, [x0, #1152]
> > > ffff8000100f272c: b4000162 cbz x2, ffff8000100f2758 <__ptrace_link+0x58>
> > > ffff8000100f2730: b900985f str wzr, [x2, #152]
> > > ffff8000100f2734: 14000004 b ffff8000100f2744 <__ptrace_link+0x44>
> > > ffff8000100f2738: 14000001 b ffff8000100f273c <__ptrace_link+0x3c>
> > > ffff8000100f273c: 14000006 b ffff8000100f2754 <__ptrace_link+0x54>
> > > ffff8000100f2740: 14000001 b ffff8000100f2744 <__ptrace_link+0x44>
> > > ffff8000100f2744: 52800028 mov w8, #0x1 // #1
> > > ffff8000100f2748: b828005f stadd w8, [x2]
> > > ffff8000100f274c: f9030002 str x2, [x0, #1536]
> > > ffff8000100f2750: d65f03c0 ret
> > > ffff8000100f2754: 140007fd b ffff8000100f4748 <ptrace_check_attach+0xf8>
> > > ...
> > >
> > > This looks like the default path (before we write over it) will take you to
> > > the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at
> > > least not what we expected to see. Also why 4 branches?
> >
> > So I reproduced this with a silly atomic_inc wrapper:
> >
> > void will_atomic_inc(atomic_t *v)
> > {
> > atomic_inc(v);
> > }
> >
> > Compiles to:
> >
> > 0000000000000018 <will_atomic_inc>:
> > 18: 14000004 b 28 <will_atomic_inc+0x10>
> > 1c: 14000001 b 20 <will_atomic_inc+0x8>
> > 20: 14000005 b 34 <will_atomic_inc+0x1c>
> > 24: 14000001 b 28 <will_atomic_inc+0x10>
> > 28: 52800028 mov w8, #0x1 // #1
> > 2c: b828001f stadd w8, [x0]
> > 30: d65f03c0 ret
> > 34: 14000027 b d0 <dump_kernel_offset+0x60>
> > 38: d65f03c0 ret
> >
> > which is going to explode.
Indeed, I can reproduce the hang with `-cpu cortex-a57` and `-cpu
cortex-a73` in QEMU. Looks like my qemu (3.1.0) doesn't recognizer
newer cores, so I might have to build QEMU from source to test the
v8.2 extension support.
https://en.wikipedia.org/wiki/Comparison_of_ARMv8-A_cores
>
> I've come up with a simple reproducer for this issue:
>
> static bool branch_jump()
> {
> asm_volatile_goto(
> "1: b %l[l_yes2]"
> : : : : l_yes2);
>
> return false;
> l_yes2:
> return true;
> }
>
> static bool branch_test()
> {
> return (!branch_jump() && !branch_jump());
> }
>
> void andy_test(int *v)
> {
> if (branch_test())
> *v = 0xff;
> }
>
> This leads to the following (it shouldn't do anything):
>
> 0000000000000000 <andy_test>:
> 0: 14000004 b 10 <andy_test+0x10>
> 4: 14000001 b 8 <andy_test+0x8>
> 8: 14000004 b 18 <andy_test+0x18>
> c: 14000001 b 10 <andy_test+0x10>
> 10: 52801fe8 mov w8, #0xff // #255
> 14: b9000008 str w8, [x0]
> 18: d65f03c0 ret
>
> The issue goes away with any of the following hunks:
>
>
> @@ -55,7 +55,7 @@ static bool branch_jump()
>
> static bool branch_test()
> {
> - return (!branch_jump() && !branch_jump());
> + return (!branch_jump());
> }
>
> void andy_test(int *v)
>
>
> or:
>
>
> @@ -53,14 +53,10 @@ static bool branch_jump()
> return true;
> }
>
> -static bool branch_test()
> -{
> - return (!branch_jump() && !branch_jump());
> -}
>
> void andy_test(int *v)
> {
> - if (branch_test())
> + if (!branch_jump() && !branch_jump())
> *v = 0xff;
> }
Thanks for the report. We squashed many bugs related to asm goto, but
it's difficult to say with 100% certainty that the current
implementation is bug free. Simply throwing more exotic forms of
control flow at it often shake out corner cases. Thank you very much
for the reduced test case, and I'll look into getting a fix ready
hopefully in time to make the clang-9 release train.
--
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 v2 11/11] coresight: etm4x: docs: Adds detailed document for programming etm4x.
From: Mike Leach @ 2019-09-03 22:46 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Jonathan Corbet, Greg Kroah-Hartman, Coresight ML,
Suzuki K. Poulose, linux-doc, linux-arm-kernel
In-Reply-To: <20190903193807.GA25787@xps15>
Hi Mathieu,
On Tue, 3 Sep 2019 at 20:38, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> Hi Mike,
>
> On Thu, Aug 29, 2019 at 10:33:21PM +0100, Mike Leach wrote:
> > Add in detailed programmers reference for users wanting to program the
> > CoreSight ETM 4.x driver using sysfs.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> > .../coresight/coresight-etm4x-reference.txt | 458 ++++++++++++++++++
> > 1 file changed, 458 insertions(+)
> > create mode 100644 Documentation/trace/coresight/coresight-etm4x-reference.txt
> >
> > diff --git a/Documentation/trace/coresight/coresight-etm4x-reference.txt b/Documentation/trace/coresight/coresight-etm4x-reference.txt
> > new file mode 100644
> > index 000000000000..f0c370870992
> > --- /dev/null
> > +++ b/Documentation/trace/coresight/coresight-etm4x-reference.txt
> > @@ -0,0 +1,458 @@
> > +ETMv4 sysfs linux driver programming reference - v2.
> > +====================================================
> > +
> > +Supplement to existing ETMv4 driver documentation.
> > +
> > +Sysfs files and directories
> > +---------------------------
> > +
> > +Root: /sys/bus/coresight/devices/etm<N>
> > +
> > +
> > +The following paragraphs explain the association between sysfs files and the
> > +ETMv4 registers that they effect. Note the register names are given without
> > +the ‘TRC’ prefix.
> > +
> > +File : mode (rw)
> > +Trace Registers : {CONFIGR + others}
> > +Notes : Bit select trace features. See ‘mode’ section below. Bits
> > + in this will cause equivalent programming of trace config and
> > + other registers to enable the features requested.
> > +Syntax & eg : 'echo bitfield > mode'
> > + bitfield up to 32 bits setting trace features.
> > +Example : $> echo 0x > mode
>
> I suspect things look different on your end than they do on mine. The biggest
> problem is related to multi-line fields. For instance the above looks like this
> on my side:
>
> File : mode (rw)
> Trace Registers : {CONFIGR + others}
> Notes : Bit select trace features. See ‘mode’ section below. Bits
> in this will cause equivalent programming of trace config and
> other registers to enable the features requested.
> Syntax & eg : 'echo bitfield > mode'
> bitfield up to 32 bits setting trace features.
> Example : $> echo 0x > mode
>
> It would be nicer to have multi-line fields aligned with the first line, such
> as:
>
> File : mode (rw)
> Trace Registers : {CONFIGR + others}
> Notes : Bit select trace features. See ‘mode’ section below. Bits
> in this will cause equivalent programming of trace config and
> other registers to enable the features requested.
> Syntax & eg : 'echo bitfield > mode'
> bitfield up to 32 bits setting trace features.
> Example : $> echo 0x > mode
>
Problem is I am not seeing any difference between what you say I am
writing and what you prefer. When i looked at the file in my text
editor the fields where aligned - I would not have submitted it
otherwise.
I am happy to revisit it but we need a way of seeing a common view.
> I'm also not sure about the prompt, i.e "$>". I suspect it should be "$" and
> an additional ">" got inserted.
prompt came from examples lifted from a DB410 session. Don't really
have any strong preference for what represents a prompt in the docs,
so happy to change it to anything appropriate.
Regards
Mike
>
> I wanted to read on but is it too difficult to know what is intentional and what
> isn't. Please address and resend.
>
> Thanks,
> Mathieu
>
> > +
> > +File : reset (wo)
> > +Trace Registers : All
> > +Notes : Reset all programming to trace nothing / no logic programmed.
> > +Syntax : 'echo 1 > reset'
> > +
> > +File : enable_source (wo)
> > +Trace Registers : PRGCTLR, All hardware regs.
> > +Notes : >0: Programs up the hardware with the current values held in
> > + the driver and enables trace.
> > + 0: disable trace hardware.
> > +Syntax : 'echo 1 > enable_source'
> > +
> > +File : cpu (ro)
> > +Trace Registers : None.
> > +Notes : CPU ID that this ETM is attached to.
> > +Example :$> cat cpu
> > + $> 0
> > +
> > +File : addr_idx (rw)
> > +Trace Registers : None.
> > +Notes : Virtual register to index address comparator and range
> > + features. Set index for first of the pair in a range.
> > +Syntax : 'echo idx > addr_idx'
> > + Where idx < nr_addr_cmp x 2
> > +
> > +File : addr_range (rw)
> > +Trace Registers : ACVR[idx, idx+1], VIIECTLR
> > +Notes : Pair of addresses for a range selected by addr_idx. Include
> > + / exclude according to the optional parameter, or if omitted
> > + uses the current ‘mode’ setting. Select comparator range in
> > + control register. Error if index is odd value.
> > +Depends : mode, addr_idx
> > +Syntax : 'echo addr1 addr2 [exclude] > addr_range'
> > + Where addr1 and addr2 define the range and addr1 < addr2.
> > + Optional exclude value - 0 for include, 1 for exclude.
> > +Example : $> echo 0x0000 0x2000 0 > addr_range
> > +
> > +File : addr_single (rw)
> > +Trace Registers : ACVR[idx]
> > +Notes : Set a single address comparator according to addr_idx. This
> > + is used if the address comparator is used as part of event
> > + generation logic etc.
> > +Depends : addr_idx
> > +Syntax : 'echo addr1 > addr_single'
> > +
> > +File : addr_start (rw)
> > +Trace Registers : ACVR[idx], VISSCTLR
> > +Notes : Set a trace start address comparator according to addr_idx.
> > + Select comparator in control register.
> > +Depends : addr_idx
> > +Syntax : 'echo addr1 > addr_start'
> > +
> > +File : addr_stop (rw)
> > +Trace Registers : ACVR[idx], VISSCTLR
> > +Notes : Set a trace stop address comparator according to addr_idx.
> > + Select comparator in control register.
> > +Depends : addr_idx
> > +Syntax : 'echo addr1 > addr_stop'
> > +
> > +File : addr_context (rw)
> > +Trace Registers : ACATR[idx,{6:4}]
> > +Notes : Link context ID comparator to address comparator addr_idx
> > +Depends : addr_idx.
> > +Syntax : 'echo ctxt_idx > addr_context'
> > + Where ctxt_idx is the index of the linked context id / vmid
> > + comparator.
> > +
> > +File : addr_ctxtype (rw)
> > +Trace Registers : ACATR[idx,{3:2}]
> > +Notes : Input value string. Set type for linked context ID comparator
> > +Depends : addr_idx
> > +Syntax : 'echo type > addr_ctxtype'
> > + Type one of {all, vmid, ctxid, none}
> > +Example : $> echo ctxid > addr_ctxtype
> > +
> > +File : addr_exlevel_s_ns (rw)
> > +Trace Registers : ACATR[idx,{14:8}]
> > +Notes : Set the ELx secure and non-secure matching bits for the
> > + selected address comparator
> > +Depends : addr_idx
> > +Syntax : 'echo val > addr_exlevel_s_ns'
> > + val is a 7 bit value for exception levels to exclude. Input
> > + value shifted to correct bits in register.
> > +Example : $> echo 0x4F > addr_exlevel_s_ns
> > +
> > +File : addr_instdatatype (rw)
> > +Trace Registers : ACATR[idx,{1:0}]
> > +Notes : Set the comparator address type for matching. Driver only
> > + supports setting instruction address type.
> > +Depends : addr_idx
> > +
> > +File : addr_cmp_view (ro)
> > +Trace Registers : ACVR[idx, idx+1], ACATR[idx], VIIECTLR
> > +Notes : Read the currently selected address comparator. If part of
> > + address range then display both addresses.
> > +Depends : addr_idx
> > +Syntax : 'cat addr_cmp_view'
> > +Example : $> cat addr_cmp_view
> > + addr_cmp[0] range 0x0 0xffffffffffffffff include ctrl(0x4b00)
> > +
> > +File : nr_addr_cmp (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of address comparator pairs
> > +
> > +File : sshot_idx (rw)
> > +Trace Registers : None
> > +Notes : Select single shot register set.
> > +
> > +File : sshot_ctrl (rw)
> > +Trace Registers : SSCCR[idx]
> > +Notes : Access a single shot comparator control register.
> > +Depends : sshot_idx
> > +Syntax : 'echo val > sshot_ctrl'
> > + Writes val into the selected control register.
> > +
> > +File : sshot_status (ro)
> > +Trace Registers : SSCSR[idx]
> > +Notes : Read a single shot comparator status register
> > +Depends : sshot_idx
> > +Syntax : 'cat sshot_status'
> > + Read status.
> > +Example : $> cat sshot_status
> > + 0x1
> > +
> > +File : sshot_pe_ctrl (rw)
> > +Trace Registers : SSPCICR[idx]
> > +Notes : Access a single shot PE comparator input control register.
> > +Depends : sshot_idx
> > +Syntax : echo val > sshot_pe_ctrl
> > + Writes val into the selected control register.
> > +
> > +File : ns_exlevel_vinst (rw)
> > +Trace Registers : VICTLR{23:20}
> > +Notes : Program non-secure exception level filters. Set / clear NS
> > + exception filter bits. Setting ‘1’ excludes trace from the
> > + exception level.
> > +Syntax : 'echo bitfield > ns_exlevel_viinst'
> > + Where bitfield contains bits to set clear for EL0 to EL2
> > +Example : %> echo 0x4 > ns_exlevel_viinst
> > + ; Exclude EL2 NS trace.
> > +
> > +File : vinst_pe_cmp_start_stop (rw)
> > +Trace Registers : VIPCSSCTLR
> > +Notes : Access PE start stop comparator input control registers
> > +
> > +File : bb_ctrl (rw)
> > +Trace Registers : BBCTLR
> > +Notes : Define ranges that Branch Broadcast will operate in.
> > + Default (0x0) is all addresses.
> > +Depends : BB enabled.
> > +
> > +File : cyc_threshold (rw)
> > +Trace Registers : CCCTLR
> > +Notes : Set the threshold for which cycle counts will be emitted.
> > + Error if attempt to set below minimum defined in IDR3, masked
> > + to width of valid bits.
> > +Depends : CC enabled.
> > +
> > +File : syncfreq (rw)
> > +Trace Registers : SYNCPR
> > +Notes : Set trace synchronisation period. Power of 2 value, 0 (off)
> > + or 8-20. Driver defaults to 12 (every 4096 bytes).
> > +
> > +File : cntr_idx (rw)
> > +Trace Registers : none
> > +Notes : Select the counter to access
> > +Syntax : 'echo idx > cntr_idx'
> > + Where idx < nr_cntr
> > +
> > +File : cntr_ctrl (rw)
> > +Trace Registers : CNTCTLR[idx]
> > +Notes : Set counter control value
> > +Depends : cntr_idx
> > +Syntax : 'echo val > cntr_ctrl'
> > + Where val is per ETMv4 spec.
> > +
> > +File : cntrldvr (rw)
> > +Trace Registers : CNTRLDVR[idx]
> > +Notes : Set counter reload value
> > +Depends : cntr_idx
> > +Syntax : 'echo val > cntrldvr'
> > + Where val is per ETMv4 spec.
> > +
> > +File : nr_cntr (ro)
> > +Trace Registers : From IDR5
> > +Notes : Number of counters implemented.
> > +
> > +File : ctxid_idx (rw)
> > +Trace Registers : None
> > +Notes : Select the context ID comparator to access
> > +Syntax : 'echo idx > ctxid_idx'
> > + Where idx < numcidc
> > +
> > +File : ctxid_pid (rw)
> > +Trace Registers : CIDCVR[idx]
> > +Notes : Set the context ID comparator value
> > +Depends : ctxid_idx
> > +
> > +File : ctxid_masks (rw)
> > +Trace Registers : CIDCCTLR0, CIDCCTLR1, CIDCVR<0-7>
> > +Notes : Pair of values to set the byte masks for 1-8 context ID
> > + comparators. Automatically clears masked bytes to 0 in CID
> > + value registers.
> > +Syntax : 'echo m3m2m1m0 [m7m6m5m4] > ctxid_masks'
> > + 32 bit values made up of mask bytes, where mN represents a
> > + byte mask value for Ctxt ID comparator N.
> > + Second value not required on systems that have fewer than 4
> > + context ID comparators
> > +
> > +File : numcidc (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of Context ID comparators
> > +
> > +File : vmid_idx (rw)
> > +Trace Registers : None
> > +Notes : Select the VM ID comparator to access.
> > +Syntax : 'echo idx > vmid_idx'
> > + Where idx < numvmidc
> > +
> > +File : vmid_val (rw)
> > +Trace Registers : VMIDCVR[idx]
> > +Notes : Set the VM ID comparator value
> > +Depends : vmid_idx
> > +
> > +File : vmid_masks (rw)
> > +Trace Registers : VMIDCCTLR0, VMIDCCTLR1, VMIDCVR<0-7>
> > +Notes : Pair of values to set the byte masks for 1-8 VM ID
> > + comparators. Automatically clears masked bytes to 0 in VMID
> > + value registers.
> > +Syntax : 'echo m3m2m1m0 [m7m6m5m4] > vmid_masks'
> > + Where mN represents a byte mask value for VMID comparator N.
> > + Second value not required on systems that have fewer than
> > + 4 VMID comparators.
> > +
> > +File : numvmidc (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of VMID comparators
> > +
> > +File : res_idx (rw)
> > +Trace Registers : None.
> > +Notes : Select the resource selector control to access. Must be 2 or
> > + higher as selectors 0 and 1 are hardwired.
> > +Syntax : 'echo idx > res_idx'
> > + Where 2 <= idx < nr_resource x 2
> > +
> > +File : res_ctrl (rw)
> > +Trace Registers : RSCTLR[idx]
> > +Notes : Set resource selector control value. Value per ETMv4 spec.
> > +Depends : res_idx
> > +Syntax : 'echo val > res_cntr'
> > + Where val is per ETMv4 spec.
> > +
> > +File : nr_resource (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of resource selector pairs
> > +
> > +File : event (rw)
> > +Trace Registers : EVENTCTRL0R
> > +Notes : Set up to 4 implemented event fields.
> > +Syntax : 'echo ev3ev2ev1ev0 > event'
> > + Where evN is an 8 bit event field. Up to 4 event fields make up
> > + the 32bit input value. Number of valid fields implementation
> > + dependent defined in IDR0.
> > +
> > +File : event_instren (rw)
> > +Trace Registers : EVENTCTRL1R
> > +Notes : Choose events which insert event packets into trace stream.
> > +Depends : EVENTCTRL0R
> > +Syntax : 'echo bitfield > event_instren'
> > + Where bitfield is up to 4 bits according to number of event
> > + fields.
> > +
> > +File : event_ts (rw)
> > +Trace Registers : TSCTLR
> > +Notes : Set the event that will generate timestamp requests.
> > +Depends : TS activated
> > +Syntax : 'echo evfield > event_ts'
> > + Where evfield is an 8 bit event selector.
> > +
> > +File : seq_idx (rw)
> > +Trace Registers : None
> > +Notes : Sequencer event register select - 0 to 2
> > +
> > +
> > +File : seq_state (rw)
> > +Trace Registers : SEQSTR
> > +Notes : Sequencer current state - 0 to 3.
> > +
> > +File : seq_event (rw)
> > +Trace Registers : SEQEVR[idx]
> > +Notes : State transition event registers
> > +Depends : seq_idx
> > +Syntax : 'echo evBevF > seq_event'
> > + Where evBevF is a 16 bit value made up of two event selectors,
> > + evB - back, evF - forwards.
> > +
> > +File : seq_reset_event (rw)
> > +Trace Registers : SEQRSTEVR
> > +Notes : Sequencer reset event
> > +Syntax : 'echo evfield > seq_reset_event'
> > + Where evfield is an 8 bit event selector.
> > +
> > +File : nrseqstate (ro)
> > +Trace Registers : From IDR5
> > +Notes : Number of sequencer states (0 or 4)
> > +
> > +File : nr_pe_cmp (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of PE comparator inputs
> > +
> > +File : nr_ext_inp (ro)
> > +Trace Registers : From IDR5
> > +Notes : Number of external inputs
> > +
> > +File : nr_ss_cmp (ro)
> > +Trace Registers : From IDR4
> > +Notes : Number of Single Shot control registers
> > +
> > +Note: When programming any address comparator the driver will tag the
> > +comparator with a type used - i.e. RANGE, SINGLE, START, STOP. Once this tag
> > +is set, then only the values can be changed using the same sysfs file / type
> > +used to program it.
> > +
> > +Thus:-
> > +% echo 0 > addr_idx ; select address comparator 0
> > +% echo 0x1000 0x5000 0 > addr_range ; set address range on comparators 0 and 1.
> > +% echo 0x2000 > addr_start ; this will error as comparator 0 is a
> > + ; range comparator
> > +% echo 2 > addr_idx ; select address comparator 2
> > +% echo 0x2000 > addr_start ; this is OK as comparator 2 is unused,
> > +% echo 0x3000 > addr_stop ; this will error as comparator 2 a start
> > + ; address comparator
> > +% echo 2 > addr_idx ; select address comparator 3
> > +% echo 0x3000 > addr_stop ; this is OK
> > +
> > +To remove programming on all the comparators (and all the other hardware) use
> > +the reset parameter:
> > +
> > +% echo 1 > reset
> > +
> > +The ‘mode’ sysfs parameter.
> > +---------------------------
> > +
> > +This is a bitfield selection parameter that sets the overall trace mode for the
> > +ETM. The table below describes the bits, using the defines from the driver
> > +source file, along with a description of the feature these represent. Many
> > +features are optional and therefore dependent on implementation in the
> > +hardware.
> > +
> > +Bit assignements shown below:-
> > +
> > +bit (0) : #define ETM_MODE_EXCLUDE
> > +description : This is the default value for the include / exclude function when
> > + setting address ranges. Set 1 for exclude range. When the mode
> > + parameter is set this value is applied to the currently indexed
> > + address range.
> > +
> > +bit (4) : #define ETM_MODE_BB
> > +description : Set to enable branch broadcast if supported in hardware [IDR0].
> > +
> > +bit (5) : #define ETMv4_MODE_CYCACC
> > +description : Set to enable cycle accurate trace if supported [IDR0].
> > +
> > +bit (6) : ETMv4_MODE_CTXID
> > +description : Set to enable context ID tracing if supported in hardware [IDR2].
> > +
> > +bit (7) : ETM_MODE_VMID
> > +description : Set to enable virtual machine ID tracing if supported [IDR2].
> > +
> > +bit (11) : ETMv4_MODE_TIMESTAMP
> > +description : Set to enable timestamp generation if supported [IDR0].
> > +
> > +bit (12) : ETM_MODE_RETURNSTACK
> > +description : Set to enable trace return stack use if supported [IDR0].
> > +
> > +bit (13-14) : ETM_MODE_QELEM(val)
> > +description : ‘val’ determines level of Q element support enabled if
> > + implemented by the ETM [IDR0]
> > +
> > +bit (19) : ETM_MODE_ATB_TRIGGER
> > +description : Set to enable the ATBTRIGGER bit in the event control register
> > + [EVENTCTLR1] if supported [IDR5].
> > +
> > +bit (20) : ETM_MODE_LPOVERRIDE
> > +description : Set to enable the LPOVERRIDE bit in the event control register
> > + [EVENTCTLR1], if supported [IDR5].
> > +
> > +bit (21) : ETM_MODE_ISTALL_EN
> > +description : Set to enable the ISTALL bit in the stall control register
> > + [STALLCTLR]
> > +
> > +bit (23) : ETM_MODE_INSTPRIO
> > +description : Set to enable the INSTPRIORITY bit in the stall control register
> > + [STALLCTLR] , if supported [IDR0].
> > +
> > +bit (24) : ETM_MODE_NOOVERFLOW
> > +description : Set to enable the NOOVERFLOW bit in the stall control register
> > + [STALLCTLR], if supported [IDR3].
> > +
> > +bit (25) : ETM_MODE_TRACE_RESET
> > +description : Set to enable the TRCRESET bit in the viewinst control register
> > + [VICTLR] , if supported [IDR3].
> > +
> > +bit (26) : ETM_MODE_TRACE_ERR
> > +description : Set to enable the TRCCTRL bit in the viewinst control register
> > + [VICTLR].
> > +
> > +bit (27) : ETM_MODE_VIEWINST_STARTSTOP
> > +description : Set the initial state value of the ViewInst start / stop logic
> > + in the viewinst control register [VICTLR]
> > +
> > +bit (30) : ETM_MODE_EXCL_KERN
> > +description : Set default trace setup to exclude kernel mode trace (see note a)
> > +
> > +bit (31) : ETM_MODE_EXCL_USER
> > +description : Set default trace setup to exclude user space trace (see note a)
> > +
> > +Note a) On startup the ETM is programmed to trace the complete address space
> > +using address range comparator 0. ‘mode’ bits 30 / 31 modify this setting to
> > +set EL exclude bits for NS state in either user space (EL0) or kernel space
> > +(EL1) in the address range comparator. (the default setting excludes all
> > +secure EL, and NS EL2)
> > +
> > +Once the reset parameter has been used, and/or custom programming has been
> > +implemented - using these bits will result in the EL bits for address
> > +comparator 0 being set in the same way.
> > +
> > +Note b) Bits 2-3, 8-10, 15-16, 18, 22, control features that only work with
> > +data trace. As A profile data trace is architecturally prohibited in ETMv4,
> > +these have been omitted here. Possible uses could be where a kernel has
> > +support for control of R or M profile infrastructure as part of a heterogeneous
> > +system.
> > +
> > +Bits 17, 28-29 are unused.
> > --
> > 2.17.1
> >
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [GIT PULL] ARM: mvebu: dt64 for v5.4 (#2)
From: Marek Behun @ 2019-09-03 22:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andrew Lunn, Jason Cooper, Gregory CLEMENT, SoC Team, arm-soc,
Olof Johansson, Linux ARM, Sebastian Hesselbarth
In-Reply-To: <CAK8P3a0BqjtVoTrUedDGHBUv8gwL23XWqYM2831v7G+23i8++A@mail.gmail.com>
On Tue, 3 Sep 2019 23:05:47 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Sep 3, 2019 at 2:41 PM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
> > Here is the second pull request for dt64 for mvebu for v5.4.
> >
> > For the Turris Mox board there was dependencies with moxtet header which
> > was already merged in your arm/drivers branch. That the reason why I
> > merged this branch in my mvebu/dt64 branch.
> >
> > Let me know if it is a problem and if you want that I do it in a
> > different way.
>
> I don't really like this, but it's too late to do it right now. The problem is
> that I should have not picked up the patches from the list in the first
> place if there are these dependencies.
>
> This could have been communicated better in the patch series, but
> it really my own fault.
>
> > ----------------------------------------------------------------
> > mvebu dt64 for 5.4 (part 2)
> >
> > Add support for Turris Mox board (Armada 3720 SoC based)
> >
> > ----------------------------------------------------------------
> > Marek Behún (3):
> > arm64: dts: marvell: armada-37xx: add SPI CS1 pinctrl
> > dt-bindings: marvell: document Turris Mox compatible
> > arm64: dts: marvell: add DTS for Turris Mox
>
> I think the best way forward would be for me to apply the
> remaining patches on top of the arm/drivers branch, to avoid
> also pulling in your other DT changes into arm/drivers, or pulling
> in all of arm/drivers into arm/dt.
>
> Would that work for you?
>
> Arnd
Hi Arnd,
I also sent you a series for fixing some gcc warnings for the moxtet
driver, with subject
[PATCH armsoc/drivers 0/2] Turris Mox moxtet warnings fixes
and a new driver for Turris Mox secure firmware:
[PATCH arm/drivers 0/3] Turris Mox rWTM firmware support
I probably should have been sending these with mvebu tag to be applied
by Gregory from the beginning. I am sorry.
Will you apply these or should I send them again, this time to Gregory?
Thank you.
Marek
_______________________________________________
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 v2 09/11] coresight: etm4x: docs: Update ABI doc for sysfs features added.
From: Mathieu Poirier @ 2019-09-03 22:51 UTC (permalink / raw)
To: Greg KH
Cc: Jon Corbet, Coresight ML, open list:DOCUMENTATION,
linux-arm-kernel, Suzuki K. Poulose, Mike Leach
In-Reply-To: <20190903195951.GA25008@kroah.com>
On Tue, 3 Sep 2019 at 13:59, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 29, 2019 at 10:33:19PM +0100, Mike Leach wrote:
> > Update document to include the new sysfs features added during this
> > patchset.
> >
> > Updated to reflect the new sysfs component nameing schema.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> > .../testing/sysfs-bus-coresight-devices-etm4x | 183 +++++++++++-------
> > 1 file changed, 115 insertions(+), 68 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-etm4x b/Documentation/ABI/testing/sysfs-bus-coresight-devices-etm4x
> > index 36258bc1b473..112c50ae9986 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-etm4x
> > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-etm4x
> > @@ -1,4 +1,4 @@
> > -What: /sys/bus/coresight/devices/<memory_map>.etm/enable_source
> > +What: /sys/bus/coresight/devices/etm<N>/enable_source
>
> You are renaming sysfs directories that have been around since:
>
> > Date: April 2015
>
> ???
>
> Really?
>
> That's brave.
When I worked on the coresight sysfs ABI a while back I specifically
added it at the "testing" level as I was well aware that things could
change in the future. According to the guidelines in the
documentation userspace can rely on it which was accurate since the
interface didn't change for 4 years. But the guidelines also mention
that changes can occur before the interfaces are move to stables, and
that programs are encouraged to manifest their interest by adding
their name to the "users" field.
The interface was changed in 5.2 to support coresight from ACPI and
make things easier to understand for users. It is a lot more
intuitive to associate an ETM tracer with the CPU it belongs to by
referring to the CPU number than the memory mapped address. Given the
"testing" status of the interface and the absence of registered users
I decided to move forward with the change. If "testing" is too strict
for that I suggest to add an "experimental" category where it would be
more acceptable to change things as subsystems mature.
Thanks,
Mathieu
>
>
> What tool did you just break?
>
> 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 v5 03/10] arm64: atomics: avoid out-of-line ll/sc atomics
From: Nick Desaulniers @ 2019-09-03 22:53 UTC (permalink / raw)
To: Itaru Kitayama
Cc: Mark Rutland, Will Deacon, Peter Zijlstra, Catalin Marinas,
Ard.Biesheuvel, Andrew Murray, Nathan Chancellor, Robin Murphy,
Linux ARM
In-Reply-To: <CANW9uyuRFtNKMnSwmHWt_RebJA1ADXdZfeDHc6=yaaFH2NsyWg@mail.gmail.com>
> On Wed, Sep 4, 2019 at 7:35 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>> Thanks for the report. We squashed many bugs related to asm goto, but
>> it's difficult to say with 100% certainty that the current
>> implementation is bug free. Simply throwing more exotic forms of
>> control flow at it often shake out corner cases. Thank you very much
>> for the reduced test case, and I'll look into getting a fix ready
>> hopefully in time to make the clang-9 release train.
On Tue, Sep 3, 2019 at 3:49 PM Itaru Kitayama <itaru.kitayama@gmail.com> wrote:
>
> Do you mean that you'd do a backport to Clang 9 as well as the trunk contribution?
Yes; I think the window for merging things in the 9.0 release is still
open, though they are late in the -rc cycle. If not 9.1 bugfix is
possible.
--
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 v1 1/1] i2c: iproc: Add i2c repeated start capability
From: Ray Jui @ 2019-09-03 23:11 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mark Rutland, devicetree, Lori Hikichi, Florian Fainelli,
Shivaraj Shetty, Rayagonda Kokatanur, linux-kernel, Icarus Chau,
Rob Herring, bcm-kernel-feedback-list, linux-i2c,
linux-arm-kernel
In-Reply-To: <20190831094940.GA1138@kunai>
On 8/31/19 2:49 AM, Wolfram Sang wrote:
> Hi Ray,
>
>>> With all the limitations in place, I wonder if it might be easier to
>>> implement an smbus_xfer callback instead? What is left that makes this
>>> controller more than SMBus and real I2C?
>>>
>>
>> Right. But what is the implication of using smbus_xfer instead of
>> master_xfer in our driver?
>>
>> Does it mean it will break existing functions of the i2c app that our
>> customers developed based on i2cdev (e.g., I2C_RDWR)?
>
> If the customers uses I2C_RDWR (and it cannot be mapped to i2c_smbus_*
> calls) then this is an indication that there is some I2C functionality
> left which the HW can provide. I'd be interested which one, though.
>
>>
>> 1) Does
>
> Maybe you wanted to describe it here and it got accidently cut off? >
I think you are right that the controller does not seem to support
additional I2C features in addition to SMBUS.
However, my concern of switching to the smbus_xfer API is:
1) Some customers might have used I2C_RDWR based API from i2cdev.
Changing from master_xfer to smbus_xfer may break the existing
applications that are already developed.
2) The sound subsystem I2C regmap based implementation seems to be using
i2c_ based API instead of smbus_ based API. Does this mean this will
also break most of the audio codec drivers with I2C regmap API based usage?
Thanks,
Ray
> Regards,
>
> Wolfram
>
_______________________________________________
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 3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings
From: Bjorn Andersson @ 2019-09-03 23:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: mark.rutland, robh, Jack Pham, devicetree, gregkh, linux-usb,
khasim.mohammed, linux-kernel, kishon, linux-arm-msm, andy.gross,
Jorge Ramirez, shawn.guo, linux-arm-kernel
In-Reply-To: <5d6edee5.1c69fb81.a3896.1d05@mx.google.com>
On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
> Quoting Jack Pham (2019-09-03 10:39:24)
> > On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
> > > On 8/30/19 20:28, Stephen Boyd wrote:
> > > > Quoting Bjorn Andersson (2019-08-30 09:45:20)
> > > >> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> > > >>
> > > >>>>>
> > > >>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
> > > >>>>> you should represent this external to this node and use of_graph to
> > > >>>>> query it.
> > > >>>>
> > > >>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
> > > >>>> node (that interface does not exist).
> > > >>>>
> > > >>>> rob, do you have a suggestion?
> > > >>>
> > > >>> Shouldn't the vbus supply be in the phy? Or is this a situation where
> > > >>> the phy itself doesn't have the vbus supply going to it because the PMIC
> > > >>> gets in the way and handles the vbus for the connector by having the SoC
> > > >>> communicate with the PMIC about when to turn the vbus on and off, etc?
> > > >>>
> > > >>
> > > >> That's correct, the VBUS comes out of the PMIC and goes directly to the
> > > >> connector.
> > > >>
> > > >> The additional complicating factor here is that the connector is wired
> > > >> to a USB2 phy as well, so we need to wire up detection and vbus control
> > > >> to both of them - but I think this will be fine, if we can only figure
> > > >> out a sane way of getting hold of the vbus-supply.
> > > >>
> > > >
> > > > Does it really matter to describe this situation though? Maybe it's
> > > > simpler to throw the vbus supply into the phy and control it from the
> > > > phy driver, even if it never really goes there. Or put it into the
> > > > toplevel usb controller?
> > > >
> > > that would work for me - the connector definition seemed a better way to
> > > explain the connectivity but since we cant retrieve the supply from the
> > > external node is not of much functional use.
> > >
> > > but please let me know how to proceed. shall I add the supply back to
> > > the phy?
>
> So does the vbus actually go to the phy? I thought it never went there
> and the power for the phy was different (and possibly lower in voltage).
>
No, the PHYs use different - lower voltage - supplies to operate. VBUS
is coming from a 5V supply straight to the connector and plug-detect
logic (which is passive in this design).
> >
> > Putting it in the toplevel usb node makes sense to me, since that's
> > usually the driver that knows when it's switching into host mode and
> > needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't
> > do this but there's precedent in a couple of the other dwc3 "glues"--see
> > Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
> >
> > One exception is if the PMIC is also USB-PD capable and can do power
> > role swap, in which case the VBUS control needs to be done by the TCPM,
> > so that'd be a case where having vbus-supply in the connector node might
> > make more sense.
> >
>
> The other way is to implement the code to get the vbus supply out of a
> connector. Then any driver can do the work if it knows it needs to and
> we don't have to care that the vbus isn't going somewhere. I suppose
> that would need an of_regulator_get() sort of API that can get the
> regulator out of there? Or to make the connector into a struct device
> that can get the regulator out per some generic connector driver and
> then pass it through to the USB controller when it asks for it. Maybe
> try to prototype that out?
>
The examples given in the DT bindings describes the connector as a child
of a PMIC, with of_graph somehow tying it to the various inputs. But in
these examples vbus is handled by implicitly inside the MFD, where
extcon is informed about the plug event they toggle vbus as well.
In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
proposal will trickle down to the PHY and become a regulator calls on
either some external regulator or more typically one of the chargers in
the system.
So if we come up with a struct device for the connector and some API for
toggling the vbus we're going to have to fairly abstract entities
representing pretty much the same thing - and in a design with a mux we
would have a different setup.
Regards,
Bjorn
_______________________________________________
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 v2 1/4] mmc: sdhci-of-aspeed: Fix link failure for SPARC
From: Andrew Jeffery @ 2019-09-04 0:02 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-aspeed, Arnd Bergmann, OpenBMC Maillist, linux-mmc,
Adrian Hunter, Linux Kernel Mailing List, Joel Stanley, Linux ARM,
kbuild test robot
In-Reply-To: <CAPDyKFpWJu3RH4TWoO_wcJq0LDrM_fAUfsCC==e8O_6A8dLhiA@mail.gmail.com>
On Wed, 4 Sep 2019, at 00:18, Ulf Hansson wrote:
> On Mon, 2 Sep 2019 at 07:26, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote:
> > > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > Resolves the following build error reported by the 0-day bot:
> > > >
> > > > ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
> > > >
> > > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the
> > > > callsite to maintain build coverage for the rest of the driver.
> > > >
> > > > Reported-by: kbuild test robot <lkp@intel.com>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > > drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++----------
> > > > 1 file changed, 25 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > index d5acb5afc50f..96ca494752c5 100644
> > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = {
> > > > .remove = aspeed_sdhci_remove,
> > > > };
> > > >
> > > > -static int aspeed_sdc_probe(struct platform_device *pdev)
> > > > -
> > > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev)
> > > > {
> > > > +#if defined(CONFIG_OF_ADDRESS)
> > >
> > > This is going to be untested code forever, as no one will be running
> > > on a chip with this hardware present but OF_ADDRESS disabled.
> > >
> > > How about we make the driver depend on OF_ADDRESS instead?
> >
> > Testing is split into two pieces here: compile-time and run-time.
> > Clearly the run-time behaviour is going to be broken on configurations
> > without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think
> > that means we shouldn't allow it to be compiled in that case
> > (e.g. CONFIG_COMPILE_TEST performs a similar role).
> >
> > With respect to compile-time it's possible to compile either path as
> > demonstrated by the build failure report.
> >
> > Having said that there's no reason we couldn't do what you suggest,
> > just it wasn't the existing solution pattern for the problem (there are
> > several other drivers that suffered the same bug that were fixed in the
> > style of this patch). Either way works, it's all somewhat academic.
> > Your suggestion is more obvious in terms of correctness, but this
> > patch is basically just code motion (the only addition is the `#if`/
> > `#endif` lines over what was already there if we disregard the
> > function declaration/invocation). I'll change it if there are further
> > complaints and a reason to do a v3.
>
> I am in favor of Joel's suggestion as I don't really like having
> ifdefs bloating around in the driver (unless very good reasons).
> Please re-spin a v3.
>
> Another option is to implement stub function and to deal with error
> codes, but that sounds more like a long term thingy, if even
> applicable here.
No worries then, will post a respin shortly.
Andrew
_______________________________________________
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 v5, 02/32] dt-bindings: mediatek: add ovl_2l description for mt8183 display
From: CK Hu @ 2019-09-04 1:44 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567090254-15566-3-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Thu, 2019-08-29 at 22:50 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
Applied to mediatek-drm-next-5.5 [1], thanks.
[1]
https://github.com/ckhu-mediatek/linux.git-tags/commits/mediatek-drm-next-5.5
Regards,
CK
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> .../bindings/display/mediatek/mediatek,disp.txt | 27 +++++++++++-----------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 464b92f..8c4700f 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -27,19 +27,20 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>
> Required properties (all function blocks):
> - compatible: "mediatek,<chip>-disp-<function>", one of
> - "mediatek,<chip>-disp-ovl" - overlay (4 layers, blending, csc)
> - "mediatek,<chip>-disp-rdma" - read DMA / line buffer
> - "mediatek,<chip>-disp-wdma" - write DMA
> - "mediatek,<chip>-disp-color" - color processor
> - "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> - "mediatek,<chip>-disp-gamma" - gamma correction
> - "mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
> - "mediatek,<chip>-disp-split" - split stream to two encoders
> - "mediatek,<chip>-disp-ufoe" - data compression engine
> - "mediatek,<chip>-dsi" - DSI controller, see mediatek,dsi.txt
> - "mediatek,<chip>-dpi" - DPI controller, see mediatek,dpi.txt
> - "mediatek,<chip>-disp-mutex" - display mutex
> - "mediatek,<chip>-disp-od" - overdrive
> + "mediatek,<chip>-disp-ovl" - overlay (4 layers, blending, csc)
> + "mediatek,<chip>-disp-ovl-2l" - overlay (2 layers, blending, csc)
> + "mediatek,<chip>-disp-rdma" - read DMA / line buffer
> + "mediatek,<chip>-disp-wdma" - write DMA
> + "mediatek,<chip>-disp-color" - color processor
> + "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> + "mediatek,<chip>-disp-gamma" - gamma correction
> + "mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
> + "mediatek,<chip>-disp-split" - split stream to two encoders
> + "mediatek,<chip>-disp-ufoe" - data compression engine
> + "mediatek,<chip>-dsi" - DSI controller, see mediatek,dsi.txt
> + "mediatek,<chip>-dpi" - DPI controller, see mediatek,dpi.txt
> + "mediatek,<chip>-disp-mutex" - display mutex
> + "mediatek,<chip>-disp-od" - overdrive
> the supported chips are mt2701, mt2712 and mt8173.
> - reg: Physical base address and length of the function block register space
> - interrupts: The interrupt signal from the function block (required, except for
_______________________________________________
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 v5, 03/32] dt-bindings: mediatek: add ccorr description for mt8183 display
From: CK Hu @ 2019-09-04 1:44 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567090254-15566-4-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Thu, 2019-08-29 at 22:50 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
Applied to mediatek-drm-next-5.5 [1], thanks.
[1]
https://github.com/ckhu-mediatek/linux.git-tags/commits/mediatek-drm-next-5.5
Regards,
CK
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> Reviewed-by: Rob Herring <robh at kernel.org>
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 8c4700f..cf5fb08 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -31,6 +31,7 @@ Required properties (all function blocks):
> "mediatek,<chip>-disp-ovl-2l" - overlay (2 layers, blending, csc)
> "mediatek,<chip>-disp-rdma" - read DMA / line buffer
> "mediatek,<chip>-disp-wdma" - write DMA
> + "mediatek,<chip>-disp-ccorr" - color correction
> "mediatek,<chip>-disp-color" - color processor
> "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> "mediatek,<chip>-disp-gamma" - gamma correction
_______________________________________________
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 v5, 04/32] dt-bindings: mediatek: add dither description for mt8183 display
From: CK Hu @ 2019-09-04 1:45 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567090254-15566-5-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Thu, 2019-08-29 at 22:50 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
Applied to mediatek-drm-next-5.5 [1], thanks.
[1]
https://github.com/ckhu-mediatek/linux.git-tags/commits/mediatek-drm-next-5.5
Regards,
CK
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> Reviewed-by: Rob Herring <robh at kernel.org>
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index cf5fb08..afd3c90 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -33,6 +33,7 @@ Required properties (all function blocks):
> "mediatek,<chip>-disp-wdma" - write DMA
> "mediatek,<chip>-disp-ccorr" - color correction
> "mediatek,<chip>-disp-color" - color processor
> + "mediatek,<chip>-disp-dither" - dither
> "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> "mediatek,<chip>-disp-gamma" - gamma correction
> "mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
_______________________________________________
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 v5, 05/32] dt-bindings: mediatek: add mutex description for mt8183 display
From: CK Hu @ 2019-09-04 1:45 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567090254-15566-6-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Thu, 2019-08-29 at 22:50 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add mutex description for mt8183 display
Applied to mediatek-drm-next-5.5 [1], thanks.
[1]
https://github.com/ckhu-mediatek/linux.git-tags/commits/mediatek-drm-next-5.5
Regards,
CK
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index afd3c90..c7e2eb8 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -52,6 +52,7 @@ Required properties (all function blocks):
> For most function blocks this is just a single clock input. Only the DSI and
> DPI controller nodes have multiple clock inputs. These are documented in
> mediatek,dsi.txt and mediatek,dpi.txt, respectively.
> + An exception is that the mt8183 mutex is always free running with no clocks property.
>
> Required properties (DMA function blocks):
> - compatible: Should be one of
_______________________________________________
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 v3 0/7] add support USB for MT8183
From: Chunfeng Yun @ 2019-09-04 1:54 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Mark Rutland, devicetree, Mathias Nyman, linux-usb, linux-kernel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567150854-30033-1-git-send-email-chunfeng.yun@mediatek.com>
Hi Greg,
Please don't try to pick up this series, the dependent ones are still
under public review, I'll fix build warning and send out new version
after the dependent ones are applied
Sorry for inconvenience
Thanks
On Fri, 2019-08-30 at 15:40 +0800, Chunfeng Yun wrote:
> This series support USB DRD controller and enable it's remote
> wakeup functoin for MT8183, they depend on the following
> series patches:
>
> 1. this series add support MT6358 PMIC
> [v5,01/10] mfd: mt6397: clean up code
> https://patchwork.kernel.org/patch/11110487/
>
> 2. this series add support pericfg syscon
> [v2,1/2] dt-bindings: clock: mediatek: add pericfg for MT8183
> https://patchwork.kernel.org/patch/11118183/
>
> 3. add property mediatek,discth for tphy
> [06/11] phy: phy-mtk-tphy: add a property for disconnect threshold
> https://patchwork.kernel.org/patch/11110695/
>
> v3 changes:
> 1. changes micros define
> 2. remove #reset-cell
> 3. update dependent series
>
> v2 changes:
> add patch [7/7]
>
> Chunfeng Yun (7):
> dt-bindings: usb: mtu3: support USB wakeup for MT8183
> dt-bindings: usb: mtk-xhci: support USB wakeup for MT8183
> usb: mtu3: support ip-sleep wakeup for MT8183
> usb: mtk-xhci: support ip-sleep wakeup for MT8183
> arm64: dts: mt8183: add usb and phy nodes
> arm64: dts: mt8183: enable USB remote wakeup
> arm64: dts: mt8183: tune disconnect threshold of u2phy
>
> .../bindings/usb/mediatek,mtk-xhci.txt | 1 +
> .../devicetree/bindings/usb/mediatek,mtu3.txt | 1 +
> arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 23 +++++++
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 63 +++++++++++++++++++
> drivers/usb/host/xhci-mtk.c | 14 ++++-
> drivers/usb/mtu3/mtu3_host.c | 14 ++++-
> 6 files changed, 114 insertions(+), 2 deletions(-)
>
_______________________________________________
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: fix page faults in do_alignment
From: Jing Xiangfeng @ 2019-09-04 2:17 UTC (permalink / raw)
To: Eric W. Biederman, Russell King - ARM Linux admin
Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, sakari.ailus,
bhelgaas, tglx, linux-arm-kernel
In-Reply-To: <87mufmioqv.fsf@x220.int.ebiederm.org>
On 2019/9/3 1:36, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>
>> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>>>
>>>> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>>>>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>>>>>
>>>>>> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>>>>>>> The function do_alignment can handle misaligned address for user and
>>>>>>> kernel space. If it is a userspace access, do_alignment may fail on
>>>>>>> a low-memory situation, because page faults are disabled in
>>>>>>> probe_kernel_address.
>>>>>>>
>>>>>>> Fix this by using __copy_from_user stead of probe_kernel_address.
>>>>>>>
>>>>>>> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>>>>>>> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>>>>>>
>>>>>> NAK.
>>>>>>
>>>>>> The "scheduling while atomic warning in alignment handling code" is
>>>>>> caused by fixing up the page fault while trying to handle the
>>>>>> mis-alignment fault generated from an instruction in atomic context.
>>>>>>
>>>>>> Your patch re-introduces that bug.
>>>>>
>>>>> And the patch that fixed scheduling while atomic apparently introduced a
>>>>> regression. Admittedly a regression that took 6 years to track down but
>>>>> still.
>>>>
>>>> Right, and given the number of years, we are trading one regression for
>>>> a different regression. If we revert to the original code where we
>>>> fix up, we will end up with people complaining about a "new" regression
>>>> caused by reverting the previous fix. Follow this policy and we just
>>>> end up constantly reverting the previous revert.
>>>>
>>>> The window is very small - the page in question will have had to have
>>>> instructions read from it immediately prior to the handler being entered,
>>>> and would have had to be made "old" before subsequently being unmapped.
>>>
>>>> Rather than excessively complicating the code and making it even more
>>>> inefficient (as in your patch), we could instead retry executing the
>>>> instruction when we discover that the page is unavailable, which should
>>>> cause the page to be paged back in.
>>>
>>> My patch does not introduce any inefficiencies. It onlys moves the
>>> check for user_mode up a bit. My patch did duplicate the code.
>>>
>>>> If the page really is unavailable, the prefetch abort should cause a
>>>> SEGV to be raised, otherwise the re-execution should replace the page.
>>>>
>>>> The danger to that approach is we page it back in, and it gets paged
>>>> back out before we're able to read the instruction indefinitely.
>>>
>>> I would think either a little code duplication or a function that looks
>>> at user_mode(regs) and picks the appropriate kind of copy to do would be
>>> the best way to go. Because what needs to happen in the two cases for
>>> reading the instruction are almost completely different.
>>
>> That is what I mean. I'd prefer to avoid that with the large chunk of
>> code. How about instead adding a local replacement for
>> probe_kernel_address() that just sorts out the reading, rather than
>> duplicating all the code to deal with thumb fixup.
>
> So something like this should be fine?
>
> Jing Xiangfeng can you test this please? I think this fixes your issue
> but I don't currently have an arm development box where I could test this.
>
Yes, I have tested and it can fix my issue in kernel 4.19.
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 04b36436cbc0..b07d17ca0ae5 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -767,6 +767,23 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
> return NULL;
> }
>
> +static inline unsigned long
> +copy_instr(bool umode, void *dst, unsigned long instrptr, size_t size)
> +{
> + unsigned long result;
> + if (umode) {
> + void __user *src = (void *)instrptr;
> + result = copy_from_user(dst, src, size);
> + } else {
> + void *src = (void *)instrptr;
> + result = probe_kernel_read(dst, src, size);
> + }
> + /* Convert short reads into -EFAULT */
> + if ((result >= 0) && (result < size))
> + result = -EFAULT;
> + return result;
> +}
> +
> static int
> do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> {
> @@ -778,22 +795,24 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> u16 tinstr = 0;
> int isize = 4;
> int thumb2_32b = 0;
> + bool umode;
>
> if (interrupts_enabled(regs))
> local_irq_enable();
>
> instrptr = instruction_pointer(regs);
> + umode = user_mode(regs);
>
> if (thumb_mode(regs)) {
> - u16 *ptr = (u16 *)(instrptr & ~1);
> - fault = probe_kernel_address(ptr, tinstr);
> + unsigned long tinstrptr = instrptr & ~1;
> + fault = copy_instr(umode, &tinstr, tinstrptr, 2);
> tinstr = __mem_to_opcode_thumb16(tinstr);
> if (!fault) {
> if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
> IS_T32(tinstr)) {
> /* Thumb-2 32-bit */
> u16 tinst2 = 0;
> - fault = probe_kernel_address(ptr + 1, tinst2);
> + fault = copy_instr(umode, &tinst2, tinstrptr + 2, 2);
> tinst2 = __mem_to_opcode_thumb16(tinst2);
> instr = __opcode_thumb32_compose(tinstr, tinst2);
> thumb2_32b = 1;
> @@ -803,7 +822,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> }
> }
> } else {
> - fault = probe_kernel_address((void *)instrptr, instr);
> + fault = copy_instr(umode, &instr, instrptr, 4);
> instr = __mem_to_opcode_arm(instr);
> }
>
> @@ -812,7 +831,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> goto bad_or_fault;
> }
>
> - if (user_mode(regs))
> + if (umode)
> goto user;
>
> ai_sys += 1;
>
> .
>
_______________________________________________
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] mmc: sdhci-of-aspeed: Depend on CONFIG_OF_ADDRESS
From: Andrew Jeffery @ 2019-09-04 2:21 UTC (permalink / raw)
To: linux-mmc
Cc: ulf.hansson, kbuild test robot, linux-aspeed, Andrew Jeffery,
openbmc, adrian.hunter, linux-kernel, joel, linux-arm-kernel
Resolves the following build error reported by the 0-day bot:
ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined!
SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Depend on
CONFIG_OF_ADDRESS to ensure the driver is only built for supported
configurations.
Fixes: 2d28dbe042f4 ("mmc: sdhci-of-aspeed: Add support for the ASPEED SD controller")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
v2 was a series of 4 patches, three of which were applied leaving this build
fix to be reworked. The v2 series can be found here:
https://patchwork.ozlabs.org/cover/1156457/
drivers/mmc/host/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 0f8a230de2f3..3a52f5703286 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -157,7 +157,7 @@ config MMC_SDHCI_OF_ARASAN
config MMC_SDHCI_OF_ASPEED
tristate "SDHCI OF support for the ASPEED SDHCI controller"
depends on MMC_SDHCI_PLTFM
- depends on OF
+ depends on OF && OF_ADDRESS
help
This selects the ASPEED Secure Digital Host Controller Interface.
--
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
* Re: [PATCH 1/1] soc: qcom: geni: Provide parameter error checking
From: Bjorn Andersson @ 2019-09-04 3:19 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-arm-msm, agross, linux-kernel, linux-arm-kernel
In-Reply-To: <20190903135052.13827-1-lee.jones@linaro.org>
On Tue 03 Sep 06:50 PDT 2019, Lee Jones wrote:
> When booting with ACPI, the Geni Serial Engine is not set as the I2C/SPI
> parent and thus, the wrapper (parent device) is unassigned. This causes
> the kernel to crash with a null dereference error.
>
Now I see what you did in 8bc529b25354; i.e. stubbed all the other calls
between the SE and wrapper.
Do you think it would be possible to resolve the _DEP link to QGP[01]
somehow? For the clocks workarounds this could be resolved by us
representing that relationship using device_link and just rely on
pm_runtime to propagate the clock state.
For the DMA operation, iiuc it's the wrapper that implements the DMA
engine involved, but I'm guessing the main reason for mapping buffers on
the wrapper is so that it ends up being associated with the iommu
context of the wrapper.
Are the SMMU contexts at all represented in the ACPI world and if so do
you know how the wrapper vs SEs are bound to contexts? Can we map on
se->dev when wrapper is NULL (or perhaps always?)?
Regards,
Bjorn
> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> Since we are already at -rc7 this patch should be processed ASAP - thank you.
>
> drivers/soc/qcom/qcom-geni-se.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index d5cf953b4337..7d622ea1274e 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -630,6 +630,9 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
> struct geni_wrapper *wrapper = se->wrapper;
> u32 val;
>
> + if (!wrapper)
> + return -EINVAL;
> +
> *iova = dma_map_single(wrapper->dev, buf, len, DMA_TO_DEVICE);
> if (dma_mapping_error(wrapper->dev, *iova))
> return -EIO;
> @@ -663,6 +666,9 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
> struct geni_wrapper *wrapper = se->wrapper;
> u32 val;
>
> + if (!wrapper)
> + return -EINVAL;
> +
> *iova = dma_map_single(wrapper->dev, buf, len, DMA_FROM_DEVICE);
> if (dma_mapping_error(wrapper->dev, *iova))
> return -EIO;
> --
> 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: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-09-04 3:38 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩), srv_heupstream,
Po-Yang Huang (黃柏陽), mchehab@kernel.org,
suleiman@chromium.org, shik@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
matthias.bgg@gmail.com, Christie Yu (游雅惠),
Frederic Chen (陳俊元), hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAAFQd5DiPcUxd+R-v_-BdRx+QqZ35Riii_jpgbqr5mc3BnQvDw@mail.gmail.com>
Hi Tomasz,
On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote:
> On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote:
> > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote:
> > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote:
> > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen
> > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > >
> > > > > > > > Hi Tomasz,
> > > > > > > >
> > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote:
> > > > > > > > > Hi Jerry,
> > > > > > > > >
> > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen
> > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tomasz,
> > > > > > > > > >
> > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> > > > > > > > > > > Hi Jerry,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
[snip]
> > > > > > > > > [snip]
> > > > > > > > >
> > > > > > > > > > > > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > > > > > + struct vb2_buffer *vb;
> > > > > > > > > > >
> > > > > > > > > > > How do we guarantee here that the hardware isn't still accessing the buffers
> > > > > > > > > > > removed below?
> > > > > > > > > > >
> > > > > > > > > > Maybe we can check the driver state flag and aborting the unfinished
> > > > > > > > > > jobs?
> > > > > > > > > > (fd_hw->state == FD_ENQ)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes, we need to either cancel or wait for the currently processing
> > > > > > > > > job. It depends on hardware capabilities, but cancelling is generally
> > > > > > > > > preferred for the lower latency.
> > > > > > > > >
> > > > > > > > Ok, it the state is ENQ, then we can disable the FD hw by controlling
> > > > > > > > the registers.
> > > > > > > >
> > > > > > > > for example:
> > > > > > > > writel(0x0, fd->fd_base + FD_HW_ENABLE);
> > > > > > > > writel(0x0, fd->fd_base + FD_INT_EN);
> > > > > > > >
> > > > > > >
> > > > > > > What's exactly the effect of writing 0 to FD_HW_ENABLE?
> > > > > > >
> > > > > > Sorry, my last reply didn't solve the question,
> > > > > > we should implement a mtk_fd_job_abort() for v4l2_m2m_ops().
> > > > > >
> > > > > > which is able to readl_poll_timeout_atomic()
> > > > > > and check the HW busy bits in the register FD_INT_EN;
> > > > > >
> > > > > > if they are not cleared until timeout, we could handle the last
> > > > > > processing job.
> > > > > > Otherwise, the FD irq handler should have handled the last processing
> > > > > > job and we could continue the stop_streaming().
> > > > > >
> > > > > > For job_abort():
> > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > {
> > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > u32 val;
> > > > > > u32 ret;
> > > > > >
> > > > > > ret = readl_poll_timeout_atomic(fd->fd_base + MTK_FD_REG_OFFSET_INT_EN,
> > > > > > val,
> > > > > > (val & MTK_FD_HW_BUSY_MASK) ==
> > > > > > MTK_FD_HW_STATE_IS_BUSY,
> > > > > > USEC_PER_MSEC, MTK_FD_STOP_HW_TIMEOUT);
> > > > >
> > > > > Hmm, would it be possible to avoid the busy wait by having a
> > > > > completion that could be signalled from the interrupt handler?
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > >
> > > > I suppose that would be wakeup a wait queue in the interrupt handler,
> > > > the the wait_event_interrupt_timeout() will be used in here and system
> > > > suspend e.g. mtk_fd_suspend().
> > >
> > > Yes, that should work.
> > >
> > > > Or do you suggest to wait_event_interrupt_timeout() every frame in the
> > > > mtk_fd_ipi_handler()?
> > >
> > > Nope, we shouldn't need that.
> > >
> > > > I think maybe the readl_poll_timeout_atomic would be good enough.
> > > >
> > >
> > > Not really. Busy waiting should be avoided as much as possible. What's
> > > the point of entering suspend if you end up burning the power by
> > > spinning the CPU for some milliseconds?
> > >
> > Ok, I see, busy waiting is not a good idea, I will use the wait queue
> > instead. the job abort will refine as following:
> >
> > static void mtk_fd_job_abort(void *priv)
> > {
> > struct mtk_fd_ctx *ctx = priv;
> > struct mtk_fd_dev *fd = ctx->fd_dev;
> > u32 ret;
> >
> > ret = wait_event_interruptible_timeout
> > (fd->wq, (fd->fd_irq_result & MTK_FD_HW_IRQ_MASK),
> > usecs_to_jiffies(50000));
> > if (ret)
> > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> > dev_dbg(fd->dev, "%s, ret:%d\n", __func__, ret);
> >
> > fd->fd_irq_result = 0;
> > }
> >
> > static struct v4l2_m2m_ops fd_m2m_ops = {
> > .device_run = mtk_fd_device_run,
> > .job_abort = mtk_fd_job_abort,
>
> I'm not sure we should be using the functon above as the .job_abort
> callback. It's expected to abort the job, but we're just waiting for
> it to finish. I think we should just call mtk_fd_job_abort() manually
> from .stop_streaming.
>
Ok, I will fix it.
> > };
> >
> > and we could use it in suspend.
> > static int mtk_fd_suspend(struct device *dev)
> > {
> > struct mtk_fd_dev *fd = dev_get_drvdata(dev);
> >
> > if (pm_runtime_suspended(dev))
> > return 0;
> >
> > if (fd->fd_stream_count)
> > mtk_fd_job_abort(fd->ctx);
> >
> > /* suspend FD HW */
> > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN);
> > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE);
> > clk_disable_unprepare(fd->fd_clk);
> > dev_dbg(dev, "%s:disable clock\n", __func__);
> >
> > return 0;
> > }
> >
> > static irqreturn_t mtk_fd_irq(int irq, void *data)
> > {
> > struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
> >
> > fd->fd_irq_result = readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
> > wake_up_interruptible(&fd->wq);
>
> The wake up should be done at the very end of this function. Otherwise
> we end up with m2m framework racing with the mtk_fd_hw_job_finish()
> below.
>
> Also, I'd use a completion here rather than an open coded wait and
> wake-up. The driver could reinit_completion() before queuing a job to
> the hardware and the IRQ handler would complete(). There would be no
> need to store the IRQ flags in driver data anymore.
Ok, It will be refined as following:
suspend and stop streaming will call mtk_fd_job_abort()
static void mtk_fd_job_abort(struct mtk_fd_dev *fd)
{
u32 ret;
ret = wait_for_completion_timeout(&fd->fd_irq_done,
msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
if (ret)
mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
}
complete at irq handler
static irqreturn_t mtk_fd_irq(int irq, void *data)
{
struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
/* must read this register otherwise HW will keep sending irq */
readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
fd->output->number = readl(fd->fd_base + MTK_FD_REG_OFFSET_RESULT);
dev_dbg(fd->dev, "mtk_fd_face_num:%d\n", fd->output->number);
pm_runtime_put((fd->dev));
mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_DONE);
complete(&fd->fd_irq_done);
return IRQ_HANDLED;
}
and reinit_completion before time before sending a job to HW
static int mtk_fd_hw_job_exec(struct mtk_fd_dev *fd,
struct fd_enq_param *fd_param)
{
struct ipi_message fd_ipi_msg;
int ret;
pm_runtime_get_sync((fd->dev));
reinit_completion(&fd->fd_irq_done);
fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_ENQUEUE;
memcpy(&fd_ipi_msg.fd_enq_param, fd_param, sizeof(struct
fd_enq_param));
ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT);
if (ret) {
pm_runtime_put((fd->dev));
mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
return ret;
}
return 0;
}
>
> > fd->output->number = readl(fd->fd_base + MTK_FD_REG_OFFSET_RESULT);
> > dev_dbg(fd->dev, "mtk_fd_face_num:%d\n", fd->output->number);
> >
> > pm_runtime_put((fd->dev));
> > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_DONE);
> > return IRQ_HANDLED;
> > }
> > > >
> > > > One more thing, for the mtk_fd_video_device_register()
> > > > Sorry that I would need to use intermediate variable here since the 80
> > > > columns check.
> > > >
> > > > function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
> > > > ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> > >
> > > Why not just make it like this:
> > >
> > > ret = v4l2_m2m_register_media_controller(m2m_dev,
> > > MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > >
> > > The above line is aligned using tabs so that its end is as close to
> > > the 80 character boundary as possible.
> > >
> > I tried but the checkpatch script still gave me a check saying align to
> > the scope, I will refine as following:
> >
> > ret = v4l2_m2m_register_media_controller
> > (m2m_dev, vfd, MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
>
> Please ignore that checkpatch warning, it sometimes gives false
> positives. The above looks clearly worse and less consistent with
> kernel coding style than what I suggested.
>
Ok, I see, I will fix it.
Thanks and best regards,
Jerry
> Best regards,
> Tomasz
_______________________________________________
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 v3 06/14] media: mtk-mdp: Get rid of mtk_smi_larb_get/put
From: houlong wei @ 2019-09-04 4:07 UTC (permalink / raw)
To: Yong Wu, Matthias Brugger, Joerg Roedel, Rob Herring
Cc: youlin.pei, devicetree, Nicolas Boichat, cui.zhang,
srv_heupstream, chao.hao, Will Deacon, linux-kernel, Evan Green,
houlong.wei, Tomasz Figa, iommu, Matthias Kaehlcke,
linux-mediatek, yong.wu, minghsiu.tsai, ming-fan.chen, anan.sun,
Robin Murphy, linux-arm-kernel
In-Reply-To: <mailman.21807.1567503573.19300.linux-mediatek@lists.infradead.org>
Hi, Yong,
I have inline comment below.
> MediaTek IOMMU has already added the device_link between the consumer
> and smi-larb device. If the mdp device call the pm_runtime_get_sync,
> the smi-larb's pm_runtime_get_sync also be called automatically.
>
> CC: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> Reviewed-by: Evan Green <evgreen@chromium.org>
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 38 ---------------------------
> drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 --
> drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 1 -
> 3 files changed, 41 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index 9afe816..5985a9b 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -9,7 +9,6 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_platform.h>
> -#include <soc/mediatek/smi.h>
>
> #include "mtk_mdp_comp.h"
>
> @@ -58,14 +57,6 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> {
> int i, err;
>
> - if (comp->larb_dev) {
> - err = mtk_smi_larb_get(comp->larb_dev);
> - if (err)
> - dev_err(dev,
> - "failed to get larb, err %d. type:%d id:%d\n",
> - err, comp->type, comp->id);
> - }
In previous design,mtk_mdp_comp_clock_on() is called by each MDP
hardware component, and mtk_smi_larb_get() is also called for each MDP
hardware component which accesses DRAM via SMI larb.
Since mdp device only contains mdp_rdma component, so
pm_runtime_get_sync() will ignore other smi-larb clock. We need consider
how to enable clocks of other smi-larb associated with other mdp
component, e.g. mdp_wdma, mdp_wrot.
> for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
> if (IS_ERR(comp->clk[i]))
> continue;
> @@ -86,16 +77,11 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
> continue;
> clk_disable_unprepare(comp->clk[i]);
> }
> -
> - if (comp->larb_dev)
> - mtk_smi_larb_put(comp->larb_dev);
> }
>
> int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
> struct mtk_mdp_comp *comp, enum mtk_mdp_comp_id comp_id)
> {
> - struct device_node *larb_node;
> - struct platform_device *larb_pdev;
> int i;
>
> if (comp_id < 0 || comp_id >= MTK_MDP_COMP_ID_MAX) {
> @@ -116,30 +102,6 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
> break;
> }
>
> - /* Only DMA capable components need the LARB property */
> - comp->larb_dev = NULL;
> - if (comp->type != MTK_MDP_RDMA &&
> - comp->type != MTK_MDP_WDMA &&
> - comp->type != MTK_MDP_WROT)
> - return 0;
> -
> - larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> - if (!larb_node) {
> - dev_err(dev,
> - "Missing mediadek,larb phandle in %pOF node\n", node);
> - return -EINVAL;
> - }
> -
> - larb_pdev = of_find_device_by_node(larb_node);
> - if (!larb_pdev) {
> - dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
> - of_node_put(larb_node);
> - return -EPROBE_DEFER;
> - }
> - of_node_put(larb_node);
> -
> - comp->larb_dev = &larb_pdev->dev;
> -
> return 0;
> }
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> index 998a4b9..a2da8df 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> @@ -39,7 +39,6 @@ enum mtk_mdp_comp_id {
> * @dev_node: component device node
> * @clk: clocks required for component
> * @regs: Mapped address of component registers.
> - * @larb_dev: SMI device required for component
> * @type: component type
> * @id: component ID
> */
> @@ -47,7 +46,6 @@ struct mtk_mdp_comp {
> struct device_node *dev_node;
> struct clk *clk[2];
> void __iomem *regs;
> - struct device *larb_dev;
> enum mtk_mdp_comp_type type;
> enum mtk_mdp_comp_id id;
> };
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index fc9faec..c237ed9 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -17,7 +17,6 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/workqueue.h>
> -#include <soc/mediatek/smi.h>
>
> #include "mtk_mdp_core.h"
> #include "mtk_mdp_m2m.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: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2019-09-04 4:15 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩), srv_heupstream,
Po-Yang Huang (黃柏陽), mchehab@kernel.org,
suleiman@chromium.org, shik@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
matthias.bgg@gmail.com, Christie Yu (游雅惠),
Frederic Chen (陳俊元), hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <1567568281.18318.80.camel@mtksdccf07>
On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen
<Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote:
> > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote:
> > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > >
> > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote:
> > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > >
> > > > > > > Hi Tomasz,
> > > > > > >
> > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote:
> > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen
> > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Tomasz,
> > > > > > > > >
> > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote:
> > > > > > > > > > Hi Jerry,
> > > > > > > > > >
> > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen
> > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> [snip]
> > > > > > > > > > [snip]
> > > > > > > > > >
> > > > > > > > > > > > > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > > > > > > +{
> > > > > > > > > > > > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > > > > > > + struct vb2_buffer *vb;
> > > > > > > > > > > >
> > > > > > > > > > > > How do we guarantee here that the hardware isn't still accessing the buffers
> > > > > > > > > > > > removed below?
> > > > > > > > > > > >
> > > > > > > > > > > Maybe we can check the driver state flag and aborting the unfinished
> > > > > > > > > > > jobs?
> > > > > > > > > > > (fd_hw->state == FD_ENQ)
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Yes, we need to either cancel or wait for the currently processing
> > > > > > > > > > job. It depends on hardware capabilities, but cancelling is generally
> > > > > > > > > > preferred for the lower latency.
> > > > > > > > > >
> > > > > > > > > Ok, it the state is ENQ, then we can disable the FD hw by controlling
> > > > > > > > > the registers.
> > > > > > > > >
> > > > > > > > > for example:
> > > > > > > > > writel(0x0, fd->fd_base + FD_HW_ENABLE);
> > > > > > > > > writel(0x0, fd->fd_base + FD_INT_EN);
> > > > > > > > >
> > > > > > > >
> > > > > > > > What's exactly the effect of writing 0 to FD_HW_ENABLE?
> > > > > > > >
> > > > > > > Sorry, my last reply didn't solve the question,
> > > > > > > we should implement a mtk_fd_job_abort() for v4l2_m2m_ops().
> > > > > > >
> > > > > > > which is able to readl_poll_timeout_atomic()
> > > > > > > and check the HW busy bits in the register FD_INT_EN;
> > > > > > >
> > > > > > > if they are not cleared until timeout, we could handle the last
> > > > > > > processing job.
> > > > > > > Otherwise, the FD irq handler should have handled the last processing
> > > > > > > job and we could continue the stop_streaming().
> > > > > > >
> > > > > > > For job_abort():
> > > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > > {
> > > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > u32 val;
> > > > > > > u32 ret;
> > > > > > >
> > > > > > > ret = readl_poll_timeout_atomic(fd->fd_base + MTK_FD_REG_OFFSET_INT_EN,
> > > > > > > val,
> > > > > > > (val & MTK_FD_HW_BUSY_MASK) ==
> > > > > > > MTK_FD_HW_STATE_IS_BUSY,
> > > > > > > USEC_PER_MSEC, MTK_FD_STOP_HW_TIMEOUT);
> > > > > >
> > > > > > Hmm, would it be possible to avoid the busy wait by having a
> > > > > > completion that could be signalled from the interrupt handler?
> > > > > >
> > > > > > Best regards,
> > > > > > Tomasz
> > > > >
> > > > > I suppose that would be wakeup a wait queue in the interrupt handler,
> > > > > the the wait_event_interrupt_timeout() will be used in here and system
> > > > > suspend e.g. mtk_fd_suspend().
> > > >
> > > > Yes, that should work.
> > > >
> > > > > Or do you suggest to wait_event_interrupt_timeout() every frame in the
> > > > > mtk_fd_ipi_handler()?
> > > >
> > > > Nope, we shouldn't need that.
> > > >
> > > > > I think maybe the readl_poll_timeout_atomic would be good enough.
> > > > >
> > > >
> > > > Not really. Busy waiting should be avoided as much as possible. What's
> > > > the point of entering suspend if you end up burning the power by
> > > > spinning the CPU for some milliseconds?
> > > >
> > > Ok, I see, busy waiting is not a good idea, I will use the wait queue
> > > instead. the job abort will refine as following:
> > >
> > > static void mtk_fd_job_abort(void *priv)
> > > {
> > > struct mtk_fd_ctx *ctx = priv;
> > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > u32 ret;
> > >
> > > ret = wait_event_interruptible_timeout
> > > (fd->wq, (fd->fd_irq_result & MTK_FD_HW_IRQ_MASK),
> > > usecs_to_jiffies(50000));
> > > if (ret)
> > > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> > > dev_dbg(fd->dev, "%s, ret:%d\n", __func__, ret);
> > >
> > > fd->fd_irq_result = 0;
> > > }
> > >
> > > static struct v4l2_m2m_ops fd_m2m_ops = {
> > > .device_run = mtk_fd_device_run,
> > > .job_abort = mtk_fd_job_abort,
> >
> > I'm not sure we should be using the functon above as the .job_abort
> > callback. It's expected to abort the job, but we're just waiting for
> > it to finish. I think we should just call mtk_fd_job_abort() manually
> > from .stop_streaming.
> >
>
> Ok, I will fix it.
>
> > > };
> > >
> > > and we could use it in suspend.
> > > static int mtk_fd_suspend(struct device *dev)
> > > {
> > > struct mtk_fd_dev *fd = dev_get_drvdata(dev);
> > >
> > > if (pm_runtime_suspended(dev))
> > > return 0;
> > >
> > > if (fd->fd_stream_count)
> > > mtk_fd_job_abort(fd->ctx);
> > >
> > > /* suspend FD HW */
> > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN);
> > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE);
> > > clk_disable_unprepare(fd->fd_clk);
> > > dev_dbg(dev, "%s:disable clock\n", __func__);
> > >
> > > return 0;
> > > }
> > >
> > > static irqreturn_t mtk_fd_irq(int irq, void *data)
> > > {
> > > struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
> > >
> > > fd->fd_irq_result = readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
> > > wake_up_interruptible(&fd->wq);
> >
> > The wake up should be done at the very end of this function. Otherwise
> > we end up with m2m framework racing with the mtk_fd_hw_job_finish()
> > below.
> >
> > Also, I'd use a completion here rather than an open coded wait and
> > wake-up. The driver could reinit_completion() before queuing a job to
> > the hardware and the IRQ handler would complete(). There would be no
> > need to store the IRQ flags in driver data anymore.
> Ok, It will be refined as following:
>
> suspend and stop streaming will call mtk_fd_job_abort()
>
> static void mtk_fd_job_abort(struct mtk_fd_dev *fd)
> {
> u32 ret;
>
> ret = wait_for_completion_timeout(&fd->fd_irq_done,
> msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> if (ret)
For the _timeout variants, !ret means the timeout and ret > 0 means
that the wait ended successfully before.
Also please make sure that the timeout is big enough not to happen
normally on a heavily loaded system. Something like 1 second should be
good.
> mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> }
>
> complete at irq handler
>
> static irqreturn_t mtk_fd_irq(int irq, void *data)
> {
> struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
>
> /* must read this register otherwise HW will keep sending irq */
> readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
> fd->output->number = readl(fd->fd_base + MTK_FD_REG_OFFSET_RESULT);
> dev_dbg(fd->dev, "mtk_fd_face_num:%d\n", fd->output->number);
>
> pm_runtime_put((fd->dev));
> mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_DONE);
> complete(&fd->fd_irq_done);
> return IRQ_HANDLED;
> }
>
> and reinit_completion before time before sending a job to HW
>
> static int mtk_fd_hw_job_exec(struct mtk_fd_dev *fd,
> struct fd_enq_param *fd_param)
> {
> struct ipi_message fd_ipi_msg;
> int ret;
>
> pm_runtime_get_sync((fd->dev));
>
> reinit_completion(&fd->fd_irq_done);
> fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_ENQUEUE;
> memcpy(&fd_ipi_msg.fd_enq_param, fd_param, sizeof(struct
> fd_enq_param));
> ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT);
> if (ret) {
> pm_runtime_put((fd->dev));
> mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> return ret;
> }
> return 0;
> }
Looks good, thanks. Please also don't forget about init_completion()
in driver probe.
Best regards,
Tomasz
_______________________________________________
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