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

[Alexander Dahl <ada@thorsis.com>]

Hello Michal,

Am Freitag, 7. Oktober 2022, 08:34:15 CEST schrieb Michal Simek:
> 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.

Turned out there was a different cause for those warnings.  
See v4 of the series which I just sent.

Have a nice weekend
Alex