Hello Duan huayang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44729
to review the following change.
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
soc/mediatek/mt8192: Get dram total size from emi config
Signed-off-by: Huayang Duan huayang.duan@mediatek.com Change-Id: I5fb64e964cbf62ee70a90975583a9947558bbab6 --- M src/soc/mediatek/mt8192/emi.c 1 file changed, 63 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/44729/1
diff --git a/src/soc/mediatek/mt8192/emi.c b/src/soc/mediatek/mt8192/emi.c index acd605f..d485131 100644 --- a/src/soc/mediatek/mt8192/emi.c +++ b/src/soc/mediatek/mt8192/emi.c @@ -437,9 +437,71 @@ dramc_set_broadcast(bc_bak); }
+static int get_rank_num_by_emi(void) +{ + unsigned int emi_cona = read32(&emi_reg->cona); + + if (emi_cona & (0x3 << 16)) + return 2; + else + return 1; +} + +static int get_channel_num_by_emi(void) +{ + unsigned int emi_cona = read32(&emi_reg->cona); + + int channel_nr = 0x1 << ((emi_cona >> 8) & 0x3); + + return channel_nr; +} + +static void get_rank_size_by_emi_reg(u64 rank_size[RANK_MAX]) +{ + u32 quad_ch_ratio = 1; + u64 ch0_rank0_size, ch0_rank1_size; + u64 ch1_rank0_size, ch1_rank1_size; + u32 cen_emi_conh = read32(&emi_reg->conh); + + 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; + + if (get_channel_num_by_emi() > 1) { + rank_size[0] += ch1_rank0_size; + if (get_rank_num_by_emi() > 1) + rank_size[1] += ch1_rank1_size; + } + + dramc_dbg("DRAM rank0 size:0x%llX, DRAM rank1 size=0x%llX\n", + rank_size[0], rank_size[1]); +} + size_t sdram_size(void) { - size_t dram_size = 0x100000000; + int rank_num; + size_t dram_size = 0; + u64 rank_size[RANK_MAX]; + + get_rank_size_by_emi_reg(rank_size); + rank_num = get_rank_num_by_emi(); + + for (int i = 0; i < rank_num; i++) + dram_size += rank_size[i];
return dram_size; }
Yidi Lin has uploaded a new patch set (#24) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/44729 )
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
soc/mediatek/mt8192: Get dram total size from emi config
Signed-off-by: Huayang Duan huayang.duan@mediatek.com Change-Id: I5fb64e964cbf62ee70a90975583a9947558bbab6 --- M src/soc/mediatek/mt8192/emi.c 1 file changed, 63 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/44729/24
Yidi Lin has uploaded a new patch set (#34) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/44729 )
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
soc/mediatek/mt8192: Get dram total size from emi config
Signed-off-by: Huayang Duan huayang.duan@mediatek.com Change-Id: I5fb64e964cbf62ee70a90975583a9947558bbab6 --- M src/soc/mediatek/mt8192/emi.c 1 file changed, 63 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/44729/34
Yidi Lin has uploaded a new patch set (#36) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/44729 )
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
soc/mediatek/mt8192: Get dram total size from emi config
Signed-off-by: Huayang Duan huayang.duan@mediatek.com Change-Id: I5fb64e964cbf62ee70a90975583a9947558bbab6 --- M src/soc/mediatek/mt8192/emi.c 1 file changed, 63 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/44729/36
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
CK HU has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44729 )
Change subject: soc/mediatek/mt8192: Get dram total size from emi config ......................................................................
Abandoned
Useless
-
CK HU (Code Review) -
Paul Menzel (Code Review)
-
Yidi Lin (Code Review)