Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
include/device/device.h: Move resource debug macros
Add general debug macros that print resource information. These are added if the platform resource allocator selects HAVE_DEBUG_RESOURCES and DEFAULT_CONSOLE_LOGLEVEL_8. The macros are helpful in debugging complex resource allocation with multiple buses. The macros are moved from soc/intel/xeon_sp, where they were originally developed.
Change-Id: I2bdab7770ca5ee5901f17a8af3a9a1001b6702e4 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/Kconfig M src/include/device/device.h M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/include/soc/util.h 4 files changed, 34 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/46304/1
diff --git a/src/Kconfig b/src/Kconfig index e46a6e6..352a1e1 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -903,6 +903,21 @@
If unsure, say N.
+# Only visible if DEBUG_SPEW (8) is set. It does additional printk(BIOS_DEBUG, ...) calls. +config HAVE_DEBUG_RESOURCES + def_bool n + +config DEBUG_RESOURCES + bool "Output verbose PCI MEM and IO resource debug messages" if DEFAULT_CONSOLE_LOGLEVEL_8 + default n + depends on HAVE_DEBUG_RESOURCES + help + This option enables additional PCI memory and IO debug messages. + + Note: This option will increase the size of the coreboot image. + + If unsure, say N. + config DEBUG_CONSOLE_INIT bool "Debug console initialisation code" default n diff --git a/src/include/device/device.h b/src/include/device/device.h index 031091a..0d4205d 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -261,6 +261,24 @@ struct resource *resource, const char *comment); void show_all_devs_resources(int debug_level, const char *msg);
+/* Debug macros */ +#if CONFIG(DEBUG_RESOURCES) +#include <console/console.h> +#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \ + printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ + "end: 0x%llx, size_kb: 0x%llx\n", \ + __func__, __LINE__, type, dev_path(dev), index, (base_kb << 10), \ + (base_kb << 10) + (size_kb << 10) - 1, size_kb) + +#define LOG_IO_RESOURCE(type, dev, index, base, size) \ + printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ + "end: 0x%llx, size: 0x%llx\n", \ + __func__, __LINE__, type, dev_path(dev), index, base, base + size - 1, size) +#else /* DEBUG_RESOURCES*/ +#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) +#define LOG_IO_RESOURCE(type, dev, index, base, size) +#endif /* DEBUG_RESOURCES*/ + /* Rounding for boundaries. * Due to some chip bugs, go ahead and round IO to 16 */ diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig index e449409..d3e137b 100644 --- a/src/soc/intel/xeon_sp/Kconfig +++ b/src/soc/intel/xeon_sp/Kconfig @@ -56,6 +56,7 @@ select MICROCODE_BLOB_NOT_HOOKED_UP select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FSP_CAR + select HAVE_DEBUG_RESOURCES
config MAINBOARD_USES_FSP2_0 bool diff --git a/src/soc/intel/xeon_sp/include/soc/util.h b/src/soc/intel/xeon_sp/include/soc/util.h index 159efeb..f223efa 100644 --- a/src/soc/intel/xeon_sp/include/soc/util.h +++ b/src/soc/intel/xeon_sp/include/soc/util.h @@ -10,17 +10,6 @@ void unlock_pam_regions(void); void get_stack_busnos(uint32_t *bus);
-#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \ - printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ - "end: 0x%llx, size_kb: 0x%llx\n", \ - __func__, __LINE__, type, dev_path(dev), index, (base_kb << 10), \ - (base_kb << 10) + (size_kb << 10) - 1, size_kb) - -#define LOG_IO_RESOURCE(type, dev, index, base, size) \ - printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ - "end: 0x%llx, size: 0x%llx\n", \ - __func__, __LINE__, type, dev_path(dev), index, base, base + size - 1, size) - #define DEV_FUNC_ENTER(dev) \ printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ __FILE__, __func__, __LINE__, dev_path(dev))
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 2: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@918 PS4, Line 918: # Only visible if DEBUG_SPEW (8) is set. It does additional printk(BIOS_DEBUG, ...) calls. : config HAVE_DEBUG_RESOURCES : def_bool n Any reason to have this platform specific flag?
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@922 PS4, Line 922: config DEBUG_RESOURCES At the moment this is used to print not even 10 more lines in the whole log? Is just using BIOS_SPEW with no additional Kconfig flag not enough?
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@927 PS4, Line 927: This option enables additional PCI memory and IO debug messages. : : Note: This option will increase the size of the coreboot image. : : If unsure, say N Remove the empty lines in between
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@918 PS4, Line 918: # Only visible if DEBUG_SPEW (8) is set. It does additional printk(BIOS_DEBUG, ...) calls. : config HAVE_DEBUG_RESOURCES : def_bool n
Any reason to have this platform specific flag?
Yeah, it seems this Kconfig flag could be removed entirely. It does not look like any requirements exist external to the resource allocator to use these debug functions?
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@918 PS4, Line 918: # Only visible if DEBUG_SPEW (8) is set. It does additional printk(BIOS_DEBUG, ...) calls. : config HAVE_DEBUG_RESOURCES : def_bool n
Yeah, it seems this Kconfig flag could be removed entirely. […]
This was based on a comment from Angel about reproduceable builds. The prints have __LINE__ in them which is super helpful when you have several of these calls. https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@922 PS4, Line 922: config DEBUG_RESOURCES
At the moment this is used to print not even 10 more lines in the whole log? Is just using BIOS_SPEW […]
It seemed like these were useful and could be used generally. If others don't think it is useful we can drop them. It s seems like something like this is needed on every new chipset bringup and validation.
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@927 PS4, Line 927: This option enables additional PCI memory and IO debug messages. : : Note: This option will increase the size of the coreboot image. : : If unsure, say N
Remove the empty lines in between
It is the same formatting used for all these debug options.
Hello build bot (Jenkins), Martin Roth, Marshall Dawson, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46304
to look at the new patch set (#5).
Change subject: include/device/device.h: Move resource debug macros ......................................................................
include/device/device.h: Move resource debug macros
Add general debug macros that print resource information. These are available to select if DEFAULT_CONSOLE_LOGLEVEL_8. The macros are helpful in debugging complex resource allocation with multiple buses. The macros are moved from soc/intel/xeon_sp, where they were originally developed.
Change-Id: I2bdab7770ca5ee5901f17a8af3a9a1001b6702e4 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/Kconfig M src/include/device/device.h M src/soc/intel/xeon_sp/include/soc/util.h 3 files changed, 27 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/46304/5
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@918 PS4, Line 918: # Only visible if DEBUG_SPEW (8) is set. It does additional printk(BIOS_DEBUG, ...) calls. : config HAVE_DEBUG_RESOURCES : def_bool n
This was based on a comment from Angel about reproduceable builds. […]
Ack
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@922 PS4, Line 922: config DEBUG_RESOURCES
It seemed like these were useful and could be used generally. […]
Ack
https://review.coreboot.org/c/coreboot/+/46304/4/src/Kconfig@927 PS4, Line 927: This option enables additional PCI memory and IO debug messages. : : Note: This option will increase the size of the coreboot image. : : If unsure, say N
It is the same formatting used for all these debug options.
Ack
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 5: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
Patch Set 5: Code-Review+2
Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46304 )
Change subject: include/device/device.h: Move resource debug macros ......................................................................
include/device/device.h: Move resource debug macros
Add general debug macros that print resource information. These are available to select if DEFAULT_CONSOLE_LOGLEVEL_8. The macros are helpful in debugging complex resource allocation with multiple buses. The macros are moved from soc/intel/xeon_sp, where they were originally developed.
Change-Id: I2bdab7770ca5ee5901f17a8af3a9a1001b6702e4 Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46304 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Jay Talbott JayTalbott@sysproconsulting.com Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M src/Kconfig M src/include/device/device.h M src/soc/intel/xeon_sp/include/soc/util.h 3 files changed, 27 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Jay Talbott: Looks good to me, but someone else must approve
diff --git a/src/Kconfig b/src/Kconfig index d265da7..eda11c3 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -915,6 +915,15 @@
If unsure, say N.
+# Only visible if DEBUG_SPEW (8) is set. +config DEBUG_RESOURCES + bool "Output verbose PCI MEM and IO resource debug messages" if DEFAULT_CONSOLE_LOGLEVEL_8 + default n + help + This option enables additional PCI memory and IO debug messages. + Note: This option will increase the size of the coreboot image. + If unsure, say N. + config DEBUG_CONSOLE_INIT bool "Debug console initialisation code" default n diff --git a/src/include/device/device.h b/src/include/device/device.h index 3a0795e..eb9ef42 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -261,6 +261,24 @@ struct resource *resource, const char *comment); void show_all_devs_resources(int debug_level, const char *msg);
+/* Debug macros */ +#if CONFIG(DEBUG_RESOURCES) +#include <console/console.h> +#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \ + printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ + "end: 0x%llx, size_kb: 0x%llx\n", \ + __func__, __LINE__, type, dev_path(dev), index, (base_kb << 10), \ + (base_kb << 10) + (size_kb << 10) - 1, size_kb) + +#define LOG_IO_RESOURCE(type, dev, index, base, size) \ + printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ + "end: 0x%llx, size: 0x%llx\n", \ + __func__, __LINE__, type, dev_path(dev), index, base, base + size - 1, size) +#else /* DEBUG_RESOURCES*/ +#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) +#define LOG_IO_RESOURCE(type, dev, index, base, size) +#endif /* DEBUG_RESOURCES*/ + /* Rounding for boundaries. * Due to some chip bugs, go ahead and round IO to 16 */ diff --git a/src/soc/intel/xeon_sp/include/soc/util.h b/src/soc/intel/xeon_sp/include/soc/util.h index 159efeb..f223efa 100644 --- a/src/soc/intel/xeon_sp/include/soc/util.h +++ b/src/soc/intel/xeon_sp/include/soc/util.h @@ -10,17 +10,6 @@ void unlock_pam_regions(void); void get_stack_busnos(uint32_t *bus);
-#define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \ - printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ - "end: 0x%llx, size_kb: 0x%llx\n", \ - __func__, __LINE__, type, dev_path(dev), index, (base_kb << 10), \ - (base_kb << 10) + (size_kb << 10) - 1, size_kb) - -#define LOG_IO_RESOURCE(type, dev, index, base, size) \ - printk(BIOS_SPEW, "%s:%d res: %s, dev: %s, index: 0x%x, base: 0x%llx, " \ - "end: 0x%llx, size: 0x%llx\n", \ - __func__, __LINE__, type, dev_path(dev), index, base, base + size - 1, size) - #define DEV_FUNC_ENTER(dev) \ printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ __FILE__, __func__, __LINE__, dev_path(dev))
-
Arthur Heymans (Code Review) -
Jay Talbott (Code Review)
-
Marc Jones (Code Review)
-
Stefan Reinauer (Code Review)