Attention is currently required from: Alicja Michalska, Arthur Heymans, Eric Lai, Felix Held, Felix Singer, Nico Huber, Patrick Rudolph.
Angel Pons has posted comments on this change by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/64186?usp=email )
Change subject: haswell NRI: Configure initial MC settings ......................................................................
Patch Set 7:
(5 comments)
File src/northbridge/intel/haswell/native_raminit/configure_mc.c:
https://review.coreboot.org/c/coreboot/+/64186/comment/b7281944_0c8bb070?usp... : PS7, Line 16: if (!is_hsw_ult())
What's the purpose of this check? Does NRI currently support ULT CPUs?
On Haswell Trad (not ULT), we don't program this register. Trad doesn't support configurable channel interleave (it's forced on), and it doesn't support LPDDR (3) either.
I know this code works on Haswell Trad and Haswell ULT with DDR3, but I don't have any Haswell device with LPDDR3.
Should I add any clarification? I use this pattern quite a lot.
https://review.coreboot.org/c/coreboot/+/64186/comment/be653885_09458948?usp... : PS7, Line 641: if (!(reg32 & BIT(31))) { /** TODO: What to do if this is locked? **/
If it's locked, then reading the register will fail, right? I think it would be nice to put a debug […]
This checks if the lock bit is unset before programming. I believe that *writing* the register will fail, but I'm not sure how to handle it (proceed anyway since it shouldn't affect booting? that's the current approach). I also haven't checked if the register remains locked after a reset. I do know that the register can be locked if we are to retry raminit for some reason (e.g. failed warm/fast boot).
I'll try to refactor this to log an error if the register is locked *and* its value differs from the desired value.
https://review.coreboot.org/c/coreboot/+/64186/comment/58b24c65_80ac7afa?usp... : PS7, Line 660: /** TODO: Remember why this is only done on cold boots **/
Stored in cache?
I'm pretty sure it's that, yes
File src/northbridge/intel/haswell/native_raminit/raminit_main.c:
https://review.coreboot.org/c/coreboot/+/64186/comment/e2b65ce1_9d31a8dc?usp... : PS7, Line 58: ctrl->vdd_mv = is_hsw_ult() ? 1350 : 1500; /** FIXME: Hardcoded, does it matter? **/
Probably not, DDR3U isn't very common :)
Yeah, for now it'll do. I know this will need to change once NRI supports voltage switching (which I want to implement someday), but I should first finish the rest of the training steps (I've got a lot to clean up and upstream)
File src/northbridge/intel/haswell/native_raminit/reg_structs.h:
https://review.coreboot.org/c/coreboot/+/64186/comment/34eec391_ac4a0244?usp... : PS7, Line 12: int32_t vref : 6; // Bits 31:26
s/int/uint, or is there a good reason for it? […]
This is very much intentional - this 6-bit bitfield is actually a signed two's complement value. Conveniently enough, using bitfields for register definitions makes reading and writing these signed bitfields painless.
-
Angel Pons (Code Review)