Re: [PATCH v3 0/8] Use logging feature instead of FPGA_DEBUG

Michal Simek
2022-10-07T06:34:42Z

Hi,

On 10/6/22 16:56, Alexander Dahl wrote:
> Hello Michal,
>
> Am Donnerstag, 6. Oktober 2022, 16:44:29 CEST schrieb Michal Simek:
>> Hi,
>>
>> On 10/5/22 13:44, Alexander Dahl wrote:
>>> Hei hei,
>>>
>>> while working on FPGA support for a new device I discovered debug
>>> logging in some FPGA drivers is still done as in the old days. Bring
>>> that to what I thougt would be the currently preferred approach.
>>>
>>> Notes: Adding those Kconfig symbols in patch 3 is just to be able to
>>> build those two old drivers.
>>>
>>> All drivers touched were build tested with sandbox_defconfig and GCC 8
>>> on Debian GNU/Linux 10 (buster).
>>>
>>> Lines with other possibly questionable output were not touched, only
>>> what seemed to be designated debug output, and only for FPGA drivers
>>> having that ancient FPGA_DEBUG / PRINTF macros, so there's room for
>>> future improvements.
>>>
>>> Changelog:
>>>
>>> v2 -> v3:
>>> - Patch introducing FPGA uclass was completely reworked, sent
>>>
>>> independently from this series, and applied already, thus removed
>>>
>>> - Because requiring that new FPGA uclass changes, rebased on Michal's
>>>
>>> microblaze branch '20221005'
>>>
>>> - Removed '"%s …", __func__' and '"%d …", __line__' from log messages,
>>>
>>> because log framework can add those (enabled by CONFIG_LOGF_FUNC and
>>> CONFIG_LOGF_LINE)
>>>
>>> v1 -> v2:
>>> - Rebased on master
>>> - Added patch to introduce new FPGA uclass in front of the other patches
>>> - Use that new uclass as log category
>>> - Slightly reworded cover letter
>>>
>>> Greets
>>> Alex
>>>
>>> Cc: Michal Simek <michal.simek@amd.com>
>>>
>>> Alexander Dahl (7):
>>> fpga: altera: Use logging feature instead of FPGA_DEBUG
>>> fpga: cyclon2: Use logging feature instead of FPGA_DEBUG
>>> fpga: Add missing Kconfig symbols for old FPGA drivers
>>> fpga: ACEX1K: Use logging feature instead of FPGA_DEBUG
>>> fpga: spartan2: Use logging feature instead of FPGA_DEBUG
>>> fpga: spartan3: Use logging feature instead of FPGA_DEBUG
>>> fpga: virtex2: Use logging feature instead of FPGA_DEBUG
>>>
>>> drivers/fpga/ACEX1K.c | 37 +++++++++----------
>>> drivers/fpga/Kconfig | 12 +++++++
>>> drivers/fpga/altera.c | 11 +++---
>>> drivers/fpga/cyclon2.c | 38 +++++++++-----------
>>> drivers/fpga/spartan2.c | 80 +++++++++++++++++++----------------------
>>> drivers/fpga/spartan3.c | 80 +++++++++++++++++++----------------------
>>> drivers/fpga/virtex2.c | 69 ++++++++++++++++-------------------
>>> 7 files changed, 152 insertions(+), 175 deletions(-)
>>
>> I pushed it to CI loop and got failure.
>>
>> https://source.denx.de/u-boot/custodians/u-boot-microblaze/-/jobs/508906
>>
>> Building current source for 136 boards (64 threads, 1 job per thread)
>> m68k: + astro_mcf5373l
>> +In file included from include/linux/printk.h:4,
>> + from include/common.h:20,
>> + from drivers/fpga/spartan3.c:14:
>> +drivers/fpga/spartan3.c: In function 'spartan3_sp_load':
>> +drivers/fpga/spartan3.c:112:27: error: too many arguments for format
>> [-Werror=format-extra-args]
>> + 112 | log_debug("Function Table:\n"
>> + | ^~~~~~~~~~~~~~~~~~~
>> +include/log.h:220:24: note: in definition of macro 'log'
>> + 220 | printf(_fmt, ##_args); \
>> + | ^~~~
>> +drivers/fpga/spartan3.c:112:17: note: in expansion of macro 'log_debug'
>> + | ^~~~~~~~~
>> +cc1: all warnings being treated as errors
>> +make[3]: *** [scripts/Makefile.build:258: drivers/fpga/spartan3.o] Error 1
>> +make[2]: *** [scripts/Makefile.build:398: drivers/fpga] Error 2
>> +make[1]: *** [Makefile:1883: drivers] Error 2
>>
>> Please fix it up.
>
> Not sure if those warnings were present before on the old PRINTF calls, but we
> got them now. However the underlying problem was there before: putting to
> much things in one printf/log line. I can go split it up like in 'drivers/
> fpga/virtex2.c' already, where you have the following comment:
>
> /* Gotta split this one up (so the stack won't blow??) */
>
> Not sure however if debug printing all function pointers in those function
> tables has any value at all? Maybe that can just be dropped?

No idea if this is useful or not. But it is there and I would split it as it is
done in virtex2. Please do it in separate patch with mentioning virtex2 to have
the same change.

Thanks,
Michal