Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 73: 6
Why does this not have a macro of its own?
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 106: Enabled
That is not correct. It could be enabled/disabled based on board setting.
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 110: UserBd
Should there be an enum for what FSP expects?
Yes, Tigerlake defines the enum in src/soc/intel/tigerlake/include/soc/romstage.h and hence we are reusing it instead of redefining it here again.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 118: const struct mb_cfg *board_cfg,
nit: Would this fit on the line above?
Done
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: spd
Ack
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... PS18, Line 68: sizeof(board_cfg->dq_map[DDR_CH0])
I am confused by the comment above and the memset. […]
Ack Yes it copies all 12 bytes of data. Since the default UPD is 0, there is no point of the memset above. Removed it.