[PATCH] score: Add CPU counter support

[Sebastian Huber]

Hello Gedare,

On 2014-02-11 19:17, Gedare Bloom wrote:
> I like this idea. I have some requests about the interface though.
>
> First, is there only one possible counter or would it be extended in
> the future to multiple?

why do you need more than one free-running counter?  Linux (and many other 
operating systems) have only one get_cycles() function.

> If there may be multiple, this resource should
> be represented in the score and configured as an Object, with a
> traditional interface in the classic (rtems/rtems) API. I'm not
> convinced putting the interface into sapi is exactly the right thing
> to do, unless some piece of the public API is needed before
> initialization completes. With a classic interface, the 'manager' can
> better encapsulate the counter storage such as the frequency scalar.
> The more I think about it, the more a simple Counter manager makes
> sense to me.

No, a counter object makes no sense and would introduce cyclic dependencies. 
The use case for this free-running counter is profiling, busy wait delay 
functions and maybe also an entropy source for random numbers.

http://www.rtems.org/wiki/index.php?title=SMP#Profiling

This counter must be available early to be of any use and the overhead to read 
the counter must be as small as possible.

>
> Second, there already exists the rtems_bsp_delay() that is usually a
> nop-loop that the new solution should replace where possible. There
> are lots of other BSP "delay" functions that should be retargetted to
> this new interface where appropriate. I guess the rtems_bsp_delay()
> might not be possible to replace because of initialization
> requirements, but it may be doable on a lot of the architectures.

Yes, one motivation to add the counter to the CPU port was to clean up this 
mess, but this is another topic.

>
> Third, I have some issues with the naming convention used. This
> interface lacks a cohesive packaging structure, indeed it even
> self-documents as two APIs, the CPU Counter and Delay API. There is a
> mix of "rtems_cpu_counter_xxx", "_CPU_counter_xxx", "rtems_delay_xxx".
> I would prefer a single consistent interface. My preference would be
> to call it the "Counter" interface, and have functions named like:
> * rtems_counter_xxx: rtems_counter_delay_ticks(),
> rtems_counter_delay_ns(), rtems_counter_get(), etc.
> * _CPU_Counter_xxx: _CPU_Counter_get(), _CPU_Counter_subtract(), etc.

Thanks for your naming suggestions.  I will try to use them for the second 
version of the patch.

>
> I'm not quite sure about the _CPU_ interfaces naming convention. I
> thought in the score the CPU is the "package" and we logically
> associate 'subpackages' and their methods, like _CPU_Context_xxx(),
> _CPU_Exception_xxx(), etc. However, I see at least _CPU_atomic_Xxx()
> violates this, so I don't know what the right way should be by looking
> at the code.

Ok, if we use the rule _Package_name_Sub_package_name_Function_name, then we 
should use

_CPU_Counter_Read()

We should also adjust the CPU atomic API.

>
> Fourth, I would make the below naming changes:
>
> On Tue, Feb 11, 2014 at 11:41 AM, Sebastian Huber
> <sebastian.huber at embedded-brains.de> wrote:
>> Add a CPU counter interface to allow access to a free-running counter.
>> It is useful to measure short time intervals.  This can be used for
>> example to enable profiling of critical low-level functions.
>>
>> Add two busy wait functions rtems_delay_cpu_counter_ticks() and
>> rtems_delay_nanoseconds() implemented via the CPU counter.
>> ---
>
>> diff --git a/cpukit/sapi/include/rtems/cpucounter.h b/cpukit/sapi/include/rtems/cpucounter.h
>> new file mode 100644
>> index 0000000..086665b
>> --- /dev/null
>> +++ b/cpukit/sapi/include/rtems/cpucounter.h
[...]
>> +/**
>> + * @brief Returns the current CPU counter value.
>> + *
>> + * @return The current CPU counter value.
>> + */
>> +static inline rtems_cpu_counter rtems_cpu_counter_get( void )
>> +{
>> +  return _CPU_counter_Get();
>> +}
> I would prefer counter_read() as opposed to counter_get().

Ok.

>
>> +
>> +/**
>> + * @brief Subtracts the lower CPU counter value from the upper and returns the
>> + * result.
>> + *
>> + * This operation may be carried out as a modulo operation depending on the
>> + * range of the CPU counter register.
>> + *
>> + * @param[in] upper The upper CPU counter value.
>> + * @param[in] lower The lower CPU counter value.
>> + *
>> + * @return Returns upper - lower.
>> + */
>> +static inline rtems_cpu_counter rtems_cpu_counter_subtract(
>> +  rtems_cpu_counter upper,
>> +  rtems_cpu_counter lower
>> +)
>> +{
>> +  return _CPU_counter_Subtract( upper, lower );
>> +}
>> +
>> +/**
>> + * brief Converts a CPU counter value into nanoseconds.
>> + *
>> + * @param[in] counter A CPU counter value.
>> + *
>> + * @return The nanoseconds corresponding to the CPU counter value.
>> + */
>> +uint64_t rtems_cpu_counter_to_nanoseconds(
>> +  rtems_cpu_counter counter
>> +);
> I like rtems_counter_ticks_to_nanoseconds()

Ok.

>
>> +
>> +/**
>> + * brief Converts nanoseconds into a CPU counter value.
>> + *
>> + * @param[in] nanoseconds Some nanoseconds.
>> + *
>> + * @return The CPU counter value corresponding to the nanoseconds.
>> + */
>> +rtems_cpu_counter rtems_cpu_counter_from_nanoseconds(
>> +  uint32_t nanoseconds
>> +);
>> +
> And rtems_counter_ticks_from_nanoseconds()

I will use rtems_counter_nanoseconds_to_ticks()

>
>> +/**
>> + * brief Initializes the CPU counter to/from nanoseconds converter functions.
>> + *
>> + * This function must be used to initialize the
>> + * rtems_cpu_counter_to_nanoseconds() and rtems_cpu_counter_from_nanoseconds()
>> + * functions.  It should be called during system initialization by the board
>> + * support package.
>> + *
>> + * @param[in] uint32_t frequency The current CPU counter frequency in Hz.
>> + */
>> +void rtems_cpu_counter_initialize_converter( uint32_t frequency );
>> +
> I don't like this name, because it is not clear what a "converter" is
> just by seeing it. The correct technical term for what is done is to
> compute the prescaler to use when dividing the counter to obtain a
> nanoseconds value. So I would suggest the name:
> rtems_counter_initialize_prescaler(uint32_t frequency);

To change nanoseconds to/from ticks is clearly a conversion.  The existence of 
a prescaler is an implementation detail.  The comment states which converter 
functions are targeted.  So I will keep the name.

>
>> +/**
>> + * @brief Busy wait for some CPU counter ticks.
>> + *
>> + * This function does not disable interrupts.  Thus task switches and
>> + * interrupts can interfere with this busy wait.
>> + *
>> + * @param[in] ticks The minimum busy wait time in CPU counter ticks.
>> + */
>> +void rtems_delay_cpu_counter_ticks( rtems_cpu_counter ticks );
>> +
>> +/**
>> + * @brief Busy wait for some nanoseconds.
>> + *
>> + * This function does not disable interrupts.  Thus task switches and
>> + * interrupts can interfere with this busy wait.
>> + *
>> + * @param[in] nanoseconds The minimum busy wait time in nanoseconds.
>> + */
>> +void rtems_delay_nanoseconds( uint32_t nanoseconds );
>> +
> As mentioned before, I prefer rtems_counter_delay_ticks() and
> rtems_counter_delay_nanoseconds(); It would be good to be more clear
> that the interference can cause the delay to be longer than expected,
> but (it should be) never shorter.

Ok.

>
>> +/** @} */
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif /* __cplusplus */
>> +
>> +#endif /* _RTEMS_SAPI_CPUCOUNTER_H */


-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.