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/+/48402
to review the following change.
Change subject: nb/intel/sandybridge: Introduce `iosav_run_once_and_wait` ......................................................................
nb/intel/sandybridge: Introduce `iosav_run_once_and_wait`
Most ofte, `iosav_run_once` precedes a `wait_for_iosav` call. Add a helper function to reduce clutter. The cases where `iosav_run_once` isn't followed by `wait_for_iosav` will be handled in a follow-up.
Change-Id: Ic76f53c2db41512287f41b696a0c4df42a5e0f12 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_iosav.c 3 files changed, 24 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/48402/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index d459632..3e5d430 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -911,9 +911,7 @@
iosav_write_zqcs_sequence(channel, slotrank, 4, 101, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); } }
@@ -1053,9 +1051,7 @@ /* Send a burst of 16 back-to-back read commands (4 DCLK apart) */ iosav_write_read_mpr_sequence(channel, slotrank, ctrl->tMOD, 1, 3, 15, ctrl->CAS + 36);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static int does_lane_work(ramctr_timing *ctrl, int channel, int slotrank, int lane) @@ -1429,15 +1425,11 @@ iosav_write_misc_write_sequence(ctrl, channel, slotrank, MAX(ctrl->tRRD, (ctrl->tFAW >> 2) + 1), 4, 4, 500, 18);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
iosav_write_prea_act_read_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static void tx_dq_threshold_process(int *data, const int count) @@ -1591,9 +1583,7 @@ } program_timings(ctrl, channel);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
FOR_ALL_LANES { statistics[lane][tx_dqs] = !((MCHBAR32(lane_base[lane] + @@ -1671,9 +1661,7 @@
iosav_write_misc_write_sequence(ctrl, channel, slotrank, 3, 1, 3, 3, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
const struct iosav_ssq rd_sequence[] = { /* DRAM command PREA */ @@ -1741,9 +1729,8 @@ }; iosav_write_sequence(channel, rd_sequence, ARRAY_SIZE(rd_sequence));
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { u64 res = MCHBAR32(lane_base[lane] + GDCRTRAININGRESULT1(channel)); res |= ((u64) MCHBAR32(lane_base[lane] + @@ -1771,9 +1758,7 @@
iosav_write_zqcs_sequence(channel, slotrank, 4, 4, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
MCHBAR32_OR(SCHED_CBIT_ch(channel), 1 << 21); } @@ -1783,9 +1768,7 @@
FOR_ALL_POPULATED_CHANNELS { /* Execute the same command queue */ - iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); } }
@@ -1851,9 +1834,7 @@
iosav_write_zqcs_sequence(channel, 0, 4, 101, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
toggle_io_reset(); @@ -1928,9 +1909,8 @@ MCHBAR32(IOSAV_n_ADDRESS_LFSR_ch(channel, 1)) = 0x389abcd; MCHBAR32(IOSAV_n_ADDRESS_LFSR_ch(channel, 2)) = 0x389abcd;
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { u32 r32 = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane));
@@ -2141,9 +2121,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 500, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
FOR_ALL_LANES { stats[lane][dqs_pi] = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); @@ -2187,9 +2165,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
/* XXX: check any measured value ? */ @@ -2207,9 +2183,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
/* XXX: check any measured value ? */ @@ -2322,9 +2296,8 @@
iosav_write_data_write_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); } @@ -2419,9 +2392,7 @@
iosav_write_aggressive_write_read_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static void set_write_vref(const int channel, const u8 wr_vref) @@ -2600,9 +2571,8 @@
iosav_write_memory_test_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES if (MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane))) { printk(BIOS_EMERG, "Mini channel test failed (2): %d, %d, %d\n", diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h index 2dba494..3de29ff 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.h +++ b/src/northbridge/intel/sandybridge/raminit_common.h @@ -293,6 +293,7 @@ void iosav_run_queue(const int ch, const u8 loops, const u8 as_timer); void iosav_run_once(const int ch); void wait_for_iosav(int channel); +void iosav_run_once_and_wait(const int ch);
void iosav_write_zqcs_sequence(int channel, int slotrank, u32 gap, u32 post, u32 wrap); void iosav_write_prea_sequence(int channel, int slotrank, u32 post, u32 wrap); diff --git a/src/northbridge/intel/sandybridge/raminit_iosav.c b/src/northbridge/intel/sandybridge/raminit_iosav.c index d83dfd8..b3c6db6 100644 --- a/src/northbridge/intel/sandybridge/raminit_iosav.c +++ b/src/northbridge/intel/sandybridge/raminit_iosav.c @@ -49,6 +49,12 @@ } }
+void iosav_run_once_and_wait(const int ch) +{ + iosav_run_once(ch); + wait_for_iosav(ch); +} + void iosav_write_zqcs_sequence(int channel, int slotrank, u32 gap, u32 post, u32 wrap) { const struct iosav_ssq sequence[] = {
Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Arthur Heymans, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48402
to look at the new patch set (#4).
Change subject: nb/intel/sandybridge: Introduce `iosav_run_once_and_wait` ......................................................................
nb/intel/sandybridge: Introduce `iosav_run_once_and_wait`
Most ofte, `iosav_run_once` precedes a `wait_for_iosav` call. Add a helper function to reduce clutter. The cases where `iosav_run_once` isn't followed by `wait_for_iosav` will be handled in a follow-up.
Tested on Asus P8Z77-V LX2, still boots.
Change-Id: Ic76f53c2db41512287f41b696a0c4df42a5e0f12 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_iosav.c 3 files changed, 24 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/48402/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48402 )
Change subject: nb/intel/sandybridge: Introduce `iosav_run_once_and_wait` ......................................................................
Patch Set 7: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48402 )
Change subject: nb/intel/sandybridge: Introduce `iosav_run_once_and_wait` ......................................................................
nb/intel/sandybridge: Introduce `iosav_run_once_and_wait`
Most ofte, `iosav_run_once` precedes a `wait_for_iosav` call. Add a helper function to reduce clutter. The cases where `iosav_run_once` isn't followed by `wait_for_iosav` will be handled in a follow-up.
Tested on Asus P8Z77-V LX2, still boots.
Change-Id: Ic76f53c2db41512287f41b696a0c4df42a5e0f12 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48402 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_iosav.c 3 files changed, 24 insertions(+), 47 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 289398a..9308ed5 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -910,9 +910,7 @@
iosav_write_zqcs_sequence(channel, slotrank, 4, 101, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); } }
@@ -1052,9 +1050,7 @@ /* Send a burst of 16 back-to-back read commands (4 DCLK apart) */ iosav_write_read_mpr_sequence(channel, slotrank, ctrl->tMOD, 1, 3, 15, ctrl->CAS + 36);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static int does_lane_work(ramctr_timing *ctrl, int channel, int slotrank, int lane) @@ -1428,15 +1424,11 @@ iosav_write_misc_write_sequence(ctrl, channel, slotrank, MAX(ctrl->tRRD, (ctrl->tFAW >> 2) + 1), 4, 4, 500, 18);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
iosav_write_prea_act_read_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static void tx_dq_threshold_process(int *data, const int count) @@ -1590,9 +1582,7 @@ } program_timings(ctrl, channel);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
FOR_ALL_LANES { statistics[lane][tx_dqs] = !((MCHBAR32(lane_base[lane] + @@ -1670,9 +1660,7 @@
iosav_write_misc_write_sequence(ctrl, channel, slotrank, 3, 1, 3, 3, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
const struct iosav_ssq rd_sequence[] = { /* DRAM command PREA */ @@ -1740,9 +1728,8 @@ }; iosav_write_sequence(channel, rd_sequence, ARRAY_SIZE(rd_sequence));
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { u64 res = MCHBAR32(lane_base[lane] + GDCRTRAININGRESULT1(channel)); res |= ((u64) MCHBAR32(lane_base[lane] + @@ -1770,9 +1757,7 @@
iosav_write_zqcs_sequence(channel, slotrank, 4, 4, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
MCHBAR32_OR(SCHED_CBIT_ch(channel), 1 << 21); } @@ -1782,9 +1767,7 @@
FOR_ALL_POPULATED_CHANNELS { /* Execute the same command queue */ - iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); } }
@@ -1850,9 +1833,7 @@
iosav_write_zqcs_sequence(channel, 0, 4, 101, 31);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
toggle_io_reset(); @@ -1931,9 +1912,8 @@ MCHBAR32(IOSAV_n_ADDRESS_LFSR_ch(channel, 1)) = 0x389abcd; MCHBAR32(IOSAV_n_ADDRESS_LFSR_ch(channel, 2)) = 0x389abcd;
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { u32 r32 = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane));
@@ -2144,9 +2124,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 500, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel);
FOR_ALL_LANES { stats[lane][dqs_pi] = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); @@ -2190,9 +2168,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
/* XXX: check any measured value ? */ @@ -2210,9 +2186,7 @@ iosav_write_read_mpr_sequence( channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
/* XXX: check any measured value ? */ @@ -2325,9 +2299,8 @@
iosav_write_data_write_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES { MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); } @@ -2422,9 +2395,7 @@
iosav_write_aggressive_write_read_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); - - wait_for_iosav(channel); + iosav_run_once_and_wait(channel); }
static void set_write_vref(const int channel, const u8 wr_vref) @@ -2603,9 +2574,8 @@
iosav_write_memory_test_sequence(ctrl, channel, slotrank);
- iosav_run_once(channel); + iosav_run_once_and_wait(channel);
- wait_for_iosav(channel); FOR_ALL_LANES if (MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane))) { printk(BIOS_EMERG, "Mini channel test failed (2): %d, %d, %d\n", diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h index 2dba494..3de29ff 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.h +++ b/src/northbridge/intel/sandybridge/raminit_common.h @@ -293,6 +293,7 @@ void iosav_run_queue(const int ch, const u8 loops, const u8 as_timer); void iosav_run_once(const int ch); void wait_for_iosav(int channel); +void iosav_run_once_and_wait(const int ch);
void iosav_write_zqcs_sequence(int channel, int slotrank, u32 gap, u32 post, u32 wrap); void iosav_write_prea_sequence(int channel, int slotrank, u32 post, u32 wrap); diff --git a/src/northbridge/intel/sandybridge/raminit_iosav.c b/src/northbridge/intel/sandybridge/raminit_iosav.c index d83dfd8..b3c6db6 100644 --- a/src/northbridge/intel/sandybridge/raminit_iosav.c +++ b/src/northbridge/intel/sandybridge/raminit_iosav.c @@ -49,6 +49,12 @@ } }
+void iosav_run_once_and_wait(const int ch) +{ + iosav_run_once(ch); + wait_for_iosav(ch); +} + void iosav_write_zqcs_sequence(int channel, int slotrank, u32 gap, u32 post, u32 wrap) { const struct iosav_ssq sequence[] = {
-
Angel Pons (Code Review) -
Felix Held (Code Review)