Meera Ravindranath 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 30:
(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?
Yes, here is the link https://partnerissuetracker.corp.google.com/issues/150653436
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. […]
Ack
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. […]
Done
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/waddledoo/memory.c:
https://review.coreboot.org/c/coreboot/+/39136/29/src/mainboard/google/deded... PS29, Line 38: * the index = pin number on ddr4 part : * the value = pin number on SoC
Can you please fix the comment here to reflect the mapping. LGTM otherwise.
Done