[PATCH v2 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 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
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 */
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
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
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
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
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/49efd2a2d0129b977d38340c836bbbb1f0...
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
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/49efd2a2d0129b977d38340c836bbbb1f0...
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
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;
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"
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
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);
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);
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);
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; }
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__);@@ -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__);@@ -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
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__);
PRINTF("%s: Launching Slave Parallel Load\n", __func__);
log_debug("%s: Launching Slave Parallel Load\n", __func__);@@ -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__);
PRINTF("%s: Launching Slave Parallel Dump\n", __func__);
log_debug("%s: Launching Slave Parallel Dump\n", __func__);@@ -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);@@ -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
participants (4)
-
Alexander Dahl -
Alexander Dahl -
Michal Simek -
Simon Glass