Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
soc/intel/xeon_sp: Move debug macros to console/debug.h
Move the macros for printing debug information to the common console include directory. The macros could be used by any platform.
Change-Id: Ie237bdf8cdc42c76f38a0c820fdc92e81095f47c Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- A src/include/console/debug.h M src/soc/intel/xeon_sp/cpx/chip.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/skx/chip.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/uncore.c 8 files changed, 38 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/46093/1
diff --git a/src/include/console/debug.h b/src/include/console/debug.h new file mode 100644 index 0000000..7a04372 --- /dev/null +++ b/src/include/console/debug.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef _CONSOLE_DEBUG_H_ +#define _CONSOLE_DEBUG_H_ + +#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) + +#define DEV_FUNC_ENTER(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ + __FILE__, __func__, __LINE__, dev_path(dev)) + +#define DEV_FUNC_EXIT(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ + __func__, __LINE__, dev_path(dev)) + +#define FUNC_ENTER() \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) + +#define FUNC_EXIT() \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) + +#endif diff --git a/src/soc/intel/xeon_sp/cpx/chip.c b/src/soc/intel/xeon_sp/cpx/chip.c index 11fe44b..e0e4522 100644 --- a/src/soc/intel/xeon_sp/cpx/chip.c +++ b/src/soc/intel/xeon_sp/cpx/chip.c @@ -2,7 +2,7 @@
#include <arch/ioapic.h> #include <assert.h> -#include <console/console.h> +#include <console/debug.h> #include <cpu/x86/lapic.h> #include <device/pci.h> #include <fsp/api.h> diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index eb8c0eb..ae35424 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -3,7 +3,7 @@ #include <acpi/acpigen.h> #include <acpi/acpi.h> #include <assert.h> -#include <console/console.h> +#include <console/debug.h> #include <cpu/cpu.h> #include <cpu/intel/microcode.h> #include <cpu/intel/turbo.h> diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index 412730b..3e19bac 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -3,24 +3,8 @@ #ifndef _SOC_UTIL_H_ #define _SOC_UTIL_H_
-#include <console/console.h> #include <hob_iiouds.h> #include <hob_memmap.h> -#include <stdint.h> - -#define DEV_FUNC_ENTER(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ - __FILE__, __func__, __LINE__, dev_path(dev)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__)
struct iiostack_resource { uint8_t no_of_stacks; diff --git a/src/soc/intel/xeon_sp/include/soc/util.h b/src/soc/intel/xeon_sp/include/soc/util.h index 159efeb..8c2b597 100644 --- a/src/soc/intel/xeon_sp/include/soc/util.h +++ b/src/soc/intel/xeon_sp/include/soc/util.h @@ -3,36 +3,10 @@ #ifndef _XEON_SP_SOC_UTIL_H_ #define _XEON_SP_SOC_UTIL_H_
-#include <console/console.h> #include <hob_iiouds.h>
void get_cpubusnos(uint32_t *bus0, uint32_t *bus1, uint32_t *bus2, uint32_t *bus3); 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)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) - #endif diff --git a/src/soc/intel/xeon_sp/skx/chip.c b/src/soc/intel/xeon_sp/skx/chip.c index 845d7cb..f7d5a39 100644 --- a/src/soc/intel/xeon_sp/skx/chip.c +++ b/src/soc/intel/xeon_sp/skx/chip.c @@ -2,6 +2,7 @@
#include <cbfs.h> #include <assert.h> +#include <console/debug.h> #include <post.h> #include <device/pci.h> #include <soc/acpi.h> diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c index ea9f531..e0476f8 100644 --- a/src/soc/intel/xeon_sp/skx/cpu.c +++ b/src/soc/intel/xeon_sp/skx/cpu.c @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
-#include <console/console.h> +#include <console/debug.h> #include <intelblocks/cpulib.h> #include <cpu/cpu.h> #include <cpu/x86/mtrr.h> diff --git a/src/soc/intel/xeon_sp/uncore.c b/src/soc/intel/xeon_sp/uncore.c index a549acb..d24b5a2 100644 --- a/src/soc/intel/xeon_sp/uncore.c +++ b/src/soc/intel/xeon_sp/uncore.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <cbmem.h> -#include <console/console.h> +#include <console/debug.h> #include <cpu/x86/lapic_def.h> #include <device/pci.h> #include <device/pci_ids.h>
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
This could also be moved somewhere else, but seems like the correct spot. I was suprised we didn't have some generic macros for console debug. This might clean up a lot of copypasta printks and standardize some common console output.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... PS1, Line 4: : #include <console/debug.h> My only complaint is that I would also keep console/console.h for any file that uses a printk(), i.e. don't rely on the assumption that debug.h will include it. This probably applies to all the .c files in this change but I didn't verify.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \ Should we use a Kconfig option to enable/disable these macros? Having __LINE__ in them breaks reproducibility even when the loglevel is lower than BIOS_SPEW.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... PS1, Line 4: : #include <console/debug.h>
My only complaint is that I would also keep console/console.h for any file that uses a printk(), i. […]
IMHO, I'd even go as far as to drop the <console/console.h> inclusion from <console/debug.h>. This prevents using any macro from there unless the necessary headers have been included. Since the compiler only sees macros if they are used in the code, including <console/debug.h> without including <console/console.h> would only work if none of the macros are used.
If this approach seems too cursed, notice that <console/debug.h> does not include any headers for the `dev_path` function.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... PS1, Line 4: : #include <console/debug.h>
IMHO, I'd even go as far as to drop the <console/console.h> inclusion from <console/debug.h>. […]
Haha, my preference is to put everything a header file requires inside that header, then let the guards do their job. And you're right, I overlooked the dev_path. I'd go in one direction or the other (all or nothing), but won't argue over which.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... PS1, Line 4: : #include <console/debug.h>
Haha, my preference is to put everything a header file requires inside that header, then let the gua […]
I agree, consistency is important.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move debug macros to console/debug.h ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \
Should we use a Kconfig option to enable/disable these macros? Having __LINE__ in them breaks reprod […]
Maybe an ifdef for DEBUG_SPEW level. It wouldn't be included in a build that turned up the debug level after the build. I'd also be ok with removing __LINE__.
Also, should all of this file reside here with the use of dev_path? The FUNC stuff is general enough, but maybe the RESOURCE stuff should go in pci?
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/46093/1/src/soc/intel/xeon_sp/cpx/c... PS1, Line 4: : #include <console/debug.h>
I agree, consistency is important.
I agree with Marshall that we should include all that is required. I also agree that debug.h and console.h should be included in the source files that have printks (which is probably all of them).
Hello build bot (Jenkins), Martin Roth, Marshall Dawson, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46093
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
soc/intel/xeon_sp: Move function debug macros
Move the macros for printing debug information to debug.h in the common console include directory and device include file. These are added if the platform selects HAVE_DEBUG_FUNC and DEFAULT_CONSOLE_LOGLEVEL_8.
The macros could be used by any platform.
Change-Id: Ie237bdf8cdc42c76f38a0c820fdc92e81095f47c Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/Kconfig A src/include/console/debug.h M src/include/device/device.h M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/cpx/chip.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/skx/chip.c M src/soc/intel/xeon_sp/skx/cpu.c 10 files changed, 57 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/46093/3
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 3:
(1 comment)
I've think I addressed the comments. The resource functions are moves in a new patch: CB46304.
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \
Maybe an ifdef for DEBUG_SPEW level. […]
Done
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 3: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig@1132 PS5, Line 1132: config HAVE_DEBUG_FUNC : def_bool n : Is this needed?
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h... PS5, Line 282: #if CONFIG(DEBUG_FUNC) Does this need to be guarded here?
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig@1132 PS5, Line 1132: config HAVE_DEBUG_FUNC : def_bool n :
Is this needed?
This was from a previous comment from Angle. https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h... PS5, Line 282: #if CONFIG(DEBUG_FUNC)
Does this need to be guarded here?
This was from Angels comment: https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h... PS5, Line 282: #if CONFIG(DEBUG_FUNC)
This was from Angels comment: https://review.coreboot. […]
Sorry for going back and forth so much... I don't think the scope of these macros should be extended. If one really needs to trace function entry/exit often, it may be a good idea to clean up the code flow to improve readability.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \
Done
Angel, why would __LINE__ not be reproducible? The line numbers don't change if the source code doesn't change? Are you confusing this with __DATE__ or __TIME__ ?
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h... PS5, Line 282: #if CONFIG(DEBUG_FUNC)
Sorry for going back and forth so much... […]
This is some pretty generic stuff. Ron wanted to add "poor man's debugging" to the core code many times. I don't see how this is "extending the scope of these macros". I do not see it as beneficial to bury this stuff behind yet another config variable when we already have 10x more config variables than are possibly needed to build a firmware image. This "fake choice" just litters up the code base imo.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \
Angel, why would __LINE__ not be reproducible? The line numbers don't change if the source code does […]
If one moves the code around and the line numbers change, reproducible builds break. I've had this problem with assertions too, but made CB:42196 to work around it.
Hello build bot (Jenkins), Martin Roth, Marshall Dawson, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46093
to look at the new patch set (#6).
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
soc/intel/xeon_sp: Move function debug macros
Move the macros for printing debug information to debug.h in the common console include directory and device include file. These are available if the platform selects DEFAULT_CONSOLE_LOGLEVEL_8.
The macros could be used by any platform.
Change-Id: Ie237bdf8cdc42c76f38a0c820fdc92e81095f47c Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/Kconfig A src/include/console/debug.h M src/include/device/device.h M src/soc/intel/xeon_sp/cpx/chip.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/skx/chip.c M src/soc/intel/xeon_sp/skx/cpu.c 9 files changed, 51 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/46093/6
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 5:
(2 comments)
These macros don't do anything by default. They need to be enabled in a local config. This shouldn't effect TIMELESS builds.
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/46093/5/src/Kconfig@1132 PS5, Line 1132: config HAVE_DEBUG_FUNC : def_bool n :
This was from a previous comment from Angle. https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/46093/5/src/include/device/device.h... PS5, Line 282: #if CONFIG(DEBUG_FUNC)
This is some pretty generic stuff. […]
Ack
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 6: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h File src/include/console/debug.h:
https://review.coreboot.org/c/coreboot/+/46093/1/src/include/console/debug.h... PS1, Line 8: #define LOG_MEM_RESOURCE(type, dev, index, base_kb, size_kb) \
If one moves the code around and the line numbers change, reproducible builds break. […]
If you move code around, it is different code resulting in a different binary.
Reproducibility means the same code produces the same binary.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
Patch Set 6: Code-Review+2
Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46093 )
Change subject: soc/intel/xeon_sp: Move function debug macros ......................................................................
soc/intel/xeon_sp: Move function debug macros
Move the macros for printing debug information to debug.h in the common console include directory and device include file. These are available if the platform selects DEFAULT_CONSOLE_LOGLEVEL_8.
The macros could be used by any platform.
Change-Id: Ie237bdf8cdc42c76f38a0c820fdc92e81095f47c Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46093 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 A src/include/console/debug.h M src/include/device/device.h M src/soc/intel/xeon_sp/cpx/chip.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/skx/chip.c M src/soc/intel/xeon_sp/skx/cpu.c 9 files changed, 51 insertions(+), 31 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 eda11c3..dc98ca2 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -1123,6 +1123,16 @@ of calling function. Please note some printk related functions are omitted from trace to have good looking console dumps.
+config DEBUG_FUNC + bool "Enable function entry and exit reporting macros" if DEFAULT_CONSOLE_LOGLEVEL_8 + default n + help + This option enables additional function entry and exit debug messages + for select functions. If supported, this is less output than + the TRACE option. + Note: This option will increase the size of the coreboot image. + If unsure, say N. + config DEBUG_COVERAGE bool "Debug code coverage" default n diff --git a/src/include/console/debug.h b/src/include/console/debug.h new file mode 100644 index 0000000..174c287 --- /dev/null +++ b/src/include/console/debug.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef _CONSOLE_DEBUG_H_ +#define _CONSOLE_DEBUG_H_ + +#if CONFIG(DEBUG_FUNC) +#include <console/console.h> + +#define FUNC_ENTER() \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) + +#define FUNC_EXIT() \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) + +#else /* FUNC_DEBUG */ + +#define FUNC_ENTER() +#define FUNC_EXIT() + +#endif /* FUNC_DEBUG */ + +#endif diff --git a/src/include/device/device.h b/src/include/device/device.h index eb9ef42..8a481b2 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -279,6 +279,20 @@ #define LOG_IO_RESOURCE(type, dev, index, base, size) #endif /* DEBUG_RESOURCES*/
+#if CONFIG(DEBUG_FUNC) +#include <console/console.h> +#define DEV_FUNC_ENTER(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ + __FILE__, __func__, __LINE__, dev_path(dev)) + +#define DEV_FUNC_EXIT(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ + __func__, __LINE__, dev_path(dev)) +#else /* DEBUG_FUNC */ +#define DEV_FUNC_ENTER(dev) +#define DEV_FUNC_EXIT(dev) +#endif /* DEBUG_FUNC */ + /* Rounding for boundaries. * Due to some chip bugs, go ahead and round IO to 16 */ diff --git a/src/soc/intel/xeon_sp/cpx/chip.c b/src/soc/intel/xeon_sp/cpx/chip.c index c5a8c1c..6d2dfba 100644 --- a/src/soc/intel/xeon_sp/cpx/chip.c +++ b/src/soc/intel/xeon_sp/cpx/chip.c @@ -3,6 +3,7 @@ #include <arch/ioapic.h> #include <assert.h> #include <console/console.h> +#include <console/debug.h> #include <cpu/x86/lapic.h> #include <device/pci.h> #include <fsp/api.h> diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index 0999f6d..4afe47c 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -4,6 +4,7 @@ #include <acpi/acpi.h> #include <assert.h> #include <console/console.h> +#include <console/debug.h> #include <cpu/cpu.h> #include <cpu/intel/microcode.h> #include <cpu/intel/turbo.h> diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index 412730b..3e19bac 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -3,24 +3,8 @@ #ifndef _SOC_UTIL_H_ #define _SOC_UTIL_H_
-#include <console/console.h> #include <hob_iiouds.h> #include <hob_memmap.h> -#include <stdint.h> - -#define DEV_FUNC_ENTER(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ - __FILE__, __func__, __LINE__, dev_path(dev)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__)
struct iiostack_resource { uint8_t no_of_stacks; diff --git a/src/soc/intel/xeon_sp/include/soc/util.h b/src/soc/intel/xeon_sp/include/soc/util.h index f223efa..8c2b597 100644 --- a/src/soc/intel/xeon_sp/include/soc/util.h +++ b/src/soc/intel/xeon_sp/include/soc/util.h @@ -3,25 +3,10 @@ #ifndef _XEON_SP_SOC_UTIL_H_ #define _XEON_SP_SOC_UTIL_H_
-#include <console/console.h> #include <hob_iiouds.h>
void get_cpubusnos(uint32_t *bus0, uint32_t *bus1, uint32_t *bus2, uint32_t *bus3); void unlock_pam_regions(void); void get_stack_busnos(uint32_t *bus);
-#define DEV_FUNC_ENTER(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ - __FILE__, __func__, __LINE__, dev_path(dev)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) - #endif diff --git a/src/soc/intel/xeon_sp/skx/chip.c b/src/soc/intel/xeon_sp/skx/chip.c index 4324660..fba1e1f 100644 --- a/src/soc/intel/xeon_sp/skx/chip.c +++ b/src/soc/intel/xeon_sp/skx/chip.c @@ -2,6 +2,8 @@
#include <cbfs.h> #include <assert.h> +#include <console/console.h> +#include <console/debug.h> #include <post.h> #include <device/pci.h> #include <soc/acpi.h> diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c index bf712c3..581378b 100644 --- a/src/soc/intel/xeon_sp/skx/cpu.c +++ b/src/soc/intel/xeon_sp/skx/cpu.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <console/console.h> +#include <console/debug.h> #include <intelblocks/cpulib.h> #include <cpu/cpu.h> #include <cpu/x86/mtrr.h>
-
Angel Pons (Code Review) -
Jay Talbott (Code Review)
-
Marc Jones (Code Review)
-
Marshall Dawson (Code Review)
-
Stefan Reinauer (Code Review)