Hello Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48615
to review the following change.
Change subject: nb/intel/sandybridge: Rewrite constant values ......................................................................
nb/intel/sandybridge: Rewrite constant values
Rewrite some constants to make their meaning somewhat clearer.
Tested with BUILD_TIMELESS=1, Asus P8Z77-V LX2 does not change.
Change-Id: I321f5e61d7c695ae77e61b84728e34930f69d400 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_iosav.c M src/northbridge/intel/sandybridge/raminit_native.c 4 files changed, 33 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/48615/1
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 8acfb98..1d4354c 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -309,7 +309,7 @@
wait_txt_clear();
- wrmsr(0x000002e6, (msr_t) { .lo = 0, .hi = 0 }); + wrmsr(0x2e6, (msr_t) { .lo = 0, .hi = 0 });
const u32 sskpd = MCHBAR32(SSKPD); // !!! = 0x00000000 if ((pci_read_config16(SOUTHBRIDGE, 0xa2) & 0xa0) == 0x20 && sskpd && !s3resume) { diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index ccf2403..0b8e5ef 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -44,7 +44,7 @@ u32 reg;
/* Enable xover cmd */ - reg = 0x4000; + reg = 1 << 14;
/* Enable xover ctl */ if (rankmap & 0x03) @@ -877,7 +877,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = 0, @@ -1145,7 +1145,7 @@
FOR_ALL_LANES { ctrl->timings[channel][slotrank].lanes[lane].rcven - = upperA[lane] + rcven_delta + 0x40; + = upperA[lane] + rcven_delta + 64; } program_timings(ctrl, channel);
@@ -1234,7 +1234,7 @@ printram("4028 += 2;\n");
/* Guard against I/O latency overflow */ - if (ctrl->timings[channel][slotrank].io_latency >= 0x10) { + if (ctrl->timings[channel][slotrank].io_latency >= 16) { printk(BIOS_EMERG, "I/O latency overflow: %d, %d\n", channel, slotrank); return MAKE_ERR; @@ -1350,7 +1350,7 @@ all_high = 1; some_high = 0; FOR_ALL_LANES { - if (ctrl->timings[channel][slotrank].lanes[lane].rcven >= 0x40) + if (ctrl->timings[channel][slotrank].lanes[lane].rcven >= 64) some_high = 1; else all_high = 0; @@ -1360,8 +1360,8 @@ ctrl->timings[channel][slotrank].io_latency--; printram("4028--;\n"); FOR_ALL_LANES { - ctrl->timings[channel][slotrank].lanes[lane].rcven -= 0x40; - upperA[lane] -= 0x40; + ctrl->timings[channel][slotrank].lanes[lane].rcven -= 64; + upperA[lane] -= 64;
} } else if (some_high) { @@ -1516,7 +1516,7 @@ static void fill_pattern0(ramctr_timing *ctrl, int channel, u32 a, u32 b) { unsigned int j; - unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 0x40; + unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 64;
for (j = 0; j < 16; j++) write32((void *)(0x04000000 + channel_offset + 4 * j), j & 2 ? b : a); @@ -1537,8 +1537,8 @@ static void fill_pattern1(ramctr_timing *ctrl, int channel) { unsigned int j; - unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 0x40; - unsigned int channel_step = 0x40 * num_of_channels(ctrl); + unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 64; + unsigned int channel_step = 64 * num_of_channels(ctrl);
for (j = 0; j < 16; j++) write32((void *)(0x04000000 + channel_offset + j * 4), 0xffffffff); @@ -1676,7 +1676,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -1933,8 +1933,8 @@ static void fill_pattern5(ramctr_timing *ctrl, int channel, int patno) { unsigned int i, j; - unsigned int offset = get_precedening_channels(ctrl, channel) * 0x40; - unsigned int step = 0x40 * num_of_channels(ctrl); + unsigned int offset = get_precedening_channels(ctrl, channel) * 64; + unsigned int step = 64 * num_of_channels(ctrl);
if (patno) { u8 base8 = 0x80 >> ((patno - 1) % 8); @@ -2218,8 +2218,8 @@ * FIXME: Under some conditions, vendor BIOS sets both edges to the same value. It will * also use a single loop. It would seem that it is a debugging configuration. */ - MCHBAR32(IOSAV_DC_MASK) = 0x300; - printram("discover falling edges:\n[%x] = %x\n", IOSAV_DC_MASK, 0x300); + MCHBAR32(IOSAV_DC_MASK) = 3 << 8; + printram("discover falling edges:\n[%x] = %x\n", IOSAV_DC_MASK, 3 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_read_mpr_margin(ctrl, channel, slotrank, @@ -2228,8 +2228,8 @@ return err; }
- MCHBAR32(IOSAV_DC_MASK) = 0x200; - printram("discover rising edges:\n[%x] = %x\n", IOSAV_DC_MASK, 0x200); + MCHBAR32(IOSAV_DC_MASK) = 2 << 8; + printram("discover rising edges:\n[%x] = %x\n", IOSAV_DC_MASK, 2 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_read_mpr_margin(ctrl, channel, slotrank, @@ -2353,8 +2353,8 @@ * FIXME: Under some conditions, vendor BIOS sets both edges to the same value. It will * also use a single loop. It would seem that it is a debugging configuration. */ - MCHBAR32(IOSAV_DC_MASK) = 0x300; - printram("discover falling edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 0x300); + MCHBAR32(IOSAV_DC_MASK) = 3 << 8; + printram("discover falling edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 3 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_agrsv_read_margin(ctrl, channel, slotrank, @@ -2363,8 +2363,8 @@ return err; }
- MCHBAR32(IOSAV_DC_MASK) = 0x200; - printram("discover rising edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 0x200); + MCHBAR32(IOSAV_DC_MASK) = 2 << 8; + printram("discover rising edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 2 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_agrsv_read_margin(ctrl, channel, slotrank, @@ -2971,7 +2971,7 @@ MCHBAR32(GDCRTRAININGMOD_ch(0)) = 0;
FOR_ALL_CHANNELS { - MCHBAR32_AND(GDCRCMDDEBUGMUXCFG_Cz_S(channel), ~0x3f000000); + MCHBAR32_AND(GDCRCMDDEBUGMUXCFG_Cz_S(channel), ~(0x3f << 24)); udelay(2); } } diff --git a/src/northbridge/intel/sandybridge/raminit_iosav.c b/src/northbridge/intel/sandybridge/raminit_iosav.c index cb49e0c..86b3baa 100644 --- a/src/northbridge/intel/sandybridge/raminit_iosav.c +++ b/src/northbridge/intel/sandybridge/raminit_iosav.c @@ -94,7 +94,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -216,7 +216,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -284,7 +284,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -572,7 +572,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -670,7 +670,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -765,7 +765,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -860,7 +860,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 3613c05..0d581f5 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -577,13 +577,13 @@ /* Step 2 - Select frequency in the MCU */ reg1 = ctrl->FRQ; if (ctrl->base_freq == 100) - reg1 |= 0x100; /* Enable 100Mhz REF clock */ + reg1 |= (1 << 8); /* Enable 100Mhz REF clock */
- reg1 |= 0x80000000; /* set running bit */ + reg1 |= (1 << 31); /* set running bit */ MCHBAR32(MC_BIOS_REQ) = reg1; int i = 0; printk(BIOS_DEBUG, "PLL busy... "); - while (reg1 & 0x80000000) { + while (reg1 & (1 << 31)) { udelay(10); i++; reg1 = MCHBAR32(MC_BIOS_REQ);
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48615 )
Change subject: nb/intel/sandybridge: Rewrite constant values ......................................................................
Patch Set 1: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48615 )
Change subject: nb/intel/sandybridge: Rewrite constant values ......................................................................
nb/intel/sandybridge: Rewrite constant values
Rewrite some constants to make their meaning somewhat clearer.
Tested with BUILD_TIMELESS=1, Asus P8Z77-V LX2 does not change.
Change-Id: I321f5e61d7c695ae77e61b84728e34930f69d400 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48615 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_iosav.c M src/northbridge/intel/sandybridge/raminit_native.c 4 files changed, 33 insertions(+), 33 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 8acfb98..1d4354c 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -309,7 +309,7 @@
wait_txt_clear();
- wrmsr(0x000002e6, (msr_t) { .lo = 0, .hi = 0 }); + wrmsr(0x2e6, (msr_t) { .lo = 0, .hi = 0 });
const u32 sskpd = MCHBAR32(SSKPD); // !!! = 0x00000000 if ((pci_read_config16(SOUTHBRIDGE, 0xa2) & 0xa0) == 0x20 && sskpd && !s3resume) { diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index ccf2403..0b8e5ef 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -44,7 +44,7 @@ u32 reg;
/* Enable xover cmd */ - reg = 0x4000; + reg = 1 << 14;
/* Enable xover ctl */ if (rankmap & 0x03) @@ -877,7 +877,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = 0, @@ -1145,7 +1145,7 @@
FOR_ALL_LANES { ctrl->timings[channel][slotrank].lanes[lane].rcven - = upperA[lane] + rcven_delta + 0x40; + = upperA[lane] + rcven_delta + 64; } program_timings(ctrl, channel);
@@ -1234,7 +1234,7 @@ printram("4028 += 2;\n");
/* Guard against I/O latency overflow */ - if (ctrl->timings[channel][slotrank].io_latency >= 0x10) { + if (ctrl->timings[channel][slotrank].io_latency >= 16) { printk(BIOS_EMERG, "I/O latency overflow: %d, %d\n", channel, slotrank); return MAKE_ERR; @@ -1350,7 +1350,7 @@ all_high = 1; some_high = 0; FOR_ALL_LANES { - if (ctrl->timings[channel][slotrank].lanes[lane].rcven >= 0x40) + if (ctrl->timings[channel][slotrank].lanes[lane].rcven >= 64) some_high = 1; else all_high = 0; @@ -1360,8 +1360,8 @@ ctrl->timings[channel][slotrank].io_latency--; printram("4028--;\n"); FOR_ALL_LANES { - ctrl->timings[channel][slotrank].lanes[lane].rcven -= 0x40; - upperA[lane] -= 0x40; + ctrl->timings[channel][slotrank].lanes[lane].rcven -= 64; + upperA[lane] -= 64;
} } else if (some_high) { @@ -1516,7 +1516,7 @@ static void fill_pattern0(ramctr_timing *ctrl, int channel, u32 a, u32 b) { unsigned int j; - unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 0x40; + unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 64;
for (j = 0; j < 16; j++) write32((void *)(0x04000000 + channel_offset + 4 * j), j & 2 ? b : a); @@ -1537,8 +1537,8 @@ static void fill_pattern1(ramctr_timing *ctrl, int channel) { unsigned int j; - unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 0x40; - unsigned int channel_step = 0x40 * num_of_channels(ctrl); + unsigned int channel_offset = get_precedening_channels(ctrl, channel) * 64; + unsigned int channel_step = 64 * num_of_channels(ctrl);
for (j = 0; j < 16; j++) write32((void *)(0x04000000 + channel_offset + j * 4), 0xffffffff); @@ -1676,7 +1676,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -1933,8 +1933,8 @@ static void fill_pattern5(ramctr_timing *ctrl, int channel, int patno) { unsigned int i, j; - unsigned int offset = get_precedening_channels(ctrl, channel) * 0x40; - unsigned int step = 0x40 * num_of_channels(ctrl); + unsigned int offset = get_precedening_channels(ctrl, channel) * 64; + unsigned int step = 64 * num_of_channels(ctrl);
if (patno) { u8 base8 = 0x80 >> ((patno - 1) % 8); @@ -2218,8 +2218,8 @@ * FIXME: Under some conditions, vendor BIOS sets both edges to the same value. It will * also use a single loop. It would seem that it is a debugging configuration. */ - MCHBAR32(IOSAV_DC_MASK) = 0x300; - printram("discover falling edges:\n[%x] = %x\n", IOSAV_DC_MASK, 0x300); + MCHBAR32(IOSAV_DC_MASK) = 3 << 8; + printram("discover falling edges:\n[%x] = %x\n", IOSAV_DC_MASK, 3 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_read_mpr_margin(ctrl, channel, slotrank, @@ -2228,8 +2228,8 @@ return err; }
- MCHBAR32(IOSAV_DC_MASK) = 0x200; - printram("discover rising edges:\n[%x] = %x\n", IOSAV_DC_MASK, 0x200); + MCHBAR32(IOSAV_DC_MASK) = 2 << 8; + printram("discover rising edges:\n[%x] = %x\n", IOSAV_DC_MASK, 2 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_read_mpr_margin(ctrl, channel, slotrank, @@ -2353,8 +2353,8 @@ * FIXME: Under some conditions, vendor BIOS sets both edges to the same value. It will * also use a single loop. It would seem that it is a debugging configuration. */ - MCHBAR32(IOSAV_DC_MASK) = 0x300; - printram("discover falling edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 0x300); + MCHBAR32(IOSAV_DC_MASK) = 3 << 8; + printram("discover falling edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 3 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_agrsv_read_margin(ctrl, channel, slotrank, @@ -2363,8 +2363,8 @@ return err; }
- MCHBAR32(IOSAV_DC_MASK) = 0x200; - printram("discover rising edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 0x200); + MCHBAR32(IOSAV_DC_MASK) = 2 << 8; + printram("discover rising edges aggressive:\n[%x] = %x\n", IOSAV_DC_MASK, 2 << 8);
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { err = find_agrsv_read_margin(ctrl, channel, slotrank, @@ -2971,7 +2971,7 @@ MCHBAR32(GDCRTRAININGMOD_ch(0)) = 0;
FOR_ALL_CHANNELS { - MCHBAR32_AND(GDCRCMDDEBUGMUXCFG_Cz_S(channel), ~0x3f000000); + MCHBAR32_AND(GDCRCMDDEBUGMUXCFG_Cz_S(channel), ~(0x3f << 24)); udelay(2); } } diff --git a/src/northbridge/intel/sandybridge/raminit_iosav.c b/src/northbridge/intel/sandybridge/raminit_iosav.c index cb49e0c..86b3baa 100644 --- a/src/northbridge/intel/sandybridge/raminit_iosav.c +++ b/src/northbridge/intel/sandybridge/raminit_iosav.c @@ -94,7 +94,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -216,7 +216,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -284,7 +284,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -572,7 +572,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -670,7 +670,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -765,7 +765,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, @@ -860,7 +860,7 @@ .data_direction = SSQ_NA, }, .sp_cmd_addr = { - .address = 1024, + .address = 1 << 10, .rowbits = 6, .bank = 0, .rank = slotrank, diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 3613c05..0d581f5 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -577,13 +577,13 @@ /* Step 2 - Select frequency in the MCU */ reg1 = ctrl->FRQ; if (ctrl->base_freq == 100) - reg1 |= 0x100; /* Enable 100Mhz REF clock */ + reg1 |= (1 << 8); /* Enable 100Mhz REF clock */
- reg1 |= 0x80000000; /* set running bit */ + reg1 |= (1 << 31); /* set running bit */ MCHBAR32(MC_BIOS_REQ) = reg1; int i = 0; printk(BIOS_DEBUG, "PLL busy... "); - while (reg1 & 0x80000000) { + while (reg1 & (1 << 31)) { udelay(10); i++; reg1 = MCHBAR32(MC_BIOS_REQ);
-
Angel Pons (Code Review) -
Felix Held (Code Review)