[PATCH 1/2] powerpc: permanently include 8xx registers in reg.h

[PATCH 1/2] powerpc: permanently include 8xx registers in reg.h

[Christophe Leroy]

Most 8xx registers have specific names, so just include
reg_8xx.h all the time in reg.h in order to have them defined
even when CONFIG_PPC_8xx is not selected. This will avoid
the need for #ifdefs in C code.

Guard SPRN_ICTRL in an #ifdef CONFIG_PPC_8xx as this register
has same name but different meaning and different spr number as
another register in the mpc7450.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/include/asm/reg.h     | 2 --
 arch/powerpc/include/asm/reg_8xx.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 10caa145f98b..b17ee25df226 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -25,9 +25,7 @@
 #include <asm/reg_fsl_emb.h>
 #endif
 
-#ifdef CONFIG_PPC_8xx
 #include <asm/reg_8xx.h>
-#endif /* CONFIG_PPC_8xx */
 
 #define MSR_SF_LG	63              /* Enable 64 bit mode */
 #define MSR_ISF_LG	61              /* Interrupt 64b mode valid on 630 */
diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index 7192eece6c3e..abc663c0f1db 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h
@@ -38,7 +38,9 @@
 #define SPRN_CMPF	153
 #define SPRN_LCTRL1	156
 #define SPRN_LCTRL2	157
+#ifdef CONFIG_PPC_8xx
 #define SPRN_ICTRL	158
+#endif
 #define SPRN_BAR	159
 
 /* Commands.  Only the first few are available to the instruction cache.
-- 
2.13.3

[PATCH 2/2] powerpc: cleanup hw_irq.h

[Christophe Leroy]

SET_MSR_EE() is just use in this file and doesn't provide
any added value compared to mtmsr(). Drop it.

Add macros to use wrtee/wrteei insn.

Replace #ifdefs by IS_ENABLED()

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/include/asm/hw_irq.h | 57 ++++++++++++++++++---------------------
 arch/powerpc/include/asm/reg.h    |  2 ++
 2 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index 32a18f2f49bc..c25d57f25a8b 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -226,8 +226,8 @@ static inline bool arch_irqs_disabled(void)
 #endif /* CONFIG_PPC_BOOK3S */
 
 #ifdef CONFIG_PPC_BOOK3E
-#define __hard_irq_enable()	asm volatile("wrteei 1" : : : "memory")
-#define __hard_irq_disable()	asm volatile("wrteei 0" : : : "memory")
+#define __hard_irq_enable()	wrteei(1)
+#define __hard_irq_disable()	wrteei(0)
 #else
 #define __hard_irq_enable()	__mtmsrd(MSR_EE|MSR_RI, 1)
 #define __hard_irq_disable()	__mtmsrd(MSR_RI, 1)
@@ -280,8 +280,6 @@ extern void force_external_irq_replay(void);
 
 #else /* CONFIG_PPC64 */
 
-#define SET_MSR_EE(x)	mtmsr(x)
-
 static inline unsigned long arch_local_save_flags(void)
 {
 	return mfmsr();
@@ -289,47 +287,44 @@ static inline unsigned long arch_local_save_flags(void)
 
 static inline void arch_local_irq_restore(unsigned long flags)
 {
-#if defined(CONFIG_BOOKE)
-	asm volatile("wrtee %0" : : "r" (flags) : "memory");
-#else
-	mtmsr(flags);
-#endif
+	if (IS_ENABLED(CONFIG_BOOKE))
+		wrtee(flags);
+	else
+		mtmsr(flags);
 }
 
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags = arch_local_save_flags();
-#ifdef CONFIG_BOOKE
-	asm volatile("wrteei 0" : : : "memory");
-#elif defined(CONFIG_PPC_8xx)
-	wrtspr(SPRN_EID);
-#else
-	SET_MSR_EE(flags & ~MSR_EE);
-#endif
+
+	if (IS_ENABLED(CONFIG_BOOKE))
+		wrteei(0);
+	else if (IS_ENABLED(CONFIG_PPC_8xx))
+		wrtspr(SPRN_EID);
+	else
+		mtmsr(flags & ~MSR_EE);
+
 	return flags;
 }
 
 static inline void arch_local_irq_disable(void)
 {
-#ifdef CONFIG_BOOKE
-	asm volatile("wrteei 0" : : : "memory");
-#elif defined(CONFIG_PPC_8xx)
-	wrtspr(SPRN_EID);
-#else
-	arch_local_irq_save();
-#endif
+	if (IS_ENABLED(CONFIG_BOOKE))
+		wrteei(0);
+	else if (IS_ENABLED(CONFIG_PPC_8xx))
+		wrtspr(SPRN_EID);
+	else
+		mtmsr(mfmsr() & ~MSR_EE);
 }
 
 static inline void arch_local_irq_enable(void)
 {
-#ifdef CONFIG_BOOKE
-	asm volatile("wrteei 1" : : : "memory");
-#elif defined(CONFIG_PPC_8xx)
-	wrtspr(SPRN_EIE);
-#else
-	unsigned long msr = mfmsr();
-	SET_MSR_EE(msr | MSR_EE);
-#endif
+	if (IS_ENABLED(CONFIG_BOOKE))
+		wrteei(1);
+	else if (IS_ENABLED(CONFIG_PPC_8xx))
+		wrtspr(SPRN_EIE);
+	else
+		mtmsr(mfmsr() | MSR_EE);
 }
 
 static inline bool arch_irqs_disabled_flags(unsigned long flags)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index b17ee25df226..04896dd09455 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1361,6 +1361,8 @@ static inline void mtmsr_isync(unsigned long val)
 #endif
 #define wrtspr(rn)	asm volatile("mtspr " __stringify(rn) ",0" : \
 				     : : "memory")
+#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
+#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) : "memory")
 
 extern unsigned long msr_check_and_set(unsigned long bits);
 extern bool strict_msr_control;
-- 
2.13.3

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Nicholas Piggin]

Christophe Leroy's on August 27, 2019 6:13 pm:
> SET_MSR_EE() is just use in this file and doesn't provide
> any added value compared to mtmsr(). Drop it.
> 
> Add macros to use wrtee/wrteei insn.
> 
> Replace #ifdefs by IS_ENABLED()
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  arch/powerpc/include/asm/hw_irq.h | 57 ++++++++++++++++++---------------------
>  arch/powerpc/include/asm/reg.h    |  2 ++
>  2 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 32a18f2f49bc..c25d57f25a8b 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -226,8 +226,8 @@ static inline bool arch_irqs_disabled(void)
>  #endif /* CONFIG_PPC_BOOK3S */
>  
>  #ifdef CONFIG_PPC_BOOK3E
> -#define __hard_irq_enable()	asm volatile("wrteei 1" : : : "memory")
> -#define __hard_irq_disable()	asm volatile("wrteei 0" : : : "memory")
> +#define __hard_irq_enable()	wrteei(1)
> +#define __hard_irq_disable()	wrteei(0)
>  #else
>  #define __hard_irq_enable()	__mtmsrd(MSR_EE|MSR_RI, 1)
>  #define __hard_irq_disable()	__mtmsrd(MSR_RI, 1)
> @@ -280,8 +280,6 @@ extern void force_external_irq_replay(void);
>  
>  #else /* CONFIG_PPC64 */
>  
> -#define SET_MSR_EE(x)	mtmsr(x)
> -
>  static inline unsigned long arch_local_save_flags(void)
>  {
>  	return mfmsr();
> @@ -289,47 +287,44 @@ static inline unsigned long arch_local_save_flags(void)
>  
>  static inline void arch_local_irq_restore(unsigned long flags)
>  {
> -#if defined(CONFIG_BOOKE)
> -	asm volatile("wrtee %0" : : "r" (flags) : "memory");
> -#else
> -	mtmsr(flags);
> -#endif
> +	if (IS_ENABLED(CONFIG_BOOKE))
> +		wrtee(flags);
> +	else
> +		mtmsr(flags);
>  }
>  
>  static inline unsigned long arch_local_irq_save(void)
>  {
>  	unsigned long flags = arch_local_save_flags();
> -#ifdef CONFIG_BOOKE
> -	asm volatile("wrteei 0" : : : "memory");
> -#elif defined(CONFIG_PPC_8xx)
> -	wrtspr(SPRN_EID);
> -#else
> -	SET_MSR_EE(flags & ~MSR_EE);
> -#endif
> +
> +	if (IS_ENABLED(CONFIG_BOOKE))
> +		wrteei(0);
> +	else if (IS_ENABLED(CONFIG_PPC_8xx))
> +		wrtspr(SPRN_EID);
> +	else
> +		mtmsr(flags & ~MSR_EE);
> +
>  	return flags;
>  }
>  
>  static inline void arch_local_irq_disable(void)
>  {
> -#ifdef CONFIG_BOOKE
> -	asm volatile("wrteei 0" : : : "memory");
> -#elif defined(CONFIG_PPC_8xx)
> -	wrtspr(SPRN_EID);
> -#else
> -	arch_local_irq_save();
> -#endif
> +	if (IS_ENABLED(CONFIG_BOOKE))
> +		wrteei(0);
> +	else if (IS_ENABLED(CONFIG_PPC_8xx))
> +		wrtspr(SPRN_EID);
> +	else
> +		mtmsr(mfmsr() & ~MSR_EE);
>  }
>  
>  static inline void arch_local_irq_enable(void)
>  {
> -#ifdef CONFIG_BOOKE
> -	asm volatile("wrteei 1" : : : "memory");
> -#elif defined(CONFIG_PPC_8xx)
> -	wrtspr(SPRN_EIE);
> -#else
> -	unsigned long msr = mfmsr();
> -	SET_MSR_EE(msr | MSR_EE);
> -#endif
> +	if (IS_ENABLED(CONFIG_BOOKE))
> +		wrteei(1);
> +	else if (IS_ENABLED(CONFIG_PPC_8xx))
> +		wrtspr(SPRN_EIE);
> +	else
> +		mtmsr(mfmsr() | MSR_EE);
>  }
>  
>  static inline bool arch_irqs_disabled_flags(unsigned long flags)
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index b17ee25df226..04896dd09455 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1361,6 +1361,8 @@ static inline void mtmsr_isync(unsigned long val)
>  #endif
>  #define wrtspr(rn)	asm volatile("mtspr " __stringify(rn) ",0" : \
>  				     : : "memory")
> +#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
> +#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) : "memory")

Can you implement just one macro that uses __builtin_constant_p to 
select between the imm and reg versions? I forgot if there's some
corner cases that prevent that working with inline asm i constraints.

Otherwise, looks like a nice cleanup.

Thanks,
Nick

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Segher Boessenkool]

On Tue, Aug 27, 2019 at 10:48:24PM +1000, Nicholas Piggin wrote:
> Christophe Leroy's on August 27, 2019 6:13 pm:
> > +#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
> > +#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) : "memory")
> 
> Can you implement just one macro that uses __builtin_constant_p to 
> select between the imm and reg versions? I forgot if there's some
> corner cases that prevent that working with inline asm i constraints.

static inline void wrtee(long val)
{
	asm volatile("wrtee%I0 %0" : : "n"(val) : "memory");
}

(This output modifier goes back to the dark ages, some 2.4 or something).


Segher

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Christophe Leroy]

Le 27/08/2019 à 19:29, Segher Boessenkool a écrit :
> On Tue, Aug 27, 2019 at 10:48:24PM +1000, Nicholas Piggin wrote:
>> Christophe Leroy's on August 27, 2019 6:13 pm:
>>> +#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
>>> +#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) : "memory")
>>
>> Can you implement just one macro that uses __builtin_constant_p to
>> select between the imm and reg versions? I forgot if there's some
>> corner cases that prevent that working with inline asm i constraints.
> 
> static inline void wrtee(long val)
> {
> 	asm volatile("wrtee%I0 %0" : : "n"(val) : "memory");
> }

Great, didn't know that possibility.

Can it be used with any insn, for instance with add/addi ?
Or with mr/li ?

> 
> (This output modifier goes back to the dark ages, some 2.4 or something).

Hope Clang support it ...

Christophe

> 
> 
> Segher
> 

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Segher Boessenkool]

On Tue, Aug 27, 2019 at 07:36:35PM +0200, Christophe Leroy wrote:
> Le 27/08/2019 à 19:29, Segher Boessenkool a écrit :
> >On Tue, Aug 27, 2019 at 10:48:24PM +1000, Nicholas Piggin wrote:
> >>Christophe Leroy's on August 27, 2019 6:13 pm:
> >>>+#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
> >>>+#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) : 
> >>>"memory")
> >>
> >>Can you implement just one macro that uses __builtin_constant_p to
> >>select between the imm and reg versions? I forgot if there's some
> >>corner cases that prevent that working with inline asm i constraints.
> >
> >static inline void wrtee(long val)
> >{
> >	asm volatile("wrtee%I0 %0" : : "n"(val) : "memory");
> >}
> 
> Great, didn't know that possibility.
> 
> Can it be used with any insn, for instance with add/addi ?
> Or with mr/li ?

Any instruction, yes.  %I<n> simply outputs an "i" if operand n is a
constant integer, and nothing otherwise.

So
  asm("add%I2 %0,%1,%2" : "=r"(dst) : "r"(src1), "ri"(src1));
works well.  I don't see how you would use it for li/mr...  You can do
  asm("add%I1 %0,0,%1" : "=r"(dst) : "ri"(src));
I suppose, but that is not really an mr.

> >(This output modifier goes back to the dark ages, some 2.4 or something).
> 
> Hope Clang support it ...

I don't know, sorry.  But it is used all over the place, see sfp-machine.h
for example, so maybe?


Segher

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Christophe Leroy]

Le 27/08/2019 à 20:26, Segher Boessenkool a écrit :
> On Tue, Aug 27, 2019 at 07:36:35PM +0200, Christophe Leroy wrote:
>> Le 27/08/2019 à 19:29, Segher Boessenkool a écrit :
>>> On Tue, Aug 27, 2019 at 10:48:24PM +1000, Nicholas Piggin wrote:
>>>> Christophe Leroy's on August 27, 2019 6:13 pm:
>>>>> +#define wrtee(val)	asm volatile("wrtee %0" : : "r" (val) : "memory")
>>>>> +#define wrteei(val)	asm volatile("wrteei %0" : : "i" (val) :
>>>>> "memory")
>>>>
>>>> Can you implement just one macro that uses __builtin_constant_p to
>>>> select between the imm and reg versions? I forgot if there's some
>>>> corner cases that prevent that working with inline asm i constraints.
>>>
>>> static inline void wrtee(long val)
>>> {
>>> 	asm volatile("wrtee%I0 %0" : : "n"(val) : "memory");
>>> }
>>
>> Great, didn't know that possibility.
>>
>> Can it be used with any insn, for instance with add/addi ?
>> Or with mr/li ?
> 
> Any instruction, yes.  %I<n> simply outputs an "i" if operand n is a
> constant integer, and nothing otherwise.

Thinking about it once more, I'm not sure this form is possible, because 
wrteei expect 0 or 1. If someone calls wrtee(MSR_EE); (or any constant 
containing MSR_EE) wrteei 1 is expected. And any constant with MSR_EE 
cleared should result in wrteei 0.

> 
> So
>    asm("add%I2 %0,%1,%2" : "=r"(dst) : "r"(src1), "ri"(src1));

"ri", not "n" as for wrtee ?

Christophe

> works well.  I don't see how you would use it for li/mr...  You can do
>    asm("add%I1 %0,0,%1" : "=r"(dst) : "ri"(src));
> I suppose, but that is not really an mr.
> 
>>> (This output modifier goes back to the dark ages, some 2.4 or something).
>>
>> Hope Clang support it ...
> 
> I don't know, sorry.  But it is used all over the place, see sfp-machine.h
> for example, so maybe?
> 
> 
> Segher
> 

Re: [PATCH 2/2] powerpc: cleanup hw_irq.h

[Segher Boessenkool]

On Tue, Aug 27, 2019 at 08:33:45PM +0200, Christophe Leroy wrote:
> >So
> >   asm("add%I2 %0,%1,%2" : "=r"(dst) : "r"(src1), "ri"(src1));
> 
> "ri", not "n" as for wrtee ?

"n" means a number.  "i" means any constant integer.  The difference is
mostly that "n" does not allow relocations.  This probably does not matter
for this asm, not if you call it with correct values anyway.

(If you want to pass other than small numbers here, you need different
constraints; let's not go there).


Segher

[PATCH 1/2] powerpc: permanently include 8xx registers in reg.h

[Christophe Leroy]

Most 8xx registers have specific names, so just include
reg_8xx.h all the time in reg.h in order to have them defined
even when CONFIG_PPC_8xx is not selected. This will avoid
the need for #ifdefs in C code.

Guard SPRN_ICTRL in an #ifdef CONFIG_PPC_8xx as this register
has same name but different meaning and different spr number as
another register in the mpc7450.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/include/asm/reg.h     | 2 --
 arch/powerpc/include/asm/reg_8xx.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 10caa145f98b..b17ee25df226 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -25,9 +25,7 @@
 #include <asm/reg_fsl_emb.h>
 #endif
 
-#ifdef CONFIG_PPC_8xx
 #include <asm/reg_8xx.h>
-#endif /* CONFIG_PPC_8xx */
 
 #define MSR_SF_LG	63              /* Enable 64 bit mode */
 #define MSR_ISF_LG	61              /* Interrupt 64b mode valid on 630 */
diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index 7192eece6c3e..abc663c0f1db 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h
@@ -38,7 +38,9 @@
 #define SPRN_CMPF	153
 #define SPRN_LCTRL1	156
 #define SPRN_LCTRL2	157
+#ifdef CONFIG_PPC_8xx
 #define SPRN_ICTRL	158
+#endif
 #define SPRN_BAR	159
 
 /* Commands.  Only the first few are available to the instruction cache.
-- 
2.13.3