[PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Kever Yang]

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)
 {
-- 
2.25.1

[PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Kever Yang]

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
-- 
2.25.1

Re: [PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Kever Yang]

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)
>   {

Re: [PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Simon Glass]

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

Re: [PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Heinrich Schuchardt]

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)
>   {

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Heinrich Schuchardt]

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

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Heinrich Schuchardt]

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
>

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Simon Glass]

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

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Heinrich Schuchardt]

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

Re: [PATCH 1/2] efi_stub: Sync the debug UART definition like other platform

[Kever Yang]

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)
>>   {
>
>

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Kever Yang]

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-lukasz.czechowski@thaumatec.com/ 

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
>

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Simon Glass]

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 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

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-lukasz.czechowski@thaumatec.com/
>
> 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
> >

Re: [PATCH 2/2] config: efi-x86_payload: Enable DEBUG_UART

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]