[PATCH 1/2] efi_stub: Sync the debug UART definition like other platform
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) {
The efi_stub is useing DEBUG_UART interface by default, Enable it.
Signed-off-by: Kever Yang kever.yang@rock-chips.com ---
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
On 1/3/25 17:16, Heinrich Schuchardt wrote:
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi- x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi- x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
Building fails without a value DEBUG_UART_BASE.
Looking at function putc() in lib/efi/efi_stub.c I guess this should be DEBUG_UART_BASE=0x3f8.
Best regards
Heinrich
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
Hi Heinrich,
On Sat, 4 Jan 2025 at 05:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
If I said that I was wrong, sorry. Actually that is the stub's intent, to take over the machine, exit boot-services and carry on. It is the EFI app which is designed to use boot services.
So the stub cannot be hardware-agnostic, unfortunately. It only runs on x86 hardware, unless, for example, EFI has something like serial_getinfo() or we disable the UART / keyboard input.
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Once the stub starts, boot services are gone. See efi_main() for how this works. There is a definite point at which we stop using that and start using U-Boot's own thing.
At this point I think I may be misunderstanding what you said...
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
Regards, Simon
Am 4. Januar 2025 20:29:43 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Sat, 4 Jan 2025 at 05:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
If I said that I was wrong, sorry. Actually that is the stub's intent, to take over the machine, exit boot-services and carry on. It is the EFI app which is designed to use boot services.
So the stub cannot be hardware-agnostic, unfortunately. It only runs on x86 hardware, unless, for example, EFI has something like serial_getinfo() or we disable the UART / keyboard input.
I would assume that there is no serial console that the user has access to on many x86 systems (like on my laptop).
In the stub itself we can use the Simple Text Output Protocol and the Simple Text Input Protocol offered by the UEFI implementation that started the stub.
The UEFI specification defines a Serial Io Protocol which may be implemented but I would not know why we should restrict the usability in this way.
Once we start U-Boot of course we will use hardware specific IO, e.g. on a RISC-V system use the debug console offered by the SBI
Best regards
Heinrich
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Once the stub starts, boot services are gone. See efi_main() for how this works. There is a definite point at which we stop using that and start using U-Boot's own thing.
At this point I think I may be misunderstanding what you said...
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
Regards, Simon
Hi Heinrich and Simon,
I send these two change because I want to fix the build error below(from[1]) which is for disable the DEBUG_UART[2]. Do you have clear suggestion on how to do this in efi_stub, since I have no idea about this module, so I just follow how it works in other serial driver. +/binman/rom/intel-mrc (mrc.bin): +Image 'rom' has faked external blobs and is non-functional: descriptor.bin me.bin refcode.bin vga.bin mrc.bin +In file included from lib/efi/efi_stub.c:12: +include/debug_uart.h:205:9: error: stray '#' in program + 205 | #warning "DEBUG_UART not defined!" + | ^ +lib/efi/efi_stub.c:91:1: note: in expansion of macro 'DEBUG_UART_FUNCS' + 91 | DEBUG_UART_FUNCS + | ^~~~~~~~~~~~~~~~ +include/debug_uart.h:205:18: error: expected '=', ',', ';', 'asm' or '__attribute__' before string constant + | ^~~~~~~~~~~~~~~~~~~~~~~~~ +lib/efi/efi_stub.c:86:13: error: '_debug_uart_putc' defined but not used [-Werror=unused-function] + 86 | static void _debug_uart_putc(int ch) + | ^~~~~~~~~~~~~~~~ +make[3]: *** [scripts/Makefile.build:257: lib/efi/efi_stub.o] Error 1 +make[2]: *** [scripts/Makefile.build:398: lib/efi] Error 2
Thanks, - Kever [1] https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/987462 [2] https://patchwork.ozlabs.org/project/uboot/patch/20240918130155.299353-2-luk...
On 2025/1/5 07:02, Heinrich Schuchardt wrote:
Am 4. Januar 2025 20:29:43 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Sat, 4 Jan 2025 at 05:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
If I said that I was wrong, sorry. Actually that is the stub's intent, to take over the machine, exit boot-services and carry on. It is the EFI app which is designed to use boot services.
So the stub cannot be hardware-agnostic, unfortunately. It only runs on x86 hardware, unless, for example, EFI has something like serial_getinfo() or we disable the UART / keyboard input.
I would assume that there is no serial console that the user has access to on many x86 systems (like on my laptop).
In the stub itself we can use the Simple Text Output Protocol and the Simple Text Input Protocol offered by the UEFI implementation that started the stub.
The UEFI specification defines a Serial Io Protocol which may be implemented but I would not know why we should restrict the usability in this way.
Once we start U-Boot of course we will use hardware specific IO, e.g. on a RISC-V system use the debug console offered by the SBI
Best regards
Heinrich
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Once the stub starts, boot services are gone. See efi_main() for how this works. There is a definite point at which we stop using that and start using U-Boot's own thing.
At this point I think I may be misunderstanding what you said...
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
Regards, Simon
Hi Kever,
On Sun, 5 Jan 2025 at 23:47, Kever Yang kever.yang@rock-chips.com wrote:
Hi Heinrich and Simon,
I send these two change because I want to fix the build errorbelow(from[1]) which is for disable the DEBUG_UART[2]. Do you have clear suggestion on how to do this in efi_stub, since I have no idea about this module, so I just follow how it works in other serial driver. +/binman/rom/intel-mrc (mrc.bin): +Image 'rom' has faked external blobs and is non-functional: descriptor.bin me.bin refcode.bin vga.bin mrc.bin +In file included from lib/efi/efi_stub.c:12: +include/debug_uart.h:205:9: error: stray '#' in program
- 205 | #warning "DEBUG_UART not defined!"
| ^+lib/efi/efi_stub.c:91:1: note: in expansion of macro 'DEBUG_UART_FUNCS'
- 91 | DEBUG_UART_FUNCS
| ^~~~~~~~~~~~~~~~+include/debug_uart.h:205:18: error: expected '=', ',', ';', 'asm' or '__attribute__' before string constant
| ^~~~~~~~~~~~~~~~~~~~~~~~~+lib/efi/efi_stub.c:86:13: error: '_debug_uart_putc' defined but not used [-Werror=unused-function]
- 86 | static void _debug_uart_putc(int ch)
| ^~~~~~~~~~~~~~~~+make[3]: *** [scripts/Makefile.build:257: lib/efi/efi_stub.o] Error 1 +make[2]: *** [scripts/Makefile.build:398: lib/efi] Error 2
Yes, I see what you are doing.
The real fix is probably to not use the debug UART in the stub. There is a large comment:
* EFI uses Unicode and we don't. The easiest way to get a sensible output * function is to use the U-Boot debug UART. We use EFI's console output * function where available, and assume the built-in UART after that. We rely * on EFI to set up the UART for us and just bring in the functions here. * This last bit is a bit icky, but it's only for debugging anyway. We could * build in ns16550.c with some effort, but this is a payload loader after * all. * * Note: We avoid using printf() so we don't need to bring in lib/vsprintf.c. * That would require some refactoring since we already build this for U-Boot. * Building an EFI shared library version would have to be a separate stem. * That might push us to using the SPL framework to build this stub. However * that would involve a round of EFI-specific changes in SPL. Worth * considering if we start needing more U-Boot functionality. Note that we * could then move get_codeseg32() to arch/x86/cpu/cpu.c.
We don't have a way of building the same file twice (once for the stub and once for U-Boot). That would require my split-config series - Tom rejected the last 10 patches (which actually did the work) a few years ago[1]. But I do plan to get back to it.
For now, I suggest adding a new Kconfig, .e.g DEBUG_UART_DUMMY (dependent on DEBUG_UART) which defines the dummy functions. Set it to y by default, but n for the EFI-stub builds. Then you don't need to enable the debug UART here.
Regards, Simoon
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=341504&state=*
Thanks,
- Kever
[1] https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/987462 [2] https://patchwork.ozlabs.org/project/uboot/patch/20240918130155.299353-2-luk...
On 2025/1/5 07:02, Heinrich Schuchardt wrote:
Am 4. Januar 2025 20:29:43 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Sat, 4 Jan 2025 at 05:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 28.11.24 04:47, Kever Yang wrote:
The efi_stub is useing DEBUG_UART interface by default, Enable it.
As Simon already wrote in a code comment the implementation of the EFI stub is broken as it is not hardware agnostic.
If I said that I was wrong, sorry. Actually that is the stub's intent, to take over the machine, exit boot-services and carry on. It is the EFI app which is designed to use boot services.
So the stub cannot be hardware-agnostic, unfortunately. It only runs on x86 hardware, unless, for example, EFI has something like serial_getinfo() or we disable the UART / keyboard input.
I would assume that there is no serial console that the user has access to on many x86 systems (like on my laptop).
In the stub itself we can use the Simple Text Output Protocol and the Simple Text Input Protocol offered by the UEFI implementation that started the stub.
The UEFI specification defines a Serial Io Protocol which may be implemented but I would not know why we should restrict the usability in this way.
Once we start U-Boot of course we will use hardware specific IO, e.g. on a RISC-V system use the debug console offered by the SBI
Best regards
Heinrich
In the EFI stub we should never directly access hardware. Please, use SimpleTextOutputProtocol.OutputString() for printing.
Once the stub starts, boot services are gone. See efi_main() for how this works. There is a definite point at which we stop using that and start using U-Boot's own thing.
At this point I think I may be misunderstanding what you said...
Signed-off-by: Kever Yang kever.yang@rock-chips.com
configs/efi-x86_payload32_defconfig | 1 + configs/efi-x86_payload64_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/efi-x86_payload32_defconfig b/configs/efi-x86_payload32_defconfig index 071ddb8e36d..f0b9acc358d 100644 --- a/configs/efi-x86_payload32_defconfig +++ b/configs/efi-x86_payload32_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y
On x86 this uses the NS16550 UART driver. This is fine as long as we don't use the debug UART in the EFI stub. But correcting this is for a separate patch.
Having a debug UART available after the EFI stub makes sense.
I will update the commit message when merging.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_FIT=y diff --git a/configs/efi-x86_payload64_defconfig b/configs/efi-x86_payload64_defconfig index 71612d7fb19..b02a861e59c 100644 --- a/configs/efi-x86_payload64_defconfig +++ b/configs/efi-x86_payload64_defconfig @@ -5,6 +5,7 @@ CONFIG_DEFAULT_DEVICE_TREE="efi-x86_payload" CONFIG_PRE_CON_BUF_ADDR=0x100000 CONFIG_VENDOR_EFI=y CONFIG_TARGET_EFI_PAYLOAD=y +CONFIG_DEBUG_UART=y CONFIG_EFI=y CONFIG_EFI_STUB=y CONFIG_EFI_STUB_64BIT=y
Regards, Simon
On Mon, Jan 06, 2025 at 02:46:53PM +0800, Kever Yang wrote:
Hi Heinrich and Simon,
I send these two change because I want to fix the build error below(from[1]) which is for disable the DEBUG_UART[2]. Do you have clear suggestion on how to do this in efi_stub, since I have no idea about this module, so I just follow how it works in other serial driver.
Can you please remind me what got us going in this direction to start with? Is it related to (in the rockchip tree): commit 968c5682c14fe26bcabccf925157b5afc4427ad0 Author: Lukasz Czechowski lukasz.czechowski@thaumatec.com Date: Wed Sep 18 15:01:53 2024 +0200
ram: rockchip: Fix dependency of RAM_ROCKCHIP_DEBUG
And in turn, why is that even "default y" ? Having something that required DEBUG_UART be enabled sounds like we're a bit off in the wrong direction. Thanks.
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 fordisable 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-luk...
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) {
participants (4)
-
Heinrich Schuchardt -
Kever Yang -
Simon Glass -
Tom Rini