[PATCH v3 1/4] Improve user feedback in case of FPGA bitstream load failure

[Pieter Van Trappen]

On 04/07/2025 15:34, Tom Rini wrote:
> On Fri, Jul 04, 2025 at 03:21:08PM +0200, Michal Simek wrote:
>>
>>
>> On 7/4/25 14:38, Pieter Van Trappen wrote:
>>> On 02/07/2025 10:51, Michal Simek wrote:
>>>> On 6/26/25 11:35, vtpieter at gmail.com wrote:
>>>>> From: Pieter Van Trappen <pieter.van.trappen at cern.ch>
>>>>>
>>>>> In cmd/fpga.c, change some `debug` calls to `log_err` for important
>>>>> user feedback and use CMD_RET_FAILURE in favor of CMD_RET_USAGE due to
>>>>> its long output which hides the actual, useful return message. Change
>>>>> the remaining `debug` calls to `log_debug`. Remove all 'fpga:' and
>>>>> __func__ strings as `log_*` has this covered.
>>>>>
>>>>> For `do_fpga_loads`, move up the `do_fpga_check_params` call for more
>>>>> consistent command output; use a constant instead of multiple '5' use.
>>>>>
>>>>> In drivers/fpga/zynq*.c, change 'up to' to 'above' which corrects this
>>>>> confusing/wrong statement.
>>>>>
>>>>> Signed-off-by: Pieter Van Trappen <pieter.van.trappen at cern.ch>
>>>>> ---
>>>>>    cmd/fpga.c              | 99 +++++++++++++++++++++--------------------
>>>>>    drivers/fpga/zynqmppl.c |  4 +-
>>>>>    drivers/fpga/zynqpl.c   |  4 +-
>>>>>    3 files changed, 54 insertions(+), 53 deletions(-)
>>>>
>>>> I would be the best if you can create also cover letter.
>>>> But let me reply to this patch but issue is likely related to
>>>> others. I am also not in CC on 3/4.
>>>
>>> Hi Michal, well there's a cover letter (v3 0/4) but it's only been sent
>>> to the mailing list cause for the actual patches I use the
>>> get_maintainer.pl script (only). Probably also the reason why the 3/4
>>> patch, amending the sandbox_defconfig, didn't reach you. I'll make sure
>>> all v4 patches are sent to you.
>>>
>>>> Anyway CI is failing with
>>>> $ tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E -W -e
>>>> -- board ${TEST_PY_BD} ${OVERRIDE}
>>>> Building current source for 1 boards (1 thread, 64 jobs per thread)
>>>>      sandbox:  +   sandbox
>>>> +drivers/fpga/fpga.c:23:13: error: ‘fpga_no_sup’ defined but not
>>>> used [- Werror=unused-function]
>>>> +   23 | static void fpga_no_sup(char *fn, char *msg)
>>>> +      |             ^~~~~~~~~~~
>>>> +cc1: all warnings being treated as errors
>>>> +make[3]: *** [scripts/Makefile.build:249: drivers/fpga/fpga.o] Error 1
>>>> +make[2]: *** [scripts/Makefile.build:388: drivers/fpga] Error 2
>>>> +make[1]: *** [Makefile:1930: drivers] Error 2
>>>>
>>>> which should be fixed.
>>>
>>> OK I can fix this. Where does this CI job run? Also do you know if there
>>> a reason why not all -Werror options are on by default in the Makefile
>>> (KBUILD_CFLAGS) while they are on for the CI?
>>
>> As far as I know we have two pipelines. One in Azure and another via gitlab.
>> I do use gitlab one as the part of source.denx.de.
>>
>> I can't see any platform which has WERROR enabled by default.
>>
>> M
>>
>>   30 config WERROR
>>   31         bool "Compile U-Boot with warnings as errors"
>>   32         help
>>   33           A U-Boot build should not cause any compiler warnings, and this
>>   34           enables the '-Werror' flag to enforce that rule.
>>   35
>>   36           However, if you have a new (or very old) compiler or linker with odd
>>   37           and unusual warnings, or you have some architecture with problems,
>>   38           you may need to disable this config option in order to
>>   39           successfully build U-Boot.
>>
> 
> We've had -Werror being passed in CI, via buildman for basically
> forever, but only recently added the WERROR Kconfig option to make it
> easier for the non-buildman flow to also use it.

Thanks for the explanation, that helps understanding the difference 
between the Makefile and `buildman -E` used with CI, in addition to the 
WERROR help section. To me, and for the sake of other first 
contributors, it would make sense to switch Werror on by default but 
this has a larger impact and would probably break other workflows.

Cheers, Pieter