Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38036 )
Change subject: nb/intel/sandybridge: Add a bunch of MCHBAR defines ......................................................................
Patch Set 7:
(9 comments)
Thanks for the review!
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.h:
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 60: val_320c
this has a known name
Oops, yes: GDCRCMDPICODING_Cx
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 112: reg_5064b0
this has also a known name
Yes: PM_DLL_CONFIG. Specifically, bits 0-11 are used for the delay timer for the master DLL wakeup.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 121: reg_320c_range_threshold Also has a name
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 45: 0x4220 + 0x400 * X + 4 * Y
replace with new macro? same for the lines below
I think it would be better to rewrite this comment as documentation.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 67: 4284
this also has a better name now; maybe IOSAV_SEQ_CTL_RUN(x) ?
IOSAV_RUN_QUEUE? this macro is almost always used next to the register name.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 138: #define GDCRTRAININGRESULT1_Cz_B0(z) Gz(0x0004, z) /* Results according to PI settings */ : #define GDCRTRAININGRESULT2_Cz_B0(z) Gz(0x0008, z) : #define GDCRRXRANK0_Cz_B0(z) Gz(0x0010, z) /* Time setting for Rx data byte */ : #define GDCRRXRANK1_Cz_B0(z) Gz(0x0014, z) : #define GDCRRXRANK2_Cz_B0(z) Gz(0x0018, z) : #define GDCRRXRANK3_Cz_B0(z) Gz(0x001c, z) : #define GDCRTXRANK0_Cz_B0(z) Gz(0x0020, z) /* Time setting for Tx data byte */ : #define GDCRTXRANK1_Cz_B0(z) Gz(0x0024, z) : #define GDCRTXRANK2_Cz_B0(z) Gz(0x0028, z) : #define GDCRTXRANK3_Cz_B0(z) Gz(0x002c, z)
these don't seem to be registers, but register offsets. […]
Yes, some of these macros aren't used. I added them for the sake of completeness. However, I didn't handle the bytelane indices because the ECC lane is in the middle of bytelanes 0-3 and 4-7, so it would need additional work. Current code has the offsets in an array, so things would need to get refactored.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 184: MC Channel 0
this isn't just for channel 0, but also channel 1
Oops. Wanted to add a Channel 1, but it was never used. This should have been renamed when I introduced the indexed macros. I think that the broadcast registers are where channel 3 would be, too. Not sure if it's worth factoring out.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 185: #define TC_DBP_Cx(x) Cx(0x4000, x) /* Timing of DDR - bin parameters */
i'd strongly recommend using channel instead of x here; that would make it clearer what happens here […]
I'm not sure if the inline comments will fit, though. Will have to trim them.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 220: /* Sub-sequence special command address */ : #define IOSAV_y_SP_CMD_ADDR_Cx(x, y) LyCx(0x4200, x, y) : : /* Address update after command execution */ : #define IOSAV_y_ADDR_UPD_Cx(x, y) LyCx(0x4210, x, y) : : /* Command signals in sub-sequence command */ : #define IOSAV_y_SP_CMD_CTL_Cx(x, y) LyCx(0x4220, x, y) : : /* Sub-sequence command parameter control */ : #define IOSAV_y_SUBSEQ_CTL_Cx(x, y) LyCx(0x4230, x, y) : : /* 23-bit LFSR value of the sequence */ : #define IOSAV_y_ADDRESS_LFSR_Cx(x, y) LyCx(0x4240, x, y)
please use the CxLy macro here. […]
These are inverted for reproducibility purposes. Otherwise, the changed operations means that the fixed IOSAV sequence number does not get precalculated, which changes the binary. In any case, I'd drop CxLy instead.