HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28648 )
Change subject: nb/intel/i945: Set CxDRT1 tRPALL bit if populated with 8-bank ......................................................................
Patch Set 9:
Why are both CxDRT1 registers programmed with the same value ? Wouldn't that break if one channel has 8banks DIMMs, but the other 16banks DIMMs? What does the datasheet say? Does both channels need to be kept in sync ?
short answer:
here intel's information: Intel ® 82945G/82945G/82945GC GMCH and 82945P/82945PL MCH Datasheet (page 226): "DRAM Timing (CxDRT1): The x represents a channel, A or B represented by 0 and 1 respectively. The DRT Register defines the timing parameters for all devices in a channel. BIOS programs this register with “least common denominator” values after reading the SPD registers of each DIMM in the channel."
Mobile Intel® 945 Express Chipset Family Datasheet (page 129): "Pre-All to Activate Delay (tRPALL): This is applicable only to 8-bank architectures. Must be set to 1 if any rank is populated with 8-bank device technology. 0: tRPALL = tRP 1: tRPALL = tRP + 1"
Indeed looks like (even without this patch), when we use 2 different architectures and inbetween an empty slot i.e.: DIMM0 : 16 DIMM1 : empty DIMM2 : 8 it will not boot unless we remove the "if (sysinfo->banksize[i] == 0) continue;" here: https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/i94... this solution didn't make sense ! (or I'm wrong).
Long answer: I'm trying to figure out why my board sin't boot when: 1) the channel0 is not populated at all 2) one of 4 DIMM is populated with a ram at 533 so, I'm trying to find documented registers and make a comparison between vendor, the document and coreboot. So I think that https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/i94... is probably wrong in case channel0 is not populated, I also found some R/WO that we try a re-write again ( see https://review.coreboot.org/#/q/status:open+project:coreboot+branch:master+t... ) I just find that https://review.coreboot.org/cgit/coreboot.git/tree/src/northbridge/intel/i94... should be : temp_drt = MCHBAR32(C0DRT1) & 0xcf020088; (as [27:24] and [31:30] are reserved). this needs test
This said, probably "CxDRT1 |= (1 << 16)" is not a big deal and we can just add a comment asking to test on Mobile i945 version vendor did set that bit for both channels, but even without it, the chip I have didn't complain (if there is no empty slot in between) So I can drop this and just add a comment ... but for sure, i945 needs more investigation to find why it didn't find the rising edge if channel0 is not populated, and why it didn't support ram at 533 (at least for desktop version)