Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )
Change subject: mb/google/dedede: Read DRAM part number from CBI ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/romstage.c:
https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... PS1, Line 37: part_num_state = PART_NUM_NOT_READ;
I am not sure if we need to maintain state information. […]
* Yes, agree with you. The function originally was introduced when romstage calls this function inside the channel loop and assume we need a single DRAM part only. Therefore it makes sense to maintain state for not calling multiple times to EC for the same info. * Regarding supporting different DRAM Part Numbers in a DUT, I would suggest to defer to another CL stack if we really need to introduce it since currently we call it once in romstage of Jasperlake only. As a result, we focus on single DRAM part first.
https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... PS1, Line 53: strlen(part_num_store)
Nit: Can we store sizeof(part_num_store) instead? This way it is consistent with the non-override im […]
copy that.