Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44729 )
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
Patch Set 40:
(12 comments)
The code looks a little more complicated than needed. Maybe Hung-Te and the others have a suggestion how to improve it.
https://review.coreboot.org/c/coreboot/+/44729/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44729/40//COMMIT_MSG@8 PS40, Line 8: Tested how? Please paste the new debug log message.
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... File src/soc/mediatek/mt8192/emi.c:
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 451: int `size_t` or `unsigned int`
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 453: unsigned int uint32_t/u32 as it’s read32?
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 455: if (emi_cona & (0x3 << 16)) : return 2; : else : return 1; return (emi_cona & (0x3 << 16)) ? 2 : 1
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 463: unsigned int u32
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 465: int unsigned int or size_t
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 470: RANK_MAX The implementation below only supports two ranks (0, 1). Maybe add an assertion.
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 470: u64 Why not size_t?
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 472: u32 quad_ch_ratio = 1; 1. const? 2. Just `unsigned int`?
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 477: rank_size[0] = 0; : rank_size[1] = 0; : : ch0_rank0_size = (cen_emi_conh >> 16) & 0xf; : ch0_rank1_size = (cen_emi_conh >> 20) & 0xf; : ch1_rank0_size = (cen_emi_conh >> 24) & 0xf; : ch1_rank1_size = (cen_emi_conh >> 28) & 0xf; : : ch0_rank0_size = (ch0_rank0_size * quad_ch_ratio) << 28; : ch0_rank1_size = (ch0_rank1_size * quad_ch_ratio) << 28; : ch1_rank0_size = (ch1_rank0_size * quad_ch_ratio) << 28; : ch1_rank1_size = (ch1_rank1_size * quad_ch_ratio) << 28; : : rank_size[0] += ch0_rank0_size; : : if (get_rank_num_by_emi() > 1) : rank_size[1] += ch0_rank1_size; I’d do `rank_size[0] = c0_rank0_size`, and avoid initialization in the beginning.
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 507: int `size_t` or `unsigned int`
https://review.coreboot.org/c/coreboot/+/44729/40/src/soc/mediatek/mt8192/em... PS40, Line 509: u64 size_t