#2369 closed defect (fixed)

[PowerPC Book E] Invalid mftb instruction in _CPU_Counter_read()

Reported by: Nick Withers Owned by: Nick Withers <nick.withers@…>
Priority: normal Milestone: 4.11.1
Component: unspecified Version: 4.11
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

_CPU_Counter_read(), called e.g., when RTEMS profiling is enabled, attempts to use the mftb instruction to access the time base. This instruction does not exist on Book E processors (such as the e500 used in the MVME3100) and causes an exception on those architectures.

At least RTEMS profiling therefore does not work at least with the mvme3100 BSP.

This...:

diff --git a/cpukit/score/cpu/powerpc/rtems/score/cpu.h b/cpukit/score/cpu/powerpc/rtems/score/cpu.h
index 06cab2c..45298a4 100644
--- a/cpukit/score/cpu/powerpc/rtems/score/cpu.h
+++ b/cpukit/score/cpu/powerpc/rtems/score/cpu.h
@@ -842,7 +842,7 @@ static inline CPU_Counter_ticks _CPU_Counter_read( void )
   /* Use Alternate Time Base */
   __asm__ volatile( "mfspr %0, 526" : "=r" (value) );
 #else
-  __asm__ volatile( "mftb %0" : "=r" (value) );
+  __asm__ volatile( "mfspr %0, 268" : "=r" (value) );
 #endif
 
   return value;

...sorts it out on the mvme3100 and I don't *think* will break anything for other BSPs (I believe SPR 268 is always valid).

I wonder if we wouldn't be better off using PPC_Get_timebase_register(), though, which also checks the upper 32-bits of the timebase? Maybe that doesn't matter for the cases where _CPU_Counter_read() 's called?

Attachments (1)

ppc-mfspr.patch (2.9 KB) - added by Nick Withers on Jul 15, 2015 at 6:24:08 AM.
Proposed patch to use mfspr from the main time base universally; does not address MVME3100 CFLAGS issues

Download all attachments as: .zip

Change History (21)

comment:1 Changed on Jul 9, 2015 at 5:51:57 AM by Sebastian Huber

We have:

static inline CPU_Counter_ticks _CPU_Counter_read( void )
{
  CPU_Counter_ticks value;

#if defined(ppc8540) || defined(__PPC_CPU_E6500__)
  /* Use Alternate Time Base */
  __asm__ volatile( "mfspr %0, 526" : "=r" (value) );
#else
  __asm__ volatile( "mftb %0" : "=r" (value) );
#endif

  return value;
}

The ppc8540 is the multilib intended for the e500 processors, why is this not used?

Last edited on Jul 9, 2015 at 5:52:22 AM by Sebastian Huber (previous) (diff)

comment:2 in reply to:  1 Changed on Jul 9, 2015 at 6:14:47 AM by Nick Withers

Replying to sebastian.huber:

The ppc8540 is the multilib intended for the e500 processors, why is this not used?

No idea - where's that picked up from? I've been wondering for a quite a while why I get "-mcpu=powerpc" when building RTEMS and not "-mcpu=8540"... is that intentional / anything to do with it?

I'm thoroughly confused by PowerPC versioning in general... According to Figure 4 from http://www.freescale.com/files/32bit/doc/white_paper/POWRPCARCPRMRM.pdf there's no alternate time base on Book E, so the mfspr %0, 526 doesn't seem correct here... But then going off http://cache.freescale.com/files/32bit/doc/ref_manual/E500CORERM.pdf Table 3.44, there are a bunch of instructions that are valid for Book E but invalid for the e500, so presumably the e500 doesn't actually conform to Book E... or something?!

The marketing guff for the MVME3100 board says "System-on-chip Freescale MPC8540 with PowerPC® e500 processor core" - does that imply that not all MPC8540s use e500 cores?

comment:3 Changed on Jul 9, 2015 at 6:22:58 AM by Sebastian Huber

A BSP for the e500 should use the "-mcpu=8540 -meabi -msdata=sysv -fno-common" options.

I am not sure if the SPR 268 exists really on all PowerPCs supported by RTEMS:

static inline uint64_t PPC_Get_timebase_register( void )
{

uint32_t tbr_low;
uint32_t tbr_high;
uint32_t tbr_high_old;
uint64_t tbr;

do {

#if defined(mpx8xx) defined(mpc860) defined(mpc821)
/* See comment above (CPU_Get_timebase_low) */

asm volatile( "mftbu %0" : "=r" (tbr_high_old));
asm volatile( "mftb %0" : "=r" (tbr_low));
asm volatile( "mftbu %0" : "=r" (tbr_high));

#else

asm volatile( "mfspr %0, 269" : "=r" (tbr_high_old));
asm volatile( "mfspr %0, 268" : "=r" (tbr_low));
asm volatile( "mfspr %0, 269" : "=r" (tbr_high));

#endif

} while ( tbr_high_old != tbr_high );

tbr = tbr_high;
tbr <<= 32;
tbr |= tbr_low;
return tbr;

}

However these exceptions look more like museum hardware.

comment:4 in reply to:  3 Changed on Jul 9, 2015 at 7:48:30 AM by Nick Withers

Replying to sebastian.huber:

A BSP for the e500 should use the "-mcpu=8540 -meabi -msdata=sysv -fno-common" options.

Cheers - I tried the following:

diff --git a/c/src/lib/libbsp/powerpc/mvme3100/make/custom/mvme3100.cfg b/c/src/lib/libbsp/powerpc/mvme3100/make/custom/mvme3100.cfg
index ddd6d23..3872f18 100644
--- a/c/src/lib/libbsp/powerpc/mvme3100/make/custom/mvme3100.cfg
+++ b/c/src/lib/libbsp/powerpc/mvme3100/make/custom/mvme3100.cfg
@@ -10,10 +10,10 @@ RTEMS_CPU_MODEL=e500
 
 #  This contains the compiler options necessary to select the CPU model
 #  and (hopefully) optimize for it.
-CPU_CFLAGS = -mcpu=powerpc -msoft-float -D__ppc_generic
+CPU_CFLAGS = -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic

...and wasn't very successful [1]:

powerpc-rtems4.11-gcc -B../../../../../mvme3100/lib/ -specs bsp_specs -qrtems -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic -O2 -g -Wall -Wmissing-prototypes -Wimplicit-function-declaration -Wstrict-prototypes -Wnested-externs    -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic   -o loopback.exe init.o 
/usr/home/nick/rtems/rtems-4.11/bin/../lib/gcc/powerpc-rtems4.11/4.9.3/../../../../powerpc-rtems4.11/bin/ld: ../../../../../mvme3100/lib/librtemsbsp.a(bspgetworkarea.o): the target (__rtems_end) of a R_PPC_SDAREL16 relocation is in the wrong output section (.bss)
powerpc-rtems4.11-nm -g -n loopback.exe > loopback.num
powerpc-rtems4.11-size loopback.exe
   text	   data	    bss	    dec	    hex	filename
 390421	   7124	  19317	 416862	  65c5e	loopback.exe
powerpc-rtems4.11-objcopy -O binary loopback.exe loopback.ralf
gmake[6]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites/samples/loopback'
Making all in pppd
gmake[6]: Entering directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites/samples/pppd'
powerpc-rtems4.11-gcc -B../../../../../mvme3100/lib/ -specs bsp_specs -qrtems -DHAVE_CONFIG_H -I. -I../../../../../../../../rtems/c/src/../../testsuites/samples/pppd -I..     -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic -O2 -g -Wall -Wmissing-prototypes -Wimplicit-function-declaration -Wstrict-prototypes -Wnested-externs -MT init.o -MD -MP -MF .deps/init.Tpo -c -o init.o ../../../../../../../../rtems/c/src/../../testsuites/samples/pppd/init.c
mv -f .deps/init.Tpo .deps/init.Po
powerpc-rtems4.11-gcc -B../../../../../mvme3100/lib/ -specs bsp_specs -qrtems -DHAVE_CONFIG_H -I. -I../../../../../../../../rtems/c/src/../../testsuites/samples/pppd -I..     -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic -O2 -g -Wall -Wmissing-prototypes -Wimplicit-function-declaration -Wstrict-prototypes -Wnested-externs -MT pppdapp.o -MD -MP -MF .deps/pppdapp.Tpo -c -o pppdapp.o ../../../../../../../../rtems/c/src/../../testsuites/samples/pppd/pppdapp.c
mv -f .deps/pppdapp.Tpo .deps/pppdapp.Po
powerpc-rtems4.11-gcc -B../../../../../mvme3100/lib/ -specs bsp_specs -qrtems -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic -O2 -g -Wall -Wmissing-prototypes -Wimplicit-function-declaration -Wstrict-prototypes -Wnested-externs    -mcpu=8540 -meabi -msdata=sysv -fno-common -D__ppc_generic   -o pppd.exe init.o pppdapp.o -lpppd
/usr/home/nick/rtems/rtems-4.11/bin/../lib/gcc/powerpc-rtems4.11/4.9.3/../../../../powerpc-rtems4.11/bin/ld: ../../../../../mvme3100/lib/librtemsbsp.a(bspgetworkarea.o): the target (__rtems_end) of a R_PPC_SDAREL16 relocation is in the wrong output section (.bss)
../../../../../mvme3100/lib/librtemsbsp.a(bspgetworkarea.o): In function `bsp_work_area_initialize':
/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/lib/libbsp/powerpc/mvme3100/../../../../../../../../../rtems/c/src/lib/libbsp/powerpc/mvme3100/../../powerpc/shared/startup/bspgetworkarea.c:17:(.text+0x6): relocation truncated to fit: R_PPC_SDAREL16 against symbol `__rtems_end' defined in .bss section in pppd.exe
collect2: error: ld returned 1 exit status
Makefile:639: recipe for target 'pppd.exe' failed
gmake[6]: *** [pppd.exe] Error 1
gmake[6]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites/samples/pppd'
Makefile:718: recipe for target 'all-local' failed
gmake[5]: *** [all-local] Error 1
gmake[5]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites/samples'
Makefile:319: recipe for target 'all' failed
gmake[4]: *** [all] Error 2
gmake[4]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites/samples'
Makefile:385: recipe for target 'all-recursive' failed
gmake[3]: *** [all-recursive] Error 1
gmake[3]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100/testsuites'
Makefile:496: recipe for target 'all-recursive' failed
gmake[2]: *** [all-recursive] Error 1
gmake[2]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c/mvme3100'
Makefile:359: recipe for target 'all-recursive' failed
gmake[1]: *** [all-recursive] Error 1
gmake[1]: Leaving directory '/usr/home/nick/rtems/git/build/rtems/powerpc-rtems4.11/c'
Makefile:482: recipe for target 'all-recursive' failed
gmake: *** [all-recursive] Error 1

I am not sure if the SPR 268 exists really on all PowerPCs supported by RTEMS:

static inline uint64_t PPC_Get_timebase_register( void )
{

uint32_t tbr_low;
uint32_t tbr_high;
uint32_t tbr_high_old;
uint64_t tbr;

do {

#if defined(mpx8xx) defined(mpc860) defined(mpc821)
/* See comment above (CPU_Get_timebase_low) */

asm volatile( "mftbu %0" : "=r" (tbr_high_old));
asm volatile( "mftb %0" : "=r" (tbr_low));
asm volatile( "mftbu %0" : "=r" (tbr_high));

#else

asm volatile( "mfspr %0, 269" : "=r" (tbr_high_old));
asm volatile( "mfspr %0, 268" : "=r" (tbr_low));
asm volatile( "mfspr %0, 269" : "=r" (tbr_high));

#endif

} while ( tbr_high_old != tbr_high );

tbr = tbr_high;
tbr <<= 32;
tbr |= tbr_low;
return tbr;

}

However these exceptions look more like museum hardware.

Seems like they exist on the MPC860 at least (though they can't be used in place of mttb as they're user-level / read-only).

[1] -D__ppc_generic was left in to shut powerpc.h up, but I'll investigate whether that's appropriate and/or explicitly whack an mpc8540 check in there

comment:5 Changed on Jul 9, 2015 at 8:00:47 AM by Sebastian Huber <sebastian.huber@…>

In c2596dfbd02f783e6a88c98a5dbf325874e755cb/rtems:

bsps/powerpc: Fix small-data area issue

Update #2369.

comment:6 Changed on Jul 14, 2015 at 1:45:36 AM by Nick Withers

Sorry Sebastian, I'm still trying to work through this...

I seem to have problems with my toolset and can't even get current-source-builder-built GDB to talk to my BDI-3000.

It looks like SPE might be getting enabled (e.g., *trying* to boot gives "Welcome to rtems-4.10.99.0(PowerPC/Generic (E500/float-gprs/SPE)/mvme3100)"), so I'm trying to figure out whether the previous GCC 4.9.2 patches definitely made it in to 4.9.3 (would that explain GDB not being able to talk to the BDI though?!). Dunno what I'm on about here, still digging :-P

comment:7 in reply to:  6 Changed on Jul 14, 2015 at 2:00:56 AM by Nick Withers

Replying to nick.withers:

I seem to have problems with my toolset and can't even get current-source-builder-built GDB to talk to my BDI-3000.

It looks like SPE might be getting enabled (e.g., *trying* to boot gives "Welcome to rtems-4.10.99.0(PowerPC/Generic (E500/float-gprs/SPE)/mvme3100)"), so I'm trying to figure out whether the previous GCC 4.9.2 patches definitely made it in to 4.9.3 (would that explain GDB not being able to talk to the BDI though?!).

The patches did indeed make it in.

Latest GDB actually does work fine talking to my BDI - but only if it's not given the (presumably incorrectly-SPE-enabled) binary.

Any ideas?

Last edited on Jul 14, 2015 at 2:01:17 AM by Nick Withers (previous) (diff)

comment:8 Changed on Jul 14, 2015 at 4:50:23 AM by Nick Withers

If I build with "CPU_CFLAGS = -mcpu=8540 -meabi -msdata=sysv -msoft-float -mno-spe -fno-common" all seems kosher (or at least as kosher as it did with -mcpu=powerpc before changing the CFLAGS from what's in Git HEAD, though I haven't got back to the profiling bit yet and am expecting the Alternate Time Base access in _CPU_Counter_read() to fail).

GDB reports arch of "powerpc:common", which is what I was seeing with -mcpu=powerpc.

Without -msoft-float -mno-spe it reports "powerpc:e500" and is an unhappy camper, e.g.:

(gdb) target remote bdi:2159
Remote debugging using bdi:2159
Remote 'g' packet reply is too long: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e4008a0ac40066d2e01020298deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef

If I build with -msoft-float but not mno-spe it seems to try to link against hard-float multilibs:

/usr/home/nick/rtems/rtems-4.11/bin/../lib/gcc/powerpc-rtems4.11/4.9.3/../../../../powerpc-rtems4.11/bin/ld: Warning: /usr/home/nick/rtems/rtems-4.11/bin/../lib/gcc/powerpc-rtems4.11/4.9.3/../../../../powerpc-rtems4.11/lib/libc.a(lib_a-printf.o) uses hard float, hello.exe uses soft float

Going off this in score/powerpc.h:

#if defined(_SOFT_FLOAT) \
  || defined(__NO_FPRS__) /* e500 has unified integer/FP registers */ \
  || defined(__PPC_CPU_E6500__)
#define PPC_HAS_FPU 0
#else
#define PPC_HAS_FPU 1
#endif

...the intention's that we should be soft-float on the e500?

comment:9 Changed on Jul 14, 2015 at 6:28:03 AM by Sebastian Huber

We can still use your original patch and use the SPR 268. I don't care that much about the legacy PowerPCs that lack this register. Some of the newer cores for automotive chips don't have a time base at all, but this is a different issue.

For e500 you have the following options

  1. soft-float (-msoft-float -mno-spe),
  2. SPE with 32-bit float (-mspe -mabi=spe -mfloat-gprs=single_, and
  3. SPE with 64-bit float (e500v2, -mspe -mabi=spe -mfloat-gprs=double).

comment:10 Changed on Jul 15, 2015 at 5:54:57 AM by Nick Withers

https://sourceware.org/ml/libc-alpha/2008-04/msg00036.html and https://llvm.org/bugs/show_bug.cgi?id=23680 have some interesting discussion about mftb et al., including a quote from the PPC ISA 2.07 doco which says mfspr 268/269 should be preferred and that "It is believed that the mfspr instruction can be used to read the Time Base on most processors that comply with versions of the architecture that precede Version 2.01. Processors for which mfspr cannot be used to read the Time Base include the following.

  • 601
  • POWER3".

This and e.g., the MPC860 definitely having 'em would seem to contradict part of the comment against CPU_Get_timebase_low. Also, I don't understand the "we run in supervisory mode so that should work on all CPUs" bit... From my reading it seems clear SPR 268 / 269 should be readable in user mode too.

Also, psim (GDB 7.9) now seems to handle mftb and mfspr fine (GDB disass shows mftb in the disassembly for both, but they're the requested instructions - opcodes {31, 371} and {31, 339} - underneath), so the "PSIM currently lacks support for reading SPRs 268/269" comment seems past its use-by date. [1]

Regarding the "Alternate Time Base" currently used if defined(ppc8540) || defined(__PPC_CPU_E6500__), http://www.freescale.com/files/32bit/doc/white_paper/POWRPCARCPRMRM.pdf says: "The EIS defines an alternate time base APU, which is implemented on the e500v2. It is intended for measuring time in implementation-defined intervals. It differs from the PowerPC-defined time base in that it is not writable, it counts at a different, typically higher, frequency". I'd like to unify _CPU_Counter_read(), CPU_Get_timebase_low and PPC_Get_timebase_register() so we're consistent with how we grab the time base... Should I get rid of alternate time base accesses in _CPU_Counter_read()? I believe that's the only place they're used (through grepping for "526").

On to the CFLAGS...

1 Seems to work. GDB no longer reports it as an "powerpc:e500" but "powerpc:common", but I guess the former just means that the binary's SPE-with-shared-registers-enabled or something and the latter doesn't indicate the compilation wasn't correctly targeted. ppc8540 *is* defined, where it wasn't before.

I tried 2 with CPU_CFLAGS = -mcpu=8540 -meabi -msdata=sysv -mspe -mabi=spe -mfloat-gprs=single -fno-common (that right?) but the board resets at some point soon after bsp_start(). GDB won't talk to it (as per above). I also tried twiddling PPC_HAS_FPU on and PPC_HAS_DOUBLE off without success.

I suppose 2 would be significantly better for users with single-precision floating point applications?

[1] The sim/2376 ticket mentioned is now https://sourceware.org/bugzilla/show_bug.cgi?id=9481 . I'll shoot 'em off an email momentarily to let 'em know it's no longer an issue

Changed on Jul 15, 2015 at 6:24:08 AM by Nick Withers

Attachment: ppc-mfspr.patch added

Proposed patch to use mfspr from the main time base universally; does not address MVME3100 CFLAGS issues

comment:11 Changed on Jul 15, 2015 at 7:03:19 AM by Nick Withers <nick.withers@…>

Owner: set to Nick Withers <nick.withers@…>
Resolution: fixed
Status: newclosed

In e2fcb7dc64c040b8298684148390aba6bf4f4912/rtems:

powerpc: Fix _CPU_Counter_read()

The mftb is not available on Book E processors. Use SPR 268 instead.

Close #2369.

comment:12 Changed on Jul 15, 2015 at 7:05:15 AM by Sebastian Huber

Sorry, I didn't noticed your patch. the Alternate Time Base may run with a much higher frequency, so I would like to keep it.

comment:13 Changed on Jul 15, 2015 at 7:20:13 AM by Sebastian Huber

I would leave the c/src/lib/libcpu/powerpc/shared/include/powerpc-utility.h as is.

comment:14 in reply to:  12 ; Changed on Jul 15, 2015 at 8:20:12 AM by Nick Withers

Replying to sebastian.huber:

Sorry, I didn't noticed your patch. the Alternate Time Base may run with a much higher frequency, so I would like to keep it.

rtems_bsp_delay_in_bus_cycles() at least is gonna be wrong (or misleadingly named, perhaps) for platforms using the ATB where it happens not to be operating at bus frequency, isn't it?

Next thing though is that the ATB isn't supported for e500v1s ( http://cache.freescale.com/files/32bit/doc/ref_manual/E500CORERM.pdf p 1-19 )... Is _CPU_Counter_read()'s

#if defined(ppc8540) || defined(__PPC_CPU_E6500__)

meant to be

#if defined(ppc8548) || defined(__PPC_CPU_E6500__)

...or similar?

It'd be quite nice if we (GCC?) had individual feature flags for some of this stuff, given how all-over-the-place the various PPC cores are, wouldn't it? I'll have a crack if you think so.

Also, would you be alright with removing the "OTOH, PSIM currently lacks support for reading SPRs 268/269. You need GDB patch sim/2376 to avoid a crash..." comment in powerpc-utility.h? ​https://sourceware.org/bugzilla/show_bug.cgi?id=9481 is now closed ('twas Joel fixed the issue, back in 2010).

I've cunningly avoided thinking properly about whether ignoring the upper portion of the time base is a good idea... But, for example, isn't rtems_bsp_delay() going to stuff up if the lower 32 bits wrap?

comment:15 in reply to:  14 ; Changed on Jul 15, 2015 at 8:26:30 AM by Sebastian Huber

Replying to nick.withers:

Replying to sebastian.huber:

Sorry, I didn't noticed your patch. the Alternate Time Base may run with a much higher frequency, so I would like to keep it.

rtems_bsp_delay_in_bus_cycles() at least is gonna be wrong (or misleadingly named, perhaps) for platforms using the ATB where it happens not to be operating at bus frequency, isn't it?

Next thing though is that the ATB isn't supported for e500v1s ( http://cache.freescale.com/files/32bit/doc/ref_manual/E500CORERM.pdf p 1-19 )... Is _CPU_Counter_read()'s

#if defined(ppc8540) || defined(__PPC_CPU_E6500__)

meant to be

#if defined(ppc8548) || defined(__PPC_CPU_E6500__)

...or similar?

This is bad, since we don't have a ppc8548 multilib. In this case we have to drop the defined(ppc8540).

It'd be quite nice if we (GCC?) had individual feature flags for some of this stuff, given how all-over-the-place the various PPC cores are, wouldn't it? I'll have a crack if you think so.

Also, would you be alright with removing the "OTOH, PSIM currently lacks support for reading SPRs 268/269. You need GDB patch sim/2376 to avoid a crash..." comment in powerpc-utility.h? ​https://sourceware.org/bugzilla/show_bug.cgi?id=9481 is now closed ('twas Joel fixed the issue, back in 2010).

Ok, can you please send a separate patch to the mailing list.

I've cunningly avoided thinking properly about whether ignoring the upper portion of the time base is a good idea... But, for example, isn't rtems_bsp_delay() going to stuff up if the lower 32 bits wrap?

The use case for the _CPU_Counter_read() is to measure short time intervals for the SMP lock profiling and short delay loops for device drivers. Do you really want to busy wait for 123 seconds?

The rtems_bsp_*() are legacy functions. I would not use them.

comment:16 in reply to:  15 ; Changed on Jul 15, 2015 at 8:33:06 AM by Nick Withers

Replying to sebastian.huber:

Replying to nick.withers:

Replying to sebastian.huber:

Sorry, I didn't noticed your patch. the Alternate Time Base may run with a much higher frequency, so I would like to keep it.

rtems_bsp_delay_in_bus_cycles() at least is gonna be wrong (or misleadingly named, perhaps) for platforms using the ATB where it happens not to be operating at bus frequency, isn't it?

Next thing though is that the ATB isn't supported for e500v1s ( http://cache.freescale.com/files/32bit/doc/ref_manual/E500CORERM.pdf p 1-19 )... Is _CPU_Counter_read()'s

#if defined(ppc8540) || defined(__PPC_CPU_E6500__)

meant to be

#if defined(ppc8548) || defined(__PPC_CPU_E6500__)

...or similar?

This is bad, since we don't have a ppc8548 multilib. In this case we have to drop the defined(ppc8540).

It'd be quite nice if we (GCC?) had individual feature flags for some of this stuff, given how all-over-the-place the various PPC cores are, wouldn't it? I'll have a crack if you think so.

Also, would you be alright with removing the "OTOH, PSIM currently lacks support for reading SPRs 268/269. You need GDB patch sim/2376 to avoid a crash..." comment in powerpc-utility.h? ​https://sourceware.org/bugzilla/show_bug.cgi?id=9481 is now closed ('twas Joel fixed the issue, back in 2010).

Ok, can you please send a separate patch to the mailing list.

Shall do.

I've cunningly avoided thinking properly about whether ignoring the upper portion of the time base is a good idea... But, for example, isn't rtems_bsp_delay() going to stuff up if the lower 32 bits wrap?

The use case for the _CPU_Counter_read() is to measure short time intervals for the SMP lock profiling and short delay loops for device drivers. Do you really want to busy wait for 123 seconds?

Do you really want to busy wait 123 seconds instead of the intended short time interval because you lost the race and the lower 32 bits of the counter wrapped?

comment:17 in reply to:  16 Changed on Jul 15, 2015 at 8:39:13 AM by Sebastian Huber

Replying to nick.withers:

Replying to sebastian.huber:

Replying to nick.withers:

Replying to sebastian.huber:

[...]

The use case for the _CPU_Counter_read() is to measure short time intervals for the SMP lock profiling and short delay loops for device drivers. Do you really want to busy wait for 123 seconds?

Do you really want to busy wait 123 seconds instead of the intended short time interval because you lost the race and the lower 32 bits of the counter wrapped?

The rtems_counter_delay_ticks() doesn't have this problem. I just may not account for the interrupted time if this time is in the range of one time base period.

comment:18 in reply to:  16 ; Changed on Jul 15, 2015 at 8:41:13 AM by Nick Withers

Replying to nick.withers:

Replying to sebastian.huber:

Replying to nick.withers:

I've cunningly avoided thinking properly about whether ignoring the upper portion of the time base is a good idea... But, for example, isn't rtems_bsp_delay() going to stuff up if the lower 32 bits wrap?

The use case for the _CPU_Counter_read() is to measure short time intervals for the SMP lock profiling and short delay loops for device drivers. Do you really want to busy wait for 123 seconds?

Do you really want to busy wait 123 seconds instead of the intended short time interval because you lost the race and the lower 32 bits of the counter wrapped?

Actually, I'm full of it there I think, sorry. The 32 bit unsigned multiplication's gonna wrap too in that case, right?

comment:19 in reply to:  18 Changed on Jul 15, 2015 at 8:45:37 AM by Sebastian Huber

Replying to nick.withers:

Replying to nick.withers:

Replying to sebastian.huber:

Replying to nick.withers:

I've cunningly avoided thinking properly about whether ignoring the upper portion of the time base is a good idea... But, for example, isn't rtems_bsp_delay() going to stuff up if the lower 32 bits wrap?

The use case for the _CPU_Counter_read() is to measure short time intervals for the SMP lock profiling and short delay loops for device drivers. Do you really want to busy wait for 123 seconds?

Do you really want to busy wait 123 seconds instead of the intended short time interval because you lost the race and the lower 32 bits of the counter wrapped?

Actually, I'm full of it there I think, sorry. The 32 bit unsigned multiplication's gonna wrap too in that case, right?

Yes.

comment:20 Changed on Jul 15, 2015 at 8:53:25 AM by Sebastian Huber <sebastian.huber@…>

In 93f5adb644363d321ddfcbf48586296ed7add189/rtems:

powerpc: Do not use the ATB for e500 multilib

The e500v1 has no support for the ATB.

Update #2369.

Note: See TracTickets for help on using tickets.