Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER

From: Simon Glass <sjg@chromium.org>
To: Stefan Roese <sr@denx.de>
Cc: U-Boot Mailing List <u-boot@lists.denx.de>,
	Tom Rini <trini@konsulko.com>
Subject: Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
Date: Tue, 27 Sep 2022 19:54:52 -0600	[thread overview]
Message-ID: <CAPnjgZ2KmRUpd4t9_g_to44tw3ZBRjdXG3f2uT08_O-pjGAVnQ@mail.gmail.com> (raw)
In-Reply-To: <81bf9126-2451-1e87-c8f0-dbc2cec7843d@denx.de>

Hi Stefan,

On Mon, 26 Sept 2022 at 07:52, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 25.09.22 16:15, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
> >>
> >> The early timer functions provided via CONFIG_TIMER_EARLY don't need
> >> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
> >> and uclass interface. This patch now allow the selection of
> >> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
> >> functionality also for non CONFIG_TIMER drivers.
> >>
> >> With this change it's necessary to guard the dm_timer_init() call
> >> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> ---
> >>   common/board_f.c      | 2 +-
> >>   common/board_r.c      | 2 +-
> >>   drivers/timer/Kconfig | 1 -
> >>   3 files changed, 2 insertions(+), 3 deletions(-)
> >
> > I don't like this as it complicates the logic and also seems to be
> > adding a new feature to legacy code.
> >
> > Instead, let's enable the early timer only for driver model.
>
> Hmmm, not sure how this should work. Do you have this in mind (instead
> of this patch)?
>
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index fd8745ffc2e0..30d6efe98f29 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -39,7 +39,7 @@ config VPL_TIMER
>
>   config TIMER_EARLY
>          bool "Allow timer to be used early in U-Boot"
> -       depends on TIMER
> +       depends on DM
>          # initr_bootstage() requires a timer and is called before
> initr_dm()
>          # so only the early timer is available
>          default y if X86 && BOOTSTAGE
>
> This results in some compilation errors, like this:
>
> $ make stm32mp15_basic_defconfig
> $ make -sj
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> common/board_f.o: in function `initf_dm':
> /home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
> reference to `dm_timer_init'
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> common/board_r.o: in function `initr_dm_devices':
> /home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
> reference to `dm_timer_init'
> make: *** [Makefile:1790: u-boot] Error 1
>
> I might be missing something. Or it's not that easy and we still need
> my original implementation.

Well, given that TIMER depends on DM, I don't think you need to change that.

Also, TIMER_EARLY depends on TIMER (as it should), you should just be
able to check for IS_ENABLED(CONFIG_TIMER)

We can disable the early timer stuff for things that don't use CONFIG_TIMER

Regards,
Simon