[PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

Hi Heinrich,

On 01.08.22 09:54, Heinrich Schuchardt wrote:
> On 7/28/22 09:09, Stefan Roese wrote:
>> This patchset adds 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?
>> This patchset 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.
>>
>> Ideas how to continue:
>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
>> move the watchdog_reset call into this cyclic infrastructure as well.
>> Or to perhaps move the shell UART RX ready polling to a cyclic
>> function.
> 
> Hello Stefan
> 
> I am missing rework defining where WATCHDOG_RESET has to be called:
> 
> WATCHDOG_RESET is called in net_loop() but not in eth_rx() and eth_tx().
> This is clearly the wrong place as not all network traffic uses net_loop().
> 
> Same is true for your reference to UART rx. WATCHDOG_RESET is called in
> individual UART drivers. But this does not help if tstc() is calling
> usb_kbd_testc().
> 
> So before adding this patchset please provide the concept defining where
> WATCHDOG_RESET should be invoked.

I agree that you are addressing valid points here. But this patchset
is not intended to change the WATCHDOG_RESET functionality. These
WDT calls and their integration (sprinkling) in the U-Boot source
are very ancient. I intentionally did not change anything here to
not break / change anything in this area for now.

The points listed from you above are all valid and most likely good
improvements or even fixes to the watchdog and once accepted also the
cyclic infrastructure. Still I think it makes much sense to address
them, once this patchset is in mainline. After this, many improvements
might be done, like:

- Integrate WDT IF as a "user" into the cyclic IF
- Rename WATCHDOG_RESET to something better matching its functionality
- Improve locations, from where these cyclic IF is called (see your
   suggestions above)
- ...

> A framework like the one proposed requires documentation. This needs to
> be an integral part of the next version of the series.

Ok, I admit that I'm a bit lazy here. I'll add some documentation in
the next version.

> With the infrastructure you are building efi_timer_check() is a
> candidate for refactoring. Currently efi_timer_check() is called
> whenever the EFI sub-system is waiting for anything (keyboard input,
> polling events, network I/O).

Interesting. I have to admit that my internal knowledge of the EFI
subsystem in U-Boot is a bit "limited". If you see improvements to this
cyclic IF that should be made, so that this cyclic IF is even more
generic and can better be used by e.g. efi_timer_check(), please let
me know.

Thanks,
Stefan