Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h@31 PS4, Line 31: _JEDEC Do we need the suffix JEDEC? It was added to LPDDR3 because we had two types of SPDs one with Intel-defined custom ID for LPDDR3 and other with JEDEC-defined. But, LPDDR4 seems to have only one. So, we can skip the suffix _JEDEC just like SPD_DRAM_DDR3 or SPD_DRAM_DDR4.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
PS4: I think this file is getting messier with all the checks for each new DRAM type that we add support for. Don't mean to push this on you, but I think we should switch to using a table which describes the attributes for each memory type and then have generic functions which use that table to return the right values. Then, any time a new memory type has to be added, it just needs another entry in the table.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@63 PS4, Line 63: SPD_DRAM_LPDDR3_JEDEC I know this wasn't added here. But looking at 21-C, I think both LPDDR3 and LPDDR4 have similar definition for banks.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@109 PS4, Line 109: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION; This is not handled correctly for both LPDDR3 and LPDDR4.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@120 PS4, Line 120: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION; same here
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@131 PS4, Line 131: int busw_offset = is_memory_type_ddr4(dram_type) ? DDR4_BUS_DEV_WIDTH : : DDR3_BUS_DEV_WIDTH; and here