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:
(10 comments)
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
Oops, yes: GDCRCMDPICODING_Cx
Named it pi_coding
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 112: reg_5064b0
Yes: PM_DLL_CONFIG. Specifically, bits 0-11 are used for the delay timer for the master DLL wakeup.
Named it mdll_wake_delay
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 121: reg_320c_range_threshold
Also has a name
Not sure of what this is, but named it pi_coding_threshold
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
I think it would be better to rewrite this comment as documentation.
Rewritten it completely.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 67: 4284
IOSAV_RUN_QUEUE? this macro is almost always used next to the register name.
Done, but called it IOSAV_RUN_ONCE(length) because it only runs the sequence once, as explained in the improved comment above.
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 134: #define CxLy(r, x, y) ((r) + ((x) * 0x400) + ((y) * 4)) : #define LyCx(r, x, y) ((r) + ((y) * 4) + ((x) * 0x400))
these two are basically the same, but with different order of the parameters; please drop the second […]
Will be done in a follow-up
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)
Yes, some of these macros aren't used. I added them for the sake of completeness. […]
Dropped for now, can re-add once the code is prettier.
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 184: MC Channel 0
Oops. Wanted to add a Channel 1, but it was never used. […]
Edited the comment.
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'm not sure if the inline comments will fit, though. Will have to trim them.
Managed to squeeze a few more characters without needing to do major rework, now I use `ch` to refer to the channel variable.
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)
*for now
Added a FIXME on the CxLy and LyCx macros.