Attention is currently required from: Shelley Chen, Ravi kumar, mturney mturney, Sudheer Amrabadi. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59193 )
Change subject: libpayload: Parse DDR Information through coreboot tables ......................................................................
Patch Set 24:
(5 comments)
File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/59193/comment/5b26d3cd_97d20e5f PS24, Line 48: #include <commonlib/bsd/fmap_serialized.h> Please add it here in alphabetical order.
File src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h:
https://review.coreboot.org/c/coreboot/+/59193/comment/0585c234_9df28de4 PS24, Line 3: #ifndef _MEM_CHIP_INFO_H _COMMONLIB_BSD_MEM_CHIP_INFO_H_
https://review.coreboot.org/c/coreboot/+/59193/comment/e479490f_60241c4d PS24, Line 6: typedef Just make it a struct mem_chip_info, do not typedef. (Also, you should decide on which name you want too use for this... "mem chip info", "dram info" or whatever, and then use that name consistently everywhere.)
https://review.coreboot.org/c/coreboot/+/59193/comment/041047ea_acbefc8d PS24, Line 7: uint8_t type; There should also be an enum somewhere in this file that defines the possible values for this `type` field (e.g. MEM_CHIP_DDR3 = 1, MEM_CHIP_LPDDR3, MEM_CHIP_DDR4, MEM_CHIP_LPDDR4).
File src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h:
https://review.coreboot.org/c/coreboot/+/59193/comment/8c62b7da_d6d87fe1 PS21, Line 16: // for potential future LPDDR5 expansion
Can you please elaborate on this? How many bits are used now (and what memory it supports) and how […]
The 8 bytes of serial_id are a field in the DDR Mode Registers starting with LPDDR5. So I think it's useful to have them here since we'll eventually have LPDDR5 platforms, and for LPDDR4 they can just leave it zero.
I don't think we need this comment here, or if you want a comment then just saying "LPDDR5 only" or something like that probably makes more sense. Also, please remember coreboot coding style is /* C90 comments only */.