Attention is currently required from: Jamie Chen, Paul Menzel, Kane Chen, Andrey Petrov, Patrick Rudolph, Karthik Ramasubramanian. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56805 )
Change subject: soc/intel/apollolake: change LPDDR4 density define ......................................................................
Patch Set 1:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56805/comment/07e76548_cdcefda0 PS1, Line 7: define enum definition?
https://review.coreboot.org/c/coreboot/+/56805/comment/0efdad3a_ad64bc21 PS1, Line 13: BUG=b:178665760
File src/soc/intel/apollolake/meminit.c:
https://review.coreboot.org/c/coreboot/+/56805/comment/fe536965_7e14a6b5 PS1, Line 24: /* Per rank density in Gb */ : switch (density) { : case LP4_8Gb_DENSITY: : sz = 8; : break; : case LP4_12Gb_DENSITY: : sz = 12; : break; : case LP4_16Gb_DENSITY: : sz = 16; : break; : default: : printk(BIOS_ERR, "Invalid DRAM density: %d\n", density); : sz = 0; : break; : } I think this can be now dropped completely and `sz` can be set directly to density since that is what the density enum is defined as. Also, the caller already has a check to ensure that we never pass in incorrect density here so the default case should never occur.
https://review.coreboot.org/c/coreboot/+/56805/comment/bd436b7b_ee21db26 PS1, Line 292: rank_density nit: Update this to rank_density_gb to indicate that the rank density is now passed in Gb.
https://review.coreboot.org/c/coreboot/+/56805/comment/b2cf9e1c_779841ab PS1, Line 307: \n Gb