Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46111 )
Change subject: trogdor: Modify DDR training to use mrc_cache ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46111/6/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/46111/6/src/mainboard/google/trogdo... PS6, Line 12: RO_VPD(PRESERVE) 228K Sorry, I just submitted CB:45705 so you'll have to rebase onto that.
Would you mind shifting another 128K from RO_VPD to COREBOOT? We don't really need such a big VPD, and with the new Groot requirements it's probably better to have most of the extra free space in the CBFS part. (The RO_SECTION would increase accordingly and the FMAP would move from 0x3c0000 to 0x3e0000 then.)
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... File src/soc/qualcomm/common/qclib.c:
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... PS6, Line 78: * because we always want to retrain. Well, the reason we're not writing it back is more that recovery uses RO firmware, and RW firmware might have been updated to a different training data format.
Also, coreboot comment style is either
/* * a blank line both * at the top and the * bottom. */
or
/* no star on the left for continuing lines */
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... PS6, Line 142: (CONFIG(VBOOT) && get_recovery_mode_retrain_switch())) { The retrain isn't really important here (retrain always implies recovery anyway), the important part for retrain is that we want to erase the MRC cache on flash (so the next time RW boots it will be forced to retrain).
I believe(?) the MRC cache driver will do that automatically already if we just select CONFIG_MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN? Would be good to test out if that actually works, though. (I'm also not sure if the retrain key combination is actually enabled on Trogdor, that's also a good thing to try out. If not, please talk to Tom about turning it on.)
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... PS6, Line 143: data_size = 0; We should probably still pass the full size of the region to QcLib even if we're not intending to use it, otherwise I'd be afraid we might confuse it (e.g. maybe it needs that area as scratch space during training or something, even if we throw it away again). We should just skip the part about loading something into it. Should probably just memset() the whole thing to 0 instead.
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... PS6, Line 149: printk(BIOS_ERR, "Unable to load previous training data.\n"); I think this is exactly the case we would hit if we would run the CONFIG_MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN thing. Probably worth to also memset() the memory buffer to 0 here just to make sure we're not passing random garbage data that might confuse QcLib.
https://review.coreboot.org/c/coreboot/+/46111/6/src/soc/qualcomm/common/qcl... PS6, Line 152: data_size As mentioned on the other patch, this would probably better be REGION_SIZE(ddr_training) after all.