Felix Held 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:
(7 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 this has a known name
https://review.coreboot.org/c/coreboot/+/38036/7/src/northbridge/intel/sandy... PS7, Line 112: reg_5064b0 this has also a known 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 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 one and use the first one everywhere
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. they also seem unused
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
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. also for the following defines
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.
Maybe also swap the x and y parameters in the IOSAV_* macros; it would make more sense to me to have the sequencer slot number (0..3) as first parameter and the memory channel as second. this is just my opinion though ;)