Attention is currently required from: Alicja Michalska, Arthur Heymans, Nico Huber.
Angel Pons has posted comments on this change by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/64189?usp=email )
Change subject: haswell NRI: Add DDR3 JEDEC reset and init ......................................................................
Patch Set 8:
(3 comments)
File src/northbridge/intel/haswell/native_raminit/raminit_native.h:
https://review.coreboot.org/c/coreboot/+/64189/comment/eaeeecc9_e6c3281e?usp... : PS8, Line 79: RAMINIT_STATUS_UNSPECIFIED_ERROR, /** TODO: Deprecated in favor of specific values **/
Assuming error code reporting isn't ready yet?
This comment is because some of the code I haven't upstreamed yet still uses this, and I'd rather get rid of this one line after the thousands of lines I've still got around are at least upstreamed.
I actually don't use error code values for anything yet (0 = success, anything else = failure), but I felt it'd be nicer than hardcoding magic numbers all over the place (which was the case before). I will eventually use them to try raminit again after a failure, using more relaxed parameters or disabling a failing channel.
https://review.coreboot.org/c/coreboot/+/64189/comment/d4e30546_219bf759?usp... : PS8, Line 176: int8_t rxvref[NUM_CHANNELS][NUM_SLOTRANKS][NUM_LANES];
uint?
No, this is a signed offset. Zero offset corresponds to VDDQ/2 (half of the I/O voltage), and the offset can be positive or negative.
The timing offsets (tx_dq, txdqs, rcven, rxdqs, clk/ctl/cke pi codes...) are delays measured in "PI ticks" (1/64th of a QCLK (quadrature clock) clock cycle, or 1/128th of a DCLK (DRAM clock) clock cycle). These are unsigned since you cannot go back in time.
I could add comments to these but I think it'd be better to do so in a follow-up as some of the patches in this train add more stuff in here (so adding comments now would cause a bunch of merge conflicts).
File src/southbridge/intel/lynxpoint/pch.h:
https://review.coreboot.org/c/coreboot/+/64189/comment/943cb361_4841a832?usp... : PS8, Line 590: #define PM_CFG2_DRAM_RESET_CTL (1 << 26) /* ULT only */
NIT: Remove the space to make it look a bit nicer?
The space is there to indicate this is a bitfield within the register. Looks like this file is inconsistent with space usage but I prefer having the space to make it easier to follow
-
Angel Pons (Code Review)