[PATCH v3 0/8] Use logging feature instead of FPGA_DEBUG
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(-)
base-commit: 2d8cf392a77815f062446ef441f1078958dc1b2a
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;
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
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
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
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 47692e3207..20abf881bd 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
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 918f6db506..b001ed8b70 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 */
- 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" @@ -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 */
- 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
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 51b8d31205..760edb6883 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 = %d\n", - __func__, __LINE__, bytecount); + log_debug("done went active early, bytecount = %d\n", + bytecount); break; }
@@ -465,8 +456,8 @@ static int virtex2_ss_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", - __func__, __LINE__, bytecount); + log_debug("done went active early, bytecount = %d\n", + bytecount); break; }
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.
Thanks, Michal
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?
Greets Alex
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
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
participants (2)
-
Alexander Dahl -
Michal Simek