[PATCH v2 0/8] Use logging feature instead of FPGA_DEBUG

[PATCH v2 0/8] Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 4 is just to be able to
build those two old drivers.

All drivers touched were build tested with sandbox64_defconfig and GCC8
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:

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

Alexander Dahl (8):
  dm: fpga: Introduce new uclass
  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   | 22 +++++++++-------------
 drivers/fpga/Kconfig    | 12 ++++++++++++
 drivers/fpga/altera.c   | 13 ++++++-------
 drivers/fpga/cyclon2.c  | 24 ++++++++++--------------
 drivers/fpga/spartan2.c | 34 +++++++++++++++-------------------
 drivers/fpga/spartan3.c | 34 +++++++++++++++-------------------
 drivers/fpga/virtex2.c  | 37 +++++++++++++++----------------------
 include/dm/uclass-id.h  |  1 +
 8 files changed, 83 insertions(+), 94 deletions(-)


base-commit: 12ed6d4911ced1df099a365e0a994b54211b60f3
-- 
2.30.2

[PATCH v2 1/8] dm: fpga: Introduce new uclass

[Alexander Dahl]

For future DM based FPGA drivers and for now to have a meaningful
logging class for old FPGA drivers.

Suggested-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 include/dm/uclass-id.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index a432e43871..c2b15881ba 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -56,6 +56,7 @@ enum uclass_id {
 	UCLASS_ETH,		/* Ethernet device */
 	UCLASS_ETH_PHY,		/* Ethernet PHY device */
 	UCLASS_FIRMWARE,	/* Firmware */
+	UCLASS_FPGA,		/* FPGA device */
 	UCLASS_FUZZING_ENGINE,	/* Fuzzing engine */
 	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
 	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */
-- 
2.30.2

[PATCH v2 2/8] fpga: altera: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/altera.c b/drivers/fpga/altera.c
index 10c0475d25..e969d83de7 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,8 @@ 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("%s: Launching the %s Loader...\n",
+		  __func__, fpga->name);
 	if (fpga->load)
 		return fpga->load(desc, buf, bsize);
 	return 0;
@@ -120,8 +119,8 @@ 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("%s: Launching the %s Reader...\n",
+		  __func__, fpga->name);
 	if (fpga->dump)
 		return fpga->dump(desc, buf, bsize);
 	return 0;
-- 
2.30.2

[PATCH v2 3/8] fpga: cyclon2: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/cyclon2.c b/drivers/fpga/cyclon2.c
index 3b008facb8..7dc241b269 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("%s: Launching Passive Serial Loader\n", __func__);
 		ret_val = CYC2_ps_load(desc, buf, bsize);
 		break;
 
@@ -51,8 +47,8 @@ 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("%s: Launching Fast Passive Parallel Loader\n",
+			  __func__);
 		ret_val = CYC2_ps_load(desc, buf, bsize);
 		break;
 
@@ -72,7 +68,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("%s: Launching Passive Serial Dump\n", __func__);
 		ret_val = CYC2_ps_dump(desc, buf, bsize);
 		break;
 
@@ -99,14 +95,14 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		int cookie = desc->cookie;	/* make a local copy */
 		unsigned long ts;		/* timestamp */
 
-		PRINTF("%s: Function Table:\n"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"config:\t0x%p\n"
-- 
2.30.2

[PATCH v2 4/8] fpga: Add missing Kconfig symbols for old FPGA drivers

[Alexander Dahl]

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 e07a9cf80e..4d55f60ba9 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

[PATCH v2 5/8] fpga: ACEX1K: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/fpga/ACEX1K.c b/drivers/fpga/ACEX1K.c
index aca8049c56..3877aedb1d 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("%s: Launching Passive Serial Loader\n", __func__);
 		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("%s: Launching Passive Serial Dump\n", __func__);
 		ret_val = ACEX1K_ps_dump (desc, buf, bsize);
 		break;
 
@@ -93,8 +89,8 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		size_t bytecount = 0;
@@ -102,7 +98,7 @@ 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"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"config:\t0x%p\n"
@@ -110,7 +106,7 @@ static int ACEX1K_ps_load(Altera_desc *desc, const void *buf, size_t bsize)
 				"clk:\t0x%p\n"
 				"data:\t0x%p\n"
 				"done:\t0x%p\n\n",
-				__FUNCTION__, &fn, fn, fn->config, fn->status,
+				__func__, &fn, fn, fn->config, fn->status,
 				fn->clk, fn->data, fn->done);
 #ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK
 		printf ("Loading FPGA Device %d...", cookie);
-- 
2.30.2

[PATCH v2 6/8] fpga: spartan2: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/fpga/spartan2.c b/drivers/fpga/spartan2.c
index 47692e3207..045265d59a 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("%s: Launching Slave Serial Load\n", __func__);
 		ret_val = spartan2_ss_load(desc, buf, bsize);
 		break;
 
 	case slave_parallel:
-		PRINTF ("%s: Launching Slave Parallel Load\n", __FUNCTION__);
+		log_debug("%s: Launching Slave Parallel Load\n", __func__);
 		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("%s: Launching Slave Serial Dump\n", __func__);
 		ret_val = spartan2_ss_dump(desc, buf, bsize);
 		break;
 
 	case slave_parallel:
-		PRINTF ("%s: Launching Slave Parallel Dump\n", __FUNCTION__);
+		log_debug("%s: Launching Slave Parallel Dump\n", __func__);
 		ret_val = spartan2_sp_dump(desc, buf, bsize);
 		break;
 
@@ -100,8 +96,8 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		size_t bytecount = 0;
@@ -109,7 +105,7 @@ 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"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"pre: 0x%p\n"
@@ -124,7 +120,7 @@ static int spartan2_sp_load(xilinx_desc *desc, const void *buf, size_t bsize)
 				"busy:\t0x%p\n"
 				"abort:\t0x%p\n",
 				"post:\t0x%p\n\n",
-				__FUNCTION__, &fn, fn, fn->pre, fn->pgm, fn->init, fn->err,
+				__func__, &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);
 
@@ -302,8 +298,8 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		size_t bytecount = 0;
@@ -311,7 +307,7 @@ 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"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"pgm:\t0x%p\n"
@@ -319,7 +315,7 @@ static int spartan2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize)
 				"clk:\t0x%p\n"
 				"wr:\t0x%p\n"
 				"done:\t0x%p\n\n",
-				__FUNCTION__, &fn, fn, fn->pgm, fn->init,
+				__func__, &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);
-- 
2.30.2

[PATCH v2 7/8] fpga: spartan3: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/fpga/spartan3.c b/drivers/fpga/spartan3.c
index 918f6db506..c88a55a43f 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("%s: Launching Slave Serial Load\n", __func__);
 		ret_val = spartan3_ss_load(desc, buf, bsize);
 		break;
 
 	case slave_parallel:
-		PRINTF ("%s: Launching Slave Parallel Load\n", __FUNCTION__);
+		log_debug("%s: Launching Slave Parallel Load\n", __func__);
 		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("%s: Launching Slave Serial Dump\n", __func__);
 		ret_val = spartan3_ss_dump(desc, buf, bsize);
 		break;
 
 	case slave_parallel:
-		PRINTF ("%s: Launching Slave Parallel Dump\n", __FUNCTION__);
+		log_debug("%s: Launching Slave Parallel Dump\n", __func__);
 		ret_val = spartan3_sp_dump(desc, buf, bsize);
 		break;
 
@@ -105,8 +101,8 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		size_t bytecount = 0;
@@ -114,7 +110,7 @@ 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"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"pre: 0x%p\n"
@@ -129,7 +125,7 @@ static int spartan3_sp_load(xilinx_desc *desc, const void *buf, size_t bsize)
 				"busy:\t0x%p\n"
 				"abort:\t0x%p\n",
 				"post:\t0x%p\n\n",
-				__FUNCTION__, &fn, fn, fn->pre, fn->pgm, fn->init, fn->err,
+				__func__, &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);
 
@@ -309,8 +305,8 @@ 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("%s: start with interface functions @ 0x%p\n",
+		  __func__, fn);
 
 	if (fn) {
 		size_t bytecount = 0;
@@ -318,7 +314,7 @@ 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"
+		log_debug("%s: Function Table:\n"
 				"ptr:\t0x%p\n"
 				"struct: 0x%p\n"
 				"pgm:\t0x%p\n"
@@ -326,7 +322,7 @@ static int spartan3_ss_load(xilinx_desc *desc, const void *buf, size_t bsize)
 				"clk:\t0x%p\n"
 				"wr:\t0x%p\n"
 				"done:\t0x%p\n\n",
-				__FUNCTION__, &fn, fn, fn->pgm, fn->init,
+				__func__, &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);
-- 
2.30.2

[PATCH v2 8/8] fpga: virtex2: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

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 | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c
index 51b8d31205..fddf8ac4ce 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("%s: Launching Slave Serial Load\n", __func__);
 		ret_val = virtex2_ss_load(desc, buf, bsize);
 		break;
 
 	case slave_selectmap:
-		PRINTF("%s: Launching Slave Parallel Load\n", __func__);
+		log_debug("%s: Launching Slave Parallel Load\n", __func__);
 		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("%s: Launching Slave Serial Dump\n", __func__);
 		ret_val = virtex2_ss_dump(desc, buf, bsize);
 		break;
 
 	case slave_parallel:
-		PRINTF("%s: Launching Slave Parallel Dump\n", __func__);
+		log_debug("%s: Launching Slave Parallel Dump\n", __func__);
 		ret_val = virtex2_ssm_dump(desc, buf, bsize);
 		break;
 
@@ -150,8 +143,8 @@ 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("%s:%d: Start with interface functions @ 0x%p\n",
+		  __func__, __LINE__, fn);
 
 	if (!fn) {
 		printf("%s:%d: NULL Interface function table!\n",
@@ -160,7 +153,7 @@ 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"
+	log_debug("%s:%d: Function Table:\n"
 	       "  base   0x%p\n"
 	       "  struct 0x%p\n"
 	       "  pre    0x%p\n"
@@ -169,7 +162,7 @@ static int virtex2_slave_pre(xilinx_virtex2_slave_fns *fn, int cookie)
 	       "  error  0x%p\n",
 	       __func__, __LINE__,
 	       &fn, fn, fn->pre, fn->pgm, fn->init, fn->err);
-	PRINTF("  clock  0x%p\n"
+	log_debug("  clock  0x%p\n"
 	       "  cs     0x%p\n"
 	       "  write  0x%p\n"
 	       "  rdata  0x%p\n"
@@ -330,8 +323,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("%s:%d:done went active early, bytecount = %d\n",
+				  __func__, __LINE__, bytecount);
 			break;
 		}
 
@@ -465,8 +458,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("%s:%d:done went active early, bytecount = %d\n",
+					  __func__, __LINE__, bytecount);
 				break;
 			}
 
-- 
2.30.2

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Michal Simek]

On 9/21/22 15:22, Alexander Dahl wrote:
> For future DM based FPGA drivers and for now to have a meaningful
> logging class for old FPGA drivers.
> 
> Suggested-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>   include/dm/uclass-id.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index a432e43871..c2b15881ba 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -56,6 +56,7 @@ enum uclass_id {
>   	UCLASS_ETH,		/* Ethernet device */
>   	UCLASS_ETH_PHY,		/* Ethernet PHY device */
>   	UCLASS_FIRMWARE,	/* Firmware */
> +	UCLASS_FPGA,		/* FPGA device */
>   	UCLASS_FUZZING_ENGINE,	/* Fuzzing engine */
>   	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
>   	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */

Simon: the whole series look good to me. I am happy to take it via my tree when 
you ACK it. Also no problem if you want to take it via your tree.
Please let me know which way you want to go.

Thanks,
Michal

Re: [PATCH v2 8/8] fpga: virtex2: Use logging feature instead of FPGA_DEBUG

[Michal Simek]

On 9/21/22 15:22, Alexander Dahl wrote:
> 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 | 37 +++++++++++++++----------------------
>   1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c
> index 51b8d31205..fddf8ac4ce 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("%s: Launching Slave Serial Load\n", __func__);
>   		ret_val = virtex2_ss_load(desc, buf, bsize);
>   		break;
>   
>   	case slave_selectmap:
> -		PRINTF("%s: Launching Slave Parallel Load\n", __func__);
> +		log_debug("%s: Launching Slave Parallel Load\n", __func__);
>   		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("%s: Launching Slave Serial Dump\n", __func__);
>   		ret_val = virtex2_ss_dump(desc, buf, bsize);
>   		break;
>   
>   	case slave_parallel:
> -		PRINTF("%s: Launching Slave Parallel Dump\n", __func__);
> +		log_debug("%s: Launching Slave Parallel Dump\n", __func__);
>   		ret_val = virtex2_ssm_dump(desc, buf, bsize);
>   		break;
>   
> @@ -150,8 +143,8 @@ 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("%s:%d: Start with interface functions @ 0x%p\n",
> +		  __func__, __LINE__, fn);
>   
>   	if (!fn) {
>   		printf("%s:%d: NULL Interface function table!\n",
> @@ -160,7 +153,7 @@ 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"
> +	log_debug("%s:%d: Function Table:\n"
>   	       "  base   0x%p\n"

Above you are also aligning next lines which is not what you do here.
The same issue is visible also in other patches. I think it will be good to also 
align it to have proper coding style.

Thanks,
Michal

Re: [PATCH v2 8/8] fpga: virtex2: Use logging feature instead of FPGA_DEBUG

[Alexander Dahl]

Hello Michal,

Am Thu, Sep 22, 2022 at 12:30:08PM +0200 schrieb Michal Simek:
> 
> 
> On 9/21/22 15:22, Alexander Dahl wrote:
> > 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 | 37 +++++++++++++++----------------------
> >   1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c
> > index 51b8d31205..fddf8ac4ce 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("%s: Launching Slave Serial Load\n", __func__);
> >   		ret_val = virtex2_ss_load(desc, buf, bsize);
> >   		break;
> >   	case slave_selectmap:
> > -		PRINTF("%s: Launching Slave Parallel Load\n", __func__);
> > +		log_debug("%s: Launching Slave Parallel Load\n", __func__);
> >   		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("%s: Launching Slave Serial Dump\n", __func__);
> >   		ret_val = virtex2_ss_dump(desc, buf, bsize);
> >   		break;
> >   	case slave_parallel:
> > -		PRINTF("%s: Launching Slave Parallel Dump\n", __func__);
> > +		log_debug("%s: Launching Slave Parallel Dump\n", __func__);
> >   		ret_val = virtex2_ssm_dump(desc, buf, bsize);
> >   		break;
> > @@ -150,8 +143,8 @@ 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("%s:%d: Start with interface functions @ 0x%p\n",
> > +		  __func__, __LINE__, fn);
> >   	if (!fn) {
> >   		printf("%s:%d: NULL Interface function table!\n",
> > @@ -160,7 +153,7 @@ 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"
> > +	log_debug("%s:%d: Function Table:\n"
> >   	       "  base   0x%p\n"
> 
> Above you are also aligning next lines which is not what you do here.
> The same issue is visible also in other patches. I think it will be good to
> also align it to have proper coding style.

I think I can explain that.  I just fixed checkpatch warnings before
sending, and I got those only for lines changed.  I did only change
next lines when checkpatch complained about __FUNCTION__, which is not
stricly what I intended to change here, but did it along the way.
You're right, it makes sense to align all of them.

Will send a v3.

Greets
Alex

> 
> Thanks,
> Michal

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Simon Glass]

Hi,

On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 9/21/22 15:22, Alexander Dahl wrote:
> > For future DM based FPGA drivers and for now to have a meaningful
> > logging class for old FPGA drivers.
> >
> > Suggested-by: Michal Simek <michal.simek@amd.com>
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >   include/dm/uclass-id.h | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index a432e43871..c2b15881ba 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -56,6 +56,7 @@ enum uclass_id {
> >       UCLASS_ETH,             /* Ethernet device */
> >       UCLASS_ETH_PHY,         /* Ethernet PHY device */
> >       UCLASS_FIRMWARE,        /* Firmware */
> > +     UCLASS_FPGA,            /* FPGA device */
> >       UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> >       UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> >       UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
>
> Simon: the whole series look good to me. I am happy to take it via my tree when
> you ACK it. Also no problem if you want to take it via your tree.
> Please let me know which way you want to go.

This is a good step forward but needs a lot more work.

Please add a uclass file for the FPGA - i.e.
drivers/fpga/fpga-uclass.c - see other such files for examples.

The FPGA uclass should have methods that match the non-DM interface.
You will likely need a DM_FPGA config to allow enabling the uclass.

Also this needs a simple sandbox driver/emulator pair, so that it can
be tested, with tests in test/dm/fpga.c that use the driver.

Admittedly this should have been done ages ago. I vaguely remember
mentioning it at the time, but perhaps I missed it. In any case, all
uclasses must have an API, implementation and tests that run in CI
with sandbox. Testing is a vital part of U-Boot and lack of testing is
the main reason why we went back to the 3-month release cycle.

Regards,
Simon

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Michal Simek]

On 9/22/22 13:35, Simon Glass wrote:
> Hi,
> 
> On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 9/21/22 15:22, Alexander Dahl wrote:
>>> For future DM based FPGA drivers and for now to have a meaningful
>>> logging class for old FPGA drivers.
>>>
>>> Suggested-by: Michal Simek <michal.simek@amd.com>
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>    include/dm/uclass-id.h | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>> index a432e43871..c2b15881ba 100644
>>> --- a/include/dm/uclass-id.h
>>> +++ b/include/dm/uclass-id.h
>>> @@ -56,6 +56,7 @@ enum uclass_id {
>>>        UCLASS_ETH,             /* Ethernet device */
>>>        UCLASS_ETH_PHY,         /* Ethernet PHY device */
>>>        UCLASS_FIRMWARE,        /* Firmware */
>>> +     UCLASS_FPGA,            /* FPGA device */
>>>        UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
>>>        UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
>>>        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
>>
>> Simon: the whole series look good to me. I am happy to take it via my tree when
>> you ACK it. Also no problem if you want to take it via your tree.
>> Please let me know which way you want to go.
> 
> This is a good step forward but needs a lot more work.
> 
> Please add a uclass file for the FPGA - i.e.
> drivers/fpga/fpga-uclass.c - see other such files for examples.
> 
> The FPGA uclass should have methods that match the non-DM interface.
> You will likely need a DM_FPGA config to allow enabling the uclass.
> 
> Also this needs a simple sandbox driver/emulator pair, so that it can
> be tested, with tests in test/dm/fpga.c that use the driver.
> 
> Admittedly this should have been done ages ago. I vaguely remember
> mentioning it at the time, but perhaps I missed it. In any case, all
> uclasses must have an API, implementation and tests that run in CI
> with sandbox. Testing is a vital part of U-Boot and lack of testing is
> the main reason why we went back to the 3-month release cycle.

It can be done in steps for sure. Issues which Alex is addressing are there for 
quite some time and I think we shouldn't gate them by adding requirement to 
create the whole fpga uclass. It can be done on the top of this series.
We know that it has to happen but I wouldn't push Alex to do it as condition for 
applying this series.
 From my perspective if he has time to do, let's start with it. If not it can be 
done later.

Thanks,
Michal

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Simon Glass]

Hi Michal,

On Thu, 22 Sept 2022 at 05:45, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 9/22/22 13:35, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
> >>
> >>
> >>
> >> On 9/21/22 15:22, Alexander Dahl wrote:
> >>> For future DM based FPGA drivers and for now to have a meaningful
> >>> logging class for old FPGA drivers.
> >>>
> >>> Suggested-by: Michal Simek <michal.simek@amd.com>
> >>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> >>> ---
> >>>    include/dm/uclass-id.h | 1 +
> >>>    1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> >>> index a432e43871..c2b15881ba 100644
> >>> --- a/include/dm/uclass-id.h
> >>> +++ b/include/dm/uclass-id.h
> >>> @@ -56,6 +56,7 @@ enum uclass_id {
> >>>        UCLASS_ETH,             /* Ethernet device */
> >>>        UCLASS_ETH_PHY,         /* Ethernet PHY device */
> >>>        UCLASS_FIRMWARE,        /* Firmware */
> >>> +     UCLASS_FPGA,            /* FPGA device */
> >>>        UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> >>>        UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> >>>        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
> >>
> >> Simon: the whole series look good to me. I am happy to take it via my tree when
> >> you ACK it. Also no problem if you want to take it via your tree.
> >> Please let me know which way you want to go.
> >
> > This is a good step forward but needs a lot more work.
> >
> > Please add a uclass file for the FPGA - i.e.
> > drivers/fpga/fpga-uclass.c - see other such files for examples.
> >
> > The FPGA uclass should have methods that match the non-DM interface.
> > You will likely need a DM_FPGA config to allow enabling the uclass.
> >
> > Also this needs a simple sandbox driver/emulator pair, so that it can
> > be tested, with tests in test/dm/fpga.c that use the driver.
> >
> > Admittedly this should have been done ages ago. I vaguely remember
> > mentioning it at the time, but perhaps I missed it. In any case, all
> > uclasses must have an API, implementation and tests that run in CI
> > with sandbox. Testing is a vital part of U-Boot and lack of testing is
> > the main reason why we went back to the 3-month release cycle.
>
> It can be done in steps for sure. Issues which Alex is addressing are there for
> quite some time and I think we shouldn't gate them by adding requirement to
> create the whole fpga uclass. It can be done on the top of this series.
> We know that it has to happen but I wouldn't push Alex to do it as condition for
> applying this series.
>  From my perspective if he has time to do, let's start with it. If not it can be
> done later.

Well if this is a start, then let's make it a real start. At minimum:

- add a uclass file with the uclass driver
- we can skip having any methods for now
- add a sandbox driver which does nothing
- add a test which probes the sandbox device

That is about 50 lines of code and people can then add to it over time.

Without that, I'd rather not have the UCLASS_FPGA.

Regards,
Simon

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

Hello Simon,

Am Sun, Sep 25, 2022 at 08:15:38AM -0600 schrieb Simon Glass:
> Hi Michal,
> 
> On Thu, 22 Sept 2022 at 05:45, Michal Simek <michal.simek@amd.com> wrote:
> >
> >
> >
> > On 9/22/22 13:35, Simon Glass wrote:
> > > Hi,
> > >
> > > On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
> > >>
> > >>
> > >>
> > >> On 9/21/22 15:22, Alexander Dahl wrote:
> > >>> For future DM based FPGA drivers and for now to have a meaningful
> > >>> logging class for old FPGA drivers.
> > >>>
> > >>> Suggested-by: Michal Simek <michal.simek@amd.com>
> > >>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > >>> ---
> > >>>    include/dm/uclass-id.h | 1 +
> > >>>    1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > >>> index a432e43871..c2b15881ba 100644
> > >>> --- a/include/dm/uclass-id.h
> > >>> +++ b/include/dm/uclass-id.h
> > >>> @@ -56,6 +56,7 @@ enum uclass_id {
> > >>>        UCLASS_ETH,             /* Ethernet device */
> > >>>        UCLASS_ETH_PHY,         /* Ethernet PHY device */
> > >>>        UCLASS_FIRMWARE,        /* Firmware */
> > >>> +     UCLASS_FPGA,            /* FPGA device */
> > >>>        UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> > >>>        UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> > >>>        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
> > >>
> > >> Simon: the whole series look good to me. I am happy to take it via my tree when
> > >> you ACK it. Also no problem if you want to take it via your tree.
> > >> Please let me know which way you want to go.
> > >
> > > This is a good step forward but needs a lot more work.
> > >
> > > Please add a uclass file for the FPGA - i.e.
> > > drivers/fpga/fpga-uclass.c - see other such files for examples.
> > >
> > > The FPGA uclass should have methods that match the non-DM interface.
> > > You will likely need a DM_FPGA config to allow enabling the uclass.
> > >
> > > Also this needs a simple sandbox driver/emulator pair, so that it can
> > > be tested, with tests in test/dm/fpga.c that use the driver.
> > >
> > > Admittedly this should have been done ages ago. I vaguely remember
> > > mentioning it at the time, but perhaps I missed it. In any case, all
> > > uclasses must have an API, implementation and tests that run in CI
> > > with sandbox. Testing is a vital part of U-Boot and lack of testing is
> > > the main reason why we went back to the 3-month release cycle.
> >
> > It can be done in steps for sure. Issues which Alex is addressing are there for
> > quite some time and I think we shouldn't gate them by adding requirement to
> > create the whole fpga uclass. It can be done on the top of this series.
> > We know that it has to happen but I wouldn't push Alex to do it as condition for
> > applying this series.
> >  From my perspective if he has time to do, let's start with it. If not it can be
> > done later.
> 
> Well if this is a start, then let's make it a real start. At minimum:
> 
> - add a uclass file with the uclass driver
> - we can skip having any methods for now
> - add a sandbox driver which does nothing
> - add a test which probes the sandbox device
> 
> That is about 50 lines of code and people can then add to it over time.

FWIW, I already did that on the weekend, I just have to look over
it again and maybe give it some polishing before sending.  Draft ist
here:

https://github.com/LeSpocky/u-boot/commit/49efd2a2d0129b977d38340c836bbbb1f080043b

> Without that, I'd rather not have the UCLASS_FPGA.

That's okay I guess.  Will just take me some time, it's not that easy
if you have to learn about DM and UT first, and try it in the end of
the day. ;-)

Greets
Alex

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Simon Glass]

Hi Alex,

On Mon, 26 Sept 2022 at 00:14, Alexander Dahl <post@lespocky.de> wrote:
>
> Hello Simon,
>
> Am Sun, Sep 25, 2022 at 08:15:38AM -0600 schrieb Simon Glass:
> > Hi Michal,
> >
> > On Thu, 22 Sept 2022 at 05:45, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > >
> > >
> > > On 9/22/22 13:35, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 9/21/22 15:22, Alexander Dahl wrote:
> > > >>> For future DM based FPGA drivers and for now to have a meaningful
> > > >>> logging class for old FPGA drivers.
> > > >>>
> > > >>> Suggested-by: Michal Simek <michal.simek@amd.com>
> > > >>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > >>> ---
> > > >>>    include/dm/uclass-id.h | 1 +
> > > >>>    1 file changed, 1 insertion(+)
> > > >>>
> > > >>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > > >>> index a432e43871..c2b15881ba 100644
> > > >>> --- a/include/dm/uclass-id.h
> > > >>> +++ b/include/dm/uclass-id.h
> > > >>> @@ -56,6 +56,7 @@ enum uclass_id {
> > > >>>        UCLASS_ETH,             /* Ethernet device */
> > > >>>        UCLASS_ETH_PHY,         /* Ethernet PHY device */
> > > >>>        UCLASS_FIRMWARE,        /* Firmware */
> > > >>> +     UCLASS_FPGA,            /* FPGA device */
> > > >>>        UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> > > >>>        UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> > > >>>        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
> > > >>
> > > >> Simon: the whole series look good to me. I am happy to take it via my tree when
> > > >> you ACK it. Also no problem if you want to take it via your tree.
> > > >> Please let me know which way you want to go.
> > > >
> > > > This is a good step forward but needs a lot more work.
> > > >
> > > > Please add a uclass file for the FPGA - i.e.
> > > > drivers/fpga/fpga-uclass.c - see other such files for examples.
> > > >
> > > > The FPGA uclass should have methods that match the non-DM interface.
> > > > You will likely need a DM_FPGA config to allow enabling the uclass.
> > > >
> > > > Also this needs a simple sandbox driver/emulator pair, so that it can
> > > > be tested, with tests in test/dm/fpga.c that use the driver.
> > > >
> > > > Admittedly this should have been done ages ago. I vaguely remember
> > > > mentioning it at the time, but perhaps I missed it. In any case, all
> > > > uclasses must have an API, implementation and tests that run in CI
> > > > with sandbox. Testing is a vital part of U-Boot and lack of testing is
> > > > the main reason why we went back to the 3-month release cycle.
> > >
> > > It can be done in steps for sure. Issues which Alex is addressing are there for
> > > quite some time and I think we shouldn't gate them by adding requirement to
> > > create the whole fpga uclass. It can be done on the top of this series.
> > > We know that it has to happen but I wouldn't push Alex to do it as condition for
> > > applying this series.
> > >  From my perspective if he has time to do, let's start with it. If not it can be
> > > done later.
> >
> > Well if this is a start, then let's make it a real start. At minimum:
> >
> > - add a uclass file with the uclass driver
> > - we can skip having any methods for now
> > - add a sandbox driver which does nothing
> > - add a test which probes the sandbox device
> >
> > That is about 50 lines of code and people can then add to it over time.
>
> FWIW, I already did that on the weekend, I just have to look over
> it again and maybe give it some polishing before sending.  Draft ist
> here:
>
> https://github.com/LeSpocky/u-boot/commit/49efd2a2d0129b977d38340c836bbbb1f080043b
>
> > Without that, I'd rather not have the UCLASS_FPGA.
>
> That's okay I guess.  Will just take me some time, it's not that easy
> if you have to learn about DM and UT first, and try it in the end of
> the day. ;-)

Yes, check the docs on testing.

What you have looks OK, just needs a uclass_first_device(UCLASS_FPGA,
..) in the test to make sure it can probe the device.

Regards,
Simon