Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39136 )
Change subject: mb/google/dedede: Add memory initialization support for dedede ......................................................................
Patch Set 29:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... File src/mainboard/google/dedede/romstage.c:
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... PS29, Line 20: TODO Is there a bug tracking this TODO?
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/baseboard/memory.c:
PS29: Just a thought: I believe most variants would follow what the reference board is doing. It might make sense to move the definitions you added for waddledoo here in baseboard.
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/waddledoo/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... PS18, Line 16: empty
Furquan, I am not sure if you want to put all the SPDs now itself or do you want to add them as and when they become available.
We can add them as and when they become available. I just wanted to make sure we do not leave ID 0 empty since it unnecessarily eats 512 bytes of space. Thanks for confirming that it will be added as a follow-up.
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/waddledoo/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... PS29, Line 9: SPD_SOURCES = empty Can you please add a comment in the end indicating the ID that is used.
SPD_SOURCES = empty # 0b0000 SPD_SOURCEs += ... # 0b0001 ...