[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

SMP requires at least the ARMv6K extensions to be present, so if we're
running on SMP, the WFE and SEV instructions must be available.

However, when we run on UP, the v6K extensions may not be available,
and so we don't want WFE/SEV to be in the instruction stream.  Use the
SMP alternatives infrastructure to replace these instructions with NOPs
if we build for SMP but run on UP.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This removes one of the reasons that OMAP disables V6K on multi-omap
kernels - which subtly breaks SMP support.  Build tested and verified
by examination of resulting disassembly, but not boot tested.

 arch/arm/include/asm/spinlock.h |   37 +++++++++++++++++++++++++------------
 1 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index 17eb355..da1af52 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -5,17 +5,36 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
+/*
+ * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
+ * extensions, so when running on UP, we have to patch these instructions away.
+ */
+#define ALT_SMP(smp, up)					\
+	"9998:	" smp "\n"					\
+	"	.pushsection \".alt.smp.init\", \"a\"\n"	\
+	"	.long	9998b\n"				\
+	"	" up "\n"					\
+	"	.popsection\n"
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define SEV		ALT_SMP("sev.w", "nop.w")
+#define WFE(cond)	ALT_SMP("wfe" cond ".w", "nop.w")
+#else
+#define SEV		ALT_SMP("sev", "nop")
+#define WFE(cond)	ALT_SMP("wfe" cond, "nop")
+#endif
+
 static inline void dsb_sev(void)
 {
 #if __LINUX_ARM_ARCH__ >= 7
 	__asm__ __volatile__ (
 		"dsb\n"
-		"sev"
+		SEV
 	);
-#elif defined(CONFIG_CPU_32v6K)
+#else
 	__asm__ __volatile__ (
 		"mcr p15, 0, %0, c7, c10, 4\n"
-		"sev"
+		SEV
 		: : "r" (0)
 	);
 #endif
@@ -46,9 +65,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teqeq	%0, #0\n"
 "	bne	1b"
@@ -107,9 +124,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teq	%0, #0\n"
 "	bne	1b"
@@ -176,9 +191,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
 "1:	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"
 "	strexpl	%1, %0, [%2]\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfemi\n"
-#endif
+	WFE("mi")
 "	rsbpls	%0, %1, #0\n"
 "	bmi	1b"
 	: "=&r" (tmp), "=&r" (tmp2)

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

SMP requires at least the ARMv6K extensions to be present, so if we're
running on SMP, the WFE and SEV instructions must be available.

However, when we run on UP, the v6K extensions may not be available,
and so we don't want WFE/SEV to be in the instruction stream.  Use the
SMP alternatives infrastructure to replace these instructions with NOPs
if we build for SMP but run on UP.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This removes one of the reasons that OMAP disables V6K on multi-omap
kernels - which subtly breaks SMP support.  Build tested and verified
by examination of resulting disassembly, but not boot tested.

 arch/arm/include/asm/spinlock.h |   37 +++++++++++++++++++++++++------------
 1 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index 17eb355..da1af52 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -5,17 +5,36 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
+/*
+ * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
+ * extensions, so when running on UP, we have to patch these instructions away.
+ */
+#define ALT_SMP(smp, up)					\
+	"9998:	" smp "\n"					\
+	"	.pushsection \".alt.smp.init\", \"a\"\n"	\
+	"	.long	9998b\n"				\
+	"	" up "\n"					\
+	"	.popsection\n"
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define SEV		ALT_SMP("sev.w", "nop.w")
+#define WFE(cond)	ALT_SMP("wfe" cond ".w", "nop.w")
+#else
+#define SEV		ALT_SMP("sev", "nop")
+#define WFE(cond)	ALT_SMP("wfe" cond, "nop")
+#endif
+
 static inline void dsb_sev(void)
 {
 #if __LINUX_ARM_ARCH__ >= 7
 	__asm__ __volatile__ (
 		"dsb\n"
-		"sev"
+		SEV
 	);
-#elif defined(CONFIG_CPU_32v6K)
+#else
 	__asm__ __volatile__ (
 		"mcr p15, 0, %0, c7, c10, 4\n"
-		"sev"
+		SEV
 		: : "r" (0)
 	);
 #endif
@@ -46,9 +65,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teqeq	%0, #0\n"
 "	bne	1b"
@@ -107,9 +124,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teq	%0, #0\n"
 "	bne	1b"
@@ -176,9 +191,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
 "1:	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"
 "	strexpl	%1, %0, [%2]\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfemi\n"
-#endif
+	WFE("mi")
 "	rsbpls	%0, %1, #0\n"
 "	bmi	1b"
 	: "=&r" (tmp), "=&r" (tmp2)

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On 15 January 2011 16:11, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> SMP requires at least the ARMv6K extensions to be present, so if we're
> running on SMP, the WFE and SEV instructions must be available.
>
> However, when we run on UP, the v6K extensions may not be available,
> and so we don't want WFE/SEV to be in the instruction stream.  Use the
> SMP alternatives infrastructure to replace these instructions with NOPs
> if we build for SMP but run on UP.
[...]
> --- a/arch/arm/include/asm/spinlock.h
> +++ b/arch/arm/include/asm/spinlock.h
> @@ -5,17 +5,36 @@
>  #error SMP not supported on pre-ARMv6 CPUs
>  #endif
>
> +/*
> + * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
> + * extensions, so when running on UP, we have to patch these instructions away.
> + */
> +#define ALT_SMP(smp, up)                                       \
> +       "9998:  " smp "\n"                                      \
> +       "       .pushsection \".alt.smp.init\", \"a\"\n"        \
> +       "       .long   9998b\n"                                \
> +       "       " up "\n"                                       \
> +       "       .popsection\n"
> +
> +#ifdef CONFIG_THUMB2_KERNEL
> +#define SEV            ALT_SMP("sev.w", "nop.w")
> +#define WFE(cond)      ALT_SMP("wfe" cond ".w", "nop.w")
> +#else
> +#define SEV            ALT_SMP("sev", "nop")
> +#define WFE(cond)      ALT_SMP("wfe" cond, "nop")
> +#endif

In the SEV macro definition, can you also include the dsb? This
barrier is only there because of sev, otherwise we don't need it (we
have a dmb prior to releasing the lock).

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On 15 January 2011 16:11, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> SMP requires at least the ARMv6K extensions to be present, so if we're
> running on SMP, the WFE and SEV instructions must be available.
>
> However, when we run on UP, the v6K extensions may not be available,
> and so we don't want WFE/SEV to be in the instruction stream. ?Use the
> SMP alternatives infrastructure to replace these instructions with NOPs
> if we build for SMP but run on UP.
[...]
> --- a/arch/arm/include/asm/spinlock.h
> +++ b/arch/arm/include/asm/spinlock.h
> @@ -5,17 +5,36 @@
> ?#error SMP not supported on pre-ARMv6 CPUs
> ?#endif
>
> +/*
> + * sev and wfe are ARMv6K extensions. ?Uniprocessor ARMv6 may not have the K
> + * extensions, so when running on UP, we have to patch these instructions away.
> + */
> +#define ALT_SMP(smp, up) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? "9998: ?" smp "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? " ? ? ? .pushsection \".alt.smp.init\", \"a\"\n" ? ? ? ?\
> + ? ? ? " ? ? ? .long ? 9998b\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? " ? ? ? " up "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? " ? ? ? .popsection\n"
> +
> +#ifdef CONFIG_THUMB2_KERNEL
> +#define SEV ? ? ? ? ? ?ALT_SMP("sev.w", "nop.w")
> +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond ".w", "nop.w")
> +#else
> +#define SEV ? ? ? ? ? ?ALT_SMP("sev", "nop")
> +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond, "nop")
> +#endif

In the SEV macro definition, can you also include the dsb? This
barrier is only there because of sev, otherwise we don't need it (we
have a dmb prior to releasing the lock).

-- 
Catalin

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> On 15 January 2011 16:11, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > SMP requires at least the ARMv6K extensions to be present, so if we're
> > running on SMP, the WFE and SEV instructions must be available.
> >
> > However, when we run on UP, the v6K extensions may not be available,
> > and so we don't want WFE/SEV to be in the instruction stream.  Use the
> > SMP alternatives infrastructure to replace these instructions with NOPs
> > if we build for SMP but run on UP.
> [...]
> > --- a/arch/arm/include/asm/spinlock.h
> > +++ b/arch/arm/include/asm/spinlock.h
> > @@ -5,17 +5,36 @@
> >  #error SMP not supported on pre-ARMv6 CPUs
> >  #endif
> >
> > +/*
> > + * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
> > + * extensions, so when running on UP, we have to patch these instructions away.
> > + */
> > +#define ALT_SMP(smp, up)                                       \
> > +       "9998:  " smp "\n"                                      \
> > +       "       .pushsection \".alt.smp.init\", \"a\"\n"        \
> > +       "       .long   9998b\n"                                \
> > +       "       " up "\n"                                       \
> > +       "       .popsection\n"
> > +
> > +#ifdef CONFIG_THUMB2_KERNEL
> > +#define SEV            ALT_SMP("sev.w", "nop.w")
> > +#define WFE(cond)      ALT_SMP("wfe" cond ".w", "nop.w")
> > +#else
> > +#define SEV            ALT_SMP("sev", "nop")
> > +#define WFE(cond)      ALT_SMP("wfe" cond, "nop")
> > +#endif
> 
> In the SEV macro definition, can you also include the dsb?

No, you can't do preprocessor conditionals in the middle of a macro
definition, and I don't want to have 4 versions of the SEV stuff.

> This barrier is only there because of sev, otherwise we don't need it
> (we have a dmb prior to releasing the lock).

1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
We now have in asm/system.h:
#if __LINUX_ARM_ARCH__ >= 7 ||          \
        (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
#define sev()   __asm__ __volatile__ ("sev" : : : "memory")
#define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
#define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
#endif

2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?

3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
another review to determine how they're using sev()?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> On 15 January 2011 16:11, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > SMP requires at least the ARMv6K extensions to be present, so if we're
> > running on SMP, the WFE and SEV instructions must be available.
> >
> > However, when we run on UP, the v6K extensions may not be available,
> > and so we don't want WFE/SEV to be in the instruction stream. ?Use the
> > SMP alternatives infrastructure to replace these instructions with NOPs
> > if we build for SMP but run on UP.
> [...]
> > --- a/arch/arm/include/asm/spinlock.h
> > +++ b/arch/arm/include/asm/spinlock.h
> > @@ -5,17 +5,36 @@
> > ?#error SMP not supported on pre-ARMv6 CPUs
> > ?#endif
> >
> > +/*
> > + * sev and wfe are ARMv6K extensions. ?Uniprocessor ARMv6 may not have the K
> > + * extensions, so when running on UP, we have to patch these instructions away.
> > + */
> > +#define ALT_SMP(smp, up) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? "9998: ?" smp "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? " ? ? ? .pushsection \".alt.smp.init\", \"a\"\n" ? ? ? ?\
> > + ? ? ? " ? ? ? .long ? 9998b\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? " ? ? ? " up "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? " ? ? ? .popsection\n"
> > +
> > +#ifdef CONFIG_THUMB2_KERNEL
> > +#define SEV ? ? ? ? ? ?ALT_SMP("sev.w", "nop.w")
> > +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond ".w", "nop.w")
> > +#else
> > +#define SEV ? ? ? ? ? ?ALT_SMP("sev", "nop")
> > +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond, "nop")
> > +#endif
> 
> In the SEV macro definition, can you also include the dsb?

No, you can't do preprocessor conditionals in the middle of a macro
definition, and I don't want to have 4 versions of the SEV stuff.

> This barrier is only there because of sev, otherwise we don't need it
> (we have a dmb prior to releasing the lock).

1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
We now have in asm/system.h:
#if __LINUX_ARM_ARCH__ >= 7 ||          \
        (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
#define sev()   __asm__ __volatile__ ("sev" : : : "memory")
#define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
#define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
#endif

2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?

3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
another review to determine how they're using sev()?

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> > On 15 January 2011 16:11, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > SMP requires at least the ARMv6K extensions to be present, so if we're
> > > running on SMP, the WFE and SEV instructions must be available.
> > >
> > > However, when we run on UP, the v6K extensions may not be available,
> > > and so we don't want WFE/SEV to be in the instruction stream.  Use the
> > > SMP alternatives infrastructure to replace these instructions with NOPs
> > > if we build for SMP but run on UP.
> > [...]
> > > --- a/arch/arm/include/asm/spinlock.h
> > > +++ b/arch/arm/include/asm/spinlock.h
> > > @@ -5,17 +5,36 @@
> > >  #error SMP not supported on pre-ARMv6 CPUs
> > >  #endif
> > >
> > > +/*
> > > + * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
> > > + * extensions, so when running on UP, we have to patch these instructions away.
> > > + */
> > > +#define ALT_SMP(smp, up)                                       \
> > > +       "9998:  " smp "\n"                                      \
> > > +       "       .pushsection \".alt.smp.init\", \"a\"\n"        \
> > > +       "       .long   9998b\n"                                \
> > > +       "       " up "\n"                                       \
> > > +       "       .popsection\n"
> > > +
> > > +#ifdef CONFIG_THUMB2_KERNEL
> > > +#define SEV            ALT_SMP("sev.w", "nop.w")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond ".w", "nop.w")
> > > +#else
> > > +#define SEV            ALT_SMP("sev", "nop")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond, "nop")
> > > +#endif
> > 
> > In the SEV macro definition, can you also include the dsb?
> 
> No, you can't do preprocessor conditionals in the middle of a macro
> definition, and I don't want to have 4 versions of the SEV stuff.
> 
> > This barrier is only there because of sev, otherwise we don't need it
> > (we have a dmb prior to releasing the lock).
> 
> 1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
> We now have in asm/system.h:
> #if __LINUX_ARM_ARCH__ >= 7 ||          \
>         (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
> #define sev()   __asm__ __volatile__ ("sev" : : : "memory")
> #define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
> #define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
> #endif
> 
> 2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?
> 
> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
> another review to determine how they're using sev()?

FYI, this is how the SPEAR patches use sev():

| +static void __init wakeup_secondary(void)
| +{
| +     /* nobody is to be released from the pen yet */
| +     pen_release = -1;
| +
| +     /*
| +      * Write the address of secondary startup into the system-wide
| +      * location (presently it is in SRAM). The BootMonitor waits
| +      * for this register to become non-zero.
| +      * We must also send an sev to wake it up
| +      */
| +     __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
| +                     __io_address(SPEAR13XX_SYS_LOCATION));
| +
| +     mb();
| +
| +     /*
| +      * Send a 'sev' to wake the secondary core from WFE.
| +      */
| +     sev();
| +}

so the dsb() is inside mb(), before the outer sync call.

There should be another version of this patch coming which updates the
way the pen_release stuff is done (I hope) for the changes I made in
other platforms pen_release handling, so the above may change.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> > On 15 January 2011 16:11, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > SMP requires at least the ARMv6K extensions to be present, so if we're
> > > running on SMP, the WFE and SEV instructions must be available.
> > >
> > > However, when we run on UP, the v6K extensions may not be available,
> > > and so we don't want WFE/SEV to be in the instruction stream. ?Use the
> > > SMP alternatives infrastructure to replace these instructions with NOPs
> > > if we build for SMP but run on UP.
> > [...]
> > > --- a/arch/arm/include/asm/spinlock.h
> > > +++ b/arch/arm/include/asm/spinlock.h
> > > @@ -5,17 +5,36 @@
> > > ?#error SMP not supported on pre-ARMv6 CPUs
> > > ?#endif
> > >
> > > +/*
> > > + * sev and wfe are ARMv6K extensions. ?Uniprocessor ARMv6 may not have the K
> > > + * extensions, so when running on UP, we have to patch these instructions away.
> > > + */
> > > +#define ALT_SMP(smp, up) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > > + ? ? ? "9998: ?" smp "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > > + ? ? ? " ? ? ? .pushsection \".alt.smp.init\", \"a\"\n" ? ? ? ?\
> > > + ? ? ? " ? ? ? .long ? 9998b\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > > + ? ? ? " ? ? ? " up "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > > + ? ? ? " ? ? ? .popsection\n"
> > > +
> > > +#ifdef CONFIG_THUMB2_KERNEL
> > > +#define SEV ? ? ? ? ? ?ALT_SMP("sev.w", "nop.w")
> > > +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond ".w", "nop.w")
> > > +#else
> > > +#define SEV ? ? ? ? ? ?ALT_SMP("sev", "nop")
> > > +#define WFE(cond) ? ? ?ALT_SMP("wfe" cond, "nop")
> > > +#endif
> > 
> > In the SEV macro definition, can you also include the dsb?
> 
> No, you can't do preprocessor conditionals in the middle of a macro
> definition, and I don't want to have 4 versions of the SEV stuff.
> 
> > This barrier is only there because of sev, otherwise we don't need it
> > (we have a dmb prior to releasing the lock).
> 
> 1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
> We now have in asm/system.h:
> #if __LINUX_ARM_ARCH__ >= 7 ||          \
>         (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
> #define sev()   __asm__ __volatile__ ("sev" : : : "memory")
> #define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
> #define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
> #endif
> 
> 2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?
> 
> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
> another review to determine how they're using sev()?

FYI, this is how the SPEAR patches use sev():

| +static void __init wakeup_secondary(void)
| +{
| +     /* nobody is to be released from the pen yet */
| +     pen_release = -1;
| +
| +     /*
| +      * Write the address of secondary startup into the system-wide
| +      * location (presently it is in SRAM). The BootMonitor waits
| +      * for this register to become non-zero.
| +      * We must also send an sev to wake it up
| +      */
| +     __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
| +                     __io_address(SPEAR13XX_SYS_LOCATION));
| +
| +     mb();
| +
| +     /*
| +      * Send a 'sev' to wake the secondary core from WFE.
| +      */
| +     sev();
| +}

so the dsb() is inside mb(), before the outer sync call.

There should be another version of this patch coming which updates the
way the pen_release stuff is done (I hope) for the changes I made in
other platforms pen_release handling, so the above may change.

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On Mon, 2011-01-17 at 10:37 +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> > On 15 January 2011 16:11, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > SMP requires at least the ARMv6K extensions to be present, so if we're
> > > running on SMP, the WFE and SEV instructions must be available.
> > >
> > > However, when we run on UP, the v6K extensions may not be available,
> > > and so we don't want WFE/SEV to be in the instruction stream.  Use the
> > > SMP alternatives infrastructure to replace these instructions with NOPs
> > > if we build for SMP but run on UP.
> > [...]
> > > --- a/arch/arm/include/asm/spinlock.h
> > > +++ b/arch/arm/include/asm/spinlock.h
> > > @@ -5,17 +5,36 @@
> > >  #error SMP not supported on pre-ARMv6 CPUs
> > >  #endif
> > >
> > > +/*
> > > + * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
> > > + * extensions, so when running on UP, we have to patch these instructions away.
> > > + */
> > > +#define ALT_SMP(smp, up)                                       \
> > > +       "9998:  " smp "\n"                                      \
> > > +       "       .pushsection \".alt.smp.init\", \"a\"\n"        \
> > > +       "       .long   9998b\n"                                \
> > > +       "       " up "\n"                                       \
> > > +       "       .popsection\n"
> > > +
> > > +#ifdef CONFIG_THUMB2_KERNEL
> > > +#define SEV            ALT_SMP("sev.w", "nop.w")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond ".w", "nop.w")
> > > +#else
> > > +#define SEV            ALT_SMP("sev", "nop")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond, "nop")
> > > +#endif
> >
> > In the SEV macro definition, can you also include the dsb?
> 
> No, you can't do preprocessor conditionals in the middle of a macro
> definition, and I don't want to have 4 versions of the SEV stuff.
> 
> > This barrier is only there because of sev, otherwise we don't need it
> > (we have a dmb prior to releasing the lock).
> 
> 1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
> We now have in asm/system.h:
> #if __LINUX_ARM_ARCH__ >= 7 ||          \
>         (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
> #define sev()   __asm__ __volatile__ ("sev" : : : "memory")
> #define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
> #define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
> #endif

The whole spinlock code doesn't make much sense. But the wfe/sev are
usually no-ops if SMP isn't supported in hardware (though you still need
ARMv6K not to undef).

> 2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?

We could do but I'm not sure how confusing it would be (yet another
ARM-specific barrier).

> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
> another review to determine how they're using sev()?

We need a dsb to ensure that the prior STR completed before the sev,
otherwise we notify the other CPU but the LDREX reads the locked value
and goes into WFE again.

The SPEAR code seems ok, though I would have used a dsb() to make it
clear.

-- 
Catalin

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On Mon, 2011-01-17 at 10:37 +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> > On 15 January 2011 16:11, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > SMP requires at least the ARMv6K extensions to be present, so if we're
> > > running on SMP, the WFE and SEV instructions must be available.
> > >
> > > However, when we run on UP, the v6K extensions may not be available,
> > > and so we don't want WFE/SEV to be in the instruction stream.  Use the
> > > SMP alternatives infrastructure to replace these instructions with NOPs
> > > if we build for SMP but run on UP.
> > [...]
> > > --- a/arch/arm/include/asm/spinlock.h
> > > +++ b/arch/arm/include/asm/spinlock.h
> > > @@ -5,17 +5,36 @@
> > >  #error SMP not supported on pre-ARMv6 CPUs
> > >  #endif
> > >
> > > +/*
> > > + * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
> > > + * extensions, so when running on UP, we have to patch these instructions away.
> > > + */
> > > +#define ALT_SMP(smp, up)                                       \
> > > +       "9998:  " smp "\n"                                      \
> > > +       "       .pushsection \".alt.smp.init\", \"a\"\n"        \
> > > +       "       .long   9998b\n"                                \
> > > +       "       " up "\n"                                       \
> > > +       "       .popsection\n"
> > > +
> > > +#ifdef CONFIG_THUMB2_KERNEL
> > > +#define SEV            ALT_SMP("sev.w", "nop.w")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond ".w", "nop.w")
> > > +#else
> > > +#define SEV            ALT_SMP("sev", "nop")
> > > +#define WFE(cond)      ALT_SMP("wfe" cond, "nop")
> > > +#endif
> >
> > In the SEV macro definition, can you also include the dsb?
> 
> No, you can't do preprocessor conditionals in the middle of a macro
> definition, and I don't want to have 4 versions of the SEV stuff.
> 
> > This barrier is only there because of sev, otherwise we don't need it
> > (we have a dmb prior to releasing the lock).
> 
> 1. Does it make sense to have sev and wfe instructions in non-SMP kernels?
> We now have in asm/system.h:
> #if __LINUX_ARM_ARCH__ >= 7 ||          \
>         (__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
> #define sev()   __asm__ __volatile__ ("sev" : : : "memory")
> #define wfe()   __asm__ __volatile__ ("wfe" : : : "memory")
> #define wfi()   __asm__ __volatile__ ("wfi" : : : "memory")
> #endif

The whole spinlock code doesn't make much sense. But the wfe/sev are
usually no-ops if SMP isn't supported in hardware (though you still need
ARMv6K not to undef).

> 2. Should we have a smp_dsb() which makes the dsb conditional elsewhere?

We could do but I'm not sure how confusing it would be (yet another
ARM-specific barrier).

> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
> another review to determine how they're using sev()?

We need a dsb to ensure that the prior STR completed before the sev,
otherwise we notify the other CPU but the LDREX reads the locked value
and goes into WFE again.

The SPEAR code seems ok, though I would have used a dsb() to make it
clear.

-- 
Catalin

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On 17 January 2011 10:53, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
>> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
>> another review to determine how they're using sev()?
>
> FYI, this is how the SPEAR patches use sev():
>
> | +static void __init wakeup_secondary(void)
> | +{
> | +     /* nobody is to be released from the pen yet */
> | +     pen_release = -1;
> | +
> | +     /*
> | +      * Write the address of secondary startup into the system-wide
> | +      * location (presently it is in SRAM). The BootMonitor waits
> | +      * for this register to become non-zero.
> | +      * We must also send an sev to wake it up
> | +      */
> | +     __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
> | +                     __io_address(SPEAR13XX_SYS_LOCATION));
> | +
> | +     mb();
> | +
> | +     /*
> | +      * Send a 'sev' to wake the secondary core from WFE.
> | +      */
> | +     sev();
> | +}
>
> so the dsb() is inside mb(), before the outer sync call.

BTW, don't we need some cache flushing for the pen_release?

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Catalin Marinas]

On 17 January 2011 10:53, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
>> 3. Do we always need a dsb prior to a sev? ?Maybe the SPEAR patches need
>> another review to determine how they're using sev()?
>
> FYI, this is how the SPEAR patches use sev():
>
> | +static void __init wakeup_secondary(void)
> | +{
> | + ? ? /* nobody is to be released from the pen yet */
> | + ? ? pen_release = -1;
> | +
> | + ? ? /*
> | + ? ? ?* Write the address of secondary startup into the system-wide
> | + ? ? ?* location (presently it is in SRAM). The BootMonitor waits
> | + ? ? ?* for this register to become non-zero.
> | + ? ? ?* We must also send an sev to wake it up
> | + ? ? ?*/
> | + ? ? __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
> | + ? ? ? ? ? ? ? ? ? ? __io_address(SPEAR13XX_SYS_LOCATION));
> | +
> | + ? ? mb();
> | +
> | + ? ? /*
> | + ? ? ?* Send a 'sev' to wake the secondary core from WFE.
> | + ? ? ?*/
> | + ? ? sev();
> | +}
>
> so the dsb() is inside mb(), before the outer sync call.

BTW, don't we need some cache flushing for the pen_release?

-- 
Catalin

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 12:12:55PM +0000, Catalin Marinas wrote:
> On 17 January 2011 10:53, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
> >> 3. Do we always need a dsb prior to a sev?  Maybe the SPEAR patches need
> >> another review to determine how they're using sev()?
> >
> > FYI, this is how the SPEAR patches use sev():
> >
> > | +static void __init wakeup_secondary(void)
> > | +{
> > | +     /* nobody is to be released from the pen yet */
> > | +     pen_release = -1;
> > | +
> > | +     /*
> > | +      * Write the address of secondary startup into the system-wide
> > | +      * location (presently it is in SRAM). The BootMonitor waits
> > | +      * for this register to become non-zero.
> > | +      * We must also send an sev to wake it up
> > | +      */
> > | +     __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
> > | +                     __io_address(SPEAR13XX_SYS_LOCATION));
> > | +
> > | +     mb();
> > | +
> > | +     /*
> > | +      * Send a 'sev' to wake the secondary core from WFE.
> > | +      */
> > | +     sev();
> > | +}
> >
> > so the dsb() is inside mb(), before the outer sync call.
> 
> BTW, don't we need some cache flushing for the pen_release?

Which is why I referred to "updated patch" - that's something which
all implementations suffered from until recently.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 12:12:55PM +0000, Catalin Marinas wrote:
> On 17 January 2011 10:53, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 17, 2011 at 10:37:39AM +0000, Russell King - ARM Linux wrote:
> >> 3. Do we always need a dsb prior to a sev? ?Maybe the SPEAR patches need
> >> another review to determine how they're using sev()?
> >
> > FYI, this is how the SPEAR patches use sev():
> >
> > | +static void __init wakeup_secondary(void)
> > | +{
> > | + ? ? /* nobody is to be released from the pen yet */
> > | + ? ? pen_release = -1;
> > | +
> > | + ? ? /*
> > | + ? ? ?* Write the address of secondary startup into the system-wide
> > | + ? ? ?* location (presently it is in SRAM). The BootMonitor waits
> > | + ? ? ?* for this register to become non-zero.
> > | + ? ? ?* We must also send an sev to wake it up
> > | + ? ? ?*/
> > | + ? ? __raw_writel(BSYM(virt_to_phys(spear13xx_secondary_startup)),
> > | + ? ? ? ? ? ? ? ? ? ? __io_address(SPEAR13XX_SYS_LOCATION));
> > | +
> > | + ? ? mb();
> > | +
> > | + ? ? /*
> > | + ? ? ?* Send a 'sev' to wake the secondary core from WFE.
> > | + ? ? ?*/
> > | + ? ? sev();
> > | +}
> >
> > so the dsb() is inside mb(), before the outer sync call.
> 
> BTW, don't we need some cache flushing for the pen_release?

Which is why I referred to "updated patch" - that's something which
all implementations suffered from until recently.

Re: [PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> In the SEV macro definition, can you also include the dsb? This
> barrier is only there because of sev, otherwise we don't need it (we
> have a dmb prior to releasing the lock).

I think until we have the whole SEV situation sorted out, it's best
to leave it as-is.

Leaving the dsb in there shouldn't cause a problem for a v6+v6k+v7
kernel, it just means we're wasting a few cycles stalling the
instruction pipeline while the dsb completes, which we don't really
need to do on v6.

[PATCH] Remove CPU_32v6K dependencies in asm/spinlock.h

[Russell King - ARM Linux]

On Mon, Jan 17, 2011 at 10:15:25AM +0000, Catalin Marinas wrote:
> In the SEV macro definition, can you also include the dsb? This
> barrier is only there because of sev, otherwise we don't need it (we
> have a dmb prior to releasing the lock).

I think until we have the whole SEV situation sorted out, it's best
to leave it as-is.

Leaving the dsb in there shouldn't cause a problem for a v6+v6k+v7
kernel, it just means we're wasting a few cycles stalling the
instruction pipeline while the dsb completes, which we don't really
need to do on v6.