From: robherring2@gmail•com (Rob Herring)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 2/3] ARM: convert platform hotplug inline assembly to C
Date: Thu, 17 Jan 2013 21:34:45 -0600 [thread overview]
Message-ID: <50F8C2D5.4070405@gmail.com> (raw)
In-Reply-To: <alpine.LFD.2.02.1301171038590.6300@xanadu.home>
On 01/17/2013 09:42 AM, Nicolas Pitre wrote:
> On Thu, 17 Jan 2013, Rob Herring wrote:
>
>> On 01/16/2013 10:40 PM, Nicolas Pitre wrote:
>>> On Wed, 16 Jan 2013, Rob Herring wrote:
>>>
>>>> From: Rob Herring <rob.herring@calxeda•com>
>>>>
>>>> With the addition of set_auxcr/get_auxcr, all the hotplug inline assembly
>>>> code for exynos, imx, realview, spear13xx and vexpress can be converted to
>>>> C code.
>>>
>>> That might not be all safe. Please see
>>> http://article.gmane.org/gmane.linux.ports.arm.kernel/209584
>>
>> Other than the OR/AND operations, it's all just inline assembly
>> functions that are called, so it gets compiled to the same code. Perhaps
>> I should put noinline on the functions so they stay of limited
>> complexity. If you don't think doing this in C is okay, then there is
>> probably no point in having the set_auxcr/get_auxcr functions.
>
> I think set_auxcr/get_auxcr is fine.
>
> But your patch goes beyond simply converting those. You also converted
> the cache flush and disable from assembly to C, which on a Cortex A9
> might be unsafe if the stack is modified in the sequence according to
> the discussion I referenced.
The referenced discussion is mainly for the A15, not the A9, right? It seems
the sequences are a bit different. So this is the code for A9 (spear13xx):
flush_cache_all();
__flush_icache_all();
dsb();
/*
* Turn off coherency
*/
set_auxcr(get_auxcr() & ~0x40);
set_cr(get_cr() & ~CR_C);
which generates this:
c027f36c: ebf648d2 bl c00116bc <v7_flush_kern_cache_all>
c027f370: e3a03000 mov r3, #0
c027f374: ee073f11 mcr 15, 0, r3, cr7, cr1, {0}
c027f378: f57ff04f dsb sy
c027f37c: ee113f30 mrc 15, 0, r3, cr1, cr0, {1}
c027f380: e3c33040 bic r3, r3, #64 ; 0x40
c027f384: ee013f30 mcr 15, 0, r3, cr1, cr0, {1}
c027f388: f57ff06f isb sy
c027f38c: ee113f10 mrc 15, 0, r3, cr1, cr0, {0}
c027f390: e3c33004 bic r3, r3, #4
c027f394: ee013f10 mcr 15, 0, r3, cr1, cr0, {0}
c027f398: f57ff06f isb sy
c027f39c: e320f003 wfi
v7_flush_kern_cache_all will generate stack accesses, but I didn't change that
fact. The I cache invalidate changed from invalidate all to invalidate
inner-shareable. I believe that should be equivalent. And the mcr version of
dsb changed to a dsb instruction. Some isb's are inserted as well.
So I don't follow your concern. You can't guarantee that the compiler wouldn't
insert a data access in the middle, but then you could not guarantee that here
either (exynos A15):
asm volatile(
" mrc p15, 0, %0, c1, c0, 0\n"
" bic %0, %0, %1\n"
" mcr p15, 0, %0, c1, c0, 0\n"
: "=&r" (v)
: "Ir" (CR_C)
: "cc");
flush_cache_louis();
asm volatile(
/*
* Turn off coherency
*/
" mrc p15, 0, %0, c1, c0, 1\n"
" bic %0, %0, %1\n"
" mcr p15, 0, %0, c1, c0, 1\n"
: "=&r" (v)
: "Ir" (0x40)
: "cc");
The only thing that guarantees this code works is flush_cache_louis does not
touch the stack and you rely that compiler doesn't do anything like register
save/restore around the function call.
Rob
next prev parent reply other threads:[~2013-01-18 3:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-17 2:53 [PATCH 1/3] ARM: introduce common set_auxcr/get_auxcr functions Rob Herring
2013-01-17 2:53 ` [PATCH 2/3] ARM: convert platform hotplug inline assembly to C Rob Herring
2013-01-17 4:40 ` Nicolas Pitre
2013-01-17 14:18 ` Rob Herring
2013-01-17 15:42 ` Nicolas Pitre
2013-01-18 3:34 ` Rob Herring [this message]
2013-01-18 4:56 ` Nicolas Pitre
2013-01-17 2:53 ` [PATCH 3/3] ARM: omap2: use get_auxcr for aux ctrl register read Rob Herring
2013-01-17 4:41 ` Nicolas Pitre
2013-01-17 8:30 ` Santosh Shilimkar
2013-01-17 17:07 ` Tony Lindgren
2013-01-17 4:34 ` [PATCH 1/3] ARM: introduce common set_auxcr/get_auxcr functions Nicolas Pitre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50F8C2D5.4070405@gmail.com \
--to=robherring2@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox