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.hu...@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.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
_______________________________________________
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
