[PATCH] score: Add CPU counter support

[Gedare Bloom]

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? 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.

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.

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.

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.

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
> @@ -0,0 +1,140 @@
> +/**
> + * @file
> + *
> + * @ingroup ClassicCPUCounter
> + *
> + * @brief CPU Counter and Delay API
> + */
> +
> +/*
> + * Copyright (c) 2014 embedded brains GmbH.  All rights reserved.
> + *
> + *  embedded brains GmbH
> + *  Dornierstr. 4
> + *  82178 Puchheim
> + *  Germany
> + *  <rtems at embedded-brains.de>
> + *
> + * The license and distribution terms for this file may be
> + * found in the file LICENSE in this distribution or at
> + * http://www.rtems.com/license/LICENSE.
> + */
> +
> +#ifndef _RTEMS_SAPI_CPUCOUNTER_H
> +#define _RTEMS_SAPI_CPUCOUNTER_H
> +
> +#include <rtems/score/cpu.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * @defgroup ClassicCPUCounter CPU Counter and Delay Functions
> + *
> + * @ingroup ClassicRTEMS
> + *
> + * @brief CPU Counter and Delay API.
> + *
> + * A CPU counter is some free-running counter.  It ticks usually with a
> + * frequency close to the CPU or system bus clock.
> + *
> + * @{
> + */
> +
> +/**
> + * @brief Unsigned integer type for CPU counter values.
> + */
> +typedef CPU_counter rtems_cpu_counter;
> +
I would change the type from rtems_cpu_counter to rtems_counter_ticks
-- or rtems_cpu_counter_ticks if you keep the package name as CPU
Counter. The counter is what we call the thing doing the counting, and
the ticks is what we call the value of the counter.

If we add a Counter class to the supercore and introduce a classic
interface, the classic interface will probably need to have a
logically named structure such as rtems_counter or rtems_cpu_counter
to encapsulate its data. So I would reserve that name for now.

> +/**
> + * @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().

> +
> +/**
> + * @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()

> +
> +/**
> + * 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()

> +/**
> + * 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);

> +/**
> + * @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.

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