[PATCH v4 00/10] Use logging feature instead of FPGA_DEBUG

Alexander Dahl

7 Oct 2022 7 Oct '22

2:19 p.m.

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 1 is just to be able to build two of the 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:

v3 -> v4: - Reordered patches, Kconfig patch comes first now (made it easier to build and test the series step by step) - Added three patches fixing printf compiler warnings first, before changing to the new logging framework (so CI should not fail anymore with -Werror)

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 (10): fpga: Add missing Kconfig symbols for old FPGA drivers fpga: spartan2: Fix printf arguments warning fpga: spartan3: Fix printf arguments warning fpga: virtex2: Fix printf format string warnings fpga: altera: Use logging feature instead of FPGA_DEBUG fpga: cyclon2: Use logging feature instead of FPGA_DEBUG 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(-)

base-commit: 2d8cf392a77815f062446ef441f1078958dc1b2a

-- 2.30.2

Show replies by date

Alexander Dahl

7 Oct 7 Oct

2:19 p.m.

New subject: [PATCH v4 01/10] fpga: Add missing Kconfig symbols for old FPGA drivers

Those drivers could not be built anymore without those options present.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/Kconfig | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index e2fd16e6d2..813d6a836d 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -27,6 +27,12 @@ config FPGA_STRATIX_V help Say Y here to enable the Altera Stratix V FPGA specific driver.

+config FPGA_ACEX1K + bool "Enable Altera ACEX 1K driver" + depends on FPGA_ALTERA + help + Say Y here to enable the Altera ACEX 1K FPGA specific driver. + config FPGA_CYCLON2 bool "Enable Altera FPGA driver for Cyclone II" depends on FPGA_ALTERA @@ -71,6 +77,12 @@ config FPGA_VERSALPL Versal. The bitstream will only be generated as PDI for Versal platform.

+config FPGA_SPARTAN2 + bool "Enable Spartan2 FPGA driver" + depends on FPGA_XILINX + help + Enable Spartan2 FPGA driver. + config FPGA_SPARTAN3 bool "Enable Spartan3 FPGA driver" depends on FPGA_XILINX

-- 2.30.2

Alexander Dahl

2:19 p.m.

New subject: [PATCH v4 02/10] fpga: spartan2: Fix printf arguments warning

That extra comma messes up format arguments. Warning appears if built with FPGA_DEBUG defined:

CC drivers/fpga/spartan2.o /mnt/data/adahl/src/u-boot/drivers/fpga/spartan2.c: In function ‘spartan2_sp_load’: /mnt/data/adahl/src/u-boot/drivers/fpga/spartan2.c:112:11: warning: too many arguments for format [-Wformat-extra-args] PRINTF ("%s: Function Table:\n" ^~~~~~~~~~~~~~~~~~~~~~~ /mnt/data/adahl/src/u-boot/drivers/fpga/spartan2.c:12:37: note: in definition of macro ‘PRINTF’ #define PRINTF(fmt,args...) printf (fmt ,##args) ^~~ CC drivers/fpga/spartan3.o /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c: In function ‘spartan3_sp_load’: /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c:117:11: warning: too many arguments for format [-Wformat-extra-args] PRINTF ("%s: Function Table:\n" ^~~~~~~~~~~~~~~~~~~~~~~ /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c:17:37: note: in definition of macro ‘PRINTF’ #define PRINTF(fmt,args...) printf (fmt ,##args) ^~~

Fixes: e221174377d7 ("Initial revision") Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/spartan2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fpga/spartan2.c b/drivers/fpga/spartan2.c index 47692e3207..3817ad8bb7 100644 --- a/drivers/fpga/spartan2.c +++ b/drivers/fpga/spartan2.c @@ -122,7 +122,7 @@ static int spartan2_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) "read data:\t0x%p\n" "write data:\t0x%p\n" "busy:\t0x%p\n" - "abort:\t0x%p\n", + "abort:\t0x%p\n" "post:\t0x%p\n\n", __FUNCTION__, &fn, fn, fn->pre, fn->pgm, fn->init, fn->err, fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, fn->busy,

-- 2.30.2

Alexander Dahl

2:19 p.m.

New subject: [PATCH v4 03/10] fpga: spartan3: Fix printf arguments warning

The additional comma messes up the arguments. Warning appears if built with FPGA_DEBUG defined:

CC drivers/fpga/spartan3.o /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c: In function ‘spartan3_sp_load’: /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c:118:11: warning: too many arguments for format [-Wformat-extra-args] PRINTF ("%s: Function Table:\n" ^~~~~~~~~~~~~~~~~~~~~~~ /mnt/data/adahl/src/u-boot/drivers/fpga/spartan3.c:18:37: note: in definition of macro ‘PRINTF’ #define PRINTF(fmt,args...) printf (fmt ,##args) ^~~

Fixes: 875c78934ee2 ("Add Xilinx Spartan3 family FPGA support Patch by Kurt Stremerch, 14 February 2005") Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/spartan3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fpga/spartan3.c b/drivers/fpga/spartan3.c index 918f6db506..641216ad5c 100644 --- a/drivers/fpga/spartan3.c +++ b/drivers/fpga/spartan3.c @@ -127,7 +127,7 @@ static int spartan3_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) "read data:\t0x%p\n" "write data:\t0x%p\n" "busy:\t0x%p\n" - "abort:\t0x%p\n", + "abort:\t0x%p\n" "post:\t0x%p\n\n", __FUNCTION__, &fn, fn, fn->pre, fn->pgm, fn->init, fn->err, fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, fn->busy,

-- 2.30.2

Alexander Dahl

2:19 p.m.

New subject: [PATCH v4 04/10] fpga: virtex2: Fix printf format string warnings

Warning appears if built with FPGA_DEBUG defined:

CC drivers/fpga/virtex2.o /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c: In function ‘virtex2_ssm_load’: /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c:333:11: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=] PRINTF("%s:%d:done went active early, bytecount = %d\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __func__, __LINE__, bytecount); ~~~~~~~~~ /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c:25:37: note: in definition of macro ‘PRINTF’ #define PRINTF(fmt, args...) printf(fmt, ##args) ^~~ /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c: In function ‘virtex2_ss_load’: /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c:468:12: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=] PRINTF("%s:%d:done went active early, bytecount = %d\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __func__, __LINE__, bytecount); ~~~~~~~~~ /mnt/data/adahl/src/u-boot/drivers/fpga/virtex2.c:25:37: note: in definition of macro ‘PRINTF’ #define PRINTF(fmt, args...) printf(fmt, ##args) ^~~

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/virtex2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c index 51b8d31205..e769ceebc1 100644 --- a/drivers/fpga/virtex2.c +++ b/drivers/fpga/virtex2.c @@ -330,7 +330,7 @@ static int virtex2_ssm_load(xilinx_desc *desc, const void *buf, size_t bsize) #endif

if ((*fn->done)(cookie) == FPGA_SUCCESS) { - PRINTF("%s:%d:done went active early, bytecount = %d\n", + PRINTF("%s:%d:done went active early, bytecount = %zu\n", __func__, __LINE__, bytecount); break; } @@ -465,7 +465,7 @@ static int virtex2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) #endif

-- 2.30.2

Alexander Dahl

2:19 p.m.

New subject: [PATCH v4 05/10] fpga: altera: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/altera.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/altera.c b/drivers/fpga/altera.c index 10c0475d25..6a4f0cb9bc 100644 --- a/drivers/fpga/altera.c +++ b/drivers/fpga/altera.c @@ -7,6 +7,8 @@ * Rich Ireland, Enterasys Networks, rireland@enterasys.com. */

+#define LOG_CATEGORY UCLASS_FPGA + /* * Altera FPGA support */ @@ -16,9 +18,6 @@ #include <log.h> #include <stratixII.h>

-/* Define FPGA_DEBUG to 1 to get debug printf's */ -#define FPGA_DEBUG 0 - static const struct altera_fpga { enum altera_family family; const char *name; @@ -106,8 +105,7 @@ int altera_load(Altera_desc *desc, const void *buf, size_t bsize) if (!fpga) return FPGA_FAIL;

- debug_cond(FPGA_DEBUG, "%s: Launching the %s Loader...\n", - __func__, fpga->name); + log_debug("Launching the %s Loader...\n", fpga->name); if (fpga->load) return fpga->load(desc, buf, bsize); return 0; @@ -120,8 +118,7 @@ int altera_dump(Altera_desc *desc, const void *buf, size_t bsize) if (!fpga) return FPGA_FAIL;

- debug_cond(FPGA_DEBUG, "%s: Launching the %s Reader...\n", - __func__, fpga->name); + log_debug("Launching the %s Reader...\n", fpga->name); if (fpga->dump) return fpga->dump(desc, buf, bsize); return 0;

-- 2.30.2

Alexander Dahl

2:19 p.m.

New subject: [PATCH v4 06/10] fpga: cyclon2: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/cyclon2.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/fpga/cyclon2.c b/drivers/fpga/cyclon2.c index 3b008facb8..f264ff8c0e 100644 --- a/drivers/fpga/cyclon2.c +++ b/drivers/fpga/cyclon2.c @@ -5,18 +5,14 @@ * Based on ACE1XK.c */

+#define LOG_CATEGORY UCLASS_FPGA + #include <common.h> /* core U-Boot definitions */ +#include <log.h> #include <altera.h> #include <ACEX1K.h> /* ACEX device family */ #include <linux/delay.h>

-/* Define FPGA_DEBUG to get debug printf's */ -#ifdef FPGA_DEBUG -#define PRINTF(fmt, args...) printf(fmt, ##args) -#else -#define PRINTF(fmt, args...) -#endif - /* Note: The assumption is that we cannot possibly run fast enough to * overrun the device (the Slave Parallel mode can free run at 50MHz). * If there is a need to operate slower, define CONFIG_FPGA_DELAY in @@ -42,7 +38,7 @@ int CYC2_load(Altera_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case passive_serial: - PRINTF("%s: Launching Passive Serial Loader\n", __func__); + log_debug("Launching Passive Serial Loader\n"); ret_val = CYC2_ps_load(desc, buf, bsize); break;

@@ -51,8 +47,7 @@ int CYC2_load(Altera_desc *desc, const void *buf, size_t bsize) * done in the write() callback. Use the existing PS load * function for FPP, too. */ - PRINTF("%s: Launching Fast Passive Parallel Loader\n", - __func__); + log_debug("Launching Fast Passive Parallel Loader\n"); ret_val = CYC2_ps_load(desc, buf, bsize); break;

@@ -72,7 +67,7 @@ int CYC2_dump(Altera_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case passive_serial: - PRINTF("%s: Launching Passive Serial Dump\n", __func__); + log_debug("Launching Passive Serial Dump\n"); ret_val = CYC2_ps_dump(desc, buf, bsize); break;

@@ -99,22 +94,21 @@ static int CYC2_ps_load(Altera_desc *desc, const void *buf, size_t bsize) Altera_CYC2_Passive_Serial_fns *fn = desc->iface_fns; int ret = 0;

- PRINTF("%s: start with interface functions @ 0x%p\n", - __func__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

- PRINTF("%s: Function Table:\n" - "ptr:\t0x%p\n" - "struct: 0x%p\n" - "config:\t0x%p\n" - "status:\t0x%p\n" - "write:\t0x%p\n" - "done:\t0x%p\n\n", - __func__, &fn, fn, fn->config, fn->status, - fn->write, fn->done); + log_debug("Function Table:\n" + "ptr:\t0x%p\n" + "struct: 0x%p\n" + "config:\t0x%p\n" + "status:\t0x%p\n" + "write:\t0x%p\n" + "done:\t0x%p\n\n", + &fn, fn, fn->config, fn->status, + fn->write, fn->done); #ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK printf("Loading FPGA Device %d...", cookie); #endif

-- 2.30.2

Alexander Dahl

2:20 p.m.

New subject: [PATCH v4 07/10] fpga: ACEX1K: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/ACEX1K.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/fpga/ACEX1K.c b/drivers/fpga/ACEX1K.c index aca8049c56..a1ff47035b 100644 --- a/drivers/fpga/ACEX1K.c +++ b/drivers/fpga/ACEX1K.c @@ -7,18 +7,14 @@ * Rich Ireland, Enterasys Networks, rireland@enterasys.com. */

+#define LOG_CATEGORY UCLASS_FPGA + #include <common.h> /* core U-Boot definitions */ #include <console.h> +#include <log.h> #include <ACEX1K.h> /* ACEX device family */ #include <linux/delay.h>

-/* Define FPGA_DEBUG to get debug printf's */ -#ifdef FPGA_DEBUG -#define PRINTF(fmt,args...) printf (fmt ,##args) -#else -#define PRINTF(fmt,args...) -#endif - /* Note: The assumption is that we cannot possibly run fast enough to * overrun the device (the Slave Parallel mode can free run at 50MHz). * If there is a need to operate slower, define CONFIG_FPGA_DELAY in @@ -44,7 +40,7 @@ int ACEX1K_load(Altera_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case passive_serial: - PRINTF ("%s: Launching Passive Serial Loader\n", __FUNCTION__); + log_debug("Launching Passive Serial Loader\n"); ret_val = ACEX1K_ps_load (desc, buf, bsize); break;

@@ -64,7 +60,7 @@ int ACEX1K_dump(Altera_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case passive_serial: - PRINTF ("%s: Launching Passive Serial Dump\n", __FUNCTION__); + log_debug("Launching Passive Serial Dump\n"); ret_val = ACEX1K_ps_dump (desc, buf, bsize); break;

@@ -93,8 +89,7 @@ static int ACEX1K_ps_load(Altera_desc *desc, const void *buf, size_t bsize) Altera_ACEX1K_Passive_Serial_fns *fn = desc->iface_fns; int i;

- PRINTF ("%s: start with interface functions @ 0x%p\n", - __FUNCTION__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { size_t bytecount = 0; @@ -102,16 +97,16 @@ static int ACEX1K_ps_load(Altera_desc *desc, const void *buf, size_t bsize) int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

- PRINTF ("%s: Function Table:\n" - "ptr:\t0x%p\n" - "struct: 0x%p\n" - "config:\t0x%p\n" - "status:\t0x%p\n" - "clk:\t0x%p\n" - "data:\t0x%p\n" - "done:\t0x%p\n\n", - __FUNCTION__, &fn, fn, fn->config, fn->status, - fn->clk, fn->data, fn->done); + log_debug("Function Table:\n" + "ptr:\t0x%p\n" + "struct: 0x%p\n" + "config:\t0x%p\n" + "status:\t0x%p\n" + "clk:\t0x%p\n" + "data:\t0x%p\n" + "done:\t0x%p\n\n", + &fn, fn, fn->config, fn->status, + fn->clk, fn->data, fn->done); #ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK printf ("Loading FPGA Device %d...", cookie); #endif

-- 2.30.2

Alexander Dahl

2:20 p.m.

New subject: [PATCH v4 08/10] fpga: spartan2: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/spartan2.c | 80 +++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 43 deletions(-)

diff --git a/drivers/fpga/spartan2.c b/drivers/fpga/spartan2.c index 3817ad8bb7..f72dfdec94 100644 --- a/drivers/fpga/spartan2.c +++ b/drivers/fpga/spartan2.c @@ -4,16 +4,12 @@ * Rich Ireland, Enterasys Networks, rireland@enterasys.com. */

+#define LOG_CATEGORY UCLASS_FPGA + #include <common.h> /* core U-Boot definitions */ +#include <log.h> #include <spartan2.h> /* Spartan-II device family */

-/* Define FPGA_DEBUG to get debug printf's */ -#ifdef FPGA_DEBUG -#define PRINTF(fmt,args...) printf (fmt ,##args) -#else -#define PRINTF(fmt,args...) -#endif - #undef CONFIG_SYS_FPGA_CHECK_BUSY

/* Note: The assumption is that we cannot possibly run fast enough to @@ -46,12 +42,12 @@ static int spartan2_load(xilinx_desc *desc, const void *buf, size_t bsize,

switch (desc->iface) { case slave_serial: - PRINTF ("%s: Launching Slave Serial Load\n", __FUNCTION__); + log_debug("Launching Slave Serial Load\n"); ret_val = spartan2_ss_load(desc, buf, bsize); break;

case slave_parallel: - PRINTF ("%s: Launching Slave Parallel Load\n", __FUNCTION__); + log_debug("Launching Slave Parallel Load\n"); ret_val = spartan2_sp_load(desc, buf, bsize); break;

@@ -69,12 +65,12 @@ static int spartan2_dump(xilinx_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case slave_serial: - PRINTF ("%s: Launching Slave Serial Dump\n", __FUNCTION__); + log_debug("Launching Slave Serial Dump\n"); ret_val = spartan2_ss_dump(desc, buf, bsize); break;

case slave_parallel: - PRINTF ("%s: Launching Slave Parallel Dump\n", __FUNCTION__); + log_debug("Launching Slave Parallel Dump\n"); ret_val = spartan2_sp_dump(desc, buf, bsize); break;

@@ -100,8 +96,7 @@ static int spartan2_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) int ret_val = FPGA_FAIL; /* assume the worst */ xilinx_spartan2_slave_parallel_fns *fn = desc->iface_fns;

- PRINTF ("%s: start with interface functions @ 0x%p\n", - __FUNCTION__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { size_t bytecount = 0; @@ -109,24 +104,24 @@ static int spartan2_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

- PRINTF ("%s: Function Table:\n" - "ptr:\t0x%p\n" - "struct: 0x%p\n" - "pre: 0x%p\n" - "pgm:\t0x%p\n" - "init:\t0x%p\n" - "err:\t0x%p\n" - "clk:\t0x%p\n" - "cs:\t0x%p\n" - "wr:\t0x%p\n" - "read data:\t0x%p\n" - "write data:\t0x%p\n" - "busy:\t0x%p\n" - "abort:\t0x%p\n" - "post:\t0x%p\n\n", - __FUNCTION__, &fn, fn, fn->pre, fn->pgm, fn->init, fn->err, - fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, fn->busy, - fn->abort, fn->post); + log_debug("Function Table:\n" + "ptr:\t0x%p\n" + "struct: 0x%p\n" + "pre: 0x%p\n" + "pgm:\t0x%p\n" + "init:\t0x%p\n" + "err:\t0x%p\n" + "clk:\t0x%p\n" + "cs:\t0x%p\n" + "wr:\t0x%p\n" + "read data:\t0x%p\n" + "write data:\t0x%p\n" + "busy:\t0x%p\n" + "abort:\t0x%p\n" + "post:\t0x%p\n\n", + &fn, fn, fn->pre, fn->pgm, fn->init, fn->err, + fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, fn->busy, + fn->abort, fn->post);

/* * This code is designed to emulate the "Express Style" @@ -302,8 +297,7 @@ static int spartan2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) int i; unsigned char val;

- PRINTF ("%s: start with interface functions @ 0x%p\n", - __FUNCTION__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { size_t bytecount = 0; @@ -311,16 +305,16 @@ static int spartan2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

- PRINTF ("%s: Function Table:\n" - "ptr:\t0x%p\n" - "struct: 0x%p\n" - "pgm:\t0x%p\n" - "init:\t0x%p\n" - "clk:\t0x%p\n" - "wr:\t0x%p\n" - "done:\t0x%p\n\n", - __FUNCTION__, &fn, fn, fn->pgm, fn->init, - fn->clk, fn->wr, fn->done); + log_debug("Function Table:\n" + "ptr:\t0x%p\n" + "struct: 0x%p\n" + "pgm:\t0x%p\n" + "init:\t0x%p\n" + "clk:\t0x%p\n" + "wr:\t0x%p\n" + "done:\t0x%p\n\n", + &fn, fn, fn->pgm, fn->init, + fn->clk, fn->wr, fn->done); #ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK printf ("Loading FPGA Device %d...\n", cookie); #endif

-- 2.30.2

Alexander Dahl

2:20 p.m.

New subject: [PATCH v4 09/10] fpga: spartan3: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/spartan3.c | 80 +++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 43 deletions(-)

diff --git a/drivers/fpga/spartan3.c b/drivers/fpga/spartan3.c index 641216ad5c..b7a063a95f 100644 --- a/drivers/fpga/spartan3.c +++ b/drivers/fpga/spartan3.c @@ -9,16 +9,12 @@ * on spartan2.c (Rich Ireland, rireland@enterasys.com). */

+#define LOG_CATEGORY UCLASS_FPGA + #include <common.h> /* core U-Boot definitions */ +#include <log.h> #include <spartan3.h> /* Spartan-II device family */

-/* Define FPGA_DEBUG to get debug printf's */ -#ifdef FPGA_DEBUG -#define PRINTF(fmt,args...) printf (fmt ,##args) -#else -#define PRINTF(fmt,args...) -#endif - #undef CONFIG_SYS_FPGA_CHECK_BUSY

/* Note: The assumption is that we cannot possibly run fast enough to @@ -51,12 +47,12 @@ static int spartan3_load(xilinx_desc *desc, const void *buf, size_t bsize,

switch (desc->iface) { case slave_serial: - PRINTF ("%s: Launching Slave Serial Load\n", __FUNCTION__); + log_debug("Launching Slave Serial Load\n"); ret_val = spartan3_ss_load(desc, buf, bsize); break;

case slave_parallel: - PRINTF ("%s: Launching Slave Parallel Load\n", __FUNCTION__); + log_debug("Launching Slave Parallel Load\n"); ret_val = spartan3_sp_load(desc, buf, bsize); break;

@@ -74,12 +70,12 @@ static int spartan3_dump(xilinx_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case slave_serial: - PRINTF ("%s: Launching Slave Serial Dump\n", __FUNCTION__); + log_debug("Launching Slave Serial Dump\n"); ret_val = spartan3_ss_dump(desc, buf, bsize); break;

case slave_parallel: - PRINTF ("%s: Launching Slave Parallel Dump\n", __FUNCTION__); + log_debug("Launching Slave Parallel Dump\n"); ret_val = spartan3_sp_dump(desc, buf, bsize); break;

@@ -105,8 +101,7 @@ static int spartan3_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) int ret_val = FPGA_FAIL; /* assume the worst */ xilinx_spartan3_slave_parallel_fns *fn = desc->iface_fns;

- PRINTF ("%s: start with interface functions @ 0x%p\n", - __FUNCTION__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { size_t bytecount = 0; @@ -114,24 +109,24 @@ static int spartan3_sp_load(xilinx_desc *desc, const void *buf, size_t bsize) int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

/* * This code is designed to emulate the "Express Style" @@ -309,8 +304,7 @@ static int spartan3_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) int i; unsigned char val;

- PRINTF ("%s: start with interface functions @ 0x%p\n", - __FUNCTION__, fn); + log_debug("start with interface functions @ 0x%p\n", fn);

if (fn) { size_t bytecount = 0; @@ -318,16 +312,16 @@ static int spartan3_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) int cookie = desc->cookie; /* make a local copy */ unsigned long ts; /* timestamp */

-- 2.30.2

Alexander Dahl

2:20 p.m.

New subject: [PATCH v4 10/10] fpga: virtex2: Use logging feature instead of FPGA_DEBUG

Instead of using DEBUG or LOG_DEBUG the driver still had its own definition for debug output.

Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/virtex2.c | 69 ++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c index e769ceebc1..0d536f0d04 100644 --- a/drivers/fpga/virtex2.c +++ b/drivers/fpga/virtex2.c @@ -12,21 +12,14 @@ * on spartan2.c (Rich Ireland, rireland@enterasys.com). */

+#define LOG_CATEGORY UCLASS_FPGA + #include <common.h> #include <console.h> +#include <log.h> #include <virtex2.h> #include <linux/delay.h>

-#if 0 -#define FPGA_DEBUG -#endif - -#ifdef FPGA_DEBUG -#define PRINTF(fmt, args...) printf(fmt, ##args) -#else -#define PRINTF(fmt, args...) -#endif - /* * If the SelectMap interface can be overrun by the processor, define * CONFIG_SYS_FPGA_CHECK_BUSY and/or CONFIG_FPGA_DELAY in the board @@ -89,12 +82,12 @@ static int virtex2_load(xilinx_desc *desc, const void *buf, size_t bsize,

switch (desc->iface) { case slave_serial: - PRINTF("%s: Launching Slave Serial Load\n", __func__); + log_debug("Launching Slave Serial Load\n"); ret_val = virtex2_ss_load(desc, buf, bsize); break;

case slave_selectmap: - PRINTF("%s: Launching Slave Parallel Load\n", __func__); + log_debug("Launching Slave Parallel Load\n"); ret_val = virtex2_ssm_load(desc, buf, bsize); break;

@@ -111,12 +104,12 @@ static int virtex2_dump(xilinx_desc *desc, const void *buf, size_t bsize)

switch (desc->iface) { case slave_serial: - PRINTF("%s: Launching Slave Serial Dump\n", __func__); + log_debug("Launching Slave Serial Dump\n"); ret_val = virtex2_ss_dump(desc, buf, bsize); break;

case slave_parallel: - PRINTF("%s: Launching Slave Parallel Dump\n", __func__); + log_debug("Launching Slave Parallel Dump\n"); ret_val = virtex2_ssm_dump(desc, buf, bsize); break;

@@ -150,8 +143,7 @@ static int virtex2_slave_pre(xilinx_virtex2_slave_fns *fn, int cookie) { unsigned long ts;

- PRINTF("%s:%d: Start with interface functions @ 0x%p\n", - __func__, __LINE__, fn); + log_debug("Start with interface functions @ 0x%p\n", fn);

if (!fn) { printf("%s:%d: NULL Interface function table!\n", @@ -160,25 +152,24 @@ static int virtex2_slave_pre(xilinx_virtex2_slave_fns *fn, int cookie) }

/* Gotta split this one up (so the stack won't blow??) */ - PRINTF("%s:%d: Function Table:\n" - " base 0x%p\n" - " struct 0x%p\n" - " pre 0x%p\n" - " prog 0x%p\n" - " init 0x%p\n" - " error 0x%p\n", - __func__, __LINE__, - &fn, fn, fn->pre, fn->pgm, fn->init, fn->err); - PRINTF(" clock 0x%p\n" - " cs 0x%p\n" - " write 0x%p\n" - " rdata 0x%p\n" - " wdata 0x%p\n" - " busy 0x%p\n" - " abort 0x%p\n" - " post 0x%p\n\n", - fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, - fn->busy, fn->abort, fn->post); + log_debug("Function Table:\n" + " base 0x%p\n" + " struct 0x%p\n" + " pre 0x%p\n" + " prog 0x%p\n" + " init 0x%p\n" + " error 0x%p\n", + &fn, fn, fn->pre, fn->pgm, fn->init, fn->err); + log_debug(" clock 0x%p\n" + " cs 0x%p\n" + " write 0x%p\n" + " rdata 0x%p\n" + " wdata 0x%p\n" + " busy 0x%p\n" + " abort 0x%p\n" + " post 0x%p\n\n", + fn->clk, fn->cs, fn->wr, fn->rdata, fn->wdata, + fn->busy, fn->abort, fn->post);

#ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK printf("Initializing FPGA Device %d...\n", cookie); @@ -330,8 +321,8 @@ static int virtex2_ssm_load(xilinx_desc *desc, const void *buf, size_t bsize) #endif

if ((*fn->done)(cookie) == FPGA_SUCCESS) { - PRINTF("%s:%d:done went active early, bytecount = %zu\n", - __func__, __LINE__, bytecount); + log_debug("done went active early, bytecount = %zu\n", + bytecount); break; }

@@ -465,8 +456,8 @@ static int virtex2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize) #endif

-- 2.30.2

Michal Simek

11 Oct 11 Oct

9:22 a.m.

On 10/7/22 14:19, 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 1 is just to be able to build two of the 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:

v3 -> v4:

Reordered patches, Kconfig patch comes first now (made it easier to build and test the series step by step)

Added three patches fixing printf compiler warnings first, before changing to the new logging framework (so CI should not fail anymore with -Werror)

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 (10): fpga: Add missing Kconfig symbols for old FPGA drivers fpga: spartan2: Fix printf arguments warning fpga: spartan3: Fix printf arguments warning fpga: virtex2: Fix printf format string warnings fpga: altera: Use logging feature instead of FPGA_DEBUG fpga: cyclon2: Use logging feature instead of FPGA_DEBUG 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(-)

base-commit: 2d8cf392a77815f062446ef441f1078958dc1b2a

Applied. M

941

Age (days ago)

945

Last active (days ago)

List overview

Download

11 comments

2 participants

tags (0)

participants (2)

Alexander Dahl
Michal Simek