Change in ...coreboot[master]: nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31810
Change subject: nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set ......................................................................
nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set
Change-Id: Ia1fe691e3a5fb861afb6bf7b01a9ff23ec37858f Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/amd/amdfam10/setup_resource_map.c 1 file changed, 12 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/31810/1
diff --git a/src/northbridge/amd/amdfam10/setup_resource_map.c b/src/northbridge/amd/amdfam10/setup_resource_map.c index de2eeac..33fc0bc 100644 --- a/src/northbridge/amd/amdfam10/setup_resource_map.c +++ b/src/northbridge/amd/amdfam10/setup_resource_map.c @@ -21,8 +21,6 @@ #include <northbridge/amd/amdfam10/raminit.h> #include <northbridge/amd/amdfam10/amdfam10.h>
-#define RES_DEBUG 0 - void setup_resource_map(const u32 *register_values, u32 max) { u32 i; @@ -63,12 +61,10 @@ { u32 i;
- if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "setting up resource map ex offset....\n"); + printk(BIOS_DEBUG, "setting up resource map ex offset....\n");
for (i = 0; i < max; i += 4) { - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", + printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", i/4, register_values[i], register_values[i+1] + ((register_values[i]==RES_PCI_IO) ? offset_pci_dev : 0), register_values[i+2], @@ -83,13 +79,11 @@ dev = (register_values[i+1] & ~0xfff) + offset_pci_dev; where = register_values[i+1] & 0xfff; reg = pci_read_config32(dev, where); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "WAS: %08x\n", reg); + printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; pci_write_config32(dev, where, reg); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "NOW: %08x\n", reg); + printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; case RES_PORT_IO_8: // io 8 @@ -98,13 +92,11 @@ u32 reg; where = register_values[i+1] + offset_io_base; reg = inb(where); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "WAS: %08x\n", reg); + printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; outb(reg, where); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "NOW: %08x\n", reg); + printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; case RES_PORT_IO_32: //io32 @@ -113,32 +105,27 @@ u32 reg; where = register_values[i+1] + offset_io_base; reg = inl(where); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "WAS: %08x\n", reg); + printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; outl(reg, where); - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_SPEW, "NOW: %08x\n", reg); + printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; } }
- if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "done.\n"); + printk(BIOS_DEBUG, "done.\n"); }
void setup_resource_map_x(const u32 *register_values, u32 max) { u32 i;
- if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "setting up resource map ex offset....\n"); + printk(BIOS_DEBUG, "setting up resource map ex offset....\n");
for (i = 0; i < max; i += 4) { - if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", + printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", i/4, register_values[i],register_values[i+1], register_values[i+2], register_values[i+3]); switch (register_values[i]) { case RES_PCI_IO: //PCI @@ -179,6 +166,5 @@ } }
- if (IS_ENABLED(RES_DEBUG)) - printk(BIOS_DEBUG, "done.\n"); + printk(BIOS_DEBUG, "done.\n"); }
HAOUAS Elyes has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/31810 )
Change subject: nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set ......................................................................
nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set
Change-Id: Ia1fe691e3a5fb861afb6bf7b01a9ff23ec37858f Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/amd/amdfam10/setup_resource_map.c 1 file changed, 18 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/31810/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31810 )
Change subject: nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... File src/northbridge/amd/amdfam10/setup_resource_map.c:
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... PS2, Line 69: register_values[i+1] + ((register_values[i]==RES_PCI_IO) ? offset_pci_dev : 0), line over 96 characters
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... PS2, Line 69: register_values[i+1] + ((register_values[i]==RES_PCI_IO) ? offset_pci_dev : 0), spaces required around that '==' (ctx:VxV)
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... PS2, Line 71: register_values[i+3] + (((register_values[i] & RES_PORT_IO_32) == RES_PORT_IO_32) ? offset_io_base : 0) line over 96 characters
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... PS2, Line 129: i/4, register_values[i],register_values[i+1], register_values[i+2], register_values[i+3]); line over 96 characters
https://review.coreboot.org/#/c/31810/2/src/northbridge/amd/amdfam10/setup_r... PS2, Line 129: i/4, register_values[i],register_values[i+1], register_values[i+2], register_values[i+3]); space required after that ',' (ctx:VxV)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31810 )
Change subject: nb/amd/amdfam10: Remove redundant test if "BIOS_DEBUG" set ......................................................................
Patch Set 2:
You're changing the logic here, RES_DEBUG used to be 0 but now you enable all those prints unconditionally.
I'd recommend to leave in RES_DEBUG and just change all the 'if (IS_ENABLED(RES_DEBUG))' to a simple 'if (RES_DEBUG)'. This sort of stuff is useful to keep debug prints in that we don't want to enable by default, but want to have available easily for developers that want to enable some more log spam when they debug something.
Hello Kyösti Mälkki, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31810
to look at the new patch set (#3).
Change subject: nb/amd/amdfam10: Remove 'IS_ENABLED()' ......................................................................
nb/amd/amdfam10: Remove 'IS_ENABLED()'
Change-Id: Ia1fe691e3a5fb861afb6bf7b01a9ff23ec37858f Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/amd/amdfam10/setup_resource_map.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/31810/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31810 )
Change subject: nb/amd/amdfam10: Remove 'IS_ENABLED()' ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31810 )
Change subject: nb/amd/amdfam10: Remove 'IS_ENABLED()' ......................................................................
nb/amd/amdfam10: Remove 'IS_ENABLED()'
Change-Id: Ia1fe691e3a5fb861afb6bf7b01a9ff23ec37858f Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/31810 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/amd/amdfam10/setup_resource_map.c 1 file changed, 12 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/northbridge/amd/amdfam10/setup_resource_map.c b/src/northbridge/amd/amdfam10/setup_resource_map.c index de2eeac..3e6a437 100644 --- a/src/northbridge/amd/amdfam10/setup_resource_map.c +++ b/src/northbridge/amd/amdfam10/setup_resource_map.c @@ -63,11 +63,11 @@ { u32 i;
- if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "setting up resource map ex offset....\n");
for (i = 0; i < max; i += 4) { - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", i/4, register_values[i], register_values[i+1] + ((register_values[i]==RES_PCI_IO) ? offset_pci_dev : 0), @@ -83,12 +83,12 @@ dev = (register_values[i+1] & ~0xfff) + offset_pci_dev; where = register_values[i+1] & 0xfff; reg = pci_read_config32(dev, where); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; pci_write_config32(dev, where, reg); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; @@ -98,12 +98,12 @@ u32 reg; where = register_values[i+1] + offset_io_base; reg = inb(where); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; outb(reg, where); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; @@ -113,19 +113,19 @@ u32 reg; where = register_values[i+1] + offset_io_base; reg = inl(where); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "WAS: %08x\n", reg); reg &= register_values[i+2]; reg |= register_values[i+3]; outl(reg, where); - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_SPEW, "NOW: %08x\n", reg); } break; } }
- if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "done.\n"); }
@@ -133,11 +133,11 @@ { u32 i;
- if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "setting up resource map ex offset....\n");
for (i = 0; i < max; i += 4) { - if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "%04x: %02x %08x <- & %08x | %08x\n", i/4, register_values[i],register_values[i+1], register_values[i+2], register_values[i+3]); switch (register_values[i]) { @@ -179,6 +179,6 @@ } }
- if (IS_ENABLED(RES_DEBUG)) + if (RES_DEBUG) printk(BIOS_DEBUG, "done.\n"); }
-
build bot (Jenkins) (Code Review) -
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)