Hi Kever, On 2024/10/31 10:33 Kever Yang <[email protected]> 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 > >> > >
