[PATCH v3 0/4] Rockchip: Allow to silent TPL/SPL debug console

[PATCH v3 0/4] Rockchip: Allow to silent TPL/SPL debug console

[Lukasz Czechowski]

Currently it is not possible to completely silent the debug console
in i.e. PX30, what might be required in some embedded devices.
Disabling DEBUG_UART results in errors, due to missing symbols.
In particular, rockchip sdram driver performs calls to debug_uart
functions (i.e. printascii), defined directly in serial driver.
This patch series fixes the dependencies in Kconfig and provides
dummy implementation of debug_uart functions that are linked in case
serial driver is not used.

changes in v2:
 - Update commit titles
 - Create separate SPL and TPL symbols for RAM_ROCKCHIP_DEBUG

changes in v3:
 - Dropped [PATCH v2 3/5] ram: rockchip: Add separate RAM_ROCKCHIP_DEBUG config for TPL/SPL
   As suggested by Kever, dependency on DEBUG_UART is sufficient and this patch
   was not needed.

Lukasz Czechowski (4):
  debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART
    is not set
  ram: rockchip: Fix dependency of RAM_ROCKCHIP_DEBUG
  rockchip: px30: Weaken dependency TPL/SPL serial
  rockchip: px30: Fix hard dependency to DEBUG_UART_BOARD_INIT

 arch/arm/mach-rockchip/Kconfig |  5 ++---
 drivers/ram/rockchip/Kconfig   |  1 +
 include/debug_uart.h           | 16 ++++++++++++++++
 3 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Lukasz Czechowski]

In case DEBUG UART is not used, define dummy macros replacing
the actual function implementations that will not be available.
This allows to compile code and avoid linker errors.
Because the DEBUG_UART_FUNCS macro should not be used if
DEBUG UART is not available, redefine it to generate compilation
warning.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
---
 include/debug_uart.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/debug_uart.h b/include/debug_uart.h
index 714b369e6fe..dc0f1aa4c98 100644
--- a/include/debug_uart.h
+++ b/include/debug_uart.h
@@ -128,6 +128,8 @@ void printdec(unsigned int value);
 			(1 << CONFIG_DEBUG_UART_SHIFT), \
 		CONFIG_DEBUG_UART_SHIFT)
 
+#ifdef CONFIG_DEBUG_UART
+
 /*
  * Now define some functions - this should be inserted into the serial driver
  */
@@ -197,4 +199,18 @@ void printdec(unsigned int value);
 		_DEBUG_UART_ANNOUNCE \
 	} \
 
+#else
+
+#define DEBUG_UART_FUNCS \
+	#warning "DEBUG_UART not defined!"
+
+#define printch(ch) do{}while(0);
+#define printascii(str) do{}while(0);
+#define printhex2(value) do{}while(0);
+#define printhex4(value) do{}while(0);
+#define printhex8(value) do{}while(0);
+#define printdec(value) do{}while(0);
+
+#endif
+
 #endif
-- 
2.43.0

[PATCH v3 2/4] ram: rockchip: Fix dependency of RAM_ROCKCHIP_DEBUG

[Lukasz Czechowski]

The RAM_ROCKCHIP_DEBUG can be used only if DEBUG_UART is
available, otherwise it won't have any effect.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
---
 drivers/ram/rockchip/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
index 67c63ecba04..d707d09c1c8 100644
--- a/drivers/ram/rockchip/Kconfig
+++ b/drivers/ram/rockchip/Kconfig
@@ -15,6 +15,7 @@ if RAM_ROCKCHIP
 
 config RAM_ROCKCHIP_DEBUG
 	bool "Rockchip ram drivers debugging"
+	depends on DEBUG_UART
 	default y
 	help
 	  This enables debugging ram driver API's for the platforms
-- 
2.43.0

[PATCH v3 3/4] rockchip: px30: Weaken dependency TPL/SPL serial

[Lukasz Czechowski]

Allow to disable serial console in TPL and SPL. Weak dependency
to SPL_SERIAL and TPL_SERIAL is also used in other Rockchip boards.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
---
 arch/arm/mach-rockchip/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index fc1b638ff01..47a6275d45f 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -11,8 +11,8 @@ config ROCKCHIP_PX30
 	select TPL_TINY_FRAMEWORK if TPL
 	select TPL_NEEDS_SEPARATE_STACK if TPL
 	imply SPL_SEPARATE_BSS
-	select SPL_SERIAL
-	select TPL_SERIAL
+	imply SPL_SERIAL
+	imply TPL_SERIAL
 	select DEBUG_UART_BOARD_INIT
 	imply ROCKCHIP_COMMON_BOARD
 	imply SPL_ROCKCHIP_COMMON_BOARD
-- 
2.43.0

[PATCH v3 4/4] rockchip: px30: Fix hard dependency to DEBUG_UART_BOARD_INIT

[Lukasz Czechowski]

Because DEBUG_UART_BOARD_INIT depends on DEBUG_UART, hard dependency
to DEBUG_UART_BOARD_INIT in ROCKCHIP_PX30 can cause warnings if
DEBUG_UART is disabled.
The DEBUG_UART_BOARD_INIT is already implied by ARCH_ROCKCHIP entry.
Remove hard dependency from ROCKCHIP_PX30, so that it will be
consistent with other rockchip boards.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
---
 arch/arm/mach-rockchip/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 47a6275d45f..c9aaf6d3476 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -13,7 +13,6 @@ config ROCKCHIP_PX30
 	imply SPL_SEPARATE_BSS
 	imply SPL_SERIAL
 	imply TPL_SERIAL
-	select DEBUG_UART_BOARD_INIT
 	imply ROCKCHIP_COMMON_BOARD
 	imply SPL_ROCKCHIP_COMMON_BOARD
 	imply ARMV8_CRYPTO
-- 
2.43.0

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Kever Yang]

Hi Lukasz,

     I think this will make the error happen like this:

+common/console.c: In function 'puts':
+common/console.c:746:29: error: unused variable 'ch' 
[-Werror=unused-variable]
+  746 |                         int ch = *s++;
+      |                             ^~
+cc1: all warnings being treated as errors
+make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1


The main reason is that below patch removes "#ifdef":

c04f856822a console: remove #ifdef CONFIG when it is possible


Thanks,

- Kever

On 2024/9/18 21:01, Lukasz Czechowski wrote:
> In case DEBUG UART is not used, define dummy macros replacing
> the actual function implementations that will not be available.
> This allows to compile code and avoid linker errors.
> Because the DEBUG_UART_FUNCS macro should not be used if
> DEBUG UART is not available, redefine it to generate compilation
> warning.
>
> Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>   include/debug_uart.h | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/include/debug_uart.h b/include/debug_uart.h
> index 714b369e6fe..dc0f1aa4c98 100644
> --- a/include/debug_uart.h
> +++ b/include/debug_uart.h
> @@ -128,6 +128,8 @@ void printdec(unsigned int value);
>   			(1 << CONFIG_DEBUG_UART_SHIFT), \
>   		CONFIG_DEBUG_UART_SHIFT)
>   
> +#ifdef CONFIG_DEBUG_UART
> +
>   /*
>    * Now define some functions - this should be inserted into the serial driver
>    */
> @@ -197,4 +199,18 @@ void printdec(unsigned int value);
>   		_DEBUG_UART_ANNOUNCE \
>   	} \
>   
> +#else
> +
> +#define DEBUG_UART_FUNCS \
> +	#warning "DEBUG_UART not defined!"
> +
> +#define printch(ch) do{}while(0);
> +#define printascii(str) do{}while(0);
> +#define printhex2(value) do{}while(0);
> +#define printhex4(value) do{}while(0);
> +#define printhex8(value) do{}while(0);
> +#define printdec(value) do{}while(0);
> +
> +#endif
> +
>   #endif

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Quentin Schulz]

Hi Kever,

On 9/29/24 3:53 AM, Kever Yang wrote:
> Hi Lukasz,
> 
>      I think this will make the error happen like this:
> 
> +common/console.c: In function 'puts':
> +common/console.c:746:29: error: unused variable 'ch' [-Werror=unused- 
> variable]
> +  746 |                         int ch = *s++;
> +      |                             ^~
> +cc1: all warnings being treated as errors
> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
> 
> 
> The main reason is that below patch removes "#ifdef":
> 
> c04f856822a console: remove #ifdef CONFIG when it is possible
> 

Can you please always share the link to the pipelines that fail so 
people have an idea on how to reproduce it locally?

Here I assume it is: 
https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455

A simple way is to apply the patches, build the pine64-lts for example 
and then you'll see warnings (which aren't failing builds locally I 
believe but in CI, yes).

I think we can fool the compiler with the following:

diff --git a/include/debug_uart.h b/include/debug_uart.h
index dc0f1aa4c98..b19e44d6d0f 100644
--- a/include/debug_uart.h
+++ b/include/debug_uart.h
@@ -204,12 +204,12 @@ void printdec(unsigned int value);
  #define DEBUG_UART_FUNCS \
  	#warning "DEBUG_UART not defined!"

-#define printch(ch) do{}while(0);
-#define printascii(str) do{}while(0);
-#define printhex2(value) do{}while(0);
-#define printhex4(value) do{}while(0);
-#define printhex8(value) do{}while(0);
-#define printdec(value) do{}while(0);
+#define printch(ch) do{ (void)(ch); }while(0);
+#define printascii(str) do{ (void)(str); }while(0);
+#define printhex2(value) do{ (void)(value); }while(0);
+#define printhex4(value) do{ (void)(value); }while(0);
+#define printhex8(value) do{ (void)(value); }while(0);
+#define printdec(value) do{ (void)(value); }while(0);

  #endif

Does this make sense?

Cheers,
Quentin

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Kever Yang]

Hi Tom,

     This is regression of "#ifdef CONFIG", is it possible for us to go 
back to use "#ifdef CONFIG" in this case?

Or do you have any other suggestion for this issue?

On 2024/9/30 16:55, Quentin Schulz wrote:
> Hi Kever,
>
> On 9/29/24 3:53 AM, Kever Yang wrote:
>> Hi Lukasz,
>>
>>      I think this will make the error happen like this:
>>
>> +common/console.c: In function 'puts':
>> +common/console.c:746:29: error: unused variable 'ch' 
>> [-Werror=unused- variable]
>> +  746 |                         int ch = *s++;
>> +      |                             ^~
>> +cc1: all warnings being treated as errors
>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
>>
>>
>> The main reason is that below patch removes "#ifdef":
>>
>> c04f856822a console: remove #ifdef CONFIG when it is possible
>>
>
> Can you please always share the link to the pipelines that fail so 
> people have an idea on how to reproduce it locally?
>
> Here I assume it is: 
> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455
>
> A simple way is to apply the patches, build the pine64-lts for example 
> and then you'll see warnings (which aren't failing builds locally I 
> believe but in CI, yes).
>
> I think we can fool the compiler with the following:
>
> diff --git a/include/debug_uart.h b/include/debug_uart.h
> index dc0f1aa4c98..b19e44d6d0f 100644
> --- a/include/debug_uart.h
> +++ b/include/debug_uart.h
> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>  #define DEBUG_UART_FUNCS \
>      #warning "DEBUG_UART not defined!"
>
> -#define printch(ch) do{}while(0);
> -#define printascii(str) do{}while(0);
> -#define printhex2(value) do{}while(0);
> -#define printhex4(value) do{}while(0);
> -#define printhex8(value) do{}while(0);
> -#define printdec(value) do{}while(0);
> +#define printch(ch) do{ (void)(ch); }while(0);
> +#define printascii(str) do{ (void)(str); }while(0);
> +#define printhex2(value) do{ (void)(value); }while(0);
> +#define printhex4(value) do{ (void)(value); }while(0);
> +#define printhex8(value) do{ (void)(value); }while(0);
> +#define printdec(value) do{ (void)(value); }while(0);
>
>  #endif
>
> Does this make sense?

Hi Quentin,

     Thanks for your information about pipeline and suggestion for code 
change, but this workaround does not looks good :(


Thanks,

- Kever

>
> Cheers,
> Quentin

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Tom Rini]

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

On Fri, Oct 25, 2024 at 08:30:44PM +0800, Kever Yang wrote:

> Hi Tom,
> 
>     This is regression of "#ifdef CONFIG", is it possible for us to go back
> to use "#ifdef CONFIG" in this case?

Yes, that's fine, thanks.

-- 
Tom

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

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Łukasz Czechowski]

Hi,

On 2024/10/25 14:30, Kever Yang wrote:
>
> Hi Tom,
>
>      This is regression of "#ifdef CONFIG", is it possible for us to go
> back to use "#ifdef CONFIG" in this case?
>
> Or do you have any other suggestion for this issue?
>
> On 2024/9/30 16:55, Quentin Schulz wrote:
> > Hi Kever,
> >
> > On 9/29/24 3:53 AM, Kever Yang wrote:
> >> Hi Lukasz,
> >>
> >>      I think this will make the error happen like this:
> >>
> >> +common/console.c: In function 'puts':
> >> +common/console.c:746:29: error: unused variable 'ch'
> >> [-Werror=unused- variable]
> >> +  746 |                         int ch = *s++;
> >> +      |                             ^~
> >> +cc1: all warnings being treated as errors
> >> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
> >>
> >>
> >> The main reason is that below patch removes "#ifdef":
> >>
> >> c04f856822a console: remove #ifdef CONFIG when it is possible
> >>
> >
> > Can you please always share the link to the pipelines that fail so
> > people have an idea on how to reproduce it locally?
> >
> > Here I assume it is:
> > https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455
> >
> > A simple way is to apply the patches, build the pine64-lts for example
> > and then you'll see warnings (which aren't failing builds locally I
> > believe but in CI, yes).
> >
> > I think we can fool the compiler with the following:
> >
> > diff --git a/include/debug_uart.h b/include/debug_uart.h
> > index dc0f1aa4c98..b19e44d6d0f 100644
> > --- a/include/debug_uart.h
> > +++ b/include/debug_uart.h
> > @@ -204,12 +204,12 @@ void printdec(unsigned int value);
> >  #define DEBUG_UART_FUNCS \
> >      #warning "DEBUG_UART not defined!"
> >
> > -#define printch(ch) do{}while(0);
> > -#define printascii(str) do{}while(0);
> > -#define printhex2(value) do{}while(0);
> > -#define printhex4(value) do{}while(0);
> > -#define printhex8(value) do{}while(0);
> > -#define printdec(value) do{}while(0);
> > +#define printch(ch) do{ (void)(ch); }while(0);
> > +#define printascii(str) do{ (void)(str); }while(0);
> > +#define printhex2(value) do{ (void)(value); }while(0);
> > +#define printhex4(value) do{ (void)(value); }while(0);
> > +#define printhex8(value) do{ (void)(value); }while(0);
> > +#define printdec(value) do{ (void)(value); }while(0);
> >
> >  #endif
> >
> > Does this make sense?
>
> Hi Quentin,
>
>      Thanks for your information about pipeline and suggestion for code
> change, but this workaround does not looks good :(
>

Thanks for the suggestions. I think this code can be even simplified
to just i.e.:

@@ -204,12 +204,12 @@ void printdec(unsigned int value);
 #define DEBUG_UART_FUNCS \
        #warning "DEBUG_UART not defined!"

-#define printch(ch) do{}while(0);
-#define printascii(str) do{}while(0);
-#define printhex2(value) do{}while(0);
-#define printhex4(value) do{}while(0);
-#define printhex8(value) do{}while(0);
-#define printdec(value) do{}while(0);
+#define printch(ch) (void)ch;
+#define printascii(str) (void)str;
+#define printhex2(value) (void)value;
+#define printhex4(value) (void)value;
+#define printhex8(value) (void)value;
+#define printdec(value) (void)value;

 #endif

That will allow us to get rid of warnings. What do you think? I can't
think of another method besides creating a lot of #ifdefs in every
place debug functions are used, which will look even worse than the
dummy macros.


>
> Thanks,
>
> - Kever
>
> >
> > Cheers,
> > Quentin

Best regards,
Lukasz

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Quentin Schulz]

Hi Lukasz,

On 10/25/24 4:27 PM, Łukasz Czechowski wrote:
> Hi,
> 
> On 2024/10/25 14:30, Kever Yang wrote:
>>
>> Hi Tom,
>>
>>       This is regression of "#ifdef CONFIG", is it possible for us to go
>> back to use "#ifdef CONFIG" in this case?
>>
>> Or do you have any other suggestion for this issue?
>>
>> On 2024/9/30 16:55, Quentin Schulz wrote:
>>> Hi Kever,
>>>
>>> On 9/29/24 3:53 AM, Kever Yang wrote:
>>>> Hi Lukasz,
>>>>
>>>>       I think this will make the error happen like this:
>>>>
>>>> +common/console.c: In function 'puts':
>>>> +common/console.c:746:29: error: unused variable 'ch'
>>>> [-Werror=unused- variable]
>>>> +  746 |                         int ch = *s++;
>>>> +      |                             ^~
>>>> +cc1: all warnings being treated as errors
>>>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
>>>>
>>>>
>>>> The main reason is that below patch removes "#ifdef":
>>>>
>>>> c04f856822a console: remove #ifdef CONFIG when it is possible
>>>>
>>>
>>> Can you please always share the link to the pipelines that fail so
>>> people have an idea on how to reproduce it locally?
>>>
>>> Here I assume it is:
>>> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455
>>>
>>> A simple way is to apply the patches, build the pine64-lts for example
>>> and then you'll see warnings (which aren't failing builds locally I
>>> believe but in CI, yes).
>>>
>>> I think we can fool the compiler with the following:
>>>
>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>>> index dc0f1aa4c98..b19e44d6d0f 100644
>>> --- a/include/debug_uart.h
>>> +++ b/include/debug_uart.h
>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>>>   #define DEBUG_UART_FUNCS \
>>>       #warning "DEBUG_UART not defined!"
>>>
>>> -#define printch(ch) do{}while(0);
>>> -#define printascii(str) do{}while(0);
>>> -#define printhex2(value) do{}while(0);
>>> -#define printhex4(value) do{}while(0);
>>> -#define printhex8(value) do{}while(0);
>>> -#define printdec(value) do{}while(0);
>>> +#define printch(ch) do{ (void)(ch); }while(0);
>>> +#define printascii(str) do{ (void)(str); }while(0);
>>> +#define printhex2(value) do{ (void)(value); }while(0);
>>> +#define printhex4(value) do{ (void)(value); }while(0);
>>> +#define printhex8(value) do{ (void)(value); }while(0);
>>> +#define printdec(value) do{ (void)(value); }while(0);
>>>
>>>   #endif
>>>
>>> Does this make sense?
>>
>> Hi Quentin,
>>
>>       Thanks for your information about pipeline and suggestion for code
>> change, but this workaround does not looks good :(
>>
> 
> Thanks for the suggestions. I think this code can be even simplified
> to just i.e.:
> 
> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>   #define DEBUG_UART_FUNCS \
>          #warning "DEBUG_UART not defined!"
> 
> -#define printch(ch) do{}while(0);
> -#define printascii(str) do{}while(0);
> -#define printhex2(value) do{}while(0);
> -#define printhex4(value) do{}while(0);
> -#define printhex8(value) do{}while(0);
> -#define printdec(value) do{}while(0);
> +#define printch(ch) (void)ch;
> +#define printascii(str) (void)str;
> +#define printhex2(value) (void)value;
> +#define printhex4(value) (void)value;
> +#define printhex8(value) (void)value;
> +#define printdec(value) (void)value;
> 
>   #endif
> 
> That will allow us to get rid of warnings. What do you think? I can't
> think of another method besides creating a lot of #ifdefs in every
> place debug functions are used, which will look even worse than the
> dummy macros.
> 

You need to surround value/str/ch with parentheses though.

And we should remove the trailing semi-colon too as we cannot guarantee 
it won't be used in contexts in which the semi-colon is not allowed.

But I guess that could work? I'm not too verse in C macros so maybe I'm 
missing some corner-cases.

I think Kever is suggesting we revert the commit he linked earlier so 
that the functions used by the macros modified in this commit are always 
defined, just empty. Is this something you could test? The downside to 
this is that we would have a lot of unnecessary dead-code with loops 
calling empty functions instead of just not calling functions at all.

Cheers,
Quentin

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Kever Yang]

Hi Lukasz, Quentin,

On 2024/10/25 22:56, Quentin Schulz wrote:
> Hi Lukasz,
>
> On 10/25/24 4:27 PM, Łukasz Czechowski wrote:
>> Hi,
>>
>> On 2024/10/25 14:30, Kever Yang wrote:
>>>
>>> Hi Tom,
>>>
>>>       This is regression of "#ifdef CONFIG", is it possible for us 
>>> to go
>>> back to use "#ifdef CONFIG" in this case?
>>>
>>> Or do you have any other suggestion for this issue?
>>>
>>> On 2024/9/30 16:55, Quentin Schulz wrote:
>>>> Hi Kever,
>>>>
>>>> On 9/29/24 3:53 AM, Kever Yang wrote:
>>>>> Hi Lukasz,
>>>>>
>>>>>       I think this will make the error happen like this:
>>>>>
>>>>> +common/console.c: In function 'puts':
>>>>> +common/console.c:746:29: error: unused variable 'ch'
>>>>> [-Werror=unused- variable]
>>>>> +  746 |                         int ch = *s++;
>>>>> +      |                             ^~
>>>>> +cc1: all warnings being treated as errors
>>>>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
>>>>>
>>>>>
>>>>> The main reason is that below patch removes "#ifdef":
>>>>>
>>>>> c04f856822a console: remove #ifdef CONFIG when it is possible
>>>>>
>>>>
>>>> Can you please always share the link to the pipelines that fail so
>>>> people have an idea on how to reproduce it locally?
>>>>
>>>> Here I assume it is:
>>>> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455 
>>>>
>>>>
>>>> A simple way is to apply the patches, build the pine64-lts for example
>>>> and then you'll see warnings (which aren't failing builds locally I
>>>> believe but in CI, yes).
>>>>
>>>> I think we can fool the compiler with the following:
>>>>
>>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>>>> index dc0f1aa4c98..b19e44d6d0f 100644
>>>> --- a/include/debug_uart.h
>>>> +++ b/include/debug_uart.h
>>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>>>>   #define DEBUG_UART_FUNCS \
>>>>       #warning "DEBUG_UART not defined!"
>>>>
>>>> -#define printch(ch) do{}while(0);
>>>> -#define printascii(str) do{}while(0);
>>>> -#define printhex2(value) do{}while(0);
>>>> -#define printhex4(value) do{}while(0);
>>>> -#define printhex8(value) do{}while(0);
>>>> -#define printdec(value) do{}while(0);
>>>> +#define printch(ch) do{ (void)(ch); }while(0);
>>>> +#define printascii(str) do{ (void)(str); }while(0);
>>>> +#define printhex2(value) do{ (void)(value); }while(0);
>>>> +#define printhex4(value) do{ (void)(value); }while(0);
>>>> +#define printhex8(value) do{ (void)(value); }while(0);
>>>> +#define printdec(value) do{ (void)(value); }while(0);
>>>>
>>>>   #endif
>>>>
>>>> Does this make sense?
>>>
>>> Hi Quentin,
>>>
>>>       Thanks for your information about pipeline and suggestion for 
>>> code
>>> change, but this workaround does not looks good :(
>>>
>>
>> Thanks for the suggestions. I think this code can be even simplified
>> to just i.e.:
>>
>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>>   #define DEBUG_UART_FUNCS \
>>          #warning "DEBUG_UART not defined!"
>>
>> -#define printch(ch) do{}while(0);
>> -#define printascii(str) do{}while(0);
>> -#define printhex2(value) do{}while(0);
>> -#define printhex4(value) do{}while(0);
>> -#define printhex8(value) do{}while(0);
>> -#define printdec(value) do{}while(0);
>> +#define printch(ch) (void)ch;
>> +#define printascii(str) (void)str;
>> +#define printhex2(value) (void)value;
>> +#define printhex4(value) (void)value;
>> +#define printhex8(value) (void)value;
>> +#define printdec(value) (void)value;
>>
>>   #endif
>>
>> That will allow us to get rid of warnings. What do you think? I can't
>> think of another method besides creating a lot of #ifdefs in every
>> place debug functions are used, which will look even worse than the
>> dummy macros.
>>
>
> You need to surround value/str/ch with parentheses though.
>
> And we should remove the trailing semi-colon too as we cannot 
> guarantee it won't be used in contexts in which the semi-colon is not 
> allowed.
>
> But I guess that could work? I'm not too verse in C macros so maybe 
> I'm missing some corner-cases.
>
> I think Kever is suggesting we revert the commit he linked earlier so 
> that the functions used by the macros modified in this commit are 
> always defined, just empty. 

At first, what I think first is:

We go back to use #ifdef in the C code, and then we can apply this patch 
directly with below change:

+++ b/common/console.c
@@ -744,7 +744,8 @@ void puts(const char *s)
                 return;
         }

-       if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & 
GD_FLG_SERIAL_READY)) {
+#ifdef CONFIG_DEBUG_UART
+       if (!(gd->flags & GD_FLG_SERIAL_READY)) {
                 while (*s) {
                         int ch = *s++;

@@ -752,6 +753,7 @@ void puts(const char *s)
                 }
                 return;
         }
+#endif


Since we need to change code in function puts, then below change looks 
better, :

+++ b/common/console.c
@@ -745,11 +745,7 @@ void puts(const char *s)
         }

         if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & 
GD_FLG_SERIAL_READY)) {
-               while (*s) {
-                       int ch = *s++;
-
-                       printch(ch);
-               }
+               printascii(s);
                 return;
         }

But I don't know why use 'printch' at the beginning, maybe try to 
convert "(ch == '\n')" to '\r' which later move into  "_printch", so we 
can use printascii() now.


Thanks,

- Kever

> Is this something you could test? The downside to this is that we 
> would have a lot of unnecessary dead-code with loops calling empty 
> functions instead of just not calling functions at all.
>
> Cheers,
> Quentin
>

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Kever Yang]

Hi Lukasz,

On 2024/10/31 17:01, Kever Yang wrote:
> Hi Lukasz, Quentin,
>
> On 2024/10/25 22:56, Quentin Schulz wrote:
>> Hi Lukasz,
>>
>> On 10/25/24 4:27 PM, Łukasz Czechowski wrote:
>>> Hi,
>>>
>>> On 2024/10/25 14:30, Kever Yang wrote:
>>>>
>>>> Hi Tom,
>>>>
>>>>       This is regression of "#ifdef CONFIG", is it possible for us 
>>>> to go
>>>> back to use "#ifdef CONFIG" in this case?
>>>>
>>>> Or do you have any other suggestion for this issue?
>>>>
>>>> On 2024/9/30 16:55, Quentin Schulz wrote:
>>>>> Hi Kever,
>>>>>
>>>>> On 9/29/24 3:53 AM, Kever Yang wrote:
>>>>>> Hi Lukasz,
>>>>>>
>>>>>>       I think this will make the error happen like this:
>>>>>>
>>>>>> +common/console.c: In function 'puts':
>>>>>> +common/console.c:746:29: error: unused variable 'ch'
>>>>>> [-Werror=unused- variable]
>>>>>> +  746 |                         int ch = *s++;
>>>>>> +      |                             ^~
>>>>>> +cc1: all warnings being treated as errors
>>>>>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
>>>>>>
>>>>>>
>>>>>> The main reason is that below patch removes "#ifdef":
>>>>>>
>>>>>> c04f856822a console: remove #ifdef CONFIG when it is possible
>>>>>>
>>>>>
>>>>> Can you please always share the link to the pipelines that fail so
>>>>> people have an idea on how to reproduce it locally?
>>>>>
>>>>> Here I assume it is:
>>>>> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455 
>>>>>
>>>>>
>>>>> A simple way is to apply the patches, build the pine64-lts for 
>>>>> example
>>>>> and then you'll see warnings (which aren't failing builds locally I
>>>>> believe but in CI, yes).
>>>>>
>>>>> I think we can fool the compiler with the following:
>>>>>
>>>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>>>>> index dc0f1aa4c98..b19e44d6d0f 100644
>>>>> --- a/include/debug_uart.h
>>>>> +++ b/include/debug_uart.h
>>>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>>>>>   #define DEBUG_UART_FUNCS \
>>>>>       #warning "DEBUG_UART not defined!"
>>>>>
>>>>> -#define printch(ch) do{}while(0);
>>>>> -#define printascii(str) do{}while(0);
>>>>> -#define printhex2(value) do{}while(0);
>>>>> -#define printhex4(value) do{}while(0);
>>>>> -#define printhex8(value) do{}while(0);
>>>>> -#define printdec(value) do{}while(0);
>>>>> +#define printch(ch) do{ (void)(ch); }while(0);
>>>>> +#define printascii(str) do{ (void)(str); }while(0);
>>>>> +#define printhex2(value) do{ (void)(value); }while(0);
>>>>> +#define printhex4(value) do{ (void)(value); }while(0);
>>>>> +#define printhex8(value) do{ (void)(value); }while(0);
>>>>> +#define printdec(value) do{ (void)(value); }while(0);
>>>>>
>>>>>   #endif
>>>>>
>>>>> Does this make sense?
>>>>
>>>> Hi Quentin,
>>>>
>>>>       Thanks for your information about pipeline and suggestion for 
>>>> code
>>>> change, but this workaround does not looks good :(
>>>>
>>>
>>> Thanks for the suggestions. I think this code can be even simplified
>>> to just i.e.:
>>>
>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
>>>   #define DEBUG_UART_FUNCS \
>>>          #warning "DEBUG_UART not defined!"
>>>
>>> -#define printch(ch) do{}while(0);
>>> -#define printascii(str) do{}while(0);
>>> -#define printhex2(value) do{}while(0);
>>> -#define printhex4(value) do{}while(0);
>>> -#define printhex8(value) do{}while(0);
>>> -#define printdec(value) do{}while(0);
>>> +#define printch(ch) (void)ch;
>>> +#define printascii(str) (void)str;
>>> +#define printhex2(value) (void)value;
>>> +#define printhex4(value) (void)value;
>>> +#define printhex8(value) (void)value;
>>> +#define printdec(value) (void)value;
>>>
>>>   #endif
>>>
>>> That will allow us to get rid of warnings. What do you think? I can't
>>> think of another method besides creating a lot of #ifdefs in every
>>> place debug functions are used, which will look even worse than the
>>> dummy macros.
>>>
>>
>> You need to surround value/str/ch with parentheses though.
>>
>> And we should remove the trailing semi-colon too as we cannot 
>> guarantee it won't be used in contexts in which the semi-colon is not 
>> allowed.
>>
>> But I guess that could work? I'm not too verse in C macros so maybe 
>> I'm missing some corner-cases.
>>
>> I think Kever is suggesting we revert the commit he linked earlier so 
>> that the functions used by the macros modified in this commit are 
>> always defined, just empty. 
>
> At first, what I think first is:
>
> We go back to use #ifdef in the C code, and then we can apply this 
> patch directly with below change:
>
> +++ b/common/console.c
> @@ -744,7 +744,8 @@ void puts(const char *s)
>                 return;
>         }
>
> -       if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & 
> GD_FLG_SERIAL_READY)) {
> +#ifdef CONFIG_DEBUG_UART
> +       if (!(gd->flags & GD_FLG_SERIAL_READY)) {
>                 while (*s) {
>                         int ch = *s++;
>
> @@ -752,6 +753,7 @@ void puts(const char *s)
>                 }
>                 return;
>         }
> +#endif
>
>
> Since we need to change code in function puts, then below change looks 
> better, :
>
> +++ b/common/console.c
> @@ -745,11 +745,7 @@ void puts(const char *s)
>         }
>
>         if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & 
> GD_FLG_SERIAL_READY)) {
> -               while (*s) {
> -                       int ch = *s++;
> -
> -                       printch(ch);
> -               }
> +               printascii(s);
>                 return;
>         }
>
> But I don't know why use 'printch' at the beginning, maybe try to 
> convert "(ch == '\n')" to '\r' which later move into  "_printch", so 
> we can use printascii() now.

I have send this change to mailing list[1], if it's accepted, then I 
will apply this patch set directly without change.


Thanks,

- Kever

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20241108163612.1.Ib408a6723ba954c932968419678bd45b0767a470@changeid/

>
>
> Thanks,
>
> - Kever
>
>> Is this something you could test? The downside to this is that we 
>> would have a lot of unnecessary dead-code with loops calling empty 
>> functions instead of just not calling functions at all.
>>
>> Cheers,
>> Quentin
>>
>

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Łukasz Czechowski]

Hi Kever,

On 2024/10/31 10:33 Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Lukasz,
>
> On 2024/10/31 17:01, Kever Yang wrote:
> > Hi Lukasz, Quentin,
> >
> > On 2024/10/25 22:56, Quentin Schulz wrote:
> >> Hi Lukasz,
> >>
> >> On 10/25/24 4:27 PM, Łukasz Czechowski wrote:
> >>> Hi,
> >>>
> >>> On 2024/10/25 14:30, Kever Yang wrote:
> >>>>
> >>>> Hi Tom,
> >>>>
> >>>>       This is regression of "#ifdef CONFIG", is it possible for us
> >>>> to go
> >>>> back to use "#ifdef CONFIG" in this case?
> >>>>
> >>>> Or do you have any other suggestion for this issue?
> >>>>
> >>>> On 2024/9/30 16:55, Quentin Schulz wrote:
> >>>>> Hi Kever,
> >>>>>
> >>>>> On 9/29/24 3:53 AM, Kever Yang wrote:
> >>>>>> Hi Lukasz,
> >>>>>>
> >>>>>>       I think this will make the error happen like this:
> >>>>>>
> >>>>>> +common/console.c: In function 'puts':
> >>>>>> +common/console.c:746:29: error: unused variable 'ch'
> >>>>>> [-Werror=unused- variable]
> >>>>>> +  746 |                         int ch = *s++;
> >>>>>> +      |                             ^~
> >>>>>> +cc1: all warnings being treated as errors
> >>>>>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
> >>>>>>
> >>>>>>
> >>>>>> The main reason is that below patch removes "#ifdef":
> >>>>>>
> >>>>>> c04f856822a console: remove #ifdef CONFIG when it is possible
> >>>>>>
> >>>>>
> >>>>> Can you please always share the link to the pipelines that fail so
> >>>>> people have an idea on how to reproduce it locally?
> >>>>>
> >>>>> Here I assume it is:
> >>>>> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455
> >>>>>
> >>>>>
> >>>>> A simple way is to apply the patches, build the pine64-lts for
> >>>>> example
> >>>>> and then you'll see warnings (which aren't failing builds locally I
> >>>>> believe but in CI, yes).
> >>>>>
> >>>>> I think we can fool the compiler with the following:
> >>>>>
> >>>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
> >>>>> index dc0f1aa4c98..b19e44d6d0f 100644
> >>>>> --- a/include/debug_uart.h
> >>>>> +++ b/include/debug_uart.h
> >>>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
> >>>>>   #define DEBUG_UART_FUNCS \
> >>>>>       #warning "DEBUG_UART not defined!"
> >>>>>
> >>>>> -#define printch(ch) do{}while(0);
> >>>>> -#define printascii(str) do{}while(0);
> >>>>> -#define printhex2(value) do{}while(0);
> >>>>> -#define printhex4(value) do{}while(0);
> >>>>> -#define printhex8(value) do{}while(0);
> >>>>> -#define printdec(value) do{}while(0);
> >>>>> +#define printch(ch) do{ (void)(ch); }while(0);
> >>>>> +#define printascii(str) do{ (void)(str); }while(0);
> >>>>> +#define printhex2(value) do{ (void)(value); }while(0);
> >>>>> +#define printhex4(value) do{ (void)(value); }while(0);
> >>>>> +#define printhex8(value) do{ (void)(value); }while(0);
> >>>>> +#define printdec(value) do{ (void)(value); }while(0);
> >>>>>
> >>>>>   #endif
> >>>>>
> >>>>> Does this make sense?
> >>>>
> >>>> Hi Quentin,
> >>>>
> >>>>       Thanks for your information about pipeline and suggestion for
> >>>> code
> >>>> change, but this workaround does not looks good :(
> >>>>
> >>>
> >>> Thanks for the suggestions. I think this code can be even simplified
> >>> to just i.e.:
> >>>
> >>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
> >>>   #define DEBUG_UART_FUNCS \
> >>>          #warning "DEBUG_UART not defined!"
> >>>
> >>> -#define printch(ch) do{}while(0);
> >>> -#define printascii(str) do{}while(0);
> >>> -#define printhex2(value) do{}while(0);
> >>> -#define printhex4(value) do{}while(0);
> >>> -#define printhex8(value) do{}while(0);
> >>> -#define printdec(value) do{}while(0);
> >>> +#define printch(ch) (void)ch;
> >>> +#define printascii(str) (void)str;
> >>> +#define printhex2(value) (void)value;
> >>> +#define printhex4(value) (void)value;
> >>> +#define printhex8(value) (void)value;
> >>> +#define printdec(value) (void)value;
> >>>
> >>>   #endif
> >>>
> >>> That will allow us to get rid of warnings. What do you think? I can't
> >>> think of another method besides creating a lot of #ifdefs in every
> >>> place debug functions are used, which will look even worse than the
> >>> dummy macros.
> >>>
> >>
> >> You need to surround value/str/ch with parentheses though.
> >>
> >> And we should remove the trailing semi-colon too as we cannot
> >> guarantee it won't be used in contexts in which the semi-colon is not
> >> allowed.
> >>
> >> But I guess that could work? I'm not too verse in C macros so maybe
> >> I'm missing some corner-cases.
> >>
> >> I think Kever is suggesting we revert the commit he linked earlier so
> >> that the functions used by the macros modified in this commit are
> >> always defined, just empty.
> >
> > At first, what I think first is:
> >
> > We go back to use #ifdef in the C code, and then we can apply this
> > patch directly with below change:
> >
> > +++ b/common/console.c
> > @@ -744,7 +744,8 @@ void puts(const char *s)
> >                 return;
> >         }
> >
> > -       if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags &
> > GD_FLG_SERIAL_READY)) {
> > +#ifdef CONFIG_DEBUG_UART
> > +       if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> >                 while (*s) {
> >                         int ch = *s++;
> >
> > @@ -752,6 +753,7 @@ void puts(const char *s)
> >                 }
> >                 return;
> >         }
> > +#endif
> >
> >
> > Since we need to change code in function puts, then below change looks
> > better, :
> >
> > +++ b/common/console.c
> > @@ -745,11 +745,7 @@ void puts(const char *s)
> >         }
> >
> >         if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags &
> > GD_FLG_SERIAL_READY)) {
> > -               while (*s) {
> > -                       int ch = *s++;
> > -
> > -                       printch(ch);
> > -               }
> > +               printascii(s);
> >                 return;
> >         }
> >
> > But I don't know why use 'printch' at the beginning, maybe try to
> > convert "(ch == '\n')" to '\r' which later move into  "_printch", so
> > we can use printascii() now.
>
> I have send this change to mailing list[1], if it's accepted, then I
> will apply this patch set directly without change.
>
>
> Thanks,
>
> - Kever
>
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20241108163612.1.Ib408a6723ba954c932968419678bd45b0767a470@changeid/
>

Thank you, that sounds perfect to me.

Best regards,
Lukasz

> >
> >
> > Thanks,
> >
> > - Kever
> >
> >> Is this something you could test? The downside to this is that we
> >> would have a lot of unnecessary dead-code with loops calling empty
> >> functions instead of just not calling functions at all.
> >>
> >> Cheers,
> >> Quentin
> >>
> >

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Kever Yang]

Hi Lukasz,

     I got a new error base on patch [1], see full log here [2].

+/binman/rom/intel-mrc (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)
+      |             ^~~~~~~~~~~~~~~~
+cc1: all warnings being treated as errors
+make[3]: *** [scripts/Makefile.build:257: lib/efi/efi_stub.o] Error 1


[1]https://patchwork.ozlabs.org/project/uboot/patch/20241108163612.1.Ib408a6723ba954c932968419678bd45b0767a470@changeid/
[2] https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/958639


Thanks,
- Kever
On 2024/9/18 21:01, Lukasz Czechowski wrote:
> In case DEBUG UART is not used, define dummy macros replacing
> the actual function implementations that will not be available.
> This allows to compile code and avoid linker errors.
> Because the DEBUG_UART_FUNCS macro should not be used if
> DEBUG UART is not available, redefine it to generate compilation
> warning.
>
> Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>   include/debug_uart.h | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/include/debug_uart.h b/include/debug_uart.h
> index 714b369e6fe..dc0f1aa4c98 100644
> --- a/include/debug_uart.h
> +++ b/include/debug_uart.h
> @@ -128,6 +128,8 @@ void printdec(unsigned int value);
>   			(1 << CONFIG_DEBUG_UART_SHIFT), \
>   		CONFIG_DEBUG_UART_SHIFT)
>   
> +#ifdef CONFIG_DEBUG_UART
> +
>   /*
>    * Now define some functions - this should be inserted into the serial driver
>    */
> @@ -197,4 +199,18 @@ void printdec(unsigned int value);
>   		_DEBUG_UART_ANNOUNCE \
>   	} \
>   
> +#else
> +
> +#define DEBUG_UART_FUNCS \
> +	#warning "DEBUG_UART not defined!"
> +
> +#define printch(ch) do{}while(0);
> +#define printascii(str) do{}while(0);
> +#define printhex2(value) do{}while(0);
> +#define printhex4(value) do{}while(0);
> +#define printhex8(value) do{}while(0);
> +#define printdec(value) do{}while(0);
> +
> +#endif
> +
>   #endif

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Łukasz Czechowski]

Hi Kever,

On 2024/11/27 03:42 Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Lukasz,
>
>      I got a new error base on patch [1], see full log here [2].

Looking at the file efi_stub.c that is used in the failing
configuration it looks to me
that some functions from debug_uart.h are used here for convenience
(i.e. printhex),
even though the DEBUG_UART is not used. This is contrary to my assumption that
DEBUG_UART_FUNCS macro should not be used if DEBUG UART is not available.
In order to apply the patch then, either the efi_stub code must be
updated to not
use debug functions or the assumption must be relaxed - I'm not yet
sure what shall be
the correct approach.

>
> +/binman/rom/intel-mrc (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)
> +      |             ^~~~~~~~~~~~~~~~
> +cc1: all warnings being treated as errors
> +make[3]: *** [scripts/Makefile.build:257: lib/efi/efi_stub.o] Error 1
>
>
> [1]https://patchwork.ozlabs.org/project/uboot/patch/20241108163612.1.Ib408a6723ba954c932968419678bd45b0767a470@changeid/
> [2] https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/jobs/958639
>
>
> Thanks,
> - Kever
> On 2024/9/18 21:01, Lukasz Czechowski wrote:
> > In case DEBUG UART is not used, define dummy macros replacing
> > the actual function implementations that will not be available.
> > This allows to compile code and avoid linker errors.
> > Because the DEBUG_UART_FUNCS macro should not be used if
> > DEBUG UART is not available, redefine it to generate compilation
> > warning.
> >
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
> > ---
> >   include/debug_uart.h | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/include/debug_uart.h b/include/debug_uart.h
> > index 714b369e6fe..dc0f1aa4c98 100644
> > --- a/include/debug_uart.h
> > +++ b/include/debug_uart.h
> > @@ -128,6 +128,8 @@ void printdec(unsigned int value);
> >                       (1 << CONFIG_DEBUG_UART_SHIFT), \
> >               CONFIG_DEBUG_UART_SHIFT)
> >
> > +#ifdef CONFIG_DEBUG_UART
> > +
> >   /*
> >    * Now define some functions - this should be inserted into the serial driver
> >    */
> > @@ -197,4 +199,18 @@ void printdec(unsigned int value);
> >               _DEBUG_UART_ANNOUNCE \
> >       } \
> >
> > +#else
> > +
> > +#define DEBUG_UART_FUNCS \
> > +     #warning "DEBUG_UART not defined!"
> > +
> > +#define printch(ch) do{}while(0);
> > +#define printascii(str) do{}while(0);
> > +#define printhex2(value) do{}while(0);
> > +#define printhex4(value) do{}while(0);
> > +#define printhex8(value) do{}while(0);
> > +#define printdec(value) do{}while(0);
> > +
> > +#endif
> > +
> >   #endif

Best regards,
Lukasz

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Quentin Schulz]

Hi Łukasz, Kever,

On 11/27/24 11:21 PM, Łukasz Czechowski wrote:
> Hi Kever,
> 
> On 2024/11/27 03:42 Kever Yang <kever.yang@rock-chips.com> wrote:
>>
>> Hi Lukasz,
>>
>>       I got a new error base on patch [1], see full log here [2].
> 
> Looking at the file efi_stub.c that is used in the failing
> configuration it looks to me
> that some functions from debug_uart.h are used here for convenience
> (i.e. printhex),
> even though the DEBUG_UART is not used. This is contrary to my assumption that
> DEBUG_UART_FUNCS macro should not be used if DEBUG UART is not available.
> In order to apply the patch then, either the efi_stub code must be
> updated to not
> use debug functions or the assumption must be relaxed - I'm not yet
> sure what shall be
> the correct approach.
> 

"""
diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
index 40fc29d9adf..5172cd78a7c 100644
--- a/lib/efi/efi_stub.c
+++ b/lib/efi/efi_stub.c
@@ -83,12 +83,14 @@ void puts(const char *str)
  		putc(*str++);
  }

+#ifdef CONFIG_DEBUG_UART
  static void _debug_uart_putc(int ch)
  {
  	putc(ch);
  }

  DEBUG_UART_FUNCS
+#endif

  void *memcpy(void *dest, const void *src, size_t size)
  {
"""

Fixes it. It builds fine with and without CONFIG_DEBUG_UART set. Not 
sure if it's proper but I guess that's fine?

While reading the patch again, I think we made a small oversight.

#define printhex8(value) do{}while(0);

is an issue because ch may actually have side effects.

Take for example an interrupt register where reading is acknowledging, 
if one does:

printhex8(readl(INT_REG))

will read the register when CONFIG_DEBUG_UART is set, but not when it's not.

I am not sure if and how we can handle that so that things are executed 
but only print is not called. Is this something we should care about?

Cheers,
Quentin

Re: [PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Quentin Schulz]

Hi Łukasz,

On 1/17/25 5:18 PM, Quentin Schulz wrote:
> Hi Łukasz, Kever,
> 
> On 11/27/24 11:21 PM, Łukasz Czechowski wrote:
>> Hi Kever,
>>
>> On 2024/11/27 03:42 Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> Hi Lukasz,
>>>
>>>       I got a new error base on patch [1], see full log here [2].
>>
>> Looking at the file efi_stub.c that is used in the failing
>> configuration it looks to me
>> that some functions from debug_uart.h are used here for convenience
>> (i.e. printhex),
>> even though the DEBUG_UART is not used. This is contrary to my 
>> assumption that
>> DEBUG_UART_FUNCS macro should not be used if DEBUG UART is not available.
>> In order to apply the patch then, either the efi_stub code must be
>> updated to not
>> use debug functions or the assumption must be relaxed - I'm not yet
>> sure what shall be
>> the correct approach.
>>
> 
> """
> diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
> index 40fc29d9adf..5172cd78a7c 100644
> --- a/lib/efi/efi_stub.c
> +++ b/lib/efi/efi_stub.c
> @@ -83,12 +83,14 @@ void puts(const char *str)
>           putc(*str++);
>   }
> 
> +#ifdef CONFIG_DEBUG_UART
>   static void _debug_uart_putc(int ch)
>   {
>       putc(ch);
>   }
> 
>   DEBUG_UART_FUNCS
> +#endif
> 
>   void *memcpy(void *dest, const void *src, size_t size)
>   {
> """
> 
> Fixes it. It builds fine with and without CONFIG_DEBUG_UART set. Not 
> sure if it's proper but I guess that's fine?
> 
> While reading the patch again, I think we made a small oversight.
> 
> #define printhex8(value) do{}while(0);
> 
> is an issue because ch may actually have side effects.
> 
> Take for example an interrupt register where reading is acknowledging, 
> if one does:
> 
> printhex8(readl(INT_REG))
> 
> will read the register when CONFIG_DEBUG_UART is set, but not when it's 
> not.
> 
> I am not sure if and how we can handle that so that things are executed 
> but only print is not called. Is this something we should care about?
> 

Can you send a v4 that fixes that? I'm planning on having a release on 
the upcoming v2025.07 for our devices, so maybe you'll want to rebase 
for your client's product too? I'm pretty sure your client will not be 
the only one who doesn't want anything on UART in U-Boot on Rockchip :)

Cheers,
Quentin