coreboot-gerrit November 2020

coreboot-gerrit@coreboot.org

1 participants
3243 discussions

Change in coreboot[master]: nb/intel/sandybridge: Restore nominal Vref for current channel
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47680 ) Change subject: nb/intel/sandybridge: Restore nominal Vref for current channel ...................................................................... nb/intel/sandybridge: Restore nominal Vref for current channel After aggressive read training, program nominal Vref for the current channel, not only channel 0. This simple mistake can easily degrade memory margins, especially when running at high speed (overclocking). Tested on Asus P8H61-M PRO, still boots. Change-Id: I12630fe33c5c786c8ec131c45c27180c3887d354 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/47680/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index d60271b..7e93786 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -2368,7 +2368,8 @@ } } - MCHBAR32(GDCRTRAININGMOD_ch(0)) = 0; + /* Restore nominal Vref after training */ + MCHBAR32(GDCRTRAININGMOD_ch(channel)) = 0; printram("CPA\n"); return 0; } -- To view, visit https://review.coreboot.org/c/coreboot/+/47680 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I12630fe33c5c786c8ec131c45c27180c3887d354 Gerrit-Change-Number: 47680 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 3

Change in coreboot[master]: nb/intel/sandybridge: Rename `timC_discovery` and related
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47625 ) Change subject: nb/intel/sandybridge: Rename `timC_discovery` and related ...................................................................... nb/intel/sandybridge: Rename `timC_discovery` and related This function simply determines the best delay for the TX DQ PIs. Change-Id: If44c4f661d8c81fe41532ce2bfe3718392b9fe94 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 10 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/47625/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 664fef2..c495a8f 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1427,7 +1427,7 @@ return 0; } -static void test_timC(ramctr_timing *ctrl, int channel, int slotrank) +static void test_tx_dq(ramctr_timing *ctrl, int channel, int slotrank) { int lane; @@ -1454,7 +1454,7 @@ wait_for_iosav(channel); } -static void timC_threshold_process(int *data, const int count) +static void tx_dq_threshold_process(int *data, const int count) { int min = data[0]; int max = min; @@ -1473,9 +1473,9 @@ printram("threshold=%d min=%d max=%d\n", threshold, min, max); } -static int discover_timC(ramctr_timing *ctrl, int channel, int slotrank) +static int tx_dq_write_leveling(ramctr_timing *ctrl, int channel, int slotrank) { - int timC; + int tx_dq; int stats[NUM_LANES][MAX_TIMC + 1]; int lane; @@ -1486,14 +1486,14 @@ /* Execute command queue */ iosav_run_once(channel); - for (timC = 0; timC <= MAX_TIMC; timC++) { - FOR_ALL_LANES ctrl->timings[channel][slotrank].lanes[lane].timC = timC; + for (tx_dq = 0; tx_dq <= MAX_TIMC; tx_dq++) { + FOR_ALL_LANES ctrl->timings[channel][slotrank].lanes[lane].timC = tx_dq; program_timings(ctrl, channel); - test_timC(ctrl, channel, slotrank); + test_tx_dq(ctrl, channel, slotrank); FOR_ALL_LANES { - stats[lane][timC] = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); + stats[lane][tx_dq] = MCHBAR32(IOSAV_By_ERROR_COUNT_ch(channel, lane)); } } FOR_ALL_LANES { @@ -1506,7 +1506,7 @@ * With command training not being done yet, the lane can be erroneous. * Take the average as reference and try again to find a run. */ - timC_threshold_process(stats[lane], ARRAY_SIZE(stats[lane])); + tx_dq_threshold_process(stats[lane], ARRAY_SIZE(stats[lane])); rn = get_longest_zero_run(stats[lane], ARRAY_SIZE(stats[lane])); if (rn.all || rn.length < 8) { @@ -1903,7 +1903,7 @@ } FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { - err = discover_timC(ctrl, channel, slotrank); + err = tx_dq_write_leveling(ctrl, channel, slotrank); if (err) return err; } -- To view, visit https://review.coreboot.org/c/coreboot/+/47625 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If44c4f661d8c81fe41532ce2bfe3718392b9fe94 Gerrit-Change-Number: 47625 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: nb/intel/sandybridge: Introduce `ddr3_mirror_mrreg` helper
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47613 ) Change subject: nb/intel/sandybridge: Introduce `ddr3_mirror_mrreg` helper ...................................................................... nb/intel/sandybridge: Introduce `ddr3_mirror_mrreg` helper Write training needs to update mode register 1, but `write_mrreg` will clobber the IOSAV sequence. Reference code uses one four-subsequence to unset Qoff in MR1, run the test, and finally set Qoff again. This will be implemented in future changes, and will use the newly-added helper. Change-Id: I06a06a7bdd43dbde34af4ea2f90e00873eefe599 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 12 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/47613/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 7257674..60ce50b 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -608,17 +608,22 @@ } } +/* + * DDR3 Rank1 Address mirror swap the following pins: + * A3<->A4, A5<->A6, A7<->A8, BA0<->BA1 + */ +static void ddr3_mirror_mrreg(int *bank, u32 *addr) +{ + *bank = ((*bank >> 1) & 1) | ((*bank << 1) & 2); + *addr = (*addr & ~0x1f8) | ((*addr >> 1) & 0xa8) | ((*addr & 0xa8) << 1); +} + static void write_mrreg(ramctr_timing *ctrl, int channel, int slotrank, int reg, u32 val) { wait_for_iosav(channel); - if (ctrl->rank_mirror[channel][slotrank]) { - /* DDR3 Rank1 Address mirror - swap the following pins: - A3<->A4, A5<->A6, A7<->A8, BA0<->BA1 */ - reg = ((reg >> 1) & 1) | ((reg << 1) & 2); - val = (val & ~0x1f8) | ((val >> 1) & 0xa8) | ((val & 0xa8) << 1); - } + if (ctrl->rank_mirror[channel][slotrank]) + ddr3_mirror_mrreg(&reg, &val); const struct iosav_ssq sequence[] = { /* DRAM command MRS */ -- To view, visit https://review.coreboot.org/c/coreboot/+/47613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I06a06a7bdd43dbde34af4ea2f90e00873eefe599 Gerrit-Change-Number: 47613 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: nb/intel/sandybridge: Replace and-zero with assignment
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47608 ) Change subject: nb/intel/sandybridge: Replace and-zero with assignment ...................................................................... nb/intel/sandybridge: Replace and-zero with assignment The intent here is to clear the register, so a simple write will work. Change-Id: I547805059e911942ac2cac7bd2165af23d926a2b Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/47608/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 878dc4b..77c0173 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -2064,7 +2064,7 @@ printram("CPF\n"); FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { - MCHBAR32_AND(IOSAV_By_BW_MASK_ch(channel, lane), 0); + MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = 0; } FOR_ALL_POPULATED_CHANNELS { @@ -2087,7 +2087,7 @@ program_timings(ctrl, channel); FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { - MCHBAR32_AND(IOSAV_By_BW_MASK_ch(channel, lane), 0); + MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = 0; } return 0; } -- To view, visit https://review.coreboot.org/c/coreboot/+/47608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I547805059e911942ac2cac7bd2165af23d926a2b Gerrit-Change-Number: 47608 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: nb/intel/sandybridge: Introduce `find_predefined_pattern` function
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47607 ) Change subject: nb/intel/sandybridge: Introduce `find_predefined_pattern` function ...................................................................... nb/intel/sandybridge: Introduce `find_predefined_pattern` function Also fuse two per-channel loops together. Tested on Asus P8H61-M PRO, still boots. Change-Id: Iacc66f4364290a66d60d483055abef6e98223d16 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 59 insertions(+), 52 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/47607/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index e740f55..878dc4b 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -2390,6 +2390,63 @@ return 0; } +static void find_predefined_pattern(ramctr_timing *ctrl, const int channel) +{ + int slotrank, lane; + + fill_pattern0(ctrl, channel, 0, 0); + FOR_ALL_LANES { + MCHBAR32(IOSAV_By_BW_SERROR_C_ch(channel, lane)); + } + + FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { + ctrl->timings[channel][slotrank].lanes[lane].falling = 16; + ctrl->timings[channel][slotrank].lanes[lane].rising = 16; + } + + program_timings(ctrl, channel); + + FOR_ALL_POPULATED_RANKS { + wait_for_iosav(channel); + + iosav_write_read_mpr_sequence( + channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8); + + /* Execute command queue */ + iosav_run_once(channel); + + wait_for_iosav(channel); + } + + /* XXX: check any measured value ? */ + + FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { + ctrl->timings[channel][slotrank].lanes[lane].falling = 48; + ctrl->timings[channel][slotrank].lanes[lane].rising = 48; + } + + program_timings(ctrl, channel); + + FOR_ALL_POPULATED_RANKS { + wait_for_iosav(channel); + + iosav_write_read_mpr_sequence( + channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8); + + /* Execute command queue */ + iosav_run_once(channel); + + wait_for_iosav(channel); + } + + /* XXX: check any measured value ? */ + + FOR_ALL_LANES { + MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = + ~MCHBAR32(IOSAV_By_BW_SERROR_ch(channel, lane)) & 0xff; + } +} + int discover_edges(ramctr_timing *ctrl) { int falling_edges[NUM_CHANNELS][NUM_SLOTRANKS][NUM_LANES]; @@ -2401,62 +2458,12 @@ toggle_io_reset(); - FOR_ALL_POPULATED_CHANNELS FOR_ALL_LANES { - MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = 0; - } - FOR_ALL_POPULATED_CHANNELS { - fill_pattern0(ctrl, channel, 0, 0); FOR_ALL_LANES { - MCHBAR32(IOSAV_By_BW_SERROR_C_ch(channel, lane)); + MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = 0; } - FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { - ctrl->timings[channel][slotrank].lanes[lane].falling = 16; - ctrl->timings[channel][slotrank].lanes[lane].rising = 16; - } - - program_timings(ctrl, channel); - - FOR_ALL_POPULATED_RANKS { - wait_for_iosav(channel); - - iosav_write_read_mpr_sequence( - channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8); - - /* Execute command queue */ - iosav_run_once(channel); - - wait_for_iosav(channel); - } - - /* XXX: check any measured value ? */ - - FOR_ALL_POPULATED_RANKS FOR_ALL_LANES { - ctrl->timings[channel][slotrank].lanes[lane].falling = 48; - ctrl->timings[channel][slotrank].lanes[lane].rising = 48; - } - - program_timings(ctrl, channel); - - FOR_ALL_POPULATED_RANKS { - wait_for_iosav(channel); - - iosav_write_read_mpr_sequence( - channel, slotrank, ctrl->tMOD, 3, 4, 1, ctrl->CAS + 8); - - /* Execute command queue */ - iosav_run_once(channel); - - wait_for_iosav(channel); - } - - /* XXX: check any measured value ? */ - - FOR_ALL_LANES { - MCHBAR32(IOSAV_By_BW_MASK_ch(channel, lane)) = - ~MCHBAR32(IOSAV_By_BW_SERROR_ch(channel, lane)) & 0xff; - } + find_predefined_pattern(ctrl, channel); fill_pattern0(ctrl, channel, 0, 0xffffffff); } -- To view, visit https://review.coreboot.org/c/coreboot/+/47607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iacc66f4364290a66d60d483055abef6e98223d16 Gerrit-Change-Number: 47607 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: nb/intel/sandybridge: Rename receive enable functions
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47605 ) Change subject: nb/intel/sandybridge: Rename receive enable functions ...................................................................... nb/intel/sandybridge: Rename receive enable functions Give these functions more meaningful names. Change-Id: I6b308120d4185a3bc448213a925d5cee0d4d8bd9 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/47605/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 80657cc..257eae4 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1116,7 +1116,7 @@ return ret; } -static void discover_timA_coarse(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) +static void find_rcven_pi_coarse(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) { int timA; int statistics[NUM_LANES][128]; @@ -1146,7 +1146,7 @@ } } -static void discover_timA_fine(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) +static void fine_tune_rcven_pi(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) { int timA_delta; int statistics[NUM_LANES][51]; @@ -1192,7 +1192,7 @@ } } -static int discover_402x(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) +static int find_roundtrip_latency(ramctr_timing *ctrl, int channel, int slotrank, int *upperA) { int works[NUM_LANES]; int lane; @@ -1357,7 +1357,7 @@ ctrl->timings[channel][slotrank].roundtrip_latency = 55; program_timings(ctrl, channel); - discover_timA_coarse(ctrl, channel, slotrank, upperA); + find_rcven_pi_coarse(ctrl, channel, slotrank, upperA); all_high = 1; some_high = 0; @@ -1387,13 +1387,13 @@ prev = get_logic_delay_delta(ctrl, channel, slotrank); - err = discover_402x(ctrl, channel, slotrank, upperA); + err = find_roundtrip_latency(ctrl, channel, slotrank, upperA); if (err) return err; prev = align_rt_io_latency(ctrl, channel, slotrank, prev); - discover_timA_fine(ctrl, channel, slotrank, upperA); + fine_tune_rcven_pi(ctrl, channel, slotrank, upperA); prev = align_rt_io_latency(ctrl, channel, slotrank, prev); -- To view, visit https://review.coreboot.org/c/coreboot/+/47605 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b308120d4185a3bc448213a925d5cee0d4d8bd9 Gerrit-Change-Number: 47605 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: soc/amd/picasso: Generate ACPI CRAT objects in cb
by Marshall Dawson (Code Review) 20 Nov '20

20 Nov '20

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47727 ) Change subject: soc/amd/picasso: Generate ACPI CRAT objects in cb ...................................................................... Patch Set 3: (10 comments) I'll go ahead and give you some stuff to look over. I probably won't be able to finish agesa_acpi.c this afternoon. https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi.h File src/include/acpi/acpi.h: https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi.h@311 PS3, Line 311: struct acpi_crat_header Maybe typedef this, like on line 303. And to be consistent, I'd name it acpi_crat and the typedef to acpi_crat_t. https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat… File src/include/acpi/acpi_crat.h: https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat… PS3, Line 57: #define CRAT_PICASSO_NUM_DRAM_REG 2 This looks like it doesn't belong in a src/include file https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat… PS3, Line 121: = 0 I didn't see that this needs to be initialized. If I missed it, then this needs a comment. https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h: https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h@… PS3, Line 12: This change adds a whole lot of cpuid info into a file called msr.h. We should probably start to make an amd/cpuid.h file. https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h@… PS3, Line 14: /* EAX Leaf */ The term "leaf" may be more Intel than AMD. I'd recommend using something similar to what's in the BKDG or MSR, e.g. Fn80000005_EAX https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… File src/soc/amd/picasso/agesa_acpi.c: https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… PS3, Line 499: Please split the picasso file out of the generic crat patch, and make it follow the other. https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… PS3, Line 791: tlb_type It always feels a bit odd having a variable/argument that's the same name as its type. https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… PS3, Line 1016: &= ~ It's not clear why we would clear an enable bit. And we just cleared the mem above. Here plus line 1031 too. https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… PS3, Line 1048: num_nodes++ Was this ever initialized? Or was it initialized to 0 by zeroing the memory in acpi.c. I never found a spec, but I can see how you're following AGESA. What's the meaning of the field, and should we be setting it instead of bumping it? https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_… PS3, Line 1064: Meh I'd remove the blank line to make the code similar to below -- To view, visit https://review.coreboot.org/c/coreboot/+/47727 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If64fd624597b2ced014ba7f0332a6a48143c0e8c Gerrit-Change-Number: 47727 Gerrit-PatchSet: 3 Gerrit-Owner: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Reviewer: Justin Frodsham <justin.frodsham(a)protonmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Reviewer: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 20 Nov 2020 00:31:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: nb/intel/sandybridge: Rework timA minmax code
by Angel Pons (Code Review) 20 Nov '20

20 Nov '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47604 ) Change subject: nb/intel/sandybridge: Rework timA minmax code ...................................................................... nb/intel/sandybridge: Rework timA minmax code There's no need to use `struct timA_minmax`, since most cases only care about the difference between logic delay deltas. The final step does use the minimum logic delay across all lanes, but it's a special case. Tested on Asus P8H61-M PRO, still boots. Change-Id: I1da95520ac915ab003e1a839685cbf5f1970eb6a Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 60 insertions(+), 45 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/47604/1 diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index f20181e..80657cc 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <assert.h> #include <commonlib/helpers.h> #include <console/console.h> #include <string.h> @@ -1240,53 +1241,74 @@ return 0; } -struct timA_minmax { - int timA_min_high, timA_max_high; -}; - -static void pre_timA_change(ramctr_timing *ctrl, int channel, int slotrank, - struct timA_minmax *mnmx) +static int get_logic_delay_delta(ramctr_timing *ctrl, int channel, int slotrank) { int lane; - mnmx->timA_min_high = 7; - mnmx->timA_max_high = 0; + u16 logic_delay_min = 7; + u16 logic_delay_max = 0; FOR_ALL_LANES { - if (mnmx->timA_min_high > - (ctrl->timings[channel][slotrank].lanes[lane].timA >> 6)) - mnmx->timA_min_high = - (ctrl->timings[channel][slotrank].lanes[lane].timA >> 6); - if (mnmx->timA_max_high < - (ctrl->timings[channel][slotrank].lanes[lane].timA >> 6)) - mnmx->timA_max_high = - (ctrl->timings[channel][slotrank].lanes[lane].timA >> 6); + const u16 logic_delay = ctrl->timings[channel][slotrank].lanes[lane].timA >> 6; + + logic_delay_min = MIN(logic_delay_min, logic_delay); + logic_delay_max = MAX(logic_delay_max, logic_delay); } + + if (logic_delay_max < logic_delay_min) { + printk(BIOS_EMERG, "Logic delay max < min (%u < %u): %d, %d\n", + logic_delay_max, logic_delay_min, channel, slotrank); + } + + assert(logic_delay_max >= logic_delay_min); + + return logic_delay_max - logic_delay_min; } -static void post_timA_change(ramctr_timing *ctrl, int channel, int slotrank, - struct timA_minmax *mnmx) +static int align_rt_io_latency(ramctr_timing *ctrl, int channel, int slotrank, int prev) { - struct timA_minmax post; - int shift_402x = 0; + int latency_offset = 0; /* Get changed maxima */ - pre_timA_change(ctrl, channel, slotrank, &post); + const int post = get_logic_delay_delta(ctrl, channel, slotrank); - if (mnmx->timA_max_high - mnmx->timA_min_high < - post.timA_max_high - post.timA_min_high) - shift_402x = +1; + if (prev < post) + latency_offset = +1; - else if (mnmx->timA_max_high - mnmx->timA_min_high > - post.timA_max_high - post.timA_min_high) - shift_402x = -1; + else if (prev > post) + latency_offset = -1; else - shift_402x = 0; + latency_offset = 0; - ctrl->timings[channel][slotrank].io_latency += shift_402x; - ctrl->timings[channel][slotrank].roundtrip_latency += shift_402x; - printram("4024 += %d;\n", shift_402x); - printram("4028 += %d;\n", shift_402x); + ctrl->timings[channel][slotrank].io_latency += latency_offset; + ctrl->timings[channel][slotrank].roundtrip_latency += latency_offset; + printram("4024 += %d;\n", latency_offset); + printram("4028 += %d;\n", latency_offset); + + return post; +} + +static void compute_final_logic_delay(ramctr_timing *ctrl, int channel, int slotrank) +{ + u16 logic_delay_min = 7; + int lane; + + FOR_ALL_LANES { + const u16 logic_delay = ctrl->timings[channel][slotrank].lanes[lane].timA >> 6; + + logic_delay_min = MIN(logic_delay_min, logic_delay); + } + + if (logic_delay_min >= 2) { + printk(BIOS_WARNING, "Logic delay %u greater than 1: %d %d\n", + logic_delay_min, channel, slotrank); + } + + FOR_ALL_LANES { + ctrl->timings[channel][slotrank].lanes[lane].timA -= logic_delay_min << 6; + } + ctrl->timings[channel][slotrank].io_latency -= logic_delay_min; + printram("4028 -= %d;\n", logic_delay_min); } /* @@ -1315,7 +1337,7 @@ FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { int all_high, some_high; int upperA[NUM_LANES]; - struct timA_minmax mnmx; + int prev; wait_for_iosav(channel); @@ -1363,28 +1385,21 @@ program_timings(ctrl, channel); - pre_timA_change(ctrl, channel, slotrank, &mnmx); + prev = get_logic_delay_delta(ctrl, channel, slotrank); err = discover_402x(ctrl, channel, slotrank, upperA); if (err) return err; - post_timA_change(ctrl, channel, slotrank, &mnmx); - pre_timA_change(ctrl, channel, slotrank, &mnmx); + prev = align_rt_io_latency(ctrl, channel, slotrank, prev); discover_timA_fine(ctrl, channel, slotrank, upperA); - post_timA_change(ctrl, channel, slotrank, &mnmx); - pre_timA_change(ctrl, channel, slotrank, &mnmx); + prev = align_rt_io_latency(ctrl, channel, slotrank, prev); - FOR_ALL_LANES { - ctrl->timings[channel][slotrank].lanes[lane].timA -= - mnmx.timA_min_high * 0x40; - } - ctrl->timings[channel][slotrank].io_latency -= mnmx.timA_min_high; - printram("4028 -= %d;\n", mnmx.timA_min_high); + compute_final_logic_delay(ctrl, channel, slotrank); - post_timA_change(ctrl, channel, slotrank, &mnmx); + align_rt_io_latency(ctrl, channel, slotrank, prev); printram("4/8: %d, %d, %x, %x\n", channel, slotrank, ctrl->timings[channel][slotrank].roundtrip_latency, -- To view, visit https://review.coreboot.org/c/coreboot/+/47604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1da95520ac915ab003e1a839685cbf5f1970eb6a Gerrit-Change-Number: 47604 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: mb/google/volteer: Add keyboard layout to fw_config
by Duncan Laurie (Code Review) 20 Nov '20

20 Nov '20

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47478 ) Change subject: mb/google/volteer: Add keyboard layout to fw_config ...................................................................... mb/google/volteer: Add keyboard layout to fw_config A new field was defined for different keyboard layouts, so add this field to the list and provide the two options that were defined. Change-Id: Ic357446725e34221040705929d54cbce94c5ab8b Signed-off-by: Duncan Laurie <dlaurie(a)google.com> --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/47478/1 diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index a73d4ad..4d615c8 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -37,6 +37,10 @@ option SD_GL9755S 1 option SD_RTS5261 2 end + field KB_LAYOUT 20 21 + option KB_LAYOUT_DEFAULT 0 + option KB_LAYOUT_1 1 + end end chip soc/intel/tigerlake -- To view, visit https://review.coreboot.org/c/coreboot/+/47478 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic357446725e34221040705929d54cbce94c5ab8b Gerrit-Change-Number: 47478 Gerrit-PatchSet: 1 Gerrit-Owner: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: soc/amd/picasso: Add data fabric read helper function
by Felix Held (Code Review) 20 Nov '20

20 Nov '20

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47726 ) Change subject: soc/amd/picasso: Add data fabric read helper function ...................................................................... Patch Set 2: (7 comments) https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_f… File src/soc/amd/picasso/data_fabric.c: https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_f… PS2, Line 17: pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_CONTROL(reg), : IOMS0_FABRIC_ID << MMIO_DST_FABRIC_ID_SHIFT); : pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_BASE(reg), 0); : pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_LIMIT(reg), 0); out of scope for this patch, but having a data_fabric_write_reg32() might make this code and the code below easier to read https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_f… PS2, Line 25: pci_read_config32(SOC_DF_F0_DEV, NB_MMIO_CONTROL(reg)); this could be rewritten to use data_fabric_read_reg32(). I'm ok with doing that in a separate patch to not block this and the ACPI CRAT one https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_f… PS2, Line 176: return pci_read_config32(_SOC_DEV(DF_DEV, function), reg); it might be useful to add a comment that the macros will apply a bit mask to the values, so no bit masking is required here https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_f… PS2, Line 181: reg reg is the byte address and the two last bits get masked off, since the access is always dword/qword based, right? https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/includ… File src/soc/amd/picasso/include/soc/data_fabric.h: https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/includ… PS2, Line 28: #define D18F0_DRAM_HOLE_CTL 0x104 : #define DRAM_HOLE_CTL_VALID BIT(0) : #define DRAM_HOLE_CTL_BASE_SHFT 24 : #define DRAM_HOLE_CTL_BASE (0xFF << DRAM_HOLE_CTL_BASE_SHFT) : : #define D18F0_DRAM_BASE0 0x110 : #define DRAM_BASE_REG_VALID BIT(0) : #define DRAM_BASE_HOLE_EN BIT(1) : #define DRAM_BASE_INTLV_CH_SHFT 4 : #define DRAM_BASE_INTLV_CH (0xF << DRAM_BASE_INTLV_CH_SHFT) : #define DRAM_BASE_INTLV_SEL_SHFT 8 : #define DRAM_BASE_INTLV_SEL (0x7 << DRAM_BASE_INTLV_SEL_SHFT) : #define DRAM_BASE_ADDR_SHFT 12 : #define DRAM_BASE_ADDR (0xFFFFF << DRAM_BASE_ADDR_SHFT) : : #define D18F0_DRAM_LIMIT0 0x114 : #define DRAM_LIMIT_DST_ID_SHFT 0 : #define DRAM_LIMIT_DST_ID (0xFF << DRAM_LIMIT_DST_ID_SHFT) : #define DRAM_LIMIT_INTLV_NUM_SOCK_SHFT 8 : #define DRAM_LIMIT_INTLV_NUM_SOCK (0x1 << DRAM_LIMIT_INTLV_NUM_SOCK_SHFT) : #define DRAM_LIMIT_INTLV_NUM_DIE_SHFT 10 : #define DRAM_LIMIT_INTLV_NUM_DIE (0x3 << DRAM_LIMIT_INTLV_NUM_DIE_SHFT) : #define DRAM_LIMIT_ADDR_SHFT 12 : #define DRAM_LIMIT_ADDR (0xFFFFF << DRAM_LIMIT_ADDR_SHFT) : : #define DF_DRAM_BASE(dram_map_pair) ((dram_map_pair) * 2 * sizeof(uint32_t) \ : + D18F0_DRAM_BASE0) : #define DF_DRAM_LIMIT(dram_map_pair) ((dram_map_pair) * 2 * sizeof(uint32_t) \ : + D18F0_DRAM_LIMIT0) i'd add those in the next commit, since they aren't needed or used for what this patch adds https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/includ… PS2, Line 62: BIT(0) i'd prefer (1 << 0) over BIT(0), but both will work. your call. https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/includ… PS2, Line 70: DF_IND_CFG_INST_ID is this a mask? if so, i'dd ad _MASK as suffix -- To view, visit https://review.coreboot.org/c/coreboot/+/47726 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If0dc72063fbb99efaeea3fccef16cc1b5b8526f1 Gerrit-Change-Number: 47726 Gerrit-PatchSet: 2 Gerrit-Owner: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Reviewer: Justin Frodsham <justin.frodsham(a)protonmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Reviewer: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 20 Nov 2020 00:27:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2020