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

[Stefan Roese <sr@denx.de>]

Hi Simon,

On 01.08.22 14:22, Simon Glass wrote:
> Hi Stefan,
> 
> On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 31.07.22 03:27, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> 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.
>>>>
>>>> It's also possible to extend the "cyclic" command, to support the
>>>> creation of periodically executed shell commands (for testing etc).
>>>>
>>>> Here the Azure build, without any issues:
>>>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>> Aaron Williams (1):
>>>>     mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>>>>
>>>> Stefan Roese (6):
>>>>     time: Import time_after64() and friends from Linux
>>>>     cyclic: Add basic support for cyclic function execution infrastruture
>>>>     cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>>>>     cyclic: Integrate cyclic functionality at bootup in board_r/f
>>>>     cyclic: Add 'cyclic list' command
>>>>     sandbox: Add cyclic demo function
>>>>
>>>>    MAINTAINERS                        |   7 +
>>>>    board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>>>>    board/sandbox/sandbox.c            |  15 +++
>>>>    cmd/Kconfig                        |   7 +
>>>>    cmd/Makefile                       |   1 +
>>>>    cmd/cyclic.c                       |  40 ++++++
>>>>    common/Kconfig                     |  20 +++
>>>>    common/Makefile                    |   1 +
>>>>    common/board_f.c                   |   2 +
>>>>    common/board_r.c                   |   2 +
>>>>    common/cyclic.c                    | 112 ++++++++++++++++
>>>>    configs/octeon_nic23_defconfig     |   3 +
>>>>    configs/sandbox_defconfig          |   3 +
>>>>    fs/cramfs/uncompress.c             |   2 +-
>>>>    include/cyclic.h                   |  97 ++++++++++++++
>>>>    include/time.h                     |  19 +++
>>>>    include/watchdog.h                 |  23 +++-
>>>>    17 files changed, 547 insertions(+), 4 deletions(-)
>>>>    create mode 100644 cmd/cyclic.c
>>>>    create mode 100644 common/cyclic.c
>>>>    create mode 100644 include/cyclic.h
>>>>
>>>> --
>>>> 2.37.1
>>>>
>>>
>>> This looks interesting. I like the idea of integrating the watchdog
>>> stuff too, since you are making use of it.
>>>
>>> Would it make sense to make use of the new event system, since it has
>>> static and dynamic handlers?
>>
>> IIRC, I tried to make use of this infrastructure but it did not work
>> out. Or do you see some way to integrate this cyclic IF into the
>> event system? FWICT, the only way to get this done is to make use of
>> the (ugly) WATCHDOG_RESET calls.
> 
> Yes that makes sense. I don't see another way.
> 
> But within that, one option would be to send an event every time
> WATCHDOG_RESET is used, and have things register as an event spy, thus
> making use of that existing system. The event feature is not enabled
> by default, but it could be enabled when this functionality is needed.

I still need to double-check, sorry: You are thinking about this call-
trace:

WATCHDOG_RESET -> event IF -> cylic IF -> cylic user

?

If yes, what would be the advantage(s) against implementing this
separately?

Thanks,
Stefan