Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
nb/intel/i440bx: Clean up register_values table
The table of initial i440BX register values has a bitmask that allows preserving certain bits as they are programmed. This feature has been unused since day one and probably will never be used. Drop it to save some bytes.
Also dropping DRB, RPS, PGPOL registers from the table as they will be programmed during RAM init.
TEST=Boot tested on asus/p2b-ls, i440bx config did not change
Change-Id: I020f616455bb671fe284993a488beb6386a03d0d Signed-off-by: Keith Hui buurin@gmail.com --- M src/northbridge/intel/i440bx/raminit.c 1 file changed, 20 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/40391/1
diff --git a/src/northbridge/intel/i440bx/raminit.c b/src/northbridge/intel/i440bx/raminit.c index e2db5e7..cdfc4f6 100644 --- a/src/northbridge/intel/i440bx/raminit.c +++ b/src/northbridge/intel/i440bx/raminit.c @@ -60,7 +60,7 @@ 1, 5, 5, 2, 3, 4 };
-/* Table format: register, bitmask, value. */ +/* Table format: register, value. */ static const u8 register_values[] = { /* NBXCFG - NBX Configuration Register * 0x50 - 0x53 @@ -122,11 +122,11 @@ * 0 = A7# is sampled asserted (i.e., 0) * [01:00] Reserved */ - NBXCFG + 0, 0x00, 0x0c, + NBXCFG + 0, 0x0c, // TODO: Bit 15 should be 0 for multiprocessor boards - NBXCFG + 1, 0x00, 0x80, - NBXCFG + 2, 0x00, 0x00, - NBXCFG + 3, 0x00, 0xff, + NBXCFG + 1, 0x80, + NBXCFG + 2, 0x00, + NBXCFG + 3, 0xff,
/* DRAMC - DRAM Control Register * 0x57 @@ -156,7 +156,7 @@ * 111 = Reserved */ /* Choose SDRAM (not registered), and disable refresh for now. */ - DRAMC, 0x00, 0x08, + DRAMC, 0x08,
/* * PAM[6:0] - Programmable Attribute Map Registers @@ -190,13 +190,13 @@ * registers are not set here appropriately, the RAM in that region * will not be accessible, thus a RAM check of it will also fail. */ - PAM0, 0x00, 0x30, - PAM1, 0x00, 0x33, - PAM2, 0x00, 0x33, - PAM3, 0x00, 0x33, - PAM4, 0x00, 0x33, - PAM5, 0x00, 0x33, - PAM6, 0x00, 0x33, + PAM0, 0x30, + PAM1, 0x33, + PAM2, 0x33, + PAM3, 0x33, + PAM4, 0x33, + PAM5, 0x33, + PAM6, 0x33,
/* DRB[0:7] - DRAM Row Boundary Registers * 0x60 - 0x67 @@ -214,14 +214,6 @@ * 0x67 DRB7 = Total memory in row0+1+2+3+4+5+6+7 (in 8 MB) */ /* DRBs will be set later. */ - DRB0, 0x00, 0x00, - DRB1, 0x00, 0x00, - DRB2, 0x00, 0x00, - DRB3, 0x00, 0x00, - DRB4, 0x00, 0x00, - DRB5, 0x00, 0x00, - DRB6, 0x00, 0x00, - DRB7, 0x00, 0x00,
/* FDHC - Fixed DRAM Hole Control Register * 0x68 @@ -236,7 +228,7 @@ * [5:0] Reserved */ /* No memory holes. */ - FDHC, 0x00, 0x00, + FDHC, 0x00,
/* RPS - SDRAM Row Page Size Register * 0x74 - 0x75 @@ -261,8 +253,6 @@ * [15:14] DRB[7], row 7 */ /* Power on defaults to 2KB. Will be set later. */ - // RPS + 0, 0x00, 0x00, - // RPS + 1, 0x00, 0x00,
/* SDRAMC - SDRAM Control Register * 0x76 - 0x77 @@ -298,9 +288,9 @@ * 1 = 2 clocks of RAS# precharge */ #if CONFIG(SDRAMPWR_4DIMM) - SDRAMC + 0, 0x00, 0x10, /* The board has 4 DIMM slots. */ + SDRAMC + 0, 0x10, /* The board has 4 DIMM slots. */ #else - SDRAMC + 0, 0x00, 0x00, /* The board has 3 DIMM slots. */ + SDRAMC + 0, 0x00, /* The board has 3 DIMM slots. */ #endif
/* PGPOL - Paging Policy Register @@ -325,8 +315,6 @@ * 0111 = 32 clocks * 1xxx = Infinite (pages are not closed for idle condition) */ - PGPOL + 0, 0x00, 0x00, - PGPOL + 1, 0x00, 0xff,
/* PMCR - Power Management Control Register * 0x7a @@ -355,10 +343,10 @@ * 1 = Enable * 0 = Disable */ - /* PMCR will be set later. */ + /* PGPOL and PMCR will be set later. */
/* Enable SCRR.SRRAEN and let BX choose the SRR. */ - SCRR + 1, 0x00, 0x10, + SCRR + 1, 0x10, };
/*----------------------------------------------------------------------------- @@ -666,7 +654,6 @@ static void sdram_set_registers(void) { int i, max; - uint8_t reg;
PRINT_DEBUG("Northbridge prior to SDRAM init:\n"); DUMPNORTH(); @@ -674,11 +661,8 @@ max = ARRAY_SIZE(register_values);
/* Set registers as specified in the register_values[] array. */ - for (i = 0; i < max; i += 3) { - reg = pci_read_config8(NB, register_values[i]); - reg &= register_values[i + 1]; - reg |= register_values[i + 2] & ~(register_values[i + 1]); - pci_write_config8(NB, register_values[i], reg); + for (i = 0; i < max; i += 2) { + pci_write_config8(NB, register_values[i], register_values[i + 1]); } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/raminit.c:
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... PS1, Line 664: for (i = 0; i < max; i += 2) { braces {} are not necessary for single statement blocks
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG@9 PS1, Line 9: The table of initial i440BX register values has a bitmask that allows preserving The lines are a tad bit too long, mind reflowing this?
The table of initial i440BX register values has a bitmask that allows preserving certain bits as they are programmed. This feature has been unused since day one and probably will never be used. Drop it to save some bytes.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG@11 PS1, Line 11: and probably will never be used. Drop it to save some bytes. Please give hard numbers.
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/raminit.c:
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... PS1, Line 346: PGPOL I would prefer that comment directly below the comment for PGPOL above.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG@11 PS1, Line 11: and probably will never be used. Drop it to save some bytes.
Please give hard numbers.
37 bytes off the table itself. I don't yet have an easy way to know how many bytes the coding changes save.
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40391
to look at the new patch set (#2).
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
nb/intel/i440bx: Clean up register_values table
The table of initial i440BX register values has a bitmask that allows preserving certain bits as they are programmed. This feature has been unused since day one and probably will never be used. So drop it.
Drop DRB, RPS, PGPOL registers from the table as they will be programmed during RAM init. These two reductions combined saved ~104 bytes.
Drop unneeded SDRAMC "+0".
Slightly compact a comment block.
TEST=Boot tested on asus/p2b-ls, i440bx config did not change
Change-Id: I020f616455bb671fe284993a488beb6386a03d0d Signed-off-by: Keith Hui buurin@gmail.com --- M src/northbridge/intel/i440bx/raminit.c 1 file changed, 42 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/40391/2
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG@9 PS1, Line 9: The table of initial i440BX register values has a bitmask that allows preserving
The lines are a tad bit too long, mind reflowing this? […]
Done
https://review.coreboot.org/c/coreboot/+/40391/1//COMMIT_MSG@11 PS1, Line 11: and probably will never be used. Drop it to save some bytes.
37 bytes off the table itself. […]
Ack
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/raminit.c:
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... PS1, Line 346: PGPOL
I would prefer that comment directly below the comment for PGPOL above.
Done
https://review.coreboot.org/c/coreboot/+/40391/1/src/northbridge/intel/i440b... PS1, Line 664: for (i = 0; i < max; i += 2) {
braces {} are not necessary for single statement blocks
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
nb/intel/i440bx: Clean up register_values table
The table of initial i440BX register values has a bitmask that allows preserving certain bits as they are programmed. This feature has been unused since day one and probably will never be used. So drop it.
Drop DRB, RPS, PGPOL registers from the table as they will be programmed during RAM init. These two reductions combined saved ~104 bytes.
Drop unneeded SDRAMC "+0".
Slightly compact a comment block.
TEST=Boot tested on asus/p2b-ls, i440bx config did not change
Change-Id: I020f616455bb671fe284993a488beb6386a03d0d Signed-off-by: Keith Hui buurin@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40391 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/northbridge/intel/i440bx/raminit.c 1 file changed, 42 insertions(+), 59 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/northbridge/intel/i440bx/raminit.c b/src/northbridge/intel/i440bx/raminit.c index e2db5e7..dddcc21 100644 --- a/src/northbridge/intel/i440bx/raminit.c +++ b/src/northbridge/intel/i440bx/raminit.c @@ -60,7 +60,7 @@ 1, 5, 5, 2, 3, 4 };
-/* Table format: register, bitmask, value. */ +/* Table format: register, value. */ static const u8 register_values[] = { /* NBXCFG - NBX Configuration Register * 0x50 - 0x53 @@ -122,11 +122,11 @@ * 0 = A7# is sampled asserted (i.e., 0) * [01:00] Reserved */ - NBXCFG + 0, 0x00, 0x0c, + NBXCFG + 0, 0x0c, // TODO: Bit 15 should be 0 for multiprocessor boards - NBXCFG + 1, 0x00, 0x80, - NBXCFG + 2, 0x00, 0x00, - NBXCFG + 3, 0x00, 0xff, + NBXCFG + 1, 0x80, + NBXCFG + 2, 0x00, + NBXCFG + 3, 0xff,
/* DRAMC - DRAM Control Register * 0x57 @@ -156,7 +156,7 @@ * 111 = Reserved */ /* Choose SDRAM (not registered), and disable refresh for now. */ - DRAMC, 0x00, 0x08, + DRAMC, 0x08,
/* * PAM[6:0] - Programmable Attribute Map Registers @@ -190,13 +190,13 @@ * registers are not set here appropriately, the RAM in that region * will not be accessible, thus a RAM check of it will also fail. */ - PAM0, 0x00, 0x30, - PAM1, 0x00, 0x33, - PAM2, 0x00, 0x33, - PAM3, 0x00, 0x33, - PAM4, 0x00, 0x33, - PAM5, 0x00, 0x33, - PAM6, 0x00, 0x33, + PAM0, 0x30, + PAM1, 0x33, + PAM2, 0x33, + PAM3, 0x33, + PAM4, 0x33, + PAM5, 0x33, + PAM6, 0x33,
/* DRB[0:7] - DRAM Row Boundary Registers * 0x60 - 0x67 @@ -214,14 +214,6 @@ * 0x67 DRB7 = Total memory in row0+1+2+3+4+5+6+7 (in 8 MB) */ /* DRBs will be set later. */ - DRB0, 0x00, 0x00, - DRB1, 0x00, 0x00, - DRB2, 0x00, 0x00, - DRB3, 0x00, 0x00, - DRB4, 0x00, 0x00, - DRB5, 0x00, 0x00, - DRB6, 0x00, 0x00, - DRB7, 0x00, 0x00,
/* FDHC - Fixed DRAM Hole Control Register * 0x68 @@ -236,7 +228,7 @@ * [5:0] Reserved */ /* No memory holes. */ - FDHC, 0x00, 0x00, + FDHC, 0x00,
/* RPS - SDRAM Row Page Size Register * 0x74 - 0x75 @@ -261,8 +253,6 @@ * [15:14] DRB[7], row 7 */ /* Power on defaults to 2KB. Will be set later. */ - // RPS + 0, 0x00, 0x00, - // RPS + 1, 0x00, 0x00,
/* SDRAMC - SDRAM Control Register * 0x76 - 0x77 @@ -298,9 +288,9 @@ * 1 = 2 clocks of RAS# precharge */ #if CONFIG(SDRAMPWR_4DIMM) - SDRAMC + 0, 0x00, 0x10, /* The board has 4 DIMM slots. */ + SDRAMC, 0x10, /* The board has 4 DIMM slots. */ #else - SDRAMC + 0, 0x00, 0x00, /* The board has 3 DIMM slots. */ + SDRAMC, 0x00, /* The board has 3 DIMM slots. */ #endif
/* PGPOL - Paging Policy Register @@ -325,40 +315,38 @@ * 0111 = 32 clocks * 1xxx = Infinite (pages are not closed for idle condition) */ - PGPOL + 0, 0x00, 0x00, - PGPOL + 1, 0x00, 0xff, + /* PGPOL will be set later. */
/* PMCR - Power Management Control Register * 0x7a * - * [07:07] Power Down SDRAM Enable (PDSE) - * 1 = Enable - * 0 = Disable - * [06:06] ACPI Control Register Enable (SCRE) - * 1 = Enable - * 0 = Disable (default) - * [05:05] Suspend Refresh Type (SRT) - * 1 = Self refresh mode - * 0 = CBR fresh mode - * [04:04] Normal Refresh Enable (NREF_EN) - * 1 = Enable - * 0 = Disable - * [03:03] Quick Start Mode (QSTART) - * 1 = Quick start mode for the processor is enabled - * [02:02] Gated Clock Enable (GCLKEN) - * 1 = Enable - * 0 = Disable - * [01:01] AGP Disable (AGP_DIS) - * 1 = Disable - * 0 = Enable - * [00:00] CPU reset without PCIRST enable (CRst_En) - * 1 = Enable - * 0 = Disable + * [7] Power Down SDRAM Enable (PDSE) + * 1 = Enable + * 0 = Disable + * [6] ACPI Control Register Enable (SCRE) + * 1 = Enable + * 0 = Disable (default) + * [5] Suspend Refresh Type (SRT) + * 1 = Self refresh mode + * 0 = CBR fresh mode + * [4] Normal Refresh Enable (NREF_EN) + * 1 = Enable + * 0 = Disable + * [3] Quick Start Mode (QSTART) + * 1 = Quick start mode for the processor is enabled + * [2] Gated Clock Enable (GCLKEN) + * 1 = Enable + * 0 = Disable + * [1] AGP Disable (AGP_DIS) + * 1 = AGP disabled (Hardware strap) + * [0] CPU reset without PCIRST enable (CRst_En) + * 1 = Enable + * 0 = Disable */ /* PMCR will be set later. */
/* Enable SCRR.SRRAEN and let BX choose the SRR. */ - SCRR + 1, 0x00, 0x10, + SCRR + 1, 0x10, };
/*----------------------------------------------------------------------------- @@ -666,7 +654,6 @@ static void sdram_set_registers(void) { int i, max; - uint8_t reg;
PRINT_DEBUG("Northbridge prior to SDRAM init:\n"); DUMPNORTH(); @@ -674,12 +661,8 @@ max = ARRAY_SIZE(register_values);
/* Set registers as specified in the register_values[] array. */ - for (i = 0; i < max; i += 3) { - reg = pci_read_config8(NB, register_values[i]); - reg &= register_values[i + 1]; - reg |= register_values[i + 2] & ~(register_values[i + 1]); - pci_write_config8(NB, register_values[i], reg); - } + for (i = 0; i < max; i += 2) + pci_write_config8(NB, register_values[i], register_values[i + 1]); }
struct dimm_size {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40391 )
Change subject: nb/intel/i440bx: Clean up register_values table ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3118 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3117 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3116 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3115
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Keith Hui (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)