| Message ID | 20240918130155.299353-2-lukasz.czechowski@thaumatec.com |
|---|---|
| State | New |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Rockchip: Allow to silent TPL/SPL debug console | expand |
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
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
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
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.
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
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
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 >
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 >> >
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 > >> > >
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
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
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
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