2020-10-01 12:44:05

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 1/6] powerpc/time: Rename mftbl() to mftb()

On PPC64, we have mftb().
On PPC32, we have mftbl() and an #define mftb() mftbl().

mftb() and mftbl() are equivalent, their purpose is to read the
content of SPRN_TRBL, as returned by 'mftb' simplified instruction.

binutils seems to define 'mftbl' instruction as an equivalent
of 'mftb'.

However in both 32 bits and 64 bits documentation, only 'mftb' is
defined, and when performing a disassembly with objdump, the displayed
instruction is 'mftb'

No need to have two ways to do the same thing with different
names, rename mftbl() to have only mftb().

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/reg.h | 5 ++---
arch/powerpc/include/asm/time.h | 2 +-
2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 788058af1d44..c66dcdb47c44 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1439,19 +1439,18 @@ static inline void msr_check_and_clear(unsigned long bits)
#else /* __powerpc64__ */

#if defined(CONFIG_PPC_8xx)
-#define mftbl() ({unsigned long rval; \
+#define mftb() ({unsigned long rval; \
asm volatile("mftbl %0" : "=r" (rval)); rval;})
#define mftbu() ({unsigned long rval; \
asm volatile("mftbu %0" : "=r" (rval)); rval;})
#else
-#define mftbl() ({unsigned long rval; \
+#define mftb() ({unsigned long rval; \
asm volatile("mfspr %0, %1" : "=r" (rval) : \
"i" (SPRN_TBRL)); rval;})
#define mftbu() ({unsigned long rval; \
asm volatile("mfspr %0, %1" : "=r" (rval) : \
"i" (SPRN_TBRU)); rval;})
#endif
-#define mftb() mftbl()
#endif /* !__powerpc64__ */

#define mttbl(v) asm volatile("mttbl %0":: "r"(v))
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index a80abf64c8a5..b0fb8456305f 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -50,7 +50,7 @@ struct div_result {

static inline unsigned long get_tbl(void)
{
- return mftbl();
+ return mftb();
}

static inline unsigned int get_tbu(void)
--
2.25.0


2020-10-01 12:44:08

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 4/6] powerpc/time: Remove get_tbu()

get_tbu() is redundant with mftbu() and is not used anymore.

Remove it.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/time.h | 5 -----
1 file changed, 5 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 3ef0f4b3299e..c4ea81c966b0 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -52,11 +52,6 @@ static inline unsigned long get_tbl(void)
{
return mftb();
}
-
-static inline unsigned int get_tbu(void)
-{
- return mftbu();
-}
#endif /* !CONFIG_PPC64 */

static inline unsigned int get_rtcl(void)
--
2.25.0

2020-10-01 12:44:14

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 5/6] powerpc/time: Make get_tbl() common to PPC32 and PPC64

On PPC64, get_tbl() is defined as an alias of get_tb() which return
the result of mftb(). That exactly the same as what the PPC32 version
does. We don't need two versions.

Remove the PPC64 definition of get_tbl() and use the PPC32 version
for both.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/time.h | 7 -------
1 file changed, 7 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index c4ea81c966b0..01b054b9766f 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -41,18 +41,11 @@ struct div_result {
/* Accessor functions for the timebase (RTC on 601) registers. */
#define __USE_RTC() (IS_ENABLED(CONFIG_PPC_BOOK3S_601))

-#ifdef CONFIG_PPC64
-
/* For compatibility, get_tbl() is defined as get_tb() on ppc64 */
-#define get_tbl get_tb
-
-#else
-
static inline unsigned long get_tbl(void)
{
return mftb();
}
-#endif /* !CONFIG_PPC64 */

static inline unsigned int get_rtcl(void)
{
--
2.25.0

2020-10-01 12:45:16

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 6/6] powerpc/time: Make get_tb() common to PPC32 and PPC64

mftbu() is always defined now, so the #ifdef can be removed
and replaced by an IS_ENABLED(CONFIG_PPC64) inside the
PPC32 version of get_tb().

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/time.h | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 01b054b9766f..418edaba8bd0 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -74,16 +74,13 @@ static inline u64 get_vtb(void)
return 0;
}

-#ifdef CONFIG_PPC64
-static inline u64 get_tb(void)
-{
- return mftb();
-}
-#else /* CONFIG_PPC64 */
static inline u64 get_tb(void)
{
unsigned int tbhi, tblo, tbhi2;

+ if (IS_ENABLED(CONFIG_PPC64))
+ return mftb();
+
do {
tbhi = mftbu();
tblo = mftb();
@@ -92,7 +89,6 @@ static inline u64 get_tb(void)

return ((u64)tbhi << 32) | tblo;
}
-#endif /* !CONFIG_PPC64 */

static inline u64 get_tb_or_rtc(void)
{
--
2.25.0

2020-10-01 12:45:28

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 2/6] powerpc/time: Make mftb() common to PPC32 and PPC64

No need to have two versions that are identical.

CONFIG_PPC_CELL is only selected by PPC64 targets.
CONFIG_E500 is the only PPC64 target selecting CONFIG_FSL_BOOK3E.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/reg.h | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c66dcdb47c44..f877a576b338 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1419,8 +1419,7 @@ static inline void msr_check_and_clear(unsigned long bits)
__msr_check_and_clear(bits);
}

-#ifdef __powerpc64__
-#if defined(CONFIG_PPC_CELL) || defined(CONFIG_PPC_FSL_BOOK3E)
+#if defined(CONFIG_PPC_CELL) || defined(CONFIG_E500)
#define mftb() ({unsigned long rval; \
asm volatile( \
"90: mfspr %0, %2;\n" \
@@ -1430,28 +1429,23 @@ static inline void msr_check_and_clear(unsigned long bits)
: "=r" (rval) \
: "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL) : "cr0"); \
rval;})
+#elif defined(CONFIG_PPC_8xx)
+#define mftb() ({unsigned long rval; \
+ asm volatile("mftbl %0" : "=r" (rval)); rval;})
#else
#define mftb() ({unsigned long rval; \
asm volatile("mfspr %0, %1" : \
"=r" (rval) : "i" (SPRN_TBRL)); rval;})
#endif /* !CONFIG_PPC_CELL */

-#else /* __powerpc64__ */
-
#if defined(CONFIG_PPC_8xx)
-#define mftb() ({unsigned long rval; \
- asm volatile("mftbl %0" : "=r" (rval)); rval;})
#define mftbu() ({unsigned long rval; \
asm volatile("mftbu %0" : "=r" (rval)); rval;})
#else
-#define mftb() ({unsigned long rval; \
- asm volatile("mfspr %0, %1" : "=r" (rval) : \
- "i" (SPRN_TBRL)); rval;})
#define mftbu() ({unsigned long rval; \
asm volatile("mfspr %0, %1" : "=r" (rval) : \
"i" (SPRN_TBRU)); rval;})
#endif
-#endif /* !__powerpc64__ */

#define mttbl(v) asm volatile("mttbl %0":: "r"(v))
#define mttbu(v) asm volatile("mttbu %0":: "r"(v))
--
2.25.0

2020-10-01 12:46:43

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH 3/6] powerpc/time: Avoid using get_tbl() and get_tbu() internally

get_tbl() is confusing as it returns the content of TBL register
on PPC32 but the concatenation of TBL and TBU on PPC64.

Use mftb() instead.

Do the same with get_tbu() for consistency allthough it's name
is less confusing.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/delay.h | 2 +-
arch/powerpc/include/asm/time.h | 8 ++++----
arch/powerpc/kernel/time.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h
index 66963f7d3e64..51bb8c1476c7 100644
--- a/arch/powerpc/include/asm/delay.h
+++ b/arch/powerpc/include/asm/delay.h
@@ -54,7 +54,7 @@ extern void udelay(unsigned long usecs);
({ \
typeof(condition) __ret; \
unsigned long __loops = tb_ticks_per_usec * timeout; \
- unsigned long __start = get_tbl(); \
+ unsigned long __start = mftb(); \
\
if (delay) { \
while (!(__ret = (condition)) && \
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index b0fb8456305f..3ef0f4b3299e 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -97,9 +97,9 @@ static inline u64 get_tb(void)
unsigned int tbhi, tblo, tbhi2;

do {
- tbhi = get_tbu();
- tblo = get_tbl();
- tbhi2 = get_tbu();
+ tbhi = mftbu();
+ tblo = mftb();
+ tbhi2 = mftbu();
} while (tbhi != tbhi2);

return ((u64)tbhi << 32) | tblo;
@@ -153,7 +153,7 @@ static inline unsigned long tb_ticks_since(unsigned long tstamp)
int delta = get_rtcl() - (unsigned int) tstamp;
return delta < 0 ? delta + 1000000000 : delta;
}
- return get_tbl() - tstamp;
+ return mftb() - tstamp;
}

#define mulhwu(x,y) \
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index f85539ebb513..a9cbd5a61585 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -467,8 +467,8 @@ void __delay(unsigned long loops)
*/
spin_cpu_relax();
} else {
- start = get_tbl();
- while (get_tbl() - start < loops)
+ start = mftb();
+ while (mftb() - start < loops)
spin_cpu_relax();
}
spin_end();
--
2.25.0

2020-10-01 20:53:12

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH 1/6] powerpc/time: Rename mftbl() to mftb()

On Thu, Oct 01, 2020 at 12:42:39PM +0000, Christophe Leroy wrote:
> On PPC64, we have mftb().
> On PPC32, we have mftbl() and an #define mftb() mftbl().
>
> mftb() and mftbl() are equivalent, their purpose is to read the
> content of SPRN_TRBL, as returned by 'mftb' simplified instruction.
>
> binutils seems to define 'mftbl' instruction as an equivalent
> of 'mftb'.
>
> However in both 32 bits and 64 bits documentation, only 'mftb' is
> defined, and when performing a disassembly with objdump, the displayed
> instruction is 'mftb'
>
> No need to have two ways to do the same thing with different
> names, rename mftbl() to have only mftb().

There are mttbl and mttbu insns (and no mttb insn); they write a 32-bit
half for the time base. There is an mftb, and an mftbu. mftbu reads
the upper half, while mftb reads the *whole* register. SPR 269 is the
TBU register, while SPR 268 is called both TB and TBL. Yes, it is
confusing :-)

The "mftb" name is much clearer than "mftbl" (on 64-bit), because it
reads the whole 64-bit register. On 32-bit mftbl is clearer (but not
defined in the architecture, not officially an insn or even an extended
mnemonic).


Segher

2020-10-09 10:02:20

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH 1/6] powerpc/time: Rename mftbl() to mftb()

On Thu, 1 Oct 2020 12:42:39 +0000 (UTC), Christophe Leroy wrote:
> On PPC64, we have mftb().
> On PPC32, we have mftbl() and an #define mftb() mftbl().
>
> mftb() and mftbl() are equivalent, their purpose is to read the
> content of SPRN_TRBL, as returned by 'mftb' simplified instruction.
>
> binutils seems to define 'mftbl' instruction as an equivalent
> of 'mftb'.
>
> [...]

Applied to powerpc/next.

[1/6] powerpc/time: Rename mftbl() to mftb()
https://git.kernel.org/powerpc/c/15c102153e722cc6e0729764a7068c209a7469cd
[2/6] powerpc/time: Make mftb() common to PPC32 and PPC64
https://git.kernel.org/powerpc/c/ff125fbcd45d1706861579dbe66e31f5b3f1e779
[3/6] powerpc/time: Avoid using get_tbl() and get_tbu() internally
https://git.kernel.org/powerpc/c/942e89115b588b4b5df86930b5302a5c07b820ba
[4/6] powerpc/time: Remove get_tbu()
https://git.kernel.org/powerpc/c/e8d5bf30eafc37e31ce68bc7ccf1db970fe3cd04
[5/6] powerpc/time: Make get_tbl() common to PPC32 and PPC64
https://git.kernel.org/powerpc/c/1156a6285cd38e5a6987ddee3758e7954c56cb3d
[6/6] powerpc/time: Make get_tb() common to PPC32 and PPC64
https://git.kernel.org/powerpc/c/9686e431c683ee7b8aca0f3985c244aee3d9f30d

cheers