| Message ID | 20240417093542.275150-1-lukasz.czechowski@thaumatec.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v2] rockchip: px30-board-tpl: Use CONFIG_TPL_BANNER_PRINT flag | expand |
Hi Lukasz, I would have renamed the commit title to be something like rockchip: px30-board-tpl: sync ifdef guards with full TPL because CONFIG_TPL_BANNER_PRINT isn't really a flag but a Kconfig symbol and the effect of this patch is to not print the banner when the symbol is not selected. Another option could have been (only for the v1, since you do more now): rockchip: px30-board-tpl: print banner only if CONFIG_TPL_BANNER_PRINT On 4/17/24 11:35, lukasz.czechowski@thaumatec.com wrote: > From: Lukasz Czechowski <lukasz.czechowski@thaumatec.com> > > Display TPL init information message only when TPL_BANNER_PRINT > configuration entry is set. This allows to disable information > message in case logs on UART are unwanted. > Update parent ifdef condition to check also CONFIG_TPL_SERIAL > to match logic of the non-PX30 TPL implementation. > > Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > Cc: Kever Yang <kever.yang@rock-chips.com> > Small nitpick, the Cc would make it to the git history, which isn't really necessary. I believe you could have those below the --- and git-send-email still find them. Additionally, git send-email allows you to provide --cc and --to manually. > --- > Changes for v2: > - Updated parent ifdef condition > --- > arch/arm/mach-rockchip/px30-board-tpl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-rockchip/px30-board-tpl.c b/arch/arm/mach-rockchip/px30-board-tpl.c > index 637a5e1b18..db368a7b8c 100644 > --- a/arch/arm/mach-rockchip/px30-board-tpl.c > +++ b/arch/arm/mach-rockchip/px30-board-tpl.c > @@ -36,7 +36,7 @@ void board_init_f(ulong dummy) > { > int ret; > > -#ifdef CONFIG_DEBUG_UART > +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL) Quite interestingly, CONFIG_TPL_SERIAL cannot be disabled on PX30, maybe we should change arch/arm/mach-rockchip/Kconfig to use an `imply` instead of `select`. Anyway, no change required on my side. Only, if there's a v3 (for the Cc and commit title, the Kconfig change would be a separate patch anyway). Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Thanks, Quentin
Hi Quentin, Thanks for your response. As for the CONFIG_TPL_SERIAL flag, I agree that it should use 'imply' to be allowed to disable it, to silence the serial in TPL. However, currently, disabling this flag makes the code unbuildable (which could be the reason 'select' is used). I'd keep this change out of scope for this commit. I'll prepare v3 then, which will contain only updated Cc and commit title. Best regards, Łukasz śr., 17 kwi 2024 o 11:55 Quentin Schulz <quentin.schulz@theobroma-systems.com> napisał(a): > > Hi Lukasz, > > I would have renamed the commit title to be something like > > rockchip: px30-board-tpl: sync ifdef guards with full TPL > > because CONFIG_TPL_BANNER_PRINT isn't really a flag but a Kconfig symbol > and the effect of this patch is to not print the banner when the symbol > is not selected. > > Another option could have been (only for the v1, since you do more now): > > rockchip: px30-board-tpl: print banner only if CONFIG_TPL_BANNER_PRINT > > On 4/17/24 11:35, lukasz.czechowski@thaumatec.com wrote: > > From: Lukasz Czechowski <lukasz.czechowski@thaumatec.com> > > > > Display TPL init information message only when TPL_BANNER_PRINT > > configuration entry is set. This allows to disable information > > message in case logs on UART are unwanted. > > Update parent ifdef condition to check also CONFIG_TPL_SERIAL > > to match logic of the non-PX30 TPL implementation. > > > > Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com> > > Cc: Tom Rini <trini@konsulko.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > > Cc: Kever Yang <kever.yang@rock-chips.com> > > > > Small nitpick, the Cc would make it to the git history, which isn't > really necessary. I believe you could have those below the --- and > git-send-email still find them. > > Additionally, git send-email allows you to provide --cc and --to manually. > > > --- > > Changes for v2: > > - Updated parent ifdef condition > > --- > > arch/arm/mach-rockchip/px30-board-tpl.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-rockchip/px30-board-tpl.c b/arch/arm/mach-rockchip/px30-board-tpl.c > > index 637a5e1b18..db368a7b8c 100644 > > --- a/arch/arm/mach-rockchip/px30-board-tpl.c > > +++ b/arch/arm/mach-rockchip/px30-board-tpl.c > > @@ -36,7 +36,7 @@ void board_init_f(ulong dummy) > > { > > int ret; > > > > -#ifdef CONFIG_DEBUG_UART > > +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL) > > Quite interestingly, CONFIG_TPL_SERIAL cannot be disabled on PX30, maybe > we should change arch/arm/mach-rockchip/Kconfig to use an `imply` > instead of `select`. > > Anyway, no change required on my side. Only, if there's a v3 (for the Cc > and commit title, the Kconfig change would be a separate patch anyway). > > Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > Thanks, > Quentin
diff --git a/arch/arm/mach-rockchip/px30-board-tpl.c b/arch/arm/mach-rockchip/px30-board-tpl.c index 637a5e1b18..db368a7b8c 100644 --- a/arch/arm/mach-rockchip/px30-board-tpl.c +++ b/arch/arm/mach-rockchip/px30-board-tpl.c @@ -36,7 +36,7 @@ void board_init_f(ulong dummy) { int ret; -#ifdef CONFIG_DEBUG_UART +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL) debug_uart_init(); /* * Debug UART can be used from here if required: @@ -46,7 +46,9 @@ void board_init_f(ulong dummy) * printhex8(0x1234); * printascii("string"); */ +#if CONFIG_TPL_BANNER_PRINT printascii("U-Boot TPL board init\n"); +#endif #endif secure_timer_init();