[PATCH v2] ARM: Define wfi() macro for v6 processors

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

For v6, wfi is architected as a defined MCR instruction, so
use that definition.

Doing a no-op instead of wfi() is probably bad, so for older
processors than v6, wfi() is not defined.  If needed, some CPU-
specific wfi() will have to be defined elsewhere.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
--------------------------------------------------------------------
v2 notes:

The first version of this patch incorrectly specified the write
buffer drain MCR (c7, c5, 4) instead of WFI (c7, c0, 5).  Now fixed.
I hang my head in shame!

The first version of this patch attempted to define wfe() and sev()
to empty asms also for pre-v6K processors.  Having thought about
this some more, I think that it may be better to wait until this
is actually needed before attempting to define these to anything.
Having no definition (giving a build error) seems safer than
having a wrong definition.

 arch/arm/include/asm/system.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 97f6d60..f39cf09 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -129,6 +129,9 @@ extern unsigned int user_debug;
 #define sev()	__asm__ __volatile__ ("sev" : : : "memory")
 #define wfe()	__asm__ __volatile__ ("wfe" : : : "memory")
 #define wfi()	__asm__ __volatile__ ("wfi" : : : "memory")
+#elif __LINUX_ARM_ARCH__ == 6
+#define wfi() __asm__ __volatile__ ("mcr p15, 0, %0, c7, c0, 4" \
+				    : : "r" (0) : "memory")
 #endif
 
 #if __LINUX_ARM_ARCH__ >= 7
-- 
1.7.1

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> For v6, wfi is architected as a defined MCR instruction, so
> use that definition.
> 
> Doing a no-op instead of wfi() is probably bad, so for older
> processors than v6, wfi() is not defined.  If needed, some CPU-
> specific wfi() will have to be defined elsewhere.

This is something we kind-of already handle in a different way - see
the individual processor idle function in arch/arm/mm/proc*.S.

There's various errata work-arounds older CPUs need for wfi (or rather
its mcr equivalent) so maybe wfi() should just be an alias for a call
to that function.  Or maybe we shouldn't have a wfi() macro at all.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 08, 2011 at 11:08:08AM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> > For v6, wfi is architected as a defined MCR instruction, so
> > use that definition.
> > 
> > Doing a no-op instead of wfi() is probably bad, so for older
> > processors than v6, wfi() is not defined.  If needed, some CPU-
> > specific wfi() will have to be defined elsewhere.
> 
> This is something we kind-of already handle in a different way - see
> the individual processor idle function in arch/arm/mm/proc*.S.
> 
> There's various errata work-arounds older CPUs need for wfi (or rather
> its mcr equivalent) so maybe wfi() should just be an alias for a call
> to that function.  Or maybe we shouldn't have a wfi() macro at all.

OK-- I'll hold off on this for now.

The patch was prompted by a build failure, but I can't remember the
exact circumstances now...  if I hit the same problem again,
then it might be worth discussing further...

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Russell King - ARM Linux wrote:
> On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> > For v6, wfi is architected as a defined MCR instruction, so
> > use that definition.
> > 
> > Doing a no-op instead of wfi() is probably bad, so for older
> > processors than v6, wfi() is not defined.  If needed, some CPU-
> > specific wfi() will have to be defined elsewhere.
> 
> This is something we kind-of already handle in a different way - see
> the individual processor idle function in arch/arm/mm/proc*.S.
> 
> There's various errata work-arounds older CPUs need for wfi (or rather
> its mcr equivalent) so maybe wfi() should just be an alias for a call
> to that function.  Or maybe we shouldn't have a wfi() macro at all.

I don't see any users of the sev/wfe/wfi macros in the current kernel,
so removing them seems like a good strategy to avoid people from
using them incorrectly.

If the definitions differ between v5/v6/v7 CPUs, any common code
using them would need to do either binary patching of some sort
or abstract the difference between the CPU in some other way.

Dave, what code do you have in mind as a possible user?

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 01:11:51PM +0100, Arnd Bergmann wrote:
> On Tuesday 08 February 2011, Russell King - ARM Linux wrote:
> > On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> > > For v6, wfi is architected as a defined MCR instruction, so
> > > use that definition.
> > > 
> > > Doing a no-op instead of wfi() is probably bad, so for older
> > > processors than v6, wfi() is not defined.  If needed, some CPU-
> > > specific wfi() will have to be defined elsewhere.
> > 
> > This is something we kind-of already handle in a different way - see
> > the individual processor idle function in arch/arm/mm/proc*.S.
> > 
> > There's various errata work-arounds older CPUs need for wfi (or rather
> > its mcr equivalent) so maybe wfi() should just be an alias for a call
> > to that function.  Or maybe we shouldn't have a wfi() macro at all.
> 
> I don't see any users of the sev/wfe/wfi macros in the current kernel,
> so removing them seems like a good strategy to avoid people from
> using them incorrectly.

That's because they've only just been added.  See the massive 63-patch set
from Viresh which has been posting to this mailing list.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

> > On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> > > For v6, wfi is architected as a defined MCR instruction, so
> > > use that definition.

[...]

On Tue, Feb 08, 2011 at 01:11:51PM +0100, Arnd Bergmann wrote:
> I don't see any users of the sev/wfe/wfi macros in the current kernel,
> so removing them seems like a good strategy to avoid people from
> using them incorrectly.
> 
> If the definitions differ between v5/v6/v7 CPUs, any common code
> using them would need to do either binary patching of some sort
> or abstract the difference between the CPU in some other way.
> 
> Dave, what code do you have in mind as a possible user?

The OMAP BSP has its own version of this, which I'm suggesting
to port to the more generic macros in system.h, now that they
exist.

---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Santosh Shilimkar]

> -----Original Message-----
> From: Dave Martin [mailto:dave.martin at linaro.org]
> Sent: Tuesday, February 08, 2011 5:52 PM
> To: Arnd Bergmann
> Cc: linux-arm-kernel at lists.infradead.org; Russell King - ARM Linux;
> Nicolas Pitre; Tony Lindgren; Santosh Shilimkar; linux-
> omap at vger.kernel.org; Jean Pihet
> Subject: Re: [PATCH v2] ARM: Define wfi() macro for v6 processors
>
> > > On Tue, Feb 08, 2011 at 11:01:25AM +0000, Dave Martin wrote:
> > > > For v6, wfi is architected as a defined MCR instruction, so
> > > > use that definition.
>
> [...]
>
> On Tue, Feb 08, 2011 at 01:11:51PM +0100, Arnd Bergmann wrote:
> > I don't see any users of the sev/wfe/wfi macros in the current
> kernel,
> > so removing them seems like a good strategy to avoid people from
> > using them incorrectly.
> >
> > If the definitions differ between v5/v6/v7 CPUs, any common code
> > using them would need to do either binary patching of some sort
> > or abstract the difference between the CPU in some other way.
> >
> > Dave, what code do you have in mind as a possible user?
>
> The OMAP BSP has its own version of this, which I'm suggesting
> to port to the more generic macros in system.h, now that they
> exist.
>
Omap 'sev' version is removed some time back. We were keeping
WFI opcode version because of V6 and V7 compatibility issue. With
resent CPU_32v6K from Russell, that should also get addressed.

I shall remove the omap wfi version as soon as RMK's series is
merged.

Regards,
Santosh

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Russell King - ARM Linux wrote:
> > I don't see any users of the sev/wfe/wfi macros in the current kernel,
> > so removing them seems like a good strategy to avoid people from
> > using them incorrectly.
> 
> That's because they've only just been added.  See the massive 63-patch set
> from Viresh which has been posting to this mailing list.

Ok, I see. At least that version seems ok because the code is cpu
specific already.

The omap do_wfi macro is specifically meant to avoid the multi-cpu
case by using the hexadecimal notation, so it can still be used while
building a kernel for ARMv6 or lower. Dave's version seems to have
the opposite intention here (choosing one version per arch level),
so it wouldn't be a replacement.

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 12:53 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 February 2011, Russell King - ARM Linux wrote:
>> > I don't see any users of the sev/wfe/wfi macros in the current kernel,
>> > so removing them seems like a good strategy to avoid people from
>> > using them incorrectly.
>>
>> That's because they've only just been added. ?See the massive 63-patch set
>> from Viresh which has been posting to this mailing list.
>
> Ok, I see. At least that version seems ok because the code is cpu
> specific already.
>
> The omap do_wfi macro is specifically meant to avoid the multi-cpu
> case by using the hexadecimal notation, so it can still be used while
> building a kernel for ARMv6 or lower. Dave's version seems to have
> the opposite intention here (choosing one version per arch level),
> so it wouldn't be a replacement.

I guess there are two problems we're trying to solve here:

1) a lowest-common denominator implementation of things like wfi(),
for use in common code.  This must be based on __LINUX_ARM_ARCH__
(which IIUC gives the lowest arch supported by all the CPUs being
built for -- am I correct?)
2) definitions for specific CPUs, for non-generic code which may be
bundled together in a single kernel build.

For (1), We can sensibly try to define a generic macro.  If building
for ARMv6 and ARMv7 CPUs in the same kernel, then we have to use the
lowest-common-denominator definition, i.e., the MCR form.  For ARMv7,
we can use WFI.
For (2), I think the best approach is to use the actual "wfi"
instruction and build the affected files with the appropriate -march=
flag (omap already does that) - since those CPU-specific files should
by definition never be run if running on another CPU.  We only support
new enough tools these days that this should be supported; so "wfi"
should be preferable to ".long 0xdeadbeef" - otherwise we need lots of
#ifdef CONFIG_THUMB2_KERNEL, or a macro.  If we have a macro, it would
be better for that to be generically implemented somewhere, becasue
the requirements are the same for every BSP supporting v7.

I don't like the practice of pre-assembling bits of code with .long,
in order to allow a file to be built with wrong -march= flags, and I
would favour migrating away from this where possible ... but I accept
it's a pragmatic solution to a problem for which gcc/binutils provide
no good alternative.

As to exactly what macros should be defined and where, I don't have a
strong view--- other people's guess is probably better than mine.


I believe that my wfi() modification gets defined suitably for both
cases, with one exception:

If support for a v6K processor is included, we have no way to know
from preprocessor definitions whether a plain v6 processor is also to
be supported.  My proposed definition is incorrect in that we still
get wfi() == WFI, whereas we really want the MCR for this case.
Likewise, the definitions of wfe() and sev() may not execute on a
plain v6 processor (though actually, I think they will execute as
NOPs)

So, for v6K we should either always use MCR for wfi(), or we need to
define wfi() using ALT_SMP(wfi) ALT_UP(mcr).  But whether that's a
common enough case to care about, I can't say.


Any thoughts on all that?

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Santosh Shilimkar]

Dave,

> -----Original Message-----
> From: Dave Martin [mailto:dave.martin at linaro.org]
> Sent: Tuesday, February 08, 2011 8:16 PM
> To: Arnd Bergmann
> Cc: linux-arm-kernel at lists.infradead.org; Russell King - ARM Linux;
> Nicolas Pitre; Tony Lindgren; Santosh Shilimkar; linux-
> omap at vger.kernel.org; Jean Pihet
> Subject: Re: [PATCH v2] ARM: Define wfi() macro for v6 processors
>
[....]

> For (2), I think the best approach is to use the actual "wfi"
> instruction and build the affected files with the appropriate -
> march=
> flag (omap already does that) - since those CPU-specific files
> should
> by definition never be run if running on another CPU.  We only
> support
> new enough tools these days that this should be supported; so "wfi"
> should be preferable to ".long 0xdeadbeef" - otherwise we need lots
> of
> #ifdef CONFIG_THUMB2_KERNEL, or a macro.  If we have a macro, it
> would
> be better for that to be generically implemented somewhere, becasue
> the requirements are the same for every BSP supporting v7.
>
> I don't like the practice of pre-assembling bits of code with .long,
> in order to allow a file to be built with wrong -march= flags, and I
> would favour migrating away from this where possible ... but I
> accept
> it's a pragmatic solution to a problem for which gcc/binutils
> provide
> no good alternative.
>
How about C files where 'wfi' used using inline assembly.
Can we also specify the " -march=" for the C files as well ?

Regards,
Santosh

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 2:54 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Dave,
>
>> -----Original Message-----
>> From: Dave Martin [mailto:dave.martin at linaro.org]

[...]

>> I don't like the practice of pre-assembling bits of code with .long,
>> in order to allow a file to be built with wrong -march= flags, and I
>> would favour migrating away from this where possible ... but I
>> accept
>> it's a pragmatic solution to a problem for which gcc/binutils
>> provide
>> no good alternative.
>>
> How about C files where 'wfi' used using inline assembly.
> Can we also specify the " -march=" for the C files as well ?

Kbuild looks like it can do it, e.g. in mach-omap2/Makefile:

CFLAGS_pm_bus.o                         += -DDEBUG

... so we could:

CFLAGS_cpu_specific_object.o        += -march=armv7-a

Whether it's _safe_ to do it depends on whether code from that file
could ever get run on other processors.  I'm not so sure of the answer
to that..., but perhaps someone else has a better idea.

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Dave Martin wrote:
> I guess there are two problems we're trying to solve here:
> 
> 1) a lowest-common denominator implementation of things like wfi(),
> for use in common code.  This must be based on __LINUX_ARM_ARCH__
> (which IIUC gives the lowest arch supported by all the CPUs being
> built for -- am I correct?)
> 2) definitions for specific CPUs, for non-generic code which may be
> bundled together in a single kernel build.
> 
> For (1), We can sensibly try to define a generic macro.  If building
> for ARMv6 and ARMv7 CPUs in the same kernel, then we have to use the
> lowest-common-denominator definition, i.e., the MCR form.  For ARMv7,
> we can use WFI.

But that doesn't work if you build a combined v5/v6/v7 kernel, because
v5 supports neither form, right? I think to do that, it needs the
same kind of abstraction that we have for a number of other things
like cache management in arch/arm/mm/.

> For (2), I think the best approach is to use the actual "wfi"
> instruction and build the affected files with the appropriate -march=
> flag (omap already does that) - since those CPU-specific files should
> by definition never be run if running on another CPU.  We only support
> new enough tools these days that this should be supported; so "wfi"
> should be preferable to ".long 0xdeadbeef" - otherwise we need lots of
> #ifdef CONFIG_THUMB2_KERNEL, or a macro.  If we have a macro, it would
> be better for that to be generically implemented somewhere, becasue
> the requirements are the same for every BSP supporting v7.

Makes sense.

> I don't like the practice of pre-assembling bits of code with .long,
> in order to allow a file to be built with wrong -march= flags, and I
> would favour migrating away from this where possible ... but I accept
> it's a pragmatic solution to a problem for which gcc/binutils provide
> no good alternative.

Yes. Moreover, new instructions may always have to be that way for
a while, before they can be moved over to the proper inline
assembly.

> So, for v6K we should either always use MCR for wfi(), or we need to
> define wfi() using ALT_SMP(wfi) ALT_UP(mcr).  But whether that's a
> common enough case to care about, I can't say.
> 
> 
> Any thoughts on all that?

I think that having all instances of wfi in per-CPU source files is
good enough, because it's not performance critical, and this method
is well-supported already.

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Dave Martin wrote:
> CFLAGS_cpu_specific_object.o        += -march=armv7-a
> 
> Whether it's safe to do it depends on whether code from that file
> could ever get run on other processors.  I'm not so sure of the answer
> to that..., but perhaps someone else has a better idea.

We already do this a lot from arch/arm/mm/Makefile, and those
files are typically just one function per file, so they can easily
be proven to be safe that way.

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 3:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 February 2011, Dave Martin wrote:
>> I guess there are two problems we're trying to solve here:
>>
>> 1) a lowest-common denominator implementation of things like wfi(),
>> for use in common code. ?This must be based on __LINUX_ARM_ARCH__
>> (which IIUC gives the lowest arch supported by all the CPUs being
>> built for -- am I correct?)
>> 2) definitions for specific CPUs, for non-generic code which may be
>> bundled together in a single kernel build.
>>
>> For (1), We can sensibly try to define a generic macro. ?If building
>> for ARMv6 and ARMv7 CPUs in the same kernel, then we have to use the
>> lowest-common-denominator definition, i.e., the MCR form. ?For ARMv7,
>> we can use WFI.
>
> But that doesn't work if you build a combined v5/v6/v7 kernel, because
> v5 supports neither form, right? I think to do that, it needs the
> same kind of abstraction that we have for a number of other things
> like cache management in arch/arm/mm/.
>
>> For (2), I think the best approach is to use the actual "wfi"
>> instruction and build the affected files with the appropriate -march=
>> flag (omap already does that) - since those CPU-specific files should
>> by definition never be run if running on another CPU. ?We only support
>> new enough tools these days that this should be supported; so "wfi"
>> should be preferable to ".long 0xdeadbeef" - otherwise we need lots of
>> #ifdef CONFIG_THUMB2_KERNEL, or a macro. ?If we have a macro, it would
>> be better for that to be generically implemented somewhere, becasue
>> the requirements are the same for every BSP supporting v7.
>
> Makes sense.
>
>> I don't like the practice of pre-assembling bits of code with .long,
>> in order to allow a file to be built with wrong -march= flags, and I
>> would favour migrating away from this where possible ... but I accept
>> it's a pragmatic solution to a problem for which gcc/binutils provide
>> no good alternative.
>
> Yes. Moreover, new instructions may always have to be that way for
> a while, before they can be moved over to the proper inline
> assembly.

Why not have macros for these cases?  (Which kinda brings the
discussion full-circle...)

My immediate concern is that too often, the Thumb-2 case will be
handled incorrectly or not at all ... and the kernel will silently
build without flagging any error.  Macros would allow the fragile
definitions to go in one place.

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Dave Martin wrote:
> Why not have macros for these cases?  (Which kinda brings the
> discussion full-circle...)
> 
> My immediate concern is that too often, the Thumb-2 case will be
> handled incorrectly or not at all ... and the kernel will silently
> build without flagging any error.  Macros would allow the fragile
> definitions to go in one place.

A macro to handle the thumb2 case and weird toolchains sounds good,
but if we want to build code for multiple CPUs, we need multiple
macros, not one macro that works on a specific CPU.

We could put all those macros unconditionally into a arch
specific header, e.g.

arch/arm/include/asm/system-v7.h:
#define wfi() __asm__ __volatile__ ("wfi" : : : "memory")


arch/arm/include/asm/system-v6k.h:
#define wfi() __asm__ __volatile__ ("mcr p15, 0, %0, c7, c0, 4" \
		: : "r" (0) : "memory")

And then have each C file using them be CPU specific and include
only one (you get an gcc warning if you try to include both).
This should work fine, but it's different again from how we handle
other things like spinlocks or cache management across multiple
CPUs.

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 02:45:51PM +0000, Dave Martin wrote:
> If support for a v6K processor is included, we have no way to know
> from preprocessor definitions whether a plain v6 processor is also to
> be supported.

Yes we do.  See the v6 patches I've recently posted to various mailing
lists.

This patch series was created in the hope that it could be tested by a
sufficient number of people so that it could go into -rc1 as at the
moment, omap2plus_defconfig builds data-corrupting kernels.

Unfortunately, the response was not as good as I was hoping - it's
possible to count the number of testers on one hand.  So it's now
scheduled for the next merge window.  What this means is that v2.6.37
and v2.6.38 omap2plus_defconfig kernels _will_ _be_ _unsafe_ when run
on SMP hardware, and will remain that way.  If a fix is not suitable for
-rc, it most certainly isn't suitable for -stable.

What the patch set does demonstrate that we can know at preprocessor
time that plain v6 processors should be supported along side v6k and
v7.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 3:42 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 February 2011, Dave Martin wrote:
>> Why not have macros for these cases? ?(Which kinda brings the
>> discussion full-circle...)
>>
>> My immediate concern is that too often, the Thumb-2 case will be
>> handled incorrectly or not at all ... and the kernel will silently
>> build without flagging any error. ?Macros would allow the fragile
>> definitions to go in one place.
>
> A macro to handle the thumb2 case and weird toolchains sounds good,
> but if we want to build code for multiple CPUs, we need multiple
> macros, not one macro that works on a specific CPU.
>
> We could put all those macros unconditionally into a arch
> specific header, e.g.
>
> arch/arm/include/asm/system-v7.h:
> #define wfi() __asm__ __volatile__ ("wfi" : : : "memory")
>
>
> arch/arm/include/asm/system-v6k.h:
> #define wfi() __asm__ __volatile__ ("mcr p15, 0, %0, c7, c0, 4" \
> ? ? ? ? ? ? ? ?: : "r" (0) : "memory")
>
> And then have each C file using them be CPU specific and include
> only one (you get an gcc warning if you try to include both).
> This should work fine, but it's different again from how we handle
> other things like spinlocks or cache management across multiple
> CPUs.

Such a solution could work...

I have a (possibly silly) question ... are the definitions in
arm/system.h intended to be fully generic, or not?  Some features
suggest an attempt at genericness, but not everything there is
generic.  Maybe I have misconstrued the purpose of this header.

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 04:15:15PM +0100, Arnd Bergmann wrote:
> But that doesn't work if you build a combined v5/v6/v7 kernel, because
> v5 supports neither form, right? I think to do that, it needs the
> same kind of abstraction that we have for a number of other things
> like cache management in arch/arm/mm/.

The options are kernels which support v3-v5 or v6-v7.  There's too big
a change between v5 and v6 to combine those into one kernel.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 4:20 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 08, 2011 at 02:45:51PM +0000, Dave Martin wrote:
>> If support for a v6K processor is included, we have no way to know
>> from preprocessor definitions whether a plain v6 processor is also to
>> be supported.
>
> Yes we do. ?See the v6 patches I've recently posted to various mailing
> lists.
>
> This patch series was created in the hope that it could be tested by a
> sufficient number of people so that it could go into -rc1 as at the
> moment, omap2plus_defconfig builds data-corrupting kernels.
>
> Unfortunately, the response was not as good as I was hoping - it's
> possible to count the number of testers on one hand. ?So it's now
> scheduled for the next merge window. ?What this means is that v2.6.37
> and v2.6.38 omap2plus_defconfig kernels _will_ _be_ _unsafe_ when run
> on SMP hardware, and will remain that way. ?If a fix is not suitable for
> -rc, it most certainly isn't suitable for -stable.
> What the patch set does demonstrate that we can know at preprocessor
> time that plain v6 processors should be supported along side v6k and
> v7.

Fair enough--- I hadn't fully understood the implications there, since
I'm not really working with the v6 case for now.  Adding a
CPU_V6/CPU_V6K distinction does seem to solve that specific problem.
Unfortunately, I'm not in a position to test that easily...

I think I will alter my OMAP Thumb-2 support patches to avoid the
generic wfi() macro for now -- simply to decouple the dependency
pending discussion about system*.h.  This can be fixed later, if
appropriate.

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 4:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 08, 2011 at 04:15:15PM +0100, Arnd Bergmann wrote:
>> But that doesn't work if you build a combined v5/v6/v7 kernel, because
>> v5 supports neither form, right? I think to do that, it needs the
>> same kind of abstraction that we have for a number of other things
>> like cache management in arch/arm/mm/.
>
> The options are kernels which support v3-v5 or v6-v7. ?There's too big
> a change between v5 and v6 to combine those into one kernel.
>

Indeed... wfi() is a good example of where being generic gets hard...
some v5 processors (typically those with caches) implement the same
MCR supported by v6, but it's not universal.  Typically v4 processors
have no WFI capability.

---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 04:17:58PM +0100, Arnd Bergmann wrote:
> On Tuesday 08 February 2011, Dave Martin wrote:
> > CFLAGS_cpu_specific_object.o        += -march=armv7-a
> > 
> > Whether it's safe to do it depends on whether code from that file
> > could ever get run on other processors.  I'm not so sure of the answer
> > to that..., but perhaps someone else has a better idea.
> 
> We already do this a lot from arch/arm/mm/Makefile, and those
> files are typically just one function per file, so they can easily
> be proven to be safe that way.

No, we do that with assembly files.  It doesn't work soo well with
C files as we really don't want GCC itself to generate v7 instructions
unless we explicitly ask for them.

The other issue here is that somtimes generating code with different
-march options leads to the linker refusing to link them together...

Unfortunately, ARM toolchains have been developed with the assumption
that you're only building a program for one specific CPU, not
targetting multiple CPUs.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011 17:25:16 Russell King - ARM Linux wrote:
> On Tue, Feb 08, 2011 at 04:15:15PM +0100, Arnd Bergmann wrote:
> > But that doesn't work if you build a combined v5/v6/v7 kernel, because
> > v5 supports neither form, right? I think to do that, it needs the
> > same kind of abstraction that we have for a number of other things
> > like cache management in arch/arm/mm/.
> 
> The options are kernels which support v3-v5 or v6-v7.  There's too big
> a change between v5 and v6 to combine those into one kernel.

Ah, good to know. I never realized this with the common binary discussions.
Building for the versatile/realview platform makes it possible to select
any of ARM926 and the v6/v6k/v7 CPUs, as well as ARM7TDMI in the same
kernel config, so I assumed that this was actually a valid configuration
aside from bugs.

Would an attempt to make a combined v5/v6 kernel result in unacceptably
bad code, or is this just too much work for anyone to be bothered with
and little practical value?

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 05:47:18PM +0100, Arnd Bergmann wrote:
> On Tuesday 08 February 2011 17:25:16 Russell King - ARM Linux wrote:
> > On Tue, Feb 08, 2011 at 04:15:15PM +0100, Arnd Bergmann wrote:
> > > But that doesn't work if you build a combined v5/v6/v7 kernel, because
> > > v5 supports neither form, right? I think to do that, it needs the
> > > same kind of abstraction that we have for a number of other things
> > > like cache management in arch/arm/mm/.
> > 
> > The options are kernels which support v3-v5 or v6-v7.  There's too big
> > a change between v5 and v6 to combine those into one kernel.
> 
> Ah, good to know. I never realized this with the common binary discussions.
> Building for the versatile/realview platform makes it possible to select
> any of ARM926 and the v6/v6k/v7 CPUs, as well as ARM7TDMI in the same
> kernel config, so I assumed that this was actually a valid configuration
> aside from bugs.
> 
> Would an attempt to make a combined v5/v6 kernel result in unacceptably
> bad code, or is this just too much work for anyone to be bothered with
> and little practical value?

If you build for anything less than v6, you lose all barriers, the icache
synchronization, and proper atomic operations.  Just to name a few.

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011 17:32:29 Russell King - ARM Linux wrote:
> On Tue, Feb 08, 2011 at 04:17:58PM +0100, Arnd Bergmann wrote:
> > On Tuesday 08 February 2011, Dave Martin wrote:
> > > CFLAGS_cpu_specific_object.o        += -march=armv7-a
> > > 
> > > Whether it's safe to do it depends on whether code from that file
> > > could ever get run on other processors.  I'm not so sure of the answer
> > > to that..., but perhaps someone else has a better idea.
> > 
> > We already do this a lot from arch/arm/mm/Makefile, and those
> > files are typically just one function per file, so they can easily
> > be proven to be safe that way.
> 
> No, we do that with assembly files.  It doesn't work soo well with
> C files as we really don't want GCC itself to generate v7 instructions
> unless we explicitly ask for them.
>
> The other issue here is that somtimes generating code with different
> -march options leads to the linker refusing to link them together...

Ok, I see. Is that a bug in existing toolchains, or something more
fundamental?

I would have expected that you could at least mix all compiler options
that don't impact the ABI or the instruction set like -mthumb.

Also, I think we can still build with e.g. "-march=armv6 -Wa,-march=armv7",
which should tell the compiler to only emit armv6 instructions, but make
the assembler more permissive for inline assembly.

	Arnd

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 4:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 08, 2011 at 05:47:18PM +0100, Arnd Bergmann wrote:
>> On Tuesday 08 February 2011 17:25:16 Russell King - ARM Linux wrote:
>> > On Tue, Feb 08, 2011 at 04:15:15PM +0100, Arnd Bergmann wrote:
>> > > But that doesn't work if you build a combined v5/v6/v7 kernel, because
>> > > v5 supports neither form, right? I think to do that, it needs the
>> > > same kind of abstraction that we have for a number of other things
>> > > like cache management in arch/arm/mm/.
>> >
>> > The options are kernels which support v3-v5 or v6-v7. ?There's too big
>> > a change between v5 and v6 to combine those into one kernel.
>>
>> Ah, good to know. I never realized this with the common binary discussions.
>> Building for the versatile/realview platform makes it possible to select
>> any of ARM926 and the v6/v6k/v7 CPUs, as well as ARM7TDMI in the same
>> kernel config, so I assumed that this was actually a valid configuration
>> aside from bugs.
>>
>> Would an attempt to make a combined v5/v6 kernel result in unacceptably
>> bad code, or is this just too much work for anyone to be bothered with
>> and little practical value?
>
> If you build for anything less than v6, you lose all barriers, the icache
> synchronization, and proper atomic operations. ?Just to name a few.
>

Can we represent the restriction in Kconfig somehow?

For example:

CPU_V6 depends on CPU_COMMON_V6PLUS
CPU_V7 depends on CPU_COMMON_V6PLUS
CPU_V5 depends on CPU_COMMON_V3_TO_V5
CPU_V4 depends on CPU_COMMON_V3_TO_V5
CPU_V3 depends on CPU_COMMON_V3_TO_V5

CPU_COMMON_V6PLUS depends on !CPU_COMMON_V3_TO_V5
CPU_COMMON_V3_TO_V5 depends on !CPU_COMMON_V6PLUS

...or something like that.

---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Dave Martin]

On Tue, Feb 8, 2011 at 4:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 February 2011 17:32:29 Russell King - ARM Linux wrote:
>> On Tue, Feb 08, 2011 at 04:17:58PM +0100, Arnd Bergmann wrote:
>> > On Tuesday 08 February 2011, Dave Martin wrote:
>> > > CFLAGS_cpu_specific_object.o ? ? ? ?+= -march=armv7-a
>> > >
>> > > Whether it's safe to do it depends on whether code from that file
>> > > could ever get run on other processors. ?I'm not so sure of the answer
>> > > to that..., but perhaps someone else has a better idea.
>> >
>> > We already do this a lot from arch/arm/mm/Makefile, and those
>> > files are typically just one function per file, so they can easily
>> > be proven to be safe that way.
>>
>> No, we do that with assembly files. ?It doesn't work soo well with
>> C files as we really don't want GCC itself to generate v7 instructions
>> unless we explicitly ask for them.
>>
>> The other issue here is that somtimes generating code with different
>> -march options leads to the linker refusing to link them together...
>
> Ok, I see. Is that a bug in existing toolchains, or something more
> fundamental?
>
> I would have expected that you could at least mix all compiler options
> that don't impact the ABI or the instruction set like -mthumb.
>
> Also, I think we can still build with e.g. "-march=armv6 -Wa,-march=armv7",
> which should tell the compiler to only emit armv6 instructions, but make
> the assembler more permissive for inline assembly.

I think you can usually mix options provided the ABI isn't affected.
It could be interesting to try it just to see what happens...

I've seen this work, but I haven't tried it on a large scale such as
building the kernel...

I think the major problems in the past have usually between
OABI<->EABI, non-interworking<->interworking, non-PIC<->PIC and
floating-point ABI clashes, particularly regarding compiler helper
libs like libgcc.  I expect none of these are likely to affect the
kernel, particularly since the kernel has its own libgcc (I think).

Possibly EABI toolchains are better at handling this than the for old
ABI, but it's still not ideal: for example, if you build this with
-march=armv6

void fancy() {
    if(have_arch_v7()) {
        asm(".arch v7-a\n\t"
               /* ... fancy stuff ... */
               ".arch v6");
    } else {
        /* C implementation of fancy stuff */
    }
}

... the object is marked as requiring v7, since some v7 instructions
were emitted.  Really, the object only requires v6, but that
conclusion requires knowledge the tools don't have.  (Note that tools
guys I've talked to don't much like changing .arch mid-file, but from
my PoV it should be reasonable except for the problem of knowing what
.arch to restore to afterwards.)

If vmlinux ends up stamped as requiring v7, that has no effect
whatsoever on whether the kernel works.  That's down to what the
actual code does, as in the above example.

Cheers
---Dave

[PATCH v2] ARM: Define wfi() macro for v6 processors

[Arnd Bergmann]

On Tuesday 08 February 2011, Dave Martin wrote:
> I have a (possibly silly) question ... are the definitions in
> arm/system.h intended to be fully generic, or not?  Some features
> suggest an attempt at genericness, but not everything there is
> generic.  Maybe I have misconstrued the purpose of this header.

asm/system.h contains an ill-defined set of random stuff derived
from what used to be in there in the old days Linux-1.x and
whatever the individual architectures added to it.

There have been previous attempts to reduce the common stuff
and leave only architecture-specific definitions in there, but
it still contains for all architectures at least:

* switch_to()
* mb() and friends
* cmpxchg() and friends

These are hard to change, because each architecture has slightly
different rules for recursive header file inclusion, so you end
up breaking stuff when you touch them.

The other declarations and macros are arch specific, so we could
move them to a different place or change them if there is a good
reason to do that.

I personally think it would be good to have at least cmpxchg and
everything related to it in a separate header, like a few
architectures do (x86, sh, alpha, mips), but I don't see too
much value in doing that just on ARM while leaving the other
architectures alone.

	Arnd