Xi Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/Kconfig M src/soc/mediatek/mt8192/memory.c 2 files changed, 9 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/1
diff --git a/src/soc/mediatek/mt8192/Kconfig b/src/soc/mediatek/mt8192/Kconfig index 1d1cf7b..2932f25 100644 --- a/src/soc/mediatek/mt8192/Kconfig +++ b/src/soc/mediatek/mt8192/Kconfig @@ -32,10 +32,14 @@ bool default n help - This options enables DRAM calibration with multiple frequencies (low, + This option enables DRAM calibration with multiple frequencies (low, medium and high) for DVFS feature.
config MEMORY_TEST bool default y + help + This option will do memory basic compare test to verify the dram read + or write is as expect. + endif diff --git a/src/soc/mediatek/mt8192/memory.c b/src/soc/mediatek/mt8192/memory.c old mode 100644 new mode 100755 index b5363b0..19a702d --- a/src/soc/mediatek/mt8192/memory.c +++ b/src/soc/mediatek/mt8192/memory.c @@ -15,13 +15,13 @@ const struct ddr_base_info *ddr_info = &dparam->ddr_info;
for (u8 rank = RANK_0; rank < ddr_info->support_ranks; rank++) { - int i = complex_mem_test(addr, 0x2000); + int result = complex_mem_test(addr, 0x2000);
printk(BIOS_DEBUG, "[MEM] complex R/W mem test %s\n", - (i == 0) ? "pass" : "fail"); + (result == CB_SUCCESS) ? "pass" : "fail");
- if (i != 0) { - printk(BIOS_ERR, "DRAM memory test failed\n"); + if (result != 0) { + printk(BIOS_ERR, "[MEM] DRAM memory test failed: %d\n", result); return -1; }
Yidi Lin has uploaded a new patch set (#2) to the change originally created by Xi Chen. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/Kconfig M src/soc/mediatek/mt8192/memory.c 2 files changed, 9 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
PS2: Thank you for the quick response for the comments in [1].
Please split the Kconfig changes out into a separate commit.
[1]: https://review.coreboot.org/c/coreboot/+/44569
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 44: expect expected
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 23: result != 0 result != CB_SUCCESS
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 36: low, : medium and high I thought we have 7 different frequencies in mt8192.
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 43: will do enables
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 23: result != 0
result != CB_SUCCESS
Actually, the return type of complex_mem_test() is int (not enum cb_err), and I only see one instance of CB_SUCCESS in src/mainboard/google. I wonder if we really need to use CB_SUCCESS here, given that returning 0 is already the convention on success.
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 24: printk(BIOS_ERR, "[MEM] DRAM memory test failed: %d\n", result); How about we print only one message regardless of the test result?
int result = complex_mem_test(...); if (result != 0) printk(BIOS_ERR, "[MEM] Complex R/W mem test failed: %d", result); else printk(BIOS_INFO, "[MEM] Complex R/W mem test passed");
Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
PS2:
Thank you for the quick response for the comments in [1]. […]
Ack
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 36: low, : medium and high
I thought we have 7 different frequencies in mt8192.
Yes, 8192 contains 7 freqs. "low, medium and high" means some freqs, not only one freq. Add freqs example to clear misunderstanding.
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 43: will do
enables
Ack
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 44: expect
expected
Ack
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 24: printk(BIOS_ERR, "[MEM] DRAM memory test failed: %d\n", result);
How about we print only one message regardless of the test result? […]
Ack
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Yidi Lin, Yidi Lin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46585
to look at the new patch set (#3).
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 5 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/3
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Yidi Lin, Yidi Lin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46585
to look at the new patch set (#4).
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 5 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/4
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Yidi Lin, Yidi Lin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46585
to look at the new patch set (#5).
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 5 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46585/5/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/5/src/soc/mediatek/mt8192/mem... PS5, Line 21: printk(BIOS_ERR, "[MEM] complex R/W mem test failed: %d\n", result); line over 96 characters
Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
PS2:
Ack
split Kconfig patch to: https://review.coreboot.org/c/coreboot/+/46616/1
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Yidi Lin, Yidi Lin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46585
to look at the new patch set (#6).
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 6 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/6
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 6: Code-Review+1
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... File src/soc/mediatek/mt8192/Kconfig:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/Kco... PS2, Line 36: low, : medium and high
Yes, 8192 contains 7 freqs. […]
Ack
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/2/src/soc/mediatek/mt8192/mem... PS2, Line 23: result != 0
Actually, the return type of complex_mem_test() is int (not enum cb_err), and I only see one instanc […]
Ack
https://review.coreboot.org/c/coreboot/+/46585/6/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/6/src/soc/mediatek/mt8192/mem... PS6, Line 24: e Also add {} for the else block for consistency. See the last paragraph of https://doc.coreboot.org/coding_style.html#placing-braces-and-spaces.
Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Yidi Lin, Yidi Lin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46585
to look at the new patch set (#7).
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/46585/7
Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46585/6/src/soc/mediatek/mt8192/mem... File src/soc/mediatek/mt8192/memory.c:
https://review.coreboot.org/c/coreboot/+/46585/6/src/soc/mediatek/mt8192/mem... PS6, Line 24: e
Also add {} for the else block for consistency. See the last paragraph of https://doc.coreboot. […]
Ack
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
Patch Set 7: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46585 )
Change subject: soc/mediatek/mt8192: add dram log prefix ......................................................................
soc/mediatek/mt8192: add dram log prefix
1 Add dram log prefix: [MEM] 2 Print error code when memtest fails.
Signed-off-by: Xi Chen xixi.chen@mediatek.com Change-Id: I6c53c9cecf5996227a3e343fc703b9880d9afeac Reviewed-on: https://review.coreboot.org/c/coreboot/+/46585 Reviewed-by: Yu-Ping Wu yupingso@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/mediatek/mt8192/memory.c 1 file changed, 6 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Yu-Ping Wu: Looks good to me, approved
diff --git a/src/soc/mediatek/mt8192/memory.c b/src/soc/mediatek/mt8192/memory.c index b5363b0..5820fbf 100644 --- a/src/soc/mediatek/mt8192/memory.c +++ b/src/soc/mediatek/mt8192/memory.c @@ -15,14 +15,14 @@ const struct ddr_base_info *ddr_info = &dparam->ddr_info;
for (u8 rank = RANK_0; rank < ddr_info->support_ranks; rank++) { - int i = complex_mem_test(addr, 0x2000); + int result = complex_mem_test(addr, 0x2000);
- printk(BIOS_DEBUG, "[MEM] complex R/W mem test %s\n", - (i == 0) ? "pass" : "fail"); - - if (i != 0) { - printk(BIOS_ERR, "DRAM memory test failed\n"); + if (result != 0) { + printk(BIOS_ERR, + "[MEM] complex R/W mem test failed: %d\n", result); return -1; + } else { + printk(BIOS_DEBUG, "[MEM] complex R/W mem test passed\n"); }
addr += ddr_info->rank_size[rank];
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
Paul Menzel (Code Review)
-
Xi Chen (Code Review)
-
Xixi Chen (Code Review)
-
Yidi Lin (Code Review)
-
Yu-Ping Wu (Code Review)