Karthik Ramasubramanian 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.
From what I understand, this function is called only once during the romstage. It is not retried after that.
Thinking about the hypothetical/future case of mixing different DRAM part numbers, we might need to call this function multiple times. Even in that scenario, it does not make sense to store the part number information. We have to reach out to EC to get the part number on individual channels and nodes.
Thoughts??
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 implementation in romstage.c
That way we don't have to explicitly account for '\0' termination character.