| Message ID | 20241128114737.1.I9eac706713ffc05dccc51ad3d60833a179c9e83d@changeid |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [1/2] efi_stub: Sync the debug UART definition like other platform | expand |
Hi Tom, Simon, With these two patches, I didn't see error report in efi_stub for disable DEBUG_UART, but I still get CI build error below, but I can't identify what's the problem, could you help to take a look: https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/963215 Thanks, - Kever On 2024/11/28 11:47, Kever Yang wrote: > The debug UART interface is available when CONFIG_DEBUG_UART is defined, > sync with the other platforms to use the same definition. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > lib/efi/efi_stub.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c > index 40fc29d9adf..5179c5b2c09 100644 > --- a/lib/efi/efi_stub.c > +++ b/lib/efi/efi_stub.c > @@ -9,7 +9,6 @@ > * EFI application. It can be built either in 32-bit or 64-bit mode. > */ > > -#include <debug_uart.h> > #include <efi.h> > #include <efi_api.h> > #include <errno.h> > @@ -55,10 +54,6 @@ struct __packed desctab_info { > * considering if we start needing more U-Boot functionality. Note that we > * could then move get_codeseg32() to arch/x86/cpu/cpu.c. > */ > -void _debug_uart_init(void) > -{ > -} > - > void putc(const char ch) > { > struct efi_priv *priv = efi_get_priv(); > @@ -83,12 +78,21 @@ void puts(const char *str) > putc(*str++); > } > > -static void _debug_uart_putc(int ch) > +#ifdef CONFIG_DEBUG_UART > + > +#include <debug_uart.h> > + > +void _debug_uart_init(void) > +{ > +} > + > +static inline void _debug_uart_putc(int ch) > { > putc(ch); > } > > DEBUG_UART_FUNCS > +#endif > > void *memcpy(void *dest, const void *src, size_t size) > {
On Fri, Nov 29, 2024 at 11:22:50AM +0800, Kever Yang wrote: > Hi Tom, Simon, > > With these two patches, I didn't see error report in efi_stub for > disable DEBUG_UART, > > but I still get CI build error below, but I can't identify what's the > problem, could you help to take a look: > > https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/963215 So, with: 88 60 2 /150 qemu-x86_64 We know two boards failed to build. I don't know why buildman doesn't then list them as part of the summary, and please file an issue in Gitlab for Simon for that. However, we can try and figure it out by: ./tools/buildman/buildman -o /tmp -PEWM -x arm,powerpc --dry-run -v In that tree, and seeing what's listed there but not in the CI output summary.
Hi Tom, On Thu, 28 Nov 2024 at 20:29, Tom Rini <trini@konsulko.com> wrote: > > On Fri, Nov 29, 2024 at 11:22:50AM +0800, Kever Yang wrote: > > Hi Tom, Simon, > > > > With these two patches, I didn't see error report in efi_stub for > > disable DEBUG_UART, > > > > but I still get CI build error below, but I can't identify what's the > > problem, could you help to take a look: > > > > https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/963215 > > So, with: > 88 60 2 /150 qemu-x86_64 > > We know two boards failed to build. I don't know why buildman > doesn't then list them as part of the summary, and please file an issue > in Gitlab for Simon for that. However, we can try and figure it out by: > ./tools/buildman/buildman -o /tmp -PEWM -x arm,powerpc --dry-run -v > In that tree, and seeing what's listed there but not in the CI output > summary. Yes, something odd going on. I recently had a build failure in efi-x86_app64 which sailed right through CI. Regards, Simon
On 28.11.24 04:47, Kever Yang wrote: > The debug UART interface is available when CONFIG_DEBUG_UART is defined, > sync with the other platforms to use the same definition. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > lib/efi/efi_stub.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c > index 40fc29d9adf..5179c5b2c09 100644 > --- a/lib/efi/efi_stub.c > +++ b/lib/efi/efi_stub.c > @@ -9,7 +9,6 @@ > * EFI application. It can be built either in 32-bit or 64-bit mode. > */ > > -#include <debug_uart.h> > #include <efi.h> > #include <efi_api.h> > #include <errno.h> > @@ -55,10 +54,6 @@ struct __packed desctab_info { > * considering if we start needing more U-Boot functionality. Note that we > * could then move get_codeseg32() to arch/x86/cpu/cpu.c. > */ > -void _debug_uart_init(void) > -{ > -} > - > void putc(const char ch) > { > struct efi_priv *priv = efi_get_priv(); > @@ -83,12 +78,21 @@ void puts(const char *str) > putc(*str++); > } > > -static void _debug_uart_putc(int ch) > +#ifdef CONFIG_DEBUG_UART Why do we need this #ifdef? In other places we leave it to the linker to remove unused functions. > + > +#include <debug_uart.h> You cannot consume function _debug_uart_putc() before defining it. So either the #include statement must follow the implementations or you have to define the function prototypes in the include. I would prefer adding the missing definitions to the include and leaving it at the top of the code. Best regards Heinrich > + > +void _debug_uart_init(void) > +{ > +} > + > +static inline void _debug_uart_putc(int ch) > { > putc(ch); > } > > DEBUG_UART_FUNCS > +#endif > > void *memcpy(void *dest, const void *src, size_t size) > {
Hi Heinrich, On 2025/1/3 23:43, Heinrich Schuchardt wrote: > On 28.11.24 04:47, Kever Yang wrote: >> The debug UART interface is available when CONFIG_DEBUG_UART is defined, >> sync with the other platforms to use the same definition. >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> --- >> >> lib/efi/efi_stub.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c >> index 40fc29d9adf..5179c5b2c09 100644 >> --- a/lib/efi/efi_stub.c >> +++ b/lib/efi/efi_stub.c >> @@ -9,7 +9,6 @@ >> * EFI application. It can be built either in 32-bit or 64-bit mode. >> */ >> >> -#include <debug_uart.h> >> #include <efi.h> >> #include <efi_api.h> >> #include <errno.h> >> @@ -55,10 +54,6 @@ struct __packed desctab_info { >> * considering if we start needing more U-Boot functionality. Note >> that we >> * could then move get_codeseg32() to arch/x86/cpu/cpu.c. >> */ >> -void _debug_uart_init(void) >> -{ >> -} >> - >> void putc(const char ch) >> { >> struct efi_priv *priv = efi_get_priv(); >> @@ -83,12 +78,21 @@ void puts(const char *str) >> putc(*str++); >> } >> >> -static void _debug_uart_putc(int ch) >> +#ifdef CONFIG_DEBUG_UART > > Why do we need this #ifdef? In other places we leave it to the linker to > remove unused functions. I think you are right for most of modules, but for this DEBUG_UART, it's a little different because it's for early debug, and not so stand alone to disable. And now we are try to make it clean and able to disable, see patch[1]. > >> + >> +#include <debug_uart.h> > > You cannot consume function _debug_uart_putc() before defining it. > This is following what all other serial driver have done. And I think put all the code support for DEBUG_UART together is reasonable. This efi_stub is a little bit different and cause the DEBUG_UART not able to disable, that's why I'm doing this "sync" commit. Thanks, - Kever [1] https://patchwork.ozlabs.org/project/uboot/patch/20240918130155.299353-2-lukasz.czechowski@thaumatec.com/ > So either the #include statement must follow the implementations or you > have to define the function prototypes in the include. > > I would prefer adding the missing definitions to the include and leaving > it at the top of the code. > > Best regards > > Heinrich > >> + >> +void _debug_uart_init(void) >> +{ >> +} >> + >> +static inline void _debug_uart_putc(int ch) >> { >> putc(ch); >> } >> >> DEBUG_UART_FUNCS >> +#endif >> >> void *memcpy(void *dest, const void *src, size_t size) >> { > >
diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c index 40fc29d9adf..5179c5b2c09 100644 --- a/lib/efi/efi_stub.c +++ b/lib/efi/efi_stub.c @@ -9,7 +9,6 @@ * EFI application. It can be built either in 32-bit or 64-bit mode. */ -#include <debug_uart.h> #include <efi.h> #include <efi_api.h> #include <errno.h> @@ -55,10 +54,6 @@ struct __packed desctab_info { * considering if we start needing more U-Boot functionality. Note that we * could then move get_codeseg32() to arch/x86/cpu/cpu.c. */ -void _debug_uart_init(void) -{ -} - void putc(const char ch) { struct efi_priv *priv = efi_get_priv(); @@ -83,12 +78,21 @@ void puts(const char *str) putc(*str++); } -static void _debug_uart_putc(int ch) +#ifdef CONFIG_DEBUG_UART + +#include <debug_uart.h> + +void _debug_uart_init(void) +{ +} + +static inline void _debug_uart_putc(int ch) { putc(ch); } DEBUG_UART_FUNCS +#endif void *memcpy(void *dest, const void *src, size_t size) {
The debug UART interface is available when CONFIG_DEBUG_UART is defined, sync with the other platforms to use the same definition. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- lib/efi/efi_stub.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)