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 20:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... File src/mainboard/google/dedede/romstage.c:
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... PS18, Line 25:
extra tab not required.
Done
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... PS18, Line 26:
same here.
Done
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... PS18, Line 117: RAM_STRAP_0
These straps should be configured in gpio_table above as well.
Done
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
So, ID 0 is empty on waddledoo? Why are we leaving empty slots. We should start indexing from 0. […]
ID 0 is for the vendor Hynix and ID 2 is for vendor Samsung. But we do not have the spd data from them as of now. Will definitely update once we get the spd data from other vendors.
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/waddledoo/memory.c:
https://review.coreboot.org/c/coreboot/+/39136/18/src/mainboard/google/deded... PS18, Line 23: /* Access memory info through CBFS */
Comment seems misplaced.
Ack