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

[Simon Glass <sjg@chromium.org>]

Hi Stefan,

On Mon, 1 Aug 2022 at 08:09, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 01.08.22 15:00, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Mon, 1 Aug 2022 at 06:40, Stefan Roese <sr@denx.de> wrote:
> >>
> >> 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
> >>
> >
> > More this, i.e. I am wondering if the cyclic functionality can be done
> > using events.
> >
> > WATCHDOG_RESET -> sends watchdog event -> event spy receives event
>
> This would mean in the end, that all U-Boot targets would need to
> enable the event subsystem as well. This might lead to problems with
> image sizes on some of those especially old/ancient targets. Hmmm...

It depends on whether cyclic is a small adder to events or something
larger. Actually events are not too big, but having 'dynamic' ones
(allocated at runtime) does add a bit more.

>
> >> ?
> >>
> >> If yes, what would be the advantage(s) against implementing this
> >> separately?
> >
> > It would need handling of max CPU time and perhaps some other tweaks,
> > but it would result in less code (?) and one less thing for people to
> > learn. The cyclic command could still be provided if that is useful,
> > by showing only cyclic certain event handlers.
>
> Frankly, I'm not 100% convinced that this would really result in less
> code. Or even worse, it could (?) result in more complex code, as both
> interfaces are in the same area but still handle different tasks,
> e.g. synchronous vs. asynchronous.
>
> So my preference would be to continue on my current path. I'll integrate
> some documentation in my next patchset version and will resubmit
> hopefully sometime later this week.

OK, sounds good. You are writing the code :-)

Regards,
Simon