| Age | Commit message (Collapse) | Author |
|---|
|
Now that cyclic_unregister() is safe to call on a not-registered
cyclic_info, we can make cyclic_register() behave like the mod_timer()
and hrtimer_start() APIs in linux, in that they don't distinguish
between whether the timer was already enabled or not; from the point
of the call it is, with whatever timeout/period is set in that most
recent call.
This avoids users of the cyclic API from separately keeping track of
whether their callback is already registered or not, and even if they
know it is, can be used for changing the period (and/or the callback
function) without first doing unregister().
See also this recent'ish message from kernel maintainer Thomas
Gleixner on that API design for timer frameworks:
https://lore.kernel.org/lkml/87ikn6sibi.ffs@tglx/
First of all the question is whether add() and mod() are really
valuable distinctions. I'm not convinced at all. Back then, when we
introduced hrtimers, we came to the conclusion that hrtimer_start()
is sufficient.
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
Make cyclic_unregister() safe to call with an already unregistered, or
possibly never registered, struct cyclic_info. This is similar to how
the various timer APIs in the linux kernel work (they all allow
calling delete/cancel/... on an inactive timer object).
This means callers don't have to separately keep track of whether
their cyclic callback is registered or not, and avoids them trying to
peek into the struct cyclic_info for that information - which leads to
somewhat ugly code as it would have to be guarded by ifdef
CONFIG_CYCLIC.
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
Make the schedule() call from the CYCLIC framework a uthread scheduling
point too. This makes sense since schedule() is called from a lot of
places where uthread_schedule() needs to be called.
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
On 32 bits platforms, timer_get_us() returns an unsigned long which
is a 32 bits. timer_get_us() wraps around every 72 minutes
(2 ^ 32 / 1000000 =~ 4295 sec =~ 72 min).
So the test "if time_after_eq64(now, cyclic->next_call)" is no more
true when cyclic->next_call becomes above 32 bits max value (4294967295).
At this point after 72 min, no more cyclic function are
executed included watchdog one.
Instead of using timer_get_us(), use get_timer_us() which returns a
uint64_t, this allows a rollover every 584942 years.
Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
The only caller left is schedule(); everybody outside cyclic.c now
calls or references schedule().
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
I noticed an "unnecessary" include of <cyclic.h> in
global_data.h, in the sense that nothing in cyclic.h is needed in
order to define 'struct global_data'.
Well, it's not unnecessary, as it implicitly ensures that everybody
gets a declaration of schedule(), and schedule() is (obviously) called
all over the tree. Almost none of those places directly include
<cyclic.h>, but for historical reasons, many do include
<watchdog.h> (most schedule() instances are replacements of
WATCHDOG_RESET()).
However, very few TUs actually need the declarations of the
cyclic_register() and struct cyclic_info, and they also don't really
need anything from the watchdog.h header.
So introduce a new header which just contains a declaration of
schedule(), which can then be included from all the places that do
call schedule(). I removed the direct reference to cyclic_run(),
because we shouldn't have two public functions for doing roughly the
same without being very explicit about when one should call one or the
other.
Testing of later patches that explicitly include <schedule.h> when
schedule() is used revealed a problem with host tool build on win32,
which apparently picked up a host <schedule.h>. To avoid that problem,
put the new header in include/u-boot/ and hence make the include
statements say <u-boot/schedule.h>.
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
|
|
There are of course not a whole lot of examples in-tree yet, but
before they appear, let's make this API change: Instead of separately
allocating a 'struct cyclic_info', make the users embed such an
instance in their own structure, and make the convention that the
callback simply receives the 'struct cyclic_info *', from which the
clients can get their own data using the container_of() macro.
This has a number of advantages.
First, it means cyclic_register() simply cannot fail, simplifying the
code. The necessary storage will simply be allocated automatically
when the client's own structure is allocated (often via
uclass_priv_auto or similar).
Second, code for which CONFIG_CYCLIC is just an option can more easily
be written without #ifdefs, if we just provide an empty struct
cyclic_info {}. For example, the nested CONFIG_IS_ENABLED()s in
https://lore.kernel.org/u-boot/20240316201416.211480-1-marek.vasut+renesas@mailbox.org/
are mostly due to the existence of the 'struct cyclic_info *' member
being guarded by #ifdef CONFIG_CYCLIC.
And we do probably want to avoid the extra memory overhead of that
member when !CONFIG_CYCLIC. But that is automatic if, instead of a
'struct cyclic_info *', one simply embeds a 'struct cyclic_info',
which will have size 0 when !CONFIG_CYCLIC. Also, the no-op
cyclic_register() function can just unconditionally be called, and the
compiler will see that (1) the callback is referenced, so not emit a
warning for a maybe-unused function and (2) see that it can actually
never be reached, so not emit any code for it.
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
|
|
We are not checking the return value of strdup(), nor
freeing the string in cyclic_unregister().
However, all current users either pass a string literal or the
dev->name of the client device. So in all cases the name string will
live at least as long as the cyclic_info is registered, so just make
that a requirement.
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
|
|
Currently, we must call cyclic_init() at some point before
cyclic_register() becomes possible. That turns out to be somewhat
awkward, especially with SPL, and has resulted in a watchdog callback
not being registered, thus causing the board to prematurely reset.
We already rely on gd->cyclic reliably being set to NULL by the asm
code that clears all of gd. Now that the cyclic list is a hlist, and
thus an empty list is represented by a NULL head pointer, and struct
cyclic_drv has no other members, we can just as well drop a level of
indirection and put the hlist_head directly in struct
global_data. This doesn't increase the size of struct global_data,
gets rid of an early malloc(), and generates slightly smaller code.
But primarily, this avoids having to call cyclic_init() early; the cyclic
infrastructure is simply ready to register callbacks as soon as we
enter C code.
We can still end up with schedule() being called from asm very early,
so we still need to check that gd itself has been properly initialized
[*], but once it has, gd->cyclic_list is perfectly fine to access, and
will just be an empty list.
As for cyclic_uninit(), it was never really the opposite of
cyclic_init() since it didn't free the struct cyclic_drv nor set
gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that
in test/, and also insert a call at the end of the board_init_f
sequence so that gd->cyclic_list is a fresh empty list before we enter
board_init_r().
A small piece of ugliness is that I had to add a cast in
cyclic_get_list() to silence a "discards 'volatile' qualifier"
warning, but that is completely equivalent to the existing handling of
the uclass_root_s list_head member.
[*] I'm not really sure where we guarantee that the register used for
gd contains 0 until it gets explicitly initialized, but that must be
the case, otherwise testing gd for being NULL would not make much sense.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>
Tested-by: Tim Harvey <tharvey@gateworks.com> # imx8mm-venice-*
|
|
A hlist is headed by just a single pointer, so can only be traversed
forwards, and insertions can only happen at the head (or before/after
an existing list member). But each list node still consists of two
pointers, so arbitrary elements can still be removed in O(1).
This is precisely what we need for the cyclic_list - we never need to
traverse it backwards, and the order the callbacks appear in the list
should really not matter.
One advantage, and the main reason for doing this switch, is that an
empty list is represented by a NULL head pointer, so unlike a
list_head, it does not need separate C code to initialize - a
memset(,0,) of the containing structure is sufficient.
This is mostly mechanical:
- The iterators are updated with an h prefix, and the type of the
temporary variable changed to struct hlist_node*.
- Adding/removing is now just hlist_add_head (and not tail) and
hlist_del().
- struct members and function return values updated.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>
Tested-by: Tim Harvey <tharvey@gateworks.com> # imx8mm-venice-*
|
|
We're already relying on gd->cyclic being NULL before cyclic_init() is
called - i.e., we're relying on all of gd being zeroed before entering
any C code. And when we do populate gd->cyclic, its ->cyclic_ready
member is automatically set to true. So we can actually just rely on
testing gd->cyclic itself.
The only wrinkle is that cyclic_uninit() actually did set
->cyclic_ready to false. However, since it doesn't free gd->cyclic,
the cyclic infrastructure is actually still ready (i.e., the list_head
is properly initialized as an empty list).
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>
Tested-by: Tim Harvey <tharvey@gateworks.com> # imx8mm-venice-*
|
|
As a preparation for future patches, use a flag in gd->flags rather
than a separate member in (the singleton) struct cyclic_drv to keep
track of whether we're already inside cyclic_run().
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>
Tested-by: Tim Harvey <tharvey@gateworks.com> # imx8mm-venice-*
|
|
With the migration of the watchdog infrastructure to cyclic functions
it's been noticed, that at least one watchdog driver is broken now. As
the execution time of it's watchdog reset function is quite long.
In general it's not really necessary (right now) to disable the cyclic
function upon exceeding CPU time usage. So instead of disabling the
cylic function in this case, let's just print a warning once to show
this potential problem to the user.
Signed-off-by: Stefan Roese <sr@denx.de>
Suggested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Tom Rini <trini@konsulko.com>
Cc: Pali Rohár <pali@kernel.org>
|
|
This patch introduces a schedule() function, which shall be used instead
of the old WATCHDOG_RESET. Follow-up patches will make sure, that this
new function is used.
Signed-off-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Tom Rini <trini@konsulko.com> [am335x_evm, mx6cuboxi, rpi_3,dra7xx_evm, pine64_plus, am65x_evm, j721e_evm]
|
|
Add the basic infrastructure to periodically execute code, e.g. all
100ms. Examples for such functions might be LED blinking etc. The
functions that are hooked into this cyclic list should be small timewise
as otherwise the execution of the other code that relies on a high
frequent polling (e.g. UART rx char ready check) might be delayed too
much. This patch also adds the Kconfig option
CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
for such a cyclic function. If it's execution time exceeds this time,
this cyclic function will get removed from the cyclic list.
How is this cyclic functionality executed?
The following patch integrates the main function responsible for
calling all registered cyclic functions cyclic_run() into the
common WATCHDOG_RESET macro. This guarantees that cyclic_run() is
executed very often, which is necessary for the cyclic functions to
get scheduled and executed at their configured periods.
This cyclic infrastructure will be used by a board specific function on
the NIC23 MIPS Octeon board, which needs to check periodically, if a
PCIe FLR has occurred.
Signed-off-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
|
